Migrate the wasm filter to use grpc client cache.

[chaoqin-li1123]

Signed-off-by: chaoqin-li1123 [email protected]

Commit Message: Use AlwaysCache option for grpc client in ratelimit filter and wasm filter
Additional Description: NA
Risk Level: Medium, may have lifetime issue.
Testing: NA

[jmarantz]

can we split this into 2 PRs? If something goes wrong with one of them it'd be easier to roll back that way.

[jmarantz]

Let's have Piotr on the wasm team look at this first then I can stamp it. Looks straightforward enough.

[davinci26]

@PiotrSikora can you PTLAL?

[PiotrSikora]

I'm confused by this change.

Existing option (Grpc::CacheOption::CacheWhenRuntimeEnabled) means that cache behavior is controled using Envoy's runtime feature (envoy.reloadable_features.enable_grpc_async_client_cache), which is currently disabled by default.

Shouldn't the migration path be (1) "disabled by default" (using runtime features), (2) "enabled by default" (using runtime features), (3) "enabled in the code" with runtime feature removed?

Instead, we're migrating from "disabled by default" to "enabled in the code" without the "enabled by default" step. What's the reason for that?

[jmarantz]

/wait

[chaoqin-li1123]

The reason why we are not going through the standard runtime control procedure is that the there are many filters that use grpc cache, so this is not exactly a feature that can be controlled by a single runtime and we would like to enable grpc client cache in filters one by one.

[PiotrSikora]

The reason why we are not going through the standard runtime control procedure is that the there are many filters that use grpc cache, so this is not exactly a feature that can be controlled by a single runtime and we would like to enable grpc client cache in filters one by one.

I don't buy this argument (there is plenty of runtime features that affect multiple filters or Envoy as a whole), but it looks that few other filters were already migrated to AlwaysCache, so feel free to merge this.

[jmarantz]

I think I'd like someone outside Google, e.g. @lizan or @mattklein123 to just sanity-check this, but lgtm.

[github-actions]

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!

[jmarantz]

@envoyproxy/senior-maintainers can we get a non-googler senior maintainer review for this? Thanks!

[alyssawilk]

@jmarantz you tagged this as waiting. Is it actually waiting? If it's pure WASM we don't need a senior maintainer pass I think we just need Piotr sign off.

[alyssawilk]

But also it'll get more attention if it's not waiting, which of course exempts it from all the tooling :-/

[PiotrSikora]

@alyssawilk it's actually completely unrelated to Wasm, it's about gRPC client cache in Envoy, and it's been waiting forever since we cannot agree what's the correct process for migrating this runtime feature (see earlier comments).

[chaoqin-li1123]

I actually have another pr that flip the flag without enabling one filter each time.#19253

[alyssawilk]

Ah great. So I think the correct thing to do here is flip the runtime flag to true, then once it has been true for the standard period we can delete the flag, and switch all the call points to cache by default. @chaoqin-li1123 does this sound good to do and if so do you want to close this PR for now since the flags process will take a while?

[chaoqin-li1123]

Sounds good to me. I will close the PR.