From dfcabe08c63901b53e4b5a8381755f48cdf37bb4 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Thu, 6 Jun 2024 15:09:39 -0700 Subject: [PATCH] xds: cleanup bootstrap processing functionality (#7299) --- internal/testutils/xds/bootstrap/bootstrap.go | 1 - .../xds/e2e/setup_management_server.go | 2 +- internal/xds/bootstrap/bootstrap.go | 906 ++++++++++-------- internal/xds/bootstrap/bootstrap_test.go | 531 ++++++---- stats/opentelemetry/csm/pluginoption.go | 4 +- xds/googledirectpath/googlec2p.go | 25 +- xds/googledirectpath/googlec2p_test.go | 320 ++++--- .../balancer/cdsbalancer/cdsbalancer.go | 2 +- .../balancer/cdsbalancer/cdsbalancer_test.go | 18 +- .../balancer/clusterimpl/balancer_test.go | 53 +- .../balancer/clusterimpl/config_test.go | 13 +- .../balancer/clusterresolver/config_test.go | 14 +- .../clusterresolver/configbuilder_test.go | 17 + .../e2e_test/aggregate_cluster_test.go | 4 +- xds/internal/resolver/xds_resolver.go | 8 +- xds/internal/server/conn_wrapper.go | 2 +- xds/internal/testutils/fakeclient/client.go | 2 +- xds/internal/testutils/testutils.go | 21 - xds/internal/xdsclient/authority.go | 6 +- xds/internal/xdsclient/authority_test.go | 18 +- xds/internal/xdsclient/client_new.go | 2 +- xds/internal/xdsclient/clientimpl.go | 18 +- .../xdsclient/clientimpl_authority.go | 14 +- xds/internal/xdsclient/clientimpl_dump.go | 2 +- xds/internal/xdsclient/loadreport_test.go | 14 +- xds/internal/xdsclient/singleton.go | 2 +- .../xdsclient/tests/resource_update_test.go | 8 +- .../xdsclient/transport/internal/internal.go | 25 + .../xdsclient/transport/loadreport_test.go | 9 +- xds/internal/xdsclient/transport/transport.go | 20 +- .../transport/transport_ack_nack_test.go | 23 +- .../transport/transport_backoff_test.go | 23 +- .../xdsclient/transport/transport_new_test.go | 24 +- .../transport/transport_resource_test.go | 23 +- .../xdsclient/transport/transport_test.go | 58 +- .../xdsresource/listener_resource_type.go | 4 +- .../xdsresource/tests/unmarshal_cds_test.go | 12 +- .../xdsresource/unmarshal_cds_test.go | 25 +- xds/server.go | 4 +- 39 files changed, 1413 insertions(+), 864 deletions(-) create mode 100644 xds/internal/xdsclient/transport/internal/internal.go diff --git a/internal/testutils/xds/bootstrap/bootstrap.go b/internal/testutils/xds/bootstrap/bootstrap.go index cac946a8e..2ecd46d9a 100644 --- a/internal/testutils/xds/bootstrap/bootstrap.go +++ b/internal/testutils/xds/bootstrap/bootstrap.go @@ -111,7 +111,6 @@ func Contents(opts Options) ([]byte, error) { ClientDefaultListenerResourceNameTemplate: opts.ClientDefaultListenerResourceNameTemplate, ServerListenerResourceNameTemplate: opts.ServerListenerResourceNameTemplate, } - cfg.XdsServers[0].ServerFeatures = append(cfg.XdsServers[0].ServerFeatures, "xds_v3") if opts.IgnoreResourceDeletion { cfg.XdsServers[0].ServerFeatures = append(cfg.XdsServers[0].ServerFeatures, "ignore_resource_deletion") } diff --git a/internal/testutils/xds/e2e/setup_management_server.go b/internal/testutils/xds/e2e/setup_management_server.go index 0218d4f2c..80fe00719 100644 --- a/internal/testutils/xds/e2e/setup_management_server.go +++ b/internal/testutils/xds/e2e/setup_management_server.go @@ -57,7 +57,7 @@ func SetupManagementServer(t *testing.T, opts ManagementServerOptions) (*Managem }() nodeID := uuid.New().String() - bootstrapContents, err := DefaultBootstrapContents(nodeID, server.Address) + bootstrapContents, err := DefaultBootstrapContents(nodeID, fmt.Sprintf("passthrough:///%s", server.Address)) if err != nil { server.Stop() t.Fatal(err) diff --git a/internal/xds/bootstrap/bootstrap.go b/internal/xds/bootstrap/bootstrap.go index b8b92a6cb..7eaff2c9d 100644 --- a/internal/xds/bootstrap/bootstrap.go +++ b/internal/xds/bootstrap/bootstrap.go @@ -24,8 +24,10 @@ import ( "bytes" "encoding/json" "fmt" + "maps" "net/url" "os" + "slices" "strings" "google.golang.org/grpc" @@ -34,25 +36,17 @@ import ( "google.golang.org/grpc/internal/envconfig" "google.golang.org/grpc/internal/pretty" "google.golang.org/grpc/xds/bootstrap" - "google.golang.org/protobuf/encoding/protojson" + "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/types/known/structpb" v3corepb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" ) const ( - // The "server_features" field in the bootstrap file contains a list of - // features supported by the server: - // - A value of "xds_v3" indicates that the server supports the v3 version of - // the xDS transport protocol. - // - A value of "ignore_resource_deletion" indicates that the client should - // ignore deletion of Listener and Cluster resources in updates from the - // server. - serverFeaturesV3 = "xds_v3" serverFeaturesIgnoreResourceDeletion = "ignore_resource_deletion" - - gRPCUserAgentName = "gRPC Go" - clientFeatureNoOverprovisioning = "envoy.lb.does_not_support_overprovisioning" - clientFeatureResourceWrapper = "xds.config.resource-in-sotw" + gRPCUserAgentName = "gRPC Go" + clientFeatureNoOverprovisioning = "envoy.lb.does_not_support_overprovisioning" + clientFeatureResourceWrapper = "xds.config.resource-in-sotw" ) // For overriding in unit tests. @@ -60,12 +54,15 @@ var bootstrapFileReadFunc = os.ReadFile // ChannelCreds contains the credentials to be used while communicating with an // xDS server. It is also used to dedup servers with the same server URI. +// +// This type does not implement custom JSON marshal/unmarshal logic because it +// is straightforward to accomplish the same with json struct tags. type ChannelCreds struct { // Type contains a unique name identifying the credentials type. The only // supported types currently are "google_default" and "insecure". - Type string + Type string `json:"type,omitempty"` // Config contains the JSON configuration associated with the credentials. - Config json.RawMessage + Config json.RawMessage `json:"config,omitempty"` } // Equal reports whether cc and other are considered equal. @@ -87,164 +84,10 @@ func (cc ChannelCreds) String() string { return cc.Type + "-" + string(b) } -// ServerConfig contains the configuration to connect to a server, including -// URI, creds, and transport API version (e.g. v2 or v3). +// Authority contains configuration for an xDS control plane authority. // -// It contains unexported fields that are initialized when unmarshaled from JSON -// using either the UnmarshalJSON() method or the ServerConfigFromJSON() -// function. Hence users are strongly encouraged not to use a literal struct -// initialization to create an instance of this type, but instead unmarshal from -// JSON using one of the two available options. -type ServerConfig struct { - // ServerURI is the management server to connect to. - // - // 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 communicationg with this - // xDS server. It is also used to dedup servers with the same server URI. - Creds ChannelCreds - // ServerFeatures contains a list of features supported by this xDS server. - // It is also used to dedup servers with the same server URI and creds. - ServerFeatures []string - - // As part of unmarshalling the JSON config into this struct, we ensure that - // the credentials config is valid by building an instance of the specified - // credentials and store it here as a grpc.DialOption for easy access when - // dialing this xDS server. - credsDialOption grpc.DialOption - - // IgnoreResourceDeletion controls the behavior of the xDS client when the - // server deletes a previously sent Listener or Cluster resource. If set, the - // xDS client will not invoke the watchers' OnResourceDoesNotExist() method - // when a resource is deleted, nor will it remove the existing resource value - // from its cache. - IgnoreResourceDeletion bool - - // Cleanups are called when the xDS client for this server is closed. Allows - // cleaning up resources created specifically for this ServerConfig. - Cleanups []func() -} - -// CredsDialOption returns the configured credentials as a grpc dial option. -func (sc *ServerConfig) CredsDialOption() grpc.DialOption { - return sc.credsDialOption -} - -// String returns the string representation of the ServerConfig. -// -// This string representation will be used as map keys in federation -// (`map[ServerConfig]authority`), so that the xDS ClientConn and stream will be -// shared by authorities with different names but the same server config. -// -// It covers (almost) all the fields so the string can represent the config -// content. It doesn't cover NodeProto because NodeProto isn't used by -// federation. -func (sc *ServerConfig) String() string { - features := strings.Join(sc.ServerFeatures, "-") - return strings.Join([]string{sc.ServerURI, sc.Creds.String(), features}, "-") -} - -// MarshalJSON marshals the ServerConfig to json. -func (sc ServerConfig) MarshalJSON() ([]byte, error) { - server := xdsServer{ - ServerURI: sc.ServerURI, - ChannelCreds: []channelCreds{{Type: sc.Creds.Type, Config: sc.Creds.Config}}, - ServerFeatures: sc.ServerFeatures, - } - server.ServerFeatures = []string{serverFeaturesV3} - if sc.IgnoreResourceDeletion { - server.ServerFeatures = append(server.ServerFeatures, serverFeaturesIgnoreResourceDeletion) - } - return json.Marshal(server) -} - -// UnmarshalJSON takes the json data (a server) and unmarshals it to the struct. -func (sc *ServerConfig) UnmarshalJSON(data []byte) error { - var server xdsServer - if err := json.Unmarshal(data, &server); err != nil { - return fmt.Errorf("xds: json.Unmarshal(data) for field ServerConfig failed during bootstrap: %v", err) - } - - sc.ServerURI = server.ServerURI - sc.ServerFeatures = server.ServerFeatures - for _, f := range server.ServerFeatures { - if f == serverFeaturesIgnoreResourceDeletion { - sc.IgnoreResourceDeletion = true - } - } - for _, cc := range server.ChannelCreds { - // We stop at the first credential type that we support. - c := bootstrap.GetCredentials(cc.Type) - if c == nil { - continue - } - bundle, cancel, err := c.Build(cc.Config) - if err != nil { - return fmt.Errorf("failed to build credentials bundle from bootstrap for %q: %v", cc.Type, err) - } - sc.Creds = ChannelCreds(cc) - sc.credsDialOption = grpc.WithCredentialsBundle(bundle) - sc.Cleanups = append(sc.Cleanups, cancel) - break - } - return nil -} - -// ServerConfigFromJSON creates a new ServerConfig from the given JSON -// configuration. This is the preferred way of creating a ServerConfig when -// hand-crafting the JSON configuration. -func ServerConfigFromJSON(data []byte) (*ServerConfig, error) { - sc := new(ServerConfig) - if err := sc.UnmarshalJSON(data); err != nil { - return nil, err - } - return sc, nil -} - -// Equal reports whether sc and other are considered equal. -func (sc *ServerConfig) Equal(other *ServerConfig) bool { - switch { - case sc == nil && other == nil: - return true - case (sc != nil) != (other != nil): - return false - case sc.ServerURI != other.ServerURI: - return false - case !sc.Creds.Equal(other.Creds): - return false - case !equalStringSlice(sc.ServerFeatures, other.ServerFeatures): - return false - } - return true -} - -func equalStringSlice(a, b []string) bool { - if len(a) != len(b) { - return false - } - for i := range a { - if a[i] != b[i] { - return false - } - } - return true -} - -// unmarshalJSONServerConfigSlice unmarshals JSON to a slice. -func unmarshalJSONServerConfigSlice(data []byte) ([]*ServerConfig, error) { - var servers []*ServerConfig - if err := json.Unmarshal(data, &servers); err != nil { - return nil, fmt.Errorf("failed to unmarshal JSON to []*ServerConfig: %v", err) - } - if len(servers) < 1 { - return nil, fmt.Errorf("no management server found in JSON") - } - return servers, 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. +// This type does not implement custom JSON marshal/unmarshal logic because it +// is straightforward to accomplish the same with json struct tags. type Authority struct { // ClientListenerResourceNameTemplate is template for the name of the // Listener resource to subscribe to for a gRPC client channel. Used only @@ -259,123 +102,413 @@ type Authority struct { // // 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 + ClientListenerResourceNameTemplate string `json:"client_listener_resource_name_template,omitempty"` + // XDSServers contains the list of server configurations for this authority. + XDSServers []*ServerConfig `json:"xds_servers,omitempty"` } -// 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) +// Equal returns true if a equals other. +func (a *Authority) Equal(other *Authority) bool { + switch { + case a == nil && other == nil: + return true + case (a != nil) != (other != nil): + return false + case a.ClientListenerResourceNameTemplate != other.ClientListenerResourceNameTemplate: + return false + case !slices.EqualFunc(a.XDSServers, other.XDSServers, func(a, b *ServerConfig) bool { return a.Equal(b) }): + return false + } + return true +} + +// ServerConfig contains the configuration to connect to a server. +type ServerConfig struct { + serverURI string + channelCreds []ChannelCreds + serverFeatures []string + + // As part of unmarshalling the JSON config into this struct, we ensure that + // the credentials config is valid by building an instance of the specified + // credentials and store it here for easy access. + selectedCreds ChannelCreds + credsDialOption grpc.DialOption + + cleanups []func() +} + +// ServerURI returns the URI of the management server to connect to. +func (sc *ServerConfig) ServerURI() string { + return sc.serverURI +} + +// ChannelCreds returns the credentials configuration to use when communicating +// with this server. Also used to dedup servers with the same server URI. +func (sc *ServerConfig) ChannelCreds() []ChannelCreds { + return sc.channelCreds +} + +// ServerFeatures returns the list of features supported by this server. Also +// used to dedup servers with the same server URI and channel creds. +func (sc *ServerConfig) ServerFeatures() []string { + return sc.serverFeatures +} + +// ServerFeaturesIgnoreResourceDeletion returns true if this server supports a +// feature where the xDS client can ignore resource deletions from this server, +// as described in gRFC A53. +// +// This feature controls the behavior of the xDS client when the server deletes +// a previously sent Listener or Cluster resource. If set, the xDS client will +// not invoke the watchers' OnResourceDoesNotExist() method when a resource is +// deleted, nor will it remove the existing resource value from its cache. +func (sc *ServerConfig) ServerFeaturesIgnoreResourceDeletion() bool { + for _, sf := range sc.serverFeatures { + if sf == serverFeaturesIgnoreResourceDeletion { + return true + } + } + return false +} + +// CredsDialOption returns the first supported transport credentials from the +// configuration, as a dial option. +func (sc *ServerConfig) CredsDialOption() grpc.DialOption { + return sc.credsDialOption +} + +// Cleanups returns a collection of functions to be called when the xDS client +// for this server is closed. Allows cleaning up resources created specifically +// for this server. +func (sc *ServerConfig) Cleanups() []func() { + return sc.cleanups +} + +// Equal reports whether sc and other are considered equal. +func (sc *ServerConfig) Equal(other *ServerConfig) bool { + switch { + case sc == nil && other == nil: + return true + case (sc != nil) != (other != nil): + return false + case sc.serverURI != other.serverURI: + return false + case !slices.EqualFunc(sc.channelCreds, other.channelCreds, func(a, b ChannelCreds) bool { return a.Equal(b) }): + return false + case !slices.Equal(sc.serverFeatures, other.serverFeatures): + return false + case !sc.selectedCreds.Equal(other.selectedCreds): + return false + } + return true +} + +// String returns the string representation of the ServerConfig. +// +// This string representation will be used as map keys in federation +// (`map[ServerConfig]authority`), so that the xDS ClientConn and stream will be +// shared by authorities with different names but the same server config. +// +// It covers (almost) all the fields so the string can represent the config +// content. It doesn't cover NodeProto because NodeProto isn't used by +// federation. +func (sc *ServerConfig) String() string { + features := strings.Join(sc.serverFeatures, "-") + return strings.Join([]string{sc.serverURI, sc.selectedCreds.String(), features}, "-") +} + +// The following fields correspond 1:1 with the JSON schema for ServerConfig. +type serverConfigJSON struct { + ServerURI string `json:"server_uri"` + ChannelCreds []ChannelCreds `json:"channel_creds"` + ServerFeatures []string `json:"server_features"` +} + +// MarshalJSON returns marshaled JSON bytes corresponding to this server config. +func (sc *ServerConfig) MarshalJSON() ([]byte, error) { + server := &serverConfigJSON{ + ServerURI: sc.serverURI, + ChannelCreds: sc.channelCreds, + ServerFeatures: sc.serverFeatures, + } + return json.Marshal(server) +} + +// UnmarshalJSON takes the json data (a server) and unmarshals it to the struct. +func (sc *ServerConfig) UnmarshalJSON(data []byte) error { + server := serverConfigJSON{} + if err := json.Unmarshal(data, &server); err != nil { + return fmt.Errorf("xds: failed to JSON unmarshal server configuration during bootstrap: %v, config:\n%s", err, string(data)) } - for k, v := range jsonData { - switch k { - case "xds_servers": - servers, err := unmarshalJSONServerConfigSlice(v) - if err != nil { - return fmt.Errorf("xds: json.Unmarshal(data) for field %q failed during bootstrap: %v", k, err) - } - a.XDSServer = servers[0] - 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) - } + sc.serverURI = server.ServerURI + sc.channelCreds = server.ChannelCreds + sc.serverFeatures = server.ServerFeatures + + for _, cc := range server.ChannelCreds { + // We stop at the first credential type that we support. + c := bootstrap.GetCredentials(cc.Type) + if c == nil { + continue + } + bundle, cancel, err := c.Build(cc.Config) + if err != nil { + return fmt.Errorf("failed to build credentials bundle from bootstrap for %q: %v", cc.Type, err) + } + sc.selectedCreds = cc + sc.credsDialOption = grpc.WithCredentialsBundle(bundle) + sc.cleanups = append(sc.cleanups, cancel) + break + } + if sc.serverURI == "" { + return fmt.Errorf("xds: `server_uri` field in server config cannot be empty: %s", string(data)) + } + if sc.credsDialOption == nil { + return fmt.Errorf("xds: `channel_creds` field in server config cannot be empty: %s", string(data)) + } + return nil +} + +// ServerConfigTestingOptions specifies options for creating a new ServerConfig +// for testing purposes. +// +// # Testing-Only +type ServerConfigTestingOptions struct { + // URI is the name of the server corresponding to this server config. + URI string + // ChannelCreds contains a list of channel credentials to use when talking + // to this server. If unspecified, `insecure` credentials will be used. + ChannelCreds []ChannelCreds + // ServerFeatures represents the list of features supported by this server. + ServerFeatures []string +} + +// ServerConfigForTesting creates a new ServerConfig from the passed in options, +// for testing purposes. +// +// # Testing-Only +func ServerConfigForTesting(opts ServerConfigTestingOptions) (*ServerConfig, error) { + cc := opts.ChannelCreds + if cc == nil { + cc = []ChannelCreds{{Type: "insecure"}} + } + scInternal := &serverConfigJSON{ + ServerURI: opts.URI, + ChannelCreds: cc, + ServerFeatures: opts.ServerFeatures, + } + scJSON, err := json.Marshal(scInternal) + if err != nil { + return nil, err + } + + sc := new(ServerConfig) + if err := sc.UnmarshalJSON(scJSON); err != nil { + return nil, err + } + return sc, nil +} + +// Config is the internal representation of the bootstrap configuration provided +// to the xDS client. +type Config struct { + xDSServers []*ServerConfig + cpcs map[string]certproviderNameAndConfig + serverListenerResourceNameTemplate string + clientDefaultListenerResourceNameTemplate string + authorities map[string]*Authority + node node + + // A map from certprovider instance names to parsed buildable configs. + certProviderConfigs map[string]*certprovider.BuildableConfig +} + +// XDSServers returns the top-level list of management servers to connect to, +// ordered by priority. +func (c *Config) XDSServers() []*ServerConfig { + return c.xDSServers +} + +// CertProviderConfigs returns a map from certificate provider plugin instance +// name to their configuration. Callers must not modify the returned map. +func (c *Config) CertProviderConfigs() map[string]*certprovider.BuildableConfig { + return c.certProviderConfigs +} + +// ServerListenerResourceNameTemplate returns template for the name of the +// 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. +func (c *Config) ServerListenerResourceNameTemplate() string { + return c.serverListenerResourceNameTemplate +} + +// ClientDefaultListenerResourceNameTemplate returns 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". +func (c *Config) ClientDefaultListenerResourceNameTemplate() string { + return c.clientDefaultListenerResourceNameTemplate +} + +// Authorities returns a map of authority name to corresponding configuration. +// Callers must not modify the returned map. +// +// 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. +func (c *Config) Authorities() map[string]*Authority { + return c.authorities +} + +// Node returns xDS a v3 Node proto corresponding to the node field in the +// bootstrap configuration, which identifies a specific gRPC instance. +func (c *Config) Node() *v3corepb.Node { + return c.node.toProto() +} + +// Equal returns true if c equals other. +func (c *Config) Equal(other *Config) bool { + switch { + case c == nil && other == nil: + return true + case (c != nil) != (other != nil): + return false + case !slices.EqualFunc(c.xDSServers, other.xDSServers, func(a, b *ServerConfig) bool { return a.Equal(b) }): + return false + case !maps.EqualFunc(c.certProviderConfigs, other.certProviderConfigs, func(a, b *certprovider.BuildableConfig) bool { return a.String() == b.String() }): + return false + case c.serverListenerResourceNameTemplate != other.serverListenerResourceNameTemplate: + return false + case c.clientDefaultListenerResourceNameTemplate != other.clientDefaultListenerResourceNameTemplate: + return false + case !maps.EqualFunc(c.authorities, other.authorities, func(a, b *Authority) bool { return a.Equal(b) }): + return false + case !c.node.Equal(other.node): + return false + } + return true +} + +// The following fields correspond 1:1 with the JSON schema for Config. +type configJSON struct { + XDSServers []*ServerConfig `json:"xds_servers,omitempty"` + CertificateProviders map[string]certproviderNameAndConfig `json:"certificate_providers,omitempty"` + ServerListenerResourceNameTemplate string `json:"server_listener_resource_name_template,omitempty"` + ClientDefaultListenerResourceNameTemplate string `json:"client_default_listener_resource_name_template,omitempty"` + Authorities map[string]*Authority `json:"authorities,omitempty"` + Node node `json:"node,omitempty"` +} + +// MarshalJSON returns marshaled JSON bytes corresponding to this config. +func (c *Config) MarshalJSON() ([]byte, error) { + config := &configJSON{ + XDSServers: c.xDSServers, + CertificateProviders: c.cpcs, + ServerListenerResourceNameTemplate: c.serverListenerResourceNameTemplate, + ClientDefaultListenerResourceNameTemplate: c.clientDefaultListenerResourceNameTemplate, + Authorities: c.authorities, + Node: c.node, + } + return json.Marshal(config) +} + +// UnmarshalJSON takes the json data (the complete bootstrap configuration) and +// unmarshals it to the struct. +func (c *Config) UnmarshalJSON(data []byte) error { + // Initialize the node field with client controlled values. This ensures + // even if the bootstrap configuration did not contain the node field, we + // will have a node field with client controlled fields alone. + config := configJSON{Node: newNode()} + if err := json.Unmarshal(data, &config); err != nil { + return fmt.Errorf("xds: json.Unmarshal(%s) failed during bootstrap: %v", string(data), err) + } + + c.xDSServers = config.XDSServers + c.cpcs = config.CertificateProviders + c.serverListenerResourceNameTemplate = config.ServerListenerResourceNameTemplate + c.clientDefaultListenerResourceNameTemplate = config.ClientDefaultListenerResourceNameTemplate + c.authorities = config.Authorities + c.node = config.Node + + // Build the certificate providers configuration to ensure that it is valid. + cpcCfgs := make(map[string]*certprovider.BuildableConfig) + getBuilder := internal.GetCertificateProviderBuilder.(func(string) certprovider.Builder) + for instance, nameAndConfig := range c.cpcs { + name := nameAndConfig.PluginName + parser := getBuilder(nameAndConfig.PluginName) + if parser == nil { + // We ignore plugins that we do not know about. + continue + } + bc, err := parser.ParseConfig(nameAndConfig.Config) + if err != nil { + return fmt.Errorf("xds: config parsing for certifcate provider plugin %q failed during bootstrap: %v", name, err) + } + cpcCfgs[instance] = bc + } + c.certProviderConfigs = cpcCfgs + + // Default value of the default client listener name template is "%s". + if c.clientDefaultListenerResourceNameTemplate == "" { + c.clientDefaultListenerResourceNameTemplate = "%s" + } + if len(c.xDSServers) == 0 { + return fmt.Errorf("xds: required field `xds_servers` not found in bootstrap configuration: %s", string(data)) + } + + // 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 c.authorities { + prefix := fmt.Sprintf("xdstp://%s", url.PathEscape(name)) + if authority.ClientListenerResourceNameTemplate == "" { + authority.ClientListenerResourceNameTemplate = prefix + "/envoy.config.listener.v3.Listener/%s" + continue + } + if !strings.HasPrefix(authority.ClientListenerResourceNameTemplate, prefix) { + return fmt.Errorf("xds: field clientListenerResourceNameTemplate %q of authority %q doesn't start with prefix %q", authority.ClientListenerResourceNameTemplate, name, prefix) } } 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. -// -// Users must use one of the NewConfigXxx() functions to create a Config -// instance, and not initialize it manually. -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 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 - // NodeProto contains the Node proto to be used in xDS requests. This will be - // of type *v3corepb.Node. - NodeProto *v3corepb.Node -} - -type channelCreds struct { - Type string `json:"type"` - Config json.RawMessage `json:"config,omitempty"` -} - -type xdsServer struct { - ServerURI string `json:"server_uri"` - ChannelCreds []channelCreds `json:"channel_creds"` - ServerFeatures []string `json:"server_features"` -} - +// Returns the bootstrap configuration from env vars ${GRPC_XDS_BOOTSTRAP} or +// ${GRPC_XDS_BOOTSTRAP_CONFIG}. If both env vars are set, the former is +// preferred. And if none of the env vars are set, an error is returned. func bootstrapConfigFromEnvVariable() ([]byte, error) { fName := envconfig.XDSBootstrapFileName fContent := envconfig.XDSBootstrapFileContent - // Bootstrap file name has higher priority than bootstrap content. if fName != "" { - // If file name is set - // - If file not found (or other errors), fail - // - Otherwise, use the content. - // - // Note that even if the content is invalid, we don't failover to the - // file content env variable. logger.Debugf("Using bootstrap file with name %q", fName) return bootstrapFileReadFunc(fName) } @@ -384,8 +517,7 @@ func bootstrapConfigFromEnvVariable() ([]byte, error) { return []byte(fContent), nil } - return nil, fmt.Errorf("none of the bootstrap environment variables (%q or %q) defined", - envconfig.XDSBootstrapFileNameEnv, envconfig.XDSBootstrapFileContentEnv) + return nil, fmt.Errorf("none of the bootstrap environment variables (%q or %q) defined", envconfig.XDSBootstrapFileNameEnv, envconfig.XDSBootstrapFileContentEnv) } // NewConfig returns a new instance of Config initialized by reading the @@ -418,116 +550,126 @@ func NewConfigFromContents(data []byte) (*Config, error) { } func newConfigFromContents(data []byte) (*Config, error) { + // Normalize the input configuration. + buf := bytes.Buffer{} + err := json.Indent(&buf, data, "", "") + if err != nil { + return nil, fmt.Errorf("xds: error normalizing JSON bootstrap configuration: %v", err) + } + data = bytes.TrimSpace(buf.Bytes()) + config := &Config{} - - var jsonData map[string]json.RawMessage - if err := json.Unmarshal(data, &jsonData); err != nil { - return nil, fmt.Errorf("xds: failed to parse bootstrap config: %v", err) + if err := config.UnmarshalJSON(data); err != nil { + return nil, err } - var node *v3corepb.Node - opts := protojson.UnmarshalOptions{DiscardUnknown: true} - for k, v := range jsonData { - switch k { - case "node": - node = &v3corepb.Node{} - if err := opts.Unmarshal(v, node); err != nil { - return nil, fmt.Errorf("xds: protojson.Unmarshal(%v) for field %q failed during bootstrap: %v", string(v), k, err) - } - case "xds_servers": - servers, err := unmarshalJSONServerConfigSlice(v) - if err != nil { - return nil, fmt.Errorf("xds: json.Unmarshal(data) for field %q failed during bootstrap: %v", k, err) - } - config.XDSServer = servers[0] - case "certificate_providers": - var providerInstances map[string]json.RawMessage - if err := json.Unmarshal(v, &providerInstances); err != nil { - return nil, fmt.Errorf("xds: json.Unmarshal(%v) for field %q failed during bootstrap: %v", string(v), k, err) - } - configs := make(map[string]*certprovider.BuildableConfig) - getBuilder := internal.GetCertificateProviderBuilder.(func(string) certprovider.Builder) - for instance, data := range providerInstances { - var nameAndConfig struct { - PluginName string `json:"plugin_name"` - Config json.RawMessage `json:"config"` - } - if err := json.Unmarshal(data, &nameAndConfig); err != nil { - return nil, fmt.Errorf("xds: json.Unmarshal(%v) for field %q failed during bootstrap: %v", string(v), instance, err) - } - - name := nameAndConfig.PluginName - parser := getBuilder(nameAndConfig.PluginName) - if parser == nil { - // We ignore plugins that we do not know about. - continue - } - bc, err := parser.ParseConfig(nameAndConfig.Config) - if err != nil { - return nil, fmt.Errorf("xds: config parsing for plugin %q failed: %v", name, err) - } - configs[instance] = bc - } - config.CertProviderConfigs = configs - case "server_listener_resource_name_template": - 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) - } - case "client_default_listener_resource_name_template": - if err := json.Unmarshal(v, &config.ClientDefaultListenerResourceNameTemplate); err != nil { - return nil, fmt.Errorf("xds: json.Unmarshal(%v) for field %q failed during bootstrap: %v", string(v), k, err) - } - case "authorities": - if err := json.Unmarshal(v, &config.Authorities); 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.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.XDSServer.CredsDialOption() == nil { - return nil, fmt.Errorf("xds: required field %q doesn't contain valid value in bootstrap %s", "xds_servers.channel_creds", jsonData["xds_servers"]) - } - // 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", url.PathEscape(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) - } - } - - // Performing post-production on the node information. Some additional fields - // which are not expected to be set in the bootstrap file are populated here. - if node == nil { - node = &v3corepb.Node{} - } - node.UserAgentName = gRPCUserAgentName - node.UserAgentVersionType = &v3corepb.Node_UserAgentVersion{UserAgentVersion: grpc.Version} - node.ClientFeatures = append(node.ClientFeatures, clientFeatureNoOverprovisioning, clientFeatureResourceWrapper) - config.NodeProto = node - if logger.V(2) { logger.Infof("Bootstrap config for creating xds-client: %v", pretty.ToJSON(config)) + } else { + logger.Infof("Bootstrap config for creating xds-client: %+v", config) } return config, nil } + +// certproviderNameAndConfig is the internal representation of +// the`certificate_providers` field in the bootstrap configuration. +type certproviderNameAndConfig struct { + PluginName string `json:"plugin_name"` + Config json.RawMessage `json:"config"` +} + +// locality is the internal representation of the locality field within node. +type locality struct { + Region string `json:"region,omitempty"` + Zone string `json:"zone,omitempty"` + SubZone string `json:"sub_zone,omitempty"` +} + +func (l locality) Equal(other locality) bool { + return l.Region == other.Region && l.Zone == other.Zone && l.SubZone == other.SubZone +} + +func (l locality) isEmpty() bool { + return l.Equal(locality{}) +} + +type userAgentVersion struct { + UserAgentVersion string `json:"user_agent_version,omitempty"` +} + +// node is the internal representation of the node field in the bootstrap +// configuration. +type node struct { + ID string `json:"id,omitempty"` + Cluster string `json:"cluster,omitempty"` + Locality locality `json:"locality,omitempty"` + Metadata *structpb.Struct `json:"metadata,omitempty"` + + // The following fields are controlled by the client implementation and + // should not unmarshaled from JSON. + userAgentName string + userAgentVersionType userAgentVersion + clientFeatures []string +} + +// newNode is a convenience function to create a new node instance with fields +// controlled by the client implementation set to the desired values. +func newNode() node { + return node{ + userAgentName: gRPCUserAgentName, + userAgentVersionType: userAgentVersion{UserAgentVersion: grpc.Version}, + clientFeatures: []string{clientFeatureNoOverprovisioning, clientFeatureResourceWrapper}, + } +} + +func (n node) Equal(other node) bool { + switch { + case n.ID != other.ID: + return false + case n.Cluster != other.Cluster: + return false + case !n.Locality.Equal(other.Locality): + return false + case n.userAgentName != other.userAgentName: + return false + case n.userAgentVersionType != other.userAgentVersionType: + return false + } + + // Consider failures in JSON marshaling as being unable to perform the + // comparison, and hence return false. + nMetadata, err := n.Metadata.MarshalJSON() + if err != nil { + return false + } + otherMetadata, err := other.Metadata.MarshalJSON() + if err != nil { + return false + } + if !bytes.Equal(nMetadata, otherMetadata) { + return false + } + + return slices.Equal(n.clientFeatures, other.clientFeatures) +} + +func (n node) toProto() *v3corepb.Node { + return &v3corepb.Node{ + Id: n.ID, + Cluster: n.Cluster, + Locality: func() *v3corepb.Locality { + if n.Locality.isEmpty() { + return nil + } + return &v3corepb.Locality{ + Region: n.Locality.Region, + Zone: n.Locality.Zone, + SubZone: n.Locality.SubZone, + } + }(), + Metadata: proto.Clone(n.Metadata).(*structpb.Struct), + UserAgentName: n.userAgentName, + UserAgentVersionType: &v3corepb.Node_UserAgentVersion{UserAgentVersion: n.userAgentVersionType.UserAgentVersion}, + ClientFeatures: slices.Clone(n.clientFeatures), + } +} diff --git a/internal/xds/bootstrap/bootstrap_test.go b/internal/xds/bootstrap/bootstrap_test.go index 68a4536d3..f0e48e6e3 100644 --- a/internal/xds/bootstrap/bootstrap_test.go +++ b/internal/xds/bootstrap/bootstrap_test.go @@ -25,17 +25,16 @@ import ( "os" "testing" + v3corepb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" "github.com/google/go-cmp/cmp" - "github.com/google/go-cmp/cmp/cmpopts" "google.golang.org/grpc" "google.golang.org/grpc/credentials/tls/certprovider" "google.golang.org/grpc/internal" "google.golang.org/grpc/internal/envconfig" + "google.golang.org/grpc/internal/grpctest" "google.golang.org/grpc/xds/bootstrap" - "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/testing/protocmp" "google.golang.org/protobuf/types/known/structpb" - - v3corepb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" ) var ( @@ -177,7 +176,7 @@ var ( }, { "server_uri": "backup.never.use.com:1234", - "channel_creds": [{ "type": "not-google-default" }] + "channel_creds": [{ "type": "google_default" }] } ] }`, @@ -205,62 +204,80 @@ var ( }, }, } - v3NodeProto = &v3corepb.Node{ - Id: "ENVOY_NODE_ID", + v3Node = node{ + ID: "ENVOY_NODE_ID", Metadata: metadata, - UserAgentName: gRPCUserAgentName, - UserAgentVersionType: &v3corepb.Node_UserAgentVersion{UserAgentVersion: grpc.Version}, - ClientFeatures: []string{clientFeatureNoOverprovisioning, clientFeatureResourceWrapper}, + userAgentName: gRPCUserAgentName, + userAgentVersionType: userAgentVersion{UserAgentVersion: grpc.Version}, + clientFeatures: []string{clientFeatureNoOverprovisioning, clientFeatureResourceWrapper}, } - nilCredsConfigNoServerFeatures = &Config{ - XDSServer: &ServerConfig{ - ServerURI: "trafficdirector.googleapis.com:443", - Creds: ChannelCreds{Type: "insecure"}, - }, - NodeProto: v3NodeProto, - ClientDefaultListenerResourceNameTemplate: "%s", + configWithInsecureCreds = &Config{ + xDSServers: []*ServerConfig{{ + serverURI: "trafficdirector.googleapis.com:443", + channelCreds: []ChannelCreds{{Type: "insecure"}}, + selectedCreds: ChannelCreds{Type: "insecure"}, + }}, + node: v3Node, + clientDefaultListenerResourceNameTemplate: "%s", } - nonNilCredsConfigV3 = &Config{ - XDSServer: &ServerConfig{ - ServerURI: "trafficdirector.googleapis.com:443", - Creds: ChannelCreds{Type: "google_default"}, - ServerFeatures: []string{"xds_v3"}, - }, - NodeProto: v3NodeProto, - ClientDefaultListenerResourceNameTemplate: "%s", + configWithMultipleChannelCredsAndV3 = &Config{ + xDSServers: []*ServerConfig{{ + serverURI: "trafficdirector.googleapis.com:443", + channelCreds: []ChannelCreds{{Type: "not-google-default"}, {Type: "google_default"}}, + serverFeatures: []string{"xds_v3"}, + selectedCreds: ChannelCreds{Type: "google_default"}, + }}, + node: v3Node, + clientDefaultListenerResourceNameTemplate: "%s", } - nonNilCredsConfigWithDeletionIgnored = &Config{ - XDSServer: &ServerConfig{ - ServerURI: "trafficdirector.googleapis.com:443", - Creds: ChannelCreds{Type: "google_default"}, - IgnoreResourceDeletion: true, - ServerFeatures: []string{"ignore_resource_deletion", "xds_v3"}, - }, - NodeProto: v3NodeProto, - ClientDefaultListenerResourceNameTemplate: "%s", + configWithGoogleDefaultCredsAndV3 = &Config{ + xDSServers: []*ServerConfig{{ + serverURI: "trafficdirector.googleapis.com:443", + channelCreds: []ChannelCreds{{Type: "google_default"}}, + serverFeatures: []string{"xds_v3"}, + selectedCreds: ChannelCreds{Type: "google_default"}, + }}, + node: v3Node, + clientDefaultListenerResourceNameTemplate: "%s", } - nonNilCredsConfigNoServerFeatures = &Config{ - XDSServer: &ServerConfig{ - ServerURI: "trafficdirector.googleapis.com:443", - Creds: ChannelCreds{Type: "google_default"}, + configWithMultipleServers = &Config{ + xDSServers: []*ServerConfig{ + { + serverURI: "trafficdirector.googleapis.com:443", + channelCreds: []ChannelCreds{{Type: "google_default"}}, + serverFeatures: []string{"xds_v3"}, + selectedCreds: ChannelCreds{Type: "google_default"}, + }, + { + serverURI: "backup.never.use.com:1234", + channelCreds: []ChannelCreds{{Type: "google_default"}}, + selectedCreds: ChannelCreds{Type: "google_default"}, + }, }, - NodeProto: v3NodeProto, - ClientDefaultListenerResourceNameTemplate: "%s", + node: v3Node, + clientDefaultListenerResourceNameTemplate: "%s", + } + configWithGoogleDefaultCredsAndIgnoreResourceDeletion = &Config{ + xDSServers: []*ServerConfig{{ + serverURI: "trafficdirector.googleapis.com:443", + channelCreds: []ChannelCreds{{Type: "google_default"}}, + serverFeatures: []string{"ignore_resource_deletion", "xds_v3"}, + selectedCreds: ChannelCreds{Type: "google_default"}, + }}, + node: v3Node, + clientDefaultListenerResourceNameTemplate: "%s", + } + configWithGoogleDefaultCredsAndNoServerFeatures = &Config{ + xDSServers: []*ServerConfig{{ + serverURI: "trafficdirector.googleapis.com:443", + channelCreds: []ChannelCreds{{Type: "google_default"}}, + selectedCreds: ChannelCreds{Type: "google_default"}, + }}, + node: v3Node, + clientDefaultListenerResourceNameTemplate: "%s", } ) -func (c *Config) compare(want *Config) error { - if diff := cmp.Diff(want, c, - cmpopts.EquateEmpty(), - 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("unexpected diff in config (-want, +got):\n%s", diff) - } - return nil -} - func fileReadFromFileMap(bootstrapFileMap map[string]string, name string) ([]byte, error) { if b, ok := bootstrapFileMap[name]; ok { return []byte(b), nil @@ -293,8 +310,8 @@ func testNewConfigWithFileNameEnv(t *testing.T, fileName string, wantError bool, if wantError { return } - if err := c.compare(wantConfig); err != nil { - t.Fatal(err) + if diff := cmp.Diff(wantConfig, c); diff != "" { + t.Fatalf("Unexpected diff in bootstrap configuration (-want, +got):\n%s", diff) } } @@ -317,14 +334,13 @@ func testNewConfigWithFileContentEnv(t *testing.T, fileName string, wantError bo if wantError { return } - if err := c.compare(wantConfig); err != nil { - t.Fatal(err) + if diff := cmp.Diff(wantConfig, c); diff != "" { + t.Fatalf("Unexpected diff in bootstrap configuration (-want, +got):\n%s", diff) } } -// TestNewConfigV3ProtoFailure exercises the functionality in NewConfig with -// different bootstrap file contents which are expected to fail. -func TestNewConfigV3ProtoFailure(t *testing.T) { +// Tests NewConfig with bootstrap file contents that are expected to fail. +func (s) TestNewConfig_Failure(t *testing.T) { bootstrapFileMap := map[string]string{ "empty": "", "badJSON": `["test": 123]`, @@ -369,21 +385,10 @@ func TestNewConfigV3ProtoFailure(t *testing.T) { cancel := setupBootstrapOverride(bootstrapFileMap) defer cancel() - tests := []struct { - name string - wantError bool - }{ - {"nonExistentBootstrapFile", true}, - {"empty", true}, - {"badJSON", true}, - {"noBalancerName", true}, - {"emptyXdsServer", true}, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - testNewConfigWithFileNameEnv(t, test.name, true, nil) - testNewConfigWithFileContentEnv(t, test.name, true, nil) + for _, name := range []string{"nonExistentBootstrapFile", "empty", "badJSON", "noBalancerName", "emptyXdsServer"} { + t.Run(name, func(t *testing.T) { + testNewConfigWithFileNameEnv(t, name, true, nil) + testNewConfigWithFileContentEnv(t, name, true, nil) }) } } @@ -391,7 +396,7 @@ func TestNewConfigV3ProtoFailure(t *testing.T) { // TestNewConfigV3ProtoSuccess exercises the functionality in NewConfig with // different bootstrap file contents. It overrides the fileReadFunc by returning // bootstrap file contents defined in this test, instead of reading from a file. -func TestNewConfigV3ProtoSuccess(t *testing.T) { +func (s) TestNewConfig_Success(t *testing.T) { cancel := setupBootstrapOverride(v3BootstrapFileMap) defer cancel() @@ -400,26 +405,28 @@ func TestNewConfigV3ProtoSuccess(t *testing.T) { wantConfig *Config }{ { - "emptyNodeProto", &Config{ - XDSServer: &ServerConfig{ - ServerURI: "trafficdirector.googleapis.com:443", - Creds: ChannelCreds{Type: "insecure"}, + name: "emptyNodeProto", + wantConfig: &Config{ + xDSServers: []*ServerConfig{{ + serverURI: "trafficdirector.googleapis.com:443", + channelCreds: []ChannelCreds{{Type: "insecure"}}, + selectedCreds: ChannelCreds{Type: "insecure"}, + }}, + node: node{ + userAgentName: gRPCUserAgentName, + userAgentVersionType: userAgentVersion{UserAgentVersion: grpc.Version}, + clientFeatures: []string{clientFeatureNoOverprovisioning, clientFeatureResourceWrapper}, }, - NodeProto: &v3corepb.Node{ - UserAgentName: gRPCUserAgentName, - UserAgentVersionType: &v3corepb.Node_UserAgentVersion{UserAgentVersion: grpc.Version}, - ClientFeatures: []string{clientFeatureNoOverprovisioning, clientFeatureResourceWrapper}, - }, - ClientDefaultListenerResourceNameTemplate: "%s", + clientDefaultListenerResourceNameTemplate: "%s", }, }, - {"unknownTopLevelFieldInFile", nilCredsConfigNoServerFeatures}, - {"unknownFieldInNodeProto", nilCredsConfigNoServerFeatures}, - {"unknownFieldInXdsServer", nilCredsConfigNoServerFeatures}, - {"multipleChannelCreds", nonNilCredsConfigV3}, - {"goodBootstrap", nonNilCredsConfigV3}, - {"multipleXDSServers", nonNilCredsConfigV3}, - {"serverSupportsIgnoreResourceDeletion", nonNilCredsConfigWithDeletionIgnored}, + {"unknownTopLevelFieldInFile", configWithInsecureCreds}, + {"unknownFieldInNodeProto", configWithInsecureCreds}, + {"unknownFieldInXdsServer", configWithInsecureCreds}, + {"multipleChannelCreds", configWithMultipleChannelCredsAndV3}, + {"goodBootstrap", configWithGoogleDefaultCredsAndV3}, + {"multipleXDSServers", configWithMultipleServers}, + {"serverSupportsIgnoreResourceDeletion", configWithGoogleDefaultCredsAndIgnoreResourceDeletion}, } for _, test := range tests { @@ -436,7 +443,7 @@ func TestNewConfigV3ProtoSuccess(t *testing.T) { // "GRPC_XDS_BOOTSTRAP" which specifies the file name containing the bootstrap // configuration takes precedence over "GRPC_XDS_BOOTSTRAP_CONFIG", which // directly specifies the bootstrap configuration in itself. -func TestNewConfigBootstrapEnvPriority(t *testing.T) { +func (s) TestNewConfigBootstrapEnvPriority(t *testing.T) { oldFileReadFunc := bootstrapFileReadFunc bootstrapFileReadFunc = func(filename string) ([]byte, error) { return fileReadFromFileMap(v3BootstrapFileMap, filename) @@ -444,11 +451,11 @@ func TestNewConfigBootstrapEnvPriority(t *testing.T) { defer func() { bootstrapFileReadFunc = oldFileReadFunc }() goodFileName1 := "serverFeaturesIncludesXDSV3" - goodConfig1 := nonNilCredsConfigV3 + goodConfig1 := configWithGoogleDefaultCredsAndV3 goodFileName2 := "serverFeaturesExcludesXDSV3" goodFileContent2 := v3BootstrapFileMap[goodFileName2] - goodConfig2 := nonNilCredsConfigNoServerFeatures + goodConfig2 := configWithGoogleDefaultCredsAndNoServerFeatures origBootstrapFileName := envconfig.XDSBootstrapFileName envconfig.XDSBootstrapFileName = "" @@ -470,8 +477,8 @@ func TestNewConfigBootstrapEnvPriority(t *testing.T) { if err != nil { t.Errorf("NewConfig() failed: %v", err) } - if err := c.compare(goodConfig1); err != nil { - t.Error(err) + if diff := cmp.Diff(goodConfig1, c); diff != "" { + t.Errorf("Unexpected diff in bootstrap configuration (-want, +got):\n%s", diff) } envconfig.XDSBootstrapFileName = "" @@ -480,8 +487,8 @@ func TestNewConfigBootstrapEnvPriority(t *testing.T) { if err != nil { t.Errorf("NewConfig() failed: %v", err) } - if err := c.compare(goodConfig2); err != nil { - t.Error(err) + if diff := cmp.Diff(goodConfig2, c); diff != "" { + t.Errorf("Unexpected diff in bootstrap configuration (-want, +got):\n%s", diff) } // Set both, file name should be read. @@ -491,8 +498,8 @@ func TestNewConfigBootstrapEnvPriority(t *testing.T) { if err != nil { t.Errorf("NewConfig() failed: %v", err) } - if err := c.compare(goodConfig1); err != nil { - t.Error(err) + if diff := cmp.Diff(goodConfig1, c); diff != "" { + t.Errorf("Unexpected diff in bootstrap configuration (-want, +got):\n%s", diff) } } @@ -547,7 +554,7 @@ type fakeCertProvider struct { certprovider.Provider } -func TestNewConfigWithCertificateProviders(t *testing.T) { +func (s) TestNewConfigWithCertificateProviders(t *testing.T) { bootstrapFileMap := map[string]string{ "badJSONCertProviderConfig": ` { @@ -649,7 +656,7 @@ func TestNewConfigWithCertificateProviders(t *testing.T) { getBuilder := internal.GetCertificateProviderBuilder.(func(string) certprovider.Builder) parser := getBuilder(fakeCertProviderName) if parser == nil { - t.Fatalf("missing certprovider plugin %q", fakeCertProviderName) + t.Fatalf("Missing certprovider plugin %q", fakeCertProviderName) } wantCfg, err := parser.ParseConfig(json.RawMessage(`{"configKey": "configValue"}`)) if err != nil { @@ -659,24 +666,18 @@ func TestNewConfigWithCertificateProviders(t *testing.T) { cancel := setupBootstrapOverride(bootstrapFileMap) defer cancel() - // Cannot use xdstestutils.ServerConfigForAddress here, as it would lead to - // a cyclic dependency. - jsonCfg := `{ - "server_uri": "trafficdirector.googleapis.com:443", - "channel_creds": [{"type": "insecure"}], - "server_features": ["xds_v3"] - }` - serverCfg, err := ServerConfigFromJSON([]byte(jsonCfg)) - if err != nil { - t.Fatalf("Failed to create server config from JSON %s: %v", jsonCfg, err) - } goodConfig := &Config{ - XDSServer: serverCfg, - NodeProto: v3NodeProto, - CertProviderConfigs: map[string]*certprovider.BuildableConfig{ + xDSServers: []*ServerConfig{{ + serverURI: "trafficdirector.googleapis.com:443", + channelCreds: []ChannelCreds{{Type: "insecure"}}, + serverFeatures: []string{"xds_v3"}, + selectedCreds: ChannelCreds{Type: "insecure"}, + }}, + certProviderConfigs: map[string]*certprovider.BuildableConfig{ "fakeProviderInstance": wantCfg, }, - ClientDefaultListenerResourceNameTemplate: "%s", + clientDefaultListenerResourceNameTemplate: "%s", + node: v3Node, } tests := []struct { name string @@ -695,7 +696,7 @@ func TestNewConfigWithCertificateProviders(t *testing.T) { { name: "allUnknownCertProviders", - wantConfig: nonNilCredsConfigV3, + wantConfig: configWithGoogleDefaultCredsAndV3, }, { name: "goodCertProviderConfig", @@ -711,7 +712,7 @@ func TestNewConfigWithCertificateProviders(t *testing.T) { } } -func TestNewConfigWithServerListenerResourceNameTemplate(t *testing.T) { +func (s) TestNewConfigWithServerListenerResourceNameTemplate(t *testing.T) { cancel := setupBootstrapOverride(map[string]string{ "badServerListenerResourceNameTemplate:": ` { @@ -760,13 +761,14 @@ func TestNewConfigWithServerListenerResourceNameTemplate(t *testing.T) { { name: "goodServerListenerResourceNameTemplate", wantConfig: &Config{ - XDSServer: &ServerConfig{ - ServerURI: "trafficdirector.googleapis.com:443", - Creds: ChannelCreds{Type: "google_default"}, - }, - NodeProto: v3NodeProto, - ServerListenerResourceNameTemplate: "grpc/server?xds.resource.listening_address=%s", - ClientDefaultListenerResourceNameTemplate: "%s", + xDSServers: []*ServerConfig{{ + serverURI: "trafficdirector.googleapis.com:443", + channelCreds: []ChannelCreds{{Type: "google_default"}}, + selectedCreds: ChannelCreds{Type: "google_default"}, + }}, + node: v3Node, + serverListenerResourceNameTemplate: "grpc/server?xds.resource.listening_address=%s", + clientDefaultListenerResourceNameTemplate: "%s", }, }, } @@ -779,9 +781,9 @@ func TestNewConfigWithServerListenerResourceNameTemplate(t *testing.T) { } } -func TestNewConfigWithFederation(t *testing.T) { +func (s) TestNewConfigWithFederation(t *testing.T) { cancel := setupBootstrapOverride(map[string]string{ - "badClientListenerResourceNameTemplate": ` + "badclientListenerResourceNameTemplate": ` { "node": { "id": "ENVOY_NODE_ID" }, "xds_servers" : [{ @@ -789,7 +791,7 @@ func TestNewConfigWithFederation(t *testing.T) { }], "client_default_listener_resource_name_template": 123456789 }`, - "badClientListenerResourceNameTemplatePerAuthority": ` + "badclientListenerResourceNameTemplatePerAuthority": ` { "node": { "id": "ENVOY_NODE_ID" }, "xds_servers" : [{ @@ -898,31 +900,33 @@ func TestNewConfigWithFederation(t *testing.T) { wantErr bool }{ { - name: "badClientListenerResourceNameTemplate", + name: "badclientListenerResourceNameTemplate", wantErr: true, }, { - name: "badClientListenerResourceNameTemplatePerAuthority", + name: "badclientListenerResourceNameTemplatePerAuthority", wantErr: true, }, { name: "good", wantConfig: &Config{ - XDSServer: &ServerConfig{ - ServerURI: "trafficdirector.googleapis.com:443", - Creds: ChannelCreds{Type: "google_default"}, - }, - NodeProto: v3NodeProto, - ServerListenerResourceNameTemplate: "xdstp://xds.example.com/envoy.config.listener.v3.Listener/grpc/server?listening_address=%s", - ClientDefaultListenerResourceNameTemplate: "xdstp://xds.example.com/envoy.config.listener.v3.Listener/%s", - Authorities: map[string]*Authority{ + xDSServers: []*ServerConfig{{ + serverURI: "trafficdirector.googleapis.com:443", + channelCreds: []ChannelCreds{{Type: "google_default"}}, + selectedCreds: ChannelCreds{Type: "google_default"}, + }}, + node: v3Node, + serverListenerResourceNameTemplate: "xdstp://xds.example.com/envoy.config.listener.v3.Listener/grpc/server?listening_address=%s", + clientDefaultListenerResourceNameTemplate: "xdstp://xds.example.com/envoy.config.listener.v3.Listener/%s", + authorities: map[string]*Authority{ "xds.td.com": { ClientListenerResourceNameTemplate: "xdstp://xds.td.com/envoy.config.listener.v3.Listener/%s", - XDSServer: &ServerConfig{ - ServerURI: "td.com", - Creds: ChannelCreds{Type: "google_default"}, - ServerFeatures: []string{"xds_v3"}, - }, + XDSServers: []*ServerConfig{{ + serverURI: "td.com", + channelCreds: []ChannelCreds{{Type: "google_default"}}, + serverFeatures: []string{"xds_v3"}, + selectedCreds: ChannelCreds{Type: "google_default"}, + }}, }, }, }, @@ -930,24 +934,26 @@ func TestNewConfigWithFederation(t *testing.T) { { name: "goodWithDefaultDefaultClientListenerTemplate", wantConfig: &Config{ - XDSServer: &ServerConfig{ - ServerURI: "trafficdirector.googleapis.com:443", - Creds: ChannelCreds{Type: "google_default"}, - }, - NodeProto: v3NodeProto, - ClientDefaultListenerResourceNameTemplate: "%s", + xDSServers: []*ServerConfig{{ + serverURI: "trafficdirector.googleapis.com:443", + channelCreds: []ChannelCreds{{Type: "google_default"}}, + selectedCreds: ChannelCreds{Type: "google_default"}, + }}, + node: v3Node, + clientDefaultListenerResourceNameTemplate: "%s", }, }, { name: "goodWithDefaultClientListenerTemplatePerAuthority", wantConfig: &Config{ - XDSServer: &ServerConfig{ - ServerURI: "trafficdirector.googleapis.com:443", - Creds: ChannelCreds{Type: "google_default"}, - }, - NodeProto: v3NodeProto, - ClientDefaultListenerResourceNameTemplate: "xdstp://xds.example.com/envoy.config.listener.v3.Listener/%s", - Authorities: map[string]*Authority{ + xDSServers: []*ServerConfig{{ + serverURI: "trafficdirector.googleapis.com:443", + channelCreds: []ChannelCreds{{Type: "google_default"}}, + selectedCreds: ChannelCreds{Type: "google_default"}, + }}, + node: v3Node, + clientDefaultListenerResourceNameTemplate: "xdstp://xds.example.com/envoy.config.listener.v3.Listener/%s", + authorities: map[string]*Authority{ "xds.td.com": { ClientListenerResourceNameTemplate: "xdstp://xds.td.com/envoy.config.listener.v3.Listener/%s", }, @@ -960,13 +966,14 @@ func TestNewConfigWithFederation(t *testing.T) { { name: "goodWithNoServerPerAuthority", wantConfig: &Config{ - XDSServer: &ServerConfig{ - ServerURI: "trafficdirector.googleapis.com:443", - Creds: ChannelCreds{Type: "google_default"}, - }, - NodeProto: v3NodeProto, - ClientDefaultListenerResourceNameTemplate: "xdstp://xds.example.com/envoy.config.listener.v3.Listener/%s", - Authorities: map[string]*Authority{ + xDSServers: []*ServerConfig{{ + serverURI: "trafficdirector.googleapis.com:443", + channelCreds: []ChannelCreds{{Type: "google_default"}}, + selectedCreds: ChannelCreds{Type: "google_default"}, + }}, + node: v3Node, + clientDefaultListenerResourceNameTemplate: "xdstp://xds.example.com/envoy.config.listener.v3.Listener/%s", + authorities: map[string]*Authority{ "xds.td.com": { ClientListenerResourceNameTemplate: "xdstp://xds.td.com/envoy.config.listener.v3.Listener/%s", }, @@ -983,23 +990,18 @@ func TestNewConfigWithFederation(t *testing.T) { } } -func TestServerConfigMarshalAndUnmarshal(t *testing.T) { - jsonCfg := `{ - "server_uri": "test-server", - "channel_creds": [{"type": "insecure"}], - "server_features": ["xds_v3"] - }` - origConfig, err := ServerConfigFromJSON([]byte(jsonCfg)) +func (s) TestServerConfigMarshalAndUnmarshal(t *testing.T) { + origConfig, err := ServerConfigForTesting(ServerConfigTestingOptions{URI: "test-server", ServerFeatures: []string{"xds_v3"}}) if err != nil { - t.Fatalf("Failed to create server config from JSON %s: %v", jsonCfg, err) + t.Fatalf("Failed to create server config for testing: %v", err) } - bs, err := json.Marshal(origConfig) + marshaledCfg, err := json.Marshal(origConfig) if err != nil { t.Fatalf("failed to marshal: %v", err) } unmarshaledConfig := new(ServerConfig) - if err := json.Unmarshal(bs, unmarshaledConfig); err != nil { + if err := json.Unmarshal(marshaledCfg, unmarshaledConfig); err != nil { t.Fatalf("failed to unmarshal: %v", err) } if diff := cmp.Diff(origConfig, unmarshaledConfig); diff != "" { @@ -1007,7 +1009,7 @@ func TestServerConfigMarshalAndUnmarshal(t *testing.T) { } } -func TestDefaultBundles(t *testing.T) { +func (s) TestDefaultBundles(t *testing.T) { tests := []string{"google_default", "insecure", "tls"} for _, typename := range tests { @@ -1018,3 +1020,180 @@ func TestDefaultBundles(t *testing.T) { }) } } + +type s struct { + grpctest.Tester +} + +func Test(t *testing.T) { + grpctest.RunSubTests(t, s{}) +} + +func newStructProtoFromMap(t *testing.T, input map[string]any) *structpb.Struct { + t.Helper() + + ret, err := structpb.NewStruct(input) + if err != nil { + t.Fatalf("Failed to create new struct proto from map %v: %v", input, err) + } + return ret +} + +func (s) TestNode_MarshalAndUnmarshal(t *testing.T) { + tests := []struct { + desc string + inputJSON []byte + wantNode node + }{ + { + desc: "basic happy case", + inputJSON: []byte(`{ + "id": "id", + "cluster": "cluster", + "locality": { + "region": "region", + "zone": "zone", + "sub_zone": "sub_zone" + }, + "metadata": { + "k1": "v1", + "k2": 101, + "k3": 280.0 + } +}`), + wantNode: node{ + ID: "id", + Cluster: "cluster", + Locality: locality{ + Region: "region", + Zone: "zone", + SubZone: "sub_zone", + }, + Metadata: newStructProtoFromMap(t, map[string]any{ + "k1": "v1", + "k2": 101, + "k3": 280.0, + }), + userAgentName: "gRPC Go", + userAgentVersionType: userAgentVersion{UserAgentVersion: grpc.Version}, + clientFeatures: []string{"envoy.lb.does_not_support_overprovisioning", "xds.config.resource-in-sotw"}, + }, + }, + { + desc: "client controlled fields", + inputJSON: []byte(`{ + "id": "id", + "cluster": "cluster", + "user_agent_name": "user_agent_name", + "user_agent_version_type": { + "user_agent_version": "version" + }, + "client_features": ["feature1", "feature2"] +}`), + wantNode: node{ + ID: "id", + Cluster: "cluster", + userAgentName: "gRPC Go", + userAgentVersionType: userAgentVersion{UserAgentVersion: grpc.Version}, + clientFeatures: []string{"envoy.lb.does_not_support_overprovisioning", "xds.config.resource-in-sotw"}, + }, + }, + } + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + // Unmarshal the input JSON into a node struct and check if it + // matches expectations. + unmarshaledNode := newNode() + if err := json.Unmarshal([]byte(test.inputJSON), &unmarshaledNode); err != nil { + t.Fatal(err) + } + if diff := cmp.Diff(test.wantNode, unmarshaledNode); diff != "" { + t.Fatalf("Unexpected diff in node: (-want, +got):\n%s", diff) + } + + // Marshal the recently unmarshaled node struct into JSON and + // remarshal it into another node struct, and check that it still + // matches expectations. + marshaledJSON, err := json.Marshal(unmarshaledNode) + if err != nil { + t.Fatalf("node.MarshalJSON() failed: %v", err) + } + reUnmarshaledNode := newNode() + if err := json.Unmarshal([]byte(marshaledJSON), &reUnmarshaledNode); err != nil { + t.Fatal(err) + } + if diff := cmp.Diff(test.wantNode, reUnmarshaledNode); diff != "" { + t.Fatalf("Unexpected diff in node: (-want, +got):\n%s", diff) + } + }) + } +} + +func (s) TestNode_ToProto(t *testing.T) { + tests := []struct { + desc string + inputNode node + wantProto *v3corepb.Node + }{ + { + desc: "all fields set", + inputNode: func() node { + n := newNode() + n.ID = "id" + n.Cluster = "cluster" + n.Locality = locality{ + Region: "region", + Zone: "zone", + SubZone: "sub_zone", + } + n.Metadata = newStructProtoFromMap(t, map[string]any{ + "k1": "v1", + "k2": 101, + "k3": 280.0, + }) + return n + }(), + wantProto: &v3corepb.Node{ + Id: "id", + Cluster: "cluster", + Locality: &v3corepb.Locality{ + Region: "region", + Zone: "zone", + SubZone: "sub_zone", + }, + Metadata: newStructProtoFromMap(t, map[string]any{ + "k1": "v1", + "k2": 101, + "k3": 280.0, + }), + UserAgentName: "gRPC Go", + UserAgentVersionType: &v3corepb.Node_UserAgentVersion{UserAgentVersion: grpc.Version}, + ClientFeatures: []string{"envoy.lb.does_not_support_overprovisioning", "xds.config.resource-in-sotw"}, + }, + }, + { + desc: "some fields unset", + inputNode: func() node { + n := newNode() + n.ID = "id" + return n + }(), + wantProto: &v3corepb.Node{ + Id: "id", + UserAgentName: "gRPC Go", + UserAgentVersionType: &v3corepb.Node_UserAgentVersion{UserAgentVersion: grpc.Version}, + ClientFeatures: []string{"envoy.lb.does_not_support_overprovisioning", "xds.config.resource-in-sotw"}, + }, + }, + } + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + gotProto := test.inputNode.toProto() + if diff := cmp.Diff(test.wantProto, gotProto, protocmp.Transform()); diff != "" { + t.Fatalf("Unexpected diff in node proto: (-want, +got):\n%s", diff) + } + }) + } +} diff --git a/stats/opentelemetry/csm/pluginoption.go b/stats/opentelemetry/csm/pluginoption.go index cc99e4c7c..6f1da7e47 100644 --- a/stats/opentelemetry/csm/pluginoption.go +++ b/stats/opentelemetry/csm/pluginoption.go @@ -294,10 +294,10 @@ func getNodeID() string { if err != nil { return "" // will become "unknown" } - if cfg.NodeProto == nil { + if cfg.Node() == nil { return "" } - return cfg.NodeProto.GetId() + return cfg.Node().GetId() } // metadataExchangeKey is the key for HTTP metadata exchange. diff --git a/xds/googledirectpath/googlec2p.go b/xds/googledirectpath/googlec2p.go index 6ab7fb03f..5ea7b60d9 100644 --- a/xds/googledirectpath/googlec2p.go +++ b/xds/googledirectpath/googlec2p.go @@ -46,18 +46,13 @@ const ( c2pScheme = "google-c2p" c2pAuthority = "traffic-director-c2p.xds.googleapis.com" - tdURL = "dns:///directpath-pa.googleapis.com" - httpReqTimeout = 10 * time.Second - zoneURL = "http://metadata.google.internal/computeMetadata/v1/instance/zone" - ipv6URL = "http://metadata.google.internal/computeMetadata/v1/instance/network-interfaces/0/ipv6s" - - gRPCUserAgentName = "gRPC Go" - clientFeatureNoOverprovisioning = "envoy.lb.does_not_support_overprovisioning" - clientFeatureResourceWrapper = "xds.config.resource-in-sotw" - ipv6CapableMetadataName = "TRAFFICDIRECTOR_DIRECTPATH_C2P_IPV6_CAPABLE" - - logPrefix = "[google-c2p-resolver]" + tdURL = "dns:///directpath-pa.googleapis.com" + zoneURL = "http://metadata.google.internal/computeMetadata/v1/instance/zone" + ipv6URL = "http://metadata.google.internal/computeMetadata/v1/instance/network-interfaces/0/ipv6s" + ipv6CapableMetadataName = "TRAFFICDIRECTOR_DIRECTPATH_C2P_IPV6_CAPABLE" + httpReqTimeout = 10 * time.Second + logPrefix = "[google-c2p-resolver]" dnsName, xdsName = "dns", "xds" ) @@ -65,6 +60,8 @@ const ( var ( onGCE = googlecloud.OnGCE + randInt = rand.Int + newClientWithConfig = func(config *bootstrap.Config) (xdsclient.XDSClient, func(), error) { return xdsclient.NewWithConfig(config) } @@ -159,8 +156,6 @@ func (r *c2pResolver) Close() { r.clientCloseFunc() } -var id = fmt.Sprintf("C2P-%d", rand.Int()) - func newNodeConfig(zone string, ipv6Capable bool) string { metadata := "" if ipv6Capable { @@ -174,7 +169,7 @@ func newNodeConfig(zone string, ipv6Capable bool) string { "zone": "%s" } %s - }`, id, zone, metadata) + }`, fmt.Sprintf("C2P-%d", randInt()), zone, metadata) } func newAuthoritiesConfig(xdsServer string) string { @@ -192,7 +187,7 @@ func newXdsServerConfig(xdsServerURI string) string { { "server_uri": "%s", "channel_creds": [{"type": "google_default"}], - "server_features": ["xds_v3", "ignore_resource_deletion", "xds.config.resource-in-sotw"] + "server_features": ["ignore_resource_deletion"] }`, xdsServerURI) } diff --git a/xds/googledirectpath/googlec2p_test.go b/xds/googledirectpath/googlec2p_test.go index 5483cf55d..9934d3715 100644 --- a/xds/googledirectpath/googlec2p_test.go +++ b/xds/googledirectpath/googlec2p_test.go @@ -19,7 +19,7 @@ package googledirectpath import ( - "fmt" + "context" "strconv" "strings" "testing" @@ -29,15 +29,22 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/credentials/insecure" "google.golang.org/grpc/internal/envconfig" + "google.golang.org/grpc/internal/grpctest" "google.golang.org/grpc/internal/xds/bootstrap" "google.golang.org/grpc/resolver" "google.golang.org/grpc/xds/internal/xdsclient" - "google.golang.org/protobuf/testing/protocmp" - "google.golang.org/protobuf/types/known/structpb" - - v3corepb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" ) +const defaultTestTimeout = 10 * time.Second + +type s struct { + grpctest.Tester +} + +func Test(t *testing.T) { + grpctest.RunSubTests(t, s{}) +} + type emptyResolver struct { resolver.Resolver scheme string @@ -58,15 +65,24 @@ var ( testXDSResolver = &emptyResolver{scheme: "xds"} ) -func replaceResolvers() func() { +// replaceResolvers unregisters the real resolvers for schemes `dns` and `xds` +// and registers test resolvers instead. This allows the test to verify that +// expected resolvers are built. +func replaceResolvers(t *testing.T) { oldDNS := resolver.Get("dns") resolver.Register(testDNSResolver) oldXDS := resolver.Get("xds") resolver.Register(testXDSResolver) - return func() { + t.Cleanup(func() { resolver.Register(oldDNS) resolver.Register(oldXDS) - } + }) +} + +func simulateRunningOnGCE(t *testing.T, gce bool) { + oldOnGCE := onGCE + onGCE = func() bool { return gce } + t.Cleanup(func() { onGCE = oldOnGCE }) } type testXDSClient struct { @@ -78,25 +94,28 @@ func (c *testXDSClient) Close() { c.closed <- struct{}{} } -// Test that when bootstrap env is set and we're running on GCE, don't fallback to DNS (because -// federation is enabled by default). -func TestBuildWithBootstrapEnvSet(t *testing.T) { - defer replaceResolvers()() - builder := resolver.Get(c2pScheme) - - // make the test behave the ~same whether it's running on or off GCE - oldOnGCE := onGCE - onGCE = func() bool { return true } - defer func() { onGCE = oldOnGCE }() - - // don't actually read the bootstrap file contents - xdsClient := &testXDSClient{closed: make(chan struct{}, 1)} +// Overrides the creation of a real xDS client with a test one. +func overrideWithTestXDSClient(t *testing.T) (*testXDSClient, chan *bootstrap.Config) { + xdsC := &testXDSClient{closed: make(chan struct{}, 1)} + configCh := make(chan *bootstrap.Config, 1) oldNewClient := newClientWithConfig newClientWithConfig = func(config *bootstrap.Config) (xdsclient.XDSClient, func(), error) { - return xdsClient, func() { xdsClient.Close() }, nil + configCh <- config + return xdsC, func() { xdsC.Close() }, nil } - defer func() { newClientWithConfig = oldNewClient }() + t.Cleanup(func() { newClientWithConfig = oldNewClient }) + return xdsC, configCh +} +// Tests the scenario where the bootstrap env vars are set and we're running on +// GCE. The test builds a google-c2p resolver and verifies that an xDS resolver +// is built and that we don't fallback to DNS (because federation is enabled by +// default). +func (s) TestBuildWithBootstrapEnvSet(t *testing.T) { + replaceResolvers(t) + simulateRunningOnGCE(t, true) + + builder := resolver.Get(c2pScheme) for i, envP := range []*string{&envconfig.XDSBootstrapFileName, &envconfig.XDSBootstrapFileContent} { t.Run(strconv.Itoa(i), func(t *testing.T) { // Set bootstrap config env var. @@ -104,11 +123,16 @@ func TestBuildWithBootstrapEnvSet(t *testing.T) { *envP = "does not matter" defer func() { *envP = oldEnv }() - // Build should return xDS, not DNS. + overrideWithTestXDSClient(t) + + // Build the google-c2p resolver. r, err := builder.Build(resolver.Target{}, nil, resolver.BuildOptions{}) if err != nil { t.Fatalf("failed to build resolver: %v", err) } + defer r.Close() + + // Build should return xDS, not DNS. rr := r.(*c2pResolver) if rrr := rr.Resolver; rrr != testXDSResolver { t.Fatalf("want xds resolver, got %#v", rrr) @@ -117,134 +141,212 @@ func TestBuildWithBootstrapEnvSet(t *testing.T) { } } -// Test that when not on GCE, fallback to DNS. -func TestBuildNotOnGCE(t *testing.T) { - defer replaceResolvers()() +// Tests the scenario where we are not running on GCE. The test builds a +// google-c2p resolver and verifies that we fallback to DNS. +func (s) TestBuildNotOnGCE(t *testing.T) { + replaceResolvers(t) + simulateRunningOnGCE(t, false) builder := resolver.Get(c2pScheme) - oldOnGCE := onGCE - onGCE = func() bool { return false } - defer func() { onGCE = oldOnGCE }() - - // Build should return DNS, not xDS. + // Build the google-c2p resolver. r, err := builder.Build(resolver.Target{}, nil, resolver.BuildOptions{}) if err != nil { t.Fatalf("failed to build resolver: %v", err) } + defer r.Close() + + // Build should return DNS, not xDS. if r != testDNSResolver { t.Fatalf("want dns resolver, got %#v", r) } } -// Test that when xDS is built, the client is built with the correct config. -func TestBuildXDS(t *testing.T) { - defer replaceResolvers()() +// Test that when a google-c2p resolver is built, the xDS client is built with +// the expected config. +func (s) TestBuildXDS(t *testing.T) { + replaceResolvers(t) + simulateRunningOnGCE(t, true) builder := resolver.Get(c2pScheme) - oldOnGCE := onGCE - onGCE = func() bool { return true } - defer func() { onGCE = oldOnGCE }() - - const testZone = "test-zone" + // Override the zone returned by the metadata server. oldGetZone := getZone - getZone = func(time.Duration) string { return testZone } + getZone = func(time.Duration) string { return "test-zone" } defer func() { getZone = oldGetZone }() + // Override the random func used in the node ID. + origRandInd := randInt + randInt = func() int { return 666 } + defer func() { randInt = origRandInd }() + for _, tt := range []struct { - name string - ipv6 bool - tdURI string // traffic director URI will be overridden if this is set. + desc string + ipv6Capable bool + tdURIOverride string + wantBootstrapConfig *bootstrap.Config }{ - {name: "ipv6 true", ipv6: true}, - {name: "ipv6 false", ipv6: false}, - {name: "override TD URI", ipv6: true, tdURI: "test-uri"}, + { + desc: "ipv6 false", + wantBootstrapConfig: func() *bootstrap.Config { + cfg, err := bootstrap.NewConfigFromContents([]byte(`{ +"xds_servers": [ + { + "server_uri": "dns:///directpath-pa.googleapis.com", + "channel_creds": [{"type": "google_default"}], + "server_features": ["ignore_resource_deletion"] + } +], +"client_default_listener_resource_name_template": "%s", +"authorities": { + "traffic-director-c2p.xds.googleapis.com": { + "xds_servers": [ + { + "server_uri": "dns:///directpath-pa.googleapis.com", + "channel_creds": [{"type": "google_default"}], + "server_features": ["ignore_resource_deletion"] + } + ] + } +}, +"node": { + "id": "C2P-666", + "locality": { + "zone": "test-zone" + } +} +}`)) + if err != nil { + t.Fatalf("Bootstrap parsing failure: %v", err) + } + return cfg + }(), + }, + { + desc: "ipv6 true", + ipv6Capable: true, + wantBootstrapConfig: func() *bootstrap.Config { + cfg, err := bootstrap.NewConfigFromContents([]byte(`{ +"xds_servers": [ + { + "server_uri": "dns:///directpath-pa.googleapis.com", + "channel_creds": [{"type": "google_default"}], + "server_features": ["ignore_resource_deletion"] + } +], +"client_default_listener_resource_name_template": "%s", +"authorities": { + "traffic-director-c2p.xds.googleapis.com": { + "xds_servers": [ + { + "server_uri": "dns:///directpath-pa.googleapis.com", + "channel_creds": [{"type": "google_default"}], + "server_features": ["ignore_resource_deletion"] + } + ] + } +}, +"node": { + "id": "C2P-666", + "locality": { + "zone": "test-zone" + }, + "metadata": { + "TRAFFICDIRECTOR_DIRECTPATH_C2P_IPV6_CAPABLE": true + } +} +}`)) + if err != nil { + t.Fatalf("Bootstrap parsing failure: %v", err) + } + return cfg + }(), + }, + { + desc: "override TD URI", + ipv6Capable: true, + tdURIOverride: "test-uri", + wantBootstrapConfig: func() *bootstrap.Config { + cfg, err := bootstrap.NewConfigFromContents([]byte(`{ +"xds_servers": [ + { + "server_uri": "test-uri", + "channel_creds": [{"type": "google_default"}], + "server_features": ["ignore_resource_deletion"] + } +], +"client_default_listener_resource_name_template": "%s", +"authorities": { + "traffic-director-c2p.xds.googleapis.com": { + "xds_servers": [ + { + "server_uri": "test-uri", + "channel_creds": [{"type": "google_default"}], + "server_features": ["ignore_resource_deletion"] + } + ] + } +}, +"node": { + "id": "C2P-666", + "locality": { + "zone": "test-zone" + }, + "metadata": { + "TRAFFICDIRECTOR_DIRECTPATH_C2P_IPV6_CAPABLE": true + } +} +}`)) + if err != nil { + t.Fatalf("Bootstrap parsing failure: %v", err) + } + return cfg + }(), + }, } { - t.Run(tt.name, func(t *testing.T) { + t.Run(tt.desc, func(t *testing.T) { + // Override IPv6 capability returned by the metadata server. oldGetIPv6Capability := getIPv6Capable - getIPv6Capable = func(time.Duration) bool { return tt.ipv6 } + getIPv6Capable = func(time.Duration) bool { return tt.ipv6Capable } defer func() { getIPv6Capable = oldGetIPv6Capability }() - if tt.tdURI != "" { + // Override TD URI test only env var. + if tt.tdURIOverride != "" { oldURI := envconfig.C2PResolverTestOnlyTrafficDirectorURI - envconfig.C2PResolverTestOnlyTrafficDirectorURI = tt.tdURI - defer func() { - envconfig.C2PResolverTestOnlyTrafficDirectorURI = oldURI - }() + envconfig.C2PResolverTestOnlyTrafficDirectorURI = tt.tdURIOverride + defer func() { envconfig.C2PResolverTestOnlyTrafficDirectorURI = oldURI }() } - tXDSClient := &testXDSClient{closed: make(chan struct{}, 1)} + tXDSClient, configCh := overrideWithTestXDSClient(t) - configCh := make(chan *bootstrap.Config, 1) - oldNewClient := newClientWithConfig - newClientWithConfig = func(config *bootstrap.Config) (xdsclient.XDSClient, func(), error) { - configCh <- config - return tXDSClient, func() { tXDSClient.Close() }, nil - } - defer func() { newClientWithConfig = oldNewClient }() + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() - // Build should return DNS, not xDS. + // Build the google-c2p resolver. r, err := builder.Build(resolver.Target{}, nil, resolver.BuildOptions{}) if err != nil { t.Fatalf("failed to build resolver: %v", err) } + + // Build should return xDS, not DNS. rr := r.(*c2pResolver) if rrr := rr.Resolver; rrr != testXDSResolver { t.Fatalf("want xds resolver, got %#v, ", rrr) } - wantNode := &v3corepb.Node{ - Id: id, - Metadata: nil, - Locality: &v3corepb.Locality{Zone: testZone}, - UserAgentName: gRPCUserAgentName, - UserAgentVersionType: &v3corepb.Node_UserAgentVersion{UserAgentVersion: grpc.Version}, - ClientFeatures: []string{clientFeatureNoOverprovisioning, clientFeatureResourceWrapper}, - } - if tt.ipv6 { - wantNode.Metadata = &structpb.Struct{ - Fields: map[string]*structpb.Value{ - ipv6CapableMetadataName: { - Kind: &structpb.Value_BoolValue{BoolValue: true}, - }, - }, - } - } - wantServerConfig, err := bootstrap.ServerConfigFromJSON([]byte(fmt.Sprintf(`{ - "server_uri": "%s", - "channel_creds": [{"type": "google_default"}], - "server_features": ["xds_v3", "ignore_resource_deletion", "xds.config.resource-in-sotw"] - }`, tdURL))) - if err != nil { - t.Fatalf("Failed to build server bootstrap config: %v", err) - } - wantConfig := &bootstrap.Config{ - XDSServer: wantServerConfig, - ClientDefaultListenerResourceNameTemplate: "%s", - Authorities: map[string]*bootstrap.Authority{ - "traffic-director-c2p.xds.googleapis.com": { - XDSServer: wantServerConfig, - ClientListenerResourceNameTemplate: "xdstp://traffic-director-c2p.xds.googleapis.com/envoy.config.listener.v3.Listener/%s", - }, - }, - NodeProto: wantNode, - } - if tt.tdURI != "" { - wantConfig.XDSServer.ServerURI = tt.tdURI - } + var gotConfig *bootstrap.Config select { - case gotConfig := <-configCh: - if diff := cmp.Diff(wantConfig, gotConfig, protocmp.Transform()); diff != "" { + case gotConfig = <-configCh: + if diff := cmp.Diff(tt.wantBootstrapConfig, gotConfig); diff != "" { t.Fatalf("Unexpected diff in bootstrap config (-want +got):\n%s", diff) } - case <-time.After(time.Second): - t.Fatalf("timeout waiting for client config") + case <-ctx.Done(): + t.Fatalf("Timeout waiting for new xDS client to be built") } r.Close() select { case <-tXDSClient.closed: - case <-time.After(time.Second): - t.Fatalf("timeout waiting for client close") + case <-ctx.Done(): + t.Fatalf("Timeout waiting for xDS client to be closed") } }) } @@ -253,7 +355,7 @@ func TestBuildXDS(t *testing.T) { // TestDialFailsWhenTargetContainsAuthority attempts to Dial a target URI of // google-c2p scheme with a non-empty authority and verifies that it fails with // an expected error. -func TestBuildFailsWhenCalledWithAuthority(t *testing.T) { +func (s) TestBuildFailsWhenCalledWithAuthority(t *testing.T) { uri := "google-c2p://an-authority/resource" cc, err := grpc.Dial(uri, grpc.WithTransportCredentials(insecure.NewCredentials())) defer func() { diff --git a/xds/internal/balancer/cdsbalancer/cdsbalancer.go b/xds/internal/balancer/cdsbalancer/cdsbalancer.go index 8e97e104e..798d1c09e 100644 --- a/xds/internal/balancer/cdsbalancer/cdsbalancer.go +++ b/xds/internal/balancer/cdsbalancer/cdsbalancer.go @@ -207,7 +207,7 @@ func (b *cdsBalancer) handleSecurityConfig(config *xdsresource.SecurityConfig) e } // A root provider is required whether we are using TLS or mTLS. - cpc := b.xdsClient.BootstrapConfig().CertProviderConfigs + cpc := b.xdsClient.BootstrapConfig().CertProviderConfigs() rootProvider, err := buildProvider(cpc, config.RootInstanceName, config.RootCertName, false, true) if err != nil { return err diff --git a/xds/internal/balancer/cdsbalancer/cdsbalancer_test.go b/xds/internal/balancer/cdsbalancer/cdsbalancer_test.go index d9294092d..5a6546382 100644 --- a/xds/internal/balancer/cdsbalancer/cdsbalancer_test.go +++ b/xds/internal/balancer/cdsbalancer/cdsbalancer_test.go @@ -597,16 +597,18 @@ func (s) TestClusterUpdate_SuccessWithLRS(t *testing.T) { ServiceName: serviceName, EnableLRS: true, }) + lrsServerCfg, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{URI: fmt.Sprintf("passthrough:///%s", mgmtServer.Address)}) + if err != nil { + t.Fatalf("Failed to create LRS server config for testing: %v", err) + } + wantChildCfg := &clusterresolver.LBConfig{ DiscoveryMechanisms: []clusterresolver.DiscoveryMechanism{{ - Cluster: clusterName, - Type: clusterresolver.DiscoveryMechanismTypeEDS, - EDSServiceName: serviceName, - LoadReportingServer: &bootstrap.ServerConfig{ - ServerURI: mgmtServer.Address, - Creds: bootstrap.ChannelCreds{Type: "insecure"}, - }, - OutlierDetection: json.RawMessage(`{}`), + Cluster: clusterName, + Type: clusterresolver.DiscoveryMechanismTypeEDS, + EDSServiceName: serviceName, + LoadReportingServer: lrsServerCfg, + OutlierDetection: json.RawMessage(`{}`), }}, XDSLBPolicy: json.RawMessage(`[{"xds_wrr_locality_experimental": {"childPolicy": [{"round_robin": {}}]}}]`), } diff --git a/xds/internal/balancer/clusterimpl/balancer_test.go b/xds/internal/balancer/clusterimpl/balancer_test.go index 89a3607ed..76c96decf 100644 --- a/xds/internal/balancer/clusterimpl/balancer_test.go +++ b/xds/internal/balancer/clusterimpl/balancer_test.go @@ -59,17 +59,8 @@ const ( ) var ( - testBackendAddrs = []resolver.Address{ - {Addr: "1.1.1.1:1"}, - } - testLRSServerConfig = &bootstrap.ServerConfig{ - ServerURI: "trafficdirector.googleapis.com:443", - Creds: bootstrap.ChannelCreds{ - Type: "google_default", - }, - } - - cmpOpts = cmp.Options{ + testBackendAddrs = []resolver.Address{{Addr: "1.1.1.1:1"}} + cmpOpts = cmp.Options{ cmpopts.EquateEmpty(), cmpopts.IgnoreFields(load.Data{}, "ReportInterval"), } @@ -107,6 +98,13 @@ func (s) TestDropByCategory(t *testing.T) { dropNumerator = 1 dropDenominator = 2 ) + testLRSServerConfig, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{ + URI: "trafficdirector.googleapis.com:443", + ChannelCreds: []bootstrap.ChannelCreds{{Type: "google_default"}}, + }) + if err != nil { + t.Fatalf("Failed to create LRS server config for testing: %v", err) + } if err := b.UpdateClientConnState(balancer.ClientConnState{ ResolverState: xdsclient.SetClient(resolver.State{Addresses: testBackendAddrs}, xdsC), BalancerConfig: &LBConfig{ @@ -262,6 +260,13 @@ func (s) TestDropCircuitBreaking(t *testing.T) { defer b.Close() var maxRequest uint32 = 50 + testLRSServerConfig, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{ + URI: "trafficdirector.googleapis.com:443", + ChannelCreds: []bootstrap.ChannelCreds{{Type: "google_default"}}, + }) + if err != nil { + t.Fatalf("Failed to create LRS server config for testing: %v", err) + } if err := b.UpdateClientConnState(balancer.ClientConnState{ ResolverState: xdsclient.SetClient(resolver.State{Addresses: testBackendAddrs}, xdsC), BalancerConfig: &LBConfig{ @@ -592,6 +597,13 @@ func (s) TestLoadReporting(t *testing.T) { for i, a := range testBackendAddrs { addrs[i] = xdsinternal.SetLocalityID(a, testLocality) } + testLRSServerConfig, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{ + URI: "trafficdirector.googleapis.com:443", + ChannelCreds: []bootstrap.ChannelCreds{{Type: "google_default"}}, + }) + if err != nil { + t.Fatalf("Failed to create LRS server config for testing: %v", err) + } if err := b.UpdateClientConnState(balancer.ClientConnState{ ResolverState: xdsclient.SetClient(resolver.State{Addresses: addrs}, xdsC), BalancerConfig: &LBConfig{ @@ -715,6 +727,13 @@ func (s) TestUpdateLRSServer(t *testing.T) { for i, a := range testBackendAddrs { addrs[i] = xdsinternal.SetLocalityID(a, testLocality) } + testLRSServerConfig, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{ + URI: "trafficdirector.googleapis.com:443", + ChannelCreds: []bootstrap.ChannelCreds{{Type: "google_default"}}, + }) + if err != nil { + t.Fatalf("Failed to create LRS server config for testing: %v", err) + } if err := b.UpdateClientConnState(balancer.ClientConnState{ ResolverState: xdsclient.SetClient(resolver.State{Addresses: addrs}, xdsC), BalancerConfig: &LBConfig{ @@ -740,12 +759,14 @@ func (s) TestUpdateLRSServer(t *testing.T) { t.Fatalf("xdsClient.ReportLoad called with {%q}: want {%q}", got.Server, testLRSServerConfig) } - testLRSServerConfig2 := &bootstrap.ServerConfig{ - ServerURI: "trafficdirector-another.googleapis.com:443", - Creds: bootstrap.ChannelCreds{ - Type: "google_default", - }, + testLRSServerConfig2, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{ + URI: "trafficdirector-another.googleapis.com:443", + ChannelCreds: []bootstrap.ChannelCreds{{Type: "google_default"}}, + }) + if err != nil { + t.Fatalf("Failed to create LRS server config for testing: %v", err) } + // Update LRS server to a different name. if err := b.UpdateClientConnState(balancer.ClientConnState{ ResolverState: xdsclient.SetClient(resolver.State{Addresses: addrs}, xdsC), diff --git a/xds/internal/balancer/clusterimpl/config_test.go b/xds/internal/balancer/clusterimpl/config_test.go index 9b60e7dd6..82aef52aa 100644 --- a/xds/internal/balancer/clusterimpl/config_test.go +++ b/xds/internal/balancer/clusterimpl/config_test.go @@ -22,7 +22,6 @@ import ( "testing" "github.com/google/go-cmp/cmp" - "github.com/google/go-cmp/cmp/cmpopts" "google.golang.org/grpc/balancer" _ "google.golang.org/grpc/balancer/roundrobin" _ "google.golang.org/grpc/balancer/weightedtarget" @@ -89,6 +88,14 @@ var ( ) func TestParseConfig(t *testing.T) { + testLRSServerConfig, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{ + URI: "trafficdirector.googleapis.com:443", + ChannelCreds: []bootstrap.ChannelCreds{{Type: "google_default"}}, + }) + if err != nil { + t.Fatalf("Failed to create LRS server config for testing: %v", err) + } + tests := []struct { name string js string @@ -133,8 +140,8 @@ func TestParseConfig(t *testing.T) { if (err != nil) != tt.wantErr { t.Fatalf("parseConfig() error = %v, wantErr %v", err, tt.wantErr) } - if !cmp.Equal(got, tt.want, cmpopts.IgnoreFields(bootstrap.ServerConfig{}, "Creds")) { - t.Errorf("parseConfig() got unexpected result, diff: %v", cmp.Diff(got, tt.want)) + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Errorf("parseConfig() got unexpected diff (-want, +got): %v", diff) } }) } diff --git a/xds/internal/balancer/clusterresolver/config_test.go b/xds/internal/balancer/clusterresolver/config_test.go index f7eec5add..30befd5b9 100644 --- a/xds/internal/balancer/clusterresolver/config_test.go +++ b/xds/internal/balancer/clusterresolver/config_test.go @@ -171,14 +171,14 @@ const ( }` ) -var testLRSServerConfig = &bootstrap.ServerConfig{ - ServerURI: "trafficdirector.googleapis.com:443", - Creds: bootstrap.ChannelCreds{ - Type: "google_default", - }, -} - func TestParseConfig(t *testing.T) { + testLRSServerConfig, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{ + URI: "trafficdirector.googleapis.com:443", + ChannelCreds: []bootstrap.ChannelCreds{{Type: "google_default"}}, + }) + if err != nil { + t.Fatalf("Failed to create LRS server config for testing: %v", err) + } tests := []struct { name string js string diff --git a/xds/internal/balancer/clusterresolver/configbuilder_test.go b/xds/internal/balancer/clusterresolver/configbuilder_test.go index b30686b18..010558868 100644 --- a/xds/internal/balancer/clusterresolver/configbuilder_test.go +++ b/xds/internal/balancer/clusterresolver/configbuilder_test.go @@ -33,6 +33,7 @@ import ( "google.golang.org/grpc/balancer/weightedroundrobin" "google.golang.org/grpc/internal/hierarchy" iserviceconfig "google.golang.org/grpc/internal/serviceconfig" + "google.golang.org/grpc/internal/xds/bootstrap" "google.golang.org/grpc/resolver" "google.golang.org/grpc/xds/internal" "google.golang.org/grpc/xds/internal/balancer/clusterimpl" @@ -132,6 +133,14 @@ func init() { // TestBuildPriorityConfigJSON is a sanity check that the built balancer config // can be parsed. The behavior test is covered by TestBuildPriorityConfig. func TestBuildPriorityConfigJSON(t *testing.T) { + testLRSServerConfig, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{ + URI: "trafficdirector.googleapis.com:443", + ChannelCreds: []bootstrap.ChannelCreds{{Type: "google_default"}}, + }) + if err != nil { + t.Fatalf("Failed to create LRS server config for testing: %v", err) + } + gotConfig, _, err := buildPriorityConfigJSON([]priorityConfig{ { mechanism: DiscoveryMechanism{ @@ -317,6 +326,14 @@ func TestBuildClusterImplConfigForDNS(t *testing.T) { } func TestBuildClusterImplConfigForEDS(t *testing.T) { + testLRSServerConfig, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{ + URI: "trafficdirector.googleapis.com:443", + ChannelCreds: []bootstrap.ChannelCreds{{Type: "google_default"}}, + }) + if err != nil { + t.Fatalf("Failed to create LRS server config for testing: %v", err) + } + gotNames, gotConfigs, gotAddrs, _ := buildClusterImplConfigForEDS( newNameGenerator(2), xdsresource.EndpointsUpdate{ diff --git a/xds/internal/balancer/clusterresolver/e2e_test/aggregate_cluster_test.go b/xds/internal/balancer/clusterresolver/e2e_test/aggregate_cluster_test.go index c7d9a0960..1fb8ea732 100644 --- a/xds/internal/balancer/clusterresolver/e2e_test/aggregate_cluster_test.go +++ b/xds/internal/balancer/clusterresolver/e2e_test/aggregate_cluster_test.go @@ -89,7 +89,9 @@ func setupDNS() (chan resolver.Target, chan struct{}, chan resolver.ResolveNowOp resolveNowCh := make(chan resolver.ResolveNowOptions, 1) mr := manual.NewBuilderWithScheme("dns") - mr.BuildCallback = func(target resolver.Target, _ resolver.ClientConn, _ resolver.BuildOptions) { targetCh <- target } + mr.BuildCallback = func(target resolver.Target, _ resolver.ClientConn, _ resolver.BuildOptions) { + targetCh <- target + } mr.CloseCallback = func() { closeCh <- struct{}{} } mr.ResolveNowCallback = func(opts resolver.ResolveNowOptions) { resolveNowCh <- opts } diff --git a/xds/internal/resolver/xds_resolver.go b/xds/internal/resolver/xds_resolver.go index 40dd97267..c9f4a6e58 100644 --- a/xds/internal/resolver/xds_resolver.go +++ b/xds/internal/resolver/xds_resolver.go @@ -139,9 +139,13 @@ func (r *xdsResolver) sanityChecksOnBootstrapConfig(target resolver.Target, opts // Find the client listener template to use from the bootstrap config: // - If authority is not set in the target, use the top level template // - If authority is set, use the template from the authority map. - template := bootstrapConfig.ClientDefaultListenerResourceNameTemplate + template := bootstrapConfig.ClientDefaultListenerResourceNameTemplate() if authority := target.URL.Host; authority != "" { - a := bootstrapConfig.Authorities[authority] + authorities := bootstrapConfig.Authorities() + if authorities == nil { + return "", fmt.Errorf("xds: authority %q specified in dial target %q is not found in the bootstrap file", authority, target) + } + a := authorities[authority] if a == nil { return "", fmt.Errorf("xds: authority %q specified in dial target %q is not found in the bootstrap file", authority, target) } diff --git a/xds/internal/server/conn_wrapper.go b/xds/internal/server/conn_wrapper.go index fdba76929..0822c6f27 100644 --- a/xds/internal/server/conn_wrapper.go +++ b/xds/internal/server/conn_wrapper.go @@ -107,7 +107,7 @@ func (c *connWrapper) XDSHandshakeInfo() (*xdsinternal.HandshakeInfo, error) { return xdsinternal.NewHandshakeInfo(nil, nil, nil, false), nil } - cpc := c.parent.xdsC.BootstrapConfig().CertProviderConfigs + cpc := c.parent.xdsC.BootstrapConfig().CertProviderConfigs() // Identity provider name is mandatory on the server-side, and this is // enforced when the resource is received at the XDSClient layer. secCfg := c.filterChain.SecurityCfg diff --git a/xds/internal/testutils/fakeclient/client.go b/xds/internal/testutils/fakeclient/client.go index 2c97cba9c..806a207fa 100644 --- a/xds/internal/testutils/fakeclient/client.go +++ b/xds/internal/testutils/fakeclient/client.go @@ -108,6 +108,6 @@ func NewClientWithName(name string) *Client { loadReportCh: testutils.NewChannel(), lrsCancelCh: testutils.NewChannel(), loadStore: load.NewStore(), - bootstrapCfg: &bootstrap.Config{ClientDefaultListenerResourceNameTemplate: "%s"}, + bootstrapCfg: &bootstrap.Config{}, } } diff --git a/xds/internal/testutils/testutils.go b/xds/internal/testutils/testutils.go index 6a950fe5b..15049b6a6 100644 --- a/xds/internal/testutils/testutils.go +++ b/xds/internal/testutils/testutils.go @@ -19,10 +19,6 @@ package testutils import ( - "fmt" - "testing" - - "google.golang.org/grpc/internal/xds/bootstrap" "google.golang.org/grpc/xds/internal/xdsclient/xdsresource" "google.golang.org/grpc/xds/internal/xdsclient/xdsresource/version" ) @@ -53,20 +49,3 @@ func BuildResourceName(typeName, auth, id string, ctxParams map[string]string) s ContextParams: ctxParams, }).String() } - -// ServerConfigForAddress returns a bootstrap.ServerConfig for the given address -// with default values of insecure channel_creds and v3 server_features. -func ServerConfigForAddress(t *testing.T, addr string) *bootstrap.ServerConfig { - t.Helper() - - jsonCfg := fmt.Sprintf(`{ - "server_uri": "%s", - "channel_creds": [{"type": "insecure"}], - "server_features": ["xds_v3"] - }`, addr) - sc, err := bootstrap.ServerConfigFromJSON([]byte(jsonCfg)) - if err != nil { - t.Fatalf("Failed to create server config from JSON %s: %v", jsonCfg, err) - } - return sc -} diff --git a/xds/internal/xdsclient/authority.go b/xds/internal/xdsclient/authority.go index b0763a024..b80cbf732 100644 --- a/xds/internal/xdsclient/authority.go +++ b/xds/internal/xdsclient/authority.go @@ -118,12 +118,12 @@ func newAuthority(args authorityArgs) (*authority, error) { } tr, err := transport.New(transport.Options{ - ServerCfg: *args.serverCfg, + ServerCfg: args.serverCfg, OnRecvHandler: ret.handleResourceUpdate, OnErrorHandler: ret.newConnectionError, OnSendHandler: ret.transportOnSendHandler, Logger: args.logger, - NodeProto: args.bootstrapCfg.NodeProto, + NodeProto: args.bootstrapCfg.Node(), }) if err != nil { return nil, fmt.Errorf("creating new transport to %q: %v", args.serverCfg, err) @@ -283,7 +283,7 @@ func (a *authority) updateResourceStateAndScheduleCallbacks(rType xdsresource.Ty // resource deletion is to be ignored, the resource is not removed from // the cache and the corresponding OnResourceDoesNotExist() callback is // not invoked on the watchers. - if a.serverCfg.IgnoreResourceDeletion { + if a.serverCfg.ServerFeaturesIgnoreResourceDeletion() { if !state.deletionIgnored { state.deletionIgnored = true a.logger.Warningf("Ignoring resource deletion for resource %q of type %q", name, rType.TypeName()) diff --git a/xds/internal/xdsclient/authority_test.go b/xds/internal/xdsclient/authority_test.go index 17140ff37..83954a098 100644 --- a/xds/internal/xdsclient/authority_test.go +++ b/xds/internal/xdsclient/authority_test.go @@ -64,11 +64,21 @@ func setupTest(ctx context.Context, t *testing.T, opts e2e.ManagementServerOptio t.Fatalf("Failed to spin up the xDS management server: %q", err) } + contents, err := e2e.DefaultBootstrapContents(nodeID, ms.Address) + if err != nil { + t.Fatalf("Failed to create bootstrap configuration: %v", err) + } + config, err := bootstrap.NewConfigFromContents(contents) + if err != nil { + t.Fatalf("Failed to build bootstrap configuration: %v", err) + } + serverCfg, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{URI: ms.Address}) + if err != nil { + t.Fatalf("Failed to create server config for testing: %v", err) + } a, err := newAuthority(authorityArgs{ - serverCfg: testutils.ServerConfigForAddress(t, ms.Address), - bootstrapCfg: &bootstrap.Config{ - NodeProto: &v3corepb.Node{Id: nodeID}, - }, + serverCfg: serverCfg, + bootstrapCfg: config, serializer: grpcsync.NewCallbackSerializer(ctx), resourceTypeGetter: rtRegistry.get, watchExpiryTimeout: watchExpiryTimeout, diff --git a/xds/internal/xdsclient/client_new.go b/xds/internal/xdsclient/client_new.go index 8dec8f34b..3e0acd5fc 100644 --- a/xds/internal/xdsclient/client_new.go +++ b/xds/internal/xdsclient/client_new.go @@ -84,7 +84,7 @@ func newWithConfig(config *bootstrap.Config, watchExpiryTimeout time.Duration, i } c.logger = prefixLogger(c) - c.logger.Infof("Created client to xDS management server: %s", config.XDSServer) + c.logger.Infof("Created client to xDS management server: %s", config.XDSServers()[0]) return c, nil } diff --git a/xds/internal/xdsclient/clientimpl.go b/xds/internal/xdsclient/clientimpl.go index 7321250d6..9f619016a 100644 --- a/xds/internal/xdsclient/clientimpl.go +++ b/xds/internal/xdsclient/clientimpl.go @@ -85,17 +85,17 @@ func (c *clientImpl) close() { c.authorityMu.Unlock() c.serializerClose() - for _, f := range c.config.XDSServer.Cleanups { - f() - } - for _, a := range c.config.Authorities { - if a.XDSServer == nil { - // The server for this authority is the top-level one, cleaned up above. - continue - } - for _, f := range a.XDSServer.Cleanups { + for _, s := range c.config.XDSServers() { + for _, f := range s.Cleanups() { f() } } + for _, a := range c.config.Authorities() { + for _, s := range a.XDSServers { + for _, f := range s.Cleanups() { + f() + } + } + } c.logger.Infof("Shutdown") } diff --git a/xds/internal/xdsclient/clientimpl_authority.go b/xds/internal/xdsclient/clientimpl_authority.go index 69db79ee8..1ce20fabd 100644 --- a/xds/internal/xdsclient/clientimpl_authority.go +++ b/xds/internal/xdsclient/clientimpl_authority.go @@ -45,14 +45,18 @@ func (c *clientImpl) findAuthority(n *xdsresource.Name) (*authority, func(), err return nil, nil, errors.New("the xds-client is closed") } - config := c.config.XDSServer + config := c.config.XDSServers()[0] if scheme == xdsresource.FederationScheme { - cfg, ok := c.config.Authorities[authority] + authorities := c.config.Authorities() + if authorities == nil { + return nil, nil, fmt.Errorf("xds: failed to find authority %q", authority) + } + cfg, ok := authorities[authority] if !ok { return nil, nil, fmt.Errorf("xds: failed to find authority %q", authority) } - if cfg.XDSServer != nil { - config = cfg.XDSServer + if len(cfg.XDSServers) >= 1 { + config = cfg.XDSServers[0] } } @@ -110,7 +114,7 @@ func (c *clientImpl) newAuthorityLocked(config *bootstrap.ServerConfig) (_ *auth serializer: c.serializer, resourceTypeGetter: c.resourceTypes.get, watchExpiryTimeout: c.watchExpiryTimeout, - logger: grpclog.NewPrefixLogger(logger, authorityPrefix(c, config.ServerURI)), + logger: grpclog.NewPrefixLogger(logger, authorityPrefix(c, config.ServerURI())), }) if err != nil { return nil, fmt.Errorf("creating new authority for config %q: %v", config.String(), err) diff --git a/xds/internal/xdsclient/clientimpl_dump.go b/xds/internal/xdsclient/clientimpl_dump.go index 8fbc010f7..0fa75fc6b 100644 --- a/xds/internal/xdsclient/clientimpl_dump.go +++ b/xds/internal/xdsclient/clientimpl_dump.go @@ -40,7 +40,7 @@ func (c *clientImpl) DumpResources() (*v3statuspb.ClientStatusResponse, error) { Config: []*v3statuspb.ClientConfig{ { // TODO: Populate ClientScope. Need to update go-control-plane dependency. - Node: c.config.NodeProto, + Node: c.config.Node(), GenericXdsConfigs: retCfg, }, }, diff --git a/xds/internal/xdsclient/loadreport_test.go b/xds/internal/xdsclient/loadreport_test.go index 42037243b..342594a93 100644 --- a/xds/internal/xdsclient/loadreport_test.go +++ b/xds/internal/xdsclient/loadreport_test.go @@ -27,8 +27,8 @@ import ( "google.golang.org/grpc/codes" "google.golang.org/grpc/internal/testutils/xds/e2e" "google.golang.org/grpc/internal/testutils/xds/fakeserver" + "google.golang.org/grpc/internal/xds/bootstrap" "google.golang.org/grpc/status" - xdstestutils "google.golang.org/grpc/xds/internal/testutils" "google.golang.org/protobuf/testing/protocmp" v3endpointpb "github.com/envoyproxy/go-control-plane/envoy/config/endpoint/v3" @@ -44,7 +44,10 @@ func (s) TestLRSClient(t *testing.T) { defer sCleanup() nodeID := uuid.New().String() - serverCfg1 := xdstestutils.ServerConfigForAddress(t, fs1.Address) + serverCfg1, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{URI: fs1.Address}) + if err != nil { + t.Fatalf("Failed to create server config for testing: %v", err) + } bc, err := e2e.DefaultBootstrapContents(nodeID, fs1.Address) if err != nil { t.Fatalf("Failed to create bootstrap configuration: %v", err) @@ -78,8 +81,11 @@ func (s) TestLRSClient(t *testing.T) { defer sCleanup2() // Report to a different address should create new ClientConn. - serverCgf2 := xdstestutils.ServerConfigForAddress(t, fs2.Address) - store2, lrsCancel2 := xdsC.ReportLoad(serverCgf2) + serverCfg2, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{URI: fs2.Address}) + if err != nil { + t.Fatalf("Failed to create server config for testing: %v", err) + } + store2, lrsCancel2 := xdsC.ReportLoad(serverCfg2) defer lrsCancel2() if u, err := fs2.NewConnChan.Receive(ctx); err != nil { t.Errorf("unexpected timeout: %v, %v, want NewConn", u, err) diff --git a/xds/internal/xdsclient/singleton.go b/xds/internal/xdsclient/singleton.go index f981bfebb..ce1b8a8f4 100644 --- a/xds/internal/xdsclient/singleton.go +++ b/xds/internal/xdsclient/singleton.go @@ -94,7 +94,7 @@ func newRefCountedWithConfig(fallbackConfig *bootstrap.Config) (XDSClient, func( singletonClient = &clientRefCounted{clientImpl: c, refCount: 1} singletonClientImplCreateHook() - logger.Infof("xDS node ID: %s", config.NodeProto.GetId()) + logger.Infof("xDS node ID: %s", config.Node().GetId()) return singletonClient, grpcsync.OnceFunc(clientRefCountedClose), nil } diff --git a/xds/internal/xdsclient/tests/resource_update_test.go b/xds/internal/xdsclient/tests/resource_update_test.go index 5a684a00e..7451e67ab 100644 --- a/xds/internal/xdsclient/tests/resource_update_test.go +++ b/xds/internal/xdsclient/tests/resource_update_test.go @@ -32,8 +32,8 @@ import ( "google.golang.org/grpc/internal/testutils" "google.golang.org/grpc/internal/testutils/xds/e2e" "google.golang.org/grpc/internal/testutils/xds/fakeserver" + "google.golang.org/grpc/internal/xds/bootstrap" "google.golang.org/grpc/xds/internal" - xdstestutils "google.golang.org/grpc/xds/internal/testutils" "google.golang.org/grpc/xds/internal/xdsclient" "google.golang.org/grpc/xds/internal/xdsclient/xdsresource" "google.golang.org/protobuf/proto" @@ -867,7 +867,11 @@ func (s) TestHandleClusterResponseFromManagementServer(t *testing.T) { // server at that point, hence we do it here before verifying the // received update. if test.wantErr == "" { - test.wantUpdate.LRSServerConfig = xdstestutils.ServerConfigForAddress(t, mgmtServer.Address) + serverCfg, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{URI: mgmtServer.Address}) + if err != nil { + t.Fatalf("Failed to create server config for testing: %v", err) + } + test.wantUpdate.LRSServerConfig = serverCfg } cmpOpts := []cmp.Option{ cmpopts.EquateEmpty(), diff --git a/xds/internal/xdsclient/transport/internal/internal.go b/xds/internal/xdsclient/transport/internal/internal.go new file mode 100644 index 000000000..9acc33cbb --- /dev/null +++ b/xds/internal/xdsclient/transport/internal/internal.go @@ -0,0 +1,25 @@ +/* + * + * Copyright 2024 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// Package internal contains functionality internal to the transport package. +package internal + +// The following vars can be overridden by tests. +var ( + // GRPCNewClient creates a new gRPC Client. + GRPCNewClient any // func(string, ...grpc.DialOption) (*grpc.ClientConn, error) +) diff --git a/xds/internal/xdsclient/transport/loadreport_test.go b/xds/internal/xdsclient/transport/loadreport_test.go index e56102ac0..cd5e5e352 100644 --- a/xds/internal/xdsclient/transport/loadreport_test.go +++ b/xds/internal/xdsclient/transport/loadreport_test.go @@ -27,7 +27,7 @@ import ( "github.com/google/go-cmp/cmp/cmpopts" "github.com/google/uuid" "google.golang.org/grpc/internal/testutils/xds/fakeserver" - "google.golang.org/grpc/xds/internal/testutils" + "google.golang.org/grpc/internal/xds/bootstrap" "google.golang.org/grpc/xds/internal/xdsclient/transport" "google.golang.org/protobuf/testing/protocmp" "google.golang.org/protobuf/types/known/durationpb" @@ -58,10 +58,15 @@ func (s) TestReportLoad(t *testing.T) { defer cleanup() t.Logf("Started xDS management server on %s", mgmtServer.Address) + serverCfg, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{URI: mgmtServer.Address}) + if err != nil { + t.Fatalf("Failed to create server config for testing: %v", err) + } + // Create a transport to the fake management server. nodeProto := &v3corepb.Node{Id: uuid.New().String()} tr, err := transport.New(transport.Options{ - ServerCfg: *testutils.ServerConfigForAddress(t, mgmtServer.Address), + ServerCfg: serverCfg, NodeProto: nodeProto, OnRecvHandler: func(transport.ResourceUpdate) error { return nil }, // No ADS validation. OnErrorHandler: func(error) {}, // No ADS stream error handling. diff --git a/xds/internal/xdsclient/transport/transport.go b/xds/internal/xdsclient/transport/transport.go index 421ba7807..fb7860f98 100644 --- a/xds/internal/xdsclient/transport/transport.go +++ b/xds/internal/xdsclient/transport/transport.go @@ -36,6 +36,7 @@ import ( "google.golang.org/grpc/internal/xds/bootstrap" "google.golang.org/grpc/keepalive" "google.golang.org/grpc/xds/internal/xdsclient/load" + "google.golang.org/grpc/xds/internal/xdsclient/transport/internal" "google.golang.org/grpc/xds/internal/xdsclient/xdsresource" "google.golang.org/protobuf/types/known/anypb" @@ -135,7 +136,7 @@ type ResourceUpdate struct { type Options struct { // ServerCfg contains all the configuration required to connect to the xDS // management server. - ServerCfg bootstrap.ServerConfig + ServerCfg *bootstrap.ServerConfig // OnRecvHandler is the component which makes ACK/NACK decisions based on // the received resources. // @@ -169,16 +170,13 @@ type Options struct { NodeProto *v3corepb.Node } -// For overriding in unit tests. -var grpcDial = grpc.Dial +func init() { + internal.GRPCNewClient = grpc.NewClient +} // New creates a new Transport. func New(opts Options) (*Transport, error) { switch { - case opts.ServerCfg.ServerURI == "": - return nil, errors.New("missing server URI when creating a new transport") - case opts.ServerCfg.CredsDialOption() == nil: - return nil, errors.New("missing credentials when creating a new transport") case opts.OnRecvHandler == nil: return nil, errors.New("missing OnRecv callback handler when creating a new transport") case opts.OnErrorHandler == nil: @@ -197,11 +195,13 @@ func New(opts Options) (*Transport, error) { Timeout: 20 * time.Second, }), } - cc, err := grpcDial(opts.ServerCfg.ServerURI, dopts...) + grpcNewClient := internal.GRPCNewClient.(func(string, ...grpc.DialOption) (*grpc.ClientConn, error)) + cc, err := grpcNewClient(opts.ServerCfg.ServerURI(), dopts...) if err != nil { // An error from a non-blocking dial indicates something serious. - return nil, fmt.Errorf("failed to create a transport to the management server %q: %v", opts.ServerCfg.ServerURI, err) + return nil, fmt.Errorf("failed to create a transport to the management server %q: %v", opts.ServerCfg.ServerURI(), err) } + cc.Connect() boff := opts.Backoff if boff == nil { @@ -209,7 +209,7 @@ func New(opts Options) (*Transport, error) { } ret := &Transport{ cc: cc, - serverURI: opts.ServerCfg.ServerURI, + serverURI: opts.ServerCfg.ServerURI(), onRecvHandler: opts.OnRecvHandler, onErrorHandler: opts.OnErrorHandler, onSendHandler: opts.OnSendHandler, diff --git a/xds/internal/xdsclient/transport/transport_ack_nack_test.go b/xds/internal/xdsclient/transport/transport_ack_nack_test.go index e458101e2..b59ac0f79 100644 --- a/xds/internal/xdsclient/transport/transport_ack_nack_test.go +++ b/xds/internal/xdsclient/transport/transport_ack_nack_test.go @@ -29,7 +29,7 @@ import ( "google.golang.org/grpc/codes" "google.golang.org/grpc/internal/testutils" "google.golang.org/grpc/internal/testutils/xds/e2e" - xdstestutils "google.golang.org/grpc/xds/internal/testutils" + "google.golang.org/grpc/internal/xds/bootstrap" "google.golang.org/grpc/xds/internal/xdsclient/transport" "google.golang.org/grpc/xds/internal/xdsclient/xdsresource/version" "google.golang.org/protobuf/proto" @@ -133,9 +133,14 @@ func (s) TestSimpleAckAndNack(t *testing.T) { SkipValidation: true, }) + serverCfg, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{URI: mgmtServer.Address}) + if err != nil { + t.Fatalf("Failed to create server config for testing: %v", err) + } + // Create a new transport. tr, err := transport.New(transport.Options{ - ServerCfg: *xdstestutils.ServerConfigForAddress(t, mgmtServer.Address), + ServerCfg: serverCfg, OnRecvHandler: dataModelValidator, OnErrorHandler: func(err error) {}, OnSendHandler: func(*transport.ResourceSendInfo) {}, @@ -313,9 +318,14 @@ func (s) TestInvalidFirstResponse(t *testing.T) { SkipValidation: true, }) + serverCfg, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{URI: mgmtServer.Address}) + if err != nil { + t.Fatalf("Failed to create server config for testing: %v", err) + } + // Create a new transport. tr, err := transport.New(transport.Options{ - ServerCfg: *xdstestutils.ServerConfigForAddress(t, mgmtServer.Address), + ServerCfg: serverCfg, NodeProto: &v3corepb.Node{Id: nodeID}, OnRecvHandler: dataModelValidator, OnErrorHandler: func(err error) {}, @@ -435,9 +445,14 @@ func (s) TestResourceIsNotRequestedAnymore(t *testing.T) { SkipValidation: true, }) + serverCfg, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{URI: mgmtServer.Address}) + if err != nil { + t.Fatalf("Failed to create server config for testing: %v", err) + } + // Create a new transport. tr, err := transport.New(transport.Options{ - ServerCfg: *xdstestutils.ServerConfigForAddress(t, mgmtServer.Address), + ServerCfg: serverCfg, NodeProto: &v3corepb.Node{Id: nodeID}, OnRecvHandler: dataModelValidator, OnErrorHandler: func(err error) {}, diff --git a/xds/internal/xdsclient/transport/transport_backoff_test.go b/xds/internal/xdsclient/transport/transport_backoff_test.go index 96ba3e9dd..4f6283f88 100644 --- a/xds/internal/xdsclient/transport/transport_backoff_test.go +++ b/xds/internal/xdsclient/transport/transport_backoff_test.go @@ -30,7 +30,7 @@ import ( "google.golang.org/grpc/connectivity" "google.golang.org/grpc/internal/testutils" "google.golang.org/grpc/internal/testutils/xds/e2e" - xdstestutils "google.golang.org/grpc/xds/internal/testutils" + "google.golang.org/grpc/internal/xds/bootstrap" "google.golang.org/grpc/xds/internal/xdsclient/transport" "google.golang.org/grpc/xds/internal/xdsclient/xdsresource/version" "google.golang.org/protobuf/testing/protocmp" @@ -96,11 +96,16 @@ func (s) TestTransport_BackoffAfterStreamFailure(t *testing.T) { return 0 } + serverCfg, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{URI: mgmtServer.Address}) + if err != nil { + t.Fatalf("Failed to create server config for testing: %v", err) + } + // Create a new transport. Since we are only testing backoff behavior here, // we can pass a no-op data model layer implementation. nodeID := uuid.New().String() tr, err := transport.New(transport.Options{ - ServerCfg: *xdstestutils.ServerConfigForAddress(t, mgmtServer.Address), + ServerCfg: serverCfg, OnRecvHandler: func(transport.ResourceUpdate) error { return nil }, // No data model layer validation. OnErrorHandler: func(err error) { select { @@ -258,10 +263,15 @@ func (s) TestTransport_RetriesAfterBrokenStream(t *testing.T) { SkipValidation: true, }) + serverCfg, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{URI: mgmtServer.Address}) + if err != nil { + t.Fatalf("Failed to create server config for testing: %v", err) + } + // Create a new transport. Since we are only testing backoff behavior here, // we can pass a no-op data model layer implementation. tr, err := transport.New(transport.Options{ - ServerCfg: *xdstestutils.ServerConfigForAddress(t, mgmtServer.Address), + ServerCfg: serverCfg, OnRecvHandler: func(transport.ResourceUpdate) error { return nil }, // No data model layer validation. OnErrorHandler: func(err error) { select { @@ -389,11 +399,16 @@ func (s) TestTransport_ResourceRequestedBeforeStreamCreation(t *testing.T) { // stream to the management server. lis.Stop() + serverCfg, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{URI: mgmtServer.Address}) + if err != nil { + t.Fatalf("Failed to create server config for testing: %v", err) + } + // Create a new transport. Since we are only testing backoff behavior here, // we can pass a no-op data model layer implementation. nodeID := uuid.New().String() tr, err := transport.New(transport.Options{ - ServerCfg: *xdstestutils.ServerConfigForAddress(t, mgmtServer.Address), + ServerCfg: serverCfg, OnRecvHandler: func(transport.ResourceUpdate) error { return nil }, // No data model layer validation. OnErrorHandler: func(error) {}, // No stream error handling. OnSendHandler: func(*transport.ResourceSendInfo) {}, // No on send handler diff --git a/xds/internal/xdsclient/transport/transport_new_test.go b/xds/internal/xdsclient/transport/transport_new_test.go index 811082873..ca994735f 100644 --- a/xds/internal/xdsclient/transport/transport_new_test.go +++ b/xds/internal/xdsclient/transport/transport_new_test.go @@ -22,7 +22,6 @@ import ( "testing" "google.golang.org/grpc/internal/xds/bootstrap" - "google.golang.org/grpc/xds/internal/testutils" "google.golang.org/grpc/xds/internal/xdsclient/transport" v3corepb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" @@ -31,25 +30,20 @@ import ( // TestNew covers that New() returns an error if the input *ServerConfig // contains invalid content. func (s) TestNew(t *testing.T) { + serverCfg, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{URI: "server-address"}) + if err != nil { + t.Fatalf("Failed to create server config for testing: %v", err) + } + tests := []struct { name string opts transport.Options wantErrStr string }{ - { - name: "missing server URI", - opts: transport.Options{ServerCfg: bootstrap.ServerConfig{}}, - wantErrStr: "missing server URI when creating a new transport", - }, - { - name: "missing credentials", - opts: transport.Options{ServerCfg: bootstrap.ServerConfig{ServerURI: "server-address"}}, - wantErrStr: "missing credentials when creating a new transport", - }, { name: "missing onRecv handler", opts: transport.Options{ - ServerCfg: *testutils.ServerConfigForAddress(t, "server-address"), + ServerCfg: serverCfg, NodeProto: &v3corepb.Node{}, }, wantErrStr: "missing OnRecv callback handler when creating a new transport", @@ -57,7 +51,7 @@ func (s) TestNew(t *testing.T) { { name: "missing onError handler", opts: transport.Options{ - ServerCfg: *testutils.ServerConfigForAddress(t, "server-address"), + ServerCfg: serverCfg, NodeProto: &v3corepb.Node{}, OnRecvHandler: func(transport.ResourceUpdate) error { return nil }, OnSendHandler: func(*transport.ResourceSendInfo) {}, @@ -68,7 +62,7 @@ func (s) TestNew(t *testing.T) { { name: "missing onSend handler", opts: transport.Options{ - ServerCfg: *testutils.ServerConfigForAddress(t, "server-address"), + ServerCfg: serverCfg, NodeProto: &v3corepb.Node{}, OnRecvHandler: func(transport.ResourceUpdate) error { return nil }, OnErrorHandler: func(error) {}, @@ -78,7 +72,7 @@ func (s) TestNew(t *testing.T) { { name: "happy case", opts: transport.Options{ - ServerCfg: *testutils.ServerConfigForAddress(t, "server-address"), + ServerCfg: serverCfg, NodeProto: &v3corepb.Node{}, OnRecvHandler: func(transport.ResourceUpdate) error { return nil }, OnErrorHandler: func(error) {}, diff --git a/xds/internal/xdsclient/transport/transport_resource_test.go b/xds/internal/xdsclient/transport/transport_resource_test.go index 4816e2c7a..b1c8aaf47 100644 --- a/xds/internal/xdsclient/transport/transport_resource_test.go +++ b/xds/internal/xdsclient/transport/transport_resource_test.go @@ -30,7 +30,7 @@ import ( "google.golang.org/grpc/internal/grpctest" "google.golang.org/grpc/internal/testutils" "google.golang.org/grpc/internal/testutils/xds/fakeserver" - xdstestutils "google.golang.org/grpc/xds/internal/testutils" + "google.golang.org/grpc/internal/xds/bootstrap" "google.golang.org/grpc/xds/internal/xdsclient/transport" "google.golang.org/grpc/xds/internal/xdsclient/xdsresource/version" "google.golang.org/protobuf/testing/protocmp" @@ -175,10 +175,15 @@ func (s) TestHandleResponseFromManagementServer(t *testing.T) { t.Logf("Started xDS management server on %s", mgmtServer.Address) mgmtServer.XDSResponseChan <- &fakeserver.Response{Resp: test.managementServerResponse} + serverCfg, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{URI: mgmtServer.Address}) + if err != nil { + t.Fatalf("Failed to create server config for testing: %v", err) + } + // Create a new transport. resourcesCh := testutils.NewChannel() tr, err := transport.New(transport.Options{ - ServerCfg: *xdstestutils.ServerConfigForAddress(t, mgmtServer.Address), + ServerCfg: serverCfg, // No validation. Simply push received resources on a channel. OnRecvHandler: func(update transport.ResourceUpdate) error { resourcesCh.Send(&resourcesWithTypeURL{ @@ -226,9 +231,14 @@ func (s) TestEmptyListenerResourceOnStreamRestart(t *testing.T) { mgmtServer, cleanup := startFakeManagementServer(t) defer cleanup() t.Logf("Started xDS management server on %s", mgmtServer.Address) + + serverCfg, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{URI: mgmtServer.Address}) + if err != nil { + t.Fatalf("Failed to create server config for testing: %v", err) + } nodeProto := &v3corepb.Node{Id: uuid.New().String()} tr, err := transport.New(transport.Options{ - ServerCfg: *xdstestutils.ServerConfigForAddress(t, mgmtServer.Address), + ServerCfg: serverCfg, OnRecvHandler: func(update transport.ResourceUpdate) error { return nil }, @@ -314,9 +324,14 @@ func (s) TestEmptyClusterResourceOnStreamRestartWithListener(t *testing.T) { mgmtServer, cleanup := startFakeManagementServer(t) defer cleanup() t.Logf("Started xDS management server on %s", mgmtServer.Address) + + serverCfg, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{URI: mgmtServer.Address}) + if err != nil { + t.Fatalf("Failed to create server config for testing: %v", err) + } nodeProto := &v3corepb.Node{Id: uuid.New().String()} tr, err := transport.New(transport.Options{ - ServerCfg: *xdstestutils.ServerConfigForAddress(t, mgmtServer.Address), + ServerCfg: serverCfg, OnRecvHandler: func(update transport.ResourceUpdate) error { return nil }, diff --git a/xds/internal/xdsclient/transport/transport_test.go b/xds/internal/xdsclient/transport/transport_test.go index 9ab603f01..6ef8ccfbf 100644 --- a/xds/internal/xdsclient/transport/transport_test.go +++ b/xds/internal/xdsclient/transport/transport_test.go @@ -15,25 +15,19 @@ * limitations under the License. */ -package transport +package transport_test import ( "testing" - v3corepb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" "google.golang.org/grpc" - "google.golang.org/grpc/internal/grpctest" - xdstestutils "google.golang.org/grpc/xds/internal/testutils" + "google.golang.org/grpc/internal/xds/bootstrap" + "google.golang.org/grpc/xds/internal/xdsclient/transport" + "google.golang.org/grpc/xds/internal/xdsclient/transport/internal" + + v3corepb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" ) -type s struct { - grpctest.Tester -} - -func Test(t *testing.T) { - grpctest.RunSubTests(t, s{}) -} - func (s) TestNewWithGRPCDial(t *testing.T) { // Override the dialer with a custom one. customDialerCalled := false @@ -41,43 +35,47 @@ func (s) TestNewWithGRPCDial(t *testing.T) { customDialerCalled = true return grpc.NewClient(target, opts...) } - oldDial := grpcDial - grpcDial = customDialer - defer func() { grpcDial = oldDial }() + oldDial := internal.GRPCNewClient + internal.GRPCNewClient = customDialer + defer func() { internal.GRPCNewClient = oldDial }() - // Create a new transport and ensure that the custom dialer was called. - opts := Options{ - ServerCfg: *xdstestutils.ServerConfigForAddress(t, "server-address"), - NodeProto: &v3corepb.Node{}, - OnRecvHandler: func(ResourceUpdate) error { return nil }, - OnErrorHandler: func(error) {}, - OnSendHandler: func(*ResourceSendInfo) {}, - } - c, err := New(opts) + serverCfg, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{URI: "server-address"}) if err != nil { - t.Fatalf("New(%v) failed: %v", opts, err) + t.Fatalf("Failed to create server config for testing: %v", err) + } + // Create a new transport and ensure that the custom dialer was called. + opts := transport.Options{ + ServerCfg: serverCfg, + NodeProto: &v3corepb.Node{}, + OnRecvHandler: func(transport.ResourceUpdate) error { return nil }, + OnErrorHandler: func(error) {}, + OnSendHandler: func(*transport.ResourceSendInfo) {}, + } + c, err := transport.New(opts) + if err != nil { + t.Fatalf("transport.New(%v) failed: %v", opts, err) } defer c.Close() if !customDialerCalled { - t.Fatalf("New(%+v) custom dialer called = false, want true", opts) + t.Fatalf("transport.New(%+v) custom dialer called = false, want true", opts) } customDialerCalled = false // Reset the dialer, create a new transport and ensure that our custom // dialer is no longer called. - grpcDial = grpc.NewClient - c, err = New(opts) + internal.GRPCNewClient = grpc.NewClient + c, err = transport.New(opts) defer func() { if c != nil { c.Close() } }() if err != nil { - t.Fatalf("New(%v) failed: %v", opts, err) + t.Fatalf("transport.New(%v) failed: %v", opts, err) } if customDialerCalled { - t.Fatalf("New(%+v) custom dialer called = true, want false", opts) + t.Fatalf("transport.New(%+v) custom dialer called = true, want false", opts) } } diff --git a/xds/internal/xdsclient/xdsresource/listener_resource_type.go b/xds/internal/xdsclient/xdsresource/listener_resource_type.go index 4337e4e06..2436d72f8 100644 --- a/xds/internal/xdsclient/xdsresource/listener_resource_type.go +++ b/xds/internal/xdsclient/xdsresource/listener_resource_type.go @@ -60,12 +60,12 @@ func securityConfigValidator(bc *bootstrap.Config, sc *SecurityConfig) error { return nil } if sc.IdentityInstanceName != "" { - if _, ok := bc.CertProviderConfigs[sc.IdentityInstanceName]; !ok { + if _, ok := bc.CertProviderConfigs()[sc.IdentityInstanceName]; !ok { return fmt.Errorf("identity certificate provider instance name %q missing in bootstrap configuration", sc.IdentityInstanceName) } } if sc.RootInstanceName != "" { - if _, ok := bc.CertProviderConfigs[sc.RootInstanceName]; !ok { + if _, ok := bc.CertProviderConfigs()[sc.RootInstanceName]; !ok { return fmt.Errorf("root certificate provider instance name %q missing in bootstrap configuration", sc.RootInstanceName) } } diff --git a/xds/internal/xdsclient/xdsresource/tests/unmarshal_cds_test.go b/xds/internal/xdsclient/xdsresource/tests/unmarshal_cds_test.go index fff78fe00..dde89b272 100644 --- a/xds/internal/xdsclient/xdsresource/tests/unmarshal_cds_test.go +++ b/xds/internal/xdsclient/xdsresource/tests/unmarshal_cds_test.go @@ -99,6 +99,10 @@ func (s) TestValidateCluster_Success(t *testing.T) { return customLBConfig{}, nil }, }) + serverCfg, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{URI: "test-server"}) + if err != nil { + t.Fatalf("Failed to create server config for testing: %v", err) + } defer func(old bool) { envconfig.LeastRequestLB = old }(envconfig.LeastRequestLB) envconfig.LeastRequestLB = true @@ -214,11 +218,11 @@ func (s) TestValidateCluster_Success(t *testing.T) { ServiceName: serviceName, EnableLRS: true, }), - serverCfg: &bootstrap.ServerConfig{ServerURI: "test-server-uri"}, + serverCfg: serverCfg, wantUpdate: xdsresource.ClusterUpdate{ ClusterName: clusterName, EDSServiceName: serviceName, - LRSServerConfig: &bootstrap.ServerConfig{ServerURI: "test-server-uri"}, + LRSServerConfig: serverCfg, }, wantLBConfig: &iserviceconfig.BalancerConfig{ Name: wrrlocality.Name, @@ -251,11 +255,11 @@ func (s) TestValidateCluster_Success(t *testing.T) { } return c }(), - serverCfg: &bootstrap.ServerConfig{ServerURI: "test-server-uri"}, + serverCfg: serverCfg, wantUpdate: xdsresource.ClusterUpdate{ ClusterName: clusterName, EDSServiceName: serviceName, - LRSServerConfig: &bootstrap.ServerConfig{ServerURI: "test-server-uri"}, + LRSServerConfig: serverCfg, MaxRequests: func() *uint32 { i := uint32(512); return &i }(), }, wantLBConfig: &iserviceconfig.BalancerConfig{ diff --git a/xds/internal/xdsclient/xdsresource/unmarshal_cds_test.go b/xds/internal/xdsclient/xdsresource/unmarshal_cds_test.go index ce7846ce5..eeea3d580 100644 --- a/xds/internal/xdsclient/xdsresource/unmarshal_cds_test.go +++ b/xds/internal/xdsclient/xdsresource/unmarshal_cds_test.go @@ -1263,6 +1263,11 @@ func (s) TestUnmarshalCluster(t *testing.T) { }) ) + serverCfg, err := bootstrap.ServerConfigForTesting(bootstrap.ServerConfigTestingOptions{URI: "test-server"}) + if err != nil { + t.Fatalf("Failed to create server config for testing: %v", err) + } + tests := []struct { name string resource *anypb.Any @@ -1319,48 +1324,48 @@ func (s) TestUnmarshalCluster(t *testing.T) { { name: "v3 cluster", resource: v3ClusterAny, - serverCfg: &bootstrap.ServerConfig{ServerURI: "test-server-uri"}, + serverCfg: serverCfg, wantName: v3ClusterName, wantUpdate: ClusterUpdate{ ClusterName: v3ClusterName, EDSServiceName: v3Service, - LRSServerConfig: &bootstrap.ServerConfig{ServerURI: "test-server-uri"}, + LRSServerConfig: serverCfg, Raw: v3ClusterAny, }, }, { name: "v3 cluster wrapped", resource: testutils.MarshalAny(t, &v3discoverypb.Resource{Resource: v3ClusterAny}), - serverCfg: &bootstrap.ServerConfig{ServerURI: "test-server-uri"}, + serverCfg: serverCfg, wantName: v3ClusterName, wantUpdate: ClusterUpdate{ ClusterName: v3ClusterName, EDSServiceName: v3Service, - LRSServerConfig: &bootstrap.ServerConfig{ServerURI: "test-server-uri"}, + LRSServerConfig: serverCfg, Raw: v3ClusterAny, }, }, { name: "v3 cluster with EDS config source self", resource: v3ClusterAnyWithEDSConfigSourceSelf, - serverCfg: &bootstrap.ServerConfig{ServerURI: "test-server-uri"}, + serverCfg: serverCfg, wantName: v3ClusterName, wantUpdate: ClusterUpdate{ ClusterName: v3ClusterName, EDSServiceName: v3Service, - LRSServerConfig: &bootstrap.ServerConfig{ServerURI: "test-server-uri"}, + LRSServerConfig: serverCfg, Raw: v3ClusterAnyWithEDSConfigSourceSelf, }, }, { name: "v3 cluster with telemetry case", resource: v3ClusterAnyWithTelemetryLabels, - serverCfg: &bootstrap.ServerConfig{ServerURI: "test-server-uri"}, + serverCfg: serverCfg, wantName: v3ClusterName, wantUpdate: ClusterUpdate{ ClusterName: v3ClusterName, EDSServiceName: v3Service, - LRSServerConfig: &bootstrap.ServerConfig{ServerURI: "test-server-uri"}, + LRSServerConfig: serverCfg, Raw: v3ClusterAnyWithTelemetryLabels, TelemetryLabels: map[string]string{ "csm.service_name": "grpc-service", @@ -1371,12 +1376,12 @@ func (s) TestUnmarshalCluster(t *testing.T) { { name: "v3 metadata ignore other types not string and not com.google.csm.telemetry_labels", resource: v3ClusterAnyWithTelemetryLabelsIgnoreSome, - serverCfg: &bootstrap.ServerConfig{ServerURI: "test-server-uri"}, + serverCfg: serverCfg, wantName: v3ClusterName, wantUpdate: ClusterUpdate{ ClusterName: v3ClusterName, EDSServiceName: v3Service, - LRSServerConfig: &bootstrap.ServerConfig{ServerURI: "test-server-uri"}, + LRSServerConfig: serverCfg, Raw: v3ClusterAnyWithTelemetryLabelsIgnoreSome, TelemetryLabels: map[string]string{ "csm.service_name": "grpc-service", diff --git a/xds/server.go b/xds/server.go index 126aff067..5b29dae49 100644 --- a/xds/server.go +++ b/xds/server.go @@ -108,7 +108,7 @@ func NewGRPCServer(opts ...grpc.ServerOption) (*GRPCServer, error) { // Listener resource name template is mandatory on the server side. cfg := xdsClient.BootstrapConfig() - if cfg.ServerListenerResourceNameTemplate == "" { + if cfg.ServerListenerResourceNameTemplate() == "" { xdsClientClose() return nil, errors.New("missing server_listener_resource_name_template in the bootstrap configuration") } @@ -191,7 +191,7 @@ func (s *GRPCServer) Serve(lis net.Listener) error { // string, it will be replaced with the server's listening "IP:port" (e.g., // "0.0.0.0:8080", "[::]:8080"). cfg := s.xdsC.BootstrapConfig() - name := bootstrap.PopulateResourceTemplate(cfg.ServerListenerResourceNameTemplate, lis.Addr().String()) + name := bootstrap.PopulateResourceTemplate(cfg.ServerListenerResourceNameTemplate(), lis.Addr().String()) // Create a listenerWrapper which handles all functionality required by // this particular instance of Serve().