Repository navigation
[HYPERSHELL-110] feat: per-gateway Gateway Console - #147
Conversation
Add openshell-gateway-console.spec.md defining Option 1: co-deploy the
OpenShell dashboard per gateway, fronted by an oauth2-proxy sidecar, and
provisioned automatically whenever the gateway has routing enabled.
Key decisions captured:
- Dedicated confidential per-gateway Keycloak console client
({name}-{id}-console) with audience + client-role mappers targeting the
gateway client, so browser tokens carry aud={name}-{id} and
hypershell.roles; fullScopeAllowed=false preserves isolation.
- oauth2-proxy runs as a sidecar (upstream=localhost); control plane fetches
the client secret from Keycloak and stores it with a generated cookie
secret in a per-gateway Secret.
- HTTP exposure via a Gateway API HTTPRoute at
console-<ns>.<base-domain> on the shared Gateway's wildcard cert.
- Authorization enforced at the gateway via the existing OIDC Role Bridge.
- Covers lifecycle/cleanup, atomicity, NetworkPolicies, RBAC, data-model
(read-only consoleAddress), and shared-Gateway HTTP-listener prerequisite.
Link the new sub-spec from openshell-gateway.spec.md.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rewrite openshell-gateway-console.spec.md with short active-voice
sentences, "must"/"must not" for obligation, and one consistent subject
("the reconciler").
Remove redundant prose: drop the ASCII topology and the Architecture and
Security Considerations sections (all restated the requirements), and cut
every scenario that only echoed its requirement. Spec drops from ~640 to
380 lines with no loss of testable requirements.
Simplify the parent sub-spec index line to match.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
4ad2426 to
967f372
Compare
Clarify that the reconciler provisions the confidential console client in the same reconciliation pass as the gateway CLI client, reusing the same Keycloak Admin access. Correct the dashboard runtime contract against the upstream project: - OPENSHELL_GATEWAY_URL is host:port with no scheme; TLS is enabled by the presence of GATEWAY_CA_CERT (was grpcs://...). - Add PORT and ADMIN_ROLE=openshell-admin. - Dashboard binds all interfaces on 8000 so the kubelet can probe it; the Service and NetworkPolicies keep 8000 unreachable from outside the pod (a loopback-only listener would break the readiness probe). - Readiness probe: TCP on 8000 when no health path exists. Prerequisites: upstream publishes no registry image, so the platform must build openshell-dashboard from source and load/push it, then pin the digest. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: openshift-online/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Add a read-only console_address field to the Gateway kind, mirroring route_address. The control plane sets it to the per-gateway console URL when it deploys the OpenShell dashboard; the management web-console and CLI use it to link to the console. It is absent from all create paths and cannot be set or updated by a user. - migration adds a nullable console_address column - model, presenter, and handler expose it on the REST API (patch-only) - proto and gRPC handler expose it on UpdateGatewayRequest (field 20) and Gateway (field 21) - regenerated openapi client, gRPC stubs, and Go/TypeScript SDK types Part of HYPERSHELL-3 (OpenShell Gateway Console). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add ProvisionConsoleClient/DeleteConsoleClient to the Keycloak client for
the per-gateway console. The console client ({name}-{id}-console) is
confidential (oauth2-proxy needs a client secret) with PKCE S256 and
fullScopeAllowed=false to keep per-gateway isolation. Its three protocol
mappers (audience, client-roles, sub) target the gateway CLI client
({name}-{id}), so browser tokens carry aud={name}-{id} and
hypershell.roles and the gateway validates them like CLI tokens.
ProvisionConsoleClient rolls the client back if mapper creation fails, so
a failed cycle leaves no partial client. GetConsoleClientSecret returns
the secret on the already-exists reconcile path.
Part of HYPERSHELL-3 (OpenShell Gateway Console).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Deploy the OpenShell dashboard and an oauth2-proxy sidecar with each routed gateway. The console follows the route lifecycle: it is reconciled in the Gateway API pass (route enabled + Gateway API available) and torn down when the route is removed or the gateway is deleted. reconcileConsole (console.go) provisions the confidential console client, stores its secret plus a stable cookie-secret in the openshell-console-oauth2 Secret, and reconciles the Deployment, Service, HTTPRoute, and two NetworkPolicies as update-or-create unstructured resources. The dashboard binds all interfaces on 8000 for the kubelet probe while the Service and NetworkPolicies keep 8000 unreachable, so only the in-pod oauth2-proxy reaches it. Missing prerequisites (Keycloak, base domain) warn and skip without failing the gateway reconcile; the console URL is published to the gateway's console_address, and cleared on teardown. - config.go: DefaultConsoleImage/DefaultOAuth2ProxyImage on ImageDefaults; ConsoleAddressUpdater and UpdateConsoleAddress on ReconcileOpts; DeleteConsoleClient on KeycloakClientAPI - reconciler.go: wire console reconcile/cleanup, add httproutes to the Gateway API sweep and kindToResource, delete the console client on gateway delete - reconciler/reconciler.go: makeConsoleAddressUpdater publishes console_address via gRPC Part of HYPERSHELL-3 (OpenShell Gateway Console). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Deploy-side wiring for the per-gateway OpenShell console: - controller-rbac: grant the controller httproutes (create/update/delete) so it can reconcile the console HTTPRoute. - kind/gateway.yaml: broaden the grpc listener (*.gw.localhost) to accept HTTPRoute alongside GRPCRoute. The console host console-<ns>.gw.localhost falls under the same wildcard cert, so it reuses this listener instead of a conflicting second HTTPS listener on the same host/port. - kind/kustomization.yaml: set GATEWAY_API_HTTP_LISTENER_NAME=grpc so console HTTPRoutes attach to that listener, and point the controller at the locally built console/oauth2-proxy images (IfNotPresent). - build-console-image.sh + `make build-console`: build the upstream dashboard image from source at a pinned ref and load it, plus oauth2-proxy, into Kind. Upstream publishes no registry image, so the platform builds it locally. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The per-gateway console's oauth2-proxy sidecar crash-looped on startup, leaving the console Deployment at 1/2 Ready and no working login, for two independent reasons found while verifying HYPERSHELL-3 end-to-end on Kind: 1. OIDC discovery failed with x509 "unknown authority" whenever the issuer is served with a privately-signed certificate. The gateway pod already solves this via the gateway-trusted-ca ConfigMap the control plane copies into each tenant namespace. Extend the same config-driven mechanism to the sidecar: when that ConfigMap is present, mount the bundle and set OAUTH2_PROXY_PROVIDER_CA_FILES. In production the issuer is publicly trusted, the ConfigMap is absent, and the sidecar uses the system trust store -- so no code path is dev-specific. 2. The cookie secret was standard-base64 (44 chars, +/ alphabet, = padding). oauth2-proxy strips padding, URL-base64-decodes, and requires 16/24/32 decoded bytes; the standard-base64 value fails that decode and is used verbatim as 44 bytes, which it rejects for AES. Generate it with RawURLEncoding of 32 bytes (43 chars, URL-safe, no padding). Verified on Kind: routed gateway -> console pod 2/2 Running, the console URL 302-redirects to Keycloak /authorize with the per-gateway confidential client (PKCE S256), and console_address is published on the gateway record. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
reconcileConsoleSecret captured the Secrets().Get result in err, then reused err for generateConsoleCookieSecret(). On a brand-new namespace the Get returns NotFound, but generating the cookie cleared err to nil, so the code fell through to the Update path and failed with "secrets openshell-console-oauth2 not found" -- aborting the whole console reconcile so no console was ever deployed for newly created routed gateways. Capture the not-found state before err is reused, and branch create vs update on that. Accept kubernetes.Interface so the path is unit-testable with a fake clientset; add regression tests for the create-when-absent and preserve-cookie-on-update paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The per-gateway OpenShell console clients request the gateway-roles scope
(OAUTH2_PROXY_SCOPE) and the control plane lists it in the console client's
defaultClientScopes, but the dev realm never defined it. Keycloak rejected the
login with "invalid_scope: Invalid scopes: ... gateway-roles" before the
console could load.
Add the gateway-roles client scope with an oidc-usermodel-client-role-mapper
that emits client roles under resource_access.${client_id}.roles, matching
openshell-gateway-keycloak.spec.md which treats this scope as a realm
prerequisite that must exist before gateways are created.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
toGatewayRecord never mapped the SDK's console_address field onto GatewayRecord.consoleUrl, so the "Open console" button and row action -- both gated on consoleUrl -- never rendered even when the API returned a console address. Map it through and cover both the present and absent cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The dashboard reached the gateway with a schemeless OPENSHELL_GATEWAY_URL and
only a server CA, so its gRPC client dialed h2c cleartext into the gateway's
TLS listener (connection reset while reading the server preface) and, even over
TLS, presented no client certificate for the gateway's mutually-authenticated
admin API. Every dashboard->gateway call failed with 502 Unavailable
("openshell gateway is unreachable").
Use the grpcs:// scheme and mount the per-namespace openshell-client cert/key
(openshell-client-tls, already provisioned by the cert-manager reconcile),
wiring GATEWAY_CLIENT_CERT/GATEWAY_CLIENT_KEY alongside GATEWAY_CA_CERT.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…oles The gateway admin API enforces openshell-admin and openshell-user independently (admin does not imply user), so a gateway owner granted only openshell-admin could read gateway info but was refused "list workspaces" with "role 'openshell-user' required". Map gateway:owner to both openshell-admin and openshell-user, and loop the assign/remove paths over the role set. gateway:viewer still maps to openshell-user alone. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
With fullScopeAllowed=false (required for per-gateway isolation), Keycloak filters every role mapper's output to the requesting client's scope. The per-gateway console client had the client-role mapper wired to hypershell.roles but was never granted scope for the gateway client's openshell-admin/openshell-user roles, so Keycloak stripped them from the access token. The dashboard then presented a token with no hypershell.roles and the gateway denied every call with "role 'openshell-user' required" (the aud claim, which is not scope-filtered, was present, masking the cause). Add client scope-mappings granting the console client the gateway client's roles during provisioning, and reconcile them on the already-exists path so consoles provisioned before this grant are healed. Only this one gateway client's roles are granted, so fullScopeAllowed stays false and per-gateway isolation is preserved. Document the required scope mapping in the console spec. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The console address is published while the gateway is still provisioning, before its route is programmed and the connection command surfaces. The console button gated only on consoleUrl, so it appeared seconds before the one-time setup commands. Gate the button on isGatewayReadyToConnect (phase running + endpoint) -- the same predicate the connection command uses -- so both surface together. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A routed gateway parked at Provisioning after its Deployment became ready and relied on the health reconciler's next tick to promote it to Running. That loop runs every 30s, so the connection command and console button could stay hidden for up to ~30s after the pods (and route) were actually ready. Add a bounded route-readiness poll to the provisioning path: observe the exposure every 2s for up to 90s and promote straight to Running once the route is programmed. On timeout, park at Provisioning as before and let the 30s health reconciler continue enforcing the full route-readiness grace window. Steady-state health cadence is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
client-go's workqueue marks a key dirty when Add is called while it is being processed, and re-queues it for immediate handling on Done. The gateway reconciler writes phase=Provisioning before it does its work, and that self-write emits a watch event that re-enqueues the key mid-reconcile. On a failing reconcile that turned AddRateLimited's backoff into a no-op: Done re-queued the key immediately, it was re-handled with no delay, it wrote Provisioning again, and the loop spun -- re-hammering the API server and Keycloak and sustaining its own self-event storm. Gate handling on a per-key backoff floor. On failure, record now+delay (the limiter's capped-exponential When) and schedule the retry with AddAfter; while a key is within that floor, processNext re-defers it with a cheap Get/AddAfter/Done cycle instead of invoking the handler. Dirty re-adds thus cost nothing but a requeue and the backoff cadence is honored, so a persistently failing gateway retries at the intended interval rather than spinning. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The teardown verification trusted a fixed wall-clock window: five minutes after a clean teardown it stopped probing for resurrected route/console resources. But a stale provisioning pass creates the GRPCRoute before its TLS-secret wait and fail-closed route-intent re-check, so elapsed time is not proof the resurrection race has drained -- a late pass can recreate resources after any window expires, and the health loop would never notice. Replace the window with an indefinite, low-frequency re-check: a settled (torn-down, addressless) gateway is re-verified at most once per routeVerifyInterval, forever. Between checks the completion marker is trusted, so steady-state cost stays at one cheap absence probe per interval per gateway (not per tick) at fleet scale, while a resurrected resource is always eventually caught rather than hidden by a wall-clock guess. Addresses Amber review 4978613478 finding 3 (health.go:44). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addressed Amber review 4978613478 (3 Majors)The three findings shared one root cause: the fire-and-forget requeuer replayed the frozen original event on a goroutine that could race the reconciler, with a finite attempt budget. Rather than patch each symptom, I replaced it with the idiomatic controller pattern — a per-resource rate-limiting workqueue — which dissolves all three at the source. Finding 1 — nil-result cancel (watcher.go:185) → Finding 2 — finite retry budget (requeue.go:132) → Finding 3 — wall-clock verify window (health.go:44) → Also fixed (two follow-on hazards the workqueue introduced)
Per-resource serialization, coalescing, indefinite retry, backoff preservation, and the phase-clear transform are covered by unit tests ( |
The reconcile queue's tests drive backoff with a short-delay rate limiter and real time, so the withClock test option was never used and tripped the unused linter in CI. Remove it; the now field still defaults to time.Now. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jsell-rh
left a comment
There was a problem hiding this comment.
The per-key workqueue closes the three findings from the prior review: reconciliation is serialized and coalesced, retries retain capped backoff indefinitely, and teardown absence continues to be verified at low frequency without a wall-clock cutoff; the latest unused-option cleanup is behavior-neutral. The recovery path is still process-local, however, so a controller restart can permanently discard the only retry for a gateway already persisted in a phase that suppresses normal reconciliation.
Major
components/control-plane/internal/watcher/watcher.go:159creates an empty in-memory queue and subscribes only to future broker events. If the controller restarts afterProvisioningwas persisted but before a failed reconcile completes, the queued retry disappears;WatchGatewaysemits no initial snapshot, an ordinaryProvisioningevent would be stopped by the phase gate anyway, and the health loop leaves missing provisioning work untouched. Seed recoverable gateways from current API state on startup/reconnect using a forced retry for active phases, or persist the reconciliation intent outside the process. Confidence: High (99%).
Assessment
REQUEST_CHANGES (submitted as COMMENT because this is an author self-review).
Findings Summary (ordered by severity, highest first):
- [Major] Controller restart discards the only retry for persisted Provisioning gateways - Reconciliation / Durability (L159)
Convention Checklist (omit conventions not applicable to the diff):
| Convention | Result |
|---|---|
| Reconciliation work is serialized per resource | Pass |
| Retry backoff survives dirty-key re-adds | Pass |
| Latest desired state is coalesced for retries | Pass |
| Retry intent survives controller restart | Fail |
| Cleanup verification remains convergence-based | Pass |
| Proper context propagation | Pass |
| No secrets in logs or responses | Pass |
| Conventional commit messages | Pass |
Opening a gRPC server-streaming RPC is not a subscription handshake: the client's WatchGateways call can return before the server registers its broker subscription. A client that lists to seed its state right after the call can therefore snapshot state and then miss an event that fires in the gap before the subscription goes live. Flush the stream header once the subscription is live so a client that blocks on the response header knows no event can be missed after Header() returns, and can safely LIST without a list-watch gap. Sending an empty header is a no-op for clients that ignore it. Make the watch integration tests wait on Header() instead of a fixed sleep, which is both deterministic and asserts the handshake fires. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…WATCH The gateway watch stream sends only future events and never replays state on (re)connect, so the reconcile queue's in-process retries are lost on a controller restart. A gateway persisted at Provisioning when the controller died -- before its workload was created or a terminal phase recorded -- was never re-enqueued and stayed stranded until its spec next changed. Add the LIST half of the standard controller LIST-then-WATCH pattern: - Seed the queue from the current inventory on every (re)connect. Gateways in a gate-suppressed active phase (Provisioning, Degraded) are seeded with the phase cleared so the recovery reconcile runs instead of being skipped by the phase gate; Running gateways are seeded verbatim (they no-op at the gate, no re-provision flap); unphased/Failed reconcile normally. This is a one-shot seed per connect, not a periodic forced resync, so the health reconciler that legitimately owns active phases is not trampled. - Wait for the stream subscription header before seeding, so the LIST cannot race ahead of the server subscription and miss an event (paired with the api-server header flush). - Coalesce version-aware (on updated_at): a stale seed snapshot never clobbers a newer live event, nor a buffered out-of-order live event a newer seed. A pending (unprocessed) delete stays terminal so a stale non-delete snapshot cannot resurrect a resource mid-teardown. - Treat the list as authoritative for absence, but only after confirmation. Offset pagination is not a consistent snapshot: a concurrent create/delete shifts offsets, so a still-live gateway can slide across a page boundary and be omitted from the union of pages. Pruning on list-absence alone would cancel that live gateway's only retry, so each omitted tracked id is confirmed with a point GetGateway and pruned only on NotFound; any other outcome keeps the retry. The NamespaceGCReconciler, which rechecks liveness before deleting, handles orphaned-namespace cleanup -- safer than synthesizing a delete from an inconsistent list. - Scope each watch attempt to its own cancelable context so the RPC and its server-side subscription are torn down before reconnecting, instead of leaking one subscription per reconnect when connectAndRecv returns while the stream is still healthy (e.g. a post-header seed failure). The reconcile queue runs on the parent context, so pending retries survive. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Restart-recovery for gateway reconciliation (review 4978760953)Pushed The gateway watch stream only replays future events, so an in-process retry was lost on a controller restart — a gateway persisted at Fix — the LIST half of the standard controller LIST-then-WATCH pattern, plus the correctness subtleties a naive seed would miss:
Also resolved the outstanding earlier Major (review 4978490596): Verification: |
jsell-rh
left a comment
There was a problem hiding this comment.
The subscription header, per-attempt cancellation, version-aware queue, and reconnect seed materially improve recovery, and the ordinary restart case now re-drives persisted work. Two ordering gaps can still lose the only forced recovery: offset pagination can omit a gateway unknown to a fresh process, and an equal-version live event can erase the seed-only phase bypass.
Major
components/control-plane/internal/watcher/watcher.go:304treats one offset-paginated pass as a complete startup inventory even though this function correctly notes that those pages are not a consistent snapshot. On a fresh controller the queue has no prior key for a gateway skipped when a concurrent delete shifts later rows across a page boundary; the delete event is buffered, but the stable skipped gateway emits no event and therefore never reaches the point-read fallback. A persistedProvisioninggateway can still remain undiscovered and stranded indefinitely. Use a snapshot/cursor inventory or repeat full passes until the ID set is stable before draining the subscribed stream. Confidence: High (99%).components/control-plane/internal/watcher/requeue.go:180drops only strictly older payloads, but the forced seed is a phase-cleared clone with the sameupdated_atas the source Gateway. If that same-version live event was buffered during the LIST, it is accepted later and overwrites the cleared-phase payload; the first queue attempt hasNumRequeues == 0, so no retry transform restores the bypass, the reconciler phase-gates the event, andForgetdiscards recovery. Track a sticky per-key force/recovery bit and coalesce it independently of the latest payload until an actual handler attempt consumes it. Confidence: High (99%).
Assessment
REQUEST_CHANGES (submitted as COMMENT because this is an author self-review).
Findings Summary (ordered by severity, highest first):
- [Major] Offset pagination can omit a gateway unknown to the fresh recovery queue - Reconciliation / Durability (L304)
- [Major] Equal-version live events can erase the forced phase-gate bypass - Reconciliation / Concurrency (L180)
Convention Checklist (omit conventions not applicable to the diff):
| Convention | Result |
|---|---|
| Watch subscription established before inventory seed | Pass |
| Startup inventory is complete under concurrent mutation | Fail |
| Forced recovery survives payload coalescing | Fail |
| Pending deletes remain terminal | Pass |
| Missed-delete retries require point-read confirmation before pruning | Pass |
| Stream attempts cancel their broker subscriptions | Pass |
| Proper context propagation | Pass |
| No secrets in logs or responses | Pass |
| Conventional commit messages | Pass |
| // Stop on the authoritative Total, or a short/empty page (defensive, so a | ||
| // misreported Total cannot spin forever) -- mirrors listAllGateways. | ||
| total := int(resp.GetMetadata().GetTotal()) | ||
| if len(items) == 0 || len(items) < gatewaySeedPageSize || (total > 0 && seeded >= total) { |
There was a problem hiding this comment.
[Major] Do not treat this offset-paginated pass as a complete startup inventory. A fresh process has no prior queue key for a gateway that pagination skips. For example, a delete after page 1 shifts later rows left; page 2 can omit a stable Provisioning gateway while the collected count still reaches the new total or a short page. The subscribed stream buffers the delete, but it has no event for that skipped gateway, and the point-read fallback only examines keys already known to the queue. Use a consistent snapshot/cursor, or repeat full inventory passes until the ID set is stable before draining live events, so restart recovery cannot miss the very gateway it must discover.
Confidence: High (99%).
| // Version-aware coalescing: drop an event older than the pending payload | ||
| // so a stale snapshot never clobbers newer live state (and vice versa). | ||
| // The newer payload is already queued, so there is nothing to schedule. | ||
| if q.versionOf != nil && q.versionOf(ev) < q.versionOf(prev) { |
There was a problem hiding this comment.
[Major] Preserve forced recovery independently of payload version. The seed clears phase on a clone but retains the source Gateway's updated_at. If the corresponding live event was buffered during the LIST, this strict < comparison accepts its equal version and overwrites the phase-cleared seed. The first attempt still has NumRequeues == 0, so the retry transform does not run; the phase gate returns success and Forget removes recovery. Keep a sticky per-key force bit that is ORed across coalesced events and consumed only by a real handler attempt, rather than encoding the bypass solely in the mutable latest payload.
Confidence: High (99%).
jsell-rh
left a comment
There was a problem hiding this comment.
The subscription handshake closes the initial list-watch gap, but the client then stops reading the stream for the entire inventory seed. Because the API server EventBroker deliberately drops events for slow subscribers after its fixed 256-event buffer fills, a busy or slow seed can still permanently lose live changes.
Major
components/control-plane/internal/watcher/watcher.go:191performs all inventory passes before the firstRecv. The server can drain broker events into gRPC only until per-stream HTTP/2 flow control blocks; after that, the broker non-blockingly drops events once its 256-slot subscriber channel fills. Two stable ID-set passes do not detect update-only traffic, so a gateway updated after its row was read can retain a stale desired state indefinitely if that live event is dropped. Start draining the stream immediately after the header handshake, enqueueing events concurrently with the seed through the version-aware queue, and cancel/join that receiver if seeding fails. Confidence: High (99%).
Assessment
REQUEST_CHANGES (submitted as COMMENT because this is an author self-review).
Findings Summary (ordered by severity, highest first):
- [Major] Pausing
Recvduring inventory seeding can overflow the lossy watch broker - Reconciliation / Durability (L191)
Convention Checklist (omit conventions not applicable to the diff):
| Convention | Result |
|---|---|
| Watch subscription established before inventory seed | Pass |
| Live watch is drained while inventory seed runs | Fail |
| Event transport preserves mutations during seed | Fail |
| Version-aware seed/live coalescing | Pass |
| Stream attempts cancel their broker subscriptions | Pass |
| Proper context propagation | Pass |
| No secrets in logs or responses | Pass |
| Conventional commit messages | Pass |
…e, and concurrent stream drain Three correctness fixes to the LIST-then-WATCH gateway seed: 1. Stable inventory: repeat full list passes until two consecutive passes agree on the ID set, rather than trusting a single offset-paginated pass that can skip a still-live gateway across a page boundary. On a fresh process such a gateway has no tracked retry and emits no event, so a single-pass seed would strand it. If the inventory never stabilizes within 5 passes, return an error so watchLoop reconnects and retries. 2. Sticky force bit: track the phase-gate bypass as a per-key recovery mark independent of the payload, so an equal-version live event that overwrites the forced seed payload cannot erase the bypass before it is applied. The mark is consumed atomically with the payload snapshot under one lock, closing the window where a forced enqueue landing between snapshot and clear would be erased. 3. Concurrent stream drain: read the watch stream in a goroutine while seeding, so the API server's 256-slot per-subscriber event broker buffer stays drained. Without this, a seed that takes long enough for >256 events to fire loses them permanently (the broker drops, the stream never replays), stranding any gateway whose only spec change occurred during the seed window. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jsell-rh
left a comment
There was a problem hiding this comment.
The stable-pass inventory and equal-version sticky-force changes address the prior pagination and overwrite cases, and Recv now starts immediately after the subscription handshake so the broker buffer is drained. The new concurrency path still permits a dead-watch seed gap, can erase a pending delete, and mishandles force when versions advance, so recovery remains incomplete.
Major
components/control-plane/internal/watcher/watcher.go:221runs the entire seed before observingstreamErr. IfRecvfails or reaches EOF during a long list pass, mutations after that failure can fall between the dead subscription and the next reconnect while the stable ID set remains unchanged. Run receive and seed under coordinated cancellation, select on both results, and cancel and join the other operation as soon as either fails. Confidence: High (99%).components/control-plane/internal/watcher/watcher.go:355unconditionally prunes after a staleknownKeyssnapshot. The now-concurrent receiver can enqueue a delete after that snapshot but beforeGetGatewayreturns NotFound;prunethen removes the pending delete, and the queued worker finds no payload, skipping teardown. Make pruning conditional on the current queue entry still matching the non-delete snapshot, or retain/synthesize the delete. Confidence: High (99%).components/control-plane/internal/watcher/requeue.go:209returns before settingforcedwhen an older forced seed meets a newer live payload. If the newer payload is stillProvisioningorDegraded, its normal attempt phase-gates and succeeds, so the restart recovery is forgotten. Coalesce force against the retained latest payload under the same lock, preserving it whenever that latest state still requires active-phase recovery. Confidence: High (98%).
Minor
components/control-plane/internal/watcher/requeue.go:216never clears an accepted force mark when a newerRunningpayload replaces the active-phase seed. On a reconnect during an in-flight reconcile, the eventual Running event is therefore phase-cleared and provisioned again, despite the stated goal of avoiding a healthy re-provision flap. Recompute or clear the mark when a strictly newer payload proves forced recovery is no longer needed. Confidence: High (95%).
Assessment
REQUEST_CHANGES (submitted as COMMENT because this is an author self-review).
Findings Summary (ordered by severity, highest first):
- [Major] Stream failure during the seed reopens the list-watch loss window - Reconciliation / Concurrency (L221)
- [Major] Seed absence pruning can erase a concurrently queued delete - Reconciliation / Lifecycle (L355)
- [Major] A newer active live payload can cause the older forced seed to lose recovery - Reconciliation / Durability (L209)
- [Minor] A force mark can survive a newer Running payload and trigger redundant provisioning - Reconciliation / State (L216)
Convention Checklist (omit conventions not applicable to the diff):
| Convention | Result |
|---|---|
| Watch subscription established before inventory seed | Pass |
| Watch stream drained during inventory seed | Pass |
| Stream failure atomically aborts the seed | Fail |
| Startup inventory stabilizes across offset pagination | Pass |
| Forced recovery survives equal-version coalescing | Pass |
| Forced recovery follows newer payload state | Fail |
| Pending deletes remain terminal | Fail |
| Seed and receiver cancellation is joined | Fail |
| No secrets in logs or responses | Pass |
| Conventional commit messages | Pass |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jsell-rh
left a comment
There was a problem hiding this comment.
Commit 306bee9 closes the remaining gateway watch recovery races: receiver termination cancels and joins the seed, compare-and-prune preserves concurrent deletes, and terminal tombstones prevent a completed delete from being resurrected by stale inventory. Forced recovery now follows the retained payload across version coalescing and remains durable across health-written phases; the earlier Minor suggestion to clear force on Running is withdrawn because that phase does not prove GatewayReconciler.Handle succeeded.
Findings
No findings.
Assessment
APPROVE (submitted as COMMENT because this is an author self-review).
Findings Summary (ordered by severity, highest first):
No findings.
Convention Checklist (omit conventions not applicable to the diff):
| Convention | Result |
|---|---|
| Watch subscription established before inventory seed | Pass |
| Watch stream drained during inventory seed | Pass |
| Stream failure atomically aborts the seed | Pass |
| Startup inventory stabilizes across offset pagination | Pass |
| Forced recovery survives equal- and older-version coalescing | Pass |
| Forced recovery survives independent health phase updates | Pass |
| Pending and completed deletes remain terminal | Pass |
| Seed and receiver cancellation is joined | Pass |
| Handler failures retain durable retry state | Pass |
| No secrets in logs or responses | Pass |
| Conventional commit messages | Pass |
The realm's authoritative clientScopes list suppressed Keycloak's auto-seeded defaults, and hypershell-control-plane had no defaultClientScopes, so its client-credentials token omitted preferred_username. The API server then resolved the caller by sub (UUID) instead of service-account-hypershell-control-plane, so every gRPC read/watch failed RBAC as PermissionDenied. Attach the profile scope to the control-plane client so its username->preferred_username mapper runs, deterministically stamping preferred_username=service-account-hypershell-control-plane to match the RBAC_SERVICE_ACCOUNTS trusted bypass. Identity scope only; least privilege and RBAC semantics unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
knownKeys is reached only through the gatewaySeedSink interface, whose compile-time assertion lives in watcher.go. golangci's unused analyzer does not trace that dispatch for a generic type, so it flags the reachable method as unused. Add a documented //nolint:unused on the method; no behavioral change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jsell-rh
left a comment
There was a problem hiding this comment.
Commits bba025f and 6a3a035 address the two concrete CI failures without changing product behavior: the Kind control-plane client now receives the existing profile mapper so its service-account identity matches the configured RBAC bypass, and the generic queue method carries a narrowly documented unused suppression. The authorization change restores the intended trusted service-account path without granting roles or weakening enforcement, while the lint-only change leaves runtime queue semantics untouched.
Findings
No findings.
Assessment
APPROVE (submitted as COMMENT because this is an author self-review).
Findings Summary (ordered by severity, highest first):
No findings.
Convention Checklist (omit conventions not applicable to the diff):
| Convention | Result |
|---|---|
| Control-plane service-account token exposes the configured stable identity | Pass |
| Trusted service-account bypass remains exact-match and narrowly scoped | Pass |
| RBAC enforcement remains enabled and no bypassed method set is broadened | Pass |
| Attached Keycloak scope provides identity claims without granting roles | Pass |
| Generic queue method remains reachable through the asserted seed-sink contract | Pass |
| Lint suppression is local, documented, and behavior-neutral | Pass |
| No secrets in logs or responses | Pass |
| Conventional commit messages | Pass |
jsell-rh
left a comment
There was a problem hiding this comment.
Head fbdc983 only merges target branch main at 66155b2; both incoming commits are already part of the target branch, and the resulting tree exactly matches the standard Git three-way merge. The effective PR change set therefore retains the previously reviewed console implementation and CI fixes without conflict-resolution drift or new PR-owned behavior.
Findings
No findings.
Assessment
APPROVE (submitted as COMMENT because this is an author self-review).
Findings Summary (ordered by severity, highest first):
No findings.
Convention Checklist (omit conventions not applicable to the diff):
| Convention | Result |
|---|---|
| Target-branch refresh contains no manual conflict-resolution drift | Pass |
| Effective PR diff preserves reviewed console lifecycle and authorization boundaries | Pass |
| Previously accepted CI identity and lint fixes remain intact | Pass |
| Protected historical review threads remain unresolved | Pass |
| No secrets in logs or responses | Pass |
Merge commit cleanly records current main |
Pass |
Summary
Specs Option 1 for HYPERSHELL-3: co-deploy the OpenShell dashboard ("Gateway Console") with each gateway that has a route, fronted by an oauth2-proxy sidecar. No code yet — this PR adds one sub-spec and links it from the parent.
The console is authenticated by a per-gateway confidential Keycloak client (
{name}-{id}-console) whose audience and client-role mappers target the gateway client ({name}-{id}). Browser tokens therefore carryaud = {name}-{id}andhypershell.roles, so the gateway validates console traffic with the same rules it applies to the CLI, and the existing OIDC Role Bridge governs access.fullScopeAllowed = falsepreserves per-gateway isolation.Settled decisions
routegets a console; no separate opt-in field.console-<ns>.<base-domain>on the shared Gateway's wildcard cert.What the spec covers
Enablement, confidential client + mappers, credential Secret, Deployment (dashboard + oauth2-proxy, restricted SecurityContext, probes), Service + HTTPRoute, two NetworkPolicies, lifecycle/cleanup, atomicity/idempotency,
consoleAddress(read-only data-model field + migration), RBAC (httproutes), and the shared-Gateway HTTP-listener prerequisite.Commits
spec per-gateway Gateway Console— the initial spec + parent index linkprune and simplify console spec— prune ~640 → 380 lines with no loss of testable requirementsNot included
Implementation, the shared-Gateway HTTP listener (admin prerequisite), and the OpenShell dashboard image (upstream dependency).
🤖 Generated with Claude Code