From 29deb6bfa173768ba53b14c0b5c8af13854eb3a2 Mon Sep 17 00:00:00 2001 From: Menghan Li Date: Tue, 2 Nov 2021 10:26:08 -0700 Subject: [PATCH] xds/bootstrap: refactor to support top level and per-authority server config (#4892) --- xds/csds/csds.go | 2 +- xds/googledirectpath/googlec2p.go | 10 +- xds/googledirectpath/googlec2p_test.go | 13 +- xds/internal/xdsclient/bootstrap/bootstrap.go | 321 ++++++++++++------ .../xdsclient/bootstrap/bootstrap_test.go | 125 ++++--- xds/internal/xdsclient/client.go | 34 +- xds/internal/xdsclient/client_test.go | 16 +- xds/internal/xdsclient/dump_test.go | 32 +- xds/internal/xdsclient/loadreport.go | 4 +- xds/internal/xdsclient/loadreport_test.go | 10 +- xds/internal/xdsclient/xdsclient_test.go | 36 +- xds/server_test.go | 32 +- 12 files changed, 384 insertions(+), 251 deletions(-) diff --git a/xds/csds/csds.go b/xds/csds/csds.go index 1d817fbcc865..23f9c760b637 100644 --- a/xds/csds/csds.go +++ b/xds/csds/csds.go @@ -127,7 +127,7 @@ func (s *ClientStatusDiscoveryServer) buildClientStatusRespForReq(req *v3statusp ret := &v3statuspb.ClientStatusResponse{ Config: []*v3statuspb.ClientConfig{ { - Node: nodeProtoToV3(s.xdsClient.BootstrapConfig().NodeProto), + Node: nodeProtoToV3(s.xdsClient.BootstrapConfig().XDSServer.NodeProto), GenericXdsConfigs: configs, }, }, diff --git a/xds/googledirectpath/googlec2p.go b/xds/googledirectpath/googlec2p.go index b9f1c712014e..4f753e49c71a 100644 --- a/xds/googledirectpath/googlec2p.go +++ b/xds/googledirectpath/googlec2p.go @@ -103,10 +103,12 @@ func (c2pResolverBuilder) Build(t resolver.Target, cc resolver.ClientConn, opts balancerName = tdURL } config := &bootstrap.Config{ - BalancerName: balancerName, - Creds: grpc.WithCredentialsBundle(google.NewDefaultCredentials()), - TransportAPI: version.TransportV3, - NodeProto: newNode(<-zoneCh, <-ipv6CapableCh), + XDSServer: &bootstrap.ServerConfig{ + ServerURI: balancerName, + Creds: grpc.WithCredentialsBundle(google.NewDefaultCredentials()), + TransportAPI: version.TransportV3, + NodeProto: newNode(<-zoneCh, <-ipv6CapableCh), + }, } // Create singleton xds client with this config. The xds client will be diff --git a/xds/googledirectpath/googlec2p_test.go b/xds/googledirectpath/googlec2p_test.go index a208fad66c58..c8162317cc30 100644 --- a/xds/googledirectpath/googlec2p_test.go +++ b/xds/googledirectpath/googlec2p_test.go @@ -211,15 +211,18 @@ func TestBuildXDS(t *testing.T) { } } wantConfig := &bootstrap.Config{ - BalancerName: tdURL, - TransportAPI: version.TransportV3, - NodeProto: wantNode, + XDSServer: &bootstrap.ServerConfig{ + ServerURI: tdURL, + TransportAPI: version.TransportV3, + NodeProto: wantNode, + }, } if tt.tdURI != "" { - wantConfig.BalancerName = tt.tdURI + wantConfig.XDSServer.ServerURI = tt.tdURI } cmpOpts := cmp.Options{ - cmpopts.IgnoreFields(bootstrap.Config{}, "Creds"), + cmpopts.IgnoreFields(bootstrap.ServerConfig{}, "Creds"), + cmp.AllowUnexported(bootstrap.ServerConfig{}), protocmp.Transform(), } select { diff --git a/xds/internal/xdsclient/bootstrap/bootstrap.go b/xds/internal/xdsclient/bootstrap/bootstrap.go index fa229d99593e..7f75525cc631 100644 --- a/xds/internal/xdsclient/bootstrap/bootstrap.go +++ b/xds/internal/xdsclient/bootstrap/bootstrap.go @@ -25,6 +25,7 @@ import ( "encoding/json" "fmt" "io/ioutil" + "strings" v2corepb "github.com/envoyproxy/go-control-plane/envoy/api/v2/core" v3corepb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" @@ -58,34 +59,164 @@ var gRPCVersion = fmt.Sprintf("%s %s", gRPCUserAgentName, grpc.Version) // For overriding in unit tests. var bootstrapFileReadFunc = ioutil.ReadFile -// Config provides the xDS client with several key bits of information that it -// requires in its interaction with the management server. The Config is -// initialized from the bootstrap file. -type Config struct { - // BalancerName is the name of the management server to connect to. +// ServerConfig contains the configuration to connect to a server, including +// URI, creds, and transport API version (e.g. v2 or v3). +type ServerConfig struct { + // ServerURI is the management server to connect to. // - // The bootstrap file contains a list of servers (with name+creds), but we - // pick the first one. - BalancerName string + // The bootstrap file contains an ordered list of xDS servers to contact for + // this authority. The first one is picked. + ServerURI string // Creds contains the credentials to be used while talking to the xDS // server, as a grpc.DialOption. Creds grpc.DialOption + // credsType is the type of the creds. It will be used to dedup servers. + credsType string // TransportAPI indicates the API version of xDS transport protocol to use. // This describes the xDS gRPC endpoint and version of // DiscoveryRequest/Response used on the wire. TransportAPI version.TransportAPI // NodeProto contains the Node proto to be used in xDS requests. The actual // type depends on the transport protocol version used. + // + // Note that it's specified in the bootstrap globally for all the servers, + // but we keep it in each server config so that its type (e.g. *v2pb.Node or + // *v3pb.Node) is consistent with the transport API version. NodeProto proto.Message +} + +// UnmarshalJSON takes the json data (a list of servers) and unmarshals the +// first one in the list. +func (sc *ServerConfig) UnmarshalJSON(data []byte) error { + var servers []*xdsServer + if err := json.Unmarshal(data, &servers); err != nil { + return fmt.Errorf("xds: json.Unmarshal(data) for field xds_servers failed during bootstrap: %v", err) + } + if len(servers) < 1 { + return fmt.Errorf("xds: bootstrap file parsing failed during bootstrap: file doesn't contain any management server to connect to") + } + xs := servers[0] + sc.ServerURI = xs.ServerURI + for _, cc := range xs.ChannelCreds { + // We stop at the first credential type that we support. + sc.credsType = cc.Type + if cc.Type == credsGoogleDefault { + sc.Creds = grpc.WithCredentialsBundle(google.NewDefaultCredentials()) + break + } else if cc.Type == credsInsecure { + sc.Creds = grpc.WithTransportCredentials(insecure.NewCredentials()) + break + } + } + for _, f := range xs.ServerFeatures { + if f == serverFeaturesV3 { + sc.TransportAPI = version.TransportV3 + } + } + return nil +} + +// Authority contains configuration for an Authority for an xDS control plane +// server. See the Authorities field in the Config struct for how it's used. +type Authority struct { + // ClientListenerResourceNameTemplate is template for the name of the + // Listener resource to subscribe to for a gRPC client channel. Used only + // when the channel is created using an "xds:" URI with this authority name. + // + // The token "%s", if present in this string, will be replaced + // with %-encoded service authority (i.e., the path part of the target + // URI used to create the gRPC channel). + // + // Must start with "xdstp:///". If it does not, + // that is considered a bootstrap file parsing error. + // + // If not present in the bootstrap file, defaults to + // "xdstp:///envoy.config.listener.v3.Listener/%s". + ClientListenerResourceNameTemplate string + // XDSServer contains the management server and config to connect to for + // this authority. + XDSServer *ServerConfig +} + +// UnmarshalJSON implement json unmarshaller. +func (a *Authority) UnmarshalJSON(data []byte) error { + var jsonData map[string]json.RawMessage + if err := json.Unmarshal(data, &jsonData); err != nil { + return fmt.Errorf("xds: failed to parse authority: %v", err) + } + + for k, v := range jsonData { + switch k { + case "xds_servers": + if err := json.Unmarshal(v, &a.XDSServer); err != nil { + return fmt.Errorf("xds: json.Unmarshal(%v) for field %q failed during bootstrap: %v", string(v), k, err) + } + case "client_listener_resource_name_template": + if err := json.Unmarshal(v, &a.ClientListenerResourceNameTemplate); err != nil { + return fmt.Errorf("xds: json.Unmarshal(%v) for field %q failed during bootstrap: %v", string(v), k, err) + } + } + } + return nil +} + +// Config provides the xDS client with several key bits of information that it +// requires in its interaction with the management server. The Config is +// initialized from the bootstrap file. +type Config struct { + // XDSServer is the management server to connect to. + // + // The bootstrap file contains a list of servers (with name+creds), but we + // pick the first one. + XDSServer *ServerConfig // CertProviderConfigs contains a mapping from certificate provider plugin // instance names to parsed buildable configs. CertProviderConfigs map[string]*certprovider.BuildableConfig // ServerListenerResourceNameTemplate is a template for the name of the - // Listener resource to subscribe to for a gRPC server. If the token `%s` is - // present in the string, it will be replaced with the server's listening - // "IP:port" (e.g., "0.0.0.0:8080", "[::]:8080"). For example, a value of - // "example/resource/%s" could become "example/resource/0.0.0.0:8080". + // Listener resource to subscribe to for a gRPC server. + // + // If starts with "xdstp:", will be interpreted as a new-style name, + // in which case the authority of the URI will be used to select the + // relevant configuration in the "authorities" map. + // + // The token "%s", if present in this string, will be replaced with the IP + // and port on which the server is listening. (e.g., "0.0.0.0:8080", + // "[::]:8080"). For example, a value of "example/resource/%s" could become + // "example/resource/0.0.0.0:8080". If the template starts with "xdstp:", + // the replaced string will be %-encoded. + // + // There is no default; if unset, xDS-based server creation fails. ServerListenerResourceNameTemplate string + // A template for the name of the Listener resource to subscribe to + // for a gRPC client channel. Used only when the channel is created + // with an "xds:" URI with no authority. + // + // If starts with "xdstp:", will be interpreted as a new-style name, + // in which case the authority of the URI will be used to select the + // relevant configuration in the "authorities" map. + // + // The token "%s", if present in this string, will be replaced with + // the service authority (i.e., the path part of the target URI + // used to create the gRPC channel). If the template starts with + // "xdstp:", the replaced string will be %-encoded. + // + // Defaults to "%s". + ClientDefaultListenerResourceNameTemplate string + + // Authorities is a map of authority name to corresponding configuration. + // + // This is used in the following cases: + // - A gRPC client channel is created using an "xds:" URI that includes + // an authority. + // - A gRPC client channel is created using an "xds:" URI with no + // authority, but the "client_default_listener_resource_name_template" + // field above turns it into an "xdstp:" URI. + // - A gRPC server is created and the + // "server_listener_resource_name_template" field is an "xdstp:" URI. + // + // In any of those cases, it is an error if the specified authority is + // not present in this map. + Authorities map[string]*Authority } type channelCreds struct { @@ -125,34 +256,6 @@ func bootstrapConfigFromEnvVariable() ([]byte, error) { // NewConfig returns a new instance of Config initialized by reading the // bootstrap file found at ${GRPC_XDS_BOOTSTRAP}. // -// The format of the bootstrap file will be as follows: -// { -// "xds_servers": [ -// { -// "server_uri": , -// "channel_creds": [ -// { -// "type": , -// "config": -// } -// ], -// "server_features": [ ... ], -// } -// ], -// "node": , -// "certificate_providers" : { -// "default": { -// "plugin_name": "default-plugin-name", -// "config": { default plugin config in JSON } -// }, -// "foo": { -// "plugin_name": "foo", -// "config": { foo plugin config in JSON } -// } -// }, -// "server_listener_resource_name_template": "grpc/server?xds.resource.listening_address=%s" -// } -// // 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. @@ -162,6 +265,8 @@ func bootstrapConfigFromEnvVariable() ([]byte, error) { // fields left unspecified, in which case the caller should use some sane // defaults. func NewConfig() (*Config, error) { + // Examples of the bootstrap json can be found in the generator tests + // https://github.com/GoogleCloudPlatform/traffic-director-grpc-bootstrap/blob/master/main_test.go. data, err := bootstrapConfigFromEnvVariable() if err != nil { return nil, fmt.Errorf("xds: Failed to read bootstrap config: %v", err) @@ -181,7 +286,7 @@ func NewConfigFromContents(data []byte) (*Config, error) { return nil, fmt.Errorf("xds: Failed to parse bootstrap config: %v", err) } - serverSupportsV3 := false + var node *v3corepb.Node m := jsonpb.Unmarshaler{AllowUnknownFields: true} for k, v := range jsonData { switch k { @@ -192,37 +297,14 @@ func NewConfigFromContents(data []byte) (*Config, error) { // "build_version" field. In any case, the unmarshal will succeed // because we have set the `AllowUnknownFields` option on the // unmarshaler. - n := &v3corepb.Node{} - if err := m.Unmarshal(bytes.NewReader(v), n); err != nil { + node = &v3corepb.Node{} + if err := m.Unmarshal(bytes.NewReader(v), node); err != nil { 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 { + if err := json.Unmarshal(v, &config.XDSServer); err != nil { return nil, fmt.Errorf("xds: json.Unmarshal(%v) for field %q failed during bootstrap: %v", string(v), k, err) } - if len(servers) < 1 { - return nil, fmt.Errorf("xds: bootstrap file parsing failed during bootstrap: file doesn't contain any management server to connect to") - } - xs := servers[0] - config.BalancerName = xs.ServerURI - for _, cc := range xs.ChannelCreds { - // We stop at the first credential type that we support. - if cc.Type == credsGoogleDefault { - config.Creds = grpc.WithCredentialsBundle(google.NewDefaultCredentials()) - break - } else if cc.Type == credsInsecure { - config.Creds = grpc.WithTransportCredentials(insecure.NewCredentials()) - break - } - } - for _, f := range xs.ServerFeatures { - switch f { - case serverFeaturesV3: - serverSupportsV3 = true - } - } case "certificate_providers": var providerInstances map[string]json.RawMessage if err := json.Unmarshal(v, &providerInstances); err != nil { @@ -256,27 +338,42 @@ func NewConfigFromContents(data []byte) (*Config, error) { if err := json.Unmarshal(v, &config.ServerListenerResourceNameTemplate); err != nil { return nil, fmt.Errorf("xds: json.Unmarshal(%v) for field %q failed during bootstrap: %v", string(v), k, err) } + default: + logger.Warningf("Bootstrap content has unknown field: %s", k) } // Do not fail the xDS bootstrap when an unknown field is seen. This can // happen when an older version client reads a newer version bootstrap // file with new fields. } - if config.BalancerName == "" { + if config.ClientDefaultListenerResourceNameTemplate == "" { + // Default value of the default client listener name template is "%s". + config.ClientDefaultListenerResourceNameTemplate = "%s" + } + if config.XDSServer == nil { + return nil, fmt.Errorf("xds: Required field %q not found in bootstrap %s", "xds_servers", jsonData["xds_servers"]) + } + if config.XDSServer.ServerURI == "" { return nil, fmt.Errorf("xds: Required field %q not found in bootstrap %s", "xds_servers.server_uri", jsonData["xds_servers"]) } - if config.Creds == nil { + if config.XDSServer.Creds == nil { return nil, fmt.Errorf("xds: Required field %q doesn't contain valid value in bootstrap %s", "xds_servers.channel_creds", jsonData["xds_servers"]) } - - // We end up using v3 transport protocol version only if the server supports - // v3, indicated by the presence of "xds_v3" in server_features. The default - // value of the enum type "version.TransportAPI" is v2. - if serverSupportsV3 { - config.TransportAPI = version.TransportV3 + // Post-process the authorities' client listener resource template field: + // - if set, it must start with "xdstp:///" + // - if not set, it defaults to "xdstp:///envoy.config.listener.v3.Listener/%s" + for name, authority := range config.Authorities { + prefix := fmt.Sprintf("xdstp://%s", name) + if authority.ClientListenerResourceNameTemplate == "" { + authority.ClientListenerResourceNameTemplate = prefix + "/envoy.config.listener.v3.Listener/%s" + continue + } + if !strings.HasPrefix(authority.ClientListenerResourceNameTemplate, prefix) { + return nil, fmt.Errorf("xds: field ClientListenerResourceNameTemplate %q of authority %q doesn't start with prefix %q", authority.ClientListenerResourceNameTemplate, name, prefix) + } } - if err := config.updateNodeProto(); err != nil { + if err := config.updateNodeProto(node); err != nil { return nil, err } logger.Infof("Bootstrap config for creating xds-client: %v", pretty.ToJSON(config)) @@ -285,47 +382,57 @@ func NewConfigFromContents(data []byte) (*Config, error) { // updateNodeProto updates the node proto read from the bootstrap file. // -// Node proto in Config contains a v3.Node protobuf message corresponding to the -// JSON contents found in the bootstrap file. This method performs some post +// The input node is a v3.Node protobuf message corresponding to the JSON +// contents found in the bootstrap file. This method performs some post // processing on it: -// 1. If we don't find a nodeProto in the bootstrap file, we create an empty one -// here. That way, callers of this function can always expect that the NodeProto -// field is non-nil. -// 2. If the transport protocol version to be used is not v3, we convert the -// current v3.Node proto in a v2.Node proto. -// 3. Some additional fields which are not expected to be set in the bootstrap +// 1. If the node is nil, we create an empty one here. That way, callers of this +// function can always expect that the NodeProto field is non-nil. +// 2. Some additional fields which are not expected to be set in the bootstrap // file are populated here. -func (c *Config) updateNodeProto() error { - if c.TransportAPI == version.TransportV3 { - v3, _ := c.NodeProto.(*v3corepb.Node) - if v3 == nil { - v3 = &v3corepb.Node{} - } - v3.UserAgentName = gRPCUserAgentName - v3.UserAgentVersionType = &v3corepb.Node_UserAgentVersion{UserAgentVersion: grpc.Version} - v3.ClientFeatures = append(v3.ClientFeatures, clientFeatureNoOverprovisioning) - c.NodeProto = v3 - return nil +// 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. +func (c *Config) updateNodeProto(node *v3corepb.Node) error { + v3 := node + if v3 == nil { + v3 = &v3corepb.Node{} } + v3.UserAgentName = gRPCUserAgentName + v3.UserAgentVersionType = &v3corepb.Node_UserAgentVersion{UserAgentVersion: grpc.Version} + v3.ClientFeatures = append(v3.ClientFeatures, clientFeatureNoOverprovisioning) v2 := &v2corepb.Node{} - if c.NodeProto != nil { - v3, err := proto.Marshal(c.NodeProto) - if err != nil { - return fmt.Errorf("xds: proto.Marshal(%v): %v", c.NodeProto, err) - } - if err := proto.Unmarshal(v3, v2); err != nil { - return fmt.Errorf("xds: proto.Unmarshal(%v): %v", v3, err) - } + v3bytes, err := proto.Marshal(v3) + if err != nil { + return fmt.Errorf("xds: proto.Marshal(%v): %v", v3, err) + } + if err := proto.Unmarshal(v3bytes, v2); err != nil { + return fmt.Errorf("xds: proto.Unmarshal(%v): %v", v3bytes, err) } - c.NodeProto = v2 - // BuildVersion is deprecated, and is replaced by user_agent_name and // user_agent_version. But the management servers are still using the old // field, so we will keep both set. v2.BuildVersion = gRPCVersion - v2.UserAgentName = gRPCUserAgentName v2.UserAgentVersionType = &v2corepb.Node_UserAgentVersion{UserAgentVersion: grpc.Version} - v2.ClientFeatures = append(v2.ClientFeatures, clientFeatureNoOverprovisioning) + + switch c.XDSServer.TransportAPI { + case version.TransportV2: + c.XDSServer.NodeProto = v2 + case version.TransportV3: + c.XDSServer.NodeProto = v3 + } + + for _, a := range c.Authorities { + if a.XDSServer == nil { + continue + } + switch a.XDSServer.TransportAPI { + case version.TransportV2: + a.XDSServer.NodeProto = v2 + case version.TransportV3: + a.XDSServer.NodeProto = v3 + } + } + return nil } diff --git a/xds/internal/xdsclient/bootstrap/bootstrap_test.go b/xds/internal/xdsclient/bootstrap/bootstrap_test.go index 501d62102d21..9d6ead0ff5b5 100644 --- a/xds/internal/xdsclient/bootstrap/bootstrap_test.go +++ b/xds/internal/xdsclient/bootstrap/bootstrap_test.go @@ -30,6 +30,7 @@ import ( "github.com/golang/protobuf/proto" structpb "github.com/golang/protobuf/ptypes/struct" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "google.golang.org/grpc" "google.golang.org/grpc/credentials/google" @@ -207,59 +208,44 @@ var ( ClientFeatures: []string{clientFeatureNoOverprovisioning}, } nilCredsConfigV2 = &Config{ - BalancerName: "trafficdirector.googleapis.com:443", - Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), - NodeProto: v2NodeProto, + XDSServer: &ServerConfig{ + ServerURI: "trafficdirector.googleapis.com:443", + Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), + credsType: "insecure", + NodeProto: v2NodeProto, + }, + ClientDefaultListenerResourceNameTemplate: "%s", } nonNilCredsConfigV2 = &Config{ - BalancerName: "trafficdirector.googleapis.com:443", - Creds: grpc.WithCredentialsBundle(google.NewComputeEngineCredentials()), - NodeProto: v2NodeProto, + XDSServer: &ServerConfig{ + ServerURI: "trafficdirector.googleapis.com:443", + Creds: grpc.WithCredentialsBundle(google.NewComputeEngineCredentials()), + credsType: "google_default", + NodeProto: v2NodeProto, + }, + ClientDefaultListenerResourceNameTemplate: "%s", } nonNilCredsConfigV3 = &Config{ - BalancerName: "trafficdirector.googleapis.com:443", - Creds: grpc.WithCredentialsBundle(google.NewComputeEngineCredentials()), - TransportAPI: version.TransportV3, - NodeProto: v3NodeProto, + XDSServer: &ServerConfig{ + ServerURI: "trafficdirector.googleapis.com:443", + Creds: grpc.WithCredentialsBundle(google.NewComputeEngineCredentials()), + credsType: "google_default", + TransportAPI: version.TransportV3, + NodeProto: v3NodeProto, + }, + ClientDefaultListenerResourceNameTemplate: "%s", } ) func (c *Config) compare(want *Config) error { - if c.BalancerName != want.BalancerName { - return fmt.Errorf("config.BalancerName is %s, want %s", c.BalancerName, want.BalancerName) - } - // Since Creds is of type grpc.DialOption interface, where the - // implementation is provided by a function, it is not possible to compare. - if (c.Creds != nil) != (want.Creds != nil) { - return fmt.Errorf("config.Creds is %#v, want %#v", c.Creds, want.Creds) - } - if c.TransportAPI != want.TransportAPI { - return fmt.Errorf("config.TransportAPI is %v, want %v", c.TransportAPI, want.TransportAPI) - - } - if diff := cmp.Diff(want.NodeProto, c.NodeProto, cmp.Comparer(proto.Equal)); diff != "" { - return fmt.Errorf("config.NodeProto diff (-want, +got):\n%s", diff) - } - if c.ServerListenerResourceNameTemplate != want.ServerListenerResourceNameTemplate { - return fmt.Errorf("config.ServerListenerResourceNameTemplate is %q, want %q", c.ServerListenerResourceNameTemplate, want.ServerListenerResourceNameTemplate) - } - - // A vanilla cmp.Equal or cmp.Diff will not produce useful error message - // here. So, we iterate through the list of configs and compare them one at - // a time. - gotCfgs := c.CertProviderConfigs - wantCfgs := want.CertProviderConfigs - if len(gotCfgs) != len(wantCfgs) { - return fmt.Errorf("config.CertProviderConfigs is %d entries, want %d", len(gotCfgs), len(wantCfgs)) - } - for instance, gotCfg := range gotCfgs { - wantCfg, ok := wantCfgs[instance] - if !ok { - return fmt.Errorf("config.CertProviderConfigs has unexpected plugin instance %q with config %q", instance, gotCfg.String()) - } - if got, want := gotCfg.String(), wantCfg.String(); got != want { - return fmt.Errorf("config.CertProviderConfigs for plugin instance %q has config %q, want %q", instance, got, want) - } + if diff := cmp.Diff(c, want, + cmpopts.EquateEmpty(), + cmp.AllowUnexported(ServerConfig{}), + cmp.Comparer(proto.Equal), + cmp.Comparer(func(a, b grpc.DialOption) bool { return (a != nil) == (b != nil) }), + cmp.Transformer("certproviderconfigstring", func(a *certprovider.BuildableConfig) string { return a.String() }), + ); diff != "" { + return fmt.Errorf("diff: %v", diff) } return nil } @@ -285,6 +271,7 @@ func setupBootstrapOverride(bootstrapFileMap map[string]string) func() { // This function overrides the bootstrap file NAME env variable, to test the // code that reads file with the given fileName. func testNewConfigWithFileNameEnv(t *testing.T, fileName string, wantError bool, wantConfig *Config) { + t.Helper() origBootstrapFileName := env.BootstrapFileName env.BootstrapFileName = fileName defer func() { env.BootstrapFileName = origBootstrapFileName }() @@ -304,10 +291,10 @@ func testNewConfigWithFileNameEnv(t *testing.T, fileName string, wantError bool, // This function overrides the bootstrap file CONTENT env variable, to test the // code that uses the content from env directly. func testNewConfigWithFileContentEnv(t *testing.T, fileName string, wantError bool, wantConfig *Config) { + t.Helper() b, err := bootstrapFileReadFunc(fileName) if err != nil { - // If file reading failed, skip this test. - return + t.Skip(err) } origBootstrapContent := env.BootstrapFileContent env.BootstrapFileContent = string(b) @@ -404,14 +391,18 @@ func TestNewConfigV2ProtoSuccess(t *testing.T) { }{ { "emptyNodeProto", &Config{ - BalancerName: "trafficdirector.googleapis.com:443", - Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), - NodeProto: &v2corepb.Node{ - BuildVersion: gRPCVersion, - UserAgentName: gRPCUserAgentName, - UserAgentVersionType: &v2corepb.Node_UserAgentVersion{UserAgentVersion: grpc.Version}, - ClientFeatures: []string{clientFeatureNoOverprovisioning}, + XDSServer: &ServerConfig{ + ServerURI: "trafficdirector.googleapis.com:443", + Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), + credsType: "insecure", + NodeProto: &v2corepb.Node{ + BuildVersion: gRPCVersion, + UserAgentName: gRPCUserAgentName, + UserAgentVersionType: &v2corepb.Node_UserAgentVersion{UserAgentVersion: grpc.Version}, + ClientFeatures: []string{clientFeatureNoOverprovisioning}, + }, }, + ClientDefaultListenerResourceNameTemplate: "%s", }, }, {"unknownTopLevelFieldInFile", nilCredsConfigV2}, @@ -670,13 +661,17 @@ func TestNewConfigWithCertificateProviders(t *testing.T) { defer cancel() goodConfig := &Config{ - BalancerName: "trafficdirector.googleapis.com:443", - Creds: grpc.WithCredentialsBundle(google.NewComputeEngineCredentials()), - TransportAPI: version.TransportV3, - NodeProto: v3NodeProto, + XDSServer: &ServerConfig{ + ServerURI: "trafficdirector.googleapis.com:443", + Creds: grpc.WithCredentialsBundle(google.NewComputeEngineCredentials()), + credsType: "google_default", + TransportAPI: version.TransportV3, + NodeProto: v3NodeProto, + }, CertProviderConfigs: map[string]*certprovider.BuildableConfig{ "fakeProviderInstance": wantCfg, }, + ClientDefaultListenerResourceNameTemplate: "%s", } tests := []struct { name string @@ -760,11 +755,15 @@ func TestNewConfigWithServerListenerResourceNameTemplate(t *testing.T) { { name: "goodServerListenerResourceNameTemplate", wantConfig: &Config{ - BalancerName: "trafficdirector.googleapis.com:443", - Creds: grpc.WithCredentialsBundle(google.NewComputeEngineCredentials()), - TransportAPI: version.TransportV2, - NodeProto: v2NodeProto, - ServerListenerResourceNameTemplate: "grpc/server?xds.resource.listening_address=%s", + XDSServer: &ServerConfig{ + ServerURI: "trafficdirector.googleapis.com:443", + Creds: grpc.WithCredentialsBundle(google.NewComputeEngineCredentials()), + credsType: "google_default", + TransportAPI: version.TransportV2, + NodeProto: v2NodeProto, + }, + ServerListenerResourceNameTemplate: "grpc/server?xds.resource.listening_address=%s", + ClientDefaultListenerResourceNameTemplate: "%s", }, }, } diff --git a/xds/internal/xdsclient/client.go b/xds/internal/xdsclient/client.go index 3230c66c06e3..8b7d4bad04f9 100644 --- a/xds/internal/xdsclient/client.go +++ b/xds/internal/xdsclient/client.go @@ -28,8 +28,6 @@ import ( "sync" "time" - v2corepb "github.com/envoyproxy/go-control-plane/envoy/api/v2/core" - v3corepb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" "github.com/golang/protobuf/proto" "google.golang.org/protobuf/types/known/anypb" @@ -629,27 +627,18 @@ type clientImpl struct { // newWithConfig returns a new xdsClient with the given config. func newWithConfig(config *bootstrap.Config, watchExpiryTimeout time.Duration) (*clientImpl, error) { switch { - case config.BalancerName == "": + case config.XDSServer == nil: + return nil, errors.New("xds: no xds_server provided") + case config.XDSServer.ServerURI == "": return nil, errors.New("xds: no xds_server name provided in options") - case config.Creds == nil: + case config.XDSServer.Creds == nil: return nil, errors.New("xds: no credentials provided in options") - case config.NodeProto == nil: + case config.XDSServer.NodeProto == nil: return nil, errors.New("xds: no node_proto provided in options") } - switch config.TransportAPI { - case version.TransportV2: - if _, ok := config.NodeProto.(*v2corepb.Node); !ok { - return nil, fmt.Errorf("xds: Node proto type (%T) does not match API version: %v", config.NodeProto, config.TransportAPI) - } - case version.TransportV3: - if _, ok := config.NodeProto.(*v3corepb.Node); !ok { - return nil, fmt.Errorf("xds: Node proto type (%T) does not match API version: %v", config.NodeProto, config.TransportAPI) - } - } - dopts := []grpc.DialOption{ - config.Creds, + config.XDSServer.Creds, grpc.WithKeepaliveParams(keepalive.ClientParameters{ Time: 5 * time.Minute, Timeout: 20 * time.Second, @@ -677,23 +666,24 @@ func newWithConfig(config *bootstrap.Config, watchExpiryTimeout time.Duration) ( lrsClients: make(map[string]*lrsClient), } - cc, err := grpc.Dial(config.BalancerName, dopts...) + cc, err := grpc.Dial(config.XDSServer.ServerURI, dopts...) if err != nil { // An error from a non-blocking dial indicates something serious. - return nil, fmt.Errorf("xds: failed to dial balancer {%s}: %v", config.BalancerName, err) + return nil, fmt.Errorf("xds: failed to dial balancer {%s}: %v", config.XDSServer.ServerURI, err) } c.cc = cc c.logger = prefixLogger((c)) - c.logger.Infof("Created ClientConn to xDS management server: %s", config.BalancerName) + c.logger.Infof("Created ClientConn to xDS management server: %s", config.XDSServer) - apiClient, err := newAPIClient(config.TransportAPI, cc, BuildOptions{ + apiClient, err := newAPIClient(config.XDSServer.TransportAPI, cc, BuildOptions{ Parent: c, Validator: c.updateValidator, - NodeProto: config.NodeProto, + NodeProto: config.XDSServer.NodeProto, Backoff: backoff.DefaultExponential.Backoff, Logger: c.logger, }) if err != nil { + cc.Close() return nil, err } c.apiClient = apiClient diff --git a/xds/internal/xdsclient/client_test.go b/xds/internal/xdsclient/client_test.go index 7c3423cd5ad7..2a6d6ae2a536 100644 --- a/xds/internal/xdsclient/client_test.go +++ b/xds/internal/xdsclient/client_test.go @@ -82,9 +82,11 @@ func clientOpts(balancerName string, overrideWatchExpiryTimeout bool) (*bootstra watchExpiryTimeout = defaultTestWatchExpiryTimeout } return &bootstrap.Config{ - BalancerName: balancerName, - Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), - NodeProto: xdstestutils.EmptyNodeProtoV2, + XDSServer: &bootstrap.ServerConfig{ + ServerURI: balancerName, + Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), + NodeProto: xdstestutils.EmptyNodeProtoV2, + }, }, watchExpiryTimeout } @@ -269,9 +271,11 @@ func (s) TestClientNewSingleton(t *testing.T) { oldBootstrapNewConfig := bootstrapNewConfig bootstrapNewConfig = func() (*bootstrap.Config, error) { return &bootstrap.Config{ - BalancerName: testXDSServer, - Creds: grpc.WithInsecure(), - NodeProto: xdstestutils.EmptyNodeProtoV2, + XDSServer: &bootstrap.ServerConfig{ + ServerURI: testXDSServer, + Creds: grpc.WithInsecure(), + NodeProto: xdstestutils.EmptyNodeProtoV2, + }, }, nil } defer func() { bootstrapNewConfig = oldBootstrapNewConfig }() diff --git a/xds/internal/xdsclient/dump_test.go b/xds/internal/xdsclient/dump_test.go index d03479ca4ade..c162a9418f23 100644 --- a/xds/internal/xdsclient/dump_test.go +++ b/xds/internal/xdsclient/dump_test.go @@ -75,9 +75,11 @@ func (s) TestLDSConfigDump(t *testing.T) { } client, err := xdsclient.NewWithConfigForTesting(&bootstrap.Config{ - BalancerName: testXDSServer, - Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), - NodeProto: xdstestutils.EmptyNodeProtoV2, + XDSServer: &bootstrap.ServerConfig{ + ServerURI: testXDSServer, + Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), + NodeProto: xdstestutils.EmptyNodeProtoV2, + }, }, defaultTestWatchExpiryTimeout) if err != nil { t.Fatalf("failed to create client: %v", err) @@ -189,9 +191,11 @@ func (s) TestRDSConfigDump(t *testing.T) { } client, err := xdsclient.NewWithConfigForTesting(&bootstrap.Config{ - BalancerName: testXDSServer, - Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), - NodeProto: xdstestutils.EmptyNodeProtoV2, + XDSServer: &bootstrap.ServerConfig{ + ServerURI: testXDSServer, + Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), + NodeProto: xdstestutils.EmptyNodeProtoV2, + }, }, defaultTestWatchExpiryTimeout) if err != nil { t.Fatalf("failed to create client: %v", err) @@ -303,9 +307,11 @@ func (s) TestCDSConfigDump(t *testing.T) { } client, err := xdsclient.NewWithConfigForTesting(&bootstrap.Config{ - BalancerName: testXDSServer, - Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), - NodeProto: xdstestutils.EmptyNodeProtoV2, + XDSServer: &bootstrap.ServerConfig{ + ServerURI: testXDSServer, + Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), + NodeProto: xdstestutils.EmptyNodeProtoV2, + }, }, defaultTestWatchExpiryTimeout) if err != nil { t.Fatalf("failed to create client: %v", err) @@ -403,9 +409,11 @@ func (s) TestEDSConfigDump(t *testing.T) { } client, err := xdsclient.NewWithConfigForTesting(&bootstrap.Config{ - BalancerName: testXDSServer, - Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), - NodeProto: xdstestutils.EmptyNodeProtoV2, + XDSServer: &bootstrap.ServerConfig{ + ServerURI: testXDSServer, + Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), + NodeProto: xdstestutils.EmptyNodeProtoV2, + }, }, defaultTestWatchExpiryTimeout) if err != nil { t.Fatalf("failed to create client: %v", err) diff --git a/xds/internal/xdsclient/loadreport.go b/xds/internal/xdsclient/loadreport.go index 32a71dada7fb..4df9a5c0c3a4 100644 --- a/xds/internal/xdsclient/loadreport.go +++ b/xds/internal/xdsclient/loadreport.go @@ -115,12 +115,12 @@ func (lrsC *lrsClient) startStream() { var cc *grpc.ClientConn lrsC.parent.logger.Infof("Starting load report to server: %s", lrsC.server) - if lrsC.server == "" || lrsC.server == lrsC.parent.config.BalancerName { + if lrsC.server == "" || lrsC.server == lrsC.parent.config.XDSServer.ServerURI { // Reuse the xDS client if server is the same. cc = lrsC.parent.cc } else { lrsC.parent.logger.Infof("LRS server is different from management server, starting a new ClientConn") - ccNew, err := grpc.Dial(lrsC.server, lrsC.parent.config.Creds) + ccNew, err := grpc.Dial(lrsC.server, lrsC.parent.config.XDSServer.Creds) if err != nil { // An error from a non-blocking dial indicates something serious. lrsC.parent.logger.Infof("xds: failed to dial load report server {%s}: %v", lrsC.server, err) diff --git a/xds/internal/xdsclient/loadreport_test.go b/xds/internal/xdsclient/loadreport_test.go index 88a08eb43fda..db31de6cf78b 100644 --- a/xds/internal/xdsclient/loadreport_test.go +++ b/xds/internal/xdsclient/loadreport_test.go @@ -55,10 +55,12 @@ func (s) TestLRSClient(t *testing.T) { defer sCleanup() xdsC, err := xdsclient.NewWithConfigForTesting(&bootstrap.Config{ - BalancerName: fs.Address, - Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), - NodeProto: &v2corepb.Node{}, - TransportAPI: version.TransportV2, + XDSServer: &bootstrap.ServerConfig{ + ServerURI: fs.Address, + Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), + TransportAPI: version.TransportV2, + NodeProto: &v2corepb.Node{}, + }, }, defaultClientWatchExpiryTimeout) if err != nil { t.Fatalf("failed to create xds client: %v", err) diff --git a/xds/internal/xdsclient/xdsclient_test.go b/xds/internal/xdsclient/xdsclient_test.go index f348df481615..23d6d6f54183 100644 --- a/xds/internal/xdsclient/xdsclient_test.go +++ b/xds/internal/xdsclient/xdsclient_test.go @@ -56,34 +56,42 @@ func (s) TestNew(t *testing.T) { { name: "empty-balancer-name", config: &bootstrap.Config{ - Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), - NodeProto: testutils.EmptyNodeProtoV2, + XDSServer: &bootstrap.ServerConfig{ + Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), + NodeProto: testutils.EmptyNodeProtoV2, + }, }, wantErr: true, }, { name: "empty-dial-creds", config: &bootstrap.Config{ - BalancerName: testXDSServer, - NodeProto: testutils.EmptyNodeProtoV2, + XDSServer: &bootstrap.ServerConfig{ + ServerURI: testXDSServer, + NodeProto: testutils.EmptyNodeProtoV2, + }, }, wantErr: true, }, { name: "empty-node-proto", config: &bootstrap.Config{ - BalancerName: testXDSServer, - Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), + XDSServer: &bootstrap.ServerConfig{ + ServerURI: testXDSServer, + Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), + }, }, wantErr: true, }, { name: "node-proto-version-mismatch", config: &bootstrap.Config{ - BalancerName: testXDSServer, - Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), - NodeProto: testutils.EmptyNodeProtoV3, - TransportAPI: version.TransportV2, + XDSServer: &bootstrap.ServerConfig{ + ServerURI: testXDSServer, + Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), + TransportAPI: version.TransportV2, + NodeProto: testutils.EmptyNodeProtoV3, + }, }, wantErr: true, }, @@ -91,9 +99,11 @@ func (s) TestNew(t *testing.T) { { name: "happy-case", config: &bootstrap.Config{ - BalancerName: testXDSServer, - Creds: grpc.WithInsecure(), - NodeProto: testutils.EmptyNodeProtoV2, + XDSServer: &bootstrap.ServerConfig{ + ServerURI: testXDSServer, + Creds: grpc.WithInsecure(), + NodeProto: testutils.EmptyNodeProtoV2, + }, }, }, } diff --git a/xds/server_test.go b/xds/server_test.go index 63cb6878ee7a..501b8ba76e20 100644 --- a/xds/server_test.go +++ b/xds/server_test.go @@ -322,9 +322,11 @@ func setupOverrides() (*fakeGRPCServer, *testutils.Channel, func()) { newXDSClient = func() (xdsclient.XDSClient, error) { c := fakeclient.NewClient() c.SetBootstrapConfig(&bootstrap.Config{ - BalancerName: "dummyBalancer", - Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), - NodeProto: xdstestutils.EmptyNodeProtoV3, + XDSServer: &bootstrap.ServerConfig{ + ServerURI: "dummyBalancer", + Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), + NodeProto: xdstestutils.EmptyNodeProtoV3, + }, ServerListenerResourceNameTemplate: testServerListenerResourceNameTemplate, CertProviderConfigs: certProviderConfigs, }) @@ -352,9 +354,11 @@ func setupOverridesForXDSCreds(includeCertProviderCfg bool) (*testutils.Channel, newXDSClient = func() (xdsclient.XDSClient, error) { c := fakeclient.NewClient() bc := &bootstrap.Config{ - BalancerName: "dummyBalancer", - Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), - NodeProto: xdstestutils.EmptyNodeProtoV3, + XDSServer: &bootstrap.ServerConfig{ + ServerURI: "dummyBalancer", + Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), + NodeProto: xdstestutils.EmptyNodeProtoV3, + }, ServerListenerResourceNameTemplate: testServerListenerResourceNameTemplate, } if includeCertProviderCfg { @@ -599,18 +603,22 @@ func (s) TestServeBootstrapConfigInvalid(t *testing.T) { { desc: "certificate provider config is missing", bootstrapConfig: &bootstrap.Config{ - BalancerName: "dummyBalancer", - Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), - NodeProto: xdstestutils.EmptyNodeProtoV3, + XDSServer: &bootstrap.ServerConfig{ + ServerURI: "dummyBalancer", + Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), + NodeProto: xdstestutils.EmptyNodeProtoV3, + }, ServerListenerResourceNameTemplate: testServerListenerResourceNameTemplate, }, }, { desc: "server_listener_resource_name_template is missing", bootstrapConfig: &bootstrap.Config{ - BalancerName: "dummyBalancer", - Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), - NodeProto: xdstestutils.EmptyNodeProtoV3, + XDSServer: &bootstrap.ServerConfig{ + ServerURI: "dummyBalancer", + Creds: grpc.WithTransportCredentials(insecure.NewCredentials()), + NodeProto: xdstestutils.EmptyNodeProtoV3, + }, CertProviderConfigs: certProviderConfigs, }, },