xds: minor cleanup in xdsclient bootstrap code (#5195)

internal/envconfig/xds.go

@@ -26,13 +26,13 @@ import (
 const (
 	// XDSBootstrapFileNameEnv is the env variable to set bootstrap file name.
 	// Do not use this and read from env directly. Its value is read and kept in
-	// variable BootstrapFileName.
+	// variable XDSBootstrapFileName.
 	//
 	// When both bootstrap FileName and FileContent are set, FileName is used.
 	XDSBootstrapFileNameEnv = "GRPC_XDS_BOOTSTRAP"
-	// XDSBootstrapFileContentEnv is the env variable to set bootstrapp file
+	// XDSBootstrapFileContentEnv is the env variable to set bootstrap file
 	// content. Do not use this and read from env directly. Its value is read
-	// and kept in variable BootstrapFileName.
+	// and kept in variable XDSBootstrapFileContent.
 	//
 	// When both bootstrap FileName and FileContent are set, FileName is used.
 	XDSBootstrapFileContentEnv = "GRPC_XDS_BOOTSTRAP_CONFIG"

xds/internal/resolver/xds_resolver.go

@@ -37,13 +37,13 @@ import (
 
 const xdsScheme = "xds"
 
-// NewBuilder creates a new xds resolver builder using a specific xds bootstrap
-// config, so tests can use multiple xds clients in different ClientConns at
-// the same time.
-func NewBuilder(config []byte) (resolver.Builder, error) {
+// NewBuilderForTesting creates a new xds resolver builder using a specific xds
+// bootstrap config, so tests can use multiple xds clients in different
+// ClientConns at the same time.
+func NewBuilderForTesting(config []byte) (resolver.Builder, error) {
 	return &xdsResolverBuilder{
 		newXDSClient: func() (xdsclient.XDSClient, error) {
-			return xdsclient.NewClientWithBootstrapContents(config)
+			return xdsclient.NewWithBootstrapContentsForTesting(config)
 		},
 	}, nil
 }

xds/internal/xdsclient/bootstrap/bootstrap.go

@@ -65,19 +65,20 @@ var gRPCVersion = fmt.Sprintf("%s %s", gRPCUserAgentName, grpc.Version)
 // For overriding in unit tests.
 var bootstrapFileReadFunc = ioutil.ReadFile
 
-// insecureCredsBuilder encapsulates a insecure credential that is built using a
-// JSON config.
+// insecureCredsBuilder implements the `Credentials` interface defined in
+// package `xds/bootstrap` and encapsulates an insecure credential.
 type insecureCredsBuilder struct{}
 
 func (i *insecureCredsBuilder) Build(json.RawMessage) (credentials.Bundle, error) {
 	return insecure.NewBundle(), nil
 }
+
 func (i *insecureCredsBuilder) Name() string {
 	return "insecure"
 }
 
-// googleDefaultCredsBuilder encapsulates a Google Default credential that is built using a
-// JSON config.
+// googleDefaultCredsBuilder implements the `Credentials` interface defined in
+// package `xds/boostrap` and encapsulates a Google Default credential.
 type googleDefaultCredsBuilder struct{}
 
 func (d *googleDefaultCredsBuilder) Build(json.RawMessage) (credentials.Bundle, error) {
@@ -328,11 +329,13 @@ func bootstrapConfigFromEnvVariable() ([]byte, error) {
 }
 
 // NewConfig returns a new instance of Config initialized by reading the
-// bootstrap file found at ${GRPC_XDS_BOOTSTRAP}.
+// bootstrap file found at ${GRPC_XDS_BOOTSTRAP} or bootstrap contents specified
+// at ${GRPC_XDS_BOOTSTRAP_CONFIG}. If both env vars are set, the former is
+// preferred.
 //
-// Currently, we support exactly one type of credential, which is
-// "google_default", where we use the host's default certs for transport
-// credentials and a Google oauth token for call credentials.
+// We support a credential registration mechanism and only credentials
+// registered through that mechanism will be accepted here. See package
+// `xds/bootstrap` for details.
 //
 // This function tries to process as much of the bootstrap file as possible (in
 // the presence of the errors) and may return a Config object with certain
@@ -346,13 +349,18 @@ func NewConfig() (*Config, error) {
 		return nil, fmt.Errorf("xds: Failed to read bootstrap config: %v", err)
 	}
 	logger.Debugf("Bootstrap content: %s", data)
-	return NewConfigFromContents(data)
+	return newConfigFromContents(data)
+}
+
+// NewConfigFromContentsForTesting returns a new Config using the specified
+// bootstrap file contents instead of reading the environment variable.
+//
+// This is only suitable for testing purposes.
+func NewConfigFromContentsForTesting(data []byte) (*Config, error) {
+	return newConfigFromContents(data)
 }
 
-// NewConfigFromContents returns a new Config using the specified bootstrap
-// file contents instead of reading the environment variable.  This is only
-// suitable for testing purposes.
-func NewConfigFromContents(data []byte) (*Config, error) {
+func newConfigFromContents(data []byte) (*Config, error) {
 	config := &Config{}
 
 	var jsonData map[string]json.RawMessage
@@ -483,7 +491,7 @@ func NewConfigFromContents(data []byte) (*Config, error) {
 // file are populated here.
 // 3. For each server config (both top level and in each authority), we set its
 // node field to the v3.Node, or a v2.Node with the same content, depending on
-// the server's transprot API version.
+// the server's transport API version.
 func (c *Config) updateNodeProto(node *v3corepb.Node) error {
 	v3 := node
 	if v3 == nil {
@@ -493,11 +501,11 @@ func (c *Config) updateNodeProto(node *v3corepb.Node) error {
 	v3.UserAgentVersionType = &v3corepb.Node_UserAgentVersion{UserAgentVersion: grpc.Version}
 	v3.ClientFeatures = append(v3.ClientFeatures, clientFeatureNoOverprovisioning)
 
-	v2 := &v2corepb.Node{}
 	v3bytes, err := proto.Marshal(v3)
 	if err != nil {
 		return fmt.Errorf("xds: proto.Marshal(%v): %v", v3, err)
 	}
+	v2 := &v2corepb.Node{}
 	if err := proto.Unmarshal(v3bytes, v2); err != nil {
 		return fmt.Errorf("xds: proto.Unmarshal(%v): %v", v3bytes, err)
 	}

xds/internal/xdsclient/singleton.go

@@ -160,10 +160,10 @@ func (c *clientRefCounted) Close() {
 	}
 }
 
-// NewWithConfigForTesting is exported for testing only.
+// NewWithConfigForTesting returns an xdsClient for the specified bootstrap
+// config, separate from the global singleton.
 //
-// Note that this function doesn't set the singleton, so that the testing states
-// don't leak.
+// This should be used for testing purposes only.
 func NewWithConfigForTesting(config *bootstrap.Config, watchExpiryTimeout time.Duration) (XDSClient, error) {
 	cl, err := newWithConfig(config, watchExpiryTimeout, defaultIdleAuthorityDeleteTimeout)
 	if err != nil {
@@ -172,10 +172,11 @@ func NewWithConfigForTesting(config *bootstrap.Config, watchExpiryTimeout time.D
 	return &clientRefCounted{clientImpl: cl, refCount: 1}, nil
 }
 
-// NewClientWithBootstrapContents returns an xds client for this config,
-// separate from the global singleton.  This should be used for testing
-// purposes only.
-func NewClientWithBootstrapContents(contents []byte) (XDSClient, error) {
+// NewWithBootstrapContentsForTesting returns an xdsClient for this config,
+// separate from the global singleton.
+//
+// This should be used for testing purposes only.
+func NewWithBootstrapContentsForTesting(contents []byte) (XDSClient, error) {
 	// Normalize the contents
 	buf := bytes.Buffer{}
 	err := json.Indent(&buf, contents, "", "")
@@ -198,7 +199,7 @@ func NewClientWithBootstrapContents(contents []byte) (XDSClient, error) {
 		c.mu.Unlock()
 	}
 
-	bcfg, err := bootstrap.NewConfigFromContents(contents)
+	bcfg, err := bootstrap.NewConfigFromContentsForTesting(contents)
 	if err != nil {
 		return nil, fmt.Errorf("xds: error with bootstrap config: %v", err)
 	}

xds/server.go

@@ -161,11 +161,13 @@ func (s *GRPCServer) initXDSClient() error {
 	}
 
 	newXDSClient := newXDSClient
-	if s.opts.bootstrapContents != nil {
+	if s.opts.bootstrapContentsForTesting != nil {
+		// Bootstrap file contents may be specified as a server option for tests.
 		newXDSClient = func() (xdsclient.XDSClient, error) {
-			return xdsclient.NewClientWithBootstrapContents(s.opts.bootstrapContents)
+			return xdsclient.NewWithBootstrapContentsForTesting(s.opts.bootstrapContentsForTesting)
 		}
 	}
+
 	client, err := newXDSClient()
 	if err != nil {
 		return fmt.Errorf("xds: failed to create xds-client: %v", err)

xds/server_options.go

@@ -26,8 +26,8 @@ import (
 )
 
 type serverOptions struct {
-	modeCallback      ServingModeCallbackFunc
-	bootstrapContents []byte
+	modeCallback                ServingModeCallbackFunc
+	bootstrapContentsForTesting []byte
 }
 
 type serverOption struct {
@@ -72,5 +72,5 @@ type ServingModeChangeArgs struct {
 // Notice: This API is EXPERIMENTAL and may be changed or removed in a
 // later release.
 func BootstrapContentsForTesting(contents []byte) grpc.ServerOption {
-	return &serverOption{apply: func(o *serverOptions) { o.bootstrapContents = contents }}
+	return &serverOption{apply: func(o *serverOptions) { o.bootstrapContentsForTesting = contents }}
 }

xds/xds.go

@@ -90,5 +90,5 @@ func init() {
 // Notice: This API is EXPERIMENTAL and may be changed or removed in a
 // later release.
 func NewXDSResolverWithConfigForTesting(bootstrapConfig []byte) (resolver.Builder, error) {
-	return xdsresolver.NewBuilder(bootstrapConfig)
+	return xdsresolver.NewBuilderForTesting(bootstrapConfig)
 }