SncRedisBundle

SncRedisBundle copied to clipboard

Hi guys, I've been dealing with an issue that is not directly related to this bundle If you want more details https://github.com/phpredis/phpredis/issues/1726 and https://bugs.php.net/bug.php?id=79501

TL;DR with PHP >= 7.4, tls v1.3 is used by default and seems to result in random frozen connections (Both Predis and PHPRedis sa it's at the php layer) The quick fix in the meanwhile is to force the connection to use tls 1.2, this is achieved by using the following connection string tlsv1.2://my-redis-server:6379 instead of tls://my-redis-server:6379

After looking at the factories, the change is pretty basic to do Here: https://github.com/snc/SncRedisBundle/blob/master/src/Factory/PhpredisClientFactory.php#L134 And here: https://github.com/snc/SncRedisBundle/blob/master/src/Factory/PredisParametersFactory.php#L49

But what I am not sure about and why I am creating this issue is to ask your opinion on how to provide this information in the snc bundle config.

So far I've come up with 2 ideas:

1. Add an option in the config (Easy and quick to do)

snc_redis:
    clients:
        default:
            type: phpredis
            dsn: 'rediss://[email protected]:6379'
            options:
                tls_version: 1.2

2. Add the information in the DSN (Requires extra parsing and "hard" to understand but closer to what is done by the bundle)

snc_redis:
    clients:
        default:
            type: phpredis
            dsn: 'redissv1.2://[email protected]:6379'

I'm gonna provide a MR for that, I just wanted your opinion first
On my side I think I'm leaning more towards the 2nd option as it's closer to the reality even if more complex

I've noticed this issue too: https://github.com/snc/SncRedisBundle/issues/604 which is kind of related without being related
The TLS version is not an SSL options but part of the URL itself

Depends how likely is that this version differs between environments, eg. prod/dev. If it's unlikely, non-dns option is fine. Otherwise both should be supported, like with other connection related options

I think it's unlikely to have a different tls version between environments, so i'll start with the non-dsn option and i'll see for the other one later

But I think it's likely that prod will have TLS enabled, but dev not. What would happen in that case in dev if tls_version is specified?

I guess you are right this is something that can happen even if I don't really like it but anyway I had in mind to ignore the tls_version options if TLS is not enabled

Yeah that's not very intuitive. I was thinking that since DSN will be different for prod/dev anyways, TLS could also be part of it. Similar like is done with server_version in DBAL.

I agree it can lead to confusion but also why would anyone specify a TLS version if they are not using encryption? But I am not against adding it to the DSN, it makes sense

I'm not sure to follow what you mean with the server_version in DBAL, isn't it the opposite of inside the DSN?

No, serverVersion can be added inside DATABASE_URL like example at https://github.com/symfony/recipes/blob/7d43cf1da3f24d9470a4a41cee19cf72a03f553b/doctrine/doctrine-bundle/2.4/manifest.json#L14

Ohhh I wasn't aware of this, yep it definitely sounds like the best option here let's move in this direction

We've been experiencing some slow connections to redis lately and after a lot of debugging and searching it seems our issue is exactly this... any workaround we can use while this is implemented? ( we are using predis) Thank you

I'll try to resume my work on this feature for the bundle this evening or at least this week, it shouldn't take long I was just kind of struggling to correctly 100% unit test it
As a workaround in the meanwhile I guess the easiest would be to downgrade the php version, I remember also some people talking about adding stunnel locally to "proxy" the request using tls 1.2 all the time but it implies extra infrastructure work