fix(rate-limiting) removes race condition(redis) for better accuracy #8227
Conversation
| if current_usage and reset then | ||
| local remaining = max(0, limit - current_usage) | ||
| local stop = current_usage > limit | ||
| local headers | ||
| if not conf.hide_client_headers then | ||
| headers = {} | ||
|
|
||
| headers[RATELIMIT_LIMIT] = limit | ||
| headers[RATELIMIT_REMAINING] = remaining | ||
| headers[RATELIMIT_RESET] = reset | ||
| end |
There was a problem hiding this comment.
I think this is still a fixed window of time. The window is the TTL on a key, which never moves. So when the TTL expires after the fixed amount of time, the counter resets.
There was a problem hiding this comment.
IMHO TTL is fixed but the window of time is not.
For example, if the time unit is 1 minute, fixed window counter guarantees an average rate in window [00:00, 00:01), [00:01,00:02), etc.
Suppose the limit is 10 per minute.
In fixed window if 10 requests arives at 00:00:59 and more 10 requests arives at 00:01:01 we have 20 alowed requests in the interval of 3 seconds.
With redis TTL it is not possible. At the same cenario only the first 10 requests are allowed. The last 10 recives 429 status code. The window limit slides between time window [00:00, 00:01) and [00:01,00:02)
But I'm open to suggestions
There was a problem hiding this comment.
I think you would have to keep timestamps of each request in a sorted set.
and on each request drop the timestamps between -inf and (now - window_time), add now. the count of items in the set is how many requests have been made in the window.
if the number is greater than the configured value, the limit has been reached
This would make the window sliding over the points in time as the requests are made.
mikefero
added
the
version-compatibility
giovanibrioni
force-pushed
the
fix/rate-limiting-accuracy-redis
branch
from
f304d5b
to
66fbaba
Compare
|
We have no plans to add a sliding window to this plugin. We can accept the improvements to Redis command used. Are you still up for adding support for that? |
What's the reasoning here? |
|
Sliding window is an enterprise-only feature at this point. |
Can we implement an algorithm that isn't a sliding window? |
Could you elaborate a bit? |
giovanibrioni
force-pushed
the
fix/rate-limiting-accuracy-redis
branch
from
ef88517
to
ba4cb93
Compare
giovanibrioni
force-pushed
the
fix/rate-limiting-accuracy-redis
branch
from
6e191c5
to
d4ed891
Compare
|
@hbagdi, |
hbagdi
added
the
author/community
|
Closing in favor of #10559 |
it is same fix made in PR #8227, but I remove sliding-window feature. Kong have no plans to add a sliding window to this plugin, Sliding window is an enterprise-only feature at this point
…g accuracy (#10559) it is same fix made in PR #8227, but with the sliding-window feature removed. Sliding window is an enterprise-only feature at this point Co-authored-by: Chrono <[email protected]> Co-authored-by: Qi <[email protected]>
Summary
fix rate-limiting accuracy reported on issue: #5311
There is a low impact change to the current code and this implementation has better accuracy on redis policy
Full changelog
[it removes the race condition problem getting keys on redis. it uses the return of the increment command as the usage counter ensuring atomicity, different from the current one, which allows multiple requests to get the same value of the redis key while the counter increment is done asynchronously]
[it uses sliding window algorithm which is the most suitable for better accuracy]
[it works only with redis polity because redis is capable of guaranteeing atomicity and performance, which other polices could not guarantee]
[it maintains the current functioning and configuration of the plugin. the fixed window setting is the default to not impact routes already configured]
[it has an even better performance than the current plugin using the redis policy and fixed window, as it does only 1 operation per request in redis (eval) instead of the two operations per request that the current one (get and eval)]
Issues resolved
Fix #5311
KAG-981