Converting match_delegate filter and composite filter as dual filter. by yanjunxiang-google · Pull Request #32777 · envoyproxy/envoy

yanjunxiang-google · 2024-03-07T20:51:44Z

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:]

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google · 2024-03-07T20:56:34Z

+
+  Factory::MatchTreeValidationVisitor validation_visitor(*factory.matchingRequirements());
+
+  Envoy::Http::Matching::HttpFilterActionContext action_context{prefix, absl::nullopt,


Is this an issue to pass in an absl::nullopt into this data object? @wbpcode

The reason we pass in an absl::nullopt is due to there is no Server::Configuration::FactoryContext object can be retrieved here.

yanjunxiang-google · 2024-03-07T22:31:55Z

/assign @tyxia @wbpcode @snowp

…_delegate_dual Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

wbpcode

Thanks for your contribution. This lgtm overall. Only two comments are added.

wbpcode · 2024-03-11T01:47:38Z

+    DualInfo, Server::Configuration::FactoryContext&) {
+  return createFilterFactory(proto_config);
+}
+
+absl::StatusOr<Http::FilterFactoryCb> BufferFilterFactory::createFilterFactoryFromProtoTyped(
+    const envoy::extensions::filters::http::buffer::v3::Buffer& proto_config, const std::string&,
+    DualInfo, Server::Configuration::UpstreamFactoryContext&) {
+  return createFilterFactory(proto_config);
+}
+
+absl::StatusOr<Http::FilterFactoryCb> BufferFilterFactory::createFilterFactory(
+    const envoy::extensions::filters::http::buffer::v3::Buffer& proto_config) {


Please do this in separated PR. Thanks.

To convert the match_delegate filter into dual filter, I have to change factory_base.h to have two interfaces of createFilterFactoryFromProtoTyped(), one for Server::Configuration::FactoryContext, and one for Server::Configuration::UpstreamFactoryContext&. Thus I have to change all the existing dual filter implementation.

I see. I think the change is because the match_delegate needs to access the original FactoryContext and UpstreamFactoryContext.

Then, rather than changing the DualFactoryBase's methods here, I prefer just to change the match_delegate to avoid inheriting from DualFactoryBase.
Thats say, the match_delegate could inherit the NamedHttpFilterConfigFactory and UpstreamHttpFilterConfigFactory .

If we do think the DualFactoryBase should update it's methods, we can do the refactoring in separated PR.

Do you mean have match_delegate speciall inerit from NamedHttpFilterConfigFactory and UpstreamHttpFilterConfigFactory to do something similar as the DualFactoryBase:

envoy/source/extensions/filters/http/common/factory_base.h

Line 121 in 10a1003

class DualFactoryBase : public CommonFactoryBase<ConfigProto, RouteConfigProto>,

, but slightly different?

I thought all dual filter should inherit from DualFactoryBase based on this note: https://github.com/alyssawilk/envoy/blob/main/source/docs/upstream_filters.md

My 2 cents: I tend to agree with @wbpcode that separate inheritance from NamedHttpFilterConfigFactory and UpstreamHttpFilterConfigFactory is probably a better idea (if this approach works).

Given the uniqueness of match_delegate filter, i think other filters won't need this new API. we can make match_delegate as an exception to that doc.

I don't have strong preference on this. However, if we go that route, and if another filter also has logic based on factory type(NamedHttpFilterConfigFactory/UpstreamHttpFilterConfigFactory), then we will have to make it another exception if we want to convert into dual filter.

Overall, I feel the way current DualFactoryBase method: createFilterFactoryFromProtoTyped() implementation, which only support parameter type ServerFactoryContext, greatly limited its flexibility. We should let it be able to support both FactoryContext and UpstreamFactoryContext, same as createFilterFactoryFromProto() in that class.

Kind ping on this!

Yeah. The match_delegate could be a special case. The docs just a recommend way to develop a new dual filter and not a strict rule.

cc @yanjunxiang-google

I created a new PR for this: #33013. In the new PR, the match_delegate filter is inherited from NamedHttpFilterConfigFactory and UpstreamHttpFilterConfigFactory , kind like DualFactoryBase.

wbpcode · 2024-03-11T01:54:34Z

  auto message = Config::Utility::translateAnyToFactoryConfig(
-      proto_config.extension_config().typed_config(), context.messageValidationVisitor(), factory);
+      proto_config.extension_config().typed_config(),
+      context.serverFactoryContext().messageValidationVisitor(), factory);


Hah, here is the common problem. You cannot use the context.serverFactoryContext().messageValidationVisitor() to replace the messageValidationVisitor() from correct context.

By the way, seems that a skilled envoy developer (from google) even cannot use this messageValidationVistor correctly, so I think we should re-discuss our previous plan to use single messageValidationVisitor(). cc @htuch

Thanks for the comments!

Do you mean for downstream context(FactoryContext), it should be context->messageValidationVisitor(), and can not be context.serverFactoryContext().messageValidationVisitor()? Is this messageValidationVisitor two different things or same thing accessed by two different means?

If they are two different things, I can pass in the messageValidationVisitor() from the caller.

They may be different things. 🤣

@wbpcode if you have a proposal to move #31073 forward happy to discuss, I agree there is some inconsistency of use here and it would be worth evaluating options to make it correct-by-default.

I will try to find some free time to do that.

tyxia

Great work! Thanks, Yanjun

Could you please take a look at CI failure. I guess AdmissionControlFilter also needs similar change as it is dual filter already

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

wbpcode · 2024-03-17T02:47:18Z

/wait

juanmolle · 2024-03-20T15:18:16Z

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.

yanjunxiang-google · 2024-03-20T19:25:53Z

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.

Could you please share a little bit background on this?

juanmolle · 2024-03-20T20:53:02Z

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

Any reason you close the PR. It seams core reviewers agree on this change and I feel it is really useful

yanjunxiang-google · 2024-03-20T21:00:38Z

Thanks for the info! I will take a look.

BTW, I closed this PR because some comments above says we should treat match_delegate filter as special case. Thus I created a new PR: #33013. Let's continue the review process under that PR.

yanjunxiang-google · 2024-03-20T21:44:59Z

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

Any reason you close the PR. It seams core reviewers agree on this change and I feel it is really useful

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.

@wbpcode could you please confirm?

Converting match_delegate filter and composite filter as dual filter.

2e35cfa

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google requested review from alyssawilk, htuch, mattklein123, soulxu, wbpcode and yanavlasov as code owners March 7, 2024 20:51

yanjunxiang-google commented Mar 7, 2024

View reviewed changes

yanjunxiang-google mentioned this pull request Mar 7, 2024

Make match_delegate filter a dual filter #32194

Closed

repokitteh-read-only Bot assigned snowp, tyxia and wbpcode Mar 7, 2024

Merge branch 'main' of https://github.com/envoyproxy/envoy into match…

44853a2

…_delegate_dual Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google marked this pull request as draft March 7, 2024 23:54

yanjunxiang-google marked this pull request as ready for review March 8, 2024 20:03

adding template functions to remove duplicated code

9ff099b

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

wbpcode reviewed Mar 11, 2024

View reviewed changes

tyxia reviewed Mar 11, 2024

View reviewed changes

wbpcode added the waiting label Mar 13, 2024

passing in message validation visitor

b419f8c

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

repokitteh-read-only Bot removed the waiting label Mar 13, 2024

repokitteh-read-only Bot added the waiting label Mar 17, 2024

yanjunxiang-google closed this Mar 20, 2024

yanjunxiang-google mentioned this pull request Mar 20, 2024

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

Merged


		Factory::MatchTreeValidationVisitor validation_visitor(*factory.matchingRequirements());

		Envoy::Http::Matching::HttpFilterActionContext action_context{prefix, absl::nullopt,

Conversation

yanjunxiang-google commented Mar 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanjunxiang-google commented Mar 7, 2024

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanjunxiang-google Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyxia Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanjunxiang-google Mar 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyxia left a comment

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Mar 17, 2024

Uh oh!

juanmolle commented Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanjunxiang-google commented Mar 20, 2024

Uh oh!

juanmolle commented Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanjunxiang-google commented Mar 20, 2024

Uh oh!

yanjunxiang-google commented Mar 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

yanjunxiang-google Mar 11, 2024 •

edited

Loading

tyxia Mar 13, 2024 •

edited

Loading

yanjunxiang-google Mar 14, 2024 •

edited

Loading

wbpcode Mar 12, 2024 •

edited

Loading

juanmolle commented Mar 20, 2024 •

edited

Loading

juanmolle commented Mar 20, 2024 •

edited

Loading