fix(control-plane): stop Keycloak event storms - #230
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 |
Amber reviewStatus: Complete VerdictAPPROVE. This is a tight, well-scoped fix for two real Keycloak event-storm root causes (token-lifetime unit bug and an over-strict service-account convergence predicate), backed by clear regression tests and matching spec updates. I found no blocking, critical, or major issues. SummaryThe Strengths
Minor
Cross-PR coordinationNo material cross-PR coordination issue requires maintainer action. 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
APPROVE. This is a tight, well-scoped fix for two real Keycloak event-storm root causes (token-lifetime unit bug and an over-strict service-account convergence predicate), backed by clear regression tests and matching spec updates. I found no blocking, critical, or major issues.
Summary
The expires_in fix is correct: the old time.Duration(float64(expiresIn) * 0.8) treated the seconds value as nanoseconds, so a 300s token "expired" after 240ns and the provider re-authenticated on essentially every call (the CLIENT_LOGIN storm). The new time.Duration(expiresIn) * time.Second * 8 / 10 multiplies before dividing, so no premature truncation and the 80% threshold is honored (240s for a 300s token). The service-account convergence change is equally sound: defaultClientScopesConverged accepts an empty list or exactly ["service_account"] (Keycloak's provider-managed built-in scope), while repair still sends empty scope lists and every other default/optional scope continues to fail closed to the repair path.
Strengths
- Test coverage is genuinely additive, not a contract rewrite. New tests cover token reuse, concurrent sharing, and post-threshold refresh; the drift table gains
service_account + rogue-scope, duplicatedservice_account, and injected optional-scope cases, all of which must still repair. No pre-existing assertion was flipped from "accepts" to "rejects," so the Test Diff Scrutiny checks pass cleanly. - The optional-to-accepted broadening (blank ->
service_account) is safe for pre-existing data: an empty default-scope list is still treated as converged, so older Keycloak versions and existing clients need no backfill. The repair path re-emitting empty scopes is idempotent because Keycloak re-adds the built-in scope while service accounts are enabled. - Specs and the reconciliation checkpoint are updated alongside the code (OI-7, SA-14), including the Keycloak 26.1 rationale and the older-version fallback.
Minor
- [Minor]
token_provider.go:91- the expiry expression is correct but a little opaque. A short intermediate (e.g.ttl := time.Duration(expiresIn) * time.Second; tp.expiry = time.Now().Add(ttl * 8 / 10)) would make the "seconds, then 80%" intent self-evident and guard against a future edit reintroducing the unit bug. Non-blocking.
Cross-PR coordination
No material cross-PR coordination issue requires maintainer action.
Findings Summary (ordered by severity, highest first)
- [Minor] Expiry computation is correct but opaque; a named TTL intermediate would harden intent - Readability (token_provider.go:91)
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 | Pass |
| Reconcile (not create-or-skip) pattern | Pass |
| Test Diff Scrutiny (no silent contract flips) | Pass |
| Optional -> required has fallback/backfill | Pass (broadening is backward-compatible) |
| Conventional commit messages | Pass |
| Spec updated with behavior change | Pass |
| // when service accounts are enabled. The repair payload stays empty because | ||
| // Keycloak owns this scope. All other scopes are drift. | ||
| func defaultClientScopesConverged(scopes []string) bool { | ||
| return len(scopes) == 0 || (len(scopes) == 1 && scopes[0] == "service_account") |
There was a problem hiding this comment.
This reads well and is backward-compatible: an empty default-scope list stays converged (older Keycloak / existing clients need no backfill), and only-service_account is accepted as the provider-managed built-in scope. Confirmed that repair still fails closed - service_account plus any other scope, a duplicated service_account, and any optional scope all continue to trigger repair, and the repair payload sends empty scope lists that Keycloak re-populates idempotently. No action needed.
Amber reviewStatus: Complete VerdictAPPROVE. This is a tight, correctly-scoped bug fix that eliminates two Keycloak event storms: a token-lifetime unit error that expired the control-plane service token almost instantly, and a scope-convergence predicate that never reached a fixed point against Keycloak's provider-managed What I verifiedRoot cause 1 - token TTL unit bug ( Root cause 2 - scope convergence never fixed-points ( Test Diff Scrutiny. All test hunks are additive - new test functions and new table cases. No pre-existing assertion was flipped from accept->reject or optional->required, so no silent guarantee was removed. No fallback/backfill concern applies (the empty-list branch preserves the older-Keycloak behavior). Findings
Cross-PR coordinationAnother open pull request hardens management-API JWT auth by enforcing the 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
APPROVE. This is a tight, correctly-scoped bug fix that eliminates two Keycloak event storms: a token-lifetime unit error that expired the control-plane service token almost instantly, and a scope-convergence predicate that never reached a fixed point against Keycloak's provider-managed service_account scope. The change is well-tested (all test changes are additive, no existing guarantees were flipped) and the desired-state specs and reconciliation checkpoint were updated to match.
What I verified
Root cause 1 - token TTL unit bug (token_provider.go). The old line time.Duration(float64(expiresIn) * 0.8) treated expires_in (seconds) as raw time.Duration nanoseconds, so a 300s token expired in ~240ns and every Token() call minted a fresh grant - producing the CLIENT_LOGIN storm. The new ttl := time.Duration(expiresIn) * time.Second; expiry = now + ttl*8/10 interprets the value as seconds and refreshes at 80% (240s). Integer precedence ((ttl*8)/10) and overflow are both fine for realistic lifetimes. TestTokenRefreshesAfterThreshold asserts the ~240s threshold and TestTokenReusesCachedToken / TestConcurrentTokenCallsShareCachedToken prove exactly one grant is issued. The mutex is held across the network fetch, which correctly collapses the concurrent thundering-herd into a single grant.
Root cause 2 - scope convergence never fixed-points (serviceaccountkeycloak/client.go). Repair sends an empty defaultClientScopes, but Keycloak 26.1+ re-adds the built-in service_account scope whenever serviceAccountsEnabled is true, so the previous len(DefaultClientScopes) > 0 check always saw drift and re-repaired forever. defaultClientScopesConverged now accepts an empty list or exactly ["service_account"] and treats everything else as drift. Fail-closed security posture is preserved: the new drift cases (service_account+rogue, duplicated service_account, any optional scope) are covered and still route to repair.
Test Diff Scrutiny. All test hunks are additive - new test functions and new table cases. No pre-existing assertion was flipped from accept->reject or optional->required, so no silent guarantee was removed. No fallback/backfill concern applies (the empty-list branch preserves the older-Keycloak behavior).
Findings
- [Minor] Maintainability (
serviceaccountkeycloak/client.go:312): the built-in scope name"service_account"is a magic literal in the predicate. Promoting it to a named constant near the other Keycloak field constants would tie it to the spec (openshell-gateway-service-accounts.spec.md) and make the Keycloak-26.1 coupling greppable. Non-blocking.
Cross-PR coordination
Another open pull request hardens management-API JWT auth by enforcing the hypershell-frontend audience and failing closed on gRPC calls whose tokens are not minted for the management API. This PR's TokenProvider is exactly the token that GRPCCredentials attaches to every control-plane->API-server gRPC call, and both PRs also add requirements to specs/platform/oidc-integration.spec.md. Maintainers should coordinate so that the token this PR now caches and reuses carries the audience that the other PR will enforce; otherwise, after that PR merges, the control plane will efficiently reuse a token the API server rejects. This is a shared control-plane-token contract and a merge-order/assumption decision, not just file overlap - please align the two before merging both.
Findings Summary (ordered by severity, highest first):
- [Minor] Built-in scope name is a magic literal; prefer a named constant tied to the spec - Maintainability (client.go:312)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| Reconcile pattern (not create-or-skip) | Pass |
| Proper context propagation | Pass |
| Test Diff Scrutiny (no flipped assertions) | Pass |
| Specs updated to match behavior | Pass |
| Conventional commit messages | Pass |
| // Refresh at 80% of TTL to avoid using an expired token. | ||
| tp.expiry = time.Now().Add(time.Duration(float64(expiresIn) * 0.8)) | ||
| ttl := time.Duration(expiresIn) * time.Second | ||
| tp.expiry = time.Now().Add(ttl * 8 / 10) |
There was a problem hiding this comment.
Root-cause fix confirmed. The prior time.Duration(float64(expiresIn) * 0.8) interpreted expires_in (seconds) as nanoseconds, so a 300s token expired in ~240ns and every call re-authenticated - the CLIENT_LOGIN storm. time.Duration(expiresIn) * time.Second then ttl*8/10 correctly yields a 240s refresh threshold. Precedence (ttl*8)/10 and int64 range are both safe for realistic lifetimes. TestTokenRefreshesAfterThreshold locks in the ~240s expectation.
|
Cross-PR audience check for #182: confirmed. The repository-managed hypershell-control-plane Keycloak client has an OIDC audience mapper with included.client.audience set to hypershell-frontend. PR #182 configures the API server with --jwt-audience=hypershell-frontend. These contracts match, so the cached control-plane token will pass the new audience check. An external identity provider must configure its control-plane client with the same audience mapper. |
Amber reviewStatus: Complete VerdictAPPROVE. This is a tightly-scoped, correct fix for two real defects that together produced the Keycloak event storm: the token cache treated What I verifiedToken lifetime fix ( Scope convergence fix ( Test diff scrutiny - The changes to Specs & checkpoint - |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
APPROVE. This is a tightly-scoped, correct fix for two real defects that together produced the Keycloak event storm: the token cache treated expires_in as nanoseconds (so every gRPC call re-ran the Client Credentials grant), and the service-account convergence predicate rejected Keycloak 26.1's provider-managed service_account scope (so every reconcile repaired the client). The change is well-tested, keeps the security-broadening drift checks intact, and updates the desired-state specs to match.
What I verified
Token lifetime fix (token_provider.go) - The old line time.Now().Add(time.Duration(float64(expiresIn) * 0.8)) interpreted expiresIn=300 as 240ns, so the cache expired almost immediately and every Token() call minted a fresh grant. The new ttl := time.Duration(expiresIn) * time.Second; tp.expiry = time.Now().Add(ttl * 8 / 10) correctly yields a 240s refresh window. Integer ordering (multiply before divide) preserves precision, and the values involved cannot overflow int64 for any realistic expires_in. Correct root-cause fix. Confidence: High.
Scope convergence fix (serviceaccountkeycloak/client.go) - defaultClientScopesConverged accepts an empty list (older Keycloak) or exactly [service_account] (Keycloak 26.1+), while !defaultClientScopesConverged(...) and len(OptionalClientScopes) > 0 still force repair for any other default scope, duplicates, or any optional scope. Repair sends empty scope lists and relies on Keycloak re-adding the built-in scope while service accounts are enabled, which converges on the next pass without looping. The reasoning is documented in the code comment and the spec. Confidence: High.
Test diff scrutiny - The changes to client_test.go are strictly additive: new no-write and update-payload tests plus three new security-broadening drift cases (built-in + rogue scope, duplicated built-in scope, injected optional scope). No pre-existing assertion was flipped from reject->accept; the loosening is confined to the exact provider-managed scope and is covered by paired drift tests. This is the correct way to widen a convergence rule. Confidence: High.
Specs & checkpoint - oidc-integration.spec.md (OI-7) and openshell-gateway-service-accounts.spec.md (SA-14) are updated with matching requirements and scenarios; RECONCILE.md records the wave. Config/behavior stay separate.
Cross-PR coordination
No material cross-PR coordination issue requires maintainer action.
Findings Summary (ordered by severity, highest first)
- [Minor] No log or metric is emitted when a new Client Credentials grant is obtained, which makes future grant-frequency regressions (the exact class of bug this PR fixes) hard to observe - Observability (token_provider.go L89-L92)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs or responses | Pass |
| Reconcile (update-or-create), not create-or-skip | Pass |
| Proper context propagation | Pass |
| Test Diff Scrutiny (no silently-flipped assertions) | Pass |
| Spec/desired-state updated to match code | Pass |
| Conventional commit messages | Pass |
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
APPROVE. This is a correct, well-scoped fix for a real production defect (a units bug that turned every control-plane gRPC call into a fresh Keycloak grant, plus a convergence predicate that repaired Keycloak's own built-in scope forever), and it lands with focused regression tests, a fallback for older Keycloak, and matching desired-state specs. No blocking issues; only minor observations.
What this fixes (root cause)
Two independent causes of the Keycloak event storm are addressed:
- Token TTL unit bug (
token_provider.go):time.Duration(float64(expiresIn) * 0.8)interpretedexpires_inseconds as nanoseconds. A 300s token was cached for ~240ns, so nearly every call minted a newclient_credentialstoken → repeatedCLIENT_LOGINevents. Nowtime.Duration(expiresIn) * time.Secondwith an integer 80% threshold. Verified correct; no overflow for realistic lifetimes. - Service-account convergence loop (
serviceaccountkeycloak/client.go): Keycloak 26.1+ auto-adds a built-inservice_accountdefault client scope whenserviceAccountsEnabledis true. The old zero-write predicate treated any non-empty default scope as drift, so every reconcile "repaired" a scope Keycloak immediately re-added → repeated client-repair writes.defaultClientScopesConvergednow accepts an empty list or exactly["service_account"], while repair keeps sending empty scope lists.
Strengths
- Security-conscious logging: only the client ID is logged (via
%q, which neutralizes CR/LF log injection); no token or secret is emitted. A test asserts this. - Good drift coverage: extra default scope, duplicated built-in scope, and injected optional scope all still fail closed to repair.
- Backward compatible: the empty-scope branch preserves behavior for pre-26.1 Keycloak, so no backfill/migration is required for existing clients.
- Concurrency and refresh-threshold behavior are covered by sequential, concurrent, and threshold tests.
Test Diff Scrutiny
The changes to client_test.go are additive: new no-write/update-payload tests and new drift table cases. The pre-existing default client scope injected (rogue-scope) assertion is retained, and no existing assertion was flipped from accept→reject. The predicate change is a documented loosening (accept the provider-managed scope) with a preserved fallback (empty list), so it does not delete a prior guarantee.
Minor observations
- The
INFO/WARNstring-prefix logging matches existing style in the file; if structured logging is adopted later, promoteclient/refreshto fields. Not blocking.
Cross-PR coordination
No material cross-PR coordination issue requires maintainer action.
Findings Summary (ordered by severity, highest first):
- [Minor] Grant log uses a string level prefix rather than structured fields; safe and consistent with existing code, no change required - Observability (
token_provider.goL94)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs or error messages | Pass |
| Input validated / log injection prevented | Pass |
| Reconcile pattern (fail-closed to repair) | Pass |
| Test Diff Scrutiny (no silently flipped assertions) | Pass |
| Conventional commit message | Pass |
| // Refresh at 80% of TTL to avoid using an expired token. | ||
| tp.expiry = time.Now().Add(time.Duration(float64(expiresIn) * 0.8)) | ||
| ttl := time.Duration(expiresIn) * time.Second | ||
| refreshAfter := ttl * 8 / 10 |
There was a problem hiding this comment.
Root cause confirmed and correctly fixed. The prior time.Duration(float64(expiresIn) * 0.8) treated expiresIn (a value in seconds) as nanoseconds, so a 300s token was cached for ~240ns and effectively every Token() call re-ran the client_credentials grant — that is the Keycloak CLIENT_LOGIN event storm. time.Duration(expiresIn) * time.Second restores the intended unit and the 80% threshold. Confidence: High.
The integer form ttl * 8 / 10 avoids float rounding and cannot overflow for any realistic expires_in (max 900s), so this is a clean choice.
| ttl := time.Duration(expiresIn) * time.Second | ||
| refreshAfter := ttl * 8 / 10 | ||
| tp.expiry = time.Now().Add(refreshAfter) | ||
| log.Printf("INFO got OIDC access token for client %q; refresh in %s", tp.clientID, refreshAfter) |
There was a problem hiding this comment.
Minor (observability / security). The grant log records only clientID via %q — which escapes embedded CR/LF and blocks log injection — and never logs the token or client secret, matching security.spec.md. The regression test asserts both properties, which is exactly right. No change required; if the control plane later adopts structured logging you may want to move client/refresh to fields.
| // defaultClientScopesConverged accepts the built-in scope that Keycloak adds | ||
| // when service accounts are enabled. The repair payload stays empty because | ||
| // Keycloak owns this scope. All other scopes are drift. | ||
| func defaultClientScopesConverged(scopes []string) bool { |
There was a problem hiding this comment.
The predicate loosening is safe and self-terminating: defaultClientScopesConverged accepts only an empty list (older Keycloak) or exactly ["service_account"] (Keycloak 26.1+ built-in), while repair still PUTs empty scope lists and Keycloak re-adds only its own service_account scope — so the accept/repair cycle converges instead of looping. Drift cases (extra default scope, duplicated built-in, any optional scope) still fail closed to repair and are covered by the new table cases. This is additive to the existing rogue-scope guarantee, not a flipped assertion. Confidence: High.
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR fixes two real production defects: the control-plane OAuth token cache treated expires_in as nanoseconds (so a 300s token "expired" after 240ns and was re-fetched on nearly every gRPC call), and the service-account convergence predicate rejected Keycloak 26.1+'s auto-added built-in service_account default scope (so every reconcile issued a repair write). Both fixes are correct, minimal, spec-backed, and covered by strong additive tests (sequential/concurrent/threshold reuse, provider-managed scope, and broadening-drift cases); I found no blocking issues.
What this fixes
token_provider.go:time.Duration(float64(expiresIn) * 0.8)interpreted the numeric TTL as nanoseconds. Forexpires_in=300the cache window was 240ns, so the token was effectively never reused and each gRPC call triggered a freshclient_credentialsgrant — the CLIENT_LOGIN event storm. Multiplying bytime.Secondand computingttl * 8 / 10is the correct 80%-of-lifetime refresh window.serviceaccountkeycloak/client.go:defaultClientScopesConvergednow accepts an empty list or exactly["service_account"]as converged, while any additional default scope, any duplicate, and any optional scope still fail closed to repair. This stops the repeated no-op repair writes without weakening drift detection.
Test Diff Scrutiny
No pre-existing assertion was flipped to erase a guarantee. The changes to client_test.go are additive and strengthen coverage: new drift cases assert that service_account + a rogue scope, a duplicated built-in scope, and an injected optional scope are all still repaired. The convergence predicate is loosened only for the exact provider-managed scope, and that loosening is proven safe by the accompanying no-write test plus the new drift tests. This is the intended fix, not a silently removed contract.
Findings
- [Minor / Low confidence] The repair path (
updateRepresentation) sends an explicit emptydefaultClientScopes, relying on Keycloak to re-add the built-inservice_accountscope whileserviceAccountsEnabledis true. This matches the spec's stated behavior and is exercised byTestUpdateRepresentationSendsEmptyClientScopeLists, but it is an assumption about Keycloak's server-side behavior (client-representation PUT vs. thedefault-client-scopessub-resource). Worth a one-line confirmation against a live Keycloak 26.1+ if not already validated. Not blocking.
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs or responses | Pass (%q quotes client ID, escaping control chars; secret/token never logged, verified by TestTokenGrantLogContainsSafeRefreshDetails) |
| Log injection prevented | Pass (%q on operator-supplied client ID) |
| Reconcile pattern (not create-or-skip) | Pass |
| Spec updated to match behavior | Pass |
| Tests additive, no flipped guarantees | Pass |
Cross-PR coordination
Another open pull request introduces mandatory JWT audience enforcement on the management API (requiring aud=hypershell-frontend on non-bypassed HTTP and gRPC endpoints) and assumes the control plane's service-account token already carries that audience via a dedicated Keycloak mapper. This PR makes the control plane cache and reuse that client_credentials token (attached to every outgoing gRPC call via PerRPCCredentials, including non-watch authenticated calls such as status write-backs, which are not in the JWT-bypass method set). Maintainers should decide and confirm that the hypershell-control-plane Keycloak client is provisioned with the hypershell-frontend audience mapper before the audience-enforcement change lands; otherwise the long-lived token this PR now reuses will be rejected as unauthenticated on those calls. This is a shared-assumption/ordering decision between the two changes, not a text merge conflict. See #182.
| ttl := time.Duration(expiresIn) * time.Second | ||
| refreshAfter := ttl * 8 / 10 | ||
| tp.expiry = time.Now().Add(refreshAfter) | ||
| log.Printf("INFO got OIDC access token for client %q; refresh in %s", tp.clientID, refreshAfter) |
There was a problem hiding this comment.
Root-cause fix confirmed. The previous time.Duration(float64(expiresIn) * 0.8) treated the integer TTL as nanoseconds, so expires_in=300 produced a 240ns cache window and the token was re-fetched on nearly every gRPC call — the CLIENT_LOGIN storm. time.Duration(expiresIn) * time.Second then ttl * 8 / 10 is correct. The new INFO log is safe: %q quotes/escapes the operator-supplied client ID (log-injection safe) and neither the token nor secret is logged.
| // when service accounts are enabled. The repair payload stays empty because | ||
| // Keycloak owns this scope. All other scopes are drift. | ||
| func defaultClientScopesConverged(scopes []string) bool { | ||
| return len(scopes) == 0 || (len(scopes) == 1 && scopes[0] == builtInServiceAccountScope) |
There was a problem hiding this comment.
Good fail-closed predicate: only an empty list or exactly ["service_account"] converges; extras, duplicates, and optional scopes still route to repair (covered by the new drift cases). One thing to confirm: updateRepresentation sends an empty defaultClientScopes and relies on Keycloak re-adding the built-in scope while serviceAccountsEnabled is true. That matches the spec, but since Keycloak also exposes a dedicated default-client-scopes sub-resource, a quick check against a live Keycloak 26.1+ that the client-representation PUT truly preserves service_account would remove the last assumption here. Non-blocking.
1 participant
Summary
This stops repeated CLIENT_LOGIN events and repeated service-account client repair writes.
Verification