api: add ClientTrafficPolicy option to emit Retry-After header on rate-limit 429 responses - #9770
devilleweppenaar wants to merge 16 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>
|
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 81.31% 81.32%
=======================================
Files 264 264
Lines 40973 40980 +7
=======================================
+ Hits 33319 33328 +9
+ Misses 7653 7651 -2
Partials 1 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>
|
can we build on top of #8742 instead of adding the setting in CTP |
@arkodg I believe we could. The main reason I went with the CTP option first is that I was copying the pattern of how |
|
@arkodg I took another look and recalled why what you suggested is not currently possible. Per-rule granularity is tracked separately in #9078, and it's blocked on upstream: envoyproxy/envoy#46289 added This PR is scoped to the filter-wide CTP option for that reason, mirroring |
|
can we evaluate the tradeoffs & correctness of setting this field by default instead of |
|
@arkodg I think there's a variant that gets us the consistency without losing expressiveness. Rather than reusing This addresses your point (no explicit opt-in needed, matches the X-RateLimit-* precedent) while keeping the two headers independently controllable, which matters because they're not equivalent standards-wise: Retry-After (RFC 9110 section 10.2.3) is a stable, general-purpose HTTP mechanism reused across 429/503/3xx with broad client support, while X-RateLimit-/RateLimit- (draft-ietf-httpapi-ratelimit-headers) is still a WG draft that discloses actual quota thresholds. The draft itself treats them as separate-but-coexisting signals with defined precedence when both appear, not one on/off switch. Bundling them under a single flag also makes "X-RateLimit-* off, Retry-After on" (a real use case: avoid disclosing exact limits, still let clients back off gracefully) inexpressible. I am happy to update the PR to reflect the default on |
|
hey @devilleweppenaar +1 to opt out approach using |
# Conflicts: # examples/extension-server/go.mod # examples/extension-server/go.sum # go.mod # go.sum # test/e2e/tests/ratelimit.go # test/go.mod # test/go.sum
Emit the Retry-After header by default on rate-limited 429 responses, opting out via ClientTrafficPolicy.spec.headers.disableRetryAfterHeader instead of opting in. This mirrors disableRateLimitHeaders and keeps "X-RateLimit-* off, Retry-After on" expressible, per review feedback on envoyproxy#9770. Signed-off-by: De Ville Weppenaar <11049609+devilleweppenaar@users.noreply.github.com>
Signed-off-by: De Ville Weppenaar <11049609+devilleweppenaar@users.noreply.github.com>
|
Hey @arkodg, I've made the proposed changes to this PR (disableRetryAfterHeader, opt-out with default-on) and updated the description. It's ready for another review when you have a moment. |
Per review feedback from @arkodg: rate-limited 429s now include Retry-After by default, which is a behavior change for existing users. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: De Ville Weppenaar <11049609+devilleweppenaar@users.noreply.github.com>
This branch has not been deployed
3 participants
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.disableRetryAfterHeader(bool, defaultfalse) to control it — the header is emitted by default, and can be opted out per listener, mirroring the existingdisableRateLimitHeadersfield end to end (API type -> IR -> both rate limit filter translators).Per discussion on this PR, the field was renamed from
enableRetryAfterHeader(opt-in, defaultfalse) todisableRetryAfterHeader(opt-out, defaultfalse/emitted) for consistency withdisableRateLimitHeaders, while keepingX-RateLimit-* off, Retry-After onindependently expressible.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/.Retry-Afteris emitted by default,disableRetryAfterHeaderopts out.