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

Fix problems connecting to redis sentinel with SSL #5660

Merged
merged 3 commits into from
Jun 28, 2022
Merged

Conversation

amanda11
Copy link
Contributor

When connecting to redis SSL via sentinel then 2 problems are encountered:

  1. redis 3.5.3 did not support ssl parameters for sentinel, see sentinel: Add SentinelManagedSSLConnection redis/redis-py#1419. So upping the version of redis library
  2. When using eventlet and redis SSL then there are connection problems, due to the fact the redis library has expected timeouts, but monkey patched SSL library is throwing different exception. See Eventlet raises a different exception on SSL timeouts vs standard Python eventlet/eventlet#692. This causes problems on connecting to redis SSL via sentinel. It also is likely to be the cause of error: SSL Error with st2 sensor connection redis #5337 as the stack trace encountered is the same.
    Therefore when monkey patching SSL library, keep the exception raised on timeout to be socket.timeout.

@pull-request-size pull-request-size bot added the size/S PR that changes 10-29 lines. Very easy to review. label Jun 21, 2022
@amanda11 amanda11 marked this pull request as draft June 21, 2022 10:03
@amanda11 amanda11 changed the title Fix problems connecting to redis sentinel with SSL [WIP] Fix problems connecting to redis sentinel with SSL Jun 21, 2022
@amanda11 amanda11 added this to the 3.8.0 milestone Jun 21, 2022
@amanda11 amanda11 marked this pull request as ready for review June 21, 2022 11:15
@amanda11 amanda11 changed the title [WIP] Fix problems connecting to redis sentinel with SSL Fix problems connecting to redis sentinel with SSL Jun 21, 2022
@raviorch
Copy link

Amanda, Please let us know which version of Redis you used to fix the Redis and Sentinel issues for TLS. Redis officially supports TLS from their 6.0 version onwards. Text from redis.io : "SSL/TLS is supported by Redis starting with version 6 as an optional feature that needs to be enabled at compile time." Are you using the latest Redis version 7.0 or 6.2.2?

@amanda11
Copy link
Contributor Author

amanda11 commented Jun 23, 2022

I used 6.2.7 and compiled at run-time to enable SSL for my testing (which is latest on the 6.2 stream). But the changes here are compatible with non-SSL and SSL redis versions, so work fine with the versions that the bash installer uses (though in those cases the package manager will pull down non-SSL version of redis), therefore if using the package manager redis you won't have SSL support.
I have no reason to believe this won't work with redis 7, but I have not verified that.
This Pr is to resolve problems found with users that install redis separately with TLS enabled.

@raviorch
Copy link

Amanda, Thanks for the update, I was wondering if the ssl fix that you have put in also has the fix for providing an username password for Redis. Please let us know.

@amanda11
Copy link
Contributor Author

amanda11 commented Jun 24, 2022

As far as I know there is no issue with username/password with non-ssl redis. I've used StackStorm connected to redis secured by username/password before quite happily, on many StackStorm versions including 3.7.0

@amanda11 amanda11 requested a review from a team June 27, 2022 09:36
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventlet strikes again. 🙄

@amanda11 amanda11 merged commit 2eb6eee into master Jun 28, 2022
@amanda11 amanda11 deleted the support_redis_ssl branch June 28, 2022 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/S PR that changes 10-29 lines. Very easy to review.
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants