Repository: grpc/grpc-go
Pull request #8419 (2025-06-30 07:37:21)

xds: Avoid error logs when setting fallback bootstrap config
Initially, there was [no error log](https://github.com/grpc/grpc-go/blob/947e2a4be2bac21c84b99114c12c40bb74860afe/internal/xds/bootstrap/bootstrap.go#L819-L829) when setting the fallback bootstrap config. The [GetConfiguration](https://github.com/grpc/grpc-go/blob/947e2a4be2bac21c84b99114c12c40bb74860afe/internal/xds/bootstrap/bootstrap.go#L588-L618) function would first attempt to read the bootstrap configuration from environment variables and then fall back to the configuration provided by the c2p resolver.

A [subsequent change](https://github.com/grpc/grpc-go/pull/7898/files#diff-8c0a0efa1bb340ffe5846ebebb64f9182dcbb1218d0a51a6b3d6d340072ab07e), which added an xDS client pool, modified this behavior. It read the bootstrap config within an init() function in clientimpl.go, preventing the c2p resolver from setting its fallback configuration after channel creation. A fix was implemented ([#8050](https://github.com/grpc/grpc-go/pull/8050)) to allow setting a fallback bootstrap config on an xDS client pool, but this introduced an error log when setting the fallback config twice. This happens when multiple c2p resolvers are created in the same process.

After that, [#8164](https://github.com/grpc/grpc-go/pull/8164) reverted the behavior of reading the bootstrap in the init() function. This allows us to return to the original way—reading from environment variables and then falling back to the c2p resolver's config—without requiring an error log.

RELEASE NOTES:
* googledirectpath: Avoid error the error log `Attempt to set a bootstrap configuration...` when creating multiple directpath channels.

arjan-bal: FYI @apolcyn 

Patch:
diff --git a/internal/xds/bootstrap/bootstrap.go b/internal/xds/bootstrap/bootstrap.go
index f409e4bd..6c7b2332 100644
--- a/internal/xds/bootstrap/bootstrap.go
+++ b/internal/xds/bootstrap/bootstrap.go
@@ -576,6 +576,9 @@ func (c *Config) UnmarshalJSON(data []byte) error {
 // the presence of the errors) and may return a Config object with certain
 // fields left unspecified, in which case the caller should use some sane
 // defaults.
+//
+// This function returns an error if it's unable to parse the contents of the
+// bootstrap config. It returns (nil, nil) if none of the env vars are set.
 func GetConfiguration() (*Config, error) {
 	fName := envconfig.XDSBootstrapFileName
 	fContent := envconfig.XDSBootstrapFileContent
@@ -598,7 +601,7 @@ func GetConfiguration() (*Config, error) {
 		return NewConfigFromContents([]byte(fContent))
 	}
 
-	return nil, fmt.Errorf("bootstrap environment variables (%q or %q) not defined", envconfig.XDSBootstrapFileNameEnv, envconfig.XDSBootstrapFileContentEnv)
+	return nil, nil
 }
 
 // NewConfigFromContents creates a new bootstrap configuration from the provided
diff --git a/xds/internal/xdsclient/pool.go b/xds/internal/xdsclient/pool.go
index 113e6e8e..e5a34ae3 100644
--- a/xds/internal/xdsclient/pool.go
+++ b/xds/internal/xdsclient/pool.go
@@ -25,6 +25,7 @@ import (
 
 	v3statuspb "github.com/envoyproxy/go-control-plane/envoy/service/status/v3"
 	estats "google.golang.org/grpc/experimental/stats"
+	"google.golang.org/grpc/internal/envconfig"
 	istats "google.golang.org/grpc/internal/stats"
 	"google.golang.org/grpc/internal/xds/bootstrap"
 	"google.golang.org/protobuf/proto"
@@ -34,7 +35,10 @@ var (
 	// DefaultPool is the default pool for xDS clients. It is created at init
 	// time and reads bootstrap configuration from env vars to create the xDS
 	// client.
-	DefaultPool = &Pool{clients: make(map[string]*clientImpl)}
+	DefaultPool = &Pool{
+		clients:          make(map[string]*clientImpl),
+		getConfiguration: sync.OnceValues(bootstrap.GetConfiguration),
+	}
 )
 
 // Pool represents a pool of xDS clients that share the same bootstrap
@@ -43,9 +47,12 @@ type Pool struct {
 	// Note that mu should ideally only have to guard clients. But here, we need
 	// it to guard config as well since SetFallbackBootstrapConfig writes to
 	// config.
-	mu      sync.Mutex
-	clients map[string]*clientImpl
-	config  *bootstrap.Config
+	mu             sync.Mutex
+	clients        map[string]*clientImpl
+	fallbackConfig *bootstrap.Config
+	// getConfiguration is a sync.OnceValues that attempts to read the bootstrap
+	// configuration from environment variables once.
+	getConfiguration func() (*bootstrap.Config, error)
 }
 
 // OptionsForTesting contains options to configure xDS client creation for
@@ -78,7 +85,9 @@ type OptionsForTesting struct {
 func NewPool(config *bootstrap.Config) *Pool {
 	return &Pool{
 		clients: make(map[string]*clientImpl),
-		config:  config,
+		getConfiguration: func() (*bootstrap.Config, error) {
+			return config, nil
+		},
 	}
 }
 
@@ -154,12 +163,7 @@ func (p *Pool) GetClientForTesting(name string) (XDSClient, func(), error) {
 func (p *Pool) SetFallbackBootstrapConfig(config *bootstrap.Config) {
 	p.mu.Lock()
 	defer p.mu.Unlock()
-
-	if p.config != nil {
-		logger.Error("Attempt to set a bootstrap configuration even though one is already set via environment variables.")
-		return
-	}
-	p.config = config
+	p.fallbackConfig = config
 }
 
 // DumpResources returns the status and contents of all xDS resources.
@@ -191,7 +195,11 @@ func (p *Pool) DumpResources() *v3statuspb.ClientStatusResponse {
 func (p *Pool) BootstrapConfigForTesting() *bootstrap.Config {
 	p.mu.Lock()
 	defer p.mu.Unlock()
-	return p.config
+	cfg, _ := p.getConfiguration()
+	if cfg != nil {
+		return cfg
+	}
+	return p.fallbackConfig
 }
 
 // UnsetBootstrapConfigForTesting unsets the bootstrap configuration used by
@@ -201,7 +209,8 @@ func (p *Pool) BootstrapConfigForTesting() *bootstrap.Config {
 func (p *Pool) UnsetBootstrapConfigForTesting() {
 	p.mu.Lock()
 	defer p.mu.Unlock()
-	p.config = nil
+	p.fallbackConfig = nil
+	p.getConfiguration = sync.OnceValues(bootstrap.GetConfiguration)
 }
 
 func (p *Pool) clientRefCountedClose(name string) {
@@ -243,26 +252,19 @@ func (p *Pool) newRefCounted(name string, metricsRecorder estats.MetricsRecorder
 	p.mu.Lock()
 	defer p.mu.Unlock()
 
-	if p.config == nil {
-		if len(p.clients) != 0 || p != DefaultPool {
-			// If the current pool `p` already contains xDS clients or it is not
-			// the `DefaultPool`, the bootstrap config should have been already
-			// present in the pool.
-			return nil, nil, fmt.Errorf("xds: bootstrap configuration not set in the pool")
-		}
-		// If the current pool `p` is the `DefaultPool` and has no clients, it
-		// might be the first time an xDS client is being created on it. So,
-		// the bootstrap configuration is read from environment variables.
-		//
-		// DefaultPool is initialized with bootstrap configuration from one of the
-		// supported environment variables. 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, err := bootstrap.GetConfiguration()
-		if err != nil {
-			return nil, nil, fmt.Errorf("xds: failed to read xDS bootstrap config from env vars:  %v", err)
-		}
-		p.config = config
+	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 {
@@ -270,16 +272,16 @@ func (p *Pool) newRefCounted(name string, metricsRecorder estats.MetricsRecorder
 		return c, sync.OnceFunc(func() { p.clientRefCountedClose(name) }), nil
 	}
 
-	c, err := newClientImpl(p.config, metricsRecorder, name)
+	c, err := newClientImpl(config, metricsRecorder, name)
 	if err != nil {
 		return nil, nil, err
 	}
 	if logger.V(2) {
-		c.logger.Infof("Created client with name %q and bootstrap configuration:\n %s", name, p.config)
+		c.logger.Infof("Created client with name %q and bootstrap configuration:\n %s", name, config)
 	}
 	p.clients[name] = c
 	xdsClientImplCreateHook(name)
 
-	logger.Infof("xDS node ID: %s", p.config.Node().GetId())
+	logger.Infof("xDS node ID: %s", config.Node().GetId())
 	return c, sync.OnceFunc(func() { p.clientRefCountedClose(name) }), nil
 }


Test patch:
diff --git a/internal/xds/bootstrap/bootstrap_test.go b/internal/xds/bootstrap/bootstrap_test.go
index d0571978..88f27a4a 100644
--- a/internal/xds/bootstrap/bootstrap_test.go
+++ b/internal/xds/bootstrap/bootstrap_test.go
@@ -383,12 +383,19 @@ func (s) TestGetConfiguration_Failure(t *testing.T) {
 	cancel := setupBootstrapOverride(bootstrapFileMap)
 	defer cancel()
 
-	for _, name := range []string{"nonExistentBootstrapFile", "empty", "badJSON", "noBalancerName", "emptyXdsServer"} {
+	for _, name := range []string{"nonExistentBootstrapFile", "badJSON", "noBalancerName", "emptyXdsServer"} {
 		t.Run(name, func(t *testing.T) {
 			testGetConfigurationWithFileNameEnv(t, name, true, nil)
 			testGetConfigurationWithFileContentEnv(t, name, true, nil)
 		})
 	}
+	const name = "empty"
+	t.Run(name, func(t *testing.T) {
+		testGetConfigurationWithFileNameEnv(t, name, true, nil)
+		// If both the env vars are empty, a nil config with a nil error must be
+		// returned.
+		testGetConfigurationWithFileContentEnv(t, name, false, nil)
+	})
 }
 
 // Tests the functionality in GetConfiguration with different bootstrap file
@@ -462,9 +469,9 @@ func (s) TestGetConfiguration_BootstrapEnvPriority(t *testing.T) {
 	envconfig.XDSBootstrapFileContent = ""
 	defer func() { envconfig.XDSBootstrapFileContent = origBootstrapContent }()
 
-	// When both env variables are empty, GetConfiguration should fail.
-	if _, err := GetConfiguration(); err == nil {
-		t.Errorf("GetConfiguration() returned nil error, expected to fail")
+	// When both env variables are empty, GetConfiguration should return nil.
+	if cfg, err := GetConfiguration(); err != nil || cfg != nil {
+		t.Errorf("GetConfiguration() returned (%v, %v), want (<nil>, <nil>)", cfg, err)
 	}
 
 	// When one of them is set, it should be used.
diff --git a/xds/internal/balancer/clustermanager/clustermanager_test.go b/xds/internal/balancer/clustermanager/clustermanager_test.go
index 91166c17..3afdc834 100644
--- a/xds/internal/balancer/clustermanager/clustermanager_test.go
+++ b/xds/internal/balancer/clustermanager/clustermanager_test.go
@@ -73,7 +73,7 @@ func testPick(t *testing.T, p balancer.Picker, info balancer.PickInfo, wantSC ba
 	}
 }
 
-func TestClusterPicks(t *testing.T) {
+func (s) TestClusterPicks(t *testing.T) {
 	cc := testutils.NewBalancerClientConn(t)
 	builder := balancer.Get(balancerName)
 	parser := builder.(balancer.ConfigParser)
@@ -154,7 +154,7 @@ func TestClusterPicks(t *testing.T) {
 
 // TestConfigUpdateAddCluster covers the cases the balancer receives config
 // update with extra clusters.
-func TestConfigUpdateAddCluster(t *testing.T) {
+func (s) TestConfigUpdateAddCluster(t *testing.T) {
 	cc := testutils.NewBalancerClientConn(t)
 	builder := balancer.Get(balancerName)
 	parser := builder.(balancer.ConfigParser)
@@ -314,7 +314,7 @@ func TestConfigUpdateAddCluster(t *testing.T) {
 
 // TestRoutingConfigUpdateDeleteAll covers the cases the balancer receives
 // config update with no clusters. Pick should fail with details in error.
-func TestRoutingConfigUpdateDeleteAll(t *testing.T) {
+func (s) TestRoutingConfigUpdateDeleteAll(t *testing.T) {
 	cc := testutils.NewBalancerClientConn(t)
 	builder := balancer.Get(balancerName)
 	parser := builder.(balancer.ConfigParser)
@@ -477,7 +477,7 @@ func TestRoutingConfigUpdateDeleteAll(t *testing.T) {
 	}
 }
 
-func TestClusterManagerForwardsBalancerBuildOptions(t *testing.T) {
+func (s) TestClusterManagerForwardsBalancerBuildOptions(t *testing.T) {
 	const (
 		userAgent          = "ua"
 		defaultTestTimeout = 1 * time.Second
@@ -561,7 +561,7 @@ func init() {
 
 // TestInitialIdle covers the case that if the child reports Idle, the overall
 // state will be Idle.
-func TestInitialIdle(t *testing.T) {
+func (s) TestInitialIdle(t *testing.T) {
 	cc := testutils.NewBalancerClientConn(t)
 	builder := balancer.Get(balancerName)
 	parser := builder.(balancer.ConfigParser)
@@ -608,7 +608,7 @@ func TestInitialIdle(t *testing.T) {
 // switches this child to a pick first load balancer. Once that balancer updates
 // it's state and completes the graceful switch process the new picker should
 // reflect this change.
-func TestClusterGracefulSwitch(t *testing.T) {
+func (s) TestClusterGracefulSwitch(t *testing.T) {
 	cc := testutils.NewBalancerClientConn(t)
 	builder := balancer.Get(balancerName)
 	parser := builder.(balancer.ConfigParser)
diff --git a/xds/internal/balancer/clustermanager/config_test.go b/xds/internal/balancer/clustermanager/config_test.go
index 23c25755..ef496a2e 100644
--- a/xds/internal/balancer/clustermanager/config_test.go
+++ b/xds/internal/balancer/clustermanager/config_test.go
@@ -96,7 +96,7 @@ var (
 	wtConfig2, _ = wtConfigParser.ParseConfig([]byte(wtConfigJSON2))
 )
 
-func Test_parseConfig(t *testing.T) {
+func (s) Test_parseConfig(t *testing.T) {
 	tests := []struct {
 		name    string
 		js      string
diff --git a/xds/internal/xdsclient/pool/pool_test.go b/xds/internal/xdsclient/pool/pool_test.go
index b12cc737..8b8cfe4b 100644
--- a/xds/internal/xdsclient/pool/pool_test.go
+++ b/xds/internal/xdsclient/pool/pool_test.go
@@ -73,6 +73,10 @@ func (s) TestDefaultPool_LazyLoadBootstrapConfig(t *testing.T) {
 		t.Fatalf("DefaultPool.BootstrapConfigForTesting() = %v, want nil", cfg)
 	}
 
+	// The pool will attempt to read the env vars only once. Reset the pool's
+	// state to make it re-read the env vars during next client creation.
+	xdsclient.DefaultPool.UnsetBootstrapConfigForTesting()
+
 	_, closeFunc, err = xdsclient.DefaultPool.NewClient(t.Name(), &stats.NoopMetricsRecorder{})
 	if err != nil {
 		t.Fatalf("Failed to create xDS client: %v", err)