Skip to content

xds: env var protection for xds fallback #7483

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 7, 2024
Merged

xds: env var protection for xds fallback #7483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions internal/envconfig/envconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ var (
// by setting the environment variable "GRPC_ENFORCE_ALPN_ENABLED" to "true"
// or "false".
EnforceALPNEnabled = boolFromEnv("GRPC_ENFORCE_ALPN_ENABLED", false)
// XDSFallbackSupport is the env variable that controls whether support for
// xDS fallback is turned on. If this is unset or is false, only the first
// xDS server in the list of server configs will be used.
XDSFallbackSupport = boolFromEnv("GRPC_EXPERIMENTAL_XDS_FALLBACK", false)
)

func boolFromEnv(envVar string, def bool) bool {
Expand Down
43 changes: 38 additions & 5 deletions internal/xds/bootstrap/bootstrap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,39 @@ func (cc ChannelCreds) String() string {
return cc.Type + "-" + string(b)
}

// ServerConfigs represents a collection of server configurations.
type ServerConfigs []*ServerConfig

// Equal returns true if scs equals other.
func (scs *ServerConfigs) Equal(other *ServerConfigs) bool {
if len(*scs) != len(*other) {
return false
}
for i := range *scs {
if !(*scs)[i].Equal((*other)[i]) {
return false
}
}
return true
}

// UnmarshalJSON takes the json data (a list of server configurations) and
// unmarshals it to the struct.
func (scs *ServerConfigs) UnmarshalJSON(data []byte) error {
servers := []*ServerConfig{}
if err := json.Unmarshal(data, &servers); err != nil {
return fmt.Errorf("xds: failed to JSON unmarshal server configurations during bootstrap: %v, config:\n%s", err, string(data))
}
// Only use the first server config if fallback support is disabled.
if !envconfig.XDSFallbackSupport {
if len(servers) > 1 {
servers = servers[:1]
}
}
*scs = servers
return nil
}

// Authority contains configuration for an xDS control plane authority.
//
// This type does not implement custom JSON marshal/unmarshal logic because it
Expand All @@ -104,7 +137,7 @@ type Authority struct {
// "xdstp://<authority_name>/envoy.config.listener.v3.Listener/%s".
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"`
XDSServers ServerConfigs `json:"xds_servers,omitempty"`
}

// Equal returns true if a equals other.
Expand All @@ -116,7 +149,7 @@ func (a *Authority) Equal(other *Authority) bool {
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) }):
case !a.XDSServers.Equal(&other.XDSServers):
return false
}
return true
Expand Down Expand Up @@ -315,7 +348,7 @@ func ServerConfigForTesting(opts ServerConfigTestingOptions) (*ServerConfig, err
// Config is the internal representation of the bootstrap configuration provided
// to the xDS client.
type Config struct {
xDSServers []*ServerConfig
xDSServers ServerConfigs
cpcs map[string]certproviderNameAndConfig
serverListenerResourceNameTemplate string
clientDefaultListenerResourceNameTemplate string
Expand Down Expand Up @@ -405,7 +438,7 @@ func (c *Config) Equal(other *Config) bool {
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) }):
case !c.xDSServers.Equal(&other.xDSServers):
return false
case !maps.EqualFunc(c.certProviderConfigs, other.certProviderConfigs, func(a, b *certprovider.BuildableConfig) bool { return a.String() == b.String() }):
return false
Expand All @@ -429,7 +462,7 @@ func (c *Config) String() string {

// The following fields correspond 1:1 with the JSON schema for Config.
type configJSON struct {
XDSServers []*ServerConfig `json:"xds_servers,omitempty"`
XDSServers ServerConfigs `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"`
Expand Down
79 changes: 79 additions & 0 deletions internal/xds/bootstrap/bootstrap_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,10 @@ func (s) TestGetConfiguration_Success(t *testing.T) {

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
origFallbackEnv := envconfig.XDSFallbackSupport
envconfig.XDSFallbackSupport = true
defer func() { envconfig.XDSFallbackSupport = origFallbackEnv }()

testGetConfigurationWithFileNameEnv(t, test.name, false, test.wantConfig)
testGetConfigurationWithFileContentEnv(t, test.name, false, test.wantConfig)
})
Expand Down Expand Up @@ -1194,3 +1198,78 @@ func (s) TestNode_ToProto(t *testing.T) {
})
}
}

// Tests the case where the xDS fallback env var is set to false, and verifies
// that only the first server from the list of server configurations is used.
func (s) TestGetConfiguration_FallbackDisabled(t *testing.T) {
// TODO(easwars): Default value of "GRPC_EXPERIMENTAL_XDS_FALLBACK"
// env var is currently false. When the default is changed to true,
// explicitly set it to false here.
Comment on lines +1205 to +1207
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional: explicitly state what the test expects?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole test is expected to be run with the env var set to false. This is stated in the name of the test and the top-level comment.

Copy link
Contributor

@arvindbr8 arvindbr8 Aug 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i meant

	origFallbackEnv := envconfig.XDSFallbackSupport
	envconfig.XDSFallbackSupport = false
	defer func() { envconfig.XDSFallbackSupport = origFallbackEnv }()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry. Should have done that. But now, I will just wait for the default value to change before doing that :)


cancel := setupBootstrapOverride(map[string]string{
"multipleXDSServers": `
{
"node": {
"id": "ENVOY_NODE_ID",
"metadata": {
"TRAFFICDIRECTOR_GRPC_HOSTNAME": "trafficdirector"
}
},
"xds_servers" : [
{
"server_uri": "trafficdirector.googleapis.com:443",
"channel_creds": [{ "type": "google_default" }],
"server_features": ["xds_v3"]
},
{
"server_uri": "backup.never.use.com:1234",
"channel_creds": [{ "type": "google_default" }]
}
],
"authorities": {
"xds.td.com": {
"xds_servers": [
{
"server_uri": "td.com",
"channel_creds": [ { "type": "google_default" } ],
"server_features" : ["xds_v3"]
},
{
"server_uri": "backup.never.use.com:1234",
"channel_creds": [{ "type": "google_default" }]
}
]
}
}
}`,
})
defer cancel()

wantConfig := &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",
authorities: map[string]*Authority{
"xds.td.com": {
ClientListenerResourceNameTemplate: "xdstp://xds.td.com/envoy.config.listener.v3.Listener/%s",
XDSServers: []*ServerConfig{{
serverURI: "td.com",
channelCreds: []ChannelCreds{{Type: "google_default"}},
serverFeatures: []string{"xds_v3"},
selectedCreds: ChannelCreds{Type: "google_default"},
}},
},
},
}
t.Run("bootstrap_file_name", func(t *testing.T) {
testGetConfigurationWithFileNameEnv(t, "multipleXDSServers", false, wantConfig)
})
t.Run("bootstrap_file_contents", func(t *testing.T) {
testGetConfigurationWithFileContentEnv(t, "multipleXDSServers", false, wantConfig)
})
}