fix: TLS cert hot-reload + cluster-announce-ip (prevent CA-rotation outage)#298
Merged
Conversation
Two production-incident fixes for the Valkey/ValkeyCluster controllers. Defect ① — TLS certificate hot-reload (rolling restart on cert rotation): valkey-server caches its TLS context at startup and never re-reads the mounted cert from disk. After cert-manager rotates the leaf/CA, running pods keep serving the OLD cert and the operator's mTLS dials fail (x509 verification failure). We now compute a sha256 hash of the TLS Secret data (tls.crt + tls.key + ca.crt) and stamp it as the pod-template annotation cache.keiailab.io/tls-cert-hash on the managed StatefulSet (mirrors the existing auth-secret-hash pattern). When the Secret content changes the hash changes -> pod template changes -> StatefulSet rolling update -> pods reload the new cert. Only set when TLS is enabled; Secret absence is fail-soft (next reconcile fills it once cert-manager issues). Applies to both Valkey and ValkeyCluster controllers. Defect ② — cluster-announce-ip not set: On pod restart the pod gets a new IP but Valkey keeps announcing the OLD IP from persisted nodes.conf, breaking cluster gossip. We inject POD_IP via the downward API (status.podIP) into the valkey container env, and in cluster mode wrap the container command in `sh -c 'exec valkey-server ... --cluster-announce-ip "$POD_IP" --cluster-announce-port 6379 --cluster-announce-bus-port 16379'`. cluster-announce-ip must be a literal at startup so it cannot live in the rendered ConfigMap; CLI flags override valkey.conf so the rest of the config stays effective. exec preserves PID 1 signal/graceful-shutdown semantics. Standalone command is unchanged. Tests: unit tests for the TLS hash annotation (present, changes with secret content, both hashes independent), hashTLSSecret (empty/not-found fail-soft, deterministic, changes on cert/key/CA rotation), POD_IP downward-API env, and the cluster-announce command shape. Resources + controller (envtest) suites green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jongko54
added a commit
that referenced
this pull request
Jun 18, 2026
jongko54
added a commit
that referenced
this pull request
Jun 18, 2026
) * Fix replicasPerShard=0, cluster modules, and replica re-integration Defect ④ (replicasPerShard=0 / masters-only topology): The ValkeyCluster mutating webhook clobbered an explicit replicasPerShard=0 to 1, and the validator rejected autoFailover=true + rps=0, making a masters-only topology impossible. The field is json:"replicasPerShard" (no omitempty) with CRD +kubebuilder:default=1, so the apiserver already honors an explicit 0 (defaults apply only to absent fields). Removed the webhook 0→1 coercion and the autoFailover+rps=0 rejection — an unset field still defaults to 1, an explicit 0 is preserved (shards masters, 0 replicas). Defect ⑥ (cluster modules): Added Modules []ModuleSpec to ValkeyClusterSpec (v1alpha1 + v1alpha2), mirroring ValkeySpec exactly. Wired vc.Spec.Modules into the cluster STSParams so module init-containers + --loadmodule apply to cluster pods, and added the same official-preset / external-Redis-Stack validation (validateModules) to the ValkeyCluster validating webhook. Regenerated CRDs and DeepCopy. Conversion is JSON byte-copy so modules round-trip automatically (test added). Defect ③ (replica re-integration): The health gate passed at the slot level (cluster_state:ok) and never checked membership, so a node that fell out of CLUSTER NODES after a restart (new node id / lost nodes.conf) was never re-added. Added ensureClusterMembership: compares CLUSTER NODES against the desired topology, then CLUSTER MEET (current pod IP) + CLUSTER REPLICATE the correct shard primary for any drifted/missing member. Idempotent — healthy members are skipped, primaries are met before their replicas. Reuses the existing valkey client helpers (new exported MeetNode / ReplicateTo wrap resolveAddrIP + replicateWithRetry). The pure decision logic (detectReintegration / buildObservedMembers) is unit-tested; the live MEET/REPLICATE path needs real-cluster e2e validation (no gossip in envtest). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(cluster): forget stale fail,noaddr ghost nodes on re-integration When a node re-joins with a new node id (e.g. after CLUSTER RESET HARD), its old/dead id lingers in the gossip table as a fail,noaddr (or slave,fail,noaddr) ghost, inflating cluster_known_nodes until Valkey's slow auto-eviction. The re-integration reconcile path now also detects such ghosts and CLUSTER FORGETs them on every healthy member. detectStaleNodes (pure) selects only clearly-dead ghosts: nodes flagged fail AND (noaddr OR an addr not backed by any current expected pod). It never selects myself, a current-pod-backed member (even if transiently failing), or handshake nodes (still converging — left to Valkey's own timeout). forgetStaleNodes mirrors the scale-in/teardown forget pattern, issuing CLUSTER FORGET from all current members for gossip propagation, with a live-id guard so a real member can never be forgotten. Gated inside ensureClusterMembership (allReady && cluster_state=ok && replicasPerShard>0) so it cannot race bootstrap. Best-effort: forget failures don't block re-integration and retry next reconcile. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(cluster): make replicasPerShard a pointer so explicit 0 survives (defect ④) A ValkeyCluster with spec.replicasPerShard: 0 (masters-only topology) was silently coerced to 1. The field was a non-pointer int32 with CRD marker +kubebuilder:default=1; with a non-pointer int the apiserver cannot tell "omitted" from "explicit 0", so the default defeated an explicit 0. Fix: make ReplicasPerShard *int32 (v1alpha1 + v1alpha2), drop the CRD default=1 marker, and move nil→1 defaulting into code: - GetReplicasPerShard() accessor: nil → 1, explicit value (incl. 0) → as-is. - TotalNodes() and all controller reads route through the accessor. - Mutating webhook defaults nil→1 so stored objects carry an explicit value, while never touching an explicit 0 (masters-only preserved). JSON byte-copy conversion stays correct: pointer + omitempty makes nil↔nil and explicit-0↔explicit-0 round-trip exactly. Regenerated CRDs (no more default:1, replicasPerShard no longer required) + DeepCopy (controller-gen). Chart CRD copy re-synced (TestCRDBaseChartSync green). k8s.io/utils promoted to a direct dependency. Tests prove: nil → GetReplicasPerShard()==1 and TotalNodes uses 1; explicit 0 → ==0 and TotalNodes==shards (masters-only); explicit 2 → 2; webhook defaults nil→1 and preserves explicit 0; conversion round-trips both nil and explicit 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test: scan Command+Args for --loadmodule (cluster-mode #298+#299 interaction) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jongko54
added a commit
that referenced
this pull request
Jun 18, 2026
# Conflicts: # internal/controller/valkeycluster_controller.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Found during a live production incident: after an internal CA rotation (cert-manager regenerated
keiailab-internal-ca), the running Valkey pods kept serving certs signed by the old CA because Valkey caches its TLS context at process start and never reloads from disk. The operator's mTLS dials then failed withx509: ECDSA verification failure, clustermeetfailed, and a 3-shard ValkeyCluster went toslotsOK=0(effectively down). Recovery required manualkubectl delete pod,CLUSTER FAILOVER TAKEOVER,CLUSTER FORGET, and re-MEET/REPLICATE. Two root causes are addressed here.① TLS certificate hot-reload (rolling restart on cert change)
Valkey can't hot-reload its TLS context, so the operator now triggers a rolling restart when the TLS Secret changes:
internal/controller/tls_cert_hash.go:hashTLSSecret()computes a sha256 over the Secret'stls.crt+tls.key+ca.crt(each key name mixed in, so a CA-only rotation still changes the hash). Fail-soft on NotFound.internal/resources/statefulset.go: stamps the hash as pod-template annotationcache.keiailab.io/tls-cert-hash(TLS-enabled only) → Secret change ⇒ pod template change ⇒ StatefulSet rolling update ⇒ pods remount the new cert.ValkeyandValkeyClustercontrollers. Mirrors the existingauth-secret-hashpattern.② cluster-announce-ip set to the real pod IP
On restart a pod gets a new IP but keeps announcing the old IP from its persisted
nodes.conf, breaking gossip (nodes markedfail, manual MEET handshakes fail).internal/resources/statefulset.go: injectsPOD_IPvia downward API (status.podIP); in cluster mode wraps the command assh -c 'exec valkey-server <conf> ... --cluster-announce-ip "$POD_IP" --cluster-announce-port 6379 --cluster-announce-bus-port 16379'.execkeeps valkey-server as PID 1. No image rebuild required.Tests
go build ./...OK;go test ./internal/resources/...and./internal/controller/...(envtest) green, with new unit tests for the cert-hash annotation and announce-ip rendering.TestChartIconURLUsesCurrentValkeyAssetininternal/observability) also fails onmain; untouched here.🤖 Generated with Claude Code