fix(plugin/rate-limiting): use redis:eval() to improve rate-limiting accuracy
#10559
Conversation
There was a problem hiding this comment.
Thank you for your contribution. Can you please have a look at my comments? It would be nice to have a test that exposes the failing behavior in the previous implementation and demonstrates that the fix works. If that is too difficult to write, can you please point out how the existing tests exercise the functionality that you've changed? Thank you again, -Hans
hanshuebner
changed the title
hanshuebner
added
the
pending author feedback
hbagdi
added
the
author/community
github-actions
bot
added
changelog
chore
giovanibrioni
force-pushed
the
fix/rate-limiting-accuracy-redis-without-sliding-window
branch
from
a84a7fb
to
5e29cdf
Compare
github-actions
bot
removed
changelog
core/tracing
chore
@hanshuebner , |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
i've been trying to solve this problem for over 1 year. I already use this fix in my applications and it has been battle tested receiving more than 4 billion transactions per day in a productive environment. I'd like to help the rest of the community and I've made all the adjustments requested, but this slowness is annoying me. if the bot closes my PR I won't open another one. |
| @@ -142,4 +146,41 @@ describe("Plugin: rate-limiting (policies)", function() | |||
| end) | |||
| end | |||
|
|
|||
| describe("redis", function() | |||
oowl
force-pushed
the
fix/rate-limiting-accuracy-redis-without-sliding-window
branch
from
18cf550
to
867edbe
Compare
it is same fix made in PR Kong#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
Co-authored-by: Chrono <[email protected]>
ADD-SP
force-pushed
the
fix/rate-limiting-accuracy-redis-without-sliding-window
branch
from
867edbe
to
9eb755e
Compare
When the periodic sync to redis feature is turned on, using the `sync_rate` configuration option, keys are incremented by steps of 2 instead of 1 for requests that arrive after the `sync_rate` interval has expired. This happens because after each sync, the key is loaded again from redis and also incremented atomically (see: #10559) however the next call to `increment` also adds 1 to its value, so the key is incremented by 2 every time it's loaded from redis. This fix sets a negative delta for the key when `conf.sync_rate ~= SYNC_RATE_REALTIME` and the key was loaded from redis in order to invalidate the next call to `increment`.
…11859) * fix(rate-limiting): redis async updates When the periodic sync to redis feature is turned on, using the `sync_rate` configuration option, keys are incremented by steps of 2 instead of 1 for requests that arrive after the `sync_rate` interval has expired. This happens because after each sync, the key is loaded again from redis and also incremented atomically (see: #10559) however the next call to `increment` also adds 1 to its value, so the key is incremented by 2 every time it's loaded from redis. This fix sets a negative delta for the key when `conf.sync_rate ~= SYNC_RATE_REALTIME` and the key was loaded from redis in order to invalidate the next call to `increment`. Includes a small code refactor
…11859) * fix(rate-limiting): redis async updates When the periodic sync to redis feature is turned on, using the `sync_rate` configuration option, keys are incremented by steps of 2 instead of 1 for requests that arrive after the `sync_rate` interval has expired. This happens because after each sync, the key is loaded again from redis and also incremented atomically (see: #10559) however the next call to `increment` also adds 1 to its value, so the key is incremented by 2 every time it's loaded from redis. This fix sets a negative delta for the key when `conf.sync_rate ~= SYNC_RATE_REALTIME` and the key was loaded from redis in order to invalidate the next call to `increment`. Includes a small code refactor
…11859) * fix(rate-limiting): redis async updates When the periodic sync to redis feature is turned on, using the `sync_rate` configuration option, keys are incremented by steps of 2 instead of 1 for requests that arrive after the `sync_rate` interval has expired. This happens because after each sync, the key is loaded again from redis and also incremented atomically (see: #10559) however the next call to `increment` also adds 1 to its value, so the key is incremented by 2 every time it's loaded from redis. This fix sets a negative delta for the key when `conf.sync_rate ~= SYNC_RATE_REALTIME` and the key was loaded from redis in order to invalidate the next call to `increment`. Includes a small code refactor
…11859) * fix(rate-limiting): redis async updates When the periodic sync to redis feature is turned on, using the `sync_rate` configuration option, keys are incremented by steps of 2 instead of 1 for requests that arrive after the `sync_rate` interval has expired. This happens because after each sync, the key is loaded again from redis and also incremented atomically (see: #10559) however the next call to `increment` also adds 1 to its value, so the key is incremented by 2 every time it's loaded from redis. This fix sets a negative delta for the key when `conf.sync_rate ~= SYNC_RATE_REALTIME` and the key was loaded from redis in order to invalidate the next call to `increment`. Includes a small code refactor
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
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
Checklist
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 has an even better performance than the current plugin using the redis policy, as it does only 1 operation per request in redis (eval) instead of the two operations per request that the current one (get and incrby)]
Issue reference
Fix #5311
KAG-981