Fix shared=true when no clientSelector,#6072
Conversation
ryanhristovski
commented
May 14, 2025
- fixes bug where when shared=true with no clientSelector it does not match
- cleans up filter logic (always adds listener)
- fixes duplicate rl descriptor logic & domain matching errors (was adding descriptors to the incorrect domain before)
- fixes e2e tests
…descriptor logic Signed-off-by: Ryan Hristovski <[email protected]>
Signed-off-by: Ryan Hristovski <[email protected]>
Signed-off-by: Ryan Hristovski <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6072 +/- ##
==========================================
- Coverage 70.61% 70.57% -0.05%
==========================================
Files 219 219
Lines 36515 36487 -28
==========================================
- Hits 25786 25750 -36
- Misses 9201 9210 +9
+ Partials 1528 1527 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Ryan Hristovski <[email protected]>
Signed-off-by: Ryan Hristovski <[email protected]>
Signed-off-by: Ryan Hristovski <[email protected]>
Signed-off-by: Ryan Hristovski <[email protected]>
|
|
|
Looks like the shared ratelimit tests are flaky. |
|
@zhaohuabing hmm, looks like its flaky due to giving no grace time for EG to pickup the RL rules - I could add a sleep or something for ~2s but this is just a naive approach |
Can we use require.eventually for this? |
Signed-off-by: Ryan Hristovski <[email protected]>
|
Updated with require.eventually to get a x-rate-limit header response before starting, hoping that'll work. |
Signed-off-by: Ryan Hristovski <[email protected]>
Signed-off-by: Ryan Hristovski <[email protected]>
|
@zhaohuabing yup that worked, other flaky tests failing now |
|
/retest |
|
@ryanhristovski LGTM. Resolving the conflicts then it's good to go. |
shawnh2
left a comment
There was a problem hiding this comment.
Just one non-blocking comment. LGTM
internal/xds/translator/ratelimit.go
Outdated
| filters := []*hcmv3.HttpFilter{} | ||
| created := make(map[string]bool) | ||
|
|
||
| domains := make(map[string]struct{}) |
There was a problem hiding this comment.
can we use k8s.io/apimachinery/pkg/util/sets instead?
Signed-off-by: Ryan Hristovski <[email protected]>
Signed-off-by: Ryan Hristovski <[email protected]>
Signed-off-by: Ryan Hristovski <[email protected]>
|
@zhaohuabing @shawnh2 fixed, theres some flaky tests unrelated failing though. |
|
/retest |
1 similar comment
|
/retest |
* Fix shared=true when no clientSelector, cleanup filter logic, fix rl descriptor logic Signed-off-by: Ryan Hristovski <[email protected]> * testdata update Signed-off-by: Ryan Hristovski <[email protected]> * Linting, remove unused funcs Signed-off-by: Ryan Hristovski <[email protected]> * fix e2e Signed-off-by: Ryan Hristovski <[email protected]> (cherry picked from commit bb3c8da) Signed-off-by: Arko Dasgupta <[email protected]>
* feat: set OverlappingTLSConfig condition for merged Gateways (#5862) * set OverlappingTLSConfig condition for merged Gateways Signed-off-by: Huabing (Robin) Zhao <[email protected]> * fix lint Signed-off-by: Huabing (Robin) Zhao <[email protected]> * minor change Signed-off-by: Huabing (Robin) Zhao <[email protected]> --------- Signed-off-by: Huabing (Robin) Zhao <[email protected]> (cherry picked from commit be51e5b) Signed-off-by: Arko Dasgupta <[email protected]> * e2e: fix backend tls test (#6029) * fix backend tls test Signed-off-by: Huabing (Robin) Zhao <[email protected]> * enable backend tls test Signed-off-by: Huabing (Robin) Zhao <[email protected]> * remove gateway TLS to simplify the test Signed-off-by: Huabing (Robin) Zhao <[email protected]> * rename secret to avoid conflicts Signed-off-by: Huabing (Robin) Zhao <[email protected]> --------- Signed-off-by: Huabing (Robin) Zhao <[email protected]> (cherry picked from commit a685667) Signed-off-by: Arko Dasgupta <[email protected]> * validate gateway namespace mode and merged gateways (#6041) * validate gateway namespace mode and merged gateways in translator Signed-off-by: Karol Szwaj <[email protected]> * fix lint Signed-off-by: Karol Szwaj <[email protected]> * skip merge gateways test Signed-off-by: Karol Szwaj <[email protected]> * validate on gatewayclass and set the status Signed-off-by: Karol Szwaj <[email protected]> * skip e2e test Signed-off-by: Karol Szwaj <[email protected]> * add valid testcases Signed-off-by: Karol Szwaj <[email protected]> * Update internal/provider/kubernetes/controller.go Co-authored-by: Arko Dasgupta <[email protected]> Signed-off-by: Karol Szwaj <[email protected]> * fix lint Signed-off-by: Karol Szwaj <[email protected]> * skip merge gateways test Signed-off-by: Karol Szwaj <[email protected]> * rebase Signed-off-by: Karol Szwaj <[email protected]> --------- Signed-off-by: Karol Szwaj <[email protected]> Co-authored-by: zirain <[email protected]> Co-authored-by: Arko Dasgupta <[email protected]> (cherry picked from commit c5f6831) Signed-off-by: Arko Dasgupta <[email protected]> * Fix shared=true when no clientSelector, (#6072) * Fix shared=true when no clientSelector, cleanup filter logic, fix rl descriptor logic Signed-off-by: Ryan Hristovski <[email protected]> * testdata update Signed-off-by: Ryan Hristovski <[email protected]> * Linting, remove unused funcs Signed-off-by: Ryan Hristovski <[email protected]> * fix e2e Signed-off-by: Ryan Hristovski <[email protected]> (cherry picked from commit bb3c8da) Signed-off-by: Arko Dasgupta <[email protected]> * fix(tranlator): SubjectAltNames were being dropped from BackendTLSPolicy.validation (#6092) * Add support for SubjectAltNames from BackendTLSPolicy.validation Signed-off-by: Ankush Agarwal <[email protected]> (cherry picked from commit 35420d5) Signed-off-by: Arko Dasgupta <[email protected]> * feat: add ownerreference to infra resources when gateway namespace mode (#6100) * feat: add ownerreference to infra resources when gateway namespace mode Signed-off-by: kkk777-7 <[email protected]> (cherry picked from commit fc462a8) Signed-off-by: Arko Dasgupta <[email protected]> * fix: add FullDuplexStreamed to enum (#6103) * fix: add FullDuplexStreamed to enum Signed-off-by: Guy Daich <[email protected]> (cherry picked from commit 020d60a) Signed-off-by: Arko Dasgupta <[email protected]> * fix: Use quoted values zone annotation in topology injector (#6133) * Quoted string for zone values Signed-off-by: jukie <[email protected]> * release note Signed-off-by: jukie <[email protected]> * regen Signed-off-by: jukie <[email protected]> (cherry picked from commit ea9cb05) Signed-off-by: Arko Dasgupta <[email protected]> * fix: return early from buildwasms (#6169) return early from buildwasms Signed-off-by: Guy Daich <[email protected]> (cherry picked from commit 64624fe) Signed-off-by: Arko Dasgupta <[email protected]> * chore: bump go and purego (#6174) * chore: bump go and purego Signed-off-by: zirain <[email protected]> * fix gen Signed-off-by: zirain <[email protected]> --------- Signed-off-by: zirain <[email protected]> (cherry picked from commit 40ae9e3) Signed-off-by: Arko Dasgupta <[email protected]> * fix: translate xds udp listener (#6183) * fix: translate udp listener Signed-off-by: kkk777-7 <[email protected]> * add: tcp/udp no routes testdata in xds translator Signed-off-by: kkk777-7 <[email protected]> * add: release note Signed-off-by: kkk777-7 <[email protected]> (cherry picked from commit 8f538e7) Signed-off-by: Arko Dasgupta <[email protected]> * Change static uid to for global ratelimit dashboard (#6193) Signed-off-by: Emin Aktas <[email protected]> (cherry picked from commit f721925) Signed-off-by: Arko Dasgupta <[email protected]> * Fix broken btp ratelimit merge (#6214) * Fix broken btp ratelimit merge Signed-off-by: Ryan Hristovski <[email protected]> * lint Signed-off-by: Ryan Hristovski <[email protected]> --------- Signed-off-by: Ryan Hristovski <[email protected]> (cherry picked from commit 0f6f363) Signed-off-by: Arko Dasgupta <[email protected]> * Keep ALPN configuration for listeners with overlapping certificates when ALPN is explicitly set via ClientTrafficPolicy (#6217) Keep ALPN configuration for listeners with overlapping certificates when ALPN is explicitly set in ClientTrafficPolicy Signed-off-by: Huabing (Robin) Zhao <[email protected]> (cherry picked from commit de816a6) Signed-off-by: Arko Dasgupta <[email protected]> * fix testdata Signed-off-by: Arko Dasgupta <[email protected]> * Allow for headless envoy services (#6250) * Allow for headless envoy services Signed-off-by: Ryan Hristovski <[email protected]> * Allow headless service, cleanup Signed-off-by: Ryan Hristovski <[email protected]> * clean Signed-off-by: Ryan Hristovski <[email protected]> * Add test and comment Signed-off-by: Ryan Hristovski <[email protected]> * Fix tests Signed-off-by: Ryan Hristovski <[email protected]> (cherry picked from commit 2e168a8) Signed-off-by: Arko Dasgupta <[email protected]> * remove infra ENVOY_GATEWAY_NAMESPACE and introduce ENVOY_POD_NAMESPACE envVar for accesslog (#6221) * remove infra ENVOY_GATEWAY_NAMESPACE and introduce ENVOY_POD_NAMESPACE envVar for accesslog Signed-off-by: Karol Szwaj <[email protected]> * fix e2e test Signed-off-by: Karol Szwaj <[email protected]> --------- Signed-off-by: Karol Szwaj <[email protected]> (cherry picked from commit b7ed197) Signed-off-by: Arko Dasgupta <[email protected]> * fix lint Signed-off-by: Arko Dasgupta <[email protected]> --------- Signed-off-by: Huabing (Robin) Zhao <[email protected]> Signed-off-by: Arko Dasgupta <[email protected]> Signed-off-by: Karol Szwaj <[email protected]> Signed-off-by: Ryan Hristovski <[email protected]> Signed-off-by: Ankush Agarwal <[email protected]> Signed-off-by: kkk777-7 <[email protected]> Signed-off-by: Guy Daich <[email protected]> Signed-off-by: jukie <[email protected]> Signed-off-by: zirain <[email protected]> Signed-off-by: Emin Aktas <[email protected]> Co-authored-by: Huabing (Robin) Zhao <[email protected]> Co-authored-by: Karol Szwaj <[email protected]> Co-authored-by: zirain <[email protected]> Co-authored-by: Ryan Hristovski <[email protected]> Co-authored-by: Ankush Agarwal <[email protected]> Co-authored-by: Kota Kimura <[email protected]> Co-authored-by: Guy Daich <[email protected]> Co-authored-by: Isaac <[email protected]> Co-authored-by: Emin AKTAS <[email protected]>
* feat: set OverlappingTLSConfig condition for merged Gateways (envoyproxy#5862) * set OverlappingTLSConfig condition for merged Gateways Signed-off-by: Huabing (Robin) Zhao <[email protected]> * fix lint Signed-off-by: Huabing (Robin) Zhao <[email protected]> * minor change Signed-off-by: Huabing (Robin) Zhao <[email protected]> --------- Signed-off-by: Huabing (Robin) Zhao <[email protected]> (cherry picked from commit be51e5b) Signed-off-by: Arko Dasgupta <[email protected]> * e2e: fix backend tls test (envoyproxy#6029) * fix backend tls test Signed-off-by: Huabing (Robin) Zhao <[email protected]> * enable backend tls test Signed-off-by: Huabing (Robin) Zhao <[email protected]> * remove gateway TLS to simplify the test Signed-off-by: Huabing (Robin) Zhao <[email protected]> * rename secret to avoid conflicts Signed-off-by: Huabing (Robin) Zhao <[email protected]> --------- Signed-off-by: Huabing (Robin) Zhao <[email protected]> (cherry picked from commit a685667) Signed-off-by: Arko Dasgupta <[email protected]> * validate gateway namespace mode and merged gateways (envoyproxy#6041) * validate gateway namespace mode and merged gateways in translator Signed-off-by: Karol Szwaj <[email protected]> * fix lint Signed-off-by: Karol Szwaj <[email protected]> * skip merge gateways test Signed-off-by: Karol Szwaj <[email protected]> * validate on gatewayclass and set the status Signed-off-by: Karol Szwaj <[email protected]> * skip e2e test Signed-off-by: Karol Szwaj <[email protected]> * add valid testcases Signed-off-by: Karol Szwaj <[email protected]> * Update internal/provider/kubernetes/controller.go Co-authored-by: Arko Dasgupta <[email protected]> Signed-off-by: Karol Szwaj <[email protected]> * fix lint Signed-off-by: Karol Szwaj <[email protected]> * skip merge gateways test Signed-off-by: Karol Szwaj <[email protected]> * rebase Signed-off-by: Karol Szwaj <[email protected]> --------- Signed-off-by: Karol Szwaj <[email protected]> Co-authored-by: zirain <[email protected]> Co-authored-by: Arko Dasgupta <[email protected]> (cherry picked from commit c5f6831) Signed-off-by: Arko Dasgupta <[email protected]> * Fix shared=true when no clientSelector, (envoyproxy#6072) * Fix shared=true when no clientSelector, cleanup filter logic, fix rl descriptor logic Signed-off-by: Ryan Hristovski <[email protected]> * testdata update Signed-off-by: Ryan Hristovski <[email protected]> * Linting, remove unused funcs Signed-off-by: Ryan Hristovski <[email protected]> * fix e2e Signed-off-by: Ryan Hristovski <[email protected]> (cherry picked from commit bb3c8da) Signed-off-by: Arko Dasgupta <[email protected]> * fix(tranlator): SubjectAltNames were being dropped from BackendTLSPolicy.validation (envoyproxy#6092) * Add support for SubjectAltNames from BackendTLSPolicy.validation Signed-off-by: Ankush Agarwal <[email protected]> (cherry picked from commit 35420d5) Signed-off-by: Arko Dasgupta <[email protected]> * feat: add ownerreference to infra resources when gateway namespace mode (envoyproxy#6100) * feat: add ownerreference to infra resources when gateway namespace mode Signed-off-by: kkk777-7 <[email protected]> (cherry picked from commit fc462a8) Signed-off-by: Arko Dasgupta <[email protected]> * fix: add FullDuplexStreamed to enum (envoyproxy#6103) * fix: add FullDuplexStreamed to enum Signed-off-by: Guy Daich <[email protected]> (cherry picked from commit 020d60a) Signed-off-by: Arko Dasgupta <[email protected]> * fix: Use quoted values zone annotation in topology injector (envoyproxy#6133) * Quoted string for zone values Signed-off-by: jukie <[email protected]> * release note Signed-off-by: jukie <[email protected]> * regen Signed-off-by: jukie <[email protected]> (cherry picked from commit ea9cb05) Signed-off-by: Arko Dasgupta <[email protected]> * fix: return early from buildwasms (envoyproxy#6169) return early from buildwasms Signed-off-by: Guy Daich <[email protected]> (cherry picked from commit 64624fe) Signed-off-by: Arko Dasgupta <[email protected]> * chore: bump go and purego (envoyproxy#6174) * chore: bump go and purego Signed-off-by: zirain <[email protected]> * fix gen Signed-off-by: zirain <[email protected]> --------- Signed-off-by: zirain <[email protected]> (cherry picked from commit 40ae9e3) Signed-off-by: Arko Dasgupta <[email protected]> * fix: translate xds udp listener (envoyproxy#6183) * fix: translate udp listener Signed-off-by: kkk777-7 <[email protected]> * add: tcp/udp no routes testdata in xds translator Signed-off-by: kkk777-7 <[email protected]> * add: release note Signed-off-by: kkk777-7 <[email protected]> (cherry picked from commit 8f538e7) Signed-off-by: Arko Dasgupta <[email protected]> * Change static uid to for global ratelimit dashboard (envoyproxy#6193) Signed-off-by: Emin Aktas <[email protected]> (cherry picked from commit f721925) Signed-off-by: Arko Dasgupta <[email protected]> * Fix broken btp ratelimit merge (envoyproxy#6214) * Fix broken btp ratelimit merge Signed-off-by: Ryan Hristovski <[email protected]> * lint Signed-off-by: Ryan Hristovski <[email protected]> --------- Signed-off-by: Ryan Hristovski <[email protected]> (cherry picked from commit 0f6f363) Signed-off-by: Arko Dasgupta <[email protected]> * Keep ALPN configuration for listeners with overlapping certificates when ALPN is explicitly set via ClientTrafficPolicy (envoyproxy#6217) Keep ALPN configuration for listeners with overlapping certificates when ALPN is explicitly set in ClientTrafficPolicy Signed-off-by: Huabing (Robin) Zhao <[email protected]> (cherry picked from commit de816a6) Signed-off-by: Arko Dasgupta <[email protected]> * fix testdata Signed-off-by: Arko Dasgupta <[email protected]> * Allow for headless envoy services (envoyproxy#6250) * Allow for headless envoy services Signed-off-by: Ryan Hristovski <[email protected]> * Allow headless service, cleanup Signed-off-by: Ryan Hristovski <[email protected]> * clean Signed-off-by: Ryan Hristovski <[email protected]> * Add test and comment Signed-off-by: Ryan Hristovski <[email protected]> * Fix tests Signed-off-by: Ryan Hristovski <[email protected]> (cherry picked from commit 2e168a8) Signed-off-by: Arko Dasgupta <[email protected]> * remove infra ENVOY_GATEWAY_NAMESPACE and introduce ENVOY_POD_NAMESPACE envVar for accesslog (envoyproxy#6221) * remove infra ENVOY_GATEWAY_NAMESPACE and introduce ENVOY_POD_NAMESPACE envVar for accesslog Signed-off-by: Karol Szwaj <[email protected]> * fix e2e test Signed-off-by: Karol Szwaj <[email protected]> --------- Signed-off-by: Karol Szwaj <[email protected]> (cherry picked from commit b7ed197) Signed-off-by: Arko Dasgupta <[email protected]> * fix lint Signed-off-by: Arko Dasgupta <[email protected]> --------- Signed-off-by: Huabing (Robin) Zhao <[email protected]> Signed-off-by: Arko Dasgupta <[email protected]> Signed-off-by: Karol Szwaj <[email protected]> Signed-off-by: Ryan Hristovski <[email protected]> Signed-off-by: Ankush Agarwal <[email protected]> Signed-off-by: kkk777-7 <[email protected]> Signed-off-by: Guy Daich <[email protected]> Signed-off-by: jukie <[email protected]> Signed-off-by: zirain <[email protected]> Signed-off-by: Emin Aktas <[email protected]> Co-authored-by: Huabing (Robin) Zhao <[email protected]> Co-authored-by: Karol Szwaj <[email protected]> Co-authored-by: zirain <[email protected]> Co-authored-by: Ryan Hristovski <[email protected]> Co-authored-by: Ankush Agarwal <[email protected]> Co-authored-by: Kota Kimura <[email protected]> Co-authored-by: Guy Daich <[email protected]> Co-authored-by: Isaac <[email protected]> Co-authored-by: Emin AKTAS <[email protected]> Signed-off-by: shawnh2 <[email protected]>