fix(plugin) reuse redis conn for different db & auth optimize #3293
Conversation
|
@mengskysama Hello, thank you for sending a PR! Could you include in it a regression test for the issue you are meaning to fix? |
| if conf.redis_database ~= nil then | ||
| db = conf.redis_database | ||
| end | ||
| local ok, err = red:select(db) |
There was a problem hiding this comment.
The PR makes sense. A comment here to explain why we need to select another database (even after the previous get_reused_times() would help. The fact that there is no way to retrieve which database the socket is connected to (from what I can see) is slightly annoying, we are making unnecessary round-trips, so I suspect this to raise eyebrows in the future.
| local db = 0 | ||
| if conf.redis_database ~= nil then | ||
| db = conf.redis_database | ||
| end |
There was a problem hiding this comment.
Simpler to do:
local db = conf.redis_database or 0Or even
local ok, err = red:select(conf.redis_database or 0)| ngx_log(ngx.ERR, "failed to get connect reused times: ", err) | ||
| return nil, err | ||
| end | ||
| if conf.redis_password and conf.redis_password ~= "" and times == 0 then |
There was a problem hiding this comment.
Let's move the check for times == 0 at the beginning of this condition, like so:
if times == 0 and conf.redis_password and conf.redis_password ~= "" then
endThere was a problem hiding this comment.
btw, this is slightly unrelated to the fix you are trying to write (not send the authentication frame for already established connections vs. always switching to the configured database) - this could be part of another PR (or at least another commit in the same PR, for the sake of atomicity).
thibaultcha
added
the
pr/changes requested
|
I agree with @hishamhm - setting up the test case for this will be non-trivial though... |
| ngx_log(ngx.ERR, "failed to change Redis database: ", err) | ||
| return nil, err | ||
| end | ||
| local ok, err = red:select(conf.redis_database or 0) |
There was a problem hiding this comment.
Could we add a comment above this line to explain why we need this? As we like to say, we do not trust our future selves to remember that this is because the redis connection pool conflicts with that of other instances of this plugin, or that of the response-ratelimiting plugin :)
Thanks!
| local client | ||
|
|
||
| setup(function() | ||
| helpers.dao:drop_schema() |
There was a problem hiding this comment.
This will immensely slow down the tests and should ultimately not be needed. Instead, I suggest:
helpers.run_migrations()(make sure we are up-to-date or run the migrations if not)helpers.dao:truncate_tables()
| local function flush_redis(db) | ||
| local red = redis:new() | ||
| red:set_timeout(2000) | ||
| local ok, err = red:connect(REDIS_HOST, REDIS_PORT) |
There was a problem hiding this comment.
Let's use the proper Lua idioms here and save us a lot of boilerplate:
assert(red:connect())
assert(red:auth())
assert(red:select())|
|
||
| teardown(function() | ||
| flush_redis(REDIS_DATABASE) | ||
| flush_redis(REDIS_DATABASE_SECOND) |
There was a problem hiding this comment.
I'm not sure how fast those operations are, but if we can avoid it, we should. There is no point in cleaning up behind ourselves in our tests, since they always run in a test instance and because it is actually beneficial to preserve the tests results, for a potential investigation around a failing test. We do this a lot. Plus the setup() hooks performs the same operations as well for the next run of this test suite.
| red:set_timeout(2000) | ||
| local ok, err = red:connect(REDIS_HOST, REDIS_PORT) | ||
| if not ok then | ||
| error("failed to connect to Redis: " .. err) |
There was a problem hiding this comment.
ditto: let's follow the Lua idioms and also make this a bit more readable (a few line jumps would be nice)
| assert.are.same(1, tonumber(val1)) | ||
| assert.are.same(0, tonumber(val2)) | ||
|
|
||
| -- rate-limiting plugin reuse api1 redis connection |
There was a problem hiding this comment.
Curious: did you verify that this test fails with consistency without your patch?
There was a problem hiding this comment.
I verified this test cash get fail if not patched this commit.
spec/03-plugins/24-rate-limiting/05-integration_spec.lua:164: Expected objects to be the same.
Passed in:
(number) 2
Expected:
(number) 1
There was a problem hiding this comment.
Ok, thanks for confirming :)
|
I already commit my change. Thank for your patience. |
Needed for rate-limiting plugins integration tests with the Redis strategy. From #3293 Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>
Since every instance of both rate-limiting plugin can share Redis connection pools, and because each plugin can be configured with a different Redis database, we must ensure that as each plugin gets processes in every request, it uses the Redis database it was configured for. We thus force database selection (see comment in code). Fix #3292 From #3293 Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>
|
Merged to master with some modifications. Thank you for the patch! |
Summary
If user configure response-ratelimiting、response-ratelimiting use same redis instance with
db0andotherwill get unexpected results.Full changelog
Issues resolved
Fix #3292