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

cmd/contour: add EnableXRateLimitHeaders config file setting #3457

Merged
merged 1 commit into from
Mar 16, 2021

Conversation

skriss
Copy link
Member

@skriss skriss commented Mar 8, 2021

Adds the EnableXRateLimitHeaders field to the config file's
RateLimitService block. When set to true, adds the X-RateLimit
headers to responses that required checking the RLS.

Closes #3431.

Signed-off-by: Steve Kriss [email protected]

@skriss skriss added this to the 1.14.0 milestone Mar 8, 2021
@skriss skriss requested a review from a team as a code owner March 8, 2021 18:38
@skriss skriss requested review from danehans and youngnick and removed request for a team March 8, 2021 18:38
@skriss
Copy link
Member Author

skriss commented Mar 8, 2021

I assumed that folks agreed with #3431 (comment), but I'd still appreciate input one way or another on that. Easy enough to change things if we want to switch to a non-boolean config option.

@skriss skriss requested a review from stevesloka March 8, 2021 18:39
@codecov
Copy link

codecov bot commented Mar 8, 2021

Codecov Report

Merging #3457 (f6159f4) into main (7b18f01) will increase coverage by 0.05%.
The diff coverage is 64.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3457      +/-   ##
==========================================
+ Coverage   75.15%   75.21%   +0.05%     
==========================================
  Files          98       98              
  Lines        6586     6592       +6     
==========================================
+ Hits         4950     4958       +8     
+ Misses       1523     1521       -2     
  Partials      113      113              
Impacted Files Coverage Δ
cmd/contour/serve.go 0.00% <0.00%> (ø)
internal/envoy/v3/ratelimit.go 100.00% <100.00%> (ø)
internal/xdscache/v3/listener.go 93.36% <100.00%> (+0.03%) ⬆️
internal/k8s/log.go 69.56% <0.00%> (+6.52%) ⬆️

Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

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

LGTM assuming the boolean option is cool with everyone

@stevesloka stevesloka added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Mar 12, 2021
@skriss
Copy link
Member Author

skriss commented Mar 15, 2021

sounds like using the bool works for everyone, but I'll leave this open for one more day in case @youngnick or @xaleeks have further input.

Adds the EnableXRateLimitHeaders field to the config file's
RateLimitService block. When set to true, adds the X-RateLimit
headers to responses that required checking the RLS.

Closes projectcontour#3431.

Signed-off-by: Steve Kriss <[email protected]>
@skriss skriss merged commit 723081e into projectcontour:main Mar 16, 2021
@skriss skriss deleted the fix-3431 branch March 16, 2021 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

global rate limiting: allow configuring enable_x_ratelimit_headers
3 participants