From 709091fe14006fc0ee8ee57db32439d8ff88c36d Mon Sep 17 00:00:00 2001 From: Menghan Li Date: Wed, 8 Apr 2020 13:57:52 -0700 Subject: [PATCH] service config: move balancer config parsing to internal (#3504) And make it a json unmarshaller, easy to use. --- internal/serviceconfig/serviceconfig.go | 89 ++++++++++++ internal/serviceconfig/serviceconfig_test.go | 135 +++++++++++++++++++ service_config.go | 44 +----- 3 files changed, 230 insertions(+), 38 deletions(-) create mode 100644 internal/serviceconfig/serviceconfig.go create mode 100644 internal/serviceconfig/serviceconfig_test.go diff --git a/internal/serviceconfig/serviceconfig.go b/internal/serviceconfig/serviceconfig.go new file mode 100644 index 00000000..87f20a3f --- /dev/null +++ b/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") +} diff --git a/internal/serviceconfig/serviceconfig_test.go b/internal/serviceconfig/serviceconfig_test.go new file mode 100644 index 00000000..b8abaae0 --- /dev/null +++ b/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)) + } + }) + } +} diff --git a/service_config.go b/service_config.go index 5a80a575..c8267fc8 100644 --- a/service_config.go +++ b/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, } }