feat: announce per-shard hostnames for external clients#279
Conversation
### Summary Introduce an optional `externalAccess` block on ValkeyCluster as the foundation for exposing a cluster to clients outside Kubernetes. When the field is omitted the cluster is internal-only and renders identically to before. As the first capability under this flag, enabling external access announces a human-readable node name so cluster events reference the ValkeyNode name instead of only the opaque node ID. ### Implementation - Added `ExternalAccessSpec` (currently `enabled`) to `ValkeyClusterSpec` and mirrored the field onto `ValkeyNodeSpec`, copied verbatim in `buildClusterValkeyNode` alongside the other propagated spec fields. - When external access is enabled, `buildContainersDef` appends `--cluster-announce-human-nodename <node name>` to the server command, reusing the existing CLI-arg seam that already sets `--cluster-announce-ip`. Node-to-node traffic is unaffected. - `cluster-announce-human-nodename` is a Valkey 9.0+ directive, which matches the operator's documented baseline. ### Limitations This change only adds the API and the human-nodename announce. Per-shard Services, external hostnames, and client endpoint selection are added in follow-up changes. ### Testing - Unit tests assert the human nodename is announced when enabled and that a nil or disabled `externalAccess` leaves the rendered command unchanged. - `make test` and `make lint` pass locally. Signed-off-by: Steven Crothers <steven@scrothers.com>
### Summary When external access is enabled, create one Service per shard that exposes each node on its own port, and report the resulting external ports per shard under `status.externalEndpoints`. This is the networking layer that makes a cluster reachable from outside Kubernetes; node-to-node traffic is unaffected. ### Features / Behaviour Changes - `externalAccess` gains `serviceType` (NodePort default, or LoadBalancer), `externalTrafficPolicy`, and `serviceAnnotations`. - `status.externalEndpoints` reports each shard's external ports, indexed by node, so users can discover Kubernetes-allocated NodePorts. ### Implementation - `reconcileShardServices` upserts a Service per shard, selecting that shard's pods. Each Service has one port per node whose `targetPort` references a node-unique container port name (`vk-n<idx>`), so a port resolves to exactly one pod. The server container port is renamed accordingly when external access is enabled. - NodePort ports are allocated by Kubernetes and read back from the Service (preserved across updates); LoadBalancer ports are `6379 + nodeIndex`. - Shard Services carry the standard labels (including managed-by, so the manager cache sees them) and are owned by the cluster. Services for shards beyond the desired count, or all of them when disabled, are deleted. - `updateStatus` persists `externalEndpoints` alongside the conditions. ### Limitations External hostnames and client endpoint selection (so cross-shard MOVED redirects resolve externally) are added in follow-up changes. DNS is the user's responsibility; the operator only sets the configured annotations. ### Testing - Unit tests cover the per-node port layout, NodePort preservation, and the NodePort vs LoadBalancer endpoint reporting. - envtest covers per-shard Service creation, the managed-by label, a no-op second reconcile with stable ports, and scale-in / disable teardown. - An e2e spec exercises NodePort allocation, status reporting, and single-endpoint-per-port resolution on a kind cluster. - `make test` and `make lint` pass locally. Signed-off-by: Steven Crothers <steven@scrothers.com>
### Summary Add a per-shard client-facing hostname to external access. When a domain is configured, every node in a shard announces `<hostnamePrefix>-<shardIndex>.<domain>` so clients can be directed to a stable name. This is announced as metadata only; switching clients to it is a follow-up change. Node-to-node traffic continues to use pod IPs. ### Features / Behaviour Changes - `externalAccess` gains `hostnamePrefix` (default `shard`) and `domain`. - With `domain` set, each shard announces `<hostnamePrefix>-<shardIndex>.<domain>`. ### Implementation - `buildContainersDef` appends `--cluster-announce-hostname` with the shard hostname when external access is enabled and a domain is set, reusing the same CLI-arg path as the other announce flags. - `hostnamePrefix` is validated as a DNS label via a kubebuilder pattern. ### Limitations The hostname is metadata until clients are switched to it (a follow-up sets `cluster-preferred-endpoint-type`). DNS records that resolve the hostnames to the shard Services are the user's responsibility. When TLS is enabled, the certificate must additionally cover every shard hostname. ### Testing - Unit tests cover the hostname format and that no hostname is announced without a domain. - A regression test confirms the CLUSTER NODES address parser still extracts the node IP when an announced hostname is appended to the address field, so the reconciler's pod-IP correlation is unaffected. - The e2e spec verifies the announced hostname appears in CLUSTER NODES. - `make test` and `make lint` pass locally. Signed-off-by: Steven Crothers <steven@scrothers.com>
|
| Filename | Overview |
|---|---|
| internal/controller/valkeycluster_controller.go | Adds reconcileShardServices (create/update/delete per-shard NodePort/LoadBalancer Services) and wires it into the reconcile loop; has a churn bug when externalTrafficPolicy is unset. |
| internal/controller/valkeynode_resources.go | Appends --cluster-announce-human-nodename and --cluster-announce-hostname to the Valkey command when external access is enabled; missing guard for empty HostnamePrefix. |
| api/v1alpha1/valkeycluster_types.go | Adds ExternalAccessSpec (hostnamePrefix, domain, serviceType, externalTrafficPolicy, serviceAnnotations) and ShardEndpoint types with CRD markers; Domain field lacks format validation. |
| internal/controller/utils.go | Adds shardClientPortName (node-unique container port name, capped at 15 chars) and shardHostname helpers; both are correct but shardHostname has no guard for empty prefix. |
| internal/controller/shard_services_test.go | Integration tests verifying Service creation per shard, scale-in deletion, disable-access cleanup, and NodePort stability across reconciles; envtest-backed and covers the key lifecycle paths. |
| test/e2e/valkeycluster_external_access_test.go | E2E spec verifying NodePort Services, status reporting, endpoint slices, and announced hostname appearing in CLUSTER NODES; well-written and covers the observable user-facing contract. |
| api/v1alpha1/valkeynode_types.go | Adds ExternalAccess *ExternalAccessSpec to ValkeyNodeSpec, propagated verbatim from the owning cluster; straightforward and correct. |
| api/v1alpha1/zz_generated.deepcopy.go | Auto-generated deep copy for ExternalAccessSpec, ShardEndpoint, and their embedding structs; generated output is correct. |
| internal/controller/shard_services_unit_test.go | Unit tests for buildShardServicePorts (including NodePort preservation across updates) and shardEndpointFromService for both NodePort and LoadBalancer; correct and well-structured. |
| internal/valkey/clusterstate_test.go | Adds regression test confirming the CLUSTER NODES address parser extracts the pod IP correctly when an announced hostname suffix is appended; parser logic is verified correct. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant R as Reconciler
participant K8s as Kubernetes API
participant V as Valkey Node
R->>K8s: reconcileShardServices()
loop per shard
R->>K8s: CreateOrUpdate shard Service (ports: vk-n0, vk-n1 per node)
K8s-->>R: Service with allocated NodePorts
R->>R: shardEndpointFromService() → NodePort list
end
R->>R: "cluster.Status.ExternalEndpoints = endpoints"
R->>K8s: reconcileValkeyNode() per shard/node
R->>K8s: CreateOrUpdate ValkeyNode CR (ExternalAccess copied from cluster)
K8s-->>R: ValkeyNode
note over R,V: ValkeyNode controller
R->>V: Pod: valkey-server --cluster-announce-ip POD_IP
R->>V: --cluster-announce-human-nodename node-name
R->>V: --cluster-announce-hostname prefix-shard.domain
V-->>K8s: CLUSTER NODES shows hostname as metadata
note over V,K8s: cluster-bus still uses pod IPs
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant R as Reconciler
participant K8s as Kubernetes API
participant V as Valkey Node
R->>K8s: reconcileShardServices()
loop per shard
R->>K8s: CreateOrUpdate shard Service (ports: vk-n0, vk-n1 per node)
K8s-->>R: Service with allocated NodePorts
R->>R: shardEndpointFromService() → NodePort list
end
R->>R: "cluster.Status.ExternalEndpoints = endpoints"
R->>K8s: reconcileValkeyNode() per shard/node
R->>K8s: CreateOrUpdate ValkeyNode CR (ExternalAccess copied from cluster)
K8s-->>R: ValkeyNode
note over R,V: ValkeyNode controller
R->>V: Pod: valkey-server --cluster-announce-ip POD_IP
R->>V: --cluster-announce-human-nodename node-name
R->>V: --cluster-announce-hostname prefix-shard.domain
V-->>K8s: CLUSTER NODES shows hostname as metadata
note over V,K8s: cluster-bus still uses pod IPs
Comments Outside Diff (2)
-
internal/controller/valkeynode_resources.go, line 906-909 (link)Empty
HostnamePrefixproduces an invalid DNS labelshardHostname(ea.HostnamePrefix, ...)is called without a fallback whenea.HostnamePrefix == "".shardHostname("", "1", "example.com")returns-1.example.com, which starts with a hyphen and is therefore an invalid DNS label. Valkey would receive--cluster-announce-hostname -1.example.com, which may be rejected or silently misconfigured.In normal API-server usage, the
+kubebuilder:default=shardannotation prevents this. However, objects migrated before the field was added, objects created programmatically, or a future webhook misconfiguration could leaveHostnamePrefixas the Go zero value. Addingif ea.HostnamePrefix == "" { ea.HostnamePrefix = "shard" }(consistent with howServiceTypeis guarded) closes this gap without changing observable behaviour. -
api/v1alpha1/valkeycluster_types.go, line 52-56 (link)Domainfield accepts arbitrary strings without DNS format validationUnlike
HostnamePrefix, theDomainfield carries no+kubebuilder:validation:Patternannotation. A value with invalid DNS characters, a trailing dot (valkey.example.com.), or a bare label without a dot (e.g.,localdomain) would pass API validation and be embedded verbatim in--cluster-announce-hostname, resulting in a malformed FQDN advertised to clients. Adding a permissive DNS-name pattern (e.g.,^([a-z0-9]([a-z0-9-]*[a-z0-9])?\.)+[a-z]{2,}$) would catch obvious mistakes at admission time.
Reviews (1): Last reviewed commit: "feat: announce per-shard hostnames for e..." | Re-trigger Greptile
There was a problem hiding this comment.
Pull request overview
Adds the “per-shard announced hostname” layer for external cluster access, so each node can publish a stable client-facing shard hostname (for CLUSTER metadata / TLS SNI) while keeping cluster-bus traffic on pod IPs and preserving the reconciler’s pod-IP correlation.
Changes:
- Introduces
spec.externalAccess(and mirroredValkeyNodeSpec.externalAccess) plusstatus.externalEndpointsto support per-shard external exposure metadata. - Creates/maintains one Service per shard (NodePort default, LoadBalancer optional) and reports shard external ports into status.
- When
externalAccess.domainis set, appends--cluster-announce-hostname <prefix>-<shardIndex>.<domain>to the server args; adds unit + e2e coverage and docs/sample manifest.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/valkeycluster_external_access_test.go | New e2e covering shard Services/status and that the announced hostname appears in CLUSTER NODES. |
| internal/valkey/clusterstate_test.go | Regression test for parsing CLUSTER NODES when ,hostname is appended to the address field. |
| internal/controller/valkeynode_resources.go | Adds announce flags (cluster-announce-human-nodename, cluster-announce-hostname) and per-node client port naming when external access is enabled. |
| internal/controller/valkeynode_resources_test.go | Unit tests for the new external-access rendering behavior in container args/ports. |
| internal/controller/valkeycluster_controller.go | Reconciles per-shard Services, reads back external ports into status, and propagates ExternalAccess into ValkeyNode specs. |
| internal/controller/utils.go | Adds helpers for per-node port naming and shard hostname formatting. |
| internal/controller/shard_services_unit_test.go | Unit tests for ServicePort layout and endpoint reporting logic. |
| internal/controller/shard_services_test.go | envtest coverage for reconcile behavior (create/cleanup/stability) of shard Services. |
| docs/valkeycluster.md | Documents externalAccess behavior, defaults, status reporting, and TLS cert considerations. |
| config/samples/v1alpha1_valkeycluster-external-access.yaml | New sample CR demonstrating external access + domain. |
| config/samples/kustomization.yaml | Includes the new sample manifest. |
| config/crd/bases/valkey.io_valkeynodes.yaml | CRD schema updates for ValkeyNodeSpec.externalAccess. |
| config/crd/bases/valkey.io_valkeyclusters.yaml | CRD schema updates for spec.externalAccess and status.externalEndpoints. |
| api/v1alpha1/zz_generated.deepcopy.go | Deepcopy updates for new API/status types. |
| api/v1alpha1/valkeynode_types.go | Adds ValkeyNodeSpec.externalAccess. |
| api/v1alpha1/valkeycluster_types.go | Adds ExternalAccessSpec and ShardEndpoint / externalEndpoints status. |
Files not reviewed (1)
- api/v1alpha1/zz_generated.deepcopy.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Domain is the DNS domain under which shard hostnames are announced. When set, | ||
| // each node announces the hostname "<hostnamePrefix>-<shardIndex>.<domain>" to | ||
| // clients in addition to its IP. The hostname must resolve to the shard's Service. | ||
| // +optional | ||
| Domain string `json:"domain,omitempty"` |
| // NodePorts are the external ports of the shard's nodes, indexed by node index | ||
| // (NodePorts[0] is the node-index 0 port). The address to reach each port | ||
| // depends on the Service type and the user's DNS configuration. | ||
| // +optional | ||
| NodePorts []int32 `json:"nodePorts,omitempty"` |
| } | ||
| Eventually(verifyStatus).Should(Succeed()) | ||
|
|
||
| By("verifying each shard Service port resolves to exactly one endpoint") |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| svc.Spec.ExternalTrafficPolicy = ea.ExternalTrafficPolicy | ||
| svc.Spec.Selector = map[string]string{ |
There was a problem hiding this comment.
svc.Spec.ExternalTrafficPolicy = ea.ExternalTrafficPolicy assigns "" when the field is omitted from the cluster spec. Kubernetes defaults the missing field to Cluster on Create and on every subsequent PUT. After that, each reconcile reads back Cluster, the mutate function writes "", CreateOrUpdate detects a difference (via equality.Semantic.DeepEqual), and issues an Update — which Kubernetes normalizes back to Cluster. The cycle repeats on every reconcile tick.
ServiceType avoids exactly this pattern with an explicit guard (if serviceType == "" { serviceType = corev1.ServiceTypeNodePort }). The same guard is needed here. Every user who enables external access without explicitly setting externalTrafficPolicy (including the sample manifest) will drive continuous spurious API writes proportional to the number of shards.
Part of the external cluster access effort (umbrella #276). Stacked on #278 (
external-clusters/shard-services).Summary
This adds a stable, client-facing hostname per shard. When a
domainis set, every node in a shard announces<hostnamePrefix>-<shardIndex>.<domain>, for exampleshard-0.valkey.example.com. The prefix defaults toshardand can be overridden, so multiple clusters can share one domain without colliding.One thing worth calling out: this PR only announces the hostname as metadata. It shows up in CLUSTER SLOTS and is useful for TLS SNI, but it does not change where clients are sent yet. That switch is the final PR. The cluster bus keeps using pod IPs throughout, which is what keeps the reconciler's pod-IP correlation working.
Features / Behaviour Changes
externalAccessgainshostnamePrefix(defaultshard) anddomain.domainset, each shard announces<hostnamePrefix>-<shardIndex>.<domain>.Implementation
buildContainersDefappends--cluster-announce-hostnamewith the shard hostname when external access is enabled and a domain is set, via the same argument path as the other announce flags.hostnamePrefixis validated as a DNS label with a kubebuilder pattern.Limitations
The hostname stays metadata until the next PR sets
cluster-preferred-endpoint-type. You own the DNS records that resolve these hostnames to the shard Services. When TLS is enabled, the certificate needs to cover the shard hostnames in addition to the internal Service FQDN. The docs have a note on this.Testing
make testandmake lintpass locally. Seedocs/valkeycluster.md.Checklist
make testandmake lintinstead)