api: new field to the FilterConfig to disable a filter in specific route or vh #25927

wbpcode · 2023-03-06T06:58:21Z

Commit Message: api: new field to the FilterConfig to disable a filter in specific route or vh
Risk Level: low.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

…ute or vh Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

repokitteh-read-only · 2023-03-06T06:58:33Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #25927 was opened by wbpcode.

see: more, trace.

wbpcode · 2023-03-06T06:59:32Z

draft idea to the #20867

htuch · 2023-03-07T03:59:20Z

Is this just a generic filter disable at round config override level? Seems reasonable to me. CC @envoyproxy/api-shepherds

wbpcode · 2023-03-07T07:05:09Z

Is this just a generic filter disable at round config override level? Seems reasonable to me. CC @envoyproxy/api-shepherds

Yeap. By this way, we can disable any filter in a specific route and needn't change any API of the filter. It means that the different routes could execute complete different filter chains.

ideally, we can skip the disabled filters' creation when we create the filter chain. Then the disable filters will not bring any additional overhead at runtime.

adisuissa · 2023-03-07T14:35:27Z

Thanks, this LGTM, and I like the idea of making it general across all filters.
One thing to note is what should happen to "Overridden" configs (some also have their own "disabled" flag) that have this flag set to true.
IMHO the config should be rejected, but not sure how to implement this in a better way other than looking at all filters adding this validation.

ideally, we can skip the disabled filters' creation when we create the filter chain. Then the disable filters will not bring any additional overhead at runtime.
That would be nice, but I'm not sure how this can be done for a per-route override. Some other route may need that filter, and so it is challenging to know which filters to instantiate a-priori.

markdroth · 2023-03-07T16:27:56Z

api/envoy/config/route/v3/route_components.proto

@@ -2384,4 +2384,8 @@ message FilterConfig {
  // not support the specified filter, it may ignore the map entry rather
  // than rejecting the config.
  bool is_optional = 2;
+
+  // If true, the filter is disabled in the route or virtual host and the


I think the comment here should make it clear that the intent is to not even include the filter in the filter chain in the first place, as opposed to including the filter and having it be a no-op. Otherwise it's not clear why we need this, since most individual filters already have a way to turn themselves into a no-op.

I think we should also document the fact that the decision to exclude the filter is made when the route is initially chosen for the request, before the first filter is called. If one of the filters later changes attributes of the request and recomputes the route, it is too late at that point to change the set of filters that will be run for the request.

Wow, you got it. that's what I want completely. 👍

Although, in my original opinion, the behavior depend on the implementation so I initially intend to document it more detailedly in a seperated doc.

kyessenov · 2023-03-08T03:03:03Z

This sounds reasonable to me too, as an alternative to composite filters for the typed_per_filter_configs, as a way to exempt some traffic from filters.

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

wbpcode · 2023-03-08T07:31:57Z

More detailed comment is added.

Advantages of this way:

Better performance. We can evaluate all these disable flag at config loading phase and skip unnecessary filter creations. And the disabled filters will skipped completely, no any decode*/encode* methods will be called.
Less memory overhead at runtime. Disabled filters will not be created.
Generic way to disable any filter and no any code/API change is necessary to the target filter.
Route level filter chain. Customize filter chain by enable/disable different filters set. (May be we can add a flag to tell envoy only add the explictly enabled filters to the filter chain in the future. 🤔 It will be helpful if there are lots of filters and most of routes only need one or two of these filters, for example enable cors for route A, enable ext_authz for route B, and enable cache for route C, etc.)

Disadvantages of this way:

Not friendly to the route refresh.
Some more memory to store the enable/disable flag.

kyessenov · 2023-03-08T17:10:09Z

@wbpcode I don't recall - does this require that a filter explicitly implements "most specific filter config" in code, or is this automatic?

wbpcode · 2023-03-09T01:40:14Z

@wbpcode I don't recall - does this require that a filter explicitly implements "most specific filter config" in code, or is this automatic?

This is automatic and it affects the filter chain directly, acting outside the filter.

wbpcode · 2023-03-09T02:54:21Z

/retest

repokitteh-read-only · 2023-03-09T02:54:28Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #25927 (comment) was created by @wbpcode.

see: more, trace.

htuch

LGTM

api/envoy/config/route/v3/route_components.proto

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

wbpcode · 2023-03-09T06:01:29Z

Seems there is no explict objection to the API and functions. I think we can merge it and then I can prepare a prototype of the implementation in the next days. 😄

markdroth · 2023-03-10T17:36:24Z

/lgtm api

wbpcode · 2023-03-13T06:59:30Z

A stamp from maintainer is still necessary. Friendly ping @htuch 😺

htuch · 2023-03-14T04:34:42Z

api/envoy/config/route/v3/route_components.proto

+  //   initial route will not be added back to the filter chain because the filter chain is already
+  //   created and it is too late to change the chain.
+  //
+  //   This field only make sense for the downstream HTTP filters for now.


Should add a not-implemented-yet annotation or merge this into the implementation PR. Otherwise LGTM, thanks.

The FilterConfig message are annotated as not-implemented. So, it's unnecessary to add the annotation seperatedly for this field.

htuch

/lgtm api

api: new field to the FilterConfig to disable a filter in specific ro…

309e9ca

…ute or vh Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

repokitteh-read-only bot added the api label Mar 6, 2023

repokitteh-read-only bot assigned markdroth Mar 6, 2023

wbpcode assigned htuch and kyessenov and unassigned markdroth Mar 6, 2023

markdroth reviewed Mar 7, 2023

View reviewed changes

wbpcode added 2 commits March 8, 2023 07:02

more detailed comments

142ec59

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

more detailed comments

a65b75c

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

htuch reviewed Mar 9, 2023

View reviewed changes

api/envoy/config/route/v3/route_components.proto Outdated Show resolved Hide resolved

update is_disabled to disabled

2cc0d4a

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

wbpcode mentioned this pull request Mar 10, 2023

[cache-filter] Add disabled option #25916

Merged

repokitteh-read-only bot removed the api label Mar 10, 2023

htuch reviewed Mar 14, 2023

View reviewed changes

kyessenov removed their assignment Mar 14, 2023

htuch approved these changes Mar 16, 2023

View reviewed changes

htuch merged commit 987bf50 into envoyproxy:main Mar 16, 2023

kyessenov mentioned this pull request Apr 3, 2023

enable Wasm filter enable per route #26440

Open

This was referenced May 5, 2023

http: route level filter chain support #27209

Closed

http: route level filter disabled support #27210

Merged

adisuissa mentioned this pull request Aug 11, 2023

match_delegate: Add runtime control #28937

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: new field to the FilterConfig to disable a filter in specific route or vh #25927

api: new field to the FilterConfig to disable a filter in specific route or vh #25927

wbpcode commented Mar 6, 2023

repokitteh-read-only bot commented Mar 6, 2023

wbpcode commented Mar 6, 2023

htuch commented Mar 7, 2023

wbpcode commented Mar 7, 2023 •

edited

Loading

adisuissa commented Mar 7, 2023

markdroth Mar 7, 2023

wbpcode Mar 8, 2023

kyessenov commented Mar 8, 2023

wbpcode commented Mar 8, 2023

kyessenov commented Mar 8, 2023

wbpcode commented Mar 9, 2023

wbpcode commented Mar 9, 2023

repokitteh-read-only bot commented Mar 9, 2023

htuch left a comment

wbpcode commented Mar 9, 2023

markdroth commented Mar 10, 2023

wbpcode commented Mar 13, 2023

htuch Mar 14, 2023 •

edited

Loading

wbpcode Mar 14, 2023

htuch left a comment

api: new field to the FilterConfig to disable a filter in specific route or vh #25927

api: new field to the FilterConfig to disable a filter in specific route or vh #25927

Conversation

wbpcode commented Mar 6, 2023

repokitteh-read-only bot commented Mar 6, 2023

wbpcode commented Mar 6, 2023

htuch commented Mar 7, 2023

wbpcode commented Mar 7, 2023 • edited Loading

adisuissa commented Mar 7, 2023

markdroth Mar 7, 2023

Choose a reason for hiding this comment

wbpcode Mar 8, 2023

Choose a reason for hiding this comment

kyessenov commented Mar 8, 2023

wbpcode commented Mar 8, 2023

kyessenov commented Mar 8, 2023

wbpcode commented Mar 9, 2023

wbpcode commented Mar 9, 2023

repokitteh-read-only bot commented Mar 9, 2023

htuch left a comment

Choose a reason for hiding this comment

wbpcode commented Mar 9, 2023

markdroth commented Mar 10, 2023

wbpcode commented Mar 13, 2023

htuch Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

wbpcode Mar 14, 2023

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

wbpcode commented Mar 7, 2023 •

edited

Loading

htuch Mar 14, 2023 •

edited

Loading