From ddfa85f42b96428bfeef4c97a7e9a173774acf9b Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 9 Nov 2022 14:29:22 -0800 Subject: [PATCH] xDS: fix error message when the request does not match any route (#31593) * improve error message when there is no matching route * clean up ConfigSelector API * clang-format * fix client_lb_end2end_test * clang-format --- .../filters/client_channel/client_channel.cc | 16 ++++----- .../filters/client_channel/config_selector.h | 33 +++++++++---------- .../resolver/xds/xds_resolver.cc | 21 ++++-------- test/cpp/end2end/client_lb_end2end_test.cc | 7 ++-- .../end2end/xds/xds_routing_end2end_test.cc | 16 +++++++++ 5 files changed, 48 insertions(+), 45 deletions(-) diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 6235de48dc2..8decf64b424 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -1446,8 +1446,8 @@ void ClientChannel::UpdateServiceConfigInDataPlaneLocked() { config_selector = MakeRefCounted(saved_service_config_); } - ChannelArgs new_args = config_selector->ModifyChannelArgs( - channel_args_.SetObject(this).SetObject(service_config)); + ChannelArgs new_args = + channel_args_.SetObject(this).SetObject(service_config); bool enable_retries = !new_args.WantMinimalStack() && new_args.GetBool(GRPC_ARG_ENABLE_RETRIES).value_or(true); @@ -2172,11 +2172,11 @@ grpc_error_handle ClientChannel::CallData::ApplyServiceConfigToCallLocked( ConfigSelector* config_selector = chand->config_selector_.get(); if (config_selector != nullptr) { // Use the ConfigSelector to determine the config for the call. - ConfigSelector::CallConfig call_config = + auto call_config = config_selector->GetCallConfig({&path_, initial_metadata, arena_}); - if (!call_config.status.ok()) { + if (!call_config.ok()) { return absl_status_to_grpc_error(MaybeRewriteIllegalStatusCode( - std::move(call_config.status), "ConfigSelector")); + call_config.status(), "ConfigSelector")); } // Create a ClientChannelServiceConfigCallData for the call. This stores // a ref to the ServiceConfig and caches the right set of parsed configs @@ -2185,9 +2185,9 @@ grpc_error_handle ClientChannel::CallData::ApplyServiceConfigToCallLocked( // below us in the stack, and it will be cleaned up when the call ends. auto* service_config_call_data = arena_->New( - std::move(call_config.service_config), call_config.method_configs, - std::move(call_config.call_attributes), - call_config.call_dispatch_controller, call_context_); + std::move(call_config->service_config), call_config->method_configs, + std::move(call_config->call_attributes), + call_config->call_dispatch_controller, call_context_); // Apply our own method params to the call. auto* method_params = static_cast( service_config_call_data->GetMethodParsedConfig( diff --git a/src/core/ext/filters/client_channel/config_selector.h b/src/core/ext/filters/client_channel/config_selector.h index 66588a2872e..5112a5a466f 100644 --- a/src/core/ext/filters/client_channel/config_selector.h +++ b/src/core/ext/filters/client_channel/config_selector.h @@ -24,7 +24,7 @@ #include #include -#include "absl/status/status.h" +#include "absl/status/statusor.h" #include "absl/strings/string_view.h" #include @@ -72,8 +72,6 @@ class ConfigSelector : public RefCounted { }; struct CallConfig { - // Can be set to indicate the call should be failed. - absl::Status status; // The per-method parsed configs that will be passed to // ServiceConfigCallData. const ServiceConfigParser::ParsedConfigVector* method_configs = nullptr; @@ -90,10 +88,6 @@ class ConfigSelector : public RefCounted { virtual const char* name() const = 0; - // Will be called only if the two objects have the same name, so - // subclasses can be free to safely down-cast the argument. - virtual bool Equals(const ConfigSelector* other) const = 0; - static bool Equals(const ConfigSelector* cs1, const ConfigSelector* cs2) { if (cs1 == nullptr) return cs2 == nullptr; if (cs2 == nullptr) return false; @@ -104,12 +98,10 @@ class ConfigSelector : public RefCounted { // The channel will call this when the resolver returns a new ConfigSelector // to determine what set of dynamic filters will be configured. virtual std::vector GetFilters() { return {}; } - // Modifies channel args to be passed to the dynamic filter stack. - virtual ChannelArgs ModifyChannelArgs(const ChannelArgs& args) { - return args; - } - virtual CallConfig GetCallConfig(GetCallConfigArgs args) = 0; + // Returns the call config to use for the call, or a status to fail + // the call with. + virtual absl::StatusOr GetCallConfig(GetCallConfigArgs args) = 0; grpc_arg MakeChannelArg() const; static RefCountedPtr GetFromChannelArgs( @@ -119,6 +111,11 @@ class ConfigSelector : public RefCounted { const ConfigSelector* b) { return QsortCompare(a, b); } + + private: + // Will be called only if the two objects have the same name, so + // subclasses can be free to safely down-cast the argument. + virtual bool Equals(const ConfigSelector* other) const = 0; }; // Default ConfigSelector that gets the MethodConfig from the service config. @@ -134,11 +131,7 @@ class DefaultConfigSelector : public ConfigSelector { const char* name() const override { return "default"; } - // Only comparing the ConfigSelector itself, not the underlying - // service config, so we always return true. - bool Equals(const ConfigSelector* /*other*/) const override { return true; } - - CallConfig GetCallConfig(GetCallConfigArgs args) override { + absl::StatusOr GetCallConfig(GetCallConfigArgs args) override { CallConfig call_config; call_config.method_configs = service_config_->GetMethodParsedConfigVector(*args.path); @@ -146,10 +139,14 @@ class DefaultConfigSelector : public ConfigSelector { return call_config; } + // Only comparing the ConfigSelector itself, not the underlying + // service config, so we always return true. + bool Equals(const ConfigSelector* /*other*/) const override { return true; } + private: RefCountedPtr service_config_; }; } // namespace grpc_core -#endif /* GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_CONFIG_SELECTOR_H */ +#endif // GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_CONFIG_SELECTOR_H 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 5f9b3927149..27ecf833c1e 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 @@ -309,14 +309,12 @@ class XdsResolver : public Resolver { clusters_ == other_xds->clusters_; } - CallConfig GetCallConfig(GetCallConfigArgs args) override; + absl::StatusOr GetCallConfig(GetCallConfigArgs args) override; std::vector GetFilters() override { return filters_; } - ChannelArgs ModifyChannelArgs(const ChannelArgs& args) override; - private: struct Route { struct ClusterWeightState { @@ -627,11 +625,6 @@ XdsResolver::XdsConfigSelector::CreateMethodConfig( return nullptr; } -ChannelArgs XdsResolver::XdsConfigSelector::ModifyChannelArgs( - const ChannelArgs& args) { - return args; -} - void XdsResolver::XdsConfigSelector::MaybeAddCluster(const std::string& name) { if (clusters_.find(name) == clusters_.end()) { auto it = resolver_->cluster_state_map_.find(name); @@ -667,13 +660,14 @@ absl::optional HeaderHashHelper( return XXH64(header_value->data(), header_value->size(), 0); } -ConfigSelector::CallConfig XdsResolver::XdsConfigSelector::GetCallConfig( - GetCallConfigArgs args) { +absl::StatusOr +XdsResolver::XdsConfigSelector::GetCallConfig(GetCallConfigArgs args) { auto route_index = XdsRouting::GetRouteForRequest( RouteListIterator(&route_table_), StringViewFromSlice(*args.path), args.initial_metadata); if (!route_index.has_value()) { - return CallConfig(); + return absl::UnavailableError( + "No matching route found in xDS route config"); } auto& entry = route_table_[*route_index]; // Found a route match @@ -681,10 +675,7 @@ ConfigSelector::CallConfig XdsResolver::XdsConfigSelector::GetCallConfig( absl::get_if( &entry.route.action); if (route_action == nullptr) { - CallConfig call_config; - call_config.status = - absl::UnavailableError("Matching route has inappropriate action"); - return call_config; + return absl::UnavailableError("Matching route has inappropriate action"); } std::string cluster_name; RefCountedPtr method_config; diff --git a/test/cpp/end2end/client_lb_end2end_test.cc b/test/cpp/end2end/client_lb_end2end_test.cc index 359713e7082..a19e29b50c5 100644 --- a/test/cpp/end2end/client_lb_end2end_test.cc +++ b/test/cpp/end2end/client_lb_end2end_test.cc @@ -2798,10 +2798,9 @@ TEST_F(ControlPlaneStatusRewritingTest, RewritesFromConfigSelector) { bool Equals(const ConfigSelector* other) const override { return status_ == static_cast(other)->status_; } - CallConfig GetCallConfig(GetCallConfigArgs /*args*/) override { - CallConfig config; - config.status = status_; - return config; + absl::StatusOr GetCallConfig( + GetCallConfigArgs /*args*/) override { + return status_; } private: diff --git a/test/cpp/end2end/xds/xds_routing_end2end_test.cc b/test/cpp/end2end/xds/xds_routing_end2end_test.cc index 25a8a3861f6..dff0c9bf5b8 100644 --- a/test/cpp/end2end/xds/xds_routing_end2end_test.cc +++ b/test/cpp/end2end/xds/xds_routing_end2end_test.cc @@ -215,6 +215,22 @@ TEST_P(LdsRdsTest, ChooseLastRoute) { EXPECT_EQ(response_state->state, AdsServiceImpl::ResponseState::ACKED); } +TEST_P(LdsRdsTest, NoMatchingRoute) { + RouteConfiguration route_config = default_route_config_; + route_config.mutable_virtual_hosts(0) + ->mutable_routes(0) + ->mutable_match() + ->set_prefix("/unknown/method"); + 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); +} + // Tests that LDS client should ignore route which has query_parameters. TEST_P(LdsRdsTest, RouteMatchHasQueryParameters) { RouteConfiguration route_config = default_route_config_;