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 2021/12/22 06:05:49 UTC
[GitHub] [apisix] spacewander opened a new pull request #5881: feat(limit-count): allow sharing counter
spacewander opened a new pull request #5881:
URL: https://github.com/apache/apisix/pull/5881
Signed-off-by: spacewander <sp...@gmail.com>
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
Fix #5342
### Pre-submission checklist:
<!--
Please follow the requirements:
1. Use Draft if the PR is not ready to be reviewed
2. Test is required for the feat/fix PR, unless you have a good reason
3. Doc is required for the feat PR
4. Use a new commit to resolve review instead of `push -f`
5. If you need to resolve merge conflicts after the PR is reviewed, please merge master but do not rebase
6. Use "request review" to notify the reviewer once you have resolved the review
-->
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [x] Have you added corresponding test cases?
* [x] Have you modified the corresponding document?
* [ ] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix/tree/master#community) first**
--
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 change in pull request #5881: feat(limit-count): allow sharing counter
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5881:
URL: https://github.com/apache/apisix/pull/5881#discussion_r774830359
##########
File path: apisix/plugins/limit-count.lua
##########
@@ -192,7 +248,12 @@ function _M.access(conf, ctx)
key = ctx.var["remote_addr"]
end
- key = key .. ctx.conf_type .. ctx.conf_version
+ if not conf.group then
+ key = key .. ctx.conf_type .. ctx.conf_version
+ else
+ key = key .. conf.group
Review comment:
Yes, the previous limit-count is two-dimension:
per-route x per-key
This PR allows to use:
group x per-key
If we want to limit only according to the group, we can use a constant variable, like hostname, so the per-key will be always a single point.
In the next PR, I will introduce a new key type that is equal to a constant variable, so we don't need a special configuration for the key part.
--
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 change in pull request #5881: feat(limit-count): allow sharing counter
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5881:
URL: https://github.com/apache/apisix/pull/5881#discussion_r774500765
##########
File path: t/plugin/limit-count2.t
##########
@@ -480,11 +448,240 @@ passed
ngx.say(json.encode(ress))
}
}
---- request
-GET /t
---- no_error_log
-[error]
--- response_body
[200,200,503,503]
--- error_log
The value of the configured key is empty, use client IP instead
+
+
+
+=== TEST 15: limit count in group
+--- 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,
+ [[{
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 60,
+ "rejected_code": 503,
+ "group": "services_1"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ return
+ end
+
+ local code, body = t('/apisix/admin/routes/2',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 60,
+ "rejected_code": 503,
+ "group": "services_1"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello_chunked"
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ return
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 16: hit multiple paths
+--- config
+ location /t {
+ content_by_lua_block {
+ local json = require "t.toolkit.json"
+ local http = require "resty.http"
+ local uri1 = "http://127.0.0.1:" .. ngx.var.server_port
+ .. "/hello"
+ local uri2 = "http://127.0.0.1:" .. ngx.var.server_port
+ .. "/hello_chunked"
+ local ress = {}
+ for i = 1, 4 do
+ local httpc = http.new()
+ local uri
+ if i % 2 == 1 then
+ uri = uri1
+ else
+ uri = uri2
+ end
+
+ local res, err = httpc:request_uri(uri)
+ if not res then
+ ngx.say(err)
+ return
+ end
+ table.insert(ress, res.status)
+ end
+ ngx.say(json.encode(ress))
+ }
+ }
+--- response_body
+[200,200,503,503]
+
+
+
+=== TEST 17: limit count in group, configuration is from services
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/services/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 60,
+ "rejected_code": 503,
+ "group": "afafafhao"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ return
+ end
+
+
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "service_id": "1",
+ "uri": "/hello"
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ return
+ end
+ local code, body = t('/apisix/admin/routes/2',
+ ngx.HTTP_PUT,
+ [[{
+ "service_id": "1",
+ "uri": "/hello_chunked"
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ return
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 18: hit multiple paths
+--- config
+ location /t {
+ content_by_lua_block {
+ local json = require "t.toolkit.json"
+ local http = require "resty.http"
+ local uri1 = "http://127.0.0.1:" .. ngx.var.server_port
+ .. "/hello"
+ local uri2 = "http://127.0.0.1:" .. ngx.var.server_port
+ .. "/hello_chunked"
+ local ress = {}
+ for i = 1, 4 do
+ local httpc = http.new()
+ local uri
+ if i % 2 == 1 then
+ uri = uri1
+ else
+ uri = uri2
+ end
+
+ local res, err = httpc:request_uri(uri)
+ if not res then
+ ngx.say(err)
+ return
+ end
+ table.insert(ress, res.status)
+ end
+ ngx.say(json.encode(ress))
+ }
+ }
+--- response_body
+[200,200,503,503]
+
+
+
+=== TEST 19: configuration from the same group should be the same
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/services/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "limit-count": {
+ "count": 1,
+ "time_window": 60,
+ "rejected_code": 503,
+ "group": "afafafhao"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.print(body)
+ }
+ }
+--- error_code: 400
+--- error_log
+[error]
+--- response_body
+{"error_msg":"failed to check the configuration of plugin limit-count err: group conf mismatched"}
Review comment:
We need to update the group too. Otherwise, the same group will have two different configuration.
--
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 #5881: feat(limit-count): allow sharing counter
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #5881:
URL: https://github.com/apache/apisix/pull/5881
--
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 change in pull request #5881: feat(limit-count): allow sharing counter
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5881:
URL: https://github.com/apache/apisix/pull/5881#discussion_r774835603
##########
File path: apisix/plugins/limit-count.lua
##########
@@ -192,7 +248,12 @@ function _M.access(conf, ctx)
key = ctx.var["remote_addr"]
end
- key = key .. ctx.conf_type .. ctx.conf_version
+ if not conf.group then
+ key = key .. ctx.conf_type .. ctx.conf_version
+ else
+ key = key .. conf.group
Review comment:
Look like the hostname is not constant if the cluster limit-count is used. Anyway, we can solve it in the next PR.
--
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 change in pull request #5881: feat(limit-count): allow sharing counter
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5881:
URL: https://github.com/apache/apisix/pull/5881#discussion_r774499913
##########
File path: docs/zh/latest/plugins/limit-count.md
##########
@@ -112,6 +113,54 @@ curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335
你也可以通过 web 界面来完成上面的操作，先增加一个 route，然后在插件页面中添加 limit-count 插件：
+我们也支持在多个 Route 间共享同样的限流计数器。举个例子，
+
+```
+curl -i http://127.0.0.1:9080/apisix/admin/services/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+{
+ "plugins": {
+ "limit-count": {
+ "count": 1,
+ "time_window": 60,
+ "rejected_code": 503,
+ "key": "remote_addr",
+ "group": "services_1#1640140620"
+ }
+ },
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": {
+ "127.0.0.1:1980": 1
+ }
+ }
+}'
+```
+
+每个配置了 `group` 为 `services_1#1640140620` 的 Route 都将共享同一个每个 IP 地址只能访问两次的计数器。
Review comment:
Good catch!
--
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] gxthrj commented on a change in pull request #5881: feat(limit-count): allow sharing counter
Posted by GitBox <gi...@apache.org>.
gxthrj commented on a change in pull request #5881:
URL: https://github.com/apache/apisix/pull/5881#discussion_r774587145
##########
File path: apisix/plugins/limit-count.lua
##########
@@ -192,7 +248,12 @@ function _M.access(conf, ctx)
key = ctx.var["remote_addr"]
end
- key = key .. ctx.conf_type .. ctx.conf_version
+ if not conf.group then
+ key = key .. ctx.conf_type .. ctx.conf_version
+ else
+ key = key .. conf.group
Review comment:
So traffic will eventually be limited according to the key generated by the combination of key and group.
But `key` can not be empty (if `key` is empty, `remote_addr` will be set as the default key),
so it is never possible to limit the rate only according to the `group`.
--
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] tzssangglass commented on a change in pull request #5881: feat(limit-count): allow sharing counter
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on a change in pull request #5881:
URL: https://github.com/apache/apisix/pull/5881#discussion_r774361920
##########
File path: t/plugin/limit-count2.t
##########
@@ -480,11 +448,240 @@ passed
ngx.say(json.encode(ress))
}
}
---- request
-GET /t
---- no_error_log
-[error]
--- response_body
[200,200,503,503]
--- error_log
The value of the configured key is empty, use client IP instead
+
+
+
+=== TEST 15: limit count in group
+--- 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,
+ [[{
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 60,
+ "rejected_code": 503,
+ "group": "services_1"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ return
+ end
+
+ local code, body = t('/apisix/admin/routes/2',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 60,
+ "rejected_code": 503,
+ "group": "services_1"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello_chunked"
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ return
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 16: hit multiple paths
+--- config
+ location /t {
+ content_by_lua_block {
+ local json = require "t.toolkit.json"
+ local http = require "resty.http"
+ local uri1 = "http://127.0.0.1:" .. ngx.var.server_port
+ .. "/hello"
+ local uri2 = "http://127.0.0.1:" .. ngx.var.server_port
+ .. "/hello_chunked"
+ local ress = {}
+ for i = 1, 4 do
+ local httpc = http.new()
+ local uri
+ if i % 2 == 1 then
+ uri = uri1
+ else
+ uri = uri2
+ end
+
+ local res, err = httpc:request_uri(uri)
+ if not res then
+ ngx.say(err)
+ return
+ end
+ table.insert(ress, res.status)
+ end
+ ngx.say(json.encode(ress))
+ }
+ }
+--- response_body
+[200,200,503,503]
+
+
+
+=== TEST 17: limit count in group, configuration is from services
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/services/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 60,
+ "rejected_code": 503,
+ "group": "afafafhao"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ return
+ end
+
+
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "service_id": "1",
+ "uri": "/hello"
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ return
+ end
+ local code, body = t('/apisix/admin/routes/2',
+ ngx.HTTP_PUT,
+ [[{
+ "service_id": "1",
+ "uri": "/hello_chunked"
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ return
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 18: hit multiple paths
+--- config
+ location /t {
+ content_by_lua_block {
+ local json = require "t.toolkit.json"
+ local http = require "resty.http"
+ local uri1 = "http://127.0.0.1:" .. ngx.var.server_port
+ .. "/hello"
+ local uri2 = "http://127.0.0.1:" .. ngx.var.server_port
+ .. "/hello_chunked"
+ local ress = {}
+ for i = 1, 4 do
+ local httpc = http.new()
+ local uri
+ if i % 2 == 1 then
+ uri = uri1
+ else
+ uri = uri2
+ end
+
+ local res, err = httpc:request_uri(uri)
+ if not res then
+ ngx.say(err)
+ return
+ end
+ table.insert(ress, res.status)
+ end
+ ngx.say(json.encode(ress))
+ }
+ }
+--- response_body
+[200,200,503,503]
+
+
+
+=== TEST 19: configuration from the same group should be the same
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/services/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "limit-count": {
+ "count": 1,
+ "time_window": 60,
+ "rejected_code": 503,
+ "group": "afafafhao"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.print(body)
+ }
+ }
+--- error_code: 400
+--- error_log
+[error]
+--- response_body
+{"error_msg":"failed to check the configuration of plugin limit-count err: group conf mismatched"}
Review comment:
How can we update the limit-count config?
##########
File path: docs/zh/latest/plugins/limit-count.md
##########
@@ -112,6 +113,54 @@ curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335
你也可以通过 web 界面来完成上面的操作，先增加一个 route，然后在插件页面中添加 limit-count 插件：
+我们也支持在多个 Route 间共享同样的限流计数器。举个例子，
Review comment:
```suggestion
我们也支持在多个 Route 间共享同一个限流计数器。举个例子，
```
When different limit-count plugins have the same group, it means that these plugins share a unique limit counter and requests passing through them will increment on this counter.
I don't know if I'm wrong.
--
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] leslie-tsang commented on a change in pull request #5881: feat(limit-count): allow sharing counter
Posted by GitBox <gi...@apache.org>.
leslie-tsang commented on a change in pull request #5881:
URL: https://github.com/apache/apisix/pull/5881#discussion_r774437880
##########
File path: docs/zh/latest/plugins/limit-count.md
##########
@@ -112,6 +113,54 @@ curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335
你也可以通过 web 界面来完成上面的操作，先增加一个 route，然后在插件页面中添加 limit-count 插件：
+我们也支持在多个 Route 间共享同样的限流计数器。举个例子，
+
+```
+curl -i http://127.0.0.1:9080/apisix/admin/services/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+{
+ "plugins": {
+ "limit-count": {
+ "count": 1,
+ "time_window": 60,
+ "rejected_code": 503,
+ "key": "remote_addr",
+ "group": "services_1#1640140620"
+ }
+ },
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": {
+ "127.0.0.1:1980": 1
+ }
+ }
+}'
+```
+
+每个配置了 `group` 为 `services_1#1640140620` 的 Route 都将共享同一个每个 IP 地址只能访问两次的计数器。
Review comment:
```suggestion
每个配置了 `group` 为 `services_1#1640140620` 的 Route 都将共享同一个每个 IP 地址每分钟只能访问一次的计数器。
```
Would be better ?
I'm confused about the count attr of limit-count.
I don't know if I'm wrong.
--
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 change in pull request #5881: feat(limit-count): allow sharing counter
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5881:
URL: https://github.com/apache/apisix/pull/5881#discussion_r774500877
##########
File path: docs/zh/latest/plugins/limit-count.md
##########
@@ -112,6 +113,54 @@ curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335
你也可以通过 web 界面来完成上面的操作，先增加一个 route，然后在插件页面中添加 limit-count 插件：
+我们也支持在多个 Route 间共享同样的限流计数器。举个例子，
Review comment:
Yes, you are right.
--
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