router: deprecate optional http filters and add route/vh level is_optional support in the typed_per_filter_config

[wbpcode]

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.

[wbpcode]

cc @markdroth because you concern #15025

[wbpcode]

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]

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]

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]

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

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]

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

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]

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]

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]

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]

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

[repokitteh-read-only]

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]

cc @alyssawilk for runtime guard.

[wbpcode]

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

[alyssawilk]

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]

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.

[wbpcode]

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

[wbpcode]

/retest

The review outdated.

[wbpcode]

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]

close #15025