Skip to content

GitOps hardening: console ingress mode, idempotent DB delete, JWT override warning, roles_claim validation - #218

Merged
markturansky merged 7 commits into
mainfrom
hypershell-gitops-hardening
Aug 28, 2026
Merged

GitOps hardening: console ingress mode, idempotent DB delete, JWT override warning, roles_claim validation#218
markturansky merged 7 commits into
mainfrom
hypershell-gitops-hardening

Conversation

@markturansky

Copy link
Copy Markdown
Collaborator

Summary

GitOps-hardening fixes surfaced while rolling Vault-backed OIDC + JWT/RBAC and the per-gateway Keycloak provisioner on route-mode ROSA clusters. Each item was validated against the code before fixing; every fix ships with tests.

Changes

  • P0-1 / P0-2 — Console ingress ignores GATEWAY_INGRESS_MODE (e309909)
    Console ingress now branches on the mode like the gateway path does: route → OpenShift Route (edge, redirect), gateway-apiHTTPRoute with a fail-fast guard on empty GATEWAY_API_GATEWAY_NAME, none → skip. Previously it unconditionally emitted an HTTPRoute with parentRef.name: "", which the API rejected forever in a 30s self-heal loop, leaving the console unservable on route-mode clusters.

  • P1-1 — ManagedDatabase NotFound during delete loops forever (a01a522)
    Gateway delete-reconcile now treats a NotFound from resolveDatabaseConfig as already-cleaned and finalizes the gateway. Any other resolve error still fails so transient conditions are retried. Previously the deleted-database case failed the whole reconcile, retried every 30s indefinitely.

  • P2-3 — API_ENV=development silently overrode --enable-jwt=true (31c977c)
    The dev environment still force-disables JWT, but now logs a loud WARN naming the overridden flag and pointing to API_ENV=development_oidc instead of silently turning every request into a 401.

  • P3-1 — roles_claim validation gap (5d0cd7c)
    roles_claim is now required (and format-validated as a dot-separated JWT claim path) whenever admin_role/user_role are set, catching dead-on-arrival gateways at config-validation time instead of at first login.

  • RBAC identity derivation (5263139) — carried on this branch.

Testing

  • go test ./... in components/control-plane and the relevant components/api-server packages — all green.
  • go build + go vet clean on both components.
  • New/extended tests: console ingress dispatch + readiness, idempotent DB-NotFound delete (in-process gRPC stub), dev JWT override warning, ValidateOIDCConfig table (10 subtests).

Notes / out of scope

  • P3-3 (hsctl delete verb) is already resolvedcomponents/cli is built as hsctl (Makefile:167) and already ships a full delete tree including hsctl delete gateway <id>. No change needed.
  • Heavier confirmed items (P1-2 CNPG namespace persistence, P1-3 version-skew health gate, P2-1/P2-2 keycloak secret watch, P2-4 auth-aware readiness, P3-4 e2e driver) are deferred to follow-ups.

🤖 Generated with Claude Code

user and others added 5 commits August 27, 2026 12:25
AuthorizeApi is attached on the parent apiV1Router, which in gorilla/mux
runs its .Use() middleware BEFORE the child subrouters. The child-level
AuthenticateAccountJWT is what populates the username context, so reading
auth.GetUsernameFromContext at this parent-router middleware always saw an
empty username and rejected every authenticated request with
"401 Unauthorized: missing identity" whenever RBAC_ENFORCE=true.

Derive identity via auth.GetAuthPayload(r) instead, exactly like the
sibling UserProvisioningMiddleware (also attached at the parent level).
On HTTP the framework's global jwtHandler has already validated the token
and placed it in the request context, so GetAuthPayload works at any router
depth while GetUsernameFromContext does not.

HTTP-only fix: the gRPC path is already correct. hypershell registers its
RBAC interceptors as post-auth (plugins/rbac/grpc_init.go) and the
framework's AuthUnaryInterceptor calls SetUsernameContext before post-auth
interceptors run, so GetUsernameFromContext is populated there.

Adds TestAuthorizeApiAllowsBoundUserFromJWTContext as a regression guard
and updates the existing conceal-mutations test to inject identity the way
the real HTTP flow does (validated token in context).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…0-1, P0-2)

The per-gateway console unconditionally emitted a Gateway API HTTPRoute whose
parentRef.name came from GATEWAY_API_GATEWAY_NAME. On a route-mode cluster that
var is unset, so the name rendered empty, the API server rejected the object,
and the self-heal loop retried every tick -- the console never became servable
under the managed Keycloak posture.

- reconcileConsoleIngress now follows the effective ingress mode: an OpenShift
  edge-terminated Route in route mode, the HTTPRoute in gateway-api mode, and
  nothing in none mode (mirrors the gateway ingress path).
- gateway-api mode fails fast when GATEWAY_API_GATEWAY_NAME is unset instead of
  submitting an invalid HTTPRoute (P0-2), mirroring reconcileGatewayAPIResources.
- console_address readiness (syncConsoleAddress) and the self-heal path now
  resolve mode too: ConsoleExposureReady dispatches to the HTTPRoute or the
  OpenShift Route accordingly, and selfHealConsole sets HasGatewayAPI so the
  console reconcile picks the same mode the provisioning path would.
- deleteConsole sweeps both ingress objects regardless of mode.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ete as idempotent (P1-1)

A gateway delete-reconcile resolves the gateway's ManagedDatabase to obtain
teardown config. When the ManagedDatabase was already deleted, GetManagedDatabase
returns NotFound, which was appended to deleteErrs and failed the whole
delete-reconcile. The watcher then retried every 30s forever, because the
ManagedDatabase never comes back.

Treat NotFound as already-cleaned and continue finalizing the gateway; any other
resolve error still fails so a transient condition is retried.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…=true (P2-3)

The development environment's OverrideConfig runs after CLI flags are parsed and
force-disabled Auth.EnableJWT with no signal, so --enable-jwt=true silently
turned into 401 'missing identity' on every request. Log a loud warning naming
the overridden flag and pointing to API_ENV=development_oidc before applying the
override.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…apping is set (P3-1)

A gateway with admin_role/user_role but no roles_claim can never map a token to a
role, producing a dead-on-arrival gateway (the default roles_claim=groups is
absent on realms that emit roles under 'roles'/'realm_access.roles'). Reject that
combination at config-validation time and validate that any roles_claim is a
well-formed dot-separated JWT claim path.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown

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: Pro Plus

Run ID: 6d89ec91-119c-4ba5-abdf-60e30610e91e

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

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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

@jsell-rh

jsell-rh commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Amber review: changes requested

Amber review

Status: Complete

Verdict

REQUEST_CHANGES. The three genuinely net-new fixes (idempotent DB-NotFound delete, dev JWT override warning, roles_claim validation) are well-reasoned and well-tested, but this branch is cut from a stale base and re-implements two changes that are already on main with a different design, so it will not merge cleanly and needs a rebase plus one open question on the roles_claim tightening.

Amber Analysis

This PR bundles four GitOps-hardening fixes. Two of them (console OpenShift Route ingress + readiness, and RBAC identity derivation from the JWT payload) have already landed on main since this branch's base commit (b9f05c7), so a large part of the diff is duplicated and, in the console case, diverges behaviorally from the merged implementation. The remaining three fixes are solid, with one optional -> required validation change that needs a fallback decision.

Blocking / structural

1. [Major] Stale base — console ingress + RBAC identity are already merged on main, with a divergent design.
This branch's merge-base is b9f05c7. main is now at 73d12be and already includes the RBAC identity fix (#215, AuthorizeApi reading the JWT payload) and the console OpenShift Route ingress support (#216). main's console.go already defines ConsoleExposureReady (branching on ingress mode), consoleOpenShiftRouteReady, consoleHTTPRouteReady, and a reconcileConsole that branches ingress creation on gatewayIngressMode(opts) (including IngressModeNone). This PR re-introduces the same behavior under different names (ConsoleOpenShiftRouteReady, ConsoleRouteReady, consoleRouteGVR, reconcileConsoleIngress, buildConsoleRoute) and re-applies the identical authorization.go change. As-is the branch will conflict on rebase and duplicates merged work. Please rebase onto current main, drop the console-ingress and authorization.go changes that are now redundant, and keep only the net-new P1-1 / P2-3 / P3-1 fixes.

2. [Major] ConsoleExposureReady treats unknown / "none" ingress mode as ready, diverging from the merged implementation.
In this branch, ConsoleExposureReady's default case returns (true, "", nil). The merged version on main returns false for IngressModeNone ("no console ingress mode selected") and an error for an unsupported mode. Returning ready when there is no managed console ingress lets syncConsoleAddress publish console_address against a host that has no Route/HTTPRoute — exactly the dead-link failure this PR set out to eliminate. This concern disappears once you adopt main's implementation (finding 1), which is the recommended path.

3. [Major] roles_claim moves from optional to required with no fallback for pre-existing BYO configs.
ValidateOIDCConfig now rejects any config where admin_role/user_role are set but roles_claim is empty, and ValidateGatewayConfig runs on the reconcile path (reconciler.go:67). manifests.go:164-165 only emits roles_claim when non-empty, so a blank value previously delegated to the gateway's own default. Managed gateways are unaffected because the provisioning path always sets RolesClaim: "hypershell.roles" (reconciler.go:1585). But a custom/BYO OIDC config that set admin_role/user_role and left roles_claim blank — relying on the OpenShell gateway's default claim — would now fail validation on the next reconcile with no migration or fallback. Please confirm whether the gateway defaults roles_claim (e.g. to roles) when the key is omitted. If it does, default RolesClaim to that value instead of erroring; if it does not, the "dead on arrival" justification holds and this is fine as written. This is the optional -> required scrutiny case (records could predate the change), so it needs an explicit answer before merge.

What's good (keep)

  • P1-1 (idempotent DB-NotFound delete) is correct. resolveDatabaseConfig wraps the gRPC error with %w, and status.Code/status.FromError under grpc v1.83.0 unwraps via errors.As, so status.Code(dbErr) == codes.NotFound matches; the in-process gRPC test exercises the real path and proves it. The NotFound carve-out is correctly scoped so transient errors still fail and retry.
  • P2-3 (dev JWT override warning) is clean: the override behavior is unchanged, the warning is only emitted when --enable-jwt=true was explicitly requested, the helper is extracted for testability, and no secrets are logged.
  • Tests are additive and individually justified — no pre-existing assertion was flipped from accept to reject.

Cross-PR coordination

No material cross-PR coordination issue requires maintainer action.


Findings Summary (ordered by severity, highest first):

  1. [Major] Stale base — console ingress + RBAC identity already merged on main; rebase and drop duplicates - Structural / Spec Consistency (console.go, authorization.go)
  2. [Major] ConsoleExposureReady returns ready for unknown/"none" mode, diverging from merged design; can publish a dead console link - Reconciliation (console.go L118-125)
  3. [Major] roles_claim optional -> required with no fallback for BYO configs; confirm gateway default - Test Diff Scrutiny (validation.go L176-188)
  4. [Minor] Confusing readiness naming: ConsoleRouteReady checks the HTTPRoute, not an OpenShift Route - Convention (console.go L136)

Convention Checklist:

Convention Result
No panic() in production code Pass
Errors wrapped with fmt.Errorf context Pass
errors.IsNotFound/gRPC NotFound handled Pass
No secrets in logs or responses Pass
Input validated (roles_claim / JWT claim path) Pass
Reconcile pattern (delete idempotency) Pass
Test Diff Scrutiny (optional -> required) Needs confirmation
Branch current with main / no duplicated merged work Fail

@jsell-rh jsell-rh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict

REQUEST_CHANGES. The three genuinely net-new fixes (idempotent DB-NotFound delete, dev JWT override warning, roles_claim validation) are well-reasoned and well-tested, but this branch is cut from a stale base and re-implements two changes that are already on main with a different design, so it will not merge cleanly and needs a rebase plus one open question on the roles_claim tightening.

Amber Analysis

This PR bundles four GitOps-hardening fixes. Two of them (console OpenShift Route ingress + readiness, and RBAC identity derivation from the JWT payload) have already landed on main since this branch's base commit (b9f05c7), so a large part of the diff is duplicated and, in the console case, diverges behaviorally from the merged implementation. The remaining three fixes are solid, with one optional -> required validation change that needs a fallback decision.

Blocking / structural

1. [Major] Stale base — console ingress + RBAC identity are already merged on main, with a divergent design.
This branch's merge-base is b9f05c7. main is now at 73d12be and already includes the RBAC identity fix (#215, AuthorizeApi reading the JWT payload) and the console OpenShift Route ingress support (#216). main's console.go already defines ConsoleExposureReady (branching on ingress mode), consoleOpenShiftRouteReady, consoleHTTPRouteReady, and a reconcileConsole that branches ingress creation on gatewayIngressMode(opts) (including IngressModeNone). This PR re-introduces the same behavior under different names (ConsoleOpenShiftRouteReady, ConsoleRouteReady, consoleRouteGVR, reconcileConsoleIngress, buildConsoleRoute) and re-applies the identical authorization.go change. As-is the branch will conflict on rebase and duplicates merged work. Please rebase onto current main, drop the console-ingress and authorization.go changes that are now redundant, and keep only the net-new P1-1 / P2-3 / P3-1 fixes.

2. [Major] ConsoleExposureReady treats unknown / "none" ingress mode as ready, diverging from the merged implementation.
In this branch, ConsoleExposureReady's default case returns (true, "", nil). The merged version on main returns false for IngressModeNone ("no console ingress mode selected") and an error for an unsupported mode. Returning ready when there is no managed console ingress lets syncConsoleAddress publish console_address against a host that has no Route/HTTPRoute — exactly the dead-link failure this PR set out to eliminate. This concern disappears once you adopt main's implementation (finding 1), which is the recommended path.

3. [Major] roles_claim moves from optional to required with no fallback for pre-existing BYO configs.
ValidateOIDCConfig now rejects any config where admin_role/user_role are set but roles_claim is empty, and ValidateGatewayConfig runs on the reconcile path (reconciler.go:67). manifests.go:164-165 only emits roles_claim when non-empty, so a blank value previously delegated to the gateway's own default. Managed gateways are unaffected because the provisioning path always sets RolesClaim: "hypershell.roles" (reconciler.go:1585). But a custom/BYO OIDC config that set admin_role/user_role and left roles_claim blank — relying on the OpenShell gateway's default claim — would now fail validation on the next reconcile with no migration or fallback. Please confirm whether the gateway defaults roles_claim (e.g. to roles) when the key is omitted. If it does, default RolesClaim to that value instead of erroring; if it does not, the "dead on arrival" justification holds and this is fine as written. This is the optional -> required scrutiny case (records could predate the change), so it needs an explicit answer before merge.

What's good (keep)

  • P1-1 (idempotent DB-NotFound delete) is correct. resolveDatabaseConfig wraps the gRPC error with %w, and status.Code/status.FromError under grpc v1.83.0 unwraps via errors.As, so status.Code(dbErr) == codes.NotFound matches; the in-process gRPC test exercises the real path and proves it. The NotFound carve-out is correctly scoped so transient errors still fail and retry.
  • P2-3 (dev JWT override warning) is clean: the override behavior is unchanged, the warning is only emitted when --enable-jwt=true was explicitly requested, the helper is extracted for testability, and no secrets are logged.
  • Tests are additive and individually justified — no pre-existing assertion was flipped from accept to reject.

Cross-PR coordination

No material cross-PR coordination issue requires maintainer action.


Findings Summary (ordered by severity, highest first):

  1. [Major] Stale base — console ingress + RBAC identity already merged on main; rebase and drop duplicates - Structural / Spec Consistency (console.go, authorization.go)
  2. [Major] ConsoleExposureReady returns ready for unknown/"none" mode, diverging from merged design; can publish a dead console link - Reconciliation (console.go L118-125)
  3. [Major] roles_claim optional -> required with no fallback for BYO configs; confirm gateway default - Test Diff Scrutiny (validation.go L176-188)
  4. [Minor] Confusing readiness naming: ConsoleRouteReady checks the HTTPRoute, not an OpenShift Route - Convention (console.go L136)

Convention Checklist:

Convention Result
No panic() in production code Pass
Errors wrapped with fmt.Errorf context Pass
errors.IsNotFound/gRPC NotFound handled Pass
No secrets in logs or responses Pass
Input validated (roles_claim / JWT claim path) Pass
Reconcile pattern (delete idempotency) Pass
Test Diff Scrutiny (optional -> required) Needs confirmation
Branch current with main / no duplicated merged work Fail

case IngressModeGatewayAPI:
return ConsoleRouteReady(ctx, dynamicClient, namespace)
default:
return true, "", nil

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] Divergent readiness for unknown/"none" ingress mode.

The default case returns (true, "", nil), so any mode that isn't route/gateway-api (including IngressModeNone) is reported ready. syncConsoleAddress then publishes console_address even though no console Route/HTTPRoute exists — the dead-link case this PR is trying to prevent.

The version already merged on main (#216) returns false for IngressModeNone and errors on an unsupported mode. Rebasing onto main and adopting that implementation resolves this — this whole function is now duplicated there.

// the gateway is dead on arrival (P3-1). The claim path itself must be a valid
// JWT claim reference so it can locate the roles in the token.
if rolesMapped && oidc.RolesClaim == "" {
return fmt.Errorf("roles_claim is required when admin_role/user_role are set; " +

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] optional -> required without a fallback for pre-existing BYO configs.

roles_claim is now required whenever admin_role/user_role are set, and ValidateGatewayConfig runs on the reconcile path (reconciler.go:67). manifests.go:164-165 only emits roles_claim when non-empty, so a blank value previously delegated to the gateway's own default. Managed gateways are unaffected (reconciler.go:1585 always sets hypershell.roles), but a custom OIDC config that set the roles but left roles_claim blank — relying on the gateway default — would now fail on its next reconcile with no migration/fallback.

Please confirm whether the OpenShell gateway defaults roles_claim (e.g. to roles) when the key is omitted. If it does, default RolesClaim to that value here instead of erroring; if it does not, the "dead on arrival" justification holds and this is fine. Needs an explicit answer before merge.

…dening

# Conflicts:
#	components/control-plane/internal/gateway/console.go
#	components/control-plane/internal/reconciler/health.go
#	components/control-plane/internal/reconciler/reconciler.go
@jsell-rh

jsell-rh commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Amber review: comment

Amber review

Status: Complete

Verdict

Three well-scoped GitOps-hardening fixes (idempotent DB-NotFound delete, dev-JWT override warning, roles_claim validation), each shipped with focused tests and clear WHY-comments. The code is correct and convention-compliant; I have only minor, non-blocking observations plus a documentation nudge for the optional->required roles_claim tightening.

Amber Analysis

P1-1 — idempotent ManagedDatabase NotFound on delete (reconciler.go)
Correct. On NotFound, deleteDBConfig stays zero-valued, so the later deleteDBConfig.Provider == "deployment" branch is skipped and no phantom deployment-DB delete is attempted — the right behavior when the record is already gone. status.Code(dbErr) is called on the error returned by resolveDatabaseConfig, which wraps the gRPC error with %w; this works because grpc-go's status.FromError unwraps the chain, and the new TestGatewayDeleteWithAlreadyDeletedDatabaseIsIdempotent exercises exactly that wrapped path. The companion ...ResolveErrorStillFails test keeps the carve-out scoped to NotFound. Good aggregation via stderrors.Join.

P2-3 — dev JWT override warning (e_development.go)
Good. glog is already the api-server's logger (main.go, pkg/rbac/*), so this is consistent. Extracting devJWTOverrideWarning for testability is a nice touch, and the message names the overridden flag and points to development_oidc. No secrets logged.

P3-1 — roles_claim validation (validation.go)
Solid. The regex is linear (no ReDoS), messages are actionable, and error strings carry no secrets. See the inline note on the optional->required tightening.

Test Diff Scrutiny: validation_test.go is purely additive (new TestValidateOIDCConfig, 0 deletions) — no pre-existing assertion was flipped. e_development_test.go is new. No removed guarantees.

Cross-PR coordination

No material cross-PR coordination issue requires maintainer action.

Findings Summary (ordered by severity, highest first)

  1. [Minor] roles_claim moves from optional to required when admin_role/user_role are set; the managed Keycloak path always supplies it, but manually-authored OIDC configs that predate this rule will now fail validation — worth calling out in the PR/spec — Test Diff Scrutiny / Spec Completeness (validation.go L182)
  2. [Minor] NotFound idempotency relies on status.Code unwrapping the %w-wrapped error from resolveDatabaseConfig; a one-line note (or asserting on the wrapped error) would protect the behavior against future refactors — Maintainability (reconciler.go L1335)

Convention Checklist

Convention Result
No panic() in production code Pass
Errors wrapped with fmt.Errorf context Pass
errors.IsNotFound/gRPC NotFound handled Pass
No secrets in logs or error messages Pass
Input validated (JWT claim path) Pass
Status/error paths propagate (no swallowed failures) Pass
Reconcile idempotency on delete Pass
Test diff adds coverage without flipping guarantees Pass
Conventional commit messages Pass

@jsell-rh jsell-rh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict

Three well-scoped GitOps-hardening fixes (idempotent DB-NotFound delete, dev-JWT override warning, roles_claim validation), each shipped with focused tests and clear WHY-comments. The code is correct and convention-compliant; I have only minor, non-blocking observations plus a documentation nudge for the optional->required roles_claim tightening.

Amber Analysis

P1-1 — idempotent ManagedDatabase NotFound on delete (reconciler.go)
Correct. On NotFound, deleteDBConfig stays zero-valued, so the later deleteDBConfig.Provider == "deployment" branch is skipped and no phantom deployment-DB delete is attempted — the right behavior when the record is already gone. status.Code(dbErr) is called on the error returned by resolveDatabaseConfig, which wraps the gRPC error with %w; this works because grpc-go's status.FromError unwraps the chain, and the new TestGatewayDeleteWithAlreadyDeletedDatabaseIsIdempotent exercises exactly that wrapped path. The companion ...ResolveErrorStillFails test keeps the carve-out scoped to NotFound. Good aggregation via stderrors.Join.

P2-3 — dev JWT override warning (e_development.go)
Good. glog is already the api-server's logger (main.go, pkg/rbac/*), so this is consistent. Extracting devJWTOverrideWarning for testability is a nice touch, and the message names the overridden flag and points to development_oidc. No secrets logged.

P3-1 — roles_claim validation (validation.go)
Solid. The regex is linear (no ReDoS), messages are actionable, and error strings carry no secrets. See the inline note on the optional->required tightening.

Test Diff Scrutiny: validation_test.go is purely additive (new TestValidateOIDCConfig, 0 deletions) — no pre-existing assertion was flipped. e_development_test.go is new. No removed guarantees.

Cross-PR coordination

No material cross-PR coordination issue requires maintainer action.

Findings Summary (ordered by severity, highest first)

  1. [Minor] roles_claim moves from optional to required when admin_role/user_role are set; the managed Keycloak path always supplies it, but manually-authored OIDC configs that predate this rule will now fail validation — worth calling out in the PR/spec — Test Diff Scrutiny / Spec Completeness (validation.go L182)
  2. [Minor] NotFound idempotency relies on status.Code unwrapping the %w-wrapped error from resolveDatabaseConfig; a one-line note (or asserting on the wrapped error) would protect the behavior against future refactors — Maintainability (reconciler.go L1335)

Convention Checklist

Convention Result
No panic() in production code Pass
Errors wrapped with fmt.Errorf context Pass
errors.IsNotFound/gRPC NotFound handled Pass
No secrets in logs or error messages Pass
Input validated (JWT claim path) Pass
Status/error paths propagate (no swallowed failures) Pass
Reconcile idempotency on delete Pass
Test diff adds coverage without flipping guarantees Pass
Conventional commit messages Pass

// without a claim to read roles from, no token can ever map to admin/user and
// the gateway is dead on arrival (P3-1). The claim path itself must be a valid
// JWT claim reference so it can locate the roles in the token.
if rolesMapped && oidc.RolesClaim == "" {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Minor] optional -> required tightening. Requiring roles_claim whenever admin_role/user_role are set is the right call for catching dead-on-arrival gateways. The managed Keycloak path (reconciler.go sets RolesClaim: "hypershell.roles" alongside the roles) always supplies it, so the default deployment flow is safe. The one gap: a manually-authored OIDC config persisted before this rule that has roles set but no roles_claim will now fail ValidateGatewayConfig on the next reconcile. Such a config never actually mapped roles, so failing loudly is an improvement rather than a regression — but please call this out explicitly in the PR body/spec so operators expect the new rejection instead of being surprised by it.

// deleteErrs instead would fail the whole delete-reconcile, which the
// watcher retries every 30s -- forever, because the ManagedDatabase
// never comes back. Any other error still fails so it is retried.
if status.Code(dbErr) == codes.NotFound {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Minor] correctness note. This branch depends on status.Code(dbErr) seeing NotFound even though resolveDatabaseConfig returns fmt.Errorf("resolve ManagedDatabase %s: %w", ...). It works today because grpc-go's status.FromError unwraps the %w chain, and the new idempotency test covers the wrapped path. To keep this robust against future refactors of the error wrapping, consider a short comment here (or asserting on the wrapped-error form) so a later change that swaps %w for a non-unwrapping wrapper doesn't silently turn the delete back into an infinite retry. Also worth noting for the record: on NotFound, deleteDBConfig stays zero-valued, so the later Provider == "deployment" delete is correctly skipped — good.

…cile

Addresses Amber Finding 3 on #218. The P3-1 change made roles_claim a hard
requirement whenever admin_role/user_role are set. Because ValidateGatewayConfig
runs on the reconcile path (ReconcileGateway), a pre-existing BYO OIDC config
that set the roles but omitted roles_claim -- relying on the gateway's own
default (groups) -- would start failing to reconcile with no migration.

Drop the required check and keep only the format validation for a non-empty
roles_claim. A blank value continues to delegate to the gateway default, so
manifests.go emits nothing and behavior is unchanged for existing gateways.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@markturansky

Copy link
Copy Markdown
Collaborator Author

Thanks for the review. Status on the four findings:

Finding 1 (stale base — duplicate console/RBAC + redundant authorization.go): Resolved by merging main (#216) into this branch. There are no duplicate console symbols left, deploy/base/controller-rbac.yaml has the single routes + routes/custom-host rules from #216, and pkg/rbac/authorization.go is now byte-identical to main (my redundant change collapsed in the merge).

Finding 2 (ConsoleExposureReady default returned ready): Resolved by the merge — the file is now #216's version. ConsoleExposureReady returns false for IngressModeNone and an error for the default/unknown case; it never reports ready on an unrecognized mode.

Finding 3 (roles_claim optional→required, no BYO fallback): Good catch — fixed in 2fb845c. Confirmed the OpenShell gateway does default roles_claim (to groups), and ValidateGatewayConfig runs on the reconcile path, so a hard-error would have broken pre-existing BYO gateways that set the roles but omitted roles_claim. I dropped the required check and kept only the format validation for a non-empty value; a blank value continues to delegate to the gateway default (manifests.go emits nothing), so behavior is unchanged for existing configs. Test roles mapped without roles_claim ... flipped to wantErr: false accordingly.

Finding 4 (ConsoleRouteReady naming): Resolved by the merge — the symbol is now ConsoleExposureReady from #216, covering both gateway-api and route modes.

@jsell-rh

jsell-rh commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Amber review: comment

Amber review

Status: Complete

Verdict

Three tightly-scoped, well-tested bug fixes (idempotent DB-NotFound delete, dev JWT-override warning, roles_claim format validation) that all follow HyperShell conventions and introduce no blockers. My only substantive concerns are that the PR description overstates the actual net diff and one adjacent defense-in-depth gap; the code itself is safe to merge.

Amber Analysis

The net diff against main is 6 files: the delete-path idempotency fix, the dev JWT warning, and roles_claim format validation, each with tests. The changes are correct and idiomatic. Notably, the branch handles the classic optional→required trap well: the final commit (2fb845c) deliberately reverses P3-1 from "require roles_claim" to "default a blank roles_claim to the gateway's own default," so no pre-existing OIDC config is broken on the reconcile path. The new TestValidateOIDCConfig is additive and no pre-existing test assertion was flipped.

Findings

[Minor] PR description overstates scope — P0-1/P0-2 console ingress is not in this diff (Docs consistency, High confidence)
The body lists console-ingress mode branching (e309909) as a change of this PR, but the net diff against main contains no console/manifests changes — that work is already on main (it was merged and then merged back into this branch). Reviewers and the eventual merger should not expect console behavior changes here. Please trim the body to match the actual diff.

[Minor] PR description contradicts the shipped roles_claim behavior (Docs / Test Diff Scrutiny, High confidence)
The body says "roles_claim is now required ... whenever admin_role/user_role are set." The shipped code (validation.go:184) does the opposite and correctly so: a blank roles_claim is delegated to the gateway default and only a non-empty value is format-validated. The description should be updated to describe the backward-safe behavior that actually merged, otherwise a future reader may "fix" it back into a breaking required-field.

[Minor] Defense-in-depth: sibling OIDC fields are still interpolated into TOML unvalidated (Security, Medium confidence)
roles_claim is now format-validated (claimPathRegex) before it is written into the gateway TOML, which closes a config-injection surface for that field. The adjacent fields rendered in manifests.goissuer, audience, admin_role, user_role, scopes_claim — are still fmt.Sprintf-ed into quoted TOML strings with no format validation, so a value containing a quote/newline could still break or inject into the rendered config. Out of this PR's strict scope, but worth a follow-up to apply the same validation posture to the siblings.

What I verified

  • Delete idempotency is correctly scoped. resolveDatabaseConfig wraps the gRPC error with %w; status.Code(dbErr) (grpc v1.83) unwraps via errors.As, so the codes.NotFound check matches the wrapped status. The companion TestGatewayDeleteDatabaseResolveErrorStillFails proves a non-NotFound (Unavailable) error still fails the reconcile so transient conditions are retried. Good.
  • Dev JWT warning fires only when --enable-jwt=true was explicitly requested, names the flag, and points to development_oidc; the override itself is unchanged and still covered by TestDevOverrideConfigDisablesJWT.
  • No secrets logged; the delete-path log uses only the resource IDs.

Cross-PR coordination

No material cross-PR coordination issue requires maintainer action.


Findings Summary (ordered by severity, highest first):

  1. [Minor] PR description claims console-ingress (P0-1/P0-2) changes that are not in the net diff - Docs consistency (body)
  2. [Minor] PR description says roles_claim is "required" but shipped code makes it optional/format-only - Docs / Test Diff Scrutiny (L184)
  3. [Minor] Sibling OIDC fields (issuer/audience/admin_role/user_role/scopes_claim) rendered into TOML without format validation - Security (defense-in-depth) (manifests.go)

Convention Checklist:

Convention Result
No panic() in production code Pass
Errors wrapped with fmt.Errorf context Pass
gRPC/IsNotFound handled for 404 scenarios Pass
No secrets in logs or responses Pass
Input validated (roles_claim claim path) Pass
Reconcile idempotency (delete path) Pass
Context propagation (stream ctx) Pass
Test Diff Scrutiny (no flipped assertions; additive tests) Pass
Conventional commit messages Pass

@jsell-rh jsell-rh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict

Three tightly-scoped, well-tested bug fixes (idempotent DB-NotFound delete, dev JWT-override warning, roles_claim format validation) that all follow HyperShell conventions and introduce no blockers. My only substantive concerns are that the PR description overstates the actual net diff and one adjacent defense-in-depth gap; the code itself is safe to merge.

Amber Analysis

The net diff against main is 6 files: the delete-path idempotency fix, the dev JWT warning, and roles_claim format validation, each with tests. The changes are correct and idiomatic. Notably, the branch handles the classic optional→required trap well: the final commit (2fb845c) deliberately reverses P3-1 from "require roles_claim" to "default a blank roles_claim to the gateway's own default," so no pre-existing OIDC config is broken on the reconcile path. The new TestValidateOIDCConfig is additive and no pre-existing test assertion was flipped.

Findings

[Minor] PR description overstates scope — P0-1/P0-2 console ingress is not in this diff (Docs consistency, High confidence)
The body lists console-ingress mode branching (e309909) as a change of this PR, but the net diff against main contains no console/manifests changes — that work is already on main (it was merged and then merged back into this branch). Reviewers and the eventual merger should not expect console behavior changes here. Please trim the body to match the actual diff.

[Minor] PR description contradicts the shipped roles_claim behavior (Docs / Test Diff Scrutiny, High confidence)
The body says "roles_claim is now required ... whenever admin_role/user_role are set." The shipped code (validation.go:184) does the opposite and correctly so: a blank roles_claim is delegated to the gateway default and only a non-empty value is format-validated. The description should be updated to describe the backward-safe behavior that actually merged, otherwise a future reader may "fix" it back into a breaking required-field.

[Minor] Defense-in-depth: sibling OIDC fields are still interpolated into TOML unvalidated (Security, Medium confidence)
roles_claim is now format-validated (claimPathRegex) before it is written into the gateway TOML, which closes a config-injection surface for that field. The adjacent fields rendered in manifests.goissuer, audience, admin_role, user_role, scopes_claim — are still fmt.Sprintf-ed into quoted TOML strings with no format validation, so a value containing a quote/newline could still break or inject into the rendered config. Out of this PR's strict scope, but worth a follow-up to apply the same validation posture to the siblings.

What I verified

  • Delete idempotency is correctly scoped. resolveDatabaseConfig wraps the gRPC error with %w; status.Code(dbErr) (grpc v1.83) unwraps via errors.As, so the codes.NotFound check matches the wrapped status. The companion TestGatewayDeleteDatabaseResolveErrorStillFails proves a non-NotFound (Unavailable) error still fails the reconcile so transient conditions are retried. Good.
  • Dev JWT warning fires only when --enable-jwt=true was explicitly requested, names the flag, and points to development_oidc; the override itself is unchanged and still covered by TestDevOverrideConfigDisablesJWT.
  • No secrets logged; the delete-path log uses only the resource IDs.

Cross-PR coordination

No material cross-PR coordination issue requires maintainer action.


Findings Summary (ordered by severity, highest first):

  1. [Minor] PR description claims console-ingress (P0-1/P0-2) changes that are not in the net diff - Docs consistency (body)
  2. [Minor] PR description says roles_claim is "required" but shipped code makes it optional/format-only - Docs / Test Diff Scrutiny (L184)
  3. [Minor] Sibling OIDC fields (issuer/audience/admin_role/user_role/scopes_claim) rendered into TOML without format validation - Security (defense-in-depth) (manifests.go)

Convention Checklist:

Convention Result
No panic() in production code Pass
Errors wrapped with fmt.Errorf context Pass
gRPC/IsNotFound handled for 404 scenarios Pass
No secrets in logs or responses Pass
Input validated (roles_claim claim path) Pass
Reconcile idempotency (delete path) Pass
Context propagation (stream ctx) Pass
Test Diff Scrutiny (no flipped assertions; additive tests) Pass
Conventional commit messages Pass

// its reconciliation with no migration. A claim the realm never emits still
// can't be caught here without the token (P3-1); this only rejects a
// syntactically invalid claim path.
if oidc.RolesClaim != "" && !claimPathRegex.MatchString(oidc.RolesClaim) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct call to make this optional rather than required — a blank roles_claim delegates to the gateway default, so pre-existing BYO OIDC configs keep reconciling. This closes the optional→required trap.

Two notes:

  • The PR body still says roles_claim is "now required whenever admin_role/user_role are set," which is the opposite of what shipped here. Please update the description so a future reader doesn't reintroduce the breaking required-field.
  • This regex now guards roles_claim against TOML injection when it is interpolated in manifests.go. The sibling OIDC fields there (issuer, audience, admin_role, user_role, scopes_claim) are still written into quoted TOML strings without format validation — consider a follow-up to apply the same posture.

// deleteErrs instead would fail the whole delete-reconcile, which the
// watcher retries every 30s -- forever, because the ManagedDatabase
// never comes back. Any other error still fails so it is retried.
if status.Code(dbErr) == codes.NotFound {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified this works with the wrapped error: resolveDatabaseConfig returns fmt.Errorf("resolve ManagedDatabase %s: %w", ...), and status.Code (grpc v1.83) unwraps via errors.As, so the codes.NotFound branch matches. The companion test also confirms a non-NotFound (Unavailable) error still fails so the watcher retries. Nice, idempotent and correctly scoped.

@markturansky
markturansky enabled auto-merge August 28, 2026 03:33
@markturansky
markturansky added this pull request to the merge queue Aug 28, 2026
Merged via the queue into main with commit 04d3322 Aug 28, 2026
19 of 20 checks passed
@markturansky
markturansky deleted the hypershell-gitops-hardening branch August 28, 2026 03:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants