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: ratelimit not working with both headers and cidr matches #4377

Merged
merged 8 commits into from
Oct 21, 2024
Merged

fix: ratelimit not working with both headers and cidr matches #4377

merged 8 commits into from
Oct 21, 2024

Conversation

shawnh2
Copy link
Contributor

@shawnh2 shawnh2 commented Oct 1, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #4351

@shawnh2 shawnh2 requested a review from a team as a code owner October 1, 2024 04:22
@codecov Codecov
Copy link

codecov bot commented Oct 1, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.85%. Comparing base (26ec49a) to head (6c2b20f).
Report is 49 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4377      +/-   ##
==========================================
+ Coverage   65.83%   65.85%   +0.01%     
==========================================
  Files         203      203              
  Lines       30699    30700       +1     
==========================================
+ Hits        20212    20216       +4     
+ Misses       9335     9333       -2     
+ Partials     1152     1151       -1

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

arkodg
arkodg previously approved these changes Oct 18, 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 !

@arkodg arkodg requested review from a team October 18, 2024 02:08
zhaohuabing
zhaohuabing previously approved these changes Oct 18, 2024
View reviewed changes
Copy link
Member

@zhaohuabing zhaohuabing left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix! Could you also modify the headerMatch in the test and add an example in the comment to help future readers to better understand the code?

@shawnh2 shawnh2 dismissed stale reviews from zhaohuabing and arkodg via d7553b3 October 19, 2024 07:58
@shawnh2 shawnh2 requested review from arkodg, zhaohuabing and a team October 19, 2024 07:58
@shawnh2
Copy link
Contributor Author

shawnh2 commented Oct 20, 2024

/retest

@arkodg arkodg merged commit 66c0b51 into envoyproxy:main Oct 21, 2024
23 of 24 checks passed
@shawnh2 shawnh2 deleted the fix-rl branch October 22, 2024 00:26
@arkodg arkodg mentioned this pull request Oct 30, 2024
Merged
guydc pushed a commit to guydc/gateway that referenced this pull request Oct 30, 2024
@shawnh2 @zhaohuabing @guydc
fix: ratelimit not working with both headers and cidr matches (envoyp…
1b4c7d9 
…roxy#4377)

* fix ratelimit descriptors do not respect both headers and cidr match for one rule

Signed-off-by: shawnh2 <[email protected]>

* fix gen-check and lint

Signed-off-by: shawnh2 <[email protected]>

* fix ratelimit e2e test

Signed-off-by: shawnh2 <[email protected]>

* add more comment and update test case

Signed-off-by: shawnh2 <[email protected]>

---------

Signed-off-by: shawnh2 <[email protected]>
Co-authored-by: Huabing Zhao <[email protected]>
(cherry picked from commit 66c0b51)
Signed-off-by: Guy Daich <[email protected]>
guydc pushed a commit to guydc/gateway that referenced this pull request Oct 31, 2024
@shawnh2 @zhaohuabing @guydc
fix: ratelimit not working with both headers and cidr matches (envoyp…
828ab6c 
…roxy#4377)

* fix ratelimit descriptors do not respect both headers and cidr match for one rule

Signed-off-by: shawnh2 <[email protected]>

* fix gen-check and lint

Signed-off-by: shawnh2 <[email protected]>

* fix ratelimit e2e test

Signed-off-by: shawnh2 <[email protected]>

* add more comment and update test case

Signed-off-by: shawnh2 <[email protected]>

---------

Signed-off-by: shawnh2 <[email protected]>
Co-authored-by: Huabing Zhao <[email protected]>
(cherry picked from commit 66c0b51)
Signed-off-by: Guy Daich <[email protected]>
guydc added a commit that referenced this pull request Oct 31, 2024
@guydc @dependabot
@zhaohuabing @qicz @arkodg @zirain @oscarboher @haorenfsa @shawnh2
Release v1.1.3 cherry-pick (#4578)
13fee13 
* build(deps): bump github.com/docker/docker from 27.2.0+incompatible to 27.3.1+incompatible (#4303)

build(deps): bump github.com/docker/docker

Bumps [github.com/docker/docker](https://github.com/docker/docker) from 27.2.0+incompatible to 27.3.1+incompatible.
- [Release notes](https://github.com/docker/docker/releases)
- [Commits](moby/moby@v27.2.0...v27.3.1)

---
updated-dependencies:
- dependency-name: github.com/docker/docker
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Huabing Zhao <[email protected]>
(cherry picked from commit cd4e9b1)
Signed-off-by: Guy Daich <[email protected]>

* bugfix: EG loglevel error for admin and metrics module (#4340)

* bugfix: EG loglevel error for admin and metrics module

Signed-off-by: qicz <[email protected]>

* fix ut

Signed-off-by: qicz <[email protected]>

* polish

Signed-off-by: qicz <[email protected]>

---------

Signed-off-by: qicz <[email protected]>
(cherry picked from commit 9b161ee)
Signed-off-by: Guy Daich <[email protected]>

* fix: some status updates are discarded by the status updater (#4337)

Signed-off-by: Huabing Zhao <[email protected]>
(cherry picked from commit 14830c7)
Signed-off-by: Guy Daich <[email protected]>

* fix: Unsupported listener protocol type error for nil supportKinds assign gateway status. (#4345)

(cherry picked from commit 7babca9)
Signed-off-by: Guy Daich <[email protected]>

* set invalid Listener.SupportedKinds to empty list (#4352)

* set invalid Listener.SupportedKinds to empty list

Fixes: #4216

Relates to https://kubernetes.slack.com/archives/CR0H13KGA/p1727457195236889

Signed-off-by: Arko Dasgupta <[email protected]>

* lint

Signed-off-by: Arko Dasgupta <[email protected]>

---------

Signed-off-by: Arko Dasgupta <[email protected]>
(cherry picked from commit 2fdf069)
Signed-off-by: Guy Daich <[email protected]>

* bugfix: ignore some unnecessary requests to apiserver. (#4362)

Signed-off-by: qicz <[email protected]>
Co-authored-by: zirain <[email protected]>
(cherry picked from commit fe1e8bd)
Signed-off-by: Guy Daich <[email protected]>

* fix: rateLimitDeployment ignoring pod labels and annotation merge (#4228)

* fix labels and annotation merges for rate limit deployment

Signed-off-by: Oscar Boher <[email protected]>

* fix tests and label merge

Signed-off-by: Oscar Boher <[email protected]>

* fix annotation merge if prometheus was disabled and annotations were defined

Signed-off-by: Oscar Boher <[email protected]>

* renamed labels and annotations to specify they apply to pods only

Signed-off-by: Oscar Boher <[email protected]>

* linter

Signed-off-by: Oscar Boher <[email protected]>

* fix resource provider tests to new annotation behavior

Signed-off-by: Oscar Boher <[email protected]>

* go linter

Signed-off-by: Oscar Boher <[email protected]>

* fix gen-check

Signed-off-by: Oscar Boher <[email protected]>

* pod labels selector comment

Signed-off-by: Oscar Boher <[email protected]>

---------

Signed-off-by: Oscar Boher <[email protected]>
Co-authored-by: zirain <[email protected]>
Co-authored-by: Arko Dasgupta <[email protected]>
(cherry picked from commit cf84927)
Signed-off-by: Guy Daich <[email protected]>

* fix dashboard typos (#4422)

Signed-off-by: haorenfsa <[email protected]>
Signed-off-by: Guy Daich <[email protected]>

* fix: ratelimit not working with both headers and cidr matches  (#4377)

* fix ratelimit descriptors do not respect both headers and cidr match for one rule

Signed-off-by: shawnh2 <[email protected]>

* fix gen-check and lint

Signed-off-by: shawnh2 <[email protected]>

* fix ratelimit e2e test

Signed-off-by: shawnh2 <[email protected]>

* add more comment and update test case

Signed-off-by: shawnh2 <[email protected]>

---------

Signed-off-by: shawnh2 <[email protected]>
Co-authored-by: Huabing Zhao <[email protected]>
(cherry picked from commit 66c0b51)
Signed-off-by: Guy Daich <[email protected]>

---------

Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: Guy Daich <[email protected]>
Signed-off-by: qicz <[email protected]>
Signed-off-by: Huabing Zhao <[email protected]>
Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: Oscar Boher <[email protected]>
Signed-off-by: haorenfsa <[email protected]>
Signed-off-by: shawnh2 <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Huabing Zhao <[email protected]>
Co-authored-by: qi <[email protected]>
Co-authored-by: Arko Dasgupta <[email protected]>
Co-authored-by: zirain <[email protected]>
Co-authored-by: Oscar Boher <[email protected]>
Co-authored-by: shaoyue <[email protected]>
Co-authored-by: sh2 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arkodg arkodg arkodg approved these changes

@zhaohuabing zhaohuabing zhaohuabing approved these changes

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

Successfully merging this pull request may close these issues.

Ratelimiting doesn't work with both clientSelectors (i.e. headers and sourceCIDR) enabled
3 participants
@shawnh2 @zhaohuabing @arkodg