Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

xds: support case insensitive path matching #7506

Merged
merged 3 commits into from Oct 15, 2020
Merged

xds: support case insensitive path matching #7506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
245f901
Add case sensitivity option to path matcher.
voidzcy Oct 10, 2020
3a64799
Enhance PathMatcher cretion.
voidzcy Oct 10, 2020
d85cbb8
Fix tests.
voidzcy Oct 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 6 additions & 25 deletions xds/src/main/java/io/grpc/xds/EnvoyProtoData.java
View file
Open in desktop
Expand Up @@ -918,16 +918,6 @@ RouteAction getRouteAction() {
return routeAction;
}

// TODO(chengyuanzhang): delete and do not use after routing feature is always ON.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be irrelevant change... This PR cleans up some PathMatcher/RouteMatch APIs, which are used here. So I am just deleting this unused code.

boolean isDefaultRoute() {
// For backward compatibility, all the other matchers are ignored.
String prefix = routeMatch.getPathMatch().getPrefix();
if (prefix != null) {
return prefix.isEmpty() || prefix.equals("/");
}
return false;
}

@Override
public boolean equals(Object o) {
if (this == o) {
Expand Down Expand Up @@ -992,17 +982,12 @@ static StructOrError<Route> fromEnvoyProtoRoute(
}

@VisibleForTesting
@SuppressWarnings("deprecation")
@Nullable
static StructOrError<RouteMatch> convertEnvoyProtoRouteMatch(
io.envoyproxy.envoy.config.route.v3.RouteMatch proto) {
if (proto.getQueryParametersCount() != 0) {
return null;
}
if (proto.hasCaseSensitive() && !proto.getCaseSensitive().getValue()) {
return StructOrError.fromError("Unsupported match option: case insensitive");
}

StructOrError<PathMatcher> pathMatch = convertEnvoyProtoPathMatcher(proto);
if (pathMatch.getErrorDetail() != null) {
return StructOrError.fromError(pathMatch.getErrorDetail());
Expand Down Expand Up @@ -1032,32 +1017,28 @@ static StructOrError<RouteMatch> convertEnvoyProtoRouteMatch(
pathMatch.getStruct(), Collections.unmodifiableList(headerMatchers), fractionMatch));
}

@SuppressWarnings("deprecation")
private static StructOrError<PathMatcher> convertEnvoyProtoPathMatcher(
io.envoyproxy.envoy.config.route.v3.RouteMatch proto) {
String path = null;
String prefix = null;
Pattern safeRegEx = null;
boolean caseSensitive = proto.getCaseSensitive().getValue();
switch (proto.getPathSpecifierCase()) {
case PREFIX:
prefix = proto.getPrefix();
break;
return StructOrError.fromStruct(
PathMatcher.fromPrefix(proto.getPrefix(), caseSensitive));
case PATH:
path = proto.getPath();
break;
return StructOrError.fromStruct(PathMatcher.fromPath(proto.getPath(), caseSensitive));
case SAFE_REGEX:
String rawPattern = proto.getSafeRegex().getRegex();
Pattern safeRegEx;
try {
safeRegEx = Pattern.compile(rawPattern);
} catch (PatternSyntaxException e) {
return StructOrError.fromError("Malformed safe regex pattern: " + e.getMessage());
}
break;
return StructOrError.fromStruct(PathMatcher.fromRegEx(safeRegEx));
case PATHSPECIFIER_NOT_SET:
default:
return StructOrError.fromError("Unknown path match type");
}
return StructOrError.fromStruct(new PathMatcher(path, prefix, safeRegEx));
}

private static StructOrError<FractionMatcher> convertEnvoyProtoFraction(
Expand Down
62 changes: 25 additions & 37 deletions xds/src/main/java/io/grpc/xds/RouteMatch.java
View file
Open in desktop
Expand Up @@ -45,10 +45,8 @@ final class RouteMatch {
this.headerMatchers = headerMatchers;
}

@VisibleForTesting
RouteMatch(@Nullable String pathPrefixMatch, @Nullable String pathExactMatch) {
this(
new PathMatcher(pathExactMatch, pathPrefixMatch, null),
static RouteMatch withPathExactOnly(String pathExact) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this used for test only?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

return new RouteMatch(PathMatcher.fromPath(pathExact, true),
Collections.<HeaderMatcher>emptyList(), null);
}

Expand Down Expand Up @@ -82,19 +80,6 @@ boolean matches(String path, Map<String, Iterable<String>> headers) {
return fractionMatch == null || fractionMatch.matches();
}

PathMatcher getPathMatch() {
return pathMatch;
}

List<HeaderMatcher> getHeaderMatchers() {
return Collections.unmodifiableList(headerMatchers);
}

@Nullable
FractionMatcher getFractionMatch() {
return fractionMatch;
}

@Override
public boolean equals(Object o) {
if (this == o) {
Expand Down Expand Up @@ -132,35 +117,37 @@ static final class PathMatcher {
private final String prefix;
@Nullable
private final Pattern regEx;
private final boolean caseSensitive;

PathMatcher(@Nullable String path, @Nullable String prefix, @Nullable Pattern regEx) {
private PathMatcher(@Nullable String path, @Nullable String prefix, @Nullable Pattern regEx,
boolean caseSensitive) {
this.path = path;
this.prefix = prefix;
this.regEx = regEx;
this.caseSensitive = caseSensitive;
}

private boolean matches(String fullMethodName) {
if (path != null) {
return path.equals(fullMethodName);
} else if (prefix != null) {
return fullMethodName.startsWith(prefix);
}
return regEx.matches(fullMethodName);
static PathMatcher fromPath(String path, boolean caseSensitive) {
return new PathMatcher(path, null, null, caseSensitive);
}

@Nullable
String getPath() {
return path;
static PathMatcher fromPrefix(String prefix, boolean caseSensitive) {
return new PathMatcher(null, prefix, null, caseSensitive);
}

@Nullable
String getPrefix() {
return prefix;
static PathMatcher fromRegEx(Pattern regEx) {
return new PathMatcher(null, null, regEx, false /* doesn't matter */);
}

@Nullable
Pattern getRegEx() {
return regEx;
boolean matches(String fullMethodName) {
if (path != null) {
return caseSensitive ? path.equals(fullMethodName) : path.equalsIgnoreCase(fullMethodName);
} else if (prefix != null) {
return caseSensitive
? fullMethodName.startsWith(prefix)
: fullMethodName.toLowerCase().startsWith(prefix.toLowerCase());
}
return regEx.matches(fullMethodName);
}

@Override
Expand All @@ -174,25 +161,26 @@ public boolean equals(Object o) {
PathMatcher that = (PathMatcher) o;
return Objects.equals(path, that.path)
&& Objects.equals(prefix, that.prefix)
&& Objects.equals(caseSensitive, that.caseSensitive)
&& Objects.equals(
regEx == null ? null : regEx.pattern(),
that.regEx == null ? null : that.regEx.pattern());
}

@Override
public int hashCode() {
return Objects.hash(path, prefix, regEx == null ? null : regEx.pattern());
return Objects.hash(path, prefix, caseSensitive, regEx == null ? null : regEx.pattern());
}

@Override
public String toString() {
ToStringHelper toStringHelper =
MoreObjects.toStringHelper(this);
if (path != null) {
toStringHelper.add("path", path);
toStringHelper.add("path", path).add("caseSensitive", caseSensitive);
}
if (prefix != null) {
toStringHelper.add("prefix", prefix);
toStringHelper.add("prefix", prefix).add("caseSensitive", caseSensitive);
}
if (regEx != null) {
toStringHelper.add("regEx", regEx.pattern());
Expand Down
67 changes: 19 additions & 48 deletions xds/src/test/java/io/grpc/xds/EnvoyProtoDataTest.java
View file
Open in desktop
Expand Up @@ -50,7 +50,6 @@
import java.util.Arrays;
import java.util.Collections;
import java.util.concurrent.TimeUnit;
import javax.annotation.Nullable;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand Down Expand Up @@ -211,7 +210,7 @@ public void convertRoute() {
assertThat(struct1.getStruct())
.isEqualTo(
new Route(
new RouteMatch(new PathMatcher("/service/method", null, null),
new RouteMatch(PathMatcher.fromPath("/service/method", false),
Collections.<HeaderMatcher>emptyList(), null),
new RouteAction(TimeUnit.SECONDS.toNanos(15L), "cluster-foo", null)));

Expand Down Expand Up @@ -259,38 +258,6 @@ public void convertRoute_skipWithUnsupportedAction() {
assertThat(Route.fromEnvoyProtoRoute(proto)).isNull();
}

@Test
public void isDefaultRoute() {
StructOrError<Route> struct1 = Route.fromEnvoyProtoRoute(buildSimpleRouteProto("", null));
StructOrError<Route> struct2 = Route.fromEnvoyProtoRoute(buildSimpleRouteProto("/", null));
StructOrError<Route> struct3 =
Route.fromEnvoyProtoRoute(buildSimpleRouteProto("/service/", null));
StructOrError<Route> struct4 =
Route.fromEnvoyProtoRoute(buildSimpleRouteProto(null, "/service/method"));

assertThat(struct1.getStruct().isDefaultRoute()).isTrue();
assertThat(struct2.getStruct().isDefaultRoute()).isTrue();
assertThat(struct3.getStruct().isDefaultRoute()).isFalse();
assertThat(struct4.getStruct().isDefaultRoute()).isFalse();
}

private static io.envoyproxy.envoy.config.route.v3.Route buildSimpleRouteProto(
@Nullable String pathPrefix, @Nullable String path) {
io.envoyproxy.envoy.config.route.v3.Route.Builder routeBuilder =
io.envoyproxy.envoy.config.route.v3.Route.newBuilder()
.setName("simple-route")
.setRoute(io.envoyproxy.envoy.config.route.v3.RouteAction.newBuilder()
.setCluster("simple-cluster"));
if (pathPrefix != null) {
routeBuilder.setMatch(io.envoyproxy.envoy.config.route.v3.RouteMatch.newBuilder()
.setPrefix(pathPrefix));
} else if (path != null) {
routeBuilder.setMatch(io.envoyproxy.envoy.config.route.v3.RouteMatch.newBuilder()
.setPath(path));
}
return routeBuilder.build();
}

@Test
public void convertRouteMatch_pathMatching() {
// path_specifier = prefix
Expand All @@ -300,7 +267,13 @@ public void convertRouteMatch_pathMatching() {
assertThat(struct1.getErrorDetail()).isNull();
assertThat(struct1.getStruct()).isEqualTo(
new RouteMatch(
new PathMatcher(null, "/", null), Collections.<HeaderMatcher>emptyList(), null));
PathMatcher.fromPrefix("/", false), Collections.<HeaderMatcher>emptyList(), null));

proto1 = proto1.toBuilder().setCaseSensitive(BoolValue.newBuilder().setValue(true)).build();
struct1 = Route.convertEnvoyProtoRouteMatch(proto1);
assertThat(struct1.getStruct()).isEqualTo(
new RouteMatch(
PathMatcher.fromPrefix("/", true), Collections.<HeaderMatcher>emptyList(), null));

// path_specifier = path
io.envoyproxy.envoy.config.route.v3.RouteMatch proto2 =
Expand All @@ -311,7 +284,14 @@ public void convertRouteMatch_pathMatching() {
assertThat(struct2.getErrorDetail()).isNull();
assertThat(struct2.getStruct()).isEqualTo(
new RouteMatch(
new PathMatcher("/service/method", null, null),
PathMatcher.fromPath("/service/method", false),
Collections.<HeaderMatcher>emptyList(), null));

proto2 = proto2.toBuilder().setCaseSensitive(BoolValue.newBuilder().setValue(true)).build();
struct2 = Route.convertEnvoyProtoRouteMatch(proto2);
assertThat(struct2.getStruct()).isEqualTo(
new RouteMatch(
PathMatcher.fromPath("/service/method", true),
Collections.<HeaderMatcher>emptyList(), null));

// path_specifier = safe_regex
Expand All @@ -323,18 +303,9 @@ public void convertRouteMatch_pathMatching() {
assertThat(struct4.getErrorDetail()).isNull();
assertThat(struct4.getStruct()).isEqualTo(
new RouteMatch(
new PathMatcher(null, null, Pattern.compile(".")),
PathMatcher.fromRegEx(Pattern.compile(".")),
Collections.<HeaderMatcher>emptyList(), null));

// case_sensitive = false
io.envoyproxy.envoy.config.route.v3.RouteMatch proto5 =
io.envoyproxy.envoy.config.route.v3.RouteMatch.newBuilder()
.setCaseSensitive(BoolValue.newBuilder().setValue(false))
.build();
StructOrError<RouteMatch> struct5 = Route.convertEnvoyProtoRouteMatch(proto5);
assertThat(struct5.getErrorDetail()).isNotNull();
assertThat(struct5.getStruct()).isNull();

// query_parameters is set
io.envoyproxy.envoy.config.route.v3.RouteMatch proto6 =
io.envoyproxy.envoy.config.route.v3.RouteMatch.newBuilder()
Expand Down Expand Up @@ -370,7 +341,7 @@ public void convertRouteMatch_withHeaderMatching() {
assertThat(struct.getStruct())
.isEqualTo(
new RouteMatch(
new PathMatcher(null, "", null),
PathMatcher.fromPrefix("", false),
Arrays.asList(
new HeaderMatcher(":scheme", null, null, null, null, "http", null, false),
new HeaderMatcher(":method", "PUT", null, null, null, null, null, false)),
Expand All @@ -394,7 +365,7 @@ public void convertRouteMatch_withRuntimeFraction() {
assertThat(struct.getStruct())
.isEqualTo(
new RouteMatch(
new PathMatcher(null, "", null), Collections.<HeaderMatcher>emptyList(),
PathMatcher.fromPrefix( "", false), Collections.<HeaderMatcher>emptyList(),
new FractionMatcher(30, 100)));
}

Expand Down