Skip to content

Commit 49e224f

Browse files
easwarseaswars
authored
xds/googlec2p: Revert PR 8829 that removed SetFallbackBootstrapConfig (#8869)
This API is still used internally in google3. This reverts commit e6ca417. RELEASE NOTES: none
1 parent 61d569d commit 49e224f

File tree

6 files changed

+80
-75
lines changed

6 files changed

+80
-75
lines changed

‎internal/xds/bootstrap/bootstrap.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -658,7 +658,7 @@ func (c *Config) UnmarshalJSON(data []byte) error {
658658
// defaults.
659659
//
660660
// This function returns an error if it's unable to parse the contents of the
661-
// bootstrap config. It returns error if none of the env vars are set.
661+
// bootstrap config. It returns (nil, nil) if none of the env vars are set.
662662
func GetConfiguration() (*Config, error) {
663663
fName := envconfig.XDSBootstrapFileName
664664
fContent := envconfig.XDSBootstrapFileContent
@@ -681,7 +681,7 @@ func GetConfiguration() (*Config, error) {
681681
return NewConfigFromContents([]byte(fContent))
682682
}
683683

684-
return nil, fmt.Errorf("none of the bootstrap environment variables (%q or %q) are set", envconfig.XDSBootstrapFileNameEnv, envconfig.XDSBootstrapFileContentEnv)
684+
return nil, nil
685685
}
686686

687687
// NewConfigFromContents creates a new bootstrap configuration from the provided

‎internal/xds/bootstrap/bootstrap_test.go‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -525,8 +525,9 @@ func (s) TestGetConfiguration_Failure(t *testing.T) {
525525
const name = "empty"
526526
t.Run(name, func(t *testing.T) {
527527
testGetConfigurationWithFileNameEnv(t, name, true, nil)
528-
// If both the env vars are empty, an error must be returned.
529-
testGetConfigurationWithFileContentEnv(t, name, true, nil)
528+
// If both the env vars are empty, a nil config with a nil error must be
529+
// returned.
530+
testGetConfigurationWithFileContentEnv(t, name, false, nil)
530531
})
531532
}
532533

@@ -664,9 +665,9 @@ func (s) TestGetConfiguration_BootstrapEnvPriority(t *testing.T) {
664665
envconfig.XDSBootstrapFileContent = ""
665666
defer func() { envconfig.XDSBootstrapFileContent = origBootstrapContent }()
666667

667-
// When both env variables are empty, GetConfiguration should return error.
668-
if _, err := GetConfiguration(); err == nil {
669-
t.Errorf("GetConfiguration() returned nil, want error")
668+
// When both env variables are empty, GetConfiguration should return nil.
669+
if cfg, err := GetConfiguration(); err != nil || cfg != nil {
670+
t.Errorf("GetConfiguration() returned (%v, %v), want (<nil>, <nil>)", cfg, err)
670671
}
671672

672673
// When one of them is set, it should be used.

‎internal/xds/xdsclient/pool.go‎

Lines changed: 55 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525

2626
v3statuspb "github.com/envoyproxy/go-control-plane/envoy/service/status/v3"
2727
estats "google.golang.org/grpc/experimental/stats"
28+
"google.golang.org/grpc/internal/envconfig"
2829
istats "google.golang.org/grpc/internal/stats"
2930
"google.golang.org/grpc/internal/xds/bootstrap"
3031
"google.golang.org/protobuf/proto"
@@ -43,12 +44,14 @@ var (
4344
// Pool represents a pool of xDS clients that share the same bootstrap
4445
// configuration.
4546
type Pool struct {
46-
mu sync.Mutex
47-
clients map[string]*clientImpl
47+
// Note that mu should ideally only have to guard clients. But here, we need
48+
// it to guard config as well since SetFallbackBootstrapConfig writes to
49+
// config.
50+
mu sync.Mutex
51+
clients map[string]*clientImpl
52+
fallbackConfig *bootstrap.Config // TODO(i/8661): remove fallbackConfig.
4853
// getConfiguration is a sync.OnceValues that attempts to read the bootstrap
49-
// configuration from environment variables once if bootstrap.GetConfiguration
50-
// is set in the case of DefaultPool. When the Pool is created with a
51-
// specific configuration, it returns that configuration directly.
54+
// configuration from environment variables once.
5255
getConfiguration func() (*bootstrap.Config, error)
5356
}
5457

@@ -79,82 +82,46 @@ type OptionsForTesting struct {
7982

8083
// NewPool creates a new xDS client pool with the given bootstrap config.
8184
//
82-
// If a nil bootstrap config is passed, the caller is expected to pass the
83-
// config when creating the xDS client by using NewClientWithConfig instead of
84-
// NewClient. If they specify a nil config and use NewClient, client creation
85-
// will fail.
85+
// If a nil bootstrap config is passed and SetFallbackBootstrapConfig is not
86+
// called before a call to NewClient, the latter will fail. i.e. if there is an
87+
// attempt to create an xDS client from the pool without specifying bootstrap
88+
// configuration (either at pool creation time or by setting the fallback
89+
// bootstrap configuration), xDS client creation will fail.
8690
func NewPool(config *bootstrap.Config) *Pool {
8791
return &Pool{
8892
clients: make(map[string]*clientImpl),
8993
getConfiguration: func() (*bootstrap.Config, error) {
90-
if config == nil {
91-
return nil, fmt.Errorf("xds: bootstrap config cannot be nil")
92-
}
9394
return config, nil
9495
},
9596
}
9697
}
9798

98-
// getBootstrapConfiguration returns the config specified at pool creation time.
99-
func (p *Pool) getBootstrapConfiguration() (*bootstrap.Config, error) {
100-
p.mu.Lock()
101-
defer p.mu.Unlock()
102-
config, err := p.getConfiguration()
103-
if err != nil {
104-
return nil, fmt.Errorf("xds: failed to read xDS bootstrap config: %v", err)
105-
}
106-
return config, nil
107-
}
108-
109-
// getConfig returns the provided config if it is non-nil. Otherwise, it
110-
// delegates to getBootstrapConfiguration to retrieve the configuration.
111-
func (p *Pool) getConfig(config *bootstrap.Config) (*bootstrap.Config, error) {
112-
if config != nil {
113-
return config, nil
114-
}
115-
return p.getBootstrapConfiguration()
116-
}
117-
118-
// NewClientWithConfig returns an xDS client with the given name from the pool.
119-
// If the client doesn't already exist, it creates a new xDS client using the
120-
// provided config and adds it to the pool. The provided config takes precedence
121-
// over any config passed during pool creation. This should be used when a
122-
// non-default config is required. If the provided config is nil, it attempts
123-
// to load the configuration provided at pool creation time.
99+
// NewClientWithConfig returns an xDS client with the given name from the pool. If the
100+
// client doesn't already exist, it creates a new xDS client and adds it to the
101+
// pool.
124102
//
125103
// The second return value represents a close function which the caller is
126104
// expected to invoke once they are done using the client. It is safe for the
127105
// caller to invoke this close function multiple times.
128106
func (p *Pool) NewClientWithConfig(name string, metricsRecorder estats.MetricsRecorder, config *bootstrap.Config) (XDSClient, func(), error) {
129-
config, err := p.getConfig(config)
130-
if err != nil {
131-
return nil, nil, err
132-
}
133107
return p.newRefCounted(name, metricsRecorder, defaultWatchExpiryTimeout, config)
134108
}
135109

136110
// NewClient returns an xDS client with the given name from the pool. If the
137-
// client doesn't already exist, it loads the configuration and creates a new
138-
// xDS client and adds it to the pool.
111+
// client doesn't already exist, it creates a new xDS client and adds it to the
112+
// pool.
139113
//
140114
// The second return value represents a close function which the caller is
141115
// expected to invoke once they are done using the client. It is safe for the
142116
// caller to invoke this close function multiple times.
143117
func (p *Pool) NewClient(name string, metricsRecorder estats.MetricsRecorder) (XDSClient, func(), error) {
144-
config, err := p.getConfig(nil)
145-
if err != nil {
146-
return nil, nil, err
147-
}
148-
return p.newRefCounted(name, metricsRecorder, defaultWatchExpiryTimeout, config)
118+
return p.newRefCounted(name, metricsRecorder, defaultWatchExpiryTimeout, nil)
149119
}
150120

151121
// NewClientForTesting returns an xDS client configured with the provided
152122
// options from the pool. If the client doesn't already exist, it creates a new
153123
// xDS client and adds it to the pool.
154124
//
155-
// If the config in options is nil, it attempts to load the configuration
156-
// provided at the time of pool creation.
157-
//
158125
// The second return value represents a close function which the caller is
159126
// expected to invoke once they are done using the client. It is safe for the
160127
// caller to invoke this close function multiple times.
@@ -175,13 +142,7 @@ func (p *Pool) NewClientForTesting(opts OptionsForTesting) (XDSClient, func(), e
175142
if opts.MetricsRecorder == nil {
176143
opts.MetricsRecorder = istats.NewMetricsRecorderList(nil)
177144
}
178-
179-
config, err := p.getConfig(opts.Config)
180-
if err != nil {
181-
return nil, nil, err
182-
}
183-
184-
c, cancel, err := p.newRefCounted(opts.Name, opts.MetricsRecorder, opts.WatchExpiryTimeout, config)
145+
c, cancel, err := p.newRefCounted(opts.Name, opts.MetricsRecorder, opts.WatchExpiryTimeout, opts.Config)
185146
if err != nil {
186147
return nil, nil, err
187148
}
@@ -211,6 +172,16 @@ func (p *Pool) GetClientForTesting(name string) (XDSClient, func(), error) {
211172
return c, sync.OnceFunc(func() { p.clientRefCountedClose(name) }), nil
212173
}
213174

175+
// SetFallbackBootstrapConfig is used to specify a bootstrap configuration
176+
// that will be used as a fallback when the bootstrap environment variables
177+
// are not defined.
178+
// TODO(i/8661): remove SetFallbackBootstrapConfig function.
179+
func (p *Pool) SetFallbackBootstrapConfig(config *bootstrap.Config) {
180+
p.mu.Lock()
181+
defer p.mu.Unlock()
182+
p.fallbackConfig = config
183+
}
184+
214185
// DumpResources returns the status and contents of all xDS resources.
215186
func (p *Pool) DumpResources() *v3statuspb.ClientStatusResponse {
216187
p.mu.Lock()
@@ -241,7 +212,10 @@ func (p *Pool) BootstrapConfigForTesting() *bootstrap.Config {
241212
p.mu.Lock()
242213
defer p.mu.Unlock()
243214
cfg, _ := p.getConfiguration()
244-
return cfg
215+
if cfg != nil {
216+
return cfg
217+
}
218+
return p.fallbackConfig
245219
}
246220

247221
// UnsetBootstrapConfigForTesting unsets the bootstrap configuration used by
@@ -251,6 +225,7 @@ func (p *Pool) BootstrapConfigForTesting() *bootstrap.Config {
251225
func (p *Pool) UnsetBootstrapConfigForTesting() {
252226
p.mu.Lock()
253227
defer p.mu.Unlock()
228+
p.fallbackConfig = nil
254229
p.getConfiguration = sync.OnceValues(bootstrap.GetConfiguration)
255230
}
256231

@@ -293,9 +268,7 @@ func (p *Pool) clientRefCountedClose(name string) {
293268
// newRefCounted creates a new reference counted xDS client implementation for
294269
// name, if one does not exist already. If an xDS client for the given name
295270
// exists, it gets a reference to it and returns it.
296-
//
297-
// The config should not be nil.
298-
func (p *Pool) newRefCounted(name string, metricsRecorder estats.MetricsRecorder, watchExpiryTimeout time.Duration, config *bootstrap.Config) (*clientImpl, func(), error) {
271+
func (p *Pool) newRefCounted(name string, metricsRecorder estats.MetricsRecorder, watchExpiryTimeout time.Duration, bConfig *bootstrap.Config) (*clientImpl, func(), error) {
299272
p.mu.Lock()
300273
defer p.mu.Unlock()
301274

@@ -304,6 +277,25 @@ func (p *Pool) newRefCounted(name string, metricsRecorder estats.MetricsRecorder
304277
return c, sync.OnceFunc(func() { p.clientRefCountedClose(name) }), nil
305278
}
306279

280+
config := bConfig
281+
if config == nil {
282+
var err error
283+
config, err = p.getConfiguration()
284+
if err != nil {
285+
return nil, nil, fmt.Errorf("xds: failed to read xDS bootstrap config from env vars: %v", err)
286+
}
287+
if config == nil {
288+
// If the environment variables are not set, then fallback bootstrap
289+
// configuration should be set before attempting to create an xDS client,
290+
// else xDS client creation will fail.
291+
config = p.fallbackConfig
292+
}
293+
}
294+
295+
if config == nil {
296+
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)
297+
}
298+
307299
c, err := newClientImpl(config, metricsRecorder, name, watchExpiryTimeout)
308300
if err != nil {
309301
return nil, nil, err

‎internal/xds/xdsclient/pool/pool_ext_test.go‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -185,8 +185,11 @@ func (s) TestNestedXDSChannel(t *testing.T) {
185185
if err != nil {
186186
t.Fatalf("Failed to create bootstrap configuration: %v", err)
187187
}
188-
189-
testutils.CreateBootstrapFileForTesting(t, bootstrapContents)
188+
config, err := bootstrap.NewConfigFromContents(bootstrapContents)
189+
if err != nil {
190+
t.Fatalf("Failed to parse bootstrap contents: %v", err)
191+
}
192+
xdsclient.DefaultPool.SetFallbackBootstrapConfig(config)
190193
defer func() { xdsclient.DefaultPool.UnsetBootstrapConfigForTesting() }()
191194

192195
// Update the management server that holds resources for resolving the real

‎xds/csds/csds_e2e_test.go‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import (
3535
"google.golang.org/grpc/internal/pretty"
3636
"google.golang.org/grpc/internal/testutils"
3737
"google.golang.org/grpc/internal/testutils/xds/e2e"
38+
"google.golang.org/grpc/internal/xds/bootstrap"
3839
"google.golang.org/grpc/internal/xds/xdsclient"
3940
"google.golang.org/grpc/internal/xds/xdsclient/xdsresource"
4041
"google.golang.org/grpc/xds/csds"
@@ -220,10 +221,14 @@ func (s) TestCSDS(t *testing.T) {
220221
// Create a bootstrap contents pointing to the above management server.
221222
nodeID := uuid.New().String()
222223
bootstrapContents := e2e.DefaultBootstrapContents(t, nodeID, mgmtServer.Address)
224+
config, err := bootstrap.NewConfigFromContents(bootstrapContents)
225+
if err != nil {
226+
t.Fatalf("Failed to parse bootstrap contents: %s, %v", string(bootstrapContents), err)
227+
}
223228
// We use the default xDS client pool here because the CSDS service reports
224229
// on the state of the default xDS client which is implicitly managed
225230
// within the xdsclient.DefaultPool.
226-
testutils.CreateBootstrapFileForTesting(t, bootstrapContents)
231+
xdsclient.DefaultPool.SetFallbackBootstrapConfig(config)
227232
defer func() { xdsclient.DefaultPool.UnsetBootstrapConfigForTesting() }()
228233
// Create two xDS clients, with different names. These should end up
229234
// creating two different xDS clients.
@@ -419,10 +424,14 @@ func (s) TestCSDS_NACK(t *testing.T) {
419424
// Create a bootstrap contents pointing to the above management server.
420425
nodeID := uuid.New().String()
421426
bootstrapContents := e2e.DefaultBootstrapContents(t, nodeID, mgmtServer.Address)
427+
config, err := bootstrap.NewConfigFromContents(bootstrapContents)
428+
if err != nil {
429+
t.Fatalf("Failed to parse bootstrap contents: %s, %v", string(bootstrapContents), err)
430+
}
422431
// We use the default xDS client pool here because the CSDS service reports
423432
// on the state of the default xDS client which is implicitly managed
424433
// within the xdsclient.DefaultPool.
425-
testutils.CreateBootstrapFileForTesting(t, bootstrapContents)
434+
xdsclient.DefaultPool.SetFallbackBootstrapConfig(config)
426435
defer func() { xdsclient.DefaultPool.UnsetBootstrapConfigForTesting() }()
427436
// Create two xDS clients, with different names. These should end up
428437
// creating two different xDS clients.

‎xds/server_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ func (s) TestNewServer_Failure(t *testing.T) {
178178
{
179179
desc: "bootstrap env var not set",
180180
serverOpts: []grpc.ServerOption{grpc.Creds(xdsCreds), BootstrapContentsForTesting(nil)},
181-
wantErr: "failed to read xDS bootstrap config",
181+
wantErr: "failed to read xDS bootstrap config from env vars",
182182
},
183183
{
184184
desc: "empty bootstrap config",

0 commit comments

Comments
 (0)