From 6fc3cd0ede1a7e103abb5827b1aeff903d5c6f5c Mon Sep 17 00:00:00 2001 From: mscb402 Date: Wed, 28 Dec 2022 11:38:33 +0800 Subject: [PATCH 01/15] feat: limit count plugin support return new header X-RateLimit-Reset and return header when rejected --- apisix/plugins/limit-count/init.lua | 66 ++++++++-- docs/en/latest/plugins/limit-count.md | 11 +- docs/zh/latest/plugins/limit-count.md | 11 +- t/plugin/limit-count4.t | 173 ++++++++++++++++++++++++++ 4 files changed, 247 insertions(+), 14 deletions(-) create mode 100644 t/plugin/limit-count4.t diff --git a/apisix/plugins/limit-count/init.lua b/apisix/plugins/limit-count/init.lua index c9051d2e14ef..41486a6836ad 100644 --- a/apisix/plugins/limit-count/init.lua +++ b/apisix/plugins/limit-count/init.lua @@ -20,6 +20,7 @@ local apisix_plugin = require("apisix.plugin") local tab_insert = table.insert local ipairs = ipairs local pairs = pairs +local ngx_time = ngx.time local plugin_name = "limit-count" @@ -38,7 +39,9 @@ local lrucache = core.lrucache.new({ local group_conf_lru = core.lrucache.new({ type = 'plugin', }) - +local resetlru = core.lrucache.new({ + type = 'plugin', +}) local policy_to_additional_properties = { redis = { @@ -91,8 +94,8 @@ local schema = { group = {type = "string"}, key = {type = "string", default = "remote_addr"}, key_type = {type = "string", - enum = {"var", "var_combination", "constant"}, - default = "var", + enum = {"var", "var_combination", "constant"}, + default = "var", }, rejected_code = { type = "integer", minimum = 200, maximum = 599, default = 503 @@ -172,9 +175,9 @@ function _M.check_schema(conf) for _, field in ipairs(fields) do if not core.table.deep_eq(prev_conf[field], conf[field]) then core.log.error("previous limit-conn group ", prev_conf.group, - " conf: ", core.json.encode(prev_conf)) + " conf: ", core.json.encode(prev_conf)) core.log.error("current limit-conn group ", conf.group, - " conf: ", core.json.encode(conf)) + " conf: ", core.json.encode(conf)) return false, "group conf mismatched" end end @@ -189,17 +192,17 @@ local function create_limit_obj(conf) if not conf.policy or conf.policy == "local" then return limit_local_new("plugin-" .. plugin_name, conf.count, - conf.time_window) + conf.time_window) end if conf.policy == "redis" then return limit_redis_new("plugin-" .. plugin_name, - conf.count, conf.time_window, conf) + conf.count, conf.time_window, conf) end if conf.policy == "redis-cluster" then return limit_redis_cluster_new("plugin-" .. plugin_name, conf.count, - conf.time_window, conf) + conf.time_window, conf) end return nil @@ -216,7 +219,7 @@ local function gen_limit_key(conf, ctx, key) -- because of the change elsewhere. -- A route which reuses a previous route's ID will inherits its counter. local new_key = ctx.conf_type .. ctx.conf_id .. ':' .. apisix_plugin.conf_version(conf) - .. ':' .. key + .. ':' .. key if conf._vid then -- conf has _vid means it's from workflow plugin, add _vid to the key -- so that the counter is unique per action. @@ -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() + return { + endtime=0, + } + end + + return resetlru(key,"",create,conf) +end function _M.rate_limit(conf, ctx) core.log.info("ver: ", ctx.conf_version) @@ -287,6 +299,21 @@ function _M.rate_limit(conf, ctx) if not delay then local err = remaining if err == "rejected" then + + local end_time_obj = get_end_time(conf,key) + local end_time = end_time_obj.endtime + local reset = end_time - ngx_time() + if reset < 0 then + reset = 0 + end + + -- 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, + "X-RateLimit-Reset", reset) + end + if conf.rejected_msg then return conf.rejected_code, { error_msg = conf.rejected_msg } end @@ -300,9 +327,28 @@ function _M.rate_limit(conf, ctx) return 500, {error_msg = "failed to limit count"} end + local reset + if remaining == conf.count - 1 then + -- set an end time + 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) + end_time_obj.endtime = end_time + else + -- read from lrucache + local end_time_obj = get_end_time(conf,key) + local end_time = end_time_obj.endtime + reset = end_time - ngx_time() + if reset < 0 then + reset = 0 + end + end + if conf.show_limit_quota_header then core.response.set_header("X-RateLimit-Limit", conf.count, - "X-RateLimit-Remaining", remaining) + "X-RateLimit-Remaining", remaining, + "X-RateLimit-Reset", reset) end end diff --git a/docs/en/latest/plugins/limit-count.md b/docs/en/latest/plugins/limit-count.md index b098dbd32e20..21aaf2504758 100644 --- a/docs/en/latest/plugins/limit-count.md +++ b/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`: ```shell curl -i http://127.0.0.1:9080/index.html @@ -267,16 +267,20 @@ Content-Length: 13175 Connection: keep-alive X-RateLimit-Limit: 2 X-RateLimit-Remaining: 0 +X-RateLimit-Reset: 58 Server: APISIX web server ``` -When you visit for a third time in the 60 seconds, you will receive a response with 503 code: +When you visit for a third time in the 60 seconds, you will receive a response with 503 code. Currently, in the case of rejection, the limit count headers is also returned: ```shell HTTP/1.1 503 Service Temporarily Unavailable Content-Type: text/html Content-Length: 194 Connection: keep-alive +X-RateLimit-Limit: 2 +X-RateLimit-Remaining: 0 +X-RateLimit-Reset: 58 Server: APISIX web server ``` @@ -287,6 +291,9 @@ HTTP/1.1 503 Service Temporarily Unavailable Content-Type: text/html Content-Length: 194 Connection: keep-alive +X-RateLimit-Limit: 2 +X-RateLimit-Remaining: 0 +X-RateLimit-Reset: 58 Server: APISIX web server {"error_msg":"Requests are too frequent, please try again later."} diff --git a/docs/zh/latest/plugins/limit-count.md b/docs/zh/latest/plugins/limit-count.md index 3055d22677a6..ffcc3bf6608b 100644 --- a/docs/zh/latest/plugins/limit-count.md +++ b/docs/zh/latest/plugins/limit-count.md @@ -253,7 +253,7 @@ curl -i http://127.0.0.1:9180/apisix/admin/routes/1 \ curl -i http://127.0.0.1:9080/index.html ``` -在执行测试命令的前两次都会正常访问。其中响应头中包含了 `X-RateLimit-Limit` 和 `X-RateLimit-Remaining` 字段,分别代表限制的总请求数和剩余还可以发送的请求数: +在执行测试命令的前两次都会正常访问。其中响应头中包含了 `X-RateLimit-Limit` 和 `X-RateLimit-Remaining` 和 `X-RateLimit-Reset` 字段,分别代表限制的总请求数和剩余还可以发送的请求数以及计数器剩余重置的秒数: ```shell HTTP/1.1 200 OK @@ -262,16 +262,20 @@ Content-Length: 13175 Connection: keep-alive X-RateLimit-Limit: 2 X-RateLimit-Remaining: 0 +X-RateLimit-Reset: 58 Server: APISIX web server ``` -当第三次进行测试访问时,会收到包含 `503` HTTP 状态码的响应头,表示插件生效: +当第三次进行测试访问时,会收到包含 `503` HTTP 状态码的响应头,目前在拒绝的情况下,也会返回相关的头,表示插件生效: ```shell HTTP/1.1 503 Service Temporarily Unavailable Content-Type: text/html Content-Length: 194 Connection: keep-alive +X-RateLimit-Limit: 2 +X-RateLimit-Remaining: 0 +X-RateLimit-Reset: 58 Server: APISIX web server ``` @@ -282,6 +286,9 @@ HTTP/1.1 503 Service Temporarily Unavailable Content-Type: text/html Content-Length: 194 Connection: keep-alive +X-RateLimit-Limit: 2 +X-RateLimit-Remaining: 0 +X-RateLimit-Reset: 58 Server: APISIX web server {"error_msg":"Requests are too frequent, please try again later."} diff --git a/t/plugin/limit-count4.t b/t/plugin/limit-count4.t new file mode 100644 index 000000000000..d9154c37329f --- /dev/null +++ b/t/plugin/limit-count4.t @@ -0,0 +1,173 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +BEGIN { + if ($ENV{TEST_NGINX_CHECK_LEAK}) { + $SkipReason = "unavailable for the hup tests"; + + } else { + $ENV{TEST_NGINX_USE_HUP} = 1; + undef $ENV{TEST_NGINX_USE_STAP}; + } +} + +use t::APISIX 'no_plan'; + +add_block_preprocessor(sub { + my ($block) = @_; + + if (!$block->request) { + $block->set_value("request", "GET /t"); + } + + if (!$block->error_log && !$block->no_error_log) { + $block->set_value("no_error_log", "[error]\n[alert]"); + } +}); + +run_tests; + +__DATA__ + +=== TEST 1: set route, counter will be shared +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/routes/1', + ngx.HTTP_PUT, + [[{ + "uri": "/hello", + "plugins": { + "limit-count": { + "count": 1, + "time_window": 60 + } + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + } + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- response_body +passed + + + +=== TEST 2: test X-RateLimit-Reset second number could be decline +--- config + location /t { + content_by_lua_block { + local json = require "t.toolkit.json" + local http = require "resty.http" + local uri = "http://127.0.0.1:" .. ngx.var.server_port + .. "/hello" + local old_X_RateLimit_Reset = 61 + for i = 1, 2 do + local httpc = http.new() + local res, err = httpc:request_uri(uri) + if not res then + ngx.say(err) + return + end + ngx.sleep(2) + if tonumber(res.headers["X-RateLimit-Reset"]) < old_X_RateLimit_Reset then + old_X_RateLimit_Reset = tonumber(res.headers["X-RateLimit-Reset"]) + ngx.say("OK") + else + ngx.say("WRONG") + end + end + ngx.say("Done") + } + } +--- response_body +OK +OK +Done + + + +=== TEST 3: set route, counter will be shared +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/routes/1', + ngx.HTTP_PUT, + [[{ + "uri": "/hello", + "plugins": { + "limit-count": { + "count": 2, + "time_window": 60 + } + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + } + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- response_body +passed + + + +=== TEST 4: test header X-RateLimit-Remaining exit when limit rejected +--- config + location /t { + content_by_lua_block { + local json = require "t.toolkit.json" + local http = require "resty.http" + local uri = "http://127.0.0.1:" .. ngx.var.server_port + .. "/hello" + local ress = {} + for i = 1, 3 do + local httpc = http.new() + local res, err = httpc:request_uri(uri) + if not res then + ngx.say(err) + return + end + ngx.sleep(1) + table.insert(ress, res.headers["X-RateLimit-Remaining"]) + + end + ngx.say(json.encode(ress)) + } + } +--- response_body +["1","0","0"] From 2ab6f24c97f78c460f0a814c8bee4c77aff615f7 Mon Sep 17 00:00:00 2001 From: mscb402 Date: Wed, 28 Dec 2022 13:35:11 +0800 Subject: [PATCH 02/15] chore: format code --- apisix/plugins/limit-count/init.lua | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/apisix/plugins/limit-count/init.lua b/apisix/plugins/limit-count/init.lua index 41486a6836ad..1f2379e5b013 100644 --- a/apisix/plugins/limit-count/init.lua +++ b/apisix/plugins/limit-count/init.lua @@ -94,8 +94,8 @@ local schema = { group = {type = "string"}, key = {type = "string", default = "remote_addr"}, key_type = {type = "string", - enum = {"var", "var_combination", "constant"}, - default = "var", + enum = {"var", "var_combination", "constant"}, + default = "var", }, rejected_code = { type = "integer", minimum = 200, maximum = 599, default = 503 @@ -175,9 +175,9 @@ function _M.check_schema(conf) for _, field in ipairs(fields) do if not core.table.deep_eq(prev_conf[field], conf[field]) then core.log.error("previous limit-conn group ", prev_conf.group, - " conf: ", core.json.encode(prev_conf)) + " conf: ", core.json.encode(prev_conf)) core.log.error("current limit-conn group ", conf.group, - " conf: ", core.json.encode(conf)) + " conf: ", core.json.encode(conf)) return false, "group conf mismatched" end end @@ -192,17 +192,17 @@ local function create_limit_obj(conf) if not conf.policy or conf.policy == "local" then return limit_local_new("plugin-" .. plugin_name, conf.count, - conf.time_window) + conf.time_window) end if conf.policy == "redis" then return limit_redis_new("plugin-" .. plugin_name, - conf.count, conf.time_window, conf) + conf.count, conf.time_window, conf) end if conf.policy == "redis-cluster" then return limit_redis_cluster_new("plugin-" .. plugin_name, conf.count, - conf.time_window, conf) + conf.time_window, conf) end return nil @@ -219,7 +219,7 @@ local function gen_limit_key(conf, ctx, key) -- because of the change elsewhere. -- A route which reuses a previous route's ID will inherits its counter. local new_key = ctx.conf_type .. ctx.conf_id .. ':' .. apisix_plugin.conf_version(conf) - .. ':' .. key + .. ':' .. key if conf._vid then -- conf has _vid means it's from workflow plugin, add _vid to the key -- so that the counter is unique per action. From 613ece633292120b0480c804663b5152c3d72ac7 Mon Sep 17 00:00:00 2001 From: mscb402 Date: Wed, 28 Dec 2022 16:07:02 +0800 Subject: [PATCH 03/15] chore: update code --- apisix/plugins/limit-count/init.lua | 16 ++++++++-------- docs/en/latest/plugins/limit-count.md | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/apisix/plugins/limit-count/init.lua b/apisix/plugins/limit-count/init.lua index 1f2379e5b013..de97d7dfbb8c 100644 --- a/apisix/plugins/limit-count/init.lua +++ b/apisix/plugins/limit-count/init.lua @@ -39,7 +39,7 @@ local lrucache = core.lrucache.new({ local group_conf_lru = core.lrucache.new({ type = 'plugin', }) -local resetlru = core.lrucache.new({ +local reset_lru = core.lrucache.new({ type = 'plugin', }) @@ -245,14 +245,14 @@ 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() - return { - endtime=0, - } - end +local function create_end_time() + return { + endtime=0, + } +end - return resetlru(key,"",create,conf) +local function get_end_time(conf,key) + return reset_lru(key, "", create_end_time, conf) end function _M.rate_limit(conf, ctx) diff --git a/docs/en/latest/plugins/limit-count.md b/docs/en/latest/plugins/limit-count.md index 21aaf2504758..09f22f43f5d5 100644 --- a/docs/en/latest/plugins/limit-count.md +++ b/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` and `X-RateLimit-Reset`: +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`, represents the total number of requests that are limited, the number of requests that can still be sent, and the number of seconds left for the counter to reset: ```shell curl -i http://127.0.0.1:9080/index.html From c332ab671ac941f2bda7a3e18e226ec55c18b478 Mon Sep 17 00:00:00 2001 From: mscb402 Date: Wed, 28 Dec 2022 17:16:24 +0800 Subject: [PATCH 04/15] chore: using ngx shared to cache --- apisix/cli/ngx_tpl.lua | 1 + apisix/plugins/limit-count/init.lua | 29 ++++++----------------------- conf/config-default.yaml | 1 + t/APISIX.pm | 1 + t/cli/test_main.sh | 6 ++++++ 5 files changed, 15 insertions(+), 23 deletions(-) diff --git a/apisix/cli/ngx_tpl.lua b/apisix/cli/ngx_tpl.lua index c1e46220a4e4..87587ff2a039 100644 --- a/apisix/cli/ngx_tpl.lua +++ b/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"] *}; {% end %} {% if enabled_plugins["prometheus"] and not enabled_stream_plugins["prometheus"] then %} diff --git a/apisix/plugins/limit-count/init.lua b/apisix/plugins/limit-count/init.lua index de97d7dfbb8c..cb8396d82978 100644 --- a/apisix/plugins/limit-count/init.lua +++ b/apisix/plugins/limit-count/init.lua @@ -20,9 +20,9 @@ local apisix_plugin = require("apisix.plugin") local tab_insert = table.insert local ipairs = ipairs local pairs = pairs +local dict = ngx.shared["plugin-limit-count-reset-header"] local ngx_time = ngx.time - local plugin_name = "limit-count" local limit_redis_cluster_new local limit_redis_new @@ -39,9 +39,6 @@ local lrucache = core.lrucache.new({ local group_conf_lru = core.lrucache.new({ type = 'plugin', }) -local reset_lru = core.lrucache.new({ - type = 'plugin', -}) local policy_to_additional_properties = { redis = { @@ -245,16 +242,6 @@ local function gen_limit_obj(conf, ctx) return core.lrucache.plugin_ctx(lrucache, ctx, extra_key, create_limit_obj, conf) end -local function create_end_time() - return { - endtime=0, - } -end - -local function get_end_time(conf,key) - return reset_lru(key, "", create_end_time, conf) -end - function _M.rate_limit(conf, ctx) core.log.info("ver: ", ctx.conf_version) @@ -299,9 +286,7 @@ function _M.rate_limit(conf, ctx) if not delay then local err = remaining if err == "rejected" then - - local end_time_obj = get_end_time(conf,key) - local end_time = end_time_obj.endtime + local end_time = (dict:get(key) or 0) local reset = end_time - ngx_time() if reset < 0 then reset = 0 @@ -331,14 +316,12 @@ function _M.rate_limit(conf, ctx) if remaining == conf.count - 1 then -- set an end time local end_time = ngx_time() + conf.time_window - -- save to lrucache by key + -- save to dict by key + dict:set(key,end_time,conf.time_window) reset = conf.time_window - local end_time_obj = get_end_time(conf,key) - end_time_obj.endtime = end_time else - -- read from lrucache - local end_time_obj = get_end_time(conf,key) - local end_time = end_time_obj.endtime + -- read from dict + local end_time = (dict:get(key) or 0) reset = end_time - ngx_time() if reset < 0 then reset = 0 diff --git a/conf/config-default.yaml b/conf/config-default.yaml index 4dca7bac0e93..d0afcc39252f 100755 --- a/conf/config-default.yaml +++ b/conf/config-default.yaml @@ -238,6 +238,7 @@ nginx_config: # config for render the template to generate n balancer-ewma-locks: 10m balancer-ewma-last-touched-at: 10m plugin-limit-count-redis-cluster-slot-lock: 1m + plugin-limit-count-reset-header: 10m tracing_buffer: 10m plugin-api-breaker: 10m etcd-cluster-health-check: 10m diff --git a/t/APISIX.pm b/t/APISIX.pm index 22e143ba2244..fe534b20c73b 100644 --- a/t/APISIX.pm +++ b/t/APISIX.pm @@ -533,6 +533,7 @@ _EOC_ lua_shared_dict plugin-limit-req 10m; lua_shared_dict plugin-limit-count 10m; + lua_shared_dict plugin-limit-count-reset-header 10m; lua_shared_dict plugin-limit-conn 10m; lua_shared_dict internal-status 10m; lua_shared_dict upstream-healthcheck 32m; diff --git a/t/cli/test_main.sh b/t/cli/test_main.sh index f5b78be7f952..ae6aa6af5763 100755 --- a/t/cli/test_main.sh +++ b/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 prometheus-metrics: 20m plugin-limit-conn: 20m upstream-healthcheck: 20m @@ -842,6 +843,11 @@ if ! grep "plugin-limit-count 20m;" conf/nginx.conf > /dev/null; then exit 1 fi +if ! grep "plugin-limit-count-reset-header 20m;" conf/nginx.conf > /dev/null; then + echo "failed: 'plugin-limit-count-reset-header 20m;' not in nginx.conf" + exit 1 +fi + if ! grep "prometheus-metrics 20m;" conf/nginx.conf > /dev/null; then echo "failed: 'prometheus-metrics 20m;' not in nginx.conf" exit 1 From 6813e4f7e74944a381b52934e7f444dc0d5be316 Mon Sep 17 00:00:00 2001 From: mscb402 Date: Fri, 30 Dec 2022 14:16:59 +0800 Subject: [PATCH 05/15] chore: support redis and redis-cluster --- apisix/plugins/limit-count/init.lua | 26 +-- .../plugins/limit-count/limit-count-local.lua | 45 +++++ .../limit-count/limit-count-redis-cluster.lua | 15 ++ .../plugins/limit-count/limit-count-redis.lua | 65 ++++-- t/plugin/limit-count-redis-cluster.t | 116 +++++++++++ t/plugin/limit-count-redis3.t | 188 ++++++++++++++++++ t/plugin/limit-count4.t | 2 +- 7 files changed, 420 insertions(+), 37 deletions(-) create mode 100644 apisix/plugins/limit-count/limit-count-local.lua create mode 100644 t/plugin/limit-count-redis3.t diff --git a/apisix/plugins/limit-count/init.lua b/apisix/plugins/limit-count/init.lua index cb8396d82978..372b66da951c 100644 --- a/apisix/plugins/limit-count/init.lua +++ b/apisix/plugins/limit-count/init.lua @@ -14,19 +14,20 @@ -- See the License for the specific language governing permissions and -- limitations under the License. -- -local limit_local_new = require("resty.limit.count").new local core = require("apisix.core") local apisix_plugin = require("apisix.plugin") local tab_insert = table.insert local ipairs = ipairs local pairs = pairs -local dict = ngx.shared["plugin-limit-count-reset-header"] -local ngx_time = ngx.time local plugin_name = "limit-count" local limit_redis_cluster_new local limit_redis_new +local limit_local_new do + local redis_src = "apisix.plugins.limit-count.limit-count-local" + limit_local_new = require(redis_src).new + local redis_src = "apisix.plugins.limit-count.limit-count-redis" limit_redis_new = require(redis_src).new @@ -286,11 +287,7 @@ function _M.rate_limit(conf, ctx) if not delay then local err = remaining if err == "rejected" then - local end_time = (dict:get(key) or 0) - local reset = end_time - ngx_time() - if reset < 0 then - reset = 0 - end + local reset = lim:read_reset(key) -- show count limit header when rejected if conf.show_limit_quota_header then @@ -314,18 +311,9 @@ function _M.rate_limit(conf, ctx) local reset if remaining == conf.count - 1 then - -- set an end time - local end_time = ngx_time() + conf.time_window - -- save to dict by key - dict:set(key,end_time,conf.time_window) - reset = conf.time_window + reset = lim:set_endtime(key, conf.time_window) else - -- read from dict - local end_time = (dict:get(key) or 0) - reset = end_time - ngx_time() - if reset < 0 then - reset = 0 - end + reset = lim:read_reset(key) end if conf.show_limit_quota_header then diff --git a/apisix/plugins/limit-count/limit-count-local.lua b/apisix/plugins/limit-count/limit-count-local.lua new file mode 100644 index 000000000000..b5d880adfc32 --- /dev/null +++ b/apisix/plugins/limit-count/limit-count-local.lua @@ -0,0 +1,45 @@ +local limit_local_new = require("resty.limit.count").new +local ngx_time = ngx.time + +local _M = {} + +local mt = { + __index = _M +} + +function _M.set_endtime(self,key,time_window) + -- set an end time + local end_time = ngx_time() + time_window + -- save to dict by key + self.dict:set(key, end_time, time_window) + + local reset = time_window + return reset +end + +function _M.read_reset(self, key) + -- read from dict + local end_time = (self.dict:get(key) or 0) + local reset = end_time - ngx_time() + if reset < 0 then + reset = 0 + end + return reset +end + +function _M.new(plugin_name, limit, window, conf) + assert(limit > 0 and window > 0) + + local self = { + limit_count = limit_local_new(plugin_name, limit, window, conf), + dict = ngx.shared["plugin-limit-count-reset-header"] + } + + return setmetatable(self, mt) +end + +function _M.incoming(self, key, commit) + return self.limit_count:incoming(key, commit) +end + +return _M diff --git a/apisix/plugins/limit-count/limit-count-redis-cluster.lua b/apisix/plugins/limit-count/limit-count-redis-cluster.lua index 27d4e85faa4e..817e1093c601 100644 --- a/apisix/plugins/limit-count/limit-count-redis-cluster.lua +++ b/apisix/plugins/limit-count/limit-count-redis-cluster.lua @@ -84,6 +84,21 @@ function _M.new(plugin_name, limit, window, conf) return setmetatable(self, mt) end +function _M.set_endtime(self,key,time_window) + return time_window +end + +function _M.read_reset(self, key) + local red = self.red_cli + + local ttl, err = red:ttl(key) + if err then + core.log.err("key: ", key, " read_reset with error: ", err) + return 0 + end + + return ttl +end function _M.incoming(self, key) local red = self.red_cli diff --git a/apisix/plugins/limit-count/limit-count-redis.lua b/apisix/plugins/limit-count/limit-count-redis.lua index d5c4648248bc..5596fbe471e1 100644 --- a/apisix/plugins/limit-count/limit-count-redis.lua +++ b/apisix/plugins/limit-count/limit-count-redis.lua @@ -37,25 +37,9 @@ local script = core.string.compress_script([=[ return redis.call('incrby', KEYS[1], -1) ]=]) - -function _M.new(plugin_name, limit, window, conf) - assert(limit > 0 and window > 0) - - local self = { - limit = limit, - window = window, - conf = conf, - plugin_name = plugin_name, - } - return setmetatable(self, mt) -end - - -function _M.incoming(self, key) - local conf = self.conf +local function redis_cli(conf) local red = redis_new() local timeout = conf.redis_timeout or 1000 -- 1sec - core.log.info("ttl key: ", key, " timeout: ", timeout) red:set_timeouts(timeout, timeout, timeout) @@ -85,6 +69,53 @@ function _M.incoming(self, key) -- core.log.info(" err: ", err) return nil, err end + return red, nil +end + +function _M.new(plugin_name, limit, window, conf) + assert(limit > 0 and window > 0) + + local self = { + limit = limit, + window = window, + conf = conf, + plugin_name = plugin_name, + } + return setmetatable(self, mt) +end + +function _M.set_endtime(self,key,time_window) + return time_window +end + +function _M.read_reset(self, key) + local conf = self.conf + local red, err = redis_cli(conf) + if err then + return red, err + end + + local ttl, err = red:ttl(key) + if err then + core.log.err("key: ", key, " read_reset with error: ", err) + return 0 + end + + local ok, err = red:set_keepalive(10000, 100) + if not ok then + core.log.err("key: ", key, " read_reset with error: ", err) + return 0 + end + + return ttl +end + +function _M.incoming(self, key) + local conf = self.conf + local red, err = redis_cli(conf) + if err then + return red, err + end local limit = self.limit local window = self.window diff --git a/t/plugin/limit-count-redis-cluster.t b/t/plugin/limit-count-redis-cluster.t index 7f5eb2c1f8d7..13e1e39d3c84 100644 --- a/t/plugin/limit-count-redis-cluster.t +++ b/t/plugin/limit-count-redis-cluster.t @@ -384,3 +384,119 @@ GET /hello hello world --- error_log connection refused + + + +=== TEST 12: set route, counter will be shared +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/routes/1', + ngx.HTTP_PUT, + [[{ + "uri": "/hello", + "plugins": { + "limit-count": { + "count": 1, + "time_window": 60, + "policy": "redis-cluster", + "redis_cluster_nodes": [ + "127.0.0.1:5000", + "127.0.0.1:5001" + ], + "redis_cluster_name": "redis-cluster-1" + } + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + } + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- response_body +passed + + + +=== TEST 13: test X-RateLimit-Reset second number could be decline +--- config + location /t { + content_by_lua_block { + local json = require "t.toolkit.json" + local http = require "resty.http" + local uri = "http://127.0.0.1:" .. ngx.var.server_port + .. "/hello" + local old_X_RateLimit_Reset = 61 + for i = 1, 2 do + local httpc = http.new() + local res, err = httpc:request_uri(uri) + if not res then + ngx.say(err) + return + end + ngx.sleep(2) + if tonumber(res.headers["X-RateLimit-Reset"]) < old_X_RateLimit_Reset then + old_X_RateLimit_Reset = tonumber(res.headers["X-RateLimit-Reset"]) + ngx.say("OK") + else + ngx.say("WRONG") + end + end + ngx.say("Done") + } + } +--- response_body +OK +OK +Done + + + +=== TEST 14: set route, counter will be shared +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/routes/1', + ngx.HTTP_PUT, + [[{ + "uri": "/hello", + "plugins": { + "limit-count": { + "count": 2, + "time_window": 60, + "policy": "redis-cluster", + "redis_cluster_nodes": [ + "127.0.0.1:5000", + "127.0.0.1:5001" + ], + "redis_cluster_name": "redis-cluster-1" + } + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + } + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- response_body +passed diff --git a/t/plugin/limit-count-redis3.t b/t/plugin/limit-count-redis3.t new file mode 100644 index 000000000000..77c7c2590b33 --- /dev/null +++ b/t/plugin/limit-count-redis3.t @@ -0,0 +1,188 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +BEGIN { + if ($ENV{TEST_NGINX_CHECK_LEAK}) { + $SkipReason = "unavailable for the hup tests"; + + } else { + $ENV{TEST_NGINX_USE_HUP} = 1; + undef $ENV{TEST_NGINX_USE_STAP}; + } +} + +use t::APISIX 'no_plan'; + +repeat_each(1); +no_long_string(); +no_shuffle(); +no_root_location(); + +add_block_preprocessor(sub { + my ($block) = @_; + + if (!$block->request) { + $block->set_value("request", "GET /t"); + } + + if (!$block->error_log && !$block->no_error_log) { + $block->set_value("no_error_log", "[error]\n[alert]"); + } +}); + +run_tests; + +__DATA__ + +=== TEST 1: set route, counter will be shared +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/routes/1', + ngx.HTTP_PUT, + [[{ + "uri": "/hello", + "plugins": { + "limit-count": { + "count": 1, + "time_window": 60, + "policy": "redis", + "redis_host": "127.0.0.1", + "redis_port": 6379, + "redis_database": 1, + "redis_timeout": 1001 + } + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + } + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- response_body +passed + + + +=== TEST 2: test X-RateLimit-Reset second number could be decline +--- config + location /t { + content_by_lua_block { + local json = require "t.toolkit.json" + local http = require "resty.http" + local uri = "http://127.0.0.1:" .. ngx.var.server_port + .. "/hello" + local old_X_RateLimit_Reset = 61 + for i = 1, 2 do + local httpc = http.new() + local res, err = httpc:request_uri(uri) + if not res then + ngx.say(err) + return + end + ngx.sleep(2) + if tonumber(res.headers["X-RateLimit-Reset"]) < old_X_RateLimit_Reset then + old_X_RateLimit_Reset = tonumber(res.headers["X-RateLimit-Reset"]) + ngx.say("OK") + else + ngx.say("WRONG") + end + end + ngx.say("Done") + } + } +--- response_body +OK +OK +Done + + + +=== TEST 3: set route, counter will be shared +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/routes/1', + ngx.HTTP_PUT, + [[{ + "uri": "/hello", + "plugins": { + "limit-count": { + "count": 2, + "time_window": 60, + "policy": "redis", + "redis_host": "127.0.0.1", + "redis_port": 6379, + "redis_database": 1, + "redis_timeout": 1001 + } + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + } + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- response_body +passed + + + +=== TEST 4: test header X-RateLimit-Remaining exist when limit rejected +--- config + location /t { + content_by_lua_block { + local json = require "t.toolkit.json" + local http = require "resty.http" + local uri = "http://127.0.0.1:" .. ngx.var.server_port + .. "/hello" + local ress = {} + for i = 1, 3 do + local httpc = http.new() + local res, err = httpc:request_uri(uri) + if not res then + ngx.say(err) + return + end + ngx.sleep(1) + table.insert(ress, res.headers["X-RateLimit-Remaining"]) + + end + ngx.say(json.encode(ress)) + } + } +--- response_body +["1","0","0"] diff --git a/t/plugin/limit-count4.t b/t/plugin/limit-count4.t index d9154c37329f..bcefe5156fd0 100644 --- a/t/plugin/limit-count4.t +++ b/t/plugin/limit-count4.t @@ -146,7 +146,7 @@ passed -=== TEST 4: test header X-RateLimit-Remaining exit when limit rejected +=== TEST 4: test header X-RateLimit-Remaining exist when limit rejected --- config location /t { content_by_lua_block { From eae4304f29d5a8f40a6f616add86d003477c36ee Mon Sep 17 00:00:00 2001 From: mscb402 Date: Fri, 30 Dec 2022 14:44:04 +0800 Subject: [PATCH 06/15] chore: fix e2e error --- .../plugins/limit-count/limit-count-local.lua | 19 +++++++++++++ t/plugin/limit-count-redis-cluster.t | 28 +++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/apisix/plugins/limit-count/limit-count-local.lua b/apisix/plugins/limit-count/limit-count-local.lua index b5d880adfc32..c71bda356e6a 100644 --- a/apisix/plugins/limit-count/limit-count-local.lua +++ b/apisix/plugins/limit-count/limit-count-local.lua @@ -1,5 +1,24 @@ +-- +-- Licensed to the Apache Software Foundation (ASF) under one or more +-- contributor license agreements. See the NOTICE file distributed with +-- this work for additional information regarding copyright ownership. +-- The ASF licenses this file to You under the Apache License, Version 2.0 +-- (the "License"); you may not use this file except in compliance with +-- the License. You may obtain a copy of the License at +-- +-- http://www.apache.org/licenses/LICENSE-2.0 +-- +-- Unless required by applicable law or agreed to in writing, software +-- distributed under the License is distributed on an "AS IS" BASIS, +-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +-- See the License for the specific language governing permissions and +-- limitations under the License. +-- local limit_local_new = require("resty.limit.count").new +local ngx = ngx local ngx_time = ngx.time +local assert = assert +local setmetatable = setmetatable local _M = {} diff --git a/t/plugin/limit-count-redis-cluster.t b/t/plugin/limit-count-redis-cluster.t index 13e1e39d3c84..5502147f6964 100644 --- a/t/plugin/limit-count-redis-cluster.t +++ b/t/plugin/limit-count-redis-cluster.t @@ -500,3 +500,31 @@ Done } --- response_body passed + + + +=== TEST 15: test header X-RateLimit-Remaining exist when limit rejected +--- config + location /t { + content_by_lua_block { + local json = require "t.toolkit.json" + local http = require "resty.http" + local uri = "http://127.0.0.1:" .. ngx.var.server_port + .. "/hello" + local ress = {} + for i = 1, 3 do + local httpc = http.new() + local res, err = httpc:request_uri(uri) + if not res then + ngx.say(err) + return + end + ngx.sleep(1) + table.insert(ress, res.headers["X-RateLimit-Remaining"]) + + end + ngx.say(json.encode(ress)) + } + } +--- response_body +["1","0","0"] From bd32ca8ac5fa6ba9141ea09b7bf97f7c75d3025b Mon Sep 17 00:00:00 2001 From: mscb402 Date: Fri, 30 Dec 2022 17:34:00 +0800 Subject: [PATCH 07/15] chore: try fix redis cluster error --- apisix/plugins/limit-count/init.lua | 4 +- .../limit-count/limit-count-redis-cluster.lua | 10 +- .../plugins/limit-count/limit-count-redis.lua | 8 +- t/plugin/limit-count-redis-cluster.t | 144 ------------------ t/plugin/limit-count-redis-cluster2.t | 89 +++++++++++ 5 files changed, 105 insertions(+), 150 deletions(-) create mode 100644 t/plugin/limit-count-redis-cluster2.t diff --git a/apisix/plugins/limit-count/init.lua b/apisix/plugins/limit-count/init.lua index 372b66da951c..893aadece3e8 100644 --- a/apisix/plugins/limit-count/init.lua +++ b/apisix/plugins/limit-count/init.lua @@ -25,8 +25,8 @@ local limit_redis_cluster_new local limit_redis_new local limit_local_new do - local redis_src = "apisix.plugins.limit-count.limit-count-local" - limit_local_new = require(redis_src).new + local local_src = "apisix.plugins.limit-count.limit-count-local" + limit_local_new = require(local_src).new local redis_src = "apisix.plugins.limit-count.limit-count-redis" limit_redis_new = require(redis_src).new diff --git a/apisix/plugins/limit-count/limit-count-redis-cluster.lua b/apisix/plugins/limit-count/limit-count-redis-cluster.lua index 817e1093c601..09a6fc47f2ae 100644 --- a/apisix/plugins/limit-count/limit-count-redis-cluster.lua +++ b/apisix/plugins/limit-count/limit-count-redis-cluster.lua @@ -36,6 +36,13 @@ local script = core.string.compress_script([=[ end return redis.call('incrby', KEYS[1], -1) ]=]) +local script2 = core.string.compress_script([=[ + local t = redis.call('ttl', KEYS[1]) + if t < 0 then + return 0 + end + return t +]=]) local function new_redis_cluster(conf) @@ -91,9 +98,8 @@ end function _M.read_reset(self, key) local red = self.red_cli - local ttl, err = red:ttl(key) + local ttl, err = red:eval(script2,1,key) if err then - core.log.err("key: ", key, " read_reset with error: ", err) return 0 end diff --git a/apisix/plugins/limit-count/limit-count-redis.lua b/apisix/plugins/limit-count/limit-count-redis.lua index 5596fbe471e1..49756d917e70 100644 --- a/apisix/plugins/limit-count/limit-count-redis.lua +++ b/apisix/plugins/limit-count/limit-count-redis.lua @@ -97,13 +97,17 @@ function _M.read_reset(self, key) local ttl, err = red:ttl(key) if err then - core.log.err("key: ", key, " read_reset with error: ", err) + core.log.error("key: ", key, " read_reset with error: ", err) return 0 end local ok, err = red:set_keepalive(10000, 100) if not ok then - core.log.err("key: ", key, " read_reset with error: ", err) + core.log.error("key: ", key, " read_reset with error: ", err) + return 0 + end + + if ttl < 0 then return 0 end diff --git a/t/plugin/limit-count-redis-cluster.t b/t/plugin/limit-count-redis-cluster.t index 5502147f6964..7f5eb2c1f8d7 100644 --- a/t/plugin/limit-count-redis-cluster.t +++ b/t/plugin/limit-count-redis-cluster.t @@ -384,147 +384,3 @@ GET /hello hello world --- error_log connection refused - - - -=== TEST 12: set route, counter will be shared ---- config - location /t { - content_by_lua_block { - local t = require("lib.test_admin").test - local code, body = t('/apisix/admin/routes/1', - ngx.HTTP_PUT, - [[{ - "uri": "/hello", - "plugins": { - "limit-count": { - "count": 1, - "time_window": 60, - "policy": "redis-cluster", - "redis_cluster_nodes": [ - "127.0.0.1:5000", - "127.0.0.1:5001" - ], - "redis_cluster_name": "redis-cluster-1" - } - }, - "upstream": { - "nodes": { - "127.0.0.1:1980": 1 - }, - "type": "roundrobin" - } - }]] - ) - - if code >= 300 then - ngx.status = code - end - ngx.say(body) - } - } ---- response_body -passed - - - -=== TEST 13: test X-RateLimit-Reset second number could be decline ---- config - location /t { - content_by_lua_block { - local json = require "t.toolkit.json" - local http = require "resty.http" - local uri = "http://127.0.0.1:" .. ngx.var.server_port - .. "/hello" - local old_X_RateLimit_Reset = 61 - for i = 1, 2 do - local httpc = http.new() - local res, err = httpc:request_uri(uri) - if not res then - ngx.say(err) - return - end - ngx.sleep(2) - if tonumber(res.headers["X-RateLimit-Reset"]) < old_X_RateLimit_Reset then - old_X_RateLimit_Reset = tonumber(res.headers["X-RateLimit-Reset"]) - ngx.say("OK") - else - ngx.say("WRONG") - end - end - ngx.say("Done") - } - } ---- response_body -OK -OK -Done - - - -=== TEST 14: set route, counter will be shared ---- config - location /t { - content_by_lua_block { - local t = require("lib.test_admin").test - local code, body = t('/apisix/admin/routes/1', - ngx.HTTP_PUT, - [[{ - "uri": "/hello", - "plugins": { - "limit-count": { - "count": 2, - "time_window": 60, - "policy": "redis-cluster", - "redis_cluster_nodes": [ - "127.0.0.1:5000", - "127.0.0.1:5001" - ], - "redis_cluster_name": "redis-cluster-1" - } - }, - "upstream": { - "nodes": { - "127.0.0.1:1980": 1 - }, - "type": "roundrobin" - } - }]] - ) - - if code >= 300 then - ngx.status = code - end - ngx.say(body) - } - } ---- response_body -passed - - - -=== TEST 15: test header X-RateLimit-Remaining exist when limit rejected ---- config - location /t { - content_by_lua_block { - local json = require "t.toolkit.json" - local http = require "resty.http" - local uri = "http://127.0.0.1:" .. ngx.var.server_port - .. "/hello" - local ress = {} - for i = 1, 3 do - local httpc = http.new() - local res, err = httpc:request_uri(uri) - if not res then - ngx.say(err) - return - end - ngx.sleep(1) - table.insert(ress, res.headers["X-RateLimit-Remaining"]) - - end - ngx.say(json.encode(ress)) - } - } ---- response_body -["1","0","0"] diff --git a/t/plugin/limit-count-redis-cluster2.t b/t/plugin/limit-count-redis-cluster2.t new file mode 100644 index 000000000000..d5363c016d8b --- /dev/null +++ b/t/plugin/limit-count-redis-cluster2.t @@ -0,0 +1,89 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +use t::APISIX 'no_plan'; + +repeat_each(1); +no_long_string(); +no_shuffle(); +no_root_location(); + +add_block_preprocessor(sub { + my ($block) = @_; + + if (!$block->request) { + $block->set_value("request", "GET /t"); + } + + if (!$block->error_log && !$block->no_error_log) { + $block->set_value("no_error_log", "[error]\n[alert]"); + } +}); + +run_tests; + +__DATA__ + +=== TEST 1: update route, use new limit configuration +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/routes/1', + ngx.HTTP_PUT, + [[{ + "uri": "/hello2", + "plugins": { + "limit-count": { + "count": 1, + "time_window": 60, + "key": "remote_addr", + "policy": "redis-cluster", + "redis_cluster_nodes": [ + "127.0.0.1:5000", + "127.0.0.1:5001" + ], + "redis_cluster_name": "redis-cluster-1" + } + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + } + }]] + ) + local old_X_RateLimit_Reset = 61 + for i = 1, 3 do + local _, _, headers = t('/hello2', ngx.HTTP_GET) + ngx.sleep(1) + if tonumber(headers["X-RateLimit-Reset"]) < old_X_RateLimit_Reset then + old_X_RateLimit_Reset = tonumber(headers["X-RateLimit-Reset"]) + ngx.say("OK") + else + ngx.say("WRONG") + end + end + ngx.say("Done") + } + } +--- response_body +OK +OK +OK +Done From 46285a61403d30d165070099397c4e6cdfdc89a7 Mon Sep 17 00:00:00 2001 From: mscb402 Date: Tue, 3 Jan 2023 11:56:57 +0800 Subject: [PATCH 08/15] chore: fix test error --- .../limit-count/limit-count-redis-cluster.lua | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/apisix/plugins/limit-count/limit-count-redis-cluster.lua b/apisix/plugins/limit-count/limit-count-redis-cluster.lua index 09a6fc47f2ae..93d68accbfcf 100644 --- a/apisix/plugins/limit-count/limit-count-redis-cluster.lua +++ b/apisix/plugins/limit-count/limit-count-redis-cluster.lua @@ -36,13 +36,6 @@ local script = core.string.compress_script([=[ end return redis.call('incrby', KEYS[1], -1) ]=]) -local script2 = core.string.compress_script([=[ - local t = redis.call('ttl', KEYS[1]) - if t < 0 then - return 0 - end - return t -]=]) local function new_redis_cluster(conf) @@ -97,11 +90,14 @@ end function _M.read_reset(self, key) local red = self.red_cli - - local ttl, err = red:eval(script2,1,key) + key = self.plugin_name .. tostring(key) + local ttl, err = red:ttl(key) if err then return 0 end + if ttl < 0 then + return 0 + end return ttl end From ff6b2fd1e1659876bf83bcf5eb6379e64a598d7a Mon Sep 17 00:00:00 2001 From: mscb402 Date: Wed, 4 Jan 2023 09:22:07 +0800 Subject: [PATCH 09/15] chore: remove plugin-limit-count-reset-header setting --- apisix/cli/ngx_tpl.lua | 2 +- conf/config-default.yaml | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/apisix/cli/ngx_tpl.lua b/apisix/cli/ngx_tpl.lua index 87587ff2a039..7a1f4e9c7722 100644 --- a/apisix/cli/ngx_tpl.lua +++ b/apisix/cli/ngx_tpl.lua @@ -276,7 +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"] *}; + lua_shared_dict plugin-limit-count-reset-header {* http.lua_shared_dict["plugin-limit-count"] *}; {% end %} {% if enabled_plugins["prometheus"] and not enabled_stream_plugins["prometheus"] then %} diff --git a/conf/config-default.yaml b/conf/config-default.yaml index d0afcc39252f..4dca7bac0e93 100755 --- a/conf/config-default.yaml +++ b/conf/config-default.yaml @@ -238,7 +238,6 @@ nginx_config: # config for render the template to generate n balancer-ewma-locks: 10m balancer-ewma-last-touched-at: 10m plugin-limit-count-redis-cluster-slot-lock: 1m - plugin-limit-count-reset-header: 10m tracing_buffer: 10m plugin-api-breaker: 10m etcd-cluster-health-check: 10m From 34646d3d30ed829e79069439bfb88292a49ce93a Mon Sep 17 00:00:00 2001 From: mscb402 Date: Wed, 4 Jan 2023 11:49:40 +0800 Subject: [PATCH 10/15] chore: put set_endtime to incoming fucntion --- apisix/plugins/limit-count/init.lua | 11 +----- .../plugins/limit-count/limit-count-local.lua | 20 ++++++++--- .../limit-count/limit-count-redis-cluster.lua | 34 ++++++------------- .../plugins/limit-count/limit-count-redis.lua | 21 +++++++----- t/plugin/limit-count-redis3.t | 4 +-- 5 files changed, 41 insertions(+), 49 deletions(-) diff --git a/apisix/plugins/limit-count/init.lua b/apisix/plugins/limit-count/init.lua index 893aadece3e8..192b0879795a 100644 --- a/apisix/plugins/limit-count/init.lua +++ b/apisix/plugins/limit-count/init.lua @@ -283,12 +283,10 @@ function _M.rate_limit(conf, ctx) key = gen_limit_key(conf, ctx, key) core.log.info("limit key: ", key) - local delay, remaining = lim:incoming(key, true) + local delay, remaining, reset = lim:incoming(key, true, conf) if not delay then local err = remaining if err == "rejected" then - local reset = lim:read_reset(key) - -- show count limit header when rejected if conf.show_limit_quota_header then core.response.set_header("X-RateLimit-Limit", conf.count, @@ -309,13 +307,6 @@ function _M.rate_limit(conf, ctx) return 500, {error_msg = "failed to limit count"} end - local reset - if remaining == conf.count - 1 then - reset = lim:set_endtime(key, conf.time_window) - else - reset = lim:read_reset(key) - end - if conf.show_limit_quota_header then core.response.set_header("X-RateLimit-Limit", conf.count, "X-RateLimit-Remaining", remaining, diff --git a/apisix/plugins/limit-count/limit-count-local.lua b/apisix/plugins/limit-count/limit-count-local.lua index c71bda356e6a..78986687523b 100644 --- a/apisix/plugins/limit-count/limit-count-local.lua +++ b/apisix/plugins/limit-count/limit-count-local.lua @@ -26,7 +26,7 @@ local mt = { __index = _M } -function _M.set_endtime(self,key,time_window) +local function set_endtime(self, key, time_window) -- set an end time local end_time = ngx_time() + time_window -- save to dict by key @@ -36,7 +36,7 @@ function _M.set_endtime(self,key,time_window) return reset end -function _M.read_reset(self, key) +local function read_reset(self, key) -- read from dict local end_time = (self.dict:get(key) or 0) local reset = end_time - ngx_time() @@ -57,8 +57,20 @@ function _M.new(plugin_name, limit, window, conf) return setmetatable(self, mt) end -function _M.incoming(self, key, commit) - return self.limit_count:incoming(key, commit) +function _M.incoming(self, key, commit, conf) + local delay, remaining = self.limit_count:incoming(key, commit) + local reset = 0 + if not delay then + return delay, remaining, reset + end + + if remaining == conf.count - 1 then + reset = set_endtime(self, key, conf.time_window) + else + reset = read_reset(self, key) + end + + return delay, remaining, reset end return _M diff --git a/apisix/plugins/limit-count/limit-count-redis-cluster.lua b/apisix/plugins/limit-count/limit-count-redis-cluster.lua index 93d68accbfcf..2916491e75ac 100644 --- a/apisix/plugins/limit-count/limit-count-redis-cluster.lua +++ b/apisix/plugins/limit-count/limit-count-redis-cluster.lua @@ -30,11 +30,12 @@ local mt = { local script = core.string.compress_script([=[ - if redis.call('ttl', KEYS[1]) < 0 then + local ttl = redis.call('ttl', KEYS[1]) + if ttl < 0 then redis.call('set', KEYS[1], ARGV[1] - 1, 'EX', ARGV[2]) - return ARGV[1] - 1 + return {ARGV[1] - 1, ARGV[2]} end - return redis.call('incrby', KEYS[1], -1) + return {redis.call('incrby', KEYS[1], -1), ttl} ]=]) @@ -84,23 +85,6 @@ function _M.new(plugin_name, limit, window, conf) return setmetatable(self, mt) end -function _M.set_endtime(self,key,time_window) - return time_window -end - -function _M.read_reset(self, key) - local red = self.red_cli - key = self.plugin_name .. tostring(key) - local ttl, err = red:ttl(key) - if err then - return 0 - end - if ttl < 0 then - return 0 - end - - return ttl -end function _M.incoming(self, key) local red = self.red_cli @@ -108,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] + local ttl = res[2] if err then - return nil, err + return nil, err, ttl end if remaining < 0 then - return nil, "rejected" + return nil, "rejected", ttl end - return 0, remaining + return 0, remaining, ttl end diff --git a/apisix/plugins/limit-count/limit-count-redis.lua b/apisix/plugins/limit-count/limit-count-redis.lua index 49756d917e70..73276a294dad 100644 --- a/apisix/plugins/limit-count/limit-count-redis.lua +++ b/apisix/plugins/limit-count/limit-count-redis.lua @@ -30,11 +30,12 @@ local mt = { local script = core.string.compress_script([=[ - if redis.call('ttl', KEYS[1]) < 0 then + local ttl = redis.call('ttl', KEYS[1]) + if ttl < 0 then redis.call('set', KEYS[1], ARGV[1] - 1, 'EX', ARGV[2]) - return ARGV[1] - 1 + return {ARGV[1] - 1, ARGV[2]} end - return redis.call('incrby', KEYS[1], -1) + return {redis.call('incrby', KEYS[1], -1), ttl} ]=]) local function redis_cli(conf) @@ -123,24 +124,26 @@ function _M.incoming(self, key) local limit = self.limit local window = self.window - local remaining + local res key = self.plugin_name .. tostring(key) - remaining, err = red:eval(script, 1, key, limit, window) + res, err = red:eval(script, 1, key, limit, window) + local remaining = res[1] + local ttl = res[2] if err then - return nil, err + return nil, err, ttl end local ok, err = red:set_keepalive(10000, 100) if not ok then - return nil, err + return nil, err, ttl end if remaining < 0 then - return nil, "rejected" + return nil, "rejected", ttl end - return 0, remaining + return 0, remaining, ttl end diff --git a/t/plugin/limit-count-redis3.t b/t/plugin/limit-count-redis3.t index 77c7c2590b33..781f527206c0 100644 --- a/t/plugin/limit-count-redis3.t +++ b/t/plugin/limit-count-redis3.t @@ -121,7 +121,7 @@ Done -=== TEST 3: set route, counter will be shared +=== TEST 3: set router --- config location /t { content_by_lua_block { @@ -133,7 +133,7 @@ Done "plugins": { "limit-count": { "count": 2, - "time_window": 60, + "time_window": 10, "policy": "redis", "redis_host": "127.0.0.1", "redis_port": 6379, From 0feba96dde126b2d83e546e907871aa08dba6c0b Mon Sep 17 00:00:00 2001 From: mscb402 Date: Wed, 4 Jan 2023 14:01:53 +0800 Subject: [PATCH 11/15] chore: format code --- apisix/plugins/limit-count/limit-count-local.lua | 2 +- t/cli/test_main.sh | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/apisix/plugins/limit-count/limit-count-local.lua b/apisix/plugins/limit-count/limit-count-local.lua index 78986687523b..a6503885cd2e 100644 --- a/apisix/plugins/limit-count/limit-count-local.lua +++ b/apisix/plugins/limit-count/limit-count-local.lua @@ -58,7 +58,7 @@ function _M.new(plugin_name, limit, window, conf) end function _M.incoming(self, key, commit, conf) - local delay, remaining = self.limit_count:incoming(key, commit) + local delay, remaining = self.limit_count:incoming(key, commit) local reset = 0 if not delay then return delay, remaining, reset diff --git a/t/cli/test_main.sh b/t/cli/test_main.sh index ae6aa6af5763..f5b78be7f952 100755 --- a/t/cli/test_main.sh +++ b/t/cli/test_main.sh @@ -807,7 +807,6 @@ nginx_config: internal-status: 20m plugin-limit-req: 20m plugin-limit-count: 20m - plugin-limit-count-reset-header: 20m prometheus-metrics: 20m plugin-limit-conn: 20m upstream-healthcheck: 20m @@ -843,11 +842,6 @@ if ! grep "plugin-limit-count 20m;" conf/nginx.conf > /dev/null; then exit 1 fi -if ! grep "plugin-limit-count-reset-header 20m;" conf/nginx.conf > /dev/null; then - echo "failed: 'plugin-limit-count-reset-header 20m;' not in nginx.conf" - exit 1 -fi - if ! grep "prometheus-metrics 20m;" conf/nginx.conf > /dev/null; then echo "failed: 'prometheus-metrics 20m;' not in nginx.conf" exit 1 From 60d0a9a6fe18c0f7ff45fa63540ba20d5ca664aa Mon Sep 17 00:00:00 2001 From: mscb402 Date: Wed, 4 Jan 2023 14:07:58 +0800 Subject: [PATCH 12/15] chore: format code --- .../limit-count/limit-count-redis-cluster.lua | 6 ++-- .../plugins/limit-count/limit-count-redis.lua | 36 +++---------------- 2 files changed, 8 insertions(+), 34 deletions(-) diff --git a/apisix/plugins/limit-count/limit-count-redis-cluster.lua b/apisix/plugins/limit-count/limit-count-redis-cluster.lua index 2916491e75ac..1fa57aac8fb9 100644 --- a/apisix/plugins/limit-count/limit-count-redis-cluster.lua +++ b/apisix/plugins/limit-count/limit-count-redis-cluster.lua @@ -92,14 +92,16 @@ function _M.incoming(self, key) local window = self.window key = self.plugin_name .. tostring(key) + local ttl = 0 local res, err = red:eval(script, 1, key, limit, window) - local remaining = res[1] - local ttl = res[2] if err then return nil, err, ttl end + local remaining = res[1] + ttl = res[2] + if remaining < 0 then return nil, "rejected", ttl end diff --git a/apisix/plugins/limit-count/limit-count-redis.lua b/apisix/plugins/limit-count/limit-count-redis.lua index 73276a294dad..d142d754d728 100644 --- a/apisix/plugins/limit-count/limit-count-redis.lua +++ b/apisix/plugins/limit-count/limit-count-redis.lua @@ -85,36 +85,6 @@ function _M.new(plugin_name, limit, window, conf) return setmetatable(self, mt) end -function _M.set_endtime(self,key,time_window) - return time_window -end - -function _M.read_reset(self, key) - local conf = self.conf - local red, err = redis_cli(conf) - if err then - return red, err - end - - local ttl, err = red:ttl(key) - if err then - core.log.error("key: ", key, " read_reset with error: ", err) - return 0 - end - - local ok, err = red:set_keepalive(10000, 100) - if not ok then - core.log.error("key: ", key, " read_reset with error: ", err) - return 0 - end - - if ttl < 0 then - return 0 - end - - return ttl -end - function _M.incoming(self, key) local conf = self.conf local red, err = redis_cli(conf) @@ -127,14 +97,16 @@ function _M.incoming(self, key) local res key = self.plugin_name .. tostring(key) + local ttl = 0 res, err = red:eval(script, 1, key, limit, window) - local remaining = res[1] - local ttl = res[2] if err then return nil, err, ttl end + local remaining = res[1] + ttl = res[2] + local ok, err = red:set_keepalive(10000, 100) if not ok then return nil, err, ttl From fa40f8d49d7c0c7b45b42167627d8654cd1b06ac Mon Sep 17 00:00:00 2001 From: mscb402 Date: Thu, 5 Jan 2023 17:34:10 +0800 Subject: [PATCH 13/15] chore: format code --- apisix/plugins/limit-count/init.lua | 8 ++++---- apisix/plugins/limit-count/limit-count-redis.lua | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/apisix/plugins/limit-count/init.lua b/apisix/plugins/limit-count/init.lua index 192b0879795a..ce7434a6c82d 100644 --- a/apisix/plugins/limit-count/init.lua +++ b/apisix/plugins/limit-count/init.lua @@ -290,8 +290,8 @@ function _M.rate_limit(conf, ctx) -- 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, - "X-RateLimit-Reset", reset) + "X-RateLimit-Remaining", 0, + "X-RateLimit-Reset", reset) end if conf.rejected_msg then @@ -309,8 +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-Reset", reset) + "X-RateLimit-Remaining", remaining, + "X-RateLimit-Reset", reset) end end diff --git a/apisix/plugins/limit-count/limit-count-redis.lua b/apisix/plugins/limit-count/limit-count-redis.lua index d142d754d728..bed920229b0c 100644 --- a/apisix/plugins/limit-count/limit-count-redis.lua +++ b/apisix/plugins/limit-count/limit-count-redis.lua @@ -88,8 +88,8 @@ end function _M.incoming(self, key) local conf = self.conf local red, err = redis_cli(conf) - if err then - return red, err + if not red then + return red, err, 0 end local limit = self.limit From e548299c470c5cf5dbcda1dc877ea677983001ee Mon Sep 17 00:00:00 2001 From: mscb402 Date: Thu, 5 Jan 2023 17:41:06 +0800 Subject: [PATCH 14/15] chore: add dict error check --- apisix/plugins/limit-count/limit-count-local.lua | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/apisix/plugins/limit-count/limit-count-local.lua b/apisix/plugins/limit-count/limit-count-local.lua index a6503885cd2e..7abba9b416c0 100644 --- a/apisix/plugins/limit-count/limit-count-local.lua +++ b/apisix/plugins/limit-count/limit-count-local.lua @@ -30,7 +30,11 @@ local function set_endtime(self, key, time_window) -- set an end time local end_time = ngx_time() + time_window -- save to dict by key - self.dict:set(key, end_time, time_window) + local success, err = self.dict:set(key, end_time, time_window) + + if not success then + core.log.error("dict set key ", key, " error: ", err) + end local reset = time_window return reset From cae2e6e99df0efc74080ab16d61288428be3b4ac Mon Sep 17 00:00:00 2001 From: mscb402 Date: Thu, 5 Jan 2023 17:44:35 +0800 Subject: [PATCH 15/15] chore: code lint fix --- apisix/plugins/limit-count/limit-count-local.lua | 1 + 1 file changed, 1 insertion(+) diff --git a/apisix/plugins/limit-count/limit-count-local.lua b/apisix/plugins/limit-count/limit-count-local.lua index 7abba9b416c0..37e7a1dbef5b 100644 --- a/apisix/plugins/limit-count/limit-count-local.lua +++ b/apisix/plugins/limit-count/limit-count-local.lua @@ -19,6 +19,7 @@ local ngx = ngx local ngx_time = ngx.time local assert = assert local setmetatable = setmetatable +local core = require("apisix.core") local _M = {}