api listener: removing exceptions

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<bool>
+  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

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<bool>
+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));
     }

mobile/library/common/extensions/listener_managers/api_listener_manager/api_listener_manager.h

@@ -19,8 +19,9 @@ class ApiListenerManagerImpl : public ListenerManager, Logger::Loggable<Logger::
   explicit ApiListenerManagerImpl(Instance& server);
 
   // Server::ListenerManager
-  bool addOrUpdateListener(const envoy::config::listener::v3::Listener& config,
-                           const std::string& version_info, bool added_via_api) override;
+  absl::StatusOr<bool> 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<std::reference_wrapper<Network::ListenerConfig>> listeners(ListenerState) override {

source/extensions/listener_managers/listener_manager/lds_api.cc

@@ -75,7 +75,12 @@ void LdsApiImpl::onConfigUpdate(const std::vector<Config::DecodedResourceRef>& a
         // applied.
         throw EnvoyException(fmt::format("duplicate listener {} found", listener.name()));
       }
-      if (listener_manager_.addOrUpdateListener(listener, resource.get().version(), true)) {
+      absl::StatusOr<bool> 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 {

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<bool>
+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<HttpApiListener>(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;

source/extensions/listener_managers/listener_manager/listener_manager_impl.h

@@ -192,8 +192,9 @@ class ListenerManagerImpl : public ListenerManager, Logger::Loggable<Logger::Id:
   void inPlaceFilterChainUpdate(ListenerImpl& listener);
 
   // Server::ListenerManager
-  bool addOrUpdateListener(const envoy::config::listener::v3::Listener& config,
-                           const std::string& version_info, bool added_via_api) override;
+  absl::StatusOr<bool> 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);

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<bool> 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);

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();

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) {

test/extensions/listener_managers/listener_manager/listener_manager_impl_test.h

@@ -348,7 +348,11 @@ class ListenerManagerImplTest : public testing::TestWithParam<bool> {
       )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,

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;
 

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<bool>, addOrUpdateListener,
               (const envoy::config::listener::v3::Listener& config, const std::string& version_info,
                bool modifiable));
   MOCK_METHOD(void, createLdsApi,