feat: limit count plugin support X-RateLimit-Reset #8578

mscb402 · 2022-12-28T03:41:32Z

Description

limit count plugin support returns a new header X-RateLimit-Reset and return the header when Rejected

Fixes #8381

Checklist

I have explained the need for this PR and the problem it solves
I have explained the changes or the new features added to this PR
I have added tests corresponding to this change

…and return header when rejected

spacewander · 2022-12-28T05:41:58Z

apisix/plugins/limit-count/init.lua

@@ -38,7 +39,9 @@ local lrucache = core.lrucache.new({
 local group_conf_lru = core.lrucache.new({
    type = 'plugin',
 })
-
+local resetlru = core.lrucache.new({


Suggested change

local resetlru = core.lrucache.new({

local reset_lru = core.lrucache.new({

spacewander · 2022-12-28T05:42:28Z

apisix/plugins/limit-count/init.lua

+        }
+    end
+
+    return resetlru(key,"",create,conf)


Don't forget to add space around the operator

spacewander · 2022-12-28T06:53:00Z

apisix/plugins/limit-count/init.lua

@@ -242,6 +245,15 @@ local function gen_limit_obj(conf, ctx)
    return core.lrucache.plugin_ctx(lrucache, ctx, extra_key, create_limit_obj, conf)
 end

+local function get_end_time(conf,key)
+    local create = function()


Please avoid creating new function per call

spacewander · 2022-12-28T06:53:47Z

docs/en/latest/plugins/limit-count.md

@@ -254,7 +254,7 @@ curl -i http://127.0.0.1:9180/apisix/admin/routes/1 \

 ## Example usage

-The above configuration limits to 2 requests in 60 seconds. The first two requests will work and the response headers will contain the headers `X-RateLimit-Limit` and `X-RateLimit-Remaining`:
+The above configuration limits to 2 requests in 60 seconds. The first two requests will work and the response headers will contain the headers `X-RateLimit-Limit` and `X-RateLimit-Remaining` and `X-RateLimit-Reset`:


Why the English version doesn't match the Chinese one?

spacewander · 2022-12-28T07:34:28Z

apisix/plugins/limit-count/init.lua

+        local end_time = ngx_time() + conf.time_window
+        -- save to lrucache by key
+        reset = conf.time_window
+        local end_time_obj = get_end_time(conf,key)


By default, the client IP will be used as key, which means there will be a great number of keys per conf. The size of lrucache is limited.

There come two solutions:

store the end time info in the actual backend like redis cluster.
~~2. write a new cache with dynamical expire support to store the end time info, but this way is not memory-effective.~~(it doesn't work)

Personally, I prefer solution 1.

solution 1 does not work either. Because by default limit count will not use Redis as the backend. So we can't save it to Redis.

I'm using ngx.shared with expire time instead.

The ngx.shared only works with the default backend which also uses ngx.shared. If we use other backends, only one instance will execute remaining == conf.count - 1 and the end time can only be found in that instance.

Fixed, you can review it again.

spacewander · 2023-01-03T05:55:17Z

apisix/plugins/limit-count/limit-count-redis-cluster.lua

+function _M.read_reset(self, key)
+    local red = self.red_cli
+    key = self.plugin_name .. tostring(key)
+    local ttl, err = red:ttl(key)


Can we put this into the eval script to make it atomic and reduce roundtrip?

spacewander · 2023-01-03T06:38:12Z

apisix/plugins/limit-count/limit-count-local.lua

+    __index = _M
+}
+
+function _M.set_endtime(self,key,time_window)


Look like we can merge set_endtime into the incoming method, especially after implementing read_reset via eval?

spacewander · 2023-01-03T06:39:32Z

apisix/cli/ngx_tpl.lua

@@ -276,6 +276,7 @@ http {
    {% if enabled_plugins["limit-count"] then %}
    lua_shared_dict plugin-limit-count {* http.lua_shared_dict["plugin-limit-count"] *};
    lua_shared_dict plugin-limit-count-redis-cluster-slot-lock {* http.lua_shared_dict["plugin-limit-count-redis-cluster-slot-lock"] *};
+    lua_shared_dict plugin-limit-count-reset-header {* http.lua_shared_dict["plugin-limit-count-reset-header"] *};


Maybe we can let the size of plugin-limit-count-reset-header be equal to plugin-limit-count, so that people don't need to configure it twice? As these two shdict store similar data.

I removed plugin-limit-count-reset-header in config

spacewander · 2023-01-04T05:50:05Z

apisix/plugins/limit-count/limit-count-redis.lua

+    return setmetatable(self, mt)
+end
+
+function _M.set_endtime(self,key,time_window)


We don't need to expose these methods now?

yes, removed

spacewander · 2023-01-04T05:51:28Z

apisix/plugins/limit-count/limit-count-redis-cluster.lua

@@ -91,16 +92,18 @@ function _M.incoming(self, key)
    local window = self.window
    key = self.plugin_name .. tostring(key)

-    local remaining, err = red:eval(script, 1, key, limit, window)
+    local res, err = red:eval(script, 1, key, limit, window)
+    local remaining = res[1]


Please use the res after checking the err

spacewander · 2023-01-04T05:54:58Z

t/cli/test_main.sh

@@ -807,6 +807,7 @@ nginx_config:
      internal-status: 20m
      plugin-limit-req: 20m
      plugin-limit-count: 20m
+      plugin-limit-count-reset-header: 20m


We don't need this setting, right?

Yes, I forget to remove

Now I'm removed this. Pls review again

spacewander · 2023-01-04T05:56:47Z

apisix/plugins/limit-count/limit-count-local.lua

+end
+
+function _M.incoming(self, key, commit, conf)
+    local delay, remaining =  self.limit_count:incoming(key, commit)


Suggested change

local delay, remaining = self.limit_count:incoming(key, commit)

local delay, remaining = self.limit_count:incoming(key, commit)

spacewander · 2023-01-05T06:02:44Z

apisix/plugins/limit-count/init.lua

@@ -302,7 +309,8 @@ function _M.rate_limit(conf, ctx)

    if conf.show_limit_quota_header then
        core.response.set_header("X-RateLimit-Limit", conf.count,
-            "X-RateLimit-Remaining", remaining)
+                "X-RateLimit-Remaining", remaining,


Why change 4-spaces indentation to 8-spaces here?

spacewander · 2023-01-05T06:02:52Z

apisix/plugins/limit-count/init.lua

+            -- show count limit header when rejected
+            if conf.show_limit_quota_header then
+                core.response.set_header("X-RateLimit-Limit", conf.count,
+                        "X-RateLimit-Remaining", 0,


spacewander · 2023-01-05T06:05:57Z

apisix/plugins/limit-count/limit-count-local.lua

+    -- set an end time
+    local end_time = ngx_time() + time_window
+    -- save to dict by key
+    self.dict:set(key, end_time, time_window)


Better to check the err returned from set.

spacewander · 2023-01-05T06:27:17Z

apisix/plugins/limit-count/limit-count-redis.lua

+function _M.incoming(self, key)
+    local conf = self.conf
+    local red, err = redis_cli(conf)
+    if err then


Suggested change

if err then

if not red then

spacewander · 2023-01-05T06:28:55Z

apisix/plugins/limit-count/limit-count-redis.lua

+    local conf = self.conf
+    local red, err = redis_cli(conf)
+    if err then
+        return red, err


Suggested change

return red, err

return nil, err, 0

Please follow the code style: https://github.com/apache/apisix/blob/master/CODE_STYLE.md#error-handling

mscb402 added 2 commits December 28, 2022 11:38

feat: limit count plugin support return new header X-RateLimit-Reset …

6fc3cd0

…and return header when rejected

chore: format code

2ab6f24

spacewander reviewed Dec 28, 2022

View reviewed changes

mscb402 added 6 commits December 28, 2022 16:07

chore: update code

613ece6

chore: using ngx shared to cache

c332ab6

chore: support redis and redis-cluster

6813e4f

chore: fix e2e error

eae4304

chore: try fix redis cluster error

bd32ca8

chore: fix test error

46285a6

spacewander reviewed Jan 3, 2023

View reviewed changes

mscb402 added 2 commits January 4, 2023 09:22

chore: remove plugin-limit-count-reset-header setting

ff6b2fd

chore: put set_endtime to incoming fucntion

34646d3

spacewander reviewed Jan 4, 2023

View reviewed changes

mscb402 added 2 commits January 4, 2023 14:01

chore: format code

0feba96

chore: format code

60d0a9a

spacewander reviewed Jan 5, 2023

View reviewed changes

mscb402 added 3 commits January 5, 2023 17:34

chore: format code

fa40f8d

chore: add dict error check

e548299

chore: code lint fix

cae2e6e

mscb402 requested a review from spacewander January 9, 2023 03:37

spacewander approved these changes Jan 9, 2023

View reviewed changes

tokers approved these changes Jan 9, 2023

View reviewed changes

spacewander merged commit bca13a3 into apache:master Jan 10, 2023

mscb402 deleted the feat/support_X-RateLimit-Reset branch January 17, 2023 06:14

	local resetlru = core.lrucache.new({
	local reset_lru = core.lrucache.new({

	local delay, remaining = self.limit_count:incoming(key, commit)
	local delay, remaining = self.limit_count:incoming(key, commit)

feat: limit count plugin support X-RateLimit-Reset #8578

feat: limit count plugin support X-RateLimit-Reset #8578

Conversation

mscb402 commented Dec 28, 2022 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacewander Dec 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscb402 commented Dec 28, 2022 •

edited

Loading

spacewander Dec 28, 2022 •

edited

Loading