diff --git a/apisix/plugins/limit-conn/init.lua b/apisix/plugins/limit-conn/init.lua index b0cbafe815b4..1157716b4512 100644 --- a/apisix/plugins/limit-conn/init.lua +++ b/apisix/plugins/limit-conn/init.lua @@ -16,6 +16,7 @@ -- local limit_conn_new = require("resty.limit.conn").new local core = require("apisix.core") +local apisix_plugin = require("apisix.plugin") local is_http = ngx.config.subsystem == "http" local sleep = core.sleep local tonumber = tonumber @@ -128,6 +129,24 @@ local function get_rules(ctx, conf) end +local function gen_limit_key(conf, ctx, key) + local parent = conf._meta and conf._meta.parent + if not parent or not parent.resource_key then + core.log.error("failed to generate key invalid parent: ", core.json.encode(parent)) + return nil + end + + local new_key = parent.resource_key .. ':' .. apisix_plugin.conf_version(conf) .. ':' .. key + if conf._vid then + -- conf has _vid means it's from workflow plugin, add _vid to the key + -- so that the counter is unique per action. + return new_key .. ':' .. conf._vid + end + + return new_key +end + + local function create_limit_obj(conf, rule, default_conn_delay) core.log.info("create new limit-conn plugin instance") @@ -190,11 +209,12 @@ local function run_limit_conn(conf, rule, ctx) key = ctx.var["remote_addr"] end - key = key .. ctx.conf_type .. ctx.conf_version - if conf._vid then - -- conf has _vid means it's from workflow plugin, add _vid to the key - -- so that the counter is unique per action. - key = key .. ':' .. conf._vid + key = gen_limit_key(conf, ctx, key) + if not key then + if conf.allow_degradation then + return + end + return 500 end core.log.info("limit key: ", key) @@ -229,8 +249,6 @@ end function _M.increase(conf, ctx) - core.log.info("ver: ", ctx.conf_version) - local rules, err = get_rules(ctx, conf) if not rules or #rules == 0 then core.log.error("failed to get limit conn rules: ", err) diff --git a/t/plugin/limit-conn-redis.t b/t/plugin/limit-conn-redis.t index ad3224442205..4c4112d8d2e4 100644 --- a/t/plugin/limit-conn-redis.t +++ b/t/plugin/limit-conn-redis.t @@ -587,8 +587,8 @@ GET /test_concurrency --- response_body status:200, count:6 status:503, count:4 ---- error_log -limit key: 10.10.10.1route +--- error_log eval +qr/limit key: \/apisix\/routes\/1:\d+:10\.10\.10\.1/ @@ -679,8 +679,8 @@ GET /test_concurrency --- response_body status:200, count:6 status:503, count:4 ---- error_log -limit key: 10.10.10.2route +--- error_log eval +qr/limit key: \/apisix\/routes\/1:\d+:10\.10\.10\.2/ diff --git a/t/plugin/limit-conn-shared-counter.t b/t/plugin/limit-conn-shared-counter.t new file mode 100644 index 000000000000..fdd5f6651c0f --- /dev/null +++ b/t/plugin/limit-conn-shared-counter.t @@ -0,0 +1,260 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +use t::APISIX 'no_plan'; + +log_level('info'); +repeat_each(1); +no_long_string(); +no_shuffle(); +no_root_location(); + +add_block_preprocessor(sub { + my ($block) = @_; + my $port = $ENV{TEST_NGINX_SERVER_PORT}; + + if ((!defined $block->error_log) && (!defined $block->no_error_log)) { + $block->set_value("no_error_log", "[error]"); + } + + if (!defined $block->request) { + $block->set_value("request", "GET /t"); + } + + # limit-conn counts *concurrent* connections and releases the counter in the + # log phase, so the requests have to overlap: pipelined requests are handled + # one after another and would never hit the limit. + my $config = $block->config // <<_EOC_; + location /concurrent { + content_by_lua_block { + local httpc = require("resty.http") + + local function hit(uri, apikey) + local hc = httpc:new() + local res, err = hc:request_uri("http://127.0.0.1:$port" .. uri, + {headers = {apikey = apikey}}) + if not res then + ngx.log(ngx.ERR, "request to ", uri, " failed: ", err) + return 0 + end + return res.status + end + + -- two_routes: same consumer hits two different routes + -- two_consumers: two consumers hit the same route + local reqs = { + two_routes = {{"/limit_conn", "jack-key"}, {"/limit_conn2", "jack-key"}}, + two_consumers = {{"/limit_conn", "jack-key"}, {"/limit_conn", "bob-key"}}, + } + + local threads = {} + for i, req in ipairs(reqs[ngx.var.arg_case]) do + threads[i] = ngx.thread.spawn(hit, req[1], req[2]) + end + + local codes = {} + for i, th in ipairs(threads) do + local _, status = ngx.thread.wait(th) + codes[i] = status + end + + table.sort(codes) + ngx.say(table.concat(codes, ",")) + } + } +_EOC_ + + $block->set_value("config", $config); +}); + +run_tests(); + +__DATA__ + +=== TEST 1: consumer jack with limit-conn (conn = 1, key shared with bob) +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers/jack', + ngx.HTTP_PUT, + [[{ + "username": "jack", + "plugins": { + "key-auth": { + "key": "jack-key" + }, + "limit-conn": { + "conn": 1, + "burst": 0, + "default_conn_delay": 0.1, + "rejected_code": 503, + "key": "remote_addr" + } + } + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- response_body +passed + + + +=== TEST 2: consumer bob with the very same limit-conn config +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers/bob', + ngx.HTTP_PUT, + [[{ + "username": "bob", + "plugins": { + "key-auth": { + "key": "bob-key" + }, + "limit-conn": { + "conn": 1, + "burst": 0, + "default_conn_delay": 0.1, + "rejected_code": 503, + "key": "remote_addr" + } + } + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- response_body +passed + + + +=== TEST 3: set 2 routes with key-auth, both proxying to the slow upstream +--- 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, + [[{ + "uri": "/limit_conn", + "plugins": { + "key-auth": {} + }, + "upstream": { + "type": "roundrobin", + "nodes": { + "127.0.0.1:1980": 1 + } + } + }]] + ) + if code >= 300 then + ngx.status = code + ngx.say(body) + return + end + + code, body = t('/apisix/admin/routes/2', + ngx.HTTP_PUT, + [[{ + "uri": "/limit_conn2", + "plugins": { + "key-auth": {}, + "proxy-rewrite": { + "uri": "/limit_conn" + } + }, + "upstream": { + "type": "roundrobin", + "nodes": { + "127.0.0.1:1980": 1 + } + } + }]] + ) + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- response_body +passed + + + +=== TEST 4: the counter is shared across routes and keyed by the consumer +--- request +GET /concurrent?case=two_routes +--- response_body +200,503 +--- error_log eval +qr/limit key: \/apisix\/consumers\/jack:\d+:127\.0\.0\.1/ +--- no_error_log +[error] +--- timeout: 10 + + + +=== TEST 5: different consumers keep their own counter +jack and bob resolve to the same key value (remote_addr) on the same route, so +only the parent resource_key keeps them apart. +--- request +GET /concurrent?case=two_consumers +--- response_body +200,200 +--- error_log eval +qr/limit key: \/apisix\/consumers\/bob:\d+:127\.0\.0\.1/ +--- timeout: 10 + + + +=== TEST 6: clean up +The routes and consumers created here would otherwise leak into the next file of +the same CI job: /limit_conn2 is expected to 404 in t/plugin/workflow3.t. +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + for _, uri in ipairs({"/apisix/admin/routes/1", "/apisix/admin/routes/2", + "/apisix/admin/consumers/jack", + "/apisix/admin/consumers/bob"}) do + local code = t(uri, ngx.HTTP_DELETE) + if code >= 300 then + ngx.status = code + ngx.say("failed to delete ", uri) + return + end + end + ngx.say("done") + } + } +--- response_body +done diff --git a/t/plugin/limit-conn.t b/t/plugin/limit-conn.t index 93c69730fc62..06bbdeec8a9e 100644 --- a/t/plugin/limit-conn.t +++ b/t/plugin/limit-conn.t @@ -622,8 +622,8 @@ GET /test_concurrency 503 503 503 ---- error_log -limit key: 10.10.10.1route +--- error_log eval +qr/limit key: \/apisix\/routes\/1:\d+:10\.10\.10\.1/ @@ -713,8 +713,8 @@ GET /test_concurrency 503 503 503 ---- error_log -limit key: 10.10.10.2route +--- error_log eval +qr/limit key: \/apisix\/routes\/1:\d+:10\.10\.10\.2/ @@ -987,8 +987,8 @@ GET /test_concurrency 200 200 200 ---- error_log_like eval -qr/limit key: consumer_jackroute&consumer\d+/ +--- error_log eval +qr/limit key: \/apisix\/routes\/\d+:\d+:consumer_jack/ @@ -1076,8 +1076,8 @@ GET /test_concurrency 503 503 503 ---- error_log_like eval -qr/limit key: consumer_jackroute&consumer\d+/ +--- error_log eval +qr/limit key: \/apisix\/routes\/\d+:\d+:consumer_jack/ diff --git a/t/plugin/workflow3.t b/t/plugin/workflow3.t index e993381af02a..60f1ffab1dae 100644 --- a/t/plugin/workflow3.t +++ b/t/plugin/workflow3.t @@ -262,6 +262,11 @@ GET /test_concurrency2 503 503 503 +--- error_log eval +[ + qr/limit key: \/apisix\/routes\/1:\d+:127\.0\.0\.1:1/, + qr/limit key: \/apisix\/routes\/1:\d+:127\.0\.0\.1:2/, +] diff --git a/t/stream-plugin/limit-conn.t b/t/stream-plugin/limit-conn.t index c6c7c89c0c0d..3a733475ef16 100644 --- a/t/stream-plugin/limit-conn.t +++ b/t/stream-plugin/limit-conn.t @@ -141,6 +141,11 @@ GET /test_concurrency 200 200 --- stream_enable +--- error_log eval +qr/limit key: \/apisix\/stream_routes\/1:\d+:127\.0\.0\.1/ +--- no_error_log +[error] +[alert]