diff --git a/source/extensions/filters/http/local_ratelimit/config.cc b/source/extensions/filters/http/local_ratelimit/config.cc index fbc5ed1d06c46..e9c61f2a9c7e4 100644 --- a/source/extensions/filters/http/local_ratelimit/config.cc +++ b/source/extensions/filters/http/local_ratelimit/config.cc @@ -12,12 +12,14 @@ namespace Extensions { namespace HttpFilters { namespace LocalRateLimitFilter { -Http::FilterFactoryCb LocalRateLimitFilterConfig::createFilterFactoryFromProtoTyped( +absl::StatusOr LocalRateLimitFilterConfig::createFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::local_ratelimit::v3::LocalRateLimit& proto_config, const std::string&, Server::Configuration::FactoryContext& context) { - FilterConfigSharedPtr filter_config = - std::make_shared(proto_config, context.serverFactoryContext(), context.scope()); + absl::Status creation_status = absl::OkStatus(); + FilterConfigSharedPtr filter_config = std::make_shared( + proto_config, context.serverFactoryContext(), context.scope(), creation_status); + RETURN_IF_NOT_OK_REF(creation_status); return [filter_config](Http::FilterChainFactoryCallbacks& callbacks) -> void { callbacks.addStreamFilter(std::make_shared(filter_config)); }; @@ -28,8 +30,10 @@ LocalRateLimitFilterConfig::createHttpFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::local_ratelimit::v3::LocalRateLimit& proto_config, const std::string&, Server::Configuration::ServerFactoryContext& context) { + absl::Status creation_status = absl::OkStatus(); FilterConfigSharedPtr filter_config = - std::make_shared(proto_config, context, context.scope()); + std::make_shared(proto_config, context, context.scope(), creation_status); + RETURN_IF_NOT_OK_REF(creation_status); return [filter_config](Http::FilterChainFactoryCallbacks& callbacks) -> void { callbacks.addStreamFilter(std::make_shared(filter_config)); }; @@ -39,7 +43,12 @@ absl::StatusOr LocalRateLimitFilterConfig::createRouteSpecificFilterConfigTyped( const envoy::extensions::filters::http::local_ratelimit::v3::LocalRateLimit& proto_config, Server::Configuration::ServerFactoryContext& context, ProtobufMessage::ValidationVisitor&) { - return std::make_shared(proto_config, context, context.scope(), true); + absl::Status creation_status = absl::OkStatus(); + auto filter_config = + std::make_shared(proto_config, context, context.scope(), creation_status, + /*per_route=*/true); + RETURN_IF_NOT_OK_REF(creation_status); + return filter_config; } /** diff --git a/source/extensions/filters/http/local_ratelimit/config.h b/source/extensions/filters/http/local_ratelimit/config.h index 80d3940deb283..3ab569bb59112 100644 --- a/source/extensions/filters/http/local_ratelimit/config.h +++ b/source/extensions/filters/http/local_ratelimit/config.h @@ -14,13 +14,13 @@ namespace LocalRateLimitFilter { * Config registration for the local rate limit filter. @see NamedHttpFilterConfigFactory. */ class LocalRateLimitFilterConfig - : public Common::FactoryBase< + : public Common::ExceptionFreeFactoryBase< envoy::extensions::filters::http::local_ratelimit::v3::LocalRateLimit> { public: - LocalRateLimitFilterConfig() : FactoryBase("envoy.filters.http.local_ratelimit") {} + LocalRateLimitFilterConfig() : ExceptionFreeFactoryBase("envoy.filters.http.local_ratelimit") {} private: - Http::FilterFactoryCb createFilterFactoryFromProtoTyped( + absl::StatusOr createFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::local_ratelimit::v3::LocalRateLimit& proto_config, const std::string& stats_prefix, Server::Configuration::FactoryContext& context) override; absl::StatusOr createHttpFilterFactoryFromProtoTyped( diff --git a/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc b/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc index 31b8135476af6..aecfac7b097e6 100644 --- a/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc +++ b/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc @@ -25,7 +25,8 @@ const std::string& PerConnectionRateLimiter::key() { FilterConfig::FilterConfig( const envoy::extensions::filters::http::local_ratelimit::v3::LocalRateLimit& config, - Server::Configuration::CommonFactoryContext& context, Stats::Scope& scope, const bool per_route) + Server::Configuration::CommonFactoryContext& context, Stats::Scope& scope, + absl::Status& creation_status, const bool per_route) : dispatcher_(context.mainThreadDispatcher()), status_(toErrorCode(config.status().code())), stats_(generateStats(config.stat_prefix(), scope)), fill_interval_(std::chrono::milliseconds( @@ -52,12 +53,6 @@ FilterConfig::FilterConfig( ? std::optional( Envoy::Runtime::FractionalPercent(config.filter_enforced(), runtime_)) : std::nullopt), - response_headers_parser_(THROW_OR_RETURN_VALUE( - Envoy::Router::HeaderParser::configure(config.response_headers_to_add()), - Router::HeaderParserPtr)), - request_headers_parser_(THROW_OR_RETURN_VALUE( - Envoy::Router::HeaderParser::configure(config.request_headers_to_add_when_not_enforced()), - Router::HeaderParserPtr)), stage_(static_cast(config.stage())), has_descriptors_(!config.descriptors().empty()), enable_x_rate_limit_headers_(config.enable_x_ratelimit_headers() == @@ -67,19 +62,32 @@ FilterConfig::FilterConfig( config.rate_limited_as_resource_exhausted() ? std::make_optional(Grpc::Status::WellKnownGrpcStatus::ResourceExhausted) : std::nullopt) { + auto response_headers_parser_or = + Envoy::Router::HeaderParser::configure(config.response_headers_to_add()); + SET_AND_RETURN_IF_NOT_OK(response_headers_parser_or.status(), creation_status); + response_headers_parser_ = std::move(response_headers_parser_or.value()); + + auto request_headers_parser_or = + Envoy::Router::HeaderParser::configure(config.request_headers_to_add_when_not_enforced()); + SET_AND_RETURN_IF_NOT_OK(request_headers_parser_or.status(), creation_status); + request_headers_parser_ = std::move(request_headers_parser_or.value()); + // Note: no token bucket is fine for the global config, which would be the case for enabling // the filter globally but disabled and then applying limits at the virtual host or // route level. At the virtual or route level, it makes no sense to have an no token - // bucket so we throw an error. If there's no token bucket configured globally or + // bucket so we return an error. If there's no token bucket configured globally or // at the vhost/route level, no rate limiting is applied. if (per_route && !config.has_token_bucket()) { - throw EnvoyException("local rate limit token bucket must be set for per filter configs"); + creation_status = absl::InvalidArgumentError( + "local rate limit token bucket must be set for per filter configs"); + return; } - absl::Status creation_status; rate_limit_config_ = std::make_unique( config.rate_limits(), context, creation_status); - THROW_IF_NOT_OK_REF(creation_status); + if (!creation_status.ok()) { + return; + } if (rate_limit_config_->empty()) { if (!config.descriptors().empty()) { @@ -94,11 +102,15 @@ FilterConfig::FilterConfig( Filters::Common::LocalRateLimit::ShareProviderSharedPtr share_provider; if (config.has_local_cluster_rate_limit()) { if (rate_limit_per_connection_) { - throw EnvoyException("local_cluster_rate_limit is set and " - "local_rate_limit_per_downstream_connection is set to true"); + creation_status = + absl::InvalidArgumentError("local_cluster_rate_limit is set and " + "local_rate_limit_per_downstream_connection is set to true"); + return; } if (!context.clusterManager().localClusterName().has_value()) { - throw EnvoyException("local_cluster_rate_limit is set but no local cluster name is present"); + creation_status = absl::InvalidArgumentError( + "local_cluster_rate_limit is set but no local cluster name is present"); + return; } // If the local cluster name is set then the relevant cluster must exist or the cluster @@ -106,7 +118,9 @@ FilterConfig::FilterConfig( share_provider_manager_ = Filters::Common::LocalRateLimit::ShareProviderManager::singleton( dispatcher_, context.clusterManager(), context.singletonManager()); if (!share_provider_manager_) { - throw EnvoyException("local_cluster_rate_limit is set but no local cluster is present"); + creation_status = absl::InvalidArgumentError( + "local_cluster_rate_limit is set but no local cluster is present"); + return; } share_provider = share_provider_manager_->getShareProvider(config.local_cluster_rate_limit()); diff --git a/source/extensions/filters/http/local_ratelimit/local_ratelimit.h b/source/extensions/filters/http/local_ratelimit/local_ratelimit.h index a8f67b421d4fc..cfe52be345991 100644 --- a/source/extensions/filters/http/local_ratelimit/local_ratelimit.h +++ b/source/extensions/filters/http/local_ratelimit/local_ratelimit.h @@ -76,7 +76,7 @@ class FilterConfig : public Router::RouteSpecificFilterConfig, public: FilterConfig(const envoy::extensions::filters::http::local_ratelimit::v3::LocalRateLimit& config, Server::Configuration::CommonFactoryContext& context, Stats::Scope& scope, - const bool per_route = false); + absl::Status& creation_status, const bool per_route = false); ~FilterConfig() override { // Ensure that the LocalRateLimiterImpl instance will be destroyed on the thread where its inner // timer is created and running. diff --git a/source/extensions/filters/http/mcp/config.cc b/source/extensions/filters/http/mcp/config.cc index 1093304e2c2c0..f10bbe45c03be 100644 --- a/source/extensions/filters/http/mcp/config.cc +++ b/source/extensions/filters/http/mcp/config.cc @@ -9,7 +9,7 @@ namespace Extensions { namespace HttpFilters { namespace Mcp { -Http::FilterFactoryCb McpFilterConfigFactory::createFilterFactoryFromProtoTyped( +absl::StatusOr McpFilterConfigFactory::createFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::mcp::v3::Mcp& proto_config, const std::string& stats_prefix, Server::Configuration::FactoryContext& context) { diff --git a/source/extensions/filters/http/mcp/config.h b/source/extensions/filters/http/mcp/config.h index 143c4d08153e4..348559093814a 100644 --- a/source/extensions/filters/http/mcp/config.h +++ b/source/extensions/filters/http/mcp/config.h @@ -14,14 +14,14 @@ namespace Mcp { /** * Config factory for MCP filter. */ -class McpFilterConfigFactory - : public Common::FactoryBase { +class McpFilterConfigFactory : public Common::ExceptionFreeFactoryBase< + envoy::extensions::filters::http::mcp::v3::Mcp, + envoy::extensions::filters::http::mcp::v3::McpOverride> { public: - McpFilterConfigFactory() : FactoryBase("envoy.filters.http.mcp") {} + McpFilterConfigFactory() : ExceptionFreeFactoryBase("envoy.filters.http.mcp") {} private: - Http::FilterFactoryCb createFilterFactoryFromProtoTyped( + absl::StatusOr createFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::mcp::v3::Mcp& proto_config, const std::string& stats_prefix, Server::Configuration::FactoryContext& context) override; diff --git a/source/extensions/filters/http/mcp_router/config.cc b/source/extensions/filters/http/mcp_router/config.cc index b87f7ea81c84d..474ea349dba0a 100644 --- a/source/extensions/filters/http/mcp_router/config.cc +++ b/source/extensions/filters/http/mcp_router/config.cc @@ -10,7 +10,8 @@ namespace Extensions { namespace HttpFilters { namespace McpRouter { -Http::FilterFactoryCb McpRouterFilterConfigFactory::createFilterFactoryFromProtoTyped( +absl::StatusOr +McpRouterFilterConfigFactory::createFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::mcp_router::v3::McpRouter& proto_config, const std::string& stats_prefix, Server::Configuration::FactoryContext& context) { diff --git a/source/extensions/filters/http/mcp_router/config.h b/source/extensions/filters/http/mcp_router/config.h index 1d29a64fa8943..4786682a88217 100644 --- a/source/extensions/filters/http/mcp_router/config.h +++ b/source/extensions/filters/http/mcp_router/config.h @@ -14,9 +14,10 @@ namespace McpRouter { * Config factory for MCP router filter. */ class McpRouterFilterConfigFactory - : public Common::FactoryBase { + : public Common::ExceptionFreeFactoryBase< + envoy::extensions::filters::http::mcp_router::v3::McpRouter> { public: - McpRouterFilterConfigFactory() : FactoryBase("envoy.filters.http.mcp_router") {} + McpRouterFilterConfigFactory() : ExceptionFreeFactoryBase("envoy.filters.http.mcp_router") {} private: bool @@ -24,7 +25,7 @@ class McpRouterFilterConfigFactory Server::Configuration::ServerFactoryContext&) override { return true; } - Http::FilterFactoryCb createFilterFactoryFromProtoTyped( + absl::StatusOr createFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::mcp_router::v3::McpRouter& proto_config, const std::string& stats_prefix, Server::Configuration::FactoryContext& context) override; }; diff --git a/source/extensions/filters/http/on_demand/config.cc b/source/extensions/filters/http/on_demand/config.cc index 3cfa686908e92..b92e1f70ea80e 100644 --- a/source/extensions/filters/http/on_demand/config.cc +++ b/source/extensions/filters/http/on_demand/config.cc @@ -9,12 +9,14 @@ namespace Extensions { namespace HttpFilters { namespace OnDemand { -Http::FilterFactoryCb OnDemandFilterFactory::createFilterFactoryFromProtoTyped( +absl::StatusOr OnDemandFilterFactory::createFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::on_demand::v3::OnDemand& proto_config, const std::string&, Server::Configuration::FactoryContext& context) { + absl::Status creation_status = absl::OkStatus(); OnDemandFilterConfigSharedPtr config = std::make_shared( proto_config, context.serverFactoryContext().clusterManager(), - context.messageValidationVisitor()); + context.messageValidationVisitor(), creation_status); + RETURN_IF_NOT_OK_REF(creation_status); return [config](Http::FilterChainFactoryCallbacks& callbacks) -> void { callbacks.addStreamDecoderFilter(std::make_shared(config)); }; @@ -23,8 +25,10 @@ Http::FilterFactoryCb OnDemandFilterFactory::createFilterFactoryFromProtoTyped( absl::StatusOr OnDemandFilterFactory::createHttpFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::on_demand::v3::OnDemand& proto_config, const std::string&, Server::Configuration::ServerFactoryContext& context) { + absl::Status creation_status = absl::OkStatus(); OnDemandFilterConfigSharedPtr config = std::make_shared( - proto_config, context.clusterManager(), context.messageValidationVisitor()); + proto_config, context.clusterManager(), context.messageValidationVisitor(), creation_status); + RETURN_IF_NOT_OK_REF(creation_status); return [config](Http::FilterChainFactoryCallbacks& callbacks) -> void { callbacks.addStreamDecoderFilter(std::make_shared(config)); }; @@ -35,8 +39,11 @@ OnDemandFilterFactory::createRouteSpecificFilterConfigTyped( const envoy::extensions::filters::http::on_demand::v3::PerRouteConfig& proto_config, Server::Configuration::ServerFactoryContext& context, ProtobufMessage::ValidationVisitor& validation_visitor) { - return std::make_shared(proto_config, context.clusterManager(), - validation_visitor); + absl::Status creation_status = absl::OkStatus(); + auto config = std::make_shared(proto_config, context.clusterManager(), + validation_visitor, creation_status); + RETURN_IF_NOT_OK_REF(creation_status); + return config; } /** diff --git a/source/extensions/filters/http/on_demand/config.h b/source/extensions/filters/http/on_demand/config.h index 4e1a7d5280f76..21066f1b2b8b2 100644 --- a/source/extensions/filters/http/on_demand/config.h +++ b/source/extensions/filters/http/on_demand/config.h @@ -14,14 +14,14 @@ namespace OnDemand { /** * Config registration for the OnDemand filter. @see NamedHttpFilterConfigFactory. */ -class OnDemandFilterFactory - : public Common::FactoryBase { +class OnDemandFilterFactory : public Common::ExceptionFreeFactoryBase< + envoy::extensions::filters::http::on_demand::v3::OnDemand, + envoy::extensions::filters::http::on_demand::v3::PerRouteConfig> { public: - OnDemandFilterFactory() : FactoryBase(HttpFilterNames::get().OnDemand) {} + OnDemandFilterFactory() : ExceptionFreeFactoryBase(HttpFilterNames::get().OnDemand) {} private: - Http::FilterFactoryCb createFilterFactoryFromProtoTyped( + absl::StatusOr createFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::on_demand::v3::OnDemand& proto_config, const std::string&, Server::Configuration::FactoryContext& context) override; diff --git a/source/extensions/filters/http/on_demand/on_demand_update.cc b/source/extensions/filters/http/on_demand/on_demand_update.cc index 6b6240d15fcf6..41006f2487746 100644 --- a/source/extensions/filters/http/on_demand/on_demand_update.cc +++ b/source/extensions/filters/http/on_demand/on_demand_update.cc @@ -53,7 +53,7 @@ DecodeHeadersBehaviorPtr DecodeHeadersBehavior::cdsRds(Upstream::OdCdsApiHandleP namespace { -DecodeHeadersBehaviorPtr createDecodeHeadersBehavior( +absl::StatusOr createDecodeHeadersBehavior( OptRef odcds_config, Upstream::ClusterManager& cm, ProtobufMessage::ValidationVisitor& validation_visitor) { if (!odcds_config.has_value()) { @@ -65,10 +65,10 @@ DecodeHeadersBehaviorPtr createDecodeHeadersBehavior( // For xDS-TP based configs, both the odcds_config->source and // odcds_config->resources_locator must be empty. if (!odcds_config->has_source() && odcds_config->resources_locator().empty()) { - odcds = THROW_OR_RETURN_VALUE(cm.allocateOdCdsApi(&Upstream::XdstpOdCdsApiImpl::create, - odcds_config->source(), std::nullopt, - validation_visitor), - Upstream::OdCdsApiHandlePtr); + auto odcds_or = cm.allocateOdCdsApi(&Upstream::XdstpOdCdsApiImpl::create, + odcds_config->source(), std::nullopt, validation_visitor); + RETURN_IF_NOT_OK_REF(odcds_or.status()); + odcds = std::move(odcds_or.value()); } } // TODO(adisuissa): Once the @@ -82,26 +82,26 @@ DecodeHeadersBehaviorPtr createDecodeHeadersBehavior( if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.odcds_over_ads_fix")) { if (odcds_config->source().config_source_specifier_case() == envoy::config::core::v3::ConfigSource::ConfigSourceSpecifierCase::kAds) { - odcds = THROW_OR_RETURN_VALUE(cm.allocateOdCdsApi(&Upstream::XdstpOdCdsApiImpl::create, - odcds_config->source(), std::nullopt, - validation_visitor), - Upstream::OdCdsApiHandlePtr); + auto odcds_or = + cm.allocateOdCdsApi(&Upstream::XdstpOdCdsApiImpl::create, odcds_config->source(), + std::nullopt, validation_visitor); + RETURN_IF_NOT_OK_REF(odcds_or.status()); + odcds = std::move(odcds_or.value()); } } if (odcds == nullptr) { - odcds = THROW_OR_RETURN_VALUE(cm.allocateOdCdsApi(&Upstream::OdCdsApiImpl::create, - odcds_config->source(), std::nullopt, - validation_visitor), - Upstream::OdCdsApiHandlePtr); + auto odcds_or = cm.allocateOdCdsApi(&Upstream::OdCdsApiImpl::create, odcds_config->source(), + std::nullopt, validation_visitor); + RETURN_IF_NOT_OK_REF(odcds_or.status()); + odcds = std::move(odcds_or.value()); } } else { - auto locator = THROW_OR_RETURN_VALUE( - Config::XdsResourceIdentifier::decodeUrl(odcds_config->resources_locator()), - xds::core::v3::ResourceLocator); - odcds = THROW_OR_RETURN_VALUE(cm.allocateOdCdsApi(&Upstream::OdCdsApiImpl::create, - odcds_config->source(), locator, - validation_visitor), - Upstream::OdCdsApiHandlePtr); + auto locator_or = Config::XdsResourceIdentifier::decodeUrl(odcds_config->resources_locator()); + RETURN_IF_NOT_OK_REF(locator_or.status()); + auto odcds_or = cm.allocateOdCdsApi(&Upstream::OdCdsApiImpl::create, odcds_config->source(), + locator_or.value(), validation_visitor); + RETURN_IF_NOT_OK_REF(odcds_or.status()); + odcds = std::move(odcds_or.value()); } } // If changing the default timeout, please update the documentation in on_demand.proto too. @@ -126,15 +126,23 @@ OnDemandFilterConfig::OnDemandFilterConfig(DecodeHeadersBehaviorPtr behavior) OnDemandFilterConfig::OnDemandFilterConfig( const envoy::extensions::filters::http::on_demand::v3::OnDemand& proto_config, - Upstream::ClusterManager& cm, ProtobufMessage::ValidationVisitor& validation_visitor) - : OnDemandFilterConfig( - createDecodeHeadersBehavior(getOdCdsConfig(proto_config), cm, validation_visitor)) {} + Upstream::ClusterManager& cm, ProtobufMessage::ValidationVisitor& validation_visitor, + absl::Status& creation_status) { + auto behavior_or = + createDecodeHeadersBehavior(getOdCdsConfig(proto_config), cm, validation_visitor); + SET_AND_RETURN_IF_NOT_OK(behavior_or.status(), creation_status); + behavior_ = std::move(behavior_or.value()); +} OnDemandFilterConfig::OnDemandFilterConfig( const envoy::extensions::filters::http::on_demand::v3::PerRouteConfig& proto_config, - Upstream::ClusterManager& cm, ProtobufMessage::ValidationVisitor& validation_visitor) - : OnDemandFilterConfig( - createDecodeHeadersBehavior(getOdCdsConfig(proto_config), cm, validation_visitor)) {} + Upstream::ClusterManager& cm, ProtobufMessage::ValidationVisitor& validation_visitor, + absl::Status& creation_status) { + auto behavior_or = + createDecodeHeadersBehavior(getOdCdsConfig(proto_config), cm, validation_visitor); + SET_AND_RETURN_IF_NOT_OK(behavior_or.status(), creation_status); + behavior_ = std::move(behavior_or.value()); +} OnDemandRouteUpdate::OnDemandRouteUpdate(OnDemandFilterConfigSharedPtr config) : config_(std::move(config)) { diff --git a/source/extensions/filters/http/on_demand/on_demand_update.h b/source/extensions/filters/http/on_demand/on_demand_update.h index afbf6d69bdc59..c0edc57a82bc9 100644 --- a/source/extensions/filters/http/on_demand/on_demand_update.h +++ b/source/extensions/filters/http/on_demand/on_demand_update.h @@ -42,11 +42,13 @@ class OnDemandFilterConfig : public Router::RouteSpecificFilterConfig { // Constructs config from extension's proto config. OnDemandFilterConfig( const envoy::extensions::filters::http::on_demand::v3::OnDemand& proto_config, - Upstream::ClusterManager& cm, ProtobufMessage::ValidationVisitor& validation_visitor); + Upstream::ClusterManager& cm, ProtobufMessage::ValidationVisitor& validation_visitor, + absl::Status& creation_status); // Constructs config from extension's per-route proto config. OnDemandFilterConfig( const envoy::extensions::filters::http::on_demand::v3::PerRouteConfig& proto_config, - Upstream::ClusterManager& cm, ProtobufMessage::ValidationVisitor& validation_visitor); + Upstream::ClusterManager& cm, ProtobufMessage::ValidationVisitor& validation_visitor, + absl::Status& creation_status); DecodeHeadersBehavior& decodeHeadersBehavior() const { return *behavior_; } diff --git a/source/extensions/filters/http/original_src/original_src_config_factory.cc b/source/extensions/filters/http/original_src/original_src_config_factory.cc index a7bebc9111b55..318c60712fafa 100644 --- a/source/extensions/filters/http/original_src/original_src_config_factory.cc +++ b/source/extensions/filters/http/original_src/original_src_config_factory.cc @@ -12,7 +12,7 @@ namespace Extensions { namespace HttpFilters { namespace OriginalSrc { -Http::FilterFactoryCb OriginalSrcConfigFactory::createFilterFactoryFromProtoTyped( +absl::StatusOr OriginalSrcConfigFactory::createFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::original_src::v3::OriginalSrc& proto_config, const std::string&, Server::Configuration::FactoryContext&) { Config config(proto_config); diff --git a/source/extensions/filters/http/original_src/original_src_config_factory.h b/source/extensions/filters/http/original_src/original_src_config_factory.h index be1c8837cf993..5b4ec92230bad 100644 --- a/source/extensions/filters/http/original_src/original_src_config_factory.h +++ b/source/extensions/filters/http/original_src/original_src_config_factory.h @@ -13,11 +13,12 @@ namespace OriginalSrc { * Config registration for the original_src filter. */ class OriginalSrcConfigFactory - : public Common::FactoryBase { + : public Common::ExceptionFreeFactoryBase< + envoy::extensions::filters::http::original_src::v3::OriginalSrc> { public: - OriginalSrcConfigFactory() : FactoryBase("envoy.filters.http.original_src") {} + OriginalSrcConfigFactory() : ExceptionFreeFactoryBase("envoy.filters.http.original_src") {} - Http::FilterFactoryCb createFilterFactoryFromProtoTyped( + absl::StatusOr createFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::original_src::v3::OriginalSrc& proto_config, const std::string& stat_prefix, Server::Configuration::FactoryContext& context) override; }; diff --git a/test/extensions/filters/http/local_ratelimit/BUILD b/test/extensions/filters/http/local_ratelimit/BUILD index fc9974258d517..a1730c7d472b2 100644 --- a/test/extensions/filters/http/local_ratelimit/BUILD +++ b/test/extensions/filters/http/local_ratelimit/BUILD @@ -37,6 +37,7 @@ envoy_extension_cc_test( deps = [ "//source/extensions/filters/http/local_ratelimit:config", "//test/mocks/server:server_mocks", + "//test/test_common:status_utility_lib", ], ) diff --git a/test/extensions/filters/http/local_ratelimit/config_test.cc b/test/extensions/filters/http/local_ratelimit/config_test.cc index e7375161be912..96677a2d504ee 100644 --- a/test/extensions/filters/http/local_ratelimit/config_test.cc +++ b/test/extensions/filters/http/local_ratelimit/config_test.cc @@ -3,6 +3,7 @@ #include "test/mocks/server/mocks.h" #include "test/mocks/upstream/priority_set.h" +#include "test/test_common/status_utility.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -12,6 +13,9 @@ namespace Extensions { namespace HttpFilters { namespace LocalRateLimitFilter { +using StatusHelpers::HasStatus; +using StatusHelpers::IsOk; + TEST(Factory, GlobalEmptyConfig) { const std::string yaml = R"( stat_prefix: test @@ -103,11 +107,10 @@ stat_prefix: test TestUtility::loadFromYaml(config_yaml, *proto_config); NiceMock context; - EXPECT_THROW(factory - .createRouteSpecificFilterConfig(*proto_config, context, - ProtobufMessage::getNullValidationVisitor()) - .value(), - EnvoyException); + EXPECT_THAT(factory.createRouteSpecificFilterConfig(*proto_config, context, + ProtobufMessage::getNullValidationVisitor()), + HasStatus(absl::StatusCode::kInvalidArgument, + "local rate limit token bucket must be set for per filter configs")); } TEST(Factory, FillTimerTooLow) { @@ -263,11 +266,9 @@ stat_prefix: test NiceMock context; - EXPECT_THROW(factory - .createRouteSpecificFilterConfig(*proto_config, context, - ProtobufMessage::getNullValidationVisitor()) - .value(), - EnvoyException); + EXPECT_THAT(factory.createRouteSpecificFilterConfig(*proto_config, context, + ProtobufMessage::getNullValidationVisitor()), + Not(IsOk())); } TEST(Factory, LocalClusterRateLimitAndLocalRateLimitPerDownstreamConnection) { @@ -297,14 +298,11 @@ local_rate_limit_per_downstream_connection: true NiceMock context; - EXPECT_THROW_WITH_MESSAGE( - factory - .createRouteSpecificFilterConfig(*proto_config, context, - ProtobufMessage::getNullValidationVisitor()) - .value(), - EnvoyException, - "local_cluster_rate_limit is set and local_rate_limit_per_downstream_connection is set to " - "true"); + auto config_or = factory.createRouteSpecificFilterConfig( + *proto_config, context, ProtobufMessage::getNullValidationVisitor()); + EXPECT_THAT(config_or, HasStatus(absl::StatusCode::kInvalidArgument, + "local_cluster_rate_limit is set and " + "local_rate_limit_per_downstream_connection is set to true")); } TEST(Factory, LocalClusterRateLimitAndWithoutLocalClusterName) { @@ -333,12 +331,11 @@ local_cluster_rate_limit: {} NiceMock context; - EXPECT_THROW_WITH_MESSAGE( - factory - .createRouteSpecificFilterConfig(*proto_config, context, - ProtobufMessage::getNullValidationVisitor()) - .value(), - EnvoyException, "local_cluster_rate_limit is set but no local cluster name is present"); + auto config_or = factory.createRouteSpecificFilterConfig( + *proto_config, context, ProtobufMessage::getNullValidationVisitor()); + EXPECT_THAT(config_or, + HasStatus(absl::StatusCode::kInvalidArgument, + "local_cluster_rate_limit is set but no local cluster name is present")); } TEST(Factory, LocalClusterRateLimitAndWithoutLocalCluster) { @@ -368,12 +365,11 @@ local_cluster_rate_limit: {} NiceMock context; context.cluster_manager_.local_cluster_name_ = "local_cluster"; - EXPECT_THROW_WITH_MESSAGE( - factory - .createRouteSpecificFilterConfig(*proto_config, context, - ProtobufMessage::getNullValidationVisitor()) - .value(), - EnvoyException, "local_cluster_rate_limit is set but no local cluster is present"); + auto config_or = factory.createRouteSpecificFilterConfig( + *proto_config, context, ProtobufMessage::getNullValidationVisitor()); + EXPECT_THAT(config_or, + HasStatus(absl::StatusCode::kInvalidArgument, + "local_cluster_rate_limit is set but no local cluster is present")); } TEST(Factory, LocalClusterRateLimit) { diff --git a/test/extensions/filters/http/local_ratelimit/filter_test.cc b/test/extensions/filters/http/local_ratelimit/filter_test.cc index 864bbc23180ed..9878f472be852 100644 --- a/test/extensions/filters/http/local_ratelimit/filter_test.cc +++ b/test/extensions/filters/http/local_ratelimit/filter_test.cc @@ -101,8 +101,10 @@ class FilterTest : public testing::Test { envoy::extensions::filters::http::local_ratelimit::v3::LocalRateLimit config; TestUtility::loadFromYaml(yaml, config); - config_ = - std::make_shared(config, factory_context_, *stats_.rootScope(), per_route); + absl::Status creation_status = absl::OkStatus(); + config_ = std::make_shared(config, factory_context_, *stats_.rootScope(), + creation_status, per_route); + ASSERT_TRUE(creation_status.ok()) << creation_status.message(); filter_ = std::make_shared(config_); filter_->setDecoderFilterCallbacks(decoder_callbacks_); diff --git a/test/extensions/filters/http/on_demand/BUILD b/test/extensions/filters/http/on_demand/BUILD index 784afcc3ce76d..458deb1e12840 100644 --- a/test/extensions/filters/http/on_demand/BUILD +++ b/test/extensions/filters/http/on_demand/BUILD @@ -33,6 +33,7 @@ envoy_extension_cc_test( "//test/mocks/http:http_mocks", "//test/mocks/router:router_mocks", "//test/mocks/upstream:upstream_mocks", + "//test/test_common:status_utility_lib", "//test/test_common:test_runtime_lib", "//test/test_common:utility_lib", ], diff --git a/test/extensions/filters/http/on_demand/on_demand_filter_test.cc b/test/extensions/filters/http/on_demand/on_demand_filter_test.cc index 0d274774cebe8..d2a8a91739591 100644 --- a/test/extensions/filters/http/on_demand/on_demand_filter_test.cc +++ b/test/extensions/filters/http/on_demand/on_demand_filter_test.cc @@ -6,12 +6,14 @@ #include "test/mocks/http/mocks.h" #include "test/mocks/router/mocks.h" #include "test/mocks/upstream/mocks.h" +#include "test/test_common/status_utility.h" #include "test/test_common/test_runtime.h" #include "test/test_common/utility.h" #include "gmock/gmock.h" #include "gtest/gtest.h" +using testing::Invoke; using testing::Return; using testing::ReturnRef; @@ -20,6 +22,9 @@ namespace Extensions { namespace HttpFilters { namespace OnDemand { +using StatusHelpers::HasStatus; +using StatusHelpers::IsOk; + class OnDemandFilterTest : public testing::Test { public: void SetUp() override { @@ -107,6 +112,21 @@ TEST_F(OnDemandFilterTest, TestDecodeHeadersWhenRouteIsNotAvailable) { EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(headers, true)); } +// tests onRouteConfigUpdateCompletion() invoked synchronously while decodeHeaders() is still +// active: the callback must not continue decoding (decode_headers_active_ == true). +TEST_F(OnDemandFilterTest, TestDecodeHeadersRouteConfigUpdateCompletesSynchronously) { + Http::TestRequestHeaderMapImpl headers; + EXPECT_CALL(decoder_callbacks_, route()).WillRepeatedly(Return(OptRef{})); + // Invoke the route config update callback synchronously, before requestRouteConfigUpdate() + // returns, i.e. while decode_headers_active_ is still true. + EXPECT_CALL(decoder_callbacks_.downstream_callbacks_, requestRouteConfigUpdate(_)) + .WillOnce(Invoke( + [](Http::RouteConfigUpdatedCallbackSharedPtr callback) -> void { (*callback)(true); })); + // continueDecoding() must not be called from within decodeHeaders(). + EXPECT_CALL(decoder_callbacks_, continueDecoding()).Times(0); + filter_->decodeHeaders(headers, true); +} + TEST_F(OnDemandFilterTest, TestDecodeTrailers) { Http::TestRequestTrailerMapImpl headers; EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(headers)); @@ -222,6 +242,34 @@ TEST_F(OnDemandFilterTest, OnRouteConfigUpdateCompletionRestartsActiveStream) { filter_->onRouteConfigUpdateCompletion(true); } +// Tests onRouteConfigUpdateCompletion() with the on_demand_track_end_stream flag disabled +// (old behavior: stream recreation is gated on the absence of a decoding buffer). +TEST_F(OnDemandFilterTest, OnRouteConfigUpdateCompletionRestartsActiveStreamOldBehavior) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.reloadable_features.on_demand_track_end_stream", "false"}}); + Http::TestRequestHeaderMapImpl headers; + filter_->decodeHeaders(headers, true); + // No decoding buffer, so the stream can be recreated. + EXPECT_CALL(decoder_callbacks_, recreateStream(_)).WillOnce(Return(true)); + filter_->onRouteConfigUpdateCompletion(true); +} + +// Tests onClusterDiscoveryCompletion() with the on_demand_track_end_stream flag disabled +// (old behavior: stream recreation is gated on the absence of a decoding buffer). +TEST_F(OnDemandFilterTest, OnClusterDiscoveryCompletionClusterFoundOldBehavior) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.on_demand_cluster_no_recreate_stream", "false"}, + {"envoy.reloadable_features.on_demand_track_end_stream", "false"}}); + Http::TestRequestHeaderMapImpl headers; + filter_->decodeHeaders(headers, true); + // No decoding buffer, so the stream can be recreated. + EXPECT_CALL(decoder_callbacks_, continueDecoding()).Times(0); + EXPECT_CALL(decoder_callbacks_.downstream_callbacks_, clearRouteCache()); + EXPECT_CALL(decoder_callbacks_, recreateStream(_)).WillOnce(Return(true)); + filter_->onClusterDiscoveryCompletion(Upstream::ClusterDiscoveryStatus::Available); +} + // tests onClusterDiscoveryCompletion when a cluster is missing TEST_F(OnDemandFilterTest, OnClusterDiscoveryCompletionClusterNotFound) { EXPECT_CALL(decoder_callbacks_.downstream_callbacks_, clearRouteCache()).Times(0); @@ -327,15 +375,26 @@ TEST(OnDemandConfigTest, Basic) { ProtobufMessage::StrictValidationVisitorImpl visitor; envoy::extensions::filters::http::on_demand::v3::OnDemand config; - OnDemandFilterConfig config1(config, cm, visitor); + absl::Status status1 = absl::OkStatus(); + OnDemandFilterConfig config1(config, cm, visitor, status1); + EXPECT_THAT(status1, IsOk()); config.mutable_odcds(); - OnDemandFilterConfig config2(config, cm, visitor); + absl::Status status2 = absl::OkStatus(); + OnDemandFilterConfig config2(config, cm, visitor, status2); + EXPECT_THAT(status2, IsOk()); config.mutable_odcds()->set_resources_locator("foo"); - EXPECT_THROW_WITH_MESSAGE( - { OnDemandFilterConfig config3(config, cm, visitor); }, EnvoyException, - "foo does not have a xdstp:, http: or file: scheme"); + absl::Status status3 = absl::OkStatus(); + OnDemandFilterConfig config3(config, cm, visitor, status3); + EXPECT_THAT(status3, HasStatus(absl::StatusCode::kInvalidArgument, + "foo does not have a xdstp:, http: or file: scheme")); + + // A valid xdstp resources_locator is decoded and an OdCds API is allocated with it. + config.mutable_odcds()->set_resources_locator("xdstp://foo/envoy.config.cluster.v3.Cluster/bar"); + absl::Status status4 = absl::OkStatus(); + OnDemandFilterConfig config4(config, cm, visitor, status4); + EXPECT_THAT(status4, IsOk()); } } // namespace OnDemand