From cf58cb8fd279cbb38681e308e8c33eaa2f380bb7 Mon Sep 17 00:00:00 2001
From: samugi <samuele@konghq.com>
Date: Thu, 26 Oct 2023 19:18:29 +0200
Subject: [PATCH] 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: https://github.com/Kong/kong/pull/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`.
---
 .../kong/rate-limiting-fix-redis-sync-rate.yml  |  3 +++
 kong/plugins/rate-limiting/policies/init.lua    | 11 ++++-------
 .../23-rate-limiting/02-policies_spec.lua       | 17 ++++++++++++++---
 3 files changed, 21 insertions(+), 10 deletions(-)
 create mode 100644 changelog/unreleased/kong/rate-limiting-fix-redis-sync-rate.yml

diff --git a/changelog/unreleased/kong/rate-limiting-fix-redis-sync-rate.yml b/changelog/unreleased/kong/rate-limiting-fix-redis-sync-rate.yml
new file mode 100644
index 000000000000..959e7263dc6b
--- /dev/null
+++ b/changelog/unreleased/kong/rate-limiting-fix-redis-sync-rate.yml
@@ -0,0 +1,3 @@
+message: "**Rate Limiting**: fix to provide better accuracy in counters when sync_rate is used with the redis policy."
+type: bugfix
+scope: Plugin
diff --git a/kong/plugins/rate-limiting/policies/init.lua b/kong/plugins/rate-limiting/policies/init.lua
index 12f9f32983e8..f20a2ea5b4d4 100644
--- a/kong/plugins/rate-limiting/policies/init.lua
+++ b/kong/plugins/rate-limiting/policies/init.lua
@@ -206,14 +206,9 @@ local function update_local_counters(conf, periods, limits, identifier, value)
     if limits[period] then
       local cache_key = get_local_key(conf, identifier, period, period_date)
 
-      if cur_delta[cache_key] then
-        cur_delta[cache_key] = cur_delta[cache_key] + value
-      else
-        cur_delta[cache_key] = value
-      end
+      cur_delta[cache_key] = (cur_delta[cache_key] or 0) + value
     end
   end
-  
 end
 
 return {
@@ -346,7 +341,9 @@ return {
       if conf.sync_rate ~= SYNC_RATE_REALTIME then
         cur_usage[cache_key] = current_metric or 0
         cur_usage_expire_at[cache_key] = periods[period] + EXPIRATION[period]
-        cur_delta[cache_key] = 0
+        -- The key was just read from Redis using `incr`, which incremented it
+        -- by 1. Adjust the value to account for the prior increment.
+        cur_delta[cache_key] = -1
       end
 
       return current_metric or 0
diff --git a/spec/03-plugins/23-rate-limiting/02-policies_spec.lua b/spec/03-plugins/23-rate-limiting/02-policies_spec.lua
index 7ce052080e18..29474c399410 100644
--- a/spec/03-plugins/23-rate-limiting/02-policies_spec.lua
+++ b/spec/03-plugins/23-rate-limiting/02-policies_spec.lua
@@ -217,9 +217,20 @@ describe("Plugin: rate-limiting (policies)", function()
           assert.equal(0, metric)
 
           for i = 1, 3 do
-            assert(policies.redis.increment(conf, { [period] = 2 }, identifier, current_timestamp, 1))
-            metric = assert(policies.redis.usage(conf, identifier, period, current_timestamp))
-            assert.equal(i, metric)
+            -- "second" keys expire too soon to check the async increment.
+            -- Let's verify all the other scenarios:
+            if not (period == "second" and sync_rate ~= SYNC_RATE_REALTIME) then
+              assert(policies.redis.increment(conf, { [period] = 2 }, identifier, current_timestamp, 1))
+
+              -- give time to the async increment to happen
+              if sync_rate ~= SYNC_RATE_REALTIME then
+                local sleep_time = 1 + (sync_rate > 0 and sync_rate or 0)
+                ngx.sleep(sleep_time)
+              end
+
+              metric = assert(policies.redis.usage(conf, identifier, period, current_timestamp))
+              assert.equal(i, metric)
+            end
           end
         end
       end)