diff --git a/source/extensions/filters/http/credential_injector/config.cc b/source/extensions/filters/http/credential_injector/config.cc index 92ff78c9c7b1d..92874d7457e97 100644 --- a/source/extensions/filters/http/credential_injector/config.cc +++ b/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, diff --git a/source/extensions/filters/http/dynamic_modules/factory.cc b/source/extensions/filters/http/dynamic_modules/factory.cc index 3c18380641c9a..324076c102f14 100644 --- a/source/extensions/filters/http/dynamic_modules/factory.cc +++ b/source/extensions/filters/http/dynamic_modules/factory.cc @@ -150,9 +150,7 @@ absl::StatusOr 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 diff --git a/source/extensions/filters/http/ext_proc/config.cc b/source/extensions/filters/http/ext_proc/config.cc index 475c73db598e8..73a333c94d588 100644 --- a/source/extensions/filters/http/ext_proc/config.cc +++ b/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( 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( 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), diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 13c5a06193d5a..32d07efff806f 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/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_( diff --git a/source/extensions/filters/http/ext_proc/ext_proc.h b/source/extensions/filters/http/ext_proc/ext_proc.h index a2efc42b88486..5c57c984906a5 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.h +++ b/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_; } diff --git a/source/extensions/filters/http/ext_proc/matching_utils.cc b/source/extensions/filters/http/ext_proc/matching_utils.cc index 8638dce27d92e..eb5faa048da49 100644 --- a/source/extensions/filters/http/ext_proc/matching_utils.cc +++ b/source/extensions/filters/http/ext_proc/matching_utils.cc @@ -12,18 +12,23 @@ namespace HttpFilters { namespace ExternalProcessing { absl::flat_hash_map -ExpressionManager::initExpressions(const Protobuf::RepeatedPtrField& matchers) { +ExpressionManager::initExpressions(const Protobuf::RepeatedPtrField& matchers, + absl::Status& creation_status) { absl::flat_hash_map 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 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(); @@ -31,8 +36,9 @@ ExpressionManager::initExpressions(const Protobuf::RepeatedPtrField 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())); } diff --git a/source/extensions/filters/http/ext_proc/matching_utils.h b/source/extensions/filters/http/ext_proc/matching_utils.h index a9af473a79a10..b5be5410ee0a8 100644 --- a/source/extensions/filters/http/ext_proc/matching_utils.h +++ b/source/extensions/filters/http/ext_proc/matching_utils.h @@ -21,10 +21,11 @@ class ExpressionManager : public Logger::Loggable { ExpressionManager(Extensions::Filters::Common::Expr::BuilderInstanceSharedConstPtr builder, const LocalInfo::LocalInfo& local_info, const Protobuf::RepeatedPtrField& request_matchers, - const Protobuf::RepeatedPtrField& response_matchers) + const Protobuf::RepeatedPtrField& 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 { private: absl::flat_hash_map - initExpressions(const Protobuf::RepeatedPtrField& matchers); + initExpressions(const Protobuf::RepeatedPtrField& matchers, + absl::Status& creation_status); const Extensions::Filters::Common::Expr::BuilderInstanceSharedConstPtr builder_; const LocalInfo::LocalInfo& local_info_; diff --git a/source/extensions/filters/http/header_mutation/config.cc b/source/extensions/filters/http/header_mutation/config.cc index 0cc7a2659ad2f..96bc42ce3c3a9 100644 --- a/source/extensions/filters/http/header_mutation/config.cc +++ b/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(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(filter_config)); diff --git a/source/extensions/http/ext_proc/processing_request_modifiers/mapped_attribute_builder/mapped_attribute_builder.cc b/source/extensions/http/ext_proc/processing_request_modifiers/mapped_attribute_builder/mapped_attribute_builder.cc index 2147d6f6cae3c..6dc5ab4600354 100644 --- a/source/extensions/http/ext_proc/processing_request_modifiers/mapped_attribute_builder/mapped_attribute_builder.cc +++ b/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) { diff --git a/source/extensions/http/ext_proc/processing_request_modifiers/mapped_attribute_builder/mapped_attribute_builder.h b/source/extensions/http/ext_proc/processing_request_modifiers/mapped_attribute_builder/mapped_attribute_builder.h index 7065df1d93203..eef8ec24b52b0 100644 --- a/source/extensions/http/ext_proc/processing_request_modifiers/mapped_attribute_builder/mapped_attribute_builder.h +++ b/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; diff --git a/source/extensions/http/ext_proc/processing_request_modifiers/mapped_attribute_builder/mapped_attribute_builder_factory.cc b/source/extensions/http/ext_proc/processing_request_modifiers/mapped_attribute_builder/mapped_attribute_builder_factory.cc index 16e02b164bbe4..c9ba5b6d7a0bc 100644 --- a/source/extensions/http/ext_proc/processing_request_modifiers/mapped_attribute_builder/mapped_attribute_builder_factory.cc +++ b/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(proto_config, builder, context); + absl::Status creation_status = absl::OkStatus(); + auto builder_instance = + std::make_unique(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 builder_instance; } REGISTER_FACTORY( diff --git a/test/extensions/dynamic_modules/http/BUILD b/test/extensions/dynamic_modules/http/BUILD index 4306b0de1f834..2895932b495ce 100644 --- a/test/extensions/dynamic_modules/http/BUILD +++ b/test/extensions/dynamic_modules/http/BUILD @@ -29,6 +29,7 @@ envoy_cc_test( "//test/mocks/server:server_mocks", "//test/test_common:environment_lib", "//test/test_common:simulated_time_system_lib", + "//test/test_common:status_utility_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/extensions/filters/http/dynamic_modules/v3:pkg_cc_proto", ], diff --git a/test/extensions/dynamic_modules/http/config_test.cc b/test/extensions/dynamic_modules/http/config_test.cc index dc8c33dc62880..e8fa258edbc6b 100644 --- a/test/extensions/dynamic_modules/http/config_test.cc +++ b/test/extensions/dynamic_modules/http/config_test.cc @@ -18,6 +18,7 @@ #include "test/mocks/network/mocks.h" #include "test/mocks/server/mocks.h" #include "test/test_common/environment.h" +#include "test/test_common/status_utility.h" #include "test/test_common/utility.h" #include "gmock/gmock.h" @@ -29,9 +30,10 @@ using testing::ReturnRef; namespace Envoy { namespace Server { namespace Configuration { - namespace { +using StatusHelpers::HasStatus; + // Reads the value of a ``dynamic_modules.`` failure counter tagged with the given filter // name. uint64_t failureCounter(Stats::Scope& scope, absl::string_view leaf, absl::string_view filter) { @@ -127,7 +129,7 @@ TEST_F(DynamicModuleFilterConfigTest, NoModuleOrName) { testing::HasSubstr("Either 'name' or 'module' must be specified")); } -TEST_F(DynamicModuleFilterConfigTest, RemoteSourceWithoutInitManagerThrows) { +TEST_F(DynamicModuleFilterConfigTest, RemoteSourceWithoutInitManagerReturnsError) { const std::string yaml = R"EOF( dynamic_module_config: module: @@ -146,11 +148,10 @@ TEST_F(DynamicModuleFilterConfigTest, RemoteSourceWithoutInitManagerThrows) { // The ServerFactoryContext path has no init manager, so remote sources should be rejected. DynamicModuleConfigFactory factory; - EXPECT_THROW_WITH_REGEX( - factory - .createHttpFilterFactoryFromProto(proto_config, "stats", context_.server_factory_context_) - .value(), - EnvoyException, "Remote module sources require an init manager"); + EXPECT_THAT(factory.createHttpFilterFactoryFromProto(proto_config, "stats", + context_.server_factory_context_), + HasStatus(absl::StatusCode::kInvalidArgument, + "Remote module sources require an init manager")); } TEST_F(DynamicModuleFilterConfigTest, RemoteSourceRegistersInitTarget) { diff --git a/test/extensions/filters/http/credential_injector/config_test.cc b/test/extensions/filters/http/credential_injector/config_test.cc index f8ab11868636c..894a7e6ccafac 100644 --- a/test/extensions/filters/http/credential_injector/config_test.cc +++ b/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 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 diff --git a/test/extensions/filters/http/ext_proc/BUILD b/test/extensions/filters/http/ext_proc/BUILD index d2e2436bbce5e..e783428f05f72 100644 --- a/test/extensions/filters/http/ext_proc/BUILD +++ b/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", ], ) diff --git a/test/extensions/filters/http/ext_proc/config_test.cc b/test/extensions/filters/http/ext_proc/config_test.cc index 7d83a1e3e46d1..3dc2db65965dd 100644 --- a/test/extensions/filters/http/ext_proc/config_test.cc +++ b/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 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 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 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 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 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 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( diff --git a/test/extensions/filters/http/ext_proc/filter_test.cc b/test/extensions/filters/http/ext_proc/filter_test.cc index fe8a3189fc6ba..cbb0160944fb2 100644 --- a/test/extensions/filters/http/ext_proc/filter_test.cc +++ b/test/extensions/filters/http/ext_proc/filter_test.cc @@ -180,8 +180,11 @@ class HttpFilterTest : public testing::Test { auto builder_ptr = Envoy::Extensions::Filters::Common::Expr::createBuilder({}); builder_ = std::make_shared( std::move(builder_ptr)); + absl::Status creation_status = absl::OkStatus(); config_ = std::make_shared(proto_config, 200ms, 10000, *stats_store_.rootScope(), - "", is_upstream_filter, builder_, factory_context_); + "", is_upstream_filter, builder_, factory_context_, + creation_status); + ASSERT_TRUE(creation_status.ok()); filter_ = std::make_unique(config_, std::move(client_)); filter_->setEncoderFilterCallbacks(encoder_callbacks_); EXPECT_CALL(encoder_callbacks_, bufferLimit()).WillRepeatedly(Return(BufferSize)); diff --git a/test/extensions/filters/http/ext_proc/filter_test_common.cc b/test/extensions/filters/http/ext_proc/filter_test_common.cc index 445112ad4ddda..11c2bf83d9a3e 100644 --- a/test/extensions/filters/http/ext_proc/filter_test_common.cc +++ b/test/extensions/filters/http/ext_proc/filter_test_common.cc @@ -116,8 +116,11 @@ void HttpFilterTest::initialize(std::string&& yaml, bool is_upstream_filter) { auto builder_ptr = Envoy::Extensions::Filters::Common::Expr::createBuilder({}); builder_ = std::make_shared( std::move(builder_ptr)); + absl::Status creation_status = absl::OkStatus(); config_ = std::make_shared(proto_config, 200ms, 10000, *stats_store_.rootScope(), - "", is_upstream_filter, builder_, factory_context_); + "", is_upstream_filter, builder_, factory_context_, + creation_status); + ASSERT_TRUE(creation_status.ok()); filter_ = std::make_unique(config_, std::move(client_)); filter_->setEncoderFilterCallbacks(encoder_callbacks_); EXPECT_CALL(encoder_callbacks_, bufferLimit()).WillRepeatedly(Return(BufferSize)); diff --git a/test/extensions/filters/http/ext_proc/mapped_attribute_builder_test.cc b/test/extensions/filters/http/ext_proc/mapped_attribute_builder_test.cc index 6d2ea17f93233..c629c500c9a6f 100644 --- a/test/extensions/filters/http/ext_proc/mapped_attribute_builder_test.cc +++ b/test/extensions/filters/http/ext_proc/mapped_attribute_builder_test.cc @@ -33,8 +33,10 @@ class MappedAttributeBuilderTest : public testing::Test { void initialize(const std::string& yaml) { TestUtility::loadFromYaml(yaml, proto_config_); - builder_ = - std::make_unique(proto_config_, expr_builder_, factory_context_); + absl::Status creation_status = absl::OkStatus(); + builder_ = std::make_unique(proto_config_, expr_builder_, + factory_context_, creation_status); + EXPECT_TRUE(creation_status.ok()); EXPECT_CALL(callbacks_, streamInfo()).WillRepeatedly(ReturnRef(stream_info_)); EXPECT_CALL(stream_info_, dynamicMetadata()).WillRepeatedly(ReturnRef(metadata_)); } diff --git a/test/extensions/filters/http/ext_proc/matching_utils_test.cc b/test/extensions/filters/http/ext_proc/matching_utils_test.cc index 99fb2aca2e161..bcd088d16970b 100644 --- a/test/extensions/filters/http/ext_proc/matching_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/matching_utils_test.cc @@ -28,8 +28,10 @@ class ExpressionManagerTest : public testing::Test { auto builder = Filters::Common::Expr::getBuilder(context_); Protobuf::RepeatedPtrField request_matchers; Protobuf::RepeatedPtrField response_matchers; - expression_manager_ = std::make_unique(builder, context_.local_info_, - request_matchers, response_matchers); + absl::Status creation_status = absl::OkStatus(); + expression_manager_ = std::make_unique( + builder, context_.local_info_, request_matchers, response_matchers, creation_status); + EXPECT_TRUE(creation_status.ok()); } NiceMock context_; @@ -45,9 +47,10 @@ TEST_F(ExpressionManagerTest, InvalidExpression) { Protobuf::RepeatedPtrField request_matchers; request_matchers.Add("undefined_func()"); auto builder = Filters::Common::Expr::getBuilder(context_); - EXPECT_THROW( - { ExpressionManager test_manager(builder, context_.local_info_, request_matchers, {}); }, - EnvoyException); + absl::Status creation_status = absl::OkStatus(); + ExpressionManager test_manager(builder, context_.local_info_, request_matchers, {}, + creation_status); + EXPECT_FALSE(creation_status.ok()); } TEST_F(ExpressionManagerTest, RepeatedMatchers) { @@ -55,7 +58,10 @@ TEST_F(ExpressionManagerTest, RepeatedMatchers) { request_matchers.Add("true"); request_matchers.Add("true"); auto builder = Filters::Common::Expr::getBuilder(context_); - ExpressionManager test_manager(builder, context_.local_info_, request_matchers, {}); + absl::Status creation_status = absl::OkStatus(); + ExpressionManager test_manager(builder, context_.local_info_, request_matchers, {}, + creation_status); + ASSERT_TRUE(creation_status.ok()); EXPECT_TRUE(test_manager.hasRequestExpr()); } @@ -76,7 +82,8 @@ TEST(ExpressionManagerTest, CelUnavailableTest) { request_matchers.Add("true"); // When CEL is not available, this should log a warning but not throw - ExpressionManager manager(builder, context.local_info_, request_matchers, {}); + absl::Status creation_status = absl::OkStatus(); + ExpressionManager manager(builder, context.local_info_, request_matchers, {}, creation_status); EXPECT_FALSE(manager.hasRequestExpr()); } diff --git a/test/extensions/filters/http/ext_proc/ordering_test.cc b/test/extensions/filters/http/ext_proc/ordering_test.cc index 70c5d07cfef39..78d669e0006bb 100644 --- a/test/extensions/filters/http/ext_proc/ordering_test.cc +++ b/test/extensions/filters/http/ext_proc/ordering_test.cc @@ -75,9 +75,11 @@ class OrderingTest : public testing::Test { auto builder_ptr = Envoy::Extensions::Filters::Common::Expr::createBuilder({}); auto builder = std::make_shared( std::move(builder_ptr)); + absl::Status creation_status = absl::OkStatus(); config_ = std::make_shared(proto_config, kMessageTimeout, kMaxMessageTimeoutMs, *stats_store_.rootScope(), "", false, builder, - factory_context_); + factory_context_, creation_status); + ASSERT_TRUE(creation_status.ok()); filter_ = std::make_unique(config_, std::move(client_)); filter_->setEncoderFilterCallbacks(encoder_callbacks_); filter_->setDecoderFilterCallbacks(decoder_callbacks_); diff --git a/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc b/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc index 3b629657ae698..b749432d06098 100644 --- a/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc +++ b/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc @@ -98,9 +98,14 @@ DEFINE_PROTO_FUZZER( auto builder_ptr = Envoy::Extensions::Filters::Common::Expr::createBuilder({}); auto builder = std::make_shared( std::move(builder_ptr)); + absl::Status creation_status = absl::OkStatus(); config = std::make_shared( proto_config, std::chrono::milliseconds(200), 200, *stats_store.rootScope(), "", false, - builder, mocks.factory_context_); + builder, mocks.factory_context_, creation_status); + if (!creation_status.ok()) { + ENVOY_LOG_MISC(debug, "Error creating ext_proc filter config: {}", creation_status.message()); + return; + } } catch (const EnvoyException& e) { ENVOY_LOG_MISC(debug, "EnvoyException during ext_proc filter config validation: {}", e.what()); return; diff --git a/tools/code_format/config.yaml b/tools/code_format/config.yaml index c186b19769319..01cfe70679252 100644 --- a/tools/code_format/config.yaml +++ b/tools/code_format/config.yaml @@ -229,6 +229,7 @@ paths: - source/extensions/http/cache/file_system_http_cache/config.cc - source/extensions/http/custom_response - source/extensions/http/early_header_mutation + - source/extensions/http/ext_proc/processing_request_modifiers/mapped_attribute_builder/mapped_attribute_builder_factory.cc - source/extensions/http/injected_credentials - source/extensions/http/stateful_session - source/extensions/io_socket/user_space