xds: change route data validation logic (#7047)

Changed the logic of parsing Route to skip Routes with action that specifies cluster_header. Eliminate unnecessary validation logic in XdsClientImpl, of which is already covered in converters.
2020-05-18 23:23:25 +00:00 · 2020-05-18 23:23:25 +00:00 · a86fc47c04
parent 02e3c00c39
commit a86fc47c04
4 changed files with 73 additions and 60 deletions
--- a/xds/src/main/java/io/grpc/xds/EnvoyProtoData.java
+++ b/xds/src/main/java/io/grpc/xds/EnvoyProtoData.java
@ -500,6 +500,9 @@ final class EnvoyProtoData {
        default:
          return StructOrError.fromError("Unknown action type: " + proto.getActionCase());
      }
+      if (routeAction == null) {
+        return null;
+      }
      if (routeAction.getErrorDetail() != null) {
        return StructOrError.fromError(
            "Invalid route [" + proto.getName() + "]: " + routeAction.getErrorDetail());
@ -931,16 +934,11 @@ final class EnvoyProtoData {
    @Nullable
    private final String cluster;
    @Nullable
-    private final String clusterHeader;
-    @Nullable
    private final List<ClusterWeight> weightedClusters;

    @VisibleForTesting
-    RouteAction(
-        @Nullable String cluster, @Nullable String clusterHeader,
-        @Nullable List<ClusterWeight> weightedClusters) {
+    RouteAction(@Nullable String cluster, @Nullable List<ClusterWeight> weightedClusters) {
      this.cluster = cluster;
-      this.clusterHeader = clusterHeader;
      this.weightedClusters = weightedClusters;
    }

@ -949,11 +947,6 @@ final class EnvoyProtoData {
      return cluster;
    }

-    @Nullable
-    String getClusterHeader() {
-      return clusterHeader;
-    }
-
    @Nullable
    List<ClusterWeight> getWeightedCluster() {
      return weightedClusters;
@ -969,13 +962,12 @@ final class EnvoyProtoData {
      }
      RouteAction that = (RouteAction) o;
      return Objects.equals(cluster, that.cluster)
-              && Objects.equals(clusterHeader, that.clusterHeader)
-              && Objects.equals(weightedClusters, that.weightedClusters);
+          && Objects.equals(weightedClusters, that.weightedClusters);
    }

    @Override
    public int hashCode() {
-      return Objects.hash(cluster, clusterHeader, weightedClusters);
+      return Objects.hash(cluster, weightedClusters);
    }

    @Override
@ -984,28 +976,24 @@ final class EnvoyProtoData {
      if (cluster != null) {
        toStringHelper.add("cluster", cluster);
      }
-      if (clusterHeader != null) {
-        toStringHelper.add("clusterHeader", clusterHeader);
-      }
      if (weightedClusters != null) {
        toStringHelper.add("weightedClusters", weightedClusters);
      }
      return toStringHelper.toString();
    }

+    @Nullable
    @VisibleForTesting
    static StructOrError<RouteAction> fromEnvoyProtoRouteAction(
        io.envoyproxy.envoy.api.v2.route.RouteAction proto) {
      String cluster = null;
-      String clusterHeader = null;
      List<ClusterWeight> weightedClusters = null;
      switch (proto.getClusterSpecifierCase()) {
        case CLUSTER:
          cluster = proto.getCluster();
          break;
        case CLUSTER_HEADER:
-          clusterHeader = proto.getClusterHeader();
-          break;
+          return null;
        case WEIGHTED_CLUSTERS:
          List<io.envoyproxy.envoy.api.v2.route.WeightedCluster.ClusterWeight> clusterWeights
              = proto.getWeightedClusters().getClustersList();
@ -1020,7 +1008,7 @@ final class EnvoyProtoData {
          return StructOrError.fromError(
              "Unknown cluster specifier: " + proto.getClusterSpecifierCase());
      }
-      return StructOrError.fromStruct(new RouteAction(cluster, clusterHeader, weightedClusters));
+      return StructOrError.fromStruct(new RouteAction(cluster, weightedClusters));
    }
  }

--- a/xds/src/main/java/io/grpc/xds/XdsClientImpl.java
+++ b/xds/src/main/java/io/grpc/xds/XdsClientImpl.java
@ -864,30 +864,10 @@ final class XdsClientImpl extends XdsClient {
          "Virtual host [" + virtualHost.getName()
              + "] contains non-default route as the last route");
    }
-    // We only validate the default route unless path matching is enabled.
    if (!enableExperimentalRouting) {
      EnvoyProtoData.Route defaultRoute = Iterables.getLast(routes);
-      if (defaultRoute.getRouteAction().getCluster() == null) {
-        throw new InvalidProtoDataException(
-            "Virtual host [" + virtualHost.getName()
-                + "] default route contains no cluster name");
-      }
      return Collections.singletonList(defaultRoute);
    }
-
-    // We do more validation if path matching is enabled, but whether every single route is
-    // required to be valid for grpc is TBD.
-    // For now we consider the whole list invalid if anything invalid for grpc is found.
-    // TODO(zdapeng): Fix it if the decision is different from current implementation.
-    // TODO(zdapeng): Add test for validation.
-    for (EnvoyProtoData.Route route : routes) {
-      if (route.getRouteAction().getCluster() == null
-          && route.getRouteAction().getWeightedCluster() == null) {
-        throw new InvalidProtoDataException(
-            "Virtual host [" + virtualHost.getName()
-                + "] contains route without cluster or weighted cluster");
-      }
-    }
    return Collections.unmodifiableList(routes);
  }

--- a/xds/src/test/java/io/grpc/xds/EnvoyProtoDataTest.java
+++ b/xds/src/test/java/io/grpc/xds/EnvoyProtoDataTest.java
@ -105,7 +105,7 @@ public class EnvoyProtoDataTest {
            new Route(
                new RouteMatch(
                    null, "/service/method", null, null, Collections.<HeaderMatcher>emptyList()),
-                new RouteAction("cluster-foo", null, null)));
+                new RouteAction("cluster-foo", null)));

    io.envoyproxy.envoy.api.v2.route.Route unsupportedProto =
        io.envoyproxy.envoy.api.v2.route.Route.newBuilder()
@ -118,6 +118,39 @@ public class EnvoyProtoDataTest {
    assertThat(unsupportedStruct.getStruct()).isNull();
  }

+  @Test
+  public void convertRoute_skipWithUnsupportedMatcher() {
+    io.envoyproxy.envoy.api.v2.route.Route proto =
+        io.envoyproxy.envoy.api.v2.route.Route.newBuilder()
+            .setName("ignore me")
+            .setMatch(
+                io.envoyproxy.envoy.api.v2.route.RouteMatch.newBuilder()
+                    .setPath("/service/method")
+                    .addQueryParameters(
+                        io.envoyproxy.envoy.api.v2.route.QueryParameterMatcher
+                            .getDefaultInstance()))
+            .setRoute(
+                io.envoyproxy.envoy.api.v2.route.RouteAction.newBuilder()
+                    .setCluster("cluster-foo"))
+            .build();
+    assertThat(Route.fromEnvoyProtoRoute(proto)).isNull();
+  }
+
+  @Test
+  public void convertRoute_skipWithUnsupportedAction() {
+    io.envoyproxy.envoy.api.v2.route.Route proto =
+        io.envoyproxy.envoy.api.v2.route.Route.newBuilder()
+            .setName("ignore me")
+            .setMatch(
+                io.envoyproxy.envoy.api.v2.route.RouteMatch.newBuilder()
+                    .setPath("/service/method"))
+            .setRoute(
+                io.envoyproxy.envoy.api.v2.route.RouteAction.newBuilder()
+                    .setClusterHeader("some cluster header"))
+            .build();
+    assertThat(Route.fromEnvoyProtoRoute(proto)).isNull();
+  }
+
  @Test
  public void isDefaultRoute() {
    StructOrError<Route> struct1 = Route.fromEnvoyProtoRoute(buildSimpleRouteProto("", null));
@ -315,7 +348,6 @@ public class EnvoyProtoDataTest {
    StructOrError<RouteAction> struct1 = RouteAction.fromEnvoyProtoRouteAction(proto1);
    assertThat(struct1.getErrorDetail()).isNull();
    assertThat(struct1.getStruct().getCluster()).isEqualTo("cluster-foo");
-    assertThat(struct1.getStruct().getClusterHeader()).isNull();
    assertThat(struct1.getStruct().getWeightedCluster()).isNull();

    // cluster_specifier = cluster_header
@ -324,10 +356,7 @@ public class EnvoyProtoDataTest {
            .setClusterHeader("cluster-bar")
            .build();
    StructOrError<RouteAction> struct2 = RouteAction.fromEnvoyProtoRouteAction(proto2);
-    assertThat(struct2.getErrorDetail()).isNull();
-    assertThat(struct2.getStruct().getCluster()).isNull();
-    assertThat(struct2.getStruct().getClusterHeader()).isEqualTo("cluster-bar");
-    assertThat(struct2.getStruct().getWeightedCluster()).isNull();
+    assertThat(struct2).isNull();

    // cluster_specifier = weighted_cluster
    io.envoyproxy.envoy.api.v2.route.RouteAction proto3 =
@ -342,7 +371,6 @@ public class EnvoyProtoDataTest {
    StructOrError<RouteAction> struct3 = RouteAction.fromEnvoyProtoRouteAction(proto3);
    assertThat(struct3.getErrorDetail()).isNull();
    assertThat(struct3.getStruct().getCluster()).isNull();
-    assertThat(struct3.getStruct().getClusterHeader()).isNull();
    assertThat(struct3.getStruct().getWeightedCluster())
        .containsExactly(new ClusterWeight("cluster-baz", 100));

--- a/xds/src/test/java/io/grpc/xds/XdsClientImplTest.java
+++ b/xds/src/test/java/io/grpc/xds/XdsClientImplTest.java
@ -61,6 +61,7 @@ import io.envoyproxy.envoy.api.v2.core.ConfigSource;
 import io.envoyproxy.envoy.api.v2.core.HealthStatus;
 import io.envoyproxy.envoy.api.v2.core.Node;
 import io.envoyproxy.envoy.api.v2.endpoint.ClusterStats;
+import io.envoyproxy.envoy.api.v2.route.QueryParameterMatcher;
 import io.envoyproxy.envoy.api.v2.route.RedirectAction;
 import io.envoyproxy.envoy.api.v2.route.Route;
 import io.envoyproxy.envoy.api.v2.route.RouteAction;
@ -727,10 +728,7 @@ public class XdsClientImplTest {
            new EnvoyProtoData.RouteMatch(
                /* prefix= */ null,
                /* path= */ "/service1/method1"),
-            new EnvoyProtoData.RouteAction(
-                "cl1.googleapis.com",
-                null,
-                null)));
+            new EnvoyProtoData.RouteAction("cl1.googleapis.com", null)));
    assertThat(routes.get(1)).isEqualTo(
        new EnvoyProtoData.Route(
            // path match with weighted cluster route
@ -738,7 +736,6 @@ public class XdsClientImplTest {
                /* prefix= */ null,
                /* path= */ "/service2/method2"),
            new EnvoyProtoData.RouteAction(
-                null,
                null,
                ImmutableList.of(
                    new EnvoyProtoData.ClusterWeight("cl21.googleapis.com", 30),
@ -750,10 +747,7 @@ public class XdsClientImplTest {
            new EnvoyProtoData.RouteMatch(
                /* prefix= */ "/service1/",
                /* path= */ null),
-            new EnvoyProtoData.RouteAction(
-                "cl1.googleapis.com",
-                null,
-                null)));
+            new EnvoyProtoData.RouteAction("cl1.googleapis.com", null)));
    assertThat(routes.get(3)).isEqualTo(
        new EnvoyProtoData.Route(
            // default match with cluster route
@ -761,9 +755,7 @@ public class XdsClientImplTest {
                /* prefix= */ "",
                /* path= */ null),
            new EnvoyProtoData.RouteAction(
-                "cluster.googleapis.com",
-                null,
-                null)));
+                "cluster.googleapis.com", null)));
  }

  /**
@ -3472,6 +3464,31 @@ public class XdsClientImplTest {
    XdsClientImpl.populateRoutesInVirtualHost(virtualHost);
  }

+  @Test
+  public void populateRoutesInVirtualHost_NoUsableRoute() {
+    VirtualHost virtualHost =
+        VirtualHost.newBuilder()
+            .setName("virtualhost00.googleapis.com")  // don't care
+            .addDomains(TARGET_AUTHORITY)
+            .addRoutes(
+                // route with unsupported action
+                Route.newBuilder()
+                    .setRoute(RouteAction.newBuilder().setClusterHeader("cluster header string"))
+                    .setMatch(RouteMatch.newBuilder().setPrefix("/")))
+            .addRoutes(
+                // route with unsupported matcher type
+                Route.newBuilder()
+                    .setRoute(RouteAction.newBuilder().setCluster("cluster.googleapis.com"))
+                    .setMatch(
+                        RouteMatch.newBuilder()
+                            .setPrefix("/")
+                            .addQueryParameters(QueryParameterMatcher.getDefaultInstance())))
+            .build();
+
+    thrown.expect(XdsClientImpl.InvalidProtoDataException.class);
+    XdsClientImpl.populateRoutesInVirtualHost(virtualHost);
+  }
+
  @Test
  public void messagePrinter_printLdsResponse() {
    MessagePrinter printer = new MessagePrinter();