feat: unconditional control-plane self-registration with no-auth support HYPERSHELL-297

[markturansky]

Summary

• Every control plane now self-registers at startup unconditionally, whether or not OIDC authentication is configured
• When auth is enabled, the managed cluster record is keyed on the JWT OIDC subject; when auth is disabled (local development), it is keyed on the cluster name alone with an empty oidc_subject
• Adds managed-cluster-registrar Keycloak realm role and assigns it to the hypershell-control-plane service account so the JWT carries the required role claim
• Includes platform spec updates that define the unified registration model

API server changes

• Registration handler tolerates missing token (no-auth path)
• New FindByNameNoOIDCSubject DAO method separates authenticated and unauthenticated identity spaces
• Service uses dual-key advisory lock (oidc_subject or name: prefix) and conditional lookup

Control plane changes

• Registration is unconditional at startup (no longer gated on OIDC + name both being set)
• ManagedClusterName defaults to "local" when HYPERSHELL_MANAGED_CLUSTER_NAME is unset
• Nil TokenSource omits Authorization header (safe interface-nil pattern)
• 3 new unit tests for registration client (no-token, with-token, 403-forbidden)

Keycloak realm config

• Added managed-cluster-registrar realm role
• Added roles client scope to hypershell-control-plane so realm_access.roles appears in JWTs
• Added service account user with the registrar role mapped

Test plan

• go vet ./... and go test ./... pass on both api-server and control-plane
• No-auth local dev: controller registers with name alone, empty oidc_subject, 201/200 idempotent
• Auth-enabled Kind cluster: controller registers with OIDC token, oidc_subject populated from JWT sub claim, managed-cluster-registrar role enforced
• ROKS hysh-ibm-01 validation (auth-enabled, route ingress mode)

🤖 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: Advanced

Run ID: 21cdad54-5193-4d50-8aac-df6ebabe4c6f

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.}

[amber-review-bot]

Amber review

Status: Complete

View the submitted review.

[github-actions]

HyperShell environment deploying

Deploying commit ae094f2 to an ephemeral OpenShift environment. This
comment will update in place once the environment is ready.

[amber-review-bot]

Verdict

The em-dash cleanup landed, but make check still exits 1 because the +300 lines shifted the whitelisted content without updating .forbidden-terms-whitelist.json, so the repo policy gate remains red. The remote/cross-cloud trust-boundary questions are still open, and the new commit introduced an internal contradiction between the server-side filter now described in control-plane.spec.md and the client-side filter still required in global-architecture.spec.md.

Summary

Solid, well-structured spec work: the "unified self-registration" reframing reads clearly and the added local-dev (auth-disabled) path is a genuine improvement. Two of the prior findings are partly addressed, but the CI gate still fails and the cross-cloud watch-stream authorization/authentication questions remain, so I am keeping the verdict at REQUEST_CHANGES.

Findings

[Critical] Repository policy check still fails - stale whitelist (CI blocker). I re-ran scripts/check_forbidden_terms.py (invoked by make check, Makefile:208-209,232) against the current head and it exits 1. The em dashes are gone (good), but the added lines shifted the whitelisted content while .forbidden-terms-whitelist.json is unchanged in this PR, so the checker now reports stale whitelist entries at global-architecture.spec.md:103 and :1190 and re-flags acp at :106,107 and vteam at :1512. Fix: update the line numbers in .forbidden-terms-whitelist.json to track the shifted content. Existing thread: #284 (comment) . Confidence: High (reproduced locally, exit 1).

[Major] Server-side vs client-side cluster_id filtering now contradict each other, and neither states an authorization requirement. The new commit rewrote control-plane.spec.md:222 to say the filter "is applied server-side ... the API server returns only matching gateways," which addresses the earlier exposure in principle. But the new requirement "Control Plane Watches Only Its Own Gateways" (global-architecture.spec.md:541-551) still specifies the client-side behavior: the API server "emits a Gateway event with cluster_id=XYZ" and "the control plane SHALL ignore the event." These two contracts cannot both hold. Separately, "the control plane passes its cluster_id in the WatchGateways request" does not close the exposure unless the API server authorizes that cluster_id against the caller's authenticated identity - otherwise a remote control plane can request any cluster_id. Please reconcile the two files and require the server to scope the stream to the caller's own identity, not to a client-supplied cluster_id. Existing thread: #284 (comment) . Confidence: High.

[Major] Externally exposed gRPC watch still has no stated authN/authZ requirement. Requirement: gRPC External Access (global-architecture.spec.md:601-612) and the HYPERSHELL-333 section (:458-483) mandate TLS and system-trust credentials on the externally reachable hub gRPC endpoint, but do not require the WatchGateways stream itself to be OIDC-authenticated and scoped to the caller's cluster_id. Once this endpoint is published to the public internet, TLS-only means any TLS client can open a watch. Please add a requirement that the external gRPC watch enforces the control plane's OIDC token and scopes the stream to its cluster_id. This interacts with the JWT-audience/bypass work (see Cross-PR section). Existing thread: #284 (comment) . Confidence: High.

[Minor] Registration idempotency key is still name-only in the requirement scenario. The prose at global-architecture.spec.md:339-340 now correctly says "the same identity and name," but the requirement scenario "Repeated registration is idempotent" (:515-519) still keys on name alone ("re-registers with the same name ... SHALL return the same cluster_id"), with no mention of oidc_subject. The authoritative managed-cluster-registration.spec.md keys on (oidc_subject, name). Align the scenario so it does not read as if any holder of the shared managed-cluster-registrar role could resolve another control plane's record by name. Existing thread: #284 (comment) . Confidence: High.

Cross-PR coordination

A material design overlap exists with the open PR that defines a "controller-local execution" model (each execution controller watches its parent API and reconciles only its assigned gateways in its own cluster): #272. That PR rewrites the exact passages this PR rewrites in global-architecture.spec.md - the "two reconciliation planes" note, the "Operational Role" and "Control Plane Reconciliation Flow" prose, and the control-plane-to-gateway edges of the Mermaid topology - but with a different framing and a Draft "Status scope," while this PR reframes the same passages as "unified self-registration" ("there is no distinction between hub and spoke," a Cloud Hub's own control plane is just another ManagedCluster) and keeps them Active. The topology edges also diverge: this PR keeps a single hub ACP reconciling into remote M*GW namespaces, while #272 replaces those with per-cluster execution controllers reconciling locally. Both cannot land unchanged. Maintainers need to decide the canonical execution model and naming, which framing is authoritative, the Active/Draft status, and the merge order.

A second coordination point exists with the open PR that enforces management-API JWT issuer/audience validation: #182. That PR requires aud=hypershell-frontend on both HTTP and gRPC management endpoints (with a scenario rejecting tokens whose aud is not hypershell-frontend) and keeps gRPC watch streams on the JWT-bypass list as "trusted in-cluster services." This PR requires a remote control plane to authenticate its REST /managed_clusters/registration call and its cross-cloud gRPC watch via OIDC client_credentials using the hyp{N}-mc{NN} client with the managed-cluster-registrar role. These must be reconciled: the registration token this PR describes will be rejected unless the hyp{N}-mc{NN} client is configured to carry aud=hypershell-frontend (#182 anticipates this via dedicated Keycloak mappers, but this PR's OIDC client table does not state it), and #182's "gRPC watch bypasses JWT" assumption cannot hold once the watch endpoint is exposed cross-cloud per HYPERSHELL-333. The owners should agree on the audience the control-plane token carries and on whether the external gRPC watch is authenticated before either lands.

Previous concerns

• [Critical] CI blocker: em dashes + stale whitelist (#284 (comment)) - partially addressed, still present. Em dashes are removed, but scripts/check_forbidden_terms.py still exits 1: .forbidden-terms-whitelist.json is unchanged so entries at global-architecture.spec.md:103 and :1190 are stale and acp (:106,107) / vteam (:1512) are re-flagged.
• [Major] Client-side-only cluster_id filtering (#284 (comment)) - partially addressed, still present. control-plane.spec.md:222 now states server-side filtering, but global-architecture.spec.md:541-551 still specifies client-side filtering, and no file requires the server to authorize the cluster_id against the caller's identity.
• [Major] External gRPC watch lacks authN/authZ requirement (#284 (comment)) - still present. Requirement: gRPC External Access (global-architecture.spec.md:601-612) mandates TLS but not OIDC authentication or cluster_id scoping of the stream.
• [Minor] Idempotency key stated name-only (#284 (comment)) - partially addressed, still present. The prose at :339-340 now says "same identity and name," but the requirement scenario at :515-519 still keys on name alone.

Findings Summary (ordered by severity, highest first)

1. [Critical] Repo policy check fails: stale whitelist entries break make check - Convention / CI (global-architecture.spec.md:103,106,107,1190,1512; .forbidden-terms-whitelist.json)
2. [Major] Server-side vs client-side cluster_id filtering contradict each other; no identity-scoped authorization stated - Security / Spec Consistency (control-plane.spec.md:222; global-architecture.spec.md:541-551)
3. [Major] Externally exposed gRPC watch lacks a stated authN/authZ requirement - Security (global-architecture.spec.md:601-612)
4. [Minor] Registration idempotency scenario stated name-only vs authoritative (oidc_subject, name) - Spec Consistency (global-architecture.spec.md:515-519)

Convention Checklist

Convention	Result
No em dashes (repo policy)	Pass
Whitelist entries current (make check)	Fail
Secret references, not inline secrets	Pass
Input/identity validation defined	Partial (idempotency key wording)
Cross-spec references consistent	Fail (server-side vs client-side filter)
Conventional commit message	Pass

[amber-review-bot]

[Major] This client-side filter contradicts the server-side filter now stated in control-plane.spec.md:222. That file says the API server "returns only matching gateways" (server-side), but this scenario has the API server emit a cluster_id=XYZ event that the control plane must ignore (client-side). Both cannot hold. Also, filtering by a cluster_id the caller supplies in the WatchGateways request is not isolation unless the API server authorizes that cluster_id against the caller's authenticated identity; otherwise a remote control plane can request any cluster's gateways over the cross-cloud gRPC endpoint (HYPERSHELL-333). Reconcile the two files and require the server to scope the stream to the caller's own identity. Related prior thread: #284 (comment)

[rh-amarin]

Does this HYPERSHELL_MANAGED_CLUSTER_NAME need to be defined in other deploys like openshift?