feat: announce cluster nodes by hostname for TLS-friendly discovery - #296
feat: announce cluster nodes by hostname for TLS-friendly discovery#296daanvinken wants to merge 5 commits into
Conversation
|
this is squarely the TLS-discovery gap, nice to see it addressed. the shape looks good: defaulting to IP so existing clusters don't roll, wiring two things from having run hostname-announce with TLS on a parallel operator: the FQDN hardcodes the announced value is a per-pod FQDN, so the server cert has to carry each pod's name as a SAN. a single wildcard minor: does the per-pod FQDN resolve under |
|
@melancholictheory incredibly useful comment, thanks. 1. Hardcoded 2. Wildcard SAN guidance. 3. Deployment workloadType. |
|
all three sound right. keeping clusterDomain defaulted to |
Standard TLS clients verify SNI against the connection host, so once a
cluster-mode client seeds against the headless Service and receives
CLUSTER SLOTS, it re-dials each node by the announced value. The
operator has always used --cluster-announce-ip $(POD_IP), which forces
those follow-up dials to a pod IP. Server certificates typically carry
only DNS SANs (pod IPs are ephemeral and not practical to list), so
every re-dial fails SNI verification unless the client sets
InsecureSkipVerify.
Add spec.networking.preferredEndpointType (IP | Hostname, default IP)
mirroring valkey's cluster-preferred-endpoint-type directive. When
Hostname:
* pod.spec.subdomain is set to the cluster's headless Service name so
kubelet publishes <pod>.<svc>.<ns>.svc.cluster.local DNS records;
* the server container gets --cluster-announce-hostname pointing at
that FQDN (POD_NAME resolved from the downward API);
* cluster-preferred-endpoint-type: hostname is added to valkey.conf.
Defaults preserve current behaviour: existing clusters without a
Networking block keep announcing IPs and do not roll on operator
upgrade (subdomain is only populated in Hostname mode).
Signed-off-by: Daan Vinken <daanvinken@tythus.com>
Kubelet's --cluster-domain is user-configurable (defaults to cluster.local, but many operators run mycompany.internal or similar). Hardcoding the suffix in the announced FQDN produces a name that doesn't resolve on those clusters, so Hostname mode fails at DNS lookup before TLS ever comes into play. Add spec.networking.clusterDomain (default cluster.local) and use it when constructing the --cluster-announce-hostname value. Signed-off-by: Daan Vinken <daanvinken@tythus.com>
Deployment pods don't have stable names across restarts, so the --cluster-announce-hostname value would change on every roll and cluster gossip would treat the recreated node as a brand-new member. Reject the combination at admission time with a CEL XValidation on ValkeyClusterSpec. Signed-off-by: Daan Vinken <daanvinken@tythus.com>
… mode Reviewers pointed out the sample and docs didn't say what the server cert actually has to cover. Add explicit wildcard-SAN guidance (*.<headless-svc>.<namespace>.svc.<clusterDomain>) to both the sample YAML and the TLS section of the docs. Also document the clusterDomain knob and the Hostname-requires-StatefulSet constraint that the CRD now enforces via CEL. Signed-off-by: Daan Vinken <daanvinken@tythus.com>
Cover the path that breaks with announce-by-IP and DNS-only certs: 3 shards, preferredEndpointType=Hostname, wildcard SAN, assert CLUSTER SLOTS advertises pod FQDNs and multi-key cluster SETs work over TLS after topology refresh. Testing - make test - go test -c -tags e2e ./test/e2e/ Signed-off-by: daanvinken <daanvinken@tythus.com>
3ae4817 to
80758e1
Compare
|
Rebased onto current
Still draft until e2e is green in CI if we want a final pass there. |
## Summary Phase 1a of #318: introduce `spec.networking` and move TLS to `spec.networking.tls`. **Hard break (no dual-read):** top-level `spec.tls` is removed from the API. `GetTLS()` only reads `spec.networking.tls`. There is no fallback and no deprecation warning path. When TLS is set, `certificate.secretName` is required. ```yaml spec: networking: tls: certificate: secretName: valkey-tls ``` ## Breaking upgrade / release note Same approach as the scheduling move (v0.4.0): document in **release notes** + docs. For TLS the failure mode is worse than scheduling (security downgrade), so the wording must state the consequence, not only that the field moved. **Migrate every ValkeyCluster to `spec.networking.tls` before rolling the new CRD/operator.** After the CRD drops top-level `tls`, the API server prunes that field from objects already in etcd. The operator then sees no TLS config and reconciles the cluster **without TLS (plaintext)**. Clients that expected TLS fail. That is intentional 0.x behaviour; the release note is the protection, not a dual-read. ### Draft for the next operator GitHub release Copy into the release body (do not use a soft “fields moved” line alone): ```markdown ### Breaking: TLS under `spec.networking` - Top-level `spec.tls` is **removed**. Use `spec.networking.tls`. - **Upgrade order:** migrate every ValkeyCluster to `spec.networking.tls` **before** applying the new CRD / operator image. - **If you upgrade with only top-level `spec.tls` still set, TLS is turned off.** The unknown field is dropped from the object; the cluster comes back up serving **plaintext**. This is not a silent rename of a working field—migrate first, then roll CRDs. ```yaml # before spec: tls: certificate: secretName: valkey-tls # after spec: networking: tls: certificate: secretName: valkey-tls ``` ``` Also in-repo: `docs/valkeycluster.md` (TLS section) has the same upgrade-order warning. ## Behaviour | Spec after this change | Result | |---|---| | `networking.tls` set | TLS used | | only old top-level `tls` (pre-migration) | pruned after CRD roll → **no TLS** | | neither | no TLS | `ValkeyNode` still uses top-level `node.Spec.TLS` as an **internal** primitive filled from the cluster (not a mirrored `NetworkingSpec`). ## Out of scope - Hostname discovery (`networking.discovery`, `clusterDomain`) — #296 / follow-up under #318 - External access (`networking.external`) — #276 - Split-TLS / mTLS under `networking.tls` — design follow-up - Dual-read of top-level `spec.tls` (explicitly not done; see review thread) ## Testing - `go test ./api/v1alpha1/ ./internal/controller/` - `make generate manifests` - e2e TLS manifest uses `networking.tls` Umbrella #318 stays open for discovery/external (sub-issues next). Signed-off-by: daanvinken <daanvinken@tythus.com>
2 participants
Summary
fixes #297
Add
spec.networking.preferredEndpointType(IP|Hostname, defaultIP) toValkeyCluster, mirroring valkey's owncluster-preferred-endpoint-typedirective, so operators of TLS clusters can have nodes advertise DNS-resolvable FQDNs instead of pod IPs.Motivation
Standard TLS clients (go-redis, Lettuce, redis-py, …) verify SNI against the connection host. A cluster-mode client seeds against the headless Service, receives
CLUSTER SLOTS, and re-dials each node by the announced value. The operator has always used--cluster-announce-ip $(POD_IP), which forces those re-dials to pod IPs. Server certificates usually only carry DNS SANs — pod IPs are ephemeral and not practical to list — so every re-dial fails SNI verification unless the client setsInsecureSkipVerify. The workaround defeats most of the point of TLS.Implementation
New
NetworkingSpeconValkeyClusterSpec, mirrored onValkeyNodeSpecand populated by the cluster controller. WhenPreferredEndpointType == Hostname:pod.spec.subdomainis set to the cluster's headless Service name so kubelet publishes<pod>.<svc>.<ns>.svc.cluster.localDNS records.--cluster-announce-hostname <FQDN>using$(POD_NAME)from the downward API.cluster-preferred-endpoint-type: hostnameis added tovalkey.confviabuildManagedConfig.A cert-manager Certificate that covers
*.<headless-svc>.<ns>.svc.cluster.localthen satisfies SNI on every dial.Limitations
IPtoHostnamerolls all pods (subdomain is part of the pod template hash). Called out in the docs.cluster.local. Non-default cluster domains would need this constant lifted out.ValkeyClusterconsumers on theIPpath; the field is optional and defaults toIP.Testing
internal/controller/valkeynode_resources_test.go: table-driven coverage ofvalkeyAnnounceArgsAndEnvfor nil / IP / Hostname, plusTestBuildValkeyNodePodTemplateSpec_HostnameSetsSubdomainand a matching negative case asserting IP-mode leavesSubdomainempty (so operator upgrades don't roll existing clusters).internal/controller/config_test.go:buildManagedConfigemitscluster-preferred-endpoint-typeonly onHostname.internal/controller/valkeycluster_controller_test.go:buildClusterValkeyNodepropagatesNetworkingfrom cluster to ValkeyNode.make testpasses locally;pre-commit run --all-filespasses.Checklist
spec.networking.preferredEndpointTypewith kubebuilder defaultmake generate manifestsregeneratedzz_generated.deepcopy.goand CRDsspec.networkingunchangedconfig/samples/docs/valkeycluster.mdGenerated with Claude Code Opus 4.8.