Skip to content

Commit

Permalink
envconfig: remove env vars for on-by-default features (#6749)
Browse files Browse the repository at this point in the history
  • Loading branch information
@dfawley
dfawley authored Oct 26, 2023
1 parent c76d75f commit 8190d88
Show file tree
Hide file tree
Showing 46 changed files with 77 additions and 814 deletions.
6 changes: 0 additions & 6 deletions internal/envconfig/envconfig.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,11 @@ import (
var (
// TXTErrIgnore is set if TXT errors should be ignored ("GRPC_GO_IGNORE_TXT_ERRORS" is not "false").
TXTErrIgnore = boolFromEnv("GRPC_GO_IGNORE_TXT_ERRORS", true)
// AdvertiseCompressors is set if registered compressor should be advertised
// ("GRPC_GO_ADVERTISE_COMPRESSORS" is not "false").
AdvertiseCompressors = boolFromEnv("GRPC_GO_ADVERTISE_COMPRESSORS", true)
// 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)
// PickFirstLBConfig is set if we should support configuration of the
// pick_first LB policy.
PickFirstLBConfig = boolFromEnv("GRPC_EXPERIMENTAL_PICKFIRST_LB_CONFIG", true)
// LeastRequestLB is set if we should support the least_request_experimental
// LB policy, which can be enabled by setting the environment variable
// "GRPC_EXPERIMENTAL_ENABLE_LEAST_REQUEST" to "true".
Expand Down
39 changes: 0 additions & 39 deletions internal/envconfig/xds.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,46 +50,7 @@ var (
//
// When both bootstrap FileName and FileContent are set, FileName is used.
XDSBootstrapFileContent = os.Getenv(XDSBootstrapFileContentEnv)
// XDSRingHash indicates whether ring hash support is enabled, which can be
// disabled by setting the environment variable
// "GRPC_XDS_EXPERIMENTAL_ENABLE_RING_HASH" to "false".
XDSRingHash = boolFromEnv("GRPC_XDS_EXPERIMENTAL_ENABLE_RING_HASH", true)
// XDSClientSideSecurity is used to control processing of security
// configuration on the client-side.
//
// Note that there is no env var protection for the server-side because we
// have a brand new API on the server-side and users explicitly need to use
// the new API to get security integration on the server.
XDSClientSideSecurity = boolFromEnv("GRPC_XDS_EXPERIMENTAL_SECURITY_SUPPORT", true)
// XDSAggregateAndDNS indicates whether processing of aggregated cluster and
// DNS cluster is enabled, which can be disabled by setting the environment
// variable "GRPC_XDS_EXPERIMENTAL_ENABLE_AGGREGATE_AND_LOGICAL_DNS_CLUSTER"
// to "false".
XDSAggregateAndDNS = boolFromEnv("GRPC_XDS_EXPERIMENTAL_ENABLE_AGGREGATE_AND_LOGICAL_DNS_CLUSTER", true)

// XDSRBAC indicates whether xDS configured RBAC HTTP Filter is enabled,
// which can be disabled by setting the environment variable
// "GRPC_XDS_EXPERIMENTAL_RBAC" to "false".
XDSRBAC = boolFromEnv("GRPC_XDS_EXPERIMENTAL_RBAC", true)
// XDSOutlierDetection indicates whether outlier detection support is
// enabled, which can be disabled by setting the environment variable
// "GRPC_EXPERIMENTAL_ENABLE_OUTLIER_DETECTION" to "false".
XDSOutlierDetection = boolFromEnv("GRPC_EXPERIMENTAL_ENABLE_OUTLIER_DETECTION", true)
// XDSFederation indicates whether federation support is enabled, which can
// be enabled by setting the environment variable
// "GRPC_EXPERIMENTAL_XDS_FEDERATION" to "true".
XDSFederation = boolFromEnv("GRPC_EXPERIMENTAL_XDS_FEDERATION", true)

// XDSRLS indicates whether processing of Cluster Specifier plugins and
// support for the RLS CLuster Specifier is enabled, which can be disabled by
// setting the environment variable "GRPC_EXPERIMENTAL_XDS_RLS_LB" to
// "false".
XDSRLS = boolFromEnv("GRPC_EXPERIMENTAL_XDS_RLS_LB", true)

// C2PResolverTestOnlyTrafficDirectorURI is the TD URI for testing.
C2PResolverTestOnlyTrafficDirectorURI = os.Getenv("GRPC_TEST_ONLY_GOOGLE_C2P_RESOLVER_TRAFFIC_DIRECTOR_URI")
// XDSCustomLBPolicy indicates whether Custom LB Policies are enabled, which
// can be disabled by setting the environment variable
// "GRPC_EXPERIMENTAL_XDS_CUSTOM_LB_CONFIG" to "false".
XDSCustomLBPolicy = boolFromEnv("GRPC_EXPERIMENTAL_XDS_CUSTOM_LB_CONFIG", true)
)
5 changes: 0 additions & 5 deletions internal/grpcutil/compressor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ package grpcutil

import (
"strings"

"google.golang.org/grpc/internal/envconfig"
)

// RegisteredCompressorNames holds names of the registered compressors.
Expand All @@ -40,8 +38,5 @@ func IsCompressorNameRegistered(name string) bool {
// RegisteredCompressors returns a string of registered compressor names
// separated by comma.
func RegisteredCompressors() string {
if !envconfig.AdvertiseCompressors {
return ""
}
return strings.Join(RegisteredCompressorNames, ",")
}
20 changes: 3 additions & 17 deletions internal/grpcutil/compressor_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,27 +20,13 @@ package grpcutil

import (
"testing"

"google.golang.org/grpc/internal/envconfig"
)

func TestRegisteredCompressors(t *testing.T) {
defer func(c []string) { RegisteredCompressorNames = c }(RegisteredCompressorNames)
defer func(v bool) { envconfig.AdvertiseCompressors = v }(envconfig.AdvertiseCompressors)
RegisteredCompressorNames = []string{"gzip", "snappy"}
tests := []struct {
desc string
enabled bool
want string
}{
{desc: "compressor_ad_disabled", enabled: false, want: ""},
{desc: "compressor_ad_enabled", enabled: true, want: "gzip,snappy"},
}
for _, tt := range tests {
envconfig.AdvertiseCompressors = tt.enabled
compressors := RegisteredCompressors()
if compressors != tt.want {
t.Fatalf("Unexpected compressors got:%s, want:%s", compressors, tt.want)
}
if got, want := RegisteredCompressors(), "gzip,snappy"; got != want {
t.Fatalf("Unexpected compressors got:%s, want:%s", got, want)

}
}
9 changes: 3 additions & 6 deletions internal/testutils/xds/bootstrap/bootstrap.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,12 +116,9 @@ func Contents(opts Options) ([]byte, error) {
cfg.XdsServers[0].ServerFeatures = append(cfg.XdsServers[0].ServerFeatures, "ignore_resource_deletion")
}

auths := make(map[string]authority)
if envconfig.XDSFederation {
// This will end up using the top-level server list for new style
// resources with empty authority.
auths[""] = authority{}
}
// This will end up using the top-level server list for new style
// resources with empty authority.
auths := map[string]authority{"": {}}
for n, auURI := range opts.Authorities {
auths[n] = authority{XdsServers: []server{{
ServerURI: auURI,
Expand Down
14 changes: 0 additions & 14 deletions pickfirst.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (

"google.golang.org/grpc/balancer"
"google.golang.org/grpc/connectivity"
"google.golang.org/grpc/internal/envconfig"
internalgrpclog "google.golang.org/grpc/internal/grpclog"
"google.golang.org/grpc/internal/grpcrand"
"google.golang.org/grpc/internal/pretty"
Expand Down Expand Up @@ -65,19 +64,6 @@ type pfConfig struct {
}

func (*pickfirstBuilder) ParseConfig(js json.RawMessage) (serviceconfig.LoadBalancingConfig, error) {
if !envconfig.PickFirstLBConfig {
// Prior to supporting loadbalancing configuration, the pick_first LB
// policy did not implement the balancer.ConfigParser interface. This
// meant that if a non-empty configuration was passed to it, the service
// config unmarshaling code would throw a warning log, but would
// continue using the pick_first LB policy. The code below ensures the
// same behavior is retained if the env var is not set.
if string(js) != "{}" {
logger.Warningf("Ignoring non-empty balancer configuration %q for the pick_first LB policy", string(js))
}
return nil, nil
}

var cfg pfConfig
if err := json.Unmarshal(js, &cfg); err != nil {
return nil, fmt.Errorf("pickfirst: unable to unmarshal LB policy config: %s, error: %v", string(js), err)
Expand Down
18 changes: 0 additions & 18 deletions test/compressor_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import (
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/encoding"
"google.golang.org/grpc/internal/envconfig"
"google.golang.org/grpc/internal/stubserver"
"google.golang.org/grpc/metadata"
"google.golang.org/grpc/status"
Expand Down Expand Up @@ -408,23 +407,6 @@ func (s) TestUnregisteredSetSendCompressorFailure(t *testing.T) {
})
}

func (s) TestUnadvertisedSetSendCompressorFailure(t *testing.T) {
// Disable client compressor advertisement.
defer func(b bool) { envconfig.AdvertiseCompressors = b }(envconfig.AdvertiseCompressors)
envconfig.AdvertiseCompressors = false

resCompressor := "gzip"
wantErr := status.Error(codes.Unknown, "unable to set send compressor: client does not support compressor \"gzip\"")

t.Run("unary", func(t *testing.T) {
testUnarySetSendCompressorFailure(t, resCompressor, wantErr)
})

t.Run("stream", func(t *testing.T) {
testStreamSetSendCompressorFailure(t, resCompressor, wantErr)
})
}

func testUnarySetSendCompressorFailure(t *testing.T, resCompressor string, wantErr error) {
ss := &stubserver.StubServer{
EmptyCallF: func(ctx context.Context, in *testpb.Empty) (*testpb.Empty, error) {
Expand Down
76 changes: 2 additions & 74 deletions test/pickfirst_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import (
"google.golang.org/grpc/credentials/insecure"
"google.golang.org/grpc/internal"
"google.golang.org/grpc/internal/channelz"
"google.golang.org/grpc/internal/envconfig"
"google.golang.org/grpc/internal/grpcrand"
"google.golang.org/grpc/internal/stubserver"
"google.golang.org/grpc/internal/testutils"
Expand Down Expand Up @@ -376,8 +375,6 @@ func (s) TestPickFirst_StickyTransientFailure(t *testing.T) {

// Tests the PF LB policy with shuffling enabled.
func (s) TestPickFirst_ShuffleAddressList(t *testing.T) {
defer func(old bool) { envconfig.PickFirstLBConfig = old }(envconfig.PickFirstLBConfig)
envconfig.PickFirstLBConfig = true
const serviceConfig = `{"loadBalancingConfig": [{"pick_first":{ "shuffleAddressList": true }}]}`

// Install a shuffler that always reverses two entries.
Expand Down Expand Up @@ -429,45 +426,6 @@ func (s) TestPickFirst_ShuffleAddressList(t *testing.T) {
}
}

// Tests the PF LB policy with the environment variable support of address list
// shuffling disabled.
func (s) TestPickFirst_ShuffleAddressListDisabled(t *testing.T) {
defer func(old bool) { envconfig.PickFirstLBConfig = old }(envconfig.PickFirstLBConfig)
envconfig.PickFirstLBConfig = false
const serviceConfig = `{"loadBalancingConfig": [{"pick_first":{ "shuffleAddressList": true }}]}`

// Install a shuffler that always reverses two entries.
origShuf := grpcrand.Shuffle
defer func() { grpcrand.Shuffle = origShuf }()
grpcrand.Shuffle = func(n int, f func(int, int)) {
if n != 2 {
t.Errorf("Shuffle called with n=%v; want 2", n)
return
}
f(0, 1) // reverse the two addresses
}

// Set up our backends.
cc, r, backends := setupPickFirst(t, 2)
addrs := stubBackendsToResolverAddrs(backends)

ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()

// Send a config with shuffling enabled. This will reverse the addresses,
// so we should connect to backend 1 if shuffling is supported. However
// with it disabled at the start of the test, we will connect to backend 0
// instead.
shufState := resolver.State{
ServiceConfig: parseServiceConfig(t, r, serviceConfig),
Addresses: []resolver.Address{addrs[0], addrs[1]},
}
r.UpdateState(shufState)
if err := pickfirst.CheckRPCsToBackend(ctx, cc, addrs[0]); err != nil {
t.Fatal(err)
}
}

// Test config parsing with the env var turned on and off for various scenarios.
func (s) TestPickFirst_ParseConfig_Success(t *testing.T) {
// Install a shuffler that always reverses two entries.
Expand All @@ -483,49 +441,23 @@ func (s) TestPickFirst_ParseConfig_Success(t *testing.T) {

tests := []struct {
name string
envVar bool
serviceConfig string
wantFirstAddr bool
}{
{
name: "env var disabled with empty pickfirst config",
envVar: false,
serviceConfig: `{"loadBalancingConfig": [{"pick_first":{}}]}`,
wantFirstAddr: true,
},
{
name: "env var disabled with non-empty good pickfirst config",
envVar: false,
serviceConfig: `{"loadBalancingConfig": [{"pick_first":{ "shuffleAddressList": true }}]}`,
wantFirstAddr: true,
},
{
name: "env var disabled with non-empty bad pickfirst config",
envVar: false,
serviceConfig: `{"loadBalancingConfig": [{"pick_first":{ "shuffleAddressList": 666 }}]}`,
wantFirstAddr: true,
},
{
name: "env var enabled with empty pickfirst config",
envVar: true,
name: "empty pickfirst config",
serviceConfig: `{"loadBalancingConfig": [{"pick_first":{}}]}`,
wantFirstAddr: true,
},
{
name: "env var enabled with empty good pickfirst config",
envVar: true,
name: "empty good pickfirst config",
serviceConfig: `{"loadBalancingConfig": [{"pick_first":{ "shuffleAddressList": true }}]}`,
wantFirstAddr: false,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
// Set the env var as specified by the test table.
origPickFirstLBConfig := envconfig.PickFirstLBConfig
envconfig.PickFirstLBConfig = test.envVar
defer func() { envconfig.PickFirstLBConfig = origPickFirstLBConfig }()

// Set up our backends.
cc, r, backends := setupPickFirst(t, 2)
addrs := stubBackendsToResolverAddrs(backends)
Expand Down Expand Up @@ -555,10 +487,6 @@ func (s) TestPickFirst_ParseConfig_Success(t *testing.T) {

// Test config parsing for a bad service config.
func (s) TestPickFirst_ParseConfig_Failure(t *testing.T) {
origPickFirstLBConfig := envconfig.PickFirstLBConfig
envconfig.PickFirstLBConfig = true
defer func() { envconfig.PickFirstLBConfig = origPickFirstLBConfig }()

// Service config should fail with the below config. Name resolvers are
// expected to perform this parsing before they push the parsed service
// config to the channel.
Expand Down
7 changes: 0 additions & 7 deletions test/xds/xds_client_affinity_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (

"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"
"google.golang.org/grpc/internal/envconfig"
"google.golang.org/grpc/internal/stubserver"
"google.golang.org/grpc/internal/testutils"
"google.golang.org/grpc/internal/testutils/xds/e2e"
Expand Down Expand Up @@ -84,12 +83,6 @@ func ringhashCluster(clusterName, edsServiceName string) *v3clusterpb.Cluster {
// propagated to pick the ring_hash policy. It doesn't test the affinity
// behavior in ring_hash policy.
func (s) TestClientSideAffinitySanityCheck(t *testing.T) {
defer func() func() {
old := envconfig.XDSRingHash
envconfig.XDSRingHash = true
return func() { envconfig.XDSRingHash = old }
}()()

managementServer, nodeID, _, resolver, cleanup1 := e2e.SetupManagementServer(t, e2e.ManagementServerOptions{})
defer cleanup1()

Expand Down
5 changes: 0 additions & 5 deletions test/xds/xds_client_custom_lb_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,11 +93,6 @@ func clusterWithLBConfiguration(t *testing.T, clusterName, edsServiceName string
// first) child load balancing policy, and asserts the correct distribution
// based on the locality weights and the endpoint picking policy specified.
func (s) TestWrrLocality(t *testing.T) {
oldCustomLBSupport := envconfig.XDSCustomLBPolicy
envconfig.XDSCustomLBPolicy = true
defer func() {
envconfig.XDSCustomLBPolicy = oldCustomLBSupport
}()
oldLeastRequestLBSupport := envconfig.LeastRequestLB
envconfig.LeastRequestLB = true
defer func() {
Expand Down
13 changes: 0 additions & 13 deletions test/xds/xds_client_federation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import (
"google.golang.org/grpc/codes"
"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"
"google.golang.org/grpc/internal/testutils/xds/bootstrap"
Expand All @@ -54,10 +53,6 @@ import (
// - CDS: old style, no authority (default authority)
// - EDS: new style, in a different authority
func (s) TestClientSideFederation(t *testing.T) {
oldXDSFederation := envconfig.XDSFederation
envconfig.XDSFederation = true
defer func() { envconfig.XDSFederation = oldXDSFederation }()

// Start a management server as the default authority.
serverDefaultAuth, err := e2e.StartManagementServer(e2e.ManagementServerOptions{})
if err != nil {
Expand Down Expand Up @@ -150,10 +145,6 @@ func (s) TestClientSideFederation(t *testing.T) {
// in the bootstrap configuration. The test verifies that RPCs on the ClientConn
// fail with an appropriate error.
func (s) TestFederation_UnknownAuthorityInDialTarget(t *testing.T) {
oldXDSFederation := envconfig.XDSFederation
envconfig.XDSFederation = true
defer func() { envconfig.XDSFederation = oldXDSFederation }()

// Setting up the management server is not *really* required for this test
// case. All we need is a bootstrap configuration which does not contain the
// authority mentioned in the dial target. But setting up the management
Expand Down Expand Up @@ -209,10 +200,6 @@ func (s) TestFederation_UnknownAuthorityInDialTarget(t *testing.T) {
// with an authority which is not specified in the bootstrap configuration. The
// test verifies that RPCs fail with an appropriate error.
func (s) TestFederation_UnknownAuthorityInReceivedResponse(t *testing.T) {
oldXDSFederation := envconfig.XDSFederation
envconfig.XDSFederation = true
defer func() { envconfig.XDSFederation = oldXDSFederation }()

mgmtServer, err := e2e.StartManagementServer(e2e.ManagementServerOptions{})
if err != nil {
t.Fatalf("Failed to spin up the xDS management server: %v", err)
Expand Down
Loading

0 comments on commit 8190d88

Please sign in to comment.