Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 15 commits into from
Jan 10, 2023
Merged
1 change: 1 addition & 0 deletions apisix/cli/ngx_tpl.lua
Original file line number Diff line number Diff line change
Expand Up @@ -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"] *};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{% end %}

{% if enabled_plugins["prometheus"] and not enabled_stream_plugins["prometheus"] then %}
Expand Down
27 changes: 22 additions & 5 deletions apisix/plugins/limit-count/init.lua
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +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 plugin_name = "limit-count"
local limit_redis_cluster_new
local limit_redis_new
local limit_local_new
do
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

Expand All @@ -39,7 +41,6 @@ local group_conf_lru = core.lrucache.new({
type = 'plugin',
})


local policy_to_additional_properties = {
redis = {
properties = {
Expand Down Expand Up @@ -242,7 +243,6 @@ local function gen_limit_obj(conf, ctx)
return core.lrucache.plugin_ctx(lrucache, ctx, extra_key, create_limit_obj, conf)
end


function _M.rate_limit(conf, ctx)
core.log.info("ver: ", ctx.conf_version)

Expand Down Expand Up @@ -287,6 +287,15 @@ function _M.rate_limit(conf, ctx)
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,
"X-RateLimit-Remaining", 0,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

"X-RateLimit-Reset", reset)
end

if conf.rejected_msg then
return conf.rejected_code, { error_msg = conf.rejected_msg }
end
Expand All @@ -300,9 +309,17 @@ 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)
"X-RateLimit-Remaining", remaining,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

"X-RateLimit-Reset", reset)
end
end

Expand Down
64 changes: 64 additions & 0 deletions apisix/plugins/limit-count/limit-count-local.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
--
-- 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 = {}

local mt = {
__index = _M
}

function _M.set_endtime(self,key,time_window)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, fixed

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to check the err returned from set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed


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
17 changes: 17 additions & 0 deletions apisix/plugins/limit-count/limit-count-redis-cluster.lua
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,23 @@ 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

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
Expand Down
69 changes: 52 additions & 17 deletions apisix/plugins/limit-count/limit-count-redis.lua
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -85,6 +69,57 @@ 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to expose these methods now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, removed

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)
if err then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if err then
if not red then

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

return red, err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

end

local limit = self.limit
local window = self.window
Expand Down
1 change: 1 addition & 0 deletions conf/config-default.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 9 additions & 2 deletions docs/en/latest/plugins/limit-count.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`, 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
Expand All @@ -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
```

Expand All @@ -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."}
Expand Down
11 changes: 9 additions & 2 deletions docs/zh/latest/plugins/limit-count.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
```

Expand All @@ -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."}
Expand Down
1 change: 1 addition & 0 deletions t/APISIX.pm
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
6 changes: 6 additions & 0 deletions t/cli/test_main.sh
Original file line number Diff line number Diff line change
Expand Up @@ -807,6 +807,7 @@ nginx_config:
internal-status: 20m
plugin-limit-req: 20m
plugin-limit-count: 20m
plugin-limit-count-reset-header: 20m
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this setting, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I forget to remove

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I'm removed this. Pls review again

prometheus-metrics: 20m
plugin-limit-conn: 20m
upstream-healthcheck: 20m
Expand Down Expand Up @@ -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
Expand Down
Loading