original_ip: removing exceptions (#36311)

Risk Level: low Testing: updated tests Docs Changes: n/a Release Notes: n/a envoyproxy/envoy-mobile#176 Signed-off-by: Alyssa Wilk <[email protected]>
envoyproxy · Sep 27, 2024 · 0ffb420 · 0ffb420
1 parent f7d42a5
commit 0ffb420
Show file tree

Hide file tree

Showing 13 changed files with 33 additions and 23 deletions.
diff --git a/envoy/http/original_ip_detection.h b/envoy/http/original_ip_detection.h
@@ -74,12 +74,12 @@ class OriginalIPDetectionFactory : public Envoy::Config::TypedFactory {
   ~OriginalIPDetectionFactory() override = default;
 
   /**
-   * Creates a particular extension implementation.
+   * Creates a particular extension implementation or return an error status.
    *
    * @param config supplies the configuration for the original IP detection extension.
    * @return OriginalIPDetectionSharedPtr the extension instance.
    */
-  virtual OriginalIPDetectionSharedPtr
+  virtual absl::StatusOr<OriginalIPDetectionSharedPtr>
   createExtension(const Protobuf::Message& config,
                   Server::Configuration::FactoryContext& context) PURE;
 

diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc
@@ -517,12 +517,13 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
     }
 
     auto extension = factory->createExtension(extension_config.typed_config(), context_);
-    if (!extension) {
+    SET_AND_RETURN_IF_NOT_OK(extension.status(), creation_status);
+    if (!*extension) {
       creation_status = absl::InvalidArgumentError(fmt::format(
           "Original IP detection extension could not be created: '{}'", extension_config.name()));
       return;
     }
-    original_ip_detection_extensions_.push_back(extension);
+    original_ip_detection_extensions_.push_back(*extension);
   }
 
   const auto& header_mutation_extensions = config.early_header_mutation_extensions();

diff --git a/source/extensions/http/original_ip_detection/custom_header/config.cc b/source/extensions/http/original_ip_detection/custom_header/config.cc
@@ -13,7 +13,7 @@ namespace Http {
 namespace OriginalIPDetection {
 namespace CustomHeader {
 
-Envoy::Http::OriginalIPDetectionSharedPtr
+absl::StatusOr<Envoy::Http::OriginalIPDetectionSharedPtr>
 CustomHeaderIPDetectionFactory::createExtension(const Protobuf::Message& message,
                                                 Server::Configuration::FactoryContext& context) {
   auto mptr = Envoy::Config::Utility::translateAnyToFactoryConfig(

diff --git a/source/extensions/http/original_ip_detection/custom_header/config.h b/source/extensions/http/original_ip_detection/custom_header/config.h
@@ -18,7 +18,7 @@ namespace CustomHeader {
  */
 class CustomHeaderIPDetectionFactory : public Envoy::Http::OriginalIPDetectionFactory {
 public:
-  Envoy::Http::OriginalIPDetectionSharedPtr
+  absl::StatusOr<Envoy::Http::OriginalIPDetectionSharedPtr>
   createExtension(const Protobuf::Message& message,
                   Server::Configuration::FactoryContext& context) override;
 

diff --git a/source/extensions/http/original_ip_detection/xff/config.cc b/source/extensions/http/original_ip_detection/xff/config.cc
@@ -13,15 +13,15 @@ namespace Http {
 namespace OriginalIPDetection {
 namespace Xff {
 
-Envoy::Http::OriginalIPDetectionSharedPtr
+absl::StatusOr<Envoy::Http::OriginalIPDetectionSharedPtr>
 XffIPDetectionFactory::createExtension(const Protobuf::Message& message,
                                        Server::Configuration::FactoryContext& context) {
   auto mptr = Envoy::Config::Utility::translateAnyToFactoryConfig(
       dynamic_cast<const ProtobufWkt::Any&>(message), context.messageValidationVisitor(), *this);
   const auto& proto_config = MessageUtil::downcastAndValidate<
       const envoy::extensions::http::original_ip_detection::xff::v3::XffConfig&>(
       *mptr, context.messageValidationVisitor());
-  return std::make_shared<XffIPDetection>(proto_config);
+  return XffIPDetection::create(proto_config);
 }
 
 REGISTER_FACTORY(XffIPDetectionFactory, Envoy::Http::OriginalIPDetectionFactory);

diff --git a/source/extensions/http/original_ip_detection/xff/config.h b/source/extensions/http/original_ip_detection/xff/config.h
@@ -18,7 +18,7 @@ namespace Xff {
  */
 class XffIPDetectionFactory : public Envoy::Http::OriginalIPDetectionFactory {
 public:
-  Envoy::Http::OriginalIPDetectionSharedPtr
+  absl::StatusOr<Envoy::Http::OriginalIPDetectionSharedPtr>
   createExtension(const Protobuf::Message& message,
                   Server::Configuration::FactoryContext& context) override;
 

diff --git a/source/extensions/http/original_ip_detection/xff/xff.cc b/source/extensions/http/original_ip_detection/xff/xff.cc
@@ -9,13 +9,19 @@ namespace Http {
 namespace OriginalIPDetection {
 namespace Xff {
 
+absl::StatusOr<std::unique_ptr<XffIPDetection>> XffIPDetection::create(
+    const envoy::extensions::http::original_ip_detection::xff::v3::XffConfig& config) {
+
+  if (config.has_xff_trusted_cidrs() && config.xff_num_trusted_hops() > 0) {
+    return absl::InvalidArgumentError("Cannot set both xff_num_trusted_hops and xff_trusted_cidrs");
+  }
+  return std::unique_ptr<XffIPDetection>(new XffIPDetection(config));
+}
+
 XffIPDetection::XffIPDetection(
     const envoy::extensions::http::original_ip_detection::xff::v3::XffConfig& config)
     : xff_num_trusted_hops_(config.xff_num_trusted_hops()),
       skip_xff_append_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, skip_xff_append, true)) {
-  if (config.has_xff_trusted_cidrs() && config.xff_num_trusted_hops() > 0) {
-    throwEnvoyExceptionOrPanic("Cannot set both xff_num_trusted_hops and xff_trusted_cidrs");
-  }
   if (config.has_xff_trusted_cidrs()) {
     xff_trusted_cidrs_.reserve(config.xff_trusted_cidrs().cidrs().size());
     for (const envoy::config::core::v3::CidrRange& entry : config.xff_trusted_cidrs().cidrs()) {

diff --git a/source/extensions/http/original_ip_detection/xff/xff.h b/source/extensions/http/original_ip_detection/xff/xff.h
@@ -19,15 +19,19 @@ namespace Xff {
 class XffIPDetection : public Envoy::Http::OriginalIPDetection,
                        Logger::Loggable<Logger::Id::config> {
 public:
-  XffIPDetection(const envoy::extensions::http::original_ip_detection::xff::v3::XffConfig& config);
+  static absl::StatusOr<std::unique_ptr<XffIPDetection>>
+  create(const envoy::extensions::http::original_ip_detection::xff::v3::XffConfig& config);
+
   XffIPDetection(uint32_t xff_num_trusted_hops, bool skip_xff_append);
   XffIPDetection(const std::vector<Network::Address::CidrRange> xff_trusted_cidrs,
                  bool skip_xff_append);
 
   Envoy::Http::OriginalIPDetectionResult
   detect(Envoy::Http::OriginalIPDetectionParams& params) override;
 
-private:
+protected:
+  XffIPDetection(const envoy::extensions::http::original_ip_detection::xff::v3::XffConfig& config);
+
   const uint32_t xff_num_trusted_hops_;
   std::vector<Network::Address::CidrRange> xff_trusted_cidrs_;
   const bool skip_xff_append_;

diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/test/extensions/filters/network/http_connection_manager/config_test.cc
@@ -2586,7 +2586,7 @@ namespace {
 
 class OriginalIPDetectionExtensionNotCreatedFactory : public Http::OriginalIPDetectionFactory {
 public:
-  Http::OriginalIPDetectionSharedPtr
+  absl::StatusOr<Http::OriginalIPDetectionSharedPtr>
   createExtension(const Protobuf::Message&, Server::Configuration::FactoryContext&) override {
     return nullptr;
   }

diff --git a/test/extensions/http/original_ip_detection/custom_header/config_test.cc b/test/extensions/http/original_ip_detection/custom_header/config_test.cc
@@ -29,7 +29,7 @@ TEST(CustomHeaderFactoryTest, Basic) {
   TestUtility::loadFromYaml(yaml, typed_config);
 
   NiceMock<Server::Configuration::MockFactoryContext> context;
-  EXPECT_NE(factory->createExtension(typed_config.typed_config(), context), nullptr);
+  EXPECT_NE(*factory->createExtension(typed_config.typed_config(), context), nullptr);
 }
 
 TEST(CustomHeaderFactoryTest, InvalidHeaderName) {
@@ -47,7 +47,7 @@ TEST(CustomHeaderFactoryTest, InvalidHeaderName) {
   TestUtility::loadFromYaml(yaml, typed_config);
 
   NiceMock<Server::Configuration::MockFactoryContext> context;
-  EXPECT_THROW_WITH_REGEX(factory->createExtension(typed_config.typed_config(), context),
+  EXPECT_THROW_WITH_REGEX(*factory->createExtension(typed_config.typed_config(), context),
                           EnvoyException,
                           "Proto constraint validation failed.*does not match regex pattern.*");
 }

diff --git a/test/extensions/http/original_ip_detection/xff/config_test.cc b/test/extensions/http/original_ip_detection/xff/config_test.cc
@@ -29,7 +29,7 @@ TEST(CustomHeaderFactoryTest, Basic) {
   TestUtility::loadFromYaml(yaml, typed_config);
 
   NiceMock<Server::Configuration::MockFactoryContext> context;
-  EXPECT_NE(factory->createExtension(typed_config.typed_config(), context), nullptr);
+  EXPECT_NE(*factory->createExtension(typed_config.typed_config(), context), nullptr);
 }
 
 } // namespace Xff

diff --git a/test/extensions/http/original_ip_detection/xff/xff_test.cc b/test/extensions/http/original_ip_detection/xff/xff_test.cc
@@ -18,7 +18,7 @@ class XffNumTrustedHopsTest : public testing::Test {
   XffNumTrustedHopsTest() {
     envoy::extensions::http::original_ip_detection::xff::v3::XffConfig config;
     config.set_xff_num_trusted_hops(1);
-    xff_extension_ = std::make_shared<XffIPDetection>(config);
+    xff_extension_ = *XffIPDetection::create(config);
   }
 
   std::shared_ptr<XffIPDetection> xff_extension_;
@@ -60,7 +60,7 @@ class XffTrustedCidrsTest : public testing::Test {
     cidr3->set_address_prefix("2001:db8:7e57:1::");
     cidr3->mutable_prefix_len()->set_value(64);
     config.mutable_skip_xff_append()->set_value(false);
-    xff_extension_ = std::make_shared<XffIPDetection>(config);
+    xff_extension_ = *XffIPDetection::create(config);
   }
 
   std::shared_ptr<XffIPDetection> xff_extension_;
@@ -146,8 +146,8 @@ TEST(XffInvalidConfigTest, InvalidConfig) {
   cidr->set_address_prefix("192.0.2.0");
   cidr->mutable_prefix_len()->set_value(24);
 
-  EXPECT_THROW_WITH_MESSAGE(std::make_shared<XffIPDetection>(config), EnvoyException,
-                            "Cannot set both xff_num_trusted_hops and xff_trusted_cidrs");
+  EXPECT_EQ(XffIPDetection::create(config).status().message(),
+            "Cannot set both xff_num_trusted_hops and xff_trusted_cidrs");
 }
 
 } // namespace Xff

diff --git a/tools/code_format/config.yaml b/tools/code_format/config.yaml
@@ -221,7 +221,6 @@ paths:
     - source/extensions/http/custom_response
     - source/extensions/http/early_header_mutation
     - source/extensions/http/injected_credentials
-    - source/extensions/http/original_ip_detection
     - source/extensions/http/stateful_session
     - source/extensions/io_socket/user_space
     - source/extensions/key_value