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

src/core/ext/filters/client_channel/client_channel.cc

@@ -1446,8 +1446,8 @@ void ClientChannel::UpdateServiceConfigInDataPlaneLocked() {
     config_selector =
         MakeRefCounted<DefaultConfigSelector>(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<ClientChannelServiceConfigCallData>(
-            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<ClientChannelMethodParsedConfig*>(
         service_config_call_data->GetMethodParsedConfig(

src/core/ext/filters/client_channel/config_selector.h

@@ -24,7 +24,7 @@
 #include <utility>
 #include <vector>
 
-#include "absl/status/status.h"
+#include "absl/status/statusor.h"
 #include "absl/strings/string_view.h"
 
 #include <grpc/impl/codegen/grpc_types.h>
@@ -72,8 +72,6 @@ class ConfigSelector : public RefCounted<ConfigSelector> {
   };
 
   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<ConfigSelector> {
 
   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<ConfigSelector> {
   // 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<const grpc_channel_filter*> 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<CallConfig> GetCallConfig(GetCallConfigArgs args) = 0;
 
   grpc_arg MakeChannelArg() const;
   static RefCountedPtr<ConfigSelector> GetFromChannelArgs(
@@ -119,6 +111,11 @@ class ConfigSelector : public RefCounted<ConfigSelector> {
                                 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<CallConfig> 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<ServiceConfig> 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

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<CallConfig> GetCallConfig(GetCallConfigArgs args) override;
 
     std::vector<const grpc_channel_filter*> 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<uint64_t> HeaderHashHelper(
   return XXH64(header_value->data(), header_value->size(), 0);
 }
 
-ConfigSelector::CallConfig XdsResolver::XdsConfigSelector::GetCallConfig(
-    GetCallConfigArgs args) {
+absl::StatusOr<ConfigSelector::CallConfig>
+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<XdsRouteConfigResource::Route::RouteAction>(
           &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<ServiceConfig> method_config;

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<const FailConfigSelector*>(other)->status_;
     }
-    CallConfig GetCallConfig(GetCallConfigArgs /*args*/) override {
-      CallConfig config;
-      config.status = status_;
-      return config;
+    absl::StatusOr<CallConfig> GetCallConfig(
+        GetCallConfigArgs /*args*/) override {
+      return status_;
     }
 
    private:

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_;