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.

Author: ejona86

xds/src/main/java/io/grpc/xds/VirtualHost.java

@@ -218,12 +218,13 @@ private static RouteAction create(
       abstract static class ClusterWeight {
         abstract String name();
 
-        abstract int weight();
+        abstract long weight();
 
         abstract ImmutableMap<String, FilterConfig> filterConfigOverrides();
 
         static ClusterWeight create(
-            String name, int weight, Map<String, FilterConfig> filterConfigOverrides) {
+            String name, long weight, Map<String, FilterConfig> filterConfigOverrides) {
+          checkArgument(weight >= 0, "weight must not be negative");
           return new AutoValue_VirtualHost_Route_RouteAction_ClusterWeight(
               name, weight, ImmutableMap.copyOf(filterConfigOverrides));
         }

xds/src/main/java/io/grpc/xds/XdsNameResolver.java

@@ -384,17 +384,17 @@ static boolean matchHostName(String hostName, String pattern) {
   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 @@ public Result selectConfig(PickSubchannelArgs args) {
           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,22 +432,24 @@ public Result selectConfig(PickSubchannelArgs args) {
           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;
         }
         if (timeoutNanos <= 0) {
           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<String, ?> rawServiceConfig =
           generateServiceConfigWithMethodConfig(timeoutNanos, retryPolicy);
@@ -459,8 +462,7 @@ public Result selectConfig(PickSubchannelArgs args) {
                 "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 <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(
@@ -469,7 +471,7 @@ public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(
           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<ReqT, RespT>(
@@ -757,6 +759,8 @@ private void updateRoutes(List<VirtualHost> virtualHosts, long httpMaxStreamDura
           }
           ClientInterceptor filters = createFilters(filterConfigs, virtualHost, route, null);
           routesData.add(new RouteData(route.routeMatch(), route.routeAction(), filters));
+        } else {
+          // Discard route
         }
       }
 

xds/src/main/java/io/grpc/xds/XdsRouteConfigureResource.java

@@ -496,8 +496,9 @@ static StructOrError<RouteAction> parseRouteAction(
             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 @@ static StructOrError<VirtualHost.Route.RouteAction.ClusterWeight> parseClusterWe
               + 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.