[Fix] ratelimit: Track all buckets in selector rules

When a rule defines several buckets (e.g. "200 / 1h" plus "30 / 1m")
and uses a selector, limit_to_prefixes keyed the prefixes table by the
selector value alone. Every bucket therefore mapped to the same Redis
key and only the last bucket in the array was ever tracked, so the
other limits were silently ignored.

Give each bucket a distinct key by prefixing the selector value with a
per-bucket id (burst + rate), mirroring the burst component that
gen_rate_key already prepends for the non-selector path. The
non-selector path is unchanged, so its existing Redis keys are kept.

Adds a functional regression test (two buckets, burst 2 + burst 20)
that fails before the fix because the restrictive bucket is ignored.

Closes #6059

diff --git a/src/plugins/lua/ratelimit.lua b/src/plugins/lua/ratelimit.lua
index 695ef3ddb049929b0e243037e1c9008d15af9bb5..f0a6c836ca490ac97ec271c3e52fb8b6deaeb8c9 100644 (file)
--- a/src/plugins/lua/ratelimit.lua
+++ b/src/plugins/lua/ratelimit.lua
@@ -280,19 +280,31 @@ local function make_prefix(redis_key, name, bucket)
   }
 end
 
+-- Distinguishes buckets within a single rule so that, e.g. "200 / 1h" and
+-- "30 / 1m" map to separate Redis keys instead of collapsing onto the same
+-- selector value (see issue #6059). Uses burst + rate, mirroring the burst
+-- component that gen_rate_key already prepends for the non-selector path.
+local function bucket_id(bucket)
+  return string.format('%s:%s',
+    lua_util.round(100000.0 / bucket.burst), bucket.rate)
+end
+
 local function limit_to_prefixes(task, k, v, prefixes)
   local n = 0
   for _, bucket in ipairs(v.buckets) do
     if v.selector then
       local selectors = lua_selectors.process_selectors(task, v.selector)
       if selectors then
+        local bid = bucket_id(bucket)
         local combined = lua_selectors.combine_selectors(task, selectors, ':')
         if type(combined) == 'string' then
-          prefixes[combined] = make_prefix(combined, k, bucket)
+          local rkey = bid .. ':' .. combined
+          prefixes[rkey] = make_prefix(rkey, k, bucket)
           n = n + 1
         else
           fun.each(function(p)
-            prefixes[p] = make_prefix(p, k, bucket)
+            local rkey = bid .. ':' .. p
+            prefixes[rkey] = make_prefix(rkey, k, bucket)
             n = n + 1
           end, combined)
         end

diff --git a/test/functional/cases/500_ratelimit.robot b/test/functional/cases/500_ratelimit.robot
index 85e7b353cd9a69a02bcf54ef056be06f510663ad..0490306d1109044399fc11bb92b818566afaf002 100644 (file)
--- a/test/functional/cases/500_ratelimit.robot
+++ b/test/functional/cases/500_ratelimit.robot
@@ -46,3 +46,9 @@ RATELIMIT CHECK BUILTIN
 
 RATELIMIT CHECK SELECTOR
   Basic Test  foo@example.net  special@example.net  2
+
+RATELIMIT CHECK MULTIPLE BUCKETS
+  # Regression for #6059: with two buckets (burst 2 and burst 20) the most
+  # restrictive one must fire. Before the fix only the last bucket (burst 20)
+  # was tracked, so a soft reject would never happen at the third message.
+  Basic Test  bar@example.net  multi@example.net  2

diff --git a/test/functional/configs/ratelimit.conf b/test/functional/configs/ratelimit.conf
index 36bae88feeeea1f01aaee95a91822b61bac6d74b..9e6034b77e366536e1d7ed181cd8f415bf57b5cd 100644 (file)
--- a/test/functional/configs/ratelimit.conf
+++ b/test/functional/configs/ratelimit.conf
@@ -19,5 +19,20 @@ ratelimit {
         rate = "1 / 1s";
       }
     }
+    to_multi_buckets {
+      # Two buckets in one rule: the restrictive one (burst = 2) must apply,
+      # not just the last one in the array (burst = 20). Regression for #6059.
+      selector = "id('multi');to.in('multi@example.net')";
+      bucket = [
+        {
+          burst = 2;
+          rate = "1 / 1s";
+        },
+        {
+          burst = 20;
+          rate = "1 / 1s";
+        }
+      ];
+    }
   }
 }