From f7ce3ee9d54e30cf8ce944b8b3c3f945eb856f64 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 12 Jun 2024 15:19:26 -0700 Subject: [PATCH] [call v3] add dynamic filter support to client channel (#36877) Also made some minor improvements to the `ConfigSelector` API. Closes #36877 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/36877 from markdroth:client_channel_v3_dynamic_filters 6a539fe3200dd81b8ca6fd1aa3955ecc8f75666a PiperOrigin-RevId: 642755276 --- CMakeLists.txt | 2 - Makefile | 1 - Package.swift | 1 - build_autogenerated.yaml | 2 - config.m4 | 1 - config.w32 | 1 - gRPC-Core.podspec | 1 - grpc.gemspec | 1 - package.xml | 1 - src/core/BUILD | 6 +- src/core/client_channel/client_channel.cc | 7 +- src/core/client_channel/config_selector.cc | 60 ----------- src/core/client_channel/config_selector.h | 31 +++--- src/core/resolver/xds/xds_resolver.cc | 40 +++++-- src/core/xds/grpc/xds_http_fault_filter.cc | 4 + src/core/xds/grpc/xds_http_fault_filter.h | 1 + src/core/xds/grpc/xds_http_filters.h | 4 + src/core/xds/grpc/xds_http_rbac_filter.cc | 4 + src/core/xds/grpc/xds_http_rbac_filter.h | 1 + .../grpc/xds_http_stateful_session_filter.cc | 5 + .../grpc/xds_http_stateful_session_filter.h | 1 + src/python/grpcio/grpc_core_dependencies.py | 1 - .../client_channel/client_channel_test.cc | 102 +++++++++++++++++- test/cpp/end2end/client_lb_end2end_test.cc | 5 +- tools/doxygen/Doxyfile.c++.internal | 1 - tools/doxygen/Doxyfile.core.internal | 1 - 26 files changed, 180 insertions(+), 105 deletions(-) delete mode 100644 src/core/client_channel/config_selector.cc diff --git a/CMakeLists.txt b/CMakeLists.txt index c379a81ef28..00d7dec71a4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1853,7 +1853,6 @@ add_library(grpc src/core/client_channel/client_channel_filter.cc src/core/client_channel/client_channel_plugin.cc src/core/client_channel/client_channel_service_config.cc - src/core/client_channel/config_selector.cc src/core/client_channel/dynamic_filters.cc src/core/client_channel/global_subchannel_pool.cc src/core/client_channel/load_balanced_call_destination.cc @@ -2946,7 +2945,6 @@ add_library(grpc_unsecure src/core/client_channel/client_channel_filter.cc src/core/client_channel/client_channel_plugin.cc src/core/client_channel/client_channel_service_config.cc - src/core/client_channel/config_selector.cc src/core/client_channel/dynamic_filters.cc src/core/client_channel/global_subchannel_pool.cc src/core/client_channel/load_balanced_call_destination.cc diff --git a/Makefile b/Makefile index 1d39b0f27e9..aa1295bfa64 100644 --- a/Makefile +++ b/Makefile @@ -675,7 +675,6 @@ LIBGRPC_SRC = \ src/core/client_channel/client_channel_filter.cc \ src/core/client_channel/client_channel_plugin.cc \ src/core/client_channel/client_channel_service_config.cc \ - src/core/client_channel/config_selector.cc \ src/core/client_channel/dynamic_filters.cc \ src/core/client_channel/global_subchannel_pool.cc \ src/core/client_channel/load_balanced_call_destination.cc \ diff --git a/Package.swift b/Package.swift index 706e261a442..903fceaa748 100644 --- a/Package.swift +++ b/Package.swift @@ -136,7 +136,6 @@ let package = Package( "src/core/client_channel/client_channel_plugin.cc", "src/core/client_channel/client_channel_service_config.cc", "src/core/client_channel/client_channel_service_config.h", - "src/core/client_channel/config_selector.cc", "src/core/client_channel/config_selector.h", "src/core/client_channel/connector.h", "src/core/client_channel/dynamic_filters.cc", diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index fa157a9fbae..dad0979993b 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -1258,7 +1258,6 @@ libs: - src/core/client_channel/client_channel_filter.cc - src/core/client_channel/client_channel_plugin.cc - src/core/client_channel/client_channel_service_config.cc - - src/core/client_channel/config_selector.cc - src/core/client_channel/dynamic_filters.cc - src/core/client_channel/global_subchannel_pool.cc - src/core/client_channel/load_balanced_call_destination.cc @@ -2714,7 +2713,6 @@ libs: - src/core/client_channel/client_channel_filter.cc - src/core/client_channel/client_channel_plugin.cc - src/core/client_channel/client_channel_service_config.cc - - src/core/client_channel/config_selector.cc - src/core/client_channel/dynamic_filters.cc - src/core/client_channel/global_subchannel_pool.cc - src/core/client_channel/load_balanced_call_destination.cc diff --git a/config.m4 b/config.m4 index 7a6ce84d3d7..6b3d155e3fa 100644 --- a/config.m4 +++ b/config.m4 @@ -50,7 +50,6 @@ if test "$PHP_GRPC" != "no"; then src/core/client_channel/client_channel_filter.cc \ src/core/client_channel/client_channel_plugin.cc \ src/core/client_channel/client_channel_service_config.cc \ - src/core/client_channel/config_selector.cc \ src/core/client_channel/dynamic_filters.cc \ src/core/client_channel/global_subchannel_pool.cc \ src/core/client_channel/load_balanced_call_destination.cc \ diff --git a/config.w32 b/config.w32 index 657bdf0b37f..030bffccc0b 100644 --- a/config.w32 +++ b/config.w32 @@ -15,7 +15,6 @@ if (PHP_GRPC != "no") { "src\\core\\client_channel\\client_channel_filter.cc " + "src\\core\\client_channel\\client_channel_plugin.cc " + "src\\core\\client_channel\\client_channel_service_config.cc " + - "src\\core\\client_channel\\config_selector.cc " + "src\\core\\client_channel\\dynamic_filters.cc " + "src\\core\\client_channel\\global_subchannel_pool.cc " + "src\\core\\client_channel\\load_balanced_call_destination.cc " + diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index 1116516edbd..f23ac164ff6 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -255,7 +255,6 @@ Pod::Spec.new do |s| 'src/core/client_channel/client_channel_plugin.cc', 'src/core/client_channel/client_channel_service_config.cc', 'src/core/client_channel/client_channel_service_config.h', - 'src/core/client_channel/config_selector.cc', 'src/core/client_channel/config_selector.h', 'src/core/client_channel/connector.h', 'src/core/client_channel/dynamic_filters.cc', diff --git a/grpc.gemspec b/grpc.gemspec index 91416b2ac1f..92494c37b7d 100644 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -142,7 +142,6 @@ Gem::Specification.new do |s| s.files += %w( src/core/client_channel/client_channel_plugin.cc ) s.files += %w( src/core/client_channel/client_channel_service_config.cc ) s.files += %w( src/core/client_channel/client_channel_service_config.h ) - s.files += %w( src/core/client_channel/config_selector.cc ) s.files += %w( src/core/client_channel/config_selector.h ) s.files += %w( src/core/client_channel/connector.h ) s.files += %w( src/core/client_channel/dynamic_filters.cc ) diff --git a/package.xml b/package.xml index 523bc3aa67b..3d88d93e590 100644 --- a/package.xml +++ b/package.xml @@ -124,7 +124,6 @@ - diff --git a/src/core/BUILD b/src/core/BUILD index 76d3eb99400..f6b6b13ea10 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -3295,9 +3295,6 @@ grpc_cc_library( grpc_cc_library( name = "config_selector", - srcs = [ - "client_channel/config_selector.cc", - ], hdrs = [ "client_channel/config_selector.h", ], @@ -3313,9 +3310,11 @@ grpc_cc_library( "channel_fwd", "client_channel_internal_header", "grpc_service_config", + "interception_chain", "metadata_batch", "ref_counted", "slice", + "unique_type_name", "useful", "//:gpr_public_hdrs", "//:grpc_public_hdrs", @@ -5215,6 +5214,7 @@ grpc_cc_library( "grpc_tls_credentials", "grpc_transport_chttp2_client_connector", "init_internally", + "interception_chain", "iomgr_fwd", "json", "json_args", diff --git a/src/core/client_channel/client_channel.cc b/src/core/client_channel/client_channel.cc index 004f87bbba0..87a66c75d91 100644 --- a/src/core/client_channel/client_channel.cc +++ b/src/core/client_channel/client_channel.cc @@ -1195,14 +1195,17 @@ void ClientChannel::UpdateServiceConfigInDataPlaneLocked() { } CoreConfiguration::Get().channel_init().AddToInterceptionChainBuilder( GRPC_CLIENT_CHANNEL, builder); - // TODO(roth): add filters returned by config selector - // Create call destination. + // Add filters returned by the config selector (e.g., xDS HTTP filters). + config_selector->AddFilters(builder); + // TODO(roth, ctiller): When we implement the retry interceptor, that + // needs to be added *after* the filters added by the config selector. const bool enable_retries = !channel_args_.WantMinimalStack() && channel_args_.GetBool(GRPC_ARG_ENABLE_RETRIES).value_or(true); if (enable_retries) { Crash("call v3 stack does not yet support retries"); } + // Create call destination. auto top_of_stack_call_destination = builder.Build(call_destination_); // Send result to data plane. if (!top_of_stack_call_destination.ok()) { diff --git a/src/core/client_channel/config_selector.cc b/src/core/client_channel/config_selector.cc deleted file mode 100644 index 9264e4c1618..00000000000 --- a/src/core/client_channel/config_selector.cc +++ /dev/null @@ -1,60 +0,0 @@ -// -// Copyright 2020 gRPC authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// - -#include - -#include "src/core/client_channel/config_selector.h" - -#include "src/core/lib/channel/channel_args.h" -#include "src/core/util/useful.h" - -namespace grpc_core { - -namespace { - -void* ConfigSelectorArgCopy(void* p) { - ConfigSelector* config_selector = static_cast(p); - config_selector->Ref().release(); - return p; -} - -void ConfigSelectorArgDestroy(void* p) { - ConfigSelector* config_selector = static_cast(p); - config_selector->Unref(); -} - -int ConfigSelectorArgCmp(void* p, void* q) { return QsortCompare(p, q); } - -const grpc_arg_pointer_vtable kChannelArgVtable = { - ConfigSelectorArgCopy, ConfigSelectorArgDestroy, ConfigSelectorArgCmp}; - -} // namespace - -grpc_arg ConfigSelector::MakeChannelArg() const { - return grpc_channel_arg_pointer_create( - const_cast(GRPC_ARG_CONFIG_SELECTOR), - const_cast(this), &kChannelArgVtable); -} - -RefCountedPtr ConfigSelector::GetFromChannelArgs( - const grpc_channel_args& args) { - ConfigSelector* config_selector = - grpc_channel_args_find_pointer(&args, - GRPC_ARG_CONFIG_SELECTOR); - return config_selector != nullptr ? config_selector->Ref() : nullptr; -} - -} // namespace grpc_core diff --git a/src/core/client_channel/config_selector.h b/src/core/client_channel/config_selector.h index 0a0a6d54c80..d049ae95836 100644 --- a/src/core/client_channel/config_selector.h +++ b/src/core/client_channel/config_selector.h @@ -35,8 +35,10 @@ #include "src/core/lib/channel/channel_fwd.h" #include "src/core/lib/gprpp/ref_counted.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" +#include "src/core/lib/gprpp/unique_type_name.h" #include "src/core/lib/resource_quota/arena.h" #include "src/core/lib/slice/slice.h" +#include "src/core/lib/transport/interception_chain.h" #include "src/core/lib/transport/metadata_batch.h" #include "src/core/service_config/service_config.h" #include "src/core/util/useful.h" @@ -50,34 +52,32 @@ namespace grpc_core { // MethodConfig and provide input to LB policies on a per-call basis. class ConfigSelector : public RefCounted { public: - struct GetCallConfigArgs { - grpc_metadata_batch* initial_metadata; - Arena* arena; - ClientChannelServiceConfigCallData* service_config_call_data; - }; - ~ConfigSelector() override = default; - virtual const char* name() const = 0; + virtual UniqueTypeName name() const = 0; static bool Equals(const ConfigSelector* cs1, const ConfigSelector* cs2) { if (cs1 == nullptr) return cs2 == nullptr; if (cs2 == nullptr) return false; - if (strcmp(cs1->name(), cs2->name()) != 0) return false; + if (cs1->name() != cs2->name()) return false; return cs1->Equals(cs2); } // The channel will call this when the resolver returns a new ConfigSelector // to determine what set of dynamic filters will be configured. + virtual void AddFilters(InterceptionChainBuilder& /*builder*/) {} + // TODO(roth): Remove this once the legacy filter stack goes away. virtual std::vector GetFilters() { return {}; } - // Returns the call config to use for the call, or a status to fail - // the call with. + // Gets the configuration for the call and stores it in service config + // call data. + struct GetCallConfigArgs { + grpc_metadata_batch* initial_metadata; + Arena* arena; + ClientChannelServiceConfigCallData* service_config_call_data; + }; virtual absl::Status GetCallConfig(GetCallConfigArgs args) = 0; - grpc_arg MakeChannelArg() const; - static RefCountedPtr GetFromChannelArgs( - const grpc_channel_args& args); static absl::string_view ChannelArgName() { return GRPC_ARG_CONFIG_SELECTOR; } static int ChannelArgsCompare(const ConfigSelector* a, const ConfigSelector* b) { @@ -101,7 +101,10 @@ class DefaultConfigSelector final : public ConfigSelector { DCHECK(service_config_ != nullptr); } - const char* name() const override { return "default"; } + UniqueTypeName name() const override { + static UniqueTypeName::Factory kFactory("default"); + return kFactory.Create(); + } absl::Status GetCallConfig(GetCallConfigArgs args) override { Slice* path = args.initial_metadata->get_pointer(HttpPathMetadata()); diff --git a/src/core/resolver/xds/xds_resolver.cc b/src/core/resolver/xds/xds_resolver.cc index 5f582bc63f8..286f4e0dedd 100644 --- a/src/core/resolver/xds/xds_resolver.cc +++ b/src/core/resolver/xds/xds_resolver.cc @@ -270,7 +270,10 @@ class XdsResolver final : public Resolver { RefCountedPtr route_config_data); ~XdsConfigSelector() override; - const char* name() const override { return "XdsConfigSelector"; } + UniqueTypeName name() const override { + static UniqueTypeName::Factory kFactory("XdsConfigSelector"); + return kFactory.Create(); + } bool Equals(const ConfigSelector* other) const override { const auto* other_xds = static_cast(other); @@ -281,14 +284,14 @@ class XdsResolver final : public Resolver { absl::Status GetCallConfig(GetCallConfigArgs args) override; - std::vector GetFilters() override { - return filters_; - } + void AddFilters(InterceptionChainBuilder& builder) override; + + std::vector GetFilters() override; private: RefCountedPtr resolver_; RefCountedPtr route_config_data_; - std::vector filters_; + std::vector filters_; }; class XdsRouteStateAttributeImpl final : public XdsRouteStateAttribute { @@ -641,12 +644,9 @@ XdsResolver::XdsConfigSelector::XdsConfigSelector( http_filter_registry.GetFilterForType( http_filter.config.config_proto_type_name); CHECK_NE(filter_impl, nullptr); - // Add C-core filter to list. - if (filter_impl->channel_filter() != nullptr) { - filters_.push_back(filter_impl->channel_filter()); - } + // Add filter to list. + filters_.push_back(filter_impl); } - filters_.push_back(&ClusterSelectionFilter::kFilter); } XdsResolver::XdsConfigSelector::~XdsConfigSelector() { @@ -799,6 +799,26 @@ absl::Status XdsResolver::XdsConfigSelector::GetCallConfig( return absl::OkStatus(); } +void XdsResolver::XdsConfigSelector::AddFilters( + InterceptionChainBuilder& builder) { + for (const XdsHttpFilterImpl* filter : filters_) { + filter->AddFilter(builder); + } + builder.Add(); +} + +std::vector +XdsResolver::XdsConfigSelector::GetFilters() { + std::vector filters; + for (const XdsHttpFilterImpl* filter : filters_) { + if (filter->channel_filter() != nullptr) { + filters.push_back(filter->channel_filter()); + } + } + filters.push_back(&ClusterSelectionFilter::kFilter); + return filters; +} + // // XdsResolver::XdsRouteStateAttributeImpl // diff --git a/src/core/xds/grpc/xds_http_fault_filter.cc b/src/core/xds/grpc/xds_http_fault_filter.cc index 48156bd8cc3..0c65e13deb2 100644 --- a/src/core/xds/grpc/xds_http_fault_filter.cc +++ b/src/core/xds/grpc/xds_http_fault_filter.cc @@ -214,6 +214,10 @@ XdsHttpFaultFilter::GenerateFilterConfigOverride( return GenerateFilterConfig(context, std::move(extension), errors); } +void XdsHttpFaultFilter::AddFilter(InterceptionChainBuilder& builder) const { + builder.Add(); +} + const grpc_channel_filter* XdsHttpFaultFilter::channel_filter() const { return &FaultInjectionFilter::kFilter; } diff --git a/src/core/xds/grpc/xds_http_fault_filter.h b/src/core/xds/grpc/xds_http_fault_filter.h index cd6dfd78691..ff2394c7a28 100644 --- a/src/core/xds/grpc/xds_http_fault_filter.h +++ b/src/core/xds/grpc/xds_http_fault_filter.h @@ -44,6 +44,7 @@ class XdsHttpFaultFilter final : public XdsHttpFilterImpl { absl::optional GenerateFilterConfigOverride( const XdsResourceType::DecodeContext& context, XdsExtension extension, ValidationErrors* errors) const override; + void AddFilter(InterceptionChainBuilder& builder) const override; const grpc_channel_filter* channel_filter() const override; ChannelArgs ModifyChannelArgs(const ChannelArgs& args) const override; absl::StatusOr GenerateServiceConfig( diff --git a/src/core/xds/grpc/xds_http_filters.h b/src/core/xds/grpc/xds_http_filters.h index b61690fd74d..6e5cda52b57 100644 --- a/src/core/xds/grpc/xds_http_filters.h +++ b/src/core/xds/grpc/xds_http_filters.h @@ -35,6 +35,7 @@ #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/channel/channel_fwd.h" #include "src/core/lib/gprpp/validation_errors.h" +#include "src/core/lib/transport/interception_chain.h" #include "src/core/util/json/json.h" #include "src/core/util/json/json_writer.h" #include "src/core/xds/grpc/xds_common_types.h" @@ -96,6 +97,8 @@ class XdsHttpFilterImpl { ValidationErrors* errors) const = 0; // C-core channel filter implementation. + virtual void AddFilter(InterceptionChainBuilder& builder) const = 0; + // TODO(roth): Remove this once the legacy filter stack goes away. virtual const grpc_channel_filter* channel_filter() const = 0; // Modifies channel args that may affect service config parsing (not @@ -135,6 +138,7 @@ class XdsHttpRouterFilter final : public XdsHttpFilterImpl { absl::optional GenerateFilterConfigOverride( const XdsResourceType::DecodeContext& context, XdsExtension extension, ValidationErrors* errors) const override; + void AddFilter(InterceptionChainBuilder& /*builder*/) const override {} const grpc_channel_filter* channel_filter() const override { return nullptr; } absl::StatusOr GenerateServiceConfig( const FilterConfig& /*hcm_filter_config*/, diff --git a/src/core/xds/grpc/xds_http_rbac_filter.cc b/src/core/xds/grpc/xds_http_rbac_filter.cc index ee81008bd90..6ba09042c88 100644 --- a/src/core/xds/grpc/xds_http_rbac_filter.cc +++ b/src/core/xds/grpc/xds_http_rbac_filter.cc @@ -564,6 +564,10 @@ XdsHttpRbacFilter::GenerateFilterConfigOverride( return FilterConfig{OverrideConfigProtoName(), std::move(rbac_json)}; } +void XdsHttpRbacFilter::AddFilter(InterceptionChainBuilder& builder) const { + builder.Add(); +} + const grpc_channel_filter* XdsHttpRbacFilter::channel_filter() const { return &RbacFilter::kFilterVtable; } diff --git a/src/core/xds/grpc/xds_http_rbac_filter.h b/src/core/xds/grpc/xds_http_rbac_filter.h index f95fc5d8693..ad357a978e2 100644 --- a/src/core/xds/grpc/xds_http_rbac_filter.h +++ b/src/core/xds/grpc/xds_http_rbac_filter.h @@ -44,6 +44,7 @@ class XdsHttpRbacFilter final : public XdsHttpFilterImpl { absl::optional GenerateFilterConfigOverride( const XdsResourceType::DecodeContext& context, XdsExtension extension, ValidationErrors* errors) const override; + void AddFilter(InterceptionChainBuilder& builder) const override; const grpc_channel_filter* channel_filter() const override; ChannelArgs ModifyChannelArgs(const ChannelArgs& args) const override; absl::StatusOr GenerateServiceConfig( diff --git a/src/core/xds/grpc/xds_http_stateful_session_filter.cc b/src/core/xds/grpc/xds_http_stateful_session_filter.cc index 698e1776361..7b0b05219cd 100644 --- a/src/core/xds/grpc/xds_http_stateful_session_filter.cc +++ b/src/core/xds/grpc/xds_http_stateful_session_filter.cc @@ -194,6 +194,11 @@ XdsHttpStatefulSessionFilter::GenerateFilterConfigOverride( Json::FromObject(std::move(config))}; } +void XdsHttpStatefulSessionFilter::AddFilter( + InterceptionChainBuilder& builder) const { + builder.Add(); +} + const grpc_channel_filter* XdsHttpStatefulSessionFilter::channel_filter() const { return &StatefulSessionFilter::kFilter; diff --git a/src/core/xds/grpc/xds_http_stateful_session_filter.h b/src/core/xds/grpc/xds_http_stateful_session_filter.h index b734b0fb135..4df7a04a5d8 100644 --- a/src/core/xds/grpc/xds_http_stateful_session_filter.h +++ b/src/core/xds/grpc/xds_http_stateful_session_filter.h @@ -44,6 +44,7 @@ class XdsHttpStatefulSessionFilter final : public XdsHttpFilterImpl { absl::optional GenerateFilterConfigOverride( const XdsResourceType::DecodeContext& context, XdsExtension extension, ValidationErrors* errors) const override; + void AddFilter(InterceptionChainBuilder& builder) const override; const grpc_channel_filter* channel_filter() const override; ChannelArgs ModifyChannelArgs(const ChannelArgs& args) const override; absl::StatusOr GenerateServiceConfig( diff --git a/src/python/grpcio/grpc_core_dependencies.py b/src/python/grpcio/grpc_core_dependencies.py index ae570fb582a..8b24084ed2b 100644 --- a/src/python/grpcio/grpc_core_dependencies.py +++ b/src/python/grpcio/grpc_core_dependencies.py @@ -24,7 +24,6 @@ CORE_SOURCE_FILES = [ 'src/core/client_channel/client_channel_filter.cc', 'src/core/client_channel/client_channel_plugin.cc', 'src/core/client_channel/client_channel_service_config.cc', - 'src/core/client_channel/config_selector.cc', 'src/core/client_channel/dynamic_filters.cc', 'src/core/client_channel/global_subchannel_pool.cc', 'src/core/client_channel/load_balanced_call_destination.cc', diff --git a/test/core/client_channel/client_channel_test.cc b/test/core/client_channel/client_channel_test.cc index 1aa8c644816..a280eae8a71 100644 --- a/test/core/client_channel/client_channel_test.cc +++ b/test/core/client_channel/client_channel_test.cc @@ -23,7 +23,9 @@ #include #include "src/core/lib/address_utils/parse_address.h" +#include "src/core/lib/channel/promise_based_filter.h" #include "src/core/lib/config/core_configuration.h" +#include "src/core/service_config/service_config_impl.h" #include "test/core/call/yodel/yodel_test.h" namespace grpc_core { @@ -77,11 +79,21 @@ class ClientChannelTest : public YodelTest { } Resolver::Result MakeSuccessfulResolutionResult( - absl::string_view endpoint_address) { + absl::string_view endpoint_address, + absl::StatusOr> service_config = nullptr, + RefCountedPtr config_selector = nullptr) { Resolver::Result result; grpc_resolved_address address; CHECK(grpc_parse_uri(URI::Parse(endpoint_address).value(), &address)); result.addresses = EndpointAddressesList({EndpointAddresses{address, {}}}); + result.service_config = std::move(service_config); + if (config_selector != nullptr) { + CHECK(result.service_config.ok()) + << "channel does not use ConfigSelector without service config"; + CHECK(*result.service_config != nullptr) + << "channel does not use ConfigSelector without service config"; + result.args = ChannelArgs().SetObject(std::move(config_selector)); + } return result; } @@ -268,6 +280,94 @@ CLIENT_CHANNEL_TEST(StartCall) { WaitForAllPendingWork(); } +// A filter that adds metadata foo=bar. +class TestFilter : public ImplementChannelFilter { + public: + class Call { + public: + void OnClientInitialMetadata(ClientMetadata& md) { + md.Append("foo", Slice::FromStaticString("bar"), + [](absl::string_view error, const Slice&) { + FAIL() << "error encoding metadata: " << error; + }); + } + + static const NoInterceptor OnClientToServerMessage; + static const NoInterceptor OnClientToServerHalfClose; + static const NoInterceptor OnServerInitialMetadata; + static const NoInterceptor OnServerToClientMessage; + static const NoInterceptor OnServerTrailingMetadata; + static const NoInterceptor OnFinalize; + }; + + static absl::StatusOr> Create( + const ChannelArgs& /*args*/, ChannelFilter::Args /*filter_args*/) { + return std::make_unique(); + } +}; + +const NoInterceptor TestFilter::Call::OnClientToServerMessage; +const NoInterceptor TestFilter::Call::OnClientToServerHalfClose; +const NoInterceptor TestFilter::Call::OnServerInitialMetadata; +const NoInterceptor TestFilter::Call::OnServerToClientMessage; +const NoInterceptor TestFilter::Call::OnServerTrailingMetadata; +const NoInterceptor TestFilter::Call::OnFinalize; + +// A config selector that adds TestFilter as a dynamic filter. +class TestConfigSelector : public ConfigSelector { + public: + UniqueTypeName name() const override { + static UniqueTypeName::Factory kFactory("test"); + return kFactory.Create(); + } + + void AddFilters(InterceptionChainBuilder& builder) override { + builder.Add(); + } + + absl::Status GetCallConfig(GetCallConfigArgs /*args*/) override { + return absl::OkStatus(); + } + + // Any instance of this class will behave the same, so all comparisons + // are true. + bool Equals(const ConfigSelector* /*other*/) const override { return true; } +}; + +CLIENT_CHANNEL_TEST(ConfigSelectorWithDynamicFilters) { + auto& channel = InitChannel(ChannelArgs()); + auto call = MakeCallPair(MakeClientInitialMetadata(), channel.event_engine(), + channel.call_arena_allocator()->MakeArena()); + channel.StartCall(std::move(call.handler)); + auto service_config = ServiceConfigImpl::Create(ChannelArgs(), "{}"); + ASSERT_TRUE(service_config.ok()); + QueueNameResolutionResult(MakeSuccessfulResolutionResult( + "ipv4:127.0.0.1:1234", std::move(service_config), + MakeRefCounted())); + auto call_handler = TickUntilCallStarted(); + SpawnTestSeq( + call_handler, "check_initial_metadata", + [call_handler]() mutable { + return call_handler.PullClientInitialMetadata(); + }, + [](ValueOrFailure md) { + EXPECT_TRUE(md.ok()); + if (md.ok()) { + std::string buffer; + auto value = (*md)->GetStringValue("foo", &buffer); + EXPECT_TRUE(value.has_value()); + if (value.has_value()) EXPECT_EQ(*value, "bar"); + } + return Empty{}; + }); + SpawnTestSeq(call.initiator, "cancel", + [call_initiator = call.initiator]() mutable { + call_initiator.Cancel(); + return Empty{}; + }); + WaitForAllPendingWork(); +} + // TODO(ctiller, roth): MANY more test cases // - Resolver returns an error for the initial result, then returns a valid // result. diff --git a/test/cpp/end2end/client_lb_end2end_test.cc b/test/cpp/end2end/client_lb_end2end_test.cc index cc0a6bbccfa..ea6156b545d 100644 --- a/test/cpp/end2end/client_lb_end2end_test.cc +++ b/test/cpp/end2end/client_lb_end2end_test.cc @@ -2947,7 +2947,10 @@ TEST_F(ControlPlaneStatusRewritingTest, RewritesFromConfigSelector) { public: explicit FailConfigSelector(absl::Status status) : status_(std::move(status)) {} - const char* name() const override { return "FailConfigSelector"; } + grpc_core::UniqueTypeName name() const override { + static grpc_core::UniqueTypeName::Factory kFactory("FailConfigSelector"); + return kFactory.Create(); + } bool Equals(const ConfigSelector* other) const override { return status_ == static_cast(other)->status_; } diff --git a/tools/doxygen/Doxyfile.c++.internal b/tools/doxygen/Doxyfile.c++.internal index 49d88672e31..c3797ac0f86 100644 --- a/tools/doxygen/Doxyfile.c++.internal +++ b/tools/doxygen/Doxyfile.c++.internal @@ -1104,7 +1104,6 @@ src/core/client_channel/client_channel_internal.h \ src/core/client_channel/client_channel_plugin.cc \ src/core/client_channel/client_channel_service_config.cc \ src/core/client_channel/client_channel_service_config.h \ -src/core/client_channel/config_selector.cc \ src/core/client_channel/config_selector.h \ src/core/client_channel/connector.h \ src/core/client_channel/dynamic_filters.cc \ diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index 3cb1e1de44c..64d33400ab2 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -907,7 +907,6 @@ src/core/client_channel/client_channel_internal.h \ src/core/client_channel/client_channel_plugin.cc \ src/core/client_channel/client_channel_service_config.cc \ src/core/client_channel/client_channel_service_config.h \ -src/core/client_channel/config_selector.cc \ src/core/client_channel/config_selector.h \ src/core/client_channel/connector.h \ src/core/client_channel/dynamic_filters.cc \