From 7ad7876e99e0217914783308920701ca0b8b4dd6 Mon Sep 17 00:00:00 2001 From: yifeizhuang Date: Thu, 9 Sep 2021 12:15:27 -0700 Subject: [PATCH] fix header matcher for null value (#8503) --- .../java/io/grpc/xds/XdsNameResolver.java | 32 +------------------ .../java/io/grpc/xds/internal/Matchers.java | 9 ++---- .../io/grpc/xds/internal/MatcherTest.java | 13 ++++++++ 3 files changed, 17 insertions(+), 37 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java index bb73f3c1823..e2905dee26e 100644 --- a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java +++ b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java @@ -576,7 +576,7 @@ static boolean matchRoute(RouteMatch routeMatch, String fullMethodName, return false; } for (HeaderMatcher headerMatcher : routeMatch.headerMatchers()) { - if (!matchHeader(headerMatcher, getHeaderValue(headers, headerMatcher.name()))) { + if (!headerMatcher.matches(getHeaderValue(headers, headerMatcher.name()))) { return false; } } @@ -597,36 +597,6 @@ private static boolean matchPath(PathMatcher pathMatcher, String fullMethodName) return pathMatcher.regEx().matches(fullMethodName); } - // TODO(zivy): consider reuse Matchers.HeaderMatcher.matches() - private static boolean matchHeader(HeaderMatcher headerMatcher, @Nullable String value) { - if (headerMatcher.present() != null) { - return (value == null) == headerMatcher.present().equals(headerMatcher.inverted()); - } - if (value == null) { - return false; - } - boolean baseMatch; - if (headerMatcher.exactValue() != null) { - baseMatch = headerMatcher.exactValue().equals(value); - } else if (headerMatcher.safeRegEx() != null) { - baseMatch = headerMatcher.safeRegEx().matches(value); - } else if (headerMatcher.range() != null) { - long numValue; - try { - numValue = Long.parseLong(value); - baseMatch = numValue >= headerMatcher.range().start() - && numValue <= headerMatcher.range().end(); - } catch (NumberFormatException ignored) { - baseMatch = false; - } - } else if (headerMatcher.prefix() != null) { - baseMatch = value.startsWith(headerMatcher.prefix()); - } else { - baseMatch = value.endsWith(headerMatcher.suffix()); - } - return baseMatch != headerMatcher.inverted(); - } - @Nullable private static String getHeaderValue(Metadata headers, String headerName) { if (headerName.endsWith(Metadata.BINARY_HEADER_SUFFIX)) { diff --git a/xds/src/main/java/io/grpc/xds/internal/Matchers.java b/xds/src/main/java/io/grpc/xds/internal/Matchers.java index 28ec8418297..3bf7b7723e2 100644 --- a/xds/src/main/java/io/grpc/xds/internal/Matchers.java +++ b/xds/src/main/java/io/grpc/xds/internal/Matchers.java @@ -117,13 +117,8 @@ private static HeaderMatcher create(String name, @Nullable String exactValue, /** Returns the matching result. */ public boolean matches(@Nullable String value) { - if (present() != null) { - return (value == null) == present().equals(inverted()); - } - // FIXME(zivy@): invert result for null value. - // https://github.com/envoyproxy/envoy/blob/0fae6970ddaf93f024908ba304bbd2b34e997a51/source/common/http/header_utility.cc#L130 if (value == null) { - return false; + return present() != null && present() == inverted(); } boolean baseMatch; if (exactValue() != null) { @@ -141,6 +136,8 @@ public boolean matches(@Nullable String value) { } } else if (prefix() != null) { baseMatch = value.startsWith(prefix()); + } else if (present() != null) { + baseMatch = present(); } else { baseMatch = value.endsWith(suffix()); } diff --git a/xds/src/test/java/io/grpc/xds/internal/MatcherTest.java b/xds/src/test/java/io/grpc/xds/internal/MatcherTest.java index 4fb4acc41f6..93a9b7087d6 100644 --- a/xds/src/test/java/io/grpc/xds/internal/MatcherTest.java +++ b/xds/src/test/java/io/grpc/xds/internal/MatcherTest.java @@ -127,45 +127,58 @@ public void headerMatcher() { HeaderMatcher matcher = HeaderMatcher.forExactValue("version", "v1", false); assertThat(matcher.matches("v1")).isTrue(); assertThat(matcher.matches("v2")).isFalse(); + assertThat(matcher.matches(null)).isFalse(); matcher = HeaderMatcher.forExactValue("version", "v1", true); assertThat(matcher.matches("v1")).isFalse(); assertThat(matcher.matches( "v2")).isTrue(); + assertThat(matcher.matches(null)).isFalse(); matcher = HeaderMatcher.forPresent("version", true, false); assertThat(matcher.matches("any")).isTrue(); assertThat(matcher.matches(null)).isFalse(); matcher = HeaderMatcher.forPresent("version", true, true); assertThat(matcher.matches("version")).isFalse(); + assertThat(matcher.matches(null)).isTrue(); matcher = HeaderMatcher.forPresent("version", false, true); assertThat(matcher.matches("tag")).isTrue(); + assertThat(matcher.matches(null)).isFalse(); matcher = HeaderMatcher.forPresent("version", false, false); assertThat(matcher.matches("tag")).isFalse(); + assertThat(matcher.matches(null)).isTrue(); matcher = HeaderMatcher.forPrefix("version", "v2", false); assertThat(matcher.matches("v22")).isTrue(); + assertThat(matcher.matches(null)).isFalse(); matcher = HeaderMatcher.forPrefix("version", "v2", true); assertThat(matcher.matches("v22")).isFalse(); + assertThat(matcher.matches(null)).isFalse(); matcher = HeaderMatcher.forSuffix("version", "v1", false); assertThat(matcher.matches("xv1")).isTrue(); assertThat(matcher.matches("v1x")).isFalse(); + assertThat(matcher.matches(null)).isFalse(); matcher = HeaderMatcher.forSuffix("version", "v2", true); assertThat(matcher.matches("xv1")).isTrue(); assertThat(matcher.matches("1v2")).isFalse(); + assertThat(matcher.matches(null)).isFalse(); matcher = HeaderMatcher.forSafeRegEx("version", Pattern.compile("v2.*"), false); assertThat(matcher.matches("v2..")).isTrue(); assertThat(matcher.matches("v1")).isFalse(); + assertThat(matcher.matches(null)).isFalse(); matcher = HeaderMatcher.forSafeRegEx("version", Pattern.compile("v1\\..*"), true); assertThat(matcher.matches("v1.43")).isFalse(); assertThat(matcher.matches("v2")).isTrue(); + assertThat(matcher.matches(null)).isFalse(); matcher = HeaderMatcher.forRange("version", Range.create(8080L, 8090L), false); assertThat(matcher.matches("8080")).isTrue(); assertThat(matcher.matches("1")).isFalse(); + assertThat(matcher.matches(null)).isFalse(); matcher = HeaderMatcher.forRange("version", Range.create(8080L, 8090L), true); assertThat(matcher.matches("1")).isTrue(); assertThat(matcher.matches("8080")).isFalse(); + assertThat(matcher.matches(null)).isFalse(); } }