From e5a37d59faa750345ef4f14cbc53049581984b5f Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 11 Jan 2023 08:03:55 -0800 Subject: [PATCH] xDS: don't NACK RouteConfig with a VirtualHost containing no valid routes (#32069) --- .../lb_policy/xds/xds_cluster_manager.cc | 12 ----------- .../resolver/xds/xds_resolver.cc | 12 +++++------ src/core/ext/xds/xds_route_config.cc | 4 ---- .../xds_route_config_resource_type_test.cc | 19 ------------------ .../end2end/xds/xds_routing_end2end_test.cc | 20 +++++++++++++++---- 5 files changed, 22 insertions(+), 45 deletions(-) diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc index ab423053299..d0537c15d33 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc @@ -101,8 +101,6 @@ class XdsClusterManagerLbConfig : public LoadBalancingPolicy::Config { } static const JsonLoaderInterface* JsonLoader(const JsonArgs&); - void JsonPostLoad(const Json& json, const JsonArgs&, - ValidationErrors* errors); private: std::map cluster_map_; @@ -666,16 +664,6 @@ const JsonLoaderInterface* XdsClusterManagerLbConfig::JsonLoader( return loader; } -void XdsClusterManagerLbConfig::JsonPostLoad(const Json&, const JsonArgs&, - ValidationErrors* errors) { - if (cluster_map_.empty()) { - ValidationErrors::ScopedField field(errors, ".children"); - if (!errors->FieldHasErrors()) { - errors->AddError("no valid children configured"); - } - } -} - class XdsClusterManagerLbFactory : public LoadBalancingPolicyFactory { public: OrphanablePtr CreateLoadBalancingPolicy( 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 5ade1ece0e9..becdbb51b76 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 @@ -373,7 +373,7 @@ class XdsResolver : public Resolver { std::string route_config_name_; RouteConfigWatcher* route_config_watcher_ = nullptr; - XdsRouteConfigResource::VirtualHost current_virtual_host_; + absl::optional current_virtual_host_; std::map cluster_specifier_plugin_map_; @@ -443,8 +443,8 @@ XdsResolver::XdsConfigSelector::XdsConfigSelector( // weighted_cluster_state field points to the memory in the route field, so // moving the entry in a reallocation will cause the string_view to point to // invalid data. - route_table_.reserve(resolver_->current_virtual_host_.routes.size()); - for (auto& route : resolver_->current_virtual_host_.routes) { + route_table_.reserve(resolver_->current_virtual_host_->routes.size()); + for (auto& route : resolver_->current_virtual_host_->routes) { if (GRPC_TRACE_FLAG_ENABLED(grpc_xds_resolver_trace)) { gpr_log(GPR_INFO, "[xds_resolver %p] XdsConfigSelector %p: route: %s", resolver_.get(), this, route.ToString().c_str()); @@ -598,7 +598,7 @@ XdsResolver::XdsConfigSelector::CreateMethodConfig( static_cast(resolver_->xds_client_->bootstrap()) .http_filter_registry(), resolver_->current_listener_.http_filters, - resolver_->current_virtual_host_, route, cluster_weight, + resolver_->current_virtual_host_.value(), route, cluster_weight, resolver_->args_); if (!result.ok()) return result.status(); for (const auto& p : result->per_filter_configs) { @@ -999,7 +999,7 @@ void XdsResolver::OnResourceDoesNotExist(std::string context) { if (xds_client_ == nullptr) { return; } - current_virtual_host_.routes.clear(); + current_virtual_host_.reset(); Result result; result.addresses.emplace(); result.service_config = ServiceConfigImpl::Create(args_, "{}"); @@ -1051,7 +1051,7 @@ XdsResolver::CreateServiceConfig() { } void XdsResolver::GenerateResult() { - if (current_virtual_host_.routes.empty()) return; + if (!current_virtual_host_.has_value()) return; // First create XdsConfigSelector, which may add new entries to the cluster // state map, and then CreateServiceConfig for LB policies. absl::Status status; diff --git a/src/core/ext/xds/xds_route_config.cc b/src/core/ext/xds/xds_route_config.cc index cb48d930dc3..c076f03407e 100644 --- a/src/core/ext/xds/xds_route_config.cc +++ b/src/core/ext/xds/xds_route_config.cc @@ -1056,7 +1056,6 @@ XdsRouteConfigResource XdsRouteConfigResource::Parse( } // Parse routes. ValidationErrors::ScopedField field2(errors, ".routes"); - const size_t original_error_size = errors->size(); size_t num_routes; const envoy_config_route_v3_Route* const* routes = envoy_config_route_v3_VirtualHost_routes(virtual_hosts[i], &num_routes); @@ -1067,9 +1066,6 @@ XdsRouteConfigResource XdsRouteConfigResource::Parse( &cluster_specifier_plugins_not_seen, errors); if (route.has_value()) vhost.routes.emplace_back(std::move(*route)); } - if (errors->size() == original_error_size && vhost.routes.empty()) { - errors->AddError("no valid routes in VirtualHost"); - } } // For cluster specifier plugins that were not used in any route action, // delete them from the update, since they will never be used. 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 29019524ea8..f59dd250421 100644 --- a/test/core/xds/xds_route_config_resource_type_test.cc +++ b/test/core/xds/xds_route_config_resource_type_test.cc @@ -274,25 +274,6 @@ TEST_F(VirtualHostTest, NoDomainsSpecified) { << decode_result.resource.status(); } -TEST_F(VirtualHostTest, NoRoutesInVirtualHost) { - RouteConfiguration route_config; - route_config.set_name("foo"); - auto* vhost = route_config.add_virtual_hosts(); - vhost->add_domains("*"); - 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 " - "error:no valid routes in VirtualHost]") - << decode_result.resource.status(); -} - // // typed_per_filter_config tests // diff --git a/test/cpp/end2end/xds/xds_routing_end2end_test.cc b/test/cpp/end2end/xds/xds_routing_end2end_test.cc index 72bf32e1db3..ed3106ae421 100644 --- a/test/cpp/end2end/xds/xds_routing_end2end_test.cc +++ b/test/cpp/end2end/xds/xds_routing_end2end_test.cc @@ -482,13 +482,25 @@ TEST_P(LdsRdsTest, NoMatchingRoute) { EXPECT_EQ(response_state->state, AdsServiceImpl::ResponseState::ACKED); } +TEST_P(LdsRdsTest, EmptyRouteList) { + RouteConfiguration route_config = default_route_config_; + route_config.mutable_virtual_hosts(0)->clear_routes(); + SetRouteConfiguration(balancer_.get(), route_config); + CheckRpcSendFailure(DEBUG_LOCATION, StatusCode::UNAVAILABLE, + "No matching route found in xDS route config"); + // Do a bit of polling, to allow the ACK to get to the ADS server. + channel_->WaitForConnected(grpc_timeout_milliseconds_to_deadline(100)); + auto response_state = RouteConfigurationResponseState(balancer_.get()); + ASSERT_TRUE(response_state.has_value()); + EXPECT_EQ(response_state->state, AdsServiceImpl::ResponseState::ACKED); +} + // Testing just one example of an invalid resource here. // Unit tests for XdsRouteConfigResourceType have exhaustive tests for all // of the invalid cases. TEST_P(LdsRdsTest, NacksInvalidRouteConfig) { RouteConfiguration route_config = default_route_config_; - auto* route1 = route_config.mutable_virtual_hosts(0)->mutable_routes(0); - route1->mutable_match()->set_prefix("grpc.testing.EchoTest1Service/"); + route_config.mutable_virtual_hosts(0)->mutable_routes(0)->clear_match(); SetRouteConfiguration(balancer_.get(), route_config); const auto response_state = WaitForRdsNack(DEBUG_LOCATION); ASSERT_TRUE(response_state.has_value()) << "timed out waiting for NACK"; @@ -505,8 +517,8 @@ TEST_P(LdsRdsTest, NacksInvalidRouteConfig) { "field:api_listener.api_listener.value[" "envoy.extensions.filters.network.http_connection_manager.v3" ".HttpConnectionManager].route_config.", - "virtual_hosts[0].routes " - "error:no valid routes in VirtualHost]]")); + "virtual_hosts[0].routes[0].match " + "error:field not present]]")); } // Tests that LDS client should fail RPCs with UNAVAILABLE status code if the