-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
SentinelManagedConnection needs a SSLSentinelManagedConnection to complement SSLConnection #1306
Labels
Comments
nickwilliams-eventbrite
added a commit
to eventbrite/pysoa
that referenced
this issue
Mar 13, 2020
- Update functional tests to use Redis 5 and 6 instead of Redis 4 and 5. - Update Redis 6 Sentinel tests to use ACLs (authentication) and TLSv1.2 (TLSv1.3 cannot be supported on Ubuntu 16.04). - Add a few more functional tests. - This proves that: - PySOA can use Redis 6 without ACL and TLS - PySOA can use Redis 6 with ACL - PySOA can use Redis 6 with TLS with unprotected Sentinel - PySOA can use Redis 6 with TLS with Sentinel with TLS - Created redis/redis-py#1306. - Created redis/redis#6986. - Created antirez/redis#6985.
I can confirm this issue exists. I believe @nickwilliams-eventbrite's solution is the correct approach. |
@nickwilliams-eventbrite Hey Nick, sorry it's taken so long for me to get to this. If you're still open to making a PR I'm fine with multiple inheritance. |
4 tasks
This issue is marked stale. It will be closed in 30 days if it is not updated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Version: 3.4.1
Platform: 2.7.17, 3.5.9, and 3.7.6
Description: With Redis 6, Redis provides TLS support directly. This means that replicas and Sentinels can communicate with Redis over TLS. When Sentinel communicates with Redis over TLS, it gives its clients the TLS address for the Redis master and replicas when the master and replica address are requested. However, the
Sentinel
client in this library contains onlySentinelManagedConnection
, andSentinelManagedConnection
does not understand TLS. It extendsConnection
directly.So, if I configure my Sentinel like this:
I get a
TypeError
thatSentinelManagedConnection
does not have keyword argumentsssl_ca_certs
, etc. But if I configure it like this, without SSL info:I get SSL handshake errors on the Redis server and unexpected-disconnect errors on the client (because the server is expecting a TLS handshake and the client is not).
I was able to solve this problem very simply. I created this class:
And then I configured my Sentinel like this:
Now it works perfectly.
The
Sentinel
class should work similarly to theRedis
class—it should doconnection_kwargs.pop('ssl', False)
and, if that value is true, it should change the connection class fromSentinelManagedConnection
toSSLSentinelManagedConnection
.I could happily and quickly put together a pull request implementing this, but I didn't know how you felt about multiple inheritance and thought you might want to take a different approach, so I'll wait a few days for word from you about how you feel about my approach before I put that together.
The text was updated successfully, but these errors were encountered: