diff --git a/ci/filter_example_setup.sh b/ci/filter_example_setup.sh index 4fef840e0389d..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="d135632a6e96268ab2c6a62e1cceec25c728ccf9" +ENVOY_FILTER_EXAMPLE_GITSHA="dfdc226d44d1b7c300e6e691e2e8ada98b045edb" ENVOY_FILTER_EXAMPLE_SRCDIR="${BUILD_DIR}/envoy-filter-example" # shellcheck disable=SC2034 diff --git a/docs/root/intro/arch_overview/http/http_filters.rst b/docs/root/intro/arch_overview/http/http_filters.rst index 0bca514dd95f0..1c1079c982a65 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::isTerminalFilterByProto(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 273a004a60f18..097c1616099ca 100644 --- a/include/envoy/filter/http/filter_config_provider.h +++ b/include/envoy/filter/http/filter_config_provider.h @@ -35,11 +35,15 @@ 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_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) PURE; + const std::string& stat_prefix, bool last_filter_in_filter_chain, + 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 42aa7db3a6bdc..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() { 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() { return false; } + virtual bool isTerminalFilterByProto(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 85fc1d6246ac8..8f5f74b000258 100644 --- a/source/common/filter/http/filter_config_discovery_impl.cc +++ b/source/common/filter/http/filter_config_discovery_impl.cc @@ -32,17 +32,25 @@ void validateTypeUrlHelper(const std::string& type_url, DynamicFilterConfigProviderImpl::DynamicFilterConfigProviderImpl( FilterConfigSubscriptionSharedPtr& subscription, const absl::flat_hash_set& require_type_urls, + Server::Configuration::FactoryContext& factory_context, - Envoy::Http::FilterFactoryCb default_config) + Envoy::Http::FilterFactoryCb default_config, bool last_filter_in_filter_chain, + const std::string& filter_chain_type) : subscription_(subscription), require_type_urls_(require_type_urls), default_configuration_(default_config ? absl::make_optional(default_config) : absl::nullopt), - 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_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(); }); } @@ -78,6 +86,13 @@ void DynamicFilterConfigProviderImpl::onConfigUpdate(Envoy::Http::FilterFactoryC }); } +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_filter_chain_); +} + void DynamicFilterConfigProviderImpl::onConfigRemoved( Config::ConfigAppliedCb applied_on_all_threads) { tls_.runOnAllThreads( @@ -156,9 +171,14 @@ void FilterConfigSubscription::onConfigUpdate( } ProtobufTypes::MessagePtr message = Config::Utility::translateAnyToFactoryConfig( filter_config.typed_config(), validator_, factory); + 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); + } Envoy::Http::FilterFactoryCb factory_callback = factory.createFilterFactoryFromProto(*message, stat_prefix_, factory_context_); ENVOY_LOG(debug, "Updating filter config {}", filter_config_name_); + Common::applyToAllWithCleanup( filter_config_providers_, [&factory_callback, &version_info](DynamicFilterConfigProviderImpl* provider, @@ -170,6 +190,8 @@ void FilterConfigSubscription::onConfigUpdate( last_config_ = factory_callback; last_type_url_ = type_url; last_version_info_ = version_info; + last_filter_name_ = factory.name(); + last_filter_is_terminal_ = is_terminal_filter; } void FilterConfigSubscription::onConfigUpdate( @@ -188,6 +210,8 @@ void FilterConfigSubscription::onConfigUpdate( last_config_hash_ = 0; last_config_ = absl::nullopt; last_type_url_ = ""; + last_filter_is_terminal_ = false; + last_filter_name_ = ""; } else if (!added_resources.empty()) { onConfigUpdate(added_resources, added_resources[0].get().version()); } @@ -235,7 +259,8 @@ std::shared_ptr FilterConfigProviderManagerImpl::getSu DynamicFilterConfigProviderPtr 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_filter_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. @@ -249,6 +274,7 @@ DynamicFilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFil auto factory_type_url = TypeUtil::typeUrlToDescriptorFullName(type_url); require_type_urls.emplace(factory_type_url); } + Envoy::Http::FilterFactoryCb default_config = nullptr; if (config_source.has_default_config()) { auto* default_factory = @@ -265,12 +291,17 @@ DynamicFilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFil ProtobufTypes::MessagePtr message = Config::Utility::translateAnyToFactoryConfig( config_source.default_config(), factory_context.messageValidationVisitor(), *default_factory); + Config::Utility::validateTerminalFilters( + filter_config_name, default_factory->name(), filter_chain_type, + default_factory->isTerminalFilterByProto(*message, factory_context), + last_filter_in_filter_config); default_config = default_factory->createFilterFactoryFromProto(*message, stat_prefix, factory_context); } auto provider = std::make_unique( - subscription, require_type_urls, factory_context, default_config); + subscription, require_type_urls, factory_context, default_config, + 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()) { @@ -286,6 +317,8 @@ DynamicFilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFil if (subscription->lastConfig().has_value()) { TRY_ASSERT_MAIN_THREAD { provider->validateTypeUrl(subscription->lastTypeUrl()); + provider->validateTerminalFilter(filter_config_name, subscription->lastFilterName(), + subscription->isLastFilterTerminal()); last_config_valid = true; } END_TRY catch (const EnvoyException& e) { diff --git a/source/common/filter/http/filter_config_discovery_impl.h b/source/common/filter/http/filter_config_discovery_impl.h index 05aed5646ff9d..5027bc863f5f5 100644 --- a/source/common/filter/http/filter_config_discovery_impl.h +++ b/source/common/filter/http/filter_config_discovery_impl.h @@ -37,11 +37,15 @@ class DynamicFilterConfigProviderImpl : public DynamicFilterConfigProvider { DynamicFilterConfigProviderImpl(FilterConfigSubscriptionSharedPtr& subscription, const absl::flat_hash_set& require_type_urls, Server::Configuration::FactoryContext& factory_context, - Envoy::Http::FilterFactoryCb default_config); + Envoy::Http::FilterFactoryCb default_config, + bool last_filter_in_filter_chain, + const std::string& filter_chain_type); + ~DynamicFilterConfigProviderImpl() override; void validateTypeUrl(const std::string& type_url) const; - + void validateTerminalFilter(const std::string& name, const std::string& filter_type, + bool is_terminal_filter); // Config::ExtensionConfigProvider const std::string& name() override; absl::optional config() override; @@ -74,6 +78,8 @@ class DynamicFilterConfigProviderImpl : public DynamicFilterConfigProvider { // case no warming is requested by any other filter config provider. Init::TargetImpl init_target_; + const bool last_filter_in_filter_chain_; + const std::string filter_chain_type_; friend class FilterConfigProviderManagerImpl; }; @@ -115,6 +121,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 std::string& lastFilterName() { return last_filter_name_; } + bool isLastFilterTerminal() { return last_filter_is_terminal_; } void incrementConflictCounter(); private: @@ -134,6 +142,8 @@ class FilterConfigSubscription absl::optional last_config_{absl::nullopt}; std::string last_type_url_; std::string last_version_info_; + std::string last_filter_name_; + bool last_filter_is_terminal_; Server::Configuration::FactoryContext& factory_context_; ProtobufMessage::ValidationVisitor& validator_; @@ -183,7 +193,8 @@ class FilterConfigProviderManagerImpl : public FilterConfigProviderManager, 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) override; + const std::string& stat_prefix, bool last_filter_in_filter_chain, + 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..35073f3d540ef 100644 --- a/source/extensions/filters/http/common/factory_base.h +++ b/source/extensions/filters/http/common/factory_base.h @@ -42,10 +42,21 @@ class FactoryBase : public Server::Configuration::NamedHttpFilterConfigFactory { std::string name() const override { return name_; } + 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: + virtual bool isTerminalFilterByProtoTyped(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..604c3423481bf 100644 --- a/source/extensions/filters/http/router/config.h +++ b/source/extensions/filters/http/router/config.h @@ -22,9 +22,11 @@ class RouterFilterConfig public: RouterFilterConfig() : FactoryBase(HttpFilterNames::get().Router) {} - bool isTerminalFilter() override { return true; } - private: + bool isTerminalFilterByProtoTyped(const envoy::extensions::filters::http::router::v3::Router&, + Server::Configuration::FactoryContext&) override { + return true; + } 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 ba5bcf52159ae..9e818369b4a8e 100644 --- a/source/extensions/filters/network/common/factory_base.h +++ b/source/extensions/filters/network/common/factory_base.h @@ -44,13 +44,22 @@ class FactoryBase : public Server::Configuration::NamedNetworkFilterConfigFactor std::string name() const override { return name_; } - bool isTerminalFilter() override { 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: FactoryBase(const std::string& name, bool is_terminal = false) : 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 1e8691234404a..ff680dc835714 100644 --- a/source/extensions/filters/network/direct_response/config.cc +++ b/source/extensions/filters/network/direct_response/config.cc @@ -32,7 +32,11 @@ class DirectResponseConfigFactory }; } - bool isTerminalFilter() override { return true; } + 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 179c14d2a3df7..119b1252d8931 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 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 03562a77568ea..4b6b0ba70c64a 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -505,8 +505,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:: @@ -524,7 +524,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.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( @@ -542,7 +542,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() && @@ -558,13 +558,11 @@ 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 d2c889a12aa98..f6685a644fb69 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -186,12 +186,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..81b44c3d36316 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -107,11 +107,12 @@ 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.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/common/filter/http/filter_config_discovery_impl_test.cc b/test/common/filter/http/filter_config_discovery_impl_test.cc index 70e7a36e80c0b..33cff50a28367 100644 --- a/test/common/filter/http/filter_config_discovery_impl_test.cc +++ b/test/common/filter/http/filter_config_discovery_impl_test.cc @@ -78,7 +78,9 @@ class FilterConfigDiscoveryImplTest : public FilterConfigDiscoveryTestBase { ~FilterConfigDiscoveryImplTest() override { factory_context_.thread_local_.shutdownThread(); } DynamicFilterConfigProviderPtr createProvider(std::string name, bool warm, - bool default_configuration) { + bool default_configuration, + bool last_filter_config = true) { + EXPECT_CALL(init_manager_, add(_)); envoy::config::core::v3::ExtensionConfigSource config_source; @@ -107,11 +109,11 @@ 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, 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) { @@ -376,6 +378,32 @@ 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(true, false, 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..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()); + 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 40ec2c38e4f98..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()); + EXPECT_TRUE(factory.isTerminalFilterByProto(proto_config, context_)); } TEST_F(HttpConnectionManagerConfigTest, BadHttpConnectionMangerConfig) { @@ -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/redis_proxy/config_test.cc b/test/extensions/filters/network/redis_proxy/config_test.cc index ce0276dfae1a6..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()); + 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()); + 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()); + 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()); + 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 efb3be1fb8faf..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()); + 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 9b3db4c4ebc8d..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()); + EXPECT_TRUE(factory_.isTerminalFilterByProto(config, context_)); Network::MockConnection connection; EXPECT_CALL(connection, addReadFilter(_)); diff --git a/test/integration/BUILD b/test/integration/BUILD index 96225d6c76d6b..499a76930b776 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -1097,6 +1097,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 8d8271a21f89c..d1a9b80cc3dfd 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, BasicFailTerminalFilterNotAtEndOfFilterChain) { + 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); + ASSERT_TRUE(response->waitForEndStream()); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("500", response->headers().getStatusValue()); +} + } // namespace } // namespace Envoy diff --git a/test/integration/filters/BUILD b/test/integration/filters/BUILD index 6d5782b380704..5d8c073a5bb76 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..bc693629451fb --- /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 terminal 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/tcp_conn_pool_integration_test.cc b/test/integration/tcp_conn_pool_integration_test.cc index a4cca156cb67c..bf02e97f5ccf0 100644 --- a/test/integration/tcp_conn_pool_integration_test.cc +++ b/test/integration/tcp_conn_pool_integration_test.cc @@ -101,7 +101,10 @@ 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 isTerminalFilterByProto(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..8ec76fe3a9615 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 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 805214bbccb14..fc370054d27a2 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -495,7 +495,10 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, NotTerminalLast) { filter_chains: - filters: - name: envoy.filters.network.tcp_proxy - typed_config: {} + 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"; @@ -539,7 +542,11 @@ class TestStatsConfigFactory : public Configuration::NamedNetworkFilterConfigFac } std::string name() const override { return "stats_test"; } - bool isTerminalFilter() override { return true; } + + bool isTerminalFilterByProto(const Protobuf::Message&, + Server::Configuration::FactoryContext&) override { + return true; + } private: Network::FilterFactoryCb commonFilterFactory(Configuration::FactoryContext& context) {