feat(*): support default database
We ensure the default database 0 is used if there is no database is provided, so that the correct database is selected whenever a connection with a different database is retrieved from the connection pool.
FTI-5839
Question: wouldn't it be better, whenever this problem exists, to properly set the pool key instead, to include the database id? This can be done by passing the
conf.poolconfiguration field.
It feels wrong to me having connections (to different databases) being reused with an additional step where the database is switched. The selection step would defeat the benefit of reusing a connection at all.
Fixing the issue via pool is feasible. However, this small PR is to ensure the library always switch to the default database 0 before read/write operations, as the resty.redis does not do this for us.
As a public lib, it may be used by multiple parties and even external users, and we do not know how they use this lib. They can reuse the connection for different databases, and we cannot assume anything. There are other libraries that using the default behaviour, like this https://github.com/ledgetech/lua-resty-redis-connector/blob/03dff6da124ec0a6ea6fa1f7fd2a0a47dc27c33d/lib/resty/redis/connector.lua#L426.
A closed PR https://github.com/bungle/lua-resty-session/pull/177 tries to use connection pool. It was closed because it should be implemented on the client side, namely the OIDC plugin side. It focuses on a more severe problem: vulnerability. This is now tracked at here https://konghq.atlassian.net/browse/FTI-5839?focusedCommentId=137641.
@bungle could you check this PR?
I feel the same as @samugi. @outsinre there are:
- application that uses this library
- this library that uses a redis connection driver (like lua-resty-redis)
- the connection driver
It is strange that we try to fix this issue in a middle man 2. I mean somewhere, the connection pooling does not work as expected, and why it is a problem of a middle man? Selecting database makes additional roundtrip to Redis. That feels like it defeats the purpose of connection pooling. We don't always execute in Kong e.g.
SET SCHEMA <schema>;
SET TIME ZONE <time-zone>;
when we get connection from connection pool. Still it is possible that some code or plugin changes the schema with SET SCHEMA and returns it back to pool, and then subsequent requests that expect different schema start to fail.
This issue is basically everywhere, so why are we solving it here in a middle man, making this library slower when using Redis? On normal case, not forcefully calling the SELECT is 99,999% of time fine. So this workarounds a rare issue, and pehaps introduces penalty on a common use case.
Thus I think we need to look this from either end, the application or the driver. This library already has that pool parameter that the application can use. The driver can possibly be more intelligent too (e.g. not do this: if omitted, then the connection pool name will be generated from the string template <host>:<port> or <unix-socket-path>.).
About the default database. Here is quote from Redis documentation:
Select the Redis logical database having the specified zero-based numeric index. New connections always use the database 0.
So why do we need to have DEFAULT_DATABASE=0 and that SELECT call when Redis defaults to 0 itself on new connections?
The DEFAULT_DATABASE=0 is setting a default database.
I think we all understand the issue. This PR is not a must for Kong as we have other solutions. It does introduce an RTT if the database is 0, but it guarantees the correct database is selected for Redis operations. We do not if other users (not Kong) using this lib can follow the correct pool setting.
I will prioritize the Kong side PR bugfix.