Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xds: remove env var protetion of advanced routing features #4529

Merged
merged 1 commit into from Jun 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 0 additions & 14 deletions internal/xds/env/env.go
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
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
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
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
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
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
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
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
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
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