From 7a96bed8375c6eed6b4be257a3d76a5cc097a82b Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 19 Jan 2023 08:01:26 -0800 Subject: [PATCH] xDS: fix WeightedClusters total weight handling (#32134) * xDS: fix WeightedClusters total weight handling * iwyu --- .../resolver/xds/xds_resolver.cc | 7 +- src/core/ext/xds/xds_route_config.cc | 5 + .../xds_route_config_resource_type_test.cc | 33 +++++++ .../end2end/xds/xds_routing_end2end_test.cc | 98 ++++++++++++++++--- 4 files changed, 122 insertions(+), 21 deletions(-) diff --git a/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc b/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc index 4e283df9e70..00f81789171 100644 --- a/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc +++ b/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc @@ -688,11 +688,8 @@ XdsResolver::XdsConfigSelector::GetCallConfig(GetCallConfigArgs args) { [&](const std::vector< XdsRouteConfigResource::Route::RouteAction::ClusterWeight>& /*weighted_clusters*/) { - const uint32_t key = - rand() % - entry - .weighted_cluster_state[entry.weighted_cluster_state.size() - 1] - .range_end; + const uint32_t key = absl::Uniform( + absl::BitGen(), 0, entry.weighted_cluster_state.back().range_end); // Find the index in weighted clusters corresponding to key. size_t mid = 0; size_t start_index = 0; diff --git a/src/core/ext/xds/xds_route_config.cc b/src/core/ext/xds/xds_route_config.cc index c076f03407e..00e3f58b863 100644 --- a/src/core/ext/xds/xds_route_config.cc +++ b/src/core/ext/xds/xds_route_config.cc @@ -21,6 +21,7 @@ #include #include +#include #include #include #include @@ -835,6 +836,7 @@ absl::optional RouteActionParse( GPR_ASSERT(weighted_clusters_proto != nullptr); std::vector action_weighted_clusters; + uint64_t total_weight = 0; size_t clusters_size; const envoy_config_route_v3_WeightedCluster_ClusterWeight* const* clusters = envoy_config_route_v3_WeightedCluster_clusters(weighted_clusters_proto, @@ -874,12 +876,15 @@ absl::optional RouteActionParse( } else { cluster.weight = google_protobuf_UInt32Value_value(weight_proto); if (cluster.weight == 0) continue; + total_weight += cluster.weight; } // Add entry to WeightedClusters. action_weighted_clusters.emplace_back(std::move(cluster)); } if (action_weighted_clusters.empty()) { errors->AddError("no valid clusters specified"); + } else if (total_weight > std::numeric_limits::max()) { + errors->AddError("sum of cluster weights exceeds uint32 max"); } route_action.action = std::move(action_weighted_clusters); } else if (XdsRlsEnabled() && diff --git a/test/core/xds/xds_route_config_resource_type_test.cc b/test/core/xds/xds_route_config_resource_type_test.cc index 845c9ae28fb..424c3df5b37 100644 --- a/test/core/xds/xds_route_config_resource_type_test.cc +++ b/test/core/xds/xds_route_config_resource_type_test.cc @@ -14,7 +14,10 @@ // limitations under the License. // +#include + #include +#include #include #include #include @@ -1627,6 +1630,36 @@ TEST_F(WeightedClusterTest, Invalid) { << decode_result.resource.status(); } +TEST_F(WeightedClusterTest, TotalWeightExceedsUint32Max) { + RouteConfiguration route_config; + route_config.set_name("foo"); + auto* vhost = route_config.add_virtual_hosts(); + vhost->add_domains("*"); + auto* route_proto = vhost->add_routes(); + route_proto->mutable_match()->set_prefix(""); + auto* weighted_clusters_proto = + route_proto->mutable_route()->mutable_weighted_clusters(); + auto* cluster_weight_proto = weighted_clusters_proto->add_clusters(); + cluster_weight_proto->set_name("cluster1"); + cluster_weight_proto->mutable_weight()->set_value( + std::numeric_limits::max()); + cluster_weight_proto = weighted_clusters_proto->add_clusters(); + cluster_weight_proto->set_name("cluster2"); + cluster_weight_proto->mutable_weight()->set_value(1); + std::string serialized_resource; + ASSERT_TRUE(route_config.SerializeToString(&serialized_resource)); + auto* resource_type = XdsRouteConfigResourceType::Get(); + auto decode_result = + resource_type->Decode(decode_context_, serialized_resource); + EXPECT_EQ(decode_result.resource.status().code(), + absl::StatusCode::kInvalidArgument); + EXPECT_EQ(decode_result.resource.status().message(), + "errors validating RouteConfiguration resource: [" + "field:virtual_hosts[0].routes[0].route.weighted_clusters " + "error:sum of cluster weights exceeds uint32 max]") + << decode_result.resource.status(); +} + // // RLS tests // diff --git a/test/cpp/end2end/xds/xds_routing_end2end_test.cc b/test/cpp/end2end/xds/xds_routing_end2end_test.cc index ed3106ae421..c19eeeb1755 100644 --- a/test/cpp/end2end/xds/xds_routing_end2end_test.cc +++ b/test/cpp/end2end/xds/xds_routing_end2end_test.cc @@ -952,10 +952,6 @@ TEST_P(LdsRdsTest, XdsRoutingWeightedCluster) { route1->mutable_route()->mutable_weighted_clusters()->add_clusters(); weighted_cluster3->set_name(kNotUsedClusterName); weighted_cluster3->mutable_weight()->set_value(0); - route1->mutable_route() - ->mutable_weighted_clusters() - ->mutable_total_weight() - ->set_value(kWeight75 + kWeight25); auto* default_route = new_route_config.mutable_virtual_hosts(0)->add_routes(); default_route->mutable_match()->set_prefix(""); default_route->mutable_route()->set_cluster(kDefaultClusterName); @@ -984,6 +980,88 @@ TEST_P(LdsRdsTest, XdsRoutingWeightedCluster) { ::testing::DoubleNear(kWeight25Percent, kErrorTolerance)); } +TEST_P(LdsRdsTest, XdsRoutingWeightedClusterNoIntegerOverflow) { + CreateAndStartBackends(3); + const char* kNewCluster1Name = "new_cluster_1"; + const char* kNewEdsService1Name = "new_eds_service_name_1"; + const char* kNewCluster2Name = "new_cluster_2"; + const char* kNewEdsService2Name = "new_eds_service_name_2"; + const size_t kNumEchoRpcs = 10; // RPCs that will go to a fixed backend. + const uint32_t kWeight1 = std::numeric_limits::max() / 3; + const uint32_t kWeight2 = std::numeric_limits::max() - kWeight1; + const double kErrorTolerance = 0.05; + const double kWeight1Percent = + static_cast(kWeight1) / std::numeric_limits::max(); + const double kWeight2Percent = + static_cast(kWeight2) / std::numeric_limits::max(); + const size_t kNumEcho1Rpcs = + ComputeIdealNumRpcs(kWeight2Percent, kErrorTolerance); + // Populate new EDS resources. + EdsResourceArgs args({ + {"locality0", CreateEndpointsForBackends(0, 1)}, + }); + EdsResourceArgs args1({ + {"locality0", CreateEndpointsForBackends(1, 2)}, + }); + EdsResourceArgs args2({ + {"locality0", CreateEndpointsForBackends(2, 3)}, + }); + balancer_->ads_service()->SetEdsResource(BuildEdsResource(args)); + balancer_->ads_service()->SetEdsResource( + BuildEdsResource(args1, kNewEdsService1Name)); + balancer_->ads_service()->SetEdsResource( + BuildEdsResource(args2, kNewEdsService2Name)); + // Populate new CDS resources. + Cluster new_cluster1 = default_cluster_; + new_cluster1.set_name(kNewCluster1Name); + new_cluster1.mutable_eds_cluster_config()->set_service_name( + kNewEdsService1Name); + balancer_->ads_service()->SetCdsResource(new_cluster1); + Cluster new_cluster2 = default_cluster_; + new_cluster2.set_name(kNewCluster2Name); + new_cluster2.mutable_eds_cluster_config()->set_service_name( + kNewEdsService2Name); + balancer_->ads_service()->SetCdsResource(new_cluster2); + // Populating Route Configurations for LDS. + RouteConfiguration new_route_config = default_route_config_; + auto* route1 = new_route_config.mutable_virtual_hosts(0)->mutable_routes(0); + route1->mutable_match()->set_prefix("/grpc.testing.EchoTest1Service/"); + auto* weighted_cluster1 = + route1->mutable_route()->mutable_weighted_clusters()->add_clusters(); + weighted_cluster1->set_name(kNewCluster1Name); + weighted_cluster1->mutable_weight()->set_value(kWeight1); + auto* weighted_cluster2 = + route1->mutable_route()->mutable_weighted_clusters()->add_clusters(); + weighted_cluster2->set_name(kNewCluster2Name); + weighted_cluster2->mutable_weight()->set_value(kWeight2); + auto* default_route = new_route_config.mutable_virtual_hosts(0)->add_routes(); + default_route->mutable_match()->set_prefix(""); + default_route->mutable_route()->set_cluster(kDefaultClusterName); + SetRouteConfiguration(balancer_.get(), new_route_config); + WaitForAllBackends(DEBUG_LOCATION, 0, 1); + WaitForAllBackends(DEBUG_LOCATION, 1, 3, /*check_status=*/nullptr, + WaitForBackendOptions(), + RpcOptions().set_rpc_service(SERVICE_ECHO1)); + CheckRpcSendOk(DEBUG_LOCATION, kNumEchoRpcs); + CheckRpcSendOk(DEBUG_LOCATION, kNumEcho1Rpcs, + RpcOptions().set_rpc_service(SERVICE_ECHO1)); + // Make sure RPCs all go to the correct backend. + EXPECT_EQ(kNumEchoRpcs, backends_[0]->backend_service()->request_count()); + EXPECT_EQ(0, backends_[0]->backend_service1()->request_count()); + EXPECT_EQ(0, backends_[1]->backend_service()->request_count()); + const int weight1_request_count = + backends_[1]->backend_service1()->request_count(); + EXPECT_EQ(0, backends_[2]->backend_service()->request_count()); + const int weight2_request_count = + backends_[2]->backend_service1()->request_count(); + gpr_log(GPR_INFO, "target1 received %d rpcs and target2 received %d rpcs", + weight1_request_count, weight2_request_count); + EXPECT_THAT(static_cast(weight1_request_count) / kNumEcho1Rpcs, + ::testing::DoubleNear(kWeight1Percent, kErrorTolerance)); + EXPECT_THAT(static_cast(weight2_request_count) / kNumEcho1Rpcs, + ::testing::DoubleNear(kWeight2Percent, kErrorTolerance)); +} + TEST_P(LdsRdsTest, RouteActionWeightedTargetDefaultRoute) { CreateAndStartBackends(3); const char* kNewCluster1Name = "new_cluster_1"; @@ -1035,10 +1113,6 @@ TEST_P(LdsRdsTest, RouteActionWeightedTargetDefaultRoute) { route1->mutable_route()->mutable_weighted_clusters()->add_clusters(); weighted_cluster2->set_name(kNewCluster2Name); weighted_cluster2->mutable_weight()->set_value(kWeight25); - route1->mutable_route() - ->mutable_weighted_clusters() - ->mutable_total_weight() - ->set_value(kWeight75 + kWeight25); SetRouteConfiguration(balancer_.get(), new_route_config); WaitForAllBackends(DEBUG_LOCATION, 1, 3); CheckRpcSendOk(DEBUG_LOCATION, kNumEchoRpcs); @@ -1124,10 +1198,6 @@ TEST_P(LdsRdsTest, XdsRoutingWeightedClusterUpdateWeights) { route1->mutable_route()->mutable_weighted_clusters()->add_clusters(); weighted_cluster2->set_name(kNewCluster2Name); weighted_cluster2->mutable_weight()->set_value(kWeight25); - route1->mutable_route() - ->mutable_weighted_clusters() - ->mutable_total_weight() - ->set_value(kWeight75 + kWeight25); auto* default_route = new_route_config.mutable_virtual_hosts(0)->add_routes(); default_route->mutable_match()->set_prefix(""); default_route->mutable_route()->set_cluster(kDefaultClusterName); @@ -1264,10 +1334,6 @@ TEST_P(LdsRdsTest, XdsRoutingWeightedClusterUpdateClusters) { route1->mutable_route()->mutable_weighted_clusters()->add_clusters(); weighted_cluster2->set_name(kDefaultClusterName); weighted_cluster2->mutable_weight()->set_value(kWeight25); - route1->mutable_route() - ->mutable_weighted_clusters() - ->mutable_total_weight() - ->set_value(kWeight75 + kWeight25); auto* default_route = new_route_config.mutable_virtual_hosts(0)->add_routes(); default_route->mutable_match()->set_prefix(""); default_route->mutable_route()->set_cluster(kDefaultClusterName);