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

Support multiples of units for rate limits #33277

Open
harpunius opened this issue Apr 2, 2024 · 13 comments
Open

Support multiples of units for rate limits #33277

harpunius opened this issue Apr 2, 2024 · 13 comments
Labels
enhancement Feature requests. Not bugs or questions. no stalebot Disables stalebot from closing an issue

Comments

@harpunius
Copy link

harpunius commented Apr 2, 2024

Title: Support multiples of units for rate limits

Description:

We want to be able to specify rate limit intervals more flexibly than per-minute or per-second, for example per 30 seconds or per 10 minutes. Concretely, we have some bursts over 2-3 minutes that we want to restrict. We can only do this by rate limiting per-minute or per-hour, the former not reflecting continuous load and the latter not re-allowing traffic until a full hour has passed.

This is also described in envoyproxy/ratelimit#190 and I planned to contribute this feature for envoyproxy/ratelimit but got stuck at the centralised protobuf definition, hence this issue.

My proposed solution would simply be to extend RateLimit with a new field:

uint32 unitMultiplier = 4;

Relevant Links:
The relevant call (for redis) is https://github.com/envoyproxy/ratelimit/blob/main/src/redis/fixed_cache_impl.go#L155. The ratelimit service iterates over a list of internal limits []*config.RateLimit objects, that each contain a Limit struct (https://github.com/envoyproxy/ratelimit/blob/3654bfd73dc728debfc280b2097664f595036197/src/config/config.go#L22), pointing to the generated pb.go file (https://github.com/envoyproxy/go-control-plane/blob/main/envoy/service/ratelimit/v3/rls.pb.go#L357-L367).

@harpunius harpunius added enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Apr 2, 2024
@soulxu
Copy link
Member

soulxu commented Apr 2, 2024

cc @esmet @mattklein123

@soulxu soulxu removed the triage Issue requires triage label Apr 2, 2024
@harpunius
Copy link
Author

I added my implementation built on the proposed Limit.UnitMultiplier field in envoyproxy/ratelimit#552. Can you please have a look? ping: @esmet @mattklein123

Copy link

github-actions bot commented May 9, 2024

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label May 9, 2024
@harpunius
Copy link
Author

Hey @mattklein123 @esmet, this is still relevant. I had a brief look into the c++ ratelimit headers. I guess we would need to extend those as well?

const uint32_t window = convertRateLimitUnit(status.current_limit().unit());

@github-actions github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label May 13, 2024
Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 12, 2024
@harpunius
Copy link
Author

harpunius commented Jun 13, 2024

Not stale, waiting for input.

@github-actions github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Jun 13, 2024
Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 13, 2024
Copy link

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 20, 2024
@ashish-quillbot
Copy link

Waiting for this feature

@dnbn
Copy link

dnbn commented Jul 21, 2024

Waiting for it too

@Martin-happy
Copy link

Martin-happy commented Aug 14, 2024

in need of this !
help wanted

@harpunius
Copy link
Author

Not stale. Ping @esmet @mattklein123, can you please have a look?

@soulxu can you please mark this as not stale again? Thanks!

@soulxu soulxu reopened this Aug 30, 2024
@soulxu soulxu added no stalebot Disables stalebot from closing an issue and removed stale stalebot believes this issue/PR has not been touched recently labels Aug 30, 2024
@ramaraochavali
Copy link
Contributor

@harpunius Can you please make the necessary API change and propose a PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests. Not bugs or questions. no stalebot Disables stalebot from closing an issue
Projects
None yet
Development

No branches or pull requests

6 participants