feat: support local Wasm via EnvoyProxy wasmModules allowlist - #9530
daanvinken wants to merge 19 commits into
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@zhaohuabing if you have some time 🙏 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53e53febd3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| wasms []ir.Wasm | ||
| wasmFailOpen bool | ||
| ) | ||
| if wasms, wasmError, wasmFailOpen = t.buildWasms(policy, resources, gtwCtx.envoyProxy); wasmError != nil { |
There was a problem hiding this comment.
Scope filesystem Wasm errors per ancestor
When an HTTPRoute is attached to multiple managed Gateways, this call now resolves the filesystem module against each Gateway's EnvoyProxy but appends any failure to one errs value returned to processEnvoyExtensionPolicyForRoute, which then sets the translation error on all ancestor refs. If one Gateway allowlists the module and another does not, the successful ancestor is still marked translation-failed even though its routes get the filter; please report/apply the error per parentRef instead of aggregating it across all ancestors.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pre-existing bug with ancestor refs. This should be fixed but is separate from your changes.
53e53fe to
089e532
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9530 +/- ##
==========================================
+ Coverage 81.31% 81.35% +0.03%
==========================================
Files 264 264
Lines 40994 41044 +50
==========================================
+ Hits 33336 33391 +55
+ Misses 7657 7652 -5
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
f4fe92e to
fddff27
Compare
containsWasm (internal/xds/translator/globalresources.go:144) returns true for any Wasm, so createWasmHTTPServiceCluster still emits the wasm_cluster (the EG HTTP serving cluster, plus its upstream TLS transportSocket) even when every Wasm is a Filesystem source that never contacts it. This is visible in the PR's own fixture wasm-filesystem.clusters.yaml, which contains a full wasm_cluster that no filter references. Suggest gating on the presence of a remote source: This avoids shipping a dead cluster (and its client-cert secret dependency) to proxies that only use filesystem modules — which is precisely the "skip the control-plane path" benefit the PR advertises.
In translateEnvoyExtensionPolicyForRoute, buildWasms moved from once-per-policy to once-per-parentRef (necessary, since the filesystem path resolves against gtwCtx.envoyProxy, which can differ per Gateway). This matches the existing buildLuas/buildDynamicModules placement, so structurally it's fine. The wrinkle unique to Wasm: for HTTP/Image sources this now calls t.WasmCache.Get() N times for a route with N parentRefs, where it previously ran once. For IfNotPresent this is a cheap cache hit, but with pullPolicy: Always it can trigger repeated fetches. Not a correctness bug, but a behavioral change worth a comment or a second look — a small memoization keyed by resolved config would eliminate it if it matters.
The field-level annotation changed from Enum=HTTP;Image;ConfigMap to Enum=HTTP;Image;Filesystem (wasm_types.go:41). ConfigMap was never implemented (no code path, and the type alias only listed HTTP;Image), so this is stale cleanup rather than a real removal. Still, it's technically a narrowing of the accepted API surface — worth an explicit mention in the PR description/release note in case anyone had it set (it would have been non-functional regardless). |
Register Wasm modules on EnvoyProxy.spec.wasmModules and reference them by name from EnvoyExtensionPolicy with type Filesystem. Envoy loads the local file via AsyncDataSource.local and skips the control-plane fetch/cache path used by HTTP and Image sources. Includes generated CRD/helm fixtures, API docs, and the jwt DnsCluster fixture update needed for gen-check. Fixes envoyproxy#9448 Signed-off-by: daanvinken <daanvinken@tythus.com>
fddff27 to
3e206c5
Compare
Only create the control-plane wasm HTTP service cluster (and gate the client certificate) when a route uses a remote HTTP/Image code source. Filesystem modules load a local path and never use that cluster. Signed-off-by: daanvinken <daanvinken@tythus.com>
Filesystem resolution is Gateway-scoped, so buildWasms runs per parentRef like Lua and DynamicModules. HTTP/Image may call WasmCache.Get once per parent; IfNotPresent hits the cache, Always may re-fetch. Signed-off-by: daanvinken <daanvinken@tythus.com>
ConfigMap was never implemented; the code source enum is now HTTP, Image, and Filesystem only. Signed-off-by: daanvinken <daanvinken@tythus.com>
|
Super helpful, thanks @zhaohuabing I've addressed your comments. I'll see when I can carve out some time to deploy this on an actual cluster and test. |
|
Should we mirror |
You should be able to add an e2e test for this, following the dynamic module example. You can use this WASM file. for the test. |
I agree with the suggested shape for filesystem WASM modules but long-term handling of image/http sources might get awkward so we should discuss that as well. |
Register wasmModules with source.type Local and source.local.path, and reference them from EnvoyExtensionPolicy as type EnvoyProxyModule. Leaves room for additional EnvoyProxy module sources later without implementing HTTP/OCI registration yet. Signed-off-by: daanvinken <daanvinken@tythus.com>
Add a custom Envoy image with the example Wasm module baked in, wire it into EXAMPLE_APPS, and add an e2e that registers the module on EnvoyProxy and asserts the filter response header. Signed-off-by: daanvinken <daanvinken@tythus.com>
The example Wasm filter appends text to the response body, which breaks JSON extraction of request properties. Match the HTTP Wasm e2e workaround. Signed-off-by: daanvinken <daanvinken@tythus.com>
|
Ready for another review, @zhaohuabing |
Some of the tests are flaky. You can comment |
|
Hi @daanvinken Thanks for working on this. I'll bring up the API design at this week's community meeting so we can reach a decision and unblock you. |
|
Pending discussion at community sync but LGTM overall, thanks! |
|
/retest |
Keep wasm-module-test in EXAMPLE_APPS using the new multiline list. Signed-off-by: daanvinken <daanvinken@tythus.com>
Upstream now emits rds.configSource.initialFetchTimeout: 0s. Signed-off-by: daanvinken <daanvinken@tythus.com>
|
Should we reference the module from the existing with code taking precedence while it still exists, and a registry lookup by name when it's absent. cc @guydc @arkodg do we want to deprecate the inline sources and remove them eventually? |
|
Thanks @zhaohuabing.
Two options:
I would not reuse I'm leaning towards 2. I would wait for @guydc / @arkodg on whether inline HTTP/Image should be deprecated. |
|
+1 to inline deprecation. |
|
Agreed, option 2 is what I had in mind. With code optional, the name-only entry mirrors dynamicModules and there's no one-member union to clean up later. Since @guydc is +1 on deprecating inline sources, I think this unblocks the PR. |
When EnvoyExtensionPolicy.wasm[].code is omitted, wasm[].name is looked up in EnvoyProxy.spec.wasmModules. Drop the EnvoyProxy code source union member. HTTP and Image stay on code. Signed-off-by: daanvinken <daanvinken@tythus.com>
Add MaxLength=253 on Wasm.Name so the omitted-code matches() rule stays under the CRD CEL budget. Remove initialFetchTimeout from the wasm-filesystem fixture; main no longer emits it. Signed-off-by: daanvinken <daanvinken@tythus.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9d522ab3d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: daanvinken <daanvinken@tythus.com>
| // | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=4096 | ||
| // +kubebuilder:validation:Pattern=`^/.*` |
There was a problem hiding this comment.
Non-blocking nit: LocalDynamicModuleSource.Path documents the same absolute-path requirement but only validates length, while other API path fields generally use only MinLength. Should we add ^/.* consistently? This can be tracked as a separate API cleanup.
There was a problem hiding this comment.
Sounds good, I'll track seperately.
Other comments are addressed, thanks again
The helper only matches remote HTTP/Image Wasm, not name-only local modules. Signed-off-by: daanvinken <daanvinken@tythus.com>
|
/retest |
Description
Adds local Wasm modules registered on EnvoyProxy, so Envoy can load
files from the proxy filesystem without the control-plane fetch/cache
path used by HTTP and Image sources.
Design matches #9448 and review discussion (DynamicModule-style allowlist):
EnvoyProxy.spec.wasmModuleswithsource.type: Localandsource.local.pathEnvoyExtensionPolicyby omittingcodeandsetting
wasm[].nameto the registered module name (no path on the policy)AsyncDataSource.localfor Local modulescodestill required for those sources)wasm_clusteris only created when a remote Wasm source is presentThe Wasm code type enum is now
HTTPandImage.The unused
ConfigMapenum value (never implemented) was removed.Remote/OCI registration on EnvoyProxy is intentionally not implemented yet;
the
sourceshape leaves room for that later.Fixes #9448
Testing
unknown module name is rejected)
code.local.filenameand no unusedwasm_clustercodeplus wasmModules Local source(
examples/wasm-module-test), assertsx-wasm-custom: FOO