exceptions: moving localAddress code over to statusOr (envoyproxy#30894)

Risk Level: low Testing: n/a Docs Changes: n/a Release Notes: n/a envoyproxy/envoy-mobile#176 Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
ramaraochavali · Nov 16, 2023 · 4f46697 · 4f46697
1 parent 2b7aaf0
commit 4f46697
Show file tree

Hide file tree

Showing 8 changed files with 27 additions and 18 deletions.
diff --git a/envoy/upstream/upstream.h b/envoy/upstream/upstream.h
@@ -99,7 +99,7 @@ class UpstreamLocalAddressSelectorFactory : public Config::TypedFactory {
    *   from cluster config. If the bind config from the cluster manager, the param
    *   is empty.
    */
-  virtual UpstreamLocalAddressSelectorConstSharedPtr
+  virtual absl::StatusOr<UpstreamLocalAddressSelectorConstSharedPtr>
   createLocalAddressSelector(std::vector<UpstreamLocalAddress> upstream_local_addresses,
                              absl::optional<std::string> cluster_name) const PURE;
 

diff --git a/source/common/grpc/google_grpc_utils.cc b/source/common/grpc/google_grpc_utils.cc
@@ -38,8 +38,8 @@ getGoogleGrpcChannelCredentials(const envoy::config::core::v3::GrpcService& grpc
         google_grpc_credentials_factory_name);
   }
   if (credentials_factory == nullptr) {
-    throwEnvoyExceptionOrPanic(absl::StrCat("Unknown google grpc credentials factory: ",
-                                            google_grpc_credentials_factory_name));
+    throw EnvoyException(absl::StrCat("Unknown google grpc credentials factory: ",
+                                      google_grpc_credentials_factory_name));
   }
   return credentials_factory->getChannelCredentials(grpc_service, api);
 }

diff --git a/source/common/upstream/default_local_address_selector_factory.cc b/source/common/upstream/default_local_address_selector_factory.cc
@@ -9,18 +9,19 @@ namespace {
 constexpr absl::string_view kDefaultLocalAddressSelectorName =
     "envoy.upstream.local_address_selector.default_local_address_selector";
 
-void validate(const std::vector<::Envoy::Upstream::UpstreamLocalAddress>& upstream_local_addresses,
-              absl::optional<std::string> cluster_name) {
+absl::Status
+validate(const std::vector<::Envoy::Upstream::UpstreamLocalAddress>& upstream_local_addresses,
+         absl::optional<std::string> cluster_name) {
 
   if (upstream_local_addresses.empty()) {
-    throwEnvoyExceptionOrPanic(
+    return absl::InvalidArgumentError(
         fmt::format("{}'s upstream binding config has no valid source address.",
                     !(cluster_name.has_value()) ? "Bootstrap"
                                                 : fmt::format("Cluster {}", cluster_name.value())));
   }
 
   if (upstream_local_addresses.size() > 2) {
-    throwEnvoyExceptionOrPanic(fmt::format(
+    return absl::InvalidArgumentError(fmt::format(
         "{}'s upstream binding config has more than one extra/additional source addresses. Only "
         "one extra/additional source can be supported in BindConfig's "
         "extra_source_addresses/additional_source_addresses field",
@@ -42,14 +43,15 @@ void validate(const std::vector<::Envoy::Upstream::UpstreamLocalAddress>& upstre
 
     if (upstream_local_addresses[0].address_->ip()->version() ==
         upstream_local_addresses[1].address_->ip()->version()) {
-      throwEnvoyExceptionOrPanic(fmt::format(
+      return absl::InvalidArgumentError(fmt::format(
           "{}'s upstream binding config has two same IP version source addresses. Only two "
           "different IP version source addresses can be supported in BindConfig's source_address "
           "and extra_source_addresses/additional_source_addresses fields",
           !(cluster_name.has_value()) ? "Bootstrap"
                                       : fmt::format("Cluster {}", cluster_name.value())));
     }
   }
+  return absl::OkStatus();
 }
 
 } // namespace
@@ -58,11 +60,14 @@ std::string DefaultUpstreamLocalAddressSelectorFactory::name() const {
   return std::string(kDefaultLocalAddressSelectorName);
 }
 
-UpstreamLocalAddressSelectorConstSharedPtr
+absl::StatusOr<UpstreamLocalAddressSelectorConstSharedPtr>
 DefaultUpstreamLocalAddressSelectorFactory::createLocalAddressSelector(
     std::vector<::Envoy::Upstream::UpstreamLocalAddress> upstream_local_addresses,
     absl::optional<std::string> cluster_name) const {
-  validate(upstream_local_addresses, cluster_name);
+  absl::Status status = validate(upstream_local_addresses, cluster_name);
+  if (!status.ok()) {
+    return status;
+  }
   return std::make_shared<DefaultUpstreamLocalAddressSelector>(std::move(upstream_local_addresses));
 }
 

diff --git a/source/common/upstream/default_local_address_selector_factory.h b/source/common/upstream/default_local_address_selector_factory.h
@@ -18,7 +18,7 @@ class DefaultUpstreamLocalAddressSelectorFactory : public UpstreamLocalAddressSe
 public:
   std::string name() const override;
 
-  UpstreamLocalAddressSelectorConstSharedPtr createLocalAddressSelector(
+  absl::StatusOr<UpstreamLocalAddressSelectorConstSharedPtr> createLocalAddressSelector(
       std::vector<::Envoy::Upstream::UpstreamLocalAddress> upstream_local_addresses,
       absl::optional<std::string> cluster_name) const override;
 

diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc
@@ -354,14 +354,16 @@ Envoy::Upstream::UpstreamLocalAddressSelectorConstSharedPtr createUpstreamLocalA
         Config::Utility::getAndCheckFactory<UpstreamLocalAddressSelectorFactory>(typed_extension,
                                                                                  false);
   }
-  return local_address_selector_factory->createLocalAddressSelector(
+  auto selector_or_error = local_address_selector_factory->createLocalAddressSelector(
       parseBindConfig(
           bind_config, cluster_name,
           buildBaseSocketOptions(cluster_config, bootstrap_bind_config.value_or(
                                                      envoy::config::core::v3::BindConfig{})),
           buildClusterSocketOptions(cluster_config, bootstrap_bind_config.value_or(
                                                         envoy::config::core::v3::BindConfig{}))),
       cluster_name);
+  THROW_IF_STATUS_NOT_OK(selector_or_error, throw);
+  return selector_or_error.value();
 }
 
 } // namespace

diff --git a/test/common/upstream/default_local_address_selector_test.cc b/test/common/upstream/default_local_address_selector_test.cc
@@ -18,9 +18,10 @@ namespace {
 TEST(ConfigTest, EmptyUpstreamAddresses) {
   DefaultUpstreamLocalAddressSelectorFactory factory;
   std::vector<UpstreamLocalAddress> upstream_local_addresses;
-  EXPECT_THROW_WITH_MESSAGE(
-      factory.createLocalAddressSelector(upstream_local_addresses, absl::nullopt), EnvoyException,
-      "Bootstrap's upstream binding config has no valid source address.");
+  EXPECT_EQ(factory.createLocalAddressSelector(upstream_local_addresses, absl::nullopt)
+                .status()
+                .message(),
+            "Bootstrap's upstream binding config has no valid source address.");
 }
 
 TEST(ConfigTest, NullUpstreamAddress) {
@@ -30,7 +31,7 @@ TEST(ConfigTest, NullUpstreamAddress) {
       UpstreamLocalAddress{nullptr, std::make_shared<Network::ConnectionSocket::Options>()});
   // This should be exception free.
   UpstreamLocalAddressSelectorConstSharedPtr selector =
-      factory.createLocalAddressSelector(upstream_local_addresses, absl::nullopt);
+      factory.createLocalAddressSelector(upstream_local_addresses, absl::nullopt).value();
 } // NOLINT(clang-analyzer-cplusplus.NewDeleteLeaks)
 
 } // namespace

diff --git a/test/common/upstream/test_local_address_selector.h b/test/common/upstream/test_local_address_selector.h
@@ -40,7 +40,7 @@ class TestUpstreamLocalAddressSelectorFactory : public UpstreamLocalAddressSelec
       bool return_empty_source_address = false)
       : num_calls_(num_calls), return_empty_source_address_{return_empty_source_address} {}
 
-  UpstreamLocalAddressSelectorConstSharedPtr createLocalAddressSelector(
+  absl::StatusOr<UpstreamLocalAddressSelectorConstSharedPtr> createLocalAddressSelector(
       std::vector<::Envoy::Upstream::UpstreamLocalAddress> upstream_local_addresses,
       absl::optional<std::string>) const override {
     return std::make_shared<TestUpstreamLocalAddressSelector>(upstream_local_addresses, num_calls_,

diff --git a/test/mocks/upstream/cluster_info.h b/test/mocks/upstream/cluster_info.h
@@ -85,7 +85,8 @@ class MockUpstreamLocalAddressSelector : public UpstreamLocalAddressSelector {
 
 class MockUpstreamLocalAddressSelectorFactory : public UpstreamLocalAddressSelectorFactory {
 public:
-  MOCK_METHOD(UpstreamLocalAddressSelectorConstSharedPtr, createLocalAddressSelector,
+  MOCK_METHOD(absl::StatusOr<UpstreamLocalAddressSelectorConstSharedPtr>,
+              createLocalAddressSelector,
               (std::vector<::Envoy::Upstream::UpstreamLocalAddress> upstream_local_addresses,
                absl::optional<std::string> cluster_name),
               (const));