feat(registration): managed cluster self-registration via OIDC client credentials - #265
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 |
931b763 to
c4189db
Compare
Amber reviewStatus: Complete |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
REQUEST_CHANGES. This is a well-structured spec for managed-cluster self-registration, but the new /registration route is written with a hyphenated path (/managed-clusters/registration) that contradicts both this repo's existing underscore route convention and this PR's own data-model.spec.md entry, and the branch also carries undescribed RBAC code (including a hook-breaking em dash) that belongs to a separate change.
The spec itself is clear, includes concrete scenarios, and correctly separates spoke-reported last_seen_at liveness from the reconciler-owned status field. The design decisions (single register+heartbeat endpoint, admin-gated managed-cluster-registrar role, server-assigned immutable oidc_subject, fail-closed startup) are sound and internally consistent. My blocking concern is the interface/naming inconsistency described below, plus PR-scope hygiene.
Findings
[Major] Route path is inconsistent with the rest of the API and with this PR's own data model
Every existing route in the platform uses underscored collection names (/managed_clusters, /managed_databases, /gateway_releases), and plugins/managedClusters/plugin.go registers PathPrefix("/managed_clusters"). This PR's data-model.spec.md correctly adds POST /managed_clusters/registration (underscore), but managed-cluster-registration.spec.md, control-plane.spec.md, and rbac-enforcement.spec.md all write /managed-clusters/registration (hyphen). A spec that drives implementation must be internally consistent; pick the underscore form everywhere so the implementer does not register a route that mismatches the rest of the surface.
[Minor] Em dash violates the repository text policy and will fail the pre-commit hook
plugins/roleBindings/integration_test.go:467 contains a U+2014 em dash in a code comment. CLAUDE.md forbids em dashes and the pre-commit hook rejects them; replace it with a hyphen. (This line is part of the stacked RBAC change - see Cross-PR coordination.)
[Minor] Spec does not require validation of the caller-supplied name
The name field is caller-supplied and becomes part of the (oidc_subject, name) upsert key and a stored record. The security standard requires validating user input (K8s DNS label format) and preventing log injection. The spec should state that name is validated before it is persisted or logged.
Cross-PR coordination
This pull request's branch is stacked on and re-includes the RBAC default-role code owned by PR #263 (pkg/rbac/grpc_interceptor.go, pkg/rbac/user_provisioning.go, and the roleBindings plugin/service/tests), which is not mentioned in this PR's body or title (this PR presents itself as spec-only). Maintainers must decide and enforce a merge order: land #263 first, then rebase this branch so its diff contains only the HYPERSHELL-326 spec files. Merging the two independently risks a duplicate or conflicting application of the identical RBAC change and makes this PR's code diff impossible to review in isolation.
Findings Summary (ordered by severity, highest first)
- [Major]
/registrationroute uses hyphenated/managed-clusters/...in three specs, contradicting the underscore convention and this PR's own data-model entry - Spec Consistency / Interface - [Minor] Em dash in
integration_test.go:467will fail the pre-commit hook - Convention - [Minor] Spec does not require validation of the caller-supplied
name- Security / Input Validation
Convention Checklist
| Convention | Result |
|---|---|
| Image references consistent across manifests | N/A |
| Interface/route naming consistent across specs | Fail |
| No em dashes in text files | Fail |
| Input validation specified for user-supplied fields | Fail |
| Secret references, not inline secrets (OIDC creds via env/secret) | Pass |
| Conventional commit messages | Pass |
| Reconcile/fail-closed startup semantics defined | Pass |
|
|
||
| ## API | ||
|
|
||
| ### POST /api/hypershell/v1/managed-clusters/registration |
There was a problem hiding this comment.
[Major] Route naming inconsistency. This spec (and control-plane.spec.md / rbac-enforcement.spec.md) uses the hyphenated path /managed-clusters/registration, but every existing collection route uses underscores (/managed_clusters, /managed_databases, /gateway_releases) and plugins/managedClusters/plugin.go registers PathPrefix("/managed_clusters"). Even this PR's data-model.spec.md line 230 uses /managed_clusters/registration. Standardize on the underscore form across all four specs so the implementation matches the rest of the API surface.
|
|
||
| ### Requirement: Managed Cluster Self-Registration RBAC | ||
|
|
||
| The `POST /api/hypershell/v1/managed-clusters/registration` endpoint SHALL require the |
There was a problem hiding this comment.
Same route-naming issue as the main spec: /managed-clusters/registration (hyphen) here vs /managed_clusters/registration (underscore) in data-model.spec.md and the existing route convention. Please align.
| userID, userErr := userService.UpsertByUsername(context.Background(), "sync-jwt-no-default", nil, nil) | ||
| Expect(userErr).NotTo(HaveOccurred()) | ||
|
|
||
| // User has platform:admin in their JWT — they have realm roles, so defaults must not apply. |
There was a problem hiding this comment.
[Minor] Em dash (U+2014). CLAUDE.md forbids em dashes and the pre-commit hook rejects them - replace the — with a hyphen. Note this line is part of the stacked RBAC change (see the Cross-PR coordination note in the top-level review).
|
|
||
| ### Requirement: Idempotent Registration | ||
|
|
||
| `POST /managed-clusters/registration` SHALL be idempotent on the `(oidc_subject, name)` key. |
There was a problem hiding this comment.
[Minor] Input validation. name is caller-supplied and becomes part of the (oidc_subject, name) upsert key and a persisted/loggable record. Per the security standard, the spec should require validating name (K8s DNS label format) and sanitizing it before logging.
c4189db to
803a9c7
Compare
Amber reviewStatus: Complete |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
This PR is described as a spec-only change for managed-cluster self-registration (HYPERSHELL-326), but its diff against main also carries ~200 lines of undescribed RBAC default-role production code and tests (HYPERSHELL-262). The self-registration spec itself is well-structured, but the endpoint path is inconsistent across the specs and the bundled code needs cross-PR coordination before merge.
Findings
[Major] Undescribed production code bundled into a spec PR (Scope / PR hygiene)
The PR body and test plan describe only spec files, yet the diff modifies pkg/rbac/grpc_interceptor.go, pkg/rbac/user_provisioning.go, and four plugins/roleBindings/* files with the "assign gateway:creator by default" behavior. Reviewers cannot properly evaluate changes that the description does not mention, and merging them here silently ships an RBAC behavior change under a spec title. Please either split the RBAC code out of this PR (rebase onto main once that work lands separately) or update the PR body/test plan to fully describe and own the code change. Confidence: High.
[Major] Registration endpoint path is inconsistent across the specs (Spec Consistency)
specs/platform/data-model.spec.md registers the route as /managed_clusters/registration (underscore), matching every other route in that table (/managed_clusters, /gateway_releases, /managed_databases). The new spec, control-plane.spec.md, and rbac-enforcement.spec.md all use /managed-clusters/registration (hyphen). Since these specs drive implementation, the two forms will produce a route mismatch. Pick one form (the existing underscore convention) and apply it everywhere. Confidence: High.
[Minor] Zero-role JWT re-grants gateway:creator after full Keycloak revocation (Security / Behavior)
In SyncJWTRoles, when len(jwtRoles) == 0 the default roles are merged into the effective set, and the reconcile loop then deletes any previously-synced global bindings not in that set. A user whose Keycloak roles are all revoked (e.g. a former platform:admin) will have that binding removed and gateway:creator granted on the next request. This may be intended, but it means "revoke everything in Keycloak" silently becomes "downgrade to gateway:creator" rather than "no platform access". Please confirm this is the desired fail-state and document it. Confidence: Medium.
[Minor] Registration response shape and startup-failure behavior are under-specified (Spec Completeness)
The /registration response is defined as { "cluster_id": "<KSUID>" }, which diverges from the full ManagedCluster object returned by the other managed_clusters routes; consider stating explicitly whether that is intentional. The "Registration failure blocks startup" requirement also leaves "log the error and exit (or retry with backoff per operator configuration)" ambiguous - a spec requirement should pin down the default behavior rather than leaving exit-vs-retry to interpretation. Confidence: Medium.
Cross-PR coordination
A separate open pull request (#263, HYPERSHELL-262, "assign gateway:creator by default on user provisioning") owns exactly the RBAC default-role code that this PR also carries: pkg/rbac/grpc_interceptor.go, pkg/rbac/user_provisioning.go, and plugins/roleBindings/{service.go,plugin.go,integration_test.go,testmain_test.go}. This PR includes an earlier, divergent snapshot of that work - notably the "bootstrap defaults only when the JWT carries zero roles" (if len(jwtRoles) == 0) logic, whereas #263 has since added a commit that merges default roles more broadly. Both PRs also edit specs/security/rbac-enforcement.spec.md (four-role -> five-role narrative and the role/permission tables). Maintainers must decide which implementation of the default-role behavior is authoritative and establish merge order (land #263 first, then rebase this PR so only the HYPERSHELL-326 spec changes remain), otherwise the two PRs will overwrite each other's RBAC logic and produce conflicting versions of the shared spec.
Findings Summary (ordered by severity, highest first)
- [Major] Undescribed RBAC production code bundled into a spec-only PR - Scope / PR hygiene
- [Major] Registration endpoint path inconsistent:
/managed_clusters/registrationvs/managed-clusters/registration- Spec Consistency - [Minor] Zero-role JWT re-grants
gateway:creatorafter full Keycloak revocation - Security / Behavior (service.go L113) - [Minor] Registration response shape and startup-failure behavior under-specified - Spec Completeness
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (oidc_subject server-assigned, immutable) | Pass |
| Reconcile pattern (SyncJWTRoles) | Pass |
| Interface/route naming consistent across specs | Fail |
| PR description matches diff scope | Fail |
| Conventional commit messages | Pass |
| | GET/PATCH/DELETE | `/gateway_releases/{id}` | Get/Update/Delete | | ||
| | GET/POST | `/managed_clusters` | List/Create | | ||
| | GET/PATCH/DELETE | `/managed_clusters/{id}` | Get/Update/Delete | | ||
| | POST | `/managed_clusters/registration` | Self-register spoke; idempotent on (oidc_subject, name); updates last_seen_at on every call | |
There was a problem hiding this comment.
Route naming inconsistency: this table (and every other route in it) uses the underscore form /managed_clusters/registration, but managed-cluster-registration.spec.md, control-plane.spec.md, and rbac-enforcement.spec.md all use the hyphen form /managed-clusters/registration. Since these specs drive implementation, the two forms will diverge into a real route mismatch. Please standardize on the existing underscore convention across all four files.
|
|
||
| ## API | ||
|
|
||
| ### POST /api/hypershell/v1/managed-clusters/registration |
There was a problem hiding this comment.
This uses the hyphenated path /api/hypershell/v1/managed-clusters/registration, but the canonical route table in data-model.spec.md (and all sibling routes) uses underscores (/managed_clusters/registration). Align these to avoid an implementation-time route mismatch.
| // Users who already have Keycloak realm-role assignments (even non-synced | ||
| // ones) are managed entirely through Keycloak; applying defaults to them | ||
| // would grant capabilities that Keycloak intentionally withheld. | ||
| if len(jwtRoles) == 0 { |
There was a problem hiding this comment.
Because the reconcile below deletes any previously-synced global binding not in jwtRoleSet, a user whose Keycloak roles are fully revoked (len(jwtRoles) == 0) will lose their prior binding (e.g. platform:admin) and be granted the default gateway:creator on the next request. Please confirm this downgrade-on-full-revocation is the intended fail-state and document it. (Note: this code is also the subject of a separate open PR - see the Cross-PR coordination section of the top-level review.)
|
|
||
| // defaultRolesFromEnv reads RBAC_DEFAULT_ROLES (comma-separated role names). | ||
| // Defaults to gateway:creator so all authenticated users can create gateways. | ||
| func defaultRolesFromEnv() []string { |
There was a problem hiding this comment.
This RBAC default-role code is not described in the PR body, which presents this as a spec-only change (HYPERSHELL-326). Undescribed production changes are hard to review and effectively ship an RBAC behavior change under a spec title. Please either move this code to its own PR or expand the PR description/test plan to own it. See the Cross-PR coordination section for the overlap with the HYPERSHELL-262 work.
… are assigned SyncJWTRoles was guarded by `if len(jwtRoles) > 0` at both the HTTP and gRPC call sites, so users whose JWT carried no Keycloak realm roles never had the call made and therefore never received the configured default role bindings (e.g. gateway:creator). Remove the guard at both sites; the context-key assignment (needed by downstream middleware) remains gated on non-empty roles. SyncJWTRoles now runs unconditionally so the default-roles path is exercised on every provisioned user. Also adds a startup warning when an RBAC_DEFAULT_ROLES entry is not in JWTSyncedRoles, and fixes integration tests that relied on user ID being absent from context (which masked missing gateway:owner preconditions). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ials HYPERSHELL-326 Add spec for spoke control-planes to self-register using existing OIDC client_credentials, eliminating manual cluster_id distribution from gitops. - New spec: managed-cluster-registration.spec.md -- /registration sub-resource within managedClusters plugin; idempotent on (oidc_subject, name); updates last_seen_at on every call, serving as both registration and heartbeat loop - data-model: add oidc_subject and last_seen_at to ManagedCluster entity; add /managed_clusters/registration to API reference - rbac-enforcement: add managed-cluster-registrar role (Keycloak JWT, global scope, no gateway permissions); add requirement with grant/deny scenarios - control-plane: add spoke startup self-registration requirement; /registration called before WatchGateways, then looped every 60s for last_seen_at updates - index: register new spec in the spec registry Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… merged PR #263 PR #263 (HYPERSHELL-262) established that: - Only platform:admin and gateway:creator are in JWTSyncedRoles - gateway:creator is applied to all users by default via RBAC_DEFAULT_ROLES - isAuthorized falls through to hasGatewayCreator for unhandled resources This meant the managed-cluster-registrar spec was wrong in two ways: 1. Enforcement claim: said "no new RBAC machinery needed" but the hasGatewayCreator fallback would allow ALL users to call /registration 2. Isolation claim: "grants no gateway access" is false when RBAC_DEFAULT_ROLES=gateway:creator (the default) Fix: - managed-cluster-registrar is now specified as JWT-direct (checked live from JWT claim in isAuthorized), consistent with HypershellAdminRole - isAuthorized needs a dedicated case for POST managed_clusters/registration - Not added to JWTSyncedRoles; no DB RoleBinding lifecycle - Seeded as a built-in role for discoverability only - Isolation scenario scoped to RBAC_DEFAULT_ROLES= (Keycloak-only mode) - Note added that default config gives spokes gateway:creator too - Also fixes gofmt indentation introduced by leftover HYPERSHELL-262 commit Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
803a9c7 to
5cdf0de
Compare
… RBAC posture The expectation is full RBAC enforcement where Keycloak is the sole authority for all permissions -- gateway creation, platform admin access, and managed cluster self-registration. No role is auto-assigned in production. Changes: - Production posture is now authoritative: RBAC_ENFORCE=true + RBAC_DEFAULT_ROLES= - RBAC_DEFAULT_ROLES=gateway:creator is explicitly labeled as a local-dev convenience only; SHALL NOT appear in production or staging overlays - All "Keycloak-only mode" scenario caveats removed -- that IS the mode - managed-cluster-registrar isolation is now a guarantee, not a footnote - Operator Note rewritten: produce the Keycloak setup steps required before enabling enforcement (gateway:creator, platform:admin, managed-cluster-registrar) - Design decisions updated to reflect Keycloak-exclusive role assignment Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Three issues identified in amber review of PR #265: 1. Path consistency: standardize all endpoint references to use underscore form (/managed_clusters/registration) matching every other route in data-model.spec.md and the existing API convention. 2. Response shape: document explicitly that POST /registration returns only { "cluster_id" } (not the full ManagedCluster object). The spoke needs exactly one field; the narrow shape is intentional and differs from GET /managed_clusters/{id} by design. 3. Startup-failure behavior: replace the ambiguous "exit or retry per operator configuration" with a concrete split: - 403 Forbidden: exit immediately with a clear error (retrying is pointless without a Keycloak role change) - Transient errors (network, 5xx): retry with exponential backoff Same semantics applied consistently in both control-plane.spec.md and managed-cluster-registration.spec.md. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…egistration
Add the full stack for spoke self-registration via POST /managed_clusters/registration:
API Server (Wave 2 OpenAPI + Wave 3 SDK):
- POST /api/hypershell/v1/managed_clusters/registration endpoint
- ManagedClusterRegistrationRequest/Response schemas
- ManagedCluster gains oidc_subject and last_seen_at fields
- SDK regenerated with RegisterManagedCluster operation
API Server Backend (Wave 4):
- ManagedCluster model: OIDCSubject string, LastSeenAt *time.Time
- Migration: add oidc_subject + last_seen_at columns; partial unique index on
(oidc_subject, name) WHERE oidc_subject IS NOT NULL AND oidc_subject <> ''
- DAO: FindByOIDCSubject for upsert lookup
- Service: Register() upserts on (oidc_subject, name); returns 201 on create,
200 on heartbeat; advisory-locks on oidc_subject to prevent duplicate creates
- Handler: Register() extracts sub claim from JWT; custom handler writes 201/200
- Plugin: /registration route registered before /{id} to prevent mux capture
- Presenter: PresentRegistrationResponse; PresentManagedCluster includes new fields
- RBAC: hasManagedClusterRegistrar(); isAuthorized() dedicated case for resource
"registration" + POST - JWT-direct, bypasses hasGatewayCreator fallback
- Roles: seed managed-cluster-registrar built-in role (DB record for discoverability;
not in JWTSyncedRoles, no DB binding lifecycle)
Control Plane (Wave 6):
- Config: ManagedClusterName from HYPERSHELL_MANAGED_CLUSTER_NAME
- internal/registration: HTTP client; ErrForbidden sentinel for fail-closed
- main.go: registerWithBackoff() - 403 exits immediately, other errors retry
with exponential backoff; ClusterID resolved at runtime from registration;
60s heartbeat goroutine updates last_seen_at continuously
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…gistration endpoint Adds ManagedCluster.oidc_subject, ManagedCluster.last_seen_at, and the RegisterManagedCluster operation to both the TypeScript and Go SDKs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…DCSubject and LastSeenAt GORM auto-naming converts OIDCSubject -> o_id_c_subject (a word-per-capital expansion). Add column: tags to force the correct snake_case column names that the migration already creates (oidc_subject, last_seen_at). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
markturansky
changed the title
Amber reviewStatus: Complete |
- Remove description-to-Status mapping (blocker: Status is reconciler-owned; description supplied at registration was silently corrupting that field) - Move registration RBAC check before userID gate so transient user-provisioning DB failures return a retryable error, not a fatal 403 that causes the spoke to exit (critical: ErrForbidden was non-retryable) - Promote managed-cluster-registrar string to const roleManagedClusterRegistrar in rbac package to avoid magic literal drift - Use errors.Is(err, gorm.ErrRecordNotFound) instead of direct pointer compare - Add context parameter and 30s timeout to registration.Client.Register so in-flight HTTP calls are cancelled on shutdown and never hang indefinitely - Escalate heartbeat log from WARN to ERROR after 5 consecutive failures - Implement Replace in managedClusterDaoMock (was NotImplemented) - Add 5 RBAC unit tests covering registration path: role allow/deny, method restriction, and the userID-gate bypass regression guard Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
REQUEST_CHANGES. The self-registration stack is well structured (advisory-locked upsert, JWT-direct RBAC branch, backoff-with-fatal-on-403 startup), but three Major issues need fixing before merge: the registration description is written into the cluster Status column, the shipped production posture does not actually deliver the isolation guarantee the spec now asserts, and the entire feature ships with no tests despite the spec mandating registrar grant/deny coverage.
Amber Analysis
A clean, well-commented implementation of spoke self-registration. The advisory lock keyed on the OIDC subject, the 201/200 create-vs-heartbeat semantics, the non-retryable 403 handling, and the JWT-direct RBAC path (kept out of JWTSyncedRoles) all match the spec's intent. My concerns are correctness of one field mapping, a gap between the new spec's production-posture claims and what the PR actually ships, missing input validation, and the complete absence of tests.
Findings
[Major] description is stored in the Status column - components/api-server/plugins/managedClusters/service.go:190
Register writes the caller-supplied description into cluster.Status (cluster.Status = &description). Status is a semantically distinct field (cluster operational status) surfaced in GET /managed_clusters responses. A registration description will masquerade as cluster status, and the reconciler/dashboard consumers of status will see attacker/operator-controlled free text. The ManagedCluster model has no Description column; either add one (and migrate) or drop the mapping. Confidence: High.
[Major] Production overlay does not deliver the spec's isolation guarantee - deploy/openshift/kustomization.yaml (not modified by this PR)
specs/security/rbac-enforcement.spec.md now states the OpenShift overlay SHALL ship with RBAC_DEFAULT_ROLES= (empty) and asserts an isolation guarantee ("a spoke service account holding only managed-cluster-registrar has no gateway permissions"). The overlay is not changed here, and the code default remains gateway:creator (components/api-server/plugins/roleBindings/plugin.go:45). As shipped, RBAC_DEFAULT_ROLES is unset, so every authenticated principal - including a spoke - receives gateway:creator. The spec scenario "managed-cluster-registrar grants no gateway access" would fail in the production overlay. Either update the overlay to set RBAC_DEFAULT_ROLES= in this PR or soften the spec's normative claim so it does not describe behavior the PR does not deliver. Confidence: High.
[Major] No tests for any of the new surface area - service/handler/RBAC/registration client
There are no _test.go changes in this PR. The new upsert logic (create vs heartbeat, name-mismatch 409, advisory locking), the isAuthorized registration branch, and the control-plane registration.Client (403 vs transient, missing cluster_id) are all untested. specs/security/rbac-enforcement.spec.md explicitly requires integration coverage of managed-cluster-registrar grant/deny and the JWT-direct path. Please add at least: an authorization unit test for the registrar branch, a service test for create/heartbeat/409, and a client test for the 403/transient/parse paths. Confidence: High.
[Major] Registration name is not validated as a K8s DNS label - components/api-server/plugins/managedClusters/handler.go:46
The handler validates only that name is non-empty; the OpenAPI schema imposes no pattern. name becomes a persistent resource identifier (and part of the unique index). The security standard requires resource names to be validated as K8s DNS labels. Add DNS-label validation (reject on invalid) before the upsert. Confidence: Medium.
[Minor] isAuthorized registration case is broader than the spec - components/api-server/pkg/rbac/authorization.go:257
The spec pseudocode scopes the check to resource == \"managed_clusters\" && resourceID == \"registration\", but the code matches resource == \"registration\" for any POST. It works today because the route template's last segment resolves to registration, but it is brittle: any future collection that adds a /registration subpath would silently fall under the registrar check. Consider scoping to the managed_clusters path as the spec describes. Confidence: Medium.
[Minor] New self-registered clusters have empty Provider/KubeconfigSecret - components/api-server/plugins/managedClusters/service.go:158
The create path leaves Provider and KubeconfigSecret empty. This is presumably intended for the pull model, but it is worth confirming that downstream consumers (presenter, reconciler filters, any provider-based logic) tolerate a blank provider. Confidence: Low.
Cross-PR coordination
Coordination is required with the user-registration/activity-stats work in PR #264. Both PRs regenerate the same shared generated artifacts - the internal OpenAPI client (components/api-server/pkg/api/openapi/**, openapi.yaml) and both the Go and TypeScript SDKs - and both add a new authorization case to components/api-server/pkg/rbac/authorization.go, so whichever merges second must regenerate against the combined spec or the SDK drift check will fail. More substantively, PR #264 extends the custom trex-sdk-generator to emit collection-level literal subpaths (e.g. /users/stats), which is exactly the shape of this PR's /managed_clusters/registration endpoint. In this PR the custom SDK regeneration did not emit a registration client method (only the model fields and the spec-SHA header changed in components/sdk-go/client/managed_cluster_api.go), so the endpoint is absent from the Go/TypeScript SDKs even though the PR body claims a RegisterManagedCluster operation. Maintainers should decide the merge order and, once PR #264's generator support lands, regenerate so this endpoint is represented in the custom SDKs for parity.
Findings Summary (ordered by severity, highest first)
- [Major]
descriptionwritten into theStatuscolumn - Data Model Correctness (service.go:190) - [Major] Overlay does not set
RBAC_DEFAULT_ROLES=, so the spec's isolation guarantee is not delivered - Spec Consistency / Security Posture (deploy/openshift/kustomization.yaml, plugin.go:45) - [Major] No tests for the registration endpoint, service, RBAC branch, or client - Missing Tests
- [Major] Registration
namenot validated as a K8s DNS label - Input Validation (handler.go:46) - [Minor]
isAuthorizedregistration case broader than spec - Spec/Code Consistency (authorization.go:257) - [Minor] Self-registered clusters have empty
Provider/KubeconfigSecret- Data Completeness (service.go:158)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound/ErrRecordNotFound handled |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (K8s DNS labels) | Fail |
| Reconcile/upsert pattern (not create-or-skip) | Pass |
Migrations idempotent (IF NOT EXISTS) |
Pass |
| OpenAPI/SDK generated, not hand-edited | Pass |
| Tests for new behavior | Fail |
| Spec matches shipped implementation | Fail |
| Conventional commit messages | Pass |
| LastSeenAt: &now, | ||
| } | ||
| if description != "" { | ||
| cluster.Status = &description |
There was a problem hiding this comment.
[Major] The registration description is written into cluster.Status. Status is a distinct field (cluster operational status) that is returned in GET /managed_clusters responses and consumed downstream, so a registration description ends up masquerading as cluster status. The model has no Description column - either add one (with a migration) or drop this mapping. Storing free-text into Status corrupts the status semantics.
| return | ||
| } | ||
|
|
||
| if svcErr := handlers.ValidateNotEmpty(&req, "Name", "name")(); svcErr != nil { |
There was a problem hiding this comment.
[Major] name is only checked for non-emptiness here, and the OpenAPI schema has no pattern. This value becomes a persistent resource identifier and part of the (oidc_subject, name) unique index. Per the security standard, resource names must be validated as K8s DNS labels. Add DNS-label validation and reject invalid input before the upsert.
|
|
||
| func isAuthorized(method string, resource string, resourceID string, gatewayID string, bindings []BindingSummary, jwtRoles []string) bool { | ||
| // JWT-direct: managed-cluster-registrar is never DB-synced; check JWT claim only. | ||
| if resource == "registration" && method == http.MethodPost { |
There was a problem hiding this comment.
[Minor] The spec scopes this to resource == "managed_clusters" && resourceID == "registration", but this matches resource == "registration" for any POST. It works today only because the route template's last segment resolves to registration; any future collection adding a /registration subpath would silently be gated by the registrar role instead of its own rules. Consider scoping to the managed_clusters path as the spec pseudocode describes.
|
@amber-review-bot Responding to your most recent review (head 4085a69). Fixed in 2190d87:
Also fixed (bonus, from our own review):
Not yet addressed:
|
Amber reviewStatus: Stopped The pull request head changed before Amber posted the review. A later job can review the new head. |
…isolation - Validate registration name as K8s DNS label (RFC 1123) in handler before upsert: lowercase alphanumeric + hyphens, start/end alphanumeric, max 63 chars; reject with 400 MalformedRequest on invalid input (amber Finding 4) - Add RBAC_DEFAULT_ROLES= (explicit empty) to the OpenShift overlay so spoke service accounts holding only managed-cluster-registrar receive no gateway permissions; without this the default gateway:creator grant contradicts the isolation guarantee stated in rbac-enforcement.spec.md (amber Finding 2) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Amber reviewStatus: Stopped The pull request head changed before Amber posted the review. A later job can review the new head. |
…to ManagedCluster test fixtures The SDK generator emits all ManagedCluster properties as required. The new fields added by the self-registration feature were missing from the test fixtures causing TS2322 type errors in the web console quality gate. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Amber reviewStatus: Complete |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
COMMENT. The spoke self-registration stack is well-structured: JWT-direct registrar authorization that bypasses the user-provisioning gate (so a transient DB failure never yields a fatal 403), advisory-locked upsert on (oidc_subject, name), a partial-index migration, and backoff/heartbeat on the control plane. Two items deserve maintainer attention before merge: the deploy overlay globally disables the default gateway:creator grant, and the OpenAPI description field is silently dropped.
I reviewed against CLAUDE.md, the security spec, and the control-plane conventions spec. No blockers or critical issues found. Findings below.
Major
1. RBAC_DEFAULT_ROLES: "" disables the default gateway:creator grant for every user in this overlay, not just spokes - deploy/openshift/kustomization.yaml:122
defaultRolesFromEnv() (roleBindings/plugin.go) defaults to gateway:creator when the var is unset; setting it to explicit empty returns nil for all provisioned identities, including human users. This overlay-wide switch regresses the default-grant behavior added in commit 4e349e1 (HYPERSHELL-262) for humans in order to keep spoke service accounts (which hold only managed-cluster-registrar) from receiving gateway permissions. Since the registration path is already JWT-direct and never provisions a binding, consider a narrower mechanism (e.g. excluding registrar-only service accounts from default-role assignment) rather than turning defaults off globally, or confirm this overlay is spoke-only and document the human-user impact.
Minor
2. Registration request description is accepted by the schema but silently discarded - components/api-server/plugins/managedClusters/service.go:159, handler.go:85
The OpenAPI ManagedClusterRegistrationRequest declares description, and the handler reads it and passes it into Register(ctx, name, description, oidcSubject), but the service never uses the description parameter (the model has no Description field). Clients that send description will see it vanish. Either persist it or remove it from the schema so the contract matches behavior.
3. Response marshal error is swallowed - components/api-server/plugins/managedClusters/handler.go:99
if payload, err := json.Marshal(resp); err == nil { ... } writes the status code first and then drops the body if marshalling fails, returning a 200/201 with an empty body and no logged error. Log the marshal failure so a serialization regression is observable.
4. If event emission fails after a first-time create, the create event is never re-emitted - components/api-server/plugins/managedClusters/service.go:196
On the create path a failed events.Create returns an error after the row is already persisted. The next heartbeat finds the existing record and takes the Replace branch, which emits no event, so the control plane may never receive the create notification for that cluster. This mirrors the existing Create pattern, but with the 60s heartbeat it becomes a silent, self-perpetuating gap; consider making create-event emission idempotent/retryable on the heartbeat path.
Cross-PR coordination
Two open pull requests require maintainer coordination:
-
#182 (enforce management API JWT audience): It makes the management API reject any Bearer token whose
auddoes not containhypershell-frontend(HTTP and gRPC), relying on a dedicated Keycloak audience mapper on thehypershell-control-planeclient. This PR's spoke registration client reuses the control-plane OIDC token provider to call the RESTPOST /managed_clusters/registrationendpoint. If #182 merges without the control-plane client's audience mapper covering this REST call, registration will fail closed with 401 andregisterWithBackoffwill retry indefinitely (401 is not treated as non-retryable). Maintainers should confirm the control-plane client_credentials token carriesaud=hypershell-frontendand agree on merge order / Keycloak provisioning so the two audience assumptions stay consistent. -
#251 (add deploy/gitops-base for GitOps fleet deployment): This PR's spoke-isolation invariant depends on
RBAC_DEFAULT_ROLES: ""being present in the deployment so a registrar-only service account never receives a defaultgateway:creatorbinding. #251 introduces a parallel gitops-base layer that patches the same api-server/controller configuration (jwt-enforce, RBAC env) but does not setRBAC_DEFAULT_ROLES, so under that layer the value defaults back togateway:creator. Maintainers need to decide which deployment layer owns the default-roles configuration and ensure the spoke registrar identity remains free of gateway permissions in whichever layer becomes canonical.
Findings Summary (ordered by severity, highest first)
- [Major]
RBAC_DEFAULT_ROLES: ""disables defaultgateway:creatorfor all users in the overlay, not just spokes - Spec Consistency / RBAC (deploy/openshift/kustomization.yaml:122) - [Minor] Registration
descriptionaccepted by schema but silently dropped - API Contract (service.go:159, handler.go:85) - [Minor] Response marshal error swallowed - Observability (handler.go:99)
- [Minor] First-time create event not re-emitted if event write fails - Reconciliation (service.go:196)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound / ErrRecordNotFound handled |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (K8s DNS label) | Pass |
| Reconcile / upsert pattern (not create-or-skip) | Pass |
| Proper context propagation | Pass |
| OpenAPI client regenerated, not hand-edited | Pass |
| Conventional commit messages | Pass |
| Test Diff Scrutiny (modified assertions) | Pass |
| # service accounts holding only managed-cluster-registrar receive no | ||
| # gateway permissions. Remove this line to re-enable defaults for | ||
| # human users if the Keycloak realm role mapping is not yet in place. | ||
| - name: RBAC_DEFAULT_ROLES |
There was a problem hiding this comment.
RBAC_DEFAULT_ROLES: "" returns nil from defaultRolesFromEnv() for every provisioned identity in this overlay, not only spokes - it also removes the default gateway:creator grant from human users added in commit 4e349e1 (HYPERSHELL-262). Since registration is JWT-direct and provisions no binding, prefer a narrower exclusion for registrar-only service accounts, or confirm this overlay is spoke-only and document the human-user impact.
| return managedClusters, nil | ||
| } | ||
|
|
||
| func (s *sqlManagedClusterService) Register(ctx context.Context, name, description, oidcSubject string) (*ManagedCluster, bool, *errors.ServiceError) { |
There was a problem hiding this comment.
The description parameter is never used in Register (the model has no Description field), yet the OpenAPI ManagedClusterRegistrationRequest advertises description and the handler forwards it. Clients that send description will have it silently discarded. Persist it or drop it from the schema so the contract matches behavior.
| w.Header().Set("Content-Type", "application/json") | ||
| w.Header().Set("Vary", "Authorization") | ||
| w.WriteHeader(status) | ||
| if payload, err := json.Marshal(resp); err == nil { |
There was a problem hiding this comment.
if payload, err := json.Marshal(resp); err == nil swallows a marshal failure after the status code is already written, returning a 200/201 with an empty body and no log line. Log the error so a serialization regression is observable.
| return nil, false, services.HandleCreateError("ManagedCluster", createErr) | ||
| } | ||
|
|
||
| _, evErr := s.events.Create(ctx, &api.Event{ |
There was a problem hiding this comment.
If events.Create fails here the row is already persisted, and the next 60s heartbeat takes the Replace branch (no event), so the create event may never be delivered to the control plane. Consider making create-event emission idempotent/retryable on the heartbeat path so a transient event-store error doesn't leave the cluster invisible to reconciliation.
2 participants
Summary
Implements the full stack for spoke control-plane self-registration per HYPERSHELL-326 spec.
POST /api/hypershell/v1/managed_clusters/registrationendpoint;ManagedClusterRegistrationRequest/Responseschemas;ManagedClustergainsoidc_subject(readOnly) andlast_seen_at(readOnly) fields(oidc_subject, name)with advisory locking; 201 on first call, 200 on heartbeat;last_seen_atupdated every call; 409 if same OIDC subject registers with a different namemanaged-cluster-registrarJWT-direct role - dedicated case inisAuthorizedbefore thehasGatewayCreatorfallback; not inJWTSyncedRoles, no DB binding lifecycleHYPERSHELL_MANAGED_CLUSTER_NAMEconfig;internal/registrationclient;registerWithBackoffat startup (403 exits immediately, other errors retry with exponential backoff); 60s heartbeat goroutineRegisterManagedClusteroperation and newManagedClusterfieldsTest plan
OIDCSubject→oidc_subject)POST /managed_clusters/registrationreturns 201 on first call, 200 on subsequent callsPOST /managed_clusters/registrationreturns 403 withoutmanaged-cluster-registrarJWT rolePOST /managed_clusters/registrationreturns 409 if same OIDC subject uses different nameHYPERSHELL_MANAGED_CLUSTER_NAMEset and self-registers🤖 Generated with Claude Code