xds: perform header matching on concatenated values (grpc#7215)

Combine values of header fields with the same key to a comma-separated string before performing header matching.
dfawley · Jan 15, 2021 · b5a73f8 · b5a73f8
1 parent cbb14f7
commit b5a73f8
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 38 deletions.
diff --git 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.List;
 import java.util.Map;
 import java.util.Objects;
-import java.util.Set;
 import javax.annotation.Nullable;
 
 /**
@@ -60,7 +60,7 @@ final class RouteMatch {
    *
    * <p>Match is not deterministic if a runtime fraction match rule presents in this RouteMatch.
    */
-  boolean matches(String path, Map<String, Set<String>> headers) {
+  boolean matches(String path, Map<String, Iterable<String>> headers) {
     if (!pathMatch.matches(path)) {
       return false;
     }
@@ -229,35 +229,31 @@ static final class HeaderMatcher {
       this.isInvertedMatch = isInvertedMatch;
     }
 
-    private boolean matchesValue(@Nullable Set<String> values) {
+    private boolean matchesValue(@Nullable Iterable<String> 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
@@ -42,7 +42,6 @@
 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 @@ static final class RouteMatchingSubchannelPicker extends SubchannelPicker {
     @Override
     public PickResult pickSubchannel(PickSubchannelArgs args) {
       // Index ASCII headers by keys.
-      Map<String, Set<String>> asciiHeaders = new HashMap<>();
+      Map<String, Iterable<String>> asciiHeaders = new HashMap<>();
       Metadata headers = args.getHeaders();
       for (String headerName : headers.keys()) {
         if (headerName.endsWith(Metadata.BINARY_HEADER_SUFFIX)) {
           continue;
         }
-        Set<String> headerValues = new HashSet<>();
         Metadata.Key<String> 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<RouteMatch, SubchannelPicker> 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
@@ -26,9 +26,7 @@
 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 @@
 @RunWith(JUnit4.class)
 public class RouteMatchTest {
 
-  private final Map<String, Set<String>> headers = new HashMap<>();
+  private final Map<String, Iterable<String>> 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 void routeMatching_withHeaders() {
                 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