xds: allow duplicated route matcher and prefix='/'

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

@@ -421,6 +421,16 @@ final class EnvoyProtoData {
       return hasRegex;
     }
 
+    boolean isDefaultMatcher() {
+      if (hasRegex) {
+        return false;
+      }
+      if (!path.isEmpty()) {
+        return false;
+      }
+      return prefix.isEmpty() || prefix.equals("/");
+    }
+
     @Override
     public boolean equals(Object o) {
       if (this == o) {

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

@@ -916,8 +916,7 @@ final class XdsClientImpl extends XdsClient {
     if (!enablePathMatching) {
       EnvoyProtoData.Route route = routes.get(routes.size() - 1);
       RouteMatch routeMatch = route.getRouteMatch();
-      if (!routeMatch.getPath().isEmpty() || !routeMatch.getPrefix().isEmpty()
+      if (!routeMatch.isDefaultMatcher()) {
-          || routeMatch.hasRegex()) {
         return "The last route must be the default route";
       }
       if (route.getRouteAction() == null) {
@@ -934,50 +933,34 @@ final class XdsClientImpl extends XdsClient {
     // For now we consider the whole list invalid if anything invalid for grpc is found.
     // TODO(zdapeng): Fix it if the decision is different from current implementation.
     // TODO(zdapeng): Add test for validation.
-    Set<String> prefixMatches = new HashSet<>();
-    Set<String> pathMatches = new HashSet<>();
     for (int i = 0; i < routes.size(); i++) {
       EnvoyProtoData.Route route = routes.get(i);
-
+      RouteAction routeAction = route.getRouteAction();
-      if (route.getRouteAction() == null) {
+      if (routeAction == null) {
         return "Route action is not specified for one of the routes";
       }
-
       RouteMatch routeMatch = route.getRouteMatch();
+      if (!routeMatch.isDefaultMatcher()) {
         String prefix = routeMatch.getPrefix();
         String path = routeMatch.getPath();
         if (!prefix.isEmpty()) {
           if (!prefix.startsWith("/") || !prefix.endsWith("/") || prefix.length() < 3) {
             return "Prefix route match must be in the format of '/service/'";
           }
-        if (prefixMatches.contains(prefix)) {
-          return "Duplicate prefix match found";
-        }
-        prefixMatches.add(prefix);
         } else if (!path.isEmpty()) {
           int lastSlash = path.lastIndexOf('/');
           if (!path.startsWith("/") || lastSlash == 0 || lastSlash == path.length() - 1) {
             return "Path route match must be in the format of '/service/method'";
           }
-        if (pathMatches.contains(path)) {
-          return "Duplicate path match found";
-        }
-        pathMatches.add(path);
         } else if (routeMatch.hasRegex()) {
           return "Regex route match not supported";
-      } else { // Default route match
-        if (i != routes.size() - 1) {
-          return "Default route found but is not the last route in the route list";
         }
       }
-
       if (i == routes.size() - 1) {
-        if (!prefix.isEmpty() || !path.isEmpty()) {
+        if (!routeMatch.isDefaultMatcher()) {
           return "The last route must be the default route";
         }
       }
-
-      RouteAction routeAction = route.getRouteAction();
       if (routeAction.getCluster().isEmpty() && routeAction.getWeightedCluster().isEmpty()) {
         return "Either cluster or weighted cluster route action must be provided";
       }

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

@@ -226,6 +226,7 @@ final class XdsNameResolver extends NameResolver {
     for (Route route : routesUpdate) {
       String service = "";
       String method = "";
+      if (!route.getRouteMatch().isDefaultMatcher()) {
         String prefix = route.getRouteMatch().getPrefix();
         String path = route.getRouteMatch().getPath();
         if (!prefix.isEmpty()) {
@@ -235,6 +236,7 @@ final class XdsNameResolver extends NameResolver {
           service = path.substring(1, splitIndex);
           method = path.substring(splitIndex + 1);
         }
+      }
       Map<String, String> methodName = ImmutableMap.of("service", service, "method", method);
       String actionName;
       RouteAction routeAction = route.getRouteAction();

xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java

@@ -376,8 +376,9 @@ public class XdsNameResolverTest {
                         ImmutableMap.of(
                             "cluster-bar.googleapis.com", 30, "cluster-bar2.googleapis.com", 70)))
                 .build(),
-            // default, routed to cluster
+            // default with prefix = "/", routed to cluster
             Route.newBuilder()
+                .setMatch(RouteMatch.newBuilder().setPrefix("/"))
                 .setRoute(buildClusterRoute("cluster-hello.googleapis.com"))
                 .build());
     HttpConnectionManager httpConnectionManager =
@@ -477,6 +478,104 @@ public class XdsNameResolverTest {
             "cluster-foo.googleapis.com", 20, "cluster-bar.googleapis.com", 80));
   }
 
+  @Test
+  @SuppressWarnings("unchecked")
+  public void resolve_resourceUpdated_allowDuplicateMatchers() {
+    XdsClientImpl.enablePathMatching = true;
+    xdsNameResolver.start(mockListener);
+    assertThat(responseObservers).hasSize(1);
+    StreamObserver<DiscoveryResponse> responseObserver = responseObservers.poll();
+    // Simulate receiving another LDS response that tells client to do RDS.
+    String routeConfigName = "route-foo.googleapis.com";
+    responseObserver.onNext(
+        buildLdsResponseForRdsResource("1", AUTHORITY, routeConfigName, "0001"));
+
+    // Client sent an RDS request for resource "route-foo.googleapis.com" (Omitted in this test).
+    List<Route> protoRoutes =
+        ImmutableList.of(
+            // path match, routed to cluster
+            Route.newBuilder()
+                .setMatch(buildPathMatch("fooSvc", "hello"))
+                .setRoute(buildClusterRoute("cluster-hello.googleapis.com"))
+                .build(),
+            // prefix match, routed to cluster
+            Route.newBuilder()
+                .setMatch(buildPrefixMatch("fooSvc"))
+                .setRoute(buildClusterRoute("cluster-foo.googleapis.com"))
+                .build(),
+            // duplicate path match, routed to weighted clusters
+            Route.newBuilder()
+                .setMatch(buildPathMatch("fooSvc", "hello"))
+                .setRoute(buildWeightedClusterRoute(ImmutableMap.of(
+                    "cluster-hello.googleapis.com", 40,  "cluster-hello2.googleapis.com", 60)))
+                .build(),
+            // duplicage prefix match, routed to weighted clusters
+            Route.newBuilder()
+                .setMatch(buildPrefixMatch("fooSvc"))
+                .setRoute(
+                    buildWeightedClusterRoute(
+                        ImmutableMap.of(
+                            "cluster-bar.googleapis.com", 30, "cluster-bar2.googleapis.com", 70)))
+                .build(),
+            // default, routed to cluster
+            Route.newBuilder()
+                .setRoute(buildClusterRoute("cluster-hello.googleapis.com"))
+                .build());
+    List<Any> routeConfigs = ImmutableList.of(
+        Any.pack(
+            buildRouteConfiguration(
+                routeConfigName,
+                ImmutableList.of(buildVirtualHostForRoutes(AUTHORITY, protoRoutes)))));
+    responseObserver.onNext(
+        buildDiscoveryResponse("0", routeConfigs, XdsClientImpl.ADS_TYPE_URL_RDS, "0000"));
+
+    ArgumentCaptor<ResolutionResult> resolutionResultCaptor = ArgumentCaptor.forClass(null);
+    verify(mockListener).onResult(resolutionResultCaptor.capture());
+    ResolutionResult result = resolutionResultCaptor.getValue();
+    assertThat(result.getAddresses()).isEmpty();
+    Map<String, ?> serviceConfig = (Map<String, ?>) result.getServiceConfig().getConfig();
+
+    List<Map<String, ?>> rawLbConfigs =
+        (List<Map<String, ?>>) serviceConfig.get("loadBalancingConfig");
+    Map<String, ?> lbConfig = Iterables.getOnlyElement(rawLbConfigs);
+    assertThat(lbConfig.keySet()).containsExactly("xds_routing_experimental");
+    Map<String, ?> rawConfigValues = (Map<String, ?>) lbConfig.get("xds_routing_experimental");
+    assertThat(rawConfigValues.keySet()).containsExactly("action", "route");
+    Map<String, Map<String, ?>> actions =
+        (Map<String, Map<String, ?>>) rawConfigValues.get("action");
+    List<Map<String, ?>> routes = (List<Map<String, ?>>) rawConfigValues.get("route");
+    assertThat(routes).hasSize(5);
+    for (Map<String, ?> route : routes) {
+      assertThat(route.keySet()).containsExactly("methodName", "action");
+    }
+    assertThat((Map<String, ?>) routes.get(0).get("methodName"))
+        .containsExactly("service", "fooSvc", "method", "hello");
+    String action0 = (String) routes.get(0).get("action");
+    assertThat((Map<String, ?>) routes.get(1).get("methodName"))
+        .containsExactly("service", "fooSvc", "method", "");
+    String action1 = (String) routes.get(1).get("action");
+    assertThat((Map<String, ?>) routes.get(2).get("methodName"))
+        .containsExactly("service", "fooSvc", "method", "hello");
+    String action2 = (String) routes.get(2).get("action");
+    assertThat((Map<String, ?>) routes.get(3).get("methodName"))
+        .containsExactly("service", "fooSvc", "method", "");
+    String action3 = (String) routes.get(3).get("action");
+    assertThat((Map<String, ?>) routes.get(4).get("methodName"))
+        .containsExactly("service", "", "method", "");
+    String action4 = (String) routes.get(4).get("action");
+    assertCdsPolicy(actions.get(action0), "cluster-hello.googleapis.com");
+    assertCdsPolicy(actions.get(action1), "cluster-foo.googleapis.com");
+    assertWeightedTargetPolicy(
+        actions.get(action2),
+        ImmutableMap.of(
+            "cluster-hello.googleapis.com", 40,  "cluster-hello2.googleapis.com", 60));
+    assertWeightedTargetPolicy(
+        actions.get(action3),
+        ImmutableMap.of(
+            "cluster-bar.googleapis.com", 30, "cluster-bar2.googleapis.com", 70));
+    assertThat(action4).isEqualTo(action0);
+  }
+
   /** Asserts that the given action contains a single CDS policy with the given cluster name. */
   @SuppressWarnings("unchecked")
   private static void assertCdsPolicy(Map<String, ?> action, String clusterName) {