feat(envoyextensionpolicy): Implement TLS configuration for WASM code source.#7865
feat(envoyextensionpolicy): Implement TLS configuration for WASM code source.#7865achernev wants to merge 12 commits intoenvoyproxy:mainfrom
Conversation
|
Tested and working with the following (certain elements removed for brevity): apiVersion: gateway.envoyproxy.io/v1alpha1
kind: EnvoyExtensionPolicy
spec:
wasm:
- name: wasm-image-test
code:
type: Image
image:
url: registry.example.com/library/wasm-filter:latest
tls:
caCertificateRef:
name: existing-cluster-trust-bundle
group: ""
kind: ClusterTrustBundle |
… source. Signed-off-by: Anton Chernev <anton.chernev@gmail.com>
Signed-off-by: Anton Chernev <anton.chernev@gmail.com>
|
@achernev Thanks for picking this up! Can we add an e2e test for the https wasm source? |
|
@zhaohuabing Will do. A couple of questions:
|
You should be able to run WASM e2e test using " E2E_RUN_TEST=WasmHTTPCodeSource make e2e", it'll create a local kind cluster, install EG, and run the test.
Yes, you can modify the existing test: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7865 +/- ##
==========================================
+ Coverage 73.65% 73.68% +0.03%
==========================================
Files 237 237
Lines 35653 35798 +145
==========================================
+ Hits 26259 26379 +120
- Misses 7530 7550 +20
- Partials 1864 1869 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Anton Chernev <anton.chernev@gmail.com>
Signed-off-by: Anton Chernev <anton.chernev@gmail.com>
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@zhaohuabing Added an HTTP TLS e2e test. Had to expand the resource indexer a bit to include objects referenced by the code source configuration because it was only considering ConfigMaps from the LUA section. With regard to the Codecov report: the lines not covered by tests are the fallback to an empty cert pool when |
Signed-off-by: Anton Chernev <anton.chernev@gmail.com>
|
@zhaohuabing Regarding the failing test called This is to say nothing of the fact that the logic in func AlmostEquals(actual, expect, offset int) bool {
upper := actual + offset
lower := actual - offset
if expect < lower || expect > upper {
return false
}
return true
}With this run's success number (49), that means that the numbers are: upper = 49 + 3 = 52, lower = 49 - 3 = 46, expect = 50 * 0.9 = 45, 45 < 46 is still a fail, hence the outcome. I am not sure why the |
|
Hi @achernev No worries. These tests are flaky and they are not related to this PR. I'll review the PR later when I get a moment. Thanks for your patience! |
Signed-off-by: Anton Chernev <anton.chernev@gmail.com>
Signed-off-by: Anton Chernev <anton.chernev@gmail.com>
|
@zhaohuabing Apologies for the delay here. I've fixed the test that broke and added a couple more to cover for most of the changes in the indexer. |
|
Overall this looks good. I just left a minor comment. |
|
I've removed the duplicated function. |
Signed-off-by: Anton Chernev <anton.chernev@gmail.com>
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
Signed-off-by: Anton Chernev <anton.chernev@gmail.com>
|
@zhaohuabing Apologies, missed the lint for the tests. |
|
@zhaohuabing Is that due to a flaky test? Looks like the server side does not come up in time for |
Yes, it's flaky. |
|
/retest |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions! |
|
@zhaohuabing I know you have a release cadence and policy but I’m wondering if this will make it into mainline? Thanks! |
What type of PR is this?
This implements an existing API.
What this PR does / why we need it:
This change wires in the existing API for configuring TLS when obtaining WASM code from a remote source. It covers both HTTP and OCI sources, and expands the feature to support
ClusterTrustBundlesalongsideConfigMapsandSecrets.The only change under
/apiis to remove the+notImplementedHideflags from theWasmCodeSourceTLSConfigparts of the configuration.Which issue(s) this PR fixes:
Fixes #4466.
Release Notes: Yes