From 26f74399b094a267c6aff370dab8a957000f3f08 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 26 Oct 2022 09:46:37 -0700 Subject: [PATCH] xDS: change remaining registries to be non-global (#31293) * json_object_loader: refactor ErrorList into its own library * fix observability_config_test * generate_projects * iwyu * WIP * xDS: make HTTP filter registry non-global * make cluster specifier plugin registry non-global * fix test * clang-format * general-purpose utility for validation xDS extensions * plumb XdsExtension into HTTP filters and ClusterSpecifiers * clang-format * fix tests * iwyu * fix build with old compilers * finish plumbing ValidationErrors into Listener, including HTTP filters * fix tests * WIP * use absl::variant for listener type * use absl::variant for RDS name vs. inline RouteConfig * first passing test * adding more tests * xDS RBAC: remove env var protection * fix federation server test * add another test * fix sanity * first pass of client-side validation, except for HTTP filter configs * minor cleanup and TODO * clean up HTTP filter registration * fix unused param * fix tests * WIP on xds_http_filters_test * clang-format * add tests for fault injection filter * RBAC filter tests * Automated change: Fix sanity tests * fix sanity * iwyu * fix asan problem * started adding server-side listener tests * DownstreamTlsContext tests * update filter chain duplicate detection code * clang-format * migrate remaining tests from e2e tests and remove duplicates * parameterized HCM tests * clang-format * generate_projects * iwyu * remove unnecessary dependency * remove another unnecessary dep * remove more unneeded deps * remove no_op_http_filter * add death test for duplicate HTTP filter registration * clang-format * remove unnecessary StaticConfigName() method from Router filter * clang-format * iwyu Co-authored-by: markdroth --- .../resolver/xds/xds_resolver.cc | 7 ++- src/core/ext/xds/xds_bootstrap_grpc.h | 11 ++++ src/core/ext/xds/xds_client.cc | 2 +- src/core/ext/xds/xds_client_grpc.cc | 9 +--- src/core/ext/xds/xds_cluster.h | 3 +- .../ext/xds/xds_cluster_specifier_plugin.cc | 47 ++++++++--------- .../ext/xds/xds_cluster_specifier_plugin.h | 43 +++++++++++----- src/core/ext/xds/xds_endpoint.h | 3 +- src/core/ext/xds/xds_http_filters.cc | 50 ++++++------------- src/core/ext/xds/xds_http_filters.h | 34 ++++++++++--- src/core/ext/xds/xds_listener.cc | 8 +-- src/core/ext/xds/xds_listener.h | 10 +++- src/core/ext/xds/xds_resource_type.h | 3 +- src/core/ext/xds/xds_route_config.cc | 10 +++- src/core/ext/xds/xds_route_config.h | 10 +++- src/core/ext/xds/xds_routing.cc | 3 +- src/core/ext/xds/xds_routing.h | 2 + src/core/ext/xds/xds_server_config_fetcher.cc | 42 ++++++++++++---- test/core/xds/xds_client_test.cc | 2 +- test/core/xds/xds_http_filters_test.cc | 20 +++----- 20 files changed, 193 insertions(+), 126 deletions(-) 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 870e67bc416..7d06d6f8325 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 @@ -523,11 +523,14 @@ XdsResolver::XdsConfigSelector::XdsConfigSelector( } } // Populate filter list. + const auto& http_filter_registry = + static_cast(resolver_->xds_client_->bootstrap()) + .http_filter_registry(); for (const auto& http_filter : resolver_->current_listener_.http_filters) { // Find filter. This is guaranteed to succeed, because it's checked // at config validation time in the XdsApi code. const XdsHttpFilterImpl* filter_impl = - XdsHttpFilterRegistry::GetFilterForType( + http_filter_registry.GetFilterForType( http_filter.config.config_proto_type_name); GPR_ASSERT(filter_impl != nullptr); // Add C-core filter to list. @@ -600,6 +603,8 @@ XdsResolver::XdsConfigSelector::CreateMethodConfig( } // Handle xDS HTTP filters. auto result = XdsRouting::GeneratePerHTTPFilterConfigs( + static_cast(resolver_->xds_client_->bootstrap()) + .http_filter_registry(), resolver_->current_listener_.http_filters, resolver_->current_virtual_host_, route, cluster_weight, resolver_->args_); diff --git a/src/core/ext/xds/xds_bootstrap_grpc.h b/src/core/ext/xds/xds_bootstrap_grpc.h index 1fa20713b62..e88bec69013 100644 --- a/src/core/ext/xds/xds_bootstrap_grpc.h +++ b/src/core/ext/xds/xds_bootstrap_grpc.h @@ -31,6 +31,8 @@ #include "src/core/ext/xds/certificate_provider_store.h" #include "src/core/ext/xds/xds_bootstrap.h" +#include "src/core/ext/xds/xds_cluster_specifier_plugin.h" +#include "src/core/ext/xds/xds_http_filters.h" #include "src/core/lib/gprpp/validation_errors.h" #include "src/core/lib/json/json.h" #include "src/core/lib/json/json_args.h" @@ -148,6 +150,13 @@ class GrpcXdsBootstrap : public XdsBootstrap { const { return certificate_providers_; } + const XdsHttpFilterRegistry& http_filter_registry() const { + return http_filter_registry_; + } + const XdsClusterSpecifierPluginRegistry& cluster_specifier_plugin_registry() + const { + return cluster_specifier_plugin_registry_; + } // Exposed for testing purposes only. const std::map& authorities() const { @@ -161,6 +170,8 @@ class GrpcXdsBootstrap : public XdsBootstrap { std::string server_listener_resource_name_template_; std::map authorities_; CertificateProviderStore::PluginDefinitionMap certificate_providers_; + XdsHttpFilterRegistry http_filter_registry_; + XdsClusterSpecifierPluginRegistry cluster_specifier_plugin_registry_; }; } // namespace grpc_core diff --git a/src/core/ext/xds/xds_client.cc b/src/core/ext/xds/xds_client.cc index cbf6f480c98..b128d097969 100644 --- a/src/core/ext/xds/xds_client.cc +++ b/src/core/ext/xds/xds_client.cc @@ -1641,7 +1641,7 @@ void XdsClient::MaybeRegisterResourceTypeLocked( return; } resource_types_.emplace(resource_type->type_url(), resource_type); - resource_type->InitUpbSymtab(symtab_.ptr()); + resource_type->InitUpbSymtab(this, symtab_.ptr()); } const XdsResourceType* XdsClient::GetResourceTypeLocked( diff --git a/src/core/ext/xds/xds_client_grpc.cc b/src/core/ext/xds/xds_client_grpc.cc index 8733760fcc8..20273bfc847 100644 --- a/src/core/ext/xds/xds_client_grpc.cc +++ b/src/core/ext/xds/xds_client_grpc.cc @@ -26,7 +26,6 @@ #include "absl/base/thread_annotations.h" #include "absl/status/status.h" #include "absl/strings/string_view.h" -#include "absl/synchronization/mutex.h" #include "absl/types/optional.h" #include @@ -38,8 +37,6 @@ #include "src/core/ext/xds/xds_bootstrap.h" #include "src/core/ext/xds/xds_bootstrap_grpc.h" #include "src/core/ext/xds/xds_channel_args.h" -#include "src/core/ext/xds/xds_cluster_specifier_plugin.h" -#include "src/core/ext/xds/xds_http_filters.h" #include "src/core/ext/xds/xds_transport.h" #include "src/core/ext/xds/xds_transport_grpc.h" #include "src/core/lib/channel/channel_args.h" @@ -65,11 +62,7 @@ namespace grpc_core { namespace { -Mutex* g_mu = [] { - XdsHttpFilterRegistry::Init(); - XdsClusterSpecifierPluginRegistry::Init(); - return new Mutex; -}(); +Mutex* g_mu = new Mutex; const grpc_channel_args* g_channel_args ABSL_GUARDED_BY(*g_mu) = nullptr; GrpcXdsClient* g_xds_client ABSL_GUARDED_BY(*g_mu) = nullptr; char* g_fallback_bootstrap_config ABSL_GUARDED_BY(*g_mu) = nullptr; diff --git a/src/core/ext/xds/xds_cluster.h b/src/core/ext/xds/xds_cluster.h index e72c0e8142f..2e3503c9dc1 100644 --- a/src/core/ext/xds/xds_cluster.h +++ b/src/core/ext/xds/xds_cluster.h @@ -36,6 +36,7 @@ #include "src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.h" #include "src/core/ext/xds/xds_bootstrap.h" #include "src/core/ext/xds/xds_bootstrap_grpc.h" +#include "src/core/ext/xds/xds_client.h" #include "src/core/ext/xds/xds_common_types.h" #include "src/core/ext/xds/xds_resource_type.h" #include "src/core/ext/xds/xds_resource_type_impl.h" @@ -100,7 +101,7 @@ class XdsClusterResourceType bool AllResourcesRequiredInSotW() const override { return true; } - void InitUpbSymtab(upb_DefPool* symtab) const override { + void InitUpbSymtab(XdsClient*, upb_DefPool* symtab) const override { envoy_config_cluster_v3_Cluster_getmsgdef(symtab); envoy_extensions_clusters_aggregate_v3_ClusterConfig_getmsgdef(symtab); envoy_extensions_transport_sockets_tls_v3_UpstreamTlsContext_getmsgdef( diff --git a/src/core/ext/xds/xds_cluster_specifier_plugin.cc b/src/core/ext/xds/xds_cluster_specifier_plugin.cc index 769097a9e69..f54b8d3a4d4 100644 --- a/src/core/ext/xds/xds_cluster_specifier_plugin.cc +++ b/src/core/ext/xds/xds_cluster_specifier_plugin.cc @@ -41,6 +41,10 @@ namespace grpc_core { +// +// XdsRouteLookupClusterSpecifierPlugin +// + const char* kXdsRouteLookupClusterSpecifierPluginConfigName = "grpc.lookup.v1.RouteLookupClusterSpecifier"; @@ -114,41 +118,34 @@ XdsRouteLookupClusterSpecifierPlugin::GenerateLoadBalancingPolicyConfig( return lb_policy_config.Dump(); } -namespace { - -using PluginRegistryMap = - std::map>; +// +// XdsClusterSpecifierPluginRegistry +// -PluginRegistryMap* g_plugin_registry = nullptr; +XdsClusterSpecifierPluginRegistry::XdsClusterSpecifierPluginRegistry() { + RegisterPlugin(std::make_unique(), + kXdsRouteLookupClusterSpecifierPluginConfigName); +} -} // namespace +void XdsClusterSpecifierPluginRegistry::RegisterPlugin( + std::unique_ptr plugin, + absl::string_view config_proto_type_name) { + registry_[config_proto_type_name] = std::move(plugin); +} const XdsClusterSpecifierPluginImpl* XdsClusterSpecifierPluginRegistry::GetPluginForType( - absl::string_view config_proto_type_name) { - auto it = g_plugin_registry->find(config_proto_type_name); - if (it == g_plugin_registry->end()) return nullptr; + absl::string_view config_proto_type_name) const { + auto it = registry_.find(config_proto_type_name); + if (it == registry_.end()) return nullptr; return it->second.get(); } -void XdsClusterSpecifierPluginRegistry::PopulateSymtab(upb_DefPool* symtab) { - for (const auto& p : *g_plugin_registry) { +void XdsClusterSpecifierPluginRegistry::PopulateSymtab( + upb_DefPool* symtab) const { + for (const auto& p : registry_) { p.second->PopulateSymtab(symtab); } } -void XdsClusterSpecifierPluginRegistry::RegisterPlugin( - std::unique_ptr plugin, - absl::string_view config_proto_type_name) { - (*g_plugin_registry)[config_proto_type_name] = std::move(plugin); -} - -void XdsClusterSpecifierPluginRegistry::Init() { - g_plugin_registry = new PluginRegistryMap; - RegisterPlugin(std::make_unique(), - kXdsRouteLookupClusterSpecifierPluginConfigName); -} - -void XdsClusterSpecifierPluginRegistry::Shutdown() { delete g_plugin_registry; } - } // namespace grpc_core diff --git a/src/core/ext/xds/xds_cluster_specifier_plugin.h b/src/core/ext/xds/xds_cluster_specifier_plugin.h index 2d6d6605675..3e98d9d1a9f 100644 --- a/src/core/ext/xds/xds_cluster_specifier_plugin.h +++ b/src/core/ext/xds/xds_cluster_specifier_plugin.h @@ -19,8 +19,10 @@ #include +#include #include #include +#include #include "absl/status/statusor.h" #include "absl/strings/string_view.h" @@ -54,18 +56,35 @@ class XdsRouteLookupClusterSpecifierPlugin class XdsClusterSpecifierPluginRegistry { public: - static void RegisterPlugin( - std::unique_ptr plugin, - absl::string_view config_proto_type_name); - - static void PopulateSymtab(upb_DefPool* symtab); - - static const XdsClusterSpecifierPluginImpl* GetPluginForType( - absl::string_view config_proto_type_name); - - // Global init and shutdown. - static void Init(); - static void Shutdown(); + XdsClusterSpecifierPluginRegistry(); + + // Not copyable. + XdsClusterSpecifierPluginRegistry(const XdsClusterSpecifierPluginRegistry&) = + delete; + XdsClusterSpecifierPluginRegistry& operator=( + const XdsClusterSpecifierPluginRegistry&) = delete; + + // Movable. + XdsClusterSpecifierPluginRegistry( + XdsClusterSpecifierPluginRegistry&& other) noexcept + : registry_(std::move(other.registry_)) {} + XdsClusterSpecifierPluginRegistry& operator=( + XdsClusterSpecifierPluginRegistry&& other) noexcept { + registry_ = std::move(other.registry_); + return *this; + } + + void RegisterPlugin(std::unique_ptr plugin, + absl::string_view config_proto_type_name); + + void PopulateSymtab(upb_DefPool* symtab) const; + + const XdsClusterSpecifierPluginImpl* GetPluginForType( + absl::string_view config_proto_type_name) const; + + private: + std::map> + registry_; }; } // namespace grpc_core diff --git a/src/core/ext/xds/xds_endpoint.h b/src/core/ext/xds/xds_endpoint.h index 614f179c6f9..0638bf23516 100644 --- a/src/core/ext/xds/xds_endpoint.h +++ b/src/core/ext/xds/xds_endpoint.h @@ -32,6 +32,7 @@ #include "envoy/config/endpoint/v3/endpoint.upbdefs.h" #include "upb/def.h" +#include "src/core/ext/xds/xds_client.h" #include "src/core/ext/xds/xds_client_stats.h" #include "src/core/ext/xds/xds_resource_type.h" #include "src/core/ext/xds/xds_resource_type_impl.h" @@ -128,7 +129,7 @@ class XdsEndpointResourceType DecodeResult Decode(const XdsResourceType::DecodeContext& context, absl::string_view serialized_resource) const override; - void InitUpbSymtab(upb_DefPool* symtab) const override { + void InitUpbSymtab(XdsClient*, upb_DefPool* symtab) const override { envoy_config_endpoint_v3_ClusterLoadAssignment_getmsgdef(symtab); } }; diff --git a/src/core/ext/xds/xds_http_filters.cc b/src/core/ext/xds/xds_http_filters.cc index ac2089c4fde..c6b8c0b47f0 100644 --- a/src/core/ext/xds/xds_http_filters.cc +++ b/src/core/ext/xds/xds_http_filters.cc @@ -81,54 +81,36 @@ XdsHttpRouterFilter::GenerateFilterConfigOverride( // XdsHttpFilterRegistry // -namespace { - -using FilterOwnerList = std::vector>; -using FilterRegistryMap = std::map; - -FilterOwnerList* g_filters = nullptr; -FilterRegistryMap* g_filter_registry = nullptr; - -} // namespace +XdsHttpFilterRegistry::XdsHttpFilterRegistry(bool register_builtins) { + if (register_builtins) { + RegisterFilter(std::make_unique()); + RegisterFilter(std::make_unique()); + RegisterFilter(std::make_unique()); + } +} void XdsHttpFilterRegistry::RegisterFilter( std::unique_ptr filter) { - GPR_ASSERT(g_filter_registry->emplace(filter->ConfigProtoName(), filter.get()) - .second); + GPR_ASSERT( + registry_map_.emplace(filter->ConfigProtoName(), filter.get()).second); auto override_proto_name = filter->OverrideConfigProtoName(); if (!override_proto_name.empty()) { - GPR_ASSERT( - g_filter_registry->emplace(override_proto_name, filter.get()).second); + GPR_ASSERT(registry_map_.emplace(override_proto_name, filter.get()).second); } - g_filters->push_back(std::move(filter)); + owning_list_.push_back(std::move(filter)); } const XdsHttpFilterImpl* XdsHttpFilterRegistry::GetFilterForType( - absl::string_view proto_type_name) { - auto it = g_filter_registry->find(proto_type_name); - if (it == g_filter_registry->end()) return nullptr; + absl::string_view proto_type_name) const { + auto it = registry_map_.find(proto_type_name); + if (it == registry_map_.end()) return nullptr; return it->second; } -void XdsHttpFilterRegistry::PopulateSymtab(upb_DefPool* symtab) { - for (const auto& filter : *g_filters) { +void XdsHttpFilterRegistry::PopulateSymtab(upb_DefPool* symtab) const { + for (const auto& filter : owning_list_) { filter->PopulateSymtab(symtab); } } -void XdsHttpFilterRegistry::Init(bool register_builtins) { - g_filters = new FilterOwnerList; - g_filter_registry = new FilterRegistryMap; - if (register_builtins) { - RegisterFilter(std::make_unique()); - RegisterFilter(std::make_unique()); - RegisterFilter(std::make_unique()); - } -} - -void XdsHttpFilterRegistry::Shutdown() { - delete g_filter_registry; - delete g_filters; -} - } // namespace grpc_core diff --git a/src/core/ext/xds/xds_http_filters.h b/src/core/ext/xds/xds_http_filters.h index 88d659905a0..7c087ccb131 100644 --- a/src/core/ext/xds/xds_http_filters.h +++ b/src/core/ext/xds/xds_http_filters.h @@ -19,8 +19,12 @@ #include +#include +#include #include #include +#include +#include #include "absl/status/status.h" #include "absl/status/statusor.h" @@ -145,16 +149,32 @@ class XdsHttpRouterFilter : public XdsHttpFilterImpl { class XdsHttpFilterRegistry { public: - static void RegisterFilter(std::unique_ptr filter); + explicit XdsHttpFilterRegistry(bool register_builtins = true); + + // Not copyable. + XdsHttpFilterRegistry(const XdsHttpFilterRegistry&) = delete; + XdsHttpFilterRegistry& operator=(const XdsHttpFilterRegistry&) = delete; + + // Movable. + XdsHttpFilterRegistry(XdsHttpFilterRegistry&& other) noexcept + : owning_list_(std::move(other.owning_list_)), + registry_map_(std::move(other.registry_map_)) {} + XdsHttpFilterRegistry& operator=(XdsHttpFilterRegistry&& other) noexcept { + owning_list_ = std::move(other.owning_list_); + registry_map_ = std::move(other.registry_map_); + return *this; + } + + void RegisterFilter(std::unique_ptr filter); - static const XdsHttpFilterImpl* GetFilterForType( - absl::string_view proto_type_name); + const XdsHttpFilterImpl* GetFilterForType( + absl::string_view proto_type_name) const; - static void PopulateSymtab(upb_DefPool* symtab); + void PopulateSymtab(upb_DefPool* symtab) const; - // Global init and shutdown. - static void Init(bool register_builtins = true); - static void Shutdown(); + private: + std::vector> owning_list_; + std::map registry_map_; }; } // namespace grpc_core diff --git a/src/core/ext/xds/xds_listener.cc b/src/core/ext/xds/xds_listener.cc index 6fbe1eebf28..d5649600c1d 100644 --- a/src/core/ext/xds/xds_listener.cc +++ b/src/core/ext/xds/xds_listener.cc @@ -354,6 +354,9 @@ XdsListenerResource::HttpConnectionManager HttpConnectionManagerParse( // http_filters { ValidationErrors::ScopedField field(errors, ".http_filters"); + const auto& http_filter_registry = + static_cast(context.client->bootstrap()) + .http_filter_registry(); size_t num_filters = 0; const auto* http_filters = envoy_extensions_filters_network_http_connection_manager_v3_HttpConnectionManager_http_filters( @@ -396,8 +399,7 @@ XdsListenerResource::HttpConnectionManager HttpConnectionManagerParse( auto extension = ExtractXdsExtension(context, typed_config, errors); const XdsHttpFilterImpl* filter_impl = nullptr; if (extension.has_value()) { - filter_impl = - XdsHttpFilterRegistry::GetFilterForType(extension->type); + filter_impl = http_filter_registry.GetFilterForType(extension->type); } if (filter_impl == nullptr) { if (!is_optional) errors->AddError("unsupported filter type"); @@ -431,7 +433,7 @@ XdsListenerResource::HttpConnectionManager HttpConnectionManagerParse( // out of which only one gets added in the final list. for (const auto& http_filter : http_connection_manager.http_filters) { const XdsHttpFilterImpl* filter_impl = - XdsHttpFilterRegistry::GetFilterForType( + http_filter_registry.GetFilterForType( http_filter.config.config_proto_type_name); if (&http_filter != &http_connection_manager.http_filters.back()) { // Filters before the last filter must not be terminal. diff --git a/src/core/ext/xds/xds_listener.h b/src/core/ext/xds/xds_listener.h index 67d41c02ba9..7a289939dd7 100644 --- a/src/core/ext/xds/xds_listener.h +++ b/src/core/ext/xds/xds_listener.h @@ -37,6 +37,8 @@ #include "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.upbdefs.h" #include "upb/def.h" +#include "src/core/ext/xds/xds_bootstrap_grpc.h" +#include "src/core/ext/xds/xds_client.h" #include "src/core/ext/xds/xds_common_types.h" #include "src/core/ext/xds/xds_http_filters.h" #include "src/core/ext/xds/xds_resource_type.h" @@ -206,11 +208,15 @@ class XdsListenerResourceType bool AllResourcesRequiredInSotW() const override { return true; } - void InitUpbSymtab(upb_DefPool* symtab) const override { + void InitUpbSymtab(XdsClient* xds_client, + upb_DefPool* symtab) const override { envoy_config_listener_v3_Listener_getmsgdef(symtab); envoy_extensions_filters_network_http_connection_manager_v3_HttpConnectionManager_getmsgdef( symtab); - XdsHttpFilterRegistry::PopulateSymtab(symtab); + const auto& http_filter_registry = + static_cast(xds_client->bootstrap()) + .http_filter_registry(); + http_filter_registry.PopulateSymtab(symtab); } }; diff --git a/src/core/ext/xds/xds_resource_type.h b/src/core/ext/xds/xds_resource_type.h index fe030a248e0..9ac4c02cfaf 100644 --- a/src/core/ext/xds/xds_resource_type.h +++ b/src/core/ext/xds/xds_resource_type.h @@ -95,7 +95,8 @@ class XdsResourceType { // properly in logs. // Note: This won't actually work properly until upb adds support for // Any fields in textproto printing (internal b/178821188). - virtual void InitUpbSymtab(upb_DefPool* symtab) const = 0; + virtual void InitUpbSymtab(XdsClient* xds_client, + upb_DefPool* symtab) const = 0; }; } // namespace grpc_core diff --git a/src/core/ext/xds/xds_route_config.cc b/src/core/ext/xds/xds_route_config.cc index 7a63e1ceec5..d6cb9120155 100644 --- a/src/core/ext/xds/xds_route_config.cc +++ b/src/core/ext/xds/xds_route_config.cc @@ -333,6 +333,9 @@ ClusterSpecifierPluginParse( const envoy_config_route_v3_RouteConfiguration* route_config) { XdsRouteConfigResource::ClusterSpecifierPluginMap cluster_specifier_plugin_map; + const auto& cluster_specifier_plugin_registry = + static_cast(context.client->bootstrap()) + .cluster_specifier_plugin_registry(); size_t num_cluster_specifier_plugins; const envoy_config_route_v3_ClusterSpecifierPlugin* const* cluster_specifier_plugin = @@ -368,7 +371,7 @@ ClusterSpecifierPluginParse( bool is_optional = envoy_config_route_v3_ClusterSpecifierPlugin_is_optional( cluster_specifier_plugin[i]); const XdsClusterSpecifierPluginImpl* cluster_specifier_plugin_impl = - XdsClusterSpecifierPluginRegistry::GetPluginForType(extension->type); + cluster_specifier_plugin_registry.GetPluginForType(extension->type); std::string lb_policy_config; if (cluster_specifier_plugin_impl == nullptr) { if (!is_optional) { @@ -631,8 +634,11 @@ ParseTypedPerFilterConfig( return errors.status("could not determine extension type"); } GPR_ASSERT(extension.has_value()); + const auto& http_filter_registry = + static_cast(context.client->bootstrap()) + .http_filter_registry(); const XdsHttpFilterImpl* filter_impl = - XdsHttpFilterRegistry::GetFilterForType(extension->type); + http_filter_registry.GetFilterForType(extension->type); if (filter_impl == nullptr) { if (is_optional) continue; return absl::InvalidArgumentError(absl::StrCat( diff --git a/src/core/ext/xds/xds_route_config.h b/src/core/ext/xds/xds_route_config.h index d9d6dea6b69..738f65786d5 100644 --- a/src/core/ext/xds/xds_route_config.h +++ b/src/core/ext/xds/xds_route_config.h @@ -36,6 +36,8 @@ #include "re2/re2.h" #include "upb/def.h" +#include "src/core/ext/xds/xds_bootstrap_grpc.h" +#include "src/core/ext/xds/xds_client.h" #include "src/core/ext/xds/xds_cluster_specifier_plugin.h" #include "src/core/ext/xds/xds_http_filters.h" #include "src/core/ext/xds/xds_resource_type.h" @@ -224,9 +226,13 @@ class XdsRouteConfigResourceType DecodeResult Decode(const XdsResourceType::DecodeContext& context, absl::string_view serialized_resource) const override; - void InitUpbSymtab(upb_DefPool* symtab) const override { + void InitUpbSymtab(XdsClient* xds_client, + upb_DefPool* symtab) const override { envoy_config_route_v3_RouteConfiguration_getmsgdef(symtab); - XdsClusterSpecifierPluginRegistry::PopulateSymtab(symtab); + const auto& cluster_specifier_plugin_registry = + static_cast(xds_client->bootstrap()) + .cluster_specifier_plugin_registry(); + cluster_specifier_plugin_registry.PopulateSymtab(symtab); } }; diff --git a/src/core/ext/xds/xds_routing.cc b/src/core/ext/xds/xds_routing.cc index ebca6d2c731..e59d6b9d50e 100644 --- a/src/core/ext/xds/xds_routing.cc +++ b/src/core/ext/xds/xds_routing.cc @@ -220,6 +220,7 @@ const XdsHttpFilterImpl::FilterConfig* FindFilterConfigOverride( absl::StatusOr XdsRouting::GeneratePerHTTPFilterConfigs( + const XdsHttpFilterRegistry& http_filter_registry, const std::vector& http_filters, const XdsRouteConfigResource::VirtualHost& vhost, @@ -233,7 +234,7 @@ XdsRouting::GeneratePerHTTPFilterConfigs( // Find filter. This is guaranteed to succeed, because it's checked // at config validation time in the XdsApi code. const XdsHttpFilterImpl* filter_impl = - XdsHttpFilterRegistry::GetFilterForType( + http_filter_registry.GetFilterForType( http_filter.config.config_proto_type_name); GPR_ASSERT(filter_impl != nullptr); // If there is not actually any C-core filter associated with this diff --git a/src/core/ext/xds/xds_routing.h b/src/core/ext/xds/xds_routing.h index 59fe4dfee5e..93e79614f0d 100644 --- a/src/core/ext/xds/xds_routing.h +++ b/src/core/ext/xds/xds_routing.h @@ -31,6 +31,7 @@ #include "absl/strings/string_view.h" #include "absl/types/optional.h" +#include "src/core/ext/xds/xds_http_filters.h" #include "src/core/ext/xds/xds_listener.h" #include "src/core/ext/xds/xds_route_config.h" #include "src/core/lib/channel/channel_args.h" @@ -90,6 +91,7 @@ class XdsRouting { // Generates a map of per_filter_configs. \a args is consumed. static absl::StatusOr GeneratePerHTTPFilterConfigs( + const XdsHttpFilterRegistry& http_filter_registry, const std::vector& http_filters, const XdsRouteConfigResource::VirtualHost& vhost, diff --git a/src/core/ext/xds/xds_server_config_fetcher.cc b/src/core/ext/xds/xds_server_config_fetcher.cc index 0e1f3da3042..d72e19c55b1 100644 --- a/src/core/ext/xds/xds_server_config_fetcher.cc +++ b/src/core/ext/xds/xds_server_config_fetcher.cc @@ -315,6 +315,7 @@ class XdsServerConfigFetcher::ListenerWatcher::FilterChainMatchManager:: XdsServerConfigSelector : public ServerConfigSelector { public: static absl::StatusOr> Create( + const XdsHttpFilterRegistry& http_filter_registry, XdsRouteConfigResource rds_update, const std::vector& http_filters); @@ -378,12 +379,18 @@ class XdsServerConfigFetcher::ListenerWatcher::FilterChainMatchManager:: : public ServerConfigSelectorProvider { public: StaticXdsServerConfigSelectorProvider( + RefCountedPtr xds_client, absl::StatusOr static_resource, std::vector http_filters) - : static_resource_(std::move(static_resource)), + : xds_client_(std::move(xds_client)), + static_resource_(std::move(static_resource)), http_filters_(std::move(http_filters)) {} + ~StaticXdsServerConfigSelectorProvider() override { + xds_client_.reset(DEBUG_LOCATION, "StaticXdsServerConfigSelectorProvider"); + } + absl::StatusOr> Watch( std::unique_ptr watcher) override { @@ -392,8 +399,10 @@ class XdsServerConfigFetcher::ListenerWatcher::FilterChainMatchManager:: if (!static_resource_.ok()) { return static_resource_.status(); } - return XdsServerConfigSelector::Create(static_resource_.value(), - http_filters_); + return XdsServerConfigSelector::Create( + static_cast(xds_client_->bootstrap()) + .http_filter_registry(), + static_resource_.value(), http_filters_); } void Orphan() override {} @@ -401,6 +410,7 @@ class XdsServerConfigFetcher::ListenerWatcher::FilterChainMatchManager:: void CancelWatch() override { watcher_.reset(); } private: + RefCountedPtr xds_client_; absl::StatusOr static_resource_; std::vector http_filters_; @@ -1051,16 +1061,18 @@ absl::StatusOr XdsServerConfigFetcher::ListenerWatcher:: RefCountedPtr server_config_selector_provider; RefCountedPtr channel_stack_modifier; RefCountedPtr xds_certificate_provider; - // Add config selector filter. - std::vector filters; // Iterate the list of HTTP filters in reverse since in Core, received data // flows *up* the stack. + std::vector filters; + const auto& http_filter_registry = + static_cast(xds_client_->bootstrap()) + .http_filter_registry(); for (const auto& http_filter : filter_chain->http_connection_manager.http_filters) { // Find filter. This is guaranteed to succeed, because it's checked // at config validation time in the XdsApi code. const XdsHttpFilterImpl* filter_impl = - XdsHttpFilterRegistry::GetFilterForType( + http_filter_registry.GetFilterForType( http_filter.config.config_proto_type_name); GPR_ASSERT(filter_impl != nullptr); // Some filters like the router filter are no-op filters and do not have @@ -1069,6 +1081,7 @@ absl::StatusOr XdsServerConfigFetcher::ListenerWatcher:: filters.push_back(filter_impl->channel_filter()); } } + // Add config selector filter. filters.push_back(&kServerConfigSelectorFilter); channel_stack_modifier = MakeRefCounted(std::move(filters)); @@ -1092,6 +1105,8 @@ absl::StatusOr XdsServerConfigFetcher::ListenerWatcher:: [&](const XdsRouteConfigResource& route_config) { server_config_selector_provider = MakeRefCounted( + xds_client_->Ref(DEBUG_LOCATION, + "StaticXdsServerConfigSelectorProvider"), route_config, filter_chain->http_connection_manager.http_filters); }); @@ -1122,6 +1137,7 @@ absl::StatusOr< FilterChainMatchManager::XdsServerConfigSelector>> XdsServerConfigFetcher::ListenerWatcher::FilterChainMatchManager:: XdsServerConfigSelector::Create( + const XdsHttpFilterRegistry& http_filter_registry, XdsRouteConfigResource rds_update, const std::vector< XdsListenerResource::HttpConnectionManager::HttpFilter>& @@ -1139,7 +1155,8 @@ XdsServerConfigFetcher::ListenerWatcher::FilterChainMatchManager:: absl::get_if( &route.action) == nullptr; auto result = XdsRouting::GeneratePerHTTPFilterConfigs( - http_filters, vhost, route, nullptr, ChannelArgs()); + http_filter_registry, http_filters, vhost, route, nullptr, + ChannelArgs()); if (!result.ok()) return result.status(); std::vector fields; fields.reserve(result->per_filter_configs.size()); @@ -1265,7 +1282,10 @@ XdsServerConfigFetcher::ListenerWatcher::FilterChainMatchManager:: if (!resource.ok()) { return resource.status(); } - return XdsServerConfigSelector::Create(resource.value(), http_filters_); + return XdsServerConfigSelector::Create( + static_cast(xds_client_->bootstrap()) + .http_filter_registry(), + resource.value(), http_filters_); } void XdsServerConfigFetcher::ListenerWatcher::FilterChainMatchManager:: @@ -1286,8 +1306,10 @@ void XdsServerConfigFetcher::ListenerWatcher::FilterChainMatchManager:: // DynamicXdsServerConfigSelectorProvider while holding a lock, but if that // ever changes, we would want to invoke the update outside the critical // region with the use of a WorkSerializer. - watcher_->OnServerConfigSelectorUpdate( - XdsServerConfigSelector::Create(*resource_, http_filters_)); + watcher_->OnServerConfigSelectorUpdate(XdsServerConfigSelector::Create( + static_cast(xds_client_->bootstrap()) + .http_filter_registry(), + *resource_, http_filters_)); } void XdsServerConfigFetcher::ListenerWatcher::FilterChainMatchManager:: diff --git a/test/core/xds/xds_client_test.cc b/test/core/xds/xds_client_test.cc index e84e329c448..5a231509e59 100644 --- a/test/core/xds/xds_client_test.cc +++ b/test/core/xds/xds_client_test.cc @@ -355,7 +355,7 @@ class XdsClientTest : public ::testing::Test { } return result; } - void InitUpbSymtab(upb_DefPool* /*symtab*/) const override {} + void InitUpbSymtab(XdsClient*, upb_DefPool* /*symtab*/) const override {} static google::protobuf::Any EncodeAsAny(const ResourceStruct& resource) { google::protobuf::Any any; diff --git a/test/core/xds/xds_http_filters_test.cc b/test/core/xds/xds_http_filters_test.cc index cca65b44803..bc3addce010 100644 --- a/test/core/xds/xds_http_filters_test.cc +++ b/test/core/xds/xds_http_filters_test.cc @@ -72,12 +72,6 @@ using ::envoy::extensions::filters::http::router::v3::Router; class XdsHttpFilterTest : public ::testing::Test { protected: - XdsHttpFilterTest() { - // Start with a clean registry for each test. - XdsHttpFilterRegistry::Shutdown(); - XdsHttpFilterRegistry::Init(); - } - XdsExtension MakeXdsExtension(const grpc::protobuf::Message& message) { google::protobuf::Any any; any.PackFrom(message); @@ -93,11 +87,12 @@ class XdsHttpFilterTest : public ::testing::Test { return extension; } - static const XdsHttpFilterImpl* GetFilter(absl::string_view type) { - return XdsHttpFilterRegistry::GetFilterForType( + const XdsHttpFilterImpl* GetFilter(absl::string_view type) { + return registry_.GetFilterForType( absl::StripPrefix(type, "type.googleapis.com/")); } + XdsHttpFilterRegistry registry_; ValidationErrors errors_; upb::Arena arena_; std::string type_url_storage_; @@ -112,15 +107,14 @@ using XdsHttpFilterRegistryTest = XdsHttpFilterTest; TEST_F(XdsHttpFilterRegistryTest, Basic) { // Start with an empty registry. - XdsHttpFilterRegistry::Shutdown(); - XdsHttpFilterRegistry::Init(/*register_builtins=*/false); + registry_ = XdsHttpFilterRegistry(/*register_builtins=*/false); // Returns null when a filter has not yet been registered. XdsExtension extension = MakeXdsExtension(Router()); EXPECT_EQ(GetFilter(extension.type), nullptr); // Now register the filter. auto filter = std::make_unique(); auto* filter_ptr = filter.get(); - XdsHttpFilterRegistry::RegisterFilter(std::move(filter)); + registry_.RegisterFilter(std::move(filter)); // And check that it is now present. EXPECT_EQ(GetFilter(extension.type), filter_ptr); } @@ -131,9 +125,7 @@ TEST_F(XdsHttpFilterRegistryDeathTest, DuplicateRegistryFails) { GTEST_FLAG_SET(death_test_style, "threadsafe"); ASSERT_DEATH( // The router filter is already in the registry. - XdsHttpFilterRegistry::RegisterFilter( - std::make_unique()), - ""); + registry_.RegisterFilter(std::make_unique()), ""); } //