From 6351a55c3895e5658b2c59769c81109d962d0e04 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Thu, 10 Jun 2021 09:33:06 -0700 Subject: [PATCH] xds: remove env var protetion of advanced routing features (#4529) --- internal/xds/env/env.go | 14 ------- xds/internal/balancer/edsbalancer/eds_impl.go | 4 -- .../balancer/edsbalancer/eds_impl_test.go | 9 ----- xds/internal/httpfilter/fault/fault.go | 5 +-- xds/internal/httpfilter/fault/fault_test.go | 9 ----- xds/internal/resolver/serviceconfig.go | 3 +- xds/internal/resolver/xds_resolver_test.go | 36 ++++++------------ xds/internal/xdsclient/cds_test.go | 3 -- xds/internal/xdsclient/lds_test.go | 38 ------------------- xds/internal/xdsclient/rds_test.go | 30 --------------- xds/internal/xdsclient/xds.go | 9 +---- 11 files changed, 15 insertions(+), 145 deletions(-) diff --git a/internal/xds/env/env.go b/internal/xds/env/env.go index db9ac93b968c..05a017f2611b 100644 --- a/internal/xds/env/env.go +++ b/internal/xds/env/env.go @@ -39,9 +39,6 @@ const ( // When both bootstrap FileName and FileContent are set, FileName is used. BootstrapFileContentEnv = "GRPC_XDS_BOOTSTRAP_CONFIG" - circuitBreakingSupportEnv = "GRPC_XDS_EXPERIMENTAL_CIRCUIT_BREAKING" - timeoutSupportEnv = "GRPC_XDS_EXPERIMENTAL_ENABLE_TIMEOUT" - faultInjectionSupportEnv = "GRPC_XDS_EXPERIMENTAL_FAULT_INJECTION" clientSideSecuritySupportEnv = "GRPC_XDS_EXPERIMENTAL_SECURITY_SUPPORT" aggregateAndDNSSupportEnv = "GRPC_XDS_EXPERIMENTAL_ENABLE_AGGREGATE_AND_LOGICAL_DNS_CLUSTER" @@ -63,17 +60,6 @@ var ( // When both bootstrap FileName and FileContent are set, FileName is used. BootstrapFileContent = os.Getenv(BootstrapFileContentEnv) - // CircuitBreakingSupport indicates whether circuit breaking support is - // enabled, which can be disabled by setting the environment variable - // "GRPC_XDS_EXPERIMENTAL_CIRCUIT_BREAKING" to "false". - CircuitBreakingSupport = !strings.EqualFold(os.Getenv(circuitBreakingSupportEnv), "false") - // TimeoutSupport indicates whether support for max_stream_duration in - // route actions is enabled. This can be disabled by setting the - // environment variable "GRPC_XDS_EXPERIMENTAL_ENABLE_TIMEOUT" to "false". - TimeoutSupport = !strings.EqualFold(os.Getenv(timeoutSupportEnv), "false") - // FaultInjectionSupport is used to control both fault injection and HTTP - // filter support. - FaultInjectionSupport = !strings.EqualFold(os.Getenv(faultInjectionSupportEnv), "false") // ClientSideSecuritySupport is used to control processing of security // configuration on the client-side. // diff --git a/xds/internal/balancer/edsbalancer/eds_impl.go b/xds/internal/balancer/edsbalancer/eds_impl.go index db11dec6f2f8..c4dfd6dd6d0b 100644 --- a/xds/internal/balancer/edsbalancer/eds_impl.go +++ b/xds/internal/balancer/edsbalancer/eds_impl.go @@ -31,7 +31,6 @@ import ( "google.golang.org/grpc/connectivity" "google.golang.org/grpc/internal" "google.golang.org/grpc/internal/grpclog" - "google.golang.org/grpc/internal/xds/env" "google.golang.org/grpc/resolver" "google.golang.org/grpc/status" xdsi "google.golang.org/grpc/xds/internal" @@ -418,9 +417,6 @@ func (edsImpl *edsBalancerImpl) handleSubConnStateChange(sc balancer.SubConn, s // updateServiceRequestsConfig handles changes to the circuit breaking configuration. func (edsImpl *edsBalancerImpl) updateServiceRequestsConfig(serviceName string, max *uint32) { - if !env.CircuitBreakingSupport { - return - } edsImpl.pickerMu.Lock() var updatePicker bool if edsImpl.serviceRequestsCounter == nil || edsImpl.serviceRequestsCounter.ServiceName != serviceName { diff --git a/xds/internal/balancer/edsbalancer/eds_impl_test.go b/xds/internal/balancer/edsbalancer/eds_impl_test.go index 2c1498dd3f78..8599573a2917 100644 --- a/xds/internal/balancer/edsbalancer/eds_impl_test.go +++ b/xds/internal/balancer/edsbalancer/eds_impl_test.go @@ -35,7 +35,6 @@ import ( "google.golang.org/grpc/balancer/roundrobin" "google.golang.org/grpc/connectivity" "google.golang.org/grpc/internal/balancer/stub" - "google.golang.org/grpc/internal/xds/env" xdsinternal "google.golang.org/grpc/xds/internal" "google.golang.org/grpc/xds/internal/balancer/balancergroup" "google.golang.org/grpc/xds/internal/testutils" @@ -575,10 +574,6 @@ func (s) TestEDS_UpdateSubBalancerName(t *testing.T) { } func (s) TestEDS_CircuitBreaking(t *testing.T) { - origCircuitBreakingSupport := env.CircuitBreakingSupport - env.CircuitBreakingSupport = true - defer func() { env.CircuitBreakingSupport = origCircuitBreakingSupport }() - cc := testutils.NewTestClientConn(t) edsb := newEDSBalancerImpl(cc, balancer.BuildOptions{}, nil, nil, nil) edsb.enqueueChildBalancerStateUpdate = edsb.updateState @@ -812,10 +807,6 @@ func (s) TestDropPicker(t *testing.T) { } func (s) TestEDS_LoadReport(t *testing.T) { - origCircuitBreakingSupport := env.CircuitBreakingSupport - env.CircuitBreakingSupport = true - defer func() { env.CircuitBreakingSupport = origCircuitBreakingSupport }() - // We create an xdsClientWrapper with a dummy xdsClient which only // implements the LoadStore() method to return the underlying load.Store to // be used. diff --git a/xds/internal/httpfilter/fault/fault.go b/xds/internal/httpfilter/fault/fault.go index ee2ed9fd4922..42f8e70af93b 100644 --- a/xds/internal/httpfilter/fault/fault.go +++ b/xds/internal/httpfilter/fault/fault.go @@ -33,7 +33,6 @@ import ( "google.golang.org/grpc/codes" "google.golang.org/grpc/internal/grpcrand" iresolver "google.golang.org/grpc/internal/resolver" - "google.golang.org/grpc/internal/xds/env" "google.golang.org/grpc/metadata" "google.golang.org/grpc/status" "google.golang.org/grpc/xds/internal/httpfilter" @@ -63,9 +62,7 @@ var statusMap = map[int]codes.Code{ } func init() { - if env.FaultInjectionSupport { - httpfilter.Register(builder{}) - } + httpfilter.Register(builder{}) } type builder struct { diff --git a/xds/internal/httpfilter/fault/fault_test.go b/xds/internal/httpfilter/fault/fault_test.go index 1c6db707c6a3..46606449cb9d 100644 --- a/xds/internal/httpfilter/fault/fault_test.go +++ b/xds/internal/httpfilter/fault/fault_test.go @@ -40,10 +40,8 @@ import ( "google.golang.org/grpc/internal/grpctest" "google.golang.org/grpc/internal/testutils" "google.golang.org/grpc/internal/xds" - "google.golang.org/grpc/internal/xds/env" "google.golang.org/grpc/metadata" "google.golang.org/grpc/status" - "google.golang.org/grpc/xds/internal/httpfilter" xtestutils "google.golang.org/grpc/xds/internal/testutils" "google.golang.org/grpc/xds/internal/testutils/e2e" "google.golang.org/protobuf/types/known/wrapperspb" @@ -140,13 +138,6 @@ func clientSetup(t *testing.T) (*e2e.ManagementServer, string, uint32, func()) { } } -func init() { - env.FaultInjectionSupport = true - // Manually register to avoid a race between this init and the one that - // check the env var to register the fault injection filter. - httpfilter.Register(builder{}) -} - func (s) TestFaultInjection_Unary(t *testing.T) { type subcase struct { name string diff --git a/xds/internal/resolver/serviceconfig.go b/xds/internal/resolver/serviceconfig.go index 31941fdc022c..2521b0d0193e 100644 --- a/xds/internal/resolver/serviceconfig.go +++ b/xds/internal/resolver/serviceconfig.go @@ -28,7 +28,6 @@ import ( "google.golang.org/grpc/codes" iresolver "google.golang.org/grpc/internal/resolver" "google.golang.org/grpc/internal/wrr" - "google.golang.org/grpc/internal/xds/env" "google.golang.org/grpc/status" "google.golang.org/grpc/xds/internal/balancer/clustermanager" "google.golang.org/grpc/xds/internal/httpfilter" @@ -179,7 +178,7 @@ func (cs *configSelector) SelectConfig(rpcInfo iresolver.RPCInfo) (*iresolver.RP Interceptor: interceptor, } - if env.TimeoutSupport && rt.maxStreamDuration != 0 { + if rt.maxStreamDuration != 0 { config.MethodConfig.Timeout = &rt.maxStreamDuration } diff --git a/xds/internal/resolver/xds_resolver_test.go b/xds/internal/resolver/xds_resolver_test.go index a41920998272..a519557df6ac 100644 --- a/xds/internal/resolver/xds_resolver_test.go +++ b/xds/internal/resolver/xds_resolver_test.go @@ -38,7 +38,6 @@ import ( iresolver "google.golang.org/grpc/internal/resolver" "google.golang.org/grpc/internal/testutils" "google.golang.org/grpc/internal/wrr" - "google.golang.org/grpc/internal/xds/env" "google.golang.org/grpc/resolver" "google.golang.org/grpc/serviceconfig" "google.golang.org/grpc/status" @@ -686,7 +685,6 @@ func (s) TestXDSResolverWRR(t *testing.T) { } func (s) TestXDSResolverMaxStreamDuration(t *testing.T) { - defer func(old bool) { env.TimeoutSupport = old }(env.TimeoutSupport) xdsC := fakeclient.NewClient() xdsR, tcc, cancel := testSetup(t, setupOpts{ xdsClientFunc: func() (xdsclient.XDSClient, error) { return xdsC, nil }, @@ -740,35 +738,25 @@ func (s) TestXDSResolverMaxStreamDuration(t *testing.T) { } testCases := []struct { - name string - method string - timeoutSupport bool - want *time.Duration + name string + method string + want *time.Duration }{{ - name: "RDS setting", - method: "/foo/method", - timeoutSupport: true, - want: newDurationP(5 * time.Second), + name: "RDS setting", + method: "/foo/method", + want: newDurationP(5 * time.Second), }, { - name: "timeout support disabled", - method: "/foo/method", - timeoutSupport: false, - want: nil, + name: "explicit zero in RDS; ignore LDS", + method: "/bar/method", + want: nil, }, { - name: "explicit zero in RDS; ignore LDS", - method: "/bar/method", - timeoutSupport: true, - want: nil, - }, { - name: "no config in RDS; fallback to LDS", - method: "/baz/method", - timeoutSupport: true, - want: newDurationP(time.Second), + name: "no config in RDS; fallback to LDS", + method: "/baz/method", + want: newDurationP(time.Second), }} for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - env.TimeoutSupport = tc.timeoutSupport req := iresolver.RPCInfo{ Method: tc.method, Context: context.Background(), diff --git a/xds/internal/xdsclient/cds_test.go b/xds/internal/xdsclient/cds_test.go index 83b9071ad134..88bfe21a7bdb 100644 --- a/xds/internal/xdsclient/cds_test.go +++ b/xds/internal/xdsclient/cds_test.go @@ -303,9 +303,6 @@ func (s) TestValidateCluster_Success(t *testing.T) { }, } - origCircuitBreakingSupport := env.CircuitBreakingSupport - env.CircuitBreakingSupport = true - defer func() { env.CircuitBreakingSupport = origCircuitBreakingSupport }() oldAggregateAndDNSSupportEnv := env.AggregateAndDNSSupportEnv env.AggregateAndDNSSupportEnv = true defer func() { env.AggregateAndDNSSupportEnv = oldAggregateAndDNSSupportEnv }() diff --git a/xds/internal/xdsclient/lds_test.go b/xds/internal/xdsclient/lds_test.go index 1667698bd578..ebebde84b6a1 100644 --- a/xds/internal/xdsclient/lds_test.go +++ b/xds/internal/xdsclient/lds_test.go @@ -34,7 +34,6 @@ import ( "google.golang.org/protobuf/types/known/durationpb" "google.golang.org/grpc/internal/testutils" - "google.golang.org/grpc/internal/xds/env" "google.golang.org/grpc/xds/internal/httpfilter" "google.golang.org/grpc/xds/internal/version" @@ -186,7 +185,6 @@ func (s) TestUnmarshalListener_ClientSide(t *testing.T) { wantUpdate map[string]ListenerUpdate wantMD UpdateMetadata wantErr bool - disableFI bool // disable fault injection }{ { name: "non-listener resource", @@ -358,18 +356,6 @@ func (s) TestUnmarshalListener_ClientSide(t *testing.T) { Version: testVersion, }, }, - { - name: "v3 with custom filter, fault injection disabled", - resources: []*anypb.Any{v3LisWithFilters(customFilter)}, - wantUpdate: map[string]ListenerUpdate{ - v3LDSTarget: {RouteConfigName: v3RouteConfigName, MaxStreamDuration: time.Second, Raw: v3LisWithFilters(customFilter)}, - }, - wantMD: UpdateMetadata{ - Status: ServiceStatusACKed, - Version: testVersion, - }, - disableFI: true, - }, { name: "v3 with two filters with same name", resources: []*anypb.Any{v3LisWithFilters(customFilter, customFilter)}, @@ -477,22 +463,6 @@ func (s) TestUnmarshalListener_ClientSide(t *testing.T) { Version: testVersion, }, }, - { - name: "v3 with error filter, fault injection disabled", - resources: []*anypb.Any{v3LisWithFilters(errFilter)}, - wantUpdate: map[string]ListenerUpdate{ - v3LDSTarget: { - RouteConfigName: v3RouteConfigName, - MaxStreamDuration: time.Second, - Raw: v3LisWithFilters(errFilter), - }, - }, - wantMD: UpdateMetadata{ - Status: ServiceStatusACKed, - Version: testVersion, - }, - disableFI: true, - }, { name: "v2 listener resource", resources: []*anypb.Any{v2Lis}, @@ -572,9 +542,6 @@ func (s) TestUnmarshalListener_ClientSide(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - oldFI := env.FaultInjectionSupport - env.FaultInjectionSupport = !test.disableFI - update, md, err := UnmarshalListener(testVersion, test.resources, nil) if (err != nil) != test.wantErr { t.Fatalf("UnmarshalListener(), got err: %v, wantErr: %v", err, test.wantErr) @@ -585,7 +552,6 @@ func (s) TestUnmarshalListener_ClientSide(t *testing.T) { if diff := cmp.Diff(md, test.wantMD, cmpOptsIgnoreDetails); diff != "" { t.Errorf("got unexpected metadata, diff (-got +want): %v", diff) } - env.FaultInjectionSupport = oldFI }) } } @@ -1287,10 +1253,6 @@ func (s) TestUnmarshalListener_ServerSide(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - oldFI := env.FaultInjectionSupport - env.FaultInjectionSupport = true - defer func() { env.FaultInjectionSupport = oldFI }() - gotUpdate, md, err := UnmarshalListener(testVersion, test.resources, nil) if (err != nil) != (test.wantErr != "") { t.Fatalf("UnmarshalListener(), got err: %v, wantErr: %v", err, test.wantErr) diff --git a/xds/internal/xdsclient/rds_test.go b/xds/internal/xdsclient/rds_test.go index 660a2f29d21a..adabe5f41671 100644 --- a/xds/internal/xdsclient/rds_test.go +++ b/xds/internal/xdsclient/rds_test.go @@ -29,7 +29,6 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "google.golang.org/grpc/internal/testutils" - "google.golang.org/grpc/internal/xds/env" "google.golang.org/grpc/xds/internal/httpfilter" "google.golang.org/grpc/xds/internal/version" "google.golang.org/protobuf/types/known/durationpb" @@ -88,7 +87,6 @@ func (s) TestRDSGenerateRDSUpdateFromRouteConfiguration(t *testing.T) { rc *v3routepb.RouteConfiguration wantUpdate RouteConfigUpdate wantError bool - disableFI bool // disable fault injection }{ { name: "default-route-match-field-is-nil", @@ -474,19 +472,10 @@ func (s) TestRDSGenerateRDSUpdateFromRouteConfiguration(t *testing.T) { rc: goodRouteConfigWithFilterConfigs(map[string]*anypb.Any{"foo": wrappedOptionalFilter("unknown.custom.filter")}), wantUpdate: goodUpdateWithFilterConfigs(nil), }, - { - name: "good-route-config-with-http-err-filter-config-fi-disabled", - disableFI: true, - rc: goodRouteConfigWithFilterConfigs(map[string]*anypb.Any{"foo": errFilterConfig}), - wantUpdate: goodUpdateWithFilterConfigs(nil), - }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - oldFI := env.FaultInjectionSupport - env.FaultInjectionSupport = !test.disableFI - gotUpdate, gotError := generateRDSUpdateFromRouteConfiguration(test.rc, nil, false) if (gotError != nil) != test.wantError || !cmp.Equal(gotUpdate, test.wantUpdate, cmpopts.EquateEmpty(), @@ -494,8 +483,6 @@ func (s) TestRDSGenerateRDSUpdateFromRouteConfiguration(t *testing.T) { return fmt.Sprint(fc) })) { t.Errorf("generateRDSUpdateFromRouteConfiguration(%+v, %v) returned unexpected, diff (-want +got):\\n%s", test.rc, ldsTarget, cmp.Diff(test.wantUpdate, gotUpdate, cmpopts.EquateEmpty())) - - env.FaultInjectionSupport = oldFI } }) } @@ -815,7 +802,6 @@ func (s) TestRoutesProtoToSlice(t *testing.T) { routes []*v3routepb.Route wantRoutes []*Route wantErr bool - disableFI bool // disable fault injection }{ { name: "no path", @@ -1182,12 +1168,6 @@ func (s) TestRoutesProtoToSlice(t *testing.T) { routes: goodRouteWithFilterConfigs(map[string]*anypb.Any{"foo": wrappedOptionalFilter("custom.filter")}), wantRoutes: goodUpdateWithFilterConfigs(map[string]httpfilter.FilterConfig{"foo": filterConfig{Override: customFilterConfig}}), }, - { - name: "with custom HTTP filter config, FI disabled", - disableFI: true, - routes: goodRouteWithFilterConfigs(map[string]*anypb.Any{"foo": customFilterConfig}), - wantRoutes: goodUpdateWithFilterConfigs(nil), - }, { name: "with erroring custom HTTP filter config", routes: goodRouteWithFilterConfigs(map[string]*anypb.Any{"foo": errFilterConfig}), @@ -1198,12 +1178,6 @@ func (s) TestRoutesProtoToSlice(t *testing.T) { routes: goodRouteWithFilterConfigs(map[string]*anypb.Any{"foo": wrappedOptionalFilter("err.custom.filter")}), wantErr: true, }, - { - name: "with erroring custom HTTP filter config, FI disabled", - disableFI: true, - routes: goodRouteWithFilterConfigs(map[string]*anypb.Any{"foo": errFilterConfig}), - wantRoutes: goodUpdateWithFilterConfigs(nil), - }, { name: "with unknown custom HTTP filter config", routes: goodRouteWithFilterConfigs(map[string]*anypb.Any{"foo": unknownFilterConfig}), @@ -1226,10 +1200,6 @@ func (s) TestRoutesProtoToSlice(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - oldFI := env.FaultInjectionSupport - env.FaultInjectionSupport = !tt.disableFI - defer func() { env.FaultInjectionSupport = oldFI }() - got, err := routesProtoToSlice(tt.routes, nil, false) if (err != nil) != tt.wantErr { t.Fatalf("routesProtoToSlice() error = %v, wantErr %v", err, tt.wantErr) diff --git a/xds/internal/xdsclient/xds.go b/xds/internal/xdsclient/xds.go index 44fd883e3f36..dd68f6c68bc8 100644 --- a/xds/internal/xdsclient/xds.go +++ b/xds/internal/xdsclient/xds.go @@ -178,7 +178,7 @@ func validateHTTPFilterConfig(cfg *anypb.Any, lds, optional bool) (httpfilter.Fi } func processHTTPFilterOverrides(cfgs map[string]*anypb.Any) (map[string]httpfilter.FilterConfig, error) { - if !env.FaultInjectionSupport || len(cfgs) == 0 { + if len(cfgs) == 0 { return nil, nil } m := make(map[string]httpfilter.FilterConfig) @@ -207,10 +207,6 @@ func processHTTPFilterOverrides(cfgs map[string]*anypb.Any) (map[string]httpfilt } func processHTTPFilters(filters []*v3httppb.HttpFilter, server bool) ([]HTTPFilter, error) { - if !env.FaultInjectionSupport { - return nil, nil - } - ret := make([]HTTPFilter, 0, len(filters)) seenNames := make(map[string]bool, len(filters)) for _, filter := range filters { @@ -776,9 +772,6 @@ func securityConfigFromCommonTLSContext(common *v3tlspb.CommonTlsContext) (*Secu // the received cluster resource. Returns nil if no CircuitBreakers or no // Thresholds in CircuitBreakers. func circuitBreakersFromCluster(cluster *v3clusterpb.Cluster) *uint32 { - if !env.CircuitBreakingSupport { - return nil - } for _, threshold := range cluster.GetCircuitBreakers().GetThresholds() { if threshold.GetPriority() != v3corepb.RoutingPriority_DEFAULT { continue