xds: propogate bootstrap error to grpc.Dial (#3330)

xds/internal/balancer/edsbalancer/eds_test.go

@@ -47,12 +47,12 @@ import (
 func init() {
 	balancer.Register(&edsBalancerBuilder{})
 
-	bootstrapConfigNew = func() *bootstrap.Config {
+	bootstrapConfigNew = func() (*bootstrap.Config, error) {
 		return &bootstrap.Config{
-			BalancerName: "",
+			BalancerName: testBalancerNameFooBar,
 			Creds:        grpc.WithInsecure(),
 			NodeProto:    &corepb.Node{},
-		}
+		}, nil
 	}
 }
 
@@ -215,6 +215,15 @@ func setup(edsLBCh *testutils.Channel, xdsClientCh *testutils.Channel) func() {
 //   balancerName in the lbConfig. We expect xdsClient objects to created
 //   whenever the balancerName changes.
 func (s) TestXDSConfigBalancerNameUpdate(t *testing.T) {
+	oldBootstrapConfigNew := bootstrapConfigNew
+	bootstrapConfigNew = func() (*bootstrap.Config, error) {
+		// Return an error from bootstrap, so the eds balancer will use
+		// BalancerName from the config.
+		//
+		// TODO: remove this when deleting BalancerName from config.
+		return nil, fmt.Errorf("no bootstrap available")
+	}
+	defer func() { bootstrapConfigNew = oldBootstrapConfigNew }()
 	edsLBCh := testutils.NewChannel()
 	xdsClientCh := testutils.NewChannel()
 	cancel := setup(edsLBCh, xdsClientCh)

xds/internal/balancer/edsbalancer/xds_client_wrapper.go

@@ -131,9 +131,10 @@ func (c *xdsclientWrapper) updateXDSClient(config *EDSConfig, attr *attributes.A
 		}
 	}
 
-	clientConfig := bootstrapConfigNew()
-	if clientConfig.BalancerName == "" {
-		clientConfig.BalancerName = config.BalancerName
+	clientConfig, err := bootstrapConfigNew()
+	if err != nil {
+		// TODO: propagate this error to ClientConn, and fail RPCs if necessary.
+		clientConfig = &bootstrap.Config{BalancerName: config.BalancerName}
 	}
 
 	if c.balancerName == clientConfig.BalancerName {

xds/internal/balancer/edsbalancer/xds_client_wrapper_test.go

@@ -26,11 +26,13 @@ import (
 	corepb "github.com/envoyproxy/go-control-plane/envoy/api/v2/core"
 	"github.com/golang/protobuf/proto"
 	"github.com/google/go-cmp/cmp"
+	"google.golang.org/grpc"
 	"google.golang.org/grpc/attributes"
 	"google.golang.org/grpc/balancer"
 	"google.golang.org/grpc/resolver"
 	xdsinternal "google.golang.org/grpc/xds/internal"
 	xdsclient "google.golang.org/grpc/xds/internal/client"
+	"google.golang.org/grpc/xds/internal/client/bootstrap"
 	"google.golang.org/grpc/xds/internal/testutils"
 	"google.golang.org/grpc/xds/internal/testutils/fakeclient"
 	"google.golang.org/grpc/xds/internal/testutils/fakeserver"
@@ -93,6 +95,15 @@ func (s) TestClientWrapperWatchEDS(t *testing.T) {
 		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
+			oldBootstrapConfigNew := bootstrapConfigNew
+			bootstrapConfigNew = func() (*bootstrap.Config, error) {
+				return &bootstrap.Config{
+					BalancerName: fakeServer.Address,
+					Creds:        grpc.WithInsecure(),
+					NodeProto:    &corepb.Node{},
+				}, nil
+			}
+			defer func() { bootstrapConfigNew = oldBootstrapConfigNew }()
 			cw.handleUpdate(&EDSConfig{
 				BalancerName:   fakeServer.Address,
 				EDSServiceName: test.edsServiceName,

xds/internal/client/bootstrap/bootstrap.go

@@ -98,26 +98,23 @@ type xdsServer struct {
 // 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.
-func NewConfig() *Config {
+func NewConfig() (*Config, error) {
 	config := &Config{}
 
 	fName, ok := os.LookupEnv(fileEnv)
 	if !ok {
-		grpclog.Errorf("xds: %s environment variable not set", fileEnv)
-		return config
+		return nil, fmt.Errorf("xds: %s environment variable not set", fileEnv)
 	}
 
 	grpclog.Infof("xds: Reading bootstrap file from %s", fName)
 	data, err := fileReadFunc(fName)
 	if err != nil {
-		grpclog.Errorf("xds: bootstrap file {%v} read failed: %v", fName, err)
-		return config
+		return nil, fmt.Errorf("xds: bootstrap file {%v} read failed: %v", fName, err)
 	}
 
 	var jsonData map[string]json.RawMessage
 	if err := json.Unmarshal(data, &jsonData); err != nil {
-		grpclog.Errorf("xds: json.Unmarshal(%v) failed during bootstrap: %v", string(data), err)
-		return config
+		return nil, fmt.Errorf("xds: json.Unmarshal(%v) failed during bootstrap: %v", string(data), err)
 	}
 
 	m := jsonpb.Unmarshaler{AllowUnknownFields: true}
@@ -126,19 +123,16 @@ func NewConfig() *Config {
 		case "node":
 			n := &corepb.Node{}
 			if err := m.Unmarshal(bytes.NewReader(v), n); err != nil {
-				grpclog.Errorf("xds: jsonpb.Unmarshal(%v) for field %q failed during bootstrap: %v", string(v), k, err)
-				break
+				return nil, fmt.Errorf("xds: jsonpb.Unmarshal(%v) for field %q failed during bootstrap: %v", string(v), k, err)
 			}
 			config.NodeProto = n
 		case "xds_servers":
 			var servers []*xdsServer
 			if err := json.Unmarshal(v, &servers); err != nil {
-				grpclog.Errorf("xds: json.Unmarshal(%v) for field %q failed during bootstrap: %v", string(v), k, err)
-				break
+				return nil, fmt.Errorf("xds: json.Unmarshal(%v) for field %q failed during bootstrap: %v", string(v), k, err)
 			}
 			if len(servers) < 1 {
-				grpclog.Errorf("xds: bootstrap file parsing failed during bootstrap: file doesn't contain any xds server to connect to")
-				break
+				return nil, fmt.Errorf("xds: bootstrap file parsing failed during bootstrap: file doesn't contain any xds server to connect to")
 			}
 			xs := servers[0]
 			config.BalancerName = xs.ServerURI
@@ -155,6 +149,10 @@ func NewConfig() *Config {
 		}
 	}
 
+	if config.BalancerName == "" {
+		return nil, fmt.Errorf("xds: xds_server name is expected, but not found in bootstrap file")
+	}
+
 	// If we don't find a nodeProto in the bootstrap file, we just create an
 	// empty one here. That way, callers of this function can always expect
 	// that the NodeProto field is non-nil.
@@ -164,5 +162,5 @@ func NewConfig() *Config {
 	config.NodeProto.BuildVersion = gRPCVersion
 
 	grpclog.Infof("xds: bootstrap.NewConfig returning: %+v", config)
-	return config
+	return config, nil
 }

xds/internal/client/bootstrap/bootstrap_test.go

@@ -60,8 +60,9 @@ var (
 // file.
 func TestNewConfig(t *testing.T) {
 	bootstrapFileMap := map[string]string{
-		"empty":   "",
-		"badJSON": `["test": 123]`,
+		"empty":          "",
+		"badJSON":        `["test": 123]`,
+		"noBalancerName": `{"node": {"id": "ENVOY_NODE_ID"}}`,
 		"emptyNodeProto": `
 		{
 			"xds_servers" : [{
@@ -101,7 +102,10 @@ func TestNewConfig(t *testing.T) {
 				"metadata": {
 				    "TRAFFICDIRECTOR_GRPC_HOSTNAME": "trafficdirector"
 			    }
-			}
+			},
+			"xds_servers" : [{
+				"server_uri": "trafficdirector.googleapis.com:443"
+			}]
 		}`,
 		"unknownFieldInXdsServer": `
 		{
@@ -213,23 +217,25 @@ func TestNewConfig(t *testing.T) {
 	tests := []struct {
 		name       string
 		wantConfig *Config
+		wantError  bool
 	}{
-		{"nonExistentBootstrapFile", &Config{}},
-		{"empty", &Config{}},
-		{"badJSON", &Config{}},
+		{"nonExistentBootstrapFile", nil, true},
+		{"empty", nil, true},
+		{"badJSON", nil, true},
 		{"emptyNodeProto", &Config{
 			BalancerName: "trafficdirector.googleapis.com:443",
 			NodeProto:    &corepb.Node{BuildVersion: gRPCVersion},
-		}},
-		{"emptyXdsServer", &Config{NodeProto: nodeProto}},
-		{"unknownTopLevelFieldInFile", nilCredsConfig},
-		{"unknownFieldInNodeProto", &Config{NodeProto: nodeProto}},
-		{"unknownFieldInXdsServer", nilCredsConfig},
-		{"emptyChannelCreds", nilCredsConfig},
-		{"nonGoogleDefaultCreds", nilCredsConfig},
-		{"multipleChannelCreds", nonNilCredsConfig},
-		{"goodBootstrap", nonNilCredsConfig},
-		{"multipleXDSServers", nonNilCredsConfig},
+		}, false},
+		{"noBalancerName", nil, true},
+		{"emptyXdsServer", nil, true},
+		{"unknownTopLevelFieldInFile", nilCredsConfig, false},
+		{"unknownFieldInNodeProto", nilCredsConfig, false},
+		{"unknownFieldInXdsServer", nilCredsConfig, false},
+		{"emptyChannelCreds", nilCredsConfig, false},
+		{"nonGoogleDefaultCreds", nilCredsConfig, false},
+		{"multipleChannelCreds", nonNilCredsConfig, false},
+		{"goodBootstrap", nonNilCredsConfig, false},
+		{"multipleXDSServers", nonNilCredsConfig, false},
 	}
 
 	for _, test := range tests {
@@ -237,7 +243,16 @@ func TestNewConfig(t *testing.T) {
 			if err := os.Setenv(fileEnv, test.name); err != nil {
 				t.Fatalf("os.Setenv(%s, %s) failed with error: %v", fileEnv, test.name, err)
 			}
-			config := NewConfig()
+			config, err := NewConfig()
+			if err != nil {
+				if !test.wantError {
+					t.Fatalf("unexpected error %v", err)
+				}
+				return
+			}
+			if test.wantError {
+				t.Fatalf("wantError: %v, got error %v", test.wantError, err)
+			}
 			if config.BalancerName != test.wantConfig.BalancerName {
 				t.Errorf("config.BalancerName is %s, want %s", config.BalancerName, test.wantConfig.BalancerName)
 			}
@@ -253,8 +268,8 @@ func TestNewConfig(t *testing.T) {
 
 func TestNewConfigEnvNotSet(t *testing.T) {
 	os.Unsetenv(fileEnv)
-	wantConfig := Config{}
-	if config := NewConfig(); *config != wantConfig {
-		t.Errorf("NewConfig() returned : %#v, wanted an empty Config object", config)
+	config, err := NewConfig()
+	if err == nil {
+		t.Errorf("NewConfig() returned: %#v, <nil>, wanted non-nil error", config)
 	}
 }

xds/internal/resolver/xds_resolver.go

@@ -21,7 +21,6 @@ package resolver
 
 import (
 	"context"
-	"errors"
 	"fmt"
 
 	"google.golang.org/grpc"
@@ -57,9 +56,9 @@ type xdsResolverBuilder struct{}
 // The xds bootstrap process is performed (and a new xds client is built) every
 // time an xds resolver is built.
 func (b *xdsResolverBuilder) Build(t resolver.Target, cc resolver.ClientConn, rbo resolver.BuildOptions) (resolver.Resolver, error) {
-	config := newXDSConfig()
-	if config.BalancerName == "" {
-		return nil, errors.New("xds: balancerName not found in bootstrap file")
+	config, err := newXDSConfig()
+	if err != nil {
+		return nil, fmt.Errorf("xds: failed to read bootstrap file: %v", err)
 	}
 	if config.Creds == nil {
 		// TODO: Once we start supporting a mechanism to register credential

xds/internal/resolver/xds_resolver_test.go

@@ -172,7 +172,12 @@ func TestResolverBuilder(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			// Fake out the bootstrap process by providing our own config.
 			oldConfigMaker := newXDSConfig
-			newXDSConfig = func() *bootstrap.Config { return &test.config }
+			newXDSConfig = func() (*bootstrap.Config, error) {
+				if test.config.BalancerName == "" {
+					return nil, fmt.Errorf("no balancer name found in config")
+				}
+				return &test.config, nil
+			}
 			// Fake out the xdsClient creation process by providing a fake.
 			oldClientMaker := newXDSClient
 			newXDSClient = test.xdsClientFunc
@@ -208,7 +213,7 @@ func testSetup(t *testing.T, opts setupOpts) (*xdsResolver, *testClientConn, fun
 	t.Helper()
 
 	oldConfigMaker := newXDSConfig
-	newXDSConfig = func() *bootstrap.Config { return opts.config }
+	newXDSConfig = func() (*bootstrap.Config, error) { return opts.config, nil }
 	oldClientMaker := newXDSClient
 	newXDSClient = opts.xdsClientFunc
 	cancel := func() {