From 1f66bc9efb148461b7b9cf4403532994c69b9b3d Mon Sep 17 00:00:00 2001 From: Menghan Li Date: Thu, 23 Jan 2020 12:45:58 -0800 Subject: [PATCH] xds: propogate bootstrap error to grpc.Dial (#3330) --- xds/internal/balancer/edsbalancer/eds_test.go | 15 ++++- .../edsbalancer/xds_client_wrapper.go | 7 ++- .../edsbalancer/xds_client_wrapper_test.go | 11 ++++ xds/internal/client/bootstrap/bootstrap.go | 26 ++++----- .../client/bootstrap/bootstrap_test.go | 55 ++++++++++++------- xds/internal/resolver/xds_resolver.go | 7 +-- xds/internal/resolver/xds_resolver_test.go | 9 ++- 7 files changed, 84 insertions(+), 46 deletions(-) diff --git a/xds/internal/balancer/edsbalancer/eds_test.go b/xds/internal/balancer/edsbalancer/eds_test.go index 138630d7893..1f47c517c81 100644 --- a/xds/internal/balancer/edsbalancer/eds_test.go +++ b/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) diff --git a/xds/internal/balancer/edsbalancer/xds_client_wrapper.go b/xds/internal/balancer/edsbalancer/xds_client_wrapper.go index 891c318865e..b32fb97f59a 100644 --- a/xds/internal/balancer/edsbalancer/xds_client_wrapper.go +++ b/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 { diff --git a/xds/internal/balancer/edsbalancer/xds_client_wrapper_test.go b/xds/internal/balancer/edsbalancer/xds_client_wrapper_test.go index c7cd9ccf7e5..b492afec6e6 100644 --- a/xds/internal/balancer/edsbalancer/xds_client_wrapper_test.go +++ b/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, diff --git a/xds/internal/client/bootstrap/bootstrap.go b/xds/internal/client/bootstrap/bootstrap.go index cc2150cccf7..deb2c6c367e 100644 --- a/xds/internal/client/bootstrap/bootstrap.go +++ b/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 } diff --git a/xds/internal/client/bootstrap/bootstrap_test.go b/xds/internal/client/bootstrap/bootstrap_test.go index a55e1d0f7de..91620062cd2 100644 --- a/xds/internal/client/bootstrap/bootstrap_test.go +++ b/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, , wanted non-nil error", config) } } diff --git a/xds/internal/resolver/xds_resolver.go b/xds/internal/resolver/xds_resolver.go index 2d0ee4f89ff..d961918e4c3 100644 --- a/xds/internal/resolver/xds_resolver.go +++ b/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 diff --git a/xds/internal/resolver/xds_resolver_test.go b/xds/internal/resolver/xds_resolver_test.go index c471b80493a..62c4576f9c4 100644 --- a/xds/internal/resolver/xds_resolver_test.go +++ b/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() {