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/).

I personally still think that explicit configuration would be more clear. Give users the ability to control this searching behaviour. And this also keep the possibility to create more flexible searching policy in the future (e.g., get route level filter config by authentication/user info header).

And may be the fallback_policy can be used as default policy then the gRPC can complete ignore this API?

PS: this extendibility may also can be used to close #8659.

@mattklein123

If the fallback policy is applied, as the comments of the API, when we try to get route level filter config of a filter at runtime, the custom config name (HttpFilter.name) will be used first to search config from the typed_per_filter_config. And if we get nothing by the custom config name, then we will use the canonical filter name try again.

For most of current users, two time additional hash searching for every filter is necessary.
But it will not break most of current users except they use one filter's canonical filter name as another filter's custom config name. This exception is highly unlikely to happen.

So, yeah, fallback policy can covers most of known cases. But I personally prefer the explict configuration rather than implicit fallback. 😂 I want all these behaviors can be controlled by the configuration.

Of course, #8659 also a reason of that why I add this field.

Can we add explicit config later if it's really needed? Any config we add adds cost and it's not clear to me that there is enough benefit here? WDYT?

/wait

FWIW, I agree with @mattklein123. I think this approach is way too heavy-weight.

@wbpcode I hear that you are saying that you prefer this behavior to be explicitly configurable, but I'm not really hearing you give a compelling reason for that preference. Given the cost of supporting these kinds of configuration knobs, I think there needs to be a significant offsetting benefit to them in order to make them worthwhile to support. The main benefit that I could imagine in this case would be if there was a significant likelihood that we would break existing configs with the fallback approach, but you seem to agree that that approach is very unlikely to cause that kind of problem.

The other thing you mentioned was the overhead of doing multiple hash lookups. If that's the real concern here, then how about adding a runtime knob to allow Envoy to stop doing the lookup by the filter impl's canonical name? We can even use that as a way to transition away from the old behavior: for now, the knob's default can be to continue to support the old behavior, but in some future release we can change the default to disable it. And in the distant future, maybe we can even completely remove the legacy behavior entirely.

I hear that you are saying that you prefer this behavior to be explicitly configurable, but I'm not really hearing you give a compelling reason for that preference.

Part of the reason for this is my personal preference. Part of the reason is the duplication of searches. The last part is because I fear we may never be able to remove the legacy behaviour.

But since we all agree that the current configuration is too heavy, I will respect the community's opinion.

Part of the reason is the duplication of searches. The last part is because I fear we may never be able to remove the legacy behaviour.

Wouldn't the runtime knob I suggested address those two concerns?

Wouldn't the runtime knob I suggested address those two concerns?

🤔 If we can never remove this legacy behaviour, then we need keep the runtime knob forever. I usually use runtime knob in the code paths that will be removed after a few months.
But you can also can see many other uage ways of runtime guard, so this is just my personal perference and habit.

Anyway, I will respect the community's opinion.

convert this to draft but keep it open for #8659.

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!