Convert match_delegate and composite filter as dual filter (2nd approach)

[yanjunxiang-google]

Convert match_delegate and composite filter as dual filter.

As match_delegate filter has downstream filter factory logic, treat it as special case. i.e, Having it inherit from
NamedHttpFilterConfigFactory, and UpstreamHttpFilterConfigFactory, kind like DualFactoryBase.

This PR is an alternative approach of #32777.

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

[yanjunxiang-google]

/assign @wbpcode @tyxia @juanmolle

[repokitteh-read-only]

@juanmolle cannot be assigned to this issue.

🐱

Caused by: a #33013 (comment) was created by @yanjunxiang-google.

see: more, trace.

[yanavlasov]

/wait

[tyxia]

/assign @tyxia

[tyxia]

Thanks for working on this!

This PR has made some good progress, but i think it is missing some changes for delegating to upstream filter. Please see my comments for more details.

[tyxia]

/wait

(label was removed by my comment...)

[juanmolle]

I have left a comment in the old PR, not an issue I guess but could need to verify

Just one comment, there are some concerns regarding accessing streamInfo at the moment to convert the filter into dual filter. this filter push data into filter_state https://github.com/envoyproxy/envoy/blob/main/source/extensions/filters/http/composite/filter.cc#L143 could that be a problem? if that is the case perhaps this logic could be skiped if it is an upstream config.

According to this document https://github.com/envoyproxy/envoy/blob/main/source/docs/upstream_filters.md upstream filters should access streamInfo in preference as const. It does not say it is a must and that was my question about.

ref
Either the filter does not access streamInfo in a non-const way, or you test and document how the filter interacts with hedging and retries. Note that for hedging, a single downstream StreamInfo is accessible in parallel to both instances of the upstream HTTP filter instance, so it must be resiliant to parallel access.

On the other hand as upstream filter that piece of code probably has no sens and could be skipped now and evaluate it later. (probably just keep the matched action is ok on retries to deduplicate it)
https://github.com/envoyproxy/envoy/blob/main/source/extensions/filters/http/common/factory_base.h#L135

From @yanjunxiang-google investigation:

I think updateFilterState() in composite filter is needed even the filter is in upstream filter chain.
A couple of reasons:

• We probably want to setup the actions filter state for other filters to access.
• The filter state has life time as FilterChain, so the filter state will get out-of-life during retries/hedging. So, it won't be an issue in those cases.

[tyxia]

Nice work!

Mostly LGTM, modulo some comments. Thanks!

[wbpcode]

LGTM overall with nit comment. Thanks.

[wbpcode]

/wait-any

[yanjunxiang-google]

The filter state has life time as FilterChain, so the filter state will get out-of-life during retries/hedging. So, it won't be an issue in those cases.

Are you sure to this point? Seem the composite filter set the state to the downstream stream info and which have almost same lifetime with the whole request (except the internal redirect).

I think the original concern is that if the filter is upstream filter chain, and in case of hedging or retries, how the filter state should behave? In that case, I guess the logic here can handle it:

envoy/source/extensions/filters/http/composite/filter.cc

Line 147 in d07b522

if (info != nullptr) {

. i.e, it always update the filter state to the latestion action_name.

You raised a good point that the composite filter might be configured in both downstream and upstream filter chain. Is your concern that as there is just one key:

envoy/source/extensions/filters/http/composite/filter.h

Line 20 in d07b522

constexpr absl::string_view MatchedActionsFilterStateKey =

, that will cause conflicts?

Anyway, I raised an issue #33343 to track this.

If the consensus is to skip writing the filter state if the composite filter is in upstream, I will create a separate PR to address it.

[yanjunxiang-google]

Kind ping!

[yanavlasov]

Would it work if you split this review in two, one for each filter, to make it easier to review?

/wait-any

[yanjunxiang-google]

Would it work if you split this review in two, one for each filter, to make it easier to review?

/wait-any

Thanks for the comments!
I thought about that. The problem is that match_delegate and composite filter have to work together to achieve dynamic filter chain selection feature. We need to convert them together to be able to write tests to verify they indeed work in the upstream filter chain.