feat(*): support default database

[outsinre]

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

[samugi]

If I understood this change correctly, we are making sure that the session library selects database 0 when conf.database is not specified for the session's storage.

This is normally not needed, because Redis' default database is also 0, and the database is not a required parameter to establish a connection, so it'd feel more clean to let Redis do its thing and automatically default to 0 without setting it explicitly ourselves.

However, the reason this is done here (please confirm) is that in case of connection reuse by the redis client, if the pool key is not set properly, we may end up reusing connections to different databases which may result in unexpected content on the storage and failures. This change ensures that the database is automatically switched to 0 whenever it's not specified, so this would happen with client_1 and client_2 sharing the same pool:

1. client_1 connects to redis with conf.database=7
2. client_1 explicitly selects db: 7
3. client_2 wants to use db: 0, it has conf.database=nil, it reuses open connection from client_1 and is connected to db: 7
4. cilent_2 explicitly selects db: 0

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.pool configuration 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.

[outsinre]

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.pool configuration 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 #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.

[outsinre]

@bungle could you check this PR?

[bungle]

I feel the same as @samugi. @outsinre there are:

1. application that uses this library
2. this library that uses a redis connection driver (like lua-resty-redis)
3. 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?

[outsinre]

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.