Copilot commented on code in PR #12872:
URL: https://github.com/apache/apisix/pull/12872#discussion_r2680869840
##########
apisix/plugins/limit-conn/util.lua:
##########
@@ -64,7 +75,20 @@ function _M.leaving(self, red, key, req_latency)
assert(key)
key = "limit_conn" .. ":" .. key
- local conn, err = red:incrby(key, -1)
+ local conn, err
+ if self.conf.key_ttl then
+ red:init_pipeline()
+ red:incrby(key, -1)
+ red:expire(key, self.conf.key_ttl)
+ local res, err = red:commit_pipeline()
+ if not res then
+ return nil, err
Review Comment:
The return statement has inconsistent indentation - it uses 5 spaces
(leading space before 4-space indent) while the rest of the code uses 4-space
indentation. This should be corrected to use 4 spaces for consistency with the
rest of the file.
```suggestion
return nil, err
```
##########
apisix/plugins/limit-req/util.lua:
##########
@@ -57,14 +57,16 @@ function _M.incoming(self, red, key, commit)
end
if commit then
- local ok
- local err
- ok, err = red:set(excess_key, excess)
+ -- limit-req does rate limiting per second, so we set the ttl to 2
seconds
+ local ttl = 2
Review Comment:
The limit-req plugin uses a hardcoded TTL of 2 seconds for Redis keys, while
the limit-conn plugin has a configurable key_ttl parameter with a default of 60
seconds. This inconsistency means:
1. Users cannot configure the TTL for limit-req Redis keys
2. The 2-second TTL may be too short for high-burst scenarios where requests
are queued
Consider making the TTL configurable for limit-req as well, similar to
limit-conn. The TTL could be calculated as ceil(burst/rate) + a safety margin
to ensure keys persist long enough for delayed requests to be processed.
```suggestion
-- limit-req does rate limiting per second. Allow configurable TTL
via self.key_ttl,
-- defaulting to 2 seconds to preserve existing behavior if not set.
local ttl = self.key_ttl or 2
```
##########
t/plugin/limit-req-redis-ttl.t:
##########
@@ -0,0 +1,113 @@
+#
+# 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';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+no_shuffle();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+ my $config = $block->config // <<_EOC_;
+ location /check {
+ content_by_lua_block {
+ local redis = require "resty.redis"
+ local red = redis:new()
+ red:connect("127.0.0.1", 6379)
+ red:flushall()
+
+
+ -- make a request to /access
+ local httpc = require("resty.http").new()
+ local uri = "http://127.0.0.1:"; .. ngx.var.server_port .. "/access"
+ local res, err = httpc:request_uri(uri, {
+ method = "GET"
+ })
+
+ if not res then
+ ngx.say("failed to request: ", err)
+ return
+ end
+
+ -- key format might include conf_type and conf_version, e.g.
limit_req:127.0.0.1route99excess
+ local keys, err =
red:keys("limit_req:limit_req_ttl_test_127.0.0.1*")
+ if not keys or #keys == 0 then
+ ngx.say("no keys found")
+ return
+ end
+
+ local ttl = red:ttl(keys[1])
+ if ttl > 0 and ttl <= 11 then
Review Comment:
The test expects a TTL of up to 11 seconds (based on the formula
ceil(burst/rate) + 1 = ceil(10/1) + 1 = 11), but the implementation in
apisix/plugins/limit-req/util.lua sets a hardcoded TTL of 2 seconds. This test
will pass since 2 is within the range (0, 11], but the test is validating
against the wrong expected value.
The test should check for TTL <= 2 to match the actual implementation, or
the implementation should be updated to use a calculated TTL based on burst and
rate parameters.
```suggestion
if ttl > 0 and ttl <= 2 then
```
##########
t/plugin/limit-req-redis-cluster-ttl.t:
##########
@@ -0,0 +1,168 @@
+#
+# 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';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+no_shuffle();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+ my $config = $block->config // <<_EOC_;
+ location /check {
+ content_by_lua_block {
+ local redis = require "resty.redis"
+ local redis_cluster = require "resty.rediscluster"
+ local serv_list = {
+ { ip = "127.0.0.1", port = 5000 },
+ { ip = "127.0.0.1", port = 5001 },
+ { ip = "127.0.0.1", port = 5002 },
+ { ip = "127.0.0.1", port = 5003 },
+ { ip = "127.0.0.1", port = 5004 },
+ { ip = "127.0.0.1", port = 5005 },
+ }
+ local config = {
+ name = "test-cluster",
+ serv_list = serv_list,
+ keepalive_timeout = 60000,
+ keepalive_cons = 1000,
+ connect_timeout = 1000,
+ socket_timeout = 1000,
+ dict_name = "plugin-limit-req-redis-cluster-slot-lock",
+ }
+
+ -- Flush all keys in the cluster
+ for _, node in ipairs(serv_list) do
+ local red = redis:new()
+ red:set_timeout(1000)
+ local ok, err = red:connect(node.ip, node.port)
+ if ok then
+ red:flushall()
+ red:close()
+ end
+ end
+
+ local red = redis_cluster:new(config)
+
+ -- make a request to /access
+ local httpc = require("resty.http").new()
+ local uri = "http://127.0.0.1:"; .. ngx.var.server_port .. "/access"
+ local res, err = httpc:request_uri(uri, {
+ method = "GET"
+ })
+
+ if not res then
+ ngx.say("failed to request: ", err)
+ return
+ end
+
+ local found_key
+ for _, node in ipairs(serv_list) do
+ local red_node = redis:new()
+ red_node:set_timeout(1000)
+ local ok, err = red_node:connect(node.ip, node.port)
+ if ok then
+ local keys, err =
red_node:keys("limit_req:limit_req_ttl_test_*")
+ if keys and #keys > 0 then
+ found_key = keys[1]
+ red_node:close()
+ break
+ end
+ red_node:close()
+ end
+ end
+
+ if not found_key then
+ ngx.say("no keys found")
+ return
+ end
+
+ local ttl, err = red:ttl(found_key)
+
+ if not ttl or ttl == -2 then
+ ngx.say("no keys found")
+ return
+ end
+
+ -- for rate=1, burst=10 -> ttl = ceil(10/1)+1 = 11.
+ if ttl > 0 and ttl <= 11 then
Review Comment:
The comment indicates that the TTL calculation formula is ceil(burst/rate) +
1, which would equal 11 seconds for rate=1 and burst=10. However, this formula
and comment appear to be copied from limit-req tests but are actually checking
a hardcoded 2-second TTL in the implementation. This comment is misleading and
should be updated to reflect the actual TTL value of 2 seconds that is set in
the implementation.
```suggestion
-- ttl is expected to be 2 seconds as set by the current
limit-req implementation.
if ttl == 2 then
```
##########
apisix/utils/redis-schema.lua:
##########
@@ -78,4 +78,14 @@ local _M = {
schema = policy_to_additional_properties
}
+
+function _M.ttl_policy_schema(kind, ttl)
+ local schema = policy_to_additional_properties[kind]
+ schema.properties.key_ttl = {
+ type = "integer", default = ttl,
+ }
+ return schema
+end
Review Comment:
This function mutates the original schema object from
policy_to_additional_properties by directly adding the key_ttl property. This
means that after the first call, the schema will be modified permanently for
all subsequent uses. This could lead to unexpected behavior if the schema is
reused elsewhere.
Consider creating a deep copy of the schema before adding the key_ttl
property to avoid mutating the shared schema object.
##########
t/plugin/limit-req-redis-cluster-ttl.t:
##########
@@ -0,0 +1,168 @@
+#
+# 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';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+no_shuffle();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+ my $config = $block->config // <<_EOC_;
+ location /check {
+ content_by_lua_block {
+ local redis = require "resty.redis"
+ local redis_cluster = require "resty.rediscluster"
+ local serv_list = {
+ { ip = "127.0.0.1", port = 5000 },
+ { ip = "127.0.0.1", port = 5001 },
+ { ip = "127.0.0.1", port = 5002 },
+ { ip = "127.0.0.1", port = 5003 },
+ { ip = "127.0.0.1", port = 5004 },
+ { ip = "127.0.0.1", port = 5005 },
+ }
+ local config = {
+ name = "test-cluster",
+ serv_list = serv_list,
+ keepalive_timeout = 60000,
+ keepalive_cons = 1000,
+ connect_timeout = 1000,
+ socket_timeout = 1000,
+ dict_name = "plugin-limit-req-redis-cluster-slot-lock",
+ }
+
+ -- Flush all keys in the cluster
+ for _, node in ipairs(serv_list) do
+ local red = redis:new()
+ red:set_timeout(1000)
+ local ok, err = red:connect(node.ip, node.port)
+ if ok then
+ red:flushall()
+ red:close()
+ end
+ end
+
+ local red = redis_cluster:new(config)
+
+ -- make a request to /access
+ local httpc = require("resty.http").new()
+ local uri = "http://127.0.0.1:"; .. ngx.var.server_port .. "/access"
+ local res, err = httpc:request_uri(uri, {
+ method = "GET"
+ })
+
+ if not res then
+ ngx.say("failed to request: ", err)
+ return
+ end
+
+ local found_key
+ for _, node in ipairs(serv_list) do
+ local red_node = redis:new()
+ red_node:set_timeout(1000)
+ local ok, err = red_node:connect(node.ip, node.port)
+ if ok then
+ local keys, err =
red_node:keys("limit_req:limit_req_ttl_test_*")
+ if keys and #keys > 0 then
+ found_key = keys[1]
+ red_node:close()
+ break
+ end
+ red_node:close()
+ end
+ end
+
+ if not found_key then
+ ngx.say("no keys found")
+ return
+ end
+
+ local ttl, err = red:ttl(found_key)
+
+ if not ttl or ttl == -2 then
+ ngx.say("no keys found")
+ return
+ end
+
+ -- for rate=1, burst=10 -> ttl = ceil(10/1)+1 = 11.
+ if ttl > 0 and ttl <= 11 then
Review Comment:
Similar to the redis test, this test expects a TTL of up to 11 seconds based
on the formula ceil(burst/rate) + 1, but the implementation uses a hardcoded
2-second TTL. The test should check for TTL <= 2 to match the actual
implementation, or the implementation should use a calculated TTL.
```suggestion
-- implementation currently uses a fixed TTL of 2 seconds
if ttl > 0 and ttl <= 2 then
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
