allow HTTP filters to be marked as optional

[markdroth]

#14982 added new fields to the API to allow HTTP filters to be marked as optional -- i.e., if a filter is not known to a client, the client will ignore that particular filter instead of NACKing the entire config. gRPC is implementing this now, and we'd like Envoy to have the same behavior.

CC @htuch

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[markdroth]

@htuch, can we tag this so that it doesn't go stale? Thanks!

[markdroth]

It doesn't look like this was fully fixed. It looks like #16119 added support for the HttpFilter.is_optional field but did not add support for the FilterConfig.is_optional field in typed_per_filter_config. Instead, it looks like it plumbed a list of optional filters down from the HCM config down into the RouteConfig validation code, which is used to determine which entries in typed_per_filter_config are optional.

I don't think that approach will actually work once Envoy has the kind of caching xDS client layer that @adisuissa is working on for #13009, because I don't think that caching layer would be able to tie the RDS watch to an individual Listener that uses it, since there could actually be multiple Listeners that use the same RDS resource, and those two Listeners could have different lists of optional filters.

Given that, I think Envoy still needs to support the FilterConfig.is_optional field.

[htuch]

@wbpcode

[soulxu]

It doesn't look like this was fully fixed. It looks like #16119 added support for the HttpFilter.is_optional field but did not add support for the FilterConfig.is_optional field in typed_per_filter_config. Instead, it looks like it plumbed a list of optional filters down from the HCM config down into the RouteConfig validation code, which is used to determine which entries in typed_per_filter_config are optional.

Maybe I should this question #27210 (comment) here.

Not sure why we need is_optional for pre route config. Since for the upgrade case, the new filter and its pre-route config should be optional, otherwise it will still break the old version envoy.

[wbpcode]

Not sure why we need is_optional for pre route config. Since for the upgrade case, the new filter and its pre-route config should be optional, otherwise it will still break the old version envoy.

To avoid route config rejection?

Miss understand the question, sorry, ignore this message.

[markdroth]

Not sure why we need is_optional for pre route config. Since for the upgrade case, the new filter and its pre-route config should be optional, otherwise it will still break the old version envoy.

I'm not sure what you mean by "pre route config".

The reason for needing is_optional in the per-route/vh filter configs is the same as the reason for needing is_optional in the HCM config. When the data plane receives the RDS resource, it needs to validate that resource -- and that validation needs to evaluate only that RDS resource in isolation, without any knowledge of which HCM instance(s) the RouteConfig may be used with, as per discussion in #26097. The validation needs to look up the filter implementation based on the type_url and make sure that the filter implementation is supported; if not, it needs to NACK the RDS resource. The is_optional field allows the data plane to know that if the filter implementation is not supported, it's okay to ignore it instead of NACKing the RDS resource.

[soulxu]

Not sure why we need is_optional for pre route config. Since for the upgrade case, the new filter and its pre-route config should be optional, otherwise it will still break the old version envoy.

I'm not sure what you mean by "pre route config".

sorry, I mean the pre-route/vh filter configs here.

The reason for needing is_optional in the per-route/vh filter configs is the same as the reason for needing is_optional in the HCM config. When the data plane receives the RDS resource, it needs to validate that resource -- and that validation needs to evaluate only that RDS resource in isolation, without any knowledge of which HCM instance(s) the RouteConfig may be used with, as per discussion in #26097. The validation needs to look up the filter implementation based on the type_url and make sure that the filter implementation is supported; if not, it needs to NACK the RDS resource. The is_optional field allows the data plane to know that if the filter implementation is not supported, it's okay to ignore it instead of NACKing the RDS resource.

If allow the user config is_optional in the per-route/vh filter configs, then it may have the case: HCM marks the filter's is_optional as false, then per-route/vh marks the filter's is_optional as true. I'm thinking why we need such case.

I guess that for the case: HCM still using the old config (doesn't include the new filter yet), then the RDS pushes a new config which includes a new filter's pre-route/vh filter configs, then we need to mark that new filter's pre-route/vh filter configs as optional. Then envoy won't NACK that config.

[markdroth]

sorry, I mean the pre-route/vh filter configs here.

I am guessing that you actually mean per-route, not pre-route. :)

If allow the user config is_optional in the per-route/vh filter configs, then it may have the case: HCM marks the filter's is_optional as false, then per-route/vh marks the filter's is_optional as true. I'm thinking why we need such case.

Yes, that config is possible. In that case, if the filter is not supported by a data plane, then the data plane will accept the HCM config but reject the RDS resource.

There's probably no use-case where that's actually desirable, but I don't see how we can do better without breaking cachability.

I guess that for the case: HCM still using the old config (doesn't include the new filter yet), then the RDS pushes a new config which includes a new filter's pre-route/vh filter configs, then we need to mark that new filter's pre-route/vh filter configs as optional. Then envoy won't NACK that config.

Yes, if a filter is actually not supported by the data plane, then the filter's config needs to be marked as is_optional in both the HCM config and in the per-route/vh config.

[soulxu]

sorry, I mean the pre-route/vh filter configs here.

I am guessing that you actually mean per-route, not pre-route. :)

thanks! finally knowing where I make the thing confused :)

[wbpcode]

closed by #27263