match delegate: making exception free

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
alyssawilk · Mar 27, 2024 · d84b964 · d84b964
1 parent f902ee6
commit d84b964
Show file tree

Hide file tree

Showing 4 changed files with 19 additions and 24 deletions.
diff --git a/source/common/http/match_delegate/config.cc b/source/common/http/match_delegate/config.cc
@@ -268,7 +268,7 @@ void DelegatingStreamFilter::setEncoderFilterCallbacks(
   encoder_filter_->setEncoderFilterCallbacks(callbacks);
 }
 
-Envoy::Http::FilterFactoryCb MatchDelegateConfig::createFilterFactoryFromProtoTyped(
+absl::StatusOr<Envoy::Http::FilterFactoryCb> MatchDelegateConfig::createFilterFactoryFromProtoTyped(
     const envoy::extensions::common::matching::v3::ExtensionWithMatcher& proto_config,
     const std::string& prefix, Server::Configuration::FactoryContext& context) {
 
@@ -280,9 +280,7 @@ Envoy::Http::FilterFactoryCb MatchDelegateConfig::createFilterFactoryFromProtoTy
   auto message = Config::Utility::translateAnyToFactoryConfig(
       proto_config.extension_config().typed_config(), context.messageValidationVisitor(), factory);
   auto filter_factory_or_error = factory.createFilterFactoryFromProto(*message, prefix, context);
-  if (!filter_factory_or_error.ok()) {
-    throwEnvoyExceptionOrPanic(std::string(filter_factory_or_error.status().message()));
-  }
+  RETURN_IF_STATUS_NOT_OK(filter_factory_or_error);
   auto filter_factory = filter_factory_or_error.value();
 
   Factory::MatchTreeValidationVisitor validation_visitor(*factory.matchingRequirements());
@@ -303,8 +301,8 @@ Envoy::Http::FilterFactoryCb MatchDelegateConfig::createFilterFactoryFromProtoTy
 
   if (!validation_visitor.errors().empty()) {
     // TODO(snowp): Output all violations.
-    throwEnvoyExceptionOrPanic(fmt::format("requirement violation while creating match tree: {}",
-                                           validation_visitor.errors()[0]));
+    return absl::InvalidArgumentError(fmt::format(
+        "requirement violation while creating match tree: {}", validation_visitor.errors()[0]));
   }
 
   Matcher::MatchTreeSharedPtr<Envoy::Http::HttpMatchingData> match_tree = nullptr;

diff --git a/source/common/http/match_delegate/config.h b/source/common/http/match_delegate/config.h
@@ -104,15 +104,15 @@ class DelegatingStreamFilter : public Logger::Loggable<Logger::Id::http>,
 };
 
 class MatchDelegateConfig
-    : public Extensions::HttpFilters::Common::FactoryBase<
+    : public Extensions::HttpFilters::Common::ExceptionFreeFactoryBase<
           envoy::extensions::common::matching::v3::ExtensionWithMatcher,
           envoy::extensions::common::matching::v3::ExtensionWithMatcherPerRoute> {
 public:
   // TODO(wbpcode): move this filter to 'source/extensions/filters/http'.
-  MatchDelegateConfig() : FactoryBase("envoy.filters.http.match_delegate") {}
+  MatchDelegateConfig() : ExceptionFreeFactoryBase("envoy.filters.http.match_delegate") {}
 
 private:
-  Envoy::Http::FilterFactoryCb createFilterFactoryFromProtoTyped(
+  absl::StatusOr<Envoy::Http::FilterFactoryCb> createFilterFactoryFromProtoTyped(
       const envoy::extensions::common::matching::v3::ExtensionWithMatcher& proto_config,
       const std::string&, Server::Configuration::FactoryContext& context) override;
 

diff --git a/test/common/http/match_delegate/config_test.cc b/test/common/http/match_delegate/config_test.cc
@@ -77,7 +77,7 @@ TEST(MatchWrapper, WithMatcher) {
 )EOF");
 
   MatchDelegateConfig match_delegate_config;
-  auto cb = match_delegate_config.createFilterFactoryFromProto(config, "", factory_context);
+  auto cb = match_delegate_config.createFilterFactoryFromProto(config, "", factory_context).value();
 
   Envoy::Http::MockFilterChainFactoryCallbacks factory_callbacks;
   testing::InSequence s;
@@ -96,7 +96,7 @@ TEST(MatchWrapper, WithMatcher) {
         EXPECT_NE(nullptr, dynamic_cast<DelegatingStreamFilter*>(filter.get()));
       }));
   EXPECT_CALL(factory_callbacks, addAccessLogHandler(testing::IsNull()));
-  cb.value()(factory_callbacks);
+  cb(factory_callbacks);
 }
 
 TEST(MatchWrapper, PerRouteConfig) {
@@ -161,7 +161,7 @@ TEST(MatchWrapper, DEPRECATED_FEATURE_TEST(WithDeprecatedMatcher)) {
 )EOF");
 
   MatchDelegateConfig match_delegate_config;
-  auto cb = match_delegate_config.createFilterFactoryFromProto(config, "", factory_context);
+  auto cb = match_delegate_config.createFilterFactoryFromProto(config, "", factory_context).value();
 
   Envoy::Http::MockFilterChainFactoryCallbacks factory_callbacks;
   testing::InSequence s;
@@ -180,7 +180,7 @@ TEST(MatchWrapper, DEPRECATED_FEATURE_TEST(WithDeprecatedMatcher)) {
         EXPECT_NE(nullptr, dynamic_cast<DelegatingStreamFilter*>(filter.get()));
       }));
   EXPECT_CALL(factory_callbacks, addAccessLogHandler(testing::IsNull()));
-  cb.value()(factory_callbacks);
+  cb(factory_callbacks);
 }
 
 TEST(MatchWrapper, QueryParamMatcherYaml) {
@@ -213,8 +213,8 @@ TEST(MatchWrapper, QueryParamMatcherYaml) {
 )EOF");
 
   MatchDelegateConfig match_delegate_config;
-  auto cb = match_delegate_config.createFilterFactoryFromProto(config, "", factory_context);
-  EXPECT_TRUE(cb.value());
+  auto cb = match_delegate_config.createFilterFactoryFromProto(config, "", factory_context).value();
+  EXPECT_TRUE(cb);
 }
 
 TEST(MatchWrapper, WithMatcherInvalidDataInput) {
@@ -247,14 +247,12 @@ TEST(MatchWrapper, WithMatcherInvalidDataInput) {
 )EOF");
 
   MatchDelegateConfig match_delegate_config;
-  EXPECT_THROW_WITH_REGEX(
-      match_delegate_config.createFilterFactoryFromProto(config, "", factory_context)
-          .status()
-          .IgnoreError(),
-      EnvoyException,
-      "requirement violation while creating match tree: INVALID_ARGUMENT: data input typeUrl "
-      "type.googleapis.com/envoy.type.matcher.v3.HttpResponseHeaderMatchInput not permitted "
-      "according to allowlist");
+  EXPECT_EQ(match_delegate_config.createFilterFactoryFromProto(config, "", factory_context)
+                .status()
+                .message(),
+            "requirement violation while creating match tree: INVALID_ARGUMENT: data input typeUrl "
+            "type.googleapis.com/envoy.type.matcher.v3.HttpResponseHeaderMatchInput not permitted "
+            "according to allowlist");
 }
 
 struct TestAction : Matcher::ActionBase<ProtobufWkt::StringValue> {};

diff --git a/tools/code_format/config.yaml b/tools/code_format/config.yaml
@@ -121,7 +121,6 @@ paths:
     - source/common/http/http2/codec_impl.cc
     - source/common/http/hash_policy.cc
     - source/common/http/conn_manager_utility.cc
-    - source/common/http/match_delegate/config.cc
     - source/common/protobuf/yaml_utility.cc
     - source/common/protobuf/visitor.cc
     - source/common/protobuf/utility.cc