From d6813c93870b618cef3cd7090f4a14af3b2c1191 Mon Sep 17 00:00:00 2001 From: yifeizhuang Date: Tue, 6 Dec 2022 14:30:08 -0800 Subject: [PATCH] Nack xds response when weighter cluster is sum zero --- .../grpc/xds/XdsRouteConfigureResource.java | 5 ++ .../io/grpc/xds/XdsClientImplDataTest.java | 22 +++++++++ .../io/grpc/xds/XdsClientImplTestBase.java | 46 +++++++++++++++++++ 3 files changed, 73 insertions(+) diff --git a/xds/src/main/java/io/grpc/xds/XdsRouteConfigureResource.java b/xds/src/main/java/io/grpc/xds/XdsRouteConfigureResource.java index 0dfc32f805e..e6c73d2df28 100644 --- a/xds/src/main/java/io/grpc/xds/XdsRouteConfigureResource.java +++ b/xds/src/main/java/io/grpc/xds/XdsRouteConfigureResource.java @@ -511,6 +511,7 @@ static StructOrError parseRouteAction( return StructOrError.fromError("No cluster found in weighted cluster list"); } List weightedClusters = new ArrayList<>(); + int clusterWeightSum = 0; for (io.envoyproxy.envoy.config.route.v3.WeightedCluster.ClusterWeight clusterWeight : clusterWeights) { StructOrError clusterWeightOrError = @@ -519,8 +520,12 @@ static StructOrError parseRouteAction( return StructOrError.fromError("RouteAction contains invalid ClusterWeight: " + clusterWeightOrError.getErrorDetail()); } + clusterWeightSum += clusterWeight.getWeight().getValue(); weightedClusters.add(clusterWeightOrError.getStruct()); } + if (clusterWeightSum <= 0) { + return StructOrError.fromError("Sum of cluster weights should be above 0."); + } return StructOrError.fromStruct(VirtualHost.Route.RouteAction.forWeightedClusters( weightedClusters, hashPolicies, timeoutNano, retryPolicy)); case CLUSTER_SPECIFIER_PLUGIN: diff --git a/xds/src/test/java/io/grpc/xds/XdsClientImplDataTest.java b/xds/src/test/java/io/grpc/xds/XdsClientImplDataTest.java index 8c6ccc27806..f47239c123d 100644 --- a/xds/src/test/java/io/grpc/xds/XdsClientImplDataTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsClientImplDataTest.java @@ -521,6 +521,28 @@ public void parseRouteAction_withWeightedCluster() { ClusterWeight.create("cluster-bar", 70, ImmutableMap.of())); } + @Test + public void parseRouteAction_weightedClusterSum() { + io.envoyproxy.envoy.config.route.v3.RouteAction proto = + io.envoyproxy.envoy.config.route.v3.RouteAction.newBuilder() + .setWeightedClusters( + WeightedCluster.newBuilder() + .addClusters( + WeightedCluster.ClusterWeight + .newBuilder() + .setName("cluster-foo") + .setWeight(UInt32Value.newBuilder().setValue(0))) + .addClusters(WeightedCluster.ClusterWeight + .newBuilder() + .setName("cluster-bar") + .setWeight(UInt32Value.newBuilder().setValue(0)))) + .build(); + StructOrError struct = + XdsRouteConfigureResource.parseRouteAction(proto, filterRegistry, false, + ImmutableMap.of(), ImmutableSet.of()); + assertThat(struct.getErrorDetail()).isEqualTo("Sum of cluster weights should be above 0."); + } + @Test public void parseRouteAction_withTimeoutByGrpcTimeoutHeaderMax() { io.envoyproxy.envoy.config.route.v3.RouteAction proto = diff --git a/xds/src/test/java/io/grpc/xds/XdsClientImplTestBase.java b/xds/src/test/java/io/grpc/xds/XdsClientImplTestBase.java index ca996af074a..bc91f39cb2f 100644 --- a/xds/src/test/java/io/grpc/xds/XdsClientImplTestBase.java +++ b/xds/src/test/java/io/grpc/xds/XdsClientImplTestBase.java @@ -40,6 +40,7 @@ import com.google.protobuf.util.Durations; import io.envoyproxy.envoy.config.cluster.v3.OutlierDetection; import io.envoyproxy.envoy.config.route.v3.FilterConfig; +import io.envoyproxy.envoy.config.route.v3.WeightedCluster; import io.envoyproxy.envoy.extensions.filters.http.router.v3.Router; import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CertificateProviderPluginInstance; import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CommonTlsContext; @@ -1363,6 +1364,51 @@ public void rdsResponseErrorHandling_someResourcesFailedUnpack() { verify(rdsResourceWatcher).onChanged(any(RdsUpdate.class)); } + @Test + public void rdsResponseErrorHandling_nackWeightedSumZero() { + DiscoveryRpcCall call = startResourceWatcher(XdsRouteConfigureResource.getInstance(), + RDS_RESOURCE, rdsResourceWatcher); + verifyResourceMetadataRequested(RDS, RDS_RESOURCE); + + io.envoyproxy.envoy.config.route.v3.RouteAction routeAction = + io.envoyproxy.envoy.config.route.v3.RouteAction.newBuilder() + .setWeightedClusters( + WeightedCluster.newBuilder() + .addClusters( + WeightedCluster.ClusterWeight + .newBuilder() + .setName("cluster-foo") + .setWeight(UInt32Value.newBuilder().setValue(0))) + .addClusters(WeightedCluster.ClusterWeight + .newBuilder() + .setName("cluster-bar") + .setWeight(UInt32Value.newBuilder().setValue(0)))) + .build(); + io.envoyproxy.envoy.config.route.v3.Route route = + io.envoyproxy.envoy.config.route.v3.Route.newBuilder() + .setName("route-blade") + .setMatch( + io.envoyproxy.envoy.config.route.v3.RouteMatch.newBuilder() + .setPath("/service/method")) + .setRoute(routeAction) + .build(); + + Any zeroWeightSum = Any.pack(mf.buildRouteConfiguration(RDS_RESOURCE, + Arrays.asList(mf.buildVirtualHost(Arrays.asList(route), ImmutableMap.of())))); + List resources = ImmutableList.of(zeroWeightSum); + call.sendResponse(RDS, resources, VERSION_1, "0000"); + + List errors = ImmutableList.of( + "RDS response RouteConfiguration \'route-configuration.googleapis.com\' validation error: " + + "RouteConfiguration contains invalid virtual host: Virtual host [do not care] " + + "contains invalid route : Route [route-blade] contains invalid RouteAction: " + + "Sum of cluster weights should be above 0."); + verifySubscribedResourcesMetadataSizes(0, 0, 1, 0); + // The response is NACKed with the same error message. + call.verifyRequestNack(RDS, RDS_RESOURCE, "", "0000", NODE, errors); + verify(rdsResourceWatcher, never()).onChanged(any(RdsUpdate.class)); + } + /** * Tests a subscribed RDS resource transitioned to and from the invalid state. *