feat: add per-shard Services for external cluster access - #278
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>
|
| Filename | Overview |
|---|---|
| internal/controller/valkeycluster_controller.go | Adds reconcileShardServices and deleteExcessShardServices; the ExternalTrafficPolicy zero-value assignment causes a reconcile loop in production when the field is omitted. |
| api/v1alpha1/valkeycluster_types.go | Adds ExternalAccessSpec and ShardEndpoint types; ExternalTrafficPolicy is missing a +kubebuilder:default=Cluster annotation, leaving its zero value as "" rather than "Cluster". |
| internal/controller/valkeynode_resources.go | Renames the server container port to vk-n{idx} when external access is enabled and appends --cluster-announce-human-nodename; logic is clean and backward-compatible. |
| internal/controller/shard_services_unit_test.go | Good unit coverage of port-building and NodePort preservation; tests are focused and correct. |
| internal/controller/shard_services_test.go | envtest integration tests for shard service lifecycle; the no-op second reconcile test does not verify absence of updates, so the ExternalTrafficPolicy loop is not caught here. |
| test/e2e/valkeycluster_external_access_test.go | e2e test covers NodePort allocation and status reporting; endpoint-count assertion is too weak to verify the single-pod-per-port invariant. |
| api/v1alpha1/valkeynode_types.go | Adds ExternalAccess field to ValkeyNodeSpec, correctly forwarded from the cluster spec. |
| internal/controller/utils.go | Adds shardClientPortName helper; straightforward and within the 15-character port-name constraint. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Op as Operator Reconciler
participant K8s as Kubernetes API
participant Svc as Shard Service
participant Pod as Valkey Pod
Op->>K8s: reconcileShardServices (per shard)
K8s-->>Op: "existing Service (ExternalTrafficPolicy=Cluster)"
Op->>Op: "mutate: set ExternalTrafficPolicy="""
Note over Op: "" != "Cluster" triggers Update
Op->>K8s: "Update Service (ExternalTrafficPolicy="" / omitempty)"
K8s-->>Op: normalizes back to "Cluster"
Note over Op,K8s: Loop: every reconcile triggers a no-op Service Update
Op->>K8s: CreateOrUpdate shard Service (NodePort)
K8s-->>Op: allocated NodePorts preserved via buildShardServicePorts
Op->>K8s: status.externalEndpoints patched with ShardEndpoints
Op->>Pod: "container port renamed vk-n{idx} (external access on)"
Pod->>Svc: "targetPort=vk-n{idx} resolves to exactly one pod endpoint"
%%{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 Op as Operator Reconciler
participant K8s as Kubernetes API
participant Svc as Shard Service
participant Pod as Valkey Pod
Op->>K8s: reconcileShardServices (per shard)
K8s-->>Op: "existing Service (ExternalTrafficPolicy=Cluster)"
Op->>Op: "mutate: set ExternalTrafficPolicy="""
Note over Op: "" != "Cluster" triggers Update
Op->>K8s: "Update Service (ExternalTrafficPolicy="" / omitempty)"
K8s-->>Op: normalizes back to "Cluster"
Note over Op,K8s: Loop: every reconcile triggers a no-op Service Update
Op->>K8s: CreateOrUpdate shard Service (NodePort)
K8s-->>Op: allocated NodePorts preserved via buildShardServicePorts
Op->>K8s: status.externalEndpoints patched with ShardEndpoints
Op->>Pod: "container port renamed vk-n{idx} (external access on)"
Pod->>Svc: "targetPort=vk-n{idx} resolves to exactly one pod endpoint"
Comments Outside Diff (3)
-
internal/controller/valkeycluster_controller.go, line 717 (link)Reconcile loop when
ExternalTrafficPolicyis unsetea.ExternalTrafficPolicyis the zero value""when the user omits the field. Assigning""tosvc.Spec.ExternalTrafficPolicyinside the mutate function causescontrollerutil.CreateOrUpdateto see a diff on every reconcile: the API server normalises the field to"Cluster"after creation, but the mutate function resets it to""each time, makingequality.Semantic.DeepEqualreturn false and triggering an update on every pass through the reconcile loop. The update itself is a no-op from Kubernetes's perspective (the field isomitemptyso the API server keeps"Cluster"), but it generates a ResourceVersion bump, fires a watch event, and re-queues the reconciler indefinitely.The fix is to apply the same defaulting pattern already used for
serviceType:externalTrafficPolicy := ea.ExternalTrafficPolicy if externalTrafficPolicy == "" { externalTrafficPolicy = corev1.ServiceExternalTrafficPolicyCluster } svc.Spec.ExternalTrafficPolicy = externalTrafficPolicy
The envtest "no-op second reconcile" test does not catch this because the fake API server in envtest stores
""exactly as written rather than normalising it to"Cluster". -
test/e2e/valkeycluster_external_access_test.go, line 993-1002 (link)Endpoint-count assertion is not tight enough
verifyEndpointsonly asserts that the EndpointSlice for shard 0 is non-empty (NotTo(BeEmpty())). The whole point of the per-node port trick is that each Service port resolves to exactly one pod, not just any pod. A misconfigured port name (e.g. a missingLabelNodeIndexlabel leavingshardClientPortNamereturning"vk-n") could still produce a non-empty list and pass this check. Consider asserting the count equalsnodesPerShard(or at least> 0with a distinct check per port) to validate the invariant the implementation relies on.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
api/v1alpha1/valkeycluster_types.go, line 35-37 (link)ExternalTrafficPolicyhas no kubebuilder defaultThe
ServiceTypefield carries// +kubebuilder:default=NodePort, which means the CRD storesNodePortwhen the field is omitted.ExternalTrafficPolicyhas no equivalent annotation, so the field is stored as""in the CR. This is the root structural cause of the reconcile-loop issue flagged in the controller: the zero-value""propagates into the Service mutate function unchanged. Adding// +kubebuilder:default=Clusterwould make the CR field self-consistent with how Kubernetes defaults the Service field, eliminate the need for the nil-check guard in the controller, and match the documented default indocs/valkeycluster.md.
Reviews (1): Last reviewed commit: "feat: expose each shard through an exter..." | Re-trigger Greptile
There was a problem hiding this comment.
Pull request overview
This PR adds the “networking layer” for externalAccess by introducing one Service per shard (with one port per node) so external clients can target a specific primary/replica, and by publishing the externally reachable ports on ValkeyCluster.status.externalEndpoints.
Changes:
- Reconcile per-shard Services when
spec.externalAccess.enabledis true (NodePort default, optionally LoadBalancer) and clean them up on scale-in/disable. - Rename each ValkeyNode’s client container port to a node-unique name (
vk-n<idx>) when external access is enabled so Service ports can be pinned to exactly one pod. - Add status reporting, docs, sample manifest, and unit/envtest/e2e coverage for the new external access behavior.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
test/e2e/valkeycluster_external_access_test.go |
New e2e spec for per-shard NodePort Services and status.externalEndpoints. |
internal/controller/valkeynode_resources.go |
Renames the client container port (and appends announce args) when external access is enabled. |
internal/controller/valkeynode_resources_test.go |
Adds unit tests around external access container command rendering. |
internal/controller/valkeycluster_controller.go |
Adds reconcileShardServices, status write-back, and propagates ExternalAccess into ValkeyNodes. |
internal/controller/utils.go |
Adds shardClientPortName helper used by per-shard Services and node port renaming. |
internal/controller/shard_services_unit_test.go |
Unit tests for per-node ServicePort layout and endpoint extraction. |
internal/controller/shard_services_test.go |
Envtest-style coverage for shard Service reconcile, cleanup, and NodePort stability. |
docs/valkeycluster.md |
Documents externalAccess behavior and fields, and mentions status reporting. |
config/samples/v1alpha1_valkeycluster-external-access.yaml |
New sample CR enabling external access. |
config/samples/kustomization.yaml |
Adds the new external-access sample to the samples kustomization. |
config/crd/bases/valkey.io_valkeynodes.yaml |
CRD schema updates for ValkeyNode.spec.externalAccess. |
config/crd/bases/valkey.io_valkeyclusters.yaml |
CRD schema updates for ValkeyCluster.spec.externalAccess and status.externalEndpoints. |
api/v1alpha1/zz_generated.deepcopy.go |
Generated deep-copy updates for new API/status types. |
api/v1alpha1/valkeynode_types.go |
Adds ValkeyNode.spec.externalAccess propagation field. |
api/v1alpha1/valkeycluster_types.go |
Defines ExternalAccessSpec and ShardEndpoint status type. |
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.
| if ea := node.Spec.ExternalAccess; ea != nil && ea.Enabled { | ||
| containers[0].Command = append(containers[0].Command, | ||
| "--cluster-announce-human-nodename", node.Name) | ||
| containers[0].Ports[0].Name = shardClientPortName(node.Labels[LabelNodeIndex]) | ||
| } |
| func TestBuildContainersDef_ExternalAccessHumanNodename(t *testing.T) { | ||
| node := newTestValkeyNode("mycluster-1-2", "test-ns") | ||
| node.Spec.ExternalAccess = &valkeyv1.ExternalAccessSpec{Enabled: true} | ||
|
|
||
| containers, err := buildContainersDef(node) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Equal(t, | ||
| []string{"valkey-server", "/config/valkey.conf", "--cluster-announce-ip", "$(POD_IP)", "--cluster-announce-human-nodename", "mycluster-1-2"}, | ||
| containers[0].Command, | ||
| "enabling external access should announce the ValkeyNode name as the human nodename") | ||
| } |
| By("verifying each shard Service port resolves to exactly one endpoint") | ||
| verifyEndpoints := func(g Gomega) { | ||
| cmd := exec.Command("kubectl", "get", "endpointslices", | ||
| "-l", fmt.Sprintf("kubernetes.io/service-name=%s", controllerShardServiceName(clusterName, 0)), | ||
| "-o", "jsonpath={range .items[*].endpoints[*]}{.addresses[0]}{\"\\n\"}{end}") | ||
| output, err := utils.Run(cmd) | ||
| g.Expect(err).NotTo(HaveOccurred()) | ||
| g.Expect(utils.GetNonEmptyLines(output)).NotTo(BeEmpty()) | ||
| } | ||
| Eventually(verifyEndpoints).Should(Succeed()) |
| // 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"` |
| | `externalTrafficPolicy` | `Cluster` (default) or `Local`. Use `Local` to preserve the client source IP. | | ||
| | `serviceAnnotations` | Applied to each per-shard Service, e.g. for external-dns or a cloud load-balancer controller. | | ||
|
|
||
| With `NodePort`, Kubernetes allocates the external ports; the operator reads them back and reports them per shard under `status.externalEndpoints` (indexed by node). With `LoadBalancer`, each shard's node ports are `6379 + nodeIndex`. |
| assert.Equal(t, int32(DefaultPort+i), p.Port) | ||
| // targetPort references the node-unique container port name so the port | ||
| // reaches exactly one pod. | ||
| name := "vk-n" + strconv.Itoa(i) |
Part of the external cluster access effort (umbrella #276). Stacked on #277 (
external-clusters/human-nodename), so please review that one first.Summary
This is the networking layer. When external access is enabled, the operator now creates one Service per shard and exposes each node in the shard on its own port. That means a client can reach a specific primary or replica, which is what cluster clients need for MOVED/ASK redirects to work later in the stack.
The external ports are surfaced on the cluster status under
status.externalEndpoints, so users can find the Kubernetes-allocated NodePorts without digging throughkubectl get svc.Features / Behaviour Changes
externalAccessgainsserviceType(NodePortdefault, orLoadBalancer),externalTrafficPolicy, andserviceAnnotations.status.externalEndpointsreports each shard's external ports, indexed by node.Implementation
reconcileShardServicesupserts a Service per shard that selects only that shard's pods. Each Service has one port per node, and that port'stargetPortreferences a node-unique container port name (vk-n<idx>). Kubernetes only adds a pod to a Service port's endpoints if the pod declares a container port with that name, so each port resolves to exactly one pod. The server container port is renamed accordingly when external access is on.6379 + nodeIndex.managed-byso the manager's cache sees them, and are owned by the cluster. Services for shards beyond the desired count (or all of them when external access is disabled) are deleted.Limitations
This makes shards reachable but doesn't yet hand clients an external endpoint, so cross-shard MOVED redirects still point at pod IPs. That comes in the next two PRs. DNS is the user's responsibility; the operator only applies the annotations you give it.
Testing
managed-bylabel, a no-op second reconcile with stable ports, and scale-in / disable teardown.make testandmake lintpass locally. Seedocs/valkeycluster.md.Checklist
make testandmake lintinstead)