You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/12/28 03:41:34 UTC
[GitHub] [apisix] mscb402 opened a new pull request, #8578: feat: limit count plugin support X-RateLimit-Reset
mscb402 opened a new pull request, #8578:
URL: https://github.com/apache/apisix/pull/8578
### Description
limit count plugin support returns a new header `X-RateLimit-Reset` and return the header when Rejected
Fixes #8381
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [x] I have updated the documentation to reflect this change
- [x] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #8578: feat: limit count plugin support X-RateLimit-Reset
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8578:
URL: https://github.com/apache/apisix/pull/8578#discussion_r1058736087
##########
apisix/plugins/limit-count/init.lua:
##########
@@ -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)
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #8578: feat: limit count plugin support X-RateLimit-Reset
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8578:
URL: https://github.com/apache/apisix/pull/8578#discussion_r1058122572
##########
apisix/plugins/limit-count/init.lua:
##########
@@ -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)
Review Comment:
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:
1. 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mscb402 commented on a diff in pull request #8578: feat: limit count plugin support X-RateLimit-Reset
Posted by GitBox <gi...@apache.org>.
mscb402 commented on code in PR #8578:
URL: https://github.com/apache/apisix/pull/8578#discussion_r1059263060
##########
apisix/plugins/limit-count/init.lua:
##########
@@ -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)
Review Comment:
Fixed, you can review it again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mscb402 commented on a diff in pull request #8578: feat: limit count plugin support X-RateLimit-Reset
Posted by GitBox <gi...@apache.org>.
mscb402 commented on code in PR #8578:
URL: https://github.com/apache/apisix/pull/8578#discussion_r1061072202
##########
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"] *};
Review Comment:
I removed `plugin-limit-count-reset-header` in config
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #8578: feat: limit count plugin support X-RateLimit-Reset
Posted by GitBox <gi...@apache.org>.
spacewander merged PR #8578:
URL: https://github.com/apache/apisix/pull/8578
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mscb402 commented on a diff in pull request #8578: feat: limit count plugin support X-RateLimit-Reset
Posted by GitBox <gi...@apache.org>.
mscb402 commented on code in PR #8578:
URL: https://github.com/apache/apisix/pull/8578#discussion_r1061115855
##########
apisix/plugins/limit-count/limit-count-local.lua:
##########
@@ -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)
Review Comment:
ok, fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mscb402 commented on a diff in pull request #8578: feat: limit count plugin support X-RateLimit-Reset
Posted by GitBox <gi...@apache.org>.
mscb402 commented on code in PR #8578:
URL: https://github.com/apache/apisix/pull/8578#discussion_r1061115948
##########
apisix/plugins/limit-count/limit-count-redis-cluster.lua:
##########
@@ -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)
Review Comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mscb402 commented on a diff in pull request #8578: feat: limit count plugin support X-RateLimit-Reset
Posted by GitBox <gi...@apache.org>.
mscb402 commented on code in PR #8578:
URL: https://github.com/apache/apisix/pull/8578#discussion_r1062281134
##########
apisix/plugins/limit-count/limit-count-local.lua:
##########
@@ -0,0 +1,76 @@
+--
+-- 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
+}
+
+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)
Review Comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mscb402 commented on a diff in pull request #8578: feat: limit count plugin support X-RateLimit-Reset
Posted by GitBox <gi...@apache.org>.
mscb402 commented on code in PR #8578:
URL: https://github.com/apache/apisix/pull/8578#discussion_r1058140196
##########
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()
+ return {
+ endtime=0,
+ }
+ end
+
+ return resetlru(key,"",create,conf)
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #8578: feat: limit count plugin support X-RateLimit-Reset
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8578:
URL: https://github.com/apache/apisix/pull/8578#discussion_r1060308412
##########
apisix/plugins/limit-count/limit-count-local.lua:
##########
@@ -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)
Review Comment:
Look like we can merge `set_endtime` into the `incoming` method, especially after implementing `read_reset` via eval?
##########
apisix/plugins/limit-count/limit-count-redis-cluster.lua:
##########
@@ -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)
Review Comment:
Can we put this into the eval script to make it atomic and reduce roundtrip?
##########
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"] *};
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mscb402 commented on a diff in pull request #8578: feat: limit count plugin support X-RateLimit-Reset
Posted by GitBox <gi...@apache.org>.
mscb402 commented on code in PR #8578:
URL: https://github.com/apache/apisix/pull/8578#discussion_r1061157915
##########
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
Review Comment:
Yes, I forget to remove
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #8578: feat: limit count plugin support X-RateLimit-Reset
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8578:
URL: https://github.com/apache/apisix/pull/8578#discussion_r1058071416
##########
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()
+ return {
+ endtime=0,
+ }
+ end
+
+ return resetlru(key,"",create,conf)
Review Comment:
Don't forget to add space around the operator
##########
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({
Review Comment:
```suggestion
local reset_lru = core.lrucache.new({
```
##########
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()
Review Comment:
Please avoid creating new function per call
##########
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`:
Review Comment:
Why the English version doesn't match the Chinese one?
##########
apisix/plugins/limit-count/init.lua:
##########
@@ -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)
Review Comment:
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:
1. 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.
Personally, I prefer solution 1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mscb402 commented on a diff in pull request #8578: feat: limit count plugin support X-RateLimit-Reset
Posted by GitBox <gi...@apache.org>.
mscb402 commented on code in PR #8578:
URL: https://github.com/apache/apisix/pull/8578#discussion_r1058185434
##########
apisix/plugins/limit-count/init.lua:
##########
@@ -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)
Review Comment:
I'm using `ngx.shared` with expire time instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mscb402 commented on a diff in pull request #8578: feat: limit count plugin support X-RateLimit-Reset
Posted by GitBox <gi...@apache.org>.
mscb402 commented on code in PR #8578:
URL: https://github.com/apache/apisix/pull/8578#discussion_r1058140047
##########
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`:
Review Comment:
fixed
##########
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()
Review Comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mscb402 commented on a diff in pull request #8578: feat: limit count plugin support X-RateLimit-Reset
Posted by GitBox <gi...@apache.org>.
mscb402 commented on code in PR #8578:
URL: https://github.com/apache/apisix/pull/8578#discussion_r1058144301
##########
apisix/plugins/limit-count/init.lua:
##########
@@ -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)
Review Comment:
By default, There use `resty.limit.count` this package for limit traffic. And I read the code of Resty Limit Count package using `ngx.shared.DICT` to save data. Can I use this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mscb402 commented on a diff in pull request #8578: feat: limit count plugin support X-RateLimit-Reset
Posted by GitBox <gi...@apache.org>.
mscb402 commented on code in PR #8578:
URL: https://github.com/apache/apisix/pull/8578#discussion_r1058144301
##########
apisix/plugins/limit-count/init.lua:
##########
@@ -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)
Review Comment:
By default, There use `resty.limit.count` this package for limit traffic. And I read the code of Resty Limit Count package using `ngx.shared.DICT` to save data. Can I use this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #8578: feat: limit count plugin support X-RateLimit-Reset
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8578:
URL: https://github.com/apache/apisix/pull/8578#discussion_r1061153877
##########
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]
Review Comment:
Please use the res after checking the err
##########
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
Review Comment:
We don't need this setting, right?
##########
apisix/plugins/limit-count/limit-count-local.lua:
##########
@@ -0,0 +1,76 @@
+--
+-- 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
+}
+
+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 reset = time_window
+ return reset
+end
+
+local function 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, conf)
+ local delay, remaining = self.limit_count:incoming(key, commit)
Review Comment:
```suggestion
local delay, remaining = self.limit_count:incoming(key, commit)
```
##########
apisix/plugins/limit-count/limit-count-redis.lua:
##########
@@ -85,27 +70,80 @@ 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)
Review Comment:
We don't need to expose these methods now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mscb402 commented on a diff in pull request #8578: feat: limit count plugin support X-RateLimit-Reset
Posted by GitBox <gi...@apache.org>.
mscb402 commented on code in PR #8578:
URL: https://github.com/apache/apisix/pull/8578#discussion_r1061160176
##########
apisix/plugins/limit-count/limit-count-redis.lua:
##########
@@ -85,27 +70,80 @@ 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)
Review Comment:
yes, removed
##########
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]
Review Comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mscb402 commented on a diff in pull request #8578: feat: limit count plugin support X-RateLimit-Reset
Posted by GitBox <gi...@apache.org>.
mscb402 commented on code in PR #8578:
URL: https://github.com/apache/apisix/pull/8578#discussion_r1058141023
##########
apisix/plugins/limit-count/init.lua:
##########
@@ -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)
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #8578: feat: limit count plugin support X-RateLimit-Reset
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8578:
URL: https://github.com/apache/apisix/pull/8578#discussion_r1062134953
##########
apisix/plugins/limit-count/init.lua:
##########
@@ -283,10 +283,17 @@ 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
+ -- 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,
Review Comment:
Ditto
##########
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,
Review Comment:
Why change 4-spaces indentation to 8-spaces here?
##########
apisix/plugins/limit-count/limit-count-local.lua:
##########
@@ -0,0 +1,76 @@
+--
+-- 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
+}
+
+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)
Review Comment:
Better to check the err returned from `set`.
##########
apisix/plugins/limit-count/limit-count-redis.lua:
##########
@@ -85,27 +70,52 @@ 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.incoming(self, key)
+ local conf = self.conf
+ local red, err = redis_cli(conf)
+ if err then
Review Comment:
```suggestion
if not red then
```
##########
apisix/plugins/limit-count/limit-count-redis.lua:
##########
@@ -85,27 +70,52 @@ 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.incoming(self, key)
+ local conf = self.conf
+ local red, err = redis_cli(conf)
+ if err then
+ return red, err
Review Comment:
```suggestion
return nil, err, 0
```
Please follow the code style: https://github.com/apache/apisix/blob/master/CODE_STYLE.md#error-handling
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mscb402 commented on a diff in pull request #8578: feat: limit count plugin support X-RateLimit-Reset
Posted by GitBox <gi...@apache.org>.
mscb402 commented on code in PR #8578:
URL: https://github.com/apache/apisix/pull/8578#discussion_r1062274841
##########
apisix/plugins/limit-count/limit-count-redis.lua:
##########
@@ -85,27 +70,52 @@ 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.incoming(self, key)
+ local conf = self.conf
+ local red, err = redis_cli(conf)
+ if err then
+ return red, err
Review Comment:
fixed
##########
apisix/plugins/limit-count/limit-count-redis.lua:
##########
@@ -85,27 +70,52 @@ 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.incoming(self, key)
+ local conf = self.conf
+ local red, err = redis_cli(conf)
+ if err then
Review Comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mscb402 commented on a diff in pull request #8578: feat: limit count plugin support X-RateLimit-Reset
Posted by GitBox <gi...@apache.org>.
mscb402 commented on code in PR #8578:
URL: https://github.com/apache/apisix/pull/8578#discussion_r1062275120
##########
apisix/plugins/limit-count/init.lua:
##########
@@ -283,10 +283,17 @@ 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
+ -- 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,
Review Comment:
fixed
##########
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,
Review Comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mscb402 commented on a diff in pull request #8578: feat: limit count plugin support X-RateLimit-Reset
Posted by GitBox <gi...@apache.org>.
mscb402 commented on code in PR #8578:
URL: https://github.com/apache/apisix/pull/8578#discussion_r1061158028
##########
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
Review Comment:
Now I'm removed this. Pls review again
##########
apisix/plugins/limit-count/limit-count-local.lua:
##########
@@ -0,0 +1,76 @@
+--
+-- 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
+}
+
+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 reset = time_window
+ return reset
+end
+
+local function 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, conf)
+ local delay, remaining = self.limit_count:incoming(key, commit)
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org