From 7a81255b0fe20508ea1c3fe1b37fa8759d1165a1 Mon Sep 17 00:00:00 2001 From: Madhav Bissa Date: Wed, 24 Jun 2026 00:38:57 +0530 Subject: [PATCH 1/3] xds: parse allowed_grpc_services from the bootstrap config Adds parsing and credential resolution for the allowed_grpc_services bootstrap field (gRFC A102), used to validate side-channel targets received from untrusted xDS servers. The channel-credential selection stops at the first supported type; call credentials are built only when the call-creds env var is enabled, and are otherwise retained as config without being built. RELEASE NOTES: n/a --- internal/xds/bootstrap/bootstrap.go | 153 ++++++++++++++ internal/xds/bootstrap/bootstrap_test.go | 243 +++++++++++++++++++++++ 2 files changed, 396 insertions(+) diff --git a/internal/xds/bootstrap/bootstrap.go b/internal/xds/bootstrap/bootstrap.go index 79fb79020cf8..2647509de877 100644 --- a/internal/xds/bootstrap/bootstrap.go +++ b/internal/xds/bootstrap/bootstrap.go @@ -120,6 +120,137 @@ func (ccs CallCredsConfigs) String() string { return strings.Join(creds, ",") } +// AllowedGrpcService contains credentials config for an allowed gRPC service. +type AllowedGrpcService struct { + channelCreds []ChannelCreds + callCredsConfigs []CallCredsConfig + selectedChannelCreds ChannelCreds + selectedCallCreds []credentials.PerRPCCredentials + dialOptions []grpc.DialOption + cleanups []func() +} + +// ChannelCreds returns the list of parsed channel credentials. +func (a *AllowedGrpcService) ChannelCreds() []ChannelCreds { + return a.channelCreds +} + +// CallCredsConfigs returns the list of call credentials configurations. +func (a *AllowedGrpcService) CallCredsConfigs() []CallCredsConfig { + return a.callCredsConfigs +} + +// SelectedChannelCreds returns the chosen supported channel credentials. +func (a *AllowedGrpcService) SelectedChannelCreds() ChannelCreds { + return a.selectedChannelCreds +} + +// DialOptions returns dial options built from these credentials. +func (a *AllowedGrpcService) DialOptions() []grpc.DialOption { + return a.dialOptions +} + +// Cleanups returns cleanups to run when the service is no longer needed. +func (a *AllowedGrpcService) Cleanups() []func() { + return a.cleanups +} + +// Equal reports whether a and other are considered equal. +func (a *AllowedGrpcService) Equal(other *AllowedGrpcService) bool { + switch { + case a == nil && other == nil: + return true + case (a != nil) != (other != nil): + return false + case !slices.EqualFunc(a.channelCreds, other.channelCreds, func(x, y ChannelCreds) bool { return x.Equal(y) }): + return false + case !slices.EqualFunc(a.callCredsConfigs, other.callCredsConfigs, func(x, y CallCredsConfig) bool { return x.Equal(y) }): + return false + } + return true +} + +type allowedGrpcServiceJSON struct { + ChannelCreds []ChannelCreds `json:"channel_creds,omitempty"` + CallCredsConfigs []CallCredsConfig `json:"call_creds,omitempty"` +} + +// UnmarshalJSON parses the JSON data and validates its credentials. +func (a *AllowedGrpcService) UnmarshalJSON(data []byte) error { + var jsonS allowedGrpcServiceJSON + if err := json.Unmarshal(data, &jsonS); err != nil { + return err + } + a.channelCreds = jsonS.ChannelCreds + a.callCredsConfigs = jsonS.CallCredsConfigs + + cleanups := []func(){} + dialOptions := []grpc.DialOption{} + selectedCallCreds := []credentials.PerRPCCredentials{} + + runCleanups := func() { + for _, f := range cleanups { + f() + } + } + + var credsDialOption grpc.DialOption + for _, cc := range jsonS.ChannelCreds { + c := bootstrap.GetChannelCredentials(cc.Type) + if c == nil { + continue + } + bundle, cancel, err := c.Build(cc.Config) + if err != nil { + runCleanups() + return fmt.Errorf("failed to build credentials bundle from bootstrap for allowed grpc service: type %q, err: %v", cc.Type, err) + } + a.selectedChannelCreds = cc + credsDialOption = grpc.WithCredentialsBundle(bundle) + if d, ok := bundle.(extraDialOptions); ok { + dialOptions = append(dialOptions, d.DialOptions()...) + } + cleanups = append(cleanups, cancel) + break + } + + if credsDialOption == nil { + runCleanups() + return fmt.Errorf("xds: no supported channel credentials found for allowed grpc service in config:\n%s", string(data)) + } + dialOptions = append(dialOptions, credsDialOption) + + if envconfig.XDSBootstrapCallCredsEnabled { + for _, cfg := range jsonS.CallCredsConfigs { + c := bootstrap.GetCallCredentials(cfg.Type) + if c == nil { + continue + } + callCreds, cancel, err := c.Build(cfg.Config) + if err != nil { + runCleanups() + return fmt.Errorf("failed to build call credentials from bootstrap for allowed grpc service: type %q, err: %v", cfg.Type, err) + } + selectedCallCreds = append(selectedCallCreds, callCreds) + dialOptions = append(dialOptions, grpc.WithPerRPCCredentials(callCreds)) + cleanups = append(cleanups, cancel) + } + } + + a.dialOptions = dialOptions + a.selectedCallCreds = selectedCallCreds + a.cleanups = cleanups + return nil +} + +// MarshalJSON marshals the allowed gRPC service into JSON format. +func (a *AllowedGrpcService) MarshalJSON() ([]byte, error) { + return json.Marshal(allowedGrpcServiceJSON{ + ChannelCreds: a.channelCreds, + CallCredsConfigs: a.callCredsConfigs, + }) +} + // ServerConfigs represents a collection of server configurations. type ServerConfigs []*ServerConfig @@ -461,6 +592,14 @@ type Config struct { // A map from certprovider instance names to parsed buildable configs. certProviderConfigs map[string]*certprovider.BuildableConfig + + allowedGrpcServices map[string]*AllowedGrpcService +} + +// AllowedGrpcServices returns the allowlist of gRPC services. +// Callers must not modify the returned map. +func (c *Config) AllowedGrpcServices() map[string]*AllowedGrpcService { + return c.allowedGrpcServices } // XDSServers returns the top-level list of management servers to connect to, @@ -552,6 +691,8 @@ func (c *Config) Equal(other *Config) bool { return false case !maps.EqualFunc(c.authorities, other.authorities, func(a, b *Authority) bool { return a.Equal(b) }): return false + case !maps.EqualFunc(c.allowedGrpcServices, other.allowedGrpcServices, func(a, b *AllowedGrpcService) bool { return a.Equal(b) }): + return false case !c.node.Equal(other.node): return false } @@ -572,6 +713,7 @@ type configJSON struct { ClientDefaultListenerResourceNameTemplate string `json:"client_default_listener_resource_name_template,omitempty"` Authorities map[string]*Authority `json:"authorities,omitempty"` Node node `json:"node,omitempty"` + AllowedGrpcServices map[string]*AllowedGrpcService `json:"allowed_grpc_services,omitempty"` } // MarshalJSON returns marshaled JSON bytes corresponding to this config. @@ -583,6 +725,7 @@ func (c *Config) MarshalJSON() ([]byte, error) { ClientDefaultListenerResourceNameTemplate: c.clientDefaultListenerResourceNameTemplate, Authorities: c.authorities, Node: c.node, + AllowedGrpcServices: c.allowedGrpcServices, } return json.MarshalIndent(config, " ", " ") } @@ -604,6 +747,7 @@ func (c *Config) UnmarshalJSON(data []byte) error { c.clientDefaultListenerResourceNameTemplate = config.ClientDefaultListenerResourceNameTemplate c.authorities = config.Authorities c.node = config.Node + c.allowedGrpcServices = config.AllowedGrpcServices // Build the certificate providers configuration to ensure that it is valid. cpcCfgs := make(map[string]*certprovider.BuildableConfig) @@ -723,6 +867,8 @@ type ConfigOptionsForTesting struct { // Node identifies the gRPC client/server node in the // proxyless service mesh. Node json.RawMessage + // AllowedGrpcServices is the allowlist of gRPC services. + AllowedGrpcServices json.RawMessage } // NewContentsForTesting creates a new bootstrap configuration from the passed in @@ -754,6 +900,12 @@ func NewContentsForTesting(opts ConfigOptionsForTesting) ([]byte, error) { if err := json.Unmarshal(opts.Node, &node); err != nil { return nil, fmt.Errorf("failed to unmarshal node configuration %s: %v", string(opts.Node), err) } + allowedGrpcServices := make(map[string]*AllowedGrpcService) + if len(opts.AllowedGrpcServices) > 0 { + if err := json.Unmarshal(opts.AllowedGrpcServices, &allowedGrpcServices); err != nil { + return nil, fmt.Errorf("failed to unmarshal allowed grpc services configuration: %v", err) + } + } cfgJSON := configJSON{ XDSServers: servers, CertificateProviders: certProviders, @@ -761,6 +913,7 @@ func NewContentsForTesting(opts ConfigOptionsForTesting) ([]byte, error) { ClientDefaultListenerResourceNameTemplate: opts.ClientDefaultListenerResourceNameTemplate, Authorities: authorities, Node: node, + AllowedGrpcServices: allowedGrpcServices, } contents, err := json.MarshalIndent(cfgJSON, " ", " ") if err != nil { diff --git a/internal/xds/bootstrap/bootstrap_test.go b/internal/xds/bootstrap/bootstrap_test.go index 0dd87416a30c..9de43fdc95ee 100644 --- a/internal/xds/bootstrap/bootstrap_test.go +++ b/internal/xds/bootstrap/bootstrap_test.go @@ -1032,6 +1032,13 @@ func (s) TestGetConfiguration_Federation(t *testing.T) { "server_features" : ["xds_v3"] }] } + }, + "allowed_grpc_services": { + "dns:///whitelisted-ext-proc:443": { + "channel_creds": [ + { "type": "insecure" } + ] + } } }`, // If client_default_listener_resource_name_template is not set, it @@ -1129,6 +1136,15 @@ func (s) TestGetConfiguration_Federation(t *testing.T) { }}, }, }, + allowedGrpcServices: func() map[string]*AllowedGrpcService { + var svc AllowedGrpcService + if err := json.Unmarshal([]byte(`{"channel_creds": [{"type": "insecure"}]}`), &svc); err != nil { + panic(err) + } + return map[string]*AllowedGrpcService{ + "dns:///whitelisted-ext-proc:443": &svc, + } + }(), }, }, { @@ -1710,3 +1726,230 @@ func (s) TestBootstrap_SelectedCallCreds_WhenNotCCNotEnabled(t *testing.T) { t.Errorf("Call creds count = %d, want %d", len(dialOpts), 0) } } + +func (s) TestBootstrap_AllowedGrpcServices(t *testing.T) { + testutils.SetEnvConfig(t, &envconfig.XDSBootstrapCallCredsEnabled, true) + bootstrapFileMap := map[string]string{ + "allowedGrpcServicesGood": ` + { + "node": { + "id": "ENVOY_NODE_ID" + }, + "xds_servers" : [{ + "server_uri": "trafficdirector.googleapis.com:443", + "channel_creds": [ + { "type": "insecure" } + ] + }], + "allowed_grpc_services": { + "dns:///sharding-service:443": { + "channel_creds": [ + { "type": "insecure" } + ] + } + } + }`, + "allowedGrpcServicesWithCallCreds": ` + { + "node": { + "id": "ENVOY_NODE_ID" + }, + "xds_servers" : [{ + "server_uri": "trafficdirector.googleapis.com:443", + "channel_creds": [ + { "type": "insecure" } + ] + }], + "allowed_grpc_services": { + "dns:///sharding-service:443": { + "channel_creds": [ + { "type": "insecure" } + ], + "call_creds": [ + { "type": "jwt_token_file", "config": {"jwt_token_file": "/var/run/secrets/tokens/istio-token"} } + ] + } + } + }`, + "allowedGrpcServicesBadCreds": ` + { + "node": { + "id": "ENVOY_NODE_ID" + }, + "xds_servers" : [{ + "server_uri": "trafficdirector.googleapis.com:443", + "channel_creds": [ + { "type": "insecure" } + ] + }], + "allowed_grpc_services": { + "dns:///sharding-service:443": { + "channel_creds": [ + { "type": "unsupported_cred_type" } + ] + } + } + }`, + } + cancel := setupBootstrapOverride(bootstrapFileMap) + defer cancel() + + tests := []struct { + name string + fileName string + wantErr bool + wantChannelCredType string + wantCallCredTypes []string + }{ + { + name: "good allowed_grpc_services", + fileName: "allowedGrpcServicesGood", + wantChannelCredType: "insecure", + }, + { + name: "allowed_grpc_services with call creds", + fileName: "allowedGrpcServicesWithCallCreds", + wantChannelCredType: "insecure", + wantCallCredTypes: []string{"jwt_token_file"}, + }, + { + name: "bad allowed_grpc_services", + fileName: "allowedGrpcServicesBadCreds", + wantErr: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + origBootstrapFileName := envconfig.XDSBootstrapFileName + envconfig.XDSBootstrapFileName = test.fileName + defer func() { envconfig.XDSBootstrapFileName = origBootstrapFileName }() + + cfg, err := GetConfiguration() + if err != nil && !test.wantErr { + t.Fatalf("GetConfiguration() got unexpected error: %v, want: success", err) + } + if err == nil && test.wantErr { + t.Fatalf("GetConfiguration() got: success, want: error") + } + if test.wantErr { + return + } + allowed := cfg.AllowedGrpcServices() + if len(allowed) != 1 { + t.Fatalf("AllowedGrpcServices count got: %d, want: 1", len(allowed)) + } + svc, ok := allowed["dns:///sharding-service:443"] + if !ok { + t.Fatalf("AllowedGrpcServices missing key \"dns:///sharding-service:443\"") + } + if got := svc.SelectedChannelCreds().Type; got != test.wantChannelCredType { + t.Errorf("SelectedChannelCreds type got: %q, want: %q", got, test.wantChannelCredType) + } + if len(svc.CallCredsConfigs()) != len(test.wantCallCredTypes) { + t.Fatalf("CallCredsConfigs count got: %d, want: %d", len(svc.CallCredsConfigs()), len(test.wantCallCredTypes)) + } + for i, wantType := range test.wantCallCredTypes { + if got := svc.CallCredsConfigs()[i].Type; got != wantType { + t.Errorf("CallCredsConfig[%d] type got: %q, want: %q", i, got, wantType) + } + } + }) + } +} + +// TestBootstrap_AllowedGrpcServices_CredentialIteration verifies that the +// channel-credential selection skips unsupported types and stops at the first +// supported one, and that call credentials are only built when the call-creds +// env var is enabled. +func (s) TestBootstrap_AllowedGrpcServices_CredentialIteration(t *testing.T) { + const target = "dns:///sharding-service:443" + bootstrapFileMap := map[string]string{ + // First channel-creds entry is an unsupported type; the supported + // "insecure" entry follows and must be selected. + "channelCredsIteration": ` + { + "node": { "id": "ENVOY_NODE_ID" }, + "xds_servers" : [{ + "server_uri": "trafficdirector.googleapis.com:443", + "channel_creds": [{ "type": "insecure" }] + }], + "allowed_grpc_services": { + "dns:///sharding-service:443": { + "channel_creds": [ + { "type": "unsupported_cred_type" }, + { "type": "insecure" } + ] + } + } + }`, + "withCallCreds": ` + { + "node": { "id": "ENVOY_NODE_ID" }, + "xds_servers" : [{ + "server_uri": "trafficdirector.googleapis.com:443", + "channel_creds": [{ "type": "insecure" }] + }], + "allowed_grpc_services": { + "dns:///sharding-service:443": { + "channel_creds": [{ "type": "insecure" }], + "call_creds": [ + { "type": "jwt_token_file", "config": {"jwt_token_file": "/var/run/secrets/tokens/istio-token"} } + ] + } + } + }`, + } + cancel := setupBootstrapOverride(bootstrapFileMap) + defer cancel() + + getService := func(t *testing.T) *AllowedGrpcService { + t.Helper() + cfg, err := GetConfiguration() + if err != nil { + t.Fatalf("GetConfiguration() failed: %v", err) + } + svc, ok := cfg.AllowedGrpcServices()[target] + if !ok { + t.Fatalf("AllowedGrpcServices() missing key %q", target) + } + return svc + } + + t.Run("channel_creds_skips_unsupported", func(t *testing.T) { + testutils.SetEnvConfig(t, &envconfig.XDSBootstrapCallCredsEnabled, true) + testutils.SetEnvConfig(t, &envconfig.XDSBootstrapFileName, "channelCredsIteration") + svc := getService(t) + if got := svc.SelectedChannelCreds().Type; got != "insecure" { + t.Errorf("SelectedChannelCreds().Type got: %q, want: %q", got, "insecure") + } + // Only the channel-creds dial option is expected (no call creds). + if got := len(svc.DialOptions()); got != 1 { + t.Errorf("len(DialOptions()) got: %d, want: 1", got) + } + }) + + t.Run("call_creds_applied_when_enabled", func(t *testing.T) { + testutils.SetEnvConfig(t, &envconfig.XDSBootstrapCallCredsEnabled, true) + testutils.SetEnvConfig(t, &envconfig.XDSBootstrapFileName, "withCallCreds") + svc := getService(t) + // One channel-creds dial option plus one per-RPC call-creds option. + if got := len(svc.DialOptions()); got != 2 { + t.Errorf("len(DialOptions()) got: %d, want: 2", got) + } + }) + + t.Run("call_creds_ignored_when_disabled", func(t *testing.T) { + testutils.SetEnvConfig(t, &envconfig.XDSBootstrapCallCredsEnabled, false) + testutils.SetEnvConfig(t, &envconfig.XDSBootstrapFileName, "withCallCreds") + svc := getService(t) + // Call creds must not be built, so only the channel-creds option remains. + if got := len(svc.DialOptions()); got != 1 { + t.Errorf("len(DialOptions()) got: %d, want: 1", got) + } + // The parsed call-creds config is still retained for round-tripping. + if got := len(svc.CallCredsConfigs()); got != 1 { + t.Errorf("len(CallCredsConfigs()) got: %d, want: 1", got) + } + }) +} From b5abb0fa69086b59869f11ec1a074ee6a3721dd4 Mon Sep 17 00:00:00 2001 From: Madhav Bissa Date: Wed, 24 Jun 2026 01:40:35 +0530 Subject: [PATCH 2/3] xds: release built credentials when bootstrap parsing fails --- internal/xds/bootstrap/bootstrap.go | 38 +++++++++++++-- internal/xds/bootstrap/bootstrap_test.go | 59 ++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 3 deletions(-) diff --git a/internal/xds/bootstrap/bootstrap.go b/internal/xds/bootstrap/bootstrap.go index 2647509de877..325d75527d8b 100644 --- a/internal/xds/bootstrap/bootstrap.go +++ b/internal/xds/bootstrap/bootstrap.go @@ -181,9 +181,9 @@ func (a *AllowedGrpcService) UnmarshalJSON(data []byte) error { if err := json.Unmarshal(data, &jsonS); err != nil { return err } - a.channelCreds = jsonS.ChannelCreds - a.callCredsConfigs = jsonS.CallCredsConfigs + // Build into locals and assign the receiver only on success, so a + // mid-parse error never leaves the receiver partially mutated. cleanups := []func(){} dialOptions := []grpc.DialOption{} selectedCallCreds := []credentials.PerRPCCredentials{} @@ -195,6 +195,7 @@ func (a *AllowedGrpcService) UnmarshalJSON(data []byte) error { } var credsDialOption grpc.DialOption + var selectedChannelCreds ChannelCreds for _, cc := range jsonS.ChannelCreds { c := bootstrap.GetChannelCredentials(cc.Type) if c == nil { @@ -205,7 +206,7 @@ func (a *AllowedGrpcService) UnmarshalJSON(data []byte) error { runCleanups() return fmt.Errorf("failed to build credentials bundle from bootstrap for allowed grpc service: type %q, err: %v", cc.Type, err) } - a.selectedChannelCreds = cc + selectedChannelCreds = cc credsDialOption = grpc.WithCredentialsBundle(bundle) if d, ok := bundle.(extraDialOptions); ok { dialOptions = append(dialOptions, d.DialOptions()...) @@ -237,6 +238,9 @@ func (a *AllowedGrpcService) UnmarshalJSON(data []byte) error { } } + a.channelCreds = jsonS.ChannelCreds + a.callCredsConfigs = jsonS.CallCredsConfigs + a.selectedChannelCreds = selectedChannelCreds a.dialOptions = dialOptions a.selectedCallCreds = selectedCallCreds a.cleanups = cleanups @@ -741,6 +745,33 @@ func (c *Config) UnmarshalJSON(data []byte) error { return fmt.Errorf("xds: json.Unmarshal(%s) failed during bootstrap: %v", string(data), err) } + // Unmarshaling above eagerly built credentials (bundles, file + // watchers) for each server config and allowed gRPC service. If + // bootstrap parsing fails after this point, release them: the caller + // discards this Config without ever calling their cleanups. + parsedSuccessfully := false + defer func() { + if parsedSuccessfully { + return + } + for _, sc := range config.XDSServers { + if sc == nil { + continue + } + for _, cleanup := range sc.Cleanups() { + cleanup() + } + } + for _, svc := range config.AllowedGrpcServices { + if svc == nil { + continue + } + for _, cleanup := range svc.Cleanups() { + cleanup() + } + } + }() + c.xDSServers = config.XDSServers c.cpcs = config.CertificateProviders c.serverListenerResourceNameTemplate = config.ServerListenerResourceNameTemplate @@ -788,6 +819,7 @@ func (c *Config) UnmarshalJSON(data []byte) error { return fmt.Errorf("xds: field clientListenerResourceNameTemplate %q of authority %q doesn't start with prefix %q", authority.ClientListenerResourceNameTemplate, name, prefix) } } + parsedSuccessfully = true return nil } diff --git a/internal/xds/bootstrap/bootstrap_test.go b/internal/xds/bootstrap/bootstrap_test.go index 9de43fdc95ee..f48c3e88a5e5 100644 --- a/internal/xds/bootstrap/bootstrap_test.go +++ b/internal/xds/bootstrap/bootstrap_test.go @@ -29,6 +29,7 @@ import ( "github.com/google/go-cmp/cmp" "google.golang.org/grpc" "google.golang.org/grpc/credentials" + "google.golang.org/grpc/credentials/insecure" "google.golang.org/grpc/credentials/jwt" "google.golang.org/grpc/credentials/tls/certprovider" "google.golang.org/grpc/internal" @@ -1953,3 +1954,61 @@ func (s) TestBootstrap_AllowedGrpcServices_CredentialIteration(t *testing.T) { } }) } + +// observableChannelCreds is a fake bootstrap channel-credentials builder whose +// teardown is observable, used to verify cleanup-on-error behavior. +type observableChannelCreds struct{ onCancel func() } + +func (o observableChannelCreds) Build(json.RawMessage) (credentials.Bundle, func(), error) { + return insecure.NewBundle(), o.onCancel, nil +} + +func (observableChannelCreds) Name() string { return "test_observable_channel_creds" } + +// TestBootstrap_CleanupOnUnmarshalError verifies that credentials built while +// unmarshaling allowed gRPC services (and server configs) are torn down if +// bootstrap parsing fails afterwards, rather than leaking. +func (s) TestBootstrap_CleanupOnUnmarshalError(t *testing.T) { + cancels := 0 + bootstrap.RegisterChannelCredentials(observableChannelCreds{onCancel: func() { cancels++ }}) + + // allowed_grpc_services builds the observable creds, then parsing fails + // because xds_servers is empty. The built creds must be cleaned up. + const failCfg = `{ + "xds_servers": [], + "allowed_grpc_services": { + "dns:///side-channel:443": { + "channel_creds": [{"type": "test_observable_channel_creds"}] + } + } + }` + var failed Config + if err := failed.UnmarshalJSON([]byte(failCfg)); err == nil { + t.Fatal("Config.UnmarshalJSON() succeeded, want error for empty xds_servers") + } + if cancels != 1 { + t.Errorf("cleanup ran %d times after a failed bootstrap parse, want 1", cancels) + } + + // On a successful parse, cleanups are retained for the Config's lifetime + // and must NOT run during parsing. + cancels = 0 + const okCfg = `{ + "xds_servers": [{ + "server_uri": "trafficdirector.googleapis.com:443", + "channel_creds": [{"type": "insecure"}] + }], + "allowed_grpc_services": { + "dns:///side-channel:443": { + "channel_creds": [{"type": "test_observable_channel_creds"}] + } + } + }` + var ok Config + if err := ok.UnmarshalJSON([]byte(okCfg)); err != nil { + t.Fatalf("Config.UnmarshalJSON() failed: %v", err) + } + if cancels != 0 { + t.Errorf("cleanup ran %d times on a successful parse, want 0", cancels) + } +} From 04e365cfa4f4cdc7dc3278d5e75a7d6dcafcb6ea Mon Sep 17 00:00:00 2001 From: Madhav Bissa Date: Wed, 24 Jun 2026 02:23:20 +0530 Subject: [PATCH 3/3] fixing review comments --- internal/xds/bootstrap/bootstrap.go | 48 ++++-- internal/xds/bootstrap/bootstrap_test.go | 179 ++++++++++++++++++----- 2 files changed, 176 insertions(+), 51 deletions(-) diff --git a/internal/xds/bootstrap/bootstrap.go b/internal/xds/bootstrap/bootstrap.go index 325d75527d8b..de1c7840f212 100644 --- a/internal/xds/bootstrap/bootstrap.go +++ b/internal/xds/bootstrap/bootstrap.go @@ -276,6 +276,16 @@ func (scs *ServerConfigs) Equal(other *ServerConfigs) bool { func (scs *ServerConfigs) UnmarshalJSON(data []byte) error { servers := []*ServerConfig{} if err := json.Unmarshal(data, &servers); err != nil { + // Some servers may have built credentials before the failure; + // release them so they don't leak when the slice is dropped. + for _, sc := range servers { + if sc == nil { + continue + } + for _, cleanup := range sc.Cleanups() { + cleanup() + } + } return fmt.Errorf("xds: failed to JSON unmarshal server configurations during bootstrap: %v, config:\n%s", err, string(data)) } *scs = servers @@ -741,20 +751,15 @@ func (c *Config) UnmarshalJSON(data []byte) error { // even if the bootstrap configuration did not contain the node field, we // will have a node field with client controlled fields alone. config := configJSON{Node: newNode()} - if err := json.Unmarshal(data, &config); err != nil { - return fmt.Errorf("xds: json.Unmarshal(%s) failed during bootstrap: %v", string(data), err) - } - // Unmarshaling above eagerly built credentials (bundles, file - // watchers) for each server config and allowed gRPC service. If - // bootstrap parsing fails after this point, release them: the caller - // discards this Config without ever calling their cleanups. + // json.Unmarshal below eagerly builds credentials (bundles, file + // watchers) for server configs (top-level and per-authority) and + // allowed gRPC services. Install the cleanup before unmarshaling so + // resources built before a mid-way unmarshal failure, or a later + // validation error, are released when this Config is discarded. parsedSuccessfully := false - defer func() { - if parsedSuccessfully { - return - } - for _, sc := range config.XDSServers { + releaseServerCreds := func(servers ServerConfigs) { + for _, sc := range servers { if sc == nil { continue } @@ -762,6 +767,18 @@ func (c *Config) UnmarshalJSON(data []byte) error { cleanup() } } + } + defer func() { + if parsedSuccessfully { + return + } + releaseServerCreds(config.XDSServers) + for _, authority := range config.Authorities { + if authority == nil { + continue + } + releaseServerCreds(authority.XDSServers) + } for _, svc := range config.AllowedGrpcServices { if svc == nil { continue @@ -772,6 +789,10 @@ func (c *Config) UnmarshalJSON(data []byte) error { } }() + if err := json.Unmarshal(data, &config); err != nil { + return fmt.Errorf("xds: json.Unmarshal(%s) failed during bootstrap: %v", string(data), err) + } + c.xDSServers = config.XDSServers c.cpcs = config.CertificateProviders c.serverListenerResourceNameTemplate = config.ServerListenerResourceNameTemplate @@ -810,6 +831,9 @@ func (c *Config) UnmarshalJSON(data []byte) error { // - if set, it must start with "xdstp:///" // - if not set, it defaults to "xdstp:///envoy.config.listener.v3.Listener/%s" for name, authority := range c.authorities { + if authority == nil { + return fmt.Errorf("xds: authority %q has no configuration in bootstrap", name) + } prefix := fmt.Sprintf("xdstp://%s", url.PathEscape(name)) if authority.ClientListenerResourceNameTemplate == "" { authority.ClientListenerResourceNameTemplate = prefix + "/envoy.config.listener.v3.Listener/%s" diff --git a/internal/xds/bootstrap/bootstrap_test.go b/internal/xds/bootstrap/bootstrap_test.go index f48c3e88a5e5..9a927e4fe624 100644 --- a/internal/xds/bootstrap/bootstrap_test.go +++ b/internal/xds/bootstrap/bootstrap_test.go @@ -1944,7 +1944,8 @@ func (s) TestBootstrap_AllowedGrpcServices_CredentialIteration(t *testing.T) { testutils.SetEnvConfig(t, &envconfig.XDSBootstrapCallCredsEnabled, false) testutils.SetEnvConfig(t, &envconfig.XDSBootstrapFileName, "withCallCreds") svc := getService(t) - // Call creds must not be built, so only the channel-creds option remains. + // Call creds must not be built, so only the channel-creds + // option remains. if got := len(svc.DialOptions()); got != 1 { t.Errorf("len(DialOptions()) got: %d, want: 1", got) } @@ -1966,49 +1967,149 @@ func (o observableChannelCreds) Build(json.RawMessage) (credentials.Bundle, func func (observableChannelCreds) Name() string { return "test_observable_channel_creds" } // TestBootstrap_CleanupOnUnmarshalError verifies that credentials built while -// unmarshaling allowed gRPC services (and server configs) are torn down if -// bootstrap parsing fails afterwards, rather than leaking. +// unmarshaling server configs (top-level and per-authority) and allowed gRPC +// services are released if bootstrap parsing fails, rather than leaking. It +// covers failures during later validation, in nested authorities, and mid-way +// through json.Unmarshal itself. func (s) TestBootstrap_CleanupOnUnmarshalError(t *testing.T) { cancels := 0 bootstrap.RegisterChannelCredentials(observableChannelCreds{onCancel: func() { cancels++ }}) - // allowed_grpc_services builds the observable creds, then parsing fails - // because xds_servers is empty. The built creds must be cleaned up. - const failCfg = `{ - "xds_servers": [], - "allowed_grpc_services": { - "dns:///side-channel:443": { - "channel_creds": [{"type": "test_observable_channel_creds"}] + tests := []struct { + name string + cfg string + wantErr bool + wantCancels int + }{ + { + // allowed_grpc_services builds creds, then validation + // fails on the empty xds_servers list. + name: "validation_error_releases_allowed_service_creds", + cfg: `{ + "xds_servers": [], + "allowed_grpc_services": { + "dns:///side-channel:443": { + "channel_creds": [{"type": "test_observable_channel_creds"}] + } + } + }`, + wantErr: true, + wantCancels: 1, + }, + { + // A per-authority server builds creds, then validation + // fails on the invalid authority listener template. + name: "validation_error_releases_authority_server_creds", + cfg: `{ + "xds_servers": [{ + "server_uri": "trafficdirector.googleapis.com:443", + "channel_creds": [{"type": "insecure"}] + }], + "authorities": { + "auth1": { + "client_listener_resource_name_template": "invalid-no-prefix", + "xds_servers": [{ + "server_uri": "authority-server:443", + "channel_creds": [{"type": "test_observable_channel_creds"}] + }] + } + } + }`, + wantErr: true, + wantCancels: 1, + }, + { + // allowed_grpc_services (decoded first) builds creds, + // then json.Unmarshal fails on a malformed field. + name: "midway_unmarshal_failure_releases_creds", + cfg: `{ + "allowed_grpc_services": { + "dns:///side-channel:443": { + "channel_creds": [{"type": "test_observable_channel_creds"}] + } + }, + "authorities": "should-be-an-object" + }`, + wantErr: true, + wantCancels: 1, + }, + { + // A later server in the list fails to unmarshal; the + // earlier server's built creds must be released. + name: "intra_list_server_failure_releases_earlier_creds", + cfg: `{ + "xds_servers": [ + {"server_uri": "s1:443", "channel_creds": [{"type": "test_observable_channel_creds"}]}, + {"server_uri": "s2:443", "channel_creds": [{"type": "unsupported_cred_type"}]} + ] + }`, + wantErr: true, + wantCancels: 1, + }, + { + // Same leak, but inside an authority's server list. + name: "intra_authority_server_failure_releases_creds", + cfg: `{ + "xds_servers": [{ + "server_uri": "trafficdirector.googleapis.com:443", + "channel_creds": [{"type": "insecure"}] + }], + "authorities": { + "auth1": { + "xds_servers": [ + {"server_uri": "a1:443", "channel_creds": [{"type": "test_observable_channel_creds"}]}, + {"server_uri": "a2:443", "channel_creds": [{"type": "unsupported_cred_type"}]} + ] + } + } + }`, + wantErr: true, + wantCancels: 1, + }, + { + // A null authority must be rejected, not panic. + name: "null_authority_is_rejected_without_panic", + cfg: `{ + "xds_servers": [{ + "server_uri": "trafficdirector.googleapis.com:443", + "channel_creds": [{"type": "insecure"}] + }], + "authorities": {"foo": null} + }`, + wantErr: true, + wantCancels: 0, + }, + { + // On success, cleanups are retained for the Config's + // lifetime and must not run during parsing. + name: "success_retains_cleanups", + cfg: `{ + "xds_servers": [{ + "server_uri": "trafficdirector.googleapis.com:443", + "channel_creds": [{"type": "insecure"}] + }], + "allowed_grpc_services": { + "dns:///side-channel:443": { + "channel_creds": [{"type": "test_observable_channel_creds"}] + } + } + }`, + wantErr: false, + wantCancels: 0, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + cancels = 0 + var c Config + err := c.UnmarshalJSON([]byte(test.cfg)) + if (err != nil) != test.wantErr { + t.Fatalf("Config.UnmarshalJSON() error = %v, wantErr %v", err, test.wantErr) } - } - }` - var failed Config - if err := failed.UnmarshalJSON([]byte(failCfg)); err == nil { - t.Fatal("Config.UnmarshalJSON() succeeded, want error for empty xds_servers") - } - if cancels != 1 { - t.Errorf("cleanup ran %d times after a failed bootstrap parse, want 1", cancels) - } - - // On a successful parse, cleanups are retained for the Config's lifetime - // and must NOT run during parsing. - cancels = 0 - const okCfg = `{ - "xds_servers": [{ - "server_uri": "trafficdirector.googleapis.com:443", - "channel_creds": [{"type": "insecure"}] - }], - "allowed_grpc_services": { - "dns:///side-channel:443": { - "channel_creds": [{"type": "test_observable_channel_creds"}] + if cancels != test.wantCancels { + t.Errorf("cleanups ran %d times, want %d", cancels, test.wantCancels) } - } - }` - var ok Config - if err := ok.UnmarshalJSON([]byte(okCfg)); err != nil { - t.Fatalf("Config.UnmarshalJSON() failed: %v", err) - } - if cancels != 0 { - t.Errorf("cleanup ran %d times on a successful parse, want 0", cancels) + }) } }