xds: Fix validation of HCM filter and Router httpFilter (#8039) (#8046)

grpc · Apr 2, 2021 · 5090382 · 5090382
1 parent 7590bd1
commit 5090382
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 47 deletions.
diff --git a/xds/src/main/java/io/grpc/xds/EnvoyServerProtoData.java b/xds/src/main/java/io/grpc/xds/EnvoyServerProtoData.java
@@ -386,9 +386,6 @@ static FilterChain fromEnvoyProtoFilterChain(
 
     private static void validateFilter(Filter filter)
         throws InvalidProtocolBufferException, IllegalArgumentException {
-      if (!"envoy.http_connection_manager".equals(filter.getName())) {
-        throw new IllegalArgumentException("filter " + filter.getName() + " not supported.");
-      }
       if (filter.hasConfigDiscovery()) {
         throw new IllegalArgumentException(
             "filter " + filter.getName() + " with config_discovery not supported");
@@ -417,10 +414,6 @@ private static void validateHttpConnectionManager(HttpConnectionManager hcm)
               "http-connection-manager has non-unique http-filter name:" + httpFilterName);
         }
         if (!httpFilter.getIsOptional()) {
-          if (!"envoy.router".equals(httpFilterName)) {
-            throw new IllegalArgumentException(
-                "http-connection-manager has unsupported http-filter:" + httpFilterName);
-          }
           if (httpFilter.hasConfigDiscovery()) {
             throw new IllegalArgumentException(
                 "http-connection-manager http-filter " + httpFilterName
@@ -434,6 +427,12 @@ private static void validateHttpConnectionManager(HttpConnectionManager hcm)
                   "http-connection-manager http-filter " + httpFilterName
                       + " has unsupported typed-config type:" + any.getTypeUrl());
             }
+          } else {
+            throw new IllegalArgumentException(
+                "http-connection-manager http-filter "
+                    + httpFilterName
+                    + " should have typed-config type "
+                    + "type.googleapis.com/envoy.extensions.filters.http.router.v3.Router");
           }
         }
       }
@@ -564,7 +563,7 @@ private static boolean isAcceptable(
       // reject if filer-chain-match
       // - has server_name
       // - transport protocol is other than "raw_buffer"
-      // - application_protocols is non-empty (for now accept "managed-mtls")
+      // - application_protocols is non-empty
       if (!filterChainMatch.getServerNamesList().isEmpty()) {
         return false;
       }
@@ -573,8 +572,7 @@ private static boolean isAcceptable(
         return false;
       }
       List<String> appProtocols = filterChainMatch.getApplicationProtocolsList();
-      return appProtocols.isEmpty()
-          || appProtocols.contains("managed-mtls"); // TODO(sanjaypujare): remove once TD fixed
+      return appProtocols.isEmpty();
     }
 
     public String getName() {

diff --git a/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java b/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java
@@ -758,25 +758,6 @@ public void parseServerSideListener_duplicateFilterName() {
         .isEqualTo("filerChain  has non-unique filter name:envoy.http_connection_manager");
   }
 
-  @Test
-  public void parseServerSideListener_nonHcmFilter() {
-    FilterChain filterChain =
-        buildFilterChain(
-            Filter.newBuilder()
-                .setName("xyz")
-                .setTypedConfig(Any.pack(HttpConnectionManager.getDefaultInstance()))
-                .build());
-    Listener listener =
-        Listener.newBuilder()
-            .setName("listener1")
-            .setTrafficDirection(TrafficDirection.INBOUND)
-            .addFilterChains(filterChain)
-            .build();
-    StructOrError<io.grpc.xds.EnvoyServerProtoData.Listener> struct =
-        ClientXdsClient.parseServerSideListener(listener);
-    assertThat(struct.getErrorDetail()).isEqualTo("filter xyz not supported.");
-  }
-
   @Test
   public void parseServerSideListener_configDiscoveryFilter() {
     Filter filter =
@@ -855,10 +836,14 @@ public void parseServerSideListener_duplicateHttpFilter() {
   }
 
   @Test
-  public void parseServerSideListener_unsupportedHttpFilter() {
+  public void parseServerSideListener_configDiscoveryHttpFilter() {
     Filter filter =
         buildHttpConnectionManager(
-            "envoy.http_connection_manager", HttpFilter.newBuilder().setName("hf").build());
+            "envoy.http_connection_manager",
+            HttpFilter.newBuilder()
+                .setName("envoy.router")
+                .setConfigDiscovery(ExtensionConfigSource.newBuilder().build())
+                .build());
     FilterChain filterChain = buildFilterChain(filter);
     Listener listener =
         Listener.newBuilder()
@@ -869,17 +854,20 @@ public void parseServerSideListener_unsupportedHttpFilter() {
     StructOrError<io.grpc.xds.EnvoyServerProtoData.Listener> struct =
         ClientXdsClient.parseServerSideListener(listener);
     assertThat(struct.getErrorDetail())
-        .isEqualTo("http-connection-manager has unsupported http-filter:hf");
+        .isEqualTo(
+            "http-connection-manager http-filter envoy.router uses "
+                + "config-discovery which is unsupported");
   }
 
   @Test
-  public void parseServerSideListener_configDiscoveryHttpFilter() {
+  public void parseServerSideListener_badTypeUrlHttpFilter() {
+    HTTPFault fault = HTTPFault.newBuilder().build();
     Filter filter =
         buildHttpConnectionManager(
             "envoy.http_connection_manager",
             HttpFilter.newBuilder()
                 .setName("envoy.router")
-                .setConfigDiscovery(ExtensionConfigSource.newBuilder().build())
+                .setTypedConfig(Any.pack(fault, "type.googleapis.com"))
                 .build());
     FilterChain filterChain = buildFilterChain(filter);
     Listener listener =
@@ -892,20 +880,16 @@ public void parseServerSideListener_configDiscoveryHttpFilter() {
         ClientXdsClient.parseServerSideListener(listener);
     assertThat(struct.getErrorDetail())
         .isEqualTo(
-            "http-connection-manager http-filter envoy.router uses "
-                + "config-discovery which is unsupported");
+            "http-connection-manager http-filter envoy.router has unsupported typed-config type:"
+                + "type.googleapis.com/envoy.extensions.filters.http.fault.v3.HTTPFault");
   }
 
   @Test
-  public void parseServerSideListener_badTypeUrlHttpFilter() {
-    HTTPFault fault = HTTPFault.newBuilder().build();
+  public void parseServerSideListener_missingTypeUrlHttpFilter() {
     Filter filter =
         buildHttpConnectionManager(
             "envoy.http_connection_manager",
-            HttpFilter.newBuilder()
-                .setName("envoy.router")
-                .setTypedConfig(Any.pack(fault, "type.googleapis.com"))
-                .build());
+            HttpFilter.newBuilder().setName("envoy.filters.http.router").build());
     FilterChain filterChain = buildFilterChain(filter);
     Listener listener =
         Listener.newBuilder()
@@ -917,8 +901,9 @@ public void parseServerSideListener_badTypeUrlHttpFilter() {
         ClientXdsClient.parseServerSideListener(listener);
     assertThat(struct.getErrorDetail())
         .isEqualTo(
-            "http-connection-manager http-filter envoy.router has unsupported typed-config type:"
-                + "type.googleapis.com/envoy.extensions.filters.http.fault.v3.HTTPFault");
+            "http-connection-manager http-filter envoy.filters.http.router should have "
+                + "typed-config type "
+                + "type.googleapis.com/envoy.extensions.filters.http.router.v3.Router");
   }
 
   static Filter buildHttpConnectionManager(String name, HttpFilter... httpFilters) {
@@ -937,7 +922,6 @@ static FilterChain buildFilterChain(Filter... filters) {
     return FilterChain.newBuilder()
         .setFilterChainMatch(
             FilterChainMatch.newBuilder()
-                .addAllApplicationProtocols(Arrays.asList("managed-mtls"))
                 .build())
         .setTransportSocket(TransportSocket.getDefaultInstance())
         .addAllFilters(Arrays.asList(filters))

diff --git a/xds/src/test/java/io/grpc/xds/EnvoyServerProtoDataTest.java b/xds/src/test/java/io/grpc/xds/EnvoyServerProtoDataTest.java
@@ -73,7 +73,7 @@ public void listener_convertFromListenerProto() throws InvalidProtocolBufferExce
     EnvoyServerProtoData.FilterChainMatch inFilterChainMatch = inFilter.getFilterChainMatch();
     assertThat(inFilterChainMatch).isNotNull();
     assertThat(inFilterChainMatch.getDestinationPort()).isEqualTo(8000);
-    assertThat(inFilterChainMatch.getApplicationProtocols()).containsExactly("managed-mtls");
+    assertThat(inFilterChainMatch.getApplicationProtocols()).isEmpty();
     assertThat(inFilterChainMatch.getPrefixRanges())
         .containsExactly(new EnvoyServerProtoData.CidrRange("10.20.0.15", 32));
     assertThat(inFilterChainMatch.getSourcePrefixRanges())
@@ -114,7 +114,6 @@ private static FilterChain createInFilter() {
                             .setAddressPrefix("10.30.3.0")
                             .setPrefixLen(UInt32Value.of(24))
                             .build())
-                    .addApplicationProtocols("managed-mtls")
                     .setSourceType(FilterChainMatch.ConnectionSourceType.EXTERNAL)
                     .addSourcePorts(200)
                     .addSourcePorts(300)