From 34bdb945cb8a77630363659c24baf21e9aaf891f Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Wed, 31 Jul 2024 15:38:34 -0400 Subject: [PATCH 1/2] utility: removing some exceptions (#35414) Risk Level: low Testing: updated tests Docs Changes: n/a Release Notes: n/a https://github.com/envoyproxy/envoy-mobile/issues/176 Signed-off-by: Alyssa Wilk --- source/common/config/utility.h | 15 ++++++----- source/common/filter/config_discovery_impl.h | 26 +++++++++++-------- source/common/http/filter_chain_helper.h | 6 ++--- .../listener_manager/listener_manager_impl.cc | 4 +-- .../config_filter_chain_test.cc | 12 ++++----- 5 files changed, 35 insertions(+), 28 deletions(-) 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/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."); } From f7e5b594cff7de14a4cbea3e66a4be3786660686 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Wed, 31 Jul 2024 13:46:16 -0700 Subject: [PATCH 2/2] quic: fix test flake due to no socket interface (#35526) Fixes #35467 The failure is not during an important part of the test, but we are receiving some packet while there is no socket interface. To workaround this, do an atomic swap of the interface pointer so there is never a time when we don't have a socket interface. Signed-off-by: Greg Greenway --- .../common/singleton/threadsafe_singleton.h | 21 +++++++++---------- .../listener_manager_impl_test.cc | 6 ++++-- .../common/network/listen_socket_impl_test.cc | 4 ++-- test/integration/socket_interface_swap.cc | 9 +++----- test/integration/socket_interface_swap.h | 9 +------- 5 files changed, 20 insertions(+), 29 deletions(-) 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/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