From 199a7ea3e828fc0fc4ce0de48666b222bb22218d Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Wed, 5 Feb 2025 10:37:22 -0800 Subject: [PATCH] xds: Improve XdsNR's selectConfig() variable handling The variables from the do-while are no longer initialized to let the compiler verify that the loop sets each. Unnecessary comparisons to null are also removed and is more obvious as the variables are never set to null. Added a minor optimization of computing the RPCs path once instead of once for each route. The variable declarations were also sorted to match their initialization order. This does fix an unlikely bug where if the old code could successfully matched a route but fail to retain the cluster, then when trying a second time if the route was _not_ matched it would re-use the prior route and thus infinite-loop failing to retain that same cluster. It also adds a missing cast to unsigned long for a uint32 weight. The old code would detect if the _sum_ was negative, but a weight using 32 bits would have been negative and never selected. --- .../main/java/io/grpc/xds/VirtualHost.java | 5 +-- .../java/io/grpc/xds/XdsNameResolver.java | 34 +++++++++++-------- .../grpc/xds/XdsRouteConfigureResource.java | 9 +++-- 3 files changed, 28 insertions(+), 20 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/VirtualHost.java b/xds/src/main/java/io/grpc/xds/VirtualHost.java index 7dfa0b34a3..5cc979984c 100644 --- a/xds/src/main/java/io/grpc/xds/VirtualHost.java +++ b/xds/src/main/java/io/grpc/xds/VirtualHost.java @@ -218,12 +218,13 @@ abstract class VirtualHost { abstract static class ClusterWeight { abstract String name(); - abstract int weight(); + abstract long weight(); abstract ImmutableMap filterConfigOverrides(); static ClusterWeight create( - String name, int weight, Map filterConfigOverrides) { + String name, long weight, Map filterConfigOverrides) { + checkArgument(weight >= 0, "weight must not be negative"); return new AutoValue_VirtualHost_Route_RouteAction_ClusterWeight( name, weight, ImmutableMap.copyOf(filterConfigOverrides)); } diff --git a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java index 2b4f206aef..21f5d5efce 100644 --- a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java +++ b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java @@ -384,17 +384,17 @@ final class XdsNameResolver extends NameResolver { private final class ConfigSelector extends InternalConfigSelector { @Override public Result selectConfig(PickSubchannelArgs args) { - String cluster = null; - ClientInterceptor filters = null; // null iff cluster is null - RouteData selectedRoute = null; RoutingConfig routingCfg; + RouteData selectedRoute; + String cluster; + ClientInterceptor filters; Metadata headers = args.getHeaders(); + String path = "/" + args.getMethodDescriptor().getFullMethodName(); do { routingCfg = routingConfig; + selectedRoute = null; for (RouteData route : routingCfg.routes) { - if (RoutingUtils.matchRoute( - route.routeMatch, "/" + args.getMethodDescriptor().getFullMethodName(), - headers, random)) { + if (RoutingUtils.matchRoute(route.routeMatch, path, headers, random)) { selectedRoute = route; break; } @@ -412,13 +412,14 @@ final class XdsNameResolver extends NameResolver { cluster = prefixedClusterName(action.cluster()); filters = selectedRoute.filterChoices.get(0); } else if (action.weightedClusters() != null) { + // XdsRouteConfigureResource verifies the total weight will not be 0 or exceed uint32 long totalWeight = 0; for (ClusterWeight weightedCluster : action.weightedClusters()) { totalWeight += weightedCluster.weight(); } long select = random.nextLong(totalWeight); long accumulator = 0; - for (int i = 0; i < action.weightedClusters().size(); i++) { + for (int i = 0; ; i++) { ClusterWeight weightedCluster = action.weightedClusters().get(i); accumulator += weightedCluster.weight(); if (select < accumulator) { @@ -431,13 +432,16 @@ final class XdsNameResolver extends NameResolver { cluster = prefixedClusterSpecifierPluginName(action.namedClusterSpecifierPluginConfig().name()); filters = selectedRoute.filterChoices.get(0); + } else { + // updateRoutes() discards routes with unknown actions + throw new AssertionError(); } } while (!retainCluster(cluster)); + + final RouteAction routeAction = selectedRoute.routeAction; Long timeoutNanos = null; if (enableTimeout) { - if (selectedRoute != null) { - timeoutNanos = selectedRoute.routeAction.timeoutNano(); - } + timeoutNanos = routeAction.timeoutNano(); if (timeoutNanos == null) { timeoutNanos = routingCfg.fallbackTimeoutNano; } @@ -445,8 +449,7 @@ final class XdsNameResolver extends NameResolver { timeoutNanos = null; } } - RetryPolicy retryPolicy = - selectedRoute == null ? null : selectedRoute.routeAction.retryPolicy(); + RetryPolicy retryPolicy = routeAction.retryPolicy(); // TODO(chengyuanzhang): avoid service config generation and parsing for each call. Map rawServiceConfig = generateServiceConfigWithMethodConfig(timeoutNanos, retryPolicy); @@ -459,8 +462,7 @@ final class XdsNameResolver extends NameResolver { "Failed to parse service config (method config)")); } final String finalCluster = cluster; - final long hash = generateHash(selectedRoute.routeAction.hashPolicies(), headers); - RouteData finalSelectedRoute = selectedRoute; + final long hash = generateHash(routeAction.hashPolicies(), headers); class ClusterSelectionInterceptor implements ClientInterceptor { @Override public ClientCall interceptCall( @@ -469,7 +471,7 @@ final class XdsNameResolver extends NameResolver { CallOptions callOptionsForCluster = callOptions.withOption(CLUSTER_SELECTION_KEY, finalCluster) .withOption(RPC_HASH_KEY, hash); - if (finalSelectedRoute.routeAction.autoHostRewrite()) { + if (routeAction.autoHostRewrite()) { callOptionsForCluster = callOptionsForCluster.withOption(AUTO_HOST_REWRITE_KEY, true); } return new SimpleForwardingClientCall( @@ -757,6 +759,8 @@ final class XdsNameResolver extends NameResolver { } ClientInterceptor filters = createFilters(filterConfigs, virtualHost, route, null); routesData.add(new RouteData(route.routeMatch(), route.routeAction(), filters)); + } else { + // Discard route } } diff --git a/xds/src/main/java/io/grpc/xds/XdsRouteConfigureResource.java b/xds/src/main/java/io/grpc/xds/XdsRouteConfigureResource.java index 587c7a437a..c5ca8d45cb 100644 --- a/xds/src/main/java/io/grpc/xds/XdsRouteConfigureResource.java +++ b/xds/src/main/java/io/grpc/xds/XdsRouteConfigureResource.java @@ -496,8 +496,9 @@ class XdsRouteConfigureResource extends XdsResourceType { return StructOrError.fromError("RouteAction contains invalid ClusterWeight: " + clusterWeightOrError.getErrorDetail()); } - clusterWeightSum += clusterWeight.getWeight().getValue(); - weightedClusters.add(clusterWeightOrError.getStruct()); + ClusterWeight parsedWeight = clusterWeightOrError.getStruct(); + clusterWeightSum += parsedWeight.weight(); + weightedClusters.add(parsedWeight); } if (clusterWeightSum <= 0) { return StructOrError.fromError("Sum of cluster weights should be above 0."); @@ -609,7 +610,9 @@ class XdsRouteConfigureResource extends XdsResourceType { + overrideConfigs.getErrorDetail()); } return StructOrError.fromStruct(VirtualHost.Route.RouteAction.ClusterWeight.create( - proto.getName(), proto.getWeight().getValue(), overrideConfigs.getStruct())); + proto.getName(), + Integer.toUnsignedLong(proto.getWeight().getValue()), + overrideConfigs.getStruct())); } @Nullable // null if the plugin is not supported, but it's marked as optional.