From a4125b637796cf10f4f65994deab9b32364e24bd Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Tue, 3 May 2022 20:40:39 +0000 Subject: [PATCH 01/28] Listener filter ECDS Support This PR support both TCP and UDP listener filter ECDS functionalities. Signed-off-by: Yanjun Xiang --- envoy/filter/config_provider_manager.h | 8 +- envoy/server/BUILD | 1 + envoy/server/listener_manager.h | 9 +- source/common/filter/config_discovery_impl.h | 22 +- .../network/http_connection_manager/config.cc | 2 +- source/server/BUILD | 1 + source/server/config_validation/server.h | 10 +- source/server/configuration_impl.cc | 83 ++++- source/server/configuration_impl.h | 5 +- source/server/listener_impl.cc | 18 +- source/server/listener_impl.h | 5 +- source/server/listener_manager_impl.cc | 96 +++++- source/server/listener_manager_impl.h | 158 +++++---- .../filter/config_discovery_impl_test.cc | 13 +- test/integration/BUILD | 16 + .../filters/test_listener_filter.cc | 37 +- .../filters/test_listener_filter.h | 26 ++ .../filters/test_listener_filter.proto | 7 + ...er_extension_discovery_integration_test.cc | 319 ++++++++++++++++++ .../mocks/server/listener_component_factory.h | 4 +- test/server/listener_manager_impl_test.cc | 7 +- test/server/listener_manager_impl_test.h | 14 +- 22 files changed, 726 insertions(+), 135 deletions(-) create mode 100644 test/integration/listener_extension_discovery_integration_test.cc diff --git a/envoy/filter/config_provider_manager.h b/envoy/filter/config_provider_manager.h index 158e7e158f80..8f520047c20d 100644 --- a/envoy/filter/config_provider_manager.h +++ b/envoy/filter/config_provider_manager.h @@ -19,6 +19,10 @@ using DynamicFilterConfigProvider = Envoy::Config::DynamicExtensionConfigProvide template using DynamicFilterConfigProviderPtr = std::unique_ptr>; +// Listener filter config provider aliases +using ListenerFilterFactoriesList = std::vector>; +using UdpListenerFilterFactoriesList = std::vector>; + /** * The FilterConfigProviderManager exposes the ability to get an FilterConfigProvider * for both static and dynamic filter config providers. @@ -37,12 +41,14 @@ template class FilterConfigProviderManager { * @param last_filter_in_filter_chain indicates whether this filter is the last filter in the * configured chain * @param filter_chain_type is the filter chain type + * @param listener_filter_matcher is the filter matcher for TCP listener filter. nullptr for other filter types. */ virtual DynamicFilterConfigProviderPtr createDynamicFilterConfigProvider( const envoy::config::core::v3::ExtensionConfigSource& config_source, const std::string& filter_config_name, FactoryCtx& factory_context, const std::string& stat_prefix, bool last_filter_in_filter_chain, - const std::string& filter_chain_type) PURE; + const std::string& filter_chain_type, + const Network::ListenerFilterMatcherSharedPtr& listener_filter_matcher) PURE; /** * Get an FilterConfigProviderPtr for a statically inlined filter config. diff --git a/envoy/server/BUILD b/envoy/server/BUILD index 6c0cacd41887..77eeef992b6a 100644 --- a/envoy/server/BUILD +++ b/envoy/server/BUILD @@ -248,6 +248,7 @@ envoy_cc_library( ":drain_manager_interface", ":filter_config_interface", ":guarddog_interface", + "//envoy/filter:config_provider_manager_interface", "//envoy/network:filter_interface", "//envoy/network:listen_socket_interface", "//envoy/network:socket_interface_interface", diff --git a/envoy/server/listener_manager.h b/envoy/server/listener_manager.h index e9e902379203..0c5fb82ad3a8 100644 --- a/envoy/server/listener_manager.h +++ b/envoy/server/listener_manager.h @@ -14,6 +14,7 @@ #include "envoy/server/drain_manager.h" #include "envoy/server/filter_config.h" #include "envoy/server/guarddog.h" +#include "envoy/filter/config_provider_manager.h" #include "source/common/protobuf/protobuf.h" @@ -88,9 +89,9 @@ class ListenerComponentFactory { * Creates a list of listener filter factories. * @param filters supplies the JSON configuration. * @param context supplies the factory creation context. - * @return std::vector the list of filter factories. + * @return ListenerFilterFactoriesList the list of filter factories. */ - virtual std::vector createListenerFilterFactoryList( + virtual Filter::ListenerFilterFactoriesList createListenerFilterFactoryList( const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) PURE; @@ -98,9 +99,9 @@ class ListenerComponentFactory { * Creates a list of UDP listener filter factories. * @param filters supplies the configuration. * @param context supplies the factory creation context. - * @return std::vector the list of filter factories. + * @return UdpListenerFilterFactoriesList the list of filter factories. */ - virtual std::vector createUdpListenerFilterFactoryList( + virtual Filter::UdpListenerFilterFactoriesList createUdpListenerFilterFactoryList( const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) PURE; diff --git a/source/common/filter/config_discovery_impl.h b/source/common/filter/config_discovery_impl.h index 7285b46e0595..528fd934a3a2 100644 --- a/source/common/filter/config_discovery_impl.h +++ b/source/common/filter/config_discovery_impl.h @@ -169,7 +169,8 @@ class HttpDynamicFilterConfigProviderImpl ProtobufTypes::MessagePtr&& default_config, bool last_filter_in_filter_chain, const std::string& filter_chain_type, - absl::string_view stat_prefix) + absl::string_view stat_prefix, + const Network::ListenerFilterMatcherSharedPtr&) : DynamicFilterConfigProviderImpl(subscription, require_type_urls, factory_context, std::move(default_config), last_filter_in_filter_chain, filter_chain_type, stat_prefix), @@ -203,17 +204,19 @@ class ListenerDynamicFilterConfigProviderImpl : public DynamicFilterConfigProvid const absl::flat_hash_set& require_type_urls, Server::Configuration::ListenerFactoryContext& factory_context, ProtobufTypes::MessagePtr&& default_config, bool last_filter_in_filter_chain, - const std::string& filter_chain_type, absl::string_view stat_prefix) + const std::string& filter_chain_type, absl::string_view stat_prefix, + const Network::ListenerFilterMatcherSharedPtr& listener_filter_matcher) : DynamicFilterConfigProviderImpl( subscription, require_type_urls, factory_context, std::move(default_config), last_filter_in_filter_chain, filter_chain_type, stat_prefix), - factory_context_(factory_context) {} + factory_context_(factory_context), listener_filter_matcher_(listener_filter_matcher){} void validateMessage(const std::string&, const Protobuf::Message&, const std::string&) const override {} protected: Server::Configuration::ListenerFactoryContext& factory_context_; + const Network::ListenerFilterMatcherSharedPtr listener_filter_matcher_; }; class TcpListenerDynamicFilterConfigProviderImpl @@ -227,8 +230,7 @@ class TcpListenerDynamicFilterConfigProviderImpl auto* factory = Registry::FactoryRegistry:: getFactoryByType(message.GetTypeName()); - // TODO(yanjunxiang): Change nullptr to actual listener filter matcher. - return factory->createListenerFilterFactoryFromProto(message, nullptr, factory_context_); + return factory->createListenerFilterFactoryFromProto(message, listener_filter_matcher_, factory_context_); } }; @@ -385,7 +387,8 @@ class FilterConfigProviderManagerImpl : public FilterConfigProviderManagerImplBa const envoy::config::core::v3::ExtensionConfigSource& config_source, const std::string& filter_config_name, FactoryCtx& factory_context, const std::string& stat_prefix, bool last_filter_in_filter_chain, - const std::string& filter_chain_type) override { + const std::string& filter_chain_type, + const Network::ListenerFilterMatcherSharedPtr& listener_filter_matcher) override { std::string subscription_stat_prefix; absl::string_view provider_stat_prefix; if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.top_level_ecds_stats")) { @@ -421,7 +424,7 @@ class FilterConfigProviderManagerImpl : public FilterConfigProviderManagerImplBa auto provider = createFilterConfigProviderImpl( subscription, require_type_urls, factory_context, std::move(default_config), - last_filter_in_filter_chain, filter_chain_type, provider_stat_prefix); + last_filter_in_filter_chain, filter_chain_type, provider_stat_prefix, listener_filter_matcher); // Ensure the subscription starts if it has not already. if (config_source.apply_default_config_without_warming()) { @@ -476,10 +479,11 @@ class FilterConfigProviderManagerImpl : public FilterConfigProviderManagerImplBa FilterConfigSubscriptionSharedPtr& subscription, const absl::flat_hash_set& require_type_urls, FactoryCtx& factory_context, ProtobufTypes::MessagePtr&& default_config, bool last_filter_in_filter_chain, - const std::string& filter_chain_type, absl::string_view stat_prefix) { + const std::string& filter_chain_type, absl::string_view stat_prefix, + const Network::ListenerFilterMatcherSharedPtr& listener_filter_matcher) { return std::make_unique( subscription, require_type_urls, factory_context, std::move(default_config), - last_filter_in_filter_chain, filter_chain_type, stat_prefix); + last_filter_in_filter_chain, filter_chain_type, stat_prefix, listener_filter_matcher); } }; diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index a23df90a6d27..ae31cb43e3d0 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -683,7 +683,7 @@ void HttpConnectionManagerConfig::processDynamicFilterConfig( auto filter_config_provider = filter_config_provider_manager_.createDynamicFilterConfigProvider( config_discovery, name, context_, stats_prefix_, last_filter_in_current_config, - filter_chain_type); + filter_chain_type, nullptr); filter_factories.push_back(std::move(filter_config_provider)); } diff --git a/source/server/BUILD b/source/server/BUILD index 043c78652443..8e3619afc586 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -32,6 +32,7 @@ envoy_cc_library( hdrs = ["configuration_impl.h"], deps = [ "//envoy/config:typed_config_interface", + "//envoy/filter:config_provider_manager_interface", "//envoy/http:filter_interface", "//envoy/network:connection_interface", "//envoy/network:filter_interface", diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index 70fc5c7e06e1..1854bf1eee0e 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -148,15 +148,17 @@ class ValidationInstance final : Logger::Loggable, return ProdListenerComponentFactory::createNetworkFilterFactoryList_( filters, filter_chain_factory_context); } - std::vector createListenerFilterFactoryList( + ListenerFilterFactoriesList createListenerFilterFactoryList( const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) override { - return ProdListenerComponentFactory::createListenerFilterFactoryList_(filters, context); + return ProdListenerComponentFactory::createListenerFilterFactoryList_( + filters, context, listener_manager_->getTcpListenerConfigProviderManager()); } - std::vector createUdpListenerFilterFactoryList( + UdpListenerFilterFactoriesList createUdpListenerFilterFactoryList( const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) override { - return ProdListenerComponentFactory::createUdpListenerFilterFactoryList_(filters, context); + return ProdListenerComponentFactory::createUdpListenerFilterFactoryList_( + filters, context, listener_manager_->getUdpListenerConfigProviderManager()); } Network::SocketSharedPtr createListenSocket(Network::Address::InstanceConstSharedPtr, Network::Socket::Type, diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index d5abc1439b7a..b5196ad4cc29 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -38,21 +38,90 @@ bool FilterChainUtility::buildFilterChain(Network::FilterManager& filter_manager return filter_manager.initializeReadFilters(); } +class MissingConfigTcpListenerFilter : public Network::ListenerFilter, + public Logger::Loggable { +public: + MissingConfigTcpListenerFilter() = default; + + // Network::ListenerFilter + Network::FilterStatus onAccept(Network::ListenerFilterCallbacks& cb) override { + cb_ = &cb; + ENVOY_LOG(warn, "Listener filter: new connection accepted while missing configuration. " + "Close socket and stop the iteration onAccept."); + cb_->socket().ioHandle().close(); + return Network::FilterStatus::StopIteration; + } + Network::FilterStatus onData(Network::ListenerFilterBuffer&) override { + // The socket is already closed onAccept. Just return StopIteration here. + return Network::FilterStatus::StopIteration; + } + size_t maxReadBytes() const override { return 1024; } + +private: + Network::ListenerFilterCallbacks* cb_{}; +}; + + bool FilterChainUtility::buildFilterChain( - Network::ListenerFilterManager& filter_manager, - const std::vector& factories) { - for (const Network::ListenerFilterFactoryCb& factory : factories) { - factory(filter_manager); + Network::ListenerFilterManager& filter_manager, const Filter::ListenerFilterFactoriesList& factories) { + bool added_missing_config_filter = false; + for (const auto& filter_config_provider : factories) { + auto config = filter_config_provider->config(); + if (config.has_value()) { + auto config_value = config.value(); + config_value(filter_manager); + continue; + } + // If a filter config is missing after warming, stop iteration. + if (!added_missing_config_filter) { + ENVOY_LOG_MISC(trace, "Missing filter config for a provider {}", filter_config_provider->name()); + filter_manager.addAcceptFilter(nullptr, std::make_unique()); + added_missing_config_filter = true; + } else { + ENVOY_LOG_MISC(trace, "Provider {} missing a filter config", filter_config_provider->name()); + } + } return true; } + +class MissingConfigUdpListenerFilter : public Network::UdpListenerReadFilter { +public: + MissingConfigUdpListenerFilter(Network::UdpReadFilterCallbacks& callbacks) + : UdpListenerReadFilter(callbacks) {} + + // Network::UdpListenerReadFilter callbacks + Network::FilterStatus onData(Network::UdpRecvData&) override { + return Network::FilterStatus::StopIteration; + } + Network::FilterStatus onReceiveError(Api::IoError::IoErrorCode) override { + return Network::FilterStatus::StopIteration; + } +}; + void FilterChainUtility::buildUdpFilterChain( Network::UdpListenerFilterManager& filter_manager, Network::UdpReadFilterCallbacks& callbacks, - const std::vector& factories) { - for (const Network::UdpListenerFilterFactoryCb& factory : factories) { - factory(filter_manager, callbacks); + const Filter::UdpListenerFilterFactoriesList& factories) { + + bool added_missing_config_filter = false; + + for (const auto& filter_config_provider : factories) { + auto config = filter_config_provider->config(); + if (config.has_value()) { + auto config_value = config.value(); + config_value(filter_manager, callbacks); + continue; + } + // If a UDP filter config is missing after warming, stop iteration. + if (!added_missing_config_filter) { + ENVOY_LOG_MISC(trace, "Missing UDP filter config for a provider {}", filter_config_provider->name()); + filter_manager.addReadFilter(std::make_unique(callbacks)); + added_missing_config_filter = true; + } else { + ENVOY_LOG_MISC(trace, "Provider {} missing a UDP filter config", filter_config_provider->name()); + } } } diff --git a/source/server/configuration_impl.h b/source/server/configuration_impl.h index 1539639d7f81..2e25998ecb39 100644 --- a/source/server/configuration_impl.h +++ b/source/server/configuration_impl.h @@ -11,6 +11,7 @@ #include "envoy/config/bootstrap/v3/bootstrap.pb.h" #include "envoy/config/trace/v3/http_tracer.pb.h" #include "envoy/config/typed_config.h" +#include "envoy/filter/config_provider_manager.h" #include "envoy/http/filter.h" #include "envoy/network/filter.h" #include "envoy/server/configuration.h" @@ -81,7 +82,7 @@ class FilterChainUtility { * TODO(sumukhs): Coalesce with the above as they are very similar */ static bool buildFilterChain(Network::ListenerFilterManager& filter_manager, - const std::vector& factories); + const Filter::ListenerFilterFactoriesList& factories); /** * Given a UdpListenerFilterManager and a list of factories, create a new filter chain. Chain @@ -90,7 +91,7 @@ class FilterChainUtility { static void buildUdpFilterChain(Network::UdpListenerFilterManager& filter_manager, Network::UdpReadFilterCallbacks& callbacks, - const std::vector& factories); + const Filter::UdpListenerFilterFactoriesList& factories); }; /** diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index 957c62694279..74390afc8e8e 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -701,9 +701,12 @@ void ListenerImpl::buildOriginalDstListenerFilter() { Config::Utility::getAndCheckFactoryByName( "envoy.filters.listener.original_dst"); - listener_filter_factories_.push_back(factory.createListenerFilterFactoryFromProto( - Envoy::ProtobufWkt::Empty(), - /*listener_filter_matcher=*/nullptr, *listener_factory_context_)); + Network::ListenerFilterFactoryCb callback = factory.createListenerFilterFactoryFromProto( + Envoy::ProtobufWkt::Empty(), nullptr, *listener_factory_context_ ); + auto cfg_provider = parent_.getTcpListenerConfigProviderManager(); + auto filter_config_provider = cfg_provider.createStaticFilterConfigProvider( + callback, "envoy.filters.listener.original_dst"); + listener_filter_factories_.push_back(std::move(filter_config_provider)); } } @@ -716,9 +719,14 @@ void ListenerImpl::buildProxyProtocolListenerFilter() { auto& factory = Config::Utility::getAndCheckFactoryByName( "envoy.filters.listener.proxy_protocol"); - listener_filter_factories_.push_back(factory.createListenerFilterFactoryFromProto( + + Network::ListenerFilterFactoryCb callback = factory.createListenerFilterFactoryFromProto( envoy::extensions::filters::listener::proxy_protocol::v3::ProxyProtocol(), - /*listener_filter_matcher=*/nullptr, *listener_factory_context_)); + nullptr, *listener_factory_context_ ); + auto cfg_provider = parent_.getTcpListenerConfigProviderManager(); + auto filter_config_provider = cfg_provider.createStaticFilterConfigProvider( + callback, "envoy.filters.listener.proxy_protocol"); + listener_filter_factories_.push_back(std::move(filter_config_provider)); } } diff --git a/source/server/listener_impl.h b/source/server/listener_impl.h index 8e2eee26d7bf..3beae34669ef 100644 --- a/source/server/listener_impl.h +++ b/source/server/listener_impl.h @@ -6,6 +6,7 @@ #include "envoy/config/core/v3/base.pb.h" #include "envoy/config/listener/v3/listener.pb.h" #include "envoy/config/typed_metadata.h" +#include "envoy/filter/config_provider_manager.h" #include "envoy/network/drain_decision.h" #include "envoy/network/filter.h" #include "envoy/network/listener.h" @@ -438,8 +439,8 @@ class ListenerImpl final : public Network::ListenerConfig, // RdsRouteConfigSubscription::init_target_, so the listener can wait for route configs. std::unique_ptr dynamic_init_manager_; - std::vector listener_filter_factories_; - std::vector udp_listener_filter_factories_; + Filter::ListenerFilterFactoriesList listener_filter_factories_; + Filter::UdpListenerFilterFactoriesList udp_listener_filter_factories_; std::vector access_logs_; const envoy::config::listener::v3::Listener config_; const std::string version_info_; diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index c3edc35d1847..476cff096662 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -108,11 +108,11 @@ std::vector ProdListenerComponentFactory::createNetwor return ret; } -std::vector -ProdListenerComponentFactory::createListenerFilterFactoryList_( +Filter::ListenerFilterFactoriesList ProdListenerComponentFactory::createListenerFilterFactoryList_( const Protobuf::RepeatedPtrField& filters, - Configuration::ListenerFactoryContext& context) { - std::vector ret; + Configuration::ListenerFactoryContext& context, + Filter::TcpListenerFilterConfigProviderManagerImpl& config_provider_manager) { + Filter::ListenerFilterFactoriesList ret; for (ssize_t i = 0; i < filters.size(); i++) { const auto& proto_config = filters[i]; ENVOY_LOG(debug, " filter #{}:", i); @@ -120,24 +120,57 @@ ProdListenerComponentFactory::createListenerFilterFactoryList_( ENVOY_LOG(debug, " config: {}", MessageUtil::getJsonStringFromMessageOrError( static_cast(proto_config.typed_config()))); + // dynamic listener filter configuration + if (proto_config.config_type_case() == + envoy::config::listener::v3::ListenerFilter::ConfigTypeCase::kConfigDiscovery) { + auto& config_discovery = proto_config.config_discovery(); + auto& name = proto_config.name(); + ENVOY_LOG(debug, " Listener filter: dynamic filter name: {}", name); + if (config_discovery.apply_default_config_without_warming() && + !config_discovery.has_default_config()) { + throw EnvoyException(fmt::format( + "Error: listener filter config {} applied without warming but has no default config.", name)); + } + for (const auto& type_url : config_discovery.type_urls()) { + auto factory_type_url = TypeUtil::typeUrlToDescriptorFullName(type_url); + auto* factory = Registry::FactoryRegistry< + Server::Configuration::NamedListenerFilterConfigFactory>::getFactoryByType(factory_type_url); + if (factory == nullptr) { + throw EnvoyException( + fmt::format("Error: no listener factory found for a required type URL {}.", factory_type_url)); + } + } + auto filter_config_provider = config_provider_manager.createDynamicFilterConfigProvider( + config_discovery, name, context, "tcp_listener.", false, "listener", + createListenerFilterMatcher(proto_config)); + ret.push_back(std::move(filter_config_provider)); + return ret; + } - // Now see if there is a factory that will accept the config. + // For static configuration, now see if there is a factory that will accept the config. auto& factory = Config::Utility::getAndCheckFactory( proto_config); auto message = Config::Utility::translateToFactoryConfig( proto_config, context.messageValidationVisitor(), factory); - ret.push_back(factory.createListenerFilterFactoryFromProto( - *message, createListenerFilterMatcher(proto_config), context)); + + Network::ListenerFilterFactoryCb callback = factory.createListenerFilterFactoryFromProto( + *message, createListenerFilterMatcher(proto_config), context); + + auto filter_config_provider = config_provider_manager.createStaticFilterConfigProvider( + callback, proto_config.name()); + + ENVOY_LOG(debug, " name: {}", filter_config_provider->name()); + ret.push_back(std::move(filter_config_provider)); } return ret; } -std::vector -ProdListenerComponentFactory::createUdpListenerFilterFactoryList_( +Filter::UdpListenerFilterFactoriesList ProdListenerComponentFactory::createUdpListenerFilterFactoryList_( const Protobuf::RepeatedPtrField& filters, - Configuration::ListenerFactoryContext& context) { - std::vector ret; + Configuration::ListenerFactoryContext& context, + Filter::UdpListenerFilterConfigProviderManagerImpl& config_provider_manager) { + Filter::UdpListenerFilterFactoriesList ret; for (ssize_t i = 0; i < filters.size(); i++) { const auto& proto_config = filters[i]; ENVOY_LOG(debug, " filter #{}:", i); @@ -145,15 +178,47 @@ ProdListenerComponentFactory::createUdpListenerFilterFactoryList_( ENVOY_LOG(debug, " config: {}", MessageUtil::getJsonStringFromMessageOrError( static_cast(proto_config.typed_config()))); + // dynamic UDP listener filter configuration + if (proto_config.config_type_case() == + envoy::config::listener::v3::ListenerFilter::ConfigTypeCase::kConfigDiscovery) { + auto& config_discovery = proto_config.config_discovery(); + auto& name = proto_config.name(); + ENVOY_LOG(debug, " UDP Listener filter: dynamic filter name: {}", name); + + if (config_discovery.apply_default_config_without_warming() && + !config_discovery.has_default_config()) { + throw EnvoyException(fmt::format( + "Error: listener filter config {} applied without warming but has no default config.", name)); + } - // Now see if there is a factory that will accept the config. + for (const auto& type_url : config_discovery.type_urls()) { + auto factory_type_url = TypeUtil::typeUrlToDescriptorFullName(type_url); + auto* factory = Registry::FactoryRegistry< + Server::Configuration::NamedUdpListenerFilterConfigFactory>::getFactoryByType(factory_type_url); + if (factory == nullptr) { + throw EnvoyException( + fmt::format("Error: no UDP listener factory found for a required type URL {}.", factory_type_url)); + } + } + + auto filter_config_provider = config_provider_manager.createDynamicFilterConfigProvider( + config_discovery, name, context, "udp_listener.", false, "listener", nullptr); + ret.push_back(std::move(filter_config_provider)); + return ret; + } + + // For static configuration, Now see if there is a factory that will accept the config. auto& factory = Config::Utility::getAndCheckFactory( proto_config); - auto message = Config::Utility::translateToFactoryConfig( proto_config, context.messageValidationVisitor(), factory); - ret.push_back(factory.createFilterFactoryFromProto(*message, context)); + Network::UdpListenerFilterFactoryCb callback = factory.createFilterFactoryFromProto( + *message, context); + auto filter_config_provider = config_provider_manager.createStaticFilterConfigProvider( + callback, proto_config.name()); + ENVOY_LOG(debug, " name: {}", filter_config_provider->name()); + ret.push_back(std::move(filter_config_provider)); } return ret; } @@ -253,6 +318,9 @@ ListenerManagerImpl::ListenerManagerImpl(Instance& server, workers_.emplace_back( worker_factory.createWorker(i, server.overloadManager(), absl::StrCat("worker_", i))); } + // Create TCP/UDP listener filter config provider manager instance. + tcp_listener_config_provider_manager_ = std::make_unique(); + udp_listener_config_provider_manager_ = std::make_unique(); } ProtobufTypes::MessagePtr diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index e6d93f80074c..793c126d97e3 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -18,6 +18,7 @@ #include "envoy/server/worker.h" #include "envoy/stats/scope.h" +#include "source/common/filter/config_discovery_impl.h" #include "source/common/quic/quic_stat_names.h" #include "source/server/filter_chain_factory_context_callback.h" #include "source/server/filter_chain_manager_impl.h" @@ -33,78 +34,6 @@ class TransportSocketFactoryContextImpl; class ListenerFilterChainFactoryBuilder; -/** - * Prod implementation of ListenerComponentFactory that creates real sockets and attempts to fetch - * sockets from the parent process via the hot restarter. The filter factory list is created from - * statically registered filters. - */ -class ProdListenerComponentFactory : public ListenerComponentFactory, - Logger::Loggable { -public: - ProdListenerComponentFactory(Instance& server) : server_(server) {} - - /** - * Static worker for createNetworkFilterFactoryList() that can be used directly in tests. - */ - static std::vector createNetworkFilterFactoryList_( - const Protobuf::RepeatedPtrField& filters, - Configuration::FilterChainFactoryContext& filter_chain_factory_context); - - /** - * Static worker for createListenerFilterFactoryList() that can be used directly in tests. - */ - static std::vector createListenerFilterFactoryList_( - const Protobuf::RepeatedPtrField& filters, - Configuration::ListenerFactoryContext& context); - - /** - * Static worker for createUdpListenerFilterFactoryList() that can be used directly in tests. - */ - static std::vector createUdpListenerFilterFactoryList_( - const Protobuf::RepeatedPtrField& filters, - Configuration::ListenerFactoryContext& context); - - static Network::ListenerFilterMatcherSharedPtr - createListenerFilterMatcher(const envoy::config::listener::v3::ListenerFilter& listener_filter); - - // Server::ListenerComponentFactory - LdsApiPtr createLdsApi(const envoy::config::core::v3::ConfigSource& lds_config, - const xds::core::v3::ResourceLocator* lds_resources_locator) override { - return std::make_unique( - lds_config, lds_resources_locator, server_.clusterManager(), server_.initManager(), - server_.stats(), server_.listenerManager(), - server_.messageValidationContext().dynamicValidationVisitor()); - } - std::vector createNetworkFilterFactoryList( - const Protobuf::RepeatedPtrField& filters, - Server::Configuration::FilterChainFactoryContext& filter_chain_factory_context) override { - return createNetworkFilterFactoryList_(filters, filter_chain_factory_context); - } - std::vector createListenerFilterFactoryList( - const Protobuf::RepeatedPtrField& filters, - Configuration::ListenerFactoryContext& context) override { - return createListenerFilterFactoryList_(filters, context); - } - std::vector createUdpListenerFilterFactoryList( - const Protobuf::RepeatedPtrField& filters, - Configuration::ListenerFactoryContext& context) override { - return createUdpListenerFilterFactoryList_(filters, context); - } - - Network::SocketSharedPtr createListenSocket( - Network::Address::InstanceConstSharedPtr address, Network::Socket::Type socket_type, - const Network::Socket::OptionsSharedPtr& options, BindType bind_type, - const Network::SocketCreationOptions& creation_options, uint32_t worker_index) override; - - DrainManagerPtr - createDrainManager(envoy::config::listener::v3::Listener::DrainType drain_type) override; - uint64_t nextListenerTag() override { return next_listener_tag_++; } - -private: - Instance& server_; - uint64_t next_listener_tag_{1}; -}; - class ListenerImpl; using ListenerImplPtr = std::unique_ptr; @@ -205,6 +134,12 @@ class ListenerManagerImpl : public ListenerManager, Logger::Loggable> error_state_tracker_; FailureStates overall_error_state_; Quic::QuicStatNames& quic_stat_names_; + std::unique_ptr tcp_listener_config_provider_manager_; + std::unique_ptr udp_listener_config_provider_manager_; +}; + +/** + * Prod implementation of ListenerComponentFactory that creates real sockets and attempts to fetch + * sockets from the parent process via the hot restarter. The filter factory list is created from + * statically registered filters. + */ +class ProdListenerComponentFactory : public ListenerComponentFactory, + Logger::Loggable { +public: + ProdListenerComponentFactory(Instance& server) : server_(server) {} + + /** + * Static worker for createNetworkFilterFactoryList() that can be used directly in tests. + */ + static std::vector createNetworkFilterFactoryList_( + const Protobuf::RepeatedPtrField& filters, + Configuration::FilterChainFactoryContext& filter_chain_factory_context); + + /** + * Static worker for createListenerFilterFactoryList() that can be used directly in tests. + */ + static Filter::ListenerFilterFactoriesList createListenerFilterFactoryList_( + const Protobuf::RepeatedPtrField& filters, + Configuration::ListenerFactoryContext& context, + Filter::TcpListenerFilterConfigProviderManagerImpl& config_provider_manager); + + /** + * Static worker for createUdpListenerFilterFactoryList() that can be used directly in tests. + */ + static Filter::UdpListenerFilterFactoriesList createUdpListenerFilterFactoryList_( + const Protobuf::RepeatedPtrField& filters, + Configuration::ListenerFactoryContext& context, + Filter::UdpListenerFilterConfigProviderManagerImpl& config_provider_manager); + + static Network::ListenerFilterMatcherSharedPtr + createListenerFilterMatcher(const envoy::config::listener::v3::ListenerFilter& listener_filter); + + // Server::ListenerComponentFactory + LdsApiPtr createLdsApi(const envoy::config::core::v3::ConfigSource& lds_config, + const xds::core::v3::ResourceLocator* lds_resources_locator) override { + return std::make_unique( + lds_config, lds_resources_locator, server_.clusterManager(), server_.initManager(), + server_.stats(), server_.listenerManager(), + server_.messageValidationContext().dynamicValidationVisitor()); + } + std::vector createNetworkFilterFactoryList( + const Protobuf::RepeatedPtrField& filters, + Server::Configuration::FilterChainFactoryContext& filter_chain_factory_context) override { + return createNetworkFilterFactoryList_(filters, filter_chain_factory_context); + } + Filter::ListenerFilterFactoriesList createListenerFilterFactoryList( + const Protobuf::RepeatedPtrField& filters, + Configuration::ListenerFactoryContext& context) override { + ListenerManagerImpl * listener_manager = dynamic_cast(&(server_.listenerManager())); + return createListenerFilterFactoryList_(filters, context, + listener_manager->getTcpListenerConfigProviderManager()); + } + Filter::UdpListenerFilterFactoriesList createUdpListenerFilterFactoryList( + const Protobuf::RepeatedPtrField& filters, + Configuration::ListenerFactoryContext& context) override { + ListenerManagerImpl * listener_manager = dynamic_cast(&(server_.listenerManager())); + return createUdpListenerFilterFactoryList_(filters, context, + listener_manager->getUdpListenerConfigProviderManager()); + } + Network::SocketSharedPtr createListenSocket( + Network::Address::InstanceConstSharedPtr address, Network::Socket::Type socket_type, + const Network::Socket::OptionsSharedPtr& options, BindType bind_type, + const Network::SocketCreationOptions& creation_options, uint32_t worker_index) override; + + DrainManagerPtr + createDrainManager(envoy::config::listener::v3::Listener::DrainType drain_type) override; + uint64_t nextListenerTag() override { return next_listener_tag_++; } + +private: + Instance& server_; + uint64_t next_listener_tag_{1}; }; class ListenerFilterChainFactoryBuilder : public FilterChainFactoryBuilder { diff --git a/test/common/filter/config_discovery_impl_test.cc b/test/common/filter/config_discovery_impl_test.cc index 6ae77285e58f..5a8824026d13 100644 --- a/test/common/filter/config_discovery_impl_test.cc +++ b/test/common/filter/config_discovery_impl_test.cc @@ -103,7 +103,8 @@ class FilterConfigDiscoveryImplTest : public FilterConfigDiscoveryTestBase { } return filter_config_provider_manager_->createDynamicFilterConfigProvider( - config_source, name, factory_context_, "xds.", last_filter_config, getFilterType()); + config_source, name, factory_context_, "xds.", last_filter_config, getFilterType(), + getFilterMatcher()); } void setup(bool warm = true, bool default_configuration = false, bool last_filter_config = true) { @@ -153,6 +154,7 @@ class FilterConfigDiscoveryImplTest : public FilterConfigDiscoveryTestBase { virtual const std::string getTypeUrl() const PURE; virtual const std::string getFilterType() const PURE; + virtual const Network::ListenerFilterMatcherSharedPtr& getFilterMatcher() const PURE; virtual const std::string getConfigReloadCounter() const PURE; virtual const std::string getConfigFailCounter() const PURE; @@ -173,6 +175,9 @@ class HttpFilterConfigDiscoveryImplTest return "envoy.extensions.filters.http.router.v3.Router"; } const std::string getFilterType() const override { return "http"; } + const Network::ListenerFilterMatcherSharedPtr& getFilterMatcher() const override { + return nullptr; + } const std::string getConfigReloadCounter() const override { return "extension_config_discovery.http_filter.foo.config_reload"; } @@ -189,6 +194,9 @@ class TcpListenerFilterConfigDiscoveryImplTest public: const std::string getTypeUrl() const override { return "google.protobuf.Struct"; } const std::string getFilterType() const override { return "listener"; } + const Network::ListenerFilterMatcherSharedPtr& getFilterMatcher() const override { + return nullptr; + } const std::string getConfigReloadCounter() const override { return "extension_config_discovery.tcp_listener_filter.foo.config_reload"; } @@ -208,6 +216,9 @@ class UdpListenerFilterConfigDiscoveryImplTest return "test.integration.filters.TestUdpListenerFilterConfig"; } const std::string getFilterType() const override { return "listener"; } + const Network::ListenerFilterMatcherSharedPtr& getFilterMatcher() const override { + return nullptr; + } const std::string getConfigReloadCounter() const override { return "extension_config_discovery.udp_listener_filter.foo.config_reload"; } diff --git a/test/integration/BUILD b/test/integration/BUILD index df4de923c0b5..af094351132b 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -1198,6 +1198,22 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "listener_extension_discovery_integration_test", + srcs = ["listener_extension_discovery_integration_test.cc"], + deps = [ + ":http_integration_lib", + "//test/common/grpc:grpc_client_integration_lib", + "//test/integration/filters:test_listener_filter_lib", + "//source/extensions/filters/network/tcp_proxy:config", + "//test/test_common:utility_lib", + "@envoy_api//envoy/extensions/common/matching/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto", + "@envoy_api//envoy/service/discovery/v3:pkg_cc_proto", + "@envoy_api//envoy/service/extension/v3:pkg_cc_proto", + ], +) + envoy_cc_test_library( name = "server_stats_interface", hdrs = ["server_stats.h"], diff --git a/test/integration/filters/test_listener_filter.cc b/test/integration/filters/test_listener_filter.cc index 0951083c6f8e..c8897a9ec1e1 100644 --- a/test/integration/filters/test_listener_filter.cc +++ b/test/integration/filters/test_listener_filter.cc @@ -1,6 +1,7 @@ #include "test/integration/filters/test_listener_filter.h" #include "test/integration/filters/test_listener_filter.pb.h" +#include "test/integration/filters/test_listener_filter.pb.validate.h" namespace Envoy { @@ -30,6 +31,35 @@ class TestInspectorConfigFactory : public Server::Configuration::NamedListenerFi absl::Mutex TestListenerFilter::alpn_lock_; std::string TestListenerFilter::alpn_; + +/** + * Config registration for the test TCP listener filter. + */ +class TestTcpInspectorConfigFactory : public Server::Configuration::NamedListenerFilterConfigFactory { +public: + // NamedListenerFilterConfigFactory + Network::ListenerFilterFactoryCb createListenerFilterFactoryFromProto( + const Protobuf::Message& proto_config, + const Network::ListenerFilterMatcherSharedPtr& listener_filter_matcher, + Server::Configuration::ListenerFactoryContext& context) override { + const auto& message = MessageUtil::downcastAndValidate< + const test::integration::filters::TestTcpListenerFilterConfig&>( + proto_config, context.messageValidationVisitor()); + return [listener_filter_matcher, message](Network::ListenerFilterManager& filter_manager) -> void { + filter_manager.addAcceptFilter(listener_filter_matcher, + std::make_unique(message.drain_bytes())); + }; + } + + ProtobufTypes::MessagePtr createEmptyConfigProto() override { + return ProtobufTypes::MessagePtr{new test::integration::filters::TestTcpListenerFilterConfig()}; + } + + std::string name() const override { return "envoy.filters.tcp_listener.test"; } +}; + + + /** * Config registration for the UDP test filter. */ @@ -54,8 +84,11 @@ class TestUdpInspectorConfigFactory }; REGISTER_FACTORY(TestInspectorConfigFactory, - Server::Configuration::NamedListenerFilterConfigFactory){"envoy.listener.test"}; + Server::Configuration::NamedListenerFilterConfigFactory); + +REGISTER_FACTORY(TestTcpInspectorConfigFactory, + Server::Configuration::NamedListenerFilterConfigFactory); REGISTER_FACTORY(TestUdpInspectorConfigFactory, - Server::Configuration::NamedUdpListenerFilterConfigFactory){"envoy.listener.test"}; + Server::Configuration::NamedUdpListenerFilterConfigFactory); } // namespace Envoy diff --git a/test/integration/filters/test_listener_filter.h b/test/integration/filters/test_listener_filter.h index dfb71bae40cd..96eec119f4a3 100644 --- a/test/integration/filters/test_listener_filter.h +++ b/test/integration/filters/test_listener_filter.h @@ -32,6 +32,32 @@ class TestListenerFilter : public Network::ListenerFilter { static std::string alpn_; }; +/** + * Test TCP listener filter. + */ +class TestTcpListenerFilter : public Network::ListenerFilter { +public: + TestTcpListenerFilter(const uint32_t drain_bytes) : drain_bytes_(drain_bytes){} + + Network::FilterStatus onAccept(Network::ListenerFilterCallbacks&) override { + return Network::FilterStatus::StopIteration; + } + + Network::FilterStatus onData(Network::ListenerFilterBuffer& buffer) override { + // Drain some bytes when onData. + if (drain_bytes_ && drain_bytes_ <= buffer.rawSlice().len_) { + buffer.drain(drain_bytes_); + } + return Network::FilterStatus::Continue; + } + + // Returning a non-zero number. + size_t maxReadBytes() const override { return 1024; } + +private: + const uint32_t drain_bytes_; +}; + /** * Test UDP listener filter. */ diff --git a/test/integration/filters/test_listener_filter.proto b/test/integration/filters/test_listener_filter.proto index 51ae9d14142e..d04cda10ff9e 100644 --- a/test/integration/filters/test_listener_filter.proto +++ b/test/integration/filters/test_listener_filter.proto @@ -2,6 +2,13 @@ syntax = "proto3"; package test.integration.filters; +import "validate/validate.proto"; + +// Configuration for TCP listener filter test +message TestTcpListenerFilterConfig { + uint32 drain_bytes = 1 [(validate.rules).uint32 = {gte: 2}]; +} + // Configuration for UDP listener filter test message TestUdpListenerFilterConfig { } diff --git a/test/integration/listener_extension_discovery_integration_test.cc b/test/integration/listener_extension_discovery_integration_test.cc new file mode 100644 index 000000000000..5a3bd965796e --- /dev/null +++ b/test/integration/listener_extension_discovery_integration_test.cc @@ -0,0 +1,319 @@ +#include "envoy/service/discovery/v3/discovery.pb.h" +#include "envoy/service/extension/v3/config_discovery.pb.h" + +#include "test/common/grpc/grpc_client_integration.h" +#include "test/integration/filters/test_listener_filter.pb.h" +#include "envoy/extensions/filters/network/tcp_proxy/v3/tcp_proxy.pb.h" +#include "test/integration/filters/test_listener_filter.h" +#include "test/integration/integration.h" +#include "test/test_common/utility.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace { + +class ExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, + public BaseIntegrationTest { +public: + ExtensionDiscoveryIntegrationTest() + : BaseIntegrationTest(ipVersion(), ConfigHelper::baseConfig() + R"EOF( + filter_chains: + - filters: + - name: envoy.filters.network.tcp_proxy + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy + stat_prefix: tcp_stats + cluster: cluster_0 +)EOF") {} + + void addDynamicFilter(const std::string& name, bool apply_without_warming, + bool set_default_config = true, bool rate_limit = false) { + config_helper_.addConfigModifier([&]( envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + auto* listener_filter = bootstrap.mutable_static_resources()->mutable_listeners(0)->add_listener_filters(); + listener_filter->set_name(name); + + auto* discovery = listener_filter->mutable_config_discovery(); + discovery->add_type_urls( + "type.googleapis.com/test.integration.filters.TestTcpListenerFilterConfig"); + if (set_default_config) { + auto default_configuration = test::integration::filters::TestTcpListenerFilterConfig(); + default_configuration.set_drain_bytes(kDefaultDrainBytes); + discovery->mutable_default_config()->PackFrom(default_configuration); + } + + discovery->set_apply_default_config_without_warming(apply_without_warming); + discovery->mutable_config_source()->set_resource_api_version( + envoy::config::core::v3::ApiVersion::V3); + auto* api_config_source = discovery->mutable_config_source()->mutable_api_config_source(); + api_config_source->set_api_type(envoy::config::core::v3::ApiConfigSource::GRPC); + api_config_source->set_transport_api_version(envoy::config::core::v3::ApiVersion::V3); + if (rate_limit) { + api_config_source->mutable_rate_limit_settings()->mutable_max_tokens()->set_value(10); + } + auto* grpc_service = api_config_source->add_grpc_services(); + setGrpcService(*grpc_service, "ecds_cluster", getEcdsFakeUpstream().localAddress()); + }); + + } + + void initialize() override { + defer_listener_finalization_ = true; + setUpstreamCount(1); + + // Add an xDS cluster for extension config discovery. + config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + auto* ecds_cluster = bootstrap.mutable_static_resources()->add_clusters(); + ecds_cluster->MergeFrom(bootstrap.static_resources().clusters()[0]); + ecds_cluster->set_name("ecds_cluster"); + ConfigHelper::setHttp2(*ecds_cluster); + }); + BaseIntegrationTest::initialize(); + registerTestServerPorts({port_name_}); + } + + ~ExtensionDiscoveryIntegrationTest() override { + if (ecds_connection_ != nullptr) { + AssertionResult result = ecds_connection_->close(); + RELEASE_ASSERT(result, result.message()); + result = ecds_connection_->waitForDisconnect(); + RELEASE_ASSERT(result, result.message()); + ecds_connection_.reset(); + } + } + + void createUpstreams() override { + BaseIntegrationTest::createUpstreams(); + // Create the extension config discovery upstream (fake_upstreams_[1]). + addFakeUpstream(Http::CodecType::HTTP2); + } + + void waitXdsStream() { + // Wait for ECDS stream. + auto& ecds_upstream = getEcdsFakeUpstream(); + AssertionResult result = ecds_upstream.waitForHttpConnection(*dispatcher_, ecds_connection_); + RELEASE_ASSERT(result, result.message()); + result = ecds_connection_->waitForNewStream(*dispatcher_, ecds_stream_); + RELEASE_ASSERT(result, result.message()); + ecds_stream_->startGrpcStream(); + } + + void sendXdsResponse(const std::string& version, const uint32_t drain_bytes, bool ttl = false) { + // The to-be-drained bytes has to be smaller than data size. + ASSERT(drain_bytes <= data_.size()); + + envoy::service::discovery::v3::DiscoveryResponse response; + response.set_version_info(version); + response.set_type_url("type.googleapis.com/envoy.config.core.v3.TypedExtensionConfig"); + envoy::config::core::v3::TypedExtensionConfig typed_config; + typed_config.set_name(filter_name_); + envoy::service::discovery::v3::Resource resource; + resource.set_name(filter_name_); + + auto configuration = test::integration::filters::TestTcpListenerFilterConfig(); + configuration.set_drain_bytes(drain_bytes); + typed_config.mutable_typed_config()->PackFrom(configuration); + resource.mutable_resource()->PackFrom(typed_config); + if (ttl) { + resource.mutable_ttl()->set_seconds(1); + } + response.add_resources()->PackFrom(resource); + ecds_stream_->sendGrpcMessage(response); + } + + // Client sends data_, which is drained by Envoy listener filter based on config, then received by upstream. + void sendDataVerifyResults (uint32_t drain_bytes) { + test_server_->waitUntilListenersReady(); + EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); + + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort(port_name_)); + ASSERT_TRUE(tcp_client->write(data_)); + FakeRawConnectionPtr fake_upstream_connection; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + std::string received_data; + ASSERT_TRUE(fake_upstream_connection->waitForData(data_.size() - drain_bytes, &received_data)); + const std::string expected_data = data_.substr(drain_bytes, std::string::npos); + EXPECT_EQ(expected_data, received_data); + tcp_client->close(); + } + + const uint32_t kDefaultDrainBytes = 2; + const std::string filter_name_ = "foo"; + const std::string data_ = "HelloWorld"; + const std::string port_name_ = "http"; + + FakeUpstream& getEcdsFakeUpstream() const { return *fake_upstreams_[1]; } + + // gRPC ECDS set-up + FakeHttpConnectionPtr ecds_connection_{nullptr}; + FakeStreamPtr ecds_stream_{nullptr}; +}; + +INSTANTIATE_TEST_SUITE_P(IpVersionsClientType, ExtensionDiscoveryIntegrationTest, + GRPC_CLIENT_INTEGRATION_PARAMS); + +TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccess) { + on_server_init_function_ = [&]() { waitXdsStream(); }; + addDynamicFilter(filter_name_, false); + initialize(); + + EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); + + // Send 1st config update to have listener filter drain 5 bytes of data. + sendXdsResponse("1", 5); + test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_reload", 1); + sendDataVerifyResults(5); + + // Send 2nd config update to have listener filter drain 3 bytes of data. + sendXdsResponse("2", 3); + test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_reload", 2); + sendDataVerifyResults(3); +} + +TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithTtl) { + on_server_init_function_ = [&]() { waitXdsStream(); }; + addDynamicFilter(filter_name_, false, false); + initialize(); + + EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); + + // Send 1st config update with TTL 1s, and have listener filter drain 5 bytes of data. + sendXdsResponse("1", 5, true); + test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_reload", 1); + sendDataVerifyResults(5); + + // Wait for configuration expired. Then start a TCP connection. + // The missing config listener filter will be installed to handle the connection. + test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_reload", 2); + EXPECT_LOG_CONTAINS("warn", "Close socket and stop the iteration onAccept.", + { + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort(port_name_)); + auto result = tcp_client->write(data_); + if (result) { + tcp_client->waitForDisconnect(); + } + }); +} + +TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithTtlWithDefault) { + on_server_init_function_ = [&]() { waitXdsStream(); }; + addDynamicFilter(filter_name_, false, true); + initialize(); + + EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); + + // Send 1st config update with TTL 1s, and have listener filter drain 5 bytes of data. + sendXdsResponse("1", 5, true); + test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_reload", 1); + sendDataVerifyResults(5); + + // Wait for configuration expired. The default filter will be installed. + test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_reload", 2); + // Start a TCP connection. The default filter drain 2 bytes. + sendDataVerifyResults(kDefaultDrainBytes); +} + + +// This one TBD +TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailWithDefault) { + on_server_init_function_ = [&]() { waitXdsStream(); }; + addDynamicFilter(filter_name_, false, true); + initialize(); + + EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); + + // Send config update with invalid config (drain_bytes has to >=2). + sendXdsResponse("1", 1); + test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_fail", 1); + // The default filter will be installed. Start a TCP connection. The default filter drain 2 bytes. + sendDataVerifyResults(kDefaultDrainBytes); +} + +// This one TBD +TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailWithoutDefault) { + on_server_init_function_ = [&]() { waitXdsStream(); }; + addDynamicFilter(filter_name_, false, false); + initialize(); + + EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); + + // Send config update with invalid config (drain_bytes has to >=2). + sendXdsResponse("1", 1); + test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_fail", 1); + // The missing config filter will be installed when a correction is created. + // The missing config filter will close the connection. + EXPECT_LOG_CONTAINS("warn", "Close socket and stop the iteration onAccept.", + { + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort(port_name_)); + auto result = tcp_client->write(data_); + if (result) { + tcp_client->waitForDisconnect(); + } + }); +} + + +TEST_P(ExtensionDiscoveryIntegrationTest, BasicWithoutWarming) { + on_server_init_function_ = [&]() { waitXdsStream(); }; + addDynamicFilter(filter_name_, true); + initialize(); + + // Send data without send config update. + sendDataVerifyResults(kDefaultDrainBytes); + // Send update should cause a different response. + sendXdsResponse("1", 3); + test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_reload", 1); + sendDataVerifyResults(3); +} + +TEST_P(ExtensionDiscoveryIntegrationTest, BasicWithoutWarmingFail) { + on_server_init_function_ = [&]() { waitXdsStream(); }; + addDynamicFilter(filter_name_, true); + initialize(); + + sendXdsResponse("1", 1); + test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_fail", 1); + sendDataVerifyResults(kDefaultDrainBytes); +} + +TEST_P(ExtensionDiscoveryIntegrationTest, BasicTwoSubscriptionsSameNameWithoutWarming) { + on_server_init_function_ = [&]() { waitXdsStream(); }; + addDynamicFilter(filter_name_, true); + // Adding a filter with same name overrides the previous one. + addDynamicFilter(filter_name_, false); + initialize(); + + sendXdsResponse("1", 3); + test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_reload", 1); + sendDataVerifyResults(3); +} + +TEST_P(ExtensionDiscoveryIntegrationTest, BasicTwoSubscriptionsSameNameWithWarming) { + on_server_init_function_ = [&]() { waitXdsStream(); }; + addDynamicFilter(filter_name_, false); + // Adding a filter with same name overrides the previous one. + addDynamicFilter(filter_name_, true); + initialize(); + + sendXdsResponse("1", 3); + test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_reload", 1); + sendDataVerifyResults(3); +} + +TEST_P(ExtensionDiscoveryIntegrationTest, DestroyDuringInit) { + // If rate limiting is enabled on the config source, gRPC mux drainage updates the requests + // queue size on destruction. The update calls out to stats scope nested under the extension + // config subscription stats scope. This test verifies that the stats scope outlasts the gRPC + // subscription. + on_server_init_function_ = [&]() { waitXdsStream(); }; + addDynamicFilter("foo", false, true); + initialize(); + EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); + test_server_.reset(); + auto result = ecds_connection_->waitForDisconnect(); + RELEASE_ASSERT(result, result.message()); + ecds_connection_.reset(); +} + +} // namespace +} // namespace Envoy diff --git a/test/mocks/server/listener_component_factory.h b/test/mocks/server/listener_component_factory.h index 8711e0bdf70f..c536b57fb082 100644 --- a/test/mocks/server/listener_component_factory.h +++ b/test/mocks/server/listener_component_factory.h @@ -30,10 +30,10 @@ class MockListenerComponentFactory : public ListenerComponentFactory { MOCK_METHOD(std::vector, createNetworkFilterFactoryList, (const Protobuf::RepeatedPtrField& filters, Configuration::FilterChainFactoryContext& filter_chain_factory_context)); - MOCK_METHOD(std::vector, createListenerFilterFactoryList, + MOCK_METHOD(Filter::ListenerFilterFactoriesList, createListenerFilterFactoryList, (const Protobuf::RepeatedPtrField&, Configuration::ListenerFactoryContext& context)); - MOCK_METHOD(std::vector, createUdpListenerFilterFactoryList, + MOCK_METHOD(Filter::UdpListenerFilterFactoriesList, createUdpListenerFilterFactoryList, (const Protobuf::RepeatedPtrField&, Configuration::ListenerFactoryContext& context)); MOCK_METHOD(Network::SocketSharedPtr, createListenSocket, diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index edfa69b86511..79f33c02f8a8 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -4670,13 +4670,14 @@ TEST_P(ListenerManagerImplWithRealFiltersTest, Metadata) { // Extract listener_factory_context avoid accessing private member. ON_CALL(listener_factory_, createListenerFilterFactoryList(_, _)) .WillByDefault( - Invoke([&listener_factory_context]( + Invoke([&listener_factory_context, this]( const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) - -> std::vector { + -> Filter::ListenerFilterFactoriesList { listener_factory_context = &context; - return ProdListenerComponentFactory::createListenerFilterFactoryList_(filters, context); + return ProdListenerComponentFactory::createListenerFilterFactoryList_( + filters, context, manager_->getTcpListenerConfigProviderManager()); })); server_.server_factory_context_->cluster_manager_.initializeClusters({"service_foo"}, {}); addOrUpdateListener(parseListenerFromV3Yaml(yaml)); diff --git a/test/server/listener_manager_impl_test.h b/test/server/listener_manager_impl_test.h index bfa8756b4be8..bc9385193686 100644 --- a/test/server/listener_manager_impl_test.h +++ b/test/server/listener_manager_impl_test.h @@ -89,21 +89,23 @@ class ListenerManagerImplTest : public testing::TestWithParam { })); ON_CALL(listener_factory_, createListenerFilterFactoryList(_, _)) .WillByDefault( - Invoke([](const Protobuf::RepeatedPtrField& + Invoke([this](const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) - -> std::vector { + -> Filter::ListenerFilterFactoriesList { return ProdListenerComponentFactory::createListenerFilterFactoryList_(filters, - context); + context, + manager_->getTcpListenerConfigProviderManager()); })); ON_CALL(listener_factory_, createUdpListenerFilterFactoryList(_, _)) .WillByDefault( - Invoke([](const Protobuf::RepeatedPtrField& + Invoke([this](const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) - -> std::vector { + -> Filter::UdpListenerFilterFactoriesList { return ProdListenerComponentFactory::createUdpListenerFilterFactoryList_(filters, - context); + context, + manager_->getUdpListenerConfigProviderManager()); })); ON_CALL(listener_factory_, nextListenerTag()).WillByDefault(Invoke([this]() { return listener_tag_++; From 5e88f698f84c7b0c237d4b415ebb7ba37679ec08 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Tue, 3 May 2022 23:59:50 +0000 Subject: [PATCH 02/28] fixing format error Signed-off-by: Yanjun Xiang --- envoy/filter/config_provider_manager.h | 9 +- envoy/server/listener_manager.h | 2 +- source/common/filter/config_discovery_impl.h | 12 ++- source/server/configuration_impl.cc | 18 ++-- source/server/configuration_impl.h | 7 +- source/server/listener_impl.cc | 6 +- source/server/listener_manager_impl.cc | 50 +++++----- source/server/listener_manager_impl.h | 24 +++-- test/integration/BUILD | 5 +- .../filters/test_listener_filter.cc | 17 ++-- .../filters/test_listener_filter.h | 2 +- ...er_extension_discovery_integration_test.cc | 91 ++++++++++--------- test/server/listener_manager_impl_test.h | 30 +++--- 13 files changed, 147 insertions(+), 126 deletions(-) diff --git a/envoy/filter/config_provider_manager.h b/envoy/filter/config_provider_manager.h index 8f520047c20d..91797fd3502f 100644 --- a/envoy/filter/config_provider_manager.h +++ b/envoy/filter/config_provider_manager.h @@ -20,8 +20,10 @@ template using DynamicFilterConfigProviderPtr = std::unique_ptr>; // Listener filter config provider aliases -using ListenerFilterFactoriesList = std::vector>; -using UdpListenerFilterFactoriesList = std::vector>; +using ListenerFilterFactoriesList = + std::vector>; +using UdpListenerFilterFactoriesList = + std::vector>; /** * The FilterConfigProviderManager exposes the ability to get an FilterConfigProvider @@ -41,7 +43,8 @@ template class FilterConfigProviderManager { * @param last_filter_in_filter_chain indicates whether this filter is the last filter in the * configured chain * @param filter_chain_type is the filter chain type - * @param listener_filter_matcher is the filter matcher for TCP listener filter. nullptr for other filter types. + * @param listener_filter_matcher is the filter matcher for TCP listener filter. nullptr for other + * filter types. */ virtual DynamicFilterConfigProviderPtr createDynamicFilterConfigProvider( const envoy::config::core::v3::ExtensionConfigSource& config_source, diff --git a/envoy/server/listener_manager.h b/envoy/server/listener_manager.h index 0c5fb82ad3a8..0c4ab633231f 100644 --- a/envoy/server/listener_manager.h +++ b/envoy/server/listener_manager.h @@ -6,6 +6,7 @@ #include "envoy/config/core/v3/config_source.pb.h" #include "envoy/config/listener/v3/listener.pb.h" #include "envoy/config/listener/v3/listener_components.pb.h" +#include "envoy/filter/config_provider_manager.h" #include "envoy/network/filter.h" #include "envoy/network/listen_socket.h" #include "envoy/network/listener.h" @@ -14,7 +15,6 @@ #include "envoy/server/drain_manager.h" #include "envoy/server/filter_config.h" #include "envoy/server/guarddog.h" -#include "envoy/filter/config_provider_manager.h" #include "source/common/protobuf/protobuf.h" diff --git a/source/common/filter/config_discovery_impl.h b/source/common/filter/config_discovery_impl.h index 528fd934a3a2..ea7e1879cc4e 100644 --- a/source/common/filter/config_discovery_impl.h +++ b/source/common/filter/config_discovery_impl.h @@ -209,7 +209,7 @@ class ListenerDynamicFilterConfigProviderImpl : public DynamicFilterConfigProvid : DynamicFilterConfigProviderImpl( subscription, require_type_urls, factory_context, std::move(default_config), last_filter_in_filter_chain, filter_chain_type, stat_prefix), - factory_context_(factory_context), listener_filter_matcher_(listener_filter_matcher){} + factory_context_(factory_context), listener_filter_matcher_(listener_filter_matcher) {} void validateMessage(const std::string&, const Protobuf::Message&, const std::string&) const override {} @@ -230,7 +230,8 @@ class TcpListenerDynamicFilterConfigProviderImpl auto* factory = Registry::FactoryRegistry:: getFactoryByType(message.GetTypeName()); - return factory->createListenerFilterFactoryFromProto(message, listener_filter_matcher_, factory_context_); + return factory->createListenerFilterFactoryFromProto(message, listener_filter_matcher_, + factory_context_); } }; @@ -422,9 +423,10 @@ class FilterConfigProviderManagerImpl : public FilterConfigProviderManagerImplBa last_filter_in_filter_chain, filter_chain_type, require_type_urls); } - auto provider = createFilterConfigProviderImpl( - subscription, require_type_urls, factory_context, std::move(default_config), - last_filter_in_filter_chain, filter_chain_type, provider_stat_prefix, listener_filter_matcher); + auto provider = createFilterConfigProviderImpl(subscription, require_type_urls, factory_context, + std::move(default_config), + last_filter_in_filter_chain, filter_chain_type, + provider_stat_prefix, listener_filter_matcher); // Ensure the subscription starts if it has not already. if (config_source.apply_default_config_without_warming()) { diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index b5196ad4cc29..f2627f88c8bb 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -47,7 +47,7 @@ class MissingConfigTcpListenerFilter : public Network::ListenerFilter, Network::FilterStatus onAccept(Network::ListenerFilterCallbacks& cb) override { cb_ = &cb; ENVOY_LOG(warn, "Listener filter: new connection accepted while missing configuration. " - "Close socket and stop the iteration onAccept."); + "Close socket and stop the iteration onAccept."); cb_->socket().ioHandle().close(); return Network::FilterStatus::StopIteration; } @@ -61,9 +61,8 @@ class MissingConfigTcpListenerFilter : public Network::ListenerFilter, Network::ListenerFilterCallbacks* cb_{}; }; - -bool FilterChainUtility::buildFilterChain( - Network::ListenerFilterManager& filter_manager, const Filter::ListenerFilterFactoriesList& factories) { +bool FilterChainUtility::buildFilterChain(Network::ListenerFilterManager& filter_manager, + const Filter::ListenerFilterFactoriesList& factories) { bool added_missing_config_filter = false; for (const auto& filter_config_provider : factories) { auto config = filter_config_provider->config(); @@ -74,19 +73,18 @@ bool FilterChainUtility::buildFilterChain( } // If a filter config is missing after warming, stop iteration. if (!added_missing_config_filter) { - ENVOY_LOG_MISC(trace, "Missing filter config for a provider {}", filter_config_provider->name()); + ENVOY_LOG_MISC(trace, "Missing filter config for a provider {}", + filter_config_provider->name()); filter_manager.addAcceptFilter(nullptr, std::make_unique()); added_missing_config_filter = true; } else { ENVOY_LOG_MISC(trace, "Provider {} missing a filter config", filter_config_provider->name()); } - } return true; } - class MissingConfigUdpListenerFilter : public Network::UdpListenerReadFilter { public: MissingConfigUdpListenerFilter(Network::UdpReadFilterCallbacks& callbacks) @@ -116,11 +114,13 @@ void FilterChainUtility::buildUdpFilterChain( } // If a UDP filter config is missing after warming, stop iteration. if (!added_missing_config_filter) { - ENVOY_LOG_MISC(trace, "Missing UDP filter config for a provider {}", filter_config_provider->name()); + ENVOY_LOG_MISC(trace, "Missing UDP filter config for a provider {}", + filter_config_provider->name()); filter_manager.addReadFilter(std::make_unique(callbacks)); added_missing_config_filter = true; } else { - ENVOY_LOG_MISC(trace, "Provider {} missing a UDP filter config", filter_config_provider->name()); + ENVOY_LOG_MISC(trace, "Provider {} missing a UDP filter config", + filter_config_provider->name()); } } } diff --git a/source/server/configuration_impl.h b/source/server/configuration_impl.h index 2e25998ecb39..407498093193 100644 --- a/source/server/configuration_impl.h +++ b/source/server/configuration_impl.h @@ -88,10 +88,9 @@ class FilterChainUtility { * Given a UdpListenerFilterManager and a list of factories, create a new filter chain. Chain * creation will exit early if any filters immediately close the connection. */ - static void - buildUdpFilterChain(Network::UdpListenerFilterManager& filter_manager, - Network::UdpReadFilterCallbacks& callbacks, - const Filter::UdpListenerFilterFactoriesList& factories); + static void buildUdpFilterChain(Network::UdpListenerFilterManager& filter_manager, + Network::UdpReadFilterCallbacks& callbacks, + const Filter::UdpListenerFilterFactoriesList& factories); }; /** diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index 74390afc8e8e..e20a0f32169b 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -702,7 +702,7 @@ void ListenerImpl::buildOriginalDstListenerFilter() { "envoy.filters.listener.original_dst"); Network::ListenerFilterFactoryCb callback = factory.createListenerFilterFactoryFromProto( - Envoy::ProtobufWkt::Empty(), nullptr, *listener_factory_context_ ); + Envoy::ProtobufWkt::Empty(), nullptr, *listener_factory_context_); auto cfg_provider = parent_.getTcpListenerConfigProviderManager(); auto filter_config_provider = cfg_provider.createStaticFilterConfigProvider( callback, "envoy.filters.listener.original_dst"); @@ -721,8 +721,8 @@ void ListenerImpl::buildProxyProtocolListenerFilter() { "envoy.filters.listener.proxy_protocol"); Network::ListenerFilterFactoryCb callback = factory.createListenerFilterFactoryFromProto( - envoy::extensions::filters::listener::proxy_protocol::v3::ProxyProtocol(), - nullptr, *listener_factory_context_ ); + envoy::extensions::filters::listener::proxy_protocol::v3::ProxyProtocol(), nullptr, + *listener_factory_context_); auto cfg_provider = parent_.getTcpListenerConfigProviderManager(); auto filter_config_provider = cfg_provider.createStaticFilterConfigProvider( callback, "envoy.filters.listener.proxy_protocol"); diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 476cff096662..9163f18ebcd5 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -122,22 +122,24 @@ Filter::ListenerFilterFactoriesList ProdListenerComponentFactory::createListener static_cast(proto_config.typed_config()))); // dynamic listener filter configuration if (proto_config.config_type_case() == - envoy::config::listener::v3::ListenerFilter::ConfigTypeCase::kConfigDiscovery) { + envoy::config::listener::v3::ListenerFilter::ConfigTypeCase::kConfigDiscovery) { auto& config_discovery = proto_config.config_discovery(); auto& name = proto_config.name(); ENVOY_LOG(debug, " Listener filter: dynamic filter name: {}", name); if (config_discovery.apply_default_config_without_warming() && - !config_discovery.has_default_config()) { + !config_discovery.has_default_config()) { throw EnvoyException(fmt::format( - "Error: listener filter config {} applied without warming but has no default config.", name)); + "Error: listener filter config {} applied without warming but has no default config.", + name)); } for (const auto& type_url : config_discovery.type_urls()) { auto factory_type_url = TypeUtil::typeUrlToDescriptorFullName(type_url); - auto* factory = Registry::FactoryRegistry< - Server::Configuration::NamedListenerFilterConfigFactory>::getFactoryByType(factory_type_url); + auto* factory = + Registry::FactoryRegistry:: + getFactoryByType(factory_type_url); if (factory == nullptr) { - throw EnvoyException( - fmt::format("Error: no listener factory found for a required type URL {}.", factory_type_url)); + throw EnvoyException(fmt::format( + "Error: no listener factory found for a required type URL {}.", factory_type_url)); } } auto filter_config_provider = config_provider_manager.createDynamicFilterConfigProvider( @@ -157,8 +159,8 @@ Filter::ListenerFilterFactoriesList ProdListenerComponentFactory::createListener Network::ListenerFilterFactoryCb callback = factory.createListenerFilterFactoryFromProto( *message, createListenerFilterMatcher(proto_config), context); - auto filter_config_provider = config_provider_manager.createStaticFilterConfigProvider( - callback, proto_config.name()); + auto filter_config_provider = + config_provider_manager.createStaticFilterConfigProvider(callback, proto_config.name()); ENVOY_LOG(debug, " name: {}", filter_config_provider->name()); ret.push_back(std::move(filter_config_provider)); @@ -166,7 +168,8 @@ Filter::ListenerFilterFactoriesList ProdListenerComponentFactory::createListener return ret; } -Filter::UdpListenerFilterFactoriesList ProdListenerComponentFactory::createUdpListenerFilterFactoryList_( +Filter::UdpListenerFilterFactoriesList +ProdListenerComponentFactory::createUdpListenerFilterFactoryList_( const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context, Filter::UdpListenerFilterConfigProviderManagerImpl& config_provider_manager) { @@ -188,21 +191,24 @@ Filter::UdpListenerFilterFactoriesList ProdListenerComponentFactory::createUdpLi if (config_discovery.apply_default_config_without_warming() && !config_discovery.has_default_config()) { throw EnvoyException(fmt::format( - "Error: listener filter config {} applied without warming but has no default config.", name)); + "Error: listener filter config {} applied without warming but has no default config.", + name)); } for (const auto& type_url : config_discovery.type_urls()) { auto factory_type_url = TypeUtil::typeUrlToDescriptorFullName(type_url); - auto* factory = Registry::FactoryRegistry< - Server::Configuration::NamedUdpListenerFilterConfigFactory>::getFactoryByType(factory_type_url); + auto* factory = + Registry::FactoryRegistry:: + getFactoryByType(factory_type_url); if (factory == nullptr) { throw EnvoyException( - fmt::format("Error: no UDP listener factory found for a required type URL {}.", factory_type_url)); + fmt::format("Error: no UDP listener factory found for a required type URL {}.", + factory_type_url)); } } auto filter_config_provider = config_provider_manager.createDynamicFilterConfigProvider( - config_discovery, name, context, "udp_listener.", false, "listener", nullptr); + config_discovery, name, context, "udp_listener.", false, "listener", nullptr); ret.push_back(std::move(filter_config_provider)); return ret; } @@ -213,10 +219,10 @@ Filter::UdpListenerFilterFactoriesList ProdListenerComponentFactory::createUdpLi proto_config); auto message = Config::Utility::translateToFactoryConfig( proto_config, context.messageValidationVisitor(), factory); - Network::UdpListenerFilterFactoryCb callback = factory.createFilterFactoryFromProto( - *message, context); - auto filter_config_provider = config_provider_manager.createStaticFilterConfigProvider( - callback, proto_config.name()); + Network::UdpListenerFilterFactoryCb callback = + factory.createFilterFactoryFromProto(*message, context); + auto filter_config_provider = + config_provider_manager.createStaticFilterConfigProvider(callback, proto_config.name()); ENVOY_LOG(debug, " name: {}", filter_config_provider->name()); ret.push_back(std::move(filter_config_provider)); } @@ -319,8 +325,10 @@ ListenerManagerImpl::ListenerManagerImpl(Instance& server, worker_factory.createWorker(i, server.overloadManager(), absl::StrCat("worker_", i))); } // Create TCP/UDP listener filter config provider manager instance. - tcp_listener_config_provider_manager_ = std::make_unique(); - udp_listener_config_provider_manager_ = std::make_unique(); + tcp_listener_config_provider_manager_ = + std::make_unique(); + udp_listener_config_provider_manager_ = + std::make_unique(); } ProtobufTypes::MessagePtr diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index 793c126d97e3..88d142aa560c 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -135,10 +135,10 @@ class ListenerManagerImpl : public ListenerManager, Logger::Loggable> error_state_tracker_; FailureStates overall_error_state_; Quic::QuicStatNames& quic_stat_names_; - std::unique_ptr tcp_listener_config_provider_manager_; - std::unique_ptr udp_listener_config_provider_manager_; + std::unique_ptr + tcp_listener_config_provider_manager_; + std::unique_ptr + udp_listener_config_provider_manager_; }; /** @@ -318,16 +320,18 @@ class ProdListenerComponentFactory : public ListenerComponentFactory, Filter::ListenerFilterFactoriesList createListenerFilterFactoryList( const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) override { - ListenerManagerImpl * listener_manager = dynamic_cast(&(server_.listenerManager())); - return createListenerFilterFactoryList_(filters, context, - listener_manager->getTcpListenerConfigProviderManager()); + ListenerManagerImpl* listener_manager = + dynamic_cast(&(server_.listenerManager())); + return createListenerFilterFactoryList_( + filters, context, listener_manager->getTcpListenerConfigProviderManager()); } Filter::UdpListenerFilterFactoriesList createUdpListenerFilterFactoryList( const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) override { - ListenerManagerImpl * listener_manager = dynamic_cast(&(server_.listenerManager())); - return createUdpListenerFilterFactoryList_(filters, context, - listener_manager->getUdpListenerConfigProviderManager()); + ListenerManagerImpl* listener_manager = + dynamic_cast(&(server_.listenerManager())); + return createUdpListenerFilterFactoryList_( + filters, context, listener_manager->getUdpListenerConfigProviderManager()); } Network::SocketSharedPtr createListenSocket( Network::Address::InstanceConstSharedPtr address, Network::Socket::Type socket_type, diff --git a/test/integration/BUILD b/test/integration/BUILD index af094351132b..d088c233307e 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -1203,12 +1203,11 @@ envoy_cc_test( srcs = ["listener_extension_discovery_integration_test.cc"], deps = [ ":http_integration_lib", + "//source/extensions/filters/network/tcp_proxy:config", "//test/common/grpc:grpc_client_integration_lib", "//test/integration/filters:test_listener_filter_lib", - "//source/extensions/filters/network/tcp_proxy:config", "//test/test_common:utility_lib", - "@envoy_api//envoy/extensions/common/matching/v3:pkg_cc_proto", - "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/filters/network/tcp_proxy/v3:pkg_cc_proto", "@envoy_api//envoy/service/discovery/v3:pkg_cc_proto", "@envoy_api//envoy/service/extension/v3:pkg_cc_proto", ], diff --git a/test/integration/filters/test_listener_filter.cc b/test/integration/filters/test_listener_filter.cc index c8897a9ec1e1..ae0a860aee2b 100644 --- a/test/integration/filters/test_listener_filter.cc +++ b/test/integration/filters/test_listener_filter.cc @@ -31,23 +31,24 @@ class TestInspectorConfigFactory : public Server::Configuration::NamedListenerFi absl::Mutex TestListenerFilter::alpn_lock_; std::string TestListenerFilter::alpn_; - /** * Config registration for the test TCP listener filter. */ -class TestTcpInspectorConfigFactory : public Server::Configuration::NamedListenerFilterConfigFactory { +class TestTcpInspectorConfigFactory + : public Server::Configuration::NamedListenerFilterConfigFactory { public: // NamedListenerFilterConfigFactory Network::ListenerFilterFactoryCb createListenerFilterFactoryFromProto( const Protobuf::Message& proto_config, const Network::ListenerFilterMatcherSharedPtr& listener_filter_matcher, Server::Configuration::ListenerFactoryContext& context) override { - const auto& message = MessageUtil::downcastAndValidate< - const test::integration::filters::TestTcpListenerFilterConfig&>( + const auto& message = MessageUtil::downcastAndValidate< + const test::integration::filters::TestTcpListenerFilterConfig&>( proto_config, context.messageValidationVisitor()); - return [listener_filter_matcher, message](Network::ListenerFilterManager& filter_manager) -> void { - filter_manager.addAcceptFilter(listener_filter_matcher, - std::make_unique(message.drain_bytes())); + return [listener_filter_matcher, + message](Network::ListenerFilterManager& filter_manager) -> void { + filter_manager.addAcceptFilter( + listener_filter_matcher, std::make_unique(message.drain_bytes())); }; } @@ -58,8 +59,6 @@ class TestTcpInspectorConfigFactory : public Server::Configuration::NamedListene std::string name() const override { return "envoy.filters.tcp_listener.test"; } }; - - /** * Config registration for the UDP test filter. */ diff --git a/test/integration/filters/test_listener_filter.h b/test/integration/filters/test_listener_filter.h index 96eec119f4a3..3833ba66e705 100644 --- a/test/integration/filters/test_listener_filter.h +++ b/test/integration/filters/test_listener_filter.h @@ -37,7 +37,7 @@ class TestListenerFilter : public Network::ListenerFilter { */ class TestTcpListenerFilter : public Network::ListenerFilter { public: - TestTcpListenerFilter(const uint32_t drain_bytes) : drain_bytes_(drain_bytes){} + TestTcpListenerFilter(const uint32_t drain_bytes) : drain_bytes_(drain_bytes) {} Network::FilterStatus onAccept(Network::ListenerFilterCallbacks&) override { return Network::FilterStatus::StopIteration; diff --git a/test/integration/listener_extension_discovery_integration_test.cc b/test/integration/listener_extension_discovery_integration_test.cc index 5a3bd965796e..a9a3e1ef5230 100644 --- a/test/integration/listener_extension_discovery_integration_test.cc +++ b/test/integration/listener_extension_discovery_integration_test.cc @@ -1,10 +1,10 @@ +#include "envoy/extensions/filters/network/tcp_proxy/v3/tcp_proxy.pb.h" #include "envoy/service/discovery/v3/discovery.pb.h" #include "envoy/service/extension/v3/config_discovery.pb.h" #include "test/common/grpc/grpc_client_integration.h" -#include "test/integration/filters/test_listener_filter.pb.h" -#include "envoy/extensions/filters/network/tcp_proxy/v3/tcp_proxy.pb.h" #include "test/integration/filters/test_listener_filter.h" +#include "test/integration/filters/test_listener_filter.pb.h" #include "test/integration/integration.h" #include "test/test_common/utility.h" @@ -29,13 +29,14 @@ class ExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegrationPara void addDynamicFilter(const std::string& name, bool apply_without_warming, bool set_default_config = true, bool rate_limit = false) { - config_helper_.addConfigModifier([&]( envoy::config::bootstrap::v3::Bootstrap& bootstrap) { - auto* listener_filter = bootstrap.mutable_static_resources()->mutable_listeners(0)->add_listener_filters(); + config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + auto* listener_filter = + bootstrap.mutable_static_resources()->mutable_listeners(0)->add_listener_filters(); listener_filter->set_name(name); auto* discovery = listener_filter->mutable_config_discovery(); discovery->add_type_urls( - "type.googleapis.com/test.integration.filters.TestTcpListenerFilterConfig"); + "type.googleapis.com/test.integration.filters.TestTcpListenerFilterConfig"); if (set_default_config) { auto default_configuration = test::integration::filters::TestTcpListenerFilterConfig(); default_configuration.set_drain_bytes(kDefaultDrainBytes); @@ -44,7 +45,7 @@ class ExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegrationPara discovery->set_apply_default_config_without_warming(apply_without_warming); discovery->mutable_config_source()->set_resource_api_version( - envoy::config::core::v3::ApiVersion::V3); + envoy::config::core::v3::ApiVersion::V3); auto* api_config_source = discovery->mutable_config_source()->mutable_api_config_source(); api_config_source->set_api_type(envoy::config::core::v3::ApiConfigSource::GRPC); api_config_source->set_transport_api_version(envoy::config::core::v3::ApiVersion::V3); @@ -54,7 +55,6 @@ class ExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegrationPara auto* grpc_service = api_config_source->add_grpc_services(); setGrpcService(*grpc_service, "ecds_cluster", getEcdsFakeUpstream().localAddress()); }); - } void initialize() override { @@ -98,7 +98,7 @@ class ExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegrationPara ecds_stream_->startGrpcStream(); } - void sendXdsResponse(const std::string& version, const uint32_t drain_bytes, bool ttl = false) { + void sendXdsResponse(const std::string& version, const uint32_t drain_bytes, bool ttl = false) { // The to-be-drained bytes has to be smaller than data size. ASSERT(drain_bytes <= data_.size()); @@ -121,8 +121,9 @@ class ExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegrationPara ecds_stream_->sendGrpcMessage(response); } - // Client sends data_, which is drained by Envoy listener filter based on config, then received by upstream. - void sendDataVerifyResults (uint32_t drain_bytes) { + // Client sends data_, which is drained by Envoy listener filter based on config, then received by + // upstream. + void sendDataVerifyResults(uint32_t drain_bytes) { test_server_->waitUntilListenersReady(); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); @@ -137,7 +138,7 @@ class ExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegrationPara tcp_client->close(); } - const uint32_t kDefaultDrainBytes = 2; + const uint32_t kDefaultDrainBytes = 2; const std::string filter_name_ = "foo"; const std::string data_ = "HelloWorld"; const std::string port_name_ = "http"; @@ -161,12 +162,14 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccess) { // Send 1st config update to have listener filter drain 5 bytes of data. sendXdsResponse("1", 5); - test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_reload", 1); + test_server_->waitForCounterGe( + "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 1); sendDataVerifyResults(5); // Send 2nd config update to have listener filter drain 3 bytes of data. sendXdsResponse("2", 3); - test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_reload", 2); + test_server_->waitForCounterGe( + "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 2); sendDataVerifyResults(3); } @@ -179,20 +182,21 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithTtl) { // Send 1st config update with TTL 1s, and have listener filter drain 5 bytes of data. sendXdsResponse("1", 5, true); - test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_reload", 1); + test_server_->waitForCounterGe( + "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 1); sendDataVerifyResults(5); // Wait for configuration expired. Then start a TCP connection. // The missing config listener filter will be installed to handle the connection. - test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_reload", 2); - EXPECT_LOG_CONTAINS("warn", "Close socket and stop the iteration onAccept.", - { - IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort(port_name_)); - auto result = tcp_client->write(data_); - if (result) { - tcp_client->waitForDisconnect(); - } - }); + test_server_->waitForCounterGe( + "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 2); + EXPECT_LOG_CONTAINS("warn", "Close socket and stop the iteration onAccept.", { + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort(port_name_)); + auto result = tcp_client->write(data_); + if (result) { + tcp_client->waitForDisconnect(); + } + }); } TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithTtlWithDefault) { @@ -204,16 +208,17 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithTtlWithDefault) { // Send 1st config update with TTL 1s, and have listener filter drain 5 bytes of data. sendXdsResponse("1", 5, true); - test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_reload", 1); + test_server_->waitForCounterGe( + "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 1); sendDataVerifyResults(5); // Wait for configuration expired. The default filter will be installed. - test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_reload", 2); + test_server_->waitForCounterGe( + "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 2); // Start a TCP connection. The default filter drain 2 bytes. sendDataVerifyResults(kDefaultDrainBytes); } - // This one TBD TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailWithDefault) { on_server_init_function_ = [&]() { waitXdsStream(); }; @@ -224,7 +229,8 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailWithDefault) { // Send config update with invalid config (drain_bytes has to >=2). sendXdsResponse("1", 1); - test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_fail", 1); + test_server_->waitForCounterGe( + "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_fail", 1); // The default filter will be installed. Start a TCP connection. The default filter drain 2 bytes. sendDataVerifyResults(kDefaultDrainBytes); } @@ -239,20 +245,19 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailWithoutDefault) { // Send config update with invalid config (drain_bytes has to >=2). sendXdsResponse("1", 1); - test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_fail", 1); + test_server_->waitForCounterGe( + "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_fail", 1); // The missing config filter will be installed when a correction is created. // The missing config filter will close the connection. - EXPECT_LOG_CONTAINS("warn", "Close socket and stop the iteration onAccept.", - { - IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort(port_name_)); - auto result = tcp_client->write(data_); - if (result) { - tcp_client->waitForDisconnect(); - } - }); + EXPECT_LOG_CONTAINS("warn", "Close socket and stop the iteration onAccept.", { + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort(port_name_)); + auto result = tcp_client->write(data_); + if (result) { + tcp_client->waitForDisconnect(); + } + }); } - TEST_P(ExtensionDiscoveryIntegrationTest, BasicWithoutWarming) { on_server_init_function_ = [&]() { waitXdsStream(); }; addDynamicFilter(filter_name_, true); @@ -262,7 +267,8 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicWithoutWarming) { sendDataVerifyResults(kDefaultDrainBytes); // Send update should cause a different response. sendXdsResponse("1", 3); - test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_reload", 1); + test_server_->waitForCounterGe( + "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 1); sendDataVerifyResults(3); } @@ -272,7 +278,8 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicWithoutWarmingFail) { initialize(); sendXdsResponse("1", 1); - test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_fail", 1); + test_server_->waitForCounterGe( + "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_fail", 1); sendDataVerifyResults(kDefaultDrainBytes); } @@ -284,7 +291,8 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicTwoSubscriptionsSameNameWithoutWa initialize(); sendXdsResponse("1", 3); - test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_reload", 1); + test_server_->waitForCounterGe( + "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 1); sendDataVerifyResults(3); } @@ -296,7 +304,8 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicTwoSubscriptionsSameNameWithWarmi initialize(); sendXdsResponse("1", 3); - test_server_->waitForCounterGe("extension_config_discovery.tcp_listener_filter."+ filter_name_ + ".config_reload", 1); + test_server_->waitForCounterGe( + "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 1); sendDataVerifyResults(3); } diff --git a/test/server/listener_manager_impl_test.h b/test/server/listener_manager_impl_test.h index bc9385193686..8f13a9767c7f 100644 --- a/test/server/listener_manager_impl_test.h +++ b/test/server/listener_manager_impl_test.h @@ -88,24 +88,22 @@ class ListenerManagerImplTest : public testing::TestWithParam { filters, filter_chain_factory_context); })); ON_CALL(listener_factory_, createListenerFilterFactoryList(_, _)) - .WillByDefault( - Invoke([this](const Protobuf::RepeatedPtrField& - filters, - Configuration::ListenerFactoryContext& context) - -> Filter::ListenerFilterFactoriesList { - return ProdListenerComponentFactory::createListenerFilterFactoryList_(filters, - context, - manager_->getTcpListenerConfigProviderManager()); + .WillByDefault(Invoke( + [this](const Protobuf::RepeatedPtrField& + filters, + Configuration::ListenerFactoryContext& context) + -> Filter::ListenerFilterFactoriesList { + return ProdListenerComponentFactory::createListenerFilterFactoryList_( + filters, context, manager_->getTcpListenerConfigProviderManager()); })); ON_CALL(listener_factory_, createUdpListenerFilterFactoryList(_, _)) - .WillByDefault( - Invoke([this](const Protobuf::RepeatedPtrField& - filters, - Configuration::ListenerFactoryContext& context) - -> Filter::UdpListenerFilterFactoriesList { - return ProdListenerComponentFactory::createUdpListenerFilterFactoryList_(filters, - context, - manager_->getUdpListenerConfigProviderManager()); + .WillByDefault(Invoke( + [this](const Protobuf::RepeatedPtrField& + filters, + Configuration::ListenerFactoryContext& context) + -> Filter::UdpListenerFilterFactoriesList { + return ProdListenerComponentFactory::createUdpListenerFilterFactoryList_( + filters, context, manager_->getUdpListenerConfigProviderManager()); })); ON_CALL(listener_factory_, nextListenerTag()).WillByDefault(Invoke([this]() { return listener_tag_++; From dd55ab938c3d01516987c0436b02da60f7a1a05d Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Sun, 8 May 2022 02:29:54 +0000 Subject: [PATCH 03/28] Split UDP listener filter support to a separate PR. Signed-off-by: Yanjun Xiang --- envoy/filter/config_provider_manager.h | 2 - envoy/server/listener_manager.h | 4 +- source/server/config_validation/server.h | 7 +- source/server/configuration_impl.cc | 38 +-------- source/server/configuration_impl.h | 7 +- source/server/listener_impl.h | 2 +- source/server/listener_manager_impl.cc | 81 +++++-------------- source/server/listener_manager_impl.h | 18 ++--- .../filter/config_discovery_impl_test.cc | 20 ++++- ...er_extension_discovery_integration_test.cc | 45 ++++------- .../mocks/server/listener_component_factory.h | 2 +- test/server/listener_manager_impl_test.h | 14 ++-- 12 files changed, 79 insertions(+), 161 deletions(-) diff --git a/envoy/filter/config_provider_manager.h b/envoy/filter/config_provider_manager.h index 91797fd3502f..d917a67af085 100644 --- a/envoy/filter/config_provider_manager.h +++ b/envoy/filter/config_provider_manager.h @@ -22,8 +22,6 @@ using DynamicFilterConfigProviderPtr = std::unique_ptr>; -using UdpListenerFilterFactoriesList = - std::vector>; /** * The FilterConfigProviderManager exposes the ability to get an FilterConfigProvider diff --git a/envoy/server/listener_manager.h b/envoy/server/listener_manager.h index 0c4ab633231f..281f10644bce 100644 --- a/envoy/server/listener_manager.h +++ b/envoy/server/listener_manager.h @@ -99,9 +99,9 @@ class ListenerComponentFactory { * Creates a list of UDP listener filter factories. * @param filters supplies the configuration. * @param context supplies the factory creation context. - * @return UdpListenerFilterFactoriesList the list of filter factories. + * @return std::vector the list of filter factories. */ - virtual Filter::UdpListenerFilterFactoriesList createUdpListenerFilterFactoryList( + virtual std::vector createUdpListenerFilterFactoryList( const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) PURE; diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index 1854bf1eee0e..d894a9c87a81 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -148,17 +148,16 @@ class ValidationInstance final : Logger::Loggable, return ProdListenerComponentFactory::createNetworkFilterFactoryList_( filters, filter_chain_factory_context); } - ListenerFilterFactoriesList createListenerFilterFactoryList( + Filter::ListenerFilterFactoriesList createListenerFilterFactoryList( const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) override { return ProdListenerComponentFactory::createListenerFilterFactoryList_( filters, context, listener_manager_->getTcpListenerConfigProviderManager()); } - UdpListenerFilterFactoriesList createUdpListenerFilterFactoryList( + std::vector createUdpListenerFilterFactoryList( const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) override { - return ProdListenerComponentFactory::createUdpListenerFilterFactoryList_( - filters, context, listener_manager_->getUdpListenerConfigProviderManager()); + return ProdListenerComponentFactory::createUdpListenerFilterFactoryList_(filters, context); } Network::SocketSharedPtr createListenSocket(Network::Address::InstanceConstSharedPtr, Network::Socket::Type, diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index f2627f88c8bb..8d8ef332ee62 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -85,43 +85,11 @@ bool FilterChainUtility::buildFilterChain(Network::ListenerFilterManager& filter return true; } -class MissingConfigUdpListenerFilter : public Network::UdpListenerReadFilter { -public: - MissingConfigUdpListenerFilter(Network::UdpReadFilterCallbacks& callbacks) - : UdpListenerReadFilter(callbacks) {} - - // Network::UdpListenerReadFilter callbacks - Network::FilterStatus onData(Network::UdpRecvData&) override { - return Network::FilterStatus::StopIteration; - } - Network::FilterStatus onReceiveError(Api::IoError::IoErrorCode) override { - return Network::FilterStatus::StopIteration; - } -}; - void FilterChainUtility::buildUdpFilterChain( Network::UdpListenerFilterManager& filter_manager, Network::UdpReadFilterCallbacks& callbacks, - const Filter::UdpListenerFilterFactoriesList& factories) { - - bool added_missing_config_filter = false; - - for (const auto& filter_config_provider : factories) { - auto config = filter_config_provider->config(); - if (config.has_value()) { - auto config_value = config.value(); - config_value(filter_manager, callbacks); - continue; - } - // If a UDP filter config is missing after warming, stop iteration. - if (!added_missing_config_filter) { - ENVOY_LOG_MISC(trace, "Missing UDP filter config for a provider {}", - filter_config_provider->name()); - filter_manager.addReadFilter(std::make_unique(callbacks)); - added_missing_config_filter = true; - } else { - ENVOY_LOG_MISC(trace, "Provider {} missing a UDP filter config", - filter_config_provider->name()); - } + const std::vector& factories) { + for (const Network::UdpListenerFilterFactoryCb& factory : factories) { + factory(filter_manager, callbacks); } } diff --git a/source/server/configuration_impl.h b/source/server/configuration_impl.h index 407498093193..503c396ffce3 100644 --- a/source/server/configuration_impl.h +++ b/source/server/configuration_impl.h @@ -88,9 +88,10 @@ class FilterChainUtility { * Given a UdpListenerFilterManager and a list of factories, create a new filter chain. Chain * creation will exit early if any filters immediately close the connection. */ - static void buildUdpFilterChain(Network::UdpListenerFilterManager& filter_manager, - Network::UdpReadFilterCallbacks& callbacks, - const Filter::UdpListenerFilterFactoriesList& factories); + static void + buildUdpFilterChain(Network::UdpListenerFilterManager& filter_manager, + Network::UdpReadFilterCallbacks& callbacks, + const std::vector& factories); }; /** diff --git a/source/server/listener_impl.h b/source/server/listener_impl.h index 3beae34669ef..07a525d23fea 100644 --- a/source/server/listener_impl.h +++ b/source/server/listener_impl.h @@ -440,7 +440,7 @@ class ListenerImpl final : public Network::ListenerConfig, std::unique_ptr dynamic_init_manager_; Filter::ListenerFilterFactoriesList listener_filter_factories_; - Filter::UdpListenerFilterFactoriesList udp_listener_filter_factories_; + std::vector udp_listener_filter_factories_; std::vector access_logs_; const envoy::config::listener::v3::Listener config_; const std::string version_info_; diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 9163f18ebcd5..8f74ea13eb5a 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -146,34 +146,32 @@ Filter::ListenerFilterFactoriesList ProdListenerComponentFactory::createListener config_discovery, name, context, "tcp_listener.", false, "listener", createListenerFilterMatcher(proto_config)); ret.push_back(std::move(filter_config_provider)); - return ret; - } - - // For static configuration, now see if there is a factory that will accept the config. - auto& factory = - Config::Utility::getAndCheckFactory( - proto_config); - auto message = Config::Utility::translateToFactoryConfig( - proto_config, context.messageValidationVisitor(), factory); + } else { + // For static configuration, now see if there is a factory that will accept the config. + auto& factory = + Config::Utility::getAndCheckFactory( + proto_config); + auto message = Config::Utility::translateToFactoryConfig( + proto_config, context.messageValidationVisitor(), factory); - Network::ListenerFilterFactoryCb callback = factory.createListenerFilterFactoryFromProto( - *message, createListenerFilterMatcher(proto_config), context); + Network::ListenerFilterFactoryCb callback = factory.createListenerFilterFactoryFromProto( + *message, createListenerFilterMatcher(proto_config), context); - auto filter_config_provider = - config_provider_manager.createStaticFilterConfigProvider(callback, proto_config.name()); + auto filter_config_provider = + config_provider_manager.createStaticFilterConfigProvider(callback, proto_config.name()); - ENVOY_LOG(debug, " name: {}", filter_config_provider->name()); - ret.push_back(std::move(filter_config_provider)); + ENVOY_LOG(debug, " name: {}", filter_config_provider->name()); + ret.push_back(std::move(filter_config_provider)); + } } return ret; } -Filter::UdpListenerFilterFactoriesList +std::vector ProdListenerComponentFactory::createUdpListenerFilterFactoryList_( const Protobuf::RepeatedPtrField& filters, - Configuration::ListenerFactoryContext& context, - Filter::UdpListenerFilterConfigProviderManagerImpl& config_provider_manager) { - Filter::UdpListenerFilterFactoriesList ret; + Configuration::ListenerFactoryContext& context) { + std::vector ret; for (ssize_t i = 0; i < filters.size(); i++) { const auto& proto_config = filters[i]; ENVOY_LOG(debug, " filter #{}:", i); @@ -181,50 +179,15 @@ ProdListenerComponentFactory::createUdpListenerFilterFactoryList_( ENVOY_LOG(debug, " config: {}", MessageUtil::getJsonStringFromMessageOrError( static_cast(proto_config.typed_config()))); - // dynamic UDP listener filter configuration - if (proto_config.config_type_case() == - envoy::config::listener::v3::ListenerFilter::ConfigTypeCase::kConfigDiscovery) { - auto& config_discovery = proto_config.config_discovery(); - auto& name = proto_config.name(); - ENVOY_LOG(debug, " UDP Listener filter: dynamic filter name: {}", name); - if (config_discovery.apply_default_config_without_warming() && - !config_discovery.has_default_config()) { - throw EnvoyException(fmt::format( - "Error: listener filter config {} applied without warming but has no default config.", - name)); - } - - for (const auto& type_url : config_discovery.type_urls()) { - auto factory_type_url = TypeUtil::typeUrlToDescriptorFullName(type_url); - auto* factory = - Registry::FactoryRegistry:: - getFactoryByType(factory_type_url); - if (factory == nullptr) { - throw EnvoyException( - fmt::format("Error: no UDP listener factory found for a required type URL {}.", - factory_type_url)); - } - } - - auto filter_config_provider = config_provider_manager.createDynamicFilterConfigProvider( - config_discovery, name, context, "udp_listener.", false, "listener", nullptr); - ret.push_back(std::move(filter_config_provider)); - return ret; - } - - // For static configuration, Now see if there is a factory that will accept the config. + // Now see if there is a factory that will accept the config. auto& factory = Config::Utility::getAndCheckFactory( proto_config); + auto message = Config::Utility::translateToFactoryConfig( proto_config, context.messageValidationVisitor(), factory); - Network::UdpListenerFilterFactoryCb callback = - factory.createFilterFactoryFromProto(*message, context); - auto filter_config_provider = - config_provider_manager.createStaticFilterConfigProvider(callback, proto_config.name()); - ENVOY_LOG(debug, " name: {}", filter_config_provider->name()); - ret.push_back(std::move(filter_config_provider)); + ret.push_back(factory.createFilterFactoryFromProto(*message, context)); } return ret; } @@ -324,11 +287,9 @@ ListenerManagerImpl::ListenerManagerImpl(Instance& server, workers_.emplace_back( worker_factory.createWorker(i, server.overloadManager(), absl::StrCat("worker_", i))); } - // Create TCP/UDP listener filter config provider manager instance. + // Create TCP listener filter config provider manager instance. tcp_listener_config_provider_manager_ = std::make_unique(); - udp_listener_config_provider_manager_ = - std::make_unique(); } ProtobufTypes::MessagePtr diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index 88d142aa560c..f5402faa9e0b 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -137,9 +137,6 @@ class ListenerManagerImpl : public ListenerManager, Logger::Loggable tcp_listener_config_provider_manager_; - std::unique_ptr - udp_listener_config_provider_manager_; }; /** @@ -296,10 +291,9 @@ class ProdListenerComponentFactory : public ListenerComponentFactory, /** * Static worker for createUdpListenerFilterFactoryList() that can be used directly in tests. */ - static Filter::UdpListenerFilterFactoriesList createUdpListenerFilterFactoryList_( + static std::vector createUdpListenerFilterFactoryList_( const Protobuf::RepeatedPtrField& filters, - Configuration::ListenerFactoryContext& context, - Filter::UdpListenerFilterConfigProviderManagerImpl& config_provider_manager); + Configuration::ListenerFactoryContext& context); static Network::ListenerFilterMatcherSharedPtr createListenerFilterMatcher(const envoy::config::listener::v3::ListenerFilter& listener_filter); @@ -320,18 +314,16 @@ class ProdListenerComponentFactory : public ListenerComponentFactory, Filter::ListenerFilterFactoriesList createListenerFilterFactoryList( const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) override { + // TODO(yanjunxiang): cleanup this dynamic_cast. ListenerManagerImpl* listener_manager = dynamic_cast(&(server_.listenerManager())); return createListenerFilterFactoryList_( filters, context, listener_manager->getTcpListenerConfigProviderManager()); } - Filter::UdpListenerFilterFactoriesList createUdpListenerFilterFactoryList( + std::vector createUdpListenerFilterFactoryList( const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) override { - ListenerManagerImpl* listener_manager = - dynamic_cast(&(server_.listenerManager())); - return createUdpListenerFilterFactoryList_( - filters, context, listener_manager->getUdpListenerConfigProviderManager()); + return createUdpListenerFilterFactoryList_(filters, context); } Network::SocketSharedPtr createListenSocket( Network::Address::InstanceConstSharedPtr address, Network::Socket::Type socket_type, diff --git a/test/common/filter/config_discovery_impl_test.cc b/test/common/filter/config_discovery_impl_test.cc index 5a8824026d13..274fe2c83731 100644 --- a/test/common/filter/config_discovery_impl_test.cc +++ b/test/common/filter/config_discovery_impl_test.cc @@ -171,12 +171,14 @@ class HttpFilterConfigDiscoveryImplTest HttpFilterConfigProviderManagerImpl, envoy::extensions::filters::http::router::v3::Router> { public: + HttpFilterConfigDiscoveryImplTest() : matcher_(nullptr) {} + const std::string getTypeUrl() const override { return "envoy.extensions.filters.http.router.v3.Router"; } const std::string getFilterType() const override { return "http"; } const Network::ListenerFilterMatcherSharedPtr& getFilterMatcher() const override { - return nullptr; + return matcher_; } const std::string getConfigReloadCounter() const override { return "extension_config_discovery.http_filter.foo.config_reload"; @@ -184,6 +186,8 @@ class HttpFilterConfigDiscoveryImplTest const std::string getConfigFailCounter() const override { return "extension_config_discovery.http_filter.foo.config_fail"; } + + const Network::ListenerFilterMatcherSharedPtr matcher_; }; // TCP listener filter test @@ -192,10 +196,12 @@ class TcpListenerFilterConfigDiscoveryImplTest Network::ListenerFilterFactoryCb, Server::Configuration::ListenerFactoryContext, TcpListenerFilterConfigProviderManagerImpl, Envoy::ProtobufWkt::Struct> { public: + TcpListenerFilterConfigDiscoveryImplTest() : matcher_(nullptr) {} + const std::string getTypeUrl() const override { return "google.protobuf.Struct"; } const std::string getFilterType() const override { return "listener"; } const Network::ListenerFilterMatcherSharedPtr& getFilterMatcher() const override { - return nullptr; + return matcher_; } const std::string getConfigReloadCounter() const override { return "extension_config_discovery.tcp_listener_filter.foo.config_reload"; @@ -203,6 +209,8 @@ class TcpListenerFilterConfigDiscoveryImplTest const std::string getConfigFailCounter() const override { return "extension_config_discovery.tcp_listener_filter.foo.config_fail"; } + + const Network::ListenerFilterMatcherSharedPtr matcher_; }; // UDP listener filter test @@ -212,12 +220,14 @@ class UdpListenerFilterConfigDiscoveryImplTest UdpListenerFilterConfigProviderManagerImpl, test::integration::filters::TestUdpListenerFilterConfig> { public: + UdpListenerFilterConfigDiscoveryImplTest() : matcher_(nullptr) {} + const std::string getTypeUrl() const override { return "test.integration.filters.TestUdpListenerFilterConfig"; } const std::string getFilterType() const override { return "listener"; } const Network::ListenerFilterMatcherSharedPtr& getFilterMatcher() const override { - return nullptr; + return matcher_; } const std::string getConfigReloadCounter() const override { return "extension_config_discovery.udp_listener_filter.foo.config_reload"; @@ -225,6 +235,8 @@ class UdpListenerFilterConfigDiscoveryImplTest const std::string getConfigFailCounter() const override { return "extension_config_discovery.udp_listener_filter.foo.config_fail"; } + + const Network::ListenerFilterMatcherSharedPtr matcher_; }; /*************************************************************************************** @@ -469,7 +481,7 @@ TYPED_TEST(FilterConfigDiscoveryImplTestParameter, WrongDefaultConfig) { EXPECT_THROW_WITH_MESSAGE( config_discovery_test.filter_config_provider_manager_->createDynamicFilterConfigProvider( config_source, "foo", config_discovery_test.factory_context_, "xds.", true, - config_discovery_test.getFilterType()), + config_discovery_test.getFilterType(), config_discovery_test.getFilterMatcher()), EnvoyException, "Error: cannot find filter factory foo for default filter " "configuration with type URL " diff --git a/test/integration/listener_extension_discovery_integration_test.cc b/test/integration/listener_extension_discovery_integration_test.cc index a9a3e1ef5230..da843ad2b3b2 100644 --- a/test/integration/listener_extension_discovery_integration_test.cc +++ b/test/integration/listener_extension_discovery_integration_test.cc @@ -13,10 +13,10 @@ namespace Envoy { namespace { -class ExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, - public BaseIntegrationTest { +class ListenerExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, + public BaseIntegrationTest { public: - ExtensionDiscoveryIntegrationTest() + ListenerExtensionDiscoveryIntegrationTest() : BaseIntegrationTest(ipVersion(), ConfigHelper::baseConfig() + R"EOF( filter_chains: - filters: @@ -72,7 +72,7 @@ class ExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegrationPara registerTestServerPorts({port_name_}); } - ~ExtensionDiscoveryIntegrationTest() override { + ~ListenerExtensionDiscoveryIntegrationTest() override { if (ecds_connection_ != nullptr) { AssertionResult result = ecds_connection_->close(); RELEASE_ASSERT(result, result.message()); @@ -150,10 +150,10 @@ class ExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegrationPara FakeStreamPtr ecds_stream_{nullptr}; }; -INSTANTIATE_TEST_SUITE_P(IpVersionsClientType, ExtensionDiscoveryIntegrationTest, +INSTANTIATE_TEST_SUITE_P(IpVersionsClientType, ListenerExtensionDiscoveryIntegrationTest, GRPC_CLIENT_INTEGRATION_PARAMS); -TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccess) { +TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicSuccess) { on_server_init_function_ = [&]() { waitXdsStream(); }; addDynamicFilter(filter_name_, false); initialize(); @@ -173,7 +173,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccess) { sendDataVerifyResults(3); } -TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithTtl) { +TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicSuccessWithTtl) { on_server_init_function_ = [&]() { waitXdsStream(); }; addDynamicFilter(filter_name_, false, false); initialize(); @@ -199,7 +199,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithTtl) { }); } -TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithTtlWithDefault) { +TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicSuccessWithTtlWithDefault) { on_server_init_function_ = [&]() { waitXdsStream(); }; addDynamicFilter(filter_name_, false, true); initialize(); @@ -220,7 +220,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithTtlWithDefault) { } // This one TBD -TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailWithDefault) { +TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicFailWithDefault) { on_server_init_function_ = [&]() { waitXdsStream(); }; addDynamicFilter(filter_name_, false, true); initialize(); @@ -236,7 +236,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailWithDefault) { } // This one TBD -TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailWithoutDefault) { +TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicFailWithoutDefault) { on_server_init_function_ = [&]() { waitXdsStream(); }; addDynamicFilter(filter_name_, false, false); initialize(); @@ -258,7 +258,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailWithoutDefault) { }); } -TEST_P(ExtensionDiscoveryIntegrationTest, BasicWithoutWarming) { +TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicWithoutWarming) { on_server_init_function_ = [&]() { waitXdsStream(); }; addDynamicFilter(filter_name_, true); initialize(); @@ -272,7 +272,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicWithoutWarming) { sendDataVerifyResults(3); } -TEST_P(ExtensionDiscoveryIntegrationTest, BasicWithoutWarmingFail) { +TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicWithoutWarmingFail) { on_server_init_function_ = [&]() { waitXdsStream(); }; addDynamicFilter(filter_name_, true); initialize(); @@ -283,33 +283,20 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicWithoutWarmingFail) { sendDataVerifyResults(kDefaultDrainBytes); } -TEST_P(ExtensionDiscoveryIntegrationTest, BasicTwoSubscriptionsSameNameWithoutWarming) { +TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicTwoSubscriptionsSameName) { on_server_init_function_ = [&]() { waitXdsStream(); }; addDynamicFilter(filter_name_, true); - // Adding a filter with same name overrides the previous one. addDynamicFilter(filter_name_, false); initialize(); sendXdsResponse("1", 3); test_server_->waitForCounterGe( "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 1); - sendDataVerifyResults(3); -} - -TEST_P(ExtensionDiscoveryIntegrationTest, BasicTwoSubscriptionsSameNameWithWarming) { - on_server_init_function_ = [&]() { waitXdsStream(); }; - addDynamicFilter(filter_name_, false); - // Adding a filter with same name overrides the previous one. - addDynamicFilter(filter_name_, true); - initialize(); - - sendXdsResponse("1", 3); - test_server_->waitForCounterGe( - "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 1); - sendDataVerifyResults(3); + // Each filter drain 3 bytes. + sendDataVerifyResults(6); } -TEST_P(ExtensionDiscoveryIntegrationTest, DestroyDuringInit) { +TEST_P(ListenerExtensionDiscoveryIntegrationTest, DestroyDuringInit) { // If rate limiting is enabled on the config source, gRPC mux drainage updates the requests // queue size on destruction. The update calls out to stats scope nested under the extension // config subscription stats scope. This test verifies that the stats scope outlasts the gRPC diff --git a/test/mocks/server/listener_component_factory.h b/test/mocks/server/listener_component_factory.h index c536b57fb082..89c21114bad1 100644 --- a/test/mocks/server/listener_component_factory.h +++ b/test/mocks/server/listener_component_factory.h @@ -33,7 +33,7 @@ class MockListenerComponentFactory : public ListenerComponentFactory { MOCK_METHOD(Filter::ListenerFilterFactoriesList, createListenerFilterFactoryList, (const Protobuf::RepeatedPtrField&, Configuration::ListenerFactoryContext& context)); - MOCK_METHOD(Filter::UdpListenerFilterFactoriesList, createUdpListenerFilterFactoryList, + MOCK_METHOD(std::vector, createUdpListenerFilterFactoryList, (const Protobuf::RepeatedPtrField&, Configuration::ListenerFactoryContext& context)); MOCK_METHOD(Network::SocketSharedPtr, createListenSocket, diff --git a/test/server/listener_manager_impl_test.h b/test/server/listener_manager_impl_test.h index 8f13a9767c7f..de03a786b0aa 100644 --- a/test/server/listener_manager_impl_test.h +++ b/test/server/listener_manager_impl_test.h @@ -97,13 +97,13 @@ class ListenerManagerImplTest : public testing::TestWithParam { filters, context, manager_->getTcpListenerConfigProviderManager()); })); ON_CALL(listener_factory_, createUdpListenerFilterFactoryList(_, _)) - .WillByDefault(Invoke( - [this](const Protobuf::RepeatedPtrField& - filters, - Configuration::ListenerFactoryContext& context) - -> Filter::UdpListenerFilterFactoriesList { - return ProdListenerComponentFactory::createUdpListenerFilterFactoryList_( - filters, context, manager_->getUdpListenerConfigProviderManager()); + .WillByDefault( + Invoke([](const Protobuf::RepeatedPtrField& + filters, + Configuration::ListenerFactoryContext& context) + -> std::vector { + return ProdListenerComponentFactory::createUdpListenerFilterFactoryList_(filters, + context); })); ON_CALL(listener_factory_, nextListenerTag()).WillByDefault(Invoke([this]() { return listener_tag_++; From 9f2b442e8c90cf1e426917ed4de8d3f303eff90f Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Sun, 8 May 2022 03:28:22 +0000 Subject: [PATCH 04/28] fixing CI error Signed-off-by: Yanjun Xiang --- test/config_test/config_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/config_test/config_test.cc b/test/config_test/config_test.cc index 0f01ac435b3c..bbabbf90100e 100644 --- a/test/config_test/config_test.cc +++ b/test/config_test/config_test.cc @@ -116,9 +116,9 @@ class ConfigTest { [&](const Protobuf::RepeatedPtrField& filters, Server::Configuration::ListenerFactoryContext& context) - -> std::vector { + -> Filter::ListenerFilterFactoriesList { return Server::ProdListenerComponentFactory::createListenerFilterFactoryList_( - filters, context); + filters, context, listener_manager_.getTcpListenerConfigProviderManager()); })); ON_CALL(component_factory_, createUdpListenerFilterFactoryList(_, _)) .WillByDefault(Invoke( From f9dd5a0d0a56a413f803f9a7bd4275d2b8593dfb Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Tue, 10 May 2022 03:39:40 +0000 Subject: [PATCH 05/28] addressing comments Signed-off-by: Yanjun Xiang --- source/server/configuration_impl.cc | 8 ++------ source/server/listener_impl.cc | 8 ++++---- source/server/listener_manager_impl.cc | 19 ++++++++++--------- 3 files changed, 16 insertions(+), 19 deletions(-) diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index 8d8ef332ee62..da39835226fa 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -45,20 +45,16 @@ class MissingConfigTcpListenerFilter : public Network::ListenerFilter, // Network::ListenerFilter Network::FilterStatus onAccept(Network::ListenerFilterCallbacks& cb) override { - cb_ = &cb; ENVOY_LOG(warn, "Listener filter: new connection accepted while missing configuration. " "Close socket and stop the iteration onAccept."); - cb_->socket().ioHandle().close(); + cb.socket().ioHandle().close(); return Network::FilterStatus::StopIteration; } Network::FilterStatus onData(Network::ListenerFilterBuffer&) override { // The socket is already closed onAccept. Just return StopIteration here. return Network::FilterStatus::StopIteration; } - size_t maxReadBytes() const override { return 1024; } - -private: - Network::ListenerFilterCallbacks* cb_{}; + size_t maxReadBytes() const override { return 0; } }; bool FilterChainUtility::buildFilterChain(Network::ListenerFilterManager& filter_manager, diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index e20a0f32169b..e79331297601 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -703,8 +703,8 @@ void ListenerImpl::buildOriginalDstListenerFilter() { Network::ListenerFilterFactoryCb callback = factory.createListenerFilterFactoryFromProto( Envoy::ProtobufWkt::Empty(), nullptr, *listener_factory_context_); - auto cfg_provider = parent_.getTcpListenerConfigProviderManager(); - auto filter_config_provider = cfg_provider.createStaticFilterConfigProvider( + auto& cfg_provider_manager = parent_.getTcpListenerConfigProviderManager(); + auto filter_config_provider = cfg_provider_manager.createStaticFilterConfigProvider( callback, "envoy.filters.listener.original_dst"); listener_filter_factories_.push_back(std::move(filter_config_provider)); } @@ -723,8 +723,8 @@ void ListenerImpl::buildProxyProtocolListenerFilter() { Network::ListenerFilterFactoryCb callback = factory.createListenerFilterFactoryFromProto( envoy::extensions::filters::listener::proxy_protocol::v3::ProxyProtocol(), nullptr, *listener_factory_context_); - auto cfg_provider = parent_.getTcpListenerConfigProviderManager(); - auto filter_config_provider = cfg_provider.createStaticFilterConfigProvider( + auto& cfg_provider_manager = parent_.getTcpListenerConfigProviderManager(); + auto filter_config_provider = cfg_provider_manager.createStaticFilterConfigProvider( callback, "envoy.filters.listener.proxy_protocol"); listener_filter_factories_.push_back(std::move(filter_config_provider)); } diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 8f74ea13eb5a..f50d30a966cd 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -113,6 +113,8 @@ Filter::ListenerFilterFactoriesList ProdListenerComponentFactory::createListener Configuration::ListenerFactoryContext& context, Filter::TcpListenerFilterConfigProviderManagerImpl& config_provider_manager) { Filter::ListenerFilterFactoriesList ret; + + ret.reserve(filters.size()); for (ssize_t i = 0; i < filters.size(); i++) { const auto& proto_config = filters[i]; ENVOY_LOG(debug, " filter #{}:", i); @@ -123,8 +125,8 @@ Filter::ListenerFilterFactoriesList ProdListenerComponentFactory::createListener // dynamic listener filter configuration if (proto_config.config_type_case() == envoy::config::listener::v3::ListenerFilter::ConfigTypeCase::kConfigDiscovery) { - auto& config_discovery = proto_config.config_discovery(); - auto& name = proto_config.name(); + const auto& config_discovery = proto_config.config_discovery(); + const auto& name = proto_config.name(); ENVOY_LOG(debug, " Listener filter: dynamic filter name: {}", name); if (config_discovery.apply_default_config_without_warming() && !config_discovery.has_default_config()) { @@ -133,8 +135,8 @@ Filter::ListenerFilterFactoriesList ProdListenerComponentFactory::createListener name)); } for (const auto& type_url : config_discovery.type_urls()) { - auto factory_type_url = TypeUtil::typeUrlToDescriptorFullName(type_url); - auto* factory = + const auto factory_type_url = TypeUtil::typeUrlToDescriptorFullName(type_url); + const auto* factory = Registry::FactoryRegistry:: getFactoryByType(factory_type_url); if (factory == nullptr) { @@ -151,7 +153,7 @@ Filter::ListenerFilterFactoriesList ProdListenerComponentFactory::createListener auto& factory = Config::Utility::getAndCheckFactory( proto_config); - auto message = Config::Utility::translateToFactoryConfig( + const auto message = Config::Utility::translateToFactoryConfig( proto_config, context.messageValidationVisitor(), factory); Network::ListenerFilterFactoryCb callback = factory.createListenerFilterFactoryFromProto( @@ -282,14 +284,13 @@ ListenerManagerImpl::ListenerManagerImpl(Instance& server, [this](const Matchers::StringMatcher& name_matcher) { return dumpListenerConfigs(name_matcher); })), - enable_dispatcher_stats_(enable_dispatcher_stats), quic_stat_names_(quic_stat_names) { + enable_dispatcher_stats_(enable_dispatcher_stats), quic_stat_names_(quic_stat_names), + tcp_listener_config_provider_manager_( + std::make_unique()) { for (uint32_t i = 0; i < server.options().concurrency(); i++) { workers_.emplace_back( worker_factory.createWorker(i, server.overloadManager(), absl::StrCat("worker_", i))); } - // Create TCP listener filter config provider manager instance. - tcp_listener_config_provider_manager_ = - std::make_unique(); } ProtobufTypes::MessagePtr From b2eed54d39c404e3fe2c73c326a16954b587ae70 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Tue, 10 May 2022 16:44:14 +0000 Subject: [PATCH 06/28] fixing CI error Signed-off-by: Yanjun Xiang --- source/server/listener_manager_impl.cc | 2 +- source/server/listener_manager_impl.h | 2 +- .../listener_extension_discovery_integration_test.cc | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index f50d30a966cd..6c941b623aa0 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -286,7 +286,7 @@ ListenerManagerImpl::ListenerManagerImpl(Instance& server, })), enable_dispatcher_stats_(enable_dispatcher_stats), quic_stat_names_(quic_stat_names), tcp_listener_config_provider_manager_( - std::make_unique()) { + std::make_shared()) { for (uint32_t i = 0; i < server.options().concurrency(); i++) { workers_.emplace_back( worker_factory.createWorker(i, server.overloadManager(), absl::StrCat("worker_", i))); diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index f5402faa9e0b..679a8af38c37 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -259,7 +259,7 @@ class ListenerManagerImpl : public ListenerManager, Logger::Loggable> error_state_tracker_; FailureStates overall_error_state_; Quic::QuicStatNames& quic_stat_names_; - std::unique_ptr + std::shared_ptr tcp_listener_config_provider_manager_; }; diff --git a/test/integration/listener_extension_discovery_integration_test.cc b/test/integration/listener_extension_discovery_integration_test.cc index da843ad2b3b2..d3bda0089b50 100644 --- a/test/integration/listener_extension_discovery_integration_test.cc +++ b/test/integration/listener_extension_discovery_integration_test.cc @@ -302,7 +302,7 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, DestroyDuringInit) { // config subscription stats scope. This test verifies that the stats scope outlasts the gRPC // subscription. on_server_init_function_ = [&]() { waitXdsStream(); }; - addDynamicFilter("foo", false, true); + addDynamicFilter(filter_name_, false, true); initialize(); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); test_server_.reset(); From 3d8265c4976dc7d48c7e165db8a108e7db644344 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Tue, 10 May 2022 21:57:34 +0000 Subject: [PATCH 07/28] comments Signed-off-by: Yanjun Xiang --- ...er_extension_discovery_integration_test.cc | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/test/integration/listener_extension_discovery_integration_test.cc b/test/integration/listener_extension_discovery_integration_test.cc index d3bda0089b50..3c124279bad6 100644 --- a/test/integration/listener_extension_discovery_integration_test.cc +++ b/test/integration/listener_extension_discovery_integration_test.cc @@ -25,7 +25,8 @@ class ListenerExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegra "@type": type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy stat_prefix: tcp_stats cluster: cluster_0 -)EOF") {} +)EOF"), + default_drain_bytes_(2), filter_name_("foo"), data_("HelloWorld"), port_name_("http") {} void addDynamicFilter(const std::string& name, bool apply_without_warming, bool set_default_config = true, bool rate_limit = false) { @@ -39,7 +40,7 @@ class ListenerExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegra "type.googleapis.com/test.integration.filters.TestTcpListenerFilterConfig"); if (set_default_config) { auto default_configuration = test::integration::filters::TestTcpListenerFilterConfig(); - default_configuration.set_drain_bytes(kDefaultDrainBytes); + default_configuration.set_drain_bytes(default_drain_bytes_); discovery->mutable_default_config()->PackFrom(default_configuration); } @@ -138,10 +139,10 @@ class ListenerExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegra tcp_client->close(); } - const uint32_t kDefaultDrainBytes = 2; - const std::string filter_name_ = "foo"; - const std::string data_ = "HelloWorld"; - const std::string port_name_ = "http"; + const uint32_t default_drain_bytes_; + const std::string filter_name_; + const std::string data_; + const std::string port_name_; FakeUpstream& getEcdsFakeUpstream() const { return *fake_upstreams_[1]; } @@ -216,7 +217,7 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicSuccessWithTtlWithDefault test_server_->waitForCounterGe( "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 2); // Start a TCP connection. The default filter drain 2 bytes. - sendDataVerifyResults(kDefaultDrainBytes); + sendDataVerifyResults(default_drain_bytes_); } // This one TBD @@ -232,7 +233,7 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicFailWithDefault) { test_server_->waitForCounterGe( "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_fail", 1); // The default filter will be installed. Start a TCP connection. The default filter drain 2 bytes. - sendDataVerifyResults(kDefaultDrainBytes); + sendDataVerifyResults(default_drain_bytes_); } // This one TBD @@ -264,7 +265,7 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicWithoutWarming) { initialize(); // Send data without send config update. - sendDataVerifyResults(kDefaultDrainBytes); + sendDataVerifyResults(default_drain_bytes_); // Send update should cause a different response. sendXdsResponse("1", 3); test_server_->waitForCounterGe( @@ -280,7 +281,7 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicWithoutWarmingFail) { sendXdsResponse("1", 1); test_server_->waitForCounterGe( "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_fail", 1); - sendDataVerifyResults(kDefaultDrainBytes); + sendDataVerifyResults(default_drain_bytes_); } TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicTwoSubscriptionsSameName) { From bf16c3deffc30adc08ff76d17a3d6dfa5e8fd30b Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Wed, 11 May 2022 19:28:46 +0000 Subject: [PATCH 08/28] fix CI error Signed-off-by: Yanjun Xiang --- source/server/config_validation/server.h | 11 +- source/server/listener_impl.cc | 8 +- source/server/listener_manager_impl.cc | 12 +- source/server/listener_manager_impl.h | 163 +++++++++--------- test/config_test/config_test.cc | 14 +- ...er_extension_discovery_integration_test.cc | 3 +- test/server/listener_manager_impl_test.cc | 2 +- test/server/listener_manager_impl_test.h | 8 +- 8 files changed, 117 insertions(+), 104 deletions(-) diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index d894a9c87a81..a8617ee62b7b 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -145,19 +145,22 @@ class ValidationInstance final : Logger::Loggable, std::vector createNetworkFilterFactoryList( const Protobuf::RepeatedPtrField& filters, Server::Configuration::FilterChainFactoryContext& filter_chain_factory_context) override { - return ProdListenerComponentFactory::createNetworkFilterFactoryList_( + return ProdListenerComponentFactory::createNetworkFilterFactoryListImpl( filters, filter_chain_factory_context); } Filter::ListenerFilterFactoriesList createListenerFilterFactoryList( const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) override { - return ProdListenerComponentFactory::createListenerFilterFactoryList_( - filters, context, listener_manager_->getTcpListenerConfigProviderManager()); + ProdListenerComponentFactory* listener_component = + dynamic_cast(&listener_manager_->factory_); + auto& cfg_provider_manager = listener_component->getTcpListenerConfigProviderManager(); + return ProdListenerComponentFactory::createListenerFilterFactoryListImpl(filters, context, + cfg_provider_manager); } std::vector createUdpListenerFilterFactoryList( const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) override { - return ProdListenerComponentFactory::createUdpListenerFilterFactoryList_(filters, context); + return ProdListenerComponentFactory::createUdpListenerFilterFactoryListImpl(filters, context); } Network::SocketSharedPtr createListenSocket(Network::Address::InstanceConstSharedPtr, Network::Socket::Type, diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index e79331297601..a0be8663b374 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -703,7 +703,9 @@ void ListenerImpl::buildOriginalDstListenerFilter() { Network::ListenerFilterFactoryCb callback = factory.createListenerFilterFactoryFromProto( Envoy::ProtobufWkt::Empty(), nullptr, *listener_factory_context_); - auto& cfg_provider_manager = parent_.getTcpListenerConfigProviderManager(); + ProdListenerComponentFactory* listener_component = + dynamic_cast(&parent_.factory_); + auto& cfg_provider_manager = listener_component->getTcpListenerConfigProviderManager(); auto filter_config_provider = cfg_provider_manager.createStaticFilterConfigProvider( callback, "envoy.filters.listener.original_dst"); listener_filter_factories_.push_back(std::move(filter_config_provider)); @@ -723,7 +725,9 @@ void ListenerImpl::buildProxyProtocolListenerFilter() { Network::ListenerFilterFactoryCb callback = factory.createListenerFilterFactoryFromProto( envoy::extensions::filters::listener::proxy_protocol::v3::ProxyProtocol(), nullptr, *listener_factory_context_); - auto& cfg_provider_manager = parent_.getTcpListenerConfigProviderManager(); + ProdListenerComponentFactory* listener_component = + dynamic_cast(&parent_.factory_); + auto& cfg_provider_manager = listener_component->getTcpListenerConfigProviderManager(); auto filter_config_provider = cfg_provider_manager.createStaticFilterConfigProvider( callback, "envoy.filters.listener.proxy_protocol"); listener_filter_factories_.push_back(std::move(filter_config_provider)); diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 6c941b623aa0..4d021c5740f2 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -77,7 +77,8 @@ void fillState(envoy::admin::v3::ListenersConfigDump::DynamicListenerState& stat } } // namespace -std::vector ProdListenerComponentFactory::createNetworkFilterFactoryList_( +std::vector +ProdListenerComponentFactory::createNetworkFilterFactoryListImpl( const Protobuf::RepeatedPtrField& filters, Server::Configuration::FilterChainFactoryContext& filter_chain_factory_context) { std::vector ret; @@ -108,7 +109,8 @@ std::vector ProdListenerComponentFactory::createNetwor return ret; } -Filter::ListenerFilterFactoriesList ProdListenerComponentFactory::createListenerFilterFactoryList_( +Filter::ListenerFilterFactoriesList +ProdListenerComponentFactory::createListenerFilterFactoryListImpl( const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context, Filter::TcpListenerFilterConfigProviderManagerImpl& config_provider_manager) { @@ -170,7 +172,7 @@ Filter::ListenerFilterFactoriesList ProdListenerComponentFactory::createListener } std::vector -ProdListenerComponentFactory::createUdpListenerFilterFactoryList_( +ProdListenerComponentFactory::createUdpListenerFilterFactoryListImpl( const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) { std::vector ret; @@ -284,9 +286,7 @@ ListenerManagerImpl::ListenerManagerImpl(Instance& server, [this](const Matchers::StringMatcher& name_matcher) { return dumpListenerConfigs(name_matcher); })), - enable_dispatcher_stats_(enable_dispatcher_stats), quic_stat_names_(quic_stat_names), - tcp_listener_config_provider_manager_( - std::make_shared()) { + enable_dispatcher_stats_(enable_dispatcher_stats), quic_stat_names_(quic_stat_names) { for (uint32_t i = 0; i < server.options().concurrency(); i++) { workers_.emplace_back( worker_factory.createWorker(i, server.overloadManager(), absl::StrCat("worker_", i))); diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index 679a8af38c37..e8be1a57724e 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -34,6 +34,88 @@ class TransportSocketFactoryContextImpl; class ListenerFilterChainFactoryBuilder; +/** + * Prod implementation of ListenerComponentFactory that creates real sockets and attempts to fetch + * sockets from the parent process via the hot restarter. The filter factory list is created from + * statically registered filters. + */ +class ProdListenerComponentFactory : public ListenerComponentFactory, + Logger::Loggable { +public: + ProdListenerComponentFactory(Instance& server) + : server_(server), + tcp_listener_config_provider_manager_( + std::make_unique()) {} + + /** + * Static worker for createNetworkFilterFactoryList() that can be used directly in tests. + */ + static std::vector createNetworkFilterFactoryListImpl( + const Protobuf::RepeatedPtrField& filters, + Configuration::FilterChainFactoryContext& filter_chain_factory_context); + + /** + * Static worker for createListenerFilterFactoryList() that can be used directly in tests. + */ + static Filter::ListenerFilterFactoriesList createListenerFilterFactoryListImpl( + const Protobuf::RepeatedPtrField& filters, + Configuration::ListenerFactoryContext& context, + Filter::TcpListenerFilterConfigProviderManagerImpl& config_provider_manager); + + /** + * Static worker for createUdpListenerFilterFactoryList() that can be used directly in tests. + */ + static std::vector createUdpListenerFilterFactoryListImpl( + const Protobuf::RepeatedPtrField& filters, + Configuration::ListenerFactoryContext& context); + + static Network::ListenerFilterMatcherSharedPtr + createListenerFilterMatcher(const envoy::config::listener::v3::ListenerFilter& listener_filter); + + // Server::ListenerComponentFactory + LdsApiPtr createLdsApi(const envoy::config::core::v3::ConfigSource& lds_config, + const xds::core::v3::ResourceLocator* lds_resources_locator) override { + return std::make_unique( + lds_config, lds_resources_locator, server_.clusterManager(), server_.initManager(), + server_.stats(), server_.listenerManager(), + server_.messageValidationContext().dynamicValidationVisitor()); + } + std::vector createNetworkFilterFactoryList( + const Protobuf::RepeatedPtrField& filters, + Server::Configuration::FilterChainFactoryContext& filter_chain_factory_context) override { + return createNetworkFilterFactoryListImpl(filters, filter_chain_factory_context); + } + Filter::ListenerFilterFactoriesList createListenerFilterFactoryList( + const Protobuf::RepeatedPtrField& filters, + Configuration::ListenerFactoryContext& context) override { + return createListenerFilterFactoryListImpl(filters, context, + getTcpListenerConfigProviderManager()); + } + std::vector createUdpListenerFilterFactoryList( + const Protobuf::RepeatedPtrField& filters, + Configuration::ListenerFactoryContext& context) override { + return createUdpListenerFilterFactoryListImpl(filters, context); + } + Network::SocketSharedPtr createListenSocket( + Network::Address::InstanceConstSharedPtr address, Network::Socket::Type socket_type, + const Network::Socket::OptionsSharedPtr& options, BindType bind_type, + const Network::SocketCreationOptions& creation_options, uint32_t worker_index) override; + + DrainManagerPtr + createDrainManager(envoy::config::listener::v3::Listener::DrainType drain_type) override; + uint64_t nextListenerTag() override { return next_listener_tag_++; } + + Filter::TcpListenerFilterConfigProviderManagerImpl& getTcpListenerConfigProviderManager() { + return *tcp_listener_config_provider_manager_; + } + +private: + Instance& server_; + uint64_t next_listener_tag_{1}; + std::unique_ptr + tcp_listener_config_provider_manager_; +}; + class ListenerImpl; using ListenerImplPtr = std::unique_ptr; @@ -134,9 +216,6 @@ class ListenerManagerImpl : public ListenerManager, Logger::Loggable> error_state_tracker_; FailureStates overall_error_state_; Quic::QuicStatNames& quic_stat_names_; - std::shared_ptr - tcp_listener_config_provider_manager_; -}; - -/** - * Prod implementation of ListenerComponentFactory that creates real sockets and attempts to fetch - * sockets from the parent process via the hot restarter. The filter factory list is created from - * statically registered filters. - */ -class ProdListenerComponentFactory : public ListenerComponentFactory, - Logger::Loggable { -public: - ProdListenerComponentFactory(Instance& server) : server_(server) {} - - /** - * Static worker for createNetworkFilterFactoryList() that can be used directly in tests. - */ - static std::vector createNetworkFilterFactoryList_( - const Protobuf::RepeatedPtrField& filters, - Configuration::FilterChainFactoryContext& filter_chain_factory_context); - - /** - * Static worker for createListenerFilterFactoryList() that can be used directly in tests. - */ - static Filter::ListenerFilterFactoriesList createListenerFilterFactoryList_( - const Protobuf::RepeatedPtrField& filters, - Configuration::ListenerFactoryContext& context, - Filter::TcpListenerFilterConfigProviderManagerImpl& config_provider_manager); - - /** - * Static worker for createUdpListenerFilterFactoryList() that can be used directly in tests. - */ - static std::vector createUdpListenerFilterFactoryList_( - const Protobuf::RepeatedPtrField& filters, - Configuration::ListenerFactoryContext& context); - - static Network::ListenerFilterMatcherSharedPtr - createListenerFilterMatcher(const envoy::config::listener::v3::ListenerFilter& listener_filter); - - // Server::ListenerComponentFactory - LdsApiPtr createLdsApi(const envoy::config::core::v3::ConfigSource& lds_config, - const xds::core::v3::ResourceLocator* lds_resources_locator) override { - return std::make_unique( - lds_config, lds_resources_locator, server_.clusterManager(), server_.initManager(), - server_.stats(), server_.listenerManager(), - server_.messageValidationContext().dynamicValidationVisitor()); - } - std::vector createNetworkFilterFactoryList( - const Protobuf::RepeatedPtrField& filters, - Server::Configuration::FilterChainFactoryContext& filter_chain_factory_context) override { - return createNetworkFilterFactoryList_(filters, filter_chain_factory_context); - } - Filter::ListenerFilterFactoriesList createListenerFilterFactoryList( - const Protobuf::RepeatedPtrField& filters, - Configuration::ListenerFactoryContext& context) override { - // TODO(yanjunxiang): cleanup this dynamic_cast. - ListenerManagerImpl* listener_manager = - dynamic_cast(&(server_.listenerManager())); - return createListenerFilterFactoryList_( - filters, context, listener_manager->getTcpListenerConfigProviderManager()); - } - std::vector createUdpListenerFilterFactoryList( - const Protobuf::RepeatedPtrField& filters, - Configuration::ListenerFactoryContext& context) override { - return createUdpListenerFilterFactoryList_(filters, context); - } - Network::SocketSharedPtr createListenSocket( - Network::Address::InstanceConstSharedPtr address, Network::Socket::Type socket_type, - const Network::Socket::OptionsSharedPtr& options, BindType bind_type, - const Network::SocketCreationOptions& creation_options, uint32_t worker_index) override; - - DrainManagerPtr - createDrainManager(envoy::config::listener::v3::Listener::DrainType drain_type) override; - uint64_t nextListenerTag() override { return next_listener_tag_++; } - -private: - Instance& server_; - uint64_t next_listener_tag_{1}; }; class ListenerFilterChainFactoryBuilder : public FilterChainFactoryBuilder { diff --git a/test/config_test/config_test.cc b/test/config_test/config_test.cc index bbabbf90100e..cd4208f59913 100644 --- a/test/config_test/config_test.cc +++ b/test/config_test/config_test.cc @@ -108,8 +108,8 @@ class ConfigTest { [&](const Protobuf::RepeatedPtrField& filters, Server::Configuration::FilterChainFactoryContext& context) -> std::vector { - return Server::ProdListenerComponentFactory::createNetworkFilterFactoryList_(filters, - context); + return Server::ProdListenerComponentFactory::createNetworkFilterFactoryListImpl( + filters, context); })); ON_CALL(component_factory_, createListenerFilterFactoryList(_, _)) .WillByDefault(Invoke( @@ -117,8 +117,12 @@ class ConfigTest { filters, Server::Configuration::ListenerFactoryContext& context) -> Filter::ListenerFilterFactoriesList { - return Server::ProdListenerComponentFactory::createListenerFilterFactoryList_( - filters, context, listener_manager_.getTcpListenerConfigProviderManager()); + Server::ProdListenerComponentFactory* listener_component = + dynamic_cast(&listener_manager_.factory_); + auto& cfg_provider_manager = + listener_component->getTcpListenerConfigProviderManager(); + return Server::ProdListenerComponentFactory::createListenerFilterFactoryListImpl( + filters, context, cfg_provider_manager); })); ON_CALL(component_factory_, createUdpListenerFilterFactoryList(_, _)) .WillByDefault(Invoke( @@ -126,7 +130,7 @@ class ConfigTest { filters, Server::Configuration::ListenerFactoryContext& context) -> std::vector { - return Server::ProdListenerComponentFactory::createUdpListenerFilterFactoryList_( + return Server::ProdListenerComponentFactory::createUdpListenerFilterFactoryListImpl( filters, context); })); ON_CALL(server_, serverFactoryContext()).WillByDefault(ReturnRef(server_factory_context_)); diff --git a/test/integration/listener_extension_discovery_integration_test.cc b/test/integration/listener_extension_discovery_integration_test.cc index 3c124279bad6..97acf6be6db5 100644 --- a/test/integration/listener_extension_discovery_integration_test.cc +++ b/test/integration/listener_extension_discovery_integration_test.cc @@ -30,7 +30,8 @@ class ListenerExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegra void addDynamicFilter(const std::string& name, bool apply_without_warming, bool set_default_config = true, bool rate_limit = false) { - config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + config_helper_.addConfigModifier([name, apply_without_warming, set_default_config, rate_limit, + this](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { auto* listener_filter = bootstrap.mutable_static_resources()->mutable_listeners(0)->add_listener_filters(); listener_filter->set_name(name); diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index 79f33c02f8a8..e699d22dd16e 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -4676,7 +4676,7 @@ TEST_P(ListenerManagerImplWithRealFiltersTest, Metadata) { Configuration::ListenerFactoryContext& context) -> Filter::ListenerFilterFactoriesList { listener_factory_context = &context; - return ProdListenerComponentFactory::createListenerFilterFactoryList_( + return ProdListenerComponentFactory::createListenerFilterFactoryListImpl( filters, context, manager_->getTcpListenerConfigProviderManager()); })); server_.server_factory_context_->cluster_manager_.initializeClusters({"service_foo"}, {}); diff --git a/test/server/listener_manager_impl_test.h b/test/server/listener_manager_impl_test.h index de03a786b0aa..dbad4027f3a4 100644 --- a/test/server/listener_manager_impl_test.h +++ b/test/server/listener_manager_impl_test.h @@ -84,7 +84,7 @@ class ListenerManagerImplTest : public testing::TestWithParam { [](const Protobuf::RepeatedPtrField& filters, Server::Configuration::FilterChainFactoryContext& filter_chain_factory_context) -> std::vector { - return ProdListenerComponentFactory::createNetworkFilterFactoryList_( + return ProdListenerComponentFactory::createNetworkFilterFactoryListImpl( filters, filter_chain_factory_context); })); ON_CALL(listener_factory_, createListenerFilterFactoryList(_, _)) @@ -93,7 +93,7 @@ class ListenerManagerImplTest : public testing::TestWithParam { filters, Configuration::ListenerFactoryContext& context) -> Filter::ListenerFilterFactoriesList { - return ProdListenerComponentFactory::createListenerFilterFactoryList_( + return ProdListenerComponentFactory::createListenerFilterFactoryListImpl( filters, context, manager_->getTcpListenerConfigProviderManager()); })); ON_CALL(listener_factory_, createUdpListenerFilterFactoryList(_, _)) @@ -102,8 +102,8 @@ class ListenerManagerImplTest : public testing::TestWithParam { filters, Configuration::ListenerFactoryContext& context) -> std::vector { - return ProdListenerComponentFactory::createUdpListenerFilterFactoryList_(filters, - context); + return ProdListenerComponentFactory::createUdpListenerFilterFactoryListImpl(filters, + context); })); ON_CALL(listener_factory_, nextListenerTag()).WillByDefault(Invoke([this]() { return listener_tag_++; From 7011774849d25b2f6c608c1653ccb354df666115 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Thu, 12 May 2022 02:53:05 +0000 Subject: [PATCH 09/28] fix CI error Signed-off-by: Yanjun Xiang --- test/config_test/config_test.cc | 4 +-- test/mocks/server/BUILD | 2 ++ .../server/listener_component_factory.cc | 29 ++++++++++--------- .../mocks/server/listener_component_factory.h | 20 ++++++++----- test/server/api_listener_test.cc | 5 ++-- test/server/listener_manager_impl_test.cc | 4 +-- test/server/listener_manager_impl_test.h | 7 +++-- 7 files changed, 41 insertions(+), 30 deletions(-) diff --git a/test/config_test/config_test.cc b/test/config_test/config_test.cc index cd4208f59913..079792b427e7 100644 --- a/test/config_test/config_test.cc +++ b/test/config_test/config_test.cc @@ -56,7 +56,7 @@ static std::vector unsuported_win32_configs = { class ConfigTest { public: ConfigTest(const OptionsImpl& options) - : api_(Api::createApiForTest(time_system_)), options_(options) { + : api_(Api::createApiForTest(time_system_)), options_(options), component_factory_(server_) { ON_CALL(server_, options()).WillByDefault(ReturnRef(options_)); ON_CALL(server_, sslContextManager()).WillByDefault(ReturnRef(ssl_context_manager_)); ON_CALL(server_.api_, fileSystem()).WillByDefault(ReturnRef(file_system_)); @@ -152,7 +152,7 @@ class ConfigTest { NiceMock ssl_context_manager_; OptionsImpl options_; std::unique_ptr cluster_manager_factory_; - NiceMock component_factory_; + NiceMock component_factory_; NiceMock worker_factory_; Server::ListenerManagerImpl listener_manager_{server_, component_factory_, worker_factory_, false, server_.quic_stat_names_}; diff --git a/test/mocks/server/BUILD b/test/mocks/server/BUILD index 28311cf68986..54431039e523 100644 --- a/test/mocks/server/BUILD +++ b/test/mocks/server/BUILD @@ -121,8 +121,10 @@ envoy_cc_mock( deps = [ "//envoy/server:drain_manager_interface", "//envoy/server:listener_manager_interface", + "//source/server:listener_manager_lib", "//test/mocks/network:io_handle_mocks", "//test/mocks/network:network_mocks", + "//test/mocks/server:instance_mocks", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/config/listener/v3:pkg_cc_proto", ], diff --git a/test/mocks/server/listener_component_factory.cc b/test/mocks/server/listener_component_factory.cc index 64eabf2a522c..04f15ca714a7 100644 --- a/test/mocks/server/listener_component_factory.cc +++ b/test/mocks/server/listener_component_factory.cc @@ -11,22 +11,25 @@ namespace Server { using ::testing::_; using ::testing::Invoke; -MockListenerComponentFactory::MockListenerComponentFactory() - : socket_(std::make_shared>()) { +MockProdListenerComponentFactory::MockProdListenerComponentFactory( + NiceMock& server) + : ProdListenerComponentFactory(server), + socket_(std::make_shared>()) { ON_CALL(*this, createListenSocket(_, _, _, _, _, _)) - .WillByDefault(Invoke( - [&](Network::Address::InstanceConstSharedPtr, Network::Socket::Type, - const Network::Socket::OptionsSharedPtr& options, ListenerComponentFactory::BindType, - const Network::SocketCreationOptions&, uint32_t) -> Network::SocketSharedPtr { - if (!Network::Socket::applyOptions( - options, *socket_, envoy::config::core::v3::SocketOption::STATE_PREBIND)) { - throw EnvoyException("MockListenerComponentFactory: Setting socket options failed"); - } - return socket_; - })); + .WillByDefault(Invoke([&](Network::Address::InstanceConstSharedPtr, Network::Socket::Type, + const Network::Socket::OptionsSharedPtr& options, + ListenerComponentFactory::BindType, + const Network::SocketCreationOptions&, + uint32_t) -> Network::SocketSharedPtr { + if (!Network::Socket::applyOptions(options, *socket_, + envoy::config::core::v3::SocketOption::STATE_PREBIND)) { + throw EnvoyException("MockProdListenerComponentFactory: Setting socket options failed"); + } + return socket_; + })); } -MockListenerComponentFactory::~MockListenerComponentFactory() = default; +MockProdListenerComponentFactory::~MockProdListenerComponentFactory() = default; } // namespace Server } // namespace Envoy diff --git a/test/mocks/server/listener_component_factory.h b/test/mocks/server/listener_component_factory.h index 89c21114bad1..feff02bf12ab 100644 --- a/test/mocks/server/listener_component_factory.h +++ b/test/mocks/server/listener_component_factory.h @@ -4,26 +4,28 @@ #include "envoy/server/drain_manager.h" #include "envoy/server/listener_manager.h" +#include "source/server/listener_manager_impl.h" + #include "test/mocks/network/mocks.h" +#include "test/mocks/server/instance.h" #include "gmock/gmock.h" namespace Envoy { namespace Server { -class MockListenerComponentFactory : public ListenerComponentFactory { + +class MockProdListenerComponentFactory : public ProdListenerComponentFactory { public: - MockListenerComponentFactory(); - ~MockListenerComponentFactory() override; + MockProdListenerComponentFactory(NiceMock& server); + ~MockProdListenerComponentFactory(); - DrainManagerPtr - createDrainManager(envoy::config::listener::v3::Listener::DrainType drain_type) override { + DrainManagerPtr createDrainManager(envoy::config::listener::v3::Listener::DrainType drain_type) { return DrainManagerPtr{createDrainManager_(drain_type)}; } LdsApiPtr createLdsApi(const envoy::config::core::v3::ConfigSource& lds_config, - const xds::core::v3::ResourceLocator* lds_resources_locator) override { + const xds::core::v3::ResourceLocator* lds_resources_locator) { return LdsApiPtr{createLdsApi_(lds_config, lds_resources_locator)}; } - MOCK_METHOD(LdsApi*, createLdsApi_, (const envoy::config::core::v3::ConfigSource&, const xds::core::v3::ResourceLocator*)); @@ -38,7 +40,8 @@ class MockListenerComponentFactory : public ListenerComponentFactory { Configuration::ListenerFactoryContext& context)); MOCK_METHOD(Network::SocketSharedPtr, createListenSocket, (Network::Address::InstanceConstSharedPtr address, Network::Socket::Type socket_type, - const Network::Socket::OptionsSharedPtr& options, BindType bind_type, + const Network::Socket::OptionsSharedPtr& options, + ListenerComponentFactory::BindType bind_type, const Network::SocketCreationOptions& creation_options, uint32_t worker_index)); MOCK_METHOD(DrainManager*, createDrainManager_, (envoy::config::listener::v3::Listener::DrainType drain_type)); @@ -46,5 +49,6 @@ class MockListenerComponentFactory : public ListenerComponentFactory { std::shared_ptr socket_; }; + } // namespace Server } // namespace Envoy diff --git a/test/server/api_listener_test.cc b/test/server/api_listener_test.cc index 72351b09e21e..60bed4d9ccd8 100644 --- a/test/server/api_listener_test.cc +++ b/test/server/api_listener_test.cc @@ -22,11 +22,12 @@ namespace Server { class ApiListenerTest : public testing::Test { protected: ApiListenerTest() - : listener_manager_(std::make_unique( + : listener_factory_(server_), + listener_manager_(std::make_unique( server_, listener_factory_, worker_factory_, false, server_.quic_stat_names_)) {} NiceMock server_; - NiceMock listener_factory_; + NiceMock listener_factory_; NiceMock worker_factory_; std::unique_ptr listener_manager_; }; diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index e699d22dd16e..bd28ab268b87 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -101,7 +101,7 @@ class ListenerManagerImplWithRealFiltersTest : public ListenerManagerImplTest { EXPECT_EQ(1U, manager_->listeners().size()); } else { EXPECT_THROW_WITH_MESSAGE(addOrUpdateListener(listener), EnvoyException, - "MockListenerComponentFactory: Setting socket options failed"); + "MockProdListenerComponentFactory: Setting socket options failed"); EXPECT_EQ(0U, manager_->listeners().size()); } } @@ -4677,7 +4677,7 @@ TEST_P(ListenerManagerImplWithRealFiltersTest, Metadata) { -> Filter::ListenerFilterFactoriesList { listener_factory_context = &context; return ProdListenerComponentFactory::createListenerFilterFactoryListImpl( - filters, context, manager_->getTcpListenerConfigProviderManager()); + filters, context, listener_factory_.getTcpListenerConfigProviderManager()); })); server_.server_factory_context_->cluster_manager_.initializeClusters({"service_foo"}, {}); addOrUpdateListener(parseListenerFromV3Yaml(yaml)); diff --git a/test/server/listener_manager_impl_test.h b/test/server/listener_manager_impl_test.h index dbad4027f3a4..794b812735d1 100644 --- a/test/server/listener_manager_impl_test.h +++ b/test/server/listener_manager_impl_test.h @@ -65,7 +65,8 @@ class ListenerManagerImplTest : public testing::TestWithParam { protected: ListenerManagerImplTest() - : api_(Api::createApiForTest(server_.api_.random_)), use_matcher_(GetParam()) {} + : listener_factory_(server_), api_(Api::createApiForTest(server_.api_.random_)), + use_matcher_(GetParam()) {} void SetUp() override { ON_CALL(server_, api()).WillByDefault(ReturnRef(*api_)); @@ -94,7 +95,7 @@ class ListenerManagerImplTest : public testing::TestWithParam { Configuration::ListenerFactoryContext& context) -> Filter::ListenerFilterFactoriesList { return ProdListenerComponentFactory::createListenerFilterFactoryListImpl( - filters, context, manager_->getTcpListenerConfigProviderManager()); + filters, context, listener_factory_.getTcpListenerConfigProviderManager()); })); ON_CALL(listener_factory_, createUdpListenerFilterFactoryList(_, _)) .WillByDefault( @@ -347,7 +348,7 @@ class ListenerManagerImplTest : public testing::TestWithParam { std::shared_ptr internal_registry_{ std::make_shared()}; - NiceMock listener_factory_; + NiceMock listener_factory_; NiceMock validation_visitor; MockWorker* worker_ = new MockWorker(); NiceMock worker_factory_; From e59f26fada9507f7b312d760e1ed6b244c4d1543 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Thu, 12 May 2022 16:25:27 +0000 Subject: [PATCH 10/28] fixing server_test failure Signed-off-by: Yanjun Xiang --- source/server/config_validation/server.cc | 11 ++++++----- source/server/config_validation/server.h | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index f99d8242c444..2b68569d851d 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -42,9 +42,10 @@ ValidationInstance::ValidationInstance( const Network::Address::InstanceConstSharedPtr& local_address, Stats::IsolatedStoreImpl& store, Thread::BasicLockable& access_log_lock, ComponentFactory& component_factory, Thread::ThreadFactory& thread_factory, Filesystem::Instance& file_system) - : options_(options), validation_context_(options_.allowUnknownStaticFields(), - !options.rejectUnknownDynamicFields(), - !options.ignoreUnknownDynamicFields()), + : ProdListenerComponentFactory(*dynamic_cast(this)), options_(options), + validation_context_(options_.allowUnknownStaticFields(), + !options.rejectUnknownDynamicFields(), + !options.ignoreUnknownDynamicFields()), stats_store_(store), api_(new Api::ValidationImpl(thread_factory, store, time_system, file_system, random_generator_, bootstrap_)), @@ -59,8 +60,8 @@ ValidationInstance::ValidationInstance( TRY_ASSERT_MAIN_THREAD { initialize(options, local_address, component_factory); } END_TRY catch (const EnvoyException& e) { - ENVOY_LOG(critical, "error initializing configuration '{}': {}", options.configPath(), - e.what()); + ENVOY_LOG_MISC(critical, "error initializing configuration '{}': {}", options.configPath(), + e.what()); shutdown(); throw; } diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index a8617ee62b7b..ccff9348d7db 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -60,7 +60,7 @@ bool validateConfig(const Options& options, */ class ValidationInstance final : Logger::Loggable, public Instance, - public ListenerComponentFactory, + public ProdListenerComponentFactory, public ServerLifecycleNotifier, public WorkerFactory { public: From 7a8dc70359adbb5a002182aa2dbd7e29d63d82a3 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Thu, 12 May 2022 21:24:12 +0000 Subject: [PATCH 11/28] fixing clangTidy error Signed-off-by: Yanjun Xiang --- source/common/filter/config_discovery_impl.h | 2 +- .../listener_extension_discovery_integration_test.cc | 4 ++-- test/mocks/server/listener_component_factory.h | 7 ++++--- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/source/common/filter/config_discovery_impl.h b/source/common/filter/config_discovery_impl.h index ea7e1879cc4e..cfa4821f4299 100644 --- a/source/common/filter/config_discovery_impl.h +++ b/source/common/filter/config_discovery_impl.h @@ -466,7 +466,7 @@ class FilterConfigProviderManagerImpl : public FilterConfigProviderManagerImplBa const absl::flat_hash_set& require_type_urls) const { auto* default_factory = Config::Utility::getFactoryByType(proto_config); validateProtoConfigDefaultFactory(default_factory == nullptr, filter_config_name, - proto_config.type_url()); + std::string(proto_config.type_url())); validateProtoConfigTypeUrl(Config::Utility::getFactoryType(proto_config), require_type_urls); ProtobufTypes::MessagePtr message = Config::Utility::translateAnyToFactoryConfig( proto_config, factory_context.messageValidationVisitor(), *default_factory); diff --git a/test/integration/listener_extension_discovery_integration_test.cc b/test/integration/listener_extension_discovery_integration_test.cc index 97acf6be6db5..842099f0f27d 100644 --- a/test/integration/listener_extension_discovery_integration_test.cc +++ b/test/integration/listener_extension_discovery_integration_test.cc @@ -26,7 +26,7 @@ class ListenerExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegra stat_prefix: tcp_stats cluster: cluster_0 )EOF"), - default_drain_bytes_(2), filter_name_("foo"), data_("HelloWorld"), port_name_("http") {} + filter_name_("foo"), data_("HelloWorld"), port_name_("http") {} void addDynamicFilter(const std::string& name, bool apply_without_warming, bool set_default_config = true, bool rate_limit = false) { @@ -140,7 +140,7 @@ class ListenerExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegra tcp_client->close(); } - const uint32_t default_drain_bytes_; + const uint32_t default_drain_bytes_{2}; const std::string filter_name_; const std::string data_; const std::string port_name_; diff --git a/test/mocks/server/listener_component_factory.h b/test/mocks/server/listener_component_factory.h index feff02bf12ab..69996032c70b 100644 --- a/test/mocks/server/listener_component_factory.h +++ b/test/mocks/server/listener_component_factory.h @@ -17,13 +17,14 @@ namespace Server { class MockProdListenerComponentFactory : public ProdListenerComponentFactory { public: MockProdListenerComponentFactory(NiceMock& server); - ~MockProdListenerComponentFactory(); + ~MockProdListenerComponentFactory() override; - DrainManagerPtr createDrainManager(envoy::config::listener::v3::Listener::DrainType drain_type) { + DrainManagerPtr + createDrainManager(envoy::config::listener::v3::Listener::DrainType drain_type) override { return DrainManagerPtr{createDrainManager_(drain_type)}; } LdsApiPtr createLdsApi(const envoy::config::core::v3::ConfigSource& lds_config, - const xds::core::v3::ResourceLocator* lds_resources_locator) { + const xds::core::v3::ResourceLocator* lds_resources_locator) override { return LdsApiPtr{createLdsApi_(lds_config, lds_resources_locator)}; } MOCK_METHOD(LdsApi*, createLdsApi_, From 7e70c2d69dd604f5d2d22b3a06bc949c4fe2f28e Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Sat, 14 May 2022 00:07:45 +0000 Subject: [PATCH 12/28] fixing the dynamic_cast Signed-off-by: Yanjun Xiang --- envoy/server/listener_manager.h | 10 ++++++++++ source/server/config_validation/server.cc | 11 ++++++----- source/server/config_validation/server.h | 18 +++++++++++------- source/server/listener_impl.cc | 10 ++-------- source/server/listener_manager_impl.h | 9 ++++++++- test/config_test/config_test.cc | 6 +----- 6 files changed, 38 insertions(+), 26 deletions(-) diff --git a/envoy/server/listener_manager.h b/envoy/server/listener_manager.h index 281f10644bce..263138187464 100644 --- a/envoy/server/listener_manager.h +++ b/envoy/server/listener_manager.h @@ -116,6 +116,16 @@ class ListenerComponentFactory { * @return uint64_t a listener tag usable for connection handler tracking. */ virtual uint64_t nextListenerTag() PURE; + + /** + * Create static filter config provider using the tcp_listener_config_provider_manager_. + * @return Filter::FilterConfigProviderPtr a filter config provider pointer. + * @param callback supplies a callback object with type: ListenerFilterFactoryCb. + * @param filter_config_name a listener filter name. + */ + virtual Filter::FilterConfigProviderPtr + createStaticFilterConfigProvider(const Network::ListenerFilterFactoryCb& callback, + const std::string& filter_config_name) PURE; }; /** diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index 2b68569d851d..ed40f860a9a4 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -42,10 +42,9 @@ ValidationInstance::ValidationInstance( const Network::Address::InstanceConstSharedPtr& local_address, Stats::IsolatedStoreImpl& store, Thread::BasicLockable& access_log_lock, ComponentFactory& component_factory, Thread::ThreadFactory& thread_factory, Filesystem::Instance& file_system) - : ProdListenerComponentFactory(*dynamic_cast(this)), options_(options), - validation_context_(options_.allowUnknownStaticFields(), - !options.rejectUnknownDynamicFields(), - !options.ignoreUnknownDynamicFields()), + : options_(options), validation_context_(options_.allowUnknownStaticFields(), + !options.rejectUnknownDynamicFields(), + !options.ignoreUnknownDynamicFields()), stats_store_(store), api_(new Api::ValidationImpl(thread_factory, store, time_system, file_system, random_generator_, bootstrap_)), @@ -56,7 +55,9 @@ ValidationInstance::ValidationInstance( mutex_tracer_(nullptr), grpc_context_(stats_store_.symbolTable()), http_context_(stats_store_.symbolTable()), router_context_(stats_store_.symbolTable()), time_system_(time_system), server_contexts_(*this), - quic_stat_names_(stats_store_.symbolTable()) { + quic_stat_names_(stats_store_.symbolTable()), + tcp_listener_config_provider_manager_( + std::make_unique()) { TRY_ASSERT_MAIN_THREAD { initialize(options, local_address, component_factory); } END_TRY catch (const EnvoyException& e) { diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index ccff9348d7db..dd1704ef8f0a 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -60,7 +60,7 @@ bool validateConfig(const Options& options, */ class ValidationInstance final : Logger::Loggable, public Instance, - public ProdListenerComponentFactory, + public ListenerComponentFactory, public ServerLifecycleNotifier, public WorkerFactory { public: @@ -151,11 +151,8 @@ class ValidationInstance final : Logger::Loggable, Filter::ListenerFilterFactoriesList createListenerFilterFactoryList( const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) override { - ProdListenerComponentFactory* listener_component = - dynamic_cast(&listener_manager_->factory_); - auto& cfg_provider_manager = listener_component->getTcpListenerConfigProviderManager(); - return ProdListenerComponentFactory::createListenerFilterFactoryListImpl(filters, context, - cfg_provider_manager); + return ProdListenerComponentFactory::createListenerFilterFactoryListImpl( + filters, context, *tcp_listener_config_provider_manager_); } std::vector createUdpListenerFilterFactoryList( const Protobuf::RepeatedPtrField& filters, @@ -176,7 +173,12 @@ class ValidationInstance final : Logger::Loggable, return nullptr; } uint64_t nextListenerTag() override { return 0; } - + Filter::FilterConfigProviderPtr + createStaticFilterConfigProvider(const Network::ListenerFilterFactoryCb& callback, + const std::string& filter_config_name) override { + return tcp_listener_config_provider_manager_->createStaticFilterConfigProvider( + callback, filter_config_name); + } // Server::WorkerFactory WorkerPtr createWorker(uint32_t, OverloadManager&, const std::string&) override { // Returned workers are not currently used so we can return nothing here safely vs. a @@ -234,6 +236,8 @@ class ValidationInstance final : Logger::Loggable, Event::TimeSystem& time_system_; ServerFactoryContextImpl server_contexts_; Quic::QuicStatNames quic_stat_names_; + std::unique_ptr + tcp_listener_config_provider_manager_; }; } // namespace Server diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index a0be8663b374..b82c7577af10 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -703,10 +703,7 @@ void ListenerImpl::buildOriginalDstListenerFilter() { Network::ListenerFilterFactoryCb callback = factory.createListenerFilterFactoryFromProto( Envoy::ProtobufWkt::Empty(), nullptr, *listener_factory_context_); - ProdListenerComponentFactory* listener_component = - dynamic_cast(&parent_.factory_); - auto& cfg_provider_manager = listener_component->getTcpListenerConfigProviderManager(); - auto filter_config_provider = cfg_provider_manager.createStaticFilterConfigProvider( + auto filter_config_provider = parent_.factory_.createStaticFilterConfigProvider( callback, "envoy.filters.listener.original_dst"); listener_filter_factories_.push_back(std::move(filter_config_provider)); } @@ -725,10 +722,7 @@ void ListenerImpl::buildProxyProtocolListenerFilter() { Network::ListenerFilterFactoryCb callback = factory.createListenerFilterFactoryFromProto( envoy::extensions::filters::listener::proxy_protocol::v3::ProxyProtocol(), nullptr, *listener_factory_context_); - ProdListenerComponentFactory* listener_component = - dynamic_cast(&parent_.factory_); - auto& cfg_provider_manager = listener_component->getTcpListenerConfigProviderManager(); - auto filter_config_provider = cfg_provider_manager.createStaticFilterConfigProvider( + auto filter_config_provider = parent_.factory_.createStaticFilterConfigProvider( callback, "envoy.filters.listener.proxy_protocol"); listener_filter_factories_.push_back(std::move(filter_config_provider)); } diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index e8be1a57724e..ef306a57ef64 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -89,7 +89,7 @@ class ProdListenerComponentFactory : public ListenerComponentFactory, const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) override { return createListenerFilterFactoryListImpl(filters, context, - getTcpListenerConfigProviderManager()); + *tcp_listener_config_provider_manager_); } std::vector createUdpListenerFilterFactoryList( const Protobuf::RepeatedPtrField& filters, @@ -105,6 +105,13 @@ class ProdListenerComponentFactory : public ListenerComponentFactory, createDrainManager(envoy::config::listener::v3::Listener::DrainType drain_type) override; uint64_t nextListenerTag() override { return next_listener_tag_++; } + Filter::FilterConfigProviderPtr + createStaticFilterConfigProvider(const Network::ListenerFilterFactoryCb& callback, + const std::string& filter_config_name) override { + return tcp_listener_config_provider_manager_->createStaticFilterConfigProvider( + callback, filter_config_name); + } + Filter::TcpListenerFilterConfigProviderManagerImpl& getTcpListenerConfigProviderManager() { return *tcp_listener_config_provider_manager_; } diff --git a/test/config_test/config_test.cc b/test/config_test/config_test.cc index 079792b427e7..921f3ba6a6e5 100644 --- a/test/config_test/config_test.cc +++ b/test/config_test/config_test.cc @@ -117,12 +117,8 @@ class ConfigTest { filters, Server::Configuration::ListenerFactoryContext& context) -> Filter::ListenerFilterFactoriesList { - Server::ProdListenerComponentFactory* listener_component = - dynamic_cast(&listener_manager_.factory_); - auto& cfg_provider_manager = - listener_component->getTcpListenerConfigProviderManager(); return Server::ProdListenerComponentFactory::createListenerFilterFactoryListImpl( - filters, context, cfg_provider_manager); + filters, context, component_factory_.getTcpListenerConfigProviderManager()); })); ON_CALL(component_factory_, createUdpListenerFilterFactoryList(_, _)) .WillByDefault(Invoke( From c87f049d77b13c8aa5dec5da58042b881f7c472b Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Sat, 14 May 2022 01:56:57 +0000 Subject: [PATCH 13/28] merge upstream Signed-off-by: Yanjun Xiang --- test/integration/filters/test_listener_filter.proto | 6 ------ 1 file changed, 6 deletions(-) diff --git a/test/integration/filters/test_listener_filter.proto b/test/integration/filters/test_listener_filter.proto index 51b533d5a198..90ab6d653aa5 100644 --- a/test/integration/filters/test_listener_filter.proto +++ b/test/integration/filters/test_listener_filter.proto @@ -2,14 +2,8 @@ syntax = "proto3"; package test.integration.filters; - import "validate/validate.proto"; -// Configuration for TCP listener filter test -message TestTcpListenerFilterConfig { - uint32 drain_bytes = 1 [(validate.rules).uint32 = {gte: 2}]; -} - // Configuration for inspector listener filter message TestInspectorFilterConfig { } From 265b52dc5ea19f53526a47d7f763497f160caa91 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Sat, 14 May 2022 02:35:59 +0000 Subject: [PATCH 14/28] addressing comments Signed-off-by: Yanjun Xiang --- source/server/config_validation/server.cc | 4 ++-- source/server/listener_manager_impl.cc | 7 ++----- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index c9743ba8b041..232fbcaedb76 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -61,8 +61,8 @@ ValidationInstance::ValidationInstance( TRY_ASSERT_MAIN_THREAD { initialize(options, local_address, component_factory); } END_TRY catch (const EnvoyException& e) { - ENVOY_LOG_MISC(critical, "error initializing configuration '{}': {}", options.configPath(), - e.what()); + ENVOY_LOG(critical, "error initializing configuration '{}': {}", options.configPath(), + e.what()); shutdown(); throw; } diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 4d021c5740f2..c24ebcb40ff1 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -157,14 +157,11 @@ ProdListenerComponentFactory::createListenerFilterFactoryListImpl( proto_config); const auto message = Config::Utility::translateToFactoryConfig( proto_config, context.messageValidationVisitor(), factory); - - Network::ListenerFilterFactoryCb callback = factory.createListenerFilterFactoryFromProto( + const auto callback = factory.createListenerFilterFactoryFromProto( *message, createListenerFilterMatcher(proto_config), context); - auto filter_config_provider = config_provider_manager.createStaticFilterConfigProvider(callback, proto_config.name()); - - ENVOY_LOG(debug, " name: {}", filter_config_provider->name()); + ENVOY_LOG(debug, " filter config provider name: {}", filter_config_provider->name()); ret.push_back(std::move(filter_config_provider)); } } From 92eb4fe0cfd297a3ea708a935da9f95e5ffc7f32 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Tue, 17 May 2022 22:42:21 +0000 Subject: [PATCH 15/28] addressing comments Signed-off-by: Yanjun Xiang --- source/common/filter/config_discovery_impl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/filter/config_discovery_impl.h b/source/common/filter/config_discovery_impl.h index cfc81f60a785..dc4af4e80d8e 100644 --- a/source/common/filter/config_discovery_impl.h +++ b/source/common/filter/config_discovery_impl.h @@ -466,7 +466,7 @@ class FilterConfigProviderManagerImpl : public FilterConfigProviderManagerImplBa const absl::flat_hash_set& require_type_urls) const { auto* default_factory = Config::Utility::getFactoryByType(proto_config); validateProtoConfigDefaultFactory(default_factory == nullptr, filter_config_name, - std::string(proto_config.type_url())); + proto_config.type_url()); validateProtoConfigTypeUrl(Config::Utility::getFactoryType(proto_config), require_type_urls); ProtobufTypes::MessagePtr message = Config::Utility::translateAnyToFactoryConfig( proto_config, factory_context.messageValidationVisitor(), *default_factory); From ade2ea8bba5c3b81db4dc0bd97d942c0913c9276 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Wed, 18 May 2022 18:57:41 +0000 Subject: [PATCH 16/28] addressing comments Signed-off-by: Yanjun Xiang --- envoy/server/listener_manager.h | 2 +- source/server/config_validation/server.cc | 4 +-- source/server/config_validation/server.h | 7 ++--- source/server/listener_manager_impl.h | 15 ++++------ .../filters/test_listener_filter.cc | 2 +- ...er_extension_discovery_integration_test.cc | 30 +++++++++++-------- 6 files changed, 28 insertions(+), 32 deletions(-) diff --git a/envoy/server/listener_manager.h b/envoy/server/listener_manager.h index 263138187464..85e72f9044a3 100644 --- a/envoy/server/listener_manager.h +++ b/envoy/server/listener_manager.h @@ -89,7 +89,7 @@ class ListenerComponentFactory { * Creates a list of listener filter factories. * @param filters supplies the JSON configuration. * @param context supplies the factory creation context. - * @return ListenerFilterFactoriesList the list of filter factories. + * @return Filter::ListenerFilterFactoriesList the list of filter factories. */ virtual Filter::ListenerFilterFactoriesList createListenerFilterFactoryList( const Protobuf::RepeatedPtrField& filters, diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index 232fbcaedb76..94f0413999b6 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -55,9 +55,7 @@ ValidationInstance::ValidationInstance( mutex_tracer_(nullptr), grpc_context_(stats_store_.symbolTable()), http_context_(stats_store_.symbolTable()), router_context_(stats_store_.symbolTable()), time_system_(time_system), server_contexts_(*this), - quic_stat_names_(stats_store_.symbolTable()), - tcp_listener_config_provider_manager_( - std::make_unique()) { + quic_stat_names_(stats_store_.symbolTable()) { TRY_ASSERT_MAIN_THREAD { initialize(options, local_address, component_factory); } END_TRY catch (const EnvoyException& e) { diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index dd1704ef8f0a..a58efcc7cf33 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -152,7 +152,7 @@ class ValidationInstance final : Logger::Loggable, const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) override { return ProdListenerComponentFactory::createListenerFilterFactoryListImpl( - filters, context, *tcp_listener_config_provider_manager_); + filters, context, tcp_listener_config_provider_manager_); } std::vector createUdpListenerFilterFactoryList( const Protobuf::RepeatedPtrField& filters, @@ -176,7 +176,7 @@ class ValidationInstance final : Logger::Loggable, Filter::FilterConfigProviderPtr createStaticFilterConfigProvider(const Network::ListenerFilterFactoryCb& callback, const std::string& filter_config_name) override { - return tcp_listener_config_provider_manager_->createStaticFilterConfigProvider( + return tcp_listener_config_provider_manager_.createStaticFilterConfigProvider( callback, filter_config_name); } // Server::WorkerFactory @@ -236,8 +236,7 @@ class ValidationInstance final : Logger::Loggable, Event::TimeSystem& time_system_; ServerFactoryContextImpl server_contexts_; Quic::QuicStatNames quic_stat_names_; - std::unique_ptr - tcp_listener_config_provider_manager_; + Filter::TcpListenerFilterConfigProviderManagerImpl tcp_listener_config_provider_manager_; }; } // namespace Server diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index ef306a57ef64..2a0238bcfb2e 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -42,11 +42,7 @@ class ListenerFilterChainFactoryBuilder; class ProdListenerComponentFactory : public ListenerComponentFactory, Logger::Loggable { public: - ProdListenerComponentFactory(Instance& server) - : server_(server), - tcp_listener_config_provider_manager_( - std::make_unique()) {} - + ProdListenerComponentFactory(Instance& server) : server_(server) {} /** * Static worker for createNetworkFilterFactoryList() that can be used directly in tests. */ @@ -89,7 +85,7 @@ class ProdListenerComponentFactory : public ListenerComponentFactory, const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) override { return createListenerFilterFactoryListImpl(filters, context, - *tcp_listener_config_provider_manager_); + tcp_listener_config_provider_manager_); } std::vector createUdpListenerFilterFactoryList( const Protobuf::RepeatedPtrField& filters, @@ -108,19 +104,18 @@ class ProdListenerComponentFactory : public ListenerComponentFactory, Filter::FilterConfigProviderPtr createStaticFilterConfigProvider(const Network::ListenerFilterFactoryCb& callback, const std::string& filter_config_name) override { - return tcp_listener_config_provider_manager_->createStaticFilterConfigProvider( + return tcp_listener_config_provider_manager_.createStaticFilterConfigProvider( callback, filter_config_name); } Filter::TcpListenerFilterConfigProviderManagerImpl& getTcpListenerConfigProviderManager() { - return *tcp_listener_config_provider_manager_; + return tcp_listener_config_provider_manager_; } private: Instance& server_; uint64_t next_listener_tag_{1}; - std::unique_ptr - tcp_listener_config_provider_manager_; + Filter::TcpListenerFilterConfigProviderManagerImpl tcp_listener_config_provider_manager_; }; class ListenerImpl; diff --git a/test/integration/filters/test_listener_filter.cc b/test/integration/filters/test_listener_filter.cc index b13ff99f809b..4bb3c591f36c 100644 --- a/test/integration/filters/test_listener_filter.cc +++ b/test/integration/filters/test_listener_filter.cc @@ -53,7 +53,7 @@ class TestTcpInspectorConfigFactory } ProtobufTypes::MessagePtr createEmptyConfigProto() override { - return ProtobufTypes::MessagePtr{new test::integration::filters::TestTcpListenerFilterConfig()}; + return std::make_unique(); } std::string name() const override { return "envoy.filters.tcp_listener.test"; } diff --git a/test/integration/listener_extension_discovery_integration_test.cc b/test/integration/listener_extension_discovery_integration_test.cc index 842099f0f27d..545996e1c5f4 100644 --- a/test/integration/listener_extension_discovery_integration_test.cc +++ b/test/integration/listener_extension_discovery_integration_test.cc @@ -17,16 +17,8 @@ class ListenerExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegra public BaseIntegrationTest { public: ListenerExtensionDiscoveryIntegrationTest() - : BaseIntegrationTest(ipVersion(), ConfigHelper::baseConfig() + R"EOF( - filter_chains: - - filters: - - name: envoy.filters.network.tcp_proxy - typed_config: - "@type": type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy - stat_prefix: tcp_stats - cluster: cluster_0 -)EOF"), - filter_name_("foo"), data_("HelloWorld"), port_name_("http") {} + : BaseIntegrationTest(ipVersion(), ConfigHelper::baseConfig()), filter_name_("foo"), + data_("HelloWorld"), port_name_("http") {} void addDynamicFilter(const std::string& name, bool apply_without_warming, bool set_default_config = true, bool rate_limit = false) { @@ -63,6 +55,18 @@ class ListenerExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegra defer_listener_finalization_ = true; setUpstreamCount(1); + // Add a tcp_proxy network filter. + config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0); + auto* filter_chain = listener->add_filter_chains(); + auto* filter = filter_chain->add_filters(); + filter->set_name("envoy.filters.network.tcp_proxy"); + envoy::extensions::filters::network::tcp_proxy::v3::TcpProxy config; + config.set_stat_prefix("tcp_stats"); + config.set_cluster("cluster_0"); + filter->mutable_typed_config()->PackFrom(config); + }); + // Add an xDS cluster for extension config discovery. config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { auto* ecds_cluster = bootstrap.mutable_static_resources()->add_clusters(); @@ -94,9 +98,9 @@ class ListenerExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegra // Wait for ECDS stream. auto& ecds_upstream = getEcdsFakeUpstream(); AssertionResult result = ecds_upstream.waitForHttpConnection(*dispatcher_, ecds_connection_); - RELEASE_ASSERT(result, result.message()); + ASSERT_TRUE(result); result = ecds_connection_->waitForNewStream(*dispatcher_, ecds_stream_); - RELEASE_ASSERT(result, result.message()); + ASSERT_TRUE(result); ecds_stream_->startGrpcStream(); } @@ -309,7 +313,7 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, DestroyDuringInit) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); test_server_.reset(); auto result = ecds_connection_->waitForDisconnect(); - RELEASE_ASSERT(result, result.message()); + ASSERT_TRUE(result); ecds_connection_.reset(); } From d5570d8d1d55156a5ce6b7023e3d253259af874e Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Thu, 19 May 2022 23:37:23 +0000 Subject: [PATCH 17/28] removing debug logs Signed-off-by: Yanjun Xiang --- source/server/listener_manager_impl.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index c24ebcb40ff1..ab61978bc8cb 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -129,7 +129,6 @@ ProdListenerComponentFactory::createListenerFilterFactoryListImpl( envoy::config::listener::v3::ListenerFilter::ConfigTypeCase::kConfigDiscovery) { const auto& config_discovery = proto_config.config_discovery(); const auto& name = proto_config.name(); - ENVOY_LOG(debug, " Listener filter: dynamic filter name: {}", name); if (config_discovery.apply_default_config_without_warming() && !config_discovery.has_default_config()) { throw EnvoyException(fmt::format( @@ -161,7 +160,6 @@ ProdListenerComponentFactory::createListenerFilterFactoryListImpl( *message, createListenerFilterMatcher(proto_config), context); auto filter_config_provider = config_provider_manager.createStaticFilterConfigProvider(callback, proto_config.name()); - ENVOY_LOG(debug, " filter config provider name: {}", filter_config_provider->name()); ret.push_back(std::move(filter_config_provider)); } } From c2d39dd0821314812f00e719a4aed2f8f5266115 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Sat, 21 May 2022 01:42:48 +0000 Subject: [PATCH 18/28] mock interface Signed-off-by: Yanjun Xiang --- envoy/server/listener_manager.h | 16 +++++----- source/server/config_validation/server.cc | 4 ++- source/server/config_validation/server.h | 14 ++++----- source/server/listener_impl.cc | 6 ++-- source/server/listener_manager_impl.h | 21 ++++++-------- test/config_test/config_test.cc | 12 ++++++-- .../server/listener_component_factory.cc | 29 +++++++++---------- .../mocks/server/listener_component_factory.h | 17 +++++------ test/server/listener_manager_impl_test.cc | 4 +-- test/server/listener_manager_impl_test.h | 13 ++++++--- 10 files changed, 72 insertions(+), 64 deletions(-) diff --git a/envoy/server/listener_manager.h b/envoy/server/listener_manager.h index 85e72f9044a3..07a65765ef48 100644 --- a/envoy/server/listener_manager.h +++ b/envoy/server/listener_manager.h @@ -19,6 +19,11 @@ #include "source/common/protobuf/protobuf.h" namespace Envoy { +namespace Filter { + +class TcpListenerFilterConfigProviderManagerImpl; +} // namespace Filter + namespace Server { /** @@ -118,14 +123,11 @@ class ListenerComponentFactory { virtual uint64_t nextListenerTag() PURE; /** - * Create static filter config provider using the tcp_listener_config_provider_manager_. - * @return Filter::FilterConfigProviderPtr a filter config provider pointer. - * @param callback supplies a callback object with type: ListenerFilterFactoryCb. - * @param filter_config_name a listener filter name. + * @return std::unique_ptr the TCP listener + * config provider manager. */ - virtual Filter::FilterConfigProviderPtr - createStaticFilterConfigProvider(const Network::ListenerFilterFactoryCb& callback, - const std::string& filter_config_name) PURE; + virtual const std::unique_ptr& + getTcpListenerConfigProviderManager() PURE; }; /** diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index 94f0413999b6..232fbcaedb76 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -55,7 +55,9 @@ ValidationInstance::ValidationInstance( mutex_tracer_(nullptr), grpc_context_(stats_store_.symbolTable()), http_context_(stats_store_.symbolTable()), router_context_(stats_store_.symbolTable()), time_system_(time_system), server_contexts_(*this), - quic_stat_names_(stats_store_.symbolTable()) { + quic_stat_names_(stats_store_.symbolTable()), + tcp_listener_config_provider_manager_( + std::make_unique()) { TRY_ASSERT_MAIN_THREAD { initialize(options, local_address, component_factory); } END_TRY catch (const EnvoyException& e) { diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index a58efcc7cf33..984808a2a3fa 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -152,7 +152,7 @@ class ValidationInstance final : Logger::Loggable, const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) override { return ProdListenerComponentFactory::createListenerFilterFactoryListImpl( - filters, context, tcp_listener_config_provider_manager_); + filters, context, *tcp_listener_config_provider_manager_); } std::vector createUdpListenerFilterFactoryList( const Protobuf::RepeatedPtrField& filters, @@ -173,12 +173,11 @@ class ValidationInstance final : Logger::Loggable, return nullptr; } uint64_t nextListenerTag() override { return 0; } - Filter::FilterConfigProviderPtr - createStaticFilterConfigProvider(const Network::ListenerFilterFactoryCb& callback, - const std::string& filter_config_name) override { - return tcp_listener_config_provider_manager_.createStaticFilterConfigProvider( - callback, filter_config_name); + const std::unique_ptr& + getTcpListenerConfigProviderManager() override { + return tcp_listener_config_provider_manager_; } + // Server::WorkerFactory WorkerPtr createWorker(uint32_t, OverloadManager&, const std::string&) override { // Returned workers are not currently used so we can return nothing here safely vs. a @@ -236,7 +235,8 @@ class ValidationInstance final : Logger::Loggable, Event::TimeSystem& time_system_; ServerFactoryContextImpl server_contexts_; Quic::QuicStatNames quic_stat_names_; - Filter::TcpListenerFilterConfigProviderManagerImpl tcp_listener_config_provider_manager_; + const std::unique_ptr + tcp_listener_config_provider_manager_; }; } // namespace Server diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index b82c7577af10..d54d0cea89d2 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -703,7 +703,8 @@ void ListenerImpl::buildOriginalDstListenerFilter() { Network::ListenerFilterFactoryCb callback = factory.createListenerFilterFactoryFromProto( Envoy::ProtobufWkt::Empty(), nullptr, *listener_factory_context_); - auto filter_config_provider = parent_.factory_.createStaticFilterConfigProvider( + const auto& cfg_provider_manager = parent_.factory_.getTcpListenerConfigProviderManager(); + auto filter_config_provider = cfg_provider_manager->createStaticFilterConfigProvider( callback, "envoy.filters.listener.original_dst"); listener_filter_factories_.push_back(std::move(filter_config_provider)); } @@ -722,7 +723,8 @@ void ListenerImpl::buildProxyProtocolListenerFilter() { Network::ListenerFilterFactoryCb callback = factory.createListenerFilterFactoryFromProto( envoy::extensions::filters::listener::proxy_protocol::v3::ProxyProtocol(), nullptr, *listener_factory_context_); - auto filter_config_provider = parent_.factory_.createStaticFilterConfigProvider( + const auto& cfg_provider_manager = parent_.factory_.getTcpListenerConfigProviderManager(); + auto filter_config_provider = cfg_provider_manager->createStaticFilterConfigProvider( callback, "envoy.filters.listener.proxy_protocol"); listener_filter_factories_.push_back(std::move(filter_config_provider)); } diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index 2a0238bcfb2e..155cae6e1ec3 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -42,7 +42,10 @@ class ListenerFilterChainFactoryBuilder; class ProdListenerComponentFactory : public ListenerComponentFactory, Logger::Loggable { public: - ProdListenerComponentFactory(Instance& server) : server_(server) {} + ProdListenerComponentFactory(Instance& server) + : server_(server), + tcp_listener_config_provider_manager_( + std::make_unique()) {} /** * Static worker for createNetworkFilterFactoryList() that can be used directly in tests. */ @@ -85,7 +88,7 @@ class ProdListenerComponentFactory : public ListenerComponentFactory, const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) override { return createListenerFilterFactoryListImpl(filters, context, - tcp_listener_config_provider_manager_); + *tcp_listener_config_provider_manager_); } std::vector createUdpListenerFilterFactoryList( const Protobuf::RepeatedPtrField& filters, @@ -100,22 +103,16 @@ class ProdListenerComponentFactory : public ListenerComponentFactory, DrainManagerPtr createDrainManager(envoy::config::listener::v3::Listener::DrainType drain_type) override; uint64_t nextListenerTag() override { return next_listener_tag_++; } - - Filter::FilterConfigProviderPtr - createStaticFilterConfigProvider(const Network::ListenerFilterFactoryCb& callback, - const std::string& filter_config_name) override { - return tcp_listener_config_provider_manager_.createStaticFilterConfigProvider( - callback, filter_config_name); - } - - Filter::TcpListenerFilterConfigProviderManagerImpl& getTcpListenerConfigProviderManager() { + const std::unique_ptr& + getTcpListenerConfigProviderManager() override { return tcp_listener_config_provider_manager_; } private: Instance& server_; uint64_t next_listener_tag_{1}; - Filter::TcpListenerFilterConfigProviderManagerImpl tcp_listener_config_provider_manager_; + const std::unique_ptr + tcp_listener_config_provider_manager_; }; class ListenerImpl; diff --git a/test/config_test/config_test.cc b/test/config_test/config_test.cc index 921f3ba6a6e5..026f5d009870 100644 --- a/test/config_test/config_test.cc +++ b/test/config_test/config_test.cc @@ -56,7 +56,9 @@ static std::vector unsuported_win32_configs = { class ConfigTest { public: ConfigTest(const OptionsImpl& options) - : api_(Api::createApiForTest(time_system_)), options_(options), component_factory_(server_) { + : api_(Api::createApiForTest(time_system_)), options_(options), + tcp_listener_config_provider_manager_( + std::make_unique()) { ON_CALL(server_, options()).WillByDefault(ReturnRef(options_)); ON_CALL(server_, sslContextManager()).WillByDefault(ReturnRef(ssl_context_manager_)); ON_CALL(server_.api_, fileSystem()).WillByDefault(ReturnRef(file_system_)); @@ -111,6 +113,8 @@ class ConfigTest { return Server::ProdListenerComponentFactory::createNetworkFilterFactoryListImpl( filters, context); })); + ON_CALL(component_factory_, getTcpListenerConfigProviderManager()) + .WillByDefault(ReturnRef(tcp_listener_config_provider_manager_)); ON_CALL(component_factory_, createListenerFilterFactoryList(_, _)) .WillByDefault(Invoke( [&](const Protobuf::RepeatedPtrField& @@ -118,7 +122,7 @@ class ConfigTest { Server::Configuration::ListenerFactoryContext& context) -> Filter::ListenerFilterFactoriesList { return Server::ProdListenerComponentFactory::createListenerFilterFactoryListImpl( - filters, context, component_factory_.getTcpListenerConfigProviderManager()); + filters, context, *component_factory_.getTcpListenerConfigProviderManager()); })); ON_CALL(component_factory_, createUdpListenerFilterFactoryList(_, _)) .WillByDefault(Invoke( @@ -148,7 +152,7 @@ class ConfigTest { NiceMock ssl_context_manager_; OptionsImpl options_; std::unique_ptr cluster_manager_factory_; - NiceMock component_factory_; + NiceMock component_factory_; NiceMock worker_factory_; Server::ListenerManagerImpl listener_manager_{server_, component_factory_, worker_factory_, false, server_.quic_stat_names_}; @@ -158,6 +162,8 @@ class ConfigTest { NiceMock os_sys_calls_; TestThreadsafeSingletonInjector os_calls{&os_sys_calls_}; NiceMock file_system_; + const std::unique_ptr + tcp_listener_config_provider_manager_; }; void testMerge() { diff --git a/test/mocks/server/listener_component_factory.cc b/test/mocks/server/listener_component_factory.cc index 04f15ca714a7..64eabf2a522c 100644 --- a/test/mocks/server/listener_component_factory.cc +++ b/test/mocks/server/listener_component_factory.cc @@ -11,25 +11,22 @@ namespace Server { using ::testing::_; using ::testing::Invoke; -MockProdListenerComponentFactory::MockProdListenerComponentFactory( - NiceMock& server) - : ProdListenerComponentFactory(server), - socket_(std::make_shared>()) { +MockListenerComponentFactory::MockListenerComponentFactory() + : socket_(std::make_shared>()) { ON_CALL(*this, createListenSocket(_, _, _, _, _, _)) - .WillByDefault(Invoke([&](Network::Address::InstanceConstSharedPtr, Network::Socket::Type, - const Network::Socket::OptionsSharedPtr& options, - ListenerComponentFactory::BindType, - const Network::SocketCreationOptions&, - uint32_t) -> Network::SocketSharedPtr { - if (!Network::Socket::applyOptions(options, *socket_, - envoy::config::core::v3::SocketOption::STATE_PREBIND)) { - throw EnvoyException("MockProdListenerComponentFactory: Setting socket options failed"); - } - return socket_; - })); + .WillByDefault(Invoke( + [&](Network::Address::InstanceConstSharedPtr, Network::Socket::Type, + const Network::Socket::OptionsSharedPtr& options, ListenerComponentFactory::BindType, + const Network::SocketCreationOptions&, uint32_t) -> Network::SocketSharedPtr { + if (!Network::Socket::applyOptions( + options, *socket_, envoy::config::core::v3::SocketOption::STATE_PREBIND)) { + throw EnvoyException("MockListenerComponentFactory: Setting socket options failed"); + } + return socket_; + })); } -MockProdListenerComponentFactory::~MockProdListenerComponentFactory() = default; +MockListenerComponentFactory::~MockListenerComponentFactory() = default; } // namespace Server } // namespace Envoy diff --git a/test/mocks/server/listener_component_factory.h b/test/mocks/server/listener_component_factory.h index 69996032c70b..180218182344 100644 --- a/test/mocks/server/listener_component_factory.h +++ b/test/mocks/server/listener_component_factory.h @@ -4,20 +4,16 @@ #include "envoy/server/drain_manager.h" #include "envoy/server/listener_manager.h" -#include "source/server/listener_manager_impl.h" - #include "test/mocks/network/mocks.h" -#include "test/mocks/server/instance.h" #include "gmock/gmock.h" namespace Envoy { namespace Server { - -class MockProdListenerComponentFactory : public ProdListenerComponentFactory { +class MockListenerComponentFactory : public ListenerComponentFactory { public: - MockProdListenerComponentFactory(NiceMock& server); - ~MockProdListenerComponentFactory() override; + MockListenerComponentFactory(); + ~MockListenerComponentFactory() override; DrainManagerPtr createDrainManager(envoy::config::listener::v3::Listener::DrainType drain_type) override { @@ -27,6 +23,7 @@ class MockProdListenerComponentFactory : public ProdListenerComponentFactory { const xds::core::v3::ResourceLocator* lds_resources_locator) override { return LdsApiPtr{createLdsApi_(lds_config, lds_resources_locator)}; } + MOCK_METHOD(LdsApi*, createLdsApi_, (const envoy::config::core::v3::ConfigSource&, const xds::core::v3::ResourceLocator*)); @@ -41,15 +38,15 @@ class MockProdListenerComponentFactory : public ProdListenerComponentFactory { Configuration::ListenerFactoryContext& context)); MOCK_METHOD(Network::SocketSharedPtr, createListenSocket, (Network::Address::InstanceConstSharedPtr address, Network::Socket::Type socket_type, - const Network::Socket::OptionsSharedPtr& options, - ListenerComponentFactory::BindType bind_type, + const Network::Socket::OptionsSharedPtr& options, BindType bind_type, const Network::SocketCreationOptions& creation_options, uint32_t worker_index)); MOCK_METHOD(DrainManager*, createDrainManager_, (envoy::config::listener::v3::Listener::DrainType drain_type)); MOCK_METHOD(uint64_t, nextListenerTag, ()); + MOCK_METHOD(const std::unique_ptr&, + getTcpListenerConfigProviderManager, ()); std::shared_ptr socket_; }; - } // namespace Server } // namespace Envoy diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index 8ed497c5eadc..c88aab06d09d 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -101,7 +101,7 @@ class ListenerManagerImplWithRealFiltersTest : public ListenerManagerImplTest { EXPECT_EQ(1U, manager_->listeners().size()); } else { EXPECT_THROW_WITH_MESSAGE(addOrUpdateListener(listener), EnvoyException, - "MockProdListenerComponentFactory: Setting socket options failed"); + "MockListenerComponentFactory: Setting socket options failed"); EXPECT_EQ(0U, manager_->listeners().size()); } } @@ -4677,7 +4677,7 @@ TEST_P(ListenerManagerImplWithRealFiltersTest, Metadata) { -> Filter::ListenerFilterFactoriesList { listener_factory_context = &context; return ProdListenerComponentFactory::createListenerFilterFactoryListImpl( - filters, context, listener_factory_.getTcpListenerConfigProviderManager()); + filters, context, *listener_factory_.getTcpListenerConfigProviderManager()); })); server_.server_factory_context_->cluster_manager_.initializeClusters({"service_foo"}, {}); addOrUpdateListener(parseListenerFromV3Yaml(yaml)); diff --git a/test/server/listener_manager_impl_test.h b/test/server/listener_manager_impl_test.h index 794b812735d1..fa5bce2421c0 100644 --- a/test/server/listener_manager_impl_test.h +++ b/test/server/listener_manager_impl_test.h @@ -65,8 +65,9 @@ class ListenerManagerImplTest : public testing::TestWithParam { protected: ListenerManagerImplTest() - : listener_factory_(server_), api_(Api::createApiForTest(server_.api_.random_)), - use_matcher_(GetParam()) {} + : api_(Api::createApiForTest(server_.api_.random_)), use_matcher_(GetParam()), + tcp_listener_config_provider_manager_( + std::make_unique()) {} void SetUp() override { ON_CALL(server_, api()).WillByDefault(ReturnRef(*api_)); @@ -88,6 +89,8 @@ class ListenerManagerImplTest : public testing::TestWithParam { return ProdListenerComponentFactory::createNetworkFilterFactoryListImpl( filters, filter_chain_factory_context); })); + ON_CALL(listener_factory_, getTcpListenerConfigProviderManager()) + .WillByDefault(ReturnRef(tcp_listener_config_provider_manager_)); ON_CALL(listener_factory_, createListenerFilterFactoryList(_, _)) .WillByDefault(Invoke( [this](const Protobuf::RepeatedPtrField& @@ -95,7 +98,7 @@ class ListenerManagerImplTest : public testing::TestWithParam { Configuration::ListenerFactoryContext& context) -> Filter::ListenerFilterFactoriesList { return ProdListenerComponentFactory::createListenerFilterFactoryListImpl( - filters, context, listener_factory_.getTcpListenerConfigProviderManager()); + filters, context, *listener_factory_.getTcpListenerConfigProviderManager()); })); ON_CALL(listener_factory_, createUdpListenerFilterFactoryList(_, _)) .WillByDefault( @@ -348,7 +351,7 @@ class ListenerManagerImplTest : public testing::TestWithParam { std::shared_ptr internal_registry_{ std::make_shared()}; - NiceMock listener_factory_; + NiceMock listener_factory_; NiceMock validation_visitor; MockWorker* worker_ = new MockWorker(); NiceMock worker_factory_; @@ -365,6 +368,8 @@ class ListenerManagerImplTest : public testing::TestWithParam { NiceMock> callback_; // Test parameter indicating whether the unified filter chain matcher is enabled. bool use_matcher_; + const std::unique_ptr + tcp_listener_config_provider_manager_; }; } // namespace Server } // namespace Envoy From fa94ee1c0f31ba5e73aa68b862ee6a4e7a827e66 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Sat, 21 May 2022 02:00:17 +0000 Subject: [PATCH 19/28] revert api_listener_test.cc change Signed-off-by: Yanjun Xiang --- test/server/api_listener_test.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/server/api_listener_test.cc b/test/server/api_listener_test.cc index 60bed4d9ccd8..72351b09e21e 100644 --- a/test/server/api_listener_test.cc +++ b/test/server/api_listener_test.cc @@ -22,12 +22,11 @@ namespace Server { class ApiListenerTest : public testing::Test { protected: ApiListenerTest() - : listener_factory_(server_), - listener_manager_(std::make_unique( + : listener_manager_(std::make_unique( server_, listener_factory_, worker_factory_, false, server_.quic_stat_names_)) {} NiceMock server_; - NiceMock listener_factory_; + NiceMock listener_factory_; NiceMock worker_factory_; std::unique_ptr listener_manager_; }; From 60d9a0c601ea3a996e09a6abd22a83e9a53eebf3 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Sat, 21 May 2022 02:06:01 +0000 Subject: [PATCH 20/28] revert change in test/mocks/server/BUILD Signed-off-by: Yanjun Xiang --- test/mocks/server/BUILD | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/mocks/server/BUILD b/test/mocks/server/BUILD index 54431039e523..28311cf68986 100644 --- a/test/mocks/server/BUILD +++ b/test/mocks/server/BUILD @@ -121,10 +121,8 @@ envoy_cc_mock( deps = [ "//envoy/server:drain_manager_interface", "//envoy/server:listener_manager_interface", - "//source/server:listener_manager_lib", "//test/mocks/network:io_handle_mocks", "//test/mocks/network:network_mocks", - "//test/mocks/server:instance_mocks", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/config/listener/v3:pkg_cc_proto", ], From dc5ed5c412779f9a11053f587eb487ba83c23e52 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Sat, 21 May 2022 03:22:58 +0000 Subject: [PATCH 21/28] adding more integration tests Signed-off-by: Yanjun Xiang --- ...er_extension_discovery_integration_test.cc | 97 +++++++++++++++---- 1 file changed, 79 insertions(+), 18 deletions(-) diff --git a/test/integration/listener_extension_discovery_integration_test.cc b/test/integration/listener_extension_discovery_integration_test.cc index 545996e1c5f4..a276ed6ad16c 100644 --- a/test/integration/listener_extension_discovery_integration_test.cc +++ b/test/integration/listener_extension_discovery_integration_test.cc @@ -51,6 +51,18 @@ class ListenerExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegra }); } + void addStaticFilter(const std::string& name, uint32_t drain_bytes) { + config_helper_.addConfigModifier( + [name, drain_bytes](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + auto* listener_filter = + bootstrap.mutable_static_resources()->mutable_listeners(0)->add_listener_filters(); + listener_filter->set_name(name); + auto configuration = test::integration::filters::TestTcpListenerFilterConfig(); + configuration.set_drain_bytes(drain_bytes); + listener_filter->mutable_typed_config()->PackFrom(configuration); + }); + } + void initialize() override { defer_listener_finalization_ = true; setUpstreamCount(1); @@ -104,7 +116,8 @@ class ListenerExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegra ecds_stream_->startGrpcStream(); } - void sendXdsResponse(const std::string& version, const uint32_t drain_bytes, bool ttl = false) { + void sendXdsResponse(const std::string& name, const std::string& version, + const uint32_t drain_bytes, bool ttl = false) { // The to-be-drained bytes has to be smaller than data size. ASSERT(drain_bytes <= data_.size()); @@ -112,9 +125,9 @@ class ListenerExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegra response.set_version_info(version); response.set_type_url("type.googleapis.com/envoy.config.core.v3.TypedExtensionConfig"); envoy::config::core::v3::TypedExtensionConfig typed_config; - typed_config.set_name(filter_name_); + typed_config.set_name(name); envoy::service::discovery::v3::Resource resource; - resource.set_name(filter_name_); + resource.set_name(name); auto configuration = test::integration::filters::TestTcpListenerFilterConfig(); configuration.set_drain_bytes(drain_bytes); @@ -131,6 +144,7 @@ class ListenerExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegra // upstream. void sendDataVerifyResults(uint32_t drain_bytes) { test_server_->waitUntilListenersReady(); + test_server_->waitForGaugeGe("listener_manager.workers_started", 1); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort(port_name_)); @@ -167,13 +181,13 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicSuccess) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); // Send 1st config update to have listener filter drain 5 bytes of data. - sendXdsResponse("1", 5); + sendXdsResponse(filter_name_, "1", 5); test_server_->waitForCounterGe( "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 1); sendDataVerifyResults(5); // Send 2nd config update to have listener filter drain 3 bytes of data. - sendXdsResponse("2", 3); + sendXdsResponse(filter_name_, "2", 3); test_server_->waitForCounterGe( "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 2); sendDataVerifyResults(3); @@ -187,7 +201,7 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicSuccessWithTtl) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); // Send 1st config update with TTL 1s, and have listener filter drain 5 bytes of data. - sendXdsResponse("1", 5, true); + sendXdsResponse(filter_name_, "1", 5, true); test_server_->waitForCounterGe( "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 1); sendDataVerifyResults(5); @@ -213,7 +227,7 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicSuccessWithTtlWithDefault EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); // Send 1st config update with TTL 1s, and have listener filter drain 5 bytes of data. - sendXdsResponse("1", 5, true); + sendXdsResponse(filter_name_, "1", 5, true); test_server_->waitForCounterGe( "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 1); sendDataVerifyResults(5); @@ -225,7 +239,6 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicSuccessWithTtlWithDefault sendDataVerifyResults(default_drain_bytes_); } -// This one TBD TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicFailWithDefault) { on_server_init_function_ = [&]() { waitXdsStream(); }; addDynamicFilter(filter_name_, false, true); @@ -234,14 +247,13 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicFailWithDefault) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); // Send config update with invalid config (drain_bytes has to >=2). - sendXdsResponse("1", 1); + sendXdsResponse(filter_name_, "1", 1); test_server_->waitForCounterGe( "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_fail", 1); // The default filter will be installed. Start a TCP connection. The default filter drain 2 bytes. sendDataVerifyResults(default_drain_bytes_); } -// This one TBD TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicFailWithoutDefault) { on_server_init_function_ = [&]() { waitXdsStream(); }; addDynamicFilter(filter_name_, false, false); @@ -250,11 +262,11 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicFailWithoutDefault) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); // Send config update with invalid config (drain_bytes has to >=2). - sendXdsResponse("1", 1); + sendXdsResponse(filter_name_, "1", 1); test_server_->waitForCounterGe( "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_fail", 1); - // The missing config filter will be installed when a correction is created. - // The missing config filter will close the connection. + // The missing config filter will be installed and close the connection when a correction is + // created. EXPECT_LOG_CONTAINS("warn", "Close socket and stop the iteration onAccept.", { IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort(port_name_)); auto result = tcp_client->write(data_); @@ -272,36 +284,85 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicWithoutWarming) { // Send data without send config update. sendDataVerifyResults(default_drain_bytes_); // Send update should cause a different response. - sendXdsResponse("1", 3); + sendXdsResponse(filter_name_, "1", 3); test_server_->waitForCounterGe( "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 1); sendDataVerifyResults(3); } -TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicWithoutWarmingFail) { +TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicWithoutWarmingConfigFail) { on_server_init_function_ = [&]() { waitXdsStream(); }; addDynamicFilter(filter_name_, true); initialize(); - sendXdsResponse("1", 1); + // Send data without send config update. + sendDataVerifyResults(default_drain_bytes_); + // Send config update with invalid config (drain_bytes has to >=2). + sendXdsResponse(filter_name_, "1", 1); test_server_->waitForCounterGe( "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_fail", 1); sendDataVerifyResults(default_drain_bytes_); } -TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicTwoSubscriptionsSameName) { +TEST_P(ListenerExtensionDiscoveryIntegrationTest, TwoSubscriptionsSameName) { on_server_init_function_ = [&]() { waitXdsStream(); }; addDynamicFilter(filter_name_, true); addDynamicFilter(filter_name_, false); initialize(); - sendXdsResponse("1", 3); + EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); + sendXdsResponse(filter_name_, "1", 3); test_server_->waitForCounterGe( "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 1); // Each filter drain 3 bytes. sendDataVerifyResults(6); } +// Testing it works with two static listener filter configuration. +TEST_P(ListenerExtensionDiscoveryIntegrationTest, TwoStaticFilters) { + addStaticFilter("foo", 3); + addStaticFilter("bar", 5); + initialize(); + + // filter drain 3+5 bytes. + sendDataVerifyResults(8); +} + +// Testing it works with mixed static/dynamic listener filter configuration. +TEST_P(ListenerExtensionDiscoveryIntegrationTest, TwoDynamicTwoStaticFilterMixed) { + on_server_init_function_ = [&]() { waitXdsStream(); }; + addDynamicFilter(filter_name_, false); + addStaticFilter("bar", 2); + addDynamicFilter(filter_name_, true); + addStaticFilter("foobar", 2); + initialize(); + + EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); + + sendXdsResponse(filter_name_, "1", 3); + test_server_->waitForCounterGe( + "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 1); + // filter drain 3 + 2 + 3 + 2 bytes. + sendDataVerifyResults(10); +} + +TEST_P(ListenerExtensionDiscoveryIntegrationTest, TwoDynamicTwoStaticFilterMixedDifferentOrder) { + on_server_init_function_ = [&]() { waitXdsStream(); }; + addStaticFilter("bar", 2); + addStaticFilter("baz", 2); + addDynamicFilter(filter_name_, true); + addDynamicFilter(filter_name_, false); + initialize(); + + EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); + + sendXdsResponse(filter_name_, "1", 2); + test_server_->waitForCounterGe( + "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 1); + // filter drain 2 + 2 + 2 + 2 bytes. + sendDataVerifyResults(8); +} + TEST_P(ListenerExtensionDiscoveryIntegrationTest, DestroyDuringInit) { // If rate limiting is enabled on the config source, gRPC mux drainage updates the requests // queue size on destruction. The update calls out to stats scope nested under the extension From dfa9386c0c491a8c36fd66a0d0d4c087c18f8b54 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Tue, 24 May 2022 14:01:57 +0000 Subject: [PATCH 22/28] adding missing config stats counter Signed-off-by: Yanjun Xiang --- source/server/configuration_impl.cc | 34 +++++++++++---- source/server/configuration_impl.h | 3 +- source/server/listener_impl.cc | 3 +- ...er_extension_discovery_integration_test.cc | 42 ++++++++++++------- 4 files changed, 55 insertions(+), 27 deletions(-) diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index da39835226fa..013556b37978 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -38,16 +38,29 @@ bool FilterChainUtility::buildFilterChain(Network::FilterManager& filter_manager return filter_manager.initializeReadFilters(); } +/** + * All missing listener config stats. @see stats_macros.h + */ +#define ALL_MISSING_LISTENER_CONFIG_STATS(COUNTER) COUNTER(config_missing) +/** + * Struct definition for all missing listener config stats. @see stats_macros.h + */ +struct MissingListenerConfigStats { + ALL_MISSING_LISTENER_CONFIG_STATS(GENERATE_COUNTER_STRUCT) +}; + class MissingConfigTcpListenerFilter : public Network::ListenerFilter, public Logger::Loggable { public: - MissingConfigTcpListenerFilter() = default; + MissingConfigTcpListenerFilter(Stats::Scope& stats_scope) + : scope_(stats_scope), stats_({ALL_MISSING_LISTENER_CONFIG_STATS(POOL_COUNTER(scope_))}) {} // Network::ListenerFilter Network::FilterStatus onAccept(Network::ListenerFilterCallbacks& cb) override { - ENVOY_LOG(warn, "Listener filter: new connection accepted while missing configuration. " - "Close socket and stop the iteration onAccept."); + ENVOY_LOG(debug, "Listener filter: new connection accepted while missing configuration. " + "Close socket and stop the iteration onAccept."); cb.socket().ioHandle().close(); + stats_.config_missing_.inc(); return Network::FilterStatus::StopIteration; } Network::FilterStatus onData(Network::ListenerFilterBuffer&) override { @@ -55,10 +68,15 @@ class MissingConfigTcpListenerFilter : public Network::ListenerFilter, return Network::FilterStatus::StopIteration; } size_t maxReadBytes() const override { return 0; } + +private: + Stats::Scope& scope_; + MissingListenerConfigStats stats_; }; bool FilterChainUtility::buildFilterChain(Network::ListenerFilterManager& filter_manager, - const Filter::ListenerFilterFactoriesList& factories) { + const Filter::ListenerFilterFactoriesList& factories, + Stats::Scope& stats_scope) { bool added_missing_config_filter = false; for (const auto& filter_config_provider : factories) { auto config = filter_config_provider->config(); @@ -67,14 +85,12 @@ bool FilterChainUtility::buildFilterChain(Network::ListenerFilterManager& filter config_value(filter_manager); continue; } + // If a filter config is missing after warming, stop iteration. if (!added_missing_config_filter) { - ENVOY_LOG_MISC(trace, "Missing filter config for a provider {}", - filter_config_provider->name()); - filter_manager.addAcceptFilter(nullptr, std::make_unique()); + filter_manager.addAcceptFilter(nullptr, + std::make_unique(stats_scope)); added_missing_config_filter = true; - } else { - ENVOY_LOG_MISC(trace, "Provider {} missing a filter config", filter_config_provider->name()); } } diff --git a/source/server/configuration_impl.h b/source/server/configuration_impl.h index 503c396ffce3..af959b56bd65 100644 --- a/source/server/configuration_impl.h +++ b/source/server/configuration_impl.h @@ -82,7 +82,8 @@ class FilterChainUtility { * TODO(sumukhs): Coalesce with the above as they are very similar */ static bool buildFilterChain(Network::ListenerFilterManager& filter_manager, - const Filter::ListenerFilterFactoriesList& factories); + const Filter::ListenerFilterFactoriesList& factories, + Stats::Scope& stats_scope); /** * Given a UdpListenerFilterManager and a list of factories, create a new filter chain. Chain diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index d54d0cea89d2..c2f62e25366d 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -823,7 +823,8 @@ bool ListenerImpl::createNetworkFilterChain( } bool ListenerImpl::createListenerFilterChain(Network::ListenerFilterManager& manager) { - return Configuration::FilterChainUtility::buildFilterChain(manager, listener_filter_factories_); + return Configuration::FilterChainUtility::buildFilterChain(manager, listener_filter_factories_, + listenerScope()); } void ListenerImpl::createUdpListenerFilterChain(Network::UdpListenerFilterManager& manager, diff --git a/test/integration/listener_extension_discovery_integration_test.cc b/test/integration/listener_extension_discovery_integration_test.cc index a276ed6ad16c..c15080841a21 100644 --- a/test/integration/listener_extension_discovery_integration_test.cc +++ b/test/integration/listener_extension_discovery_integration_test.cc @@ -24,8 +24,10 @@ class ListenerExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegra bool set_default_config = true, bool rate_limit = false) { config_helper_.addConfigModifier([name, apply_without_warming, set_default_config, rate_limit, this](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { - auto* listener_filter = - bootstrap.mutable_static_resources()->mutable_listeners(0)->add_listener_filters(); + auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0); + listener->set_stat_prefix("listener_stat"); + auto* listener_filter = listener->add_listener_filters(); + listener_filter->set_name(name); auto* discovery = listener_filter->mutable_config_discovery(); @@ -210,13 +212,21 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicSuccessWithTtl) { // The missing config listener filter will be installed to handle the connection. test_server_->waitForCounterGe( "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 2); - EXPECT_LOG_CONTAINS("warn", "Close socket and stop the iteration onAccept.", { - IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort(port_name_)); - auto result = tcp_client->write(data_); - if (result) { - tcp_client->waitForDisconnect(); - } - }); + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort(port_name_)); + auto result = tcp_client->write(data_); + if (result) { + tcp_client->waitForDisconnect(); + } + // The config_missing stats counter increases by 1. + test_server_->waitForCounterGe("listener.listener_stat.config_missing", 1); + + // Send the data again. The config_missing stats counter increases to 2. + tcp_client = makeTcpConnection(lookupPort(port_name_)); + result = tcp_client->write(data_); + if (result) { + tcp_client->waitForDisconnect(); + } + test_server_->waitForCounterGe("listener.listener_stat.config_missing", 2); } TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicSuccessWithTtlWithDefault) { @@ -267,13 +277,13 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicFailWithoutDefault) { "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_fail", 1); // The missing config filter will be installed and close the connection when a correction is // created. - EXPECT_LOG_CONTAINS("warn", "Close socket and stop the iteration onAccept.", { - IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort(port_name_)); - auto result = tcp_client->write(data_); - if (result) { - tcp_client->waitForDisconnect(); - } - }); + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort(port_name_)); + auto result = tcp_client->write(data_); + if (result) { + tcp_client->waitForDisconnect(); + } + // The config_missing stats counter increases by 1. + test_server_->waitForCounterGe("listener.listener_stat.config_missing", 1); } TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicWithoutWarming) { From 05234bdee20d93b5a059b9f6324c3637cdff9fd4 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Tue, 24 May 2022 21:03:40 +0000 Subject: [PATCH 23/28] addressing comments Signed-off-by: Yanjun Xiang --- docs/root/configuration/listeners/stats.rst | 1 + envoy/server/listener_manager.h | 1 - source/server/configuration_impl.cc | 4 ++-- .../filter/config_discovery_impl_test.cc | 23 +------------------ ...er_extension_discovery_integration_test.cc | 12 +++++----- 5 files changed, 10 insertions(+), 31 deletions(-) diff --git a/docs/root/configuration/listeners/stats.rst b/docs/root/configuration/listeners/stats.rst index 85aa1f687e5c..d27271178f4c 100644 --- a/docs/root/configuration/listeners/stats.rst +++ b/docs/root/configuration/listeners/stats.rst @@ -24,6 +24,7 @@ with the following statistics: downstream_global_cx_overflow, Counter, Total connections rejected due to enforcement of global connection limit downstream_pre_cx_timeout, Counter, Sockets that timed out during listener filter processing downstream_pre_cx_active, Gauge, Sockets currently undergoing listener filter processing + extension_config_missing, Counter, Total connections closed due to missing listener filter extension configuration global_cx_overflow, Counter, Total connections rejected due to enforcement of the global connection limit no_filter_chain_match, Counter, Total connections that didn't match any filter chain downstream_listener_filter_remote_close, Counter, Total connections closed by remote when peek data for listener filters diff --git a/envoy/server/listener_manager.h b/envoy/server/listener_manager.h index 07a65765ef48..65c3ac338e47 100644 --- a/envoy/server/listener_manager.h +++ b/envoy/server/listener_manager.h @@ -20,7 +20,6 @@ namespace Envoy { namespace Filter { - class TcpListenerFilterConfigProviderManagerImpl; } // namespace Filter diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index 013556b37978..2b953def8309 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -41,7 +41,7 @@ bool FilterChainUtility::buildFilterChain(Network::FilterManager& filter_manager /** * All missing listener config stats. @see stats_macros.h */ -#define ALL_MISSING_LISTENER_CONFIG_STATS(COUNTER) COUNTER(config_missing) +#define ALL_MISSING_LISTENER_CONFIG_STATS(COUNTER) COUNTER(extension_config_missing) /** * Struct definition for all missing listener config stats. @see stats_macros.h */ @@ -60,7 +60,7 @@ class MissingConfigTcpListenerFilter : public Network::ListenerFilter, ENVOY_LOG(debug, "Listener filter: new connection accepted while missing configuration. " "Close socket and stop the iteration onAccept."); cb.socket().ioHandle().close(); - stats_.config_missing_.inc(); + stats_.extension_config_missing_.inc(); return Network::FilterStatus::StopIteration; } Network::FilterStatus onData(Network::ListenerFilterBuffer&) override { diff --git a/test/common/filter/config_discovery_impl_test.cc b/test/common/filter/config_discovery_impl_test.cc index c7dd53ebef7e..efa5d52dfb7d 100644 --- a/test/common/filter/config_discovery_impl_test.cc +++ b/test/common/filter/config_discovery_impl_test.cc @@ -158,7 +158,7 @@ class FilterConfigDiscoveryImplTest : public FilterConfigDiscoveryTestBase { } virtual const std::string getFilterType() const PURE; - virtual const Network::ListenerFilterMatcherSharedPtr& getFilterMatcher() const PURE; + virtual const Network::ListenerFilterMatcherSharedPtr getFilterMatcher() const { return nullptr; } virtual const std::string getConfigReloadCounter() const PURE; virtual const std::string getConfigFailCounter() const PURE; @@ -175,20 +175,13 @@ class HttpFilterConfigDiscoveryImplTest HttpFilterConfigProviderManagerImpl, envoy::extensions::filters::http::router::v3::Router> { public: - HttpFilterConfigDiscoveryImplTest() : matcher_(nullptr) {} - const std::string getFilterType() const override { return "http"; } - const Network::ListenerFilterMatcherSharedPtr& getFilterMatcher() const override { - return matcher_; - } const std::string getConfigReloadCounter() const override { return "extension_config_discovery.http_filter.foo.config_reload"; } const std::string getConfigFailCounter() const override { return "extension_config_discovery.http_filter.foo.config_fail"; } - - const Network::ListenerFilterMatcherSharedPtr matcher_; }; // TCP listener filter test @@ -198,20 +191,13 @@ class TcpListenerFilterConfigDiscoveryImplTest TcpListenerFilterConfigProviderManagerImpl, ::test::integration::filters::TestInspectorFilterConfig> { public: - TcpListenerFilterConfigDiscoveryImplTest() : matcher_(nullptr) {} - const std::string getFilterType() const override { return "listener"; } - const Network::ListenerFilterMatcherSharedPtr& getFilterMatcher() const override { - return matcher_; - } const std::string getConfigReloadCounter() const override { return "extension_config_discovery.tcp_listener_filter.foo.config_reload"; } const std::string getConfigFailCounter() const override { return "extension_config_discovery.tcp_listener_filter.foo.config_fail"; } - - const Network::ListenerFilterMatcherSharedPtr matcher_; }; // UDP listener filter test @@ -221,20 +207,13 @@ class UdpListenerFilterConfigDiscoveryImplTest UdpListenerFilterConfigProviderManagerImpl, test::integration::filters::TestUdpListenerFilterConfig> { public: - UdpListenerFilterConfigDiscoveryImplTest() : matcher_(nullptr) {} - const std::string getFilterType() const override { return "listener"; } - const Network::ListenerFilterMatcherSharedPtr& getFilterMatcher() const override { - return matcher_; - } const std::string getConfigReloadCounter() const override { return "extension_config_discovery.udp_listener_filter.foo.config_reload"; } const std::string getConfigFailCounter() const override { return "extension_config_discovery.udp_listener_filter.foo.config_fail"; } - - const Network::ListenerFilterMatcherSharedPtr matcher_; }; /*************************************************************************************** diff --git a/test/integration/listener_extension_discovery_integration_test.cc b/test/integration/listener_extension_discovery_integration_test.cc index c15080841a21..447a6bf959f5 100644 --- a/test/integration/listener_extension_discovery_integration_test.cc +++ b/test/integration/listener_extension_discovery_integration_test.cc @@ -217,16 +217,16 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicSuccessWithTtl) { if (result) { tcp_client->waitForDisconnect(); } - // The config_missing stats counter increases by 1. - test_server_->waitForCounterGe("listener.listener_stat.config_missing", 1); + // The extension_config_missing stats counter increases by 1. + test_server_->waitForCounterGe("listener.listener_stat.extension_config_missing", 1); - // Send the data again. The config_missing stats counter increases to 2. + // Send the data again. The extension_config_missing stats counter increases to 2. tcp_client = makeTcpConnection(lookupPort(port_name_)); result = tcp_client->write(data_); if (result) { tcp_client->waitForDisconnect(); } - test_server_->waitForCounterGe("listener.listener_stat.config_missing", 2); + test_server_->waitForCounterGe("listener.listener_stat.extension_config_missing", 2); } TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicSuccessWithTtlWithDefault) { @@ -282,8 +282,8 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicFailWithoutDefault) { if (result) { tcp_client->waitForDisconnect(); } - // The config_missing stats counter increases by 1. - test_server_->waitForCounterGe("listener.listener_stat.config_missing", 1); + // The extension_config_missing stats counter increases by 1. + test_server_->waitForCounterGe("listener.listener_stat.extension_config_missing", 1); } TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicWithoutWarming) { From c3b0974219b3ad30f55f8c5622117a16905e2e39 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Wed, 25 May 2022 04:15:00 +0000 Subject: [PATCH 24/28] changing to return plain pointer in the interface function Signed-off-by: Yanjun Xiang --- envoy/server/listener_manager.h | 4 ++-- source/server/config_validation/server.cc | 4 +--- source/server/config_validation/server.h | 9 ++++----- source/server/listener_impl.cc | 4 ++-- source/server/listener_manager_impl.h | 14 +++++--------- test/config_test/config_test.cc | 9 +++------ test/mocks/server/listener_component_factory.h | 2 +- test/server/listener_manager_impl_test.h | 9 +++------ 8 files changed, 21 insertions(+), 34 deletions(-) diff --git a/envoy/server/listener_manager.h b/envoy/server/listener_manager.h index 65c3ac338e47..b59c2a1ba7d8 100644 --- a/envoy/server/listener_manager.h +++ b/envoy/server/listener_manager.h @@ -122,10 +122,10 @@ class ListenerComponentFactory { virtual uint64_t nextListenerTag() PURE; /** - * @return std::unique_ptr the TCP listener + * @return Filter::TcpListenerFilterConfigProviderManagerImpl* the pointer of the TCP listener * config provider manager. */ - virtual const std::unique_ptr& + virtual Filter::TcpListenerFilterConfigProviderManagerImpl* getTcpListenerConfigProviderManager() PURE; }; diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index 232fbcaedb76..94f0413999b6 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -55,9 +55,7 @@ ValidationInstance::ValidationInstance( mutex_tracer_(nullptr), grpc_context_(stats_store_.symbolTable()), http_context_(stats_store_.symbolTable()), router_context_(stats_store_.symbolTable()), time_system_(time_system), server_contexts_(*this), - quic_stat_names_(stats_store_.symbolTable()), - tcp_listener_config_provider_manager_( - std::make_unique()) { + quic_stat_names_(stats_store_.symbolTable()) { TRY_ASSERT_MAIN_THREAD { initialize(options, local_address, component_factory); } END_TRY catch (const EnvoyException& e) { diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index 984808a2a3fa..73cff10adf82 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -152,7 +152,7 @@ class ValidationInstance final : Logger::Loggable, const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) override { return ProdListenerComponentFactory::createListenerFilterFactoryListImpl( - filters, context, *tcp_listener_config_provider_manager_); + filters, context, tcp_listener_config_provider_manager_); } std::vector createUdpListenerFilterFactoryList( const Protobuf::RepeatedPtrField& filters, @@ -173,9 +173,9 @@ class ValidationInstance final : Logger::Loggable, return nullptr; } uint64_t nextListenerTag() override { return 0; } - const std::unique_ptr& + Filter::TcpListenerFilterConfigProviderManagerImpl* getTcpListenerConfigProviderManager() override { - return tcp_listener_config_provider_manager_; + return &tcp_listener_config_provider_manager_; } // Server::WorkerFactory @@ -235,8 +235,7 @@ class ValidationInstance final : Logger::Loggable, Event::TimeSystem& time_system_; ServerFactoryContextImpl server_contexts_; Quic::QuicStatNames quic_stat_names_; - const std::unique_ptr - tcp_listener_config_provider_manager_; + Filter::TcpListenerFilterConfigProviderManagerImpl tcp_listener_config_provider_manager_; }; } // namespace Server diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index c2f62e25366d..c15ec60cd420 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -703,7 +703,7 @@ void ListenerImpl::buildOriginalDstListenerFilter() { Network::ListenerFilterFactoryCb callback = factory.createListenerFilterFactoryFromProto( Envoy::ProtobufWkt::Empty(), nullptr, *listener_factory_context_); - const auto& cfg_provider_manager = parent_.factory_.getTcpListenerConfigProviderManager(); + auto* cfg_provider_manager = parent_.factory_.getTcpListenerConfigProviderManager(); auto filter_config_provider = cfg_provider_manager->createStaticFilterConfigProvider( callback, "envoy.filters.listener.original_dst"); listener_filter_factories_.push_back(std::move(filter_config_provider)); @@ -723,7 +723,7 @@ void ListenerImpl::buildProxyProtocolListenerFilter() { Network::ListenerFilterFactoryCb callback = factory.createListenerFilterFactoryFromProto( envoy::extensions::filters::listener::proxy_protocol::v3::ProxyProtocol(), nullptr, *listener_factory_context_); - const auto& cfg_provider_manager = parent_.factory_.getTcpListenerConfigProviderManager(); + auto* cfg_provider_manager = parent_.factory_.getTcpListenerConfigProviderManager(); auto filter_config_provider = cfg_provider_manager->createStaticFilterConfigProvider( callback, "envoy.filters.listener.proxy_protocol"); listener_filter_factories_.push_back(std::move(filter_config_provider)); diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index 155cae6e1ec3..9270375bc580 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -42,10 +42,7 @@ class ListenerFilterChainFactoryBuilder; class ProdListenerComponentFactory : public ListenerComponentFactory, Logger::Loggable { public: - ProdListenerComponentFactory(Instance& server) - : server_(server), - tcp_listener_config_provider_manager_( - std::make_unique()) {} + ProdListenerComponentFactory(Instance& server) : server_(server) {} /** * Static worker for createNetworkFilterFactoryList() that can be used directly in tests. */ @@ -88,7 +85,7 @@ class ProdListenerComponentFactory : public ListenerComponentFactory, const Protobuf::RepeatedPtrField& filters, Configuration::ListenerFactoryContext& context) override { return createListenerFilterFactoryListImpl(filters, context, - *tcp_listener_config_provider_manager_); + tcp_listener_config_provider_manager_); } std::vector createUdpListenerFilterFactoryList( const Protobuf::RepeatedPtrField& filters, @@ -103,16 +100,15 @@ class ProdListenerComponentFactory : public ListenerComponentFactory, DrainManagerPtr createDrainManager(envoy::config::listener::v3::Listener::DrainType drain_type) override; uint64_t nextListenerTag() override { return next_listener_tag_++; } - const std::unique_ptr& + Filter::TcpListenerFilterConfigProviderManagerImpl* getTcpListenerConfigProviderManager() override { - return tcp_listener_config_provider_manager_; + return &tcp_listener_config_provider_manager_; } private: Instance& server_; uint64_t next_listener_tag_{1}; - const std::unique_ptr - tcp_listener_config_provider_manager_; + Filter::TcpListenerFilterConfigProviderManagerImpl tcp_listener_config_provider_manager_; }; class ListenerImpl; diff --git a/test/config_test/config_test.cc b/test/config_test/config_test.cc index 026f5d009870..8ea506a64326 100644 --- a/test/config_test/config_test.cc +++ b/test/config_test/config_test.cc @@ -56,9 +56,7 @@ static std::vector unsuported_win32_configs = { class ConfigTest { public: ConfigTest(const OptionsImpl& options) - : api_(Api::createApiForTest(time_system_)), options_(options), - tcp_listener_config_provider_manager_( - std::make_unique()) { + : api_(Api::createApiForTest(time_system_)), options_(options) { ON_CALL(server_, options()).WillByDefault(ReturnRef(options_)); ON_CALL(server_, sslContextManager()).WillByDefault(ReturnRef(ssl_context_manager_)); ON_CALL(server_.api_, fileSystem()).WillByDefault(ReturnRef(file_system_)); @@ -114,7 +112,7 @@ class ConfigTest { filters, context); })); ON_CALL(component_factory_, getTcpListenerConfigProviderManager()) - .WillByDefault(ReturnRef(tcp_listener_config_provider_manager_)); + .WillByDefault(Return(&tcp_listener_config_provider_manager_)); ON_CALL(component_factory_, createListenerFilterFactoryList(_, _)) .WillByDefault(Invoke( [&](const Protobuf::RepeatedPtrField& @@ -162,8 +160,7 @@ class ConfigTest { NiceMock os_sys_calls_; TestThreadsafeSingletonInjector os_calls{&os_sys_calls_}; NiceMock file_system_; - const std::unique_ptr - tcp_listener_config_provider_manager_; + Filter::TcpListenerFilterConfigProviderManagerImpl tcp_listener_config_provider_manager_; }; void testMerge() { diff --git a/test/mocks/server/listener_component_factory.h b/test/mocks/server/listener_component_factory.h index 180218182344..7b3861cef7d6 100644 --- a/test/mocks/server/listener_component_factory.h +++ b/test/mocks/server/listener_component_factory.h @@ -43,7 +43,7 @@ class MockListenerComponentFactory : public ListenerComponentFactory { MOCK_METHOD(DrainManager*, createDrainManager_, (envoy::config::listener::v3::Listener::DrainType drain_type)); MOCK_METHOD(uint64_t, nextListenerTag, ()); - MOCK_METHOD(const std::unique_ptr&, + MOCK_METHOD(Filter::TcpListenerFilterConfigProviderManagerImpl*, getTcpListenerConfigProviderManager, ()); std::shared_ptr socket_; diff --git a/test/server/listener_manager_impl_test.h b/test/server/listener_manager_impl_test.h index fa5bce2421c0..1369b8b1c68b 100644 --- a/test/server/listener_manager_impl_test.h +++ b/test/server/listener_manager_impl_test.h @@ -65,9 +65,7 @@ class ListenerManagerImplTest : public testing::TestWithParam { protected: ListenerManagerImplTest() - : api_(Api::createApiForTest(server_.api_.random_)), use_matcher_(GetParam()), - tcp_listener_config_provider_manager_( - std::make_unique()) {} + : api_(Api::createApiForTest(server_.api_.random_)), use_matcher_(GetParam()) {} void SetUp() override { ON_CALL(server_, api()).WillByDefault(ReturnRef(*api_)); @@ -90,7 +88,7 @@ class ListenerManagerImplTest : public testing::TestWithParam { filters, filter_chain_factory_context); })); ON_CALL(listener_factory_, getTcpListenerConfigProviderManager()) - .WillByDefault(ReturnRef(tcp_listener_config_provider_manager_)); + .WillByDefault(Return(&tcp_listener_config_provider_manager_)); ON_CALL(listener_factory_, createListenerFilterFactoryList(_, _)) .WillByDefault(Invoke( [this](const Protobuf::RepeatedPtrField& @@ -368,8 +366,7 @@ class ListenerManagerImplTest : public testing::TestWithParam { NiceMock> callback_; // Test parameter indicating whether the unified filter chain matcher is enabled. bool use_matcher_; - const std::unique_ptr - tcp_listener_config_provider_manager_; + Filter::TcpListenerFilterConfigProviderManagerImpl tcp_listener_config_provider_manager_; }; } // namespace Server } // namespace Envoy From ed09a1d0a884455c653443ca0c2beb80e0aacddc Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Fri, 27 May 2022 01:53:27 +0000 Subject: [PATCH 25/28] adding matcher test Signed-off-by: Yanjun Xiang --- envoy/config/extension_config_provider.h | 5 +++ source/common/filter/config_discovery_impl.h | 43 ++++++++++--------- test/common/filter/BUILD | 1 + .../filter/config_discovery_impl_test.cc | 39 +++++++++++++++-- 4 files changed, 65 insertions(+), 23 deletions(-) diff --git a/envoy/config/extension_config_provider.h b/envoy/config/extension_config_provider.h index b2d9bc81d963..0e62f6ad5fca 100644 --- a/envoy/config/extension_config_provider.h +++ b/envoy/config/extension_config_provider.h @@ -2,6 +2,7 @@ #include "envoy/common/optref.h" #include "envoy/common/pure.h" +#include "envoy/network/filter.h" #include "source/common/protobuf/protobuf.h" @@ -62,6 +63,10 @@ class DynamicExtensionConfigProviderBase { * Applies the default configuration if one is set, otherwise does nothing. */ virtual void applyDefaultConfiguration() PURE; + /** + * Return Network::ListenerFilterMatcherSharedPtr& the listener filter matcher. + */ + virtual const Network::ListenerFilterMatcherSharedPtr& getListenerFilterMatcher() PURE; }; template diff --git a/source/common/filter/config_discovery_impl.h b/source/common/filter/config_discovery_impl.h index dc4af4e80d8e..a200725f5574 100644 --- a/source/common/filter/config_discovery_impl.h +++ b/source/common/filter/config_discovery_impl.h @@ -68,16 +68,17 @@ template class DynamicFilterConfigProviderImpl : public DynamicFilterConfigProviderImplBase, public DynamicFilterConfigProvider { public: - DynamicFilterConfigProviderImpl(FilterConfigSubscriptionSharedPtr& subscription, - const absl::flat_hash_set& require_type_urls, - Server::Configuration::FactoryContext& factory_context, - ProtobufTypes::MessagePtr&& default_config, - bool last_filter_in_filter_chain, - const std::string& filter_chain_type, - absl::string_view stat_prefix) + DynamicFilterConfigProviderImpl( + FilterConfigSubscriptionSharedPtr& subscription, + const absl::flat_hash_set& require_type_urls, + Server::Configuration::FactoryContext& factory_context, + ProtobufTypes::MessagePtr&& default_config, bool last_filter_in_filter_chain, + const std::string& filter_chain_type, absl::string_view stat_prefix, + const Network::ListenerFilterMatcherSharedPtr& listener_filter_matcher) : DynamicFilterConfigProviderImplBase(subscription, require_type_urls, last_filter_in_filter_chain, filter_chain_type), - stat_prefix_(stat_prefix), main_config_(std::make_shared()), + listener_filter_matcher_(listener_filter_matcher), stat_prefix_(stat_prefix), + main_config_(std::make_shared()), default_configuration_(std::move(default_config)), tls_(factory_context.threadLocal()) { tls_.set([](Event::Dispatcher&) { return std::make_shared(); }); }; @@ -120,9 +121,13 @@ class DynamicFilterConfigProviderImpl : public DynamicFilterConfigProviderImplBa onConfigUpdate(*default_configuration_, "", nullptr); } } + const Network::ListenerFilterMatcherSharedPtr& getListenerFilterMatcher() override { + return listener_filter_matcher_; + } protected: const std::string& getStatPrefix() const { return stat_prefix_; } + const Network::ListenerFilterMatcherSharedPtr listener_filter_matcher_; private: virtual FactoryCb instantiateFilterFactory(const Protobuf::Message& message) const PURE; @@ -163,17 +168,16 @@ class DynamicFilterConfigProviderImpl : public DynamicFilterConfigProviderImplBa class HttpDynamicFilterConfigProviderImpl : public DynamicFilterConfigProviderImpl { public: - HttpDynamicFilterConfigProviderImpl(FilterConfigSubscriptionSharedPtr& subscription, - const absl::flat_hash_set& require_type_urls, - Server::Configuration::FactoryContext& factory_context, - ProtobufTypes::MessagePtr&& default_config, - bool last_filter_in_filter_chain, - const std::string& filter_chain_type, - absl::string_view stat_prefix, - const Network::ListenerFilterMatcherSharedPtr&) + HttpDynamicFilterConfigProviderImpl( + FilterConfigSubscriptionSharedPtr& subscription, + const absl::flat_hash_set& require_type_urls, + Server::Configuration::FactoryContext& factory_context, + ProtobufTypes::MessagePtr&& default_config, bool last_filter_in_filter_chain, + const std::string& filter_chain_type, absl::string_view stat_prefix, + const Network::ListenerFilterMatcherSharedPtr& listener_filter_matcher) : DynamicFilterConfigProviderImpl(subscription, require_type_urls, factory_context, std::move(default_config), last_filter_in_filter_chain, - filter_chain_type, stat_prefix), + filter_chain_type, stat_prefix, listener_filter_matcher), factory_context_(factory_context) {} void validateMessage(const std::string& config_name, const Protobuf::Message& message, const std::string& factory_name) const override { @@ -208,15 +212,14 @@ class ListenerDynamicFilterConfigProviderImpl : public DynamicFilterConfigProvid const Network::ListenerFilterMatcherSharedPtr& listener_filter_matcher) : DynamicFilterConfigProviderImpl( subscription, require_type_urls, factory_context, std::move(default_config), - last_filter_in_filter_chain, filter_chain_type, stat_prefix), - factory_context_(factory_context), listener_filter_matcher_(listener_filter_matcher) {} + last_filter_in_filter_chain, filter_chain_type, stat_prefix, listener_filter_matcher), + factory_context_(factory_context) {} void validateMessage(const std::string&, const Protobuf::Message&, const std::string&) const override {} protected: Server::Configuration::ListenerFactoryContext& factory_context_; - const Network::ListenerFilterMatcherSharedPtr listener_filter_matcher_; }; class TcpListenerDynamicFilterConfigProviderImpl diff --git a/test/common/filter/BUILD b/test/common/filter/BUILD index 6ddd0fba951e..1d711f510766 100644 --- a/test/common/filter/BUILD +++ b/test/common/filter/BUILD @@ -15,6 +15,7 @@ envoy_cc_test( "//source/common/config:utility_lib", "//source/common/filter:config_discovery_lib", "//source/common/json:json_loader_lib", + "//source/common/network:filter_matcher_lib", "//source/extensions/filters/http/router:config", "//test/integration/filters:add_body_filter_config_lib", "//test/integration/filters:test_listener_filter_lib", diff --git a/test/common/filter/config_discovery_impl_test.cc b/test/common/filter/config_discovery_impl_test.cc index efa5d52dfb7d..4911af00bec4 100644 --- a/test/common/filter/config_discovery_impl_test.cc +++ b/test/common/filter/config_discovery_impl_test.cc @@ -11,6 +11,7 @@ #include "source/common/config/utility.h" #include "source/common/filter/config_discovery_impl.h" #include "source/common/json/json_loader.h" +#include "source/common/network/filter_matcher.h" #include "test/integration/filters/add_body_filter.pb.h" #include "test/integration/filters/test_listener_filter.h" @@ -104,7 +105,7 @@ class FilterConfigDiscoveryImplTest : public FilterConfigDiscoveryTestBase { return filter_config_provider_manager_->createDynamicFilterConfigProvider( config_source, name, factory_context_, "xds.", last_filter_config, getFilterType(), - getFilterMatcher()); + getMatcher()); } void setup(bool warm = true, bool default_configuration = false, bool last_filter_config = true) { @@ -158,7 +159,7 @@ class FilterConfigDiscoveryImplTest : public FilterConfigDiscoveryTestBase { } virtual const std::string getFilterType() const PURE; - virtual const Network::ListenerFilterMatcherSharedPtr getFilterMatcher() const { return nullptr; } + virtual const Network::ListenerFilterMatcherSharedPtr getMatcher() const { return nullptr; } virtual const std::string getConfigReloadCounter() const PURE; virtual const std::string getConfigFailCounter() const PURE; @@ -458,7 +459,7 @@ TYPED_TEST(FilterConfigDiscoveryImplTestParameter, WrongDefaultConfig) { EXPECT_THROW_WITH_MESSAGE( config_discovery_test.filter_config_provider_manager_->createDynamicFilterConfigProvider( config_source, "foo", config_discovery_test.factory_context_, "xds.", true, - config_discovery_test.getFilterType(), config_discovery_test.getFilterMatcher()), + config_discovery_test.getFilterType(), config_discovery_test.getMatcher()), EnvoyException, "Error: cannot find filter factory foo for default filter " "configuration with type URL " @@ -499,6 +500,38 @@ TYPED_TEST(FilterConfigDiscoveryImplTestParameter, TerminalFilterInvalid) { .value()); } +// TCP listener filter matcher test. +class TcpListenerFilterConfigMatcherTest : public testing::Test, + public TcpListenerFilterConfigDiscoveryImplTest { +public: + TcpListenerFilterConfigMatcherTest() : matcher_(nullptr) {} + const Network::ListenerFilterMatcherSharedPtr getMatcher() const override { return matcher_; } + void setMatcher(Network::ListenerFilterMatcherSharedPtr matcher) { matcher_ = matcher; } + + Network::ListenerFilterMatcherSharedPtr matcher_; +}; + +// Setup matcher as nullptr +TEST_F(TcpListenerFilterConfigMatcherTest, TcpListenerFilterNullMatcher) { + // By default, getMatcher() returns nullptr. + setup(); + // Verify the listener_filter_matcher_ stored in provider_ matches with the configuration. + EXPECT_EQ(provider_->getListenerFilterMatcher(), nullptr); + EXPECT_CALL(init_watcher_, ready()); +} + +// Setup matcher as any matcher. +TEST_F(TcpListenerFilterConfigMatcherTest, TcpListenerFilterAnyMatcher) { + envoy::config::listener::v3::ListenerFilterChainMatchPredicate pred; + pred.set_any_match(true); + Network::ListenerFilterMatcherSharedPtr matcher = + Network::ListenerFilterMatcherBuilder::buildListenerFilterMatcher(pred); + setMatcher(matcher); + setup(); + EXPECT_EQ(provider_->getListenerFilterMatcher(), matcher); + EXPECT_CALL(init_watcher_, ready()); +} + } // namespace } // namespace Filter } // namespace Envoy From 79679dbd13a490956a3b365a6f664cb2a15c49bc Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Fri, 27 May 2022 16:14:52 +0000 Subject: [PATCH 26/28] fixing CI error due to .h not generated correctly Signed-off-by: Yanjun Xiang --- envoy/config/BUILD | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/envoy/config/BUILD b/envoy/config/BUILD index 71bb10360edc..e94a190bc114 100644 --- a/envoy/config/BUILD +++ b/envoy/config/BUILD @@ -50,7 +50,10 @@ envoy_cc_library( envoy_cc_library( name = "extension_config_provider_interface", hdrs = ["extension_config_provider.h"], - deps = ["//source/common/protobuf"], + deps = [ + "//envoy/network:filter_interface", + "//source/common/protobuf", + ], ) envoy_cc_library( From 2864e0bbee481004d40ea866485bb128f1194824 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Mon, 30 May 2022 04:28:30 +0000 Subject: [PATCH 27/28] adding integration tests for listener filter matcher Signed-off-by: Yanjun Xiang --- ...er_extension_discovery_integration_test.cc | 61 ++++++++++++++++++- 1 file changed, 58 insertions(+), 3 deletions(-) diff --git a/test/integration/listener_extension_discovery_integration_test.cc b/test/integration/listener_extension_discovery_integration_test.cc index 447a6bf959f5..0183a70d29e8 100644 --- a/test/integration/listener_extension_discovery_integration_test.cc +++ b/test/integration/listener_extension_discovery_integration_test.cc @@ -13,6 +13,8 @@ namespace Envoy { namespace { +enum class ListenerMatcherType { NULLMATCHER, ANYMATCHER, NOTANYMATCHER }; + class ListenerExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, public BaseIntegrationTest { public: @@ -21,8 +23,10 @@ class ListenerExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegra data_("HelloWorld"), port_name_("http") {} void addDynamicFilter(const std::string& name, bool apply_without_warming, - bool set_default_config = true, bool rate_limit = false) { + bool set_default_config = true, bool rate_limit = false, + ListenerMatcherType matcher = ListenerMatcherType::NULLMATCHER) { config_helper_.addConfigModifier([name, apply_without_warming, set_default_config, rate_limit, + matcher, this](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0); listener->set_stat_prefix("listener_stat"); @@ -48,6 +52,12 @@ class ListenerExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegra if (rate_limit) { api_config_source->mutable_rate_limit_settings()->mutable_max_tokens()->set_value(10); } + // Add listener filter matcher config. + if (matcher == ListenerMatcherType::ANYMATCHER) { + listener_filter->mutable_filter_disabled()->set_any_match(true); + } else if (matcher == ListenerMatcherType::NOTANYMATCHER) { + listener_filter->mutable_filter_disabled()->mutable_not_match()->set_any_match(true); + } auto* grpc_service = api_config_source->add_grpc_services(); setGrpcService(*grpc_service, "ecds_cluster", getEcdsFakeUpstream().localAddress()); }); @@ -195,6 +205,36 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicSuccess) { sendDataVerifyResults(3); } +TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicSuccessWithAnyMatcher) { + on_server_init_function_ = [&]() { waitXdsStream(); }; + // Add dynamic filter with any matcher, which effectively disables the filter. + addDynamicFilter(filter_name_, false, true, false, ListenerMatcherType::ANYMATCHER); + initialize(); + EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); + + // Send config update to have listener filter drain 5 bytes of data. + sendXdsResponse(filter_name_, "1", 5); + test_server_->waitForCounterGe( + "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 1); + // Send data, verify the listener filter doesn't drain any data. + sendDataVerifyResults(0); +} + +TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicSuccessWithNotAnyMatcher) { + on_server_init_function_ = [&]() { waitXdsStream(); }; + // Not any matcher negates the any matcher, which effectively enable the filter. + addDynamicFilter(filter_name_, false, true, false, ListenerMatcherType::NOTANYMATCHER); + initialize(); + EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); + + // Send config update to have listener filter drain 5 bytes of data. + sendXdsResponse(filter_name_, "1", 5); + test_server_->waitForCounterGe( + "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 1); + // Send data, verify the listener filter drains five bytes data. + sendDataVerifyResults(5); +} + TEST_P(ListenerExtensionDiscoveryIntegrationTest, BasicSuccessWithTtl) { on_server_init_function_ = [&]() { waitXdsStream(); }; addDynamicFilter(filter_name_, false, false); @@ -346,7 +386,6 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, TwoDynamicTwoStaticFilterMixed addDynamicFilter(filter_name_, true); addStaticFilter("foobar", 2); initialize(); - EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); sendXdsResponse(filter_name_, "1", 3); @@ -363,7 +402,6 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, TwoDynamicTwoStaticFilterMixed addDynamicFilter(filter_name_, true); addDynamicFilter(filter_name_, false); initialize(); - EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); sendXdsResponse(filter_name_, "1", 2); @@ -373,6 +411,23 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, TwoDynamicTwoStaticFilterMixed sendDataVerifyResults(8); } +TEST_P(ListenerExtensionDiscoveryIntegrationTest, DynamicStaticFilterMixedWithAnyMatcher) { + on_server_init_function_ = [&]() { waitXdsStream(); }; + addStaticFilter("bar", 2); + addStaticFilter("baz", 2); + addDynamicFilter(filter_name_, true); + // This filter with any matcher is effectively disabled. + addDynamicFilter(filter_name_, false, true, false, ListenerMatcherType::ANYMATCHER); + initialize(); + EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); + + sendXdsResponse(filter_name_, "1", 3); + test_server_->waitForCounterGe( + "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 1); + // The filters totally drain 2 + 2 + 3 bytes. + sendDataVerifyResults(7); +} + TEST_P(ListenerExtensionDiscoveryIntegrationTest, DestroyDuringInit) { // If rate limiting is enabled on the config source, gRPC mux drainage updates the requests // queue size on destruction. The update calls out to stats scope nested under the extension From fc1e9c1c3135d019e73b98f53e40cc7f5181782b Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Mon, 30 May 2022 14:30:01 +0000 Subject: [PATCH 28/28] adding matcher integration tests for static filter Signed-off-by: Yanjun Xiang --- ...er_extension_discovery_integration_test.cc | 48 ++++++++++++------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/test/integration/listener_extension_discovery_integration_test.cc b/test/integration/listener_extension_discovery_integration_test.cc index 0183a70d29e8..8246c7ceb351 100644 --- a/test/integration/listener_extension_discovery_integration_test.cc +++ b/test/integration/listener_extension_discovery_integration_test.cc @@ -52,29 +52,36 @@ class ListenerExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegra if (rate_limit) { api_config_source->mutable_rate_limit_settings()->mutable_max_tokens()->set_value(10); } - // Add listener filter matcher config. - if (matcher == ListenerMatcherType::ANYMATCHER) { - listener_filter->mutable_filter_disabled()->set_any_match(true); - } else if (matcher == ListenerMatcherType::NOTANYMATCHER) { - listener_filter->mutable_filter_disabled()->mutable_not_match()->set_any_match(true); - } + addListenerFilterMatcher(listener_filter, matcher); auto* grpc_service = api_config_source->add_grpc_services(); setGrpcService(*grpc_service, "ecds_cluster", getEcdsFakeUpstream().localAddress()); }); } - void addStaticFilter(const std::string& name, uint32_t drain_bytes) { + void addStaticFilter(const std::string& name, uint32_t drain_bytes, + ListenerMatcherType matcher = ListenerMatcherType::NULLMATCHER) { config_helper_.addConfigModifier( - [name, drain_bytes](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + [name, drain_bytes, matcher, this](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { auto* listener_filter = bootstrap.mutable_static_resources()->mutable_listeners(0)->add_listener_filters(); listener_filter->set_name(name); auto configuration = test::integration::filters::TestTcpListenerFilterConfig(); configuration.set_drain_bytes(drain_bytes); listener_filter->mutable_typed_config()->PackFrom(configuration); + addListenerFilterMatcher(listener_filter, matcher); }); } + // Add listener filter matcher config. + void addListenerFilterMatcher(envoy::config::listener::v3::ListenerFilter* listener_filter, + ListenerMatcherType matcher) { + if (matcher == ListenerMatcherType::ANYMATCHER) { + listener_filter->mutable_filter_disabled()->set_any_match(true); + } else if (matcher == ListenerMatcherType::NOTANYMATCHER) { + listener_filter->mutable_filter_disabled()->mutable_not_match()->set_any_match(true); + } + } + void initialize() override { defer_listener_finalization_ = true; setUpstreamCount(1); @@ -378,6 +385,16 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, TwoStaticFilters) { sendDataVerifyResults(8); } +// Static listener filter configuration with matcher config. +TEST_P(ListenerExtensionDiscoveryIntegrationTest, TwoStaticFiltersWithMatcher) { + addStaticFilter("foo", 3, ListenerMatcherType::ANYMATCHER); + addStaticFilter("bar", 5, ListenerMatcherType::NOTANYMATCHER); + initialize(); + + // The filter with any matcher configured is disabled. + sendDataVerifyResults(5); +} + // Testing it works with mixed static/dynamic listener filter configuration. TEST_P(ListenerExtensionDiscoveryIntegrationTest, TwoDynamicTwoStaticFilterMixed) { on_server_init_function_ = [&]() { waitXdsStream(); }; @@ -395,7 +412,7 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, TwoDynamicTwoStaticFilterMixed sendDataVerifyResults(10); } -TEST_P(ListenerExtensionDiscoveryIntegrationTest, TwoDynamicTwoStaticFilterMixedDifferentOrder) { +TEST_P(ListenerExtensionDiscoveryIntegrationTest, DynamicStaticFilterMixedDifferentOrder) { on_server_init_function_ = [&]() { waitXdsStream(); }; addStaticFilter("bar", 2); addStaticFilter("baz", 2); @@ -411,12 +428,11 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, TwoDynamicTwoStaticFilterMixed sendDataVerifyResults(8); } -TEST_P(ListenerExtensionDiscoveryIntegrationTest, DynamicStaticFilterMixedWithAnyMatcher) { +TEST_P(ListenerExtensionDiscoveryIntegrationTest, DynamicStaticFilterMixedWithMatcher) { on_server_init_function_ = [&]() { waitXdsStream(); }; - addStaticFilter("bar", 2); - addStaticFilter("baz", 2); - addDynamicFilter(filter_name_, true); - // This filter with any matcher is effectively disabled. + addStaticFilter("bar", 2, ListenerMatcherType::NOTANYMATCHER); + addStaticFilter("baz", 2, ListenerMatcherType::ANYMATCHER); + addDynamicFilter(filter_name_, true, true, false, ListenerMatcherType::NOTANYMATCHER); addDynamicFilter(filter_name_, false, true, false, ListenerMatcherType::ANYMATCHER); initialize(); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); @@ -424,8 +440,8 @@ TEST_P(ListenerExtensionDiscoveryIntegrationTest, DynamicStaticFilterMixedWithAn sendXdsResponse(filter_name_, "1", 3); test_server_->waitForCounterGe( "extension_config_discovery.tcp_listener_filter." + filter_name_ + ".config_reload", 1); - // The filters totally drain 2 + 2 + 3 bytes. - sendDataVerifyResults(7); + // The filters with any matcher configured are disabled. They totally drain 2 + 3 bytes. + sendDataVerifyResults(5); } TEST_P(ListenerExtensionDiscoveryIntegrationTest, DestroyDuringInit) {