From a33db77c7283579ca4de8f83288efa38294ab300 Mon Sep 17 00:00:00 2001 From: qinggniq Date: Tue, 30 Mar 2021 17:38:42 +0800 Subject: [PATCH 01/23] imporve doc Signed-off-by: qinggniq --- bazel/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bazel/README.md b/bazel/README.md index 22ea2a22cd24f..ada41cb1987e6 100644 --- a/bazel/README.md +++ b/bazel/README.md @@ -844,7 +844,7 @@ The compilation database could also be used to setup editors with cross referenc For example, you can use [You Complete Me](https://valloric.github.io/YouCompleteMe/) or [clangd](https://clangd.llvm.org/) with supported editors. -For example, use following command to prepare a compilation database: +This requires Python 3.8.0+, download from [here](https://www.python.org/downloads/) if you do not have one. Then, use following command to prepare a compilation database: ``` TEST_TMPDIR=/tmp tools/gen_compilation_database.py From e93cfb40f633c8f71ea4021e1ed2899576fc73cf Mon Sep 17 00:00:00 2001 From: WangCong Date: Tue, 30 Mar 2021 19:29:14 +0800 Subject: [PATCH 02/23] Update README.md Signed-off-by: qinggniq --- bazel/README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/bazel/README.md b/bazel/README.md index ada41cb1987e6..2d984364509c4 100644 --- a/bazel/README.md +++ b/bazel/README.md @@ -844,7 +844,9 @@ The compilation database could also be used to setup editors with cross referenc For example, you can use [You Complete Me](https://valloric.github.io/YouCompleteMe/) or [clangd](https://clangd.llvm.org/) with supported editors. -This requires Python 3.8.0+, download from [here](https://www.python.org/downloads/) if you do not have one. Then, use following command to prepare a compilation database: +This requires Python 3.8.0+, download from [here](https://www.python.org/downloads/) if you do no have it installed already. + +Use following command to prepare a compilation database: ``` TEST_TMPDIR=/tmp tools/gen_compilation_database.py From 418554b3d0f81cfe43dab99dec8986582bb477cb Mon Sep 17 00:00:00 2001 From: qinggniq Date: Wed, 31 Mar 2021 10:50:58 +0800 Subject: [PATCH 03/23] fix typo Signed-off-by: qinggniq --- bazel/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bazel/README.md b/bazel/README.md index 2d984364509c4..23d675a70a698 100644 --- a/bazel/README.md +++ b/bazel/README.md @@ -844,9 +844,9 @@ The compilation database could also be used to setup editors with cross referenc For example, you can use [You Complete Me](https://valloric.github.io/YouCompleteMe/) or [clangd](https://clangd.llvm.org/) with supported editors. -This requires Python 3.8.0+, download from [here](https://www.python.org/downloads/) if you do no have it installed already. +This requires Python 3.8.0+, download from [here](https://www.python.org/downloads/) if you do not have it installed already. -Use following command to prepare a compilation database: +Use the following command to prepare a compilation database: ``` TEST_TMPDIR=/tmp tools/gen_compilation_database.py From bcf50b2ef0e5a1598ef0c96c8feabc2abb58fa44 Mon Sep 17 00:00:00 2001 From: qinggniq Date: Thu, 1 Apr 2021 14:50:31 +0800 Subject: [PATCH 04/23] change signature of isTermianlFilter Signed-off-by: qinggniq add a test Signed-off-by: qinggniq add test Signed-off-by: qinggniq add test Signed-off-by: qinggniq --- .../intro/arch_overview/http/http_filters.rst | 2 +- .../filter/http/filter_config_provider.h | 3 +- include/envoy/server/filter_config.h | 4 +- source/common/config/utility.h | 2 +- .../http/filter_config_discovery_impl.cc | 42 ++++++++++----- .../http/filter_config_discovery_impl.h | 12 +++-- .../filters/http/common/factory_base.h | 10 ++++ .../extensions/filters/http/router/config.h | 4 +- .../filters/network/common/factory_base.h | 11 +++- .../filters/network/direct_response/config.cc | 5 +- .../extensions/filters/network/echo/config.cc | 5 +- .../network/http_connection_manager/config.cc | 14 +++-- .../network/http_connection_manager/config.h | 7 +-- source/server/listener_manager_impl.cc | 6 +-- .../http/filter_config_discovery_impl_test.cc | 30 ++++++++++- .../network/dubbo_proxy/config_test.cc | 2 +- .../http_connection_manager/config_test.cc | 2 +- .../network/redis_proxy/config_test.cc | 8 +-- .../filters/network/tcp_proxy/config_test.cc | 2 +- .../network/thrift_proxy/config_test.cc | 2 +- .../extension_discovery_integration_test.cc | 51 +++++++++++++++++++ .../filters/set_response_code_filter.cc | 7 +++ .../set_response_code_filter_config.proto | 1 + .../tcp_conn_pool_integration_test.cc | 4 +- test/server/config_validation/server_test.cc | 5 +- test/server/listener_manager_impl_test.cc | 5 +- 26 files changed, 195 insertions(+), 51 deletions(-) diff --git a/docs/root/intro/arch_overview/http/http_filters.rst b/docs/root/intro/arch_overview/http/http_filters.rst index 32f177d8e5f7a..ce090d98850de 100644 --- a/docs/root/intro/arch_overview/http/http_filters.rst +++ b/docs/root/intro/arch_overview/http/http_filters.rst @@ -41,7 +41,7 @@ decoder/encoder filters): - A - B # The last configured filter has to be a terminal filter, as determined by the - # NamedHttpFilterConfigFactory::isTerminalFilter() function. This is most likely the router + # NamedHttpFilterConfigFactory::isTerminalFilter(config, context) function. This is most likely the router # filter. - C diff --git a/include/envoy/filter/http/filter_config_provider.h b/include/envoy/filter/http/filter_config_provider.h index 39d387bafda1f..f3f3d4f10da16 100644 --- a/include/envoy/filter/http/filter_config_provider.h +++ b/include/envoy/filter/http/filter_config_provider.h @@ -36,7 +36,8 @@ class FilterConfigProviderManager { virtual FilterConfigProviderPtr createDynamicFilterConfigProvider( const envoy::config::core::v3::ExtensionConfigSource& config_source, const std::string& filter_config_name, Server::Configuration::FactoryContext& factory_context, - const std::string& stat_prefix) PURE; + const std::string& stat_prefix, bool last_filter_in_current_config, + const std::string& filter_chain_type) PURE; /** * Get an FilterConfigProviderPtr for a statically inlined filter config. diff --git a/include/envoy/server/filter_config.h b/include/envoy/server/filter_config.h index 5260902f97533..ad61c968db1b3 100644 --- a/include/envoy/server/filter_config.h +++ b/include/envoy/server/filter_config.h @@ -131,7 +131,7 @@ class NamedNetworkFilterConfigFactory : public ProtocolOptionsFactory { /** * @return bool true if this filter must be the last filter in a filter chain, false otherwise. */ - virtual bool isTerminalFilter() { return false; } + virtual bool isTerminalFilter(const Protobuf::Message&, FactoryContext&) { return false; } }; /** @@ -204,7 +204,7 @@ class NamedHttpFilterConfigFactory : public ProtocolOptionsFactory { /** * @return bool true if this filter must be the last filter in a filter chain, false otherwise. */ - virtual bool isTerminalFilter() { return false; } + virtual bool isTerminalFilter(const Protobuf::Message&, FactoryContext&) { return false; } /** * @return FilterDependenciesPtr specification of dependencies required or diff --git a/source/common/config/utility.h b/source/common/config/utility.h index 6ab115b2fc56b..187be996622ca 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -458,7 +458,7 @@ class Utility { * @throws EnvoyException if there is a mismatch between design and configuration. */ static void validateTerminalFilters(const std::string& name, const std::string& filter_type, - const char* filter_chain_type, bool is_terminal_filter, + const std::string& filter_chain_type, bool is_terminal_filter, bool last_filter_in_current_config) { if (is_terminal_filter && !last_filter_in_current_config) { ExceptionUtil::throwEnvoyException( diff --git a/source/common/filter/http/filter_config_discovery_impl.cc b/source/common/filter/http/filter_config_discovery_impl.cc index b48a79adc166f..3419b920820da 100644 --- a/source/common/filter/http/filter_config_discovery_impl.cc +++ b/source/common/filter/http/filter_config_discovery_impl.cc @@ -17,16 +17,21 @@ namespace Http { DynamicFilterConfigProviderImpl::DynamicFilterConfigProviderImpl( FilterConfigSubscriptionSharedPtr& subscription, const absl::flat_hash_set& require_type_urls, - Server::Configuration::FactoryContext& factory_context) + Server::Configuration::FactoryContext& factory_context, bool last_filter_in_current_config, + const std::string& filter_chain_type) : subscription_(subscription), require_type_urls_(require_type_urls), - tls_(factory_context.threadLocal()), - init_target_("DynamicFilterConfigProviderImpl", [this]() { - subscription_->start(); - // This init target is used to activate the subscription but not wait - // for a response. It is used whenever a default config is provided to be - // used while waiting for a response. - init_target_.ready(); - }) { + tls_(factory_context.threadLocal()), init_target_("DynamicFilterConfigProviderImpl", + [this]() { + subscription_->start(); + // This init target is used to activate + // the subscription but not wait for a + // response. It is used whenever a default + // config is provided to be used while + // waiting for a response. + init_target_.ready(); + }), + last_filter_in_current_config_(last_filter_in_current_config), + filter_chain_type_(filter_chain_type) { subscription_->filter_config_providers_.insert(this); tls_.set([](Event::Dispatcher&) { return std::make_shared(); }); } @@ -64,6 +69,12 @@ void DynamicFilterConfigProviderImpl::onConfigUpdate(Envoy::Http::FilterFactoryC this->current_config_ = config; }); } +void DynamicFilterConfigProviderImpl::validateTermianlFilter(const std::string& name, + const std::string& filter_type, + bool is_terminal_filter) { + Config::Utility::validateTerminalFilters(name, filter_type, filter_chain_type_, + is_terminal_filter, last_filter_in_current_config_); +} FilterConfigSubscription::FilterConfigSubscription( const envoy::config::core::v3::ConfigSource& config_source, @@ -129,6 +140,11 @@ void FilterConfigSubscription::onConfigUpdate( } ProtobufTypes::MessagePtr message = Config::Utility::translateAnyToFactoryConfig( filter_config.typed_config(), validator_, factory); + + for (auto* provider : filter_config_providers_) { + provider->validateTermianlFilter(filter_config_name_, factory.name(), + factory.isTerminalFilter(*message, factory_context_)); + } Envoy::Http::FilterFactoryCb factory_callback = factory.createFilterFactoryFromProto(*message, stat_prefix_, factory_context_); ENVOY_LOG(debug, "Updating filter config {}", filter_config_name_); @@ -203,7 +219,8 @@ std::shared_ptr FilterConfigProviderManagerImpl::getSu FilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFilterConfigProvider( const envoy::config::core::v3::ExtensionConfigSource& config_source, const std::string& filter_config_name, Server::Configuration::FactoryContext& factory_context, - const std::string& stat_prefix) { + const std::string& stat_prefix, bool last_filter_in_current_config, + const std::string& filter_chain_type) { auto subscription = getSubscription(config_source.config_source(), filter_config_name, factory_context, stat_prefix); // For warming, wait until the subscription receives the first response to indicate readiness. @@ -217,8 +234,9 @@ FilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFilterConf auto factory_type_url = TypeUtil::typeUrlToDescriptorFullName(type_url); require_type_urls.emplace(factory_type_url); } - auto provider = std::make_unique(subscription, require_type_urls, - factory_context); + auto provider = std::make_unique( + subscription, require_type_urls, factory_context, last_filter_in_current_config, + filter_chain_type); // Ensure the subscription starts if it has not already. if (config_source.apply_default_config_without_warming()) { factory_context.initManager().add(provider->init_target_); diff --git a/source/common/filter/http/filter_config_discovery_impl.h b/source/common/filter/http/filter_config_discovery_impl.h index 1dd188c97634e..5786fe89df96d 100644 --- a/source/common/filter/http/filter_config_discovery_impl.h +++ b/source/common/filter/http/filter_config_discovery_impl.h @@ -33,11 +33,14 @@ class DynamicFilterConfigProviderImpl : public FilterConfigProvider { public: DynamicFilterConfigProviderImpl(FilterConfigSubscriptionSharedPtr& subscription, const absl::flat_hash_set& require_type_urls, - Server::Configuration::FactoryContext& factory_context); + Server::Configuration::FactoryContext& factory_context, + bool last_filter_in_current_config, + const std::string& filter_chain_type); ~DynamicFilterConfigProviderImpl() override; void validateTypeUrl(const std::string& type_url) const; - + void validateTermianlFilter(const std::string& name, const std::string& filter_type, + bool is_terminal_filter); // Config::ExtensionConfigProvider const std::string& name() override; absl::optional config() override; @@ -61,6 +64,8 @@ class DynamicFilterConfigProviderImpl : public FilterConfigProvider { // case no warming is requested by any other filter config provider. Init::TargetImpl init_target_; + const bool last_filter_in_current_config_; + const std::string filter_chain_type_; friend class FilterConfigProviderManagerImpl; }; @@ -174,7 +179,8 @@ class FilterConfigProviderManagerImpl : public FilterConfigProviderManager, FilterConfigProviderPtr createDynamicFilterConfigProvider( const envoy::config::core::v3::ExtensionConfigSource& config_source, const std::string& filter_config_name, Server::Configuration::FactoryContext& factory_context, - const std::string& stat_prefix) override; + const std::string& stat_prefix, bool last_filter_in_current_config, + const std::string& filter_chain_type) override; FilterConfigProviderPtr createStaticFilterConfigProvider(const Envoy::Http::FilterFactoryCb& config, diff --git a/source/extensions/filters/http/common/factory_base.h b/source/extensions/filters/http/common/factory_base.h index 6aee7a15b36a6..434331ca7140a 100644 --- a/source/extensions/filters/http/common/factory_base.h +++ b/source/extensions/filters/http/common/factory_base.h @@ -42,10 +42,20 @@ class FactoryBase : public Server::Configuration::NamedHttpFilterConfigFactory { std::string name() const override { return name_; } + bool isTerminalFilter(const Protobuf::Message& proto_config, + Server::Configuration::FactoryContext& context) override { + return isTerminalFilter(MessageUtil::downcastAndValidate( + proto_config, context.messageValidationVisitor()), + context); + } + protected: FactoryBase(const std::string& name) : name_(name) {} private: + virtual bool isTerminalFilter(const ConfigProto&, Server::Configuration::FactoryContext&) { + return false; + } virtual Http::FilterFactoryCb createFilterFactoryFromProtoTyped(const ConfigProto& proto_config, const std::string& stats_prefix, diff --git a/source/extensions/filters/http/router/config.h b/source/extensions/filters/http/router/config.h index d17cafe32ef94..43890107cee5d 100644 --- a/source/extensions/filters/http/router/config.h +++ b/source/extensions/filters/http/router/config.h @@ -22,7 +22,9 @@ class RouterFilterConfig public: RouterFilterConfig() : FactoryBase(HttpFilterNames::get().Router) {} - bool isTerminalFilter() override { return true; } + bool isTerminalFilter(const Protobuf::Message&, Server::Configuration::FactoryContext&) override { + return true; + } private: Http::FilterFactoryCb createFilterFactoryFromProtoTyped( diff --git a/source/extensions/filters/network/common/factory_base.h b/source/extensions/filters/network/common/factory_base.h index ba5bcf52159ae..dea6c0421ba16 100644 --- a/source/extensions/filters/network/common/factory_base.h +++ b/source/extensions/filters/network/common/factory_base.h @@ -44,7 +44,16 @@ class FactoryBase : public Server::Configuration::NamedNetworkFilterConfigFactor std::string name() const override { return name_; } - bool isTerminalFilter() override { return is_terminal_filter_; } + bool isTerminalFilter(const Protobuf::Message& proto_config, + Server::Configuration::FactoryContext& context) override { + return isTerminalFilter(MessageUtil::downcastAndValidate( + proto_config, context.messageValidationVisitor()), + context); + } + + virtual bool isTerminalFilter(const ConfigProto&, Server::Configuration::FactoryContext&) { + return is_terminal_filter_; + } protected: FactoryBase(const std::string& name, bool is_terminal = false) diff --git a/source/extensions/filters/network/direct_response/config.cc b/source/extensions/filters/network/direct_response/config.cc index 1e8691234404a..8066b47185a3e 100644 --- a/source/extensions/filters/network/direct_response/config.cc +++ b/source/extensions/filters/network/direct_response/config.cc @@ -32,7 +32,10 @@ class DirectResponseConfigFactory }; } - bool isTerminalFilter() override { return true; } + bool isTerminalFilter(const envoy::extensions::filters::network::direct_response::v3::Config&, + Server::Configuration::FactoryContext&) override { + return true; + } }; /** diff --git a/source/extensions/filters/network/echo/config.cc b/source/extensions/filters/network/echo/config.cc index 179c14d2a3df7..3ec5fad058022 100644 --- a/source/extensions/filters/network/echo/config.cc +++ b/source/extensions/filters/network/echo/config.cc @@ -29,7 +29,10 @@ class EchoConfigFactory }; } - bool isTerminalFilter() override { return true; } + bool isTerminalFilter(const envoy::extensions::filters::network::echo::v3::Echo&, + Server::Configuration::FactoryContext&) override { + return true; + } }; /** diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index fe95f635e57eb..8d7dc339bf1d3 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -502,8 +502,8 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( void HttpConnectionManagerConfig::processFilter( const envoy::extensions::filters::network::http_connection_manager::v3::HttpFilter& proto_config, - int i, absl::string_view prefix, FilterFactoriesList& filter_factories, - const char* filter_chain_type, bool last_filter_in_current_config) { + int i, const std::string& prefix, FilterFactoriesList& filter_factories, + const std::string& filter_chain_type, bool last_filter_in_current_config) { ENVOY_LOG(debug, " {} filter #{}", prefix, i); if (proto_config.config_type_case() == envoy::extensions::filters::network::http_connection_manager::v3::HttpFilter::ConfigTypeCase:: @@ -521,7 +521,7 @@ void HttpConnectionManagerConfig::processFilter( proto_config, context_.messageValidationVisitor(), factory); Http::FilterFactoryCb callback = factory.createFilterFactoryFromProto(*message, stats_prefix_, context_); - bool is_terminal = factory.isTerminalFilter(); + bool is_terminal = factory.isTerminalFilter(*message, context_); Config::Utility::validateTerminalFilters(proto_config.name(), factory.name(), filter_chain_type, is_terminal, last_filter_in_current_config); auto filter_config_provider = filter_config_provider_manager_.createStaticFilterConfigProvider( @@ -539,7 +539,7 @@ void HttpConnectionManagerConfig::processFilter( void HttpConnectionManagerConfig::processDynamicFilterConfig( const std::string& name, const envoy::config::core::v3::ExtensionConfigSource& config_discovery, - FilterFactoriesList& filter_factories, const char* filter_chain_type, + FilterFactoriesList& filter_factories, const std::string& filter_chain_type, bool last_filter_in_current_config) { ENVOY_LOG(debug, " dynamic filter name: {}", name); if (config_discovery.apply_default_config_without_warming() && @@ -555,12 +555,10 @@ void HttpConnectionManagerConfig::processDynamicFilterConfig( throw EnvoyException( fmt::format("Error: no factory found for a required type URL {}.", factory_type_url)); } - Config::Utility::validateTerminalFilters(name, factory->name(), filter_chain_type, - factory->isTerminalFilter(), - last_filter_in_current_config); } auto filter_config_provider = filter_config_provider_manager_.createDynamicFilterConfigProvider( - config_discovery, name, context_, stats_prefix_); + config_discovery, name, context_, stats_prefix_, last_filter_in_current_config, + filter_chain_type); filter_factories.push_back(std::move(filter_config_provider)); } diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 639e3f6ac9a58..c23d3e8c0692b 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -185,12 +185,13 @@ class HttpConnectionManagerConfig : Logger::Loggable, void processFilter(const envoy::extensions::filters::network::http_connection_manager::v3::HttpFilter& proto_config, - int i, absl::string_view prefix, FilterFactoriesList& filter_factories, - const char* filter_chain_type, bool last_filter_in_current_config); + int i, const std::string& prefix, FilterFactoriesList& filter_factories, + const std::string& filter_chain_type, bool last_filter_in_current_config); void processDynamicFilterConfig(const std::string& name, const envoy::config::core::v3::ExtensionConfigSource& config_discovery, - FilterFactoriesList& filter_factories, const char* filter_chain_type, + FilterFactoriesList& filter_factories, + const std::string& filter_chain_type, bool last_filter_in_current_config); void createFilterChainForFactories(Http::FilterChainFactoryCallbacks& callbacks, const FilterFactoriesList& filter_factories); diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index a89344806468c..021bfff4750cc 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -107,11 +107,11 @@ std::vector ProdListenerComponentFactory::createNetwor Config::Utility::getAndCheckFactory( proto_config); - Config::Utility::validateTerminalFilters(filters[i].name(), factory.name(), "network", - factory.isTerminalFilter(), i == filters.size() - 1); - auto message = Config::Utility::translateToFactoryConfig( proto_config, filter_chain_factory_context.messageValidationVisitor(), factory); + Config::Utility::validateTerminalFilters( + filters[i].name(), factory.name(), "network", + factory.isTerminalFilter(*message, filter_chain_factory_context), i == filters.size() - 1); Network::FilterFactoryCb callback = factory.createFilterFactoryFromProto(*message, filter_chain_factory_context); ret.push_back(callback); diff --git a/test/common/filter/http/filter_config_discovery_impl_test.cc b/test/common/filter/http/filter_config_discovery_impl_test.cc index 5768f6b247bd9..e022ab49ede85 100644 --- a/test/common/filter/http/filter_config_discovery_impl_test.cc +++ b/test/common/filter/http/filter_config_discovery_impl_test.cc @@ -76,7 +76,8 @@ class FilterConfigDiscoveryImplTest : public FilterConfigDiscoveryTestBase { } ~FilterConfigDiscoveryImplTest() override { factory_context_.thread_local_.shutdownThread(); } - FilterConfigProviderPtr createProvider(std::string name, bool warm) { + FilterConfigProviderPtr createProvider(std::string name, bool warm, + bool last_filter_config = true) { EXPECT_CALL(init_manager_, add(_)); envoy::config::core::v3::ExtensionConfigSource config_source; TestUtility::loadFromYaml(R"EOF( @@ -94,7 +95,7 @@ config_source: { ads: {} } } return filter_config_provider_manager_->createDynamicFilterConfigProvider( - config_source, name, factory_context_, "xds."); + config_source, name, factory_context_, "xds.", last_filter_config, "http"); } void setup(bool warm = true) { @@ -306,6 +307,31 @@ TEST_F(FilterConfigDiscoveryImplTest, DualProvidersInvalid) { EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value()); } +TEST_F(FilterConfigDiscoveryImplTest, TerminalFilterInvalid) { + InSequence s; + setup(); + auto provider2 = createProvider("foo", true, false); + const std::string response_yaml = R"EOF( + version_info: "1" + resources: + - "@type": type.googleapis.com/envoy.config.core.v3.TypedExtensionConfig + name: foo + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router + )EOF"; + const auto response = + TestUtility::parseYaml(response_yaml); + const auto decoded_resources = + TestUtility::decodeResources(response); + EXPECT_CALL(init_watcher_, ready()); + EXPECT_THROW_WITH_MESSAGE( + callbacks_->onConfigUpdate(decoded_resources.refvec_, response.version_info()), + EnvoyException, + "Error: terminal filter named foo of type envoy.filters.http.router must be the last filter " + "in a http filter chain."); + EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value()); +} + } // namespace } // namespace Http } // namespace Filter diff --git a/test/extensions/filters/network/dubbo_proxy/config_test.cc b/test/extensions/filters/network/dubbo_proxy/config_test.cc index bdf4b37204d0f..9aa5700d2b3e4 100644 --- a/test/extensions/filters/network/dubbo_proxy/config_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/config_test.cc @@ -62,7 +62,7 @@ TEST_F(DubboFilterConfigTest, ValidProtoConfiguration) { NiceMock context; DubboProxyFilterConfigFactory factory; Network::FilterFactoryCb cb = factory.createFilterFactoryFromProto(config, context); - EXPECT_TRUE(factory.isTerminalFilter()); + EXPECT_TRUE(factory.isTerminalFilter(config, context)); Network::MockConnection connection; EXPECT_CALL(connection, addReadFilter(_)); cb(connection); diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/test/extensions/filters/network/http_connection_manager/config_test.cc index 40ec2c38e4f98..92f8fa83b0c98 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -1278,7 +1278,7 @@ stat_prefix: router EXPECT_CALL(context_.thread_local_, allocateSlot()); Network::FilterFactoryCb cb1 = factory.createFilterFactoryFromProto(proto_config, context_); Network::FilterFactoryCb cb2 = factory.createFilterFactoryFromProto(proto_config, context_); - EXPECT_TRUE(factory.isTerminalFilter()); + EXPECT_TRUE(factory.isTerminalFilter(proto_config, context_)); } TEST_F(HttpConnectionManagerConfigTest, BadHttpConnectionMangerConfig) { diff --git a/test/extensions/filters/network/redis_proxy/config_test.cc b/test/extensions/filters/network/redis_proxy/config_test.cc index ce0276dfae1a6..62188f182e684 100644 --- a/test/extensions/filters/network/redis_proxy/config_test.cc +++ b/test/extensions/filters/network/redis_proxy/config_test.cc @@ -89,7 +89,7 @@ stat_prefix: foo NiceMock context; RedisProxyFilterConfigFactory factory; Network::FilterFactoryCb cb = factory.createFilterFactoryFromProto(proto_config, context); - EXPECT_TRUE(factory.isTerminalFilter()); + EXPECT_TRUE(factory.isTerminalFilter(proto_config, context)); Network::MockConnection connection; EXPECT_CALL(connection, addReadFilter(_)); cb(connection); @@ -118,7 +118,7 @@ stat_prefix: foo NiceMock context; RedisProxyFilterConfigFactory factory; Network::FilterFactoryCb cb = factory.createFilterFactoryFromProto(proto_config, context); - EXPECT_TRUE(factory.isTerminalFilter()); + EXPECT_TRUE(factory.isTerminalFilter(proto_config, context)); Network::MockConnection connection; EXPECT_CALL(connection, addReadFilter(_)); cb(connection); @@ -139,7 +139,7 @@ stat_prefix: foo NiceMock context; RedisProxyFilterConfigFactory factory; Network::FilterFactoryCb cb = factory.createFilterFactoryFromProto(proto_config, context); - EXPECT_TRUE(factory.isTerminalFilter()); + EXPECT_TRUE(factory.isTerminalFilter(proto_config, context)); Network::MockConnection connection; EXPECT_CALL(connection, addReadFilter(_)); cb(connection); @@ -200,7 +200,7 @@ stat_prefix: foo NiceMock context; RedisProxyFilterConfigFactory factory; Network::FilterFactoryCb cb = factory.createFilterFactoryFromProto(proto_config, context); - EXPECT_TRUE(factory.isTerminalFilter()); + EXPECT_TRUE(factory.isTerminalFilter(proto_config, context)); Network::MockConnection connection; EXPECT_CALL(connection, addReadFilter(_)); cb(connection); diff --git a/test/extensions/filters/network/tcp_proxy/config_test.cc b/test/extensions/filters/network/tcp_proxy/config_test.cc index efb3be1fb8faf..05e2fe583f512 100644 --- a/test/extensions/filters/network/tcp_proxy/config_test.cc +++ b/test/extensions/filters/network/tcp_proxy/config_test.cc @@ -148,7 +148,7 @@ TEST(ConfigTest, ConfigTest) { config.set_stat_prefix("prefix"); config.set_cluster("cluster"); - EXPECT_TRUE(factory.isTerminalFilter()); + EXPECT_TRUE(factory.isTerminalFilter(proto_config, context)); Network::FilterFactoryCb cb = factory.createFilterFactoryFromProto(config, context); Network::MockConnection connection; diff --git a/test/extensions/filters/network/thrift_proxy/config_test.cc b/test/extensions/filters/network/thrift_proxy/config_test.cc index 9b3db4c4ebc8d..0983c287874f4 100644 --- a/test/extensions/filters/network/thrift_proxy/config_test.cc +++ b/test/extensions/filters/network/thrift_proxy/config_test.cc @@ -57,7 +57,7 @@ class ThriftFilterConfigTestBase { void testConfig(envoy::extensions::filters::network::thrift_proxy::v3::ThriftProxy& config) { Network::FilterFactoryCb cb; EXPECT_NO_THROW({ cb = factory_.createFilterFactoryFromProto(config, context_); }); - EXPECT_TRUE(factory_.isTerminalFilter()); + EXPECT_TRUE(factory_.isTerminalFilter(proto_config, context)); Network::MockConnection connection; EXPECT_CALL(connection, addReadFilter(_)); diff --git a/test/integration/extension_discovery_integration_test.cc b/test/integration/extension_discovery_integration_test.cc index 45e68419ad051..32399582b4039 100644 --- a/test/integration/extension_discovery_integration_test.cc +++ b/test/integration/extension_discovery_integration_test.cc @@ -49,6 +49,14 @@ std::string allowAllConfig() { return "code: 200"; } std::string invalidConfig() { return "code: 90"; } +std::string terminalFilterConfig() { + return R"EOF( + prefix: "/private" + code: 200 + is_terminal_filter: true +)EOF"; +} + class ExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, public HttpIntegrationTest { public: @@ -501,6 +509,49 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailWithoutDefault) { EXPECT_EQ("500", response->headers().getStatusValue()); } +// test when terminal filter config not put at last config position +TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailTerminalFitlerNotFit) { + on_server_init_function_ = [&]() { waitXdsStream(); }; + addDynamicFilter("foo", false, false); + initialize(); + test_server_->waitForCounterGe("listener_manager.lds.update_success", 1); + EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); + registerTestServerPorts({"http"}); + sendXdsResponse("foo", "1", terminalFilterConfig()); + test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_fail", 1); + test_server_->waitUntilListenersReady(); + test_server_->waitForGaugeGe("listener_manager.workers_started", 1); + EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); + codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); + Http::TestRequestHeaderMapImpl request_headers{ + {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}; + auto response = codec_client_->makeHeaderOnlyRequest(request_headers); + response->waitForEndStream(); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("500", response->headers().getStatusValue()); +} + +TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailWithTerminalNotFit) { + on_server_init_function_ = [&]() { waitXdsStream(); }; + addDynamicFilter("foo", false, false); + initialize(); + test_server_->waitForCounterGe("listener_manager.lds.update_success", 1); + EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); + registerTestServerPorts({"http"}); + sendXdsResponse("foo", "1", invalidConfig()); + test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_fail", 1); + test_server_->waitUntilListenersReady(); + test_server_->waitForGaugeGe("listener_manager.workers_started", 1); + EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); + codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); + Http::TestRequestHeaderMapImpl request_headers{ + {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}; + auto response = codec_client_->makeHeaderOnlyRequest(request_headers); + response->waitForEndStream(); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("500", response->headers().getStatusValue()); +} + TEST_P(ExtensionDiscoveryIntegrationTest, BasicWithoutWarming) { on_server_init_function_ = [&]() { waitXdsStream(); }; addDynamicFilter("bar", true); diff --git a/test/integration/filters/set_response_code_filter.cc b/test/integration/filters/set_response_code_filter.cc index e9964b8a58e53..709431fb81696 100644 --- a/test/integration/filters/set_response_code_filter.cc +++ b/test/integration/filters/set_response_code_filter.cc @@ -59,6 +59,13 @@ class SetResponseCodeFilterFactory : public Extensions::HttpFilters::Common::Fac callbacks.addStreamFilter(std::make_shared(filter_config)); }; } + bool isTerminalFilter(const test::integration::filters::SetResponseCodeFilterConfig& proto_config, + Server::Configuration::FactoryContext&) override { + if (proto_config.is_terminal_filter()) { + return true; + } + return false; + } }; REGISTER_FACTORY(SetResponseCodeFilterFactory, Server::Configuration::NamedHttpFilterConfigFactory); diff --git a/test/integration/filters/set_response_code_filter_config.proto b/test/integration/filters/set_response_code_filter_config.proto index 09765c970d014..c28e5a82d5867 100644 --- a/test/integration/filters/set_response_code_filter_config.proto +++ b/test/integration/filters/set_response_code_filter_config.proto @@ -8,4 +8,5 @@ message SetResponseCodeFilterConfig { string prefix = 1; uint32 code = 2 [(validate.rules).uint32 = {lt: 600 gte: 200}]; string body = 3; + bool is_terminal_filter = 4; } diff --git a/test/integration/tcp_conn_pool_integration_test.cc b/test/integration/tcp_conn_pool_integration_test.cc index a4cca156cb67c..d41bc5111ea02 100644 --- a/test/integration/tcp_conn_pool_integration_test.cc +++ b/test/integration/tcp_conn_pool_integration_test.cc @@ -101,7 +101,9 @@ class TestFilterConfigFactory : public Server::Configuration::NamedNetworkFilter } std::string name() const override { CONSTRUCT_ON_FIRST_USE(std::string, "envoy.test.router"); } - bool isTerminalFilter() override { return true; } + bool isTerminalFilter(const Protobuf::Message&, Server::Configuration::FactoryContext&) override { + return true; + } }; } // namespace diff --git a/test/server/config_validation/server_test.cc b/test/server/config_validation/server_test.cc index 0228d6dbf9518..42f2a57c2e172 100644 --- a/test/server/config_validation/server_test.cc +++ b/test/server/config_validation/server_test.cc @@ -104,7 +104,10 @@ class RuntimeFeatureValidationServerTest : public ValidationServerTest { return ProtobufTypes::MessagePtr{new ProtobufWkt::Struct()}; } - bool isTerminalFilter() override { return true; } + bool isTerminalFilter(const Protobuf::Message&, + Server::Configuration::FactoryContext&) override { + return true; + } }; }; diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index 62ffb1b271425..1656b6c342293 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -539,7 +539,10 @@ class TestStatsConfigFactory : public Configuration::NamedNetworkFilterConfigFac } std::string name() const override { return "stats_test"; } - bool isTerminalFilter() override { return true; } + + bool isTerminalFilter(const Protobuf::Message&, Server::Configuration::FactoryContext&) override { + return true; + } private: Network::FilterFactoryCb commonFilterFactory(Configuration::FactoryContext& context) { From e89113fafa5aa931cae78a62014cd9dc55b21228 Mon Sep 17 00:00:00 2001 From: qinggniq Date: Thu, 1 Apr 2021 17:18:08 +0800 Subject: [PATCH 05/23] remove code Signed-off-by: qinggniq --- .../extension_discovery_integration_test.cc | 21 ------------------- .../filters/set_response_code_filter.cc | 5 +---- 2 files changed, 1 insertion(+), 25 deletions(-) diff --git a/test/integration/extension_discovery_integration_test.cc b/test/integration/extension_discovery_integration_test.cc index 32399582b4039..aa2494d407ed3 100644 --- a/test/integration/extension_discovery_integration_test.cc +++ b/test/integration/extension_discovery_integration_test.cc @@ -531,27 +531,6 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailTerminalFitlerNotFit) { EXPECT_EQ("500", response->headers().getStatusValue()); } -TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailWithTerminalNotFit) { - on_server_init_function_ = [&]() { waitXdsStream(); }; - addDynamicFilter("foo", false, false); - initialize(); - test_server_->waitForCounterGe("listener_manager.lds.update_success", 1); - EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); - registerTestServerPorts({"http"}); - sendXdsResponse("foo", "1", invalidConfig()); - test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_fail", 1); - test_server_->waitUntilListenersReady(); - test_server_->waitForGaugeGe("listener_manager.workers_started", 1); - EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); - codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); - Http::TestRequestHeaderMapImpl request_headers{ - {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}; - auto response = codec_client_->makeHeaderOnlyRequest(request_headers); - response->waitForEndStream(); - ASSERT_TRUE(response->complete()); - EXPECT_EQ("500", response->headers().getStatusValue()); -} - TEST_P(ExtensionDiscoveryIntegrationTest, BasicWithoutWarming) { on_server_init_function_ = [&]() { waitXdsStream(); }; addDynamicFilter("bar", true); diff --git a/test/integration/filters/set_response_code_filter.cc b/test/integration/filters/set_response_code_filter.cc index 709431fb81696..066edfc7aeac1 100644 --- a/test/integration/filters/set_response_code_filter.cc +++ b/test/integration/filters/set_response_code_filter.cc @@ -61,10 +61,7 @@ class SetResponseCodeFilterFactory : public Extensions::HttpFilters::Common::Fac } bool isTerminalFilter(const test::integration::filters::SetResponseCodeFilterConfig& proto_config, Server::Configuration::FactoryContext&) override { - if (proto_config.is_terminal_filter()) { - return true; - } - return false; + return proto_config.is_terminal_filter(); } }; From 3bebde562732fd92cb0ffaacfdb4cd16caad9275 Mon Sep 17 00:00:00 2001 From: qinggniq Date: Thu, 1 Apr 2021 18:30:43 +0800 Subject: [PATCH 06/23] cover more cases to validate config Signed-off-by: qinggniq fix typo Signed-off-by: qinggniq --- include/envoy/filter/http/filter_config_provider.h | 2 ++ .../filter/http/filter_config_discovery_impl.cc | 13 ++++++++++++- .../filter/http/filter_config_discovery_impl.h | 4 ++++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/include/envoy/filter/http/filter_config_provider.h b/include/envoy/filter/http/filter_config_provider.h index f3f3d4f10da16..8c86ec1fe560d 100644 --- a/include/envoy/filter/http/filter_config_provider.h +++ b/include/envoy/filter/http/filter_config_provider.h @@ -32,6 +32,8 @@ class FilterConfigProviderManager { * @param filter_config_name the filter config resource name. * @param factory_context is the context to use for the filter config provider. * @param stat_prefix supplies the stat_prefix to use for the provider stats. + * @param last_filter_in_current_config indicate the filter position at filter chain + * @param filter_chain_type is the filter chain type */ virtual FilterConfigProviderPtr createDynamicFilterConfigProvider( const envoy::config::core::v3::ExtensionConfigSource& config_source, diff --git a/source/common/filter/http/filter_config_discovery_impl.cc b/source/common/filter/http/filter_config_discovery_impl.cc index 3419b920820da..c83553fadc8bb 100644 --- a/source/common/filter/http/filter_config_discovery_impl.cc +++ b/source/common/filter/http/filter_config_discovery_impl.cc @@ -117,7 +117,7 @@ void FilterConfigSubscription::onConfigUpdate( throw EnvoyException(fmt::format( "Unexpected number of resources in ExtensionConfigDS response: {}", resources.size())); } - const auto& filter_config = dynamic_cast( + auto& filter_config = dynamic_cast( resources[0].get().resource()); if (filter_config.name() != filter_config_name_) { throw EnvoyException(fmt::format("Unexpected resource name in ExtensionConfigDS response: {}", @@ -161,6 +161,7 @@ void FilterConfigSubscription::onConfigUpdate( last_config_ = factory_callback; last_type_url_ = type_url; last_version_info_ = version_info; + last_filter_config_ = filter_config; } void FilterConfigSubscription::onConfigUpdate( @@ -251,6 +252,14 @@ FilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFilterConf if (subscription->lastConfig().has_value()) { TRY_ASSERT_MAIN_THREAD { provider->validateTypeUrl(subscription->lastTypeUrl()); + auto& factory = + Config::Utility::getAndCheckFactory( + subscription->lastFilterConfig()); + auto message = Config::Utility::translateAnyToFactoryConfig( + subscription->lastFilterConfig().typed_config(), + factory_context.messageValidationVisitor(), factory); + provider->validateTermianlFilter(filter_config_name, factory.name(), + factory.isTerminalFilter(*message, factory_context)); last_config_valid = true; } END_TRY catch (const EnvoyException& e) { @@ -279,6 +288,8 @@ FilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFilterConf ProtobufTypes::MessagePtr message = Config::Utility::translateAnyToFactoryConfig( config_source.default_config(), factory_context.messageValidationVisitor(), *default_factory); + provider->validateTermianlFilter(filter_config_name, default_factory->name(), + default_factory->isTerminalFilter(*message, factory_context)); Envoy::Http::FilterFactoryCb default_config = default_factory->createFilterFactoryFromProto(*message, stat_prefix, factory_context); provider->onConfigUpdate(default_config, "", nullptr); diff --git a/source/common/filter/http/filter_config_discovery_impl.h b/source/common/filter/http/filter_config_discovery_impl.h index 5786fe89df96d..c14590b4dc470 100644 --- a/source/common/filter/http/filter_config_discovery_impl.h +++ b/source/common/filter/http/filter_config_discovery_impl.h @@ -107,6 +107,9 @@ class FilterConfigSubscription const absl::optional& lastConfig() { return last_config_; } const std::string& lastTypeUrl() { return last_type_url_; } const std::string& lastVersionInfo() { return last_version_info_; } + const envoy::config::core::v3::TypedExtensionConfig& lastFilterConfig() { + return last_filter_config_; + } void incrementConflictCounter(); private: @@ -126,6 +129,7 @@ class FilterConfigSubscription absl::optional last_config_{absl::nullopt}; std::string last_type_url_; std::string last_version_info_; + envoy::config::core::v3::TypedExtensionConfig last_filter_config_; Server::Configuration::FactoryContext& factory_context_; ProtobufMessage::ValidationVisitor& validator_; From 18bf6ed8c23674314f932fc5cca313e8e2bbf7a9 Mon Sep 17 00:00:00 2001 From: qinggniq Date: Thu, 1 Apr 2021 18:50:47 +0800 Subject: [PATCH 07/23] fix test flakes Signed-off-by: qinggniq --- test/server/listener_manager_impl_test.cc | 43 ++++++++++++----------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index 1656b6c342293..8a259edbba041 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -486,26 +486,6 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, TerminalNotLast) { "filter in a network filter chain."); } -TEST_F(ListenerManagerImplWithRealFiltersTest, NotTerminalLast) { - const std::string yaml = R"EOF( -address: - socket_address: - address: 127.0.0.1 - port_value: 1234 -filter_chains: -- filters: - - name: envoy.filters.network.tcp_proxy - typed_config: {} - - name: unknown_but_will_not_be_processed - typed_config: {} - )EOF"; - - EXPECT_THROW_WITH_REGEX( - manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml), "", true), EnvoyException, - "Error: terminal filter named envoy.filters.network.tcp_proxy of type " - "envoy.filters.network.tcp_proxy must be the last filter in a network filter chain."); -} - TEST_F(ListenerManagerImplWithRealFiltersTest, BadFilterName) { const std::string yaml = R"EOF( address: @@ -551,6 +531,29 @@ class TestStatsConfigFactory : public Configuration::NamedNetworkFilterConfigFac } }; +TEST_F(ListenerManagerImplWithRealFiltersTest, NotTerminalLast) { + TestStatsConfigFactory filter; + Registry::InjectFactory registered(filter); + + const std::string yaml = R"EOF( +address: + socket_address: + address: 127.0.0.1 + port_value: 1234 +filter_chains: +- filters: + - name: stats_test + typed_config: {} + - name: unknown_but_will_not_be_processed + typed_config: {} + )EOF"; + + EXPECT_THROW_WITH_REGEX(manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml), "", true), + EnvoyException, + "Error: terminal filter named stats_test of type " + "stats_test must be the last filter in a network filter chain."); +} + TEST_F(ListenerManagerImplWithRealFiltersTest, StatsScopeTest) { TestStatsConfigFactory filter; Registry::InjectFactory registered(filter); From 3d81b834a6f7e6c45dc358790a87fcfbe1d4a5ea Mon Sep 17 00:00:00 2001 From: qinggniq Date: Fri, 2 Apr 2021 13:28:20 +0800 Subject: [PATCH 08/23] fix build flakes Signed-off-by: qinggniq --- .../filter/http/filter_config_provider.h | 3 +- .../http/filter_config_discovery_impl.cc | 32 ++++++++----------- .../http/filter_config_discovery_impl.h | 16 +++++----- .../http_connection_manager/config_test.cc | 1 + .../network/thrift_proxy/config_test.cc | 2 +- 5 files changed, 26 insertions(+), 28 deletions(-) diff --git a/include/envoy/filter/http/filter_config_provider.h b/include/envoy/filter/http/filter_config_provider.h index 8c86ec1fe560d..7c481e377a9e6 100644 --- a/include/envoy/filter/http/filter_config_provider.h +++ b/include/envoy/filter/http/filter_config_provider.h @@ -32,7 +32,8 @@ class FilterConfigProviderManager { * @param filter_config_name the filter config resource name. * @param factory_context is the context to use for the filter config provider. * @param stat_prefix supplies the stat_prefix to use for the provider stats. - * @param last_filter_in_current_config indicate the filter position at filter chain + * @param last_filter_in_current_config indicates whether this filter is the last filter in the + * configured chain * @param filter_chain_type is the filter chain type */ virtual FilterConfigProviderPtr createDynamicFilterConfigProvider( diff --git a/source/common/filter/http/filter_config_discovery_impl.cc b/source/common/filter/http/filter_config_discovery_impl.cc index c83553fadc8bb..76cd8d7ed50ed 100644 --- a/source/common/filter/http/filter_config_discovery_impl.cc +++ b/source/common/filter/http/filter_config_discovery_impl.cc @@ -17,7 +17,7 @@ namespace Http { DynamicFilterConfigProviderImpl::DynamicFilterConfigProviderImpl( FilterConfigSubscriptionSharedPtr& subscription, const absl::flat_hash_set& require_type_urls, - Server::Configuration::FactoryContext& factory_context, bool last_filter_in_current_config, + Server::Configuration::FactoryContext& factory_context, bool last_filter_in_filter_chain, const std::string& filter_chain_type) : subscription_(subscription), require_type_urls_(require_type_urls), tls_(factory_context.threadLocal()), init_target_("DynamicFilterConfigProviderImpl", @@ -30,7 +30,7 @@ DynamicFilterConfigProviderImpl::DynamicFilterConfigProviderImpl( // waiting for a response. init_target_.ready(); }), - last_filter_in_current_config_(last_filter_in_current_config), + last_filter_in_filter_chain_(last_filter_in_filter_chain), filter_chain_type_(filter_chain_type) { subscription_->filter_config_providers_.insert(this); tls_.set([](Event::Dispatcher&) { return std::make_shared(); }); @@ -69,11 +69,12 @@ void DynamicFilterConfigProviderImpl::onConfigUpdate(Envoy::Http::FilterFactoryC this->current_config_ = config; }); } -void DynamicFilterConfigProviderImpl::validateTermianlFilter(const std::string& name, + +void DynamicFilterConfigProviderImpl::validateTerminalFilter(const std::string& name, const std::string& filter_type, bool is_terminal_filter) { Config::Utility::validateTerminalFilters(name, filter_type, filter_chain_type_, - is_terminal_filter, last_filter_in_current_config_); + is_terminal_filter, last_filter_in_filter_chain_); } FilterConfigSubscription::FilterConfigSubscription( @@ -117,7 +118,7 @@ void FilterConfigSubscription::onConfigUpdate( throw EnvoyException(fmt::format( "Unexpected number of resources in ExtensionConfigDS response: {}", resources.size())); } - auto& filter_config = dynamic_cast( + const auto& filter_config = dynamic_cast( resources[0].get().resource()); if (filter_config.name() != filter_config_name_) { throw EnvoyException(fmt::format("Unexpected resource name in ExtensionConfigDS response: {}", @@ -140,10 +141,9 @@ void FilterConfigSubscription::onConfigUpdate( } ProtobufTypes::MessagePtr message = Config::Utility::translateAnyToFactoryConfig( filter_config.typed_config(), validator_, factory); - + bool is_terminal_filter = factory.isTerminalFilter(*message, factory_context_); for (auto* provider : filter_config_providers_) { - provider->validateTermianlFilter(filter_config_name_, factory.name(), - factory.isTerminalFilter(*message, factory_context_)); + provider->validateTerminalFilter(filter_config_name_, factory.name(), is_terminal_filter); } Envoy::Http::FilterFactoryCb factory_callback = factory.createFilterFactoryFromProto(*message, stat_prefix_, factory_context_); @@ -157,11 +157,13 @@ void FilterConfigSubscription::onConfigUpdate( } }); } + last_config_hash_ = new_hash; last_config_ = factory_callback; last_type_url_ = type_url; last_version_info_ = version_info; - last_filter_config_ = filter_config; + last_filter_name_ = factory.name(); + last_filter_is_terminal_ = is_terminal_filter; } void FilterConfigSubscription::onConfigUpdate( @@ -252,14 +254,8 @@ FilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFilterConf if (subscription->lastConfig().has_value()) { TRY_ASSERT_MAIN_THREAD { provider->validateTypeUrl(subscription->lastTypeUrl()); - auto& factory = - Config::Utility::getAndCheckFactory( - subscription->lastFilterConfig()); - auto message = Config::Utility::translateAnyToFactoryConfig( - subscription->lastFilterConfig().typed_config(), - factory_context.messageValidationVisitor(), factory); - provider->validateTermianlFilter(filter_config_name, factory.name(), - factory.isTerminalFilter(*message, factory_context)); + provider->validateTerminalFilter(filter_config_name, subscription->lastFilterName(), + subscription->isLastFilterTerminal()); last_config_valid = true; } END_TRY catch (const EnvoyException& e) { @@ -288,7 +284,7 @@ FilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFilterConf ProtobufTypes::MessagePtr message = Config::Utility::translateAnyToFactoryConfig( config_source.default_config(), factory_context.messageValidationVisitor(), *default_factory); - provider->validateTermianlFilter(filter_config_name, default_factory->name(), + provider->validateTerminalFilter(filter_config_name, default_factory->name(), default_factory->isTerminalFilter(*message, factory_context)); Envoy::Http::FilterFactoryCb default_config = default_factory->createFilterFactoryFromProto(*message, stat_prefix, factory_context); diff --git a/source/common/filter/http/filter_config_discovery_impl.h b/source/common/filter/http/filter_config_discovery_impl.h index c14590b4dc470..7b268618398cc 100644 --- a/source/common/filter/http/filter_config_discovery_impl.h +++ b/source/common/filter/http/filter_config_discovery_impl.h @@ -34,12 +34,12 @@ class DynamicFilterConfigProviderImpl : public FilterConfigProvider { DynamicFilterConfigProviderImpl(FilterConfigSubscriptionSharedPtr& subscription, const absl::flat_hash_set& require_type_urls, Server::Configuration::FactoryContext& factory_context, - bool last_filter_in_current_config, + bool last_filter_in_filter_chain, const std::string& filter_chain_type); ~DynamicFilterConfigProviderImpl() override; void validateTypeUrl(const std::string& type_url) const; - void validateTermianlFilter(const std::string& name, const std::string& filter_type, + void validateTerminalFilter(const std::string& name, const std::string& filter_type, bool is_terminal_filter); // Config::ExtensionConfigProvider const std::string& name() override; @@ -64,7 +64,7 @@ class DynamicFilterConfigProviderImpl : public FilterConfigProvider { // case no warming is requested by any other filter config provider. Init::TargetImpl init_target_; - const bool last_filter_in_current_config_; + const bool last_filter_in_filter_chain_; const std::string filter_chain_type_; friend class FilterConfigProviderManagerImpl; }; @@ -107,9 +107,8 @@ class FilterConfigSubscription const absl::optional& lastConfig() { return last_config_; } const std::string& lastTypeUrl() { return last_type_url_; } const std::string& lastVersionInfo() { return last_version_info_; } - const envoy::config::core::v3::TypedExtensionConfig& lastFilterConfig() { - return last_filter_config_; - } + const std::string& lastFilterName() { return last_filter_name_; } + bool isLastFilterTerminal() { return last_filter_is_terminal_; } void incrementConflictCounter(); private: @@ -129,7 +128,8 @@ class FilterConfigSubscription absl::optional last_config_{absl::nullopt}; std::string last_type_url_; std::string last_version_info_; - envoy::config::core::v3::TypedExtensionConfig last_filter_config_; + std::string last_filter_name_; + bool last_filter_is_terminal_; Server::Configuration::FactoryContext& factory_context_; ProtobufMessage::ValidationVisitor& validator_; @@ -183,7 +183,7 @@ class FilterConfigProviderManagerImpl : public FilterConfigProviderManager, FilterConfigProviderPtr createDynamicFilterConfigProvider( const envoy::config::core::v3::ExtensionConfigSource& config_source, const std::string& filter_config_name, Server::Configuration::FactoryContext& factory_context, - const std::string& stat_prefix, bool last_filter_in_current_config, + const std::string& stat_prefix, bool last_filter_in_filter_chain, const std::string& filter_chain_type) override; FilterConfigProviderPtr diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/test/extensions/filters/network/http_connection_manager/config_test.cc index 92f8fa83b0c98..584af271e3fe7 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -1850,6 +1850,7 @@ stat_prefix: router config_source: { resource_api_version: V3, ads: {} } default_config: "@type": type.googleapis.com/envoy.extensions.filters.http.health_check.v3.HealthCheck + pass_through_mode: false type_urls: - type.googleapis.com/envoy.extensions.filters.http.health_check.v3.HealthCheck )EOF"; diff --git a/test/extensions/filters/network/thrift_proxy/config_test.cc b/test/extensions/filters/network/thrift_proxy/config_test.cc index 0983c287874f4..8054ce553b580 100644 --- a/test/extensions/filters/network/thrift_proxy/config_test.cc +++ b/test/extensions/filters/network/thrift_proxy/config_test.cc @@ -57,7 +57,7 @@ class ThriftFilterConfigTestBase { void testConfig(envoy::extensions::filters::network::thrift_proxy::v3::ThriftProxy& config) { Network::FilterFactoryCb cb; EXPECT_NO_THROW({ cb = factory_.createFilterFactoryFromProto(config, context_); }); - EXPECT_TRUE(factory_.isTerminalFilter(proto_config, context)); + EXPECT_TRUE(factory_.isTerminalFilter(config, context_)); Network::MockConnection connection; EXPECT_CALL(connection, addReadFilter(_)); From 55dba70cb4e5a71823fef3340d8e9595b1f0fd93 Mon Sep 17 00:00:00 2001 From: qinggniq Date: Fri, 2 Apr 2021 13:33:54 +0800 Subject: [PATCH 09/23] fix typo Signed-off-by: qinggniq --- source/common/filter/http/filter_config_discovery_impl.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/filter/http/filter_config_discovery_impl.cc b/source/common/filter/http/filter_config_discovery_impl.cc index 76cd8d7ed50ed..0786600384b14 100644 --- a/source/common/filter/http/filter_config_discovery_impl.cc +++ b/source/common/filter/http/filter_config_discovery_impl.cc @@ -222,7 +222,7 @@ std::shared_ptr FilterConfigProviderManagerImpl::getSu FilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFilterConfigProvider( const envoy::config::core::v3::ExtensionConfigSource& config_source, const std::string& filter_config_name, Server::Configuration::FactoryContext& factory_context, - const std::string& stat_prefix, bool last_filter_in_current_config, + const std::string& stat_prefix, bool last_filter_in_filter_config, const std::string& filter_chain_type) { auto subscription = getSubscription(config_source.config_source(), filter_config_name, factory_context, stat_prefix); @@ -238,7 +238,7 @@ FilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFilterConf require_type_urls.emplace(factory_type_url); } auto provider = std::make_unique( - subscription, require_type_urls, factory_context, last_filter_in_current_config, + subscription, require_type_urls, factory_context, last_filter_in_filter_config, filter_chain_type); // Ensure the subscription starts if it has not already. if (config_source.apply_default_config_without_warming()) { From af0ecf2c1c58ce43d60ded3559d25310e8c3b968 Mon Sep 17 00:00:00 2001 From: qinggniq Date: Fri, 2 Apr 2021 15:15:22 +0800 Subject: [PATCH 10/23] fix build flakes Signed-off-by: qinggniq --- test/extensions/filters/network/tcp_proxy/config_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extensions/filters/network/tcp_proxy/config_test.cc b/test/extensions/filters/network/tcp_proxy/config_test.cc index 05e2fe583f512..64b9790bd6baf 100644 --- a/test/extensions/filters/network/tcp_proxy/config_test.cc +++ b/test/extensions/filters/network/tcp_proxy/config_test.cc @@ -148,7 +148,7 @@ TEST(ConfigTest, ConfigTest) { config.set_stat_prefix("prefix"); config.set_cluster("cluster"); - EXPECT_TRUE(factory.isTerminalFilter(proto_config, context)); + EXPECT_TRUE(factory.isTerminalFilter(config, context)); Network::FilterFactoryCb cb = factory.createFilterFactoryFromProto(config, context); Network::MockConnection connection; From 7b2214de1fb19f9e0ccad349357254f68f43fcea Mon Sep 17 00:00:00 2001 From: qinggniq Date: Fri, 2 Apr 2021 16:49:57 +0800 Subject: [PATCH 11/23] fix: prevent Werror=overloaded-virtual warnig Signed-off-by: qinggniq --- source/extensions/filters/http/common/factory_base.h | 1 + 1 file changed, 1 insertion(+) diff --git a/source/extensions/filters/http/common/factory_base.h b/source/extensions/filters/http/common/factory_base.h index 434331ca7140a..7032dc4f841a0 100644 --- a/source/extensions/filters/http/common/factory_base.h +++ b/source/extensions/filters/http/common/factory_base.h @@ -53,6 +53,7 @@ class FactoryBase : public Server::Configuration::NamedHttpFilterConfigFactory { FactoryBase(const std::string& name) : name_(name) {} private: + using Server::Configuration::NamedHttpFilterConfigFactory::isTerminalFilter; virtual bool isTerminalFilter(const ConfigProto&, Server::Configuration::FactoryContext&) { return false; } From a6dee8f0b60d5507c4dbc3333a9b8f2de73590cf Mon Sep 17 00:00:00 2001 From: qinggniq Date: Tue, 6 Apr 2021 13:59:47 +0800 Subject: [PATCH 12/23] fix test flakes Signed-off-by: qinggniq --- .../filter/http/filter_config_provider.h | 4 ++-- .../http/filter_config_discovery_impl_test.cc | 19 ++++++++----------- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/include/envoy/filter/http/filter_config_provider.h b/include/envoy/filter/http/filter_config_provider.h index bd50ccd39089b..097c1616099ca 100644 --- a/include/envoy/filter/http/filter_config_provider.h +++ b/include/envoy/filter/http/filter_config_provider.h @@ -35,14 +35,14 @@ class FilterConfigProviderManager { * @param filter_config_name the filter config resource name. * @param factory_context is the context to use for the filter config provider. * @param stat_prefix supplies the stat_prefix to use for the provider stats. - * @param last_filter_in_current_config indicates whether this filter is the last filter in the + * @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 */ virtual DynamicFilterConfigProviderPtr createDynamicFilterConfigProvider( const envoy::config::core::v3::ExtensionConfigSource& config_source, const std::string& filter_config_name, Server::Configuration::FactoryContext& factory_context, - const std::string& stat_prefix, bool last_filter_in_current_config, + const std::string& stat_prefix, bool last_filter_in_filter_chain, const std::string& filter_chain_type) PURE; /** diff --git a/test/common/filter/http/filter_config_discovery_impl_test.cc b/test/common/filter/http/filter_config_discovery_impl_test.cc index ca20284a2551b..aa8b8a0e7f1ad 100644 --- a/test/common/filter/http/filter_config_discovery_impl_test.cc +++ b/test/common/filter/http/filter_config_discovery_impl_test.cc @@ -77,13 +77,10 @@ class FilterConfigDiscoveryImplTest : public FilterConfigDiscoveryTestBase { } ~FilterConfigDiscoveryImplTest() override { factory_context_.thread_local_.shutdownThread(); } -<<<<<<< HEAD - FilterConfigProviderPtr createProvider(std::string name, bool warm, - bool last_filter_config = true) { -======= DynamicFilterConfigProviderPtr createProvider(std::string name, bool warm, - bool default_configuration) { ->>>>>>> a45ce6578caa682e4556a671261772730dcdfd65 + bool default_configuration, + bool last_filter_config = true) { + EXPECT_CALL(init_manager_, add(_)); envoy::config::core::v3::ExtensionConfigSource config_source; @@ -115,8 +112,8 @@ config_source: { ads: {} } config_source, name, factory_context_, "xds.", last_filter_config, "http"); } - void setup(bool warm = true, bool default_configuration = false) { - provider_ = createProvider("foo", warm, default_configuration); + void setup(bool warm = true, bool default_configuration = false, bool last_filter_config = true) { + provider_ = createProvider("foo", warm, default_configuration, last_filter_config); callbacks_ = factory_context_.cluster_manager_.subscription_factory_.callbacks_; EXPECT_CALL(*factory_context_.cluster_manager_.subscription_factory_.subscription_, start(_)); if (!warm) { @@ -358,7 +355,6 @@ TEST_F(FilterConfigDiscoveryImplTest, DualProviders) { TEST_F(FilterConfigDiscoveryImplTest, DualProvidersInvalid) { InSequence s; setup(); - auto provider2 = createProvider("foo", true, false); const std::string response_yaml = R"EOF( version_info: "1" resources: @@ -381,10 +377,11 @@ TEST_F(FilterConfigDiscoveryImplTest, DualProvidersInvalid) { EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value()); } +// Raise exception when filter is not the last filter in filter chain, but the filter is terminal +// filter. TEST_F(FilterConfigDiscoveryImplTest, TerminalFilterInvalid) { InSequence s; - setup(); - auto provider2 = createProvider("foo", true, false); + setup(true, false, false); const std::string response_yaml = R"EOF( version_info: "1" resources: From 0e56490e09b4c0503ffc970593650caf6c45ab9a Mon Sep 17 00:00:00 2001 From: qinggniq Date: Tue, 6 Apr 2021 16:08:30 +0800 Subject: [PATCH 13/23] refactor the signature of isTerminalFilter Signed-off-by: qinggniq --- .../intro/arch_overview/http/http_filters.rst | 2 +- include/envoy/server/filter_config.h | 4 ++-- .../http/filter_config_discovery_impl.cc | 5 +++-- .../filters/http/common/factory_base.h | 14 +++++++------- source/extensions/filters/http/router/config.h | 6 +++--- .../filters/network/common/factory_base.h | 18 +++++++++--------- .../filters/network/direct_response/config.cc | 5 +++-- .../extensions/filters/network/echo/config.cc | 4 ++-- .../network/http_connection_manager/config.cc | 2 +- source/server/listener_manager_impl.cc | 3 ++- .../filters/network/dubbo_proxy/config_test.cc | 2 +- .../http_connection_manager/config_test.cc | 2 +- .../filters/network/redis_proxy/config_test.cc | 8 ++++---- .../filters/network/tcp_proxy/config_test.cc | 2 +- .../network/thrift_proxy/config_test.cc | 2 +- .../filters/set_response_code_filter.cc | 5 +++-- .../tcp_conn_pool_integration_test.cc | 3 ++- test/server/config_validation/server_test.cc | 4 ++-- test/server/listener_manager_impl_test.cc | 3 ++- 19 files changed, 50 insertions(+), 44 deletions(-) diff --git a/docs/root/intro/arch_overview/http/http_filters.rst b/docs/root/intro/arch_overview/http/http_filters.rst index ce090d98850de..091838aeeeb20 100644 --- a/docs/root/intro/arch_overview/http/http_filters.rst +++ b/docs/root/intro/arch_overview/http/http_filters.rst @@ -41,7 +41,7 @@ decoder/encoder filters): - A - B # The last configured filter has to be a terminal filter, as determined by the - # NamedHttpFilterConfigFactory::isTerminalFilter(config, context) function. This is most likely the router + # NamedHttpFilterConfigFactory::isTerminalFilterByProto(config, context) function. This is most likely the router # filter. - C diff --git a/include/envoy/server/filter_config.h b/include/envoy/server/filter_config.h index c9b07016a73ee..cea17eceabdef 100644 --- a/include/envoy/server/filter_config.h +++ b/include/envoy/server/filter_config.h @@ -131,7 +131,7 @@ class NamedNetworkFilterConfigFactory : public ProtocolOptionsFactory { /** * @return bool true if this filter must be the last filter in a filter chain, false otherwise. */ - virtual bool isTerminalFilter(const Protobuf::Message&, FactoryContext&) { return false; } + virtual bool isTerminalFilterByProto(const Protobuf::Message&, FactoryContext&) { return false; } }; /** @@ -206,7 +206,7 @@ class NamedHttpFilterConfigFactory : public ProtocolOptionsFactory { /** * @return bool true if this filter must be the last filter in a filter chain, false otherwise. */ - virtual bool isTerminalFilter(const Protobuf::Message&, FactoryContext&) { return false; } + virtual bool isTerminalFilterByProto(const Protobuf::Message&, FactoryContext&) { return false; } /** * @return FilterDependenciesPtr specification of dependencies required or diff --git a/source/common/filter/http/filter_config_discovery_impl.cc b/source/common/filter/http/filter_config_discovery_impl.cc index d2545b947d709..8f5f74b000258 100644 --- a/source/common/filter/http/filter_config_discovery_impl.cc +++ b/source/common/filter/http/filter_config_discovery_impl.cc @@ -171,7 +171,7 @@ void FilterConfigSubscription::onConfigUpdate( } ProtobufTypes::MessagePtr message = Config::Utility::translateAnyToFactoryConfig( filter_config.typed_config(), validator_, factory); - bool is_terminal_filter = factory.isTerminalFilter(*message, factory_context_); + bool is_terminal_filter = factory.isTerminalFilterByProto(*message, factory_context_); for (auto* provider : filter_config_providers_) { provider->validateTerminalFilter(filter_config_name_, factory.name(), is_terminal_filter); } @@ -293,7 +293,8 @@ DynamicFilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFil *default_factory); Config::Utility::validateTerminalFilters( filter_config_name, default_factory->name(), filter_chain_type, - default_factory->isTerminalFilter(*message, factory_context), last_filter_in_filter_config); + default_factory->isTerminalFilterByProto(*message, factory_context), + last_filter_in_filter_config); default_config = default_factory->createFilterFactoryFromProto(*message, stat_prefix, factory_context); } diff --git a/source/extensions/filters/http/common/factory_base.h b/source/extensions/filters/http/common/factory_base.h index 7032dc4f841a0..35073f3d540ef 100644 --- a/source/extensions/filters/http/common/factory_base.h +++ b/source/extensions/filters/http/common/factory_base.h @@ -42,19 +42,19 @@ class FactoryBase : public Server::Configuration::NamedHttpFilterConfigFactory { std::string name() const override { return name_; } - bool isTerminalFilter(const Protobuf::Message& proto_config, - Server::Configuration::FactoryContext& context) override { - return isTerminalFilter(MessageUtil::downcastAndValidate( - proto_config, context.messageValidationVisitor()), - context); + bool isTerminalFilterByProto(const Protobuf::Message& proto_config, + Server::Configuration::FactoryContext& context) override { + return isTerminalFilterByProtoTyped(MessageUtil::downcastAndValidate( + proto_config, context.messageValidationVisitor()), + context); } protected: FactoryBase(const std::string& name) : name_(name) {} private: - using Server::Configuration::NamedHttpFilterConfigFactory::isTerminalFilter; - virtual bool isTerminalFilter(const ConfigProto&, Server::Configuration::FactoryContext&) { + virtual bool isTerminalFilterByProtoTyped(const ConfigProto&, + Server::Configuration::FactoryContext&) { return false; } virtual Http::FilterFactoryCb diff --git a/source/extensions/filters/http/router/config.h b/source/extensions/filters/http/router/config.h index 43890107cee5d..604c3423481bf 100644 --- a/source/extensions/filters/http/router/config.h +++ b/source/extensions/filters/http/router/config.h @@ -22,11 +22,11 @@ class RouterFilterConfig public: RouterFilterConfig() : FactoryBase(HttpFilterNames::get().Router) {} - bool isTerminalFilter(const Protobuf::Message&, Server::Configuration::FactoryContext&) override { +private: + bool isTerminalFilterByProtoTyped(const envoy::extensions::filters::http::router::v3::Router&, + Server::Configuration::FactoryContext&) override { return true; } - -private: Http::FilterFactoryCb createFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::router::v3::Router& proto_config, const std::string& stat_prefix, Server::Configuration::FactoryContext& context) override; diff --git a/source/extensions/filters/network/common/factory_base.h b/source/extensions/filters/network/common/factory_base.h index dea6c0421ba16..9e818369b4a8e 100644 --- a/source/extensions/filters/network/common/factory_base.h +++ b/source/extensions/filters/network/common/factory_base.h @@ -44,15 +44,11 @@ class FactoryBase : public Server::Configuration::NamedNetworkFilterConfigFactor std::string name() const override { return name_; } - bool isTerminalFilter(const Protobuf::Message& proto_config, - Server::Configuration::FactoryContext& context) override { - return isTerminalFilter(MessageUtil::downcastAndValidate( - proto_config, context.messageValidationVisitor()), - context); - } - - virtual bool isTerminalFilter(const ConfigProto&, Server::Configuration::FactoryContext&) { - return is_terminal_filter_; + bool isTerminalFilterByProto(const Protobuf::Message& proto_config, + Server::Configuration::FactoryContext& context) override { + return isTerminalFilterByProtoTyped(MessageUtil::downcastAndValidate( + proto_config, context.messageValidationVisitor()), + context); } protected: @@ -60,6 +56,10 @@ class FactoryBase : public Server::Configuration::NamedNetworkFilterConfigFactor : name_(name), is_terminal_filter_(is_terminal) {} private: + virtual bool isTerminalFilterByProtoTyped(const ConfigProto&, + Server::Configuration::FactoryContext&) { + return is_terminal_filter_; + } virtual Network::FilterFactoryCb createFilterFactoryFromProtoTyped(const ConfigProto& proto_config, Server::Configuration::FactoryContext& context) PURE; diff --git a/source/extensions/filters/network/direct_response/config.cc b/source/extensions/filters/network/direct_response/config.cc index 8066b47185a3e..ff680dc835714 100644 --- a/source/extensions/filters/network/direct_response/config.cc +++ b/source/extensions/filters/network/direct_response/config.cc @@ -32,8 +32,9 @@ class DirectResponseConfigFactory }; } - bool isTerminalFilter(const envoy::extensions::filters::network::direct_response::v3::Config&, - Server::Configuration::FactoryContext&) override { + bool isTerminalFilterByProtoTyped( + const envoy::extensions::filters::network::direct_response::v3::Config&, + Server::Configuration::FactoryContext&) override { return true; } }; diff --git a/source/extensions/filters/network/echo/config.cc b/source/extensions/filters/network/echo/config.cc index 3ec5fad058022..119b1252d8931 100644 --- a/source/extensions/filters/network/echo/config.cc +++ b/source/extensions/filters/network/echo/config.cc @@ -29,8 +29,8 @@ class EchoConfigFactory }; } - bool isTerminalFilter(const envoy::extensions::filters::network::echo::v3::Echo&, - Server::Configuration::FactoryContext&) override { + bool isTerminalFilterByProtoTyped(const envoy::extensions::filters::network::echo::v3::Echo&, + Server::Configuration::FactoryContext&) override { return true; } }; diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 0f51d0b225be3..f9081670d9ade 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -521,7 +521,7 @@ void HttpConnectionManagerConfig::processFilter( proto_config, context_.messageValidationVisitor(), factory); Http::FilterFactoryCb callback = factory.createFilterFactoryFromProto(*message, stats_prefix_, context_); - bool is_terminal = factory.isTerminalFilter(*message, context_); + bool is_terminal = factory.isTerminalFilterByProto(*message, context_); Config::Utility::validateTerminalFilters(proto_config.name(), factory.name(), filter_chain_type, is_terminal, last_filter_in_current_config); auto filter_config_provider = filter_config_provider_manager_.createStaticFilterConfigProvider( diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 021bfff4750cc..81b44c3d36316 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -111,7 +111,8 @@ std::vector ProdListenerComponentFactory::createNetwor proto_config, filter_chain_factory_context.messageValidationVisitor(), factory); Config::Utility::validateTerminalFilters( filters[i].name(), factory.name(), "network", - factory.isTerminalFilter(*message, filter_chain_factory_context), i == filters.size() - 1); + factory.isTerminalFilterByProto(*message, filter_chain_factory_context), + i == filters.size() - 1); Network::FilterFactoryCb callback = factory.createFilterFactoryFromProto(*message, filter_chain_factory_context); ret.push_back(callback); diff --git a/test/extensions/filters/network/dubbo_proxy/config_test.cc b/test/extensions/filters/network/dubbo_proxy/config_test.cc index 9aa5700d2b3e4..d98fe3feef351 100644 --- a/test/extensions/filters/network/dubbo_proxy/config_test.cc +++ b/test/extensions/filters/network/dubbo_proxy/config_test.cc @@ -62,7 +62,7 @@ TEST_F(DubboFilterConfigTest, ValidProtoConfiguration) { NiceMock context; DubboProxyFilterConfigFactory factory; Network::FilterFactoryCb cb = factory.createFilterFactoryFromProto(config, context); - EXPECT_TRUE(factory.isTerminalFilter(config, context)); + EXPECT_TRUE(factory.isTerminalFilterByProto(config, context)); Network::MockConnection connection; EXPECT_CALL(connection, addReadFilter(_)); cb(connection); diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/test/extensions/filters/network/http_connection_manager/config_test.cc index 584af271e3fe7..367f615749be0 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -1278,7 +1278,7 @@ stat_prefix: router EXPECT_CALL(context_.thread_local_, allocateSlot()); Network::FilterFactoryCb cb1 = factory.createFilterFactoryFromProto(proto_config, context_); Network::FilterFactoryCb cb2 = factory.createFilterFactoryFromProto(proto_config, context_); - EXPECT_TRUE(factory.isTerminalFilter(proto_config, context_)); + EXPECT_TRUE(factory.isTerminalFilterByProto(proto_config, context_)); } TEST_F(HttpConnectionManagerConfigTest, BadHttpConnectionMangerConfig) { diff --git a/test/extensions/filters/network/redis_proxy/config_test.cc b/test/extensions/filters/network/redis_proxy/config_test.cc index 62188f182e684..798c8f573fda7 100644 --- a/test/extensions/filters/network/redis_proxy/config_test.cc +++ b/test/extensions/filters/network/redis_proxy/config_test.cc @@ -89,7 +89,7 @@ stat_prefix: foo NiceMock context; RedisProxyFilterConfigFactory factory; Network::FilterFactoryCb cb = factory.createFilterFactoryFromProto(proto_config, context); - EXPECT_TRUE(factory.isTerminalFilter(proto_config, context)); + EXPECT_TRUE(factory.isTerminalFilterByProto(proto_config, context)); Network::MockConnection connection; EXPECT_CALL(connection, addReadFilter(_)); cb(connection); @@ -118,7 +118,7 @@ stat_prefix: foo NiceMock context; RedisProxyFilterConfigFactory factory; Network::FilterFactoryCb cb = factory.createFilterFactoryFromProto(proto_config, context); - EXPECT_TRUE(factory.isTerminalFilter(proto_config, context)); + EXPECT_TRUE(factory.isTerminalFilterByProto(proto_config, context)); Network::MockConnection connection; EXPECT_CALL(connection, addReadFilter(_)); cb(connection); @@ -139,7 +139,7 @@ stat_prefix: foo NiceMock context; RedisProxyFilterConfigFactory factory; Network::FilterFactoryCb cb = factory.createFilterFactoryFromProto(proto_config, context); - EXPECT_TRUE(factory.isTerminalFilter(proto_config, context)); + EXPECT_TRUE(factory.isTerminalFilterByProto(proto_config, context)); Network::MockConnection connection; EXPECT_CALL(connection, addReadFilter(_)); cb(connection); @@ -200,7 +200,7 @@ stat_prefix: foo NiceMock context; RedisProxyFilterConfigFactory factory; Network::FilterFactoryCb cb = factory.createFilterFactoryFromProto(proto_config, context); - EXPECT_TRUE(factory.isTerminalFilter(proto_config, context)); + EXPECT_TRUE(factory.isTerminalFilterByProto(proto_config, context)); Network::MockConnection connection; EXPECT_CALL(connection, addReadFilter(_)); cb(connection); diff --git a/test/extensions/filters/network/tcp_proxy/config_test.cc b/test/extensions/filters/network/tcp_proxy/config_test.cc index 64b9790bd6baf..1de0240088ee4 100644 --- a/test/extensions/filters/network/tcp_proxy/config_test.cc +++ b/test/extensions/filters/network/tcp_proxy/config_test.cc @@ -148,7 +148,7 @@ TEST(ConfigTest, ConfigTest) { config.set_stat_prefix("prefix"); config.set_cluster("cluster"); - EXPECT_TRUE(factory.isTerminalFilter(config, context)); + EXPECT_TRUE(factory.isTerminalFilterByProto(config, context)); Network::FilterFactoryCb cb = factory.createFilterFactoryFromProto(config, context); Network::MockConnection connection; diff --git a/test/extensions/filters/network/thrift_proxy/config_test.cc b/test/extensions/filters/network/thrift_proxy/config_test.cc index 8054ce553b580..aa52a2143ed4b 100644 --- a/test/extensions/filters/network/thrift_proxy/config_test.cc +++ b/test/extensions/filters/network/thrift_proxy/config_test.cc @@ -57,7 +57,7 @@ class ThriftFilterConfigTestBase { void testConfig(envoy::extensions::filters::network::thrift_proxy::v3::ThriftProxy& config) { Network::FilterFactoryCb cb; EXPECT_NO_THROW({ cb = factory_.createFilterFactoryFromProto(config, context_); }); - EXPECT_TRUE(factory_.isTerminalFilter(config, context_)); + EXPECT_TRUE(factory_.isTerminalFilterByProto(config, context_)); Network::MockConnection connection; EXPECT_CALL(connection, addReadFilter(_)); diff --git a/test/integration/filters/set_response_code_filter.cc b/test/integration/filters/set_response_code_filter.cc index 066edfc7aeac1..73efdea9cc676 100644 --- a/test/integration/filters/set_response_code_filter.cc +++ b/test/integration/filters/set_response_code_filter.cc @@ -59,8 +59,9 @@ class SetResponseCodeFilterFactory : public Extensions::HttpFilters::Common::Fac callbacks.addStreamFilter(std::make_shared(filter_config)); }; } - bool isTerminalFilter(const test::integration::filters::SetResponseCodeFilterConfig& proto_config, - Server::Configuration::FactoryContext&) override { + bool isTerminalFilterByProtoTyped( + const test::integration::filters::SetResponseCodeFilterConfig& proto_config, + Server::Configuration::FactoryContext&) override { return proto_config.is_terminal_filter(); } }; diff --git a/test/integration/tcp_conn_pool_integration_test.cc b/test/integration/tcp_conn_pool_integration_test.cc index d41bc5111ea02..bf02e97f5ccf0 100644 --- a/test/integration/tcp_conn_pool_integration_test.cc +++ b/test/integration/tcp_conn_pool_integration_test.cc @@ -101,7 +101,8 @@ class TestFilterConfigFactory : public Server::Configuration::NamedNetworkFilter } std::string name() const override { CONSTRUCT_ON_FIRST_USE(std::string, "envoy.test.router"); } - bool isTerminalFilter(const Protobuf::Message&, Server::Configuration::FactoryContext&) override { + bool isTerminalFilterByProto(const Protobuf::Message&, + Server::Configuration::FactoryContext&) override { return true; } }; diff --git a/test/server/config_validation/server_test.cc b/test/server/config_validation/server_test.cc index 42f2a57c2e172..8ec76fe3a9615 100644 --- a/test/server/config_validation/server_test.cc +++ b/test/server/config_validation/server_test.cc @@ -104,8 +104,8 @@ class RuntimeFeatureValidationServerTest : public ValidationServerTest { return ProtobufTypes::MessagePtr{new ProtobufWkt::Struct()}; } - bool isTerminalFilter(const Protobuf::Message&, - Server::Configuration::FactoryContext&) override { + bool isTerminalFilterByProto(const Protobuf::Message&, + Server::Configuration::FactoryContext&) override { return true; } }; diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index 8a259edbba041..bdd42a278ba46 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -520,7 +520,8 @@ class TestStatsConfigFactory : public Configuration::NamedNetworkFilterConfigFac std::string name() const override { return "stats_test"; } - bool isTerminalFilter(const Protobuf::Message&, Server::Configuration::FactoryContext&) override { + bool isTerminalFilterByProto(const Protobuf::Message&, + Server::Configuration::FactoryContext&) override { return true; } From e7075fa7288c53bd5429602cfb342220a12fc6cc Mon Sep 17 00:00:00 2001 From: qinggniq Date: Fri, 9 Apr 2021 11:46:21 +0800 Subject: [PATCH 14/23] fix extension_discovery test Signed-off-by: qinggniq --- .../http/filter_config_discovery_impl_test.cc | 1 + test/integration/BUILD | 2 + .../extension_discovery_integration_test.cc | 64 +++++-------------- test/integration/filters/BUILD | 19 ++++++ .../filters/set_is_terminal_filter.cc | 42 ++++++++++++ .../set_is_terminal_filter_config.proto | 9 +++ .../filters/set_response_code_filter.cc | 7 +- .../set_response_code_filter_config.proto | 1 - 8 files changed, 93 insertions(+), 52 deletions(-) create mode 100644 test/integration/filters/set_is_terminal_filter.cc create mode 100644 test/integration/filters/set_is_terminal_filter_config.proto diff --git a/test/common/filter/http/filter_config_discovery_impl_test.cc b/test/common/filter/http/filter_config_discovery_impl_test.cc index aa8b8a0e7f1ad..33cff50a28367 100644 --- a/test/common/filter/http/filter_config_discovery_impl_test.cc +++ b/test/common/filter/http/filter_config_discovery_impl_test.cc @@ -355,6 +355,7 @@ TEST_F(FilterConfigDiscoveryImplTest, DualProviders) { TEST_F(FilterConfigDiscoveryImplTest, DualProvidersInvalid) { InSequence s; setup(); + auto provider2 = createProvider("foo", true, false); const std::string response_yaml = R"EOF( version_info: "1" resources: diff --git a/test/integration/BUILD b/test/integration/BUILD index 447e24e93fcb0..92884a59e23b1 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -1093,6 +1093,8 @@ envoy_cc_test( deps = [ ":http_integration_lib", "//test/common/grpc:grpc_client_integration_lib", + "//test/integration/filters:set_is_terminal_filter_config_proto_cc_proto", + "//test/integration/filters:set_is_terminal_filter_lib", "//test/integration/filters:set_response_code_filter_config_proto_cc_proto", "//test/integration/filters:set_response_code_filter_lib", "//test/test_common:utility_lib", diff --git a/test/integration/extension_discovery_integration_test.cc b/test/integration/extension_discovery_integration_test.cc index e308c5b12e223..6561b5853a43a 100644 --- a/test/integration/extension_discovery_integration_test.cc +++ b/test/integration/extension_discovery_integration_test.cc @@ -50,14 +50,6 @@ std::string allowAllConfig() { return "code: 200"; } std::string invalidConfig() { return "code: 90"; } -std::string terminalFilterConfig() { - return R"EOF( - prefix: "/private" - code: 200 - is_terminal_filter: true -)EOF"; -} - class ExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, public HttpIntegrationTest { public: @@ -299,7 +291,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccess) { Http::TestRequestHeaderMapImpl request_headers{ {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}; auto response = codec_client_->makeHeaderOnlyRequest(request_headers); - response->waitForEndStream(); + ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().getStatusValue()); } @@ -307,7 +299,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccess) { {":method", "GET"}, {":path", "/private/key"}, {":scheme", "http"}, {":authority", "host"}}; { auto response = codec_client_->makeHeaderOnlyRequest(banned_request_headers); - response->waitForEndStream(); + ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("403", response->headers().getStatusValue()); } @@ -317,7 +309,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccess) { test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_reload", 2); auto response = codec_client_->makeHeaderOnlyRequest(banned_request_headers); - response->waitForEndStream(); + ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().getStatusValue()); } @@ -432,7 +424,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithMatcher) { Http::TestRequestHeaderMapImpl request_headers{ {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}; auto response = codec_client_->makeHeaderOnlyRequest(request_headers); - response->waitForEndStream(); + ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().getStatusValue()); } @@ -440,7 +432,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithMatcher) { {":method", "GET"}, {":path", "/private/key"}, {":scheme", "http"}, {":authority", "host"}}; { auto response = codec_client_->makeHeaderOnlyRequest(banned_request_headers); - response->waitForEndStream(); + ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("403", response->headers().getStatusValue()); } @@ -451,7 +443,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithMatcher) { {":authority", "host"}}; { auto response = codec_client_->makeHeaderOnlyRequest(banned_request_headers_skipped); - response->waitForEndStream(); + ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().getStatusValue()); } @@ -474,7 +466,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicDefaultMatcher) { Http::TestRequestHeaderMapImpl request_headers{ {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}; auto response = codec_client_->makeHeaderOnlyRequest(request_headers); - response->waitForEndStream(); + ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("403", response->headers().getStatusValue()); } @@ -484,7 +476,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicDefaultMatcher) { {":scheme", "http"}, {":authority", "host"}}; auto response = codec_client_->makeHeaderOnlyRequest(request_headers); - response->waitForEndStream(); + ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().getStatusValue()); } @@ -518,7 +510,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, ReuseExtensionConfig) { Http::TestRequestHeaderMapImpl request_headers{ {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}; auto response = codec_client_->makeHeaderOnlyRequest(request_headers); - response->waitForEndStream(); + ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().getStatusValue()); test_server_->waitForCounterEq("http.config_test.extension_config_discovery.foo.config_conflict", @@ -559,7 +551,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, ReuseExtensionConfigInvalid) { Http::TestRequestHeaderMapImpl request_headers{ {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}; auto response = codec_client_->makeHeaderOnlyRequest(request_headers); - response->waitForEndStream(); + ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("403", response->headers().getStatusValue()); test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_conflict", @@ -582,7 +574,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailWithDefault) { Http::TestRequestHeaderMapImpl request_headers{ {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}; auto response = codec_client_->makeHeaderOnlyRequest(request_headers); - response->waitForEndStream(); + ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("403", response->headers().getStatusValue()); } @@ -603,29 +595,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailWithoutDefault) { Http::TestRequestHeaderMapImpl request_headers{ {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}; auto response = codec_client_->makeHeaderOnlyRequest(request_headers); - response->waitForEndStream(); - ASSERT_TRUE(response->complete()); - EXPECT_EQ("500", response->headers().getStatusValue()); -} - -// test when terminal filter config not put at last config position -TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailTerminalFitlerNotFit) { - on_server_init_function_ = [&]() { waitXdsStream(); }; - addDynamicFilter("foo", false, false); - initialize(); - test_server_->waitForCounterGe("listener_manager.lds.update_success", 1); - EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); - registerTestServerPorts({"http"}); - sendXdsResponse("foo", "1", terminalFilterConfig()); - test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_fail", 1); - test_server_->waitUntilListenersReady(); - test_server_->waitForGaugeGe("listener_manager.workers_started", 1); - EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); - codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); - Http::TestRequestHeaderMapImpl request_headers{ - {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}; - auto response = codec_client_->makeHeaderOnlyRequest(request_headers); - response->waitForEndStream(); + ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("500", response->headers().getStatusValue()); } @@ -645,7 +615,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicWithoutWarming) { {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}; { auto response = codec_client_->makeHeaderOnlyRequest(request_headers); - response->waitForEndStream(); + ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("403", response->headers().getStatusValue()); } @@ -656,7 +626,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicWithoutWarming) { 1); { auto response = codec_client_->makeHeaderOnlyRequest(request_headers); - response->waitForEndStream(); + ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().getStatusValue()); } @@ -678,7 +648,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicWithoutWarmingFail) { Http::TestRequestHeaderMapImpl request_headers{ {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}; auto response = codec_client_->makeHeaderOnlyRequest(request_headers); - response->waitForEndStream(); + ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("403", response->headers().getStatusValue()); } @@ -701,7 +671,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicTwoSubscriptionsSameName) { Http::TestRequestHeaderMapImpl request_headers{ {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}; auto response = codec_client_->makeHeaderOnlyRequest(request_headers); - response->waitForEndStream(); + ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().getStatusValue()); } @@ -723,4 +693,4 @@ TEST_P(ExtensionDiscoveryIntegrationTest, DestroyDuringInit) { } } // namespace -} // namespace Envoy +} // namespace Envoy \ No newline at end of file diff --git a/test/integration/filters/BUILD b/test/integration/filters/BUILD index 6ab4dbab5e54d..80359b81e0316 100644 --- a/test/integration/filters/BUILD +++ b/test/integration/filters/BUILD @@ -265,6 +265,25 @@ envoy_proto_library( srcs = [":set_response_code_filter_config.proto"], ) +envoy_cc_test_library( + name = "set_is_terminal_filter_lib", + srcs = [ + "set_is_terminal_filter.cc", + ], + deps = [ + ":set_is_terminal_filter_config_proto_cc_proto", + "//include/envoy/http:filter_interface", + "//include/envoy/registry", + "//source/extensions/filters/http/common:factory_base_lib", + "//source/extensions/filters/http/common:pass_through_filter_lib", + ], +) + +envoy_proto_library( + name = "set_is_terminal_filter_config_proto", + srcs = [":set_is_terminal_filter_config.proto"], +) + envoy_cc_test_library( name = "stop_iteration_and_continue", srcs = [ diff --git a/test/integration/filters/set_is_terminal_filter.cc b/test/integration/filters/set_is_terminal_filter.cc new file mode 100644 index 0000000000000..0166368edbac9 --- /dev/null +++ b/test/integration/filters/set_is_terminal_filter.cc @@ -0,0 +1,42 @@ +#include + +#include "envoy/http/filter.h" +#include "envoy/registry/registry.h" + +#include "extensions/filters/http/common/factory_base.h" +#include "extensions/filters/http/common/pass_through_filter.h" + +#include "test/integration/filters/set_is_terminal_filter_config.pb.h" +#include "test/integration/filters/set_is_terminal_filter_config.pb.validate.h" + +#include "absl/strings/match.h" + +namespace Envoy { + +// A test filter that control whether it's a termianl filter by protobuf. +class SetIsTerminalFilter : public Http::PassThroughFilter {}; + +class SetIsTerminalFilterFactory : public Extensions::HttpFilters::Common::FactoryBase< + test::integration::filters::SetIsTerminalFilterConfig> { +public: + SetIsTerminalFilterFactory() : FactoryBase("set-is-terminal-filter") {} + +private: + Http::FilterFactoryCb + createFilterFactoryFromProtoTyped(const test::integration::filters::SetIsTerminalFilterConfig&, + const std::string&, + Server::Configuration::FactoryContext&) override { + + return [](Http::FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamFilter(std::make_shared()); + }; + } + bool isTerminalFilterByProtoTyped( + const test::integration::filters::SetIsTerminalFilterConfig& proto_config, + Server::Configuration::FactoryContext&) override { + return proto_config.is_terminal_filter(); + } +}; + +REGISTER_FACTORY(SetIsTerminalFilterFactory, Server::Configuration::NamedHttpFilterConfigFactory); +} // namespace Envoy diff --git a/test/integration/filters/set_is_terminal_filter_config.proto b/test/integration/filters/set_is_terminal_filter_config.proto new file mode 100644 index 0000000000000..5a12a4289c64d --- /dev/null +++ b/test/integration/filters/set_is_terminal_filter_config.proto @@ -0,0 +1,9 @@ +syntax = "proto3"; + +package test.integration.filters; + +import "validate/validate.proto"; + +message SetIsTerminalFilterConfig { + bool is_terminal_filter = 1; +} diff --git a/test/integration/filters/set_response_code_filter.cc b/test/integration/filters/set_response_code_filter.cc index 73efdea9cc676..d703592d63e16 100644 --- a/test/integration/filters/set_response_code_filter.cc +++ b/test/integration/filters/set_response_code_filter.cc @@ -59,10 +59,9 @@ class SetResponseCodeFilterFactory : public Extensions::HttpFilters::Common::Fac callbacks.addStreamFilter(std::make_shared(filter_config)); }; } - bool isTerminalFilterByProtoTyped( - const test::integration::filters::SetResponseCodeFilterConfig& proto_config, - Server::Configuration::FactoryContext&) override { - return proto_config.is_terminal_filter(); + bool isTerminalFilterByProtoTyped(const test::integration::filters::SetResponseCodeFilterConfig&, + Server::Configuration::FactoryContext&) override { + return true; } }; diff --git a/test/integration/filters/set_response_code_filter_config.proto b/test/integration/filters/set_response_code_filter_config.proto index c28e5a82d5867..09765c970d014 100644 --- a/test/integration/filters/set_response_code_filter_config.proto +++ b/test/integration/filters/set_response_code_filter_config.proto @@ -8,5 +8,4 @@ message SetResponseCodeFilterConfig { string prefix = 1; uint32 code = 2 [(validate.rules).uint32 = {lt: 600 gte: 200}]; string body = 3; - bool is_terminal_filter = 4; } From 3b3884e8feb3dfc88971e83a92a812514f369542 Mon Sep 17 00:00:00 2001 From: qinggniq Date: Fri, 9 Apr 2021 14:20:40 +0800 Subject: [PATCH 15/23] fix extension_discovery_integration_test flakes Signed-off-by: qinggniq format Signed-off-by: qinggniq fix typo Signed-off-by: qinggniq --- .../extension_discovery_integration_test.cc | 48 +++++++++++++++++-- .../filters/set_is_terminal_filter.cc | 2 +- .../filters/set_response_code_filter.cc | 4 -- 3 files changed, 44 insertions(+), 10 deletions(-) diff --git a/test/integration/extension_discovery_integration_test.cc b/test/integration/extension_discovery_integration_test.cc index 6561b5853a43a..b932049c2762c 100644 --- a/test/integration/extension_discovery_integration_test.cc +++ b/test/integration/extension_discovery_integration_test.cc @@ -4,6 +4,7 @@ #include "envoy/service/extension/v3/config_discovery.pb.h" #include "test/common/grpc/grpc_client_integration.h" +#include "test/integration/filters/set_is_terminal_filter_config.pb.h" #include "test/integration/filters/set_response_code_filter_config.pb.h" #include "test/integration/http_integration.h" #include "test/test_common/utility.h" @@ -50,6 +51,8 @@ std::string allowAllConfig() { return "code: 200"; } std::string invalidConfig() { return "code: 90"; } +std::string terminalFilterConfig() { return "is_terminal_filter: true"; } + class ExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, public HttpIntegrationTest { public: @@ -68,6 +71,8 @@ class ExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegrationPara auto* discovery = filter->mutable_config_discovery(); discovery->add_type_urls( "type.googleapis.com/test.integration.filters.SetResponseCodeFilterConfig"); + discovery->add_type_urls( + "type.googleapis.com/test.integration.filters.SetIsTerminalFilterConfig"); discovery->add_type_urls( "type.googleapis.com/envoy.extensions.common.matching.v3.ExtensionWithMatcher"); if (set_default_config) { @@ -222,19 +227,29 @@ class ExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegrationPara } void sendXdsResponse(const std::string& name, const std::string& version, - const std::string& yaml_config, bool ttl = false) { + const std::string& yaml_config, bool ttl = false, + bool is_set_resp_code_config = true) { envoy::service::discovery::v3::DiscoveryResponse response; response.set_version_info(version); response.set_type_url("type.googleapis.com/envoy.config.core.v3.TypedExtensionConfig"); - const auto configuration = - TestUtility::parseYaml( - yaml_config); + envoy::config::core::v3::TypedExtensionConfig typed_config; typed_config.set_name(name); envoy::service::discovery::v3::Resource resource; resource.set_name(name); - typed_config.mutable_typed_config()->PackFrom(configuration); + + if (is_set_resp_code_config) { + const auto configuration = + TestUtility::parseYaml( + yaml_config); + typed_config.mutable_typed_config()->PackFrom(configuration); + } else { + const auto configuration = + TestUtility::parseYaml( + yaml_config); + typed_config.mutable_typed_config()->PackFrom(configuration); + } resource.mutable_resource()->PackFrom(typed_config); if (ttl) { resource.mutable_ttl()->set_seconds(1); @@ -692,5 +707,28 @@ TEST_P(ExtensionDiscoveryIntegrationTest, DestroyDuringInit) { ecds_connection_.reset(); } +// Validate that a listener update should fail if the subscribed extension configuration make filter +// terminal but the filter position is not at the last position at filter chain. +TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailTerminalFilterNotFit) { + on_server_init_function_ = [&]() { waitXdsStream(); }; + addDynamicFilter("foo", false, false); + initialize(); + test_server_->waitForCounterGe("listener_manager.lds.update_success", 1); + EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); + registerTestServerPorts({"http"}); + sendXdsResponse("foo", "1", terminalFilterConfig(), false, false); + test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_fail", 1); + test_server_->waitUntilListenersReady(); + test_server_->waitForGaugeGe("listener_manager.workers_started", 1); + EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); + codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); + Http::TestRequestHeaderMapImpl request_headers{ + {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}; + auto response = codec_client_->makeHeaderOnlyRequest(request_headers); + response->waitForEndStream(); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("500", response->headers().getStatusValue()); +} + } // namespace } // namespace Envoy \ No newline at end of file diff --git a/test/integration/filters/set_is_terminal_filter.cc b/test/integration/filters/set_is_terminal_filter.cc index 0166368edbac9..bc693629451fb 100644 --- a/test/integration/filters/set_is_terminal_filter.cc +++ b/test/integration/filters/set_is_terminal_filter.cc @@ -13,7 +13,7 @@ namespace Envoy { -// A test filter that control whether it's a termianl filter by protobuf. +// A test filter that control whether it's a terminal filter by protobuf. class SetIsTerminalFilter : public Http::PassThroughFilter {}; class SetIsTerminalFilterFactory : public Extensions::HttpFilters::Common::FactoryBase< diff --git a/test/integration/filters/set_response_code_filter.cc b/test/integration/filters/set_response_code_filter.cc index d703592d63e16..e9964b8a58e53 100644 --- a/test/integration/filters/set_response_code_filter.cc +++ b/test/integration/filters/set_response_code_filter.cc @@ -59,10 +59,6 @@ class SetResponseCodeFilterFactory : public Extensions::HttpFilters::Common::Fac callbacks.addStreamFilter(std::make_shared(filter_config)); }; } - bool isTerminalFilterByProtoTyped(const test::integration::filters::SetResponseCodeFilterConfig&, - Server::Configuration::FactoryContext&) override { - return true; - } }; REGISTER_FACTORY(SetResponseCodeFilterFactory, Server::Configuration::NamedHttpFilterConfigFactory); From 123ac59b9b1e47b027a31cd7747270865f1268e5 Mon Sep 17 00:00:00 2001 From: qinggniq Date: Fri, 9 Apr 2021 17:53:26 +0800 Subject: [PATCH 16/23] kick ci Signed-off-by: qinggniq From 244ebb4043eafa04efa59ed5a4eca55837a183ee Mon Sep 17 00:00:00 2001 From: qinggniq Date: Fri, 9 Apr 2021 18:14:41 +0800 Subject: [PATCH 17/23] fix doc format Signed-off-by: qinggniq --- test/integration/extension_discovery_integration_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/extension_discovery_integration_test.cc b/test/integration/extension_discovery_integration_test.cc index b932049c2762c..5dc24a54c82d7 100644 --- a/test/integration/extension_discovery_integration_test.cc +++ b/test/integration/extension_discovery_integration_test.cc @@ -731,4 +731,4 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailTerminalFilterNotFit) { } } // namespace -} // namespace Envoy \ No newline at end of file +} // namespace Envoy From d72f95f00da77b79b193fe806c770f7f2f035a91 Mon Sep 17 00:00:00 2001 From: qinggniq Date: Wed, 14 Apr 2021 11:27:21 +0800 Subject: [PATCH 18/23] fix review comment Signed-off-by: qinggniq --- .../extension_discovery_integration_test.cc | 2 +- test/server/listener_manager_impl_test.cc | 46 +++++++++---------- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/test/integration/extension_discovery_integration_test.cc b/test/integration/extension_discovery_integration_test.cc index 5dc24a54c82d7..1475afb6bb593 100644 --- a/test/integration/extension_discovery_integration_test.cc +++ b/test/integration/extension_discovery_integration_test.cc @@ -709,7 +709,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, DestroyDuringInit) { // Validate that a listener update should fail if the subscribed extension configuration make filter // terminal but the filter position is not at the last position at filter chain. -TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailTerminalFilterNotFit) { +TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailTerminalFilterNotAtEndOfFilterChain) { on_server_init_function_ = [&]() { waitXdsStream(); }; addDynamicFilter("foo", false, false); initialize(); diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index bdd42a278ba46..81594b4564521 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -486,6 +486,29 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, TerminalNotLast) { "filter in a network filter chain."); } +TEST_F(ListenerManagerImplWithRealFiltersTest, NotTerminalLast) { + const std::string yaml = R"EOF( +address: + socket_address: + address: 127.0.0.1 + port_value: 1234 +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 + cluster: cluster + - name: unknown_but_will_not_be_processed + typed_config: {} + )EOF"; + + EXPECT_THROW_WITH_REGEX( + manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml), "", true), EnvoyException, + "Error: terminal filter named envoy.filters.network.tcp_proxy of type " + "envoy.filters.network.tcp_proxy must be the last filter in a network filter chain."); +} + TEST_F(ListenerManagerImplWithRealFiltersTest, BadFilterName) { const std::string yaml = R"EOF( address: @@ -532,29 +555,6 @@ class TestStatsConfigFactory : public Configuration::NamedNetworkFilterConfigFac } }; -TEST_F(ListenerManagerImplWithRealFiltersTest, NotTerminalLast) { - TestStatsConfigFactory filter; - Registry::InjectFactory registered(filter); - - const std::string yaml = R"EOF( -address: - socket_address: - address: 127.0.0.1 - port_value: 1234 -filter_chains: -- filters: - - name: stats_test - typed_config: {} - - name: unknown_but_will_not_be_processed - typed_config: {} - )EOF"; - - EXPECT_THROW_WITH_REGEX(manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml), "", true), - EnvoyException, - "Error: terminal filter named stats_test of type " - "stats_test must be the last filter in a network filter chain."); -} - TEST_F(ListenerManagerImplWithRealFiltersTest, StatsScopeTest) { TestStatsConfigFactory filter; Registry::InjectFactory registered(filter); From f3dde43ca1f8c6c31336777ffdd1ab5a0ad225cf Mon Sep 17 00:00:00 2001 From: qinggniq Date: Thu, 15 Apr 2021 10:34:48 +0800 Subject: [PATCH 19/23] fix: envoy-filter-example sha Signed-off-by: qinggniq --- ci/filter_example_setup.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/filter_example_setup.sh b/ci/filter_example_setup.sh index c3926e5d2c7a5..73dc3f6acf8ba 100644 --- a/ci/filter_example_setup.sh +++ b/ci/filter_example_setup.sh @@ -5,7 +5,7 @@ set -e # This is the hash on https://github.com/envoyproxy/envoy-filter-example.git we pin to. -ENVOY_FILTER_EXAMPLE_GITSHA="8f0f8bba38868e875613b74f57df38f543e764ba" +ENVOY_FILTER_EXAMPLE_GITSHA="da983d42da849d6c384c70d3df5e7c2b300ce3db" ENVOY_FILTER_EXAMPLE_SRCDIR="${BUILD_DIR}/envoy-filter-example" # shellcheck disable=SC2034 From dbff93bd5af874f6ce795ac54c337c6b28a729cd Mon Sep 17 00:00:00 2001 From: qinggniq Date: Thu, 15 Apr 2021 11:03:35 +0800 Subject: [PATCH 20/23] fix: change envoy-filter-exmaple url Signed-off-by: qinggniq --- ci/filter_example_setup.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/filter_example_setup.sh b/ci/filter_example_setup.sh index 73dc3f6acf8ba..a2d54f40277b9 100644 --- a/ci/filter_example_setup.sh +++ b/ci/filter_example_setup.sh @@ -16,7 +16,7 @@ ENVOY_FILTER_EXAMPLE_TESTS=( if [[ ! -d "${ENVOY_FILTER_EXAMPLE_SRCDIR}/.git" ]]; then rm -rf "${ENVOY_FILTER_EXAMPLE_SRCDIR}" - git clone https://github.com/envoyproxy/envoy-filter-example.git "${ENVOY_FILTER_EXAMPLE_SRCDIR}" + git clone https://github.com/qinggniq/envoy-filter-example.git "${ENVOY_FILTER_EXAMPLE_SRCDIR}" fi (cd "${ENVOY_FILTER_EXAMPLE_SRCDIR}" && git fetch origin && git checkout -f "${ENVOY_FILTER_EXAMPLE_GITSHA}") From 3f9afe6adf742bda2ce15f86f80c66a9d27d0138 Mon Sep 17 00:00:00 2001 From: qinggniq Date: Fri, 16 Apr 2021 11:24:06 +0800 Subject: [PATCH 21/23] fix: change sha to latest Signed-off-by: qinggniq --- ci/filter_example_setup.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/filter_example_setup.sh b/ci/filter_example_setup.sh index f6d41c09fe409..d43298f5fa992 100644 --- a/ci/filter_example_setup.sh +++ b/ci/filter_example_setup.sh @@ -5,7 +5,7 @@ set -e # This is the hash on https://github.com/envoyproxy/envoy-filter-example.git we pin to. -ENVOY_FILTER_EXAMPLE_GITSHA="953f9b286a00b1b408d32b13d6fc9970c64b7210" +ENVOY_FILTER_EXAMPLE_GITSHA="1c81b5fbbdac5125a37a20aed7e75d8eb0ed2158" ENVOY_FILTER_EXAMPLE_SRCDIR="${BUILD_DIR}/envoy-filter-example" # shellcheck disable=SC2034 From 23aade7339ba3625602f46ca1375e78f3d744b25 Mon Sep 17 00:00:00 2001 From: qinggniq Date: Fri, 16 Apr 2021 13:11:10 +0800 Subject: [PATCH 22/23] fix: build flakes Signed-off-by: qinggniq --- test/integration/extension_discovery_integration_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/extension_discovery_integration_test.cc b/test/integration/extension_discovery_integration_test.cc index 74af8b9e966b4..d1a9b80cc3dfd 100644 --- a/test/integration/extension_discovery_integration_test.cc +++ b/test/integration/extension_discovery_integration_test.cc @@ -725,7 +725,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailTerminalFilterNotAtEndOfFilte Http::TestRequestHeaderMapImpl request_headers{ {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}; auto response = codec_client_->makeHeaderOnlyRequest(request_headers); - response->waitForEndStream(); + ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("500", response->headers().getStatusValue()); } From af1a9b2f0d00988bc1a30392d11cc2dee8499153 Mon Sep 17 00:00:00 2001 From: qinggniq Date: Mon, 19 Apr 2021 23:04:09 +0800 Subject: [PATCH 23/23] fix: change envoy-filter-example sha to latest commit Signed-off-by: qinggniq --- ci/filter_example_setup.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ci/filter_example_setup.sh b/ci/filter_example_setup.sh index d43298f5fa992..0d0cd1f602805 100644 --- a/ci/filter_example_setup.sh +++ b/ci/filter_example_setup.sh @@ -5,7 +5,7 @@ set -e # This is the hash on https://github.com/envoyproxy/envoy-filter-example.git we pin to. -ENVOY_FILTER_EXAMPLE_GITSHA="1c81b5fbbdac5125a37a20aed7e75d8eb0ed2158" +ENVOY_FILTER_EXAMPLE_GITSHA="dfdc226d44d1b7c300e6e691e2e8ada98b045edb" ENVOY_FILTER_EXAMPLE_SRCDIR="${BUILD_DIR}/envoy-filter-example" # shellcheck disable=SC2034 @@ -16,7 +16,7 @@ ENVOY_FILTER_EXAMPLE_TESTS=( if [[ ! -d "${ENVOY_FILTER_EXAMPLE_SRCDIR}/.git" ]]; then rm -rf "${ENVOY_FILTER_EXAMPLE_SRCDIR}" - git clone https://github.com/qinggniq/envoy-filter-example.git "${ENVOY_FILTER_EXAMPLE_SRCDIR}" + git clone https://github.com/envoyproxy/envoy-filter-example.git "${ENVOY_FILTER_EXAMPLE_SRCDIR}" fi (cd "${ENVOY_FILTER_EXAMPLE_SRCDIR}" && git fetch origin && git checkout -f "${ENVOY_FILTER_EXAMPLE_GITSHA}")