From 42b2cbdec311e62a07eedd07e5294ee19cb506f9 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Tue, 5 Mar 2024 16:01:14 -0800 Subject: [PATCH] xds: Move LR and RLS experimental flags to where they are used That's better for code organization and also removes some grpc-isms from XdsResourceType. --- .../java/io/grpc/xds/XdsClusterResource.java | 7 ++++++ .../grpc/xds/XdsRouteConfigureResource.java | 4 ++++ .../io/grpc/xds/client/XdsResourceType.java | 17 -------------- .../grpc/xds/GrpcXdsClientImplDataTest.java | 22 +++++++++---------- .../grpc/xds/GrpcXdsClientImplTestBase.java | 6 ++--- 5 files changed, 25 insertions(+), 31 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/XdsClusterResource.java b/xds/src/main/java/io/grpc/xds/XdsClusterResource.java index 1a7ce08fa5..2bc473a566 100644 --- a/xds/src/main/java/io/grpc/xds/XdsClusterResource.java +++ b/xds/src/main/java/io/grpc/xds/XdsClusterResource.java @@ -22,6 +22,7 @@ import static io.grpc.xds.client.Bootstrapper.ServerInfo; import com.google.auto.value.AutoValue; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.MoreObjects; +import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.protobuf.Duration; @@ -50,6 +51,12 @@ import java.util.Set; import javax.annotation.Nullable; class XdsClusterResource extends XdsResourceType { + @VisibleForTesting + static boolean enableLeastRequest = + !Strings.isNullOrEmpty(System.getenv("GRPC_EXPERIMENTAL_ENABLE_LEAST_REQUEST")) + ? Boolean.parseBoolean(System.getenv("GRPC_EXPERIMENTAL_ENABLE_LEAST_REQUEST")) + : Boolean.parseBoolean(System.getProperty("io.grpc.xds.experimentalEnableLeastRequest")); + @VisibleForTesting static final String AGGREGATE_CLUSTER_TYPE_NAME = "envoy.clusters.aggregate"; static final String ADS_TYPE_URL_CDS = diff --git a/xds/src/main/java/io/grpc/xds/XdsRouteConfigureResource.java b/xds/src/main/java/io/grpc/xds/XdsRouteConfigureResource.java index 22e6533439..0a3d1406da 100644 --- a/xds/src/main/java/io/grpc/xds/XdsRouteConfigureResource.java +++ b/xds/src/main/java/io/grpc/xds/XdsRouteConfigureResource.java @@ -38,6 +38,7 @@ import io.envoyproxy.envoy.config.route.v3.RetryPolicy.RetryBackOff; import io.envoyproxy.envoy.config.route.v3.RouteConfiguration; import io.envoyproxy.envoy.type.v3.FractionalPercent; import io.grpc.Status; +import io.grpc.internal.GrpcUtil; import io.grpc.xds.ClusterSpecifierPlugin.NamedPluginConfig; import io.grpc.xds.ClusterSpecifierPlugin.PluginConfig; import io.grpc.xds.Filter.FilterConfig; @@ -67,6 +68,9 @@ import java.util.Set; import javax.annotation.Nullable; class XdsRouteConfigureResource extends XdsResourceType { + @VisibleForTesting + static boolean enableRouteLookup = GrpcUtil.getFlag("GRPC_EXPERIMENTAL_XDS_RLS_LB", true); + static final String ADS_TYPE_URL_RDS = "type.googleapis.com/envoy.config.route.v3.RouteConfiguration"; private static final String TYPE_URL_FILTER_CONFIG = diff --git a/xds/src/main/java/io/grpc/xds/client/XdsResourceType.java b/xds/src/main/java/io/grpc/xds/client/XdsResourceType.java index eefc62de8f..8c3d31604e 100644 --- a/xds/src/main/java/io/grpc/xds/client/XdsResourceType.java +++ b/xds/src/main/java/io/grpc/xds/client/XdsResourceType.java @@ -21,7 +21,6 @@ import static io.grpc.xds.client.XdsClient.canonifyResourceName; import static io.grpc.xds.client.XdsClient.isResourceNameValid; import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Strings; import com.google.protobuf.Any; import com.google.protobuf.InvalidProtocolBufferException; import com.google.protobuf.Message; @@ -44,13 +43,6 @@ public abstract class XdsResourceType { protected static final String TRANSPORT_SOCKET_NAME_TLS = "envoy.transport_sockets.tls"; @VisibleForTesting public static final String HASH_POLICY_FILTER_STATE_KEY = "io.grpc.channel_id"; - @VisibleForTesting - public static boolean enableRouteLookup = getFlag("GRPC_EXPERIMENTAL_XDS_RLS_LB", true); - @VisibleForTesting - public static boolean enableLeastRequest = - !Strings.isNullOrEmpty(System.getenv("GRPC_EXPERIMENTAL_ENABLE_LEAST_REQUEST")) - ? Boolean.parseBoolean(System.getenv("GRPC_EXPERIMENTAL_ENABLE_LEAST_REQUEST")) - : Boolean.parseBoolean(System.getProperty("io.grpc.xds.experimentalEnableLeastRequest")); protected static final String TYPE_URL_CLUSTER_CONFIG = "type.googleapis.com/envoy.extensions.clusters.aggregate.v3.ClusterConfig"; @@ -258,15 +250,6 @@ public abstract class XdsResourceType { } } - private static boolean getFlag(String envVarName, boolean enableByDefault) { - String envVar = System.getenv(envVarName); - if (enableByDefault) { - return Strings.isNullOrEmpty(envVar) || Boolean.parseBoolean(envVar); - } else { - return !Strings.isNullOrEmpty(envVar) && Boolean.parseBoolean(envVar); - } - } - @VisibleForTesting public static final class StructOrError { diff --git a/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplDataTest.java b/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplDataTest.java index 753be1a9e6..13fc435f50 100644 --- a/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplDataTest.java +++ b/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplDataTest.java @@ -173,15 +173,15 @@ public class GrpcXdsClientImplDataTest { @Before public void setUp() { - originalEnableRouteLookup = XdsResourceType.enableRouteLookup; - originalEnableLeastRequest = XdsResourceType.enableLeastRequest; + originalEnableRouteLookup = XdsRouteConfigureResource.enableRouteLookup; + originalEnableLeastRequest = XdsClusterResource.enableLeastRequest; assertThat(originalEnableLeastRequest).isFalse(); } @After public void tearDown() { - XdsResourceType.enableRouteLookup = originalEnableRouteLookup; - XdsResourceType.enableLeastRequest = originalEnableLeastRequest; + XdsRouteConfigureResource.enableRouteLookup = originalEnableRouteLookup; + XdsClusterResource.enableLeastRequest = originalEnableLeastRequest; } @Test @@ -860,7 +860,7 @@ public class GrpcXdsClientImplDataTest { @Test public void parseRouteAction_clusterSpecifier_routeLookupDisabled() { - XdsResourceType.enableRouteLookup = false; + XdsRouteConfigureResource.enableRouteLookup = false; io.envoyproxy.envoy.config.route.v3.RouteAction proto = io.envoyproxy.envoy.config.route.v3.RouteAction.newBuilder() .setClusterSpecifierPlugin(CLUSTER_SPECIFIER_PLUGIN.name()) @@ -1465,7 +1465,7 @@ public class GrpcXdsClientImplDataTest { @Test public void parseHttpConnectionManager_clusterSpecifierPlugin() throws Exception { - XdsResourceType.enableRouteLookup = true; + XdsRouteConfigureResource.enableRouteLookup = true; RouteLookupConfig routeLookupConfig = RouteLookupConfig.newBuilder() .addGrpcKeybuilders( GrpcKeyBuilder.newBuilder() @@ -1521,7 +1521,7 @@ public class GrpcXdsClientImplDataTest { @Test public void parseHttpConnectionManager_duplicatePluginName() throws Exception { - XdsResourceType.enableRouteLookup = true; + XdsRouteConfigureResource.enableRouteLookup = true; RouteLookupConfig routeLookupConfig1 = RouteLookupConfig.newBuilder() .addGrpcKeybuilders( GrpcKeyBuilder.newBuilder() @@ -1594,7 +1594,7 @@ public class GrpcXdsClientImplDataTest { @Test public void parseHttpConnectionManager_pluginNameNotFound() throws Exception { - XdsResourceType.enableRouteLookup = true; + XdsRouteConfigureResource.enableRouteLookup = true; RouteLookupConfig routeLookupConfig = RouteLookupConfig.newBuilder() .addGrpcKeybuilders( GrpcKeyBuilder.newBuilder() @@ -1647,7 +1647,7 @@ public class GrpcXdsClientImplDataTest { @Test public void parseHttpConnectionManager_optionalPlugin() throws ResourceInvalidException { - XdsResourceType.enableRouteLookup = true; + XdsRouteConfigureResource.enableRouteLookup = true; // RLS Plugin, and a route to it. RouteLookupConfig routeLookupConfig = RouteLookupConfig.newBuilder() @@ -1729,7 +1729,7 @@ public class GrpcXdsClientImplDataTest { @Test public void parseHttpConnectionManager_validateRdsConfigSource() throws Exception { - XdsResourceType.enableRouteLookup = true; + XdsRouteConfigureResource.enableRouteLookup = true; HttpConnectionManager hcm1 = HttpConnectionManager.newBuilder() @@ -1967,7 +1967,7 @@ public class GrpcXdsClientImplDataTest { @Test public void parseCluster_leastRequestLbPolicy_defaultLbConfig() throws ResourceInvalidException { - XdsResourceType.enableLeastRequest = true; + XdsClusterResource.enableLeastRequest = true; Cluster cluster = Cluster.newBuilder() .setName("cluster-foo.googleapis.com") .setType(DiscoveryType.EDS) diff --git a/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplTestBase.java b/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplTestBase.java index 3cd0115aeb..fd276a849c 100644 --- a/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplTestBase.java +++ b/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplTestBase.java @@ -304,8 +304,8 @@ public abstract class GrpcXdsClientImplTestBase { when(backoffPolicy2.nextBackoffNanos()).thenReturn(20L, 200L); // Start the server and the client. - originalEnableLeastRequest = XdsResourceType.enableLeastRequest; - XdsResourceType.enableLeastRequest = true; + originalEnableLeastRequest = XdsClusterResource.enableLeastRequest; + XdsClusterResource.enableLeastRequest = true; xdsServer = cleanupRule.register(InProcessServerBuilder .forName(serverName) .addService(adsService) @@ -376,7 +376,7 @@ public abstract class GrpcXdsClientImplTestBase { @After public void tearDown() { - XdsResourceType.enableLeastRequest = originalEnableLeastRequest; + XdsClusterResource.enableLeastRequest = originalEnableLeastRequest; xdsClient.shutdown(); channel.shutdown(); // channel not owned by XdsClient assertThat(adsEnded.get()).isTrue();