From c27e6dc31200b72b3937ea310f6e47bf53b9eebf Mon Sep 17 00:00:00 2001 From: Purnesh Dixit Date: Tue, 18 Mar 2025 09:36:56 +0530 Subject: [PATCH] xdsclient: read bootstrap config before creating the first xDS client in DefaultPool (#8164) --- xds/internal/xdsclient/clientimpl.go | 14 +--- xds/internal/xdsclient/pool.go | 20 +++++- xds/internal/xdsclient/pool/pool_test.go | 88 ++++++++++++++++++++++++ xds/server_test.go | 2 +- 4 files changed, 109 insertions(+), 15 deletions(-) create mode 100644 xds/internal/xdsclient/pool/pool_test.go diff --git a/xds/internal/xdsclient/clientimpl.go b/xds/internal/xdsclient/clientimpl.go index c30c2b45b..966986d2f 100644 --- a/xds/internal/xdsclient/clientimpl.go +++ b/xds/internal/xdsclient/clientimpl.go @@ -113,19 +113,7 @@ func init() { internal.TriggerXDSResourceNotFoundForTesting = triggerXDSResourceNotFoundForTesting xdsclientinternal.ResourceWatchStateForTesting = resourceWatchStateForTesting - // 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 { - if logger.V(2) { - logger.Infof("Failed to read xDS bootstrap config from env vars: %v", err) - } - DefaultPool = &Pool{clients: make(map[string]*clientRefCounted)} - return - } - DefaultPool = &Pool{clients: make(map[string]*clientRefCounted), config: config} + DefaultPool = &Pool{clients: make(map[string]*clientRefCounted)} } // newClientImpl returns a new xdsClient with the given config. diff --git a/xds/internal/xdsclient/pool.go b/xds/internal/xdsclient/pool.go index 4f3ad7c42..4a9c0e092 100644 --- a/xds/internal/xdsclient/pool.go +++ b/xds/internal/xdsclient/pool.go @@ -220,7 +220,25 @@ func (p *Pool) newRefCounted(name string, watchExpiryTimeout time.Duration, stre defer p.mu.Unlock() if p.config == nil { - return nil, nil, fmt.Errorf("xds: bootstrap configuration not set in the pool") + 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 } if c := p.clients[name]; c != nil { diff --git a/xds/internal/xdsclient/pool/pool_test.go b/xds/internal/xdsclient/pool/pool_test.go new file mode 100644 index 000000000..b12cc737b --- /dev/null +++ b/xds/internal/xdsclient/pool/pool_test.go @@ -0,0 +1,88 @@ +/* + * + * Copyright 2025 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package pool_test + +import ( + "encoding/json" + "fmt" + "testing" + + "github.com/google/uuid" + "google.golang.org/grpc/internal/grpctest" + "google.golang.org/grpc/internal/testutils" + "google.golang.org/grpc/internal/testutils/stats" + "google.golang.org/grpc/internal/xds/bootstrap" + "google.golang.org/grpc/xds/internal/xdsclient" +) + +type s struct { + grpctest.Tester +} + +func Test(t *testing.T) { + grpctest.RunSubTests(t, s{}) +} + +// TestDefaultPool_LazyLoadBootstrapConfig verifies that the DefaultPool +// lazily loads the bootstrap configuration from environment variables when +// an xDS client is created for the first time. +// +// If tries to create the new client in DefaultPool at the start of test when +// none of the env vars are set. This should fail. +// +// Then it sets the env var XDSBootstrapFileName and retry creating a client +// in DefaultPool. This should succeed. +func (s) TestDefaultPool_LazyLoadBootstrapConfig(t *testing.T) { + _, closeFunc, err := xdsclient.DefaultPool.NewClient(t.Name(), &stats.NoopMetricsRecorder{}) + if err == nil { + t.Fatalf("xdsclient.DefaultPool.NewClient() succeeded without setting bootstrap config env vars, want failure") + } + + bs, err := bootstrap.NewContentsForTesting(bootstrap.ConfigOptionsForTesting{ + Servers: []byte(fmt.Sprintf(`[{ + "server_uri": %q, + "channel_creds": [{"type": "insecure"}] + }]`, "non-existent-management-server")), + Node: []byte(fmt.Sprintf(`{"id": "%s"}`, uuid.New().String())), + CertificateProviders: map[string]json.RawMessage{ + "cert-provider-instance": json.RawMessage("{}"), + }, + }) + if err != nil { + t.Fatalf("Failed to create bootstrap configuration: %v", err) + } + + testutils.CreateBootstrapFileForTesting(t, bs) + if cfg := xdsclient.DefaultPool.BootstrapConfigForTesting(); cfg != nil { + t.Fatalf("DefaultPool.BootstrapConfigForTesting() = %v, want nil", cfg) + } + + _, closeFunc, err = xdsclient.DefaultPool.NewClient(t.Name(), &stats.NoopMetricsRecorder{}) + if err != nil { + t.Fatalf("Failed to create xDS client: %v", err) + } + defer func() { + closeFunc() + xdsclient.DefaultPool.UnsetBootstrapConfigForTesting() + }() + + if xdsclient.DefaultPool.BootstrapConfigForTesting() == nil { + t.Fatalf("DefaultPool.BootstrapConfigForTesting() = nil, want non-nil") + } +} diff --git a/xds/server_test.go b/xds/server_test.go index 0b55f10ff..51579333a 100644 --- a/xds/server_test.go +++ b/xds/server_test.go @@ -178,7 +178,7 @@ func (s) TestNewServer_Failure(t *testing.T) { { desc: "bootstrap env var not set", serverOpts: []grpc.ServerOption{grpc.Creds(xdsCreds), BootstrapContentsForTesting(nil)}, - wantErr: "bootstrap configuration not set in the pool", + wantErr: "failed to read xDS bootstrap config from env vars", }, { desc: "empty bootstrap config",