diff --git a/xds/internal/balancer/clusterresolver/clusterresolver.go b/xds/internal/balancer/clusterresolver/clusterresolver.go index d49014cfa43..9b373fb3697 100644 --- a/xds/internal/balancer/clusterresolver/clusterresolver.go +++ b/xds/internal/balancer/clusterresolver/clusterresolver.go @@ -23,10 +23,12 @@ import ( "encoding/json" "errors" "fmt" + "strings" "google.golang.org/grpc/attributes" "google.golang.org/grpc/balancer" "google.golang.org/grpc/balancer/base" + "google.golang.org/grpc/balancer/roundrobin" "google.golang.org/grpc/connectivity" "google.golang.org/grpc/internal/buffer" "google.golang.org/grpc/internal/grpclog" @@ -35,6 +37,7 @@ import ( "google.golang.org/grpc/resolver" "google.golang.org/grpc/serviceconfig" "google.golang.org/grpc/xds/internal/balancer/priority" + "google.golang.org/grpc/xds/internal/balancer/ringhash" "google.golang.org/grpc/xds/internal/xdsclient" "google.golang.org/grpc/xds/internal/xdsclient/xdsresource" ) @@ -99,6 +102,9 @@ func (bb) ParseConfig(c json.RawMessage) (serviceconfig.LoadBalancingConfig, err if err := json.Unmarshal(c, &cfg); err != nil { return nil, fmt.Errorf("unable to unmarshal balancer config %s into cluster-resolver config, error: %v", string(c), err) } + if lbp := cfg.XDSLBPolicy; lbp != nil && !strings.EqualFold(lbp.Name, roundrobin.Name) && !strings.EqualFold(lbp.Name, ringhash.Name) { + return nil, fmt.Errorf("unsupported child policy with name %q, not one of {%q,%q}", lbp.Name, roundrobin.Name, ringhash.Name) + } return &cfg, nil } diff --git a/xds/internal/balancer/clusterresolver/config.go b/xds/internal/balancer/clusterresolver/config.go index cb870027a4e..2458b106772 100644 --- a/xds/internal/balancer/clusterresolver/config.go +++ b/xds/internal/balancer/clusterresolver/config.go @@ -21,13 +21,10 @@ import ( "bytes" "encoding/json" "fmt" - "strings" - "google.golang.org/grpc/balancer/roundrobin" internalserviceconfig "google.golang.org/grpc/internal/serviceconfig" "google.golang.org/grpc/serviceconfig" "google.golang.org/grpc/xds/internal/balancer/outlierdetection" - "google.golang.org/grpc/xds/internal/balancer/ringhash" "google.golang.org/grpc/xds/internal/xdsclient/bootstrap" ) @@ -167,19 +164,3 @@ type LBConfig struct { // is responsible for both locality picking and endpoint picking. XDSLBPolicy *internalserviceconfig.BalancerConfig `json:"xdsLbPolicy,omitempty"` } - -const ( - rrName = roundrobin.Name - rhName = ringhash.Name -) - -func parseConfig(c json.RawMessage) (*LBConfig, error) { - var cfg LBConfig - if err := json.Unmarshal(c, &cfg); err != nil { - return nil, err - } - if lbp := cfg.XDSLBPolicy; lbp != nil && !strings.EqualFold(lbp.Name, rrName) && !strings.EqualFold(lbp.Name, rhName) { - return nil, fmt.Errorf("unsupported child policy with name %q, not one of {%q,%q}", lbp.Name, rrName, rhName) - } - return &cfg, nil -} diff --git a/xds/internal/balancer/clusterresolver/config_test.go b/xds/internal/balancer/clusterresolver/config_test.go index fb859e75ba4..6e2d8624050 100644 --- a/xds/internal/balancer/clusterresolver/config_test.go +++ b/xds/internal/balancer/clusterresolver/config_test.go @@ -23,7 +23,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" - "google.golang.org/grpc/internal/balancer/stub" + "google.golang.org/grpc/balancer" internalserviceconfig "google.golang.org/grpc/internal/serviceconfig" "google.golang.org/grpc/xds/internal/balancer/ringhash" "google.golang.org/grpc/xds/internal/xdsclient/bootstrap" @@ -91,14 +91,6 @@ func TestDiscoveryMechanismTypeUnmarshalJSON(t *testing.T) { } } -func init() { - // This is needed now for the config parsing tests to pass. Otherwise they - // will fail with "RING_HASH unsupported". - // - // TODO: delete this once ring-hash policy is implemented and imported. - stub.Register(rhName, stub.BalancerFuncs{}) -} - const ( testJSONConfig1 = `{ "discoveryMechanisms": [{ @@ -257,7 +249,7 @@ func TestParseConfig(t *testing.T) { }, XDSLBPolicy: &internalserviceconfig.BalancerConfig{ Name: ringhash.Name, - Config: nil, + Config: &ringhash.LBConfig{MinRingSize: 1024, MaxRingSize: 8388608}, // Ringhash LB config with default min and max. }, }, wantErr: false, @@ -269,11 +261,22 @@ func TestParseConfig(t *testing.T) { }, } for _, tt := range tests { + b := balancer.Get(Name) + if b == nil { + t.Fatalf("LB policy %q not registered", Name) + } + cfgParser, ok := b.(balancer.ConfigParser) + if !ok { + t.Fatalf("LB policy %q does not support config parsing", Name) + } t.Run(tt.name, func(t *testing.T) { - got, err := parseConfig([]byte(tt.js)) + got, err := cfgParser.ParseConfig([]byte(tt.js)) if (err != nil) != tt.wantErr { t.Fatalf("parseConfig() error = %v, wantErr %v", err, tt.wantErr) } + if tt.wantErr { + return + } if diff := cmp.Diff(got, tt.want); diff != "" { t.Errorf("parseConfig() got unexpected output, diff (-got +want): %v", diff) } diff --git a/xds/internal/balancer/clusterresolver/configbuilder.go b/xds/internal/balancer/clusterresolver/configbuilder.go index dc91d7fbd13..186409bf9bc 100644 --- a/xds/internal/balancer/clusterresolver/configbuilder.go +++ b/xds/internal/balancer/clusterresolver/configbuilder.go @@ -304,22 +304,22 @@ func priorityLocalitiesToClusterImpl(localities []xdsresource.Locality, priority // ChildPolicy is not set. Will be set based on xdsLBPolicy } - if xdsLBPolicy == nil || xdsLBPolicy.Name == rrName { + if xdsLBPolicy == nil || xdsLBPolicy.Name == roundrobin.Name { // If lb policy is ROUND_ROBIN: // - locality-picking policy is weighted_target // - endpoint-picking policy is round_robin - logger.Infof("xds lb policy is %q, building config with weighted_target + round_robin", rrName) + logger.Infof("xds lb policy is %q, building config with weighted_target + round_robin", roundrobin.Name) // Child of weighted_target is hardcoded to round_robin. wtConfig, addrs := localitiesToWeightedTarget(localities, priorityName, rrBalancerConfig) clusterImplCfg.ChildPolicy = &internalserviceconfig.BalancerConfig{Name: weightedtarget.Name, Config: wtConfig} return clusterImplCfg, addrs, nil } - if xdsLBPolicy.Name == rhName { + if xdsLBPolicy.Name == ringhash.Name { // If lb policy is RIHG_HASH, will build one ring_hash policy as child. // The endpoints from all localities will be flattened to one addresses // list, and the ring_hash policy will pick endpoints from it. - logger.Infof("xds lb policy is %q, building config with ring_hash", rhName) + logger.Infof("xds lb policy is %q, building config with ring_hash", ringhash.Name) addrs := localitiesToRingHash(localities, priorityName) // Set child to ring_hash, note that the ring_hash config is from // xdsLBPolicy. @@ -327,7 +327,7 @@ func priorityLocalitiesToClusterImpl(localities []xdsresource.Locality, priority return clusterImplCfg, addrs, nil } - return nil, nil, fmt.Errorf("unsupported xds LB policy %q, not one of {%q,%q}", xdsLBPolicy.Name, rrName, rhName) + return nil, nil, fmt.Errorf("unsupported xds LB policy %q, not one of {%q,%q}", xdsLBPolicy.Name, roundrobin.Name, ringhash.Name) } // localitiesToRingHash takes a list of localities (with the same priority), and diff --git a/xds/internal/balancer/clusterresolver/configbuilder_test.go b/xds/internal/balancer/clusterresolver/configbuilder_test.go index cfe7de65d1a..d050df11b02 100644 --- a/xds/internal/balancer/clusterresolver/configbuilder_test.go +++ b/xds/internal/balancer/clusterresolver/configbuilder_test.go @@ -731,7 +731,7 @@ func TestPriorityLocalitiesToClusterImpl(t *testing.T) { }, }, priorityName: "test-priority", - childPolicy: &internalserviceconfig.BalancerConfig{Name: rrName}, + childPolicy: &internalserviceconfig.BalancerConfig{Name: roundrobin.Name}, mechanism: DiscoveryMechanism{ Cluster: testClusterName, Type: DiscoveryMechanismTypeEDS, @@ -789,7 +789,7 @@ func TestPriorityLocalitiesToClusterImpl(t *testing.T) { }, }, priorityName: "test-priority", - childPolicy: &internalserviceconfig.BalancerConfig{Name: rhName, Config: &ringhash.LBConfig{MinRingSize: 1, MaxRingSize: 2}}, + childPolicy: &internalserviceconfig.BalancerConfig{Name: ringhash.Name, Config: &ringhash.LBConfig{MinRingSize: 1, MaxRingSize: 2}}, // lrsServer is nil, so LRS policy will not be used. wantConfig: &clusterimpl.LBConfig{ ChildPolicy: &internalserviceconfig.BalancerConfig{