[HYPERSHELL-177] Implement GatewayNetwork reconciliation

[JuanmaBM]

What

Replaces the no-op GatewayNetworkReconciler stub in the control plane with a real reconciler, and adds the spec that defines its contract.

A GatewayNetwork has no direct Kubernetes footprint in this scope (like a GatewayRelease). Reconciling one therefore means:

1. Validate the declared configuration:
   • topology is a recognized value (mesh, hub-spoke);
   • a hub-spoke network designates a hub_gateway_id;
   • a configured hub_gateway_id (any topology) references an existing Gateway.
2. Write back a deterministic status (Valid, or Invalid: <reason>), idempotently (no write when the persisted status already matches).

Behavior details:

• A definitive not-found for the hub gateway settles the network to Invalid; a transient hub-lookup or status-write failure surfaces as a returned error rather than silent success (re-converges on watch reconnect).
• Delete and nil-resource events are terminal no-ops with no cluster footprint.

Mirrors the sibling GatewayReleaseReconciler pattern (HYPERSHELL-173).

Out of scope (future work)

Actual gateway-to-gateway connectivity provisioning (mesh/tunnel/NetworkPolicy, inter-cluster networking) is deferred pending product definition of the network membership model (the model today designates only a single hub_gateway_id, not a set of members) and a chosen connectivity technology. This is documented explicitly in the spec's Scope Boundary.

Changes

• specs/platform/gateway-network-reconciliation.spec.md (new) + link from control-plane.spec.md
• components/control-plane/internal/reconciler/reconciler.go — real GatewayNetworkReconciler
• components/control-plane/cmd/hypershell-controller/main.go — pass gRPC conn to the constructor
• components/control-plane/internal/reconciler/gateway_network_test.go (new) — unit tests covering every spec scenario

Testing

• go build ./..., go vet ./... clean
• Full control-plane test suite green (11 new TestGatewayNetwork_* cases)
• make check green

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Team

Run ID: 9e5f63de-9b29-4da3-aaa5-63c804dc5d14

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

_{Comment @coderabbitai help to get the list of available commands.}

[jsell-rh]

Amber review

Status: Complete

View the submitted review.

[jsell-rh]

Verdict

COMMENT. This is a clean, well-tested replacement of the GatewayNetworkReconciler no-op stub with a real validation + deterministic status write-back contract, faithfully mirroring the established sibling-reconciler pattern. No blockers or defects; a couple of minor clarifications and one cross-PR ordering/coordination item are noted below.

I reviewed the diff against CLAUDE.md, the security spec, and the control-plane conventions spec.

Strengths

• Correct transient-vs-terminal error discipline: a definitive NotFound for the hub settles the network to Invalid, while a transient hub-lookup or status-write failure returns an error for requeue instead of silently succeeding (matches "never silently swallow partial failures").
• Idempotent, feedback-safe status write-back: the net.GetStatus() != desiredStatus guard prevents both redundant writes and a write→watch-event→reconcile loop, since the second pass computes the same status and no-ops.
• Partial update is safe: UpdateGatewayNetwork only sends the Status field, and the API-server handler (plugins/gatewayNetworks/grpc_handler.go) does a get-then-apply-set-fields, so no other network fields are clobbered.
• Error wrapping (fmt.Errorf("...: %w", err)), context propagation from the stream, and endSpan(reconcileErr) (an improvement over the sibling's endSpan(nil)) are all correct.
• The new test file is purely additive and covers every spec scenario, including transient failure, dangling hub, and no-redundant-write cases. No pre-existing test assertions were modified.

Minor findings

1. [Minor] updateStatus(ctx, id, status string) shadows the imported google.golang.org/grpc/status package with the parameter name status. It compiles because the package isn't referenced inside that function, but it's a readability/lint trap - Style (reconciler.go L2287).
2. [Minor] The conn == nil degraded path returns networkStatusValid for a network with a configured hub without verifying the hub exists, and the doc comment attributes the nil case to "the controller runs without a Kubernetes client." conn is the API-server gRPC connection (created at main.go:92, fatal on dial error), not the Kubernetes client, so this branch is effectively unreachable in production and the comment is misleading. Consider dropping the dead branch or correcting the rationale - Clarity (reconciler.go L2177-2180, L2269-2270).

Cross-PR coordination

• #235 (GatewayRelease reconciliation): This PR must merge after #235, and the two authors should align on one point. (a) The new spec gateway-network-reconciliation.spec.md cross-links gateway-release-reconciliation.spec.md, which only exists in #235; merging this PR first leaves a dangling reference, and both PRs add adjacent rows to the same control-plane.spec.md sub-spec table and edit the same adjacent constructor lines in main.go and the same region of reconciler.go. (b) More substantively, the two sibling reconcilers introduce divergent "healthy" status vocabularies for the same reconciliation contract - Available for a release vs Valid for a network (both share Invalid). Maintainers should decide whether that divergence is intentional or whether the sibling reconcilers should use a consistent status vocabulary, and fix the merge order accordingly.
• #200 (control plane reconciliation contract): #200 introduces specs/standards/control-plane/reconciliation-contract.spec.md as the canonical, cross-resource reconciliation standard, while this PR authors a resource-specific reconciliation contract that overlaps it. The standard requires that status writes be conditioned on the observed generation/resource-version and that controllers "own distinct status fields," whereas this reconciler conditions its write only on string equality of the current status and has no generation/version guard (the GatewayNetwork model exposes none). Maintainers should decide whether this PR's spec should reference and conform to #200's standard, and whether generation-guarded status writes are required here - this is a design decision, not a file conflict.

Findings Summary (ordered by severity, highest first):

1. [Minor] status parameter shadows the imported grpc/status package - Style (L2287)
2. [Minor] Unreachable conn == nil branch returns Valid without a hub check and has a misleading comment - Clarity (L2177-2180, L2269-2270)

Convention Checklist (omit conventions not applicable to the diff):

Convention	Result
No panic() in production code	Pass
Errors wrapped with fmt.Errorf context	Pass
errors/codes.NotFound handled	Pass
No secrets in logs or responses	Pass
Reconcile (not create-or-skip); idempotent status write-back	Pass
Status updated on error paths (span records error; transient surfaces error)	Pass
Context propagation (no context.TODO())	Pass
Never silently swallow partial failures	Pass
Test diff scrutiny (all additive; no flipped assertions)	Pass
Conventional commit message	Pass

[jsell-rh]

Minor: the parameter name status shadows the imported google.golang.org/grpc/status package. It compiles here because the package isn't referenced inside updateStatus, but it's a readability/lint trap. Consider renaming the parameter (e.g. desired).

[jsell-rh]

Minor: when r.gateways == nil this returns Valid for a network with a configured hub without verifying the hub exists. conn is the API-server gRPC connection (built at main.go:92, fatal on dial error), not the Kubernetes client, so this branch is effectively unreachable in production and the constructor comment ("controller runs without a Kubernetes client") is misleading. Consider dropping the dead branch or correcting the rationale.

[jsell-rh]

Amber review

Status: Stopped

The pull request head changed before Amber posted the review. A later job can review the new head.

[jsell-rh]

Amber review

Status: Complete

View the submitted review.

[jsell-rh]

Verdict

This is a clean, well-scoped replacement of the no-op GatewayNetworkReconciler with a real validation + deterministic status write-back reconciler, backed by a spec and a thorough table of unit tests covering every scenario. The implementation follows HyperShell conventions (error wrapping, gRPC not-found handling, idempotent write-back, no panics); I have only minor observability notes plus cross-PR coordination that needs a maintainer decision.

Amber Assessment

The change validates topology vocabulary, topology/hub coherence, and hub-reference existence, then writes Valid / Invalid: <reason> back idempotently. Transient hub-lookup and status-write failures are surfaced as errors rather than settled to a misleading Invalid, and delete / nil-resource events are terminal no-ops. Error wrapping uses fmt.Errorf("...: %w", err), not-found is discriminated via status.Code(err) == codes.NotFound, and the constructor tolerates a nil connection for tests. Good work.

Findings

[Minor] Reconcile span context is discarded even though this reconciler now makes gRPC calls - Observability (reconciler.go:2203)

_, endSpan := cpotel.StartReconcileSpan(...) drops the returned span context. The sibling reconcilers that actually issue gRPC calls (Gateway at reconciler.go:1261, ManagedDatabase at reconciler.go:151) capture it as ctx, endSpan := ... so downstream calls are children of the reconcile span. Since this reconciler now calls GetGateway and UpdateGatewayNetwork, those calls will not be nested under the reconcile span, weakening trace correlation. Capture the span context and pass it to the gRPC calls. Confidence: High.

[Minor] r.gateways == nil collapses a hub-configured network to Valid without any check - Robustness (reconciler.go:2274)

When a hub_gateway_id is set but the gateway client is nil, validate returns networkStatusValid and skips the existence check. This is documented as a test-only path and is safe in production (the constructor always wires a client when conn != nil), but the same nil-guard also silently makes updateStatus a no-op, so a mis-wired process would report success while performing no validation or write-back. Consider logging a WARN when the clients are nil in a non-test path, or asserting the invariant at construction. Confidence: Medium.

Test Diff Scrutiny

gateway_network_test.go is entirely new; no pre-existing test assertions were modified or weakened. Coverage maps 1:1 to the spec scenarios (valid/invalid topology, missing hub, dangling hub, idempotent no-write, delete/nil no-op, transient-error surfacing). No concerns.

Cross-PR coordination

• #207 (reconcile-to-request trace correlation): This PR adds a new cpotel.StartReconcileSpan(ctx, "GatewayNetwork", event.Type.String()) call site (3 args), while #207 changes that function's signature to StartReconcileSpan(ctx, kind, eventType, traceparent string) and updates all existing call sites to pass a traceparent (event-driven reconcilers pass the resource's traceparent, continuous ones pass ""). Whichever merges second will not compile against the other unless the new GatewayNetwork call site is updated. Maintainers should decide merge order and ensure this event-driven network reconcile passes the network's traceparent so it participates in span-link correlation rather than being silently omitted.

• #235 (GatewayRelease reconciliation): This PR's spec anchors a correctness claim on the sibling release reconciler being "inline and log-only (there is no reconcile queue for networks, matching the sibling release reconciler) and does not replay state on reconnect," so a surfaced transient error re-converges only when the network is next mutated. #235 changes exactly that premise: it routes the GatewayRelease watch through a reconcileQueue with per-release serialization and retry, precisely so a transient status-write failure is retried instead of relying on the next mutation. Once #235 merges, the "matching the sibling release reconciler" assumption is false, and a design decision is required: should GatewayNetwork also run through the shared retry queue (giving transient failures automatic retry), or intentionally stay inline? The two sibling reconcilers by the same author currently diverge on this, and the spec wording needs reconciling.

• #200 (control-plane reconciliation contract) and #185 (periodic world synchronization): Both establish a shared contract that reconcilers provide bounded retries and periodic resync of desired state (revision-aware queues / periodic world sync). This PR deliberately does the opposite: it is inline, does not retry, and its spec asserts the network watch "does not replay state on reconnect, so a surfaced error re-converges only when the network is next mutated, not automatically." If either contract lands and applies to GatewayNetwork, this reconciler's error-handling design and that explicit no-replay assumption become non-conformant. A maintainer decision is needed on whether GatewayNetwork adopts the shared driver (serialization/retry/resync) or is explicitly carved out.

Findings Summary (ordered by severity, highest first)

1. [Minor] Reconcile span context discarded; downstream gRPC calls not nested under the reconcile span - Observability (L2203)
2. [Minor] Nil gateway/network clients silently collapse a hub-configured network to Valid and no-op the write-back - Robustness (L2274)

Convention Checklist

Convention	Result
No panic() in production code	Pass
Errors wrapped with fmt.Errorf context	Pass
gRPC not-found handled (codes.NotFound)	Pass
No secrets in logs or responses	Pass
Input validated (topology vocabulary, hub reference)	Pass
Reconcile pattern (idempotent update, not create-or-skip)	Pass
Status updated on error paths (transient surfaced, not settled)	Pass
Context propagation (no context.TODO())	Pass
Span context propagated to downstream calls	Fail
Conventional commit messages	Pass
Test diff scrutiny (no weakened assertions)	Pass

[jsell-rh]

The span context returned by StartReconcileSpan is discarded (_, endSpan). Sibling reconcilers that make gRPC calls capture it (ctx, endSpan := ..., see the Gateway and ManagedDatabase call sites) so the calls nest under the reconcile span. This reconciler now issues GetGateway and UpdateGatewayNetwork; capture the span context and pass it to those calls so trace correlation isn't lost. (Note: #207 also changes this function's signature to take a traceparent and updates every call site - coordinate merge order so this new call site is updated too.)

[jsell-rh]

When hub_gateway_id is set but r.gateways == nil, validation short-circuits to Valid and (via the matching guard in updateStatus) the write-back becomes a no-op. This is safe for the documented test path, but a mis-wired production process would report a successful Valid reconcile while performing no validation and no status write. Consider a WARN log on the nil-client path in production, or asserting the client invariant at construction.