diff --git a/internal/envconfig/envconfig.go b/internal/envconfig/envconfig.go index 7edd196bd3d..6e6b00ffc1a 100644 --- a/internal/envconfig/envconfig.go +++ b/internal/envconfig/envconfig.go @@ -21,6 +21,7 @@ package envconfig import ( "os" + "strconv" "strings" ) @@ -36,4 +37,23 @@ var ( // AdvertiseCompressors is set if registered compressor should be advertised // ("GRPC_GO_ADVERTISE_COMPRESSORS" is not "false"). AdvertiseCompressors = !strings.EqualFold(os.Getenv(advertiseCompressorsStr), "false") + // RingHashCap indicates the maximum ring size which defaults to 4096 + // entries but may be overridden by setting the environment variable + // "GRPC_RING_HASH_CAP". This does not override the default bounds + // checking which NACKs configs specifying ring sizes > 8*1024*1024 (~8M). + RingHashCap = uint64FromEnv("GRPC_RING_HASH_CAP", 4096, 1, 8*1024*1024) ) + +func uint64FromEnv(envVar string, def, min, max uint64) uint64 { + v, err := strconv.ParseUint(os.Getenv(envVar), 10, 64) + if err != nil { + return def + } + if v < min { + return min + } + if v > max { + return max + } + return v +} diff --git a/internal/envconfig/envconfig_test.go b/internal/envconfig/envconfig_test.go new file mode 100644 index 00000000000..13923e5bbe6 --- /dev/null +++ b/internal/envconfig/envconfig_test.go @@ -0,0 +1,74 @@ +/* + * + * Copyright 2022 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 envconfig + +import ( + "os" + "testing" + + "google.golang.org/grpc/internal/grpctest" +) + +type s struct { + grpctest.Tester +} + +func Test(t *testing.T) { + grpctest.RunSubTests(t, s{}) +} + +func (s) TestUint64FromEnv(t *testing.T) { + + var testCases = []struct { + name string + val string + def, min, max uint64 + want uint64 + }{ + { + name: "error parsing", + val: "asdf", def: 5, want: 5, + }, { + name: "unset", + val: "", def: 5, want: 5, + }, { + name: "too low", + val: "5", min: 10, want: 10, + }, { + name: "too high", + val: "5", max: 2, want: 2, + }, { + name: "in range", + val: "17391", def: 13000, min: 12000, max: 18000, want: 17391, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + const testVar = "testvar" + if tc.val == "" { + os.Unsetenv(testVar) + } else { + os.Setenv(testVar, tc.val) + } + if got := uint64FromEnv(testVar, tc.def, tc.min, tc.max); got != tc.want { + t.Errorf("uint64FromEnv(%q(=%q), %v, %v, %v) = %v; want %v", testVar, tc.val, tc.def, tc.min, tc.max, got, tc.want) + } + }) + } +} diff --git a/xds/internal/balancer/ringhash/config.go b/xds/internal/balancer/ringhash/config.go index 4278b0636c7..4763120fa64 100644 --- a/xds/internal/balancer/ringhash/config.go +++ b/xds/internal/balancer/ringhash/config.go @@ -22,6 +22,7 @@ import ( "encoding/json" "fmt" + "google.golang.org/grpc/internal/envconfig" "google.golang.org/grpc/serviceconfig" ) @@ -36,8 +37,6 @@ type LBConfig struct { const ( defaultMinSize = 1024 defaultMaxSize = 4096 - // TODO(apolcyn): make makeRingSizeCap configurable, with either a dial option or global setting - maxRingSizeCap = 4096 ) func parseConfig(c json.RawMessage) (*LBConfig, error) { @@ -51,14 +50,14 @@ func parseConfig(c json.RawMessage) (*LBConfig, error) { if cfg.MaxRingSize == 0 { cfg.MaxRingSize = defaultMaxSize } - if cfg.MinRingSize > maxRingSizeCap { - cfg.MinRingSize = maxRingSizeCap - } - if cfg.MaxRingSize > maxRingSizeCap { - cfg.MaxRingSize = maxRingSizeCap - } if cfg.MinRingSize > cfg.MaxRingSize { return nil, fmt.Errorf("min %v is greater than max %v", cfg.MinRingSize, cfg.MaxRingSize) } + if cfg.MinRingSize > envconfig.RingHashCap { + cfg.MinRingSize = envconfig.RingHashCap + } + if cfg.MaxRingSize > envconfig.RingHashCap { + cfg.MaxRingSize = envconfig.RingHashCap + } return &cfg, nil } diff --git a/xds/internal/balancer/ringhash/config_test.go b/xds/internal/balancer/ringhash/config_test.go index 175301981ef..d8f9ed30bb6 100644 --- a/xds/internal/balancer/ringhash/config_test.go +++ b/xds/internal/balancer/ringhash/config_test.go @@ -22,14 +22,16 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "google.golang.org/grpc/internal/envconfig" ) func (s) TestParseConfig(t *testing.T) { tests := []struct { - name string - js string - want *LBConfig - wantErr bool + name string + js string + envConfigCap uint64 + want *LBConfig + wantErr bool }{ { name: "OK", @@ -52,9 +54,42 @@ func (s) TestParseConfig(t *testing.T) { want: nil, wantErr: true, }, + { + name: "min greater than max greater than global limit", + js: `{"minRingSize": 6000, "maxRingSize": 5000}`, + want: nil, + wantErr: true, + }, + { + name: "max greater than global limit", + js: `{"minRingSize": 1, "maxRingSize": 6000}`, + want: &LBConfig{MinRingSize: 1, MaxRingSize: 4096}, + }, + { + name: "min and max greater than global limit", + js: `{"minRingSize": 5000, "maxRingSize": 6000}`, + want: &LBConfig{MinRingSize: 4096, MaxRingSize: 4096}, + }, + { + name: "min and max less than raised global limit", + js: `{"minRingSize": 5000, "maxRingSize": 6000}`, + envConfigCap: 8000, + want: &LBConfig{MinRingSize: 5000, MaxRingSize: 6000}, + }, + { + name: "min and max greater than raised global limit", + js: `{"minRingSize": 10000, "maxRingSize": 10000}`, + envConfigCap: 8000, + want: &LBConfig{MinRingSize: 8000, MaxRingSize: 8000}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + if tt.envConfigCap != 0 { + old := envconfig.RingHashCap + defer func() { envconfig.RingHashCap = old }() + envconfig.RingHashCap = tt.envConfigCap + } got, err := parseConfig([]byte(tt.js)) if (err != nil) != tt.wantErr { t.Errorf("parseConfig() error = %v, wantErr %v", err, tt.wantErr)