Skip to content

Share ssl_* options with the Sentinel daemon connections on TLS URLs#445

Open
fatih-acar wants to merge 1 commit into
chrisguidry:mainfrom
fatih-acar:share-ssl-options-with-sentinel-daemons
Open

Share ssl_* options with the Sentinel daemon connections on TLS URLs#445
fatih-acar wants to merge 1 commit into
chrisguidry:mainfrom
fatih-acar:share-ssl-options-with-sentinel-daemons

Conversation

@fatih-acar

Copy link
Copy Markdown
Contributor

This PR shares the funneled ssl_* connection options with the Sentinel daemon connections on rediss+sentinel:// URLs, so a single private CA covers the whole topology.

Problem

parse_sentinel_url funnels the remaining query parameters through redis-py's parse_url and applies them to the data-node connections only; sentinel_kwargs carries just sentinel_username/sentinel_password and the scheme-derived ssl=True. That means on a TLS topology with a private or self-signed CA:

  • ?ssl_ca_certs=/etc/redis/ca.pem configures the master connections, but every connection to a Sentinel daemon still verifies against the system trust store with ssl_cert_reqs="required";
  • discovery fails for all daemons and master resolution raises MasterNotFoundError before a data connection is ever attempted — there is no way to make a private-CA rediss+sentinel:// URL work at all (the same applies to ?ssl_cert_reqs=none for deliberately unverified setups).

Reproduced against a real TLS Sentinel topology in Docker (master + 2 replicas + 3 Sentinels on TLS-only ports, tls-replication yes, self-signed CA): every variant failed with MasterNotFoundError, with the daemon connection reprs showing ssl_ca_certs=None, ssl_cert_reqs=required.

Fix

When the scheme is rediss+sentinel, the daemon connections now reuse the URL's funneled ssl_* options (ssl_ca_certs, ssl_cert_reqs, ssl_check_hostname, ...). Plaintext redis+sentinel:// URLs are unaffected — ssl_* options keep applying to the data nodes only, like any other redis-py option.

With the fix, the same Docker topology passes end to end: CA-verified discovery and reads/writes, the ?ssl_cert_reqs=none&ssl_check_hostname=false unverified variant, rejection when the CA is omitted (negative control), and a master SIGKILL failover under TLS (~3s, client follows the promotion).

Notes

  • prefect-redis, whose Sentinel parser deliberately mirrors this module, hit exactly this in review testing and applied the same fix in Add Redis Sentinel support to prefect-redis PrefectHQ/prefect#22377 — this PR keeps the two convergent.
  • Tests cover both the TLS sharing and the plaintext non-sharing case; pytest tests/test_sentinel_urls.py (34 passed) and prek run (ruff, pyright, loq) are green.

🤖 Generated with Claude Code

The funneled ssl_* query options previously reached only the data-node
connections, so a private CA passed as ?ssl_ca_certs= (or
?ssl_cert_reqs=none for unverified setups) left every Sentinel daemon
connection verifying against the system trust store; discovery failed
with MasterNotFoundError before a master could ever be resolved.

On rediss+sentinel:// URLs the daemon connections now reuse the URL's
ssl_* options, so one CA covers the whole topology. Plaintext daemons
are unaffected.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.

1 participant