From 34a8c9de20c237d27164a3e6fef1ec06051a860f Mon Sep 17 00:00:00 2001 From: Taylor Barrella Date: Wed, 25 Aug 2021 17:45:35 +0000 Subject: [PATCH 1/3] ecds: refactor FilterConfigProviderManagerImpl into base and derived classes Signed-off-by: Taylor Barrella --- source/common/filter/config_discovery_impl.cc | 106 ++++++++++-------- source/common/filter/config_discovery_impl.h | 56 ++++++--- 2 files changed, 100 insertions(+), 62 deletions(-) diff --git a/source/common/filter/config_discovery_impl.cc b/source/common/filter/config_discovery_impl.cc index 7c686d1664361..3a8df490867e5 100644 --- a/source/common/filter/config_discovery_impl.cc +++ b/source/common/filter/config_discovery_impl.cc @@ -67,7 +67,8 @@ void DynamicFilterConfigProviderImplBase::validateTerminalFilter(const std::stri FilterConfigSubscription::FilterConfigSubscription( const envoy::config::core::v3::ConfigSource& config_source, const std::string& filter_config_name, Server::Configuration::FactoryContext& factory_context, - const std::string& stat_prefix, FilterConfigProviderManagerImpl& filter_config_provider_manager, + const std::string& stat_prefix, + FilterConfigProviderManagerImplBase& filter_config_provider_manager, const std::string& subscription_id) : Config::SubscriptionBase( factory_context.messageValidationContext().dynamicValidationVisitor(), "name"), @@ -190,7 +191,7 @@ FilterConfigSubscription::~FilterConfigSubscription() { void FilterConfigSubscription::incrementConflictCounter() { stats_.config_conflict_.inc(); } -std::shared_ptr FilterConfigProviderManagerImpl::getSubscription( +std::shared_ptr FilterConfigProviderManagerImplBase::getSubscription( const envoy::config::core::v3::ConfigSource& config_source, const std::string& name, Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix) { // FilterConfigSubscriptions are unique based on their config source and filter config name @@ -212,6 +213,39 @@ std::shared_ptr FilterConfigProviderManagerImpl::getSu } } +void FilterConfigProviderManagerImplBase::applyLastOrDefaultConfig( + std::shared_ptr& subscription, + DynamicFilterConfigProviderImplBase& provider, const std::string& filter_config_name) { + // If the subscription already received a config, attempt to apply it. + // It is possible that the received extension config fails to satisfy the listener + // type URL constraints. This may happen if ECDS and LDS updates are racing, and the LDS + // update arrives first. In this case, use the default config, increment a metric, + // and the applied config eventually converges once ECDS update arrives. + bool last_config_valid = false; + if (subscription->lastConfig().has_value()) { + TRY_ASSERT_MAIN_THREAD { + provider.validateTypeUrl(subscription->lastTypeUrl()); + provider.validateTerminalFilter(filter_config_name, subscription->lastFilterName(), + subscription->isLastFilterTerminal()); + last_config_valid = true; + } + END_TRY catch (const EnvoyException& e) { + ENVOY_LOG(debug, "ECDS subscription {} is invalid in a listener context: {}.", + filter_config_name, e.what()); + subscription->incrementConflictCounter(); + } + if (last_config_valid) { + provider.onConfigUpdate(subscription->lastConfig().value(), subscription->lastVersionInfo(), + nullptr); + } + } + + // Apply the default config if none has been applied. + if (!last_config_valid) { + provider.applyDefaultConfiguration(); + } +} + DynamicFilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFilterConfigProvider( const envoy::config::core::v3::ExtensionConfigSource& config_source, const std::string& filter_config_name, Server::Configuration::FactoryContext& factory_context, @@ -233,26 +267,11 @@ DynamicFilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFil Envoy::Http::FilterFactoryCb default_config = nullptr; if (config_source.has_default_config()) { - auto* default_factory = - Config::Utility::getFactoryByType( - config_source.default_config()); - if (default_factory == nullptr) { - throw EnvoyException(fmt::format("Error: cannot find filter factory {} for default filter " - "configuration with type URL {}.", - filter_config_name, - config_source.default_config().type_url())); - } validateTypeUrlHelper(Config::Utility::getFactoryType(config_source.default_config()), require_type_urls); - ProtobufTypes::MessagePtr message = Config::Utility::translateAnyToFactoryConfig( - config_source.default_config(), factory_context.messageValidationVisitor(), - *default_factory); - Config::Utility::validateTerminalFilters( - filter_config_name, default_factory->name(), filter_chain_type, - default_factory->isTerminalFilterByProto(*message, factory_context), - last_filter_in_filter_config); default_config = - default_factory->createFilterFactoryFromProto(*message, stat_prefix, factory_context); + getDefaultConfig(config_source.default_config(), filter_config_name, factory_context, + stat_prefix, last_filter_in_filter_config, filter_chain_type); } auto provider = std::make_unique( @@ -263,36 +282,29 @@ DynamicFilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFil if (config_source.apply_default_config_without_warming()) { factory_context.initManager().add(provider->initTarget()); } + applyLastOrDefaultConfig(subscription, *provider, filter_config_name); + return provider; +} - // If the subscription already received a config, attempt to apply it. - // It is possible that the received extension config fails to satisfy the listener - // type URL constraints. This may happen if ECDS and LDS updates are racing, and the LDS - // update arrives first. In this case, use the default config, increment a metric, - // and the applied config eventually converges once ECDS update arrives. - bool last_config_valid = false; - if (subscription->lastConfig().has_value()) { - TRY_ASSERT_MAIN_THREAD { - provider->validateTypeUrl(subscription->lastTypeUrl()); - provider->validateTerminalFilter(filter_config_name, subscription->lastFilterName(), - subscription->isLastFilterTerminal()); - last_config_valid = true; - } - END_TRY catch (const EnvoyException& e) { - ENVOY_LOG(debug, "ECDS subscription {} is invalid in a listener context: {}.", - filter_config_name, e.what()); - subscription->incrementConflictCounter(); - } - if (last_config_valid) { - provider->onConfigUpdate(subscription->lastConfig().value(), subscription->lastVersionInfo(), - nullptr); - } - } - - // Apply the default config if none has been applied. - if (!last_config_valid) { - provider->applyDefaultConfiguration(); +Http::FilterFactoryCb HttpFilterConfigProviderManagerImpl::getDefaultConfig( + const ProtobufWkt::Any& proto_config, const std::string& filter_config_name, + Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix, + bool last_filter_in_filter_config, const std::string& filter_chain_type) const { + auto* default_factory = + Config::Utility::getFactoryByType( + proto_config); + if (default_factory == nullptr) { + throw EnvoyException(fmt::format("Error: cannot find filter factory {} for default filter " + "configuration with type URL {}.", + filter_config_name, proto_config.type_url())); } - return provider; + ProtobufTypes::MessagePtr message = Config::Utility::translateAnyToFactoryConfig( + proto_config, factory_context.messageValidationVisitor(), *default_factory); + Config::Utility::validateTerminalFilters( + filter_config_name, default_factory->name(), filter_chain_type, + default_factory->isTerminalFilterByProto(*message, factory_context), + last_filter_in_filter_config); + return default_factory->createFilterFactoryFromProto(*message, stat_prefix, factory_context); } } // namespace Filter diff --git a/source/common/filter/config_discovery_impl.h b/source/common/filter/config_discovery_impl.h index 2c550fd3faa62..1ac36ed533834 100644 --- a/source/common/filter/config_discovery_impl.h +++ b/source/common/filter/config_discovery_impl.h @@ -23,6 +23,7 @@ namespace Envoy { namespace Filter { +class FilterConfigProviderManagerImplBase; class FilterConfigProviderManagerImpl; class FilterConfigSubscription; @@ -161,7 +162,7 @@ class FilterConfigSubscription const std::string& filter_config_name, Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix, - FilterConfigProviderManagerImpl& filter_config_provider_manager, + FilterConfigProviderManagerImplBase& filter_config_provider_manager, const std::string& subscription_id); ~FilterConfigSubscription() override; @@ -204,8 +205,8 @@ class FilterConfigSubscription const std::string stat_prefix_; ExtensionConfigDiscoveryStats stats_; - // FilterConfigProviderManagerImpl maintains active subscriptions in a map. - FilterConfigProviderManagerImpl& filter_config_provider_manager_; + // FilterConfigProviderManagerImplBase maintains active subscriptions in a map. + FilterConfigProviderManagerImplBase& filter_config_provider_manager_; const std::string subscription_id_; absl::flat_hash_set filter_config_providers_; friend class DynamicFilterConfigProviderImplBase; @@ -233,17 +234,35 @@ class StaticFilterConfigProviderImpl : public FilterConfigProvider { const std::string filter_config_name_; }; +/** + * Base class for a FilterConfigProviderManager. + */ +class FilterConfigProviderManagerImplBase : Logger::Loggable { +protected: + std::shared_ptr + getSubscription(const envoy::config::core::v3::ConfigSource& config_source, + const std::string& name, Server::Configuration::FactoryContext& factory_context, + const std::string& stat_prefix); + void applyLastOrDefaultConfig(std::shared_ptr& subscription, + DynamicFilterConfigProviderImplBase& provider, + const std::string& filter_config_name); + +private: + absl::flat_hash_map> subscriptions_; + friend class FilterConfigSubscription; +}; + /** * An implementation of FilterConfigProviderManager. */ -class FilterConfigProviderManagerImpl : public FilterConfigProviderManager, - public Singleton::Instance, - Logger::Loggable { +class FilterConfigProviderManagerImpl : public FilterConfigProviderManagerImplBase, + public FilterConfigProviderManager, + public Singleton::Instance { public: DynamicFilterConfigProviderPtr createDynamicFilterConfigProvider( const envoy::config::core::v3::ExtensionConfigSource& config_source, const std::string& filter_config_name, Server::Configuration::FactoryContext& factory_context, - const std::string& stat_prefix, bool last_filter_in_filter_chain, + const std::string& stat_prefix, bool last_filter_in_filter_config, const std::string& filter_chain_type) override; FilterConfigProviderPtr @@ -252,16 +271,23 @@ class FilterConfigProviderManagerImpl : public FilterConfigProviderManager, return std::make_unique(config, filter_config_name); } -private: - std::shared_ptr - getSubscription(const envoy::config::core::v3::ConfigSource& config_source, - const std::string& name, Server::Configuration::FactoryContext& factory_context, - const std::string& stat_prefix); - absl::flat_hash_map> subscriptions_; - friend class FilterConfigSubscription; +protected: + virtual Http::FilterFactoryCb + getDefaultConfig(const ProtobufWkt::Any& proto_config, const std::string& filter_config_name, + Server::Configuration::FactoryContext& factory_context, + const std::string& stat_prefix, bool last_filter_in_filter_config, + const std::string& filter_chain_type) const PURE; }; -class HttpFilterConfigProviderManagerImpl : public FilterConfigProviderManagerImpl {}; +class HttpFilterConfigProviderManagerImpl : public FilterConfigProviderManagerImpl { +protected: + Http::FilterFactoryCb getDefaultConfig(const ProtobufWkt::Any& proto_config, + const std::string& filter_config_name, + Server::Configuration::FactoryContext& factory_context, + const std::string& stat_prefix, + bool last_filter_in_filter_config, + const std::string& filter_chain_type) const override; +}; } // namespace Filter } // namespace Envoy From d8a05b8b314adaa8b8641ef07a65c453b1a935b5 Mon Sep 17 00:00:00 2001 From: Taylor Barrella Date: Wed, 25 Aug 2021 19:17:39 +0000 Subject: [PATCH 2/3] move type url validation Signed-off-by: Taylor Barrella --- source/common/filter/config_discovery_impl.cc | 12 ++++++------ source/common/filter/config_discovery_impl.h | 15 ++++++++------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/source/common/filter/config_discovery_impl.cc b/source/common/filter/config_discovery_impl.cc index 3a8df490867e5..354789fa7c819 100644 --- a/source/common/filter/config_discovery_impl.cc +++ b/source/common/filter/config_discovery_impl.cc @@ -267,11 +267,9 @@ DynamicFilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFil Envoy::Http::FilterFactoryCb default_config = nullptr; if (config_source.has_default_config()) { - validateTypeUrlHelper(Config::Utility::getFactoryType(config_source.default_config()), - require_type_urls); - default_config = - getDefaultConfig(config_source.default_config(), filter_config_name, factory_context, - stat_prefix, last_filter_in_filter_config, filter_chain_type); + default_config = getDefaultConfig(config_source.default_config(), filter_config_name, + factory_context, stat_prefix, last_filter_in_filter_config, + filter_chain_type, require_type_urls); } auto provider = std::make_unique( @@ -289,7 +287,8 @@ DynamicFilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFil Http::FilterFactoryCb HttpFilterConfigProviderManagerImpl::getDefaultConfig( const ProtobufWkt::Any& proto_config, const std::string& filter_config_name, Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix, - bool last_filter_in_filter_config, const std::string& filter_chain_type) const { + bool last_filter_in_filter_config, const std::string& filter_chain_type, + const absl::flat_hash_set require_type_urls) const { auto* default_factory = Config::Utility::getFactoryByType( proto_config); @@ -298,6 +297,7 @@ Http::FilterFactoryCb HttpFilterConfigProviderManagerImpl::getDefaultConfig( "configuration with type URL {}.", filter_config_name, proto_config.type_url())); } + validateTypeUrlHelper(Config::Utility::getFactoryType(proto_config), require_type_urls); ProtobufTypes::MessagePtr message = Config::Utility::translateAnyToFactoryConfig( proto_config, factory_context.messageValidationVisitor(), *default_factory); Config::Utility::validateTerminalFilters( diff --git a/source/common/filter/config_discovery_impl.h b/source/common/filter/config_discovery_impl.h index 1ac36ed533834..4923100633fb6 100644 --- a/source/common/filter/config_discovery_impl.h +++ b/source/common/filter/config_discovery_impl.h @@ -276,17 +276,18 @@ class FilterConfigProviderManagerImpl : public FilterConfigProviderManagerImplBa getDefaultConfig(const ProtobufWkt::Any& proto_config, const std::string& filter_config_name, Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix, bool last_filter_in_filter_config, - const std::string& filter_chain_type) const PURE; + const std::string& filter_chain_type, + const absl::flat_hash_set require_type_urls) const PURE; }; class HttpFilterConfigProviderManagerImpl : public FilterConfigProviderManagerImpl { protected: - Http::FilterFactoryCb getDefaultConfig(const ProtobufWkt::Any& proto_config, - const std::string& filter_config_name, - Server::Configuration::FactoryContext& factory_context, - const std::string& stat_prefix, - bool last_filter_in_filter_config, - const std::string& filter_chain_type) const override; + Http::FilterFactoryCb + getDefaultConfig(const ProtobufWkt::Any& proto_config, const std::string& filter_config_name, + Server::Configuration::FactoryContext& factory_context, + const std::string& stat_prefix, bool last_filter_in_filter_config, + const std::string& filter_chain_type, + const absl::flat_hash_set require_type_urls) const override; }; } // namespace Filter From 7796988e02c048982f3c8cda62067db1a113c9fb Mon Sep 17 00:00:00 2001 From: Taylor Barrella Date: Wed, 25 Aug 2021 20:13:10 +0000 Subject: [PATCH 3/3] remove unneeded declaration Signed-off-by: Taylor Barrella --- source/common/filter/config_discovery_impl.h | 1 - 1 file changed, 1 deletion(-) diff --git a/source/common/filter/config_discovery_impl.h b/source/common/filter/config_discovery_impl.h index 4923100633fb6..c44a9bfbd43fa 100644 --- a/source/common/filter/config_discovery_impl.h +++ b/source/common/filter/config_discovery_impl.h @@ -24,7 +24,6 @@ namespace Envoy { namespace Filter { class FilterConfigProviderManagerImplBase; -class FilterConfigProviderManagerImpl; class FilterConfigSubscription; using FilterConfigSubscriptionSharedPtr = std::shared_ptr;