Xunzhuo commented on a change in pull request #5378:
URL: https://github.com/apache/apisix/pull/5378#discussion_r740749893
##########
File path: t/plugin/limit-count2.t
##########
@@ -178,3 +178,137 @@ GET /hello
--- error_code: 503
--- response_body
{"error_msg":"Requests are too frequent, please try again later."}
+
+
+
+=== TEST 6: set key type to var_combination
+--- 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,
+ "key": "$http_a $http_b",
+ "key_type": "var_combination"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 7: exceed the burst
+--- 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, 4 do
+ local httpc = http.new()
+ local res, err = httpc:request_uri(uri, {headers = {a = 1}})
+ if not res then
+ ngx.say(err)
+ return
+ end
+ table.insert(ress, res.status)
+ end
+ ngx.say(json.encode(ress))
+ }
+ }
+--- request
+GET /t
+--- no_error_log
+[error]
+--- response_body
+[200,200,503,503]
+
+
+
+=== TEST 8: don`t exceed the burst
+--- 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, 2 do
+ local httpc = http.new()
+ local res, err = httpc:request_uri(uri, {headers = {a = 2}})
Review comment:
What's more, TEST 8 use the other key to count
##########
File path: t/plugin/limit-count2.t
##########
@@ -178,3 +178,137 @@ GET /hello
--- error_code: 503
--- response_body
{"error_msg":"Requests are too frequent, please try again later."}
+
+
+
+=== TEST 6: set key type to var_combination
+--- 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,
+ "key": "$http_a $http_b",
+ "key_type": "var_combination"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 7: exceed the burst
+--- 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, 4 do
+ local httpc = http.new()
+ local res, err = httpc:request_uri(uri, {headers = {a = 1}})
+ if not res then
+ ngx.say(err)
+ return
+ end
+ table.insert(ress, res.status)
+ end
+ ngx.say(json.encode(ress))
+ }
+ }
+--- request
+GET /t
+--- no_error_log
+[error]
+--- response_body
+[200,200,503,503]
+
+
+
+=== TEST 8: don`t exceed the burst
+--- 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, 2 do
+ local httpc = http.new()
+ local res, err = httpc:request_uri(uri, {headers = {a = 2}})
Review comment:
@spacewander Yes it is, so should I remove this case or keep it?
Actually I take
[this](https://github.com/apache/apisix/pull/5302/files#diff-7645485edca150830424fd3151413a2eddb93d1dcea7fe694ce458113a6b3bf3)
as an example.
##########
File path: t/plugin/limit-count2.t
##########
@@ -178,3 +178,137 @@ GET /hello
--- error_code: 503
--- response_body
{"error_msg":"Requests are too frequent, please try again later."}
+
+
+
+=== TEST 6: set key type to var_combination
+--- 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,
+ "key": "$http_a $http_b",
+ "key_type": "var_combination"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 7: exceed the burst
+--- 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, 4 do
+ local httpc = http.new()
+ local res, err = httpc:request_uri(uri, {headers = {a = 1}})
+ if not res then
+ ngx.say(err)
+ return
+ end
+ table.insert(ress, res.status)
+ end
+ ngx.say(json.encode(ress))
+ }
+ }
+--- request
+GET /t
+--- no_error_log
+[error]
+--- response_body
+[200,200,503,503]
+
+
+
+=== TEST 8: don`t exceed the burst
+--- 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, 2 do
+ local httpc = http.new()
+ local res, err = httpc:request_uri(uri, {headers = {a = 2}})
Review comment:
@spacewander Yes it is, so should I remove this case or keep it?
Actually I take
[this](https://github.com/apache/apisix/pull/5302/files#diff-7645485edca150830424fd3151413a2eddb93d1dcea7fe694ce458113a6b3bf3)
as an example.
##########
File path: apisix/plugins/limit-count.lua
##########
@@ -36,11 +36,10 @@ local schema = {
properties = {
count = {type = "integer", exclusiveMinimum = 0},
time_window = {type = "integer", exclusiveMinimum = 0},
- key = {
- type = "string",
- enum = {"remote_addr", "server_addr", "http_x_real_ip",
- "http_x_forwarded_for", "consumer_name", "service_id"},
- default = "remote_addr",
+ key = {type = "string", default = "remote_addr"},
+ key_type = {type = "string",
+ enum = {"var", "var_combination"},
Review comment:
Thanks @tokers , if we keep the enum constraints for var like before, it
will be a problem to use var_combination, because var_combination provides a
more flexible way to key selecting.
##########
File path: t/plugin/limit-count2.t
##########
@@ -178,3 +178,137 @@ GET /hello
--- error_code: 503
--- response_body
{"error_msg":"Requests are too frequent, please try again later."}
+
+
+
+=== TEST 6: set key type to var_combination
+--- 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,
+ "key": "$http_a $http_b",
+ "key_type": "var_combination"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 7: exceed the burst
+--- 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, 4 do
+ local httpc = http.new()
+ local res, err = httpc:request_uri(uri, {headers = {a = 1}})
+ if not res then
+ ngx.say(err)
+ return
+ end
+ table.insert(ress, res.status)
+ end
+ ngx.say(json.encode(ress))
+ }
+ }
+--- request
+GET /t
+--- no_error_log
+[error]
+--- response_body
+[200,200,503,503]
+
+
+
+=== TEST 8: don`t exceed the burst
+--- 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, 2 do
+ local httpc = http.new()
+ local res, err = httpc:request_uri(uri, {headers = {a = 2}})
Review comment:
Hi @spacewander , I have updated TEST 8, please take a look, thanks~
##########
File path: docs/en/latest/plugins/limit-count.md
##########
@@ -39,7 +39,8 @@ Limit request rate by a fixed number of requests in a given
time window.
| ------------------- | ------- | --------------------------------------- |
------------- |
-------------------------------------------------------------------------------------------------------
|
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
|
| count | integer | required |
| count > 0
| the specified number of requests
threshold.
|
| time_window | integer | required |
| time_window > 0
| the time window in seconds before the request count
is reset.
|
-| key | string | optional |
"remote_addr" | ["remote_addr", "server_addr", "http_x_real_ip",
"http_x_forwarded_for", "consumer_name", "service_id"] | The user specified key
to limit the count. <br /> Now accept those as key: "remote_addr"(client's IP),
"server_addr"(server's IP), "X-Forwarded-For/X-Real-IP" in request header,
"consumer_name"(consumer's username) and "service_id".
|
+| key_type | string | optional | "var" | ["var",
"var_combination"] | the type of key. |
+| key | string | optional | "remote_addr" | | the user
specified key to limit the rate. If the `key_type` is "var", the key will be
treated as a name of variable, like "remote_addr" or "consumer_name". If the
`key_type` is "var_combination", the key will be a combination of variables,
like "$remote_addr $consumer_name". |
Review comment:
Good point~ Thanks @moonming.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
