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

sentinel: Add SentinelManagedSSLConnection #1419

Merged
merged 1 commit into from
Oct 25, 2021

Conversation

AbdealiLoKo
Copy link
Contributor

@AbdealiLoKo AbdealiLoKo commented Nov 9, 2020

Pull Request check-list

Please make sure to review and check all of these items:

  • Does $ tox pass with this change (including linting)?
  • Does travis tests pass with this change (enable it first in your forked repo and wait for the travis build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

Description of change

Create a simple class which taken in both:

  • SentinelManaged
  • SSLConnection

in cases where the sentinel is being used with a TLS enabled redis
setup.

@AbdealiLoKo
Copy link
Contributor Author

Hi, following up on #1306
I had the same issue mentioned there. So, I've created the class mentioned so that we can get that merged. I've tested this on my side and also tested that. I had this issue with Celery/Kombu - I've also tested this with Celery in my project, and it all works fine.

Example code to make it work:

import redis
import redis.sentinel
sentinel = redis.sentinel.Sentinel(
    [('x.x.x.x', 29876),],
    connection_class=redis.sentinel.SentinelManagedSSLConnection,
    ssl_keyfile='/redis/tls-gen/basic/result/client_key.pem',
    ssl_certfile='/redis/tls-gen/basic/result/client_certificate.pem',
    ssl_ca_certs='/redis/tls-gen/basic/result/ca_certificate.pem',
    ssl_cert_reqs='required',
)

master = sentinel.master_for('mymaster')
master.ping()
master.get('a')

@AbdealiLoKo
Copy link
Contributor Author

@andymccurdy Any chance of getting this merged/released ?
I've done some pretty rigorous testing on all of my projects using Redis Sentinel + TLS + Celery and things look good

@chayim
Copy link
Contributor

chayim commented Aug 29, 2021

@AbdealiJK I'd love to get this into the upcoming release and support celery. Would you mind making the changes necessary to move things into the new mixins? After, that, I'll merge.

@Leletir
Copy link

Leletir commented Oct 25, 2021

Hello @chayim, what are the missing changes necessary?

Create a simple class which takes in both:
 - SentinelManaged
 - SSLConnection
in cases where the sentinel is being used with a TLS enabled redis
setup.
@AbdealiLoKo
Copy link
Contributor Author

Seems like I missed the original ping from @chayim
I have rebased my branch on the latest - do let me know if there are any other changes required to get it merged

Thanks @Leletir for the bump :)

@Leletir
Copy link

Leletir commented Oct 25, 2021

Thank you @AbdealiJK for your work, I ended up the same solution after one hour of reverse engineering, I should have checked the issues/PR before 😅

@chayim
Copy link
Contributor

chayim commented Oct 25, 2021

Looks great everyone! Thank you so much for following up and clearing this off. On the backlog side, I intend to get some test in, but this seems reasonable to me. Merging!

@chayim chayim merged commit 8924504 into redis:master Oct 25, 2021
@Leletir
Copy link

Leletir commented Oct 25, 2021

Thank you both !

@Leletir
Copy link

Leletir commented Oct 26, 2021

@chayim do you plan to release it before the 4.0 ?

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.

3 participants