xds: server_features should be a child of xds_servers and not a s…

…ibling (#4087)
grpc · Dec 8, 2020 · 96a834a · 96a834a
1 parent bce1fdf
commit 96a834a
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 30 deletions.
diff --git a/xds/internal/client/bootstrap/bootstrap.go b/xds/internal/client/bootstrap/bootstrap.go
@@ -91,8 +91,9 @@ type channelCreds struct {
 }
 
 type xdsServer struct {
-	ServerURI    string         `json:"server_uri"`
-	ChannelCreds []channelCreds `json:"channel_creds"`
+	ServerURI      string         `json:"server_uri"`
+	ChannelCreds   []channelCreds `json:"channel_creds"`
+	ServerFeatures []string       `json:"server_features"`
 }
 
 // NewConfig returns a new instance of Config initialized by reading the
@@ -108,9 +109,9 @@ type xdsServer struct {
 //          "config": <JSON object containing config for the type>
 //        }
 //      ],
+//      "server_features": [ ... ],
 //    },
 //    "node": <JSON form of Node proto>,
-//    "server_features": [ ... ],
 //    "certificate_providers" : {
 //      "default": {
 //        "plugin_name": "default-plugin-name",
@@ -188,12 +189,7 @@ func NewConfig() (*Config, error) {
 					break
 				}
 			}
-		case "server_features":
-			var features []string
-			if err := json.Unmarshal(v, &features); err != nil {
-				return nil, fmt.Errorf("xds: json.Unmarshal(%v) for field %q failed during bootstrap: %v", string(v), k, err)
-			}
-			for _, f := range features {
+			for _, f := range xs.ServerFeatures {
 				switch f {
 				case serverFeaturesV3:
 					serverSupportsV3 = true

diff --git a/xds/internal/client/bootstrap/bootstrap_test.go b/xds/internal/client/bootstrap/bootstrap_test.go
@@ -163,9 +163,9 @@ var (
 				"server_uri": "trafficdirector.googleapis.com:443",
 				"channel_creds": [
 					{ "type": "google_default" }
-				]
-			}],
-			"server_features" : ["foo", "bar"]
+				],
+				"server_features" : ["foo", "bar"]
+			}]
 		}`,
 		"serverSupportsV3": `
 		{
@@ -179,9 +179,9 @@ var (
 				"server_uri": "trafficdirector.googleapis.com:443",
 				"channel_creds": [
 					{ "type": "google_default" }
-				]
-			}],
-			"server_features" : ["foo", "bar", "xds_v3"]
+				],
+				"server_features" : ["foo", "bar", "xds_v3"]
+			}]
 		}`,
 	}
 	metadata = &structpb.Struct{
@@ -548,9 +548,9 @@ func TestNewConfigWithCertificateProviders(t *testing.T) {
 				"server_uri": "trafficdirector.googleapis.com:443",
 				"channel_creds": [
 					{ "type": "google_default" }
-				]
+				],
+				"server_features" : ["foo", "bar", "xds_v3"],
 			}],
-			"server_features" : ["foo", "bar", "xds_v3"],
 			"certificate_providers": "bad JSON"
 		}`,
 		"allUnknownCertProviders": `
@@ -565,9 +565,9 @@ func TestNewConfigWithCertificateProviders(t *testing.T) {
 				"server_uri": "trafficdirector.googleapis.com:443",
 				"channel_creds": [
 					{ "type": "google_default" }
-				]
+				],
+				"server_features" : ["foo", "bar", "xds_v3"]
 			}],
-			"server_features" : ["foo", "bar", "xds_v3"],
 			"certificate_providers": {
 				"unknownProviderInstance1": {
 					"plugin_name": "foo",
@@ -591,9 +591,9 @@ func TestNewConfigWithCertificateProviders(t *testing.T) {
 				"server_uri": "trafficdirector.googleapis.com:443",
 				"channel_creds": [
 					{ "type": "google_default" }
-				]
+				],
+				"server_features" : ["foo", "bar", "xds_v3"],
 			}],
-			"server_features" : ["foo", "bar", "xds_v3"],
 			"certificate_providers": {
 				"unknownProviderInstance": {
 					"plugin_name": "foo",
@@ -617,9 +617,9 @@ func TestNewConfigWithCertificateProviders(t *testing.T) {
 				"server_uri": "trafficdirector.googleapis.com:443",
 				"channel_creds": [
 					{ "type": "google_default" }
-				]
+				],
+				"server_features" : ["foo", "bar", "xds_v3"]
 			}],
-			"server_features" : ["foo", "bar", "xds_v3"],
 			"certificate_providers": {
 				"unknownProviderInstance": {
 					"plugin_name": "foo",
@@ -692,7 +692,7 @@ func TestNewConfigWithCertificateProviders(t *testing.T) {
 
 			c, err := NewConfig()
 			if (err != nil) != test.wantErr {
-				t.Fatalf("NewConfig() returned: (%+v, %v), wantErr: %v", c.CertProviderConfigs, err, test.wantErr)
+				t.Fatalf("NewConfig() returned: %v, wantErr: %v", err, test.wantErr)
 			}
 			if test.wantErr {
 				return

diff --git a/xds/internal/testutils/e2e/bootstrap.go b/xds/internal/testutils/e2e/bootstrap.go
@@ -80,7 +80,7 @@ func SetupBootstrapFile(opts BootstrapOptions) (func(), error) {
 	case TransportV2:
 		// TODO: Add any v2 specific fields.
 	case TransportV3:
-		cfg.ServerFeatures = append(cfg.ServerFeatures, "xds_v3")
+		cfg.XdsServers[0].ServerFeatures = append(cfg.XdsServers[0].ServerFeatures, "xds_v3")
 	default:
 		return nil, fmt.Errorf("unsupported xDS transport protocol version: %v", opts.Version)
 	}
@@ -103,14 +103,14 @@ func SetupBootstrapFile(opts BootstrapOptions) (func(), error) {
 }
 
 type bootstrapConfig struct {
-	XdsServers     []server `json:"xds_servers,omitempty"`
-	Node           node     `json:"node,omitempty"`
-	ServerFeatures []string `json:"server_features,omitempty"`
+	XdsServers []server `json:"xds_servers,omitempty"`
+	Node       node     `json:"node,omitempty"`
 }
 
 type server struct {
-	ServerURI    string  `json:"server_uri,omitempty"`
-	ChannelCreds []creds `json:"channel_creds,omitempty"`
+	ServerURI      string   `json:"server_uri,omitempty"`
+	ChannelCreds   []creds  `json:"channel_creds,omitempty"`
+	ServerFeatures []string `json:"server_features,omitempty"`
 }
 
 type creds struct {