diff --git a/source/common/config/utility.h b/source/common/config/utility.h index ebf4dedb843cc..108e14e587c4e 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -324,20 +324,23 @@ class Utility { /** * Verify that any filter designed to be terminal is configured to be terminal, and vice versa. * @param name the name of the filter. - * @param name the type of filter. + * @param filter_type the type of filter. + * @param filter_chain_type the type of filter chain. * @param is_terminal_filter true if the filter is designed to be terminal. * @param last_filter_in_current_config true if the filter is last in the configuration. * @throws EnvoyException if there is a mismatch between design and configuration. */ - static void validateTerminalFilters(const std::string& name, const char* filter_type, - bool is_terminal_filter, bool last_filter_in_current_config) { + static void validateTerminalFilters(const std::string& name, const std::string& filter_type, + const char* filter_chain_type, bool is_terminal_filter, + bool last_filter_in_current_config) { if (is_terminal_filter && !last_filter_in_current_config) { - throw EnvoyException( - fmt::format("Error: {} must be the terminal {} filter.", name, filter_type)); + throw EnvoyException(fmt::format("Error: terminal filter named {} of type {} must be the " + "last filter in a {} filter chain.", + name, filter_type, filter_chain_type)); } else if (!is_terminal_filter && last_filter_in_current_config) { - throw EnvoyException( - fmt::format("Error: non-terminal filter {} is the last filter in a {} filter chain.", - name, filter_type)); + throw EnvoyException(fmt::format( + "Error: non-terminal filter named {} of type {} is the last filter in a {} filter chain.", + name, filter_type, filter_chain_type)); } } diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 830a18c655ab7..1c9aadaa53afc 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -395,10 +395,7 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( const auto& filters = config.http_filters(); for (int32_t i = 0; i < filters.size(); i++) { - bool is_terminal = false; - processFilter(filters[i], i, "http", filter_factories_, is_terminal); - Config::Utility::validateTerminalFilters(filters[i].name(), "http", is_terminal, - i == filters.size() - 1); + processFilter(filters[i], i, "http", filter_factories_, "http", i == filters.size() - 1); } for (const auto& upgrade_config : config.upgrade_configs()) { @@ -412,11 +409,8 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( if (!upgrade_config.filters().empty()) { std::unique_ptr factories = std::make_unique(); for (int32_t j = 0; j < upgrade_config.filters().size(); j++) { - bool is_terminal = false; - processFilter(upgrade_config.filters(j), j, name, *factories, is_terminal); - Config::Utility::validateTerminalFilters(upgrade_config.filters(j).name(), "http upgrade", - is_terminal, - j == upgrade_config.filters().size() - 1); + processFilter(upgrade_config.filters(j), j, name, *factories, "http upgrade", + j == upgrade_config.filters().size() - 1); } upgrade_filter_factories_.emplace( std::make_pair(name, FilterConfig{std::move(factories), enabled})); @@ -432,7 +426,7 @@ void HttpConnectionManagerConfig::processFilter( const envoy::extensions::filters::network::http_connection_manager::v3::HttpFilter& proto_config, int i, absl::string_view prefix, std::list& filter_factories, - bool& is_terminal) { + const char* filter_chain_type, bool last_filter_in_current_config) { ENVOY_LOG(debug, " {} filter #{}", prefix, i); ENVOY_LOG(debug, " name: {}", proto_config.name()); ENVOY_LOG(debug, " config: {}", @@ -451,7 +445,9 @@ void HttpConnectionManagerConfig::processFilter( proto_config, context_.messageValidationVisitor(), factory); Http::FilterFactoryCb callback = factory.createFilterFactoryFromProto(*message, stats_prefix_, context_); - is_terminal = factory.isTerminalFilter(); + bool is_terminal = factory.isTerminalFilter(); + Config::Utility::validateTerminalFilters(proto_config.name(), factory.name(), filter_chain_type, + is_terminal, last_filter_in_current_config); filter_factories.push_back(callback); } diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 0ebd0ee920254..287ab8c11f9f3 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -159,7 +159,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, processFilter(const envoy::extensions::filters::network::http_connection_manager::v3::HttpFilter& proto_config, int i, absl::string_view prefix, FilterFactoriesList& filter_factories, - bool& is_terminal); + const char* filter_chain_type, bool last_filter_in_current_config); /** * Determines what tracing provider to use for a given diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 5099bc1d7c803..fb9f351329785 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -104,7 +104,7 @@ std::vector ProdListenerComponentFactory::createNetwor Config::Utility::getAndCheckFactory( proto_config); - Config::Utility::validateTerminalFilters(filters[i].name(), "network", + Config::Utility::validateTerminalFilters(filters[i].name(), factory.name(), "network", factory.isTerminalFilter(), i == filters.size() - 1); auto message = Config::Utility::translateToFactoryConfig( 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 f5b78c49ce4fa..b492598c14d79 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -111,7 +111,9 @@ stat_prefix: router HttpConnectionManagerConfig(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, date_provider_, route_config_provider_manager_, scoped_routes_config_provider_manager_, http_tracer_manager_), - EnvoyException, "Error: envoy.filters.http.router must be the terminal http filter."); + EnvoyException, + "Error: terminal filter named envoy.filters.http.router of type envoy.filters.http.router " + "must be the last filter in a http filter chain."); } TEST_F(HttpConnectionManagerConfigTest, NonTerminalFilter) { @@ -141,7 +143,9 @@ stat_prefix: router date_provider_, route_config_provider_manager_, scoped_routes_config_provider_manager_, http_tracer_manager_), EnvoyException, - "Error: non-terminal filter health_check is the last filter in a http filter chain."); + "Error: non-terminal filter named health_check of type " + "envoy.filters.http.health_check is the last filter in a http filter " + "chain."); } TEST_F(HttpConnectionManagerConfigTest, MiscConfig) { @@ -1335,7 +1339,9 @@ TEST_F(FilterChainTest, CreateCustomUpgradeFilterChainWithRouterNotLast) { HttpConnectionManagerConfig(hcm_config, context_, date_provider_, route_config_provider_manager_, scoped_routes_config_provider_manager_, http_tracer_manager_), - EnvoyException, "Error: envoy.filters.http.router must be the terminal http upgrade filter."); + EnvoyException, + "Error: terminal filter named envoy.filters.http.router of type envoy.filters.http.router " + "must be the last filter in a http upgrade filter chain."); } TEST_F(FilterChainTest, InvalidConfig) { diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index 82bdacad202de..9dd43cabf7621 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -377,10 +377,10 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, TerminalNotLast) { config: {} )EOF"; - EXPECT_THROW_WITH_REGEX(manager_->addOrUpdateListener(parseListenerFromV2Yaml(yaml), "", true), - EnvoyException, - "Error: non-terminal filter non_terminal is the last " - "filter in a network filter chain."); + EXPECT_THROW_WITH_REGEX( + manager_->addOrUpdateListener(parseListenerFromV2Yaml(yaml), "", true), EnvoyException, + "Error: non-terminal filter named non_terminal of type non_terminal is the last " + "filter in a network filter chain."); } TEST_F(ListenerManagerImplWithRealFiltersTest, NotTerminalLast) { @@ -399,7 +399,8 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, NotTerminalLast) { EXPECT_THROW_WITH_REGEX( manager_->addOrUpdateListener(parseListenerFromV2Yaml(yaml), "", true), EnvoyException, - "Error: envoy.filters.network.tcp_proxy must be the terminal network filter."); + "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) {