Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: rate limit doesn't work with two(and more) listeners #3085

Merged
merged 5 commits into from
Apr 8, 2024
Merged

fix: rate limit doesn't work with two(and more) listeners #3085

merged 5 commits into from
Apr 8, 2024

Conversation

zhaohuabing
Copy link
Member

@zhaohuabing zhaohuabing commented Apr 3, 2024 •
edited
Loading

This PR fixes the issue: rate limit doesn't work with two(and more) listeners

This was caused by the missing name in RateLimitConfig resource.

Fix: #3086

@zhaohuabing
fix: rate limit doesn't work with two listeners
2b5a2e6 
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
@zhaohuabing zhaohuabing requested a review from a team as a code owner April 3, 2024 14:48
@zhaohuabing zhaohuabing marked this pull request as draft April 3, 2024 14:51
@codecov Codecov
Copy link

codecov bot commented Apr 3, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.53%. Comparing base (29946b0) to head (40e996e).

❗ Current head 40e996e differs from pull request most recent head a033009. Consider uploading reports for the commit a033009 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3085      +/-   ##
==========================================
+ Coverage   66.51%   66.53%   +0.02%     
==========================================
  Files         161      161              
  Lines       22673    22675       +2     
==========================================
+ Hits        15080    15086       +6     
+ Misses       6720     6717       -3     
+ Partials      873      872       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zhaohuabing
add e2e test for rate limit on multiple listeners
0e87bd3 
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
@zhaohuabing zhaohuabing marked this pull request as ready for review April 4, 2024 03:14
@zhaohuabing zhaohuabing changed the title fix: rate limit doesn't work with two listeners fix: rate limit doesn't work with two(and more) listeners Apr 4, 2024
@arkodg
Copy link
Contributor

arkodg commented Apr 4, 2024

thanks for debugging and fixing this one !
can you also add a yaml test in https://github.com/envoyproxy/gateway/tree/main/internal/xds/translator/testdata/in/ratelimit-config, so its easier to spot this in the future

@zhaohuabing
Copy link
Member Author

zhaohuabing commented Apr 7, 2024 •
edited
Loading

thanks for debugging and fixing this one ! can you also add a yaml test in https://github.com/envoyproxy/gateway/tree/main/internal/xds/translator/testdata/in/ratelimit-config, so its easier to spot this in the future

@zhaohuabing
address comments
a033009 
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
arkodg
arkodg approved these changes Apr 8, 2024
View reviewed changes
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

@Xunzhuo Xunzhuo merged commit a5bedbc into envoyproxy:main Apr 8, 2024
5 of 6 checks passed
@arkodg
Copy link
Contributor

arkodg commented Apr 8, 2024

hitting codecov/codecov-action#1359

arkodg pushed a commit to arkodg/gateway that referenced this pull request Apr 8, 2024
@zhaohuabing @Xunzhuo @arkodg
fix: rate limit doesn't work with two(and more) listeners (envoyproxy…
b237248 
…#3085)

* fix: rate limit doesn't work with two listeners

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* add e2e test for rate limit on multiple listeners

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* address comments

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

---------

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Co-authored-by: Xunzhuo <bitliu@tencent.com>
(cherry picked from commit a5bedbc)
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Xunzhuo added a commit that referenced this pull request Apr 8, 2024
@arkodg @zhaohuabing
@zetaab @zirain @lemonlinger @cnvergence @liorokman @vixns @Xunzhuo
[release/v1.0] Cherry-pick fixes into v1.0 (#3127)
5a15902 
* Run certgen when upgrading (#2934)

run certgen when upgrading

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
(cherry picked from commit 62ecf15)
Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* Fix: nil secret in resourceversiontable (#2982)

* fix nil secret in resourceversiontable

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* check secrets in the xds result

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

---------

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
(cherry picked from commit e880439)
Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* fix: add missing http filters to the http filter chain (#2970)

* fix: add missing http filters to the http filter chain

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* refactor

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* fix lint

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* add comments

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* remove refactor

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* remove refactor

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* fix gen

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* fix lint

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

---------

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
(cherry picked from commit f699edf)
Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* fix: allow websockets in url rewrite (#3022)

allow websockets in url rewrite

Signed-off-by: Jesse Haka <haka.jesse@gmail.com>
Co-authored-by: zirain <zirain2009@gmail.com>
(cherry picked from commit 3d51933)
Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* Set host for http health checker explicitly to avoid using the cluster name as host header for http health checking request. (#3057)

* Set host for http health checker explictly to avoid using the cluster name as host header for http health checking request

Signed-off-by: lemonlinger <lemonlinger@gmail.com>

* fix broken tests

Signed-off-by: lemonlinger <lemonlinger@gmail.com>

* fix health-check test case in xds translation

Signed-off-by: lemonlinger <lemonlinger@gmail.com>

* Simplify code and concise comments

Signed-off-by: lemonlinger <lemonlinger@gmail.com>

---------

Signed-off-by: lemonlinger <lemonlinger@gmail.com>
(cherry picked from commit 8f450a9)
Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* fix: do not create infra resources when missing translated listeners (#3043)

* fix: do not create infra resources when missing translated listeners

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

* remove empty line

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

* skip infra creation on empty listeners and log it

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

---------

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>
(cherry picked from commit 36d7141)
Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* Fix: double slashes in redirect URL (#2998)

* fix: double trailing splashs in redirect URL

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* add e2e tests

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* fix lint

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* fix test

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* fix test

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* fix test

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* fix test

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* add e2e tests

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* fix test

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* revert

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* use regex rewrite to generate the redirect url

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* use regex rewrite to generate the redirect url

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* use regex rewrite to generate the redirect url

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* remove comments

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* extract method

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* address comments

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

---------

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
(cherry picked from commit ceb697f)
Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* fix: Allow Policy to attach to multiple http listeners  (#2967)

* Fixing the clienttrafficpolicy validation.

Signed-off-by: Lior Okman <lior.okman@sap.com>

* Make SecurityPolicy validate correctly.

Signed-off-by: Lior Okman <lior.okman@sap.com>

* Reverted the SecurityPolicy validation - handled differently via
another feature.

Signed-off-by: Lior Okman <lior.okman@sap.com>

* Updated the tests to reflect that this validation isn't required for SecurityPolicy

Signed-off-by: Lior Okman <lior.okman@sap.com>

* Added some comments to explain the validation being performed.

Signed-off-by: Lior Okman <lior.okman@sap.com>

* Updated the error message as requested in the review.

Signed-off-by: Lior Okman <lior.okman@sap.com>

---------

Signed-off-by: Lior Okman <lior.okman@sap.com>
(cherry picked from commit f9409e4)
Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* fix: set path prefix for http ext auth service (#3018)

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
(cherry picked from commit 2882b7c)
Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* Change route sorting order to Exact > RegularExpression > PathPrefix (#2579)

* Change route sorting order to Exact > RegularExpression > PathPrefix

kubernetes-sigs/gateway-api#1770
kubernetes-sigs/gateway-api#1855

Signed-off-by: Stéphane Cottin <stephane.cottin@vixns.com>
(cherry picked from commit 11f56fd)
Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* fix: infraIR duplicate port translation for merged gateways (#3061)

* fix: duplicate port translation for merged gateways

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

* refactor to map

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

* rename map

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

* add seperate testcase

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

---------

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>
(cherry picked from commit 29946b0)
Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* translator: set SpawnUpstreamSpan to true (#3102)

* translator: set SpawnUpstreamSpan to true

Signed-off-by: zirain <zirain2009@gmail.com>

* update

Signed-off-by: zirain <zirain2009@gmail.com>

---------

Signed-off-by: zirain <zirain2009@gmail.com>
(cherry picked from commit 635ebfc)
Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* fix: rate limit doesn't work with two(and more) listeners (#3085)

* fix: rate limit doesn't work with two listeners

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* add e2e test for rate limit on multiple listeners

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

* address comments

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>

---------

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Co-authored-by: Xunzhuo <bitliu@tencent.com>
(cherry picked from commit a5bedbc)
Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* rerun make testdata

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

---------

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
Signed-off-by: Jesse Haka <haka.jesse@gmail.com>
Signed-off-by: lemonlinger <lemonlinger@gmail.com>
Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>
Signed-off-by: Lior Okman <lior.okman@sap.com>
Signed-off-by: Stéphane Cottin <stephane.cottin@vixns.com>
Signed-off-by: zirain <zirain2009@gmail.com>
Co-authored-by: Huabing Zhao <zhaohuabing@gmail.com>
Co-authored-by: Jesse Haka <haka.jesse@gmail.com>
Co-authored-by: zirain <zirain2009@gmail.com>
Co-authored-by: Meng <lemonlinger@gmail.com>
Co-authored-by: Karol Szwaj <karol.szwaj@gmail.com>
Co-authored-by: Lior Okman <lior.okman@sap.com>
Co-authored-by: vixns <stephane.cottin@vixns.com>
Co-authored-by: Xunzhuo <bitliu@tencent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xunzhuo Xunzhuo Xunzhuo approved these changes

@arkodg arkodg arkodg approved these changes

Assignees
No one assigned
Labels
cherrypick/release-v1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Single listener is considered for rate limiting
3 participants
@zhaohuabing @arkodg @Xunzhuo