diff --git a/balancer/rls/internal/builder.go b/balancer/rls/internal/builder.go index f8c300ec0d34..191f95c3a251 100644 --- a/balancer/rls/internal/builder.go +++ b/balancer/rls/internal/builder.go @@ -22,19 +22,7 @@ package rls import ( - "encoding/json" - "fmt" "time" - - "github.com/golang/protobuf/ptypes" - "google.golang.org/grpc/balancer" - "google.golang.org/grpc/balancer/rls/internal/keys" - "google.golang.org/grpc/grpclog" - "google.golang.org/grpc/internal/grpcutil" - "google.golang.org/grpc/resolver" - "google.golang.org/grpc/serviceconfig" - - rlspb "google.golang.org/grpc/balancer/rls/internal/proto/grpc_lookup_v1" ) const ( @@ -51,15 +39,10 @@ const ( dummyChildPolicyTarget = "target_name_to_be_filled_in_later" ) -func init() { - balancer.Register(&rlsBB{}) -} - // rlsBB helps build RLS load balancers and parse the service config to be // passed to the RLS load balancer. type rlsBB struct { - // TODO(easwars): Implement the Build() method and remove this embedding. - balancer.Builder + // TODO(easwars): Implement the Build() method and register the builder. } // Name returns the name of the RLS LB policy and helps implement the @@ -67,184 +50,3 @@ type rlsBB struct { func (*rlsBB) Name() string { return rlsBalancerName } - -// ParseConfig parses and validates the JSON representation of the service -// config and returns the loadBalancingConfig to be used by the RLS LB policy. -// -// Helps implement the balancer.ConfigParser interface. -// -// The following validation checks are performed: -// * routeLookupConfig: -// ** grpc_keybuilders field: -// - must have at least one entry -// - must not have two entries with the same Name -// - must not have any entry with a Name with the service field unset or -// empty -// - must not have any entries without a Name -// - must not have a headers entry that has required_match set -// - must not have two headers entries with the same key within one entry -// ** lookup_service field: -// - must be set and non-empty and must parse as a target URI -// ** max_age field: -// - if not specified or is greater than maxMaxAge, it will be reset to -// maxMaxAge -// ** stale_age field: -// - if the value is greater than or equal to max_age, it is ignored -// - if set, then max_age must also be set -// ** valid_targets field: -// - will be ignored -// ** cache_size_bytes field: -// - must be greater than zero -// - TODO(easwars): Define a minimum value for this field, to be used when -// left unspecified -// ** request_processing_strategy field: -// - must have a value other than STRATEGY_UNSPECIFIED -// - if set to SYNC_LOOKUP_DEFAULT_TARGET_ON_ERROR or -// ASYNC_LOOKUP_DEFAULT_TARGET_ON_MISS, the default_target field must be -// set to a non-empty value -// * childPolicy field: -// - must find a valid child policy with a valid config (the child policy must -// be able to parse the provided config successfully when we pass it a dummy -// target name in the target_field provided by the -// childPolicyConfigTargetFieldName field) -// * childPolicyConfigTargetFieldName field: -// - must be set and non-empty -func (*rlsBB) ParseConfig(c json.RawMessage) (serviceconfig.LoadBalancingConfig, error) { - cfg := &lbParsedConfig{} - if err := json.Unmarshal(c, cfg); err != nil { - return nil, fmt.Errorf("rls: json unmarshal failed for service config {%+v}: %v", string(c), err) - } - - kbMap, err := keys.MakeBuilderMap(cfg.rlsProto) - if err != nil { - return nil, err - } - - lookupService := cfg.rlsProto.GetLookupService() - if lookupService == "" { - return nil, fmt.Errorf("rls: empty lookup_service in service config {%+v}", string(c)) - } - parsedTarget := grpcutil.ParseTarget(lookupService) - if parsedTarget.Scheme == "" { - parsedTarget.Scheme = resolver.GetDefaultScheme() - } - if resolver.Get(parsedTarget.Scheme) == nil { - return nil, fmt.Errorf("rls: invalid target URI in lookup_service {%s}", lookupService) - } - - var lookupServiceTimeout, maxAge, staleAge time.Duration - - if lst := cfg.rlsProto.GetLookupServiceTimeout(); lst != nil { - lookupServiceTimeout, err = ptypes.Duration(lst) - if err != nil { - return nil, fmt.Errorf("rls: failed to parse lookup_service_timeout in service config {%+v}", string(c)) - } - } - if lookupServiceTimeout == 0 { - lookupServiceTimeout = defaultLookupServiceTimeout - } - - if ma := cfg.rlsProto.GetMaxAge(); ma != nil { - maxAge, err = ptypes.Duration(ma) - if err != nil { - return nil, fmt.Errorf("rls: failed to parse max_age in service config {%+v}", string(c)) - } - } - if sa := cfg.rlsProto.GetStaleAge(); sa != nil { - staleAge, err = ptypes.Duration(sa) - if err != nil { - return nil, fmt.Errorf("rls: failed to parse staleAge in service config {%+v}", string(c)) - } - } - if staleAge != 0 && maxAge == 0 { - return nil, fmt.Errorf("rls: stale_age is set, but max_age is not in service config {%+v}", string(c)) - } - if staleAge >= maxAge { - grpclog.Info("rls: stale_age {%v} is greater than max_age {%v}, ignoring it", staleAge, maxAge) - staleAge = 0 - } - if maxAge == 0 || maxAge > maxMaxAge { - grpclog.Infof("rls: max_age in service config is %v, using %v", maxAge, maxMaxAge) - maxAge = maxMaxAge - } - - cacheSizeBytes := cfg.rlsProto.GetCacheSizeBytes() - if cacheSizeBytes <= 0 { - return nil, fmt.Errorf("rls: cache_size_bytes must be greater than 0 in service config {%+v}", string(c)) - } - - rpStrategy := cfg.rlsProto.GetRequestProcessingStrategy() - if rpStrategy == rlspb.RouteLookupConfig_STRATEGY_UNSPECIFIED { - return nil, fmt.Errorf("rls: request_processing_strategy cannot be left unspecified in service config {%+v}", string(c)) - } - defaultTarget := cfg.rlsProto.GetDefaultTarget() - if (rpStrategy == rlspb.RouteLookupConfig_SYNC_LOOKUP_DEFAULT_TARGET_ON_ERROR || - rpStrategy == rlspb.RouteLookupConfig_ASYNC_LOOKUP_DEFAULT_TARGET_ON_MISS) && defaultTarget == "" { - return nil, fmt.Errorf("rls: request_processing_strategy is %s, but default_target is not set", rpStrategy.String()) - } - - if cfg.childPolicy == nil { - return nil, fmt.Errorf("rls: childPolicy is invalid in service config {%+v}", string(c)) - } - if cfg.targetField == "" { - return nil, fmt.Errorf("rls: childPolicyConfigTargetFieldName field is not set in service config {%+v}", string(c)) - } - cpCfg, err := validateChildPolicyConfig(cfg) - if err != nil { - return nil, err - } - - return &lbConfig{ - kbMap: kbMap, - lookupService: parsedTarget, - lookupServiceTimeout: lookupServiceTimeout, - maxAge: maxAge, - staleAge: staleAge, - cacheSizeBytes: cacheSizeBytes, - rpStrategy: rpStrategy, - cpName: cfg.childPolicy.Name, - cpTargetField: cfg.targetField, - cpConfig: cpCfg, - }, nil -} - -// validateChildPolicyConfig validates the child policy config received in the -// service config. This makes it possible for us to reject service configs -// which contain invalid child policy configs which we know will fail for sure. -// -// It does the following: -// * Unmarshals the provided child policy config into a map of string to -// json.RawMessage. This allows us to add an entry to the map corresponding -// to the targetFieldName that we received in the service config. -// * Marshals the map back into JSON, finds the config parser associated with -// the child policy and asks it to validate the config. -// * If the validation succeeded, removes the dummy entry from the map and -// returns it. If any of the above steps failed, it returns an error. -func validateChildPolicyConfig(cfg *lbParsedConfig) (map[string]json.RawMessage, error) { - var childConfig map[string]json.RawMessage - if err := json.Unmarshal(cfg.childPolicy.Config, &childConfig); err != nil { - return nil, fmt.Errorf("rls: json unmarshal failed for child policy config {%+v}: %v", cfg.childPolicy.Config, err) - } - childConfig[cfg.targetField], _ = json.Marshal(dummyChildPolicyTarget) - - jsonCfg, err := json.Marshal(childConfig) - if err != nil { - return nil, fmt.Errorf("rls: json marshal failed for child policy config {%+v}: %v", childConfig, err) - } - builder := balancer.Get(cfg.childPolicy.Name) - if builder == nil { - // This should never happen since we already made sure that the child - // policy name mentioned in the service config is a valid one. - return nil, fmt.Errorf("rls: balancer builder not found for child_policy %v", cfg.childPolicy.Name) - } - parser, ok := builder.(balancer.ConfigParser) - if !ok { - return nil, fmt.Errorf("rls: balancer builder for child_policy does not implement balancer.ConfigParser: %v", cfg.childPolicy.Name) - } - _, err = parser.ParseConfig(jsonCfg) - if err != nil { - return nil, fmt.Errorf("rls: childPolicy config validation failed: %v", err) - } - delete(childConfig, cfg.targetField) - return childConfig, nil -} diff --git a/balancer/rls/internal/builder_test.go b/balancer/rls/internal/builder_test.go deleted file mode 100644 index 66c11ff8c061..000000000000 --- a/balancer/rls/internal/builder_test.go +++ /dev/null @@ -1,488 +0,0 @@ -/* - * - * 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 rls - -import ( - "encoding/json" - "fmt" - "strings" - "testing" - "time" - - "github.com/google/go-cmp/cmp" - "google.golang.org/grpc/balancer" - "google.golang.org/grpc/resolver" - - _ "google.golang.org/grpc/balancer/grpclb" // grpclb for config parsing. - rlspb "google.golang.org/grpc/balancer/rls/internal/proto/grpc_lookup_v1" - _ "google.golang.org/grpc/internal/resolver/passthrough" // passthrough resolver. -) - -const balancerWithoutConfigParserName = "dummy_balancer" - -type dummyBB struct { - balancer.Builder -} - -func (*dummyBB) Name() string { - return balancerWithoutConfigParserName -} - -func init() { - balancer.Register(&dummyBB{}) -} - -func (lbCfg *lbConfig) Equal(other *lbConfig) bool { - // This only ignores the keyBuilderMap field because its internals are not - // exported, and hence not possible to specify in the want section of the - // test. - return lbCfg.lookupService == other.lookupService && - lbCfg.lookupServiceTimeout == other.lookupServiceTimeout && - lbCfg.maxAge == other.maxAge && - lbCfg.staleAge == other.staleAge && - lbCfg.cacheSizeBytes == other.cacheSizeBytes && - lbCfg.rpStrategy == other.rpStrategy && - lbCfg.cpName == other.cpName && - lbCfg.cpTargetField == other.cpTargetField && - cmp.Equal(lbCfg.cpConfig, other.cpConfig) -} - -func TestParseConfig(t *testing.T) { - tests := []struct { - desc string - input []byte - wantCfg *lbConfig - }{ - // Includes the following transformations: - // - lookupServiceTimeout is set to its default value, since it is not specified in the input. - // - maxAge is set to maxMaxAge since the value is too large in the input. - // - staleAge is ignore because it is higher than maxAge in the input. - { - desc: "with transformations", - input: []byte(`{ - "routeLookupConfig": { - "grpcKeybuilders": [{ - "names": [{"service": "service", "method": "method"}], - "headers": [{"key": "k1", "names": ["v1"]}] - }], - "lookupService": "passthrough:///target", - "maxAge" : "500s", - "staleAge": "600s", - "cacheSizeBytes": 1000, - "request_processing_strategy": "ASYNC_LOOKUP_DEFAULT_TARGET_ON_MISS", - "defaultTarget": "passthrough:///default" - }, - "childPolicy": [ - {"cds_experimental": {"Cluster": "my-fav-cluster"}}, - {"unknown-policy": {"unknown-field": "unknown-value"}}, - {"grpclb": {"childPolicy": [{"pickfirst": {}}]}} - ], - "childPolicyConfigTargetFieldName": "service_name" - }`), - wantCfg: &lbConfig{ - lookupService: resolver.Target{Scheme: "passthrough", Endpoint: "target"}, - lookupServiceTimeout: 10 * time.Second, // This is the default value. - maxAge: 5 * time.Minute, // This is max maxAge. - staleAge: time.Duration(0), // StaleAge is ignore because it was higher than maxAge. - cacheSizeBytes: 1000, - rpStrategy: rlspb.RouteLookupConfig_ASYNC_LOOKUP_DEFAULT_TARGET_ON_MISS, - cpName: "grpclb", - cpTargetField: "service_name", - cpConfig: map[string]json.RawMessage{"childPolicy": json.RawMessage(`[{"pickfirst": {}}]`)}, - }, - }, - { - desc: "without transformations", - input: []byte(`{ - "routeLookupConfig": { - "grpcKeybuilders": [{ - "names": [{"service": "service", "method": "method"}], - "headers": [{"key": "k1", "names": ["v1"]}] - }], - "lookupService": "passthrough:///target", - "lookupServiceTimeout" : "100s", - "maxAge": "60s", - "staleAge" : "50s", - "cacheSizeBytes": 1000, - "request_processing_strategy": "ASYNC_LOOKUP_DEFAULT_TARGET_ON_MISS", - "defaultTarget": "passthrough:///default" - }, - "childPolicy": [{"grpclb": {"childPolicy": [{"pickfirst": {}}]}}], - "childPolicyConfigTargetFieldName": "service_name" - }`), - wantCfg: &lbConfig{ - lookupService: resolver.Target{Scheme: "passthrough", Endpoint: "target"}, - lookupServiceTimeout: 100 * time.Second, - maxAge: 60 * time.Second, - staleAge: 50 * time.Second, - cacheSizeBytes: 1000, - rpStrategy: rlspb.RouteLookupConfig_ASYNC_LOOKUP_DEFAULT_TARGET_ON_MISS, - cpName: "grpclb", - cpTargetField: "service_name", - cpConfig: map[string]json.RawMessage{"childPolicy": json.RawMessage(`[{"pickfirst": {}}]`)}, - }, - }, - } - - builder := &rlsBB{} - for _, test := range tests { - t.Run(test.desc, func(t *testing.T) { - lbCfg, err := builder.ParseConfig(test.input) - if err != nil || !cmp.Equal(lbCfg, test.wantCfg) { - t.Errorf("ParseConfig(%s) = {%+v, %v}, want {%+v, nil}", string(test.input), lbCfg, err, test.wantCfg) - } - }) - } -} - -func TestParseConfigErrors(t *testing.T) { - tests := []struct { - desc string - input []byte - wantErr string - }{ - { - desc: "bad json", - input: []byte(`bad bad json`), - wantErr: "rls: json unmarshal failed for service config", - }, - { - desc: "bad grpcKeyBuilder", - input: []byte(`{ - "routeLookupConfig": { - "grpcKeybuilders": [{ - "names": [{"service": "service", "method": "method"}], - "headers": [{"key": "k1", "requiredMatch": true, "names": ["v1"]}] - }] - } - }`), - wantErr: "rls: GrpcKeyBuilder in RouteLookupConfig has required_match field set", - }, - { - desc: "empty lookup service", - input: []byte(`{ - "routeLookupConfig": { - "grpcKeybuilders": [{ - "names": [{"service": "service", "method": "method"}], - "headers": [{"key": "k1", "names": ["v1"]}] - }] - } - }`), - wantErr: "rls: empty lookup_service in service config", - }, - { - desc: "invalid lookup service URI", - input: []byte(`{ - "routeLookupConfig": { - "grpcKeybuilders": [{ - "names": [{"service": "service", "method": "method"}], - "headers": [{"key": "k1", "names": ["v1"]}] - }], - "lookupService": "badScheme:///target" - } - }`), - wantErr: "rls: invalid target URI in lookup_service", - }, - { - desc: "invalid lookup service timeout", - input: []byte(`{ - "routeLookupConfig": { - "grpcKeybuilders": [{ - "names": [{"service": "service", "method": "method"}], - "headers": [{"key": "k1", "names": ["v1"]}] - }], - "lookupService": "passthrough:///target", - "lookupServiceTimeout" : "315576000001s" - } - }`), - wantErr: "bad Duration: time: invalid duration", - }, - { - desc: "invalid max age", - input: []byte(`{ - "routeLookupConfig": { - "grpcKeybuilders": [{ - "names": [{"service": "service", "method": "method"}], - "headers": [{"key": "k1", "names": ["v1"]}] - }], - "lookupService": "passthrough:///target", - "lookupServiceTimeout" : "10s", - "maxAge" : "315576000001s" - } - }`), - wantErr: "bad Duration: time: invalid duration", - }, - { - desc: "invalid stale age", - input: []byte(`{ - "routeLookupConfig": { - "grpcKeybuilders": [{ - "names": [{"service": "service", "method": "method"}], - "headers": [{"key": "k1", "names": ["v1"]}] - }], - "lookupService": "passthrough:///target", - "lookupServiceTimeout" : "10s", - "maxAge" : "10s", - "staleAge" : "315576000001s" - } - }`), - wantErr: "bad Duration: time: invalid duration", - }, - { - desc: "invalid max age stale age combo", - input: []byte(`{ - "routeLookupConfig": { - "grpcKeybuilders": [{ - "names": [{"service": "service", "method": "method"}], - "headers": [{"key": "k1", "names": ["v1"]}] - }], - "lookupService": "passthrough:///target", - "lookupServiceTimeout" : "10s", - "staleAge" : "10s" - } - }`), - wantErr: "rls: stale_age is set, but max_age is not in service config", - }, - { - desc: "invalid cache size", - input: []byte(`{ - "routeLookupConfig": { - "grpcKeybuilders": [{ - "names": [{"service": "service", "method": "method"}], - "headers": [{"key": "k1", "names": ["v1"]}] - }], - "lookupService": "passthrough:///target", - "lookupServiceTimeout" : "10s", - "maxAge": "30s", - "staleAge" : "25s" - } - }`), - wantErr: "rls: cache_size_bytes must be greater than 0 in service config", - }, - { - desc: "invalid request processing strategy", - input: []byte(`{ - "routeLookupConfig": { - "grpcKeybuilders": [{ - "names": [{"service": "service", "method": "method"}], - "headers": [{"key": "k1", "names": ["v1"]}] - }], - "lookupService": "passthrough:///target", - "lookupServiceTimeout" : "10s", - "maxAge": "30s", - "staleAge" : "25s", - "cacheSizeBytes": 1000 - } - }`), - wantErr: "rls: request_processing_strategy cannot be left unspecified in service config", - }, - { - desc: "request processing strategy without default target", - input: []byte(`{ - "routeLookupConfig": { - "grpcKeybuilders": [{ - "names": [{"service": "service", "method": "method"}], - "headers": [{"key": "k1", "names": ["v1"]}] - }], - "lookupService": "passthrough:///target", - "lookupServiceTimeout" : "10s", - "maxAge": "30s", - "staleAge" : "25s", - "cacheSizeBytes": 1000, - "request_processing_strategy": "ASYNC_LOOKUP_DEFAULT_TARGET_ON_MISS" - } - }`), - wantErr: "default_target is not set", - }, - { - desc: "no child policy", - input: []byte(`{ - "routeLookupConfig": { - "grpcKeybuilders": [{ - "names": [{"service": "service", "method": "method"}], - "headers": [{"key": "k1", "names": ["v1"]}] - }], - "lookupService": "passthrough:///target", - "lookupServiceTimeout" : "10s", - "maxAge": "30s", - "staleAge" : "25s", - "cacheSizeBytes": 1000, - "request_processing_strategy": "ASYNC_LOOKUP_DEFAULT_TARGET_ON_MISS", - "defaultTarget": "passthrough:///default" - } - }`), - wantErr: "rls: childPolicy is invalid in service config", - }, - { - desc: "no known child policy", - input: []byte(`{ - "routeLookupConfig": { - "grpcKeybuilders": [{ - "names": [{"service": "service", "method": "method"}], - "headers": [{"key": "k1", "names": ["v1"]}] - }], - "lookupService": "passthrough:///target", - "lookupServiceTimeout" : "10s", - "maxAge": "30s", - "staleAge" : "25s", - "cacheSizeBytes": 1000, - "request_processing_strategy": "ASYNC_LOOKUP_DEFAULT_TARGET_ON_MISS", - "defaultTarget": "passthrough:///default" - }, - "childPolicy": [ - {"cds_experimental": {"Cluster": "my-fav-cluster"}}, - {"unknown-policy": {"unknown-field": "unknown-value"}} - ] - }`), - wantErr: "rls: childPolicy is invalid in service config", - }, - { - desc: "no childPolicyConfigTargetFieldName", - input: []byte(`{ - "routeLookupConfig": { - "grpcKeybuilders": [{ - "names": [{"service": "service", "method": "method"}], - "headers": [{"key": "k1", "names": ["v1"]}] - }], - "lookupService": "passthrough:///target", - "lookupServiceTimeout" : "10s", - "maxAge": "30s", - "staleAge" : "25s", - "cacheSizeBytes": 1000, - "request_processing_strategy": "ASYNC_LOOKUP_DEFAULT_TARGET_ON_MISS", - "defaultTarget": "passthrough:///default" - }, - "childPolicy": [ - {"cds_experimental": {"Cluster": "my-fav-cluster"}}, - {"unknown-policy": {"unknown-field": "unknown-value"}}, - {"grpclb": {}} - ] - }`), - wantErr: "rls: childPolicyConfigTargetFieldName field is not set in service config", - }, - { - desc: "child policy config validation failure", - input: []byte(`{ - "routeLookupConfig": { - "grpcKeybuilders": [{ - "names": [{"service": "service", "method": "method"}], - "headers": [{"key": "k1", "names": ["v1"]}] - }], - "lookupService": "passthrough:///target", - "lookupServiceTimeout" : "10s", - "maxAge": "30s", - "staleAge" : "25s", - "cacheSizeBytes": 1000, - "request_processing_strategy": "ASYNC_LOOKUP_DEFAULT_TARGET_ON_MISS", - "defaultTarget": "passthrough:///default" - }, - "childPolicy": [ - {"cds_experimental": {"Cluster": "my-fav-cluster"}}, - {"unknown-policy": {"unknown-field": "unknown-value"}}, - {"grpclb": {"childPolicy": "not-an-array"}} - ], - "childPolicyConfigTargetFieldName": "service_name" - }`), - wantErr: "rls: childPolicy config validation failed", - }, - } - - builder := &rlsBB{} - for _, test := range tests { - t.Run(test.desc, func(t *testing.T) { - lbCfg, err := builder.ParseConfig(test.input) - if lbCfg != nil || !strings.Contains(fmt.Sprint(err), test.wantErr) { - t.Errorf("ParseConfig(%s) = {%+v, %v}, want {nil, %s}", string(test.input), lbCfg, err, test.wantErr) - } - }) - } -} - -func TestValidateChildPolicyConfig(t *testing.T) { - jsonCfg := json.RawMessage(`[{"round_robin" : {}}, {"pick_first" : {}}]`) - wantChildConfig := map[string]json.RawMessage{"childPolicy": jsonCfg} - input := &lbParsedConfig{ - childPolicy: &loadBalancingConfig{ - Name: "grpclb", - Config: []byte(`{"childPolicy": [{"round_robin" : {}}, {"pick_first" : {}}]}`), - }, - targetField: "serviceName", - } - gotChildConfig, err := validateChildPolicyConfig(input) - if err != nil || !cmp.Equal(gotChildConfig, wantChildConfig) { - t.Errorf("validateChildPolicyConfig(%v) = {%v, %v}, want {%v, nil}", input, gotChildConfig, err, wantChildConfig) - } -} - -func TestValidateChildPolicyConfigErrors(t *testing.T) { - tests := []struct { - desc string - input *lbParsedConfig - wantErrPrefix string - }{ - { - desc: "bad json", - input: &lbParsedConfig{ - childPolicy: &loadBalancingConfig{ - Config: []byte(`bad bad json`), - }, - }, - wantErrPrefix: "rls: json unmarshal failed for child policy config", - }, - { - desc: "unknown child policy", - input: &lbParsedConfig{ - childPolicy: &loadBalancingConfig{ - Name: "unknown", - Config: []byte(`{}`), - }, - }, - wantErrPrefix: "rls: balancer builder not found for child_policy", - }, - { - desc: "balancer builder does not implement ConfigParser", - input: &lbParsedConfig{ - childPolicy: &loadBalancingConfig{ - Name: balancerWithoutConfigParserName, - Config: []byte(`{}`), - }, - }, - wantErrPrefix: "rls: balancer builder for child_policy does not implement balancer.ConfigParser", - }, - { - desc: "child policy config parsing failure", - input: &lbParsedConfig{ - childPolicy: &loadBalancingConfig{ - Name: "grpclb", - Config: []byte(`{"childPolicy": "not-an-array"}`), - }, - }, - wantErrPrefix: "rls: childPolicy config validation failed", - }, - } - - for _, test := range tests { - t.Run(test.desc, func(t *testing.T) { - gotChildConfig, gotErr := validateChildPolicyConfig(test.input) - if gotChildConfig != nil || !strings.HasPrefix(fmt.Sprint(gotErr), test.wantErrPrefix) { - t.Errorf("validateChildPolicyConfig(%v) = {%v, %v}, want {nil, %v}", test.input, gotChildConfig, gotErr, test.wantErrPrefix) - } - }) - } -} diff --git a/balancer/rls/internal/config.go b/balancer/rls/internal/config.go index 66abca03edd8..04bd2f1d4409 100644 --- a/balancer/rls/internal/config.go +++ b/balancer/rls/internal/config.go @@ -27,13 +27,15 @@ import ( "time" "github.com/golang/protobuf/jsonpb" + "github.com/golang/protobuf/ptypes" + durationpb "github.com/golang/protobuf/ptypes/duration" "google.golang.org/grpc/balancer" + "google.golang.org/grpc/balancer/rls/internal/keys" + rlspb "google.golang.org/grpc/balancer/rls/internal/proto/grpc_lookup_v1" "google.golang.org/grpc/grpclog" + "google.golang.org/grpc/internal/grpcutil" "google.golang.org/grpc/resolver" "google.golang.org/grpc/serviceconfig" - - "google.golang.org/grpc/balancer/rls/internal/keys" - rlspb "google.golang.org/grpc/balancer/rls/internal/proto/grpc_lookup_v1" ) // lbConfig contains the parsed and validated contents of the @@ -55,76 +57,19 @@ type lbConfig struct { cpConfig map[string]json.RawMessage } -// lbParsedConfig is an parsed internal representation of the JSON -// loadBalancing config for the RLS LB policy. This config is put through -// further validation checks and transformed into another representation before -// handing it off to the RLS LB policy. -type lbParsedConfig struct { - rlsProto *rlspb.RouteLookupConfig - childPolicy *loadBalancingConfig - targetField string -} - -// UnmarshalJSON parses the JSON-encoded byte slice in data and stores it in l. -// When unmarshalling, we iterate through the childPolicy list and select the -// first LB policy which has been registered. -// Returns error only when JSON unmarshaling fails. Further validation checks -// are required to verify the sanity of the parsed config. -func (l *lbParsedConfig) UnmarshalJSON(data []byte) error { - var jsonData map[string]json.RawMessage - if err := json.Unmarshal(data, &jsonData); err != nil { - return fmt.Errorf("bad input json data: %v", err) - } - - // We declare local variables to store the result of JSON unmarshaling, and - // set them to appropriate fields in the lbParsedConfig only if everything goes - // smoothly. - var ( - rlsProto *rlspb.RouteLookupConfig - childPolicy *loadBalancingConfig - targetField string - ) - for k, v := range jsonData { - switch k { - case "routeLookupConfig": - m := jsonpb.Unmarshaler{AllowUnknownFields: true} - rlsProto = &rlspb.RouteLookupConfig{} - if err := m.Unmarshal(bytes.NewReader(v), rlsProto); err != nil { - return fmt.Errorf("bad routeLookupConfig proto: %v", err) - } - case "childPolicy": - var lbCfgs []*loadBalancingConfig - if err := json.Unmarshal(v, &lbCfgs); err != nil { - return fmt.Errorf("bad childPolicy config: %v", err) - } - for _, lbcfg := range lbCfgs { - if balancer.Get(lbcfg.Name) != nil { - childPolicy = lbcfg - break - } - } - case "childPolicyConfigTargetFieldName": - if err := json.Unmarshal(v, &targetField); err != nil { - return fmt.Errorf("bad childPolicyConfigTargetFieldName: %v", err) - } - default: - grpclog.Warningf("rls: unknown field %q in ServiceConfig", k) - } - } - - l.rlsProto = rlsProto - l.childPolicy = childPolicy - l.targetField = targetField - return nil -} - -// MarshalJSON returns a JSON encoding of l. -func (l *lbParsedConfig) MarshalJSON() ([]byte, error) { - return nil, fmt.Errorf("lbParsedConfig.MarshalJSON() is unimplemented") +// This struct resembles the JSON respresentation of the loadBalancing config +// and makes it easier to unmarshal. +type lbConfigJSON struct { + RouteLookupConfig json.RawMessage + ChildPolicy []*loadBalancingConfig + ChildPolicyConfigTargetFieldName string } // loadBalancingConfig represents a single load balancing config, // stored in JSON format. +// +// TODO(easwars): This code seems to be repeated in a few places +// (service_config.go and in the xds code as well). Refactor and re-use. type loadBalancingConfig struct { Name string Config json.RawMessage @@ -147,3 +92,196 @@ func (l *loadBalancingConfig) UnmarshalJSON(data []byte) error { } return nil } + +// ParseConfig parses and validates the JSON representation of the service +// config and returns the loadBalancingConfig to be used by the RLS LB policy. +// +// Helps implement the balancer.ConfigParser interface. +// +// The following validation checks are performed: +// * routeLookupConfig: +// ** grpc_keybuilders field: +// - must have at least one entry +// - must not have two entries with the same Name +// - must not have any entry with a Name with the service field unset or +// empty +// - must not have any entries without a Name +// - must not have a headers entry that has required_match set +// - must not have two headers entries with the same key within one entry +// ** lookup_service field: +// - must be set and non-empty and must parse as a target URI +// ** max_age field: +// - if not specified or is greater than maxMaxAge, it will be reset to +// maxMaxAge +// ** stale_age field: +// - if the value is greater than or equal to max_age, it is ignored +// - if set, then max_age must also be set +// ** valid_targets field: +// - will be ignored +// ** cache_size_bytes field: +// - must be greater than zero +// - TODO(easwars): Define a minimum value for this field, to be used when +// left unspecified +// ** request_processing_strategy field: +// - must have a value other than STRATEGY_UNSPECIFIED +// - if set to SYNC_LOOKUP_DEFAULT_TARGET_ON_ERROR or +// ASYNC_LOOKUP_DEFAULT_TARGET_ON_MISS, the default_target field must be +// set to a non-empty value +// * childPolicy field: +// - must find a valid child policy with a valid config (the child policy must +// be able to parse the provided config successfully when we pass it a dummy +// target name in the target_field provided by the +// childPolicyConfigTargetFieldName field) +// * childPolicyConfigTargetFieldName field: +// - must be set and non-empty +func (*rlsBB) ParseConfig(c json.RawMessage) (serviceconfig.LoadBalancingConfig, error) { + cfgJSON := &lbConfigJSON{} + if err := json.Unmarshal(c, cfgJSON); err != nil { + return nil, fmt.Errorf("rls: json unmarshal failed for service config {%+v}: %v", string(c), err) + } + + m := jsonpb.Unmarshaler{AllowUnknownFields: true} + rlsProto := &rlspb.RouteLookupConfig{} + if err := m.Unmarshal(bytes.NewReader(cfgJSON.RouteLookupConfig), rlsProto); err != nil { + return nil, fmt.Errorf("rls: bad RouteLookupConfig proto {%+v}: %v", string(cfgJSON.RouteLookupConfig), err) + } + + var childPolicy *loadBalancingConfig + for _, lbcfg := range cfgJSON.ChildPolicy { + if balancer.Get(lbcfg.Name) != nil { + childPolicy = lbcfg + break + } + } + + kbMap, err := keys.MakeBuilderMap(rlsProto) + if err != nil { + return nil, err + } + + lookupService := rlsProto.GetLookupService() + if lookupService == "" { + return nil, fmt.Errorf("rls: empty lookup_service in service config {%+v}", string(c)) + } + parsedTarget := grpcutil.ParseTarget(lookupService) + if parsedTarget.Scheme == "" { + parsedTarget.Scheme = resolver.GetDefaultScheme() + } + if resolver.Get(parsedTarget.Scheme) == nil { + return nil, fmt.Errorf("rls: invalid target URI in lookup_service {%s}", lookupService) + } + + lookupServiceTimeout, err := convertDuration(rlsProto.GetLookupServiceTimeout()) + if err != nil { + return nil, fmt.Errorf("rls: failed to parse lookup_service_timeout in service config {%+v}: %v", string(c), err) + } + if lookupServiceTimeout == 0 { + lookupServiceTimeout = defaultLookupServiceTimeout + } + maxAge, err := convertDuration(rlsProto.GetMaxAge()) + if err != nil { + return nil, fmt.Errorf("rls: failed to parse max_age in service config {%+v}: %v", string(c), err) + } + staleAge, err := convertDuration(rlsProto.GetStaleAge()) + if err != nil { + return nil, fmt.Errorf("rls: failed to parse staleAge in service config {%+v}: %v", string(c), err) + } + if staleAge != 0 && maxAge == 0 { + return nil, fmt.Errorf("rls: stale_age is set, but max_age is not in service config {%+v}", string(c)) + } + if staleAge >= maxAge { + grpclog.Info("rls: stale_age {%v} is greater than max_age {%v}, ignoring it", staleAge, maxAge) + staleAge = 0 + } + if maxAge == 0 || maxAge > maxMaxAge { + grpclog.Infof("rls: max_age in service config is %v, using %v", maxAge, maxMaxAge) + maxAge = maxMaxAge + } + + cacheSizeBytes := rlsProto.GetCacheSizeBytes() + if cacheSizeBytes <= 0 { + return nil, fmt.Errorf("rls: cache_size_bytes must be greater than 0 in service config {%+v}", string(c)) + } + + rpStrategy := rlsProto.GetRequestProcessingStrategy() + if rpStrategy == rlspb.RouteLookupConfig_STRATEGY_UNSPECIFIED { + return nil, fmt.Errorf("rls: request_processing_strategy cannot be left unspecified in service config {%+v}", string(c)) + } + defaultTarget := rlsProto.GetDefaultTarget() + if (rpStrategy == rlspb.RouteLookupConfig_SYNC_LOOKUP_DEFAULT_TARGET_ON_ERROR || + rpStrategy == rlspb.RouteLookupConfig_ASYNC_LOOKUP_DEFAULT_TARGET_ON_MISS) && defaultTarget == "" { + return nil, fmt.Errorf("rls: request_processing_strategy is %s, but default_target is not set", rpStrategy.String()) + } + + if childPolicy == nil { + return nil, fmt.Errorf("rls: childPolicy is invalid in service config {%+v}", string(c)) + } + if cfgJSON.ChildPolicyConfigTargetFieldName == "" { + return nil, fmt.Errorf("rls: childPolicyConfigTargetFieldName field is not set in service config {%+v}", string(c)) + } + cpCfg, err := validateChildPolicyConfig(childPolicy, cfgJSON.ChildPolicyConfigTargetFieldName) + if err != nil { + return nil, err + } + + return &lbConfig{ + kbMap: kbMap, + lookupService: parsedTarget, + lookupServiceTimeout: lookupServiceTimeout, + maxAge: maxAge, + staleAge: staleAge, + cacheSizeBytes: cacheSizeBytes, + rpStrategy: rpStrategy, + cpName: childPolicy.Name, + cpTargetField: cfgJSON.ChildPolicyConfigTargetFieldName, + cpConfig: cpCfg, + }, nil +} + +// validateChildPolicyConfig validates the child policy config received in the +// service config. This makes it possible for us to reject service configs +// which contain invalid child policy configs which we know will fail for sure. +// +// It does the following: +// * Unmarshals the provided child policy config into a map of string to +// json.RawMessage. This allows us to add an entry to the map corresponding +// to the targetFieldName that we received in the service config. +// * Marshals the map back into JSON, finds the config parser associated with +// the child policy and asks it to validate the config. +// * If the validation succeeded, removes the dummy entry from the map and +// returns it. If any of the above steps failed, it returns an error. +func validateChildPolicyConfig(cp *loadBalancingConfig, cpTargetField string) (map[string]json.RawMessage, error) { + var childConfig map[string]json.RawMessage + if err := json.Unmarshal(cp.Config, &childConfig); err != nil { + return nil, fmt.Errorf("rls: json unmarshal failed for child policy config {%+v}: %v", cp.Config, err) + } + childConfig[cpTargetField], _ = json.Marshal(dummyChildPolicyTarget) + + jsonCfg, err := json.Marshal(childConfig) + if err != nil { + return nil, fmt.Errorf("rls: json marshal failed for child policy config {%+v}: %v", childConfig, err) + } + builder := balancer.Get(cp.Name) + if builder == nil { + // This should never happen since we already made sure that the child + // policy name mentioned in the service config is a valid one. + return nil, fmt.Errorf("rls: balancer builder not found for child_policy %v", cp.Name) + } + parser, ok := builder.(balancer.ConfigParser) + if !ok { + return nil, fmt.Errorf("rls: balancer builder for child_policy does not implement balancer.ConfigParser: %v", cp.Name) + } + _, err = parser.ParseConfig(jsonCfg) + if err != nil { + return nil, fmt.Errorf("rls: childPolicy config validation failed: %v", err) + } + delete(childConfig, cpTargetField) + return childConfig, nil +} + +func convertDuration(d *durationpb.Duration) (time.Duration, error) { + if d == nil { + return 0, nil + } + return ptypes.Duration(d) +} diff --git a/balancer/rls/internal/config_test.go b/balancer/rls/internal/config_test.go index 6476b4c6135e..51abbdeed011 100644 --- a/balancer/rls/internal/config_test.go +++ b/balancer/rls/internal/config_test.go @@ -20,133 +20,462 @@ package rls import ( "encoding/json" + "fmt" "strings" "testing" + "time" - "github.com/golang/protobuf/proto" "github.com/google/go-cmp/cmp" + "google.golang.org/grpc/balancer" rlspb "google.golang.org/grpc/balancer/rls/internal/proto/grpc_lookup_v1" + "google.golang.org/grpc/resolver" + + _ "google.golang.org/grpc/balancer/grpclb" // grpclb for config parsing. + _ "google.golang.org/grpc/internal/resolver/passthrough" // passthrough resolver. ) -// Equal is used by cmp.Equal to check equality. -func (lb *lbParsedConfig) Equal(other *lbParsedConfig) bool { - switch { - case lb == nil && other != nil: - return false - case lb != nil && other == nil: - return false - case lb == nil && other == nil: - return true - } - return proto.Equal(lb.rlsProto, other.rlsProto) && - cmp.Equal(lb.childPolicy, other.childPolicy) && - lb.targetField == other.targetField +const balancerWithoutConfigParserName = "dummy_balancer" + +type dummyBB struct { + balancer.Builder +} + +func (*dummyBB) Name() string { + return balancerWithoutConfigParserName +} + +func init() { + balancer.Register(&dummyBB{}) +} + +func (lbCfg *lbConfig) Equal(other *lbConfig) bool { + // This only ignores the keyBuilderMap field because its internals are not + // exported, and hence not possible to specify in the want section of the + // test. + return lbCfg.lookupService == other.lookupService && + lbCfg.lookupServiceTimeout == other.lookupServiceTimeout && + lbCfg.maxAge == other.maxAge && + lbCfg.staleAge == other.staleAge && + lbCfg.cacheSizeBytes == other.cacheSizeBytes && + lbCfg.rpStrategy == other.rpStrategy && + lbCfg.cpName == other.cpName && + lbCfg.cpTargetField == other.cpTargetField && + cmp.Equal(lbCfg.cpConfig, other.cpConfig) } -func TestLBConfigUnmarshalJSON(t *testing.T) { +func TestParseConfig(t *testing.T) { tests := []struct { desc string input []byte - wantCfg *lbParsedConfig + wantCfg *lbConfig }{ + // This input validates a few cases: + // - A top-level unknown field should not fail. + // - An unknown field in routeLookupConfig proto should not fail. + // - lookupServiceTimeout is set to its default value, since it is not specified in the input. + // - maxAge is set to maxMaxAge since the value is too large in the input. + // - staleAge is ignore because it is higher than maxAge in the input. { - desc: "all-good-lbCfg", - // This input validates a few cases: - // - A top-level unknown field should not fail. - // - An unknown field in routeLookupConfig proto should not fail. - // - When the child_policy list contains known and unknown child - // policies, we should end up picking the first one that we know. + desc: "with transformations", input: []byte(`{ "top-level-unknown-field": "unknown-value", "routeLookupConfig": { "unknown-field": "unknown-value", + "grpcKeybuilders": [{ + "names": [{"service": "service", "method": "method"}], + "headers": [{"key": "k1", "names": ["v1"]}] + }], + "lookupService": "passthrough:///target", + "maxAge" : "500s", + "staleAge": "600s", "cacheSizeBytes": 1000, - "defaultTarget": "foobar" + "request_processing_strategy": "ASYNC_LOOKUP_DEFAULT_TARGET_ON_MISS", + "defaultTarget": "passthrough:///default" }, "childPolicy": [ {"cds_experimental": {"Cluster": "my-fav-cluster"}}, - {"grpclb": {}}, - {"unknown-policy": {"unknown-field": "unknown-value"}} + {"unknown-policy": {"unknown-field": "unknown-value"}}, + {"grpclb": {"childPolicy": [{"pickfirst": {}}]}} ], - "childPolicyConfigTargetFieldName": "serviceName" + "childPolicyConfigTargetFieldName": "service_name" }`), - wantCfg: &lbParsedConfig{ - rlsProto: &rlspb.RouteLookupConfig{ - CacheSizeBytes: 1000, - DefaultTarget: "foobar", - }, - childPolicy: &loadBalancingConfig{ - Name: "grpclb", - Config: json.RawMessage(`{}`), + wantCfg: &lbConfig{ + lookupService: resolver.Target{Scheme: "passthrough", Endpoint: "target"}, + lookupServiceTimeout: 10 * time.Second, // This is the default value. + maxAge: 5 * time.Minute, // This is max maxAge. + staleAge: time.Duration(0), // StaleAge is ignore because it was higher than maxAge. + cacheSizeBytes: 1000, + rpStrategy: rlspb.RouteLookupConfig_ASYNC_LOOKUP_DEFAULT_TARGET_ON_MISS, + cpName: "grpclb", + cpTargetField: "service_name", + cpConfig: map[string]json.RawMessage{"childPolicy": json.RawMessage(`[{"pickfirst": {}}]`)}, + }, + }, + { + desc: "without transformations", + input: []byte(`{ + "routeLookupConfig": { + "grpcKeybuilders": [{ + "names": [{"service": "service", "method": "method"}], + "headers": [{"key": "k1", "names": ["v1"]}] + }], + "lookupService": "passthrough:///target", + "lookupServiceTimeout" : "100s", + "maxAge": "60s", + "staleAge" : "50s", + "cacheSizeBytes": 1000, + "request_processing_strategy": "ASYNC_LOOKUP_DEFAULT_TARGET_ON_MISS", + "defaultTarget": "passthrough:///default" }, - targetField: "serviceName", + "childPolicy": [{"grpclb": {"childPolicy": [{"pickfirst": {}}]}}], + "childPolicyConfigTargetFieldName": "service_name" + }`), + wantCfg: &lbConfig{ + lookupService: resolver.Target{Scheme: "passthrough", Endpoint: "target"}, + lookupServiceTimeout: 100 * time.Second, + maxAge: 60 * time.Second, + staleAge: 50 * time.Second, + cacheSizeBytes: 1000, + rpStrategy: rlspb.RouteLookupConfig_ASYNC_LOOKUP_DEFAULT_TARGET_ON_MISS, + cpName: "grpclb", + cpTargetField: "service_name", + cpConfig: map[string]json.RawMessage{"childPolicy": json.RawMessage(`[{"pickfirst": {}}]`)}, }, }, } + builder := &rlsBB{} for _, test := range tests { t.Run(test.desc, func(t *testing.T) { - lbCfg := &lbParsedConfig{} - err := lbCfg.UnmarshalJSON(test.input) + lbCfg, err := builder.ParseConfig(test.input) if err != nil || !cmp.Equal(lbCfg, test.wantCfg) { - t.Errorf("lbCfg.UnmarshalJSON(%s) = {%+v, %v} want {%+v, %v}", string(test.input), lbCfg, err, test.wantCfg, nil) + t.Errorf("ParseConfig(%s) = {%+v, %v}, want {%+v, nil}", string(test.input), lbCfg, err, test.wantCfg) } }) } } -func TestLBConfigUnmarshalJSONErrors(t *testing.T) { +func TestParseConfigErrors(t *testing.T) { tests := []struct { - desc string - input []byte - wantErrPrefix string + desc string + input []byte + wantErr string }{ { - desc: "empty input", - input: nil, - wantErrPrefix: "bad input json data", + desc: "empty input", + input: nil, + wantErr: "rls: json unmarshal failed for service config", + }, + { + desc: "bad json", + input: []byte(`bad bad json`), + wantErr: "rls: json unmarshal failed for service config", + }, + { + desc: "bad grpcKeyBuilder", + input: []byte(`{ + "routeLookupConfig": { + "grpcKeybuilders": [{ + "names": [{"service": "service", "method": "method"}], + "headers": [{"key": "k1", "requiredMatch": true, "names": ["v1"]}] + }] + } + }`), + wantErr: "rls: GrpcKeyBuilder in RouteLookupConfig has required_match field set", + }, + { + desc: "empty lookup service", + input: []byte(`{ + "routeLookupConfig": { + "grpcKeybuilders": [{ + "names": [{"service": "service", "method": "method"}], + "headers": [{"key": "k1", "names": ["v1"]}] + }] + } + }`), + wantErr: "rls: empty lookup_service in service config", + }, + { + desc: "invalid lookup service URI", + input: []byte(`{ + "routeLookupConfig": { + "grpcKeybuilders": [{ + "names": [{"service": "service", "method": "method"}], + "headers": [{"key": "k1", "names": ["v1"]}] + }], + "lookupService": "badScheme:///target" + } + }`), + wantErr: "rls: invalid target URI in lookup_service", }, { - desc: "bad JSON", - input: []byte(`bad bad json`), - wantErrPrefix: "bad input json data", + desc: "invalid lookup service timeout", + input: []byte(`{ + "routeLookupConfig": { + "grpcKeybuilders": [{ + "names": [{"service": "service", "method": "method"}], + "headers": [{"key": "k1", "names": ["v1"]}] + }], + "lookupService": "passthrough:///target", + "lookupServiceTimeout" : "315576000001s" + } + }`), + wantErr: "bad Duration: time: invalid duration", }, { - desc: "bad RLS proto", + desc: "invalid max age", input: []byte(`{ - "routeLookupConfig": "not-a-proto" + "routeLookupConfig": { + "grpcKeybuilders": [{ + "names": [{"service": "service", "method": "method"}], + "headers": [{"key": "k1", "names": ["v1"]}] + }], + "lookupService": "passthrough:///target", + "lookupServiceTimeout" : "10s", + "maxAge" : "315576000001s" + } }`), - wantErrPrefix: "bad routeLookupConfig proto", + wantErr: "bad Duration: time: invalid duration", }, { - desc: "bad child policy config", + desc: "invalid stale age", input: []byte(`{ - "childPolicy": "not-a-valid-loadbalancing-config" + "routeLookupConfig": { + "grpcKeybuilders": [{ + "names": [{"service": "service", "method": "method"}], + "headers": [{"key": "k1", "names": ["v1"]}] + }], + "lookupService": "passthrough:///target", + "lookupServiceTimeout" : "10s", + "maxAge" : "10s", + "staleAge" : "315576000001s" + } }`), - wantErrPrefix: "bad childPolicy config", + wantErr: "bad Duration: time: invalid duration", }, { - desc: "child policy config not an array", + desc: "invalid max age stale age combo", input: []byte(`{ - "childPolicy": { - "cds_experimental":{ - "Cluster": "my-fav-cluster" - } + "routeLookupConfig": { + "grpcKeybuilders": [{ + "names": [{"service": "service", "method": "method"}], + "headers": [{"key": "k1", "names": ["v1"]}] + }], + "lookupService": "passthrough:///target", + "lookupServiceTimeout" : "10s", + "staleAge" : "10s" + } + }`), + wantErr: "rls: stale_age is set, but max_age is not in service config", + }, + { + desc: "invalid cache size", + input: []byte(`{ + "routeLookupConfig": { + "grpcKeybuilders": [{ + "names": [{"service": "service", "method": "method"}], + "headers": [{"key": "k1", "names": ["v1"]}] + }], + "lookupService": "passthrough:///target", + "lookupServiceTimeout" : "10s", + "maxAge": "30s", + "staleAge" : "25s" + } + }`), + wantErr: "rls: cache_size_bytes must be greater than 0 in service config", + }, + { + desc: "invalid request processing strategy", + input: []byte(`{ + "routeLookupConfig": { + "grpcKeybuilders": [{ + "names": [{"service": "service", "method": "method"}], + "headers": [{"key": "k1", "names": ["v1"]}] + }], + "lookupService": "passthrough:///target", + "lookupServiceTimeout" : "10s", + "maxAge": "30s", + "staleAge" : "25s", + "cacheSizeBytes": 1000 } }`), - wantErrPrefix: "bad childPolicy config", + wantErr: "rls: request_processing_strategy cannot be left unspecified in service config", + }, + { + desc: "request processing strategy without default target", + input: []byte(`{ + "routeLookupConfig": { + "grpcKeybuilders": [{ + "names": [{"service": "service", "method": "method"}], + "headers": [{"key": "k1", "names": ["v1"]}] + }], + "lookupService": "passthrough:///target", + "lookupServiceTimeout" : "10s", + "maxAge": "30s", + "staleAge" : "25s", + "cacheSizeBytes": 1000, + "request_processing_strategy": "ASYNC_LOOKUP_DEFAULT_TARGET_ON_MISS" + } + }`), + wantErr: "default_target is not set", + }, + { + desc: "no child policy", + input: []byte(`{ + "routeLookupConfig": { + "grpcKeybuilders": [{ + "names": [{"service": "service", "method": "method"}], + "headers": [{"key": "k1", "names": ["v1"]}] + }], + "lookupService": "passthrough:///target", + "lookupServiceTimeout" : "10s", + "maxAge": "30s", + "staleAge" : "25s", + "cacheSizeBytes": 1000, + "request_processing_strategy": "ASYNC_LOOKUP_DEFAULT_TARGET_ON_MISS", + "defaultTarget": "passthrough:///default" + } + }`), + wantErr: "rls: childPolicy is invalid in service config", + }, + { + desc: "no known child policy", + input: []byte(`{ + "routeLookupConfig": { + "grpcKeybuilders": [{ + "names": [{"service": "service", "method": "method"}], + "headers": [{"key": "k1", "names": ["v1"]}] + }], + "lookupService": "passthrough:///target", + "lookupServiceTimeout" : "10s", + "maxAge": "30s", + "staleAge" : "25s", + "cacheSizeBytes": 1000, + "request_processing_strategy": "ASYNC_LOOKUP_DEFAULT_TARGET_ON_MISS", + "defaultTarget": "passthrough:///default" + }, + "childPolicy": [ + {"cds_experimental": {"Cluster": "my-fav-cluster"}}, + {"unknown-policy": {"unknown-field": "unknown-value"}} + ] + }`), + wantErr: "rls: childPolicy is invalid in service config", + }, + { + desc: "no childPolicyConfigTargetFieldName", + input: []byte(`{ + "routeLookupConfig": { + "grpcKeybuilders": [{ + "names": [{"service": "service", "method": "method"}], + "headers": [{"key": "k1", "names": ["v1"]}] + }], + "lookupService": "passthrough:///target", + "lookupServiceTimeout" : "10s", + "maxAge": "30s", + "staleAge" : "25s", + "cacheSizeBytes": 1000, + "request_processing_strategy": "ASYNC_LOOKUP_DEFAULT_TARGET_ON_MISS", + "defaultTarget": "passthrough:///default" + }, + "childPolicy": [ + {"cds_experimental": {"Cluster": "my-fav-cluster"}}, + {"unknown-policy": {"unknown-field": "unknown-value"}}, + {"grpclb": {}} + ] + }`), + wantErr: "rls: childPolicyConfigTargetFieldName field is not set in service config", + }, + { + desc: "child policy config validation failure", + input: []byte(`{ + "routeLookupConfig": { + "grpcKeybuilders": [{ + "names": [{"service": "service", "method": "method"}], + "headers": [{"key": "k1", "names": ["v1"]}] + }], + "lookupService": "passthrough:///target", + "lookupServiceTimeout" : "10s", + "maxAge": "30s", + "staleAge" : "25s", + "cacheSizeBytes": 1000, + "request_processing_strategy": "ASYNC_LOOKUP_DEFAULT_TARGET_ON_MISS", + "defaultTarget": "passthrough:///default" + }, + "childPolicy": [ + {"cds_experimental": {"Cluster": "my-fav-cluster"}}, + {"unknown-policy": {"unknown-field": "unknown-value"}}, + {"grpclb": {"childPolicy": "not-an-array"}} + ], + "childPolicyConfigTargetFieldName": "service_name" + }`), + wantErr: "rls: childPolicy config validation failed", }, } + builder := &rlsBB{} for _, test := range tests { t.Run(test.desc, func(t *testing.T) { - lbCfg := &lbParsedConfig{} - if err := lbCfg.UnmarshalJSON(test.input); err == nil || !strings.HasPrefix(err.Error(), test.wantErrPrefix) { - t.Errorf("lbCfg.UnmarshalJSON(%s) = %v, want %q", string(test.input), err, test.wantErrPrefix) + lbCfg, err := builder.ParseConfig(test.input) + if lbCfg != nil || !strings.Contains(fmt.Sprint(err), test.wantErr) { + t.Errorf("ParseConfig(%s) = {%+v, %v}, want {nil, %s}", string(test.input), lbCfg, err, test.wantErr) } - if !cmp.Equal(lbCfg, &lbParsedConfig{}) { - t.Errorf("lbCfg.UnmarshalJSON(%s) change lbCfg object to: {%+v}, wanted unchanged", string(test.input), lbCfg) + }) + } +} + +func TestValidateChildPolicyConfig(t *testing.T) { + jsonCfg := json.RawMessage(`[{"round_robin" : {}}, {"pick_first" : {}}]`) + wantChildConfig := map[string]json.RawMessage{"childPolicy": jsonCfg} + cp := &loadBalancingConfig{ + Name: "grpclb", + Config: []byte(`{"childPolicy": [{"round_robin" : {}}, {"pick_first" : {}}]}`), + } + cpTargetField := "serviceName" + + gotChildConfig, err := validateChildPolicyConfig(cp, cpTargetField) + if err != nil || !cmp.Equal(gotChildConfig, wantChildConfig) { + t.Errorf("validateChildPolicyConfig(%v, %v) = {%v, %v}, want {%v, nil}", cp, cpTargetField, gotChildConfig, err, wantChildConfig) + } +} + +func TestValidateChildPolicyConfigErrors(t *testing.T) { + tests := []struct { + desc string + cp *loadBalancingConfig + wantErrPrefix string + }{ + { + desc: "unknown child policy", + cp: &loadBalancingConfig{ + Name: "unknown", + Config: []byte(`{}`), + }, + wantErrPrefix: "rls: balancer builder not found for child_policy", + }, + { + desc: "balancer builder does not implement ConfigParser", + cp: &loadBalancingConfig{ + Name: balancerWithoutConfigParserName, + Config: []byte(`{}`), + }, + wantErrPrefix: "rls: balancer builder for child_policy does not implement balancer.ConfigParser", + }, + { + desc: "child policy config parsing failure", + cp: &loadBalancingConfig{ + Name: "grpclb", + Config: []byte(`{"childPolicy": "not-an-array"}`), + }, + wantErrPrefix: "rls: childPolicy config validation failed", + }, + } + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + gotChildConfig, gotErr := validateChildPolicyConfig(test.cp, "") + if gotChildConfig != nil || !strings.HasPrefix(fmt.Sprint(gotErr), test.wantErrPrefix) { + t.Errorf("validateChildPolicyConfig(%v) = {%v, %v}, want {nil, %v}", test.cp, gotChildConfig, gotErr, test.wantErrPrefix) } }) }