SentinelManagedConnection needs a SSLSentinelManagedConnection to complement SSLConnection

[nickwilliams-eventbrite]

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 only SentinelManagedConnection, and SentinelManagedConnection does not understand TLS. It extends Connection directly.

So, if I configure my Sentinel like this:

self._sentinel = redis.sentinel.Sentinel(
    hosts,
    sentinel_kwargs={'socket_connect_timeout': 5.0, 'socket_timeout': 5.0},
    **{
        'ssl_ca_certs': '/srv/run/tls/ca.crt',
        'ssl_certfile': '/srv/run/tls/redis.crt',
        'ssl_keyfile': '/srv/run/tls/redis.key',
    },
)

I get a TypeError that SentinelManagedConnection does not have keyword arguments ssl_ca_certs, etc. But if I configure it like this, without SSL info:

self._sentinel = redis.sentinel.Sentinel(
    hosts,
    sentinel_kwargs={'socket_connect_timeout': 5.0, 'socket_timeout': 5.0},
    **{},
)

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:

class SSLSentinelManagedConnection(redis.sentinel.SentinelManagedConnection, redis.SSLConnection):
    def __init__(self, **kwargs):
        self.connection_pool = kwargs.pop('connection_pool')  # replicate the init code from SentinelManagedConnection
        redis.SSLConnection.__init__(self, **kwargs)  # we cannot use super() here because it is not first in the MRO

And then I configured my Sentinel like this:

self._sentinel = redis.sentinel.Sentinel(
    hosts,
    sentinel_kwargs={'socket_connect_timeout': 5.0, 'socket_timeout': 5.0},
    **{
        'connection_class': SSLSentinelManagedConnection,
        'ssl_ca_certs': '/srv/run/tls/ca.crt',
        'ssl_certfile': '/srv/run/tls/redis.crt',
        'ssl_keyfile': '/srv/run/tls/redis.key',
    },
)

Now it works perfectly.

The Sentinel class should work similarly to the Redis class—it should do connection_kwargs.pop('ssl', False) and, if that value is true, it should change the connection class from SentinelManagedConnection to SSLSentinelManagedConnection.

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.

[nishantgeorge]

I can confirm this issue exists. I believe @nickwilliams-eventbrite's solution is the correct approach.

[andymccurdy]

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

[github-actions[bot]]

This issue is marked stale. It will be closed in 30 days if it is not updated.