diff --git a/.github/workflows/codeql-daily.yml b/.github/workflows/codeql-daily.yml index fb09e96cffbca..0449e0cfac1e1 100644 --- a/.github/workflows/codeql-daily.yml +++ b/.github/workflows/codeql-daily.yml @@ -34,7 +34,7 @@ jobs: # Initializes the CodeQL tools for scanning. - name: Initialize CodeQL - uses: github/codeql-action/init@181d5eefc20863364f96762470ba6f862bdef56b # codeql-bundle-v3.29.2 + uses: github/codeql-action/init@d6bbdef45e766d081b84a2def353b0055f728d3e # codeql-bundle-v3.29.3 # Override language selection by uncommenting this and choosing your languages with: languages: cpp @@ -75,6 +75,6 @@ jobs: git clean -xdf - name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@181d5eefc20863364f96762470ba6f862bdef56b # codeql-bundle-v3.29.2 + uses: github/codeql-action/analyze@d6bbdef45e766d081b84a2def353b0055f728d3e # codeql-bundle-v3.29.3 with: trap-caching: false diff --git a/.github/workflows/codeql-push.yml b/.github/workflows/codeql-push.yml index 60104adf03224..354c5253fa1dd 100644 --- a/.github/workflows/codeql-push.yml +++ b/.github/workflows/codeql-push.yml @@ -67,7 +67,7 @@ jobs: - name: Initialize CodeQL if: ${{ env.BUILD_TARGETS != '' }} - uses: github/codeql-action/init@181d5eefc20863364f96762470ba6f862bdef56b # codeql-bundle-v3.29.2 + uses: github/codeql-action/init@d6bbdef45e766d081b84a2def353b0055f728d3e # codeql-bundle-v3.29.3 with: languages: cpp trap-caching: false @@ -112,6 +112,6 @@ jobs: - name: Perform CodeQL Analysis if: ${{ env.BUILD_TARGETS != '' }} - uses: github/codeql-action/analyze@181d5eefc20863364f96762470ba6f862bdef56b # codeql-bundle-v3.29.2 + uses: github/codeql-action/analyze@d6bbdef45e766d081b84a2def353b0055f728d3e # codeql-bundle-v3.29.3 with: trap-caching: false diff --git a/.github/workflows/scorecard.yml b/.github/workflows/scorecard.yml index 40676cd78d81a..276280caaaa72 100644 --- a/.github/workflows/scorecard.yml +++ b/.github/workflows/scorecard.yml @@ -40,6 +40,6 @@ jobs: retention-days: 5 - name: "Upload to code-scanning" - uses: github/codeql-action/upload-sarif@181d5eefc20863364f96762470ba6f862bdef56b # v3.29.2 + uses: github/codeql-action/upload-sarif@d6bbdef45e766d081b84a2def353b0055f728d3e # v3.29.3 with: sarif_file: results.sarif diff --git a/envoy/matcher/matcher.h b/envoy/matcher/matcher.h index 531f99cf15bc4..223ed538e5952 100644 --- a/envoy/matcher/matcher.h +++ b/envoy/matcher/matcher.h @@ -91,20 +91,22 @@ class Action { } }; -using ActionConstSharedPtr = std::shared_ptr; +using ActionPtr = std::unique_ptr; +using ActionFactoryCb = std::function; template class ActionFactory : public Config::TypedFactory { public: - virtual ActionConstSharedPtr - createAction(const Protobuf::Message& config, ActionFactoryContext& action_factory_context, - ProtobufMessage::ValidationVisitor& validation_visitor) PURE; + virtual ActionFactoryCb + createActionFactoryCb(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 ActionConstSharedPtr action_; + const ActionFactoryCb action_cb_; const MatchTreeSharedPtr matcher_; bool keep_matching_{}; }; @@ -128,39 +130,30 @@ 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 -// ActionConstSharedPtr.) +// ActionFactoryCb.) struct MatchResult { public: - MatchResult(ActionConstSharedPtr cb) : result_(std::move(cb)) {} + MatchResult(ActionFactoryCb 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_); } - 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_)); - } + bool isMatch() const { return absl::holds_alternative(result_); } + ActionFactoryCb actionFactory() const { return absl::get(result_); } + ActionPtr action() const { return actionFactory()(); } 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. */ @@ -194,19 +187,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.action()); + skipped_match_cb(nested_result.actionFactory()); } return MatchResult::noMatch(); } return nested_result; } - if (on_match->action_ && on_match->keep_matching_) { + if (on_match->action_cb_ && on_match->keep_matching_) { if (skipped_match_cb) { - skipped_match_cb(on_match->action_); + skipped_match_cb(on_match->action_cb_); } return MatchResult::noMatch(); } - return MatchResult{on_match->action_}; + return MatchResult{on_match->action_cb_}; } }; diff --git a/source/common/listener_manager/filter_chain_manager_impl.cc b/source/common/listener_manager/filter_chain_manager_impl.cc index b9f6d9eede3ba..7261bea02234a 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::ActionConstSharedPtr createAction(const Protobuf::Message& config, - FilterChainActionFactoryContext&, - ProtobufMessage::ValidationVisitor&) override { - return std::make_shared( - dynamic_cast(config).value()); + 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); }; } ProtobufTypes::MessagePtr createEmptyConfigProto() override { return std::make_unique(); @@ -573,8 +573,9 @@ FilterChainManagerImpl::findFilterChainUsingMatcher(const Network::ConnectionSoc Matcher::evaluateMatch(*matcher_, data); ASSERT(match_result.isComplete(), "Matching must complete for network streams."); if (match_result.isMatch()) { - return match_result.action()->getTyped().get( - filter_chains_by_name_, info); + const Matcher::ActionPtr action = match_result.action(); + return 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 5cc9b0059b42c..a7aef43c642de 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.createAction(*message, action_factory_context_, - server_factory_context_.messageValidationVisitor()); - return [action, keep_matching = on_match.keep_matching()] { - return OnMatch{action, {}, keep_matching}; + 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}; }; } diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 7c970149f0aff..5ac7fe53a144c 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -1825,13 +1825,14 @@ RouteConstSharedPtr VirtualHostImpl::getRouteFromEntries(const RouteCallback& cb Matcher::evaluateMatch(*matcher_, data); if (match_result.isMatch()) { - const auto result = match_result.actionByMove(); + const Matcher::ActionPtr result = match_result.action(); if (result->typeUrl() == RouteMatchAction::staticTypeUrl()) { - return getRouteFromRoutes( - cb, headers, stream_info, random_value, - {std::dynamic_pointer_cast(std::move(result))}); + const RouteMatchAction& route_action = result->getTyped(); + + return getRouteFromRoutes(cb, headers, stream_info, random_value, {route_action.route()}); } 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"); @@ -2139,9 +2140,9 @@ const Envoy::Config::TypedMetadata& NullConfigImpl::typedMetadata() const { return DefaultRouteMetadataPack::get().typed_metadata_; } -Matcher::ActionConstSharedPtr -RouteMatchActionFactory::createAction(const Protobuf::Message& config, RouteActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) { +Matcher::ActionFactoryCb RouteMatchActionFactory::createActionFactoryCb( + const Protobuf::Message& config, RouteActionContext& context, + ProtobufMessage::ValidationVisitor& validation_visitor) { const auto& route_config = MessageUtil::downcastAndValidate(config, validation_visitor); @@ -2149,14 +2150,14 @@ RouteMatchActionFactory::createAction(const Protobuf::Message& config, RouteActi RouteCreator::createAndValidateRoute(route_config, context.vhost, context.factory_context, validation_visitor, false), RouteEntryImplBaseConstSharedPtr); - return route; + + return [route]() { return std::make_unique(route); }; } REGISTER_FACTORY(RouteMatchActionFactory, Matcher::ActionFactory); -Matcher::ActionConstSharedPtr -RouteListMatchActionFactory::createAction(const Protobuf::Message& config, - RouteActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) { +Matcher::ActionFactoryCb RouteListMatchActionFactory::createActionFactoryCb( + const Protobuf::Message& config, RouteActionContext& context, + ProtobufMessage::ValidationVisitor& validation_visitor) { const auto& route_config = MessageUtil::downcastAndValidate( config, validation_visitor); @@ -2168,7 +2169,7 @@ RouteListMatchActionFactory::createAction(const Protobuf::Message& config, validation_visitor, false), RouteEntryImplBaseConstSharedPtr)); } - return std::make_shared(std::move(routes)); + return [routes]() { return std::make_unique(routes); }; } REGISTER_FACTORY(RouteListMatchActionFactory, Matcher::ActionFactory); diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 5571df3e437b6..c4106e056b8bf 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -619,7 +619,6 @@ class RouteEntryImplBase : public RouteEntryAndRoute, public Matchable, public DirectResponseEntry, public PathMatchCriterion, - public Matcher::ActionBase, public std::enable_shared_from_this, Logger::Loggable { protected: @@ -1192,9 +1191,9 @@ class RouteMatchAction : public Matcher::ActionBase { public: - Matcher::ActionConstSharedPtr - createAction(const Protobuf::Message& config, RouteActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) override; + Matcher::ActionFactoryCb + createActionFactoryCb(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(); @@ -1218,9 +1217,9 @@ class RouteListMatchAction : public Matcher::ActionBase { public: - Matcher::ActionConstSharedPtr - createAction(const Protobuf::Message& config, RouteActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) override; + Matcher::ActionFactoryCb + createActionFactoryCb(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 9abe4c961df3b..e53f2265fc974 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::ActionConstSharedPtr -ActionFactory::createAction(const Protobuf::Message& config, ActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) { +Envoy::Matcher::ActionFactoryCb +ActionFactory::createActionFactoryCb(const Protobuf::Message& config, ActionContext& context, + ProtobufMessage::ValidationVisitor& validation_visitor) { const auto& action_config = MessageUtil::downcastAndValidate(config, validation_visitor); @@ -25,7 +25,7 @@ ActionFactory::createAction(const Protobuf::Message& config, ActionContext& cont context.has_log_ = true; } - return std::make_shared(name, action); + return [name, action]() { return std::make_unique(name, action); }; } REGISTER_FACTORY(ActionFactory, Envoy::Matcher::ActionFactory); @@ -138,17 +138,18 @@ bool RoleBasedAccessControlMatcherEngineImpl::handleAction( Envoy::Matcher::evaluateMatch(*matcher_, data); ASSERT(result.isComplete()); if (result.isMatch()) { - const auto& action = result.action()->getTyped(); + 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. - const auto rbac_action = action.action(); + 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 80a5781ed9e71..ad22de9ed5553 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::ActionConstSharedPtr - createAction(const Protobuf::Message& config, ActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) override; + Envoy::Matcher::ActionFactoryCb + createActionFactoryCb(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 da6941c88b90c..9d70c9c10671f 100644 --- a/source/extensions/filters/http/composite/action.cc +++ b/source/extensions/filters/http/composite/action.cc @@ -6,30 +6,23 @@ namespace HttpFilters { namespace Composite { void ExecuteFilterAction::createFilters(Http::FilterChainFactoryCallbacks& callbacks) const { - if (actionSkip()) { - return; - } - - if (auto config_value = config_provider_(); config_value.has_value()) { - (*config_value)(callbacks); - return; - } - // There is no dynamic config available. Apply missing config filter. - Envoy::Http::MissingConfigFilterFactory(callbacks); + cb_(callbacks); } -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; +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; + } + return true; } -Matcher::ActionConstSharedPtr -ExecuteFilterActionFactory::createAction(const Protobuf::Message& config, - Http::Matching::HttpFilterActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) { +Matcher::ActionFactoryCb ExecuteFilterActionFactory::createActionFactoryCb( + 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); @@ -41,20 +34,20 @@ ExecuteFilterActionFactory::createAction(const Protobuf::Message& config, if (composite_action.has_dynamic_config()) { if (context.is_downstream_) { - return createDynamicActionDownstream(composite_action, context); + return createDynamicActionFactoryCbDownstream(composite_action, context); } else { - return createDynamicActionUpstream(composite_action, context); + return createDynamicActionFactoryCbUpstream(composite_action, context); } } if (context.is_downstream_) { - return createStaticActionDownstream(composite_action, context, validation_visitor); + return createStaticActionFactoryCbDownstream(composite_action, context, validation_visitor); } else { - return createStaticActionUpstream(composite_action, context, validation_visitor); + return createStaticActionFactoryCbUpstream(composite_action, context, validation_visitor); } } -Matcher::ActionConstSharedPtr ExecuteFilterActionFactory::createDynamicActionDownstream( +Matcher::ActionFactoryCb ExecuteFilterActionFactory::createDynamicActionFactoryCbDownstream( 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()) { @@ -64,11 +57,11 @@ Matcher::ActionConstSharedPtr ExecuteFilterActionFactory::createDynamicActionDow auto provider_manager = Envoy::Http::FilterChainUtility::createSingletonDownstreamFilterConfigProviderManager( context.server_factory_context_.value()); - return createDynamicActionTyped( + return createDynamicActionFactoryCbTyped( composite_action, context, "http", context.factory_context_.value(), provider_manager); } -Matcher::ActionConstSharedPtr ExecuteFilterActionFactory::createDynamicActionUpstream( +Matcher::ActionFactoryCb ExecuteFilterActionFactory::createDynamicActionFactoryCbUpstream( const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action, Http::Matching::HttpFilterActionContext& context) { if (!context.upstream_factory_context_.has_value() || @@ -79,12 +72,12 @@ Matcher::ActionConstSharedPtr ExecuteFilterActionFactory::createDynamicActionUps auto provider_manager = Envoy::Http::FilterChainUtility::createSingletonUpstreamFilterConfigProviderManager( context.server_factory_context_.value()); - return createDynamicActionTyped( + return createDynamicActionFactoryCbTyped( composite_action, context, "router upstream http", context.upstream_factory_context_.value(), provider_manager); } -Matcher::ActionConstSharedPtr ExecuteFilterActionFactory::createActionCommon( +Matcher::ActionFactoryCb ExecuteFilterActionFactory::createActionFactoryCbCommon( const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action, Http::Matching::HttpFilterActionContext& context, Envoy::Http::FilterFactoryCb& callback, bool is_downstream) { @@ -97,17 +90,16 @@ Matcher::ActionConstSharedPtr ExecuteFilterActionFactory::createActionCommon( 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 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); + 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); + }; } -Matcher::ActionConstSharedPtr ExecuteFilterActionFactory::createStaticActionDownstream( +Matcher::ActionFactoryCb ExecuteFilterActionFactory::createStaticActionFactoryCbDownstream( const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action, Http::Matching::HttpFilterActionContext& context, ProtobufMessage::ValidationVisitor& validation_visitor) { @@ -134,10 +126,10 @@ Matcher::ActionConstSharedPtr ExecuteFilterActionFactory::createStaticActionDown *message, context.stat_prefix_, context.server_factory_context_.value()); } - return createActionCommon(composite_action, context, callback, true); + return createActionFactoryCbCommon(composite_action, context, callback, true); } -Matcher::ActionConstSharedPtr ExecuteFilterActionFactory::createStaticActionUpstream( +Matcher::ActionFactoryCb ExecuteFilterActionFactory::createStaticActionFactoryCbUpstream( const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action, Http::Matching::HttpFilterActionContext& context, ProtobufMessage::ValidationVisitor& validation_visitor) { @@ -158,7 +150,7 @@ Matcher::ActionConstSharedPtr ExecuteFilterActionFactory::createStaticActionUpst callback = callback_or_status.value(); } - return createActionCommon(composite_action, context, callback, false); + return createActionFactoryCbCommon(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 ecd9ad9bb076c..834b757a573fa 100644 --- a/source/extensions/filters/http/composite/action.h +++ b/source/extensions/filters/http/composite/action.h @@ -18,26 +18,16 @@ class ExecuteFilterAction : public Matcher::ActionBase< envoy::extensions::filters::http::composite::v3::ExecuteFilterAction> { public: - 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) {} + explicit ExecuteFilterAction(Http::FilterFactoryCb cb, const std::string& name) + : cb_(std::move(cb)), name_(name) {} void createFilters(Http::FilterChainFactoryCallbacks& callbacks) const; - const std::string& actionName() const; - - bool actionSkip() const; + const std::string& actionName() const { return name_; } private: - FilterConfigProvider config_provider_; + Http::FilterFactoryCb cb_; const std::string name_; - const absl::optional sample_; - Runtime::Loader& runtime_; }; class ExecuteFilterActionFactory @@ -46,22 +36,29 @@ class ExecuteFilterActionFactory public: std::string name() const override { return "composite-action"; } - Matcher::ActionConstSharedPtr - createAction(const Protobuf::Message& config, Http::Matching::HttpFilterActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) override; + Matcher::ActionFactoryCb + createActionFactoryCb(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::ActionConstSharedPtr createActionCommon( + Matcher::ActionFactoryCb createActionFactoryCbCommon( const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action, Http::Matching::HttpFilterActionContext& context, Envoy::Http::FilterFactoryCb& callback, bool is_downstream); template - Matcher::ActionConstSharedPtr createDynamicActionTyped( + Matcher::ActionFactoryCb createDynamicActionFactoryCbTyped( 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) { @@ -76,30 +73,35 @@ class ExecuteFilterActionFactory server_factory_context.clusterManager(), false, filter_chain_type, nullptr); Envoy::Runtime::Loader& runtime = context.server_factory_context_->runtime(); - - 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); + 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); + }; } - Matcher::ActionConstSharedPtr createDynamicActionDownstream( + Matcher::ActionFactoryCb createDynamicActionFactoryCbDownstream( const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action, Http::Matching::HttpFilterActionContext& context); - Matcher::ActionConstSharedPtr createDynamicActionUpstream( + Matcher::ActionFactoryCb createDynamicActionFactoryCbUpstream( const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action, Http::Matching::HttpFilterActionContext& context); - Matcher::ActionConstSharedPtr createStaticActionDownstream( + Matcher::ActionFactoryCb createStaticActionFactoryCbDownstream( const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action, Http::Matching::HttpFilterActionContext& context, ProtobufMessage::ValidationVisitor& validation_visitor); - Matcher::ActionConstSharedPtr createStaticActionUpstream( + Matcher::ActionFactoryCb createStaticActionFactoryCbUpstream( 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 fd12f0946ecd7..3075685da05b1 100644 --- a/source/extensions/filters/http/composite/filter.cc +++ b/source/extensions/filters/http/composite/filter.cc @@ -96,6 +96,7 @@ void Filter::encodeComplete() { void Filter::onMatchCallback(const Matcher::Action& action) { const auto& composite_action = action.getTyped(); + FactoryCallbacksWrapper wrapper(*this, dispatcher_); composite_action.createFilters(wrapper); @@ -106,12 +107,11 @@ 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,6 +137,7 @@ 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 551744416fc5b..acc86f447bf57 100644 --- a/source/extensions/filters/http/composite/filter.h +++ b/source/extensions/filters/http/composite/filter.h @@ -57,7 +57,8 @@ class Filter : public Http::StreamFilter, Logger::Loggable { public: Filter(FilterStats& stats, Event::Dispatcher& dispatcher, bool is_upstream) - : dispatcher_(dispatcher), stats_(stats), is_upstream_(is_upstream) {} + : dispatcher_(dispatcher), decoded_headers_(false), stats_(stats), is_upstream_(is_upstream) { + } // Http::StreamDecoderFilter Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& headers, @@ -123,7 +124,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_{false}; + bool decoded_headers_ : 1; // 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 84d5c1533b20d..4aa4ec67746fc 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 std::dynamic_pointer_cast(match_result.actionByMove()); + return match_result.action()->getTyped().policy_; } } // namespace CustomResponse diff --git a/source/extensions/filters/http/custom_response/policy.h b/source/extensions/filters/http/custom_response/policy.h index 8a18f647ea2e3..8a4d98080421d 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, - public Matcher::ActionBase { +class Policy : public std::enable_shared_from_this { public: + virtual ~Policy() = default; virtual Http::FilterHeadersStatus encodeHeaders(Http::ResponseHeaderMap&, bool, CustomResponseFilter&) const PURE; @@ -42,6 +42,11 @@ 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_; @@ -52,10 +57,12 @@ template class PolicyMatchActionFactory : public Matcher::ActionFactory, Logger::Loggable { public: - Matcher::ActionConstSharedPtr createAction(const Protobuf::Message& config, - CustomResponseActionFactoryContext& context, - ProtobufMessage::ValidationVisitor&) override { - return createPolicy(config, context.server_, context.stats_prefix_); + 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); + }; } 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 c80e56c9fc92f..86d32d5e63222 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::ActionConstSharedPtr createAction(const Protobuf::Message&, - Envoy::Http::Matching::HttpFilterActionContext&, - ProtobufMessage::ValidationVisitor&) override { - return std::make_shared(); + Matcher::ActionFactoryCb createActionFactoryCb(const Protobuf::Message&, + Envoy::Http::Matching::HttpFilterActionContext&, + ProtobufMessage::ValidationVisitor&) override { + return []() { return std::make_unique(); }; } 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 auto& result = match_result.action(); - if (result == nullptr || SkipAction().typeUrl() == result->typeUrl()) { + const Matcher::ActionPtr 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 6fa6245a78683..bb246bfcb8224 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::ActionConstSharedPtr createAction(const Protobuf::Message&, - ProtoApiScrubberRemoveFieldAction&, - ProtobufMessage::ValidationVisitor&) override { - return std::make_shared(); + Matcher::ActionFactoryCb createActionFactoryCb(const Protobuf::Message&, + ProtoApiScrubberRemoveFieldAction&, + ProtobufMessage::ValidationVisitor&) override { + return []() { return std::make_unique(); }; } 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 cb297c5d0dd90..34c74d7e4a6e6 100644 --- a/source/extensions/filters/http/rate_limit_quota/filter.cc +++ b/source/extensions/filters/http/rate_limit_quota/filter.cc @@ -56,9 +56,8 @@ 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) { @@ -80,7 +79,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 @@ -185,7 +184,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 @@ -216,7 +215,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.actionByMove(); + return match_result.action(); } 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 2920594c5d5f3..dcaf8b45a8d51 100644 --- a/source/extensions/filters/http/rate_limit_quota/filter.h +++ b/source/extensions/filters/http/rate_limit_quota/filter.h @@ -65,8 +65,7 @@ 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 ee6fc0a21bdc1..38eb7ee3921c7 100644 --- a/source/extensions/filters/http/rate_limit_quota/matcher.h +++ b/source/extensions/filters/http/rate_limit_quota/matcher.h @@ -51,15 +51,17 @@ class RateLimitOnMatchActionFactory : public Matcher::ActionFactory(config, validation_visitor); - return std::make_shared(std::move(bucket_settings)); + return [bucket_settings = std::move(bucket_settings)]() { + return std::make_unique(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 d70a3a99fc9d6..60fd548e078f3 100644 --- a/source/extensions/filters/network/generic_proxy/route_impl.cc +++ b/source/extensions/filters/network/generic_proxy/route_impl.cc @@ -61,12 +61,13 @@ RouteEntryImpl::RouteEntryImpl(const ProtoRouteAction& route_action, } } -Matcher::ActionConstSharedPtr -RouteMatchActionFactory::createAction(const Protobuf::Message& config, RouteActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) { +Matcher::ActionFactoryCb RouteMatchActionFactory::createActionFactoryCb( + const Protobuf::Message& config, RouteActionContext& context, + ProtobufMessage::ValidationVisitor& validation_visitor) { const auto& route_action = MessageUtil::downcastAndValidate(config, validation_visitor); - return std::make_shared(route_action, context.factory_context); + auto route = std::make_shared(route_action, context.factory_context); + return [route]() { return std::make_unique(route); }; } REGISTER_FACTORY(RouteMatchActionFactory, Matcher::ActionFactory); @@ -91,12 +92,15 @@ RouteEntryConstSharedPtr VirtualHostImpl::routeEntry(const MatchInput& request) Matcher::MatchResult match_result = Matcher::evaluateMatch(*matcher_, request); if (match_result.isMatch()) { - Matcher::ActionConstSharedPtr action = match_result.actionByMove(); + Matcher::ActionPtr action = match_result.action(); + // The only possible action that can be used within the route matching context // is the RouteMatchAction, so this must be true. - ASSERT(action->typeUrl() == RouteEntryImpl::staticTypeUrl()); - ASSERT(dynamic_cast(action.get())); - return std::dynamic_pointer_cast(std::move(action)); + ASSERT(action->typeUrl() == RouteMatchAction::staticTypeUrl()); + ASSERT(dynamic_cast(action.get())); + const RouteMatchAction& route_action = static_cast(*action); + + return route_action.route(); } 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 ef81c9818d7a3..62118b8856f6e 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, public Matcher::ActionBase { +class RouteEntryImpl : public RouteEntry { public: RouteEntryImpl(const ProtoRouteAction& route, Envoy::Server::Configuration::ServerFactoryContext& context); @@ -76,6 +76,17 @@ 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&, @@ -87,9 +98,9 @@ class RouteActionValidationVisitor : public Matcher::MatchTreeValidationVisitor< // Registered factory for RouteMatchAction. class RouteMatchActionFactory : public Matcher::ActionFactory { public: - Matcher::ActionConstSharedPtr - createAction(const Protobuf::Message& config, RouteActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) override; + Matcher::ActionFactoryCb + createActionFactoryCb(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 adfa6938a7e8f..b176034ad65ff 100644 --- a/source/extensions/filters/network/match_delegate/config.cc +++ b/source/extensions/filters/network/match_delegate/config.cc @@ -18,9 +18,10 @@ namespace Factory { class SkipActionFactory : public Matcher::ActionFactory { public: std::string name() const override { return "skip"; } - Matcher::ActionConstSharedPtr createAction(const Protobuf::Message&, NetworkFilterActionContext&, - ProtobufMessage::ValidationVisitor&) override { - return std::make_shared(); + Matcher::ActionFactoryCb createActionFactoryCb(const Protobuf::Message&, + NetworkFilterActionContext&, + ProtobufMessage::ValidationVisitor&) override { + return []() { return std::make_unique(); }; } ProtobufTypes::MessagePtr createEmptyConfigProto() override { return std::make_unique(); @@ -106,8 +107,8 @@ void DelegatingNetworkFilter::FilterMatchState::evaluateMatchTree() { match_tree_evaluated_ = match_result.isComplete(); if (match_tree_evaluated_ && match_result.isMatch()) { - const auto& result = match_result.action(); - if (result == nullptr || SkipAction().typeUrl() == result->typeUrl()) { + const Matcher::ActionPtr 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);` @@ -191,7 +192,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_REF(filter_factory_or_error.status()); + THROW_IF_NOT_OK(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 8ab3a80a569b1..0fa2a904e1e5b 100644 --- a/source/extensions/filters/udp/udp_proxy/router/router_impl.cc +++ b/source/extensions/filters/udp/udp_proxy/router/router_impl.cc @@ -15,16 +15,17 @@ namespace UdpFilters { namespace UdpProxy { namespace Router { -Matcher::ActionConstSharedPtr -RouteMatchActionFactory::createAction(const Protobuf::Message& config, RouteActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) { +Matcher::ActionFactoryCb RouteMatchActionFactory::createActionFactoryCb( + 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 std::make_shared(cluster); + + return [cluster]() { return std::make_unique(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 04bb887fde713..f506778db6af2 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::ActionConstSharedPtr - createAction(const Protobuf::Message& config, RouteActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) override; + Matcher::ActionFactoryCb + createActionFactoryCb(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 9476c572ecee7..962082481b1f3 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::ActionConstSharedPtr -ActionFactory::createAction(const Protobuf::Message& proto_config, - FilterChainActionFactoryContext& context, - ProtobufMessage::ValidationVisitor& validator) { +Matcher::ActionFactoryCb +ActionFactory::createActionFactoryCb(const Protobuf::Message& proto_config, + FilterChainActionFactoryContext& context, + ProtobufMessage::ValidationVisitor& validator) { const auto& config = MessageUtil::downcastAndValidate( proto_config, validator); @@ -35,7 +35,7 @@ ActionFactory::createAction(const Protobuf::Message& proto_config, Formatter::FormatterConstSharedPtr formatter = THROW_OR_RETURN_VALUE( Formatter::SubstitutionFormatStringUtils::fromProtoConfig(config, generic_context), Formatter::FormatterPtr); - return std::make_shared(std::move(formatter)); + return [formatter]() { return std::make_unique(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 0b893e2903031..359490c375238 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(Formatter::FormatterConstSharedPtr formatter) : formatter_(std::move(formatter)) {} + ActionImpl(const Formatter::FormatterConstSharedPtr& formatter) : formatter_(formatter) {} const Network::FilterChain* get(const Server::Configuration::FilterChainsByName& filter_chains_by_name, const StreamInfo::StreamInfo& info) const override; @@ -30,9 +30,10 @@ using FilterChainActionFactoryContext = Server::Configuration::ServerFactoryCont class ActionFactory : public Matcher::ActionFactory { public: std::string name() const override { return "envoy.matching.actions.format_string"; } - Matcher::ActionConstSharedPtr - createAction(const Protobuf::Message& proto_config, FilterChainActionFactoryContext& context, - ProtobufMessage::ValidationVisitor& validator) override; + Matcher::ActionFactoryCb + createActionFactoryCb(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 8553f75f0cde9..70d35193631e2 100644 --- a/source/extensions/router/cluster_specifiers/matcher/matcher_cluster_specifier.cc +++ b/source/extensions/router/cluster_specifiers/matcher/matcher_cluster_specifier.cc @@ -10,12 +10,14 @@ namespace Extensions { namespace Router { namespace Matcher { -Envoy::Matcher::ActionConstSharedPtr -ClusterActionFactory::createAction(const Protobuf::Message& config, ClusterActionContext&, - ProtobufMessage::ValidationVisitor& validation_visitor) { +Envoy::Matcher::ActionFactoryCb ClusterActionFactory::createActionFactoryCb( + const Protobuf::Message& config, ClusterActionContext&, + ProtobufMessage::ValidationVisitor& validation_visitor) { const auto& proto_config = MessageUtil::downcastAndValidate(config, validation_visitor); - return std::make_shared(proto_config.cluster()); + auto cluster = std::make_shared(proto_config.cluster()); + + return [cluster]() { return std::make_unique(cluster); }; } REGISTER_FACTORY(ClusterActionFactory, Envoy::Matcher::ActionFactory); @@ -41,7 +43,9 @@ class MatcherRouteEntry : public Envoy::Router::DelegatingRouteEntry { if (!match_result.isMatch()) { return; } - cluster_name_.emplace(match_result.action()->getTyped().cluster()); + + const Envoy::Matcher::ActionPtr result = match_result.action(); + cluster_name_.emplace(result->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 88e7746cea02b..52562d728cb0f 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(absl::string_view cluster) : cluster_(cluster) {} + explicit ClusterAction(std::shared_ptr cluster) : cluster_(cluster) {} - const std::string& cluster() const { return cluster_; } + const std::string& cluster() const { return *cluster_; } private: - const std::string cluster_; + const std::shared_ptr 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::ActionConstSharedPtr - createAction(const Protobuf::Message& config, ClusterActionContext& context, - ProtobufMessage::ValidationVisitor& validation_visitor) override; + Envoy::Matcher::ActionFactoryCb + createActionFactoryCb(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 2dc71b5e0c4c1..b635a25b3bf93 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_=*/nullptr, /*.matcher=*/sub_matcher, + matcher->addChild("match", OnMatch{/*.action_cb=*/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_=*/nullptr, /*.matcher=*/sub_matcher, + OnMatch{/*.action_cb=*/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_=*/nullptr, /*.matcher=*/top_on_no_match_matcher, + OnMatch{/*.action_cb=*/nullptr, /*.matcher=*/top_on_no_match_matcher, /*.keep_matching=*/false}); - matcher->addChild("match", OnMatch{/*.action_=*/nullptr, + matcher->addChild("match", OnMatch{/*.action_cb=*/nullptr, /*.matcher=*/sub_matcher_match_keeps_matching, /*.keep_matching=*/true}); - std::vector skipped_results{}; - SkippedMatchCb skipped_match_cb = [&skipped_results](const ActionConstSharedPtr& cb) { + std::vector skipped_results{}; + SkippedMatchCb skipped_match_cb = [&skipped_results](ActionFactoryCb 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 aa83f429e4bae..42246922f1ccd 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](ActionConstSharedPtr cb) { + std::vector skipped_results; + SkippedMatchCb skipped_match_cb = [&skipped_results](ActionFactoryCb 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 ActionConstSharedPtr cb) { + std::vector skipped_results; + SkippedMatchCb skipped_match_cb = [&skipped_results](const ActionFactoryCb 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_=*/nullptr, /*.matcher=*/sub_matcher_1, + OnMatch{/*.action_cb=*/nullptr, /*.matcher=*/sub_matcher_1, /*.keep_matching=*/false}); matcher.addMatcher(createSingleMatcher("string", [](auto) { return true; }), - OnMatch{/*.action_=*/nullptr, /*.matcher=*/sub_matcher_2, + OnMatch{/*.action_cb=*/nullptr, /*.matcher=*/sub_matcher_2, /*.keep_matching=*/true}); matcher.addMatcher(createSingleMatcher("string", [](auto) { return true; }), - OnMatch{/*.action_=*/nullptr, /*.matcher=*/sub_matcher_3, + OnMatch{/*.action_cb=*/nullptr, /*.matcher=*/sub_matcher_3, /*.keep_matching=*/false}); - std::vector skipped_results; - SkippedMatchCb skipped_match_cb = [&skipped_results](ActionConstSharedPtr cb) { + std::vector skipped_results; + SkippedMatchCb skipped_match_cb = [&skipped_results](ActionFactoryCb 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_=*/nullptr, + OnMatch{/*action_cb=*/nullptr, /*matcher=*/on_no_match_sub_matcher, /*keep_matching=*/false}); matcher.addMatcher( createSingleMatcher("string", [](auto) { return true; }), - OnMatch{/*action_=*/nullptr, /*matcher=*/sub_matcher_1, /*keep_matching=*/true}); + OnMatch{/*action_cb=*/nullptr, /*matcher=*/sub_matcher_1, /*keep_matching=*/true}); matcher.addMatcher( createSingleMatcher("string", [](auto) { return true; }), - OnMatch{/*action_=*/nullptr, /*matcher=*/sub_matcher_2, /*keep_matching=*/false}); + OnMatch{/*action_cb=*/nullptr, /*matcher=*/sub_matcher_2, /*keep_matching=*/false}); - std::vector skipped_results; - SkippedMatchCb skipped_match_cb = [&skipped_results](ActionConstSharedPtr cb) { + std::vector skipped_results; + SkippedMatchCb skipped_match_cb = [&skipped_results](ActionFactoryCb 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 09b31e650f582..d4072671da910 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](ActionConstSharedPtr cb) { + std::vector skipped_results; + SkippedMatchCb skipped_match_cb = [&skipped_results](ActionFactoryCb 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](ActionConstSharedPtr cb) { + std::vector skipped_results; + SkippedMatchCb skipped_match_cb = [&skipped_results](ActionFactoryCb 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](ActionConstSharedPtr cb) { + std::vector skipped_results; + SkippedMatchCb skipped_match_cb = [&skipped_results](ActionFactoryCb 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_=*/nullptr, /*.matcher=*/nested_matcher, + OnMatch{/*.action_cb=*/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](ActionConstSharedPtr cb) { + std::vector skipped_results; + SkippedMatchCb skipped_match_cb = [&skipped_results](ActionFactoryCb 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 7eeec55e96897..808decee787bc 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: - ActionConstSharedPtr createAction(const Protobuf::Message& config, absl::string_view&, - ProtobufMessage::ValidationVisitor&) override { + ActionFactoryCb createActionFactoryCb(const Protobuf::Message& config, absl::string_view&, + ProtobufMessage::ValidationVisitor&) override { const auto& string = dynamic_cast(config); - return std::make_shared(string.value()); + return [string]() { return std::make_unique(string.value()); }; } ProtobufTypes::MessagePtr createEmptyConfigProto() override { @@ -273,9 +273,14 @@ 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{std::make_shared(std::string(value)), nullptr, keep_matching}; + return OnMatch{[s = std::string(value)]() { return stringValue(s); }, nullptr, keep_matching}; } inline void PrintTo(const Action& action, std::ostream* os) { @@ -286,14 +291,23 @@ 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{Action="; - PrintTo(*result.action(), os); + *os << "Match{ActionFactoryCb="; + PrintTo(result.actionFactory(), os); *os << "}"; } else { *os << "UnknownState"; @@ -305,9 +319,9 @@ inline void PrintTo(const MatchTree& matcher, std::ostream* os) { } inline void PrintTo(const OnMatch& on_match, std::ostream* os) { - if (on_match.action_) { - *os << "{action_="; - PrintTo(on_match.action_, os); + if (on_match.action_cb_) { + *os << "{action_cb_="; + PrintTo(on_match.action_cb_, os); *os << "}"; } else if (on_match.matcher_) { *os << "{matcher_="; @@ -325,25 +339,26 @@ MATCHER(HasInsufficientData, "") { } MATCHER_P(IsActionWithType, matcher, "") { - // Takes an ActionConstSharedPtr argument, and compares its action type against matcher. + // Takes an ActionFactoryCb argument, and compares its action type against matcher. if (arg == nullptr) { return false; } - return ::testing::ExplainMatchResult(testing::Matcher(matcher), arg->typeUrl(), - result_listener); + ActionPtr action = arg(); + return ::testing::ExplainMatchResult(testing::Matcher(matcher), + action->typeUrl(), result_listener); } MATCHER_P(IsStringAction, matcher, "") { - // Takes an ActionConstSharedPtr argument, and compares its StringAction's string against matcher. + // Takes an ActionFactoryCb argument, and compares its StringAction's string against matcher. if (arg == nullptr) { return false; } - - if (arg->typeUrl() != "google.protobuf.StringValue") { + ActionPtr action = arg(); + if (action->typeUrl() != "google.protobuf.StringValue") { return false; } return ::testing::ExplainMatchResult(testing::Matcher(matcher), - arg->template getTyped().string_, + action->template getTyped().string_, result_listener); } @@ -353,7 +368,8 @@ MATCHER_P(HasStringAction, matcher, "") { if (!arg.isMatch()) { return false; } - return ::testing::ExplainMatchResult(IsStringAction(matcher), arg.action(), result_listener); + return ::testing::ExplainMatchResult(IsStringAction(matcher), arg.actionFactory(), + result_listener); } MATCHER_P(HasActionWithType, matcher, "") { @@ -362,7 +378,8 @@ MATCHER_P(HasActionWithType, matcher, "") { if (!arg.isMatch()) { return false; } - return ::testing::ExplainMatchResult(IsActionWithType(matcher), arg.action(), result_listener); + return ::testing::ExplainMatchResult(IsActionWithType(matcher), arg.actionFactory(), + 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 20e57359201d7..1ea833d294e1c 100644 --- a/test/extensions/common/matcher/domain_matcher_test.cc +++ b/test/extensions/common/matcher/domain_matcher_test.cc @@ -38,6 +38,7 @@ 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; @@ -624,8 +625,8 @@ TEST_F(DomainMatcherTest, KeepMatchingSupport) { loadConfig(yaml); validation_visitor_.setSupportKeepMatching(true); - std::vector skipped_results; - skipped_match_cb_ = [&skipped_results](const Envoy::Matcher::ActionConstSharedPtr& cb) { + std::vector skipped_results; + skipped_match_cb_ = [&skipped_results](Envoy::Matcher::ActionFactoryCb 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 d7a4536b5fba9..02f40777e9db2 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,10 +36,12 @@ 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; @@ -602,10 +604,8 @@ 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](const ActionConstSharedPtr& cb) { - skipped_results.push_back(cb); - }; + std::vector skipped_results{}; + skipped_match_cb_ = [&skipped_results](ActionFactoryCb 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 03ecafba79106..d37bd319eae93 100644 --- a/test/extensions/filters/http/composite/filter_test.cc +++ b/test/extensions/filters/http/composite/filter_test.cc @@ -86,8 +86,6 @@ 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_; @@ -116,13 +114,12 @@ TEST_F(FilterTest, StreamEncoderFilterDelegation) { ON_CALL(encoder_callbacks_.stream_info_, filterState()) .WillByDefault(testing::ReturnRef(filter_state)); - Http::FilterFactoryCb factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { + auto factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { cb.addStreamEncoderFilter(stream_filter); }; EXPECT_CALL(*stream_filter, setEncoderFilterCallbacks(_)); - ExecuteFilterAction action([&]() -> OptRef { return factory_callback; }, - "actionName", absl::nullopt, context_.runtime_loader_); + ExecuteFilterAction action(factory_callback, "actionName"); EXPECT_CALL(success_counter_, inc()); filter_.onMatchCallback(action); @@ -146,13 +143,12 @@ TEST_F(FilterTest, StreamDecoderFilterDelegation) { ON_CALL(decoder_callbacks_.stream_info_, filterState()) .WillByDefault(testing::ReturnRef(filter_state)); - Http::FilterFactoryCb factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { + auto factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { cb.addStreamDecoderFilter(stream_filter); }; EXPECT_CALL(*stream_filter, setDecoderFilterCallbacks(_)); - ExecuteFilterAction action([&]() -> OptRef { return factory_callback; }, - "actionName", absl::nullopt, context_.runtime_loader_); + ExecuteFilterAction action(factory_callback, "actionName"); EXPECT_CALL(success_counter_, inc()); filter_.onMatchCallback(action); @@ -175,15 +171,14 @@ TEST_F(FilterTest, StreamFilterDelegation) { ON_CALL(decoder_callbacks_.stream_info_, filterState()) .WillByDefault(testing::ReturnRef(filter_state)); - Http::FilterFactoryCb factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { + auto 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([&]() -> OptRef { return factory_callback; }, - "actionName", absl::nullopt, context_.runtime_loader_); + ExecuteFilterAction action(factory_callback, "actionName"); filter_.onMatchCallback(action); expectFilterStateInfo(filter_state); @@ -205,13 +200,12 @@ TEST_F(FilterTest, StreamFilterDelegationMultipleStreamFilters) { ON_CALL(decoder_callbacks_.stream_info_, filterState()) .WillByDefault(testing::ReturnRef(filter_state)); - Http::FilterFactoryCb factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { + auto factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { cb.addStreamFilter(stream_filter); cb.addStreamFilter(stream_filter); }; - ExecuteFilterAction action([&]() -> OptRef { return factory_callback; }, - "actionName", absl::nullopt, context_.runtime_loader_); + ExecuteFilterAction action(factory_callback, "actionName"); EXPECT_CALL(error_counter_, inc()); filter_.onMatchCallback(action); @@ -231,13 +225,12 @@ TEST_F(FilterTest, StreamFilterDelegationMultipleStreamDecoderFilters) { ON_CALL(decoder_callbacks_.stream_info_, filterState()) .WillByDefault(testing::ReturnRef(filter_state)); - Http::FilterFactoryCb factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { + auto factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { cb.addStreamDecoderFilter(decoder_filter); cb.addStreamDecoderFilter(decoder_filter); }; - ExecuteFilterAction action([&]() -> OptRef { return factory_callback; }, - "actionName", absl::nullopt, context_.runtime_loader_); + ExecuteFilterAction action(factory_callback, "actionName"); EXPECT_CALL(error_counter_, inc()); filter_.onMatchCallback(action); @@ -257,13 +250,12 @@ TEST_F(FilterTest, StreamFilterDelegationMultipleStreamEncoderFilters) { ON_CALL(encoder_callbacks_.stream_info_, filterState()) .WillByDefault(testing::ReturnRef(filter_state)); - Http::FilterFactoryCb factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { + auto factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { cb.addStreamEncoderFilter(encode_filter); cb.addStreamEncoderFilter(encode_filter); }; - ExecuteFilterAction action([&]() -> OptRef { return factory_callback; }, - "actionName", absl::nullopt, context_.runtime_loader_); + ExecuteFilterAction action(factory_callback, "actionName"); EXPECT_CALL(error_counter_, inc()); filter_.onMatchCallback(action); @@ -286,14 +278,13 @@ TEST_F(FilterTest, StreamFilterDelegationMultipleAccessLoggers) { ON_CALL(encoder_callbacks_.stream_info_, filterState()) .WillByDefault(testing::ReturnRef(filter_state)); - Http::FilterFactoryCb factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { + auto factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { cb.addStreamEncoderFilter(encode_filter); cb.addAccessLogHandler(access_log_1); cb.addAccessLogHandler(access_log_2); }; - ExecuteFilterAction action([&]() -> OptRef { return factory_callback; }, - "actionName", absl::nullopt, context_.runtime_loader_); + ExecuteFilterAction action(factory_callback, "actionName"); EXPECT_CALL(*encode_filter, setEncoderFilterCallbacks(_)); EXPECT_CALL(success_counter_, inc()); filter_.onMatchCallback(action); @@ -349,7 +340,8 @@ TEST(ConfigTest, TestConfig) { .server_factory_context_ = server_factory_context}; ExecuteFilterActionFactory factory; EXPECT_THROW_WITH_MESSAGE( - factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()), + factory.createActionFactoryCb(config, action_context, + ProtobufMessage::getStrictValidationVisitor()), EnvoyException, "Error: Only one of `dynamic_config` or `typed_config` can be set."); } } @@ -379,7 +371,8 @@ TEST(ConfigTest, TestDynamicConfigInDownstream) { .server_factory_context_ = server_factory_context}; ExecuteFilterActionFactory factory; EXPECT_THROW_WITH_MESSAGE( - factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()), + factory.createActionFactoryCb(config, action_context, + ProtobufMessage::getStrictValidationVisitor()), EnvoyException, "Failed to get downstream factory context or server factory context."); } @@ -407,7 +400,8 @@ TEST(ConfigTest, TestDynamicConfigInUpstream) { .server_factory_context_ = absl::nullopt}; ExecuteFilterActionFactory factory; EXPECT_THROW_WITH_MESSAGE( - factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()), + factory.createActionFactoryCb(config, action_context, + ProtobufMessage::getStrictValidationVisitor()), EnvoyException, "Failed to get upstream factory context or server factory context."); } @@ -434,7 +428,8 @@ TEST(ConfigTest, CreateFilterFromServerContextDual) { .server_factory_context_ = server_factory_context}; ExecuteFilterActionFactory factory; EXPECT_THROW_WITH_MESSAGE( - factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()), + factory.createActionFactoryCb(config, action_context, + ProtobufMessage::getStrictValidationVisitor()), EnvoyException, "DualFactoryBase: creating filter factory from server factory context is not supported"); } @@ -461,7 +456,8 @@ TEST(ConfigTest, DualFilterNoUpstreamFactoryContext) { .server_factory_context_ = server_factory_context}; ExecuteFilterActionFactory factory; EXPECT_THROW_WITH_MESSAGE( - factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()), + factory.createActionFactoryCb(config, action_context, + ProtobufMessage::getStrictValidationVisitor()), EnvoyException, "Failed to get upstream filter factory creation function"); } @@ -486,7 +482,8 @@ TEST(ConfigTest, DownstreamFilterNoFactoryContext) { .server_factory_context_ = absl::nullopt}; ExecuteFilterActionFactory factory; EXPECT_THROW_WITH_MESSAGE( - factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()), + factory.createActionFactoryCb(config, action_context, + ProtobufMessage::getStrictValidationVisitor()), EnvoyException, "Failed to get downstream filter factory creation function"); } @@ -513,7 +510,8 @@ TEST(ConfigTest, TestDownstreamFilterNoOverridingServerContext) { .server_factory_context_ = server_factory_context}; ExecuteFilterActionFactory factory; EXPECT_THROW_WITH_MESSAGE( - factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()), + factory.createActionFactoryCb(config, action_context, + ProtobufMessage::getStrictValidationVisitor()), EnvoyException, "Creating filter factory from server factory context is not supported"); } @@ -528,23 +526,12 @@ 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; - auto action = - factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()); - - EXPECT_FALSE(action->getTyped().actionSkip()); + EXPECT_TRUE(factory.isSampled(config, runtime)); } // Config test to check if sample_percent config is in place and feature enabled. @@ -562,27 +549,15 @@ 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; - auto action = - factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()); - - EXPECT_CALL(server_factory_context.runtime_loader_.snapshot_, + EXPECT_CALL(runtime.snapshot_, featureEnabled(_, testing::A())) .WillOnce(testing::Return(true)); - - EXPECT_FALSE(action->getTyped().actionSkip()); + EXPECT_TRUE(factory.isSampled(config, runtime)); } // Config test to check if sample_percent config is in place and feature not enabled. @@ -595,32 +570,18 @@ TEST(ConfigTest, TestSamplePercentInPlaceFeatureNotEnabled) { abort: http_status: 503 sample_percent: - default_value: - numerator: 30 - denominator: HUNDRED + runtime_key: )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; - auto action = - factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()); - - EXPECT_CALL(server_factory_context.runtime_loader_.snapshot_, + EXPECT_CALL(runtime.snapshot_, featureEnabled(_, testing::A())) .WillOnce(testing::Return(false)); - - EXPECT_TRUE(action->getTyped().actionSkip()); + EXPECT_FALSE(factory.isSampled(config, runtime)); } TEST_F(FilterTest, FilterStateShouldBeUpdatedWithTheMatchingActionForDynamicConfig) { @@ -647,8 +608,8 @@ TEST_F(FilterTest, FilterStateShouldBeUpdatedWithTheMatchingActionForDynamicConf .upstream_factory_context_ = upstream_factory_context, .server_factory_context_ = server_factory_context}; ExecuteFilterActionFactory factory; - auto action = - factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()); + auto action = factory.createActionFactoryCb(config, action_context, + ProtobufMessage::getStrictValidationVisitor())(); EXPECT_EQ("actionName", action->getTyped().actionName()); } @@ -678,8 +639,8 @@ TEST_F(FilterTest, FilterStateShouldBeUpdatedWithTheMatchingActionForTypedConfig .upstream_factory_context_ = upstream_factory_context, .server_factory_context_ = server_factory_context}; ExecuteFilterActionFactory factory; - auto action = - factory.createAction(config, action_context, ProtobufMessage::getStrictValidationVisitor()); + auto action = factory.createActionFactoryCb(config, action_context, + ProtobufMessage::getStrictValidationVisitor())(); EXPECT_EQ("actionName", action->getTyped().actionName()); } @@ -697,13 +658,12 @@ TEST_F(FilterTest, FilterStateShouldBeUpdatedWithTheMatchingAction) { StreamInfo::FilterState::StateType::Mutable, StreamInfo::FilterState::LifeSpan::FilterChain); - Http::FilterFactoryCb factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { + auto factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { cb.addStreamEncoderFilter(stream_filter); }; EXPECT_CALL(*stream_filter, setEncoderFilterCallbacks(_)); - ExecuteFilterAction action([&]() -> OptRef { return factory_callback; }, - "actionName", absl::nullopt, context_.runtime_loader_); + ExecuteFilterAction action(factory_callback, "actionName"); EXPECT_CALL(success_counter_, inc()); filter_.onMatchCallback(action); @@ -728,13 +688,12 @@ TEST_F(FilterTest, MatchingActionShouldNotCollitionWithOtherRootFilter) { StreamInfo::FilterState::StateType::Mutable, StreamInfo::FilterState::LifeSpan::FilterChain); - Http::FilterFactoryCb factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { + auto factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { cb.addStreamEncoderFilter(stream_filter); }; EXPECT_CALL(*stream_filter, setEncoderFilterCallbacks(_)); - ExecuteFilterAction action([&]() -> OptRef { return factory_callback; }, - "actionName", absl::nullopt, context_.runtime_loader_); + ExecuteFilterAction action(factory_callback, "actionName"); EXPECT_CALL(success_counter_, inc()); filter_.onMatchCallback(action); @@ -765,13 +724,12 @@ TEST_F(UpstreamFilterTest, StreamEncoderFilterDelegationUpstream) { ON_CALL(encoder_callbacks_.stream_info_, filterState()) .WillByDefault(testing::ReturnRef(filter_state)); - Http::FilterFactoryCb factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { + auto factory_callback = [&](Http::FilterChainFactoryCallbacks& cb) { cb.addStreamEncoderFilter(stream_filter); }; EXPECT_CALL(*stream_filter, setEncoderFilterCallbacks(_)); - ExecuteFilterAction action([&]() -> OptRef { return factory_callback; }, - "actionName", absl::nullopt, context_.runtime_loader_); + ExecuteFilterAction action(factory_callback, "actionName"); 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 2e032c3953952..0b63d20d77450 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{ - std::make_shared(), nullptr, false}); + []() { return std::make_unique(); }, nullptr, false}); return tree; } @@ -315,7 +315,7 @@ Matcher::MatchTreeSharedPtr createRequestAndRespo tree->addChild( "match", Matcher::OnMatch{ - std::make_shared(), + []() { return std::make_unique(); }, createMatchingTree( "match-header", "match"), false}); @@ -369,11 +369,13 @@ template Matcher::MatchTreeSharedPtr createMatchTreeWithOnNoMatch(const std::string& name, const std::string& value) { auto tree = *Matcher::ExactMapMatcher::create( - std::make_unique(name), Matcher::OnMatch{ - std::make_shared(), nullptr, false}); + std::make_unique(name), + Matcher::OnMatch{ + []() { return std::make_unique(); }, nullptr, false}); // No action is set on match. i.e., nullptr action factory cb. - tree->addChild(value, Matcher::OnMatch{nullptr, nullptr, false}); + tree->addChild(value, Matcher::OnMatch{[]() { return 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 20b860a1a3704..96cdd58b18195 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 1ee03a8bdd915..56aa14d4e114b 100644 --- a/test/extensions/filters/network/generic_proxy/route_test.cc +++ b/test/extensions/filters/network/generic_proxy/route_test.cc @@ -279,6 +279,16 @@ 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. */ @@ -311,11 +321,13 @@ TEST(RouteMatchActionFactoryTest, SimpleRouteMatchActionFactoryTest) { TestUtility::loadFromYaml(yaml_config, proto_config); RouteActionContext context{server_context}; - auto action = - factory.createAction(proto_config, context, server_context.messageValidationVisitor()); + auto factory_cb = factory.createActionFactoryCb(proto_config, context, + server_context.messageValidationVisitor()); + + EXPECT_EQ(factory_cb()->getTyped().route().get(), + factory_cb()->getTyped().route().get()); - EXPECT_NE(action, nullptr); - EXPECT_EQ(action->getTyped().clusterName(), "cluster_0"); + EXPECT_EQ(factory_cb()->getTyped().route()->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 fd283fb15e51c..574b886536cef 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{ - std::make_shared(), nullptr, false}); + []() { return std::make_unique(); }, 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{ - std::make_shared(), nullptr, false}); + []() { return std::make_unique(); }, 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 7028674e75f2d..8bbb3027572fe 100644 --- a/test/extensions/matching/actions/format_string/config_test.cc +++ b/test/extensions/matching/actions/format_string/config_test.cc @@ -23,8 +23,10 @@ TEST(ConfigTest, TestConfig) { testing::NiceMock factory_context; ActionFactory factory; - auto action = - factory.createAction(config, factory_context, ProtobufMessage::getStrictValidationVisitor()); + auto action_cb = factory.createActionFactoryCb(config, factory_context, + ProtobufMessage::getStrictValidationVisitor()); + ASSERT_NE(nullptr, action_cb); + auto action = action_cb(); 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 a1dc887af4abb..b419480bd4c25 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() override; + ~MockCodecEventCallbacks(); 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_;