envoy icon indicating copy to clipboard operation
envoy copied to clipboard

Support multiples of units for rate limits

Open harpunius opened this issue 10 months ago • 4 comments

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 https://github.com/envoyproxy/ratelimit/issues/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 avatar Apr 02 '24 21:04 harpunius

cc @esmet @mattklein123

soulxu avatar Apr 02 '24 23:04 soulxu

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

harpunius avatar Apr 09 '24 06:04 harpunius

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[bot] avatar May 09 '24 08:05 github-actions[bot]

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? https://github.com/envoyproxy/envoy/blob/f580ed067db0f2a48085aa33e45a3a9cf5e36bfe/source/extensions/filters/http/ratelimit/ratelimit_headers.cc#L32

harpunius avatar May 13 '24 17:05 harpunius

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[bot] avatar Jun 12 '24 20:06 github-actions[bot]

Not stale, waiting for input.

harpunius avatar Jun 13 '24 09:06 harpunius

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[bot] avatar Jul 13 '24 12:07 github-actions[bot]

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[bot] avatar Jul 20 '24 16:07 github-actions[bot]

Waiting for this feature

ashish-quillbot avatar Jul 21 '24 09:07 ashish-quillbot

Waiting for it too

dnbn avatar Jul 21 '24 09:07 dnbn

in need of this ! help wanted

Martin-happy avatar Aug 14 '24 06:08 Martin-happy

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

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

harpunius avatar Aug 29 '24 08:08 harpunius