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/11/04 13:55:43 UTC

[GitHub] [apisix] Xunzhuo opened a new pull request #5422: feat(limit-* plugin): fallback to remote_addr when key is missing

Xunzhuo opened a new pull request #5422:
URL: https://github.com/apache/apisix/pull/5422


   ### 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. -->
   
    fallback to `remote_addr` when key is missing in limit-* plugin
   
   ### 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. Use "request review" to notify the reviewer once you have resolved the review
   -->
   
   * [ ] Did you explain what problem does this PR solve? Or what new features have been added?
   * [ ] Have you added corresponding test cases?
   * [ ] 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 #5422: feat(limit-* plugin): fallback to remote_addr when key is missing

Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5422:
URL: https://github.com/apache/apisix/pull/5422#discussion_r743369137



##########
File path: t/plugin/limit-conn2.t
##########
@@ -410,32 +440,102 @@ GET /t
 
 
 
-=== TEST 11: bypass empty key
+=== TEST 11: request when key is missing
+--- request
+GET /test_concurrency
+--- timeout: 10s
+--- response_body
+200
+503
+503
+503
+503
+--- no_error_log
+[error]
+--- error_log
+The key of limit conn is empty or invalid, set $remote_addr as the key
+
+
+
+=== TEST 12: update plugin to set invalid key
 --- 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
-                        .. "/limit_conn"
-            local ress = {}
-            for i = 1, 2 do
-                local httpc = http.new()
-                local res, err = httpc:request_uri(uri)
-                if not res then
-                    ngx.say(err)
-                    return
-                end
-                table.insert(ress, res.status)
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "plugins": {
+                            "limit-conn": {
+                                "conn": 1,
+                                "burst": 0,
+                                "default_conn_delay": 0.1,
+                                "rejected_code": 503,
+                                "key": "abcdefgh",
+                                "key_type": "var_combination"
+                            }
+                        },
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/limit_conn"
+                }]],
+                [[{

Review comment:
       There is no need to check the response, right?

##########
File path: t/plugin/limit-conn2.t
##########
@@ -321,8 +351,8 @@ request latency is nil
                  [[{
                         "plugins": {
                             "limit-conn": {
-                                "conn": 2,
-                                "burst": 1,
+                                "conn": 1,

Review comment:
       Why change this configuration?

##########
File path: apisix/plugins/limit-req.lua
##########
@@ -102,10 +102,9 @@ function _M.access(conf, ctx)
     end
 
     if key == nil then
-        core.log.info("bypass the limit req as the key is empty")
-        -- Bypass the limit req when the key is empty.
-        -- This behavior is the same as Nginx
-        return
+        core.log.info("The key of limit req is empty or invalid, set $remote_addr as the key")

Review comment:
       ```suggestion
           core.log.info("The value of the configured key is empty, use client IP instead")
   ```
   
   The value is empty, not the key itself.

##########
File path: docs/en/latest/plugins/limit-conn.md
##########
@@ -42,14 +42,14 @@ Limiting request concurrency plugin.
 | default_conn_delay | number  | required    |         | default_conn_delay > 0                                                                    | the latency seconds of request when concurrent requests exceeding `conn` but below (`conn` + `burst`).                                                                                                                                                                                                                                                                                                                                                                                                                                   |
 | only_use_default_delay  | boolean | optional    | false   | [true,false]                                                                              | enable the strict mode of the latency seconds. If you set this option to `true`, it will run strictly according to the latency seconds you set without additional calculation logic.                                                                                                                                                                                                                                                                                                                      |
 | key_type      | string  | optional    |   "var"   | ["var", "var_combination"] | the type of key. |
-| key           | string  | required    |         |  | 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". |
+| key           | string  | required    |         |  | 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". If the key is empty or invalid, `remote_addr` will be set as the default key.|

Review comment:
       ```suggestion
   | key           | string  | required    |         |  | 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". If the value of the key is empty, `remote_addr` will be set as the default key.|
   ```




-- 
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] Xunzhuo commented on a change in pull request #5422: feat(limit-* plugin): fallback to remote_addr when key is missing

Posted by GitBox <gi...@apache.org>.
Xunzhuo commented on a change in pull request #5422:
URL: https://github.com/apache/apisix/pull/5422#discussion_r743382793



##########
File path: t/plugin/limit-conn2.t
##########
@@ -410,32 +440,102 @@ GET /t
 
 
 
-=== TEST 11: bypass empty key
+=== TEST 11: request when key is missing
+--- request
+GET /test_concurrency
+--- timeout: 10s
+--- response_body
+200
+503
+503
+503
+503
+--- no_error_log
+[error]
+--- error_log
+The key of limit conn is empty or invalid, set $remote_addr as the key
+
+
+
+=== TEST 12: update plugin to set invalid key
 --- 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
-                        .. "/limit_conn"
-            local ress = {}
-            for i = 1, 2 do
-                local httpc = http.new()
-                local res, err = httpc:request_uri(uri)
-                if not res then
-                    ngx.say(err)
-                    return
-                end
-                table.insert(ress, res.status)
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "plugins": {
+                            "limit-conn": {
+                                "conn": 1,
+                                "burst": 0,
+                                "default_conn_delay": 0.1,
+                                "rejected_code": 503,
+                                "key": "abcdefgh",
+                                "key_type": "var_combination"
+                            }
+                        },
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/limit_conn"
+                }]],
+                [[{

Review comment:
       Sorry, I do not understand this point? Could you give me more infos?




-- 
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] Xunzhuo commented on a change in pull request #5422: feat(limit-* plugin): fallback to remote_addr when key is missing

Posted by GitBox <gi...@apache.org>.
Xunzhuo commented on a change in pull request #5422:
URL: https://github.com/apache/apisix/pull/5422#discussion_r743379378



##########
File path: apisix/plugins/limit-req.lua
##########
@@ -102,10 +102,9 @@ function _M.access(conf, ctx)
     end
 
     if key == nil then
-        core.log.info("bypass the limit req as the key is empty")
-        -- Bypass the limit req when the key is empty.
-        -- This behavior is the same as Nginx
-        return
+        core.log.info("The key of limit req is empty or invalid, set $remote_addr as the key")

Review comment:
       Done, thanks.

##########
File path: docs/en/latest/plugins/limit-conn.md
##########
@@ -42,14 +42,14 @@ Limiting request concurrency plugin.
 | default_conn_delay | number  | required    |         | default_conn_delay > 0                                                                    | the latency seconds of request when concurrent requests exceeding `conn` but below (`conn` + `burst`).                                                                                                                                                                                                                                                                                                                                                                                                                                   |
 | only_use_default_delay  | boolean | optional    | false   | [true,false]                                                                              | enable the strict mode of the latency seconds. If you set this option to `true`, it will run strictly according to the latency seconds you set without additional calculation logic.                                                                                                                                                                                                                                                                                                                      |
 | key_type      | string  | optional    |   "var"   | ["var", "var_combination"] | the type of key. |
-| key           | string  | required    |         |  | 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". |
+| key           | string  | required    |         |  | 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". If the key is empty or invalid, `remote_addr` will be set as the default key.|

Review comment:
       Done, thanks.




-- 
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 #5422: feat(limit-* plugin): fallback to remote_addr when key is missing

Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5422:
URL: https://github.com/apache/apisix/pull/5422#discussion_r743327859



##########
File path: apisix/plugins/limit-conn/init.lua
##########
@@ -64,10 +64,9 @@ function _M.increase(conf, ctx)
     end
 
     if key == nil then
-        core.log.info("bypass the limit conn as the key is empty")
-        -- Bypass the limit conn when the key is empty.
-        -- This behavior is the same as Nginx
-        return
+        core.log.info("The key of limit conn is empty or invalid, set $remote_addr as the key")
+        -- When the key is empty or invalid, set $remote_addr as the default key
+        key = "remote_addr"

Review comment:
       The key should be the value of remote_addr not remote_addr itself

##########
File path: t/plugin/limit-conn2.t
##########
@@ -410,32 +440,102 @@ GET /t
 
 
 
-=== TEST 11: bypass empty key
+=== TEST 11: request when key is missing
+--- request
+GET /test_concurrency
+--- timeout: 10s
+--- response_body
+200
+503
+503
+503
+503
+--- no_error_log
+[error]
+--- error_log
+The key of limit conn is empty or invalid, set $remote_addr as the key
+
+
+
+=== TEST 12: update plugin to set invalid key
 --- 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
-                        .. "/limit_conn"
-            local ress = {}
-            for i = 1, 2 do
-                local httpc = http.new()
-                local res, err = httpc:request_uri(uri)
-                if not res then
-                    ngx.say(err)
-                    return
-                end
-                table.insert(ress, res.status)
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "plugins": {
+                            "limit-conn": {
+                                "conn": 1,
+                                "burst": 0,
+                                "default_conn_delay": 0.1,
+                                "rejected_code": 503,
+                                "key": "abcdefgh",
+                                "key_type": "var_combination"
+                            }
+                        },
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/limit_conn"
+                }]],
+                [[{

Review comment:
       There is no need to check the response, right?

##########
File path: t/plugin/limit-conn2.t
##########
@@ -321,8 +351,8 @@ request latency is nil
                  [[{
                         "plugins": {
                             "limit-conn": {
-                                "conn": 2,
-                                "burst": 1,
+                                "conn": 1,

Review comment:
       Why change this configuration?

##########
File path: apisix/plugins/limit-req.lua
##########
@@ -102,10 +102,9 @@ function _M.access(conf, ctx)
     end
 
     if key == nil then
-        core.log.info("bypass the limit req as the key is empty")
-        -- Bypass the limit req when the key is empty.
-        -- This behavior is the same as Nginx
-        return
+        core.log.info("The key of limit req is empty or invalid, set $remote_addr as the key")

Review comment:
       ```suggestion
           core.log.info("The value of the configured key is empty, use client IP instead")
   ```
   
   The value is empty, not the key itself.

##########
File path: docs/en/latest/plugins/limit-conn.md
##########
@@ -42,14 +42,14 @@ Limiting request concurrency plugin.
 | default_conn_delay | number  | required    |         | default_conn_delay > 0                                                                    | the latency seconds of request when concurrent requests exceeding `conn` but below (`conn` + `burst`).                                                                                                                                                                                                                                                                                                                                                                                                                                   |
 | only_use_default_delay  | boolean | optional    | false   | [true,false]                                                                              | enable the strict mode of the latency seconds. If you set this option to `true`, it will run strictly according to the latency seconds you set without additional calculation logic.                                                                                                                                                                                                                                                                                                                      |
 | key_type      | string  | optional    |   "var"   | ["var", "var_combination"] | the type of key. |
-| key           | string  | required    |         |  | 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". |
+| key           | string  | required    |         |  | 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". If the key is empty or invalid, `remote_addr` will be set as the default key.|

Review comment:
       ```suggestion
   | key           | string  | required    |         |  | 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". If the value of the key is empty, `remote_addr` will be set as the default key.|
   ```

##########
File path: t/plugin/limit-conn2.t
##########
@@ -410,32 +440,102 @@ GET /t
 
 
 
-=== TEST 11: bypass empty key
+=== TEST 11: request when key is missing
+--- request
+GET /test_concurrency
+--- timeout: 10s
+--- response_body
+200
+503
+503
+503
+503
+--- no_error_log
+[error]
+--- error_log
+The key of limit conn is empty or invalid, set $remote_addr as the key
+
+
+
+=== TEST 12: update plugin to set invalid key
 --- 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
-                        .. "/limit_conn"
-            local ress = {}
-            for i = 1, 2 do
-                local httpc = http.new()
-                local res, err = httpc:request_uri(uri)
-                if not res then
-                    ngx.say(err)
-                    return
-                end
-                table.insert(ress, res.status)
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "plugins": {
+                            "limit-conn": {
+                                "conn": 1,
+                                "burst": 0,
+                                "default_conn_delay": 0.1,
+                                "rejected_code": 503,
+                                "key": "abcdefgh",
+                                "key_type": "var_combination"
+                            }
+                        },
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/limit_conn"
+                }]],
+                [[{

Review comment:
       There is no need to check the raw response, right?

##########
File path: t/plugin/limit-conn2.t
##########
@@ -321,8 +351,8 @@ request latency is nil
                  [[{
                         "plugins": {
                             "limit-conn": {
-                                "conn": 2,
-                                "burst": 1,
+                                "conn": 1,

Review comment:
       Why changing the configuration doesn't provide a different result?

##########
File path: t/plugin/limit-conn2.t
##########
@@ -321,8 +351,8 @@ request latency is nil
                  [[{
                         "plugins": {
                             "limit-conn": {
-                                "conn": 2,
-                                "burst": 1,
+                                "conn": 1,

Review comment:
       I see. It is already covered in the new test.




-- 
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 #5422: feat(limit-* plugin): fallback to remote_addr when key is missing

Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5422:
URL: https://github.com/apache/apisix/pull/5422#discussion_r743369137



##########
File path: t/plugin/limit-conn2.t
##########
@@ -410,32 +440,102 @@ GET /t
 
 
 
-=== TEST 11: bypass empty key
+=== TEST 11: request when key is missing
+--- request
+GET /test_concurrency
+--- timeout: 10s
+--- response_body
+200
+503
+503
+503
+503
+--- no_error_log
+[error]
+--- error_log
+The key of limit conn is empty or invalid, set $remote_addr as the key
+
+
+
+=== TEST 12: update plugin to set invalid key
 --- 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
-                        .. "/limit_conn"
-            local ress = {}
-            for i = 1, 2 do
-                local httpc = http.new()
-                local res, err = httpc:request_uri(uri)
-                if not res then
-                    ngx.say(err)
-                    return
-                end
-                table.insert(ress, res.status)
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "plugins": {
+                            "limit-conn": {
+                                "conn": 1,
+                                "burst": 0,
+                                "default_conn_delay": 0.1,
+                                "rejected_code": 503,
+                                "key": "abcdefgh",
+                                "key_type": "var_combination"
+                            }
+                        },
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/limit_conn"
+                }]],
+                [[{

Review comment:
       There is no need to check the raw response, 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



[GitHub] [apisix] spacewander commented on a change in pull request #5422: feat(limit-* plugin): fallback to remote_addr when key is missing

Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5422:
URL: https://github.com/apache/apisix/pull/5422#discussion_r743416293



##########
File path: t/plugin/limit-conn2.t
##########
@@ -321,8 +351,8 @@ request latency is nil
                  [[{
                         "plugins": {
                             "limit-conn": {
-                                "conn": 2,
-                                "burst": 1,
+                                "conn": 1,

Review comment:
       I see. It is already covered in the new test.




-- 
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 #5422: feat(limit-* plugin): fallback to remote_addr when key is missing

Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5422:
URL: https://github.com/apache/apisix/pull/5422#discussion_r743327859



##########
File path: apisix/plugins/limit-conn/init.lua
##########
@@ -64,10 +64,9 @@ function _M.increase(conf, ctx)
     end
 
     if key == nil then
-        core.log.info("bypass the limit conn as the key is empty")
-        -- Bypass the limit conn when the key is empty.
-        -- This behavior is the same as Nginx
-        return
+        core.log.info("The key of limit conn is empty or invalid, set $remote_addr as the key")
+        -- When the key is empty or invalid, set $remote_addr as the default key
+        key = "remote_addr"

Review comment:
       The key should be the value of remote_addr not remote_addr itself




-- 
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] Xunzhuo commented on a change in pull request #5422: feat(limit-* plugin): fallback to remote_addr when key is missing

Posted by GitBox <gi...@apache.org>.
Xunzhuo commented on a change in pull request #5422:
URL: https://github.com/apache/apisix/pull/5422#discussion_r743388661



##########
File path: t/plugin/limit-conn2.t
##########
@@ -410,32 +440,102 @@ GET /t
 
 
 
-=== TEST 11: bypass empty key
+=== TEST 11: request when key is missing
+--- request
+GET /test_concurrency
+--- timeout: 10s
+--- response_body
+200
+503
+503
+503
+503
+--- no_error_log
+[error]
+--- error_log
+The key of limit conn is empty or invalid, set $remote_addr as the key
+
+
+
+=== TEST 12: update plugin to set invalid key
 --- 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
-                        .. "/limit_conn"
-            local ress = {}
-            for i = 1, 2 do
-                local httpc = http.new()
-                local res, err = httpc:request_uri(uri)
-                if not res then
-                    ngx.say(err)
-                    return
-                end
-                table.insert(ress, res.status)
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "plugins": {
+                            "limit-conn": {
+                                "conn": 1,
+                                "burst": 0,
+                                "default_conn_delay": 0.1,
+                                "rejected_code": 503,
+                                "key": "abcdefgh",
+                                "key_type": "var_combination"
+                            }
+                        },
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/limit_conn"
+                }]],
+                [[{

Review comment:
       Do you mean that there is no need to check this?
   ```
   --- response_body
   passed
   ```




-- 
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] Xunzhuo commented on a change in pull request #5422: feat(limit-* plugin): fallback to remote_addr when key is missing

Posted by GitBox <gi...@apache.org>.
Xunzhuo commented on a change in pull request #5422:
URL: https://github.com/apache/apisix/pull/5422#discussion_r743336785



##########
File path: apisix/plugins/limit-conn/init.lua
##########
@@ -64,10 +64,9 @@ function _M.increase(conf, ctx)
     end
 
     if key == nil then
-        core.log.info("bypass the limit conn as the key is empty")
-        -- Bypass the limit conn when the key is empty.
-        -- This behavior is the same as Nginx
-        return
+        core.log.info("The key of limit conn is empty or invalid, set $remote_addr as the key")
+        -- When the key is empty or invalid, set $remote_addr as the default key
+        key = "remote_addr"

Review comment:
       Done. Thanks

##########
File path: apisix/plugins/limit-req.lua
##########
@@ -102,10 +102,9 @@ function _M.access(conf, ctx)
     end
 
     if key == nil then
-        core.log.info("bypass the limit req as the key is empty")
-        -- Bypass the limit req when the key is empty.
-        -- This behavior is the same as Nginx
-        return
+        core.log.info("The key of limit req is empty or invalid, set $remote_addr as the key")

Review comment:
       Done, thanks.

##########
File path: docs/en/latest/plugins/limit-conn.md
##########
@@ -42,14 +42,14 @@ Limiting request concurrency plugin.
 | default_conn_delay | number  | required    |         | default_conn_delay > 0                                                                    | the latency seconds of request when concurrent requests exceeding `conn` but below (`conn` + `burst`).                                                                                                                                                                                                                                                                                                                                                                                                                                   |
 | only_use_default_delay  | boolean | optional    | false   | [true,false]                                                                              | enable the strict mode of the latency seconds. If you set this option to `true`, it will run strictly according to the latency seconds you set without additional calculation logic.                                                                                                                                                                                                                                                                                                                      |
 | key_type      | string  | optional    |   "var"   | ["var", "var_combination"] | the type of key. |
-| key           | string  | required    |         |  | 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". |
+| key           | string  | required    |         |  | 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". If the key is empty or invalid, `remote_addr` will be set as the default key.|

Review comment:
       Done, thanks.

##########
File path: t/plugin/limit-conn2.t
##########
@@ -410,32 +440,102 @@ GET /t
 
 
 
-=== TEST 11: bypass empty key
+=== TEST 11: request when key is missing
+--- request
+GET /test_concurrency
+--- timeout: 10s
+--- response_body
+200
+503
+503
+503
+503
+--- no_error_log
+[error]
+--- error_log
+The key of limit conn is empty or invalid, set $remote_addr as the key
+
+
+
+=== TEST 12: update plugin to set invalid key
 --- 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
-                        .. "/limit_conn"
-            local ress = {}
-            for i = 1, 2 do
-                local httpc = http.new()
-                local res, err = httpc:request_uri(uri)
-                if not res then
-                    ngx.say(err)
-                    return
-                end
-                table.insert(ress, res.status)
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "plugins": {
+                            "limit-conn": {
+                                "conn": 1,
+                                "burst": 0,
+                                "default_conn_delay": 0.1,
+                                "rejected_code": 503,
+                                "key": "abcdefgh",
+                                "key_type": "var_combination"
+                            }
+                        },
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/limit_conn"
+                }]],
+                [[{

Review comment:
       Sorry, I do not understand this point? Could you give me more infos?

##########
File path: t/plugin/limit-conn2.t
##########
@@ -321,8 +351,8 @@ request latency is nil
                  [[{
                         "plugins": {
                             "limit-conn": {
-                                "conn": 2,
-                                "burst": 1,
+                                "conn": 1,

Review comment:
       It is easier to test when value of key is empty.

##########
File path: t/plugin/limit-conn2.t
##########
@@ -410,32 +440,102 @@ GET /t
 
 
 
-=== TEST 11: bypass empty key
+=== TEST 11: request when key is missing
+--- request
+GET /test_concurrency
+--- timeout: 10s
+--- response_body
+200
+503
+503
+503
+503
+--- no_error_log
+[error]
+--- error_log
+The key of limit conn is empty or invalid, set $remote_addr as the key
+
+
+
+=== TEST 12: update plugin to set invalid key
 --- 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
-                        .. "/limit_conn"
-            local ress = {}
-            for i = 1, 2 do
-                local httpc = http.new()
-                local res, err = httpc:request_uri(uri)
-                if not res then
-                    ngx.say(err)
-                    return
-                end
-                table.insert(ress, res.status)
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "plugins": {
+                            "limit-conn": {
+                                "conn": 1,
+                                "burst": 0,
+                                "default_conn_delay": 0.1,
+                                "rejected_code": 503,
+                                "key": "abcdefgh",
+                                "key_type": "var_combination"
+                            }
+                        },
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/limit_conn"
+                }]],
+                [[{

Review comment:
       Do you mean that there is no need to check this?
   ```
   --- response_body
   passed
   ```

##########
File path: t/plugin/limit-conn2.t
##########
@@ -410,32 +440,102 @@ GET /t
 
 
 
-=== TEST 11: bypass empty key
+=== TEST 11: request when key is missing
+--- request
+GET /test_concurrency
+--- timeout: 10s
+--- response_body
+200
+503
+503
+503
+503
+--- no_error_log
+[error]
+--- error_log
+The key of limit conn is empty or invalid, set $remote_addr as the key
+
+
+
+=== TEST 12: update plugin to set invalid key
 --- 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
-                        .. "/limit_conn"
-            local ress = {}
-            for i = 1, 2 do
-                local httpc = http.new()
-                local res, err = httpc:request_uri(uri)
-                if not res then
-                    ngx.say(err)
-                    return
-                end
-                table.insert(ress, res.status)
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "plugins": {
+                            "limit-conn": {
+                                "conn": 1,
+                                "burst": 0,
+                                "default_conn_delay": 0.1,
+                                "rejected_code": 503,
+                                "key": "abcdefgh",
+                                "key_type": "var_combination"
+                            }
+                        },
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/limit_conn"
+                }]],
+                [[{

Review comment:
       Do you mean that there is no need to check this?
   ```
   --- response_body
   passed
   ```

##########
File path: apisix/plugins/limit-conn/init.lua
##########
@@ -64,10 +64,9 @@ function _M.increase(conf, ctx)
     end
 
     if key == nil then
-        core.log.info("bypass the limit conn as the key is empty")
-        -- Bypass the limit conn when the key is empty.
-        -- This behavior is the same as Nginx
-        return
+        core.log.info("The key of limit conn is empty or invalid, set $remote_addr as the key")
+        -- When the key is empty or invalid, set $remote_addr as the default key
+        key = "remote_addr"

Review comment:
       Done. Thanks

##########
File path: apisix/plugins/limit-req.lua
##########
@@ -102,10 +102,9 @@ function _M.access(conf, ctx)
     end
 
     if key == nil then
-        core.log.info("bypass the limit req as the key is empty")
-        -- Bypass the limit req when the key is empty.
-        -- This behavior is the same as Nginx
-        return
+        core.log.info("The key of limit req is empty or invalid, set $remote_addr as the key")

Review comment:
       Done, thanks.

##########
File path: docs/en/latest/plugins/limit-conn.md
##########
@@ -42,14 +42,14 @@ Limiting request concurrency plugin.
 | default_conn_delay | number  | required    |         | default_conn_delay > 0                                                                    | the latency seconds of request when concurrent requests exceeding `conn` but below (`conn` + `burst`).                                                                                                                                                                                                                                                                                                                                                                                                                                   |
 | only_use_default_delay  | boolean | optional    | false   | [true,false]                                                                              | enable the strict mode of the latency seconds. If you set this option to `true`, it will run strictly according to the latency seconds you set without additional calculation logic.                                                                                                                                                                                                                                                                                                                      |
 | key_type      | string  | optional    |   "var"   | ["var", "var_combination"] | the type of key. |
-| key           | string  | required    |         |  | 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". |
+| key           | string  | required    |         |  | 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". If the key is empty or invalid, `remote_addr` will be set as the default key.|

Review comment:
       Done, thanks.

##########
File path: t/plugin/limit-conn2.t
##########
@@ -410,32 +440,102 @@ GET /t
 
 
 
-=== TEST 11: bypass empty key
+=== TEST 11: request when key is missing
+--- request
+GET /test_concurrency
+--- timeout: 10s
+--- response_body
+200
+503
+503
+503
+503
+--- no_error_log
+[error]
+--- error_log
+The key of limit conn is empty or invalid, set $remote_addr as the key
+
+
+
+=== TEST 12: update plugin to set invalid key
 --- 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
-                        .. "/limit_conn"
-            local ress = {}
-            for i = 1, 2 do
-                local httpc = http.new()
-                local res, err = httpc:request_uri(uri)
-                if not res then
-                    ngx.say(err)
-                    return
-                end
-                table.insert(ress, res.status)
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "plugins": {
+                            "limit-conn": {
+                                "conn": 1,
+                                "burst": 0,
+                                "default_conn_delay": 0.1,
+                                "rejected_code": 503,
+                                "key": "abcdefgh",
+                                "key_type": "var_combination"
+                            }
+                        },
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/limit_conn"
+                }]],
+                [[{

Review comment:
       Sorry, I do not understand this point? Could you give me more infos?

##########
File path: t/plugin/limit-conn2.t
##########
@@ -321,8 +351,8 @@ request latency is nil
                  [[{
                         "plugins": {
                             "limit-conn": {
-                                "conn": 2,
-                                "burst": 1,
+                                "conn": 1,

Review comment:
       It is easier to test when value of key is empty.

##########
File path: t/plugin/limit-conn2.t
##########
@@ -410,32 +440,102 @@ GET /t
 
 
 
-=== TEST 11: bypass empty key
+=== TEST 11: request when key is missing
+--- request
+GET /test_concurrency
+--- timeout: 10s
+--- response_body
+200
+503
+503
+503
+503
+--- no_error_log
+[error]
+--- error_log
+The key of limit conn is empty or invalid, set $remote_addr as the key
+
+
+
+=== TEST 12: update plugin to set invalid key
 --- 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
-                        .. "/limit_conn"
-            local ress = {}
-            for i = 1, 2 do
-                local httpc = http.new()
-                local res, err = httpc:request_uri(uri)
-                if not res then
-                    ngx.say(err)
-                    return
-                end
-                table.insert(ress, res.status)
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "plugins": {
+                            "limit-conn": {
+                                "conn": 1,
+                                "burst": 0,
+                                "default_conn_delay": 0.1,
+                                "rejected_code": 503,
+                                "key": "abcdefgh",
+                                "key_type": "var_combination"
+                            }
+                        },
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/limit_conn"
+                }]],
+                [[{

Review comment:
       Do you mean that there is no need to check this?
   ```
   --- response_body
   passed
   ```

##########
File path: t/plugin/limit-conn2.t
##########
@@ -410,32 +440,102 @@ GET /t
 
 
 
-=== TEST 11: bypass empty key
+=== TEST 11: request when key is missing
+--- request
+GET /test_concurrency
+--- timeout: 10s
+--- response_body
+200
+503
+503
+503
+503
+--- no_error_log
+[error]
+--- error_log
+The key of limit conn is empty or invalid, set $remote_addr as the key
+
+
+
+=== TEST 12: update plugin to set invalid key
 --- 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
-                        .. "/limit_conn"
-            local ress = {}
-            for i = 1, 2 do
-                local httpc = http.new()
-                local res, err = httpc:request_uri(uri)
-                if not res then
-                    ngx.say(err)
-                    return
-                end
-                table.insert(ress, res.status)
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "plugins": {
+                            "limit-conn": {
+                                "conn": 1,
+                                "burst": 0,
+                                "default_conn_delay": 0.1,
+                                "rejected_code": 503,
+                                "key": "abcdefgh",
+                                "key_type": "var_combination"
+                            }
+                        },
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/limit_conn"
+                }]],
+                [[{

Review comment:
       Do you mean that there is no need to check this?
   ```
   --- response_body
   passed
   ```




-- 
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] Xunzhuo commented on a change in pull request #5422: feat(limit-* plugin): fallback to remote_addr when key is missing

Posted by GitBox <gi...@apache.org>.
Xunzhuo commented on a change in pull request #5422:
URL: https://github.com/apache/apisix/pull/5422#discussion_r743388661



##########
File path: t/plugin/limit-conn2.t
##########
@@ -410,32 +440,102 @@ GET /t
 
 
 
-=== TEST 11: bypass empty key
+=== TEST 11: request when key is missing
+--- request
+GET /test_concurrency
+--- timeout: 10s
+--- response_body
+200
+503
+503
+503
+503
+--- no_error_log
+[error]
+--- error_log
+The key of limit conn is empty or invalid, set $remote_addr as the key
+
+
+
+=== TEST 12: update plugin to set invalid key
 --- 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
-                        .. "/limit_conn"
-            local ress = {}
-            for i = 1, 2 do
-                local httpc = http.new()
-                local res, err = httpc:request_uri(uri)
-                if not res then
-                    ngx.say(err)
-                    return
-                end
-                table.insert(ress, res.status)
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "plugins": {
+                            "limit-conn": {
+                                "conn": 1,
+                                "burst": 0,
+                                "default_conn_delay": 0.1,
+                                "rejected_code": 503,
+                                "key": "abcdefgh",
+                                "key_type": "var_combination"
+                            }
+                        },
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/limit_conn"
+                }]],
+                [[{

Review comment:
       Do you mean that there is no need to check this?
   ```
   --- response_body
   passed
   ```




-- 
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] Xunzhuo commented on a change in pull request #5422: feat(limit-* plugin): fallback to remote_addr when key is missing

Posted by GitBox <gi...@apache.org>.
Xunzhuo commented on a change in pull request #5422:
URL: https://github.com/apache/apisix/pull/5422#discussion_r743383197



##########
File path: t/plugin/limit-conn2.t
##########
@@ -321,8 +351,8 @@ request latency is nil
                  [[{
                         "plugins": {
                             "limit-conn": {
-                                "conn": 2,
-                                "burst": 1,
+                                "conn": 1,

Review comment:
       It is easier to test when value of key is empty.




-- 
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 #5422: feat(limit-* plugin): fallback to remote_addr when key is missing

Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5422:
URL: https://github.com/apache/apisix/pull/5422#discussion_r743415242



##########
File path: t/plugin/limit-conn2.t
##########
@@ -321,8 +351,8 @@ request latency is nil
                  [[{
                         "plugins": {
                             "limit-conn": {
-                                "conn": 2,
-                                "burst": 1,
+                                "conn": 1,

Review comment:
       Why changing the configuration doesn't provide a different result?




-- 
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 #5422: feat(limit-* plugin): fallback to remote_addr when key is missing

Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #5422:
URL: https://github.com/apache/apisix/pull/5422


   


-- 
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] Xunzhuo commented on a change in pull request #5422: feat(limit-* plugin): fallback to remote_addr when key is missing

Posted by GitBox <gi...@apache.org>.
Xunzhuo commented on a change in pull request #5422:
URL: https://github.com/apache/apisix/pull/5422#discussion_r743336785



##########
File path: apisix/plugins/limit-conn/init.lua
##########
@@ -64,10 +64,9 @@ function _M.increase(conf, ctx)
     end
 
     if key == nil then
-        core.log.info("bypass the limit conn as the key is empty")
-        -- Bypass the limit conn when the key is empty.
-        -- This behavior is the same as Nginx
-        return
+        core.log.info("The key of limit conn is empty or invalid, set $remote_addr as the key")
+        -- When the key is empty or invalid, set $remote_addr as the default key
+        key = "remote_addr"

Review comment:
       Done. Thanks




-- 
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 #5422: feat(limit-* plugin): fallback to remote_addr when key is missing

Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5422:
URL: https://github.com/apache/apisix/pull/5422#discussion_r743327859



##########
File path: apisix/plugins/limit-conn/init.lua
##########
@@ -64,10 +64,9 @@ function _M.increase(conf, ctx)
     end
 
     if key == nil then
-        core.log.info("bypass the limit conn as the key is empty")
-        -- Bypass the limit conn when the key is empty.
-        -- This behavior is the same as Nginx
-        return
+        core.log.info("The key of limit conn is empty or invalid, set $remote_addr as the key")
+        -- When the key is empty or invalid, set $remote_addr as the default key
+        key = "remote_addr"

Review comment:
       The key should be the value of remote_addr not remote_addr itself

##########
File path: t/plugin/limit-conn2.t
##########
@@ -410,32 +440,102 @@ GET /t
 
 
 
-=== TEST 11: bypass empty key
+=== TEST 11: request when key is missing
+--- request
+GET /test_concurrency
+--- timeout: 10s
+--- response_body
+200
+503
+503
+503
+503
+--- no_error_log
+[error]
+--- error_log
+The key of limit conn is empty or invalid, set $remote_addr as the key
+
+
+
+=== TEST 12: update plugin to set invalid key
 --- 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
-                        .. "/limit_conn"
-            local ress = {}
-            for i = 1, 2 do
-                local httpc = http.new()
-                local res, err = httpc:request_uri(uri)
-                if not res then
-                    ngx.say(err)
-                    return
-                end
-                table.insert(ress, res.status)
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "plugins": {
+                            "limit-conn": {
+                                "conn": 1,
+                                "burst": 0,
+                                "default_conn_delay": 0.1,
+                                "rejected_code": 503,
+                                "key": "abcdefgh",
+                                "key_type": "var_combination"
+                            }
+                        },
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/limit_conn"
+                }]],
+                [[{

Review comment:
       There is no need to check the response, right?

##########
File path: t/plugin/limit-conn2.t
##########
@@ -321,8 +351,8 @@ request latency is nil
                  [[{
                         "plugins": {
                             "limit-conn": {
-                                "conn": 2,
-                                "burst": 1,
+                                "conn": 1,

Review comment:
       Why change this configuration?

##########
File path: apisix/plugins/limit-req.lua
##########
@@ -102,10 +102,9 @@ function _M.access(conf, ctx)
     end
 
     if key == nil then
-        core.log.info("bypass the limit req as the key is empty")
-        -- Bypass the limit req when the key is empty.
-        -- This behavior is the same as Nginx
-        return
+        core.log.info("The key of limit req is empty or invalid, set $remote_addr as the key")

Review comment:
       ```suggestion
           core.log.info("The value of the configured key is empty, use client IP instead")
   ```
   
   The value is empty, not the key itself.

##########
File path: docs/en/latest/plugins/limit-conn.md
##########
@@ -42,14 +42,14 @@ Limiting request concurrency plugin.
 | default_conn_delay | number  | required    |         | default_conn_delay > 0                                                                    | the latency seconds of request when concurrent requests exceeding `conn` but below (`conn` + `burst`).                                                                                                                                                                                                                                                                                                                                                                                                                                   |
 | only_use_default_delay  | boolean | optional    | false   | [true,false]                                                                              | enable the strict mode of the latency seconds. If you set this option to `true`, it will run strictly according to the latency seconds you set without additional calculation logic.                                                                                                                                                                                                                                                                                                                      |
 | key_type      | string  | optional    |   "var"   | ["var", "var_combination"] | the type of key. |
-| key           | string  | required    |         |  | 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". |
+| key           | string  | required    |         |  | 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". If the key is empty or invalid, `remote_addr` will be set as the default key.|

Review comment:
       ```suggestion
   | key           | string  | required    |         |  | 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". If the value of the key is empty, `remote_addr` will be set as the default key.|
   ```

##########
File path: t/plugin/limit-conn2.t
##########
@@ -410,32 +440,102 @@ GET /t
 
 
 
-=== TEST 11: bypass empty key
+=== TEST 11: request when key is missing
+--- request
+GET /test_concurrency
+--- timeout: 10s
+--- response_body
+200
+503
+503
+503
+503
+--- no_error_log
+[error]
+--- error_log
+The key of limit conn is empty or invalid, set $remote_addr as the key
+
+
+
+=== TEST 12: update plugin to set invalid key
 --- 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
-                        .. "/limit_conn"
-            local ress = {}
-            for i = 1, 2 do
-                local httpc = http.new()
-                local res, err = httpc:request_uri(uri)
-                if not res then
-                    ngx.say(err)
-                    return
-                end
-                table.insert(ress, res.status)
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{
+                        "plugins": {
+                            "limit-conn": {
+                                "conn": 1,
+                                "burst": 0,
+                                "default_conn_delay": 0.1,
+                                "rejected_code": 503,
+                                "key": "abcdefgh",
+                                "key_type": "var_combination"
+                            }
+                        },
+                        "upstream": {
+                            "nodes": {
+                                "127.0.0.1:1980": 1
+                            },
+                            "type": "roundrobin"
+                        },
+                        "uri": "/limit_conn"
+                }]],
+                [[{

Review comment:
       There is no need to check the raw response, right?

##########
File path: t/plugin/limit-conn2.t
##########
@@ -321,8 +351,8 @@ request latency is nil
                  [[{
                         "plugins": {
                             "limit-conn": {
-                                "conn": 2,
-                                "burst": 1,
+                                "conn": 1,

Review comment:
       Why changing the configuration doesn't provide a different result?

##########
File path: t/plugin/limit-conn2.t
##########
@@ -321,8 +351,8 @@ request latency is nil
                  [[{
                         "plugins": {
                             "limit-conn": {
-                                "conn": 2,
-                                "burst": 1,
+                                "conn": 1,

Review comment:
       I see. It is already covered in the new test.




-- 
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