Repository navigation
[HYPERSHELL-177] Implement GatewayNetwork reconciliation - #246
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
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
NotFoundfor the hub settles the network toInvalid, 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() != desiredStatusguard 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:
UpdateGatewayNetworkonly sends theStatusfield, 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, andendSpan(reconcileErr)(an improvement over the sibling'sendSpan(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
- [Minor]
updateStatus(ctx, id, status string)shadows the importedgoogle.golang.org/grpc/statuspackage with the parameter namestatus. It compiles because the package isn't referenced inside that function, but it's a readability/lint trap - Style (reconciler.go L2287). - [Minor] The
conn == nildegraded path returnsnetworkStatusValidfor 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."connis the API-server gRPC connection (created atmain.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.mdcross-linksgateway-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 samecontrol-plane.spec.mdsub-spec table and edit the same adjacent constructor lines inmain.goand the same region ofreconciler.go. (b) More substantively, the two sibling reconcilers introduce divergent "healthy" status vocabularies for the same reconciliation contract -Availablefor a release vsValidfor a network (both shareInvalid). 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.mdas 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 (theGatewayNetworkmodel 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):
- [Minor]
statusparameter shadows the importedgrpc/statuspackage - Style (L2287) - [Minor] Unreachable
conn == nilbranch returnsValidwithout 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 |
|
|
||
| // updateStatus writes the network's reconciled status back to the API server. It | ||
| // is a no-op when the network client is not configured. | ||
| func (r *GatewayNetworkReconciler) updateStatus(ctx context.Context, id, status string) error { |
There was a problem hiding this comment.
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).
| if hubID != "" { | ||
| // A configured hub must reference an existing Gateway. Skip the lookup when | ||
| // no gateway client is configured (controller without a Kubernetes client). | ||
| if r.gateways == nil { |
There was a problem hiding this comment.
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.
Amber reviewStatus: Stopped The pull request head changed before Amber posted the review. A later job can review the new head. |
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
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 toStartReconcileSpan(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
reconcileQueuewith 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)
- [Minor] Reconcile span context discarded; downstream gRPC calls not nested under the reconcile span - Observability (L2203)
- [Minor] Nil gateway/network clients silently collapse a hub-configured network to
Validand 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 |
| @@ -2165,8 +2201,102 @@ func (r *GatewayNetworkReconciler) Handle(ctx context.Context, event watcher.Eve | |||
| }() | |||
|
|
|||
| _, endSpan := cpotel.StartReconcileSpan(ctx, "GatewayNetwork", event.Type.String()) | |||
There was a problem hiding this comment.
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.)
| // no gateway client is configured (started without an API-server gRPC | ||
| // connection, e.g. in unit tests). | ||
| if r.gateways == nil { | ||
| return networkStatusValid, nil |
There was a problem hiding this comment.
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.
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
Carried-forward assessment: COMMENT.
This entry carries forward the existing Amber review of commit ee89948a75e76b2e7c3fb5acf44516c7dfc9c77e.
Original Amber review by @jsell-rh. The original review contains the findings and inline comments.
No new analysis was performed for this migration entry.
Amber reviewStatus: Complete |
Implements the GatewayNetwork reconciler in the control plane, replacing the prior no-op with a deterministic reconcile contract. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ee89948 to
4d6d5ca
Compare
Amber reviewStatus: Complete |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
COMMENT. This is a clean, well-tested reconciler that faithfully implements its companion spec, mirrors the established GatewayReleaseReconciler pattern, and correctly treats a GatewayNetwork as a resource with no Kubernetes footprint. The findings below are minor; the one item that genuinely needs maintainer attention is a cross-PR design-assumption conflict about periodic resync (see Cross-PR coordination).
Amber here. The implementation validates topology vocabulary, topology/hub coherence, and hub-reference existence, then writes a deterministic, idempotent status back to the API server; error paths surface transient failures instead of swallowing them, and the unit tests cover every spec scenario. I confirmed the API server's UpdateGatewayNetwork handler is a read-modify-Replace merge that applies only non-nil fields, so writing back {Id, Status} alone does not clobber topology/tunnel_mode/hub_gateway_id - no data-loss risk there.
Findings
[Minor] Vestigial per-network active-set guard returns nil (success) on a concurrent skip - reconciler.go Handle (around L2466-L2477) keeps the stub's active map guard: if a second event for the same network arrives while one is in flight, it returns nil without reconciling. The sibling GatewayReleaseReconciler deliberately removed this exact guard (see its comment at L1232-L1235) precisely because returning nil on a spurious skip masks a dropped reconcile. Under today's inline, single-threaded WatchGatewayNetworks loop this can never trigger (events are processed serially, and the defer clears the entry before the next Recv), so it is currently harmless. But it is dead, inconsistent with the sibling, and a latent footgun: because the network watch has no reconcile queue, if dispatch ever became concurrent a skipped event would be silently lost with nothing to retry it. Recommend dropping the guard to match the release reconciler, or documenting why networks keep it.
[Minor] Transient failures never auto-reconverge - a transient hub lookup or status-write failure is correctly surfaced as a returned error (good, not swallowed), but WatchGatewayNetworks only logs handler errors (watcher.go:840) and there is no initial list or periodic resync for networks, so a network left in a transient-error state stays stale until it is next mutated. The spec documents this as intentional and matching the sibling reconciler, so this is not a blocker - but it is the crux of the cross-PR item below.
Cross-PR coordination
This PR bakes a load-bearing design assumption into gateway-network-reconciliation.spec.md and the code: the network watch is "inline and log-only ... does not replay state on reconnect, so a surfaced error re-converges only when the network is next mutated, not automatically." A separate open pull request introduces a canonical control-plane reconciliation contract stating that all reconcilers SHALL recover work through an initial resource list and periodic resync and that a missed event SHALL NOT permanently prevent convergence. These two positions are in direct conflict: as written, this reconciler would be non-conformant to that contract the moment it lands. Maintainers need to decide (a) whether GatewayNetwork must adopt the shared initial-list + periodic-resync behavior or be explicitly exempted, and (b) the merge order, so this spec's "no replay / re-converge only on next mutation" wording does not contradict the incoming contract. The affected pull request is #200.
A second open pull request that specifies periodic control-plane world synchronization (initial recovery, paginated inventory, revision-aware queues, bounded retry) rests on the same resync premise and would likewise supersede this PR's "does not replay state on reconnect" assumption if it generalizes to networks; the same decision and ordering call is needed with #185.
Findings Summary (ordered by severity, highest first)
- [Minor] Vestigial active-set guard returns
nilon concurrent skip; inconsistent with sibling reconciler and a latent lost-event footgun - Reconciliation Pattern (reconciler.go ~L2466-L2477) - [Minor] Transient-error state does not auto-reconverge (no initial list/resync); documented as intentional but conflicts with an incoming reconciliation contract - Robustness / Cross-PR (reconciler.go L2503-L2507)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf("...: %w", err) |
Pass |
NotFound handled as deterministic outcome (not error) |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (topology vocabulary, hub reference) | Pass |
| Reconcile / idempotent status write-back (not create-or-skip) | Pass |
| Status updated / errors surfaced on failure paths | Pass |
| Proper context propagation (gRPC stream ctx) | Pass |
| Tests cover spec scenarios | Pass |
| Conventional commit message | Pass |
| No em dashes | Pass |
| if retryErr != nil { | ||
| reconcileErr = fmt.Errorf("validate gateway network %s: %w", event.ResourceID, retryErr) | ||
| return reconcileErr | ||
| } |
There was a problem hiding this comment.
[Minor] Transient failures are correctly surfaced here instead of being swallowed or settled to a misleading Invalid - good. The caveat: WatchGatewayNetworks only logs this returned error (watcher.go:840), and there is no initial list or periodic resync for networks, so a network stuck on a transient hub-lookup or status-write failure stays stale until it is next mutated. The spec documents this as intentional, but it is the exact assumption a separate open PR's reconciliation contract would override (see the Cross-PR coordination section).
| // no gateway client is configured (started without an API-server gRPC | ||
| // connection, e.g. in unit tests). | ||
| if r.gateways == nil { | ||
| return networkStatusValid, nil |
There was a problem hiding this comment.
[Info] When r.gateways == nil a hub-referencing network settles to Valid without the dangling-hub check. This is safe today only because gateways and networks are derived from the same conn, so a nil gateway client implies a nil network client and the (possibly false) Valid is never persisted. If the two clients ever diverge, this branch could persist a Valid status for a network with a dangling hub. Worth a short comment noting the coupling.
3 participants
What
Replaces the no-op
GatewayNetworkReconcilerstub in the control plane with a real reconciler, and adds the spec that defines its contract.A
GatewayNetworkhas no direct Kubernetes footprint in this scope (like aGatewayRelease). Reconciling one therefore means:topologyis a recognized value (mesh,hub-spoke);hub-spokenetwork designates ahub_gateway_id;hub_gateway_id(any topology) references an existing Gateway.status(Valid, orInvalid: <reason>), idempotently (no write when the persisted status already matches).Behavior details:
Invalid; a transient hub-lookup or status-write failure surfaces as a returned error rather than silent success (re-converges on watch reconnect).Mirrors the sibling
GatewayReleaseReconcilerpattern (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 fromcontrol-plane.spec.mdcomponents/control-plane/internal/reconciler/reconciler.go— realGatewayNetworkReconcilercomponents/control-plane/cmd/hypershell-controller/main.go— pass gRPC conn to the constructorcomponents/control-plane/internal/reconciler/gateway_network_test.go(new) — unit tests covering every spec scenarioTesting
go build ./...,go vet ./...cleanTestGatewayNetwork_*cases)make checkgreen🤖 Generated with Claude Code