Changed Outlier Detection Env Var to default true (#5673)

internal/envconfig/xds.go

@@ -84,9 +84,9 @@ var (
 	// "GRPC_XDS_EXPERIMENTAL_RBAC" to "false".
 	XDSRBAC = !strings.EqualFold(os.Getenv(rbacSupportEnv), "false")
 	// XDSOutlierDetection indicates whether outlier detection support is
-	// enabled, which can be enabled by setting the environment variable
-	// "GRPC_EXPERIMENTAL_ENABLE_OUTLIER_DETECTION" to "true".
-	XDSOutlierDetection = strings.EqualFold(os.Getenv(outlierDetectionSupportEnv), "true")
+	// enabled, which can be disabled by setting the environment variable
+	// "GRPC_EXPERIMENTAL_ENABLE_OUTLIER_DETECTION" to "false".
+	XDSOutlierDetection = !strings.EqualFold(os.Getenv(outlierDetectionSupportEnv), "false")
 	// XDSFederation indicates whether federation support is enabled.
 	XDSFederation = strings.EqualFold(os.Getenv(federationEnv), "true")
 

internal/internal.go

@@ -120,22 +120,6 @@ var (
 	//
 	// TODO: Remove this function once the RBAC env var is removed.
 	UnregisterRBACHTTPFilterForTesting func()
-
-	// RegisterOutlierDetectionBalancerForTesting registers the Outlier
-	// Detection Balancer for testing purposes, regardless of the Outlier
-	// Detection environment variable.
-	//
-	// TODO: Remove this function once the Outlier Detection env var is removed.
-	RegisterOutlierDetectionBalancerForTesting func()
-
-	// UnregisterOutlierDetectionBalancerForTesting unregisters the Outlier
-	// Detection Balancer for testing purposes. This is needed because there is
-	// no way to unregister the Outlier Detection Balancer after registering it
-	// solely for testing purposes using
-	// RegisterOutlierDetectionBalancerForTesting().
-	//
-	// TODO: Remove this function once the Outlier Detection env var is removed.
-	UnregisterOutlierDetectionBalancerForTesting func()
 )
 
 // HealthChecker defines the signature of the client-side LB channel health checking function.

test/xds/xds_client_outlier_detection_test.go

@@ -32,8 +32,6 @@ import (
 	v3routepb "github.com/envoyproxy/go-control-plane/envoy/config/route/v3"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/credentials/insecure"
-	"google.golang.org/grpc/internal"
-	"google.golang.org/grpc/internal/envconfig"
 	"google.golang.org/grpc/internal/stubserver"
 	"google.golang.org/grpc/internal/testutils/xds/e2e"
 	testgrpc "google.golang.org/grpc/test/grpc_testing"
@@ -49,14 +47,6 @@ import (
 // Detection balancer. This test verifies that an RPC is able to proceed
 // normally with this configuration.
 func (s) TestOutlierDetection_NoopConfig(t *testing.T) {
-	oldOD := envconfig.XDSOutlierDetection
-	envconfig.XDSOutlierDetection = true
-	internal.RegisterOutlierDetectionBalancerForTesting()
-	defer func() {
-		envconfig.XDSOutlierDetection = oldOD
-		internal.UnregisterOutlierDetectionBalancerForTesting()
-	}()
-
 	managementServer, nodeID, _, resolver, cleanup1 := e2e.SetupManagementServer(t, nil)
 	defer cleanup1()
 
@@ -129,14 +119,6 @@ func clusterWithOutlierDetection(clusterName, edsServiceName string, secLevel e2
 // Detection Balancer should eject the connection to the backend which
 // constantly errors, and thus RPC's should mainly go to backend 1 and 2.
 func (s) TestOutlierDetectionWithOutlier(t *testing.T) {
-	oldOD := envconfig.XDSOutlierDetection
-	envconfig.XDSOutlierDetection = true
-	internal.RegisterOutlierDetectionBalancerForTesting()
-	defer func() {
-		envconfig.XDSOutlierDetection = oldOD
-		internal.UnregisterOutlierDetectionBalancerForTesting()
-	}()
-
 	managementServer, nodeID, _, resolver, cleanup := e2e.SetupManagementServer(t, nil)
 	defer cleanup()
 

xds/internal/balancer/balancer.go

@@ -20,10 +20,11 @@
 package balancer
 
 import (
-	_ "google.golang.org/grpc/balancer/weightedtarget"               // Register the weighted_target balancer
-	_ "google.golang.org/grpc/xds/internal/balancer/cdsbalancer"     // Register the CDS balancer
-	_ "google.golang.org/grpc/xds/internal/balancer/clusterimpl"     // Register the xds_cluster_impl balancer
-	_ "google.golang.org/grpc/xds/internal/balancer/clustermanager"  // Register the xds_cluster_manager balancer
-	_ "google.golang.org/grpc/xds/internal/balancer/clusterresolver" // Register the xds_cluster_resolver balancer
-	_ "google.golang.org/grpc/xds/internal/balancer/priority"        // Register the priority balancer
+	_ "google.golang.org/grpc/balancer/weightedtarget"                // Register the weighted_target balancer
+	_ "google.golang.org/grpc/xds/internal/balancer/cdsbalancer"      // Register the CDS balancer
+	_ "google.golang.org/grpc/xds/internal/balancer/clusterimpl"      // Register the xds_cluster_impl balancer
+	_ "google.golang.org/grpc/xds/internal/balancer/clustermanager"   // Register the xds_cluster_manager balancer
+	_ "google.golang.org/grpc/xds/internal/balancer/clusterresolver"  // Register the xds_cluster_resolver balancer
+	_ "google.golang.org/grpc/xds/internal/balancer/outlierdetection" // Register the outlier_detection balancer
+	_ "google.golang.org/grpc/xds/internal/balancer/priority"         // Register the priority balancer
 )

xds/internal/balancer/cdsbalancer/cdsbalancer_security_test.go

@@ -34,7 +34,6 @@ import (
 	"google.golang.org/grpc/internal/testutils"
 	"google.golang.org/grpc/internal/xds/matcher"
 	"google.golang.org/grpc/resolver"
-	"google.golang.org/grpc/xds/internal/balancer/outlierdetection"
 	"google.golang.org/grpc/xds/internal/testutils/fakeclient"
 	"google.golang.org/grpc/xds/internal/xdsclient/bootstrap"
 	"google.golang.org/grpc/xds/internal/xdsclient/xdsresource"
@@ -251,7 +250,7 @@ func (s) TestSecurityConfigWithoutXDSCreds(t *testing.T) {
 	// returned to the CDS balancer, because we have overridden the
 	// newChildBalancer function as part of test setup.
 	cdsUpdate := xdsresource.ClusterUpdate{ClusterName: serviceName}
-	wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
+	wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
 	ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout)
 	defer ctxCancel()
 	if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil {
@@ -307,7 +306,7 @@ func (s) TestNoSecurityConfigWithXDSCreds(t *testing.T) {
 	// newChildBalancer function as part of test setup. No security config is
 	// passed to the CDS balancer as part of this update.
 	cdsUpdate := xdsresource.ClusterUpdate{ClusterName: serviceName}
-	wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
+	wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
 	ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout)
 	defer ctxCancel()
 	if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil {
@@ -463,7 +462,7 @@ func (s) TestSecurityConfigUpdate_BadToGood(t *testing.T) {
 	// create a new EDS balancer. The fake EDS balancer created above will be
 	// returned to the CDS balancer, because we have overridden the
 	// newChildBalancer function as part of test setup.
-	wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
+	wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
 	if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdateWithGoodSecurityCfg, nil}, wantCCS, edsB); err != nil {
 		t.Fatal(err)
 	}
@@ -497,7 +496,7 @@ func (s) TestGoodSecurityConfig(t *testing.T) {
 	// create a new EDS balancer. The fake EDS balancer created above will be
 	// returned to the CDS balancer, because we have overridden the
 	// newChildBalancer function as part of test setup.
-	wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
+	wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
 	ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout)
 	defer ctxCancel()
 	if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdateWithGoodSecurityCfg, nil}, wantCCS, edsB); err != nil {
@@ -550,7 +549,7 @@ func (s) TestSecurityConfigUpdate_GoodToFallback(t *testing.T) {
 	// create a new EDS balancer. The fake EDS balancer created above will be
 	// returned to the CDS balancer, because we have overridden the
 	// newChildBalancer function as part of test setup.
-	wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
+	wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
 	ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout)
 	defer ctxCancel()
 	if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdateWithGoodSecurityCfg, nil}, wantCCS, edsB); err != nil {
@@ -600,7 +599,7 @@ func (s) TestSecurityConfigUpdate_GoodToBad(t *testing.T) {
 	// create a new EDS balancer. The fake EDS balancer created above will be
 	// returned to the CDS balancer, because we have overridden the
 	// newChildBalancer function as part of test setup.
-	wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
+	wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
 	ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout)
 	defer ctxCancel()
 	if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdateWithGoodSecurityCfg, nil}, wantCCS, edsB); err != nil {
@@ -678,7 +677,7 @@ func (s) TestSecurityConfigUpdate_GoodToGood(t *testing.T) {
 			SubjectAltNameMatchers: testSANMatchers,
 		},
 	}
-	wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
+	wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
 	ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout)
 	defer ctxCancel()
 	if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil {

xds/internal/balancer/cdsbalancer/cdsbalancer_test.go

@@ -29,7 +29,6 @@ import (
 	"google.golang.org/grpc/balancer"
 	"google.golang.org/grpc/connectivity"
 	"google.golang.org/grpc/internal"
-	"google.golang.org/grpc/internal/envconfig"
 	"google.golang.org/grpc/internal/grpctest"
 	internalserviceconfig "google.golang.org/grpc/internal/serviceconfig"
 	"google.golang.org/grpc/internal/testutils"
@@ -366,14 +365,11 @@ func (s) TestUpdateClientConnStateWithSameState(t *testing.T) {
 // different updates and verifies that the expect ClientConnState is propagated
 // to the edsBalancer.
 func (s) TestHandleClusterUpdate(t *testing.T) {
-	oldOutlierDetection := envconfig.XDSOutlierDetection
-	envconfig.XDSOutlierDetection = true
 	xdsC, cdsB, edsB, _, cancel := setupWithWatch(t)
 	xdsC.SetBootstrapConfig(&bootstrap.Config{
 		XDSServer: defaultTestAuthorityServerConfig,
 	})
 	defer func() {
-		envconfig.XDSOutlierDetection = oldOutlierDetection
 		cancel()
 		cdsB.Close()
 	}()
@@ -506,7 +502,7 @@ func (s) TestHandleClusterUpdateError(t *testing.T) {
 	// returned to the CDS balancer, because we have overridden the
 	// newChildBalancer function as part of test setup.
 	cdsUpdate := xdsresource.ClusterUpdate{ClusterName: serviceName}
-	wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
+	wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
 	if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil {
 		t.Fatal(err)
 	}
@@ -591,7 +587,7 @@ func (s) TestResolverError(t *testing.T) {
 	// returned to the CDS balancer, because we have overridden the
 	// newChildBalancer function as part of test setup.
 	cdsUpdate := xdsresource.ClusterUpdate{ClusterName: serviceName}
-	wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
+	wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
 	if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil {
 		t.Fatal(err)
 	}
@@ -640,7 +636,7 @@ func (s) TestUpdateSubConnState(t *testing.T) {
 	// returned to the CDS balancer, because we have overridden the
 	// newChildBalancer function as part of test setup.
 	cdsUpdate := xdsresource.ClusterUpdate{ClusterName: serviceName}
-	wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
+	wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
 	ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout)
 	defer ctxCancel()
 	if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil {
@@ -675,7 +671,7 @@ func (s) TestCircuitBreaking(t *testing.T) {
 	// the service's counter with the new max requests.
 	var maxRequests uint32 = 1
 	cdsUpdate := xdsresource.ClusterUpdate{ClusterName: clusterName, MaxRequests: &maxRequests}
-	wantCCS := edsCCS(clusterName, &maxRequests, false, nil, outlierdetection.LBConfig{})
+	wantCCS := edsCCS(clusterName, &maxRequests, false, nil, noopODLBCfg)
 	ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout)
 	defer ctxCancel()
 	if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil {
@@ -708,7 +704,7 @@ func (s) TestClose(t *testing.T) {
 	// returned to the CDS balancer, because we have overridden the
 	// newChildBalancer function as part of test setup.
 	cdsUpdate := xdsresource.ClusterUpdate{ClusterName: serviceName}
-	wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
+	wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
 	ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout)
 	defer ctxCancel()
 	if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil {
@@ -779,7 +775,7 @@ func (s) TestExitIdle(t *testing.T) {
 	// returned to the CDS balancer, because we have overridden the
 	// newChildBalancer function as part of test setup.
 	cdsUpdate := xdsresource.ClusterUpdate{ClusterName: serviceName}
-	wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
+	wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
 	ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout)
 	defer ctxCancel()
 	if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil {

xds/internal/balancer/clusterresolver/clusterresolver_test.go

@@ -30,8 +30,6 @@ import (
 	"google.golang.org/grpc/balancer/roundrobin"
 	"google.golang.org/grpc/balancer/weightedtarget"
 	"google.golang.org/grpc/connectivity"
-	"google.golang.org/grpc/internal"
-	"google.golang.org/grpc/internal/envconfig"
 	"google.golang.org/grpc/internal/grpctest"
 	internalserviceconfig "google.golang.org/grpc/internal/serviceconfig"
 	"google.golang.org/grpc/internal/testutils"
@@ -533,13 +531,6 @@ func newLBConfigWithOneEDSAndOutlierDetection(edsServiceName string, odCfg outli
 // Configuration sent downward should have a top level Outlier Detection Policy
 // for each priority.
 func (s) TestOutlierDetection(t *testing.T) {
-	oldOutlierDetection := envconfig.XDSOutlierDetection
-	envconfig.XDSOutlierDetection = true
-	internal.RegisterOutlierDetectionBalancerForTesting()
-	defer func() {
-		envconfig.XDSOutlierDetection = oldOutlierDetection
-	}()
-
 	edsLBCh := testutils.NewChannel()
 	xdsC, cleanup := setup(edsLBCh)
 	defer cleanup()