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.

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<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 @@ 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(
+          if (RoutingUtils.matchRoute(route.routeMatch, path, headers, random)) {
-                  route.routeMatch, "/" + args.getMethodDescriptor().getFullMethodName(),
-                  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 = routeAction.timeoutNano();
-          timeoutNanos = selectedRoute.routeAction.timeoutNano();
-        }
         if (timeoutNanos == null) {
           timeoutNanos = routingCfg.fallbackTimeoutNano;
         }
@@ -445,8 +449,7 @@ final class XdsNameResolver extends NameResolver {
           timeoutNanos = null;
         }
       }
-      RetryPolicy retryPolicy =
+      RetryPolicy retryPolicy = routeAction.retryPolicy();
-          selectedRoute == null ? null : selectedRoute.routeAction.retryPolicy();
       // TODO(chengyuanzhang): avoid service config generation and parsing for each call.
       Map<String, ?> 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);
+      final long hash = generateHash(routeAction.hashPolicies(), headers);
-      RouteData finalSelectedRoute = selectedRoute;
       class ClusterSelectionInterceptor implements ClientInterceptor {
         @Override
         public <ReqT, RespT> ClientCall<ReqT, RespT> 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<ReqT, RespT>(
@@ -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
         }
       }
 

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

@@ -496,8 +496,9 @@ class XdsRouteConfigureResource extends XdsResourceType<RdsUpdate> {
             return StructOrError.fromError("RouteAction contains invalid ClusterWeight: "
                 + clusterWeightOrError.getErrorDetail());
           }
-          clusterWeightSum += clusterWeight.getWeight().getValue();
+          ClusterWeight parsedWeight = clusterWeightOrError.getStruct();
-          weightedClusters.add(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<RdsUpdate> {
               + 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.