ratelimit: support per-descriptor hits_addend #37361
Conversation
Signed-off-by: Rohit Agrawal <[email protected]>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
|
||
| // A per-descriptor override for the number of hits added to the matched limit by a request. | ||
| // This value takes precedence over the globally configured value. If neither this field nor | ||
| // the global value is set, the matched limit is incremented by 1 for each request. | ||
| // | ||
| // Additionally, this value can be overridden by setting the filter state value | ||
| // `envoy.ratelimit.hits_addend` to the desired number. If an invalid number (e.g., < 0) is | ||
| // provided, it will be ignored. | ||
| uint32 hits_addend = 3; |
There was a problem hiding this comment.
we shouldn't change the v2 proto API.
|
|
||
| // A per-descriptor override for the number of hits added to the matched limit by a request. | ||
| // This value takes precedence over the globally configured value. If neither this field nor | ||
| // the global value is set, the matched limit is incremented by 1 for each request. | ||
| // | ||
| // Additionally, this value can be overridden by setting the filter state value | ||
| // `envoy.ratelimit.hits_addend` to the desired number. If an invalid number (e.g., < 0) is | ||
| // provided, it will be ignored. | ||
| uint32 hits_addend = 3; |
There was a problem hiding this comment.
This should be part of RateLimitDescriptor, rather than part of single Entry.
| } | ||
| template <typename H> | ||
| friend H AbslHashValue(H h, // NOLINT(readability-identifier-naming) | ||
| const DescriptorEntry& entry) { | ||
| return H::combine(std::move(h), entry.key_, entry.value_); | ||
| return H::combine(std::move(h), entry.key_, entry.value_, entry.hits_addend_); |
There was a problem hiding this comment.
I think hits_addend shouldn't be used to compute the hash or be used to compare the entry.
|
Thanks for the contribution. Before going to deeper review/work, I think it would be better to let other maintainer check the requirement first to see if it's reasonable or not. |
Description
We can now provide multiple descriptor(s) in the same RateLimit request, but they all share same
hits_addend. If we want to limit the QPS and bandwidth at same time, we need to configure two rate limit filters in the same filter chain. It's a little complex and add additional overhead/latency.In this PR, we are exposing
hits_addendat the per-descriptor level which could be overridden to achieve the behavior described above.Fixes #37347
Commit Message: ratelimit: support per-descriptor hits_addend
Additional Description: This PR exposes
hits_addendat the per-descriptor level which could be overridden to achieve the behavior described above.Risk Level: Low
Testing: Added Unit Tests
Docs Changes: Added
Release Notes: Added