diff --git a/source/common/config/utility.h b/source/common/config/utility.h index a3b83c808350..6c8e5a6ca8b5 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -418,21 +418,24 @@ class Utility { * @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. + * @return a status indicating if there is a mismatch between design and configuration. */ - static void validateTerminalFilters(const std::string& name, const std::string& filter_type, - const std::string& filter_chain_type, bool is_terminal_filter, - bool last_filter_in_current_config) { + static absl::Status validateTerminalFilters(const std::string& name, + const std::string& filter_type, + 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( + return absl::InvalidArgumentError( 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) { - ExceptionUtil::throwEnvoyException(fmt::format( + return absl::InvalidArgumentError(fmt::format( "Error: non-terminal filter named {} of type {} is the last filter in a {} filter chain.", name, filter_type, filter_chain_type)); } + return absl::OkStatus(); } /** diff --git a/source/common/filter/config_discovery_impl.h b/source/common/filter/config_discovery_impl.h index 652b6f54b8c5..1aaec8a0ab3d 100644 --- a/source/common/filter/config_discovery_impl.h +++ b/source/common/filter/config_discovery_impl.h @@ -213,8 +213,9 @@ class HttpDynamicFilterConfigProviderImpl auto* factory = Registry::FactoryRegistry::getFactory(factory_name); const bool is_terminal_filter = factory->isTerminalFilterByProto(message, server_context_); - Config::Utility::validateTerminalFilters(config_name, factory_name, filter_chain_type_, - is_terminal_filter, last_filter_in_filter_chain_); + THROW_IF_NOT_OK(Config::Utility::validateTerminalFilters(config_name, factory_name, + filter_chain_type_, is_terminal_filter, + last_filter_in_filter_chain_)); } private: @@ -279,9 +280,9 @@ class DownstreamNetworkDynamicFilterConfigProviderImpl Registry::FactoryRegistry::getFactory(factory_name); const bool is_terminal_filter = factory->isTerminalFilterByProto(message, this->server_context_); - Config::Utility::validateTerminalFilters(config_name, factory_name, this->filter_chain_type_, - is_terminal_filter, - this->last_filter_in_filter_chain_); + THROW_IF_NOT_OK(Config::Utility::validateTerminalFilters( + config_name, factory_name, this->filter_chain_type_, is_terminal_filter, + this->last_filter_in_filter_chain_)); } }; @@ -668,8 +669,9 @@ class HttpFilterConfigProviderManagerImpl void validateFilters(const std::string& filter_config_name, const std::string& filter_type, const std::string& filter_chain_type, bool is_terminal_filter, bool last_filter_in_filter_chain) const override { - Config::Utility::validateTerminalFilters(filter_config_name, filter_type, filter_chain_type, - is_terminal_filter, last_filter_in_filter_chain); + THROW_IF_NOT_OK(Config::Utility::validateTerminalFilters(filter_config_name, filter_type, + filter_chain_type, is_terminal_filter, + last_filter_in_filter_chain)); } const std::string getConfigDumpType() const override { return "ecds_filter_http"; } }; @@ -695,8 +697,9 @@ class UpstreamHttpFilterConfigProviderManagerImpl void validateFilters(const std::string& filter_config_name, const std::string& filter_type, const std::string& filter_chain_type, bool is_terminal_filter, bool last_filter_in_filter_chain) const override { - Config::Utility::validateTerminalFilters(filter_config_name, filter_type, filter_chain_type, - is_terminal_filter, last_filter_in_filter_chain); + THROW_IF_NOT_OK(Config::Utility::validateTerminalFilters(filter_config_name, filter_type, + filter_chain_type, is_terminal_filter, + last_filter_in_filter_chain)); } const std::string getConfigDumpType() const override { return "ecds_filter_upstream_http"; } }; @@ -722,8 +725,9 @@ class NetworkFilterConfigProviderManagerImpl void validateFilters(const std::string& filter_config_name, const std::string& filter_type, const std::string& filter_chain_type, bool is_terminal_filter, bool last_filter_in_filter_chain) const override { - Config::Utility::validateTerminalFilters(filter_config_name, filter_type, filter_chain_type, - is_terminal_filter, last_filter_in_filter_chain); + THROW_IF_NOT_OK(Config::Utility::validateTerminalFilters(filter_config_name, filter_type, + filter_chain_type, is_terminal_filter, + last_filter_in_filter_chain)); } const std::string getConfigDumpType() const override { return "ecds_filter_network"; } }; diff --git a/source/common/http/filter_chain_helper.h b/source/common/http/filter_chain_helper.h index 51852499317c..7b8df370b0d6 100644 --- a/source/common/http/filter_chain_helper.h +++ b/source/common/http/filter_chain_helper.h @@ -149,9 +149,9 @@ class FilterChainHelper : Logger::Loggable { Http::FilterFactoryCb callback = callback_or_error.value(); dependency_manager.registerFilter(factory->name(), *factory->dependencies()); const bool is_terminal = factory->isTerminalFilterByProto(*message, server_context_); - Config::Utility::validateTerminalFilters(proto_config.name(), factory->name(), - filter_chain_type, is_terminal, - last_filter_in_current_config); + RETURN_IF_NOT_OK(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( {factory->name(), callback}, proto_config.name()); #ifdef ENVOY_ENABLE_YAML diff --git a/source/common/listener_manager/listener_manager_impl.cc b/source/common/listener_manager/listener_manager_impl.cc index d838975acd76..f0841e119d3e 100644 --- a/source/common/listener_manager/listener_manager_impl.cc +++ b/source/common/listener_manager/listener_manager_impl.cc @@ -112,11 +112,11 @@ ProdListenerComponentFactory::createNetworkFilterFactoryListImpl( auto message = Config::Utility::translateToFactoryConfig( proto_config, filter_chain_factory_context.messageValidationVisitor(), factory); - Config::Utility::validateTerminalFilters( + RETURN_IF_NOT_OK(Config::Utility::validateTerminalFilters( filters[i].name(), factory.name(), "network", factory.isTerminalFilterByProto(*message, filter_chain_factory_context.serverFactoryContext()), - is_terminal); + is_terminal)); auto callback_or_error = factory.createFilterFactoryFromProto(*message, filter_chain_factory_context); RETURN_IF_NOT_OK(callback_or_error.status()); diff --git a/source/common/singleton/threadsafe_singleton.h b/source/common/singleton/threadsafe_singleton.h index 3af33deb2548..cb5de79254bf 100644 --- a/source/common/singleton/threadsafe_singleton.h +++ b/source/common/singleton/threadsafe_singleton.h @@ -66,6 +66,9 @@ template class InjectableSingleton { } static void clear() { loader_ = nullptr; } + // Atomically replace the value, returning the old value. + static T* replaceForTest(T* new_value) { return loader_.exchange(new_value); } + protected: static std::atomic loader_; }; @@ -86,20 +89,16 @@ template class ScopedInjectableLoader { std::unique_ptr instance_; }; -// This class saves the singleton object and restore the original singleton at destroy. This class -// is not thread safe. It can be used in single thread test. -template -class StackedScopedInjectableLoader : - // To access the protected loader_. - protected InjectableSingleton { +// This class saves the singleton object and restore the original singleton at destroy. +template class StackedScopedInjectableLoaderForTest { public: - explicit StackedScopedInjectableLoader(std::unique_ptr&& instance) { - original_loader_ = InjectableSingleton::getExisting(); - InjectableSingleton::clear(); + explicit StackedScopedInjectableLoaderForTest(std::unique_ptr&& instance) { instance_ = std::move(instance); - InjectableSingleton::initialize(instance_.get()); + original_loader_ = InjectableSingleton::replaceForTest(instance_.get()); + } + ~StackedScopedInjectableLoaderForTest() { + InjectableSingleton::replaceForTest(original_loader_); } - ~StackedScopedInjectableLoader() { InjectableSingleton::loader_ = original_loader_; } private: std::unique_ptr instance_; diff --git a/test/common/listener_manager/listener_manager_impl_test.cc b/test/common/listener_manager/listener_manager_impl_test.cc index bb81bb701d7a..d296966813b4 100644 --- a/test/common/listener_manager/listener_manager_impl_test.cc +++ b/test/common/listener_manager/listener_manager_impl_test.cc @@ -2604,7 +2604,8 @@ TEST_P(ListenerManagerImplTest, BindToPortEqualToFalse) { InSequence s; auto mock_interface = std::make_unique( std::vector{Network::Address::IpVersion::v4}); - StackedScopedInjectableLoader new_interface(std::move(mock_interface)); + StackedScopedInjectableLoaderForTest new_interface( + std::move(mock_interface)); ProdListenerComponentFactory real_listener_factory(server_); EXPECT_CALL(*worker_, start(_, _)); @@ -2643,7 +2644,8 @@ TEST_P(ListenerManagerImplTest, UpdateBindToPortEqualToFalse) { InSequence s; auto mock_interface = std::make_unique( std::vector{Network::Address::IpVersion::v4}); - StackedScopedInjectableLoader new_interface(std::move(mock_interface)); + StackedScopedInjectableLoaderForTest new_interface( + std::move(mock_interface)); ProdListenerComponentFactory real_listener_factory(server_); EXPECT_CALL(*worker_, start(_, _)); diff --git a/test/common/network/listen_socket_impl_test.cc b/test/common/network/listen_socket_impl_test.cc index 0bc6bd067bdc..769a62739177 100644 --- a/test/common/network/listen_socket_impl_test.cc +++ b/test/common/network/listen_socket_impl_test.cc @@ -229,7 +229,7 @@ TEST_P(ListenSocketImplTestTcp, SupportedIpFamilyVirtualSocketIsCreatedWithNoBsd auto any_address = version_ == Address::IpVersion::v4 ? Utility::getIpv4AnyAddress() : Utility::getIpv6AnyAddress(); - StackedScopedInjectableLoader new_interface(std::move(mock_interface)); + StackedScopedInjectableLoaderForTest new_interface(std::move(mock_interface)); { EXPECT_CALL(*mock_interface_ptr, socket(_, _, _)).Times(0); @@ -245,7 +245,7 @@ TEST_P(ListenSocketImplTestTcp, DeathAtUnSupportedIpFamilyListenSocket) { auto* mock_interface_ptr = mock_interface.get(); auto the_other_address = version_ == Address::IpVersion::v4 ? Utility::getIpv6AnyAddress() : Utility::getIpv4AnyAddress(); - StackedScopedInjectableLoader new_interface(std::move(mock_interface)); + StackedScopedInjectableLoaderForTest new_interface(std::move(mock_interface)); { EXPECT_CALL(*mock_interface_ptr, socket(_, _, _)).Times(0); EXPECT_CALL(*mock_interface_ptr, socket(_, _, _, _, _)).Times(0); diff --git a/test/extensions/filters/network/http_connection_manager/config_filter_chain_test.cc b/test/extensions/filters/network/http_connection_manager/config_filter_chain_test.cc index 96175f7e4aa4..9cc93b5749b9 100644 --- a/test/extensions/filters/network/http_connection_manager/config_filter_chain_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_filter_chain_test.cc @@ -335,12 +335,12 @@ TEST_F(FilterChainTest, CreateCustomUpgradeFilterChainWithRouterNotLast) { "\x1D" "encoder-decoder-buffer-filter"); - EXPECT_THROW_WITH_MESSAGE( - HttpConnectionManagerConfig(hcm_config, context_, date_provider_, - route_config_provider_manager_, - &scoped_routes_config_provider_manager_, tracer_manager_, - filter_config_provider_manager_, creation_status_), - EnvoyException, + HttpConnectionManagerConfig config(hcm_config, context_, date_provider_, + route_config_provider_manager_, + &scoped_routes_config_provider_manager_, tracer_manager_, + filter_config_provider_manager_, creation_status_); + EXPECT_EQ( + creation_status_.message(), "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."); } diff --git a/test/integration/socket_interface_swap.cc b/test/integration/socket_interface_swap.cc index 9ea29fa7ad3b..7d29f0691a47 100644 --- a/test/integration/socket_interface_swap.cc +++ b/test/integration/socket_interface_swap.cc @@ -3,10 +3,8 @@ namespace Envoy { SocketInterfaceSwap::SocketInterfaceSwap(Network::Socket::Type socket_type) - : write_matcher_(std::make_shared(socket_type)) { - Envoy::Network::SocketInterfaceSingleton::clear(); - test_socket_interface_loader_ = std::make_unique( - std::make_unique( + : write_matcher_(std::make_shared(socket_type)), + test_socket_interface_loader_(std::make_unique( [write_matcher = write_matcher_](Envoy::Network::TestIoSocketHandle* io_handle) -> absl::optional { Api::IoErrorPtr error_override = write_matcher->returnConnectOverride(io_handle); @@ -28,8 +26,7 @@ SocketInterfaceSwap::SocketInterfaceSwap(Network::Socket::Type socket_type) }, [write_matcher = write_matcher_](Network::IoHandle::RecvMsgOutput& output) { write_matcher->readOverride(output); - })); -} + })) {} void SocketInterfaceSwap::IoHandleMatcher::setResumeWrites() { absl::MutexLock lock(&mutex_); diff --git a/test/integration/socket_interface_swap.h b/test/integration/socket_interface_swap.h index 57e82087de8f..1e16aaaf6156 100644 --- a/test/integration/socket_interface_swap.h +++ b/test/integration/socket_interface_swap.h @@ -116,15 +116,8 @@ class SocketInterfaceSwap { explicit SocketInterfaceSwap(Network::Socket::Type socket_type); - ~SocketInterfaceSwap() { - test_socket_interface_loader_.reset(); - Envoy::Network::SocketInterfaceSingleton::initialize(previous_socket_interface_); - } - - Envoy::Network::SocketInterface* const previous_socket_interface_{ - Envoy::Network::SocketInterfaceSingleton::getExisting()}; std::shared_ptr write_matcher_; - std::unique_ptr test_socket_interface_loader_; + StackedScopedInjectableLoaderForTest test_socket_interface_loader_; }; } // namespace Envoy