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.

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

feat(translator): support disabling X-RateLimit headers #3397

Merged
merged 11 commits into from
May 30, 2024

Conversation

evacchi
Copy link
Contributor

@evacchi evacchi commented May 15, 2024

Signed-off-by: Edoardo Vacchi evacchi@users.noreply.github.com

What this PR does / why we need it:

This PR exposes EnableXRatelimitHeaders as a Gateway flag. For backwards compatibility, it flips the semantics (Disable vs Enable) as to preserve the current default semantics, i.e. always emit.

@evacchi evacchi requested a review from a team as a code owner May 15, 2024 14:34
@evacchi evacchi requested a review from zhaohuabing May 15, 2024 20:23
@arkodg
Copy link
Contributor

arkodg commented May 15, 2024

hey @evacchi raised #3400 to track this, will bring this up in the community meeting tomorrow and see what others think is the best home for this knob

@zirain
Copy link
Contributor

zirain commented May 16, 2024

for my personal, API > Annotaions > Env flags.

@evacchi evacchi force-pushed the disable-ratelimit-headers branch from 33a2f87 to f9e4d4e Compare May 28, 2024 08:42
Comment on lines +18 to +21
typedConfig:
'@type': type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit
domain: first-listener
rateLimitService:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

compare this to:

- name: envoy.filters.http.ratelimit
typedConfig:
'@type': type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit
domain: first-listener
enableXRatelimitHeaders: DRAFT_VERSION_03
rateLimitService:
grpcService:
envoyGrpc:
clusterName: ratelimit_cluster
transportApiVersion: V3

the config headers.disableRateLimitHeaders: true causes the line

 enableXRatelimitHeaders: DRAFT_VERSION_03

to be omitted, because OFF is the zero value and marshalling is configured as omitempty

@evacchi evacchi requested a review from zhaohuabing May 28, 2024 11:18
@arkodg
Copy link
Contributor

arkodg commented May 28, 2024

hey @evacchi thanks for updating the PR with the API change, added some minor comments
would be good to also add 1 test case within the gateway-api lib to avoid regressions
https://github.com/envoyproxy/gateway/tree/main/internal/gatewayapi/testdata

@arkodg arkodg added this to the v1.1.0-rc1 milestone May 28, 2024
Copy link

codecov bot commented May 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.81%. Comparing base (76c08e6) to head (e334073).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3397      +/-   ##
==========================================
+ Coverage   67.77%   67.81%   +0.03%     
==========================================
  Files         166      166              
  Lines       20037    20041       +4     
==========================================
+ Hits        13581    13591      +10     
+ Misses       5466     5461       -5     
+ Partials      990      989       -1     

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

@evacchi evacchi force-pushed the disable-ratelimit-headers branch from 8423b3c to 5d23885 Compare May 29, 2024 08:06
arkodg
arkodg previously approved these changes May 29, 2024
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 May 29, 2024 18:10
Copy link
Contributor

@guydc guydc left a comment

Choose a reason for hiding this comment

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

Should we add an E2E to the RL suite for this case?

internal/ir/xds.go Outdated Show resolved Hide resolved
internal/ir/xds.go Show resolved Hide resolved
evacchi added 6 commits May 30, 2024 09:33
Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
evacchi added 2 commits May 30, 2024 11:53
Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
@evacchi
Copy link
Contributor Author

evacchi commented May 30, 2024

Attempting to add an e2e test, I think it should be correct but I do appreciate another set of eyes (also I am having some trouble with my local dev setup, so I'll have to figure that out...)

Copy link
Contributor

@guydc guydc left a comment

Choose a reason for hiding this comment

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

LGTM

@arkodg arkodg merged commit bcaa1b0 into envoyproxy:main May 30, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants