Skip to content

redis_sentinel: Add SSL/TLS, username/ACL, and Redis 7+ replica support - #3071

Open
ian28223 wants to merge 5 commits into
masterfrom
fix/redis-sentinel-ssl-tls-support
Open

redis_sentinel: Add SSL/TLS, username/ACL, and Redis 7+ replica support#3071
ian28223 wants to merge 5 commits into
masterfrom
fix/redis-sentinel-ssl-tls-support

Conversation

@ian28223

@ian28223 ian28223 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Modernizes the Redis Sentinel check to support TLS-enabled deployments and Redis 6+ ACLs.

Supersedes #2939 (opened by @amendez-primer, auto-closed for inactivity after CI kept failing). Same change set, with the CI failures fixed and review feedback addressed:

  • Formatting: ran ddev test --fmt redis_sentinel with the current ddev version, which the original review asked for but the PR never got an updated pass on.
  • Dependency validation: ddev validate dep rejects unstable >= pins. The redis dependency is now pinned exactly (redis==7.3.0, matching redisdb in integrations-core) instead of redis>=4.5.0.
  • Minimum base package floor: bumped datadog-checks-base from >=4.2.0 to >=37.10.0. The old floor imports six.moves, which isn't declared as a hard dependency, and its [deps] extra pins an old ddtrace with no cp313 wheel — both broke the "minimum base package" CI job on unmodified master too, unrelated to this PR's changes.
  • _get_sentinel_replicas() removed: a reviewer caught that redis.StrictRedis never had a sentinel_replicas method in any released version, so the hasattr() check was always False and this always fell through to sentinel_slaves() — dead code with an inaccurate description (see below).
  • ssl_check_hostname added: a reviewer caught that redis-py defaults hostname verification to True and only disables it when ssl_cert_reqs is CERT_NONE, so a sentinel reached by IP with a hostname-mismatched cert had no escape short of disabling certificate verification entirely. Matches redisdb's existing option.

Fixes #2938, #2395

Changes

1. SSL/TLS support (#2938)

The check previously created redis.StrictRedis connections without any SSL parameters, making it impossible to connect to TLS-enabled Sentinel instances (e.g. AWS ElastiCache, Azure Cache for Redis).

Added support for: ssl, ssl_certfile, ssl_keyfile, ssl_ca_certs, ssl_cert_reqs, ssl_check_hostname — matching the core redisdb integration.

2. Username / ACL support (#2395)

Redis 6+ introduced ACLs requiring a username parameter. Without it, AUTH fails with: AUTH <password> called without any password configured for the default user.

Added sentinel_username instance config option, passed through to StrictRedis(username=...).

3. Socket timeout

Added socket_timeout config option (default: 5s) to prevent the check from hanging indefinitely on unreachable sentinels.

Note: an earlier revision of this PR also claimed a Redis 7+ SENTINEL REPLICAS fallback (#1586). That code path was dead (see above) and the underlying premise didn't hold up either — SENTINEL SLAVES is still a live alias for SENTINEL REPLICAS in current Redis, not a removed subcommand — so it's been dropped rather than reimplemented.

Files changed

File Change
redis_sentinel.py SSL, username, timeout params in _load_config()
conf.yaml.example Documented all new config options
test_redis_sentinel.py Tests for SSL, username, timeout config
pyproject.toml Bumped redis dep to ==7.3.0 (pinned, matches redisdb); bumped datadog-checks-base floor to >=37.10.0
__about__.py / CHANGELOG.md Bumped to 1.2.0

Backward compatibility

  • All new options are optional with sensible defaults
  • Without ssl: true, behavior is identical to before

Test plan

  • ddev test --lint redis_sentinel passes (ruff format + check)
  • ddev test redis_sentinel -- -m unit passes (4/4 unit tests)
  • CI: lint, unit/integration tests, and minimum-base-package job all green
  • Integration tests (ddev test redis_sentinel) — pass locally except for an arm64/amd64 emulation issue with the erichsu/redis-sentinel test image unrelated to this change; CI runs on amd64 and is green

🤖 Generated with Claude Code

Recreates #2939 (closed for inactivity) with
CI failures fixed: ruff/black formatting via `ddev test --fmt`, and the
`redis` dependency pinned exactly (redis==7.3.0) and synced with
`ddev dep freeze`, since `ddev validate dep` rejects unstable `>=` pins.

Fixes #2938, #2395, #1586

Co-Authored-By: Arnold Mendez <amendez-primer@users.noreply.github.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@datadog-datadog-prod-us1

datadog-datadog-prod-us1 Bot commented Jul 23, 2026

Copy link
Copy Markdown

Code Coverage

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 87.56% (+2.48%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: f8c911b | Docs | Datadog PR Page | Give us feedback!

ian28223 and others added 2 commits July 24, 2026 10:31
datadog-checks-base==4.2.0 (the previous floor) imports `from
six.moves.urllib.parse import urlparse` in utils/common.py, but `six`
is only declared under its `deps` extra, not as an unconditional
dependency. The "minimum base package" CI job installs the bare
package, so importing datadog_checks.base fails with
`ModuleNotFoundError: No module named 'six.moves'`.

This reproduces identically on current master, unrelated to the SSL/
ACL changes in this PR. six.moves usage was removed upstream starting
in datadog-checks-base 37.2ish (confirmed 37.1.0 still imports six,
37.3.0 no longer does), so bumping the floor to 37.3.0 fixes it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
37.3.0's [deps] extra pins ddtrace==2.10.6, which has no prebuilt
wheel for cp313. The minimum-base-package CI job's build environment
lacks pkg_resources for legacy setuptools-based source builds, so
installing datadog-checks-base[deps]==37.3.0 fails trying to compile
ddtrace from source:

  ModuleNotFoundError: No module named 'pkg_resources'

datadog-checks-base 37.10.0 bumps its ddtrace pin to 2.21.4, which
ships a cp313 wheel (confirmed via `pip download --only-binary`),
avoiding the source build entirely.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ian28223
ian28223 marked this pull request as ready for review July 24, 2026 01:19
@ian28223
ian28223 requested a review from a team as a code owner July 24, 2026 01:19
Comment thread redis_sentinel/datadog_checks/redis_sentinel/redis_sentinel.py
Comment thread redis_sentinel/pyproject.toml
Comment thread redis_sentinel/datadog_checks/redis_sentinel/redis_sentinel.py Outdated
Comment thread redis_sentinel/tests/test_redis_sentinel.py Outdated
- Add ssl_check_hostname passthrough. redis-py defaults this to True
  and only auto-disables it when ssl_cert_reqs == CERT_NONE, so a
  sentinel reached by IP with a hostname-mismatched cert previously
  had no escape short of disabling certificate verification entirely
  via ssl_cert_reqs: 0. Matches redisdb's existing ssl_check_hostname
  option.
- Drop the _get_sentinel_replicas() helper and its "Redis 7+
  REPLICAS" claim. redis-py's StrictRedis has no sentinel_replicas
  method in any released version (confirmed against 7.3.0's
  commands/sentinel.py), so hasattr() was always False and the
  fallback to sentinel_slaves() ran unconditionally - identical
  behavior to master, just with dead code and an inaccurate PR
  description. Restored the direct sentinel_slaves() call and the
  corresponding mock.patch('redis.StrictRedis.sentinel_slaves', ...)
  in test_down_slaves so the real code path is exercised again.
- Bump to 1.2.0 and add a CHANGELOG entry, since extras uses a
  hand-written CHANGELOG.md with no towncrier fragments and this
  otherwise can't ship.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ian28223

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review — I verified all four points against redis-py 7.3.0 source directly and they all held up:

  1. ssl_check_hostname — confirmed SSLConnection.__init__ sets self.check_hostname = ssl_check_hostname if self.cert_reqs != ssl.CERT_NONE else False, so it stays True by default at CERT_REQUIRED/CERT_OPTIONAL. Added the passthrough + conf.yaml.example entry, matching redisdb.
  2. Version bump / CHANGELOG — bumped to 1.2.0 and added the changelog entry.
  3. _get_sentinel_replicas() — confirmed hasattr(redis.StrictRedis, 'sentinel_replicas') is False on 7.3.0 (no such method anywhere in redis.commands.sentinel.SentinelCommands), so it was dead code. Removed the helper, restored the direct sentinel_slaves() call, and dropped the inaccurate Redis 7+/Redis sentinel integration needs an update because of removed SLAVES command #1586 claim from the description.
  4. Test mock — reverted to mock.patch('redis.StrictRedis.sentinel_slaves', ...) now that the helper's gone.

Pushed in f8c911b. Lint and unit tests pass locally.

@ian28223
ian28223 requested a review from HadhemiDD August 4, 2026 03:05
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.

redis_sentinel: No SSL/TLS support for Sentinel connections

2 participants