From 43c8cdd2e99ce4694b994e1f1cc43e8ce49e71ba Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 9 Nov 2022 14:23:29 -0800 Subject: [PATCH] xDS: fix bug when a cluster specifier plugin was not used in all vhosts (#31583) --- src/core/ext/xds/xds_route_config.cc | 22 +++--- test/cpp/end2end/xds/xds_rls_end2end_test.cc | 72 ++++++++++++++++++++ 2 files changed, 83 insertions(+), 11 deletions(-) diff --git a/src/core/ext/xds/xds_route_config.cc b/src/core/ext/xds/xds_route_config.cc index af1b3c856c8..280fbf470aa 100644 --- a/src/core/ext/xds/xds_route_config.cc +++ b/src/core/ext/xds/xds_route_config.cc @@ -974,6 +974,12 @@ absl::StatusOr XdsRouteConfigResource::Parse( rds_update.cluster_specifier_plugin_map = std::move(*cluster_specifier_plugin_map); } + // Build a set of cluster_specifier_plugin configured to make sure each is + // actually referenced by a route action. + std::set cluster_specifier_plugins; + for (auto& plugin : rds_update.cluster_specifier_plugin_map) { + cluster_specifier_plugins.emplace(plugin.first); + } // Get the virtual hosts. size_t num_virtual_hosts; const envoy_config_route_v3_VirtualHost* const* virtual_hosts = @@ -1027,12 +1033,6 @@ absl::StatusOr XdsRouteConfigResource::Parse( if (num_routes < 1) { return absl::InvalidArgumentError("No route found in the virtual host."); } - // Build a set of cluster_specifier_plugin configured to make sure each is - // actually referenced by a route action. - std::set cluster_specifier_plugins; - for (auto& plugin : rds_update.cluster_specifier_plugin_map) { - cluster_specifier_plugins.emplace(plugin.first); - } // Loop over the whole list of routes for (size_t j = 0; j < num_routes; ++j) { const envoy_config_route_v3_RouteMatch* match = @@ -1096,11 +1096,11 @@ absl::StatusOr XdsRouteConfigResource::Parse( if (vhost.routes.empty()) { return absl::InvalidArgumentError("No valid routes specified."); } - // For plugins not used in route action, delete from the update to prevent - // further use. - for (auto& unused_plugin : cluster_specifier_plugins) { - rds_update.cluster_specifier_plugin_map.erase(std::string(unused_plugin)); - } + } + // For plugins not used in route action, delete from the update to prevent + // further use. + for (auto& unused_plugin : cluster_specifier_plugins) { + rds_update.cluster_specifier_plugin_map.erase(std::string(unused_plugin)); } return rds_update; } diff --git a/test/cpp/end2end/xds/xds_rls_end2end_test.cc b/test/cpp/end2end/xds/xds_rls_end2end_test.cc index 81caa0b3fc1..239ff8937c3 100644 --- a/test/cpp/end2end/xds/xds_rls_end2end_test.cc +++ b/test/cpp/end2end/xds/xds_rls_end2end_test.cc @@ -161,6 +161,78 @@ TEST_P(RlsTest, XdsRoutingClusterSpecifierPlugin) { EXPECT_EQ(kNumEchoRpcs, backends_[1]->backend_service()->request_count()); } +TEST_P(RlsTest, XdsRoutingClusterSpecifierPluginNotUsedInAllVhosts) { + ScopedExperimentalEnvVar env_var("GRPC_EXPERIMENTAL_XDS_RLS_LB"); + CreateAndStartBackends(2); + const char* kNewClusterName = "new_cluster"; + const char* kNewEdsServiceName = "new_eds_service_name"; + const size_t kNumEchoRpcs = 5; + // Populate new EDS resources. + EdsResourceArgs args({ + {"locality0", CreateEndpointsForBackends(0, 1)}, + }); + EdsResourceArgs args1({ + {"locality0", CreateEndpointsForBackends(1, 2)}, + }); + balancer_->ads_service()->SetEdsResource(BuildEdsResource(args)); + balancer_->ads_service()->SetEdsResource( + BuildEdsResource(args1, kNewEdsServiceName)); + // Populate new CDS resources. + Cluster new_cluster = default_cluster_; + new_cluster.set_name(kNewClusterName); + new_cluster.mutable_eds_cluster_config()->set_service_name( + kNewEdsServiceName); + balancer_->ads_service()->SetCdsResource(new_cluster); + // Prepare the RLSLookupConfig and configure all the keys; change route + // configurations to use cluster specifier plugin. + rls_server_->rls_service()->SetResponse( + BuildRlsRequest({{kRlsTestKey, kRlsTestValue}, + {kRlsHostKey, kServerName}, + {kRlsServiceKey, kRlsServiceValue}, + {kRlsMethodKey, kRlsMethodValue}, + {kRlsConstantKey, kRlsConstantValue}}), + BuildRlsResponse({kNewClusterName})); + RouteLookupConfig route_lookup_config; + auto* key_builder = route_lookup_config.add_grpc_keybuilders(); + auto* name = key_builder->add_names(); + name->set_service(kRlsServiceValue); + name->set_method(kRlsMethodValue); + auto* header = key_builder->add_headers(); + header->set_key(kRlsTestKey); + header->add_names(kRlsTestKey1); + header->add_names("key2"); + auto* extra_keys = key_builder->mutable_extra_keys(); + extra_keys->set_host(kRlsHostKey); + extra_keys->set_service(kRlsServiceKey); + extra_keys->set_method(kRlsMethodKey); + (*key_builder->mutable_constant_keys())[kRlsConstantKey] = kRlsConstantValue; + route_lookup_config.set_lookup_service( + absl::StrCat("localhost:", rls_server_->port())); + route_lookup_config.set_cache_size_bytes(5000); + RouteLookupClusterSpecifier rls; + *rls.mutable_route_lookup_config() = std::move(route_lookup_config); + RouteConfiguration new_route_config = default_route_config_; + auto* plugin = new_route_config.add_cluster_specifier_plugins(); + plugin->mutable_extension()->set_name(kRlsClusterSpecifierPluginInstanceName); + plugin->mutable_extension()->mutable_typed_config()->PackFrom(rls); + // Duplicate the virtual host, but with a different domain. + *new_route_config.add_virtual_hosts() = new_route_config.virtual_hosts(0); + new_route_config.mutable_virtual_hosts(1)->clear_domains(); + new_route_config.mutable_virtual_hosts(1)->add_domains("www.example.com"); + // In the original virtual host, set the default route to use the RLS plugin. + auto* default_route = + new_route_config.mutable_virtual_hosts(0)->mutable_routes(0); + default_route->mutable_route()->set_cluster_specifier_plugin( + kRlsClusterSpecifierPluginInstanceName); + SetRouteConfiguration(balancer_.get(), new_route_config); + auto rpc_options = RpcOptions().set_metadata({{kRlsTestKey1, kRlsTestValue}}); + WaitForAllBackends(DEBUG_LOCATION, 1, 2, /*check_status=*/nullptr, + WaitForBackendOptions(), rpc_options); + CheckRpcSendOk(DEBUG_LOCATION, kNumEchoRpcs, rpc_options); + // Make sure RPCs all go to the correct backend. + EXPECT_EQ(kNumEchoRpcs, backends_[1]->backend_service()->request_count()); +} + TEST_P(RlsTest, XdsRoutingClusterSpecifierPluginNacksUndefinedSpecifier) { ScopedExperimentalEnvVar env_var("GRPC_EXPERIMENTAL_XDS_RLS_LB"); RouteConfiguration new_route_config = default_route_config_;