Skip to content
This repository has been archived by the owner on Feb 21, 2023. It is now read-only.

Allow ssl connection to redis when use sentinel #953

Conversation

stanislau-arkhipenka
Copy link
Contributor

@stanislau-arkhipenka stanislau-arkhipenka commented Apr 22, 2021

This diff allows to pass ssl=True inside **connection_kwargs for Sentinel class to enable ssl connection to redis
Besides enabling functionality itself this change also makes experience with Sentinel() more consistent with Redis()

Now SentinelManagedConnection enherits from SSLConnection, instead of Connection.
This allows to populate ssl related properties from SSLConnection class.
if ssl=True SentinelManagedConnection uses parents constructor (SSLConnection)
If ssl=False SentinelManagedConnection uses grandparents constructor (Connection class)

The only downside of this implementation, is in case ssl=False instance of SentinelManagedConnection has dangling ssl-cert related properties.This properties does not affect behavior in any considerable way.
Afaik there is no obvious way to overcome this issue.

Users now can pass SSL related attributes to **connection_kwargs in Sentinel class (or alternativelly to master_for or slave_for methods)

This diff allows to pass ssl=True inside **connection_kwargs for Sentinel class to enable ssl connection to redis
Besides enabling functionality itself this change also makes experience with Sentinel() more consistent with Redis()
@codecov
Copy link

codecov bot commented Apr 22, 2021

Codecov Report

Merging #953 (dd4f77a) into master (8c7c908) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #953   +/-   ##
=======================================
  Coverage   91.85%   91.85%           
=======================================
  Files          17       17           
  Lines        5808     5808           
  Branches      489      489           
=======================================
  Hits         5335     5335           
  Misses        360      360           
  Partials      113      113           
Flag Coverage Δ
unit 91.83% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c7c908...dd4f77a. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Apr 22, 2021

This pull request introduces 2 alerts when merging 431c960 into 260888e - view on LGTM.com

new alerts:

  • 2 for Conflicting attributes in base classes

This diff allows to pass ssl=True inside **connection_kwargs for Sentinel class to enable ssl connection to redis
Besides enabling functionality itself this change also makes experience with Sentinel() more consistent with Redis()
@lgtm-com
Copy link

lgtm-com bot commented Apr 23, 2021

This pull request introduces 2 alerts when merging a7c854c into 260888e - view on LGTM.com

new alerts:

  • 2 for Conflicting attributes in base classes

@stanislau-arkhipenka
Copy link
Contributor Author

@Andrew-Chen-Wang any thoughts regarding this request?

@lgtm-com
Copy link

lgtm-com bot commented Apr 26, 2021

This pull request introduces 2 alerts when merging dd4f77a into 260888e - view on LGTM.com

new alerts:

  • 1 for Multiple calls to `__init__` during object initialization
  • 1 for First argument to super() is not enclosing class

@Andrew-Chen-Wang
Copy link
Collaborator

@stanislau-arkhipenka apologies for being super late. I'll look into this PR after finals, but take note of the PR in redis-py first: redis/redis-py#1306 I haven't take too much of a look through there, but it seems like they're doing some inheritance. Try and follow along and see if there are any diffs.

Copy link
Collaborator

@Andrew-Chen-Wang Andrew-Chen-Wang left a comment

Choose a reason for hiding this comment

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

@stanislau-arkhipenka Do you mind replicating what's been done here instead of using an "ssl" option to be more close with redis-py implementation: redis/redis-py#1419

That PR's not been reviewed by @andymccurdy yet, but the implementation and comments made by him and others (and others' implementation like that at Celery) based on the original issue seems to be The Way: redis/redis-py#1306

@stanislau-arkhipenka
Copy link
Contributor Author

@stanislau-arkhipenka Do you mind replicating what's been done here instead of using an "ssl" option to be more close with redis-py implementation: andymccurdy/redis-py#1419

That PR's not been reviewed by @andymccurdy yet, but the implementation and comments made by him and others (and others' implementation like that at Celery) based on the original issue seems to be The Way: andymccurdy/redis-py#1306

Hey, actually originally i did pretty similar diff, but LGTM was not very happy because of diamand problem. MRO seems solves this problem, but it'll be definitely harder to read the code.
Revert?

Copy link
Collaborator

@Andrew-Chen-Wang Andrew-Chen-Wang left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and fixing up the LGTM!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants