router: deprecate optional http filters and add route/vh level `is_optional` support in the typed_per_filter_config by wbpcode · Pull Request #27263 · envoyproxy/envoy

wbpcode · 2023-05-09T03:19:28Z

Commit Message: router: remove optional http filters because it's hcm dependent
Additional Description:

The ConfigImpl (route table) created by the rds may shared by multiple listeners/hcm. It means that the ConfigImpl should be hcm independent.

However, in the current codebase, the optional http filters is extracted from the HCM and used for the ConfigImpl creation.
Because the xds cache, the first hcm's http filters list will be used.

This typically a bug or potential error becasue different HCMs may have complete different http filters list and optional configuration.

Risk Level: mid.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.
[Optional Runtime guard:] n/a.

Signed-off-by: wbpcode <[email protected]>

wbpcode · 2023-05-09T03:22:01Z

cc @markdroth because you concern #15025

wbpcode · 2023-05-09T03:24:47Z

There is a quesion about the PR self:

should a runtime flag to be added? although i think this is a bug. But it was introduced at two years ago. May be some users depend on current behavior? cc @envoyproxy/runtime-guard-changes cc @mattklein123

soulxu · 2023-05-09T09:29:09Z

There is a quesion about the PR self:

should a runtime flag to be added? although i think this is a bug. But it was introduced at two years ago. May be some users depend on current behavior? cc @envoyproxy/runtime-guard-changes cc @mattklein123

As my understanding the usecase of is_optional is primary for the case of upgrade envoy in the cluster, then new filter which is passed down by the control-plane won't break the functionality of the node with old version envoy. I guess for this case, this new filter will be marked as optional at all HCMs' config, it doesn't make sense in some HCMs are optional and some HCMs aren't optional if the user want the old envoy running correctly. I guess this is why this bug won't be found until now. If we directly remove this feature, I guess this will break some users depending on this.

I quickly search the code. maybe we can add the optional config into the hash of the provider

envoy/source/common/rds/route_config_provider_manager.cc

Line 82 in 7657d0b

const uint64_t manager_identifier = MessageUtil::hash(rds);

Then we can get different provider instances for different optional config

Not sure whether this can be an acceptable temporary fix. if yes, then you can have a better fix later and won't take the risk break some users.

wbpcode · 2023-05-09T09:55:08Z

cc @soulxu See #26097 and related discussion. I think it's similar to your new idea and we had did lots of related discussion about it.

It's ok for me. depend on the decision of community.

mattklein123

Please add some kind of test, thank you.

/wait

wbpcode · 2023-05-09T15:35:30Z

@mattklein123 I think I need some more input from the community first.

Will the suggestion from @soulxu be a possible better way to solve this problem?
This is related to rds, so I want consider it more carefully and take various options into account.

markdroth · 2023-05-09T15:53:38Z

I think this is the right behavior, but I suggest combining this PR with #27210. That way, any user that runs into a problem will be able to solve it with the is_optional mechanism.

I don't entirely understand @soulxu's suggestion, but I think the behavior we want here is:

When we receive the RDS resource, we iterate through the filter configs, checking the type_url for each one to see if that filter impl is supported.
If the filter impl is not supported, the default behavior is to NACK the RDS resource.
If the filter impl is not supported but the entry is marked as is_optional, then we ignore the entry instead of NACKing.

Note that this behavior has already been agreed upon and has been implemented by gRPC. I don't think we want Envoy to do something different.

soulxu · 2023-05-09T22:56:06Z

When we receive the RDS resource, we iterate through the filter configs, checking the type_url for each one to see if that filter impl is supported.

If the filter impl is not supported, the default behavior is to NACK the RDS resource.

If the filter impl is not supported but the entry is marked as is_optional, then we ignore the entry instead of NACKing.

Do we want the HCM's is_optional apply to the pre-route/vh filter config when is_optional isn't specified in the pre-route/vh filter config?

wbpcode · 2023-05-10T02:26:00Z

I don't entirely understand @soulxu's suggestion, but I think the behavior we want here is:

It basically like #26097. We we shared same rds resource and proto config in multiple HCMs. But we may have different config providers based on the optional filters list. Rds resource will be accepted if all config providers validated it.

But this way are more complicate and not sure if has other potential problem. So I didn't consider that first.

Of course we can fix current error directly like this PR did. But this error was exist for two years. I worry about the back compatiability. If some users depend on this 'error', orz.

Not sure if a runtime guard enough.

soulxu · 2023-05-10T13:31:08Z

Of course we can fix current error directly like this PR did. But this error was exist for two years. I worry about the back compatiability. If some users depend on this 'error', orz.

Not sure if a runtime guard enough.

yea, the trouble is we documented this behavior in API doc

envoy/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

Lines 1143 to 1144 in bedd2b6

    
           // This is also same with typed per filter config. 
        
           bool is_optional = 6;

That is declared behavior, so in theory, removing it isn't right. except we consider this behavior is fully not working, then it is fine to delete it directly. But it work some case, not sure whether have people depend on it.

markdroth · 2023-05-10T17:07:10Z

Do we want the HCM's is_optional apply to the pre-route/vh filter config when is_optional isn't specified in the pre-route/vh filter config?

No. We can't do that without breaking cacheability.

It basically like #26097. We we shared same rds resource and proto config in multiple HCMs. But we may have different config providers based on the optional filters list. Rds resource will be accepted if all config providers validated it.

I don't think that will work, because the set of HCM configs can change over time. Consider the case where we initially have 3 HCM configs that point to the same RDS resource, we ACK that RDS resource, and then later we get a new HCM config that does not work with the RDS resource. At that point, the RDS resource would suddenly be considered invalid, but it's too late to NACK it.

As I said in the discussion in #26097, the current xDS ACK/NACK mechanism is inherently limited to validating the resource in isolation, without considering how it interacts with other resources. Any other behavior breaks cacheability.

yea, the trouble is we documented this behavior in API doc

I didn't realize we had that comment there, and I agree that it should be updated. But the wording there is actually so vague that I'm not sure anyone reading it would actually assume it means the current behavior.

wbpcode · 2023-05-11T01:46:09Z

I don't think that will work, because the set of HCM configs can change over time. Consider the case where we initially have 3 HCM configs that point to the same RDS resource, we ACK that RDS resource, and then later we get a new HCM config that does not work with the RDS resource. At that point, the RDS resource would suddenly be considered invalid, but it's too late to NACK it.

@markdroth yeah, I think you are right. I didn't thought this point in the past. Thanks.

wbpcode · 2023-05-11T01:47:23Z

Then, I think there no other choice. I will add a runtime guard and update doc. And also the tests.

This reverts commit 1725ce7. Signed-off-by: wbpcode <[email protected]>

Signed-off-by: wbpcode <[email protected]>

repokitteh-read-only · 2023-05-14T05:55:55Z

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #27263 was synchronize by wbpcode.

see: more, trace.

wbpcode · 2023-05-14T05:56:34Z

cc @alyssawilk for runtime guard.

Signed-off-by: wbpcode <[email protected]>

wbpcode · 2023-05-14T11:01:50Z

cc @mattklein123 ready for a formal review. Could you take a look when you have free time, thanks.

Signed-off-by: wbpcode <[email protected]>

alyssawilk · 2023-05-25T17:16:04Z

I can do second pass but first can @adisuissa and @markdroth make sure to tag resolved conversation as resolved? It's a bit hard to read at least parts of diffs with all the comments, and I don't want to resolve anything still in progress

markdroth · 2023-05-25T22:32:09Z

I don't have permission to resolve or unresolve comment threads. But I commented on the one thread that @alyssawilk resolved that I think is still open.

…emote-dependency-of-rds-to-hcm

Signed-off-by: wbpcode <[email protected]>

wbpcode · 2023-05-30T01:29:51Z

Hi, @markdroth @adisuissa, could you give a stamp about this PR if it ready for a second pass? Then we can push this to the next phase. 😄

alyssawilk

LGTM modulo one last question about API breaking changes

source/common/router/config_impl.cc

wbpcode · 2023-05-31T02:08:12Z

/retest

Signed-off-by: wbpcode <[email protected]>

The review outdated.

wbpcode · 2023-05-31T04:52:57Z

Tests were fixed and a new test to cover the new exception thowing was added.

Could you giva a new stamp? cc @alyssawilk Thanks.

wbpcode · 2023-06-07T01:48:48Z

close #15025

…tional` support in the typed_per_filter_config (envoyproxy#27263) * router: remove optional http filters because it's hcm dependent Signed-off-by: wbpcode <[email protected]> * Revert "router: remove optional http filters because it's hcm dependent" This reverts commit 1725ce7. Signed-off-by: wbpcode <[email protected]> * add runtime guard/tests/change log Signed-off-by: wbpcode <[email protected]> * fix more test Signed-off-by: wbpcode <[email protected]> * add route/virual host level optional flag support and test and change log Signed-off-by: wbpcode <[email protected]> * fix format Signed-off-by: wbpcode <[email protected]> * fix docs Signed-off-by: wbpcode <[email protected]> * fix test Signed-off-by: wbpcode <[email protected]> * Update changelogs/current.yaml Co-authored-by: Adi (Suissa) Peleg <[email protected]> Signed-off-by: code <[email protected]> * address all comments Signed-off-by: wbpcode <[email protected]> * fix conflict Signed-off-by: wbpcode <[email protected]> * address comment Signed-off-by: wbpcode <[email protected]> * address comments Signed-off-by: wbpcode <[email protected]> * fix test and add new test to cover the new exception throwing Signed-off-by: wbpcode <[email protected]> --------- Signed-off-by: wbpcode <[email protected]> Signed-off-by: code <[email protected]> Co-authored-by: Adi (Suissa) Peleg <[email protected]> Signed-off-by: Ryan Eskin <[email protected]>

router: remove optional http filters because it's hcm dependent

1725ce7

Signed-off-by: wbpcode <[email protected]>

wbpcode assigned soulxu, mattklein123, moderation and markdroth and unassigned moderation May 9, 2023

wbpcode mentioned this pull request May 9, 2023

http: route level filter disabled support #27210

Merged

mattklein123 previously requested changes May 9, 2023

View reviewed changes

repokitteh-read-only bot added the waiting label May 9, 2023

wbpcode added 2 commits May 14, 2023 05:04

Revert "router: remove optional http filters because it's hcm dependent"

517ffb5

This reverts commit 1725ce7. Signed-off-by: wbpcode <[email protected]>

add runtime guard/tests/change log

c81021c

Signed-off-by: wbpcode <[email protected]>

repokitteh-read-only bot added api and removed waiting labels May 14, 2023

wbpcode assigned alyssawilk May 14, 2023

fix more test

a67c1b6

Signed-off-by: wbpcode <[email protected]>

repokitteh-read-only bot assigned alyssawilk May 25, 2023

envoyproxy deleted a comment from repokitteh-read-only bot May 25, 2023

address comment

5d68afe

Signed-off-by: wbpcode <[email protected]>

wbpcode dismissed adisuissa’s stale review via 5d68afe May 25, 2023 01:51

wbpcode added 2 commits May 30, 2023 01:20

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-r…

f232d4b

…emote-dependency-of-rds-to-hcm

address comments

50566d1

Signed-off-by: wbpcode <[email protected]>

alyssawilk reviewed May 30, 2023

View reviewed changes

source/common/router/config_impl.cc Show resolved Hide resolved

alyssawilk previously approved these changes May 30, 2023

View reviewed changes

fix test and add new test to cover the new exception throwing

f9ab568

Signed-off-by: wbpcode <[email protected]>

wbpcode dismissed alyssawilk’s stale review via f9ab568 May 31, 2023 02:22

wbpcode requested review from mattklein123 and snowp May 31, 2023 02:22

alyssawilk approved these changes May 31, 2023

View reviewed changes

wbpcode merged commit 9ecf761 into envoyproxy:main Jun 1, 2023

wbpcode deleted the dev-remote-dependency-of-rds-to-hcm branch June 7, 2023 01:48

wbpcode mentioned this pull request Jun 7, 2023

allow HTTP filters to be marked as optional #15025

Closed

wbpcode mentioned this pull request Dec 4, 2023

docs: update route specified filter chain related docs #31158

Merged

Conversation

wbpcode commented May 9, 2023

Uh oh!

wbpcode commented May 9, 2023

Uh oh!

wbpcode commented May 9, 2023

Uh oh!

soulxu commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wbpcode commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

wbpcode commented May 9, 2023

Uh oh!

markdroth commented May 9, 2023

Uh oh!

soulxu commented May 9, 2023

Uh oh!

wbpcode commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soulxu commented May 10, 2023

Uh oh!

markdroth commented May 10, 2023

Uh oh!

wbpcode commented May 11, 2023

Uh oh!

wbpcode commented May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented May 14, 2023

Uh oh!

wbpcode commented May 14, 2023

Uh oh!

wbpcode commented May 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyssawilk commented May 25, 2023

Uh oh!

markdroth commented May 25, 2023

Uh oh!

wbpcode commented May 30, 2023

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wbpcode commented May 31, 2023

Uh oh!

wbpcode commented May 31, 2023

Uh oh!

wbpcode commented Jun 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

soulxu commented May 9, 2023 •

edited

Loading

wbpcode commented May 9, 2023 •

edited

Loading

wbpcode commented May 10, 2023 •

edited

Loading

wbpcode commented May 11, 2023 •

edited

Loading

wbpcode commented May 14, 2023 •

edited

Loading