From 92f4842f0f1b2838ffe4d68af8805eb1426df6ef Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Wed, 15 Jul 2020 21:06:02 +0000 Subject: [PATCH] xds: perform header matching on concatenated values (#7215) Combine values of header fields with the same key to a comma-separated string before performing header matching. --- xds/src/main/java/io/grpc/xds/RouteMatch.java | 42 +++++++++---------- .../io/grpc/xds/XdsRoutingLoadBalancer.java | 9 +--- .../test/java/io/grpc/xds/RouteMatchTest.java | 23 ++++++---- 3 files changed, 36 insertions(+), 38 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/RouteMatch.java b/xds/src/main/java/io/grpc/xds/RouteMatch.java index 75cbaa9e56..5b52e71c29 100644 --- a/xds/src/main/java/io/grpc/xds/RouteMatch.java +++ b/xds/src/main/java/io/grpc/xds/RouteMatch.java @@ -17,6 +17,7 @@ package io.grpc.xds; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Joiner; import com.google.common.base.MoreObjects; import com.google.common.base.MoreObjects.ToStringHelper; import com.google.re2j.Pattern; @@ -25,7 +26,6 @@ import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Objects; -import java.util.Set; import javax.annotation.Nullable; /** @@ -60,7 +60,7 @@ final class RouteMatch { * *

Match is not deterministic if a runtime fraction match rule presents in this RouteMatch. */ - boolean matches(String path, Map> headers) { + boolean matches(String path, Map> headers) { if (!pathMatch.matches(path)) { return false; } @@ -229,35 +229,31 @@ final class RouteMatch { this.isInvertedMatch = isInvertedMatch; } - private boolean matchesValue(@Nullable Set values) { + private boolean matchesValue(@Nullable Iterable values) { if (presentMatch != null) { return (values == null) == presentMatch.equals(isInvertedMatch); } if (values == null) { return false; } - boolean baseMatch = false; - for (String value : values) { - if (exactMatch != null) { - baseMatch = exactMatch.equals(value); - } else if (safeRegExMatch != null) { - baseMatch = safeRegExMatch.matches(value); - } else if (rangeMatch != null) { - long numValue; - try { - numValue = Long.parseLong(value); - } catch (NumberFormatException ignored) { - continue; - } + String valueStr = Joiner.on(",").join(values); + boolean baseMatch; + if (exactMatch != null) { + baseMatch = exactMatch.equals(valueStr); + } else if (safeRegExMatch != null) { + baseMatch = safeRegExMatch.matches(valueStr); + } else if (rangeMatch != null) { + long numValue; + try { + numValue = Long.parseLong(valueStr); baseMatch = rangeMatch.contains(numValue); - } else if (prefixMatch != null) { - baseMatch = value.startsWith(prefixMatch); - } else { - baseMatch = value.endsWith(suffixMatch); - } - if (baseMatch) { - break; + } catch (NumberFormatException ignored) { + baseMatch = false; } + } else if (prefixMatch != null) { + baseMatch = valueStr.startsWith(prefixMatch); + } else { + baseMatch = valueStr.endsWith(suffixMatch); } return baseMatch != isInvertedMatch; } diff --git a/xds/src/main/java/io/grpc/xds/XdsRoutingLoadBalancer.java b/xds/src/main/java/io/grpc/xds/XdsRoutingLoadBalancer.java index 428164ec41..1cd5045052 100644 --- a/xds/src/main/java/io/grpc/xds/XdsRoutingLoadBalancer.java +++ b/xds/src/main/java/io/grpc/xds/XdsRoutingLoadBalancer.java @@ -42,7 +42,6 @@ import io.grpc.xds.XdsRoutingLoadBalancerProvider.Route; import io.grpc.xds.XdsRoutingLoadBalancerProvider.XdsRoutingConfig; import io.grpc.xds.XdsSubchannelPickers.ErrorPicker; import java.util.HashMap; -import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -264,18 +263,14 @@ final class XdsRoutingLoadBalancer extends LoadBalancer { @Override public PickResult pickSubchannel(PickSubchannelArgs args) { // Index ASCII headers by keys. - Map> asciiHeaders = new HashMap<>(); + Map> asciiHeaders = new HashMap<>(); Metadata headers = args.getHeaders(); for (String headerName : headers.keys()) { if (headerName.endsWith(Metadata.BINARY_HEADER_SUFFIX)) { continue; } - Set headerValues = new HashSet<>(); Metadata.Key key = Metadata.Key.of(headerName, Metadata.ASCII_STRING_MARSHALLER); - for (String value : headers.getAll(key)) { - headerValues.add(value); - } - asciiHeaders.put(headerName, headerValues); + asciiHeaders.put(headerName, headers.getAll(key)); } for (Map.Entry entry : routePickers.entrySet()) { RouteMatch routeMatch = entry.getKey(); diff --git a/xds/src/test/java/io/grpc/xds/RouteMatchTest.java b/xds/src/test/java/io/grpc/xds/RouteMatchTest.java index 222a4a1de3..712758e116 100644 --- a/xds/src/test/java/io/grpc/xds/RouteMatchTest.java +++ b/xds/src/test/java/io/grpc/xds/RouteMatchTest.java @@ -26,9 +26,7 @@ import io.grpc.xds.RouteMatch.PathMatcher; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.Map; -import java.util.Set; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -38,15 +36,15 @@ import org.junit.runners.JUnit4; @RunWith(JUnit4.class) public class RouteMatchTest { - private final Map> headers = new HashMap<>(); + private final Map> headers = new HashMap<>(); @Before public void setUp() { - headers.put("content-type", Collections.singleton("application/grpc")); - headers.put("grpc-encoding", Collections.singleton("gzip")); - headers.put("user-agent", Collections.singleton("gRPC-Java")); - headers.put("content-length", Collections.singleton("1000")); - headers.put("custom-key", new HashSet<>(Arrays.asList("custom-value1", "custom-value2"))); + headers.put("content-type", Collections.singletonList("application/grpc")); + headers.put("grpc-encoding", Collections.singletonList("gzip")); + headers.put("user-agent", Collections.singletonList("gRPC-Java")); + headers.put("content-length", Collections.singletonList("1000")); + headers.put("custom-key", Arrays.asList("custom-value1", "custom-value2")); } @Test @@ -135,6 +133,15 @@ public class RouteMatchTest { null, true)), null); assertThat(routeMatch6.matches("/FooService/barMethod", headers)).isFalse(); + + RouteMatch routeMatch7 = new RouteMatch( + new PathMatcher("/FooService/barMethod", null, null), + Collections.singletonList( + new HeaderMatcher( + "custom-key", "custom-value1,custom-value2", null, null, null, null, + null, false)), + null); + assertThat(routeMatch7.matches("/FooService/barMethod", headers)).isTrue(); } @Test