fix(control-plane): reconcile existing Keycloak clients on gated gateways - #179
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: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
This hotfix correctly places an idempotent device-grant repair before the phase gate and has strong direct-handler coverage. It is not safe to merge yet because the production retry queue converts a Keycloak-only error into a full gateway reprovision, while lookup misses are reported as success and can permanently strand both deleted clients and clients hidden by a supported gateway rename.
Major
components/control-plane/internal/reconciler/reconciler.go:573-574returns lightweight Keycloak failures to a queue whose retry transform clears the gateway phase, so the retry bypasses this branch and executes the complete Kubernetes provisioning path. Preserve the gated phase for Keycloak-only retries and cover the real queue-to-handler interaction. Confidence: High (100%).components/control-plane/internal/reconciler/reconciler.go:740-747derives identity from mutableGateway.nameand treats every lookup miss as success; renamed and genuinely deleted clients therefore remain unconverged because no full reconciliation is scheduled for a Running gateway. Use an immutable/migrated client identity and explicitly restore or surface missing desired clients. Confidence: High (100%).
Verification
I reviewed the complete merge-base diff and the gateway queue, Keycloak client, API update, and OIDC persistence contracts it depends on. On GitHub's synthetic merge commit 0afd69e, go test -count=1 ./..., go vet ./..., and go test -race -count=1 ./internal/reconciler/... ./internal/keycloak/... all passed; completed policy, lint, image-build, and Konflux checks are green, with E2E Kind still running at review time.
Overall assessment: REQUEST_CHANGES
— Amber
Findings Summary (ordered by severity, highest first):
- [Major] Keycloak-only failures retry through the full gateway provisioning path - Reconciliation / Retry Semantics (L573-L574)
- [Major] Mutable-name and missing-client lookup misses are falsely reported as converged - Desired State / Identity (L740-L747)
Convention Checklist (omit conventions not applicable to the diff):
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs or error messages | Pass |
| Proper context propagation | Pass |
| Reconcile retries preserve the intended operation scope | Fail |
| Missing external desired state is propagated or reconciled | Fail |
| New behavior covered through its production queue path | Fail |
| Conventional commit message | Pass |
jsell-rh
added
amber/self-review
79437df to
786b351
Compare
…ted gateways When a Gateway is in Running, Provisioning, or Degraded phase, GatewayReconciler.Handle returns early to avoid redundant Kubernetes reprovisioning. However, external Keycloak client attributes (such as OAuth 2.0 Device Authorization Grant) can drift or be introduced in newer controller versions without advancing the gateway desired spec. Perform a lightweight, idempotent drift reconciliation on existing Keycloak clients before the early return without triggering a Kubernetes rollout. Propagate lookup/update errors to ensure queue retry, and leave missing clients to the full provisioning path to avoid out-of-band duplicate client creation.
786b351 to
509642f
Compare
Amber reviewStatus: Complete VerdictCOMMENT — This is a careful, well-documented hotfix that correctly reconciles Keycloak client drift on gated gateways without expanding into a full Kubernetes rollout; the error handling, input validation, and test coverage are strong. I found no blockers in the code itself, but there is a material cross-PR coordination issue with #151 (competing gate design on the exact same Amber Analysis Existing Running/Provisioning/Degraded gateways short-circuited before Keycloak reconciliation, so newer client attributes (the OAuth 2.0 Device Authorization Grant from #156) never converged on already-deployed gateways. This PR replaces the bare early-return with a lightweight, idempotent Keycloak drift pass that stays behind the Kubernetes reprovisioning gate, validates the persisted client identity across a trust boundary, publishes fixed status markers for missing/invalid clients, and adds a Confidence: High that the change is internally correct and convention-compliant. Medium on the two Minor items, which are design tradeoffs rather than defects. Strengths
Findings1. [Minor] Every gated reconcile now issues Keycloak API lookups ( 2. [Minor] Legacy rows without persisted OIDC identity fall back to the current (mutable) gateway name ( Cross-PR coordinationI reviewed the other open pull requests in Material conflicts requiring a maintainer decision:
No material conflict found with the remaining open PRs. #207 ( Findings Summary (ordered by severity, highest first):
Convention Checklist:
|
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT — This is a careful, well-documented hotfix that correctly reconciles Keycloak client drift on gated gateways without expanding into a full Kubernetes rollout; the error handling, input validation, and test coverage are strong. I found no blockers in the code itself, but there is a material cross-PR coordination issue with #151 (competing gate design on the exact same GatewayReconciler.Handle code path) that maintainers must resolve, plus two Minor observations below.
Amber Analysis
Existing Running/Provisioning/Degraded gateways short-circuited before Keycloak reconciliation, so newer client attributes (the OAuth 2.0 Device Authorization Grant from #156) never converged on already-deployed gateways. This PR replaces the bare early-return with a lightweight, idempotent Keycloak drift pass that stays behind the Kubernetes reprovisioning gate, validates the persisted client identity across a trust boundary, publishes fixed status markers for missing/invalid clients, and adds a PreservePayloadForRetry queue primitive so a transient Keycloak failure cannot be transformed into a full provisioning pass.
Confidence: High that the change is internally correct and convention-compliant. Medium on the two Minor items, which are design tradeoffs rather than defects.
Strengths
- Error wrapping is consistent (
fmt.Errorf("context: %w", err)) and errors are propagated for queue retry rather than swallowed. errors.As/errors.Isare used correctly to distinguish terminal validation failures (gatewayKeycloakClientIdentityError) from transient Keycloak errors and from the missing-client sentinel.- Strong input validation on a trust boundary:
containsControlCharacter, client_id/audience agreement, gateway-ownership check, and%q-quoted logging to prevent log injection — matchessecurity.spec.mdinput-validation and log-injection rules. - No
panic(), no secrets logged (client IDs are not secrets and are quoted), correct context propagation from the watch event. - Status is written on error paths (missing/invalid markers), and the health reconciler is updated so a healthy workload phase does not silently overwrite those markers — consistent with "healthy phase must not suppress drift repair."
- Test Diff Scrutiny: the only deletions in a pre-existing test file (
requeue_test.go) aregofmtstruct-field realignment plus a newpreserveOnFailfield; no existing assertion was weakened or flipped. All other test changes are additive.
Findings
1. [Minor] Every gated reconcile now issues Keycloak API lookups (reconciler.go:1401, reconciler.go:1619) — Observability / External load
Previously a Running/Provisioning/Degraded gateway returned with zero external calls. Now each such event calls reconcileExistingGatewayKeycloakClient, which performs a GetClientUUID lookup (and EnsureDeviceAuthorizationGrant, itself a GET + possible PUT). On controller startup/reconnect this is ~2×N Keycloak calls for N seeded gateways, plus one lookup per genuine gateway update event. The health reconciler only writes (and thus re-emits watch events) when phase/status actually change, so steady-state churn is bounded — but the seed burst and per-event cost are real. Consider bounding the drift check (e.g., only on reconnect/seed, or track a converged marker/generation so an already-converged client is not re-queried every event). Confidence: Medium.
2. [Minor] Legacy rows without persisted OIDC identity fall back to the current (mutable) gateway name (reconciler.go:1692) — Correctness edge
When neither client_id nor audience is persisted, existingGatewayKeycloakClientID recomputes {name}-{id} from gw.GetName(). The code comment correctly notes names are mutable, so for a legacy gateway that was renamed before client_id/audience were persisted, this yields the wrong client ID → reported as "Keycloak client is missing" with no self-recovery until a full provisioning pass. This is an acknowledged, spec-documented edge and not a regression, but worth confirming maintainers accept the "missing" outcome for renamed legacy rows rather than, say, also probing the historical name. Confidence: Medium.
Cross-PR coordination
I reviewed the other open pull requests in openshift-online/hypershell for logical, structural, and plan conflicts with #179. Open PRs at review time: #216, #214, #212, #211, #210, #209, #208, #207, #206, #201, #200, #194, #189, #188, #185, #182, #151, #150, #148, #135, #109, #75, #73.
Material conflicts requiring a maintainer decision:
-
#151 —
spec(control-plane): gate gateway re-provisioning on desired-state convergence(competing gate design + change ordering). Both PRs rewrite the same early-return gate insideGatewayReconciler.Handle. #179 keeps a phase-based gate (Phase == Running|Provisioning|Degraded) and performs Keycloak drift repair before returning; #151 replaces that gate entirely with a generation-convergence gate (observed_generation == generation) and addsobserved_generationto the proto/OpenAPI/data model. These are mutually exclusive gate implementations on one code path — whichever merges second must rebase and re-wire the Keycloak helper. #179's own body acknowledges this ("When PR #151 lands, the helper should be called inside that convergence-gate branch"). Maintainers should decide the merge order and confirm that the Keycloak drift pass still fires under #151's convergence gate (note: #151 migrates existing rows as converged, so those rows would skip the drift pass unless the helper is explicitly invoked in the convergence branch). This is the primary coordination item. -
#185 —
docs(control-plane): specify periodic world synchronization(overlapping Keycloak recovery mechanism). #185 specifies periodic, world-sync-based Keycloak role projection recovery for existing gateways and edits the sameopenshell-gateway-keycloak.spec.md. #179 introduces an event-driven, phase-gated Keycloak client attribute recovery inline inHandle. The two address different Keycloak facets (client roles vs. client attributes) and different trigger models (periodic vs. event-driven), and #185 states periodic sync "must not force Gateway phases or fight the health reconciler" — which is exactly the status-marker ownership #179 introduces. Maintainers should confirm these compose (who owns recovery of a missing/drifted existing client, and whether the world-sync pass should also drive the device-grant drift repair) rather than becoming two divergent recovery paths. -
#200 —
docs: define control plane reconciliation contract(design-consistency check). #200 defines a canonical level-based reconciliation contract with a shared driver providing keying, serialization, retry, and periodic resync, and explicitly updates the health spec so "a healthy phase does not suppress drift repair" — the same principle #179 implements in code. #179 instead adds a bespokePreservePayloadForRetryretry primitive to the existing per-key queue. This is alignment, not contradiction, but maintainers should verify #179's queue extension is compatible with (and ideally expressed in terms of) the contract #200 ratifies, so the contract does not immediately require reworking #179's retry semantics.
No material conflict found with the remaining open PRs. #207 (feat: reconcile-to-request trace correlation) also edits reconciler.go and health.go, but its changes are orthogonal span-link/trace-context additions — only a routine text merge, not a design conflict. #182 (JWT audience enforcement) operates on the API-server auth layer and does not compete with #179's read-side use of the persisted OIDC audience for client-identity derivation.
Findings Summary (ordered by severity, highest first):
- [Minor] Every gated reconcile now issues Keycloak API lookups; consider bounding the drift check to seed/reconnect or a converged marker - Observability / External load (L1401, L1619)
- [Minor] Legacy rows without persisted OIDC identity fall back to the mutable current name; a renamed legacy gateway is reported "missing" - Correctness edge (L1692)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound/missing handled for absent client |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (control chars, ownership, identity agreement) | Pass |
Log injection prevented (%q quoting) |
Pass |
| Reconcile pattern (not create-or-skip) | Pass |
| Status updated on error paths | Pass |
Proper context propagation (no context.TODO()) |
Pass |
| Test Diff Scrutiny (no weakened pre-existing assertions) | Pass |
| Spec updated alongside behavior change | Pass |
| // reconciling before the return below lets newly introduced client settings | ||
| // converge without forcing a full gateway rollout. | ||
| if gw.Phase != nil && (*gw.Phase == "Running" || *gw.Phase == "Provisioning" || *gw.Phase == "Degraded") { | ||
| if err := r.reconcileExistingGatewayKeycloakClient(ctx, event.ResourceID, gw); err != nil { |
There was a problem hiding this comment.
[Minor] Per-event Keycloak lookups on the gated path.
This now runs reconcileExistingGatewayKeycloakClient on every Running/Provisioning/Degraded event, where the code previously returned with zero external calls. Each pass does a GetClientUUID (and EnsureDeviceAuthorizationGrant, a GET + possible PUT). On controller startup/reconnect that is ~2×N Keycloak calls for N seeded gateways, plus one lookup per genuine gateway update event. Steady-state churn is bounded because the health reconciler only re-emits watch events when phase/status actually change, but consider bounding this drift check (only on reconnect/seed, or track a converged marker/generation) so an already-converged client is not re-queried on every event. Confidence: Medium.
| if gw.GetName() == "" { | ||
| return "", invalidGatewayKeycloakClientIdentity("gateway name is required for Keycloak reconciliation") | ||
| } | ||
| fallback := fmt.Sprintf("%s-%s", gw.GetName(), gatewayID) |
There was a problem hiding this comment.
[Minor] Legacy rows without persisted OIDC identity fall back to the mutable current name.
When neither client_id nor audience is persisted, the client ID is recomputed as {name}-{id} from the current gw.GetName(). As the comment above notes, names are mutable, so a legacy gateway renamed before client_id/audience were persisted will resolve to the wrong client ID → reported as Keycloak client is missing with no self-recovery until a full provisioning pass. This is a spec-documented edge and not a regression; flagging so maintainers confirm the missing outcome is acceptable for renamed legacy rows rather than also probing the historical name. Confidence: Medium.
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This is a well-scoped, carefully reasoned hotfix that lets existing gated gateways converge a newly introduced Keycloak client attribute without forcing a full Kubernetes rollout, and it comes with unusually thorough tests and a matching spec update. The core logic is correct and convention-compliant; my findings are non-blocking observations about long-run retry behaviour plus a required cross-PR coordination on the shared phase gate.
Summary
GatewayReconciler.Handle previously returned early for Running/Provisioning/Degraded gateways, so the Device Authorization Grant attribute from #156 never reached already-provisioned clients. This PR replaces that bare early-return with a lightweight, idempotent Keycloak-client drift reconcile that runs inside the phase gate, publishes fixed status markers for missing/invalid clients, and preserves the gated payload on retry so a transient Keycloak error cannot expand into full Kubernetes provisioning. The errors/stderrors alias cleanup, %q quoting for identity values, control-character validation, and the observedGatewayHealthUpdate change (which stops the health reconciler from stomping the new markers) all hang together coherently.
Strengths:
- Strong input validation on persisted identity: control-character rejection,
client_id/audienceagreement, gateway-ownership check before any Keycloak call. - Identity values are quoted (
%q) in every log/error, preventing Unicode/log-structure injection. - Terminal vs. transient error handling is deliberate: invalid identity is terminal (marker + stop), missing/lookup/update failures retry with preserved payload.
- Test coverage is genuinely additive (new
reconciler_test.go,health_test.go,requeue_test.gocases); no pre-existing assertion was flipped, so no removed guarantees.
Findings
[Minor] A missing Keycloak client on a gated Running gateway is surfaced but never automatically remediated. reconcileExistingGatewayKeycloakClient deliberately does not recreate a missing client (to avoid a partial client without RoleBindings/console mappings), and the phase gate blocks the full provisioning path for Running/Provisioning/Degraded. The result is an indefinite retry loop (maxRetries = -1) that publishes Keycloak client is missing and waits for an external actor. This is intentional and documented in the spec, but it means recovery from a genuinely deleted client requires operator action (or a phase reset) with no automated path. Worth confirming a maintainer is comfortable that "surface, never self-heal" is the desired long-term contract, or that the periodic/world-sync path is expected to own recreation. Confidence: High.
[Minor] Preserved retries reuse a stale payload snapshot, so the gw.GetStatus() guards can re-issue identical status writes each retry. On a preserved retry the queue replays the same event payload, so gw.GetStatus() still reflects the value at enqueue time, not the marker written by a prior attempt. For the missing-client branch, gw.GetStatus() != gatewayKeycloakClientMissingStatus can therefore stay true across retries and call updateGatewayStatus again each pass, each write emitting a watch event. The per-key backoff floor throttles the resulting self-events, but given the recent "stop Keycloak event storms" work this redundant write is worth avoiding — consider reading current status from the API (or tracking that the marker was already published) before re-writing. Confidence: Medium.
Cross-PR coordination
Another open pull request rewrites the same GatewayReconciler.Handle gate this PR restructures: it replaces the phase-based early return (Running/Provisioning/Degraded) with a desired-state convergence gate (observed_generation == generation) and adds the supporting proto/API/migration fields. This PR instead keeps the phase-based gate and hangs the lightweight Keycloak reconcile onto it. These are competing structural changes to the same decision point and cannot both land unchanged: maintainers must pick a merge order and decide where the Keycloak drift helper is invoked. The author already flagged the intended integration (call the helper inside the convergence-gate branch and re-express the handler fixtures as convergence rather than phase-only), so this needs an explicit ordering/ownership decision between the two authors — see PR #151.
A second open pull request specifies periodic control-plane world synchronization, including a Gateway drift-repair pass that "SHALL NOT overwrite status/phase owned by the Gateway health reconciler." That ownership model predates the new Keycloak client is missing / Keycloak client configuration is invalid status markers this PR introduces and asks the health reconciler to preserve. Because both efforts define who may write Gateway status for active gateways — and world-sync is a candidate owner for the very "recreate a missing client" gap noted above — maintainers should reconcile the status-ownership contract so the periodic sync does not clobber these markers (and to decide whether world-sync subsumes this event-driven repair) — see PR #185.
Findings Summary (ordered by severity, highest first)
- [Minor] Missing Keycloak client on a gated gateway is surfaced but never auto-remediated; recovery needs operator action - Reconciliation / Design (reconciler.go L1617)
- [Minor] Preserved retries reuse a stale payload snapshot, allowing repeated identical status writes / watch-event churn - Observability / Event storms (reconciler.go L1402)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context (%w) |
Pass |
errors.IsNotFound / missing handled |
Pass |
| No secrets in logs or error messages | Pass |
| Input validated (control chars, identity ownership) | Pass |
Log injection prevented (%q quoting) |
Pass |
| Status updated on error paths | Pass |
| Reconcile pattern (not create-or-skip) | Pass |
| Proper context propagation | Pass |
| Test Diff Scrutiny (no flipped pre-existing assertions) | Pass |
| Conventional commit message | Pass |
| return fmt.Errorf("check existing Keycloak client %q: %w", clientID, err) | ||
| } | ||
| if clientUUID == "" { | ||
| return fmt.Errorf("desired Keycloak client %q is missing: %w", clientID, errGatewayKeycloakClientMissing) |
There was a problem hiding this comment.
[Minor] A missing Keycloak client is reported (errGatewayKeycloakClientMissing) but never recreated here, and the phase gate blocks the full provisioning path for Running/Provisioning/Degraded gateways. With maxRetries = -1 this becomes an indefinite backoff loop that only publishes the Keycloak client is missing marker and waits for an external actor. This is intentional and spec-documented, but a genuinely deleted client on an active gateway then has no automated remediation path (operator or phase reset required). Please confirm this "surface, never self-heal" contract is the intended long-term behaviour, or that the periodic/world-sync path is expected to own recreation.
| } | ||
| return nil | ||
| } | ||
| if errors.Is(err, errGatewayKeycloakClientMissing) && gw.GetStatus() != gatewayKeycloakClientMissingStatus { |
There was a problem hiding this comment.
[Minor] On a preserved retry the queue replays the original event payload, so gw.GetStatus() reflects the value at enqueue time, not the marker a prior attempt just wrote. This guard can therefore stay true across retries and call updateGatewayStatus again on every pass, each write emitting a watch event. The per-key backoff floor throttles the self-events, but given the recent "stop Keycloak event storms" fix this redundant write is worth avoiding: consider reading current status from the API (or tracking that the marker was already published) before re-writing.
2 participants
Summary
Existing gateways in
Running,Provisioning, orDegradedphase return fromGatewayReconciler.Handlebeforegateway.ReconcileGatewayreaches Keycloak reconciliation. As a result, existing production clients can miss the OAuth 2.0 Device Authorization Grant attribute introduced by PR #156. Controller restart does not resolve this because startup seeding preserves the gateway phase and hits the same gate.This hotfix:
<gateway name>-<gateway resource ID>.EnsureDeviceAuthorizationGrantwhile preserving unrelated Keycloak representation fields and attributes.Tests
cd components/control-plane && go test -count=1 ./...cd components/control-plane && go test -race -count=1 ./internal/reconciler/...make lint-control-planemake checkAll passed.
PR #151 interaction
PR #151 changes the early-return condition to the convergence gate (
observed_generation == generation). Existing production rows are migrated as converged, so Keycloak drift still requires this lightweight pass. When PR #151 lands, the helper should be called inside that convergence-gate branch; the helper itself does not need to change. The handler regression fixtures should then represent convergence rather than phase-only gating.