Skip to content

Commit

Permalink
xds: remove env var protetion of advanced routing features (#4529)
Browse files Browse the repository at this point in the history
  • Loading branch information
dfawley authored Jun 10, 2021
1 parent 95e48a8 commit 6351a55
Show file tree
Hide file tree
Showing 11 changed files with 15 additions and 145 deletions.
14 changes: 0 additions & 14 deletions internal/xds/env/env.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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.
//
Expand Down
4 changes: 0 additions & 4 deletions xds/internal/balancer/edsbalancer/eds_impl.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down
9 changes: 0 additions & 9 deletions xds/internal/balancer/edsbalancer/eds_impl_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
5 changes: 1 addition & 4 deletions xds/internal/httpfilter/fault/fault.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -63,9 +62,7 @@ var statusMap = map[int]codes.Code{
}

func init() {
if env.FaultInjectionSupport {
httpfilter.Register(builder{})
}
httpfilter.Register(builder{})
}

type builder struct {
Expand Down
9 changes: 0 additions & 9 deletions xds/internal/httpfilter/fault/fault_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
3 changes: 1 addition & 2 deletions xds/internal/resolver/serviceconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}

Expand Down
36 changes: 12 additions & 24 deletions xds/internal/resolver/xds_resolver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 },
Expand Down Expand Up @@ -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(),
Expand Down
3 changes: 0 additions & 3 deletions xds/internal/xdsclient/cds_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 }()
Expand Down
38 changes: 0 additions & 38 deletions xds/internal/xdsclient/lds_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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)},
Expand Down Expand Up @@ -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},
Expand Down Expand Up @@ -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)
Expand All @@ -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
})
}
}
Expand Down Expand Up @@ -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)
Expand Down
30 changes: 0 additions & 30 deletions xds/internal/xdsclient/rds_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -474,28 +472,17 @@ 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(),
cmp.Transformer("FilterConfig", func(fc httpfilter.FilterConfig) string {
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
}
})
}
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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}),
Expand All @@ -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}),
Expand All @@ -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)
Expand Down
Loading

0 comments on commit 6351a55

Please sign in to comment.