Skip to content

Commit

Permalink
xds: allow duplicated route matcher and prefix='/'
Browse files Browse the repository at this point in the history
  • Loading branch information
@dapengzhang0 @dfawley
dapengzhang0 authored and dfawley committed Jan 15, 2021
1 parent 1fde8bf commit 1083408
Show file tree
Hide file tree
Showing 4 changed files with 138 additions and 44 deletions.
10 changes: 10 additions & 0 deletions xds/src/main/java/io/grpc/xds/EnvoyProtoData.java
View file
Expand Up @@ -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) {
Expand Down
53 changes: 18 additions & 35 deletions xds/src/main/java/io/grpc/xds/XdsClientImpl.java
View file
Expand Up @@ -916,8 +916,7 @@ private static String validateRoutes(List<EnvoyProtoData.Route> 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) {
Expand All @@ -934,50 +933,34 @@ private static String validateRoutes(List<EnvoyProtoData.Route> 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<String> prefixMatches = new HashSet<>();
Set<String> 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";
}
Expand Down
18 changes: 10 additions & 8 deletions xds/src/main/java/io/grpc/xds/XdsNameResolver.java
View file
Expand Up @@ -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<String, String> methodName = ImmutableMap.of("service", service, "method", method);
String actionName;
Expand Down
101 changes: 100 additions & 1 deletion xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java
View file
Expand Up @@ -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 =
Expand Down Expand Up @@ -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<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) {
Expand Down

0 comments on commit 1083408

Please sign in to comment.