Skip to content

feat: expose shards through Gateway API TCPRoutes - #281

Open
scrothers wants to merge 5 commits into
valkey-io:mainfrom
scrothers:external-clusters/gateway-tcproute
Open

feat: expose shards through Gateway API TCPRoutes#281
scrothers wants to merge 5 commits into
valkey-io:mainfrom
scrothers:external-clusters/gateway-tcproute

Conversation

@scrothers

Copy link
Copy Markdown

Part of the external cluster access effort (umbrella #276). Stacked on #280 (external-clusters/client-endpoint), and it adds an alternative exposure path.

Summary

This adds an optional way to expose a cluster through the Gateway API. When you set externalAccess.gateway, the operator creates one TCPRoute per node that attaches to a Gateway you already run, and forwards to the node's port on the per-shard Service. The Gateway becomes the single external surface, so the backing Services can stay internal.

Gateway API is treated as a genuinely optional dependency. The operator checks at startup whether the TCPRoute kind is served, and only then registers the watch and reconcile path. On a cluster without the Gateway API CRDs, the operator behaves exactly as it does today and never references a type the API server can't serve.

Features / Behaviour Changes

  • externalAccess.gateway with gatewayRef (same-namespace Gateway, optional sectionName) and basePort.
  • serviceType now also accepts ClusterIP, and defaults to ClusterIP when a Gateway is configured so the backend isn't redundantly exposed.
  • The announced client port is the Gateway listener port, basePort + shardIndex*(replicas+1) + nodeIndex. Clients reach the cluster via the existing hostname mechanism (domain + preferredEndpointType: hostname), with the shard hostname resolving to the Gateway.

Implementation

  • cmd/main.go runs a one-time discovery check for the TCPRoute kind. When present it installs the Gateway API scheme, adds TCPRoute to the managed-by cache filter, and sets a flag on the reconciler. SetupWithManager only watches TCPRoutes when that flag is set, so the manager starts cleanly without the CRDs.
  • reconcileShardRoutes creates one TCPRoute per node, owned by the cluster and carrying the managed-by label, with a parentRef to the Gateway and a backendRef to the shard Service on the node's port. It cleans up routes left by a scale-in, and records a warning event if a Gateway is configured while the CRDs are absent.
  • The Gateway listener port is computed by a shared helper, so the announced client port and the route's parent port can't drift.
  • Adds an RBAC rule for tcproutes (see the note below).

Limitations

  • Same-namespace Gateways only; no ReferenceGrant handling yet.
  • The operator emits TCPRoutes but does not manage the Gateway, its listeners, or DNS. You provision a listener per node port and the records that point the shard hostnames at the Gateway.
  • TCPRoute is still a v1alpha2 (experimental-channel) kind in Gateway API v1.5.1; the import will move to v1 once upstream graduates it.

Note for reviewers: this PR adds a +kubebuilder:rbac marker for tcproutes, so the ClusterRole in the valkey-helm chart needs the same rule added by hand, per CONTRIBUTING. Flagging it explicitly so Helm installs aren't left short a permission.

Testing

  • Unit tests cover the listener-port formula, the ClusterIP defaulting, the Gateway-aware client port, the route name, and the TCPRoute spec shape (parentRef, sectionName, backendRef).
  • envtest covers the gated path: with the Gateway API absent, configuring a Gateway is a no-op that records a warning, and reconcile still succeeds. (Route creation with the CRDs present is covered in e2e, since envtest doesn't load the Gateway API CRDs.)
  • An e2e spec (GatewayAPI label) installs the Gateway API CRDs, creates a Gateway and a cluster, and asserts one TCPRoute per node with the right backend, the ClusterIP default, and route cleanup when the Gateway config is removed.
  • make test and make lint pass locally.

Checklist

  • This Pull Request is related to one issue.
  • Commit message explains what changed and why
  • Tests are added or updated.
  • Documentation files are updated.
  • I have run pre-commit locally (ran make test and make lint instead)

### 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>
### Summary
Complete external access by directing clients to the per-shard hostname and
its external port. Each node announces the external client port allocated by
its shard Service, and `preferredEndpointType: hostname` makes MOVED/ASK and
CLUSTER SLOTS hand clients the shard hostname. Node-to-node traffic still uses
pod IPs, so the operator's reconciler is unaffected.

### Features / Behaviour Changes
- `externalAccess` gains `preferredEndpointType` (`ip` default, or `hostname`).
- With `hostname`, clients connecting to any shard receive cross-shard
  redirects to `<hostnamePrefix>-<shardIndex>.<domain>` and the node's
  external port.

### Implementation
- The cluster controller resolves each node's external port from
  `status.externalEndpoints` (populated by the shard Service reconcile) and
  sets it on the ValkeyNode spec. `buildContainersDef` announces it with
  `--cluster-announce-client-port`, or `--cluster-announce-client-tls-port`
  when TLS is enabled (a TLS cluster runs the client listener on the TLS port).
- `cluster-preferred-endpoint-type hostname` is added to the shared base config
  when selected, so it rolls through the existing config-hash path. It is
  cluster-wide, so it is not per-node.
- A CEL rule requires `domain` when `preferredEndpointType` is `hostname`.

### Limitations
`preferredEndpointType` is cluster-wide, so in-cluster clients are also directed
to the hostname; resolve it internally with split-horizon DNS or accept the
hairpin. To disable external access, set `preferredEndpointType: ip` before
removing `domain` or disabling, so clients are moved off the hostnames first.

### Testing
- Unit tests cover the client-port flag selection (plain vs TLS) and that
  `cluster-preferred-endpoint-type` is rendered only when hostname is selected.
- `make test` and `make lint` pass locally.

Signed-off-by: Steven Crothers <steven@scrothers.com>
### Summary
Add an optional way to expose a cluster through the Gateway API. When
externalAccess.gateway is set, the operator creates one TCPRoute per node
that attaches to a Gateway the user already runs and forwards to the node's
port on the per-shard Service, so the Gateway becomes the single external
surface and the backing Services can stay internal.

Gateway API is treated as an optional dependency. The operator discovers at
startup whether the TCPRoute kind is served and only then registers the
watch and reconcile path; on a cluster without the Gateway API CRDs it runs
exactly as before and never references a type the API server cannot serve.

### Features / Behaviour Changes
- externalAccess.gateway with gatewayRef (same-namespace Gateway, optional
  sectionName) and basePort.
- serviceType also accepts ClusterIP, and defaults to ClusterIP when a
  Gateway is configured so the backend is not redundantly exposed.
- The announced client port is the Gateway listener port,
  basePort + shardIndex*(replicas+1) + nodeIndex. Clients reach the cluster
  via the existing hostname mechanism (domain + preferredEndpointType:
  hostname), with the shard hostname resolving to the Gateway.

### Implementation
- cmd/main.go runs a one-time discovery check for the TCPRoute kind. When
  present it installs the Gateway API scheme, adds TCPRoute to the
  managed-by cache filter, and sets a flag on the reconciler.
  SetupWithManager only watches TCPRoutes when that flag is set, so the
  manager starts cleanly without the CRDs.
- reconcileShardRoutes creates one TCPRoute per node, owned by the cluster
  and labelled managed-by, with a parentRef to the Gateway and a backendRef
  to the shard Service on the node's port. It removes routes left by a
  scale-in and records a warning event if a Gateway is configured while the
  CRDs are absent.
- A shared helper computes the Gateway listener port, so the announced
  client port and the route's parent port cannot drift.

### Limitations
- Same-namespace Gateways only; no ReferenceGrant handling yet.
- The operator emits TCPRoutes but does not manage the Gateway, its
  listeners, or DNS.
- TCPRoute is a v1alpha2 (experimental-channel) kind in Gateway API v1.5.1;
  the import will move to v1 once upstream graduates it.

Note: this change adds a +kubebuilder:rbac marker for tcproutes, so the
ClusterRole in the valkey-helm chart needs the same rule added by hand, per
CONTRIBUTING.

### Testing
- Unit tests cover the listener-port formula, the ClusterIP defaulting, the
  Gateway-aware client port, the route name, and the TCPRoute spec shape.
- envtest covers the gated path: with the Gateway API absent, configuring a
  Gateway is a no-op that records a warning and still reconciles.
- An e2e spec (GatewayAPI label) installs the Gateway API CRDs, creates a
  Gateway and a cluster, and asserts one TCPRoute per node with the right
  backend, the ClusterIP default, and route cleanup on removal.
- make test and make lint pass locally.

Signed-off-by: Steven Crothers <steven@scrothers.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends the Valkey operator’s “external access” feature by adding an optional Gateway API exposure path. When spec.externalAccess.gateway is configured, the operator creates one Gateway API TCPRoute per node that attaches to an existing Gateway and forwards to the node’s port on the per-shard Service; the operator also gates all Gateway API interactions behind a startup discovery check so clusters without Gateway API CRDs behave as before.

Changes:

  • Add externalAccess API/CRD fields (including gatewayRef + basePort) and publish per-shard external endpoints in cluster status.
  • Reconcile per-shard Services and (optionally) per-node TCPRoutes; announce human node names / shard hostnames / external client ports to Valkey.
  • Add unit/envtest/e2e coverage and update docs/samples/RBAC; bump dependencies including Gateway API.

Reviewed changes

Copilot reviewed 24 out of 26 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test/e2e/valkeycluster_gateway_test.go E2E coverage for Gateway API CRD install + TCPRoute creation/cleanup behavior.
test/e2e/valkeycluster_external_access_test.go E2E coverage for per-shard NodePort Services and status port reporting.
internal/valkey/clusterstate_test.go Unit test ensuring CLUSTER NODES parsing works with appended announced hostnames.
internal/controller/valkeynode_resources.go Add external-access-related Valkey announce flags and per-node port naming.
internal/controller/valkeynode_resources_test.go Unit tests for new external access announce arguments and port naming.
internal/controller/valkeycluster_controller.go Reconcile shard Services + optional TCPRoutes; compute external client port; watch TCPRoutes conditionally.
internal/controller/utils.go Add helpers for shard client port naming and shard hostname formatting.
internal/controller/shard_services_unit_test.go Unit tests for shard Service port generation and endpoint extraction.
internal/controller/shard_services_test.go Envtest specs for shard Service reconcile + scale-in/disable cleanup behavior.
internal/controller/shard_routes_unit_test.go Unit tests for gateway listener port math and TCPRoute spec generation.
internal/controller/shard_routes_test.go Envtest specs for “Gateway API absent” gated/no-op behavior and warning event.
internal/controller/config.go Render cluster-preferred-endpoint-type hostname when external access selects hostname endpoints.
internal/controller/config_test.go Tests for preferred-endpoint-type rendering under external access.
go.mod Add/bump dependencies (including Gateway API) and refresh Kubernetes/Ginkgo/Gomega versions.
go.sum Dependency checksum updates aligned with go.mod bumps.
docs/valkeycluster.md Document external access and Gateway API configuration/behavior.
config/samples/v1alpha1_valkeycluster-gateway.yaml New sample manifest for Gateway-backed exposure.
config/samples/v1alpha1_valkeycluster-external-access.yaml New sample manifest for NodePort-based external access.
config/samples/kustomization.yaml Include new sample manifests in kustomize samples list.
config/rbac/role.yaml Add RBAC permissions for gateway.networking.k8s.io/tcproutes.
config/crd/bases/valkey.io_valkeynodes.yaml Extend ValkeyNode CRD schema with external access fields.
config/crd/bases/valkey.io_valkeyclusters.yaml Extend ValkeyCluster CRD schema with external access fields, validation, and status endpoints.
cmd/main.go Add startup discovery gate for TCPRoute kind; conditionally install scheme/cache and enable controller behavior.
api/v1alpha1/zz_generated.deepcopy.go Deepcopy generation for new API types/fields.
api/v1alpha1/valkeynode_types.go Add ExternalAccess + ExternalAccessClientPort fields to ValkeyNodeSpec.
api/v1alpha1/valkeycluster_types.go Add ExternalAccessSpec/GatewayExposureSpec/ShardEndpoint types and validations to ValkeyCluster.
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.

Comment on lines +539 to +548
svc.Labels[LabelShardIndex] = shardSelector
svc.Annotations = maps.Clone(ea.ServiceAnnotations)
svc.Spec.Type = serviceType
svc.Spec.ExternalTrafficPolicy = ea.ExternalTrafficPolicy
svc.Spec.Selector = map[string]string{
LabelCluster: cluster.Name,
LabelShardIndex: shardSelector,
}
svc.Spec.Ports = buildShardServicePorts(svc.Spec.Ports, nodesPerShard)
return controllerutil.SetControllerReference(cluster, svc, r.Scheme)
Comment on lines +641 to +643
// It is a no-op (beyond cleaning up any existing routes) unless the Gateway API is
// installed; when the user configures a Gateway on a cluster without the CRDs, it
// records a one-time warning event so the misconfiguration is visible.
@greptile-apps

greptile-apps Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds Gateway API TCPRoute exposure as an optional path for making Valkey cluster shards externally reachable. A startup discovery check conditionally registers the Gateway API scheme and cache only when TCPRoute CRDs are present, so the operator degrades gracefully on clusters without them.

  • New per-shard Services: one Service per shard with one port per node, using uniquely-named container ports (vk-n<nodeIndex>) as targetPort so each port reaches exactly one pod; NodePort values are preserved across updates.
  • TCPRoute management: one TCPRoute per node attaches to a user-provided Gateway using a flat port index formula (basePort + shardIndex*nodesPerShard + nodeIndex); routes are garbage-collected on scale-in or when Gateway config is removed.
  • cluster-announce-* flags: external access injects --cluster-announce-hostname, --cluster-announce-client-port / --cluster-announce-client-tls-port, and --cluster-announce-human-nodename into the Valkey server command; the announced client port is deterministic for Gateway-backed clusters and read from Service status for NodePort clusters.

Confidence Score: 3/5

Safe for new clusters but can wedge the operator on existing clusters that gain a Gateway config.

The NodePort-to-ClusterIP transition bug in buildShardServicePorts will cause every reconcile to fail with a Kubernetes validation error for any existing cluster that had NodePort-based external access and then has a gateway: block added. The operator gets into an update-fail loop for that cluster until the shard Services are manually patched. The overall design is sound and the happy path works correctly.

internal/controller/valkeycluster_controller.go — specifically buildShardServicePorts and the reconcileShardServices mutate closure where ExternalTrafficPolicy is applied unconditionally.

Important Files Changed

Filename Overview
internal/controller/valkeycluster_controller.go Core reconcile loop extended with shard Services and TCPRoutes; NodePort-to-ClusterIP conversion has a correctness bug, and externalTrafficPolicy is set unconditionally on all service types.
api/v1alpha1/valkeycluster_types.go New ExternalAccessSpec, GatewayExposureSpec, GatewayReference, and ShardEndpoint types added with well-formed validation markers; CEL rule enforces domain when preferredEndpointType is hostname.
cmd/main.go One-time Gateway API discovery check at startup; conditionally registers scheme and cache entry only when TCPRoute CRD is served.
internal/controller/valkeynode_resources.go Appends cluster-announce flags and renames the client container port to the node-unique name when external access is enabled; gated correctly on ea.Enabled.
internal/controller/config.go Adds cluster-preferred-endpoint-type=hostname to base config when preferredEndpointType is set; simple and correctly guarded.
api/v1alpha1/valkeynode_types.go Adds ExternalAccess and ExternalAccessClientPort fields to ValkeyNodeSpec; straightforward field propagation.
internal/controller/utils.go Two small helpers added: shardClientPortName (port naming) and shardHostname (hostname formatting); both correct and well-documented.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant op as Operator (startup)
    participant disc as Discovery API
    participant mgr as Controller Manager

    op->>disc: ServerResourcesForGroupVersion(gateway.networking.k8s.io/v1alpha2)
    alt TCPRoute found
        disc-->>op: APIResources [TCPRoute, ...]
        op->>mgr: Install gatewayv1alpha2 scheme
        op->>mgr: Cache TCPRoute with managed-by label
        op->>mgr: "GatewayAPIEnabled=true"
    else CRDs absent
        disc-->>op: 404 / error
        op->>mgr: "GatewayAPIEnabled=false"
    end

    participant reconciler as Reconcile loop
    participant k8s as Kubernetes API

    reconciler->>k8s: CreateOrUpdate per-shard Service (NodePort or ClusterIP)
    k8s-->>reconciler: Service (with allocated NodePorts)
    reconciler->>reconciler: "cluster.Status.ExternalEndpoints = shardEndpoints"

    alt "GatewayAPIEnabled && gateway configured"
        loop shardIndex x nodeIndex
            reconciler->>k8s: CreateOrUpdate TCPRoute
        end
        reconciler->>k8s: Delete excess TCPRoutes (scale-in)
    else gateway configured but CRDs absent
        reconciler->>reconciler: Emit GatewayAPIUnavailable warning event
    end

    reconciler->>k8s: CreateOrUpdate ValkeyNode (ExternalAccessClientPort set)
Loading
%%{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 (startup)
    participant disc as Discovery API
    participant mgr as Controller Manager

    op->>disc: ServerResourcesForGroupVersion(gateway.networking.k8s.io/v1alpha2)
    alt TCPRoute found
        disc-->>op: APIResources [TCPRoute, ...]
        op->>mgr: Install gatewayv1alpha2 scheme
        op->>mgr: Cache TCPRoute with managed-by label
        op->>mgr: "GatewayAPIEnabled=true"
    else CRDs absent
        disc-->>op: 404 / error
        op->>mgr: "GatewayAPIEnabled=false"
    end

    participant reconciler as Reconcile loop
    participant k8s as Kubernetes API

    reconciler->>k8s: CreateOrUpdate per-shard Service (NodePort or ClusterIP)
    k8s-->>reconciler: Service (with allocated NodePorts)
    reconciler->>reconciler: "cluster.Status.ExternalEndpoints = shardEndpoints"

    alt "GatewayAPIEnabled && gateway configured"
        loop shardIndex x nodeIndex
            reconciler->>k8s: CreateOrUpdate TCPRoute
        end
        reconciler->>k8s: Delete excess TCPRoutes (scale-in)
    else gateway configured but CRDs absent
        reconciler->>reconciler: Emit GatewayAPIUnavailable warning event
    end

    reconciler->>k8s: CreateOrUpdate ValkeyNode (ExternalAccessClientPort set)
Loading

Reviews (1): Last reviewed commit: "feat: expose shards through Gateway API ..." | Re-trigger Greptile

Comment on lines +564 to +584
// buildShardServicePorts returns one ServicePort per node, targeting the node's
// uniquely-named client port. Existing ports are passed in so Kubernetes-allocated
// NodePort values are preserved across updates. The Port value is the in-cluster
// port and only needs to be unique within the Service.
func buildShardServicePorts(existing []corev1.ServicePort, nodesPerShard int) []corev1.ServicePort {
nodePortByName := make(map[string]int32, len(existing))
for _, p := range existing {
nodePortByName[p.Name] = p.NodePort
}

ports := make([]corev1.ServicePort, 0, nodesPerShard)
for nodeIndex := range nodesPerShard {
name := shardClientPortName(strconv.Itoa(nodeIndex))
ports = append(ports, corev1.ServicePort{
Name: name,
Port: int32(DefaultPort + nodeIndex),
TargetPort: intstr.FromString(name),
NodePort: nodePortByName[name],
})
}
return ports

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 NodePort values preserved on ClusterIP conversion

buildShardServicePorts always copies the existing NodePort values from the previous service, regardless of the target service type. When a user transitions from NodePort to ClusterIP by adding a gateway: block, the mutate function receives the existing service (which has allocated NodePorts like 31000, 31001), builds the new port list with those same non-zero NodePort fields, and then also sets svc.Spec.Type = ClusterIP. Kubernetes rejects any service update where a ClusterIP service has non-zero nodePort fields. The result is a validation error on every reconcile until the user manually patches the service — the operator gets stuck in an update-fail loop for this cluster.

The fix is to pass serviceType into buildShardServicePorts and zero out NodePort when the target type is not NodePort.

Comment on lines +1010 to +1015
// gatewayListenerPort returns the Gateway listener port for a node, computed as a
// flat index across the cluster so every node maps to a distinct listener:
// basePort + shardIndex*nodesPerShard + nodeIndex.
func gatewayListenerPort(basePort int32, shardIndex, nodeIndex, nodesPerShard int) int32 {
return basePort + int32(shardIndex*nodesPerShard+nodeIndex)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The computed listener port is never checked against the 65535 maximum. With basePort=65000, 3 shards, and 3 replicas (nodesPerShard=4), the highest port reaches 65000 + 2*4 + 3 = 65011 — still fine. But with more shards or a higher base, the value silently overflows the valid port range and produces a TCPRoute whose port field the Gateway implementation rejects. Adding a CRD maximum validation or a server-side check prevents silent misconfiguration.

Suggested change
// gatewayListenerPort returns the Gateway listener port for a node, computed as a
// flat index across the cluster so every node maps to a distinct listener:
// basePort + shardIndex*nodesPerShard + nodeIndex.
func gatewayListenerPort(basePort int32, shardIndex, nodeIndex, nodesPerShard int) int32 {
return basePort + int32(shardIndex*nodesPerShard+nodeIndex)
}
// gatewayListenerPort returns the Gateway listener port for a node, computed as a
// flat index across the cluster so every node maps to a distinct listener:
// basePort + shardIndex*nodesPerShard + nodeIndex.
func gatewayListenerPort(basePort int32, shardIndex, nodeIndex, nodesPerShard int) int32 {
port := basePort + int32(shardIndex*nodesPerShard+nodeIndex)
if port > 65535 {
return 65535
}
return port
}

Comment on lines +539 to +542
svc.Labels[LabelShardIndex] = shardSelector
svc.Annotations = maps.Clone(ea.ServiceAnnotations)
svc.Spec.Type = serviceType
svc.Spec.ExternalTrafficPolicy = ea.ExternalTrafficPolicy

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 externalTrafficPolicy set unconditionally on ClusterIP services

svc.Spec.ExternalTrafficPolicy = ea.ExternalTrafficPolicy is applied to every shard Service regardless of its type. When serviceType is ClusterIP (the default with a Gateway) and the user has explicitly set externalTrafficPolicy: Local, Kubernetes 1.24+ returns a validation error because externalTrafficPolicy is only meaningful for NodePort and LoadBalancer services. This would block Service creation or update for any user who sets both externalTrafficPolicy and gateway. Guarding the field behind a type check (only write it for NodePort/LoadBalancer) avoids the rejection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants