service config: move balancer config parsing to internal (#3504)

And make it a json unmarshaller, easy to use.

internal/serviceconfig/serviceconfig.go

@@ -0,0 +1,89 @@
+/*
+ *
+ * Copyright 2020 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 serviceconfig
+
+import (
+	"encoding/json"
+	"fmt"
+
+	"google.golang.org/grpc/balancer"
+	"google.golang.org/grpc/grpclog"
+	externalserviceconfig "google.golang.org/grpc/serviceconfig"
+)
+
+// BalancerConfig is the balancer config part that service config's
+// loadBalancingConfig fields can be unmarshalled to. It's a json unmarshaller.
+//
+// https://github.com/grpc/grpc-proto/blob/54713b1e8bc6ed2d4f25fb4dff527842150b91b2/grpc/service_config/service_config.proto#L247
+type BalancerConfig struct {
+	Name   string
+	Config externalserviceconfig.LoadBalancingConfig
+}
+
+type intermediateBalancerConfig []map[string]json.RawMessage
+
+// UnmarshalJSON implements json unmarshaller.
+func (bc *BalancerConfig) UnmarshalJSON(b []byte) error {
+	var ir intermediateBalancerConfig
+	err := json.Unmarshal(b, &ir)
+	if err != nil {
+		return err
+	}
+
+	for i, lbcfg := range ir {
+		if len(lbcfg) != 1 {
+			return fmt.Errorf("invalid loadBalancingConfig: entry %v does not contain exactly 1 policy/config pair: %q", i, lbcfg)
+		}
+		var (
+			name    string
+			jsonCfg json.RawMessage
+		)
+		// Get the key:value pair from the map.
+		for name, jsonCfg = range lbcfg {
+		}
+		builder := balancer.Get(name)
+		if builder == nil {
+			// If the balancer is not registered, move on to the next config.
+			// This is not an error.
+			continue
+		}
+		bc.Name = name
+
+		parser, ok := builder.(balancer.ConfigParser)
+		if !ok {
+			if string(jsonCfg) != "{}" {
+				grpclog.Warningf("non-empty balancer configuration %q, but balancer does not implement ParseConfig", string(jsonCfg))
+			}
+			// Stop at this, though the builder doesn't support parsing config.
+			return nil
+		}
+
+		cfg, err := parser.ParseConfig(jsonCfg)
+		if err != nil {
+			return fmt.Errorf("error parsing loadBalancingConfig for policy %q: %v", name, err)
+		}
+		bc.Config = cfg
+		return nil
+	}
+	// This is reached when the for loop iterates over all entries, but didn't
+	// return. This means we had a loadBalancingConfig slice but did not
+	// encounter a registered policy. The config is considered invalid in this
+	// case.
+	return fmt.Errorf("invalid loadBalancingConfig: no supported policies found")
+}

internal/serviceconfig/serviceconfig_test.go

@@ -0,0 +1,135 @@
+/*
+ *
+ * Copyright 2020 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 serviceconfig
+
+import (
+	"encoding/json"
+	"fmt"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"google.golang.org/grpc/balancer"
+	externalserviceconfig "google.golang.org/grpc/serviceconfig"
+)
+
+type testBalancerConfigType struct {
+	externalserviceconfig.LoadBalancingConfig
+}
+
+var testBalancerConfig = testBalancerConfigType{}
+
+const (
+	testBalancerBuilderName          = "test-bb"
+	testBalancerBuilderNotParserName = "test-bb-not-parser"
+
+	testBalancerConfigJSON = `{"test-balancer-config":"true"}`
+)
+
+type testBalancerBuilder struct {
+	balancer.Builder
+}
+
+func (testBalancerBuilder) ParseConfig(js json.RawMessage) (externalserviceconfig.LoadBalancingConfig, error) {
+	if string(js) != testBalancerConfigJSON {
+		return nil, fmt.Errorf("unexpected config json")
+	}
+	return testBalancerConfig, nil
+}
+
+func (testBalancerBuilder) Name() string {
+	return testBalancerBuilderName
+}
+
+type testBalancerBuilderNotParser struct {
+	balancer.Builder
+}
+
+func (testBalancerBuilderNotParser) Name() string {
+	return testBalancerBuilderNotParserName
+}
+
+func init() {
+	balancer.Register(testBalancerBuilder{})
+	balancer.Register(testBalancerBuilderNotParser{})
+}
+
+func TestBalancerConfigUnmarshalJSON(t *testing.T) {
+	tests := []struct {
+		name    string
+		json    string
+		want    BalancerConfig
+		wantErr bool
+	}{
+		{
+			name:    "empty json",
+			json:    "",
+			wantErr: true,
+		},
+		{
+			// The config should be a slice of maps, but each map should have
+			// exactly one entry.
+			name:    "more than one entry for a map",
+			json:    `[{"balancer1":"1","balancer2":"2"}]`,
+			wantErr: true,
+		},
+		{
+			name:    "no balancer registered",
+			json:    `[{"balancer1":"1"},{"balancer2":"2"}]`,
+			wantErr: true,
+		},
+		{
+			name: "OK",
+			json: fmt.Sprintf("[{%q: %v}]", testBalancerBuilderName, testBalancerConfigJSON),
+			want: BalancerConfig{
+				Name:   testBalancerBuilderName,
+				Config: testBalancerConfig,
+			},
+			wantErr: false,
+		},
+		{
+			name: "first balancer not registered",
+			json: fmt.Sprintf(`[{"balancer1":"1"},{%q: %v}]`, testBalancerBuilderName, testBalancerConfigJSON),
+			want: BalancerConfig{
+				Name:   testBalancerBuilderName,
+				Config: testBalancerConfig,
+			},
+			wantErr: false,
+		},
+		{
+			name: "balancer registered but builder not parser",
+			json: fmt.Sprintf("[{%q: %v}]", testBalancerBuilderNotParserName, testBalancerConfigJSON),
+			want: BalancerConfig{
+				Name:   testBalancerBuilderNotParserName,
+				Config: nil,
+			},
+			wantErr: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var bc BalancerConfig
+			if err := bc.UnmarshalJSON([]byte(tt.json)); (err != nil) != tt.wantErr {
+				t.Errorf("UnmarshalJSON() error = %v, wantErr %v", err, tt.wantErr)
+			}
+			if !cmp.Equal(bc, tt.want) {
+				t.Errorf("diff: %v", cmp.Diff(bc, tt.want))
+			}
+		})
+	}
+}

service_config.go

@@ -25,10 +25,10 @@ import (
 	"strings"
 	"time"
 
-	"google.golang.org/grpc/balancer"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/grpclog"
 	"google.golang.org/grpc/internal"
+	internalserviceconfig "google.golang.org/grpc/internal/serviceconfig"
 	"google.golang.org/grpc/serviceconfig"
 )
 
@@ -249,12 +249,10 @@ type jsonMC struct {
 	RetryPolicy             *jsonRetryPolicy
 }
 
-type loadBalancingConfig map[string]json.RawMessage
-
 // TODO(lyuxuan): delete this struct after cleaning up old service config implementation.
 type jsonSC struct {
 	LoadBalancingPolicy *string
-	LoadBalancingConfig *[]loadBalancingConfig
+	LoadBalancingConfig *internalserviceconfig.BalancerConfig
 	MethodConfig        *[]jsonMC
 	RetryThrottling     *retryThrottlingPolicy
 	HealthCheckConfig   *healthCheckConfig
@@ -280,40 +278,10 @@ func parseServiceConfig(js string) *serviceconfig.ParseResult {
 		healthCheckConfig: rsc.HealthCheckConfig,
 		rawJSONString:     js,
 	}
-	if rsc.LoadBalancingConfig != nil {
-		for i, lbcfg := range *rsc.LoadBalancingConfig {
-			if len(lbcfg) != 1 {
-				err := fmt.Errorf("invalid loadBalancingConfig: entry %v does not contain exactly 1 policy/config pair: %q", i, lbcfg)
-				grpclog.Warningf(err.Error())
-				return &serviceconfig.ParseResult{Err: err}
-			}
-			var name string
-			var jsonCfg json.RawMessage
-			for name, jsonCfg = range lbcfg {
-			}
-			builder := balancer.Get(name)
-			if builder == nil {
-				continue
-			}
-			sc.lbConfig = &lbConfig{name: name}
-			if parser, ok := builder.(balancer.ConfigParser); ok {
-				var err error
-				sc.lbConfig.cfg, err = parser.ParseConfig(jsonCfg)
-				if err != nil {
-					return &serviceconfig.ParseResult{Err: fmt.Errorf("error parsing loadBalancingConfig for policy %q: %v", name, err)}
-				}
-			} else if string(jsonCfg) != "{}" {
-				grpclog.Warningf("non-empty balancer configuration %q, but balancer does not implement ParseConfig", string(jsonCfg))
-			}
-			break
-		}
-		if sc.lbConfig == nil {
-			// We had a loadBalancingConfig field but did not encounter a
-			// supported policy.  The config is considered invalid in this
-			// case.
-			err := fmt.Errorf("invalid loadBalancingConfig: no supported policies found")
-			grpclog.Warningf(err.Error())
-			return &serviceconfig.ParseResult{Err: err}
+	if c := rsc.LoadBalancingConfig; c != nil {
+		sc.lbConfig = &lbConfig{
+			name: c.Name,
+			cfg:  c.Config,
 		}
 	}