Skip to content

Fix shared=true when no clientSelector,#6072

Merged
arkodg merged 15 commits intoenvoyproxy:mainfrom
ryanhristovski:fix-no-clientselector-rl-and-domain-issue
May 27, 2025
Merged

Fix shared=true when no clientSelector,#6072
arkodg merged 15 commits intoenvoyproxy:mainfrom
ryanhristovski:fix-no-clientselector-rl-and-domain-issue

Conversation

@ryanhristovski
Copy link
Copy Markdown
Contributor

  • 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

@ryanhristovski ryanhristovski requested a review from a team as a code owner May 14, 2025 21:30
Signed-off-by: Ryan Hristovski <[email protected]>
Signed-off-by: Ryan Hristovski <[email protected]>
@codecov
Copy link
Copy Markdown

codecov bot commented May 14, 2025

Codecov Report

Attention: Patch coverage is 97.95918% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.57%. Comparing base (cbb5381) to head (793224b).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
internal/xds/translator/ratelimit.go 97.95% 0 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ryanhristovski ryanhristovski marked this pull request as draft May 14, 2025 22:42
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]>
@ryanhristovski ryanhristovski marked this pull request as ready for review May 15, 2025 02:06
@ryanhristovski
Copy link
Copy Markdown
Contributor Author

RateLimitGlobalMergeTest is passing successfully

2025-05-15T02:05:07.9767980Z --- FAIL: TestE2E/ExtProc/http_route_with_ext_proc (32.03s) seems to be failing consistently which is unrelated to this PR

zhaohuabing
zhaohuabing previously approved these changes May 15, 2025
Copy link
Copy Markdown
Member

@zhaohuabing zhaohuabing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks good. The ratlimit code is much cleaner now!

I just have a few minor non-blocking nits.

@zhaohuabing
Copy link
Copy Markdown
Member

Looks like the shared ratelimit tests are flaky.

@ryanhristovski
Copy link
Copy Markdown
Contributor Author

@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

@zhaohuabing
Copy link
Copy Markdown
Member

@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]>
@ryanhristovski
Copy link
Copy Markdown
Contributor Author

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]>
@ryanhristovski
Copy link
Copy Markdown
Contributor Author

@zhaohuabing yup that worked, other flaky tests failing now

@ryanhristovski
Copy link
Copy Markdown
Contributor Author

/retest

@zhaohuabing
Copy link
Copy Markdown
Member

@ryanhristovski LGTM. Resolving the conflicts then it's good to go.

zhaohuabing
zhaohuabing previously approved these changes May 23, 2025
shawnh2
shawnh2 previously approved these changes May 23, 2025
Copy link
Copy Markdown
Contributor

@shawnh2 shawnh2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one non-blocking comment. LGTM

filters := []*hcmv3.HttpFilter{}
created := make(map[string]bool)

domains := make(map[string]struct{})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use k8s.io/apimachinery/pkg/util/sets instead?

@ryanhristovski ryanhristovski dismissed stale reviews from shawnh2 and zhaohuabing via 6a96fbf May 27, 2025 12:38
@ryanhristovski
Copy link
Copy Markdown
Contributor Author

@zhaohuabing @shawnh2 fixed, theres some flaky tests unrelated failing though.

@ryanhristovski
Copy link
Copy Markdown
Contributor Author

/retest

1 similar comment
@ryanhristovski
Copy link
Copy Markdown
Contributor Author

/retest

@arkodg arkodg merged commit bb3c8da into envoyproxy:main May 27, 2025
55 of 63 checks passed
arkodg pushed a commit to arkodg/gateway that referenced this pull request Jun 3, 2025
* 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]>
arkodg added a commit that referenced this pull request Jun 4, 2025
* 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]>
shawnh2 pushed a commit to shawnh2/gateway that referenced this pull request Sep 15, 2025
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants