From 4265433bee1a34a64cea5145b9da0e9348d2b5bc Mon Sep 17 00:00:00 2001 From: Pranjali-2501 Date: Tue, 14 Oct 2025 10:16:38 +0000 Subject: [PATCH 1/4] add custom bootstrap config --- internal/xds/xdsclient/pool.go | 57 ++++++++++++++++++------- xds/googledirectpath/googlec2p.go | 23 +++++++++- xds/googledirectpath/googlec2p_test.go | 59 ++++++++++++++++++-------- 3 files changed, 104 insertions(+), 35 deletions(-) diff --git a/internal/xds/xdsclient/pool.go b/internal/xds/xdsclient/pool.go index eb0197e09a7f..ca2b73a4809f 100644 --- a/internal/xds/xdsclient/pool.go +++ b/internal/xds/xdsclient/pool.go @@ -73,6 +73,11 @@ type OptionsForTesting struct { // MetricsRecorder is the metrics recorder the xDS Client will use. If // unspecified, uses a no-op MetricsRecorder. MetricsRecorder estats.MetricsRecorder + + // Config is the xDS bootstrap configuration that will be used to initialize + // the client. If unset, the client will use the config provided by env + // variables. + Config *bootstrap.Config } // NewPool creates a new xDS client pool with the given bootstrap config. @@ -91,6 +96,17 @@ func NewPool(config *bootstrap.Config) *Pool { } } +// NewClientWithConfig returns an xDS client with the given name from the pool. If the +// client doesn't already exist, it creates a new xDS client and adds it to the +// pool. +// +// The second return value represents a close function which the caller is +// expected to invoke once they are done using the client. It is safe for the +// caller to invoke this close function multiple times. +func (p *Pool) NewClientWithConfig(name string, metricsRecorder estats.MetricsRecorder, config *bootstrap.Config) (XDSClient, func(), error) { + return p.newRefCounted(name, metricsRecorder, defaultWatchExpiryTimeout, config) +} + // NewClient returns an xDS client with the given name from the pool. If the // client doesn't already exist, it creates a new xDS client and adds it to the // pool. @@ -99,7 +115,7 @@ func NewPool(config *bootstrap.Config) *Pool { // expected to invoke once they are done using the client. It is safe for the // caller to invoke this close function multiple times. func (p *Pool) NewClient(name string, metricsRecorder estats.MetricsRecorder) (XDSClient, func(), error) { - return p.newRefCounted(name, metricsRecorder, defaultWatchExpiryTimeout) + return p.newRefCounted(name, metricsRecorder, defaultWatchExpiryTimeout, nil) } // NewClientForTesting returns an xDS client configured with the provided @@ -126,7 +142,7 @@ func (p *Pool) NewClientForTesting(opts OptionsForTesting) (XDSClient, func(), e if opts.MetricsRecorder == nil { opts.MetricsRecorder = istats.NewMetricsRecorderList(nil) } - c, cancel, err := p.newRefCounted(opts.Name, opts.MetricsRecorder, opts.WatchExpiryTimeout) + c, cancel, err := p.newRefCounted(opts.Name, opts.MetricsRecorder, opts.WatchExpiryTimeout, opts.Config) if err != nil { return nil, nil, err } @@ -251,30 +267,39 @@ func (p *Pool) clientRefCountedClose(name string) { // newRefCounted creates a new reference counted xDS client implementation for // name, if one does not exist already. If an xDS client for the given name // exists, it gets a reference to it and returns it. -func (p *Pool) newRefCounted(name string, metricsRecorder estats.MetricsRecorder, watchExpiryTimeout time.Duration) (*clientImpl, func(), error) { +func (p *Pool) newRefCounted(name string, metricsRecorder estats.MetricsRecorder, watchExpiryTimeout time.Duration, bConfig *bootstrap.Config) (*clientImpl, func(), error) { p.mu.Lock() defer p.mu.Unlock() - config, err := p.getConfiguration() - if err != nil { - return nil, nil, fmt.Errorf("xds: failed to read xDS bootstrap config from env vars: %v", err) + if c := p.clients[name]; c != nil { + c.incrRef() + return c, sync.OnceFunc(func() { p.clientRefCountedClose(name) }), nil } - if config == nil { - // If the environment variables are not set, then fallback bootstrap - // configuration should be set before attempting to create an xDS client, - // else xDS client creation will fail. - config = p.fallbackConfig + var ( + config *bootstrap.Config + err error + ) + + if bConfig != nil { + config = bConfig + } else { + config, err = p.getConfiguration() + if err != nil { + return nil, nil, fmt.Errorf("xds: failed to read xDS bootstrap config from env vars: %v", err) + } + if config == nil { + // If the environment variables are not set, then fallback bootstrap + // configuration should be set before attempting to create an xDS client, + // else xDS client creation will fail. + config = p.fallbackConfig + } } + if config == nil { return nil, nil, fmt.Errorf("failed to read xDS bootstrap config from env vars: bootstrap environment variables (%q or %q) not defined and fallback config not set", envconfig.XDSBootstrapFileNameEnv, envconfig.XDSBootstrapFileContentEnv) } - if c := p.clients[name]; c != nil { - c.incrRef() - return c, sync.OnceFunc(func() { p.clientRefCountedClose(name) }), nil - } - c, err := newClientImpl(config, metricsRecorder, name, watchExpiryTimeout) if err != nil { return nil, nil, err diff --git a/xds/googledirectpath/googlec2p.go b/xds/googledirectpath/googlec2p.go index 9ef59f1a92a7..ab418cb1c187 100644 --- a/xds/googledirectpath/googlec2p.go +++ b/xds/googledirectpath/googlec2p.go @@ -119,6 +119,16 @@ func getXdsServerURI() string { return fmt.Sprintf("dns:///directpath-pa.%s", universeDomain) } +type c2pResolverWrapper struct { + resolver.Resolver + cancel func() +} + +func (r *c2pResolverWrapper) Close() { + r.Resolver.Close() + r.cancel() +} + type c2pResolverBuilder struct{} func (c2pResolverBuilder) Build(t resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions) (resolver.Resolver, error) { @@ -161,7 +171,6 @@ func (c2pResolverBuilder) Build(t resolver.Target, cc resolver.ClientConn, opts if err != nil { return nil, fmt.Errorf("failed to parse bootstrap contents: %s, %v", string(cfgJSON), err) } - xdsClientPool.SetFallbackBootstrapConfig(config) t = resolver.Target{ URL: url.URL{ @@ -170,7 +179,17 @@ func (c2pResolverBuilder) Build(t resolver.Target, cc resolver.ClientConn, opts Path: t.URL.Path, }, } - return resolver.Get(xdsName).Build(t, cc, opts) + _, cancel, err := xdsClientPool.NewClientWithConfig(t.String(), opts.MetricsRecorder, config) + if err != nil { + return nil, fmt.Errorf("failed to create xds client: %v", err) + } + + r, err := resolver.Get(xdsName).Build(t, cc, opts) + if err != nil { + cancel() + return nil, err + } + return &c2pResolverWrapper{Resolver: r, cancel: cancel}, nil } func (b c2pResolverBuilder) Scheme() string { diff --git a/xds/googledirectpath/googlec2p_test.go b/xds/googledirectpath/googlec2p_test.go index 74ee65db4987..863c97efb401 100644 --- a/xds/googledirectpath/googlec2p_test.go +++ b/xds/googledirectpath/googlec2p_test.go @@ -21,6 +21,7 @@ package googledirectpath import ( "context" "encoding/json" + "net/url" "strconv" "strings" "testing" @@ -155,9 +156,9 @@ func (s) TestBuildWithBootstrapEnvSet(t *testing.T) { } defer r.Close() - // Build should return xDS, not DNS. - if r != testXDSResolver { - t.Fatalf("Build() returned %#v, want xds resolver", r) + // Build should return wrapped xDS resolver, not DNS. + if r, ok := r.(*c2pResolverWrapper); !ok || r.Resolver != testXDSResolver { + t.Fatalf("Build() returned %#v, want c2pResolverWrapper", r) } }) } @@ -314,18 +315,26 @@ func (s) TestBuildXDS(t *testing.T) { defer func() { getIPv6Capable = oldGetIPv6Capability }() // Build the google-c2p resolver. - r, err := builder.Build(resolver.Target{}, nil, resolver.BuildOptions{}) + target := resolver.Target{URL: url.URL{Scheme: c2pScheme, Path: "test-path"}} + r, err := builder.Build(target, nil, resolver.BuildOptions{}) if err != nil { t.Fatalf("failed to build resolver: %v", err) } defer r.Close() - // Build should return xDS, not DNS. - if r != testXDSResolver { - t.Fatalf("Build() returned %#v, want xds resolver", r) + // Build should return wrapped xDS resolver, not DNS. + if r, ok := r.(*c2pResolverWrapper); !ok || r.Resolver != testXDSResolver { + t.Fatalf("Build() returned %#v, want c2pResolverWrapper", r) + } + + xdsTarget := resolver.Target{URL: url.URL{Scheme: xdsName, Host: c2pAuthority, Path: target.URL.Path}} + client, close, err := xdsClientPool.GetClientForTesting(xdsTarget.String()) + if err != nil { + t.Fatalf("Failed to get xds client: %v", err) } + defer close() - gotConfig := xdsClientPool.BootstrapConfigForTesting() + gotConfig := client.BootstrapConfig() if gotConfig == nil { t.Fatalf("Failed to get bootstrap config: %v", err) } @@ -415,18 +424,26 @@ func (s) TestSetUniverseDomainNonDefault(t *testing.T) { defer func() { xdsClientPool = oldXdsClientPool }() // Build the google-c2p resolver. - r, err := builder.Build(resolver.Target{}, nil, resolver.BuildOptions{}) + target := resolver.Target{URL: url.URL{Scheme: c2pScheme, Path: "test-path"}} + r, err := builder.Build(target, nil, resolver.BuildOptions{}) if err != nil { t.Fatalf("failed to build resolver: %v", err) } defer r.Close() - // Build should return xDS, not DNS. - if r != testXDSResolver { - t.Fatalf("Build() returned %#v, want xds resolver", r) + // Build should return wrapped xDS resolver, not DNS. + if r, ok := r.(*c2pResolverWrapper); !ok || r.Resolver != testXDSResolver { + t.Fatalf("Build() returned %#v, want c2pResolverWrapper", r) } - gotConfig := xdsClientPool.BootstrapConfigForTesting() + xdsTarget := resolver.Target{URL: url.URL{Scheme: xdsName, Host: c2pAuthority, Path: target.URL.Path}} + client, close, err := xdsClientPool.GetClientForTesting(xdsTarget.String()) + if err != nil { + t.Fatalf("Failed to get xds client: %v", err) + } + defer close() + + gotConfig := client.BootstrapConfig() if gotConfig == nil { t.Fatalf("Failed to get bootstrap config: %v", err) } @@ -484,18 +501,26 @@ func (s) TestDefaultUniverseDomain(t *testing.T) { defer func() { xdsClientPool = oldXdsClientPool }() // Build the google-c2p resolver. - r, err := builder.Build(resolver.Target{}, nil, resolver.BuildOptions{}) + target := resolver.Target{URL: url.URL{Scheme: c2pScheme, Path: "test-path"}} + r, err := builder.Build(target, nil, resolver.BuildOptions{}) if err != nil { t.Fatalf("failed to build resolver: %v", err) } defer r.Close() // Build should return xDS, not DNS. - if r != testXDSResolver { - t.Fatalf("Build() returned %#v, want xds resolver", r) + if r, ok := r.(*c2pResolverWrapper); !ok || r.Resolver != testXDSResolver { + t.Fatalf("Build() returned %#v, want c2pResolverWrapper", r) + } + + xdsTarget := resolver.Target{URL: url.URL{Scheme: xdsName, Host: c2pAuthority, Path: target.URL.Path}} + client, close, err := xdsClientPool.GetClientForTesting(xdsTarget.String()) + if err != nil { + t.Fatalf("Failed to get xds client: %v", err) } + defer close() - gotConfig := xdsClientPool.BootstrapConfigForTesting() + gotConfig := client.BootstrapConfig() if gotConfig == nil { t.Fatalf("Failed to get bootstrap config: %v", err) } From c1311baacba502b47726e49c631a645200865b0f Mon Sep 17 00:00:00 2001 From: Pranjali-2501 Date: Fri, 17 Oct 2025 12:11:35 +0000 Subject: [PATCH 2/4] added new test --- internal/xds/xdsclient/pool.go | 11 +- xds/googledirectpath/googlec2p.go | 9 +- xds/googledirectpath/googlec2p_test.go | 159 ++++++++++++++++++------- 3 files changed, 127 insertions(+), 52 deletions(-) diff --git a/internal/xds/xdsclient/pool.go b/internal/xds/xdsclient/pool.go index ca2b73a4809f..4627c9185a27 100644 --- a/internal/xds/xdsclient/pool.go +++ b/internal/xds/xdsclient/pool.go @@ -276,14 +276,9 @@ func (p *Pool) newRefCounted(name string, metricsRecorder estats.MetricsRecorder return c, sync.OnceFunc(func() { p.clientRefCountedClose(name) }), nil } - var ( - config *bootstrap.Config - err error - ) - - if bConfig != nil { - config = bConfig - } else { + config := bConfig + if config == nil { + var err error config, err = p.getConfiguration() if err != nil { return nil, nil, fmt.Errorf("xds: failed to read xDS bootstrap config from env vars: %v", err) diff --git a/xds/googledirectpath/googlec2p.go b/xds/googledirectpath/googlec2p.go index ab418cb1c187..14083158f561 100644 --- a/xds/googledirectpath/googlec2p.go +++ b/xds/googledirectpath/googlec2p.go @@ -126,7 +126,7 @@ type c2pResolverWrapper struct { func (r *c2pResolverWrapper) Close() { r.Resolver.Close() - r.cancel() + r.cancel() // Release the reference to the xDS client that was created in Build(). } type c2pResolverBuilder struct{} @@ -179,6 +179,13 @@ func (c2pResolverBuilder) Build(t resolver.Target, cc resolver.ClientConn, opts Path: t.URL.Path, }, } + + // Create a new xDS client for this target using the provided bootstrap + // configuration. This client is stored in the xdsclient pool’s internal + // cache, keeping it alive and associated with this resolver until Closed(). + // While the c2p resolver itself does not directly use the client, creating + // it ensures that when the xDS resolver later requests a client for the + // same target, the existing instance will be reused. _, cancel, err := xdsClientPool.NewClientWithConfig(t.String(), opts.MetricsRecorder, config) if err != nil { return nil, fmt.Errorf("failed to create xds client: %v", err) diff --git a/xds/googledirectpath/googlec2p_test.go b/xds/googledirectpath/googlec2p_test.go index 863c97efb401..7d3ad4261635 100644 --- a/xds/googledirectpath/googlec2p_test.go +++ b/xds/googledirectpath/googlec2p_test.go @@ -32,6 +32,7 @@ import ( "google.golang.org/grpc/credentials/insecure" "google.golang.org/grpc/internal/envconfig" "google.golang.org/grpc/internal/grpctest" + "google.golang.org/grpc/internal/testutils" "google.golang.org/grpc/internal/xds/bootstrap" "google.golang.org/grpc/internal/xds/xdsclient" testgrpc "google.golang.org/grpc/interop/grpc_testing" @@ -127,6 +128,26 @@ func expectedNodeJSON(ipv6Capable bool) []byte { }`) } +// verifyXDSClientBootstrapConfig checks that an xDS client with the given name +// exists in the pool and that its bootstrap config matches the expected config. +func verifyXDSClientBootstrapConfig(t *testing.T, pool *xdsclient.Pool, name string, wantConfig *bootstrap.Config) xdsclient.XDSClient { + t.Helper() + client, close, err := pool.GetClientForTesting(name) + if err != nil { + t.Fatalf("GetClientForTesting(%q) failed to get xds client: %v", name, err) + } + defer close() + + gotConfig := client.BootstrapConfig() + if gotConfig == nil { + t.Fatalf("Failed to get bootstrap config: %v", err) + } + if diff := cmp.Diff(wantConfig, gotConfig); diff != "" { + t.Fatalf("xdsClient.BootstrapConfig() for client %q returned unexpected diff (-want +got):\n%s", name, diff) + } + return client +} + // Tests the scenario where the bootstrap env vars are set and we're running on // GCE. The test builds a google-c2p resolver and verifies that an xDS resolver // is built and that we don't fallback to DNS (because federation is enabled by @@ -328,19 +349,7 @@ func (s) TestBuildXDS(t *testing.T) { } xdsTarget := resolver.Target{URL: url.URL{Scheme: xdsName, Host: c2pAuthority, Path: target.URL.Path}} - client, close, err := xdsClientPool.GetClientForTesting(xdsTarget.String()) - if err != nil { - t.Fatalf("Failed to get xds client: %v", err) - } - defer close() - - gotConfig := client.BootstrapConfig() - if gotConfig == nil { - t.Fatalf("Failed to get bootstrap config: %v", err) - } - if diff := cmp.Diff(tt.wantBootstrapConfig, gotConfig); diff != "" { - t.Fatalf("Unexpected diff in bootstrap config (-want +got):\n%s", diff) - } + verifyXDSClientBootstrapConfig(t, xdsClientPool, xdsTarget.String(), tt.wantBootstrapConfig) }) } } @@ -436,18 +445,6 @@ func (s) TestSetUniverseDomainNonDefault(t *testing.T) { t.Fatalf("Build() returned %#v, want c2pResolverWrapper", r) } - xdsTarget := resolver.Target{URL: url.URL{Scheme: xdsName, Host: c2pAuthority, Path: target.URL.Path}} - client, close, err := xdsClientPool.GetClientForTesting(xdsTarget.String()) - if err != nil { - t.Fatalf("Failed to get xds client: %v", err) - } - defer close() - - gotConfig := client.BootstrapConfig() - if gotConfig == nil { - t.Fatalf("Failed to get bootstrap config: %v", err) - } - // Check that we use directpath-pa.test-universe-domain.test in the // bootstrap config. wantBootstrapConfig := bootstrapConfig(t, bootstrap.ConfigOptionsForTesting{ @@ -469,9 +466,9 @@ func (s) TestSetUniverseDomainNonDefault(t *testing.T) { }, Node: expectedNodeJSON(false), }) - if diff := cmp.Diff(wantBootstrapConfig, gotConfig); diff != "" { - t.Fatalf("Unexpected diff in bootstrap config (-want +got):\n%s", diff) - } + + xdsTarget := resolver.Target{URL: url.URL{Scheme: xdsName, Host: c2pAuthority, Path: target.URL.Path}} + verifyXDSClientBootstrapConfig(t, xdsClientPool, xdsTarget.String(), wantBootstrapConfig) } func (s) TestDefaultUniverseDomain(t *testing.T) { @@ -513,18 +510,6 @@ func (s) TestDefaultUniverseDomain(t *testing.T) { t.Fatalf("Build() returned %#v, want c2pResolverWrapper", r) } - xdsTarget := resolver.Target{URL: url.URL{Scheme: xdsName, Host: c2pAuthority, Path: target.URL.Path}} - client, close, err := xdsClientPool.GetClientForTesting(xdsTarget.String()) - if err != nil { - t.Fatalf("Failed to get xds client: %v", err) - } - defer close() - - gotConfig := client.BootstrapConfig() - if gotConfig == nil { - t.Fatalf("Failed to get bootstrap config: %v", err) - } - // Check that we use directpath-pa.googleapis.com in the bootstrap config wantBootstrapConfig := bootstrapConfig(t, bootstrap.ConfigOptionsForTesting{ Servers: []byte(`[{ @@ -545,9 +530,8 @@ func (s) TestDefaultUniverseDomain(t *testing.T) { }, Node: expectedNodeJSON(false), }) - if diff := cmp.Diff(wantBootstrapConfig, gotConfig); diff != "" { - t.Fatalf("Unexpected diff in bootstrap config (-want +got):\n%s", diff) - } + xdsTarget := resolver.Target{URL: url.URL{Scheme: xdsName, Host: c2pAuthority, Path: target.URL.Path}} + verifyXDSClientBootstrapConfig(t, xdsClientPool, xdsTarget.String(), wantBootstrapConfig) // Now set universe domain to something different than the default, it should fail domain := "test-universe-domain.test" @@ -574,3 +558,92 @@ func (s) TestSetUniverseDomainEmptyString(t *testing.T) { t.Fatalf("googlec2p.SetUniverseDomain(\"\") returned error: %v, want: %v", err, wantErr) } } + +// TestCreateMultipleXDSClients validates that multiple xds clients with +// different bootstrap config coexist in the same pool. It confirms +// that a client created by the google-c2p resolver does not interfere with an +// explicitly created client using a different bootstrap configuration. +func (s) TestCreateMultipleXDSClients(t *testing.T) { + replaceResolvers(t) + simulateRunningOnGCE(t, true) + useCleanUniverseDomain(t) + + // Override the zone returned by the metadata server. + oldGetZone := getZone + getZone = func(time.Duration) string { return "test-zone" } + defer func() { getZone = oldGetZone }() + + // Override the random func used in the node ID. + origRandInd := randInt + randInt = func() int { return 666 } + defer func() { randInt = origRandInd }() + + // Override IPv6 capability returned by the metadata server. + oldGetIPv6Capability := getIPv6Capable + getIPv6Capable = func(time.Duration) bool { return false } + defer func() { getIPv6Capable = oldGetIPv6Capability }() + + // Define bootstrap config for generic xds resolver + genericXDSConfig := bootstrapConfig(t, bootstrap.ConfigOptionsForTesting{ + Servers: []byte(`[{ + "server_uri": "dns:///regular-xds.googleapis.com", + "channel_creds": [{"type": "google_default"}], + "server_features": [] + }]`), + Node: []byte(`{"id": "regular-xds-node", "locality": {"zone": "test-zone"}}`), + }) + + // Override xDS client pool. + oldXdsClientPool := xdsClientPool + xdsClientPool = xdsclient.NewPool(genericXDSConfig) + defer func() { xdsClientPool = oldXdsClientPool }() + + // Define bootstrap config for c2p resolver + c2pConfig := bootstrapConfig(t, bootstrap.ConfigOptionsForTesting{ + Servers: []byte(`[{ + "server_uri": "dns:///directpath-pa.googleapis.com", + "channel_creds": [{"type": "google_default"}], + "server_features": ["ignore_resource_deletion"] + }]`), + Authorities: map[string]json.RawMessage{ + "traffic-director-c2p.xds.googleapis.com": []byte(`{ + "xds_servers": [{ + "server_uri": "dns:///directpath-pa.googleapis.com", + "channel_creds": [{"type": "google_default"}], + "server_features": ["ignore_resource_deletion"] + }] + }`), + }, + Node: expectedNodeJSON(false), + }) + + // Create generic xds client. + xdsTarget := resolver.Target{URL: *testutils.MustParseURL("xds:///target")} + _, closeGeneric, err := xdsClientPool.NewClientForTesting(xdsclient.OptionsForTesting{ + Name: xdsTarget.String(), + Config: genericXDSConfig, + }) + if err != nil { + t.Fatalf("xdsClientPool.NewClientForTesting(%q) failed: %v", xdsTarget.String(), err) + } + defer closeGeneric() + clientGeneric := verifyXDSClientBootstrapConfig(t, xdsClientPool, xdsTarget.String(), genericXDSConfig) + + // Build the google-c2p resolver. + c2pBuilder := resolver.Get(c2pScheme) + c2pTarget := resolver.Target{URL: url.URL{Scheme: c2pScheme, Path: "test-path"}} + tcc := &testutils.ResolverClientConn{Logger: t} + c2pRes, err := c2pBuilder.Build(c2pTarget, tcc, resolver.BuildOptions{}) + if err != nil { + t.Fatalf("Failed to build resolver: %v", err) + } + defer c2pRes.Close() + c2pXDSTarget := resolver.Target{URL: url.URL{Scheme: xdsName, Host: c2pAuthority, Path: c2pTarget.URL.Path}} + clientC2P := verifyXDSClientBootstrapConfig(t, xdsClientPool, c2pXDSTarget.String(), c2pConfig) + + // Verify that the two clients are distinct instances. + if clientC2P == clientGeneric { + t.Fatal("Expected two distinct xDS clients, but got the same one for both") + } +} + From 85e62a8e4fcbccbc5b94df11b91237f6c4642c01 Mon Sep 17 00:00:00 2001 From: Pranjali-2501 Date: Fri, 17 Oct 2025 13:48:41 +0000 Subject: [PATCH 3/4] added todo --- internal/xds/xdsclient/pool.go | 6 +++++- xds/googledirectpath/googlec2p_test.go | 9 +++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/internal/xds/xdsclient/pool.go b/internal/xds/xdsclient/pool.go index 4627c9185a27..8179b0f1041b 100644 --- a/internal/xds/xdsclient/pool.go +++ b/internal/xds/xdsclient/pool.go @@ -49,7 +49,7 @@ type Pool struct { // config. mu sync.Mutex clients map[string]*clientImpl - fallbackConfig *bootstrap.Config + fallbackConfig *bootstrap.Config // TODO(i/8661): remove fallbackConfig. // getConfiguration is a sync.OnceValues that attempts to read the bootstrap // configuration from environment variables once. getConfiguration func() (*bootstrap.Config, error) @@ -175,6 +175,7 @@ func (p *Pool) GetClientForTesting(name string) (XDSClient, func(), error) { // SetFallbackBootstrapConfig is used to specify a bootstrap configuration // that will be used as a fallback when the bootstrap environment variables // are not defined. +// TODO(i/8661): remove SetFallbackBootstrapConfig function. func (p *Pool) SetFallbackBootstrapConfig(config *bootstrap.Config) { p.mu.Lock() defer p.mu.Unlock() @@ -214,6 +215,7 @@ func (p *Pool) BootstrapConfigForTesting() *bootstrap.Config { if cfg != nil { return cfg } + // TODO(i/8661) return p.fallbackConfig } @@ -224,6 +226,7 @@ func (p *Pool) BootstrapConfigForTesting() *bootstrap.Config { func (p *Pool) UnsetBootstrapConfigForTesting() { p.mu.Lock() defer p.mu.Unlock() + // TODO(i/8661) p.fallbackConfig = nil p.getConfiguration = sync.OnceValues(bootstrap.GetConfiguration) } @@ -284,6 +287,7 @@ func (p *Pool) newRefCounted(name string, metricsRecorder estats.MetricsRecorder return nil, nil, fmt.Errorf("xds: failed to read xDS bootstrap config from env vars: %v", err) } if config == nil { + // TODO(i/8661) // If the environment variables are not set, then fallback bootstrap // configuration should be set before attempting to create an xDS client, // else xDS client creation will fail. diff --git a/xds/googledirectpath/googlec2p_test.go b/xds/googledirectpath/googlec2p_test.go index 7d3ad4261635..f495ded404f1 100644 --- a/xds/googledirectpath/googlec2p_test.go +++ b/xds/googledirectpath/googlec2p_test.go @@ -560,8 +560,8 @@ func (s) TestSetUniverseDomainEmptyString(t *testing.T) { } // TestCreateMultipleXDSClients validates that multiple xds clients with -// different bootstrap config coexist in the same pool. It confirms -// that a client created by the google-c2p resolver does not interfere with an +// different bootstrap config coexist in the same pool. It confirms that +// a client created by the google-c2p resolver does not interfere with an // explicitly created client using a different bootstrap configuration. func (s) TestCreateMultipleXDSClients(t *testing.T) { replaceResolvers(t) @@ -627,6 +627,7 @@ func (s) TestCreateMultipleXDSClients(t *testing.T) { t.Fatalf("xdsClientPool.NewClientForTesting(%q) failed: %v", xdsTarget.String(), err) } defer closeGeneric() + clientGeneric := verifyXDSClientBootstrapConfig(t, xdsClientPool, xdsTarget.String(), genericXDSConfig) // Build the google-c2p resolver. @@ -638,12 +639,12 @@ func (s) TestCreateMultipleXDSClients(t *testing.T) { t.Fatalf("Failed to build resolver: %v", err) } defer c2pRes.Close() + c2pXDSTarget := resolver.Target{URL: url.URL{Scheme: xdsName, Host: c2pAuthority, Path: c2pTarget.URL.Path}} clientC2P := verifyXDSClientBootstrapConfig(t, xdsClientPool, c2pXDSTarget.String(), c2pConfig) // Verify that the two clients are distinct instances. if clientC2P == clientGeneric { - t.Fatal("Expected two distinct xDS clients, but got the same one for both") + t.Fatal("Expected two distinct xDS clients, but got same") } } - From 6f4b8df3ca7e2113b972c08ab55b4a692833fb76 Mon Sep 17 00:00:00 2001 From: Pranjali-2501 Date: Fri, 17 Oct 2025 17:20:12 +0000 Subject: [PATCH 4/4] resolving comments --- internal/xds/xdsclient/pool.go | 3 -- xds/googledirectpath/googlec2p.go | 4 +-- xds/googledirectpath/googlec2p_test.go | 50 ++++++++++++-------------- 3 files changed, 24 insertions(+), 33 deletions(-) diff --git a/internal/xds/xdsclient/pool.go b/internal/xds/xdsclient/pool.go index 8179b0f1041b..fa11a9500775 100644 --- a/internal/xds/xdsclient/pool.go +++ b/internal/xds/xdsclient/pool.go @@ -215,7 +215,6 @@ func (p *Pool) BootstrapConfigForTesting() *bootstrap.Config { if cfg != nil { return cfg } - // TODO(i/8661) return p.fallbackConfig } @@ -226,7 +225,6 @@ func (p *Pool) BootstrapConfigForTesting() *bootstrap.Config { func (p *Pool) UnsetBootstrapConfigForTesting() { p.mu.Lock() defer p.mu.Unlock() - // TODO(i/8661) p.fallbackConfig = nil p.getConfiguration = sync.OnceValues(bootstrap.GetConfiguration) } @@ -287,7 +285,6 @@ func (p *Pool) newRefCounted(name string, metricsRecorder estats.MetricsRecorder return nil, nil, fmt.Errorf("xds: failed to read xDS bootstrap config from env vars: %v", err) } if config == nil { - // TODO(i/8661) // If the environment variables are not set, then fallback bootstrap // configuration should be set before attempting to create an xDS client, // else xDS client creation will fail. diff --git a/xds/googledirectpath/googlec2p.go b/xds/googledirectpath/googlec2p.go index 14083158f561..01cca3d3e720 100644 --- a/xds/googledirectpath/googlec2p.go +++ b/xds/googledirectpath/googlec2p.go @@ -121,12 +121,12 @@ func getXdsServerURI() string { type c2pResolverWrapper struct { resolver.Resolver - cancel func() + cancel func() // Release the reference to the xDS client that was created in Build(). } func (r *c2pResolverWrapper) Close() { r.Resolver.Close() - r.cancel() // Release the reference to the xDS client that was created in Build(). + r.cancel() } type c2pResolverBuilder struct{} diff --git a/xds/googledirectpath/googlec2p_test.go b/xds/googledirectpath/googlec2p_test.go index f495ded404f1..3d9972baef23 100644 --- a/xds/googledirectpath/googlec2p_test.go +++ b/xds/googledirectpath/googlec2p_test.go @@ -130,7 +130,7 @@ func expectedNodeJSON(ipv6Capable bool) []byte { // verifyXDSClientBootstrapConfig checks that an xDS client with the given name // exists in the pool and that its bootstrap config matches the expected config. -func verifyXDSClientBootstrapConfig(t *testing.T, pool *xdsclient.Pool, name string, wantConfig *bootstrap.Config) xdsclient.XDSClient { +func verifyXDSClientBootstrapConfig(t *testing.T, pool *xdsclient.Pool, name string, wantConfig *bootstrap.Config) { t.Helper() client, close, err := pool.GetClientForTesting(name) if err != nil { @@ -145,7 +145,6 @@ func verifyXDSClientBootstrapConfig(t *testing.T, pool *xdsclient.Pool, name str if diff := cmp.Diff(wantConfig, gotConfig); diff != "" { t.Fatalf("xdsClient.BootstrapConfig() for client %q returned unexpected diff (-want +got):\n%s", name, diff) } - return client } // Tests the scenario where the bootstrap env vars are set and we're running on @@ -598,25 +597,6 @@ func (s) TestCreateMultipleXDSClients(t *testing.T) { xdsClientPool = xdsclient.NewPool(genericXDSConfig) defer func() { xdsClientPool = oldXdsClientPool }() - // Define bootstrap config for c2p resolver - c2pConfig := bootstrapConfig(t, bootstrap.ConfigOptionsForTesting{ - Servers: []byte(`[{ - "server_uri": "dns:///directpath-pa.googleapis.com", - "channel_creds": [{"type": "google_default"}], - "server_features": ["ignore_resource_deletion"] - }]`), - Authorities: map[string]json.RawMessage{ - "traffic-director-c2p.xds.googleapis.com": []byte(`{ - "xds_servers": [{ - "server_uri": "dns:///directpath-pa.googleapis.com", - "channel_creds": [{"type": "google_default"}], - "server_features": ["ignore_resource_deletion"] - }] - }`), - }, - Node: expectedNodeJSON(false), - }) - // Create generic xds client. xdsTarget := resolver.Target{URL: *testutils.MustParseURL("xds:///target")} _, closeGeneric, err := xdsClientPool.NewClientForTesting(xdsclient.OptionsForTesting{ @@ -628,7 +608,7 @@ func (s) TestCreateMultipleXDSClients(t *testing.T) { } defer closeGeneric() - clientGeneric := verifyXDSClientBootstrapConfig(t, xdsClientPool, xdsTarget.String(), genericXDSConfig) + verifyXDSClientBootstrapConfig(t, xdsClientPool, xdsTarget.String(), genericXDSConfig) // Build the google-c2p resolver. c2pBuilder := resolver.Get(c2pScheme) @@ -640,11 +620,25 @@ func (s) TestCreateMultipleXDSClients(t *testing.T) { } defer c2pRes.Close() - c2pXDSTarget := resolver.Target{URL: url.URL{Scheme: xdsName, Host: c2pAuthority, Path: c2pTarget.URL.Path}} - clientC2P := verifyXDSClientBootstrapConfig(t, xdsClientPool, c2pXDSTarget.String(), c2pConfig) + // Bootstrap config for c2p resolver. + c2pConfig := bootstrapConfig(t, bootstrap.ConfigOptionsForTesting{ + Servers: []byte(`[{ + "server_uri": "dns:///directpath-pa.googleapis.com", + "channel_creds": [{"type": "google_default"}], + "server_features": ["ignore_resource_deletion"] + }]`), + Authorities: map[string]json.RawMessage{ + "traffic-director-c2p.xds.googleapis.com": []byte(`{ + "xds_servers": [{ + "server_uri": "dns:///directpath-pa.googleapis.com", + "channel_creds": [{"type": "google_default"}], + "server_features": ["ignore_resource_deletion"] + }] + }`), + }, + Node: expectedNodeJSON(false), + }) - // Verify that the two clients are distinct instances. - if clientC2P == clientGeneric { - t.Fatal("Expected two distinct xDS clients, but got same") - } + c2pXDSTarget := resolver.Target{URL: url.URL{Scheme: xdsName, Host: c2pAuthority, Path: c2pTarget.URL.Path}} + verifyXDSClientBootstrapConfig(t, xdsClientPool, c2pXDSTarget.String(), c2pConfig) }