last nits

Author: mbissa

internal/xds/bootstrap/bootstrap_test.go

@@ -1752,6 +1752,9 @@ func (s) TestAllowedGRPCServices_UnmarshalJSON(t *testing.T) {
 		// want carries the expected target and credential identities;
 		// comparisons use the Equal methods via cmp.Diff.
 		want AllowedGRPCServices
+		// wantCallCreds is the number of call credentials expected to be
+		// built for the target.
+		wantCallCreds int
 	}{
 		{
 			name: "insecure_channel_creds",
@@ -1772,6 +1775,7 @@ func (s) TestAllowedGRPCServices_UnmarshalJSON(t *testing.T) {
 					Data: json.RawMessage(`{"jwt_token_file": "/var/run/secrets/tokens/istio-token"}`),
 				}, nil)},
 			}},
+			wantCallCreds: 1,
 		},
 		{
 			// Unsupported call-creds types are skipped without error, so no
@@ -1802,6 +1806,7 @@ func (s) TestAllowedGRPCServices_UnmarshalJSON(t *testing.T) {
 					}, nil),
 				},
 			}},
+			wantCallCreds: 2,
 		},
 		{
 			name: "tls_channel_creds",
@@ -1831,11 +1836,20 @@ func (s) TestAllowedGRPCServices_UnmarshalJSON(t *testing.T) {
 				t.Errorf("AllowedGRPCServices unmarshal returned unexpected diff (-want +got):\n%s", diff)
 			}
 			// Equal compares credentials by identity only, so it cannot tell
-			// a built bundle from a nil one; verify the bundle was built.
-			chanCreds, _ := got[target].SideChannelCredentials()
+			// a built credential from a nil one; verify the credentials were
+			// built.
+			chanCreds, callCreds := got[target].SideChannelCredentials()
 			if chanCreds == nil || chanCreds.Bundle() == nil {
 				t.Error("SideChannelCredentials() returned no built channel credentials")
 			}
+			if len(callCreds) != test.wantCallCreds {
+				t.Errorf("SideChannelCredentials() returned %d call credentials, want %d", len(callCreds), test.wantCallCreds)
+			}
+			for i, cc := range callCreds {
+				if cc.Credentials() == nil {
+					t.Errorf("SideChannelCredentials() call credentials[%d] have no built credentials", i)
+				}
+			}
 		})
 	}
 }

internal/xds/credentials/tls.go

@@ -58,9 +58,9 @@ func buildTLSCredentials(config *anypb.Any, resolver CertProviderConfigResolver)
 	if rootInstanceName == "" {
 		return nil, nil, fmt.Errorf("credentials: tls credentials must specify root_certificate_provider with an instance_name")
 	}
-	rootCfg, err := certProviderConfig(resolver, rootInstanceName)
+	rootCfg, err := certProviderConfig(resolver, rootInstanceName, "root")
 	if err != nil {
-		return nil, nil, fmt.Errorf("credentials: tls credentials root certificate provider: %v", err)
+		return nil, nil, err
 	}
 	rootProvider, err := rootCfg.Build(certprovider.BuildOptions{
 		CertName: tlsCfg.GetRootCertificateProvider().GetCertificateName(),
@@ -79,10 +79,10 @@ func buildTLSCredentials(config *anypb.Any, resolver CertProviderConfigResolver)
 			rootProvider.Close()
 			return nil, nil, fmt.Errorf("credentials: tls credentials identity_certificate_provider must specify an instance_name")
 		}
-		identityCfg, err := certProviderConfig(resolver, identityInstanceName)
+		identityCfg, err := certProviderConfig(resolver, identityInstanceName, "identity")
 		if err != nil {
 			rootProvider.Close()
-			return nil, nil, fmt.Errorf("credentials: tls credentials identity certificate provider: %v", err)
+			return nil, nil, err
 		}
 		identityProvider, err := identityCfg.Build(certprovider.BuildOptions{
 			CertName:     identity.GetCertificateName(),
@@ -98,11 +98,12 @@ func buildTLSCredentials(config *anypb.Any, resolver CertProviderConfigResolver)
 }
 
 // certProviderConfig looks up the certificate provider instance with the
-// given name via the resolver.
-func certProviderConfig(resolver CertProviderConfigResolver, instanceName string) (*certprovider.BuildableConfig, error) {
+// given name via the resolver. kind names the certificate the provider is
+// used for (root or identity), for error messages.
+func certProviderConfig(resolver CertProviderConfigResolver, instanceName, kind string) (*certprovider.BuildableConfig, error) {
 	cfg, ok := resolver.CertProviderConfigs()[instanceName]
 	if !ok {
-		return nil, fmt.Errorf("certificate provider instance name %q missing in bootstrap configuration", instanceName)
+		return nil, fmt.Errorf("credentials: tls credentials %s certificate provider: instance name %q missing in bootstrap configuration", kind, instanceName)
 	}
 	return cfg, nil
 }

internal/xds/grpcservice/grpcservice_test.go

@@ -207,6 +207,25 @@ func (s) TestParse(t *testing.T) {
 			config: bootstrapConfig(t, allowedInsecure),
 			want:   &Config{TargetURI: target, ChannelCredentials: xdscreds.NewChannelCreds(nil, xdscreds.Identity{Type: "insecure"}, nil)},
 		},
+		{
+			// The proto's call-creds plugins are ignored along with its
+			// channel-creds plugins: the allowlist's call creds are used.
+			name:   "untrusted_allowlisted_uses_allowlist_call_creds",
+			gs:     googleGrpcService(target, []*anypb.Any{insecurePlugin}, []*anypb.Any{tokenPlugin}, nil),
+			sc:     untrustedServerConfig(t),
+			config: bootstrapConfig(t, `{"dns:///my-service:443":{"channel_creds":[{"type":"insecure"}],"call_creds":[{"type":"jwt_token_file","config":{"jwt_token_file":"/tokens/token"}}]}}`),
+			want: &Config{
+				TargetURI:          target,
+				ChannelCredentials: xdscreds.NewChannelCreds(nil, xdscreds.Identity{Type: "insecure"}, nil),
+				CallCredentials: []*xdscreds.CallCreds{xdscreds.NewCallCreds(nil, xdscreds.Identity{
+					Type: "jwt_token_file",
+					// Bootstrap parsing normalizes the configuration JSON
+					// via json.Indent, and the identity carries the
+					// normalized bytes.
+					Data: json.RawMessage("{\n\"jwt_token_file\": \"/tokens/token\"\n}"),
+				}, nil)},
+			},
+		},
 		{
 			name:    "untrusted_not_allowlisted",
 			gs:      googleGrpcService(target, nil, nil, nil),
@@ -279,6 +298,9 @@ func (s) TestParse(t *testing.T) {
 			if err != nil {
 				t.Fatalf("Parse() returned unexpected error: %v", err)
 			}
+			// The cmp.Diff below compares credentials by identity only, so it
+			// cannot tell a built credential from a nil one; verify the
+			// credentials were built.
 			if got.ChannelCredentials.Bundle() == nil {
 				t.Error("Parse() returned channel credentials without a built bundle")
 			}

internal/xds/httpfilter/ext_authz/config.go

@@ -29,7 +29,7 @@ import (
 type config struct {
 	httpfilter.FilterConfig
 	// grpcService is the configuration for the external authorization server.
-	grpcService grpcservice.Config
+	grpcService *grpcservice.Config
 	// filterEnabled specifies the percentage of requests to be authorized by
 	// the external authorization server.
 	filterEnabled fraction

internal/xds/httpfilter/ext_authz/ext_authz.go

@@ -46,8 +46,8 @@ func init() {
 var (
 	// TODO: Parse via grpcservice.Parse with the filter parse options,
 	// as ext_proc does, when ext_authz is wired up for gRFC A102.
-	parseGRPCServiceConfig = func(*v3corepb.GrpcService) (grpcservice.Config, error) {
-		return grpcservice.Config{}, fmt.Errorf("parseGRPCServiceConfig not implemented")
+	parseGRPCServiceConfig = func(*v3corepb.GrpcService) (*grpcservice.Config, error) {
+		return nil, fmt.Errorf("parseGRPCServiceConfig not implemented")
 	}
 )
 

internal/xds/httpfilter/ext_authz/ext_authz_test.go

@@ -53,21 +53,16 @@ func Test(t *testing.T) {
 // testParseGRPCServiceConfig is a helper function that parses a GrpcService
 // proto message into a GRPCServiceConfig. This is a temporary test
 // implementation that will be removed once gRFC A102 is implemented.
-func testParseGRPCServiceConfig(grpcService *corepb.GrpcService) (grpcservice.Config, error) {
-	if grpcService == nil {
-		return grpcservice.Config{}, nil
-	}
+func testParseGRPCServiceConfig(grpcService *corepb.GrpcService) (*grpcservice.Config, error) {
 	if grpcService.GetGoogleGrpc() == nil {
-		return grpcservice.Config{}, fmt.Errorf("only google_grpc grpc_service is supported")
+		return nil, fmt.Errorf("only google_grpc grpc_service is supported")
 	}
 	if grpcService.GetGoogleGrpc().GetTargetUri() == "" {
-		return grpcservice.Config{}, fmt.Errorf("targetURI must be a non-empty string")
+		return nil, fmt.Errorf("targetURI must be a non-empty string")
 	}
-
-	sc := grpcservice.Config{
+	return &grpcservice.Config{
 		TargetURI: grpcService.GetGoogleGrpc().GetTargetUri(),
-	}
-	return sc, nil
+	}, nil
 }
 
 var cmpOpts = []cmp.Option{
@@ -114,7 +109,7 @@ func (s) TestParseFilterConfig_Success(t *testing.T) {
 				},
 			}),
 			wantCfg: config{
-				grpcService: grpcservice.Config{
+				grpcService: &grpcservice.Config{
 					TargetURI: "localhost:1234",
 				},
 				filterEnabled: fraction{
@@ -170,7 +165,7 @@ func (s) TestParseFilterConfig_Success(t *testing.T) {
 				IncludePeerCertificate: true,
 			}),
 			wantCfg: config{
-				grpcService: grpcservice.Config{
+				grpcService: &grpcservice.Config{
 					TargetURI: "localhost:5678",
 				},
 				filterEnabled: fraction{

internal/xds/httpfilter/extproc/ext_proc.go

@@ -51,7 +51,6 @@ import (
 	"google.golang.org/protobuf/types/known/anypb"
 	"google.golang.org/protobuf/types/known/structpb"
 
-	v3corepb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
 	v3procfilterpb "github.com/envoyproxy/go-control-plane/envoy/extensions/filters/http/ext_proc/v3"
 	v3procservicegrpc "github.com/envoyproxy/go-control-plane/envoy/service/ext_proc/v3"
 	v3procservicepb "github.com/envoyproxy/go-control-plane/envoy/service/ext_proc/v3"
@@ -110,15 +109,6 @@ func validateBodyProcessingMode(mode *v3procfilterpb.ProcessingMode) error {
 	return nil
 }
 
-// parseGRPCService parses the GrpcService proto identifying the external
-// processor server. The gRFC A102 trust policy — honoring the proto's
-// credentials only when the delivering xDS management server is trusted, and
-// requiring an untrusted server's target to be present in the bootstrap
-// allowed_grpc_services map — is applied by grpcservice.Parse.
-func parseGRPCService(gs *v3corepb.GrpcService, opts httpfilter.ParseOptions) (*grpcservice.Config, error) {
-	return grpcservice.Parse(gs, opts.BootstrapConfig, opts.ServerConfig)
-}
-
 // ParseFilterConfig parses the provided filter configuration. The GrpcService
 // identifying the external processor server is validated against the provided
 // parse options, as per gRFC A102.
@@ -168,7 +158,7 @@ func (builder) ParseFilterConfig(cfg proto.Message, opts httpfilter.ParseOptions
 	if msg.GetGrpcService() == nil {
 		return nil, fmt.Errorf("extproc: empty grpc_service provided in config %v", cfg)
 	}
-	server, err := parseGRPCService(msg.GetGrpcService(), opts)
+	server, err := grpcservice.Parse(msg.GetGrpcService(), opts.BootstrapConfig, opts.ServerConfig)
 	if err != nil {
 		return nil, fmt.Errorf("extproc: failed to parse grpc_service: %v", err)
 	}
@@ -218,8 +208,8 @@ func (builder) ParseFilterConfigOverride(ov proto.Message, opts httpfilter.Parse
 	// Parse the GrpcService last, so that no error path can drop the built
 	// credentials: the caller owns them from here on.
 	var serverOpt optional.Optional[*grpcservice.Config]
-	if override.GetGrpcService() != nil {
-		server, err := parseGRPCService(override.GetGrpcService(), opts)
+	if gs := override.GetGrpcService(); gs != nil {
+		server, err := grpcservice.Parse(gs, opts.BootstrapConfig, opts.ServerConfig)
 		if err != nil {
 			return nil, fmt.Errorf("extproc: failed to parse grpc_service: %v", err)
 		}