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

add ssl support for redis with sentinel #1327

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cyberjunk
Copy link

flower can already connect to sentinel using tls by setting sentinel_kwargs in broker_options accordingly, e.g.:

'sentinel_kwargs': { 'ssl': True, 'ssl_cert_reqs': ssl.CERT_NONE }

to enable tls to sentinel without sending any client certificate or validating the server certificate.
this will be passed to the Sentinel() constructor and works fine for the connection from flower to sentinel.

however, this is only one of the two connections made from flower in a sentinel+redis setup...
the next connection which is made to the returned redis master was always without tls.

to make the connection to redis also use tls, one has to pass the according ssl related connection_kwargs to Sentinel() constructor.

I adapted the code from the existing encrypted redis-only case by reusing broker_use_ssl settings in the sentinel+redis setup to configure whether the connection to redis should be made using tls or not. checking if broker_use_ssl is defined, and if so, set ssl to True and inject the parameters provided in broker_use_ssl for the connection from flower to redis. this way flower can be configured to use tls on both, the sentinel AND the redis connection using existing configuration settings and it's working fine for me now...

PS: This also fixes a 500 error for redis+sentinel on /broker route with a server error 104 - Connection reset by peer indicating that connection to redis was made without tls.

@mher
Copy link
Owner

mher commented Sep 27, 2023

Thanks for the pull request! Can you add unit tests?

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.

None yet

2 participants