search

redis / go-redis

Redis Go client
https://redis.uptrace.dev
BSD 2-Clause "Simplified" License
20.2k stars 2.38k forks source link

Remove direct read from TLS underlying conn #3138

Closed rentziass closed 1 month ago

rentziass commented 1 month ago

Addresses #3137.

This essentially reverts https://github.com/redis/go-redis/commit/0858ed24e6d08c1940bcb86ccbf8913c761d9ec0, to avoid corrupting TLS sessions by directly reading from the underlying net.Conn. This is blocking upgrades to > 9.5.2 if using TLS.

conn_check.go is quite different on the v9.6 branch (it changed here), should I open another PR to fix that as well?

rentziass commented 1 month ago

@ofekshenawa @vladvildanov sorry for the direct ping, would anyone of you be able to have a look at this please?

vladvildanov commented 1 month ago

@rentziass Hey! Thanks for reaching us out! Just for the context, currently it reads from TLS underlying socket, so it means that it reads unencrypted data?

rentziass commented 1 month ago

@vladvildanov 👋 it's all in the issue but the tldr is that reading from (*tls.Conn).NetConn() directly corrupts TLS sessions (from NetConn() docs) and we found ourselves creating millions of connections versus the usual few thousands because of that. To be honest I'm surprised no other cluster user noticed this, it puts some noticeable pressure on the Redis cluster CPU. We thought it was an organic increase in load but it was just the constant re-creation of connections.