api: add ClientTrafficPolicy option to emit Retry-After header on rate-limit 429 responses - #9770
Open
devilleweppenaar wants to merge 12 commits into
Open
api: add ClientTrafficPolicy option to emit Retry-After header on rate-limit 429 responses#9770devilleweppenaar wants to merge 12 commits into
devilleweppenaar wants to merge 12 commits into
Conversation
Bumps github.com/envoyproxy/go-control-plane/envoy to a commit built after envoyproxy/envoy#46289 merged, which added enable_retry_after_header to the global and local rate limit HTTP filter protos. Needed before those fields can be referenced from Go. extensions.gen.go picks up a few newly available extension packages as an incidental side effect of the bump; unrelated to this change. Signed-off-by: De Ville Weppenaar <11049609+devilleweppenaar@users.noreply.github.com>
Adds ClientTrafficPolicy.spec.headers.enableRetryAfterHeader, mirroring the existing disableRateLimitHeaders field. Defaults to false. Fixes envoyproxy#9769 Signed-off-by: De Ville Weppenaar <11049609+devilleweppenaar@users.noreply.github.com>
Threads ClientTrafficPolicy.spec.headers.enableRetryAfterHeader through the IR and sets enable_retry_after_header on the generated global and local rate limit HTTP filter configs, mirroring how disableRateLimitHeaders is wired to enable_x_ratelimit_headers. Signed-off-by: De Ville Weppenaar <11049609+devilleweppenaar@users.noreply.github.com>
Adds a ClientTrafficPolicy golden-file case exercising the new field through the CRD-to-IR translation, and two xds-ir golden-file cases covering the global and local rate limit filters. Signed-off-by: De Ville Weppenaar <11049609+devilleweppenaar@users.noreply.github.com>
Signed-off-by: De Ville Weppenaar <11049609+devilleweppenaar@users.noreply.github.com>
Follow-up to the enableRetryAfterHeader CRD field: make helm-template picks up the new field in the rendered gateway-crds-helm test fixtures. Signed-off-by: De Ville Weppenaar <11049609+devilleweppenaar@users.noreply.github.com>
Companion to the root module bump so 'make gen-check' stays clean. Signed-off-by: De Ville Weppenaar <11049609+devilleweppenaar@users.noreply.github.com>
Companion to the root module bump so the test module's go.mod stays tidy and 'make lint' can load it. Signed-off-by: De Ville Weppenaar <11049609+devilleweppenaar@users.noreply.github.com>
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29a11ce516
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Match the disableRateLimitHeaders convention instead of restating the default, which the boolean type and the generated docs table already convey. Signed-off-by: De Ville Weppenaar <11049609+devilleweppenaar@users.noreply.github.com>
Adds conformance coverage for enableRetryAfterHeader against a real Envoy, for both the global and local rate limit filters, addressing review feedback that the existing coverage stopped at IR/xDS golden serialization and never exercised runtime behavior. Mirrors RateLimitHeadersDisabled's structure (ratelimit.go) and the eventual-consistency retry idiom used elsewhere in local_ratelimit.go. Retry-After's value is asserted with a small tolerance rather than an exact match, since it counts down from the window size. Signed-off-by: De Ville Weppenaar <11049609+devilleweppenaar@users.noreply.github.com>
The API doc comment said "when rate limiting is enabled", copied from disableRateLimitHeaders, but that's inaccurate here: X-RateLimit-* headers are considered on every response the filter evaluates, while Retry-After is scoped to rate-limited 429 responses only, per Envoy's own field doc. Every other place we described this option (IR, translator, e2e tests, release note) already said "rate-limited 429 responses" -- bring the public API doc in line with that instead of the other way around. Also trims a redundant "if explicitly enabled" from the translator comment. Signed-off-by: De Ville Weppenaar <11049609+devilleweppenaar@users.noreply.github.com>
Author
|
I did a review of each edited file, including generated ones. I also tested the changes locally using a kind cluster with all of the latest changes to ensure that the intended behavior:
This was running the built code from the PR branch ( |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9770 +/- ##
=======================================
Coverage 76.30% 76.30%
=======================================
Files 261 261
Lines 44351 44356 +5
=======================================
+ Hits 33842 33847 +5
- Misses 8271 8272 +1
+ Partials 2238 2237 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Resolve go.mod/go.sum conflicts in examples/extension-server and test modules by taking main's dependency versions and re-running 'go mod tidy', which resolves consistently with the go-control-plane bump via each module's local replace directive. Signed-off-by: De Ville Weppenaar <11049609+devilleweppenaar@users.noreply.github.com>
Contributor
|
can we build on top of #8742 instead of adding the setting in CTP |
Author
@arkodg I believe we could. The main reason I went with the CTP option first is that I was copying the pattern of how |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Envoy's global and local rate limit HTTP filters now support
enable_retry_after_header(envoyproxy/envoy#46289), emitting aRetry-Afterheader on rate-limited 429 responses. This addsClientTrafficPolicy.spec.headers.enableRetryAfterHeader(bool, defaultfalse) to expose it, mirroring the existingdisableRateLimitHeadersfield end to end (API type -> IR -> both rate limit filter translators).Requires bumping
go-control-planeto a commit that includes the new field; the three companiongo.mod/go.sumcommits (root,examples/extension-server,test) are that bump propagating to each module in the repo.Per-rule granularity (mirroring
xRateLimitHeaders, #8279) is tracked separately in #9078 and needs further upstream Envoy work; out of scope here.Which issue(s) this PR fixes:
Fixes #9769
PR Checklist
git commit -s).disableRateLimitHeaders, Feature Request: Granular Control for X-RateLimit Headers in BackendTrafficPolicy #8279); open to adjusting on review.make generate gen-check,make lint, andgo test ./...pass locally.make generate); no additional user-facing docs page exists for the siblingdisableRateLimitHeadersfield either.release-notes/current/new_features/.false.