Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove direct read from TLS underlying conn #3138

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

rentziass
Copy link
Contributor

Addresses #3137.

This essentially reverts 0858ed2, 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
Copy link
Contributor Author

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

@vladvildanov
Copy link
Collaborator

@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
Copy link
Contributor Author

@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.

@vladvildanov vladvildanov merged commit e786862 into redis:master Oct 7, 2024
10 checks passed
rentziass added a commit to rentziass/go-redis that referenced this pull request Oct 7, 2024
@rentziass rentziass mentioned this pull request Oct 7, 2024
vladvildanov pushed a commit that referenced this pull request Oct 9, 2024
vladvildanov pushed a commit to vladvildanov/go-redis that referenced this pull request Oct 14, 2024
vladvildanov added a commit that referenced this pull request Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants