diff --git a/envoy/matcher/matcher.h b/envoy/matcher/matcher.h index 223ed538e5952..531f99cf15bc4 100644 --- a/envoy/matcher/matcher.h +++ b/envoy/matcher/matcher.h @@ -91,22 +91,20 @@ class Action { } }; -using ActionPtr = std::unique_ptr; -using ActionFactoryCb = std::function; +using ActionConstSharedPtr = std::shared_ptr; template class ActionFactory : public Config::TypedFactory { public: - virtual ActionFactoryCb - createActionFactoryCb(const Protobuf::Message& config, - ActionFactoryContext& action_factory_context, - ProtobufMessage::ValidationVisitor& validation_visitor) PURE; + virtual ActionConstSharedPtr + createAction(const Protobuf::Message& config, ActionFactoryContext& action_factory_context, + ProtobufMessage::ValidationVisitor& validation_visitor) PURE; std::string category() const override { return "envoy.matching.action"; } }; // On match, we either return the action to perform or another match tree to match against. template struct OnMatch { - const ActionFactoryCb action_cb_; + const ActionConstSharedPtr action_; const MatchTreeSharedPtr matcher_; bool keep_matching_{}; }; @@ -130,30 +128,39 @@ template class OnMatchFactory { // - The match could not be completed due to lack of data (isInsufficientData() will return true.) // - The match was completed, no match found (isNoMatch() will return true.) // - The match was completed, match found (isMatch() will return true, action() will return the -// ActionFactoryCb.) +// ActionConstSharedPtr.) struct MatchResult { public: - MatchResult(ActionFactoryCb cb) : result_(std::move(cb)) {} + MatchResult(ActionConstSharedPtr cb) : result_(std::move(cb)) {} static MatchResult noMatch() { return MatchResult(NoMatch{}); } static MatchResult insufficientData() { return MatchResult(InsufficientData{}); } bool isInsufficientData() const { return absl::holds_alternative(result_); } bool isComplete() const { return !isInsufficientData(); } bool isNoMatch() const { return absl::holds_alternative(result_); } - bool isMatch() const { return absl::holds_alternative(result_); } - ActionFactoryCb actionFactory() const { return absl::get(result_); } - ActionPtr action() const { return actionFactory()(); } + bool isMatch() const { return absl::holds_alternative(result_); } + const ActionConstSharedPtr& action() const { + ASSERT(isMatch()); + return absl::get(result_); + } + // Returns the action by move. The caller must ensure that the MatchResult is not used after + // this call. + ActionConstSharedPtr actionByMove() { + ASSERT(isMatch()); + return absl::get(std::move(result_)); + } private: struct InsufficientData {}; struct NoMatch {}; - using Result = absl::variant; + using Result = absl::variant; Result result_; MatchResult(NoMatch) : result_(NoMatch{}) {} MatchResult(InsufficientData) : result_(InsufficientData{}) {} }; // Callback to execute against skipped matches' actions. -using SkippedMatchCb = std::function; +using SkippedMatchCb = std::function; + /** * MatchTree provides the interface for performing matches against the data provided by DataType. */ @@ -187,19 +194,19 @@ template class MatchTree { // Parent result's keep_matching skips the nested result. if (on_match->keep_matching_ && nested_result.isMatch()) { if (skipped_match_cb) { - skipped_match_cb(nested_result.actionFactory()); + skipped_match_cb(nested_result.action()); } return MatchResult::noMatch(); } return nested_result; } - if (on_match->action_cb_ && on_match->keep_matching_) { + if (on_match->action_ && on_match->keep_matching_) { if (skipped_match_cb) { - skipped_match_cb(on_match->action_cb_); + skipped_match_cb(on_match->action_); } return MatchResult::noMatch(); } - return MatchResult{on_match->action_cb_}; + return MatchResult{on_match->action_}; } }; diff --git a/source/common/listener_manager/filter_chain_manager_impl.cc b/source/common/listener_manager/filter_chain_manager_impl.cc index d8c01b920ab5e..d1aed994ab478 100644 --- a/source/common/listener_manager/filter_chain_manager_impl.cc +++ b/source/common/listener_manager/filter_chain_manager_impl.cc @@ -49,11 +49,11 @@ class FilterChainNameActionFactory : public Matcher::ActionFactory { public: std::string name() const override { return "filter-chain-name"; } - Matcher::ActionFactoryCb createActionFactoryCb(const Protobuf::Message& config, - FilterChainActionFactoryContext&, - ProtobufMessage::ValidationVisitor&) override { - const auto& name = dynamic_cast(config); - return [value = name.value()]() { return std::make_unique(value); }; + Matcher::ActionConstSharedPtr createAction(const Protobuf::Message& config, + FilterChainActionFactoryContext&, + ProtobufMessage::ValidationVisitor&) override { + return std::make_shared( + dynamic_cast(config).value()); } ProtobufTypes::MessagePtr createEmptyConfigProto() override { return std::make_unique(); @@ -582,9 +582,8 @@ FilterChainManagerImpl::findFilterChainUsingMatcher(const Network::ConnectionSoc Matcher::evaluateMatch(*matcher_, data); ASSERT(match_result.isComplete(), "Matching must complete for network streams."); if (match_result.isMatch()) { - const Matcher::ActionPtr action = match_result.action(); - return action->getTyped().get(filter_chains_by_name_, - info); + return match_result.action()->getTyped().get( + filter_chains_by_name_, info); } return default_filter_chain_.get(); } diff --git a/source/common/matcher/matcher.h b/source/common/matcher/matcher.h index a7aef43c642de..5cc9b0059b42c 100644 --- a/source/common/matcher/matcher.h +++ b/source/common/matcher/matcher.h @@ -324,10 +324,10 @@ class MatchTreeFactory : public OnMatchFactory { on_match.action().typed_config(), server_factory_context_.messageValidationVisitor(), factory); - auto action_factory = factory.createActionFactoryCb( - *message, action_factory_context_, server_factory_context_.messageValidationVisitor()); - return [action_factory, keep_matching = on_match.keep_matching()] { - return OnMatch{action_factory, {}, keep_matching}; + auto action = factory.createAction(*message, action_factory_context_, + server_factory_context_.messageValidationVisitor()); + return [action, keep_matching = on_match.keep_matching()] { + return OnMatch{action, {}, keep_matching}; }; } diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index d9e3fd2ae124e..b09a5f678de76 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -1818,14 +1818,13 @@ RouteConstSharedPtr VirtualHostImpl::getRouteFromEntries(const RouteCallback& cb Matcher::evaluateMatch(*matcher_, data); if (match_result.isMatch()) { - const Matcher::ActionPtr result = match_result.action(); + const auto result = match_result.actionByMove(); if (result->typeUrl() == RouteMatchAction::staticTypeUrl()) { - const RouteMatchAction& route_action = result->getTyped(); - - return getRouteFromRoutes(cb, headers, stream_info, random_value, {route_action.route()}); + return getRouteFromRoutes( + cb, headers, stream_info, random_value, + {std::dynamic_pointer_cast(std::move(result))}); } else if (result->typeUrl() == RouteListMatchAction::staticTypeUrl()) { const RouteListMatchAction& action = result->getTyped(); - return getRouteFromRoutes(cb, headers, stream_info, random_value, action.routes()); } PANIC("Action in router matcher should be Route or RouteList"); @@ -2133,9 +2132,9 @@ const Envoy::Config::TypedMetadata& NullConfigImpl::typedMetadata() const { return DefaultRouteMetadataPack::get().typed_metadata_; } -Matcher::ActionFactoryCb RouteMatchActionFactory::createActionFactoryCb( - const Protobuf::Message& config, RouteActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) { +Matcher::ActionConstSharedPtr +RouteMatchActionFactory::createAction(const Protobuf::Message& config, RouteActionContext& context, + ProtobufMessage::ValidationVisitor& validation_visitor) { const auto& route_config = MessageUtil::downcastAndValidate(config, validation_visitor); @@ -2143,14 +2142,14 @@ Matcher::ActionFactoryCb RouteMatchActionFactory::createActionFactoryCb( RouteCreator::createAndValidateRoute(route_config, context.vhost, context.factory_context, validation_visitor, false), RouteEntryImplBaseConstSharedPtr); - - return [route]() { return std::make_unique(route); }; + return route; } REGISTER_FACTORY(RouteMatchActionFactory, Matcher::ActionFactory); -Matcher::ActionFactoryCb RouteListMatchActionFactory::createActionFactoryCb( - const Protobuf::Message& config, RouteActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) { +Matcher::ActionConstSharedPtr +RouteListMatchActionFactory::createAction(const Protobuf::Message& config, + RouteActionContext& context, + ProtobufMessage::ValidationVisitor& validation_visitor) { const auto& route_config = MessageUtil::downcastAndValidate( config, validation_visitor); @@ -2162,7 +2161,7 @@ Matcher::ActionFactoryCb RouteListMatchActionFactory::createActionFactoryCb( validation_visitor, false), RouteEntryImplBaseConstSharedPtr)); } - return [routes]() { return std::make_unique(routes); }; + return std::make_shared(std::move(routes)); } REGISTER_FACTORY(RouteListMatchActionFactory, Matcher::ActionFactory); diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index c4106e056b8bf..5571df3e437b6 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -619,6 +619,7 @@ class RouteEntryImplBase : public RouteEntryAndRoute, public Matchable, public DirectResponseEntry, public PathMatchCriterion, + public Matcher::ActionBase, public std::enable_shared_from_this, Logger::Loggable { protected: @@ -1191,9 +1192,9 @@ class RouteMatchAction : public Matcher::ActionBase { public: - Matcher::ActionFactoryCb - createActionFactoryCb(const Protobuf::Message& config, RouteActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) override; + Matcher::ActionConstSharedPtr + createAction(const Protobuf::Message& config, RouteActionContext& context, + ProtobufMessage::ValidationVisitor& validation_visitor) override; std::string name() const override { return "route"; } ProtobufTypes::MessagePtr createEmptyConfigProto() override { return std::make_unique(); @@ -1217,9 +1218,9 @@ class RouteListMatchAction : public Matcher::ActionBase { public: - Matcher::ActionFactoryCb - createActionFactoryCb(const Protobuf::Message& config, RouteActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) override; + Matcher::ActionConstSharedPtr + createAction(const Protobuf::Message& config, RouteActionContext& context, + ProtobufMessage::ValidationVisitor& validation_visitor) override; std::string name() const override { return "route_match_action"; } ProtobufTypes::MessagePtr createEmptyConfigProto() override { return std::make_unique(); diff --git a/source/extensions/filters/common/rbac/engine_impl.cc b/source/extensions/filters/common/rbac/engine_impl.cc index e53f2265fc974..9abe4c961df3b 100644 --- a/source/extensions/filters/common/rbac/engine_impl.cc +++ b/source/extensions/filters/common/rbac/engine_impl.cc @@ -11,9 +11,9 @@ namespace Filters { namespace Common { namespace RBAC { -Envoy::Matcher::ActionFactoryCb -ActionFactory::createActionFactoryCb(const Protobuf::Message& config, ActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) { +Envoy::Matcher::ActionConstSharedPtr +ActionFactory::createAction(const Protobuf::Message& config, ActionContext& context, + ProtobufMessage::ValidationVisitor& validation_visitor) { const auto& action_config = MessageUtil::downcastAndValidate(config, validation_visitor); @@ -25,7 +25,7 @@ ActionFactory::createActionFactoryCb(const Protobuf::Message& config, ActionCont context.has_log_ = true; } - return [name, action]() { return std::make_unique(name, action); }; + return std::make_shared(name, action); } REGISTER_FACTORY(ActionFactory, Envoy::Matcher::ActionFactory); @@ -138,18 +138,17 @@ bool RoleBasedAccessControlMatcherEngineImpl::handleAction( Envoy::Matcher::evaluateMatch(*matcher_, data); ASSERT(result.isComplete()); if (result.isMatch()) { - auto action = result.action()->getTyped(); + const auto& action = result.action()->getTyped(); if (effective_policy_id != nullptr) { *effective_policy_id = action.name(); } // If there is at least an LOG action in matchers, we have to turn on and off for shared log // metadata every time when there is a connection or request. - auto rbac_action = action.action(); + const auto rbac_action = action.action(); if (has_log_) { generateLog(info, mode_, rbac_action == envoy::config::rbac::v3::RBAC::LOG); } - switch (rbac_action) { PANIC_ON_PROTO_ENUM_SENTINEL_VALUES; case envoy::config::rbac::v3::RBAC::ALLOW: diff --git a/source/extensions/filters/common/rbac/engine_impl.h b/source/extensions/filters/common/rbac/engine_impl.h index ad22de9ed5553..80a5781ed9e71 100644 --- a/source/extensions/filters/common/rbac/engine_impl.h +++ b/source/extensions/filters/common/rbac/engine_impl.h @@ -50,9 +50,9 @@ class Action : public Envoy::Matcher::ActionBase { public: - Envoy::Matcher::ActionFactoryCb - createActionFactoryCb(const Protobuf::Message& config, ActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) override; + Envoy::Matcher::ActionConstSharedPtr + createAction(const Protobuf::Message& config, ActionContext& context, + ProtobufMessage::ValidationVisitor& validation_visitor) override; std::string name() const override { return "envoy.filters.rbac.action"; } ProtobufTypes::MessagePtr createEmptyConfigProto() override { return std::make_unique(); diff --git a/source/extensions/filters/http/composite/action.cc b/source/extensions/filters/http/composite/action.cc index 9d70c9c10671f..da6941c88b90c 100644 --- a/source/extensions/filters/http/composite/action.cc +++ b/source/extensions/filters/http/composite/action.cc @@ -6,23 +6,30 @@ namespace HttpFilters { namespace Composite { void ExecuteFilterAction::createFilters(Http::FilterChainFactoryCallbacks& callbacks) const { - cb_(callbacks); -} + if (actionSkip()) { + return; + } -bool ExecuteFilterActionFactory::isSampled( - const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action, - Envoy::Runtime::Loader& runtime) { - if (composite_action.has_sample_percent() && - !runtime.snapshot().featureEnabled(composite_action.sample_percent().runtime_key(), - composite_action.sample_percent().default_value())) { - return false; + if (auto config_value = config_provider_(); config_value.has_value()) { + (*config_value)(callbacks); + return; } - return true; + // There is no dynamic config available. Apply missing config filter. + Envoy::Http::MissingConfigFilterFactory(callbacks); } -Matcher::ActionFactoryCb ExecuteFilterActionFactory::createActionFactoryCb( - const Protobuf::Message& config, Http::Matching::HttpFilterActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) { +const std::string& ExecuteFilterAction::actionName() const { return name_; } + +bool ExecuteFilterAction::actionSkip() const { + return sample_.has_value() + ? !runtime_.snapshot().featureEnabled(sample_->runtime_key(), sample_->default_value()) + : false; +} + +Matcher::ActionConstSharedPtr +ExecuteFilterActionFactory::createAction(const Protobuf::Message& config, + Http::Matching::HttpFilterActionContext& context, + ProtobufMessage::ValidationVisitor& validation_visitor) { const auto& composite_action = MessageUtil::downcastAndValidate< const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction&>( config, validation_visitor); @@ -34,20 +41,20 @@ Matcher::ActionFactoryCb ExecuteFilterActionFactory::createActionFactoryCb( if (composite_action.has_dynamic_config()) { if (context.is_downstream_) { - return createDynamicActionFactoryCbDownstream(composite_action, context); + return createDynamicActionDownstream(composite_action, context); } else { - return createDynamicActionFactoryCbUpstream(composite_action, context); + return createDynamicActionUpstream(composite_action, context); } } if (context.is_downstream_) { - return createStaticActionFactoryCbDownstream(composite_action, context, validation_visitor); + return createStaticActionDownstream(composite_action, context, validation_visitor); } else { - return createStaticActionFactoryCbUpstream(composite_action, context, validation_visitor); + return createStaticActionUpstream(composite_action, context, validation_visitor); } } -Matcher::ActionFactoryCb ExecuteFilterActionFactory::createDynamicActionFactoryCbDownstream( +Matcher::ActionConstSharedPtr ExecuteFilterActionFactory::createDynamicActionDownstream( const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action, Http::Matching::HttpFilterActionContext& context) { if (!context.factory_context_.has_value() || !context.server_factory_context_.has_value()) { @@ -57,11 +64,11 @@ Matcher::ActionFactoryCb ExecuteFilterActionFactory::createDynamicActionFactoryC auto provider_manager = Envoy::Http::FilterChainUtility::createSingletonDownstreamFilterConfigProviderManager( context.server_factory_context_.value()); - return createDynamicActionFactoryCbTyped( + return createDynamicActionTyped( composite_action, context, "http", context.factory_context_.value(), provider_manager); } -Matcher::ActionFactoryCb ExecuteFilterActionFactory::createDynamicActionFactoryCbUpstream( +Matcher::ActionConstSharedPtr ExecuteFilterActionFactory::createDynamicActionUpstream( const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action, Http::Matching::HttpFilterActionContext& context) { if (!context.upstream_factory_context_.has_value() || @@ -72,12 +79,12 @@ Matcher::ActionFactoryCb ExecuteFilterActionFactory::createDynamicActionFactoryC auto provider_manager = Envoy::Http::FilterChainUtility::createSingletonUpstreamFilterConfigProviderManager( context.server_factory_context_.value()); - return createDynamicActionFactoryCbTyped( + return createDynamicActionTyped( composite_action, context, "router upstream http", context.upstream_factory_context_.value(), provider_manager); } -Matcher::ActionFactoryCb ExecuteFilterActionFactory::createActionFactoryCbCommon( +Matcher::ActionConstSharedPtr ExecuteFilterActionFactory::createActionCommon( const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action, Http::Matching::HttpFilterActionContext& context, Envoy::Http::FilterFactoryCb& callback, bool is_downstream) { @@ -90,16 +97,17 @@ Matcher::ActionFactoryCb ExecuteFilterActionFactory::createActionFactoryCbCommon std::string name = composite_action.typed_config().name(); ASSERT(context.server_factory_context_ != absl::nullopt); Envoy::Runtime::Loader& runtime = context.server_factory_context_->runtime(); - return [cb = std::move(callback), n = std::move(name), - composite_action = std::move(composite_action), &runtime, this]() -> Matcher::ActionPtr { - if (!isSampled(composite_action, runtime)) { - return nullptr; - } - return std::make_unique(cb, n); - }; + + return std::make_shared( + [cb = std::move(callback)]() mutable -> OptRef { return cb; }, name, + composite_action.has_sample_percent() + ? absl::make_optional( + composite_action.sample_percent()) + : absl::nullopt, + runtime); } -Matcher::ActionFactoryCb ExecuteFilterActionFactory::createStaticActionFactoryCbDownstream( +Matcher::ActionConstSharedPtr ExecuteFilterActionFactory::createStaticActionDownstream( const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action, Http::Matching::HttpFilterActionContext& context, ProtobufMessage::ValidationVisitor& validation_visitor) { @@ -126,10 +134,10 @@ Matcher::ActionFactoryCb ExecuteFilterActionFactory::createStaticActionFactoryCb *message, context.stat_prefix_, context.server_factory_context_.value()); } - return createActionFactoryCbCommon(composite_action, context, callback, true); + return createActionCommon(composite_action, context, callback, true); } -Matcher::ActionFactoryCb ExecuteFilterActionFactory::createStaticActionFactoryCbUpstream( +Matcher::ActionConstSharedPtr ExecuteFilterActionFactory::createStaticActionUpstream( const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action, Http::Matching::HttpFilterActionContext& context, ProtobufMessage::ValidationVisitor& validation_visitor) { @@ -150,7 +158,7 @@ Matcher::ActionFactoryCb ExecuteFilterActionFactory::createStaticActionFactoryCb callback = callback_or_status.value(); } - return createActionFactoryCbCommon(composite_action, context, callback, false); + return createActionCommon(composite_action, context, callback, false); } REGISTER_FACTORY(ExecuteFilterActionFactory, diff --git a/source/extensions/filters/http/composite/action.h b/source/extensions/filters/http/composite/action.h index 834b757a573fa..ecd9ad9bb076c 100644 --- a/source/extensions/filters/http/composite/action.h +++ b/source/extensions/filters/http/composite/action.h @@ -18,16 +18,26 @@ class ExecuteFilterAction : public Matcher::ActionBase< envoy::extensions::filters::http::composite::v3::ExecuteFilterAction> { public: - explicit ExecuteFilterAction(Http::FilterFactoryCb cb, const std::string& name) - : cb_(std::move(cb)), name_(name) {} + using FilterConfigProvider = std::function()>; + + explicit ExecuteFilterAction( + FilterConfigProvider config_provider, const std::string& name, + const absl::optional& sample, + Runtime::Loader& runtime) + : config_provider_(std::move(config_provider)), name_(name), sample_(sample), + runtime_(runtime) {} void createFilters(Http::FilterChainFactoryCallbacks& callbacks) const; - const std::string& actionName() const { return name_; } + const std::string& actionName() const; + + bool actionSkip() const; private: - Http::FilterFactoryCb cb_; + FilterConfigProvider config_provider_; const std::string name_; + const absl::optional sample_; + Runtime::Loader& runtime_; }; class ExecuteFilterActionFactory @@ -36,29 +46,22 @@ class ExecuteFilterActionFactory public: std::string name() const override { return "composite-action"; } - Matcher::ActionFactoryCb - createActionFactoryCb(const Protobuf::Message& config, - Http::Matching::HttpFilterActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) override; + Matcher::ActionConstSharedPtr + createAction(const Protobuf::Message& config, Http::Matching::HttpFilterActionContext& context, + ProtobufMessage::ValidationVisitor& validation_visitor) override; ProtobufTypes::MessagePtr createEmptyConfigProto() override { return std::make_unique(); } - // Rolling the dice to decide whether the action will be sampled. - // By default, if sample_percent is not specified, then it is sampled. - bool isSampled( - const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action, - Envoy::Runtime::Loader& runtime); - private: - Matcher::ActionFactoryCb createActionFactoryCbCommon( + Matcher::ActionConstSharedPtr createActionCommon( const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action, Http::Matching::HttpFilterActionContext& context, Envoy::Http::FilterFactoryCb& callback, bool is_downstream); template - Matcher::ActionFactoryCb createDynamicActionFactoryCbTyped( + Matcher::ActionConstSharedPtr createDynamicActionTyped( const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action, Http::Matching::HttpFilterActionContext& context, const std::string& filter_chain_type, FactoryCtx& factory_context, std::shared_ptr& provider_manager) { @@ -73,35 +76,30 @@ class ExecuteFilterActionFactory server_factory_context.clusterManager(), false, filter_chain_type, nullptr); Envoy::Runtime::Loader& runtime = context.server_factory_context_->runtime(); - return - [provider = std::move(provider), n = std::move(name), - composite_action = std::move(composite_action), &runtime, this]() -> Matcher::ActionPtr { - if (!isSampled(composite_action, runtime)) { - return nullptr; - } - - if (auto config_value = provider->config(); config_value.has_value()) { - return std::make_unique(config_value.ref(), n); - } - // There is no dynamic config available. Apply missing config filter. - return std::make_unique(Envoy::Http::MissingConfigFilterFactory, n); - }; + + return std::make_shared( + [provider]() -> OptRef { return provider->config(); }, name, + composite_action.has_sample_percent() + ? absl::make_optional( + composite_action.sample_percent()) + : absl::nullopt, + runtime); } - Matcher::ActionFactoryCb createDynamicActionFactoryCbDownstream( + Matcher::ActionConstSharedPtr createDynamicActionDownstream( const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action, Http::Matching::HttpFilterActionContext& context); - Matcher::ActionFactoryCb createDynamicActionFactoryCbUpstream( + Matcher::ActionConstSharedPtr createDynamicActionUpstream( const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action, Http::Matching::HttpFilterActionContext& context); - Matcher::ActionFactoryCb createStaticActionFactoryCbDownstream( + Matcher::ActionConstSharedPtr createStaticActionDownstream( const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action, Http::Matching::HttpFilterActionContext& context, ProtobufMessage::ValidationVisitor& validation_visitor); - Matcher::ActionFactoryCb createStaticActionFactoryCbUpstream( + Matcher::ActionConstSharedPtr createStaticActionUpstream( const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action, Http::Matching::HttpFilterActionContext& context, ProtobufMessage::ValidationVisitor& validation_visitor); diff --git a/source/extensions/filters/http/composite/filter.cc b/source/extensions/filters/http/composite/filter.cc index 3075685da05b1..fd12f0946ecd7 100644 --- a/source/extensions/filters/http/composite/filter.cc +++ b/source/extensions/filters/http/composite/filter.cc @@ -96,7 +96,6 @@ void Filter::encodeComplete() { void Filter::onMatchCallback(const Matcher::Action& action) { const auto& composite_action = action.getTyped(); - FactoryCallbacksWrapper wrapper(*this, dispatcher_); composite_action.createFilters(wrapper); @@ -107,11 +106,12 @@ void Filter::onMatchCallback(const Matcher::Action& action) { wrapper.errors_, [](const auto& status) { return status.ToString(); })); return; } - const std::string& action_name = composite_action.actionName(); if (wrapper.filter_to_inject_.has_value()) { stats_.filter_delegation_success_.inc(); + const std::string& action_name = composite_action.actionName(); + auto createDelegatedFilterFn = Overloaded{ [this, action_name](Http::StreamDecoderFilterSharedPtr filter) { delegated_filter_ = std::make_shared(std::move(filter)); @@ -137,7 +137,6 @@ void Filter::onMatchCallback(const Matcher::Action& action) { access_loggers_.insert(access_loggers_.end(), wrapper.access_loggers_.begin(), wrapper.access_loggers_.end()); } - // TODO(snowp): Make it possible for onMatchCallback to fail the stream by issuing a local reply, // either directly or via some return status. } diff --git a/source/extensions/filters/http/composite/filter.h b/source/extensions/filters/http/composite/filter.h index acc86f447bf57..551744416fc5b 100644 --- a/source/extensions/filters/http/composite/filter.h +++ b/source/extensions/filters/http/composite/filter.h @@ -57,8 +57,7 @@ class Filter : public Http::StreamFilter, Logger::Loggable { public: Filter(FilterStats& stats, Event::Dispatcher& dispatcher, bool is_upstream) - : dispatcher_(dispatcher), decoded_headers_(false), stats_(stats), is_upstream_(is_upstream) { - } + : dispatcher_(dispatcher), stats_(stats), is_upstream_(is_upstream) {} // Http::StreamDecoderFilter Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& headers, @@ -124,7 +123,7 @@ class Filter : public Http::StreamFilter, // time will result in various FM assertions firing. // We should be protected against this by the match tree validation that only allows request // headers, this just provides some additional sanity checking. - bool decoded_headers_ : 1; + bool decoded_headers_{false}; // Wraps a stream encoder OR a stream decoder filter into a stream filter, making it easier to // delegate calls. diff --git a/source/extensions/filters/http/custom_response/config.cc b/source/extensions/filters/http/custom_response/config.cc index 4aa4ec67746fc..84d5c1533b20d 100644 --- a/source/extensions/filters/http/custom_response/config.cc +++ b/source/extensions/filters/http/custom_response/config.cc @@ -61,7 +61,7 @@ PolicySharedPtr FilterConfig::getPolicy(const ::Envoy::Http::ResponseHeaderMap& if (!match_result.isMatch()) { return PolicySharedPtr{}; } - return match_result.action()->getTyped().policy_; + return std::dynamic_pointer_cast(match_result.actionByMove()); } } // namespace CustomResponse diff --git a/source/extensions/filters/http/custom_response/policy.h b/source/extensions/filters/http/custom_response/policy.h index 8a4d98080421d..8a18f647ea2e3 100644 --- a/source/extensions/filters/http/custom_response/policy.h +++ b/source/extensions/filters/http/custom_response/policy.h @@ -19,9 +19,9 @@ namespace CustomResponse { class CustomResponseFilter; // Base class for custom response policies. -class Policy : public std::enable_shared_from_this { +class Policy : public std::enable_shared_from_this, + public Matcher::ActionBase { public: - virtual ~Policy() = default; virtual Http::FilterHeadersStatus encodeHeaders(Http::ResponseHeaderMap&, bool, CustomResponseFilter&) const PURE; @@ -42,11 +42,6 @@ struct CustomResponseFilterState : public std::enable_shared_from_this { - explicit CustomResponseMatchAction(PolicySharedPtr policy) : policy_(policy) {} - const PolicySharedPtr policy_; -}; - struct CustomResponseActionFactoryContext { Server::Configuration::ServerFactoryContext& server_; Stats::StatName stats_prefix_; @@ -57,12 +52,10 @@ template class PolicyMatchActionFactory : public Matcher::ActionFactory, Logger::Loggable { public: - Matcher::ActionFactoryCb createActionFactoryCb(const Protobuf::Message& config, - CustomResponseActionFactoryContext& context, - ProtobufMessage::ValidationVisitor&) override { - return [policy = createPolicy(config, context.server_, context.stats_prefix_)] { - return std::make_unique(policy); - }; + Matcher::ActionConstSharedPtr createAction(const Protobuf::Message& config, + CustomResponseActionFactoryContext& context, + ProtobufMessage::ValidationVisitor&) override { + return createPolicy(config, context.server_, context.stats_prefix_); } std::string category() const override { return "envoy.http.custom_response"; } diff --git a/source/extensions/filters/http/match_delegate/config.cc b/source/extensions/filters/http/match_delegate/config.cc index 86d32d5e63222..c80e56c9fc92f 100644 --- a/source/extensions/filters/http/match_delegate/config.cc +++ b/source/extensions/filters/http/match_delegate/config.cc @@ -24,10 +24,10 @@ class SkipActionFactory : public Matcher::ActionFactory { public: std::string name() const override { return "skip"; } - Matcher::ActionFactoryCb createActionFactoryCb(const Protobuf::Message&, - Envoy::Http::Matching::HttpFilterActionContext&, - ProtobufMessage::ValidationVisitor&) override { - return []() { return std::make_unique(); }; + Matcher::ActionConstSharedPtr createAction(const Protobuf::Message&, + Envoy::Http::Matching::HttpFilterActionContext&, + ProtobufMessage::ValidationVisitor&) override { + return std::make_shared(); } ProtobufTypes::MessagePtr createEmptyConfigProto() override { return std::make_unique(); @@ -120,8 +120,8 @@ void DelegatingStreamFilter::FilterMatchState::evaluateMatchTree( match_tree_evaluated_ = match_result.isComplete(); if (match_tree_evaluated_ && match_result.isMatch()) { - const Matcher::ActionPtr result = match_result.action(); - if ((result == nullptr) || (SkipAction().typeUrl() == result->typeUrl())) { + const auto& result = match_result.action(); + if (result == nullptr || SkipAction().typeUrl() == result->typeUrl()) { skip_filter_ = true; } else { ASSERT(base_filter_ != nullptr); diff --git a/source/extensions/filters/http/proto_api_scrubber/filter_config.h b/source/extensions/filters/http/proto_api_scrubber/filter_config.h index bb246bfcb8224..6fa6245a78683 100644 --- a/source/extensions/filters/http/proto_api_scrubber/filter_config.h +++ b/source/extensions/filters/http/proto_api_scrubber/filter_config.h @@ -118,10 +118,10 @@ class RemoveFieldAction : public Matcher::ActionBase { public: - Matcher::ActionFactoryCb createActionFactoryCb(const Protobuf::Message&, - ProtoApiScrubberRemoveFieldAction&, - ProtobufMessage::ValidationVisitor&) override { - return []() { return std::make_unique(); }; + Matcher::ActionConstSharedPtr createAction(const Protobuf::Message&, + ProtoApiScrubberRemoveFieldAction&, + ProtobufMessage::ValidationVisitor&) override { + return std::make_shared(); } ProtobufTypes::MessagePtr createEmptyConfigProto() override { diff --git a/source/extensions/filters/http/rate_limit_quota/filter.cc b/source/extensions/filters/http/rate_limit_quota/filter.cc index 34c74d7e4a6e6..cb297c5d0dd90 100644 --- a/source/extensions/filters/http/rate_limit_quota/filter.cc +++ b/source/extensions/filters/http/rate_limit_quota/filter.cc @@ -56,8 +56,9 @@ inline Envoy::Http::Code getDenyResponseCode(const DenyResponseSettings& setting inline std::function addDenyResponseHeadersCb(const DenyResponseSettings& settings) { - if (settings.response_headers_to_add().empty()) + if (settings.response_headers_to_add().empty()) { return nullptr; + } // Headers copied from settings for thread-safety. return [headers_to_add = settings.response_headers_to_add()](Http::ResponseHeaderMap& headers) { for (const envoy::config::core::v3::HeaderValueOption& header : headers_to_add) { @@ -79,7 +80,7 @@ Http::FilterHeadersStatus RateLimitQuotaFilter::decodeHeaders(Http::RequestHeade bool end_stream) { ENVOY_LOG(trace, "decodeHeaders: end_stream = {}", end_stream); // First, perform the request matching. - absl::StatusOr match_result = requestMatching(headers); + absl::StatusOr match_result = requestMatching(headers); if (!match_result.ok()) { // When the request is not matched by any matchers, it is ALLOWED by default // (i.e., fail-open) and its quota usage will not be reported to RLQS @@ -184,7 +185,7 @@ Http::FilterHeadersStatus RateLimitQuotaFilter::decodeHeaders(Http::RequestHeade // TODO(tyxia) Currently request matching is only performed on the request // header. -absl::StatusOr +absl::StatusOr RateLimitQuotaFilter::requestMatching(const Http::RequestHeaderMap& headers) { // Initialize the data pointer on first use and reuse it for subsequent // requests. This avoids creating the data object for every request, which @@ -215,7 +216,7 @@ RateLimitQuotaFilter::requestMatching(const Http::RequestHeaderMap& headers) { return absl::NotFoundError("Matching completed but no match result was found."); } // Return the matched result for `on_match` case. - return match_result.action(); + return match_result.actionByMove(); } void RateLimitQuotaFilter::onDestroy() { diff --git a/source/extensions/filters/http/rate_limit_quota/filter.h b/source/extensions/filters/http/rate_limit_quota/filter.h index dcaf8b45a8d51..2920594c5d5f3 100644 --- a/source/extensions/filters/http/rate_limit_quota/filter.h +++ b/source/extensions/filters/http/rate_limit_quota/filter.h @@ -65,7 +65,8 @@ class RateLimitQuotaFilter : public Http::PassThroughFilter, // Perform request matching. It returns the generated bucket ids if the // matching succeeded, error status otherwise. - absl::StatusOr requestMatching(const Http::RequestHeaderMap& headers); + absl::StatusOr + requestMatching(const Http::RequestHeaderMap& headers); Http::Matching::HttpMatchingDataImpl matchingData() { ASSERT(data_ptr_ != nullptr); diff --git a/source/extensions/filters/http/rate_limit_quota/matcher.h b/source/extensions/filters/http/rate_limit_quota/matcher.h index 38eb7ee3921c7..ee6fc0a21bdc1 100644 --- a/source/extensions/filters/http/rate_limit_quota/matcher.h +++ b/source/extensions/filters/http/rate_limit_quota/matcher.h @@ -51,17 +51,15 @@ class RateLimitOnMatchActionFactory : public Matcher::ActionFactory(config, validation_visitor); - return [bucket_settings = std::move(bucket_settings)]() { - return std::make_unique(std::move(bucket_settings)); - }; + return std::make_shared(std::move(bucket_settings)); } ProtobufTypes::MessagePtr createEmptyConfigProto() override { diff --git a/source/extensions/filters/network/generic_proxy/route_impl.cc b/source/extensions/filters/network/generic_proxy/route_impl.cc index 60fd548e078f3..d70a3a99fc9d6 100644 --- a/source/extensions/filters/network/generic_proxy/route_impl.cc +++ b/source/extensions/filters/network/generic_proxy/route_impl.cc @@ -61,13 +61,12 @@ RouteEntryImpl::RouteEntryImpl(const ProtoRouteAction& route_action, } } -Matcher::ActionFactoryCb RouteMatchActionFactory::createActionFactoryCb( - const Protobuf::Message& config, RouteActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) { +Matcher::ActionConstSharedPtr +RouteMatchActionFactory::createAction(const Protobuf::Message& config, RouteActionContext& context, + ProtobufMessage::ValidationVisitor& validation_visitor) { const auto& route_action = MessageUtil::downcastAndValidate(config, validation_visitor); - auto route = std::make_shared(route_action, context.factory_context); - return [route]() { return std::make_unique(route); }; + return std::make_shared(route_action, context.factory_context); } REGISTER_FACTORY(RouteMatchActionFactory, Matcher::ActionFactory); @@ -92,15 +91,12 @@ RouteEntryConstSharedPtr VirtualHostImpl::routeEntry(const MatchInput& request) Matcher::MatchResult match_result = Matcher::evaluateMatch(*matcher_, request); if (match_result.isMatch()) { - Matcher::ActionPtr action = match_result.action(); - + Matcher::ActionConstSharedPtr action = match_result.actionByMove(); // The only possible action that can be used within the route matching context // is the RouteMatchAction, so this must be true. - ASSERT(action->typeUrl() == RouteMatchAction::staticTypeUrl()); - ASSERT(dynamic_cast(action.get())); - const RouteMatchAction& route_action = static_cast(*action); - - return route_action.route(); + ASSERT(action->typeUrl() == RouteEntryImpl::staticTypeUrl()); + ASSERT(dynamic_cast(action.get())); + return std::dynamic_pointer_cast(std::move(action)); } ENVOY_LOG(debug, "failed to match incoming request: {}", diff --git a/source/extensions/filters/network/generic_proxy/route_impl.h b/source/extensions/filters/network/generic_proxy/route_impl.h index 62118b8856f6e..ef81c9818d7a3 100644 --- a/source/extensions/filters/network/generic_proxy/route_impl.h +++ b/source/extensions/filters/network/generic_proxy/route_impl.h @@ -32,7 +32,7 @@ using ProtoRouteConfiguration = using ProtoVirtualHost = envoy::extensions::filters::network::generic_proxy::v3::VirtualHost; using ProtoRetryPolicy = envoy::config::core::v3::RetryPolicy; -class RouteEntryImpl : public RouteEntry { +class RouteEntryImpl : public RouteEntry, public Matcher::ActionBase { public: RouteEntryImpl(const ProtoRouteAction& route, Envoy::Server::Configuration::ServerFactoryContext& context); @@ -76,17 +76,6 @@ struct RouteActionContext { Server::Configuration::ServerFactoryContext& factory_context; }; -// Action used with the matching tree to specify route to use for an incoming stream. -class RouteMatchAction : public Matcher::ActionBase { -public: - explicit RouteMatchAction(RouteEntryConstSharedPtr route) : route_(std::move(route)) {} - - RouteEntryConstSharedPtr route() const { return route_; } - -private: - RouteEntryConstSharedPtr route_; -}; - class RouteActionValidationVisitor : public Matcher::MatchTreeValidationVisitor { public: absl::Status performDataInputValidation(const Matcher::DataInputFactory&, @@ -98,9 +87,9 @@ class RouteActionValidationVisitor : public Matcher::MatchTreeValidationVisitor< // Registered factory for RouteMatchAction. class RouteMatchActionFactory : public Matcher::ActionFactory { public: - Matcher::ActionFactoryCb - createActionFactoryCb(const Protobuf::Message& config, RouteActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) override; + Matcher::ActionConstSharedPtr + createAction(const Protobuf::Message& config, RouteActionContext& context, + ProtobufMessage::ValidationVisitor& validation_visitor) override; std::string name() const override { return "envoy.matching.action.generic_proxy.route"; } ProtobufTypes::MessagePtr createEmptyConfigProto() override { return std::make_unique(); diff --git a/source/extensions/filters/network/match_delegate/config.cc b/source/extensions/filters/network/match_delegate/config.cc index b176034ad65ff..adfa6938a7e8f 100644 --- a/source/extensions/filters/network/match_delegate/config.cc +++ b/source/extensions/filters/network/match_delegate/config.cc @@ -18,10 +18,9 @@ namespace Factory { class SkipActionFactory : public Matcher::ActionFactory { public: std::string name() const override { return "skip"; } - Matcher::ActionFactoryCb createActionFactoryCb(const Protobuf::Message&, - NetworkFilterActionContext&, - ProtobufMessage::ValidationVisitor&) override { - return []() { return std::make_unique(); }; + Matcher::ActionConstSharedPtr createAction(const Protobuf::Message&, NetworkFilterActionContext&, + ProtobufMessage::ValidationVisitor&) override { + return std::make_shared(); } ProtobufTypes::MessagePtr createEmptyConfigProto() override { return std::make_unique(); @@ -107,8 +106,8 @@ void DelegatingNetworkFilter::FilterMatchState::evaluateMatchTree() { match_tree_evaluated_ = match_result.isComplete(); if (match_tree_evaluated_ && match_result.isMatch()) { - const Matcher::ActionPtr result = match_result.action(); - if ((result == nullptr) || (SkipAction().typeUrl() == result->typeUrl())) { + const auto& result = match_result.action(); + if (result == nullptr || SkipAction().typeUrl() == result->typeUrl()) { skip_filter_ = true; } else { // TODO(botengyao) this would be similar to `base_filter_->onMatchCallback(*result);` @@ -192,7 +191,7 @@ Envoy::Network::FilterFactoryCb MatchDelegateConfig::createFilterFactory( auto message = Config::Utility::translateAnyToFactoryConfig( proto_config.extension_config().typed_config(), validation, factory); auto filter_factory_or_error = factory.createFilterFactoryFromProto(*message, context); - THROW_IF_NOT_OK(filter_factory_or_error.status()); + THROW_IF_NOT_OK_REF(filter_factory_or_error.status()); auto filter_factory = filter_factory_or_error.value(); Factory::MatchTreeValidationVisitor validation_visitor(*factory.matchingRequirements()); diff --git a/source/extensions/filters/udp/udp_proxy/router/router_impl.cc b/source/extensions/filters/udp/udp_proxy/router/router_impl.cc index 0fa2a904e1e5b..8ab3a80a569b1 100644 --- a/source/extensions/filters/udp/udp_proxy/router/router_impl.cc +++ b/source/extensions/filters/udp/udp_proxy/router/router_impl.cc @@ -15,17 +15,16 @@ namespace UdpFilters { namespace UdpProxy { namespace Router { -Matcher::ActionFactoryCb RouteMatchActionFactory::createActionFactoryCb( - const Protobuf::Message& config, RouteActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) { +Matcher::ActionConstSharedPtr +RouteMatchActionFactory::createAction(const Protobuf::Message& config, RouteActionContext& context, + ProtobufMessage::ValidationVisitor& validation_visitor) { const auto& route_config = MessageUtil::downcastAndValidate< const envoy::extensions::filters::udp::udp_proxy::v3::Route&>(config, validation_visitor); const auto& cluster = route_config.cluster(); // Emplace cluster names to context to get all cluster names. context.cluster_name_.emplace(cluster); - - return [cluster]() { return std::make_unique(cluster); }; + return std::make_shared(cluster); } REGISTER_FACTORY(RouteMatchActionFactory, Matcher::ActionFactory); diff --git a/source/extensions/filters/udp/udp_proxy/router/router_impl.h b/source/extensions/filters/udp/udp_proxy/router/router_impl.h index f506778db6af2..04bb887fde713 100644 --- a/source/extensions/filters/udp/udp_proxy/router/router_impl.h +++ b/source/extensions/filters/udp/udp_proxy/router/router_impl.h @@ -32,9 +32,9 @@ class RouteMatchAction class RouteMatchActionFactory : public Matcher::ActionFactory { public: - Matcher::ActionFactoryCb - createActionFactoryCb(const Protobuf::Message& config, RouteActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) override; + Matcher::ActionConstSharedPtr + createAction(const Protobuf::Message& config, RouteActionContext& context, + ProtobufMessage::ValidationVisitor& validation_visitor) override; std::string name() const override { return "route"; } ProtobufTypes::MessagePtr createEmptyConfigProto() override { return std::make_unique(); diff --git a/source/extensions/matching/actions/format_string/config.cc b/source/extensions/matching/actions/format_string/config.cc index 962082481b1f3..9476c572ecee7 100644 --- a/source/extensions/matching/actions/format_string/config.cc +++ b/source/extensions/matching/actions/format_string/config.cc @@ -23,10 +23,10 @@ ActionImpl::get(const Server::Configuration::FilterChainsByName& filter_chains_b return nullptr; } -Matcher::ActionFactoryCb -ActionFactory::createActionFactoryCb(const Protobuf::Message& proto_config, - FilterChainActionFactoryContext& context, - ProtobufMessage::ValidationVisitor& validator) { +Matcher::ActionConstSharedPtr +ActionFactory::createAction(const Protobuf::Message& proto_config, + FilterChainActionFactoryContext& context, + ProtobufMessage::ValidationVisitor& validator) { const auto& config = MessageUtil::downcastAndValidate( proto_config, validator); @@ -35,7 +35,7 @@ ActionFactory::createActionFactoryCb(const Protobuf::Message& proto_config, Formatter::FormatterConstSharedPtr formatter = THROW_OR_RETURN_VALUE( Formatter::SubstitutionFormatStringUtils::fromProtoConfig(config, generic_context), Formatter::FormatterPtr); - return [formatter]() { return std::make_unique(formatter); }; + return std::make_shared(std::move(formatter)); } REGISTER_FACTORY(ActionFactory, Matcher::ActionFactory); diff --git a/source/extensions/matching/actions/format_string/config.h b/source/extensions/matching/actions/format_string/config.h index 359490c375238..0b893e2903031 100644 --- a/source/extensions/matching/actions/format_string/config.h +++ b/source/extensions/matching/actions/format_string/config.h @@ -17,7 +17,7 @@ namespace FormatString { class ActionImpl : public Matcher::ActionBase { public: - ActionImpl(const Formatter::FormatterConstSharedPtr& formatter) : formatter_(formatter) {} + ActionImpl(Formatter::FormatterConstSharedPtr formatter) : formatter_(std::move(formatter)) {} const Network::FilterChain* get(const Server::Configuration::FilterChainsByName& filter_chains_by_name, const StreamInfo::StreamInfo& info) const override; @@ -30,10 +30,9 @@ using FilterChainActionFactoryContext = Server::Configuration::ServerFactoryCont class ActionFactory : public Matcher::ActionFactory { public: std::string name() const override { return "envoy.matching.actions.format_string"; } - Matcher::ActionFactoryCb - createActionFactoryCb(const Protobuf::Message& proto_config, - FilterChainActionFactoryContext& context, - ProtobufMessage::ValidationVisitor& validator) override; + Matcher::ActionConstSharedPtr + createAction(const Protobuf::Message& proto_config, FilterChainActionFactoryContext& context, + ProtobufMessage::ValidationVisitor& validator) override; ProtobufTypes::MessagePtr createEmptyConfigProto() override { return std::make_unique(); } diff --git a/source/extensions/router/cluster_specifiers/matcher/matcher_cluster_specifier.cc b/source/extensions/router/cluster_specifiers/matcher/matcher_cluster_specifier.cc index 70d35193631e2..8553f75f0cde9 100644 --- a/source/extensions/router/cluster_specifiers/matcher/matcher_cluster_specifier.cc +++ b/source/extensions/router/cluster_specifiers/matcher/matcher_cluster_specifier.cc @@ -10,14 +10,12 @@ namespace Extensions { namespace Router { namespace Matcher { -Envoy::Matcher::ActionFactoryCb ClusterActionFactory::createActionFactoryCb( - const Protobuf::Message& config, ClusterActionContext&, - ProtobufMessage::ValidationVisitor& validation_visitor) { +Envoy::Matcher::ActionConstSharedPtr +ClusterActionFactory::createAction(const Protobuf::Message& config, ClusterActionContext&, + ProtobufMessage::ValidationVisitor& validation_visitor) { const auto& proto_config = MessageUtil::downcastAndValidate(config, validation_visitor); - auto cluster = std::make_shared(proto_config.cluster()); - - return [cluster]() { return std::make_unique(cluster); }; + return std::make_shared(proto_config.cluster()); } REGISTER_FACTORY(ClusterActionFactory, Envoy::Matcher::ActionFactory); @@ -43,9 +41,7 @@ class MatcherRouteEntry : public Envoy::Router::DelegatingRouteEntry { if (!match_result.isMatch()) { return; } - - const Envoy::Matcher::ActionPtr result = match_result.action(); - cluster_name_.emplace(result->getTyped().cluster()); + cluster_name_.emplace(match_result.action()->getTyped().cluster()); } private: diff --git a/source/extensions/router/cluster_specifiers/matcher/matcher_cluster_specifier.h b/source/extensions/router/cluster_specifiers/matcher/matcher_cluster_specifier.h index 52562d728cb0f..88e7746cea02b 100644 --- a/source/extensions/router/cluster_specifiers/matcher/matcher_cluster_specifier.h +++ b/source/extensions/router/cluster_specifiers/matcher/matcher_cluster_specifier.h @@ -26,21 +26,21 @@ struct ClusterActionContext {}; */ class ClusterAction : public Envoy::Matcher::ActionBase { public: - explicit ClusterAction(std::shared_ptr cluster) : cluster_(cluster) {} + explicit ClusterAction(absl::string_view cluster) : cluster_(cluster) {} - const std::string& cluster() const { return *cluster_; } + const std::string& cluster() const { return cluster_; } private: - const std::shared_ptr cluster_; + const std::string cluster_; }; // Registered factory for ClusterAction. This factory will be used to load proto configuration // from opaque config. class ClusterActionFactory : public Envoy::Matcher::ActionFactory { public: - Envoy::Matcher::ActionFactoryCb - createActionFactoryCb(const Protobuf::Message& config, ClusterActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) override; + Envoy::Matcher::ActionConstSharedPtr + createAction(const Protobuf::Message& config, ClusterActionContext& context, + ProtobufMessage::ValidationVisitor& validation_visitor) override; std::string name() const override { return "cluster"; } ProtobufTypes::MessagePtr createEmptyConfigProto() override { return std::make_unique(); diff --git a/test/common/matcher/exact_map_matcher_test.cc b/test/common/matcher/exact_map_matcher_test.cc index b635a25b3bf93..2dc71b5e0c4c1 100644 --- a/test/common/matcher/exact_map_matcher_test.cc +++ b/test/common/matcher/exact_map_matcher_test.cc @@ -109,7 +109,7 @@ TEST(ExactMapMatcherTest, RecursiveMatching) { std::make_unique( DataInputGetResult{DataInputGetResult::DataAvailability::AllDataAvailable, "match"}), stringOnMatch("no_match")); - matcher->addChild("match", OnMatch{/*.action_cb=*/nullptr, /*.matcher=*/sub_matcher, + matcher->addChild("match", OnMatch{/*.action_=*/nullptr, /*.matcher=*/sub_matcher, /*.keep_matching=*/false}); TestData data; @@ -127,7 +127,7 @@ TEST(ExactMapMatcherTest, RecursiveMatchingOnNoMatch) { std::unique_ptr> matcher = *ExactMapMatcher::create( std::make_unique( DataInputGetResult{DataInputGetResult::DataAvailability::AllDataAvailable, "blah"}), - OnMatch{/*.action_cb=*/nullptr, /*.matcher=*/sub_matcher, + OnMatch{/*.action_=*/nullptr, /*.matcher=*/sub_matcher, /*.keep_matching=*/false}); matcher->addChild("match", stringOnMatch("match")); @@ -156,14 +156,14 @@ TEST(ExactMapMatcherTest, RecursiveMatchingWithKeepMatching) { std::unique_ptr> matcher = *ExactMapMatcher::create( std::make_unique( DataInputGetResult{DataInputGetResult::DataAvailability::AllDataAvailable, "match"}), - OnMatch{/*.action_cb=*/nullptr, /*.matcher=*/top_on_no_match_matcher, + OnMatch{/*.action_=*/nullptr, /*.matcher=*/top_on_no_match_matcher, /*.keep_matching=*/false}); - matcher->addChild("match", OnMatch{/*.action_cb=*/nullptr, + matcher->addChild("match", OnMatch{/*.action_=*/nullptr, /*.matcher=*/sub_matcher_match_keeps_matching, /*.keep_matching=*/true}); - std::vector skipped_results{}; - SkippedMatchCb skipped_match_cb = [&skipped_results](ActionFactoryCb cb) { + std::vector skipped_results{}; + SkippedMatchCb skipped_match_cb = [&skipped_results](const ActionConstSharedPtr& cb) { skipped_results.push_back(cb); }; TestData data; diff --git a/test/common/matcher/list_matcher_test.cc b/test/common/matcher/list_matcher_test.cc index 42246922f1ccd..aa83f429e4bae 100644 --- a/test/common/matcher/list_matcher_test.cc +++ b/test/common/matcher/list_matcher_test.cc @@ -40,8 +40,8 @@ TEST(ListMatcherTest, KeepMatching) { matcher.addMatcher(createSingleMatcher("string", [](auto) { return true; }), stringOnMatch("matched", /*keep_matching=*/false)); - std::vector skipped_results; - SkippedMatchCb skipped_match_cb = [&skipped_results](ActionFactoryCb cb) { + std::vector skipped_results; + SkippedMatchCb skipped_match_cb = [&skipped_results](ActionConstSharedPtr cb) { skipped_results.push_back(cb); }; auto result = matcher.match(TestData(), skipped_match_cb); @@ -56,8 +56,8 @@ TEST(ListMatcherTest, KeepMatchingOnNoMatch) { matcher.addMatcher(createSingleMatcher("string", [](auto) { return true; }), stringOnMatch("keep matching 2", /*keep_matching=*/true)); - std::vector skipped_results; - SkippedMatchCb skipped_match_cb = [&skipped_results](const ActionFactoryCb cb) { + std::vector skipped_results; + SkippedMatchCb skipped_match_cb = [&skipped_results](const ActionConstSharedPtr cb) { skipped_results.push_back(cb); }; auto result = matcher.match(TestData(), skipped_match_cb); @@ -84,17 +84,17 @@ TEST(ListMatcherTest, KeepMatchingWithRecursion) { Envoy::Matcher::ListMatcher matcher(stringOnMatch("top_level on_no_match")); matcher.addMatcher(createSingleMatcher("string", [](auto) { return true; }), - OnMatch{/*.action_cb=*/nullptr, /*.matcher=*/sub_matcher_1, + OnMatch{/*.action_=*/nullptr, /*.matcher=*/sub_matcher_1, /*.keep_matching=*/false}); matcher.addMatcher(createSingleMatcher("string", [](auto) { return true; }), - OnMatch{/*.action_cb=*/nullptr, /*.matcher=*/sub_matcher_2, + OnMatch{/*.action_=*/nullptr, /*.matcher=*/sub_matcher_2, /*.keep_matching=*/true}); matcher.addMatcher(createSingleMatcher("string", [](auto) { return true; }), - OnMatch{/*.action_cb=*/nullptr, /*.matcher=*/sub_matcher_3, + OnMatch{/*.action_=*/nullptr, /*.matcher=*/sub_matcher_3, /*.keep_matching=*/false}); - std::vector skipped_results; - SkippedMatchCb skipped_match_cb = [&skipped_results](ActionFactoryCb cb) { + std::vector skipped_results; + SkippedMatchCb skipped_match_cb = [&skipped_results](ActionConstSharedPtr cb) { skipped_results.push_back(cb); }; MatchResult result = matcher.match(TestData(), skipped_match_cb); @@ -120,17 +120,17 @@ TEST(ListMatcherTest, KeepMatchingWithRecursiveOnNoMatch) { stringOnMatch("on_no_match sub match", /*keep_matching=*/true)); Envoy::Matcher::ListMatcher matcher( - OnMatch{/*action_cb=*/nullptr, + OnMatch{/*action_=*/nullptr, /*matcher=*/on_no_match_sub_matcher, /*keep_matching=*/false}); matcher.addMatcher( createSingleMatcher("string", [](auto) { return true; }), - OnMatch{/*action_cb=*/nullptr, /*matcher=*/sub_matcher_1, /*keep_matching=*/true}); + OnMatch{/*action_=*/nullptr, /*matcher=*/sub_matcher_1, /*keep_matching=*/true}); matcher.addMatcher( createSingleMatcher("string", [](auto) { return true; }), - OnMatch{/*action_cb=*/nullptr, /*matcher=*/sub_matcher_2, /*keep_matching=*/false}); + OnMatch{/*action_=*/nullptr, /*matcher=*/sub_matcher_2, /*keep_matching=*/false}); - std::vector skipped_results; - SkippedMatchCb skipped_match_cb = [&skipped_results](ActionFactoryCb cb) { + std::vector skipped_results; + SkippedMatchCb skipped_match_cb = [&skipped_results](ActionConstSharedPtr cb) { skipped_results.push_back(cb); }; MatchResult result = matcher.match(TestData(), skipped_match_cb); diff --git a/test/common/matcher/matcher_test.cc b/test/common/matcher/matcher_test.cc index d4072671da910..09b31e650f582 100644 --- a/test/common/matcher/matcher_test.cc +++ b/test/common/matcher/matcher_test.cc @@ -907,8 +907,8 @@ TEST_P(MatcherAmbiguousTest, KeepMatchingSupportInEvaluation) { validation_visitor_.setSupportKeepMatching(true); std::shared_ptr> matcher = createMatcherFromYaml(yaml)(); - std::vector skipped_results; - SkippedMatchCb skipped_match_cb = [&skipped_results](ActionFactoryCb cb) { + std::vector skipped_results; + SkippedMatchCb skipped_match_cb = [&skipped_results](ActionConstSharedPtr cb) { skipped_results.push_back(cb); }; const auto result = evaluateMatch(*matcher, TestData(), skipped_match_cb); @@ -1020,8 +1020,8 @@ TEST_P(MatcherAmbiguousTest, KeepMatchingWithRecursiveMatcher) { // Expect the nested matchers with keep_matching to be skipped and also the top-level // keep_matching setting to skip the result of the first sub-matcher. - std::vector skipped_results; - SkippedMatchCb skipped_match_cb = [&skipped_results](ActionFactoryCb cb) { + std::vector skipped_results; + SkippedMatchCb skipped_match_cb = [&skipped_results](ActionConstSharedPtr cb) { skipped_results.push_back(cb); }; MatchResult result = evaluateMatch(*matcher, TestData(), skipped_match_cb); @@ -1056,8 +1056,8 @@ TEST_P(MatcherAmbiguousTest, KeepMatchingWithUnsupportedReentry) { validation_visitor_.setSupportKeepMatching(true); std::shared_ptr> matcher = createMatcherFromYaml(yaml)(); - std::vector skipped_results; - SkippedMatchCb skipped_match_cb = [&skipped_results](ActionFactoryCb cb) { + std::vector skipped_results; + SkippedMatchCb skipped_match_cb = [&skipped_results](ActionConstSharedPtr cb) { skipped_results.push_back(cb); }; MatchResult result = evaluateMatch(*matcher, TestData(), skipped_match_cb); @@ -1130,12 +1130,12 @@ TEST_P(MatcherAmbiguousTest, KeepMatchingWithFailingNestedMatcher) { stringOnMatch("fail")); matcher->addMatcher(createSingleMatcher("string", [](auto) { return true; }), - OnMatch{/*.action_cb=*/nullptr, /*.matcher=*/nested_matcher, + OnMatch{/*.action_=*/nullptr, /*.matcher=*/nested_matcher, /*.keep_matching=*/true}); // Expect re-entry to fail due to the nested matcher. - std::vector skipped_results; - SkippedMatchCb skipped_match_cb = [&skipped_results](ActionFactoryCb cb) { + std::vector skipped_results; + SkippedMatchCb skipped_match_cb = [&skipped_results](ActionConstSharedPtr cb) { skipped_results.push_back(cb); }; MatchResult result = evaluateMatch(*matcher, TestData(), skipped_match_cb); diff --git a/test/common/matcher/test_utility.h b/test/common/matcher/test_utility.h index 808decee787bc..7eeec55e96897 100644 --- a/test/common/matcher/test_utility.h +++ b/test/common/matcher/test_utility.h @@ -162,10 +162,10 @@ struct StringAction : public ActionBase { // Factory for StringAction. class StringActionFactory : public ActionFactory { public: - ActionFactoryCb createActionFactoryCb(const Protobuf::Message& config, absl::string_view&, - ProtobufMessage::ValidationVisitor&) override { + ActionConstSharedPtr createAction(const Protobuf::Message& config, absl::string_view&, + ProtobufMessage::ValidationVisitor&) override { const auto& string = dynamic_cast(config); - return [string]() { return std::make_unique(string.value()); }; + return std::make_shared(string.value()); } ProtobufTypes::MessagePtr createEmptyConfigProto() override { @@ -273,14 +273,9 @@ void PrintTo(const FieldMatchResult& result, std::ostream* os) { } } -// Creates a StringAction from a provided string. -std::unique_ptr stringValue(absl::string_view value) { - return std::make_unique(std::string(value)); -} - // Creates an OnMatch that evaluates to a StringValue with the provided value. template OnMatch stringOnMatch(absl::string_view value, bool keep_matching = false) { - return OnMatch{[s = std::string(value)]() { return stringValue(s); }, nullptr, keep_matching}; + return OnMatch{std::make_shared(std::string(value)), nullptr, keep_matching}; } inline void PrintTo(const Action& action, std::ostream* os) { @@ -291,23 +286,14 @@ inline void PrintTo(const Action& action, std::ostream* os) { *os << "{type=" << action.typeUrl() << "}"; } -inline void PrintTo(const ActionFactoryCb& action_cb, std::ostream* os) { - if (action_cb == nullptr) { - *os << "nullptr"; - return; - } - ActionPtr action = action_cb(); - PrintTo(*action, os); -} - inline void PrintTo(const MatchResult& result, std::ostream* os) { if (result.isInsufficientData()) { *os << "InsufficientData"; } else if (result.isNoMatch()) { *os << "NoMatch"; } else if (result.isMatch()) { - *os << "Match{ActionFactoryCb="; - PrintTo(result.actionFactory(), os); + *os << "Match{Action="; + PrintTo(*result.action(), os); *os << "}"; } else { *os << "UnknownState"; @@ -319,9 +305,9 @@ inline void PrintTo(const MatchTree& matcher, std::ostream* os) { } inline void PrintTo(const OnMatch& on_match, std::ostream* os) { - if (on_match.action_cb_) { - *os << "{action_cb_="; - PrintTo(on_match.action_cb_, os); + if (on_match.action_) { + *os << "{action_="; + PrintTo(on_match.action_, os); *os << "}"; } else if (on_match.matcher_) { *os << "{matcher_="; @@ -339,26 +325,25 @@ MATCHER(HasInsufficientData, "") { } MATCHER_P(IsActionWithType, matcher, "") { - // Takes an ActionFactoryCb argument, and compares its action type against matcher. + // Takes an ActionConstSharedPtr argument, and compares its action type against matcher. if (arg == nullptr) { return false; } - ActionPtr action = arg(); - return ::testing::ExplainMatchResult(testing::Matcher(matcher), - action->typeUrl(), result_listener); + return ::testing::ExplainMatchResult(testing::Matcher(matcher), arg->typeUrl(), + result_listener); } MATCHER_P(IsStringAction, matcher, "") { - // Takes an ActionFactoryCb argument, and compares its StringAction's string against matcher. + // Takes an ActionConstSharedPtr argument, and compares its StringAction's string against matcher. if (arg == nullptr) { return false; } - ActionPtr action = arg(); - if (action->typeUrl() != "google.protobuf.StringValue") { + + if (arg->typeUrl() != "google.protobuf.StringValue") { return false; } return ::testing::ExplainMatchResult(testing::Matcher(matcher), - action->template getTyped().string_, + arg->template getTyped().string_, result_listener); } @@ -368,8 +353,7 @@ MATCHER_P(HasStringAction, matcher, "") { if (!arg.isMatch()) { return false; } - return ::testing::ExplainMatchResult(IsStringAction(matcher), arg.actionFactory(), - result_listener); + return ::testing::ExplainMatchResult(IsStringAction(matcher), arg.action(), result_listener); } MATCHER_P(HasActionWithType, matcher, "") { @@ -378,8 +362,7 @@ MATCHER_P(HasActionWithType, matcher, "") { if (!arg.isMatch()) { return false; } - return ::testing::ExplainMatchResult(IsActionWithType(matcher), arg.actionFactory(), - result_listener); + return ::testing::ExplainMatchResult(IsActionWithType(matcher), arg.action(), result_listener); } MATCHER(HasNoMatch, "") { diff --git a/test/extensions/common/matcher/domain_matcher_test.cc b/test/extensions/common/matcher/domain_matcher_test.cc index 1ea833d294e1c..20e57359201d7 100644 --- a/test/extensions/common/matcher/domain_matcher_test.cc +++ b/test/extensions/common/matcher/domain_matcher_test.cc @@ -38,7 +38,6 @@ using ::Envoy::Matcher::MatchResult; using ::Envoy::Matcher::MatchTreeFactory; using ::Envoy::Matcher::MockMatchTreeValidationVisitor; using ::Envoy::Matcher::SkippedMatchCb; -using ::Envoy::Matcher::StringAction; using ::Envoy::Matcher::StringActionFactory; using ::Envoy::Matcher::TestData; using ::Envoy::Matcher::TestDataInputBoolFactory; @@ -625,8 +624,8 @@ TEST_F(DomainMatcherTest, KeepMatchingSupport) { loadConfig(yaml); validation_visitor_.setSupportKeepMatching(true); - std::vector skipped_results; - skipped_match_cb_ = [&skipped_results](Envoy::Matcher::ActionFactoryCb cb) { + std::vector skipped_results; + skipped_match_cb_ = [&skipped_results](const Envoy::Matcher::ActionConstSharedPtr& cb) { skipped_results.push_back(cb); }; diff --git a/test/extensions/common/matcher/trie_matcher_test.cc b/test/extensions/common/matcher/trie_matcher_test.cc index 02f40777e9db2..d7a4536b5fba9 100644 --- a/test/extensions/common/matcher/trie_matcher_test.cc +++ b/test/extensions/common/matcher/trie_matcher_test.cc @@ -27,8 +27,8 @@ namespace Common { namespace Matcher { namespace { +using ::Envoy::Matcher::ActionConstSharedPtr; using ::Envoy::Matcher::ActionFactory; -using ::Envoy::Matcher::ActionFactoryCb; using ::Envoy::Matcher::CustomMatcherFactory; using ::Envoy::Matcher::DataInputGetResult; using ::Envoy::Matcher::HasInsufficientData; @@ -36,12 +36,10 @@ using ::Envoy::Matcher::HasNoMatch; using ::Envoy::Matcher::HasStringAction; using ::Envoy::Matcher::IsStringAction; using ::Envoy::Matcher::MatchResult; -using ::Envoy::Matcher::MatchTree; using ::Envoy::Matcher::MatchTreeFactory; using ::Envoy::Matcher::MatchTreePtr; using ::Envoy::Matcher::MatchTreeSharedPtr; using ::Envoy::Matcher::MockMatchTreeValidationVisitor; -using ::Envoy::Matcher::StringAction; using ::Envoy::Matcher::StringActionFactory; using ::Envoy::Matcher::TestData; using ::Envoy::Matcher::TestDataInputBoolFactory; @@ -604,8 +602,10 @@ TEST_F(TrieMatcherTest, ExerciseKeepMatching) { // Skip baz because the nested matcher is set with keep_matching. // Skip bag because the nested matcher returns on_no_match, but the top-level matcher is set to // keep_matching. - std::vector skipped_results{}; - skipped_match_cb_ = [&skipped_results](ActionFactoryCb cb) { skipped_results.push_back(cb); }; + std::vector skipped_results{}; + skipped_match_cb_ = [&skipped_results](const ActionConstSharedPtr& cb) { + skipped_results.push_back(cb); + }; auto input = TestDataInputStringFactory("192.101.0.1"); auto nested = TestDataInputBoolFactory("baz"); diff --git a/test/extensions/filters/http/composite/filter_test.cc b/test/extensions/filters/http/composite/filter_test.cc index d37bd319eae93..03ecafba79106 100644 --- a/test/extensions/filters/http/composite/filter_test.cc +++ b/test/extensions/filters/http/composite/filter_test.cc @@ -86,6 +86,8 @@ class CompositeFilterTest : public ::testing::Test { EXPECT_TRUE(MessageDifferencer::Equals(expected, *(info->serializeAsProto()))); } + testing::NiceMock context_; + testing::NiceMock decoder_callbacks_; testing::NiceMock encoder_callbacks_; Stats::MockCounter error_counter_; @@ -114,12 +116,13 @@ TEST_F(FilterTest, StreamEncoderFilterDelegation) { ON_CALL(encoder_callbacks_.stream_info_, filterState()) .WillByDefault(testing::ReturnRef(filter_state)); - auto factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { + Http::FilterFactoryCb factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { cb.addStreamEncoderFilter(stream_filter); }; EXPECT_CALL(*stream_filter, setEncoderFilterCallbacks(_)); - ExecuteFilterAction action(factory_callback, "actionName"); + ExecuteFilterAction action([&]() -> OptRef { return factory_callback; }, + "actionName", absl::nullopt, context_.runtime_loader_); EXPECT_CALL(success_counter_, inc()); filter_.onMatchCallback(action); @@ -143,12 +146,13 @@ TEST_F(FilterTest, StreamDecoderFilterDelegation) { ON_CALL(decoder_callbacks_.stream_info_, filterState()) .WillByDefault(testing::ReturnRef(filter_state)); - auto factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { + Http::FilterFactoryCb factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { cb.addStreamDecoderFilter(stream_filter); }; EXPECT_CALL(*stream_filter, setDecoderFilterCallbacks(_)); - ExecuteFilterAction action(factory_callback, "actionName"); + ExecuteFilterAction action([&]() -> OptRef { return factory_callback; }, + "actionName", absl::nullopt, context_.runtime_loader_); EXPECT_CALL(success_counter_, inc()); filter_.onMatchCallback(action); @@ -171,14 +175,15 @@ TEST_F(FilterTest, StreamFilterDelegation) { ON_CALL(decoder_callbacks_.stream_info_, filterState()) .WillByDefault(testing::ReturnRef(filter_state)); - auto factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { + Http::FilterFactoryCb factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { cb.addStreamFilter(stream_filter); }; EXPECT_CALL(*stream_filter, setDecoderFilterCallbacks(_)); EXPECT_CALL(*stream_filter, setEncoderFilterCallbacks(_)); EXPECT_CALL(success_counter_, inc()); - ExecuteFilterAction action(factory_callback, "actionName"); + ExecuteFilterAction action([&]() -> OptRef { return factory_callback; }, + "actionName", absl::nullopt, context_.runtime_loader_); filter_.onMatchCallback(action); expectFilterStateInfo(filter_state); @@ -200,12 +205,13 @@ TEST_F(FilterTest, StreamFilterDelegationMultipleStreamFilters) { ON_CALL(decoder_callbacks_.stream_info_, filterState()) .WillByDefault(testing::ReturnRef(filter_state)); - auto factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { + Http::FilterFactoryCb factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { cb.addStreamFilter(stream_filter); cb.addStreamFilter(stream_filter); }; - ExecuteFilterAction action(factory_callback, "actionName"); + ExecuteFilterAction action([&]() -> OptRef { return factory_callback; }, + "actionName", absl::nullopt, context_.runtime_loader_); EXPECT_CALL(error_counter_, inc()); filter_.onMatchCallback(action); @@ -225,12 +231,13 @@ TEST_F(FilterTest, StreamFilterDelegationMultipleStreamDecoderFilters) { ON_CALL(decoder_callbacks_.stream_info_, filterState()) .WillByDefault(testing::ReturnRef(filter_state)); - auto factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { + Http::FilterFactoryCb factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { cb.addStreamDecoderFilter(decoder_filter); cb.addStreamDecoderFilter(decoder_filter); }; - ExecuteFilterAction action(factory_callback, "actionName"); + ExecuteFilterAction action([&]() -> OptRef { return factory_callback; }, + "actionName", absl::nullopt, context_.runtime_loader_); EXPECT_CALL(error_counter_, inc()); filter_.onMatchCallback(action); @@ -250,12 +257,13 @@ TEST_F(FilterTest, StreamFilterDelegationMultipleStreamEncoderFilters) { ON_CALL(encoder_callbacks_.stream_info_, filterState()) .WillByDefault(testing::ReturnRef(filter_state)); - auto factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { + Http::FilterFactoryCb factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { cb.addStreamEncoderFilter(encode_filter); cb.addStreamEncoderFilter(encode_filter); }; - ExecuteFilterAction action(factory_callback, "actionName"); + ExecuteFilterAction action([&]() -> OptRef { return factory_callback; }, + "actionName", absl::nullopt, context_.runtime_loader_); EXPECT_CALL(error_counter_, inc()); filter_.onMatchCallback(action); @@ -278,13 +286,14 @@ TEST_F(FilterTest, StreamFilterDelegationMultipleAccessLoggers) { ON_CALL(encoder_callbacks_.stream_info_, filterState()) .WillByDefault(testing::ReturnRef(filter_state)); - auto factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { + Http::FilterFactoryCb factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { cb.addStreamEncoderFilter(encode_filter); cb.addAccessLogHandler(access_log_1); cb.addAccessLogHandler(access_log_2); }; - ExecuteFilterAction action(factory_callback, "actionName"); + ExecuteFilterAction action([&]() -> OptRef { return factory_callback; }, + "actionName", absl::nullopt, context_.runtime_loader_); EXPECT_CALL(*encode_filter, setEncoderFilterCallbacks(_)); EXPECT_CALL(success_counter_, inc()); filter_.onMatchCallback(action); @@ -340,8 +349,7 @@ TEST(ConfigTest, TestConfig) { .server_factory_context_ = server_factory_context}; ExecuteFilterActionFactory factory; EXPECT_THROW_WITH_MESSAGE( - factory.createActionFactoryCb(config, action_context, - ProtobufMessage::getStrictValidationVisitor()), + factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()), EnvoyException, "Error: Only one of `dynamic_config` or `typed_config` can be set."); } } @@ -371,8 +379,7 @@ TEST(ConfigTest, TestDynamicConfigInDownstream) { .server_factory_context_ = server_factory_context}; ExecuteFilterActionFactory factory; EXPECT_THROW_WITH_MESSAGE( - factory.createActionFactoryCb(config, action_context, - ProtobufMessage::getStrictValidationVisitor()), + factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()), EnvoyException, "Failed to get downstream factory context or server factory context."); } @@ -400,8 +407,7 @@ TEST(ConfigTest, TestDynamicConfigInUpstream) { .server_factory_context_ = absl::nullopt}; ExecuteFilterActionFactory factory; EXPECT_THROW_WITH_MESSAGE( - factory.createActionFactoryCb(config, action_context, - ProtobufMessage::getStrictValidationVisitor()), + factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()), EnvoyException, "Failed to get upstream factory context or server factory context."); } @@ -428,8 +434,7 @@ TEST(ConfigTest, CreateFilterFromServerContextDual) { .server_factory_context_ = server_factory_context}; ExecuteFilterActionFactory factory; EXPECT_THROW_WITH_MESSAGE( - factory.createActionFactoryCb(config, action_context, - ProtobufMessage::getStrictValidationVisitor()), + factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()), EnvoyException, "DualFactoryBase: creating filter factory from server factory context is not supported"); } @@ -456,8 +461,7 @@ TEST(ConfigTest, DualFilterNoUpstreamFactoryContext) { .server_factory_context_ = server_factory_context}; ExecuteFilterActionFactory factory; EXPECT_THROW_WITH_MESSAGE( - factory.createActionFactoryCb(config, action_context, - ProtobufMessage::getStrictValidationVisitor()), + factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()), EnvoyException, "Failed to get upstream filter factory creation function"); } @@ -482,8 +486,7 @@ TEST(ConfigTest, DownstreamFilterNoFactoryContext) { .server_factory_context_ = absl::nullopt}; ExecuteFilterActionFactory factory; EXPECT_THROW_WITH_MESSAGE( - factory.createActionFactoryCb(config, action_context, - ProtobufMessage::getStrictValidationVisitor()), + factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()), EnvoyException, "Failed to get downstream filter factory creation function"); } @@ -510,8 +513,7 @@ TEST(ConfigTest, TestDownstreamFilterNoOverridingServerContext) { .server_factory_context_ = server_factory_context}; ExecuteFilterActionFactory factory; EXPECT_THROW_WITH_MESSAGE( - factory.createActionFactoryCb(config, action_context, - ProtobufMessage::getStrictValidationVisitor()), + factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()), EnvoyException, "Creating filter factory from server factory context is not supported"); } @@ -526,12 +528,23 @@ TEST(ConfigTest, TestSamplePercentNotSpecifiedl) { http_status: 503 )EOF"; - testing::NiceMock server_factory_context; - NiceMock& runtime = server_factory_context.runtime_loader_; envoy::extensions::filters::http::composite::v3::ExecuteFilterAction config; TestUtility::loadFromYaml(yaml_string, config); + + testing::NiceMock server_factory_context; + testing::NiceMock factory_context; + testing::NiceMock upstream_factory_context; + Envoy::Http::Matching::HttpFilterActionContext action_context{ + .is_downstream_ = true, + .stat_prefix_ = "test", + .factory_context_ = absl::nullopt, + .upstream_factory_context_ = absl::nullopt, + .server_factory_context_ = server_factory_context}; ExecuteFilterActionFactory factory; - EXPECT_TRUE(factory.isSampled(config, runtime)); + auto action = + factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()); + + EXPECT_FALSE(action->getTyped().actionSkip()); } // Config test to check if sample_percent config is in place and feature enabled. @@ -549,15 +562,27 @@ TEST(ConfigTest, TestSamplePercentInPlaceFeatureEnabled) { denominator: HUNDRED )EOF"; - testing::NiceMock server_factory_context; - NiceMock& runtime = server_factory_context.runtime_loader_; envoy::extensions::filters::http::composite::v3::ExecuteFilterAction config; TestUtility::loadFromYaml(yaml_string, config); + + testing::NiceMock server_factory_context; + testing::NiceMock factory_context; + testing::NiceMock upstream_factory_context; + Envoy::Http::Matching::HttpFilterActionContext action_context{ + .is_downstream_ = true, + .stat_prefix_ = "test", + .factory_context_ = absl::nullopt, + .upstream_factory_context_ = absl::nullopt, + .server_factory_context_ = server_factory_context}; ExecuteFilterActionFactory factory; - EXPECT_CALL(runtime.snapshot_, + auto action = + factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()); + + EXPECT_CALL(server_factory_context.runtime_loader_.snapshot_, featureEnabled(_, testing::A())) .WillOnce(testing::Return(true)); - EXPECT_TRUE(factory.isSampled(config, runtime)); + + EXPECT_FALSE(action->getTyped().actionSkip()); } // Config test to check if sample_percent config is in place and feature not enabled. @@ -570,18 +595,32 @@ TEST(ConfigTest, TestSamplePercentInPlaceFeatureNotEnabled) { abort: http_status: 503 sample_percent: - runtime_key: + default_value: + numerator: 30 + denominator: HUNDRED )EOF"; - testing::NiceMock server_factory_context; - NiceMock& runtime = server_factory_context.runtime_loader_; envoy::extensions::filters::http::composite::v3::ExecuteFilterAction config; TestUtility::loadFromYaml(yaml_string, config); + + testing::NiceMock server_factory_context; + testing::NiceMock factory_context; + testing::NiceMock upstream_factory_context; + Envoy::Http::Matching::HttpFilterActionContext action_context{ + .is_downstream_ = true, + .stat_prefix_ = "test", + .factory_context_ = absl::nullopt, + .upstream_factory_context_ = absl::nullopt, + .server_factory_context_ = server_factory_context}; ExecuteFilterActionFactory factory; - EXPECT_CALL(runtime.snapshot_, + auto action = + factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()); + + EXPECT_CALL(server_factory_context.runtime_loader_.snapshot_, featureEnabled(_, testing::A())) .WillOnce(testing::Return(false)); - EXPECT_FALSE(factory.isSampled(config, runtime)); + + EXPECT_TRUE(action->getTyped().actionSkip()); } TEST_F(FilterTest, FilterStateShouldBeUpdatedWithTheMatchingActionForDynamicConfig) { @@ -608,8 +647,8 @@ TEST_F(FilterTest, FilterStateShouldBeUpdatedWithTheMatchingActionForDynamicConf .upstream_factory_context_ = upstream_factory_context, .server_factory_context_ = server_factory_context}; ExecuteFilterActionFactory factory; - auto action = factory.createActionFactoryCb(config, action_context, - ProtobufMessage::getStrictValidationVisitor())(); + auto action = + factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()); EXPECT_EQ("actionName", action->getTyped().actionName()); } @@ -639,8 +678,8 @@ TEST_F(FilterTest, FilterStateShouldBeUpdatedWithTheMatchingActionForTypedConfig .upstream_factory_context_ = upstream_factory_context, .server_factory_context_ = server_factory_context}; ExecuteFilterActionFactory factory; - auto action = factory.createActionFactoryCb(config, action_context, - ProtobufMessage::getStrictValidationVisitor())(); + auto action = + factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()); EXPECT_EQ("actionName", action->getTyped().actionName()); } @@ -658,12 +697,13 @@ TEST_F(FilterTest, FilterStateShouldBeUpdatedWithTheMatchingAction) { StreamInfo::FilterState::StateType::Mutable, StreamInfo::FilterState::LifeSpan::FilterChain); - auto factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { + Http::FilterFactoryCb factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { cb.addStreamEncoderFilter(stream_filter); }; EXPECT_CALL(*stream_filter, setEncoderFilterCallbacks(_)); - ExecuteFilterAction action(factory_callback, "actionName"); + ExecuteFilterAction action([&]() -> OptRef { return factory_callback; }, + "actionName", absl::nullopt, context_.runtime_loader_); EXPECT_CALL(success_counter_, inc()); filter_.onMatchCallback(action); @@ -688,12 +728,13 @@ TEST_F(FilterTest, MatchingActionShouldNotCollitionWithOtherRootFilter) { StreamInfo::FilterState::StateType::Mutable, StreamInfo::FilterState::LifeSpan::FilterChain); - auto factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { + Http::FilterFactoryCb factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { cb.addStreamEncoderFilter(stream_filter); }; EXPECT_CALL(*stream_filter, setEncoderFilterCallbacks(_)); - ExecuteFilterAction action(factory_callback, "actionName"); + ExecuteFilterAction action([&]() -> OptRef { return factory_callback; }, + "actionName", absl::nullopt, context_.runtime_loader_); EXPECT_CALL(success_counter_, inc()); filter_.onMatchCallback(action); @@ -724,12 +765,13 @@ TEST_F(UpstreamFilterTest, StreamEncoderFilterDelegationUpstream) { ON_CALL(encoder_callbacks_.stream_info_, filterState()) .WillByDefault(testing::ReturnRef(filter_state)); - auto factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { + Http::FilterFactoryCb factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { cb.addStreamEncoderFilter(stream_filter); }; EXPECT_CALL(*stream_filter, setEncoderFilterCallbacks(_)); - ExecuteFilterAction action(factory_callback, "actionName"); + ExecuteFilterAction action([&]() -> OptRef { return factory_callback; }, + "actionName", absl::nullopt, context_.runtime_loader_); EXPECT_CALL(success_counter_, inc()); filter_.onMatchCallback(action); diff --git a/test/extensions/filters/http/match_delegate/config_test.cc b/test/extensions/filters/http/match_delegate/config_test.cc index 0b63d20d77450..2e032c3953952 100644 --- a/test/extensions/filters/http/match_delegate/config_test.cc +++ b/test/extensions/filters/http/match_delegate/config_test.cc @@ -302,7 +302,7 @@ createMatchingTree(const std::string& name, const std::string& value) { std::make_unique(name), absl::nullopt); tree->addChild(value, Matcher::OnMatch{ - []() { return std::make_unique(); }, nullptr, false}); + std::make_shared(), nullptr, false}); return tree; } @@ -315,7 +315,7 @@ Matcher::MatchTreeSharedPtr createRequestAndRespo tree->addChild( "match", Matcher::OnMatch{ - []() { return std::make_unique(); }, + std::make_shared(), createMatchingTree( "match-header", "match"), false}); @@ -369,13 +369,11 @@ template Matcher::MatchTreeSharedPtr createMatchTreeWithOnNoMatch(const std::string& name, const std::string& value) { auto tree = *Matcher::ExactMapMatcher::create( - std::make_unique(name), - Matcher::OnMatch{ - []() { return std::make_unique(); }, nullptr, false}); + std::make_unique(name), Matcher::OnMatch{ + std::make_shared(), nullptr, false}); // No action is set on match. i.e., nullptr action factory cb. - tree->addChild(value, Matcher::OnMatch{[]() { return nullptr; }, - nullptr, false}); + tree->addChild(value, Matcher::OnMatch{nullptr, nullptr, false}); return tree; } diff --git a/test/extensions/filters/http/rate_limit_quota/filter_test.cc b/test/extensions/filters/http/rate_limit_quota/filter_test.cc index 96cdd58b18195..20b860a1a3704 100644 --- a/test/extensions/filters/http/rate_limit_quota/filter_test.cc +++ b/test/extensions/filters/http/rate_limit_quota/filter_test.cc @@ -150,7 +150,7 @@ class FilterTest : public testing::Test { ASSERT_TRUE(match_result.ok()); // Retrieve the matched action. const RateLimitOnMatchAction* match_action = - dynamic_cast(match_result.value().get()); + dynamic_cast(match_result.value().get()); RateLimitQuotaValidationVisitor visitor = {}; // Generate the bucket ids. @@ -277,7 +277,7 @@ TEST_F(FilterTest, RequestMatchingWithInvalidOnNoMatch) { ASSERT_TRUE(match_result.ok()); // Retrieve the matched action. const RateLimitOnMatchAction* match_action = - dynamic_cast(match_result.value().get()); + dynamic_cast(match_result.value().get()); RateLimitQuotaValidationVisitor visitor = {}; // Generate the bucket ids. diff --git a/test/extensions/filters/network/generic_proxy/route_test.cc b/test/extensions/filters/network/generic_proxy/route_test.cc index 56aa14d4e114b..1ee03a8bdd915 100644 --- a/test/extensions/filters/network/generic_proxy/route_test.cc +++ b/test/extensions/filters/network/generic_proxy/route_test.cc @@ -279,16 +279,6 @@ TEST_F(RouteEntryImplTest, NullRouteSpecificConfig) { EXPECT_EQ(route_->perFilterConfig("envoy.filters.generic.mock_filter"), nullptr); }; -/** - * Test the simple route action wrapper. - */ -TEST(RouteMatchActionTest, SimpleRouteMatchActionTest) { - auto entry = std::make_shared>(); - RouteMatchAction action(entry); - - EXPECT_EQ(action.route().get(), entry.get()); -} - /** * Test the simple data input validator. */ @@ -321,13 +311,11 @@ TEST(RouteMatchActionFactoryTest, SimpleRouteMatchActionFactoryTest) { TestUtility::loadFromYaml(yaml_config, proto_config); RouteActionContext context{server_context}; - auto factory_cb = factory.createActionFactoryCb(proto_config, context, - server_context.messageValidationVisitor()); - - EXPECT_EQ(factory_cb()->getTyped().route().get(), - factory_cb()->getTyped().route().get()); + auto action = + factory.createAction(proto_config, context, server_context.messageValidationVisitor()); - EXPECT_EQ(factory_cb()->getTyped().route()->clusterName(), "cluster_0"); + EXPECT_NE(action, nullptr); + EXPECT_EQ(action->getTyped().clusterName(), "cluster_0"); } class RouteMatcherImplTest : public testing::Test { diff --git a/test/extensions/filters/network/match_delegate/config_test.cc b/test/extensions/filters/network/match_delegate/config_test.cc index 574b886536cef..fd283fb15e51c 100644 --- a/test/extensions/filters/network/match_delegate/config_test.cc +++ b/test/extensions/filters/network/match_delegate/config_test.cc @@ -243,7 +243,7 @@ createMatchingTree(const std::string& name, const std::string& value) { std::make_unique(name), absl::nullopt); tree->addChild(value, Matcher::OnMatch{ - []() { return std::make_unique(); }, nullptr, false}); + std::make_shared(), nullptr, false}); return tree; } @@ -394,7 +394,7 @@ createMatchingTreeWithTestAction(const std::string& name, const std::string& val std::make_unique(name), absl::nullopt); tree->addChild(value, Matcher::OnMatch{ - []() { return std::make_unique(); }, nullptr, false}); + std::make_shared(), nullptr, false}); return tree; } diff --git a/test/extensions/matching/actions/format_string/config_test.cc b/test/extensions/matching/actions/format_string/config_test.cc index 8bbb3027572fe..7028674e75f2d 100644 --- a/test/extensions/matching/actions/format_string/config_test.cc +++ b/test/extensions/matching/actions/format_string/config_test.cc @@ -23,10 +23,8 @@ TEST(ConfigTest, TestConfig) { testing::NiceMock factory_context; ActionFactory factory; - auto action_cb = factory.createActionFactoryCb(config, factory_context, - ProtobufMessage::getStrictValidationVisitor()); - ASSERT_NE(nullptr, action_cb); - auto action = action_cb(); + auto action = + factory.createAction(config, factory_context, ProtobufMessage::getStrictValidationVisitor()); ASSERT_NE(nullptr, action); const auto& typed_action = action->getTyped(); diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index b419480bd4c25..a1dc887af4abb 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -141,7 +141,7 @@ class MockStreamCallbacks : public StreamCallbacks { class MockCodecEventCallbacks : public CodecEventCallbacks { public: MockCodecEventCallbacks(); - ~MockCodecEventCallbacks(); + ~MockCodecEventCallbacks() override; MOCK_METHOD(void, onCodecEncodeComplete, ()); MOCK_METHOD(void, onCodecLowLevelReset, ()); @@ -343,7 +343,7 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, MOCK_METHOD(bool, shouldLoadShed, (), (const)); Buffer::InstancePtr buffer_; - std::list callbacks_{}; + std::list callbacks_; testing::NiceMock downstream_callbacks_; testing::NiceMock active_span_; testing::NiceMock tracing_config_;