feat: limit count plugin support X-RateLimit-Reset by mscb402 · Pull Request #8578 · apache/apisix

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

Description

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

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

    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

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


Don't forget to add space around the operator

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

 end

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


Please avoid creating new function per call

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

 ## 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

+        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

+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

+    __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

    {% 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

+    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


-    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

      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

+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

    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

+            -- 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

+    -- 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

+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

+    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)

Uh oh!

Conversation

mscb402 commented Dec 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacewander Dec 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mscb402 commented Dec 28, 2022 •

edited

Loading

spacewander Dec 28, 2022 •

edited

Loading