From d71161432aa5c72eb6a20124b194476db95cbe90 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Wed, 27 May 2020 19:56:22 +0000 Subject: [PATCH] xds: remove path matcher format requirements and default route requirement for routing enabled (v1.30.x backport) (#7063) (#7070) Remove requirement for formats of path matchers. Only require the last route to be the default route for the case when routing is disabled. --- .../main/java/io/grpc/xds/EnvoyProtoData.java | 33 +++----------- .../main/java/io/grpc/xds/XdsClientImpl.java | 12 ++--- .../java/io/grpc/xds/EnvoyProtoDataTest.java | 45 ------------------- 3 files changed, 11 insertions(+), 79 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/EnvoyProtoData.java b/xds/src/main/java/io/grpc/xds/EnvoyProtoData.java index 329d7c4a74..94b22e887a 100644 --- a/xds/src/main/java/io/grpc/xds/EnvoyProtoData.java +++ b/xds/src/main/java/io/grpc/xds/EnvoyProtoData.java @@ -444,6 +444,7 @@ final class EnvoyProtoData { return routeAction; } + // TODO(chengyuanzhang): delete and do not use after routing feature is always ON. boolean isDefaultRoute() { return routeMatch.isMatchAll(); } @@ -553,16 +554,10 @@ final class EnvoyProtoData { return pathExactMatch; } - boolean isMatchAll() { - if (pathSafeRegExMatch != null || fractionMatch != null) { - return false; - } - if (!headerMatchers.isEmpty()) { - return false; - } - if (pathExactMatch != null) { - return false; - } + // TODO(chengyuanzhang): delete and do not use after routing feature is always ON. + private boolean isMatchAll() { + // For backward compatibility, all the other matchers are ignored. When routing is enabled, + // we should never care if a matcher matches all requests. if (pathPrefixMatch != null) { return pathPrefixMatch.isEmpty() || pathPrefixMatch.equals("/"); } @@ -655,27 +650,9 @@ final class EnvoyProtoData { switch (proto.getPathSpecifierCase()) { case PREFIX: prefixPathMatch = proto.getPrefix(); - // Supported prefix match format: - // "", "/" (default) - // "/service/" - if (!prefixPathMatch.isEmpty() && !prefixPathMatch.equals("/")) { - if (!prefixPathMatch.startsWith("/") || !prefixPathMatch.endsWith("/") - || prefixPathMatch.length() < 3) { - return StructOrError.fromError( - "Invalid format of prefix path match: " + prefixPathMatch); - } - } break; case PATH: exactPathMatch = proto.getPath(); - int lastSlash = exactPathMatch.lastIndexOf('/'); - // Supported exact match format: - // "/service/method" - if (!exactPathMatch.startsWith("/") || lastSlash == 0 - || lastSlash == exactPathMatch.length() - 1) { - return StructOrError.fromError( - "Invalid format of exact path match: " + exactPathMatch); - } break; case REGEX: return StructOrError.fromError("Unsupported path match type: regex"); diff --git a/xds/src/main/java/io/grpc/xds/XdsClientImpl.java b/xds/src/main/java/io/grpc/xds/XdsClientImpl.java index 40e9134ebb..ef16e6e17b 100644 --- a/xds/src/main/java/io/grpc/xds/XdsClientImpl.java +++ b/xds/src/main/java/io/grpc/xds/XdsClientImpl.java @@ -858,14 +858,14 @@ final class XdsClientImpl extends XdsClient { throw new InvalidProtoDataException( "Virtual host [" + virtualHost.getName() + "] contains no usable route"); } - // The last route must be a default route. - if (!Iterables.getLast(routes).isDefaultRoute()) { - throw new InvalidProtoDataException( - "Virtual host [" + virtualHost.getName() - + "] contains non-default route as the last route"); - } + if (!enableExperimentalRouting) { EnvoyProtoData.Route defaultRoute = Iterables.getLast(routes); + if (!defaultRoute.isDefaultRoute()) { + throw new InvalidProtoDataException( + "Virtual host [" + virtualHost.getName() + + "] contains non-default route as the last route"); + } return Collections.singletonList(defaultRoute); } return Collections.unmodifiableList(routes); diff --git a/xds/src/test/java/io/grpc/xds/EnvoyProtoDataTest.java b/xds/src/test/java/io/grpc/xds/EnvoyProtoDataTest.java index e8afb4b9e2..ee94f58939 100644 --- a/xds/src/test/java/io/grpc/xds/EnvoyProtoDataTest.java +++ b/xds/src/test/java/io/grpc/xds/EnvoyProtoDataTest.java @@ -246,51 +246,6 @@ public class EnvoyProtoDataTest { assertThat(unsetStruct.getStruct()).isNull(); } - @Test - public void convertRouteMatch_pathMatchFormat() { - StructOrError struct1 = - RouteMatch.fromEnvoyProtoRouteMatch(buildSimpleRouteMatchProto("", null)); - StructOrError struct2 = - RouteMatch.fromEnvoyProtoRouteMatch(buildSimpleRouteMatchProto("/", null)); - StructOrError struct3 = - RouteMatch.fromEnvoyProtoRouteMatch(buildSimpleRouteMatchProto("/service", null)); - StructOrError struct4 = - RouteMatch.fromEnvoyProtoRouteMatch(buildSimpleRouteMatchProto("/service/", null)); - StructOrError struct5 = - RouteMatch.fromEnvoyProtoRouteMatch(buildSimpleRouteMatchProto(null, "")); - StructOrError struct6 = - RouteMatch.fromEnvoyProtoRouteMatch(buildSimpleRouteMatchProto(null, "/service/method")); - StructOrError struct7 = - RouteMatch.fromEnvoyProtoRouteMatch(buildSimpleRouteMatchProto(null, "/service/method/")); - StructOrError struct8 = - RouteMatch.fromEnvoyProtoRouteMatch( - io.envoyproxy.envoy.api.v2.route.RouteMatch.newBuilder() - .setSafeRegex( - io.envoyproxy.envoy.type.matcher.RegexMatcher.newBuilder().setRegex("[")) - .build()); - - assertThat(struct1.getStruct()).isNotNull(); - assertThat(struct2.getStruct()).isNotNull(); - assertThat(struct3.getStruct()).isNull(); - assertThat(struct4.getStruct()).isNotNull(); - assertThat(struct5.getStruct()).isNull(); - assertThat(struct6.getStruct()).isNotNull(); - assertThat(struct7.getStruct()).isNull(); - assertThat(struct8.getStruct()).isNull(); - } - - private static io.envoyproxy.envoy.api.v2.route.RouteMatch buildSimpleRouteMatchProto( - @Nullable String pathPrefix, @Nullable String path) { - io.envoyproxy.envoy.api.v2.route.RouteMatch.Builder builder = - io.envoyproxy.envoy.api.v2.route.RouteMatch.newBuilder(); - if (pathPrefix != null) { - builder.setPrefix(pathPrefix); - } else if (path != null) { - builder.setPath(path); - } - return builder.build(); - } - @Test public void convertRouteMatch_withHeaderMatching() { io.envoyproxy.envoy.api.v2.route.RouteMatch proto =