feat: ai rate limiting redis support #12751
Conversation
dosubot
bot
added
size:XL
beardnick
changed the title
|
@Baoyuantop PTAL |
|
@beardnick many thx for your contribution, some CI tests are failed, you can fix them |
| @@ -0,0 +1,85 @@ | |||
| -- | |||
There was a problem hiding this comment.
apisix/plugins/limit-count/util.lua
I checked _M.incoming and _M.log_phase_incoming, and they both work for redis.
We should add redis to the filename of the function name.
apisix/plugins/limit-count/init.lua
Outdated
| local phase = get_phase() | ||
| if phase == "log" then | ||
| if not conf.policy or conf.policy == "local" then | ||
| lim:incoming(key, not dry_run, conf, cost) |
There was a problem hiding this comment.
This conditional statement is unnecessary, as an identical check is performed again a few lines later:
local delay, remaining, reset
if not conf.policy or conf.policy == "local" then
delay, remaining, reset = lim:incoming(key, not dry_run, conf, cost)
else
delay, remaining, reset = lim:incoming(key, cost, dry_run)
endAnd why do we need to perform two incoming operations during the log phase?
| local peek_script = core.string.compress_script([=[ | ||
| local ttl = redis.call('ttl', KEYS[1]) | ||
| local limit = tonumber(ARGV[1]) | ||
| local cost = tonumber(ARGV[3]) | ||
| if ttl < 0 then | ||
| return {limit - cost, tonumber(ARGV[2])} | ||
| end | ||
| local current = redis.call('get', KEYS[1]) | ||
| if not current then | ||
| return {limit - cost, tonumber(ARGV[2])} | ||
| end | ||
| return {tonumber(current) - cost, ttl} | ||
| ]=]) | ||
|
|
There was a problem hiding this comment.
Is it necessary to use a separate Redis script? Can we just remove
assert(tonumber(ARGV[3]) >= 1, "cost must be at least 1")
from the commit_script directly?
| location /test_concurrency { | ||
| content_by_lua_block { | ||
| require("lib.test_redis").flush_all() |
There was a problem hiding this comment.
Why were we able to pass the tests without this flush before?
There was a problem hiding this comment.
Look like the changes in this PR should be limited to the ai-rate-limiting plugin and minor tweaks to the limit-count plugin. The current PR modifies too much code and test cases for other rate limiting plugins, expanding its scope. We should minimize unnecessary code changes.
| local commit = true | ||
| if dry_run ~= nil then | ||
| commit = not dry_run | ||
| end | ||
|
|
||
| local ttl = 0 | ||
| local res, err = red:eval(script, 1, key, limit, window, cost or 1) | ||
| return util.redis_incoming(self, self.red_cli, key, commit, cost) |
There was a problem hiding this comment.
Can we directly check ngx.phase here? This way, the APIs of limit-count-redis-cluster.lua and limit-count-redis.lua do not need to be changed, still only providing an incoming function without introducing a new log_phase_incoming.
4 participants
Description
Which issue(s) this PR fixes:
Fixes #12482
Notice
limit-count-redis.luaandlimit-count-redis-cluster.luafiles to ensure they now support rate-limiting during the log phase.limit-conn-redis.luafile as a guide while implementing rate-limiting in the log phase.require("lib.test_redis").flush_all()function to every Redis rate-limiting test case. Without this addition, the tests could fail unpredictably.limit-req-redis.tandlimit-req-redis-cluster.ttest cases. After thorough verification, the correct behavior is[200, 403, 403, 403]. Previously, the results appeared as[403, 403, 403, 403]due to Redis not being properly cleaned beforehand.Checklist