diff --git a/envoy/server/listener_manager.h b/envoy/server/listener_manager.h index b59c2a1ba7d8..37d4daff291e 100644 --- a/envoy/server/listener_manager.h +++ b/envoy/server/listener_manager.h @@ -166,11 +166,13 @@ class ListenerManager { * listener is not modifiable, future calls to this function or removeListener() on behalf * of this listener will return false. * @return TRUE if a listener was added or FALSE if the listener was not updated because it is - * a duplicate of the existing listener. This routine will throw an EnvoyException if - * there is a fundamental error preventing the listener from being added or updated. + * a duplicate of the existing listener. This routine will return + * absl::InvalidArgumentError if there is a fundamental error preventing the listener + * from being added or updated. */ - virtual bool addOrUpdateListener(const envoy::config::listener::v3::Listener& config, - const std::string& version_info, bool modifiable) PURE; + virtual absl::StatusOr + addOrUpdateListener(const envoy::config::listener::v3::Listener& config, + const std::string& version_info, bool modifiable) PURE; /** * Instruct the listener manager to create an LDS API provider. This is a separate operation diff --git a/mobile/library/common/extensions/listener_managers/api_listener_manager/api_listener_manager.cc b/mobile/library/common/extensions/listener_managers/api_listener_manager/api_listener_manager.cc index 2185b2dcca7a..e3170b9231be 100644 --- a/mobile/library/common/extensions/listener_managers/api_listener_manager/api_listener_manager.cc +++ b/mobile/library/common/extensions/listener_managers/api_listener_manager/api_listener_manager.cc @@ -12,8 +12,10 @@ namespace Envoy { namespace Server { ApiListenerManagerImpl::ApiListenerManagerImpl(Instance& server) : server_(server) {} -bool ApiListenerManagerImpl::addOrUpdateListener( - const envoy::config::listener::v3::Listener& config, const std::string&, bool added_via_api) { + +absl::StatusOr +ApiListenerManagerImpl::addOrUpdateListener(const envoy::config::listener::v3::Listener& config, + const std::string&, bool added_via_api) { ENVOY_LOG(debug, "Creating API listener manager"); std::string name; if (!config.name().empty()) { @@ -29,7 +31,7 @@ bool ApiListenerManagerImpl::addOrUpdateListener( // future allow multiple ApiListeners, and allow them to be created via LDS as well as bootstrap. if (config.has_api_listener()) { if (config.has_internal_listener()) { - throw EnvoyException(fmt::format( + return absl::InvalidArgumentError(fmt::format( "error adding listener named '{}': api_listener and internal_listener cannot be both set", name)); } diff --git a/mobile/library/common/extensions/listener_managers/api_listener_manager/api_listener_manager.h b/mobile/library/common/extensions/listener_managers/api_listener_manager/api_listener_manager.h index 57028b7a8e9b..0687d544e1ae 100644 --- a/mobile/library/common/extensions/listener_managers/api_listener_manager/api_listener_manager.h +++ b/mobile/library/common/extensions/listener_managers/api_listener_manager/api_listener_manager.h @@ -19,8 +19,9 @@ class ApiListenerManagerImpl : public ListenerManager, Logger::Loggable addOrUpdateListener(const envoy::config::listener::v3::Listener& config, + const std::string& version_info, + bool added_via_api) override; void createLdsApi(const envoy::config::core::v3::ConfigSource&, const xds::core::v3::ResourceLocator*) override {} std::vector> listeners(ListenerState) override { diff --git a/source/extensions/listener_managers/listener_manager/lds_api.cc b/source/extensions/listener_managers/listener_manager/lds_api.cc index 51f795e36d5b..8a1235317a6e 100644 --- a/source/extensions/listener_managers/listener_manager/lds_api.cc +++ b/source/extensions/listener_managers/listener_manager/lds_api.cc @@ -75,7 +75,12 @@ void LdsApiImpl::onConfigUpdate(const std::vector& a // applied. throw EnvoyException(fmt::format("duplicate listener {} found", listener.name())); } - if (listener_manager_.addOrUpdateListener(listener, resource.get().version(), true)) { + absl::StatusOr update_or_error = + listener_manager_.addOrUpdateListener(listener, resource.get().version(), true); + if (!update_or_error.status().ok()) { + throw EnvoyException(std::string(update_or_error.status().message())); + } + if (update_or_error.value()) { ENVOY_LOG(info, "lds: add/update listener '{}'", listener.name()); any_applied = true; } else { diff --git a/source/extensions/listener_managers/listener_manager/listener_manager_impl.cc b/source/extensions/listener_managers/listener_manager/listener_manager_impl.cc index 648ac1a129f5..17502b66af69 100644 --- a/source/extensions/listener_managers/listener_manager/listener_manager_impl.cc +++ b/source/extensions/listener_managers/listener_manager/listener_manager_impl.cc @@ -377,8 +377,9 @@ ListenerManagerStats ListenerManagerImpl::generateStats(Stats::Scope& scope) { return {ALL_LISTENER_MANAGER_STATS(POOL_COUNTER(scope), POOL_GAUGE(scope))}; } -bool ListenerManagerImpl::addOrUpdateListener(const envoy::config::listener::v3::Listener& config, - const std::string& version_info, bool added_via_api) { +absl::StatusOr +ListenerManagerImpl::addOrUpdateListener(const envoy::config::listener::v3::Listener& config, + const std::string& version_info, bool added_via_api) { std::string name; if (!config.name().empty()) { name = config.name(); @@ -393,13 +394,11 @@ bool ListenerManagerImpl::addOrUpdateListener(const envoy::config::listener::v3: // future allow multiple ApiListeners, and allow them to be created via LDS as well as bootstrap. if (config.has_api_listener()) { if (config.has_internal_listener()) { - throw EnvoyException(fmt::format( + return absl::InvalidArgumentError(fmt::format( "error adding listener named '{}': api_listener and internal_listener cannot be both set", name)); } if (!api_listener_ && !added_via_api) { - // TODO(junr03): dispatch to different concrete constructors when there are other - // ApiListenerImplBase derived classes. api_listener_ = std::make_unique(config, server_, config.name()); return true; } else { @@ -411,7 +410,7 @@ bool ListenerManagerImpl::addOrUpdateListener(const envoy::config::listener::v3: // Address field is not required for internal listeners. if (!config.has_internal_listener() && !config.has_address()) { - throw EnvoyException( + return absl::InvalidArgumentError( fmt::format("error adding listener named '{}': address is necessary", name)); } @@ -428,7 +427,7 @@ bool ListenerManagerImpl::addOrUpdateListener(const envoy::config::listener::v3: *(it->second->mutable_last_update_attempt())); it->second->set_details(e.what()); it->second->mutable_failed_configuration()->PackFrom(config); - throw e; + return absl::InvalidArgumentError(e.what()); } error_state_tracker_.erase(it); return false; diff --git a/source/extensions/listener_managers/listener_manager/listener_manager_impl.h b/source/extensions/listener_managers/listener_manager/listener_manager_impl.h index ef5483041aa5..2e6a943ac797 100644 --- a/source/extensions/listener_managers/listener_manager/listener_manager_impl.h +++ b/source/extensions/listener_managers/listener_manager/listener_manager_impl.h @@ -192,8 +192,9 @@ class ListenerManagerImpl : public ListenerManager, Logger::Loggable addOrUpdateListener(const envoy::config::listener::v3::Listener& config, + const std::string& version_info, + bool added_via_api) override; void createLdsApi(const envoy::config::core::v3::ConfigSource& lds_config, const xds::core::v3::ResourceLocator* lds_resources_locator) override { ASSERT(lds_api_ == nullptr); diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index 51e78065f718..86ce9a2bb461 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -101,7 +101,11 @@ void MainImpl::initialize(const envoy::config::bootstrap::v3::Bootstrap& bootstr ENVOY_LOG(info, "loading {} listener(s)", listeners.size()); for (ssize_t i = 0; i < listeners.size(); i++) { ENVOY_LOG(debug, "listener #{}:", i); - server.listenerManager().addOrUpdateListener(listeners[i], "", false); + absl::StatusOr update_or_error = + server.listenerManager().addOrUpdateListener(listeners[i], "", false); + if (!update_or_error.status().ok()) { + throw EnvoyException(std::string(update_or_error.status().message())); + } } initializeWatchdogs(bootstrap, server); diff --git a/test/extensions/filters/network/echo/echo_integration_test.cc b/test/extensions/filters/network/echo/echo_integration_test.cc index acb059de384b..b78704274100 100644 --- a/test/extensions/filters/network/echo/echo_integration_test.cc +++ b/test/extensions/filters/network/echo/echo_integration_test.cc @@ -66,8 +66,11 @@ name: new_listener test_server_->setOnWorkerListenerAddedCb( [&listener_added_by_worker]() -> void { listener_added_by_worker.setReady(); }); test_server_->server().dispatcher().post([this, json, &listener_added_by_manager]() -> void { - EXPECT_TRUE(test_server_->server().listenerManager().addOrUpdateListener( - Server::parseListenerFromV3Yaml(json), "", true)); + EXPECT_TRUE(test_server_->server() + .listenerManager() + .addOrUpdateListener(Server::parseListenerFromV3Yaml(json), "", true) + .status() + .ok()); listener_added_by_manager.setReady(); }); listener_added_by_worker.waitReady(); diff --git a/test/extensions/listener_managers/listener_manager/listener_manager_impl_test.cc b/test/extensions/listener_managers/listener_manager/listener_manager_impl_test.cc index 0bd781865563..fba98bb74a38 100644 --- a/test/extensions/listener_managers/listener_manager/listener_manager_impl_test.cc +++ b/test/extensions/listener_managers/listener_manager/listener_manager_impl_test.cc @@ -784,10 +784,10 @@ TEST_P(ListenerManagerImplTest, MultipleSocketTypeSpecifiedInAddresses) { - filters: [] )EOF"; - EXPECT_THROW_WITH_MESSAGE(manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml), "", true), - EnvoyException, - "listener foo: has different socket type. The listener only " - "support same socket type for all the addresses."); + EXPECT_EQ( + manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml), "", true).status().message(), + "listener foo: has different socket type. The listener only " + "support same socket type for all the addresses."); } TEST_P(ListenerManagerImplTest, RejectNoAddresses) { @@ -797,9 +797,9 @@ TEST_P(ListenerManagerImplTest, RejectNoAddresses) { exact_balance: {} )EOF"; - EXPECT_THROW_WITH_MESSAGE(manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml), "", true), - EnvoyException, - "error adding listener named 'foo': address is necessary"); + EXPECT_EQ( + manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml), "", true).status().message(), + "error adding listener named 'foo': address is necessary"); } TEST_P(ListenerManagerImplTest, RejectMutlipleInternalAddresses) { @@ -819,10 +819,10 @@ TEST_P(ListenerManagerImplTest, RejectMutlipleInternalAddresses) { - filters: [] )EOF"; - EXPECT_THROW_WITH_MESSAGE(manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml), "", true), - EnvoyException, - "error adding listener named 'foo': use internal_listener field " - "instead of address for internal listeners"); + EXPECT_EQ( + manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml), "", true).status().message(), + "error adding listener named 'foo': use internal_listener field " + "instead of address for internal listeners"); const std::string yaml2 = R"EOF( name: "foo" @@ -842,10 +842,10 @@ TEST_P(ListenerManagerImplTest, RejectMutlipleInternalAddresses) { - filters: [] )EOF"; - EXPECT_THROW_WITH_MESSAGE(manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml2), "", true), - EnvoyException, - "error adding listener named 'foo': use internal_listener field " - "instead of address for internal listeners"); + EXPECT_EQ( + manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml2), "", true).status().message(), + "error adding listener named 'foo': use internal_listener field " + "instead of address for internal listeners"); const std::string yaml3 = R"EOF( name: "foo" @@ -862,10 +862,10 @@ TEST_P(ListenerManagerImplTest, RejectMutlipleInternalAddresses) { - filters: [] )EOF"; - EXPECT_THROW_WITH_MESSAGE(manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml3), "", true), - EnvoyException, - "error adding listener named 'foo': use internal_listener field " - "instead of address for internal listeners"); + EXPECT_EQ( + manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml3), "", true).status().message(), + "error adding listener named 'foo': use internal_listener field " + "instead of address for internal listeners"); } TEST_P(ListenerManagerImplTest, RejectIpv4CompatOnIpv4Address) { diff --git a/test/extensions/listener_managers/listener_manager/listener_manager_impl_test.h b/test/extensions/listener_managers/listener_manager/listener_manager_impl_test.h index df3cbccc49b8..68e3c967ecde 100644 --- a/test/extensions/listener_managers/listener_manager/listener_manager_impl_test.h +++ b/test/extensions/listener_managers/listener_manager/listener_manager_impl_test.h @@ -348,7 +348,11 @@ class ListenerManagerImplTest : public testing::TestWithParam { )EOF"; TestUtility::loadFromYaml(filter_chain_matcher, *listener.mutable_filter_chain_matcher()); } - return manager_->addOrUpdateListener(listener, version_info, added_via_api); + auto status_or_error = manager_->addOrUpdateListener(listener, version_info, added_via_api); + if (status_or_error.status().ok()) { + return status_or_error.value(); + } + throw EnvoyException(std::string(status_or_error.status().message())); } void testListenerUpdateWithSocketOptionsChange(const std::string& origin, diff --git a/test/mocks/server/listener_manager.cc b/test/mocks/server/listener_manager.cc index 0448ff4e7122..8c0069fcc611 100644 --- a/test/mocks/server/listener_manager.cc +++ b/test/mocks/server/listener_manager.cc @@ -5,10 +5,15 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" +using testing::_; +using testing::Return; + namespace Envoy { namespace Server { -MockListenerManager::MockListenerManager() = default; +MockListenerManager::MockListenerManager() { + ON_CALL(*this, addOrUpdateListener(_, _, _)).WillByDefault(Return(false)); +} MockListenerManager::~MockListenerManager() = default; diff --git a/test/mocks/server/listener_manager.h b/test/mocks/server/listener_manager.h index c7c855508f6d..9fc49154ba7c 100644 --- a/test/mocks/server/listener_manager.h +++ b/test/mocks/server/listener_manager.h @@ -11,7 +11,7 @@ class MockListenerManager : public ListenerManager { MockListenerManager(); ~MockListenerManager() override; - MOCK_METHOD(bool, addOrUpdateListener, + MOCK_METHOD(absl::StatusOr, addOrUpdateListener, (const envoy::config::listener::v3::Listener& config, const std::string& version_info, bool modifiable)); MOCK_METHOD(void, createLdsApi,