diff --git a/xds/src/main/java/io/grpc/xds/EnvoyProtoData.java b/xds/src/main/java/io/grpc/xds/EnvoyProtoData.java index 7e1cd803c5..329d7c4a74 100644 --- 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 weightedClusters; @VisibleForTesting - RouteAction( - @Nullable String cluster, @Nullable String clusterHeader, - @Nullable List weightedClusters) { + RouteAction(@Nullable String cluster, @Nullable List 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 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 fromEnvoyProtoRouteAction( io.envoyproxy.envoy.api.v2.route.RouteAction proto) { String cluster = null; - String clusterHeader = null; List weightedClusters = null; switch (proto.getClusterSpecifierCase()) { case CLUSTER: cluster = proto.getCluster(); break; case CLUSTER_HEADER: - clusterHeader = proto.getClusterHeader(); - break; + return null; case WEIGHTED_CLUSTERS: List 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)); } } diff --git a/xds/src/main/java/io/grpc/xds/XdsClientImpl.java b/xds/src/main/java/io/grpc/xds/XdsClientImpl.java index ea056755af..40e9134ebb 100644 --- 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); } diff --git a/xds/src/test/java/io/grpc/xds/EnvoyProtoDataTest.java b/xds/src/test/java/io/grpc/xds/EnvoyProtoDataTest.java index 86668c1011..e8afb4b9e2 100644 --- 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.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 struct1 = Route.fromEnvoyProtoRoute(buildSimpleRouteProto("", null)); @@ -315,7 +348,6 @@ public class EnvoyProtoDataTest { StructOrError 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 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 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)); diff --git a/xds/src/test/java/io/grpc/xds/XdsClientImplTest.java b/xds/src/test/java/io/grpc/xds/XdsClientImplTest.java index 6781338616..f67232f624 100644 --- 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();