moonming commented on a change in pull request #2270: URL: https://github.com/apache/apisix/pull/2270#discussion_r492793961
########## File path: doc/plugins/limit-req.md ########## @@ -37,7 +40,7 @@ limit request rate using the "leaky bucket" method. |--------- |--------|-----------| |rate |required|is the specified request rate (number per second) threshold. Requests exceeding this rate (and below `burst`) will get delayed to conform to the rate.| |burst |required|is the number of excessive requests per second allowed to be delayed. Requests exceeding this hard limit will get rejected immediately.| -| key |required|is the user specified key to limit the rate, now accept those as key: "remote_addr"(client's IP), "server_addr"(server's IP), "X-Forwarded-For/X-Real-IP" in request header.| +| key |required|is the user specified key to limit the rate, now accept those as key: "remote_addr"(client's IP), "server_addr"(server's IP), "X-Forwarded-For", "X-Real-IP/consumer_name(consumer's ID)" in request header.| Review comment: Is consumer_name or consumer_id? I'm confused. ########## File path: t/plugin/limit-req.t ########## @@ -432,3 +432,367 @@ GET /t passed --- no_error_log [error] + + + +=== TEST 12: consumer binds the limit-req plugin and `key` is `consumer_name` +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ + "username": "consumer_limit_req", + "plugins": { + "key-auth": { + "key": "auth-jack" + }, + "limit-req": { + "rate": 2, + "burst": 1, + "rejected_code": 403, + "key": "consumer_name" + } + } + }]], + [[{ + "node": { + "value": { + "username": "consumer_limit_req", + "plugins": { + "key-auth": { + "key": "auth-jack" + }, + "limit-req": { + "rate": 2, + "burst": 1, + "rejected_code": 403, + "key": "consumer_name" + } + } + } + }, + "action": "set" + }]] + ) + + ngx.status = code + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed +--- no_error_log +[error] + + + +=== TEST 13: route add "key-auth" plugin +--- 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": { + "key-auth": {} + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + }, + "desc": "上游节点", Review comment: Why Chinese? ########## File path: apisix/plugins/limit-req.lua ########## @@ -67,7 +67,12 @@ function _M.access(conf, ctx) return 500 end - local key = (ctx.var[conf.key] or "") .. ctx.conf_type .. ctx.conf_version + local key + if conf.key == "consumer_name" then + key = (ctx.consumer_id or "") .. ctx.conf_type .. ctx.conf_version Review comment: We need return error if consumer id not exist ########## File path: doc/plugins/limit-req.md ########## @@ -37,7 +40,7 @@ limit request rate using the "leaky bucket" method. |--------- |--------|-----------| |rate |required|is the specified request rate (number per second) threshold. Requests exceeding this rate (and below `burst`) will get delayed to conform to the rate.| |burst |required|is the number of excessive requests per second allowed to be delayed. Requests exceeding this hard limit will get rejected immediately.| -| key |required|is the user specified key to limit the rate, now accept those as key: "remote_addr"(client's IP), "server_addr"(server's IP), "X-Forwarded-For/X-Real-IP" in request header.| +| key |required|is the user specified key to limit the rate, now accept those as key: "remote_addr"(client's IP), "server_addr"(server's IP), "X-Forwarded-For", "X-Real-IP/consumer_name(consumer's ID)" in request header.| Review comment: Is consumer name in request header? ########## File path: t/plugin/limit-req.t ########## @@ -432,3 +432,367 @@ GET /t passed --- no_error_log [error] + + + +=== TEST 12: consumer binds the limit-req plugin and `key` is `consumer_name` +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ + "username": "consumer_limit_req", + "plugins": { + "key-auth": { + "key": "auth-jack" + }, + "limit-req": { + "rate": 2, + "burst": 1, + "rejected_code": 403, + "key": "consumer_name" + } + } + }]], + [[{ + "node": { + "value": { + "username": "consumer_limit_req", + "plugins": { + "key-auth": { + "key": "auth-jack" + }, + "limit-req": { + "rate": 2, + "burst": 1, + "rejected_code": 403, + "key": "consumer_name" + } + } + } + }, + "action": "set" + }]] + ) + + ngx.status = code + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed +--- no_error_log +[error] + + + +=== TEST 13: route add "key-auth" plugin +--- 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": { + "key-auth": {} + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + }, + "desc": "上游节点", + "uri": "/hello" + }]], + [[{ + "node": { + "value": { + "plugins": { + "key-auth": {} + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + }, + "desc": "上游节点", + "uri": "/hello" + }, + "key": "/apisix/routes/1" + }, + "action": "set" + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed +--- no_error_log +[error] + + + +=== TEST 14: not exceeding the burst +--- pipelined_requests eval +["GET /hello", "GET /hello", "GET /hello"] +--- more_headers +apikey: auth-jack +--- error_code eval +[200, 200, 200] +--- no_error_log +[error] + + + +=== TEST 15: update the limit-req plugin +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ + "username": "consumer_limit_req", + "plugins": { + "key-auth": { + "key": "auth-jack" + }, + "limit-req": { + "rate": 0.1, + "burst": 0.1, + "rejected_code": 403, + "key": "consumer_name" + } + } + }]], + [[{ + "node": { + "value": { + "username": "consumer_limit_req", + "plugins": { + "key-auth": { + "key": "auth-jack" + }, + "limit-req": { + "rate": 0.1, + "burst": 0.1, + "rejected_code": 403, + "key": "consumer_name" + } + } + } + }, + "action": "set" + }]] + ) + + ngx.status = code + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed +--- no_error_log +[error] + + + +=== TEST 16: exceeding the burst +--- pipelined_requests eval +["GET /hello", "GET /hello", "GET /hello", "GET /hello"] +--- more_headers +apikey: auth-jack +--- error_code eval +[403, 403, 403, 403] +--- no_error_log +[error] + + + +=== TEST 17: consumer binds the limit-req plugin and `key` is `remote_addr` Review comment: Are the following test cases related to this pr? ########## File path: doc/plugins/limit-req.md ########## @@ -104,6 +107,78 @@ Server: APISIX web server This means that the limit req plugin is in effect. +## How to enable on the `consumer` + +To enable the `limit-req` plugin on the consumer, it needs to be used together with the authorization plugin. Here, the key-auth authorization plugin is taken as an example. + +1. Bind the `limit-req` plugin to the consumer + +```shell +curl http://127.0.0.1:9080/apisix/admin/consumers -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d ' +{ + "username": "limit_req_consumer_name", Review comment: We need a real name! ########## File path: doc/plugins/limit-req.md ########## @@ -104,6 +107,78 @@ Server: APISIX web server This means that the limit req plugin is in effect. +## How to enable on the `consumer` + +To enable the `limit-req` plugin on the consumer, it needs to be used together with the authorization plugin. Here, the key-auth authorization plugin is taken as an example. + +1. Bind the `limit-req` plugin to the consumer + +```shell +curl http://127.0.0.1:9080/apisix/admin/consumers -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d ' +{ + "username": "limit_req_consumer_name", + "plugins": { + "key-auth": { + "key": "auth-jack" Review comment: Ditto ########## File path: doc/plugins/limit-req.md ########## @@ -37,7 +40,7 @@ limit request rate using the "leaky bucket" method. |--------- |--------|-----------| |rate |required|is the specified request rate (number per second) threshold. Requests exceeding this rate (and below `burst`) will get delayed to conform to the rate.| |burst |required|is the number of excessive requests per second allowed to be delayed. Requests exceeding this hard limit will get rejected immediately.| -| key |required|is the user specified key to limit the rate, now accept those as key: "remote_addr"(client's IP), "server_addr"(server's IP), "X-Forwarded-For/X-Real-IP" in request header.| +| key |required|is the user specified key to limit the rate, now accept those as key: "remote_addr"(client's IP), "server_addr"(server's IP), "X-Forwarded-For", "X-Real-IP/consumer_name(consumer's ID)" in request header.| Review comment: If they are the same, why use two different names? Is it id or name? ########## File path: apisix/plugins/limit-req.lua ########## @@ -67,7 +67,12 @@ function _M.access(conf, ctx) return 500 end - local key = (ctx.var[conf.key] or "") .. ctx.conf_type .. ctx.conf_version + local key + if conf.key == "consumer_name" then + key = (ctx.consumer_id or "") .. ctx.conf_type .. ctx.conf_version Review comment: NO. Error handling is necessary ########## File path: apisix/plugins/limit-req.lua ########## @@ -67,7 +67,12 @@ function _M.access(conf, ctx) return 500 end - local key = (ctx.var[conf.key] or "") .. ctx.conf_type .. ctx.conf_version + local key + if conf.key == "consumer_name" then + key = (ctx.consumer_id or "") .. ctx.conf_type .. ctx.conf_version Review comment: and test cases are also required ########## File path: t/plugin/limit-req.t ########## @@ -432,3 +432,367 @@ GET /t passed --- no_error_log [error] + + + +=== TEST 12: consumer binds the limit-req plugin and `key` is `consumer_name` +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ + "username": "consumer_limit_req", + "plugins": { + "key-auth": { + "key": "auth-jack" + }, + "limit-req": { + "rate": 2, + "burst": 1, + "rejected_code": 403, + "key": "consumer_name" + } + } + }]], + [[{ + "node": { + "value": { + "username": "consumer_limit_req", + "plugins": { + "key-auth": { + "key": "auth-jack" + }, + "limit-req": { + "rate": 2, + "burst": 1, + "rejected_code": 403, + "key": "consumer_name" + } + } + } + }, + "action": "set" + }]] + ) + + ngx.status = code + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed +--- no_error_log +[error] + + + +=== TEST 13: route add "key-auth" plugin +--- 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": { + "key-auth": {} + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + }, + "desc": "上游节点", + "uri": "/hello" + }]], + [[{ + "node": { + "value": { + "plugins": { + "key-auth": {} + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + }, + "desc": "上游节点", + "uri": "/hello" + }, + "key": "/apisix/routes/1" + }, + "action": "set" + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed +--- no_error_log +[error] + + + +=== TEST 14: not exceeding the burst +--- pipelined_requests eval +["GET /hello", "GET /hello", "GET /hello"] +--- more_headers +apikey: auth-jack +--- error_code eval +[200, 200, 200] +--- no_error_log +[error] + + + +=== TEST 15: update the limit-req plugin +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ + "username": "consumer_limit_req", + "plugins": { + "key-auth": { + "key": "auth-jack" + }, + "limit-req": { + "rate": 0.1, + "burst": 0.1, + "rejected_code": 403, + "key": "consumer_name" + } + } + }]], + [[{ + "node": { + "value": { + "username": "consumer_limit_req", + "plugins": { + "key-auth": { + "key": "auth-jack" + }, + "limit-req": { + "rate": 0.1, + "burst": 0.1, + "rejected_code": 403, + "key": "consumer_name" + } + } + } + }, + "action": "set" + }]] + ) + + ngx.status = code + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed +--- no_error_log +[error] + + + +=== TEST 16: exceeding the burst +--- pipelined_requests eval +["GET /hello", "GET /hello", "GET /hello", "GET /hello"] +--- more_headers +apikey: auth-jack +--- error_code eval +[403, 403, 403, 403] +--- no_error_log +[error] + + + +=== TEST 17: consumer binds the limit-req plugin and `key` is `remote_addr` Review comment: Please remove the code not related to pr ########## File path: t/plugin/limit-req.t ########## @@ -432,3 +432,367 @@ GET /t passed --- no_error_log [error] + + + +=== TEST 12: consumer binds the limit-req plugin and `key` is `consumer_name` +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ + "username": "consumer_limit_req", + "plugins": { + "key-auth": { + "key": "auth-jack" + }, + "limit-req": { + "rate": 2, + "burst": 1, + "rejected_code": 403, + "key": "consumer_name" + } + } + }]], + [[{ + "node": { + "value": { + "username": "consumer_limit_req", + "plugins": { + "key-auth": { + "key": "auth-jack" + }, + "limit-req": { + "rate": 2, + "burst": 1, + "rejected_code": 403, + "key": "consumer_name" + } + } + } + }, + "action": "set" + }]] + ) + + ngx.status = code + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed +--- no_error_log +[error] + + + +=== TEST 13: route add "key-auth" plugin +--- 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": { + "key-auth": {} + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + }, + "desc": "上游节点", + "uri": "/hello" + }]], + [[{ + "node": { + "value": { + "plugins": { + "key-auth": {} + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + }, + "desc": "上游节点", + "uri": "/hello" + }, + "key": "/apisix/routes/1" + }, + "action": "set" + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed +--- no_error_log +[error] + + + +=== TEST 14: not exceeding the burst +--- pipelined_requests eval +["GET /hello", "GET /hello", "GET /hello"] +--- more_headers +apikey: auth-jack +--- error_code eval +[200, 200, 200] +--- no_error_log +[error] + + + +=== TEST 15: update the limit-req plugin +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ + "username": "consumer_limit_req", + "plugins": { + "key-auth": { + "key": "auth-jack" + }, + "limit-req": { + "rate": 0.1, + "burst": 0.1, + "rejected_code": 403, + "key": "consumer_name" + } + } + }]], + [[{ + "node": { + "value": { + "username": "consumer_limit_req", + "plugins": { + "key-auth": { + "key": "auth-jack" + }, + "limit-req": { + "rate": 0.1, + "burst": 0.1, + "rejected_code": 403, + "key": "consumer_name" + } + } + } + }, + "action": "set" + }]] + ) + + ngx.status = code + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed +--- no_error_log +[error] + + + +=== TEST 16: exceeding the burst +--- pipelined_requests eval +["GET /hello", "GET /hello", "GET /hello", "GET /hello"] +--- more_headers +apikey: auth-jack +--- error_code eval +[403, 403, 403, 403] +--- no_error_log +[error] + + + +=== TEST 17: consumer binds the limit-req plugin and `key` is `remote_addr` Review comment: I can't see how these test cases are related to pr ########## File path: t/plugin/limit-req.t ########## @@ -432,3 +432,367 @@ GET /t passed --- no_error_log [error] + + + +=== TEST 12: consumer binds the limit-req plugin and `key` is `consumer_name` +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ + "username": "consumer_limit_req", + "plugins": { + "key-auth": { + "key": "auth-jack" + }, + "limit-req": { + "rate": 2, + "burst": 1, + "rejected_code": 403, + "key": "consumer_name" + } + } + }]], + [[{ + "node": { + "value": { + "username": "consumer_limit_req", + "plugins": { + "key-auth": { + "key": "auth-jack" + }, + "limit-req": { + "rate": 2, + "burst": 1, + "rejected_code": 403, + "key": "consumer_name" + } + } + } + }, + "action": "set" + }]] + ) + + ngx.status = code + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed +--- no_error_log +[error] + + + +=== TEST 13: route add "key-auth" plugin +--- 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": { + "key-auth": {} + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + }, + "desc": "上游节点", + "uri": "/hello" + }]], + [[{ + "node": { + "value": { + "plugins": { + "key-auth": {} + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + }, + "desc": "上游节点", + "uri": "/hello" + }, + "key": "/apisix/routes/1" + }, + "action": "set" + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed +--- no_error_log +[error] + + + +=== TEST 14: not exceeding the burst +--- pipelined_requests eval +["GET /hello", "GET /hello", "GET /hello"] +--- more_headers +apikey: auth-jack +--- error_code eval +[200, 200, 200] +--- no_error_log +[error] + + + +=== TEST 15: update the limit-req plugin +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ + "username": "consumer_limit_req", + "plugins": { + "key-auth": { + "key": "auth-jack" + }, + "limit-req": { + "rate": 0.1, + "burst": 0.1, + "rejected_code": 403, + "key": "consumer_name" + } + } + }]], + [[{ + "node": { + "value": { + "username": "consumer_limit_req", + "plugins": { + "key-auth": { + "key": "auth-jack" + }, + "limit-req": { + "rate": 0.1, + "burst": 0.1, + "rejected_code": 403, + "key": "consumer_name" + } + } + } + }, + "action": "set" + }]] + ) + + ngx.status = code + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed +--- no_error_log +[error] + + + +=== TEST 16: exceeding the burst +--- pipelined_requests eval +["GET /hello", "GET /hello", "GET /hello", "GET /hello"] +--- more_headers +apikey: auth-jack +--- error_code eval +[403, 403, 403, 403] +--- no_error_log +[error] + + + +=== TEST 17: consumer binds the limit-req plugin and `key` is `remote_addr` Review comment: Test case tests not coverage error conditions ########## File path: t/plugin/limit-req.t ########## @@ -432,3 +432,367 @@ GET /t passed --- no_error_log [error] + + + +=== TEST 12: consumer binds the limit-req plugin and `key` is `consumer_name` +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ + "username": "consumer_limit_req", + "plugins": { + "key-auth": { + "key": "auth-jack" + }, + "limit-req": { + "rate": 2, + "burst": 1, + "rejected_code": 403, + "key": "consumer_name" + } + } + }]], + [[{ + "node": { + "value": { + "username": "consumer_limit_req", + "plugins": { + "key-auth": { + "key": "auth-jack" + }, + "limit-req": { + "rate": 2, + "burst": 1, + "rejected_code": 403, + "key": "consumer_name" + } + } + } + }, + "action": "set" + }]] + ) + + ngx.status = code + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed +--- no_error_log +[error] + + + +=== TEST 13: route add "key-auth" plugin +--- 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": { + "key-auth": {} + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + }, + "desc": "上游节点", + "uri": "/hello" + }]], + [[{ + "node": { + "value": { + "plugins": { + "key-auth": {} + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + }, + "desc": "上游节点", + "uri": "/hello" + }, + "key": "/apisix/routes/1" + }, + "action": "set" + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed +--- no_error_log +[error] + + + +=== TEST 14: not exceeding the burst +--- pipelined_requests eval +["GET /hello", "GET /hello", "GET /hello"] +--- more_headers +apikey: auth-jack +--- error_code eval +[200, 200, 200] +--- no_error_log +[error] + + + +=== TEST 15: update the limit-req plugin +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ + "username": "consumer_limit_req", + "plugins": { + "key-auth": { + "key": "auth-jack" + }, + "limit-req": { + "rate": 0.1, + "burst": 0.1, + "rejected_code": 403, + "key": "consumer_name" + } + } + }]], + [[{ + "node": { + "value": { + "username": "consumer_limit_req", + "plugins": { + "key-auth": { + "key": "auth-jack" + }, + "limit-req": { + "rate": 0.1, + "burst": 0.1, + "rejected_code": 403, + "key": "consumer_name" + } + } + } + }, + "action": "set" + }]] + ) + + ngx.status = code + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed +--- no_error_log +[error] + + + +=== TEST 16: exceeding the burst +--- pipelined_requests eval +["GET /hello", "GET /hello", "GET /hello", "GET /hello"] +--- more_headers +apikey: auth-jack +--- error_code eval +[403, 403, 403, 403] +--- no_error_log +[error] + + + +=== TEST 17: consumer binds the limit-req plugin and `key` is `remote_addr` Review comment: Test cases not coverage error conditions ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org