feat(redis): add opt-in TLS for Redis and Sentinel - #2729

Actionable comments posted: 2

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/apps/redis/values.yaml`:
- Around line 80-83: The values schema for TLS is missing an enum constraint for
tls.authClients, so invalid strings pass chart-value validation; update the
values.yaml typedef for TLS (the TLS struct / tls.authClients entry) to add an
enum with allowed values ["no","optional","yes"] (and a default if needed) so
validation fails early; ensure the field name tls.authClients and the TLS
typedef are the ones updated so downstream RedisFailover.spec.tls.authClients
receives only valid values.

In `@packages/system/redis-operator/images/redis-operator/Dockerfile`:
- Line 3: The Dockerfile sets ARG VERSION=v1.4.0 and later downloads
https://github.com/cozystack/redis-operator/archive/refs/tags/${VERSION}.tar.gz
which 404s because that tag isn't published; either publish the v1.4.0 tag in
the cozystack/redis-operator repo or update the Dockerfile's ARG VERSION to a
valid existing tag (or a branch like main) so the tarball URL resolves; change
the value of VERSION in the Dockerfile (or adjust the download URL) and verify
the tarball URL resolves before merging.

---

Nitpick comments:
In `@packages/apps/redis/tests/tls_test.yaml`:
- Around line 279-305: Add a test case for templates/dashboard-resourcemap.yaml
that mirrors the existing cases for certmanager.yaml and redisfailover.yaml when
external: true and tls.enabled: false; specifically, under the same "Case 3"
inputs (external: true, tls.enabled: false) add a test entry for
templates/dashboard-resourcemap.yaml and assert with notContains (or notExists)
that any TLS-related keys/entries are absent (e.g., no tls, no tls.enabled, no
tls.* fields) so the tri-state override is validated end-to-end for
dashboard-resourcemap.yaml.

Code Review

This pull request introduces TLS support for the Redis application, incorporating API updates, Cert-manager resource templates, and a helper for tri-state TLS logic. It also updates the redis-operator to version 1.4.0 and switches to a internal fork. Review feedback identifies critical issues in the Dockerfile where non-existent Go and Alpine versions are specified, which will cause build failures. Additionally, it is recommended to enhance the values.schema.json by adding an enumeration and default value for the authClients field to improve validation and the dashboard user experience.

NOT LGTM — same B2/B3-class issues as mariadb #2680: the chart-managed cert-manager chain exposes both the CA and leaf private keys to tenant use access via dashboard RBAC, and the ECDSA leaf certificate is missing the digital signature usage so strict TLS clients can reject the handshake. Plus a build blocker: the Dockerfile downloads a fork tag that doesn't exist yet.

Business context: this PR shifts Redis TLS to a forked redis-operator (cozystack/redis-operator Phase 2+3 of #2662), with the chart rendering a cert-manager-issued chain and handing the leaf Secret to the operator via spec.tls.certificateSecret.secretName. The fork side is being developed in cozystack/redis-operator#1. The findings below are independent of the fork — they apply to the chart-side TLS chain as rendered.

Blockers

B1: dashboard-resources Role grants tenant use read on both <release>-tls and <release>-ca-tls — leaf AND CA private keys leaked

File: packages/apps/redis/templates/dashboard-resourcemap.yaml:24-25
Issue: when $tlsEnabled is true, the Role grants get/list/watch on <release>-tls and <release>-ca-tls. cert-manager writes both Secrets as kubernetes.io/tls with tls.crt + tls.key. <release>-tls carries the leaf server private key. <release>-ca-tls carries the CA private key.
Impact: at use access level any tenant principal in the namespace can extract:

• the Redis server's leaf private key → impersonate the Redis server, terminate TLS as rfrm-<release>, MITM other clients, decrypt past sessions for non-PFS ciphers;
• the CA's private key → mint arbitrary certificates trusted by anyone using this CA bundle (any sentinel / replication peer / external client) → full forge-and-MITM across the cluster's Redis trust domain.

Same class as the #2680 mariadb blocker. The two-Secret exposure is strictly worse than the single-CA case.
Fix: same as mariadb's: stop exposing <release>-tls and <release>-ca-tls via the chart's dashboard-resources Role. If the dashboard genuinely needs to surface connection metadata, project a ca.crt-only Secret (e.g. via cert-manager's secretTemplate with the private-key field stripped, or a tiny init job) and expose THAT. Tenants needing the leaf or CA for break-glass debugging can still use kubectl out-of-band; routine clients should resolve trust via the projected CA-only Secret.

B2: leaf ECDSA certificate is missing the digital signature usage

File: packages/apps/redis/templates/certmanager.yaml:65-68
Issue: leaf cert usages: [server auth, client auth, key encipherment]. The cert uses ECDSA (privateKey.algorithm: ECDSA, size: 256). TLS_ECDHE_ECDSA — the only meaningful TLS handshake for an ECDSA cert — requires the cert's digital signature (alias signing in cert-manager — see pkg/api/util/usages.go) bit so the server can sign the key-exchange parameters.
Impact: strict TLS clients (Go's crypto/tls, OpenSSL with purpose check) reject the cert when it lacks digitalSignature for ECDHE_ECDSA. The handshake fails with x509: certificate specifies an incompatible key usage or equivalent. Symptom: redis-cli with --tls may work depending on its OpenSSL version, but stricter clients (Go, Java with strict purpose checking, mTLS-aware tooling) fail.

Same finding as the #2680 mariadb leaf cert.
Fix: add - digital signature (or - signing) to the leaf cert's usages block. The CA cert at line 35-37 has the correct signing + cert sign already; the leaf one is the one to fix.

B3: Dockerfile downloads cozystack/redis-operator/v1.4.0 which currently returns 404 — image is unbuildable

File: packages/system/redis-operator/images/redis-operator/Dockerfile:12
Issue: RUN curl -sSL https://github.com/cozystack/redis-operator/archive/refs/tags/v1.4.0.tar.gz | tar -xzvf- --strip=1 — the tag doesn't exist. Verified: HTTP/2 404 against the tarball URL; gh api repos/cozystack/redis-operator/tags returns an empty list; the fork PR cozystack/redis-operator#1 (feat/tls-cert-manager) is still open. Out of scope per the PR body, but the chart side currently references a non-existent artifact, so anyone running make image will fail before compilation starts.
Impact: image build fails everywhere — CI for any cozystack PR that touches redis (including this one's Resolve assets / Build pipelines), local dev, fresh installs. The values.yaml:image.tag = v1.4.0 reference will also fail digest resolution when fetched.
Fix: gate this PR on the fork tag being cut (i.e. merge cozystack/redis-operator#1, tag v1.4.0, publish the image to ghcr) BEFORE merging here. Alternatively, pin to a commit SHA on the fork's feat/tls-cert-manager branch as a placeholder while the bump sequence is resolved — but the current v1.4.0 reference is dead.

Non-blocking follow-ups

1. Same schema array-type issue as #2686/#2692: "type":["boolean","null"] for tls.enabled (and tls.authClients if it inherits the pattern) in values.schema.json / cozyrds openAPISchema fails to unmarshal into apiextv1.JSONSchemaProps (single-string Type field). cozystack-api silently disables specSchema-based defaulting for this app; Helm-side defaults still apply, so user-visible breakage is minimal. Worth a separate fix across the whole TLS series — nullable: true (apiextv1 idiom) or drop type for tri-state properties.

2. templates/certmanager.yaml:24 and 47: CA cert secretName and CA Issuer's ca.secretName both reference <release>-ca-tls. Once B1 is resolved, the same change (CA private key not exposed) needs to be reflected in cozyrds secrets.include if it lists -ca-tls. Worth double-checking when fixing B1.

3. The PR body explicitly says "digest will be repinned by make image once CI publishes the release artifact". Make sure that step lands in this PR (or a follow-up) BEFORE merging — otherwise the chart references an image that doesn't exist.

4. README workflow currently directs users to the dashboard / <release>-tls Secret for client connection. After the B1 fix lands, README needs to redirect to whatever CA-only Secret replaces it.

NOT LGTM — the chart-side TLS is solid in shape, but it depends on three operator-side artifacts that do not exist yet: an unbuildable image tag, an unmerged CA-only-secret feature, and a missing loopback-IP SAN set. Each is independently merge-blocking.

Business context: adds operator-managed Redis TLS by switching to the cozystack/redis-operator fork and rendering a cert-manager CA+leaf chain handed to the operator, matching the DBaaS-wide TLS convention.

Blockers

B1: the redis-operator image references a tag that does not exist — unbuildable
File: packages/system/redis-operator/images/redis-operator/Dockerfile:3,12, packages/system/redis-operator/values.yaml:4
Issue: the Dockerfile downloads the v1.4.0 source tarball and values.yaml pins tag: v1.4.0, but no such tag exists in the fork.
Evidence: cozystack/redis-operator has no tags, no releases; the v1.4.0 tarball URL returns HTTP 404; the ghcr package was never published. The root Makefile now wires make -C packages/system/redis-operator image into build, so the whole platform build fails on this 404 before compilation.
Impact: make image / make build fails in CI, local dev, and fresh installs; values.yaml tag also fails digest resolution.
Fix: cut and push a release tag on the fork, publish the image to ghcr, then repin values.yaml to a digest — within this PR.

B2: the chart depends on an operator field (spec.tls.caCertSecretName) absent from the operator's released/master code
File: packages/apps/redis/templates/redisfailover.yaml:34, packages/apps/redis/templates/dashboard-resourcemap.yaml:29
Issue: the RBAC private-key leak fix switches the dashboard Role to grant only <release>-ca-cert (a CA-only secret) and sets spec.tls.caCertSecretName: <release>-ca-cert so the operator publishes it. But caCertSecretName exists only in the fork's open, unmerged PR (feat/tls-ca-cert-secret), not in master.
Evidence: caCertSecretName has zero occurrences in the fork's master; it appears only on the open feature branch. The CRD vendored in this PR already carries the field — meaning it was vendored from the open branch, not master. An operator built from any released or master code will never create <release>-ca-cert.
Impact: with the real operator, <release>-ca-cert is never published — the dashboard Role grants on a non-existent secret and the operator silently ignores the unknown CRD field. The headline security fix only works against an operator that has not been merged or released; the vendored CRD diverges from the operator binary it ships with.
Fix: merge the fork's CA-secret PR, fold it into the tagged release this PR pins, and vendor the CRD from that same release point so chart + CRD + binary are consistent.

B3: the chart-issued leaf cert is missing loopback IP SANs (127.0.0.1 / ::1)
File: packages/apps/redis/templates/certmanager.yaml:70-83
Issue: when the chart supplies its own cert via spec.tls.certificateSecret, the operator stops generating the SAN set. The chart leaf lists localhost as a DNS SAN only and sets no ipAddresses, but the operator's own components dial over loopback IP with full CA verification.
Evidence: the operator's generator adds ipAddresses: ["127.0.0.1", "::1"] precisely because redis_exporter is given REDIS_ADDR=rediss://127.0.0.1:26379 with REDIS_EXPORTER_TLS_CA_CERT_FILE (verification active, not skip-verify) and the sentinel monitor targets 127.0.0.1. A localhost DNS SAN does not match an IP literal in Go/openssl cert verification.
Impact: with chart-managed TLS on, the redis_exporter metrics scrape over rediss://127.0.0.1:26379 fails cert verification and the exporter/monitoring path breaks. Probes using -h localhost match the DNS SAN, so the failure is partial and easy to miss in a smoke test.
Fix: add an ipAddresses: block with 127.0.0.1 and ::1 to the leaf Certificate, mirroring the operator's own SAN generation.

Non-blocking follow-ups

1. On the authClients=yes path the chart grants tenants get on <release>-tls (the leaf, which carries the private key) so they can reuse it as a client identity (packages/apps/redis/templates/dashboard-resourcemap.yaml:34). The comment acknowledges a dedicated per-tenant client cert would be the stricter long-term design — worth a follow-up issue.
2. README tls.authClients default renders {} instead of no (packages/apps/redis/README.md:36) — a cozyvalues-gen artifact; cosmetic.

LGTM — re-review on head 3e24931b. All three blockers from the prior review are resolved.

Business context: adds operator-managed Redis TLS by switching to the cozystack/redis-operator fork and rendering a cert-manager CA+leaf chain handed to the operator, matching the DBaaS-wide TLS convention.

Blockers — resolved

1. Unbuildable image tag — RESOLVED. The fork now has a v1.4.0 tag and release; the source tarball the Dockerfile downloads returns 200, so make image / make build no longer fails. values.yaml and the Dockerfile ARG VERSION both pin v1.4.0 consistently.
2. spec.tls.caCertSecretName absent from the operator — RESOLVED. The fork's CA-only-secret PR is merged and folded into the v1.4.0 tag (the tag's tree carries CACertSecretName in the API types). An operator built from v1.4.0 will publish <release>-ca-cert, so the dashboard RBAC grant now points at a secret the operator actually creates, and the vendored CRD matches the operator binary.
3. Missing loopback IP SANs — RESOLVED. The leaf certificate now sets ipAddresses: ["127.0.0.1", "::1"] (on the <release>-tls leaf secret handed to the operator, isCA: false), with a comment explaining the redis_exporter rediss://127.0.0.1 CA-verification path. DNS SANs still cover all service forms plus the external host.

Verified: helm-unittest passes (23/23, including the TLS suite); make generate shows no drift; the leaf cert usages include digital signature + server auth + client auth; key rotation is Always with renewBefore: 720h.

Non-blocking follow-ups

1. On the authClients=yes path the chart grants tenants get on <release>-tls (the leaf, which carries the private key) so they can reuse it as a client identity (packages/apps/redis/templates/dashboard-resourcemap.yaml:34). The comment acknowledges a dedicated per-tenant client cert would be the stricter long-term design — worth a follow-up issue.
2. README tls.authClients default renders {} instead of no (packages/apps/redis/README.md:36) — a cozyvalues-gen artifact; cosmetic.

Verdict

NOT LGTM

Round two, against 25a06181 (the branch was rebased since my last round, so the commits are rewritten; I compared trees, not shas). Five of my eight findings are closed and I checked each closure by reverting it and watching a suite go red rather than by reading the change. Three are still open, all of them at two rounds now. One new defect turned up, and it is the same class as the one that closed.

I was wrong about one thing last round and want to say so plainly: my suggested redis-cli -t 5 would not have bounded the stall. -t caps the TCP connect, which succeeds against a plaintext port, so the wait is the TLS handshake after it. The inner timeout 15 in the fix is the right instrument.

Closed since my last round

• The suite stall. hack/e2e-chainsaw/redis/chainsaw-test.yaml:551 now carries timeout: 2m on the op, every redis-cli runs under timeout 15 and every kubectl under timeout 30. /usr/bin/timeout is present in both engine images, which I checked by running them rather than trusting the comment that says so. CI corroborates: on the run at d5efbd228 the flip step took 15.6s (CMD RUN 09:23:32.455, SCRIPT DONE 09:23:48.058) against 2h58m56s of silence at the same step before, and all three redis cases completed in 3m50s with the suite carrying on into qdrant and postgres.
• tls.authClients no longer changes nothing. The directive is folded length-prefixed into tlsSecretContentHash, and checker.go deletes every pod whose revision hash differs, so the roller delivers it. The reverse direction is fixed at the right layer too: check.go:44 now presents the operator's client certificate whenever the Secret carries a pair, independent of the spec, so the flip back cannot lock the operator out of its own instance. Replacing the hash lines with _ = tlsAuthClients(rf) turns TestTLSSecretHashTracksAuthClients red on all three subtests; restoring the authClients == yes gate turns TestTLSConfigForPresentsClientCertificate red on three more.
• The Secret-name collision, and by a better construction than I proposed. The separator is now a dot, so the suffix set {.ca-tls, .tls, .ca-cert, .tenant-ca} is free of mutual suffixes and R1 + ".tls" == R2 + ".ca-tls" cannot hold. That does not depend on app-name validation, and it holds even for a release name containing a dot. Rendering the adversarial pair confirms it: redis-foo-ca-tls is no longer any release's Secret name. Reverting the separator in certmanager.yaml alone fails 3 of the 48 unittests.
• The tenantsecret negative assertion. The loop now requires NotFound in the message. Driven against five stubbed outcomes it separates them correctly, and reverting the guard reproduces the old fail-open exactly: forbidden, unserved and timeout all go back to exit 0.
• The authClients description. The constraint reached values.schema.json, the README table and the Go type with identical text, and the README goes further than I asked by stating the CA key is deliberately out of reach. I verified the underlying claim instead of accepting it: tenant ClusterRoles grant core.cozystack.io/tenantsecrets and nothing on cert-manager.io or raw secrets, so neither the CA Secret nor the in-namespace Issuer is reachable.

Still open from my earlier rounds

[MAJOR] hack/e2e-chainsaw/redis/chainsaw-test.yaml:86, still nothing exercises an existing cluster (round 2)

All four Chainsaw cases still create their instances from scratch under the new operator and the new CRD:

$ grep -rn '^  name: ' hack/e2e-chainsaw/{redis,valkey}/chainsaw-test.yaml
redis/chainsaw-test.yaml:5:    name: redis
redis/chainsaw-test.yaml:86:   name: redis-tls
redis/chainsaw-test.yaml:463:  name: redis-tls-flip-rejected
valkey/chainsaw-test.yaml:5:   name: valkey
$ grep -rn 'file: redis\|file: valkey' hack/e2e-chainsaw/{redis,valkey}/chainsaw-test.yaml
redis:20 redis.yaml   redis:115 redis-tls.yaml   redis:486 redis-flip.yaml
redis:522 redis-flip-tls.yaml   valkey:20 valkey.yaml

Every fixture is a fresh apps.cozystack.io/v1alpha1 object. redis-tls-flip-rejected does edit a running plaintext instance, but it created that instance itself under this branch, so the transition an existing customer actually takes is still untested.

Two things did materially de-risk it, and both arrived as hermetic pins rather than as arguments. The spec-level CEL rule was exercised against the real apiextensions validator with the shipped cozystack CRD staged into the fixture paths, and an object with no tls block stays updatable because the rule evaluates false == false on both sides. And the rebuilt binary renders a byte-identical plaintext StatefulSet, Sentinel Deployment and both ConfigMaps compared against a pristine v3.3.5 build, for Engine: Valkey as well; TestPlaintextProbesKeepUpstreamCommands guards that and is not inert, since making redisCLITLSFlags return --tls for a plaintext failover turns all four of its subtests red.

What is still unpinned is what needs an apiserver: SSA field ownership on the already-running rfr-* StatefulSet and rfs-* Deployment, and the CreateReplace of the CRD against stored objects. A Chainsaw case that pins the operator image to the pre-patch digest, creates the instance, then swaps to the rebuilt digest would close this permanently.

[MINOR] packages/system/redis-operator/values.yaml:4, the committed digest is still the pre-patch build (round 2)

The file is untouched by this PR, and the digest it pins was stamped by abb38ab72, whose tree carries only labels.diff under images/redis-operator/patches/. So the committed tree renders spec.tls against a binary built from a patch set that does not contain tls.diff.

The signal did improve, and it came from main through the rebase rather than from this branch: redis-operator is now in the root Makefile build: list and hack/build-matrix.sh emits it, so PR CI builds the package, make image stamps the fresh digest, and the per-package job uploads it for reassembly. docs/agents/image-refs.md is edited here to drop redis-operator from the live-violations list, consistent with that. What stays reachable is any path that builds the platform bundle without rebuilding this package first: make -C packages/core/platform image on its own, or the dev-loop package-deploy flow.

The blast radius grew since my last round, through a consumer this PR did not add. Valkey landed on main and creates RedisFailover on the same operator, CRD and widened ClusterRole. The pinned digest therefore also lacks valkey-cli-auth.diff, which switches valkeyEngine.CLIAuthEnvName() from VALKEYCLI_AUTH to REDISCLI_AUTH, and that value is exported by the generated shutdown.sh and ready.sh, so it drives the readiness probe. Checked against the pinned engine images rather than reasoned about:

$ docker run --rm --entrypoint sh valkey/valkey:8.1.8 -c '...'
-- REDISCLI_AUTH:   PONG
-- VALKEYCLI_AUTH:  NOAUTH Authentication required.
$ docker run --rm --entrypoint sh valkey/valkey:7.2.13 -c '...'
v7 REDISCLI_AUTH:   PONG
v7 VALKEYCLI_AUTH:  NOAUTH Authentication required.

On the committed digest with the default authEnabled: true, a Valkey readiness script gets NOAUTH and the pods never go Ready. Through that path the stale pin fails loudly instead of silently, and the Valkey Chainsaw case asserting readyReplicas: 2 would catch it, but only because CI rebuilds the image. Pinning the rebuilt digest here removes the whole question.

[MINOR] packages/system/redis-operator/patches/rbac-tls.diff:44, still no ownership check on the Secret write (round 2)

The only change since last round is a no-op skip when the stored content already matches. CreateOrUpdateSecret still issues a full Update on whatever Secret occupies the name, and there is no ownerReference, label or adoption check anywhere on the path. The validation is still only the inequality with the TLS Secret's own name, at api/redisfailover/v1/validate.go:134.

One correction to my own description: it is slightly worse than replacing Data. generateRedisCACertSecret sets OwnerReferences, and the write is a full Update, so overwriting a foreign Secret also reparents it to the RedisFailover and it gets garbage-collected when that object is deleted.

Still not reachable from the tenant surface, for the reasons I gave last round, and still the right destination is upstream. I am leaving it open rather than closing it because nothing in this tree changed about it.

New in this round

[MAJOR] hack/e2e-chainsaw/redis/chainsaw-test.yaml:578, the negative dial probes are fail-open, the same defect that just closed one file over

if k -n "$NAMESPACE" exec rfr-redis-flip-0 -c redis -- \
  timeout 15 redis-cli --tls --insecure -h "rfr-redis-flip.$NAMESPACE.svc" -p 6379 \
  --user pinger --pass pingpass --no-auth-warning ping 2>/dev/null | grep -qx PONG; then
  echo "a TLS listener appeared on an instance whose flip was rejected" >&2
  exit 1
fi

Stderr is discarded and the verdict is "did the word PONG appear". A missing pod, a 403 on exec, a wrong container name, or the outer timeout 30 firing all produce no PONG, so the if is false and the assertion reports success. It can only fail when a TLS listener genuinely answers; it cannot tell "there is no listener" from "I could not ask". set -eu does not help, because the if consumes the pipeline's status.

Driven against stubbed outcomes, four distinct failures to ask all read as the refusal being asserted:

refused    exit=0   read as: no TLS listener
notimeout  exit=0   read as: no TLS listener
nopod      exit=0   read as: no TLS listener
rbac       exit=0   read as: no TLS listener
tlsup      exit=1   a TLS listener appeared on an instance whose flip was rejected

This is the security-relevant half of the flip case: it is the assertion that a rejected flip really did leave TLS off. The notimeout row is a mode the timeout fix introduced by putting a binary in the exec path; it is not live today because both images carry it, but nopod and rbac are live now.

The shape you already use twenty lines above is the fix, and you apply it correctly to the positive dial in the same script:

pong=$(k ... timeout 15 redis-cli --tls --insecure ... ping 2>&1) \
  || pong='(could not ask: exec failed or hit the cap)'
case "$pong" in
  *PONG*) echo "a TLS listener appeared on an instance whose flip was rejected" >&2; exit 1 ;;
  *"Connection reset"*|*"connection refused"*|*"I/O error"*|*"no reply"*) : ;;
  *) echo "cannot determine whether a TLS listener is up: $pong" >&2; exit 1 ;;
esac

hack/e2e-chainsaw/redis/chainsaw-test.yaml:399 has the identical shape for the mirror assertion, that the plaintext listener does not answer once TLS is on, and needs the same treatment.

Caveats

• The E2E job on this exact head has not finished: E2E (in-tree) in run 33645509783 is queued. My timing evidence for the closed stall comes from run 33608673237 on d5efbd228, a superseded rewrite of the same logical tip whose chainsaw file I compared against this head. A green run here would be the direct proof.
• The two .diff-anchored items and the values.yaml one are not inline comments: those files are either untouched by this PR or came from main with the rebase, so GitHub has no changed line to hang them on.
• Valkey is not this PR's work and I am not asking for anything about it here beyond the digest pin. For the record and for whoever owns that package: an externally exposed Valkey is plaintext-only with no TLS option in its schema, which is an asymmetry with Redis, and there is no check-valkey-rd-secrets.bats counterpart to the redis Secret-surface guard.

Checked and correct

• The vendored chart is not drifted from its patches, which is the failure mode where make update silently reverts a hand-edit. Pulling upstream 3.3.5 fresh and applying crd-tls.diff and rbac-tls.diff reproduces the committed charts/ tree with no differences.
• The carried patch builds and passes its own suite: upstream v3.3.5 tarball with manifests charts example .github removed exactly as the Dockerfile does, git apply --check clean for all three diffs, go build ./... exit 0, go test ./... all ok.
• TestGeneratedCRDRejectsTurningTLSOnOrOff skips in the image build because the Dockerfile deletes manifests/; staging the shipped cozystack CRD into the fixture paths runs it for real, and it passes including the accepted-transition cases.

Verdict

LGTM with non-blocking notes

Round three, against 3cf9431f. The delta since my last round is one file, hack/e2e-chainsaw/redis/chainsaw-test.yaml, +54/-13, and it is the fix to both findings I raised. I checked the two rewrites by driving them rather than by reading them, and the E2E run on this head settles the one I raised two rounds ago.

Three items stay open and none of them blocks: one is a coverage gap whose replay has been run and reported, one is a packaging inconsistency whose published artifact is already correct, and one is an upstream hardening item we agree on. They are below with what would retire each.

One correction of mine to record: you were right to change the sketch I posted. pong=$(...) || pong='(could not ask)' overwrites the captured text on every non-zero exit, and redis-cli exits 1 on the genuine refusal too, so the refusal would have been read as "could not ask" and the case would have failed on the very state it asserts. Keeping the text in pong and the status in rc is correct.

Closed since my last round

Both negative dial probes. The fail-open is gone, and I checked it by extracting each block and driving it against stubbed kubectl outcomes rather than by reading the diff:

                    flip probe                 plaintext probe
PONG                fails (listener found)     fails (listener answers)
own refusal         accepted                   accepted
other's refusal     rejected                   rejected
missing pod         rejected: cannot determine rejected: cannot determine
403 on exec         rejected: cannot determine rejected: cannot determine
outer k() cap       rejected: cannot determine rejected: cannot determine
wrong container     rejected: cannot determine rejected: cannot determine
connection refused  rejected: cannot determine rejected: cannot determine

Every "I could not ask" state now fails closed, each probe accepts only the refusal its own listener produces, and each rejects the other's. That last column is what makes this a real classification rather than a broader pattern match.

The suite stall. Settled by CI on this head rather than by argument. From the E2E job:

--- PASS: chainsaw/redis-tls-flip-rejected (75.17s)
--- PASS: chainsaw/redis-tls (60.60s)
--- PASS: chainsaw/redis (56.39s)
--- PASS: chainsaw/valkey (63.89s)

Against 2h58m56s of silence at that step and fourteen suites that never started, two rounds ago. The TLS fixture this PR's description walks through has now actually executed.

Still open, none blocking

hack/e2e-chainsaw/redis/chainsaw-test.yaml:86, no suite starts from an existing cluster

All three redis cases still create their instances from scratch under the new operator and the new CRD, so the transition an existing customer takes is not encoded anywhere that runs on every push.

I am not blocking on it, and I want to be exact about why, because the evidence is not mine. In my last round I wrote that a clean replay or an in-suite image swap would move this to a non-blocking note. You ran the replay and reported it clean with specifics: a plaintext instance on the pre-patch operator, CRD replaced first, currentRevision == updateRevision unchanged, 2/2 Ready, zero restarts, same resourceVersion, managedFields showing no writes. A hermetic review cannot reach a cluster, so that is your measurement and not my verification, and I am taking it at the value I said I would. What I did verify myself, in an earlier round and again here, is the part that does not need a cluster: the plaintext render is byte-identical across the change, the patched operator emits identical plaintext StatefulSet, Sentinel Deployment and ConfigMaps against a pristine v3.3.5 build, and the transition rule evaluates false == false on a stored object with no tls block, so it cannot fire on an upgrade that changes nothing.

What retires it: the Chainsaw case you propose as a separate PR, pinning the operator image to the pre-patch digest, creating the instance, then swapping to the rebuilt digest. Until that lands, this path is covered by a measurement nobody can re-run from the repository.

packages/system/redis-operator/patches/rbac-tls.diff:44, no ownership check on the Secret write

Unchanged this round, and by design. CreateOrUpdateSecret still issues a full Update on whatever Secret occupies the name, with no ownerReference, label or adoption check on the path, and the only validation remains the inequality with the TLS Secret's own name. Agreed that the guard belongs upstream in freshworks-oss/redis-operator#85 alongside the CA-publish opt-out, and agreed on the reparenting point: the write carries OwnerReferences, so overwriting a foreign Secret adopts it and it is garbage-collected with the failover. Not reachable from the tenant surface as the platform ships. Leaving the thread open until the upstream change lands is the right shape.

packages/system/redis-operator/values.yaml:4, the committed digest is still the pre-patch build

Third round, still untouched, still v3.3.5@sha256:551f3934..., a digest stamped by a commit whose tree carries only labels.diff. The published artifacts are fine: the package is in the root build: list, make image stamps the fresh digest, and the release and main lanes run it. What stays reachable is a tree read without a build, which is make -C packages/core/platform image on its own or the dev-loop deploy: there the chart admits and renders spec.tls against a binary that ignores it and serves plaintext, with no signal anywhere. Through Valkey the same pin also predates valkey-cli-auth.diff, so a Valkey instance built from the committed tree gets NOAUTH on its readiness script. I accept that a hand-pinned digest goes stale the moment CI stamps a new one, so I am not asking for it again; recording it because it has now survived three rounds and the next reader of this file will hit the same question.

Caveats

• The E2E Tests check on this head is green but reads in-tree e2e soft: node-join deadline missed, suite failed, merge not blocked. The suite did fail. The failures are kubernetes-latest and kubernetes-previous, both on the node-join deadline, and the job log says so explicitly, so nothing here is redis-adjacent. Worth knowing that a green tick on this lane is not the same as a green suite.
• The refusal strings the two probes now key on, I/O error for a TLS listener dropping a plaintext client and kubectl's command terminated with exit code 124 trailer for a plaintext listener holding a ClientHello, are the load-bearing constants of both assertions. I confirmed the classification logic behaves correctly given those strings; whether the shipped redis-cli and kubectl emit exactly them is what the passing E2E run on this head demonstrates, not something I checked independently. If either string ever drifts, both probes fail closed and say "cannot determine", which is the right direction to fail in.
• Not re-reviewed this round: everything outside the one changed file. The carried operator patch, the chart, the CRD and the RBAC diff were reviewed in rounds one and two and are byte-identical to what I looked at then.

LGTM. A 6500-line patch against a vendored operator is the shape I open most suspiciously, so here is what changed my mind.

It is a carry, not a fork. The patch header pins the upstream commit it was cut from (freshworks-oss/redis-operator#85, 2ad2ed8d1) and states its own retirement condition — retire when a release carrying #85 lands. The chart patches re-apply at the end of update: the same way reloader, grafana-operator, ingress-nginx, seaweedfs and kamaji carry theirs, which matters because rm -rf charts would otherwise take TLS back out on the next make update and nothing would fail.

The non-TLS change is better than the header claims. The header names MakeSlaveOfWithPort as taking effect with TLS off and argues it is safe because it changes a dial rather than a pod template. Reading the hunk, it is a latent bug fix:

- Addr: net.JoinHostPort(ip, masterPort), // this is IP and Port for the RedisFailover redis
+ Addr: net.JoinHostPort(ip, localPort),

The old code dialled the replica at ip on the master's port, with a comment that read as if that were intended. It is invisible while both run the same port, which is the same-cluster failover path, and it breaks external-master mode where masterPort belongs to a Redis outside the cluster. MakeSlaveOf now passes redisPort for both and preserves the old behaviour on that path. So this is neutral for every current deployment and a fix for one that was broken.

Opt-in is typed, not just documented. Enabled *bool with no default, and the field doc says outright that TLS is not inferred from external — which is the wrong inference someone would otherwise reach for, given both concern reachability. AuthClients carries its own consequence in the doc: the directive is read once at startup, so changing it restarts Redis and Sentinel.

The RBAC delta is the part I would hold up as an example. Create, Get and Update on Secrets, which the header states are every Secret call in the patched source; no list and no watch because the Secret is addressed by name; nothing patches or deletes. Certificates are deliberately not granted, and the rationale sits at the rule site rather than only in the commit, so a future re-vendor reads the absence as a decision instead of an omission. That is narrower than the ported operator's own chart asks for.

CI is green including E2E on 3cf9431f1.

The thing to keep an eye on is the retirement condition, since it depends on an upstream PR merging and releasing — worth tracking somewhere that outlives this PR, so the carry does not quietly become permanent.