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

Proposal: add support for x-ratelimit-* headers #12356

Closed
Pchelolo opened this issue Jul 29, 2020 · 2 comments · Fixed by #12410
Closed

Proposal: add support for x-ratelimit-* headers #12356

Pchelolo opened this issue Jul 29, 2020 · 2 comments · Fixed by #12410

Comments

@Pchelolo
Copy link
Contributor

Rational

For the client to be able to actively throttle their requests they need to know their current status with regards to rate limits. A draft RFC exists specifying RateLimit-Limit, RateLimit-Remaining, RateLimit-Reset headers, which seem to have fair adoption.

Prior art

A few pull requests were created in the ratelimit service envoyproxy/ratelimit#77, taking over envoyproxy/ratelimit#56 but they were closed due to issues with a general approach. According to @mattklein123, the better approach compared to the one taken in the pull request would be to change the ratelimit API to provide the necessary data, while setting the response headers in envoy.

Proposal

  1. Drop response_headers_to_add and request_headers_to_add from the RateLimitResponse proto in v4, deprecate the fields in v3 protocol per Add X-RateLimit-* response headers as an opt-in feature ratelimit#77 (comment)
  2. Add a field to the RateLimitResponse:DescriptorStatus:reset supporting the X-RateLimit-Reset header and implement providing the data in rate limiter service.
  3. Implement support for RateLimit-Limit, RateLimit-Remaining, RateLimit-Reset in envoy ratelimit filter under a filter configuration parameter.

I'm going to contribute all the code/testing if the proposal is approved by maintainers.
cc @junr03

@mattklein123
Copy link
Member

This proposal SGTM. The only thing I would recommend is not deprecating the request/response headers to add fields, as they may have other uses. Since they are already implemented I don't see any reason to remove them. We can add this behavior additionally. Thank you!

@Pchelolo
Copy link
Contributor Author

The only thing I would recommend is not deprecating the request/response headers to add fields, as they may have other uses. Since they are already implemented I don't see any reason to remove them. We can add this behavior additionally.

Great, thank you. This makes the job even easier :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants