From 108340857fb205c9a626f1675f23dd262e8a7b5c Mon Sep 17 00:00:00 2001 From: ZHANG Dapeng Date: Thu, 16 Apr 2020 13:50:26 -0700 Subject: [PATCH] xds: allow duplicated route matcher and prefix='/' --- .../main/java/io/grpc/xds/EnvoyProtoData.java | 10 ++ .../main/java/io/grpc/xds/XdsClientImpl.java | 53 ++++----- .../java/io/grpc/xds/XdsNameResolver.java | 18 ++-- .../java/io/grpc/xds/XdsNameResolverTest.java | 101 +++++++++++++++++- 4 files changed, 138 insertions(+), 44 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/EnvoyProtoData.java b/xds/src/main/java/io/grpc/xds/EnvoyProtoData.java index 8afbe433b7f1..d7f1c89f5418 100644 --- a/xds/src/main/java/io/grpc/xds/EnvoyProtoData.java +++ b/xds/src/main/java/io/grpc/xds/EnvoyProtoData.java @@ -421,6 +421,16 @@ boolean hasRegex() { 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) { diff --git a/xds/src/main/java/io/grpc/xds/XdsClientImpl.java b/xds/src/main/java/io/grpc/xds/XdsClientImpl.java index 5498a0c81b7a..96dbc9b2d2bf 100644 --- a/xds/src/main/java/io/grpc/xds/XdsClientImpl.java +++ b/xds/src/main/java/io/grpc/xds/XdsClientImpl.java @@ -916,8 +916,7 @@ private static String validateRoutes(List routes) { if (!enablePathMatching) { EnvoyProtoData.Route route = routes.get(routes.size() - 1); RouteMatch routeMatch = route.getRouteMatch(); - if (!routeMatch.getPath().isEmpty() || !routeMatch.getPrefix().isEmpty() - || routeMatch.hasRegex()) { + if (!routeMatch.isDefaultMatcher()) { return "The last route must be the default route"; } if (route.getRouteAction() == null) { @@ -934,50 +933,34 @@ private static String validateRoutes(List routes) { // 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 prefixMatches = new HashSet<>(); - Set pathMatches = new HashSet<>(); for (int i = 0; i < routes.size(); i++) { EnvoyProtoData.Route route = routes.get(i); - - if (route.getRouteAction() == null) { + RouteAction routeAction = route.getRouteAction(); + if (routeAction == null) { return "Route action is not specified for one of the routes"; } - RouteMatch routeMatch = route.getRouteMatch(); - 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 (!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/'"; + } + } 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'"; + } + } else if (routeMatch.hasRegex()) { + return "Regex route match not supported"; } } - 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"; } diff --git a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java index 7f9d2a6cd9b8..c503182197a8 100644 --- a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java +++ b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java @@ -226,14 +226,16 @@ public void onError(Status error) { for (Route route : routesUpdate) { String service = ""; String method = ""; - String prefix = route.getRouteMatch().getPrefix(); - String path = route.getRouteMatch().getPath(); - if (!prefix.isEmpty()) { - service = prefix.substring(1, prefix.length() - 1); - } else if (!path.isEmpty()) { - int splitIndex = path.lastIndexOf('/'); - service = path.substring(1, splitIndex); - method = path.substring(splitIndex + 1); + if (!route.getRouteMatch().isDefaultMatcher()) { + String prefix = route.getRouteMatch().getPrefix(); + String path = route.getRouteMatch().getPath(); + if (!prefix.isEmpty()) { + service = prefix.substring(1, prefix.length() - 1); + } else if (!path.isEmpty()) { + int splitIndex = path.lastIndexOf('/'); + service = path.substring(1, splitIndex); + method = path.substring(splitIndex + 1); + } } Map methodName = ImmutableMap.of("service", service, "method", method); String actionName; diff --git a/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java b/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java index 883ecfa1b19c..49e214e54741 100644 --- a/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java @@ -376,8 +376,9 @@ public void resolve_resourceUpdated_multipleRoutes() { 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 void resolve_resourceUpdated_multipleRoutes() { "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 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 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 routeConfigs = ImmutableList.of( + Any.pack( + buildRouteConfiguration( + routeConfigName, + ImmutableList.of(buildVirtualHostForRoutes(AUTHORITY, protoRoutes))))); + responseObserver.onNext( + buildDiscoveryResponse("0", routeConfigs, XdsClientImpl.ADS_TYPE_URL_RDS, "0000")); + + ArgumentCaptor resolutionResultCaptor = ArgumentCaptor.forClass(null); + verify(mockListener).onResult(resolutionResultCaptor.capture()); + ResolutionResult result = resolutionResultCaptor.getValue(); + assertThat(result.getAddresses()).isEmpty(); + Map serviceConfig = (Map) result.getServiceConfig().getConfig(); + + List> rawLbConfigs = + (List>) serviceConfig.get("loadBalancingConfig"); + Map lbConfig = Iterables.getOnlyElement(rawLbConfigs); + assertThat(lbConfig.keySet()).containsExactly("xds_routing_experimental"); + Map rawConfigValues = (Map) lbConfig.get("xds_routing_experimental"); + assertThat(rawConfigValues.keySet()).containsExactly("action", "route"); + Map> actions = + (Map>) rawConfigValues.get("action"); + List> routes = (List>) rawConfigValues.get("route"); + assertThat(routes).hasSize(5); + for (Map route : routes) { + assertThat(route.keySet()).containsExactly("methodName", "action"); + } + assertThat((Map) routes.get(0).get("methodName")) + .containsExactly("service", "fooSvc", "method", "hello"); + String action0 = (String) routes.get(0).get("action"); + assertThat((Map) routes.get(1).get("methodName")) + .containsExactly("service", "fooSvc", "method", ""); + String action1 = (String) routes.get(1).get("action"); + assertThat((Map) routes.get(2).get("methodName")) + .containsExactly("service", "fooSvc", "method", "hello"); + String action2 = (String) routes.get(2).get("action"); + assertThat((Map) routes.get(3).get("methodName")) + .containsExactly("service", "fooSvc", "method", ""); + String action3 = (String) routes.get(3).get("action"); + assertThat((Map) 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 action, String clusterName) {