http: migrate ext proc/... to exception free

source/extensions/filters/http/credential_injector/config.cc

@@ -60,12 +60,8 @@ CredentialInjectorFilterFactory::createHttpFilterFactoryFromProtoTyped(
     const envoy::extensions::filters::http::credential_injector::v3::CredentialInjector&
         proto_config,
     const std::string& stats_prefix, Server::Configuration::ServerFactoryContext& context) {
-  auto result = createFilterFactoryFromProtoHelper(proto_config, stats_prefix, context,
-                                                   context.scope(), context.initManager());
-  if (!result.ok()) {
-    ExceptionUtil::throwEnvoyException(std::string(result.status().message()));
-  }
-  return std::move(result.value());
+  return createFilterFactoryFromProtoHelper(proto_config, stats_prefix, context, context.scope(),
+                                            context.initManager());
 }
 
 REGISTER_FACTORY(CredentialInjectorFilterFactory,

source/extensions/filters/http/dynamic_modules/factory.cc

@@ -150,9 +150,7 @@ absl::StatusOr<Envoy::Http::FilterFactoryCb>
 DynamicModuleConfigFactory::createHttpFilterFactoryFromProtoTyped(
     const FilterConfig& proto_config, const std::string& stat_prefix,
     Server::Configuration::ServerFactoryContext& context) {
-  auto cb_or_error = createFilterFactory(proto_config, stat_prefix, context, context.scope());
-  THROW_IF_NOT_OK_REF(cb_or_error.status());
-  return cb_or_error.value();
+  return createFilterFactory(proto_config, stat_prefix, context, context.scope());
 }
 
 absl::StatusOr<Router::RouteSpecificFilterConfigConstSharedPtr>

source/extensions/filters/http/ext_proc/config.cc

@@ -88,10 +88,13 @@ ExternalProcessingFilterConfig::createFilterFactoryFromProtoTyped(
       PROTOBUF_GET_MS_OR_DEFAULT(proto_config, message_timeout, DefaultMessageTimeoutMs);
   const uint32_t max_message_timeout_ms =
       PROTOBUF_GET_MS_OR_DEFAULT(proto_config, max_message_timeout, DefaultMaxMessageTimeoutMs);
+  absl::Status config_creation_status = absl::OkStatus();
   auto filter_config = std::make_shared<FilterConfig>(
       proto_config, std::chrono::milliseconds(message_timeout_ms), max_message_timeout_ms,
       dual_info.scope, stats_prefix, dual_info.is_upstream,
-      Envoy::Extensions::Filters::Common::Expr::getBuilder(context), context);
+      Envoy::Extensions::Filters::Common::Expr::getBuilder(context), context,
+      config_creation_status);
+  RETURN_IF_NOT_OK_REF(config_creation_status);
   if (proto_config.has_grpc_service()) {
     return [filter_config = std::move(filter_config), &context,
             dual_info](Http::FilterChainFactoryCallbacks& callbacks) {
@@ -131,19 +134,19 @@ ExternalProcessingFilterConfig::createHttpFilterFactoryFromProtoTyped(
     const envoy::extensions::filters::http::ext_proc::v3::ExternalProcessor& proto_config,
     const std::string& stats_prefix, Server::Configuration::ServerFactoryContext& server_context) {
   // Verify configuration before creating FilterConfig
-  absl::Status result = verifyFilterConfig(proto_config);
-  if (!result.ok()) {
-    throw EnvoyException(std::string(result.message()));
-  }
+  RETURN_IF_NOT_OK(verifyFilterConfig(proto_config));
 
   const uint32_t message_timeout_ms =
       PROTOBUF_GET_MS_OR_DEFAULT(proto_config, message_timeout, DefaultMessageTimeoutMs);
   const uint32_t max_message_timeout_ms =
       PROTOBUF_GET_MS_OR_DEFAULT(proto_config, max_message_timeout, DefaultMaxMessageTimeoutMs);
+  absl::Status config_creation_status = absl::OkStatus();
   auto filter_config = std::make_shared<FilterConfig>(
       proto_config, std::chrono::milliseconds(message_timeout_ms), max_message_timeout_ms,
       server_context.scope(), stats_prefix, false,
-      Envoy::Extensions::Filters::Common::Expr::getBuilder(server_context), server_context);
+      Envoy::Extensions::Filters::Common::Expr::getBuilder(server_context), server_context,
+      config_creation_status);
+  RETURN_IF_NOT_OK_REF(config_creation_status);
 
   if (proto_config.has_grpc_service()) {
     return [filter_config = std::move(filter_config),

source/extensions/filters/http/ext_proc/ext_proc.cc

@@ -257,7 +257,8 @@ FilterConfig::FilterConfig(const ExternalProcessor& config,
                            const uint32_t max_message_timeout_ms, Stats::Scope& scope,
                            const std::string& stats_prefix, bool is_upstream,
                            Extensions::Filters::Common::Expr::BuilderInstanceSharedConstPtr builder,
-                           Server::Configuration::CommonFactoryContext& context)
+                           Server::Configuration::CommonFactoryContext& context,
+                           absl::Status& creation_status)
     : stats_(generateStats(stats_prefix, config.stat_prefix(), scope)),
       untyped_forwarding_namespaces_(
           config.metadata_options().forwarding_namespaces().untyped().begin(),
@@ -281,7 +282,7 @@ FilterConfig::FilterConfig(const ExternalProcessor& config,
       mutation_checker_(config.mutation_rules(), context.regexEngine()),
       filter_metadata_(config.filter_metadata()),
       expression_manager_(builder, context.localInfo(), config.request_attributes(),
-                          config.response_attributes()),
+                          config.response_attributes(), creation_status),
       processing_request_modifier_factory_cb_(
           createProcessingRequestModifierCb(config, builder, context)),
       on_processing_response_factory_cb_(

source/extensions/filters/http/ext_proc/ext_proc.h

@@ -248,7 +248,7 @@ class FilterConfig {
                const uint32_t max_message_timeout_ms, Stats::Scope& scope,
                const std::string& stats_prefix, bool is_upstream,
                Extensions::Filters::Common::Expr::BuilderInstanceSharedConstPtr builder,
-               Server::Configuration::CommonFactoryContext& context);
+               Server::Configuration::CommonFactoryContext& context, absl::Status& creation_status);
 
   bool failureModeAllow() const { return failure_mode_allow_; }
 

source/extensions/filters/http/ext_proc/matching_utils.cc

@@ -12,27 +12,33 @@ namespace HttpFilters {
 namespace ExternalProcessing {
 
 absl::flat_hash_map<std::string, ExpressionManager::CelExpression>
-ExpressionManager::initExpressions(const Protobuf::RepeatedPtrField<std::string>& matchers) {
+ExpressionManager::initExpressions(const Protobuf::RepeatedPtrField<std::string>& matchers,
+                                   absl::Status& creation_status) {
   absl::flat_hash_map<std::string, ExpressionManager::CelExpression> expressions;
 #if defined(USE_CEL_PARSER)
   for (const auto& matcher : matchers) {
+    if (!creation_status.ok()) {
+      return expressions;
+    }
     if (expressions.contains(matcher)) {
       continue;
     }
     const absl::StatusOr<cel::expr::ParsedExpr> parse_status =
         google::api::expr::parser::Parse(matcher);
     if (!parse_status.ok()) {
-      throw EnvoyException("Unable to parse descriptor expression: " +
-                           parse_status.status().ToString());
+      creation_status = absl::InvalidArgumentError("Unable to parse descriptor expression: " +
+                                                   parse_status.status().ToString());
+      return expressions;
     }
 
     const auto& parsed_expr = parse_status.value();
     const cel::expr::Expr& cel_expr = parsed_expr.expr();
     auto compiled_expression =
         Extensions::Filters::Common::Expr::CompiledExpression::Create(builder_, cel_expr);
     if (!compiled_expression.ok()) {
-      throw EnvoyException(
+      creation_status = absl::InvalidArgumentError(
           absl::StrCat("failed to create an expression: ", compiled_expression.status().message()));
+      return expressions;
     }
     expressions.emplace(matcher, std::move(compiled_expression.value()));
   }

source/extensions/filters/http/ext_proc/matching_utils.h

@@ -21,10 +21,11 @@ class ExpressionManager : public Logger::Loggable<Logger::Id::ext_proc> {
   ExpressionManager(Extensions::Filters::Common::Expr::BuilderInstanceSharedConstPtr builder,
                     const LocalInfo::LocalInfo& local_info,
                     const Protobuf::RepeatedPtrField<std::string>& request_matchers,
-                    const Protobuf::RepeatedPtrField<std::string>& response_matchers)
+                    const Protobuf::RepeatedPtrField<std::string>& response_matchers,
+                    absl::Status& creation_status)
       : builder_(builder), local_info_(local_info),
-        request_expr_(initExpressions(request_matchers)),
-        response_expr_(initExpressions(response_matchers)) {};
+        request_expr_(initExpressions(request_matchers, creation_status)),
+        response_expr_(initExpressions(response_matchers, creation_status)) {};
 
   bool hasRequestExpr() const { return !request_expr_.empty(); };
 
@@ -48,7 +49,8 @@ class ExpressionManager : public Logger::Loggable<Logger::Id::ext_proc> {
 
 private:
   absl::flat_hash_map<std::string, CelExpression>
-  initExpressions(const Protobuf::RepeatedPtrField<std::string>& matchers);
+  initExpressions(const Protobuf::RepeatedPtrField<std::string>& matchers,
+                  absl::Status& creation_status);
 
   const Extensions::Filters::Common::Expr::BuilderInstanceSharedConstPtr builder_;
   const LocalInfo::LocalInfo& local_info_;

source/extensions/filters/http/header_mutation/config.cc

@@ -28,9 +28,7 @@ HeaderMutationFactoryConfig::createHttpFilterFactoryFromProtoTyped(
     Server::Configuration::ServerFactoryContext& context) {
   absl::Status creation_status = absl::OkStatus();
   auto filter_config = std::make_shared<HeaderMutationConfig>(config, context, creation_status);
-  if (!creation_status.ok()) {
-    ExceptionUtil::throwEnvoyException(std::string(creation_status.message()));
-  }
+  RETURN_IF_NOT_OK_REF(creation_status);
 
   return [filter_config](Http::FilterChainFactoryCallbacks& callbacks) -> void {
     callbacks.addStreamFilter(std::make_shared<HeaderMutation>(filter_config));

source/extensions/http/ext_proc/processing_request_modifiers/mapped_attribute_builder/mapped_attribute_builder.cc

@@ -26,11 +26,12 @@ MappedAttributeBuilder::MappedAttributeBuilder(
     const envoy::extensions::http::ext_proc::processing_request_modifiers::
         mapped_attribute_builder::v3::MappedAttributeBuilder& config,
     Extensions::Filters::Common::Expr::BuilderInstanceSharedConstPtr expr_builder,
-    Server::Configuration::CommonFactoryContext& context)
+    Server::Configuration::CommonFactoryContext& context, absl::Status& creation_status)
     : config_(config),
       expression_manager_(expr_builder, context.localInfo(),
                           protoMapValuesToUniqueVector(config.mapped_request_attributes()),
-                          protoMapValuesToUniqueVector(config.mapped_response_attributes())) {}
+                          protoMapValuesToUniqueVector(config.mapped_response_attributes()),
+                          creation_status) {}
 
 bool MappedAttributeBuilder::modifyRequest(
     const Params& params, envoy::service::ext_proc::v3::ProcessingRequest& request) {

source/extensions/http/ext_proc/processing_request_modifiers/mapped_attribute_builder/mapped_attribute_builder.h

@@ -27,7 +27,8 @@ class MappedAttributeBuilder
   MappedAttributeBuilder(const envoy::extensions::http::ext_proc::processing_request_modifiers::
                              mapped_attribute_builder::v3::MappedAttributeBuilder& config,
                          Extensions::Filters::Common::Expr::BuilderInstanceSharedConstPtr builder,
-                         Server::Configuration::CommonFactoryContext& context);
+                         Server::Configuration::CommonFactoryContext& context,
+                         absl::Status& creation_status);
 
   bool modifyRequest(const Params& params,
                      envoy::service::ext_proc::v3::ProcessingRequest& request) override;

source/extensions/http/ext_proc/processing_request_modifiers/mapped_attribute_builder/mapped_attribute_builder_factory.cc

@@ -20,7 +20,13 @@ MappedAttributeBuilderFactory::createProcessingRequestModifier(
       const envoy::extensions::http::ext_proc::processing_request_modifiers::
           mapped_attribute_builder::v3::MappedAttributeBuilder&>(
       config, context.messageValidationVisitor());
-  return std::make_unique<MappedAttributeBuilder>(proto_config, builder, context);
+  absl::Status creation_status = absl::OkStatus();
+  auto buidler =
+      std::make_unique<MappedAttributeBuilder>(proto_config, builder, context, creation_status);
+
+  // TODO(wbpcode): Consider returning absl::StatusOr<> instead of throwing an exception here.
+  THROW_IF_NOT_OK_REF(creation_status);
+  return buidler;
 }
 
 REGISTER_FACTORY(

test/extensions/filters/http/credential_injector/config_test.cc

@@ -37,7 +37,7 @@ TEST(Factory, UnregisteredExtension) {
                          "type URL: 'test.mock_credential.Unregistered'"));
 }
 
-TEST(Factory, ValidConfigWithServerContext) {
+TEST(Factory, UnregisteredExtensionWithServerContext) {
   const std::string yaml_string = R"EOF(
   overwrite: true
   allow_request_without_credential: true
@@ -51,11 +51,10 @@ TEST(Factory, ValidConfigWithServerContext) {
   TestUtility::loadFromYaml(yaml_string, proto_config);
   CredentialInjectorFilterFactory factory;
   NiceMock<Server::Configuration::MockServerFactoryContext> context;
-  EXPECT_THROW_WITH_REGEX(
-      factory.createHttpFilterFactoryFromProto(proto_config, "stats", context).value(),
-      EnvoyException,
-      ".*Didn't find a registered implementation for 'undefined_credential' with "
-      "type URL: 'test.mock_credential.Unregistered'.*");
+  EXPECT_THAT(
+      factory.createHttpFilterFactoryFromProto(proto_config, "stats", context).status().message(),
+      testing::HasSubstr("Didn't find a registered implementation for 'undefined_credential' with "
+                         "type URL: 'test.mock_credential.Unregistered'"));
 }
 
 } // namespace

test/extensions/filters/http/ext_proc/BUILD

@@ -24,6 +24,7 @@ envoy_extension_cc_test(
     deps = [
         "//source/extensions/filters/http/ext_proc:config",
         "//test/mocks/server:factory_context_mocks",
+        "//test/test_common:status_utility_lib",
     ],
 )
 

test/extensions/filters/http/ext_proc/config_test.cc

@@ -1,6 +1,7 @@
 #include "source/extensions/filters/http/ext_proc/config.h"
 
 #include "test/mocks/server/factory_context.h"
+#include "test/test_common/status_utility.h"
 #include "test/test_common/utility.h"
 
 #include "gmock/gmock.h"
@@ -14,6 +15,8 @@ namespace HttpFilters {
 namespace ExternalProcessing {
 namespace {
 
+using StatusHelpers::HasStatus;
+
 TEST(HttpExtProcConfigTest, CorrectConfig) {
   std::string yaml = R"EOF(
   grpc_service:
@@ -200,12 +203,10 @@ TEST(HttpExtProcConfigTest, InvalidHttpServiceProcessingMode) {
 
   testing::NiceMock<Server::Configuration::MockFactoryContext> context;
   auto result = factory.createFilterFactoryFromProto(*proto_config, "stats", context);
-  EXPECT_FALSE(result.ok());
-  EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument);
-  EXPECT_EQ(result.status().message(),
-            "If the ext_proc filter is configured with http_service instead of gRPC service, "
-            "then the processing modes of this filter can not be configured to send body or "
-            "trailer.");
+  EXPECT_THAT(result, HasStatus(absl::StatusCode::kInvalidArgument,
+                                "If the ext_proc filter is configured with http_service instead "
+                                "of gRPC service, then the processing modes of this filter can "
+                                "not be configured to send body or trailer."));
 }
 
 TEST(HttpExtProcConfigTest, HttpServiceTrailerProcessingModeNotSKIP) {
@@ -230,12 +231,10 @@ TEST(HttpExtProcConfigTest, HttpServiceTrailerProcessingModeNotSKIP) {
 
   testing::NiceMock<Server::Configuration::MockFactoryContext> context;
   auto result = factory.createFilterFactoryFromProto(*proto_config, "stats", context);
-  EXPECT_FALSE(result.ok());
-  EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument);
-  EXPECT_EQ(result.status().message(),
-            "If the ext_proc filter is configured with http_service instead of gRPC service, "
-            "then the processing modes of this filter can not be configured to send body or "
-            "trailer.");
+  EXPECT_THAT(result, HasStatus(absl::StatusCode::kInvalidArgument,
+                                "If the ext_proc filter is configured with http_service instead "
+                                "of gRPC service, then the processing modes of this filter can "
+                                "not be configured to send body or trailer."));
 }
 
 TEST(HttpExtProcConfigTest, InvalidFullDuplexStreamedConfig) {
@@ -254,11 +253,10 @@ TEST(HttpExtProcConfigTest, InvalidFullDuplexStreamedConfig) {
 
   testing::NiceMock<Server::Configuration::MockFactoryContext> context;
   auto result = factory.createFilterFactoryFromProto(*proto_config, "stats", context);
-  EXPECT_FALSE(result.ok());
-  EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument);
-  EXPECT_EQ(result.status().message(),
-            "If the ext_proc filter has the request_body_mode set to FULL_DUPLEX_STREAMED, "
-            "then the request_trailer_mode has to be set to SEND");
+  EXPECT_THAT(result, HasStatus(absl::StatusCode::kInvalidArgument,
+                                "If the ext_proc filter has the request_body_mode set to "
+                                "FULL_DUPLEX_STREAMED, then the request_trailer_mode has to be "
+                                "set to SEND"));
 }
 
 TEST(HttpExtProcConfigTest, GrpcServiceHttpServiceBothSet) {
@@ -283,10 +281,9 @@ TEST(HttpExtProcConfigTest, GrpcServiceHttpServiceBothSet) {
 
   testing::NiceMock<Server::Configuration::MockFactoryContext> context;
   auto result = factory.createFilterFactoryFromProto(*proto_config, "stats", context);
-  EXPECT_FALSE(result.ok());
-  EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument);
-  EXPECT_EQ(result.status().message(),
-            "One and only one of grpc_service or http_service must be configured");
+  EXPECT_THAT(result, HasStatus(absl::StatusCode::kInvalidArgument,
+                                "One and only one of grpc_service or http_service must be "
+                                "configured"));
 }
 
 // Verify that the "disable_route_cache_clearing" and "route_cache_action"  setting
@@ -306,10 +303,9 @@ TEST(HttpExtProcConfigTest, InvalidRouteCacheConfig) {
 
   testing::NiceMock<Server::Configuration::MockFactoryContext> context;
   auto result = factory.createFilterFactoryFromProto(*proto_config, "stats", context);
-  EXPECT_FALSE(result.ok());
-  EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument);
-  EXPECT_EQ(result.status().message(), "disable_clear_route_cache and route_cache_action can not "
-                                       "be set to none-default at the same time.");
+  EXPECT_THAT(result, HasStatus(absl::StatusCode::kInvalidArgument,
+                                "disable_clear_route_cache and route_cache_action can not be "
+                                "set to none-default at the same time."));
 }
 
 TEST(HttpExtProcConfigTest, InvalidServiceConfigServerContext) {
@@ -331,9 +327,10 @@ TEST(HttpExtProcConfigTest, InvalidServiceConfigServerContext) {
   TestUtility::loadFromYaml(yaml, *proto_config);
 
   testing::NiceMock<Server::Configuration::MockServerFactoryContext> context;
-  EXPECT_THROW_WITH_MESSAGE(
-      factory.createHttpFilterFactoryFromProto(*proto_config, "stats", context).value(),
-      EnvoyException, "One and only one of grpc_service or http_service must be configured");
+  auto result = factory.createHttpFilterFactoryFromProto(*proto_config, "stats", context);
+  EXPECT_THAT(result, HasStatus(absl::StatusCode::kInvalidArgument,
+                                "One and only one of grpc_service or http_service must be "
+                                "configured"));
 }
 
 TEST(HttpExtProcConfigTest, EmptyConfig) {
@@ -501,10 +498,11 @@ TEST(HttpExtProcConfigTest, FullDuplexStreamedValidation) {
   TestUtility::loadFromYaml(invalid_yaml, *proto_config);
 
   auto invalid_result = factory.createFilterFactoryFromProto(*proto_config, "stats", context);
-  EXPECT_FALSE(invalid_result.ok());
-  EXPECT_EQ(invalid_result.status().message(),
-            "If the ext_proc filter has the response_body_mode set to FULL_DUPLEX_STREAMED, "
-            "then the response_trailer_mode has to be set to SEND");
+  EXPECT_THAT(invalid_result,
+              HasStatus(absl::StatusCode::kInvalidArgument,
+                        "If the ext_proc filter has the response_body_mode set to "
+                        "FULL_DUPLEX_STREAMED, then the response_trailer_mode has to be "
+                        "set to SEND"));
 
   // Verify other response_body_mode values work with SKIP trailers
   std::string other_modes_yaml = R"EOF(