fix(keycloak): propagate and verify audience mapper errors (#348) - #350
Conversation
CI Note: Unit Test failure is pre-existing, not related to this PRThe failing test is Main branch CI (
All 6 keycloak audience tests pass (including 2 new ones added by this PR). |
…owing them getOrCreateAudienceClientScope discarded errors from ensureAudienceMapper on both code paths (existing and new scope), leaving audience scopes without the oidc-audience-mapper and causing silent 401 failures. Fixes #348 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: cwiklik <cwiklikj@gmail.com>
…ncile verifyAudienceMapper runs after getOrCreateAudienceClientScope and GETs the scope's protocol mappers to confirm the oidc-audience-mapper exists with the correct audience. If missing (e.g. prior transient failure left the scope without a mapper), it re-creates it. If stale, it updates it. Also makes updateAudienceMapperIfNeeded return an error when no matching mapper is found (previously a silent no-op). Refs #348 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: cwiklik <cwiklikj@gmail.com>
17f3429 to
581e41f
Compare
The fake IDP test server only handled POST to /protocol-mappers/models but the new verifyAudienceMapper method also GETs mappers to confirm they exist. Track created mappers and serve them on GET. Signed-off-by: cwiklik <cwiklik@users.noreply.github.com> Signed-off-by: cwiklik <cwiklikj@gmail.com>
8818a6c to
0926d9c
Compare
rubambiza
left a comment
rubambiza
left a comment
There was a problem hiding this comment.
Review Summary
Well-scoped fix for #348: replaces error-discarding _ = calls with proper propagation, adds a defense-in-depth verifyAudienceMapper that re-creates missing mappers on reconcile, and adds regression tests for both failure propagation and mapper re-creation. Tests look solid and all 15 CI checks pass.
Areas reviewed: Go (internal/keycloak)
Commits: 3, all DCO signed-off, conventional format, Assisted-By: per project policy
CI: all 15 checks green
Approving. Left a couple of non-blocking suggestions/nits inline.
Assisted-By: Claude Code
| return nil | ||
| } | ||
| if mappers[i].Config == nil { | ||
| mappers[i].Config = make(map[string]string) |
There was a problem hiding this comment.
suggestion: verifyAudienceMapper and updateAudienceMapperIfNeeded share ~25 lines of GET-and-parse logic (endpoint, auth header, read body, status check, unmarshal, match loop). Consider extracting a listAudienceMappers(ctx, token, realm, scopeID) ([]protocolMapperRep, error) helper — both callers would benefit and the two match-loop behaviors (update-only vs create-if-missing) become clearer. Not blocking.
| } | ||
| slog.Debug("no matching audience mapper found for scope", "scope", scopeName, "scopeID", scopeID) | ||
| return nil | ||
| return fmt.Errorf("no matching audience mapper found for scope %q (scopeID %s)", scopeName, scopeID) |
There was a problem hiding this comment.
suggestion: updateAudienceMapperIfNeeded now returns an error when no matching mapper is found. This changes behavior for the 409 Conflict path in ensureAudienceMapper: a 409 with no name match would previously be silently ignored, now it errors. This is the right direction for visibility, but worth a brief comment on the function (or in the error string) noting that "no match" is now treated as a real failure (e.g. case mismatch or Keycloak race) rather than the previous silent no-op — makes the behavior change easier to spot later.
| return fmt.Errorf("keycloak update audience mapper: status %d: %s", resp.StatusCode, truncate(body, 256)) | ||
| } | ||
|
|
||
| // verifyAudienceMapper is a defense-in-depth check that runs on every reconcile. |
There was a problem hiding this comment.
nit: verifyAudienceMapper adds an extra GET /protocol-mappers/models on every reconcile for every audience-enabled scope. The defense-in-depth tradeoff is reasonable, but worth acknowledging the cost in the doc comment (e.g. "one extra GET per reconcile; accepted cost for catching scopes left broken by prior transient failures").
… comments Address PR #350 review feedback: - Extract shared GET-and-parse logic into listAudienceMappers helper - Document behavior change in updateAudienceMapperIfNeeded (no-match is now a real error, not a silent no-op) - Acknowledge per-reconcile GET cost in verifyAudienceMapper doc comment Signed-off-by: cwiklik <cwiklik@users.noreply.github.com> Signed-off-by: cwiklik <cwiklikj@gmail.com>
The Sandbox test creates an AgentCard via direct client then queries via the manager's cached client. On slower CI runners the cache hasn't synced yet, causing a flaky failure. Use Eventually to poll until the cache reflects the new object. Signed-off-by: cwiklik <cwiklik@users.noreply.github.com> Signed-off-by: cwiklik <cwiklikj@gmail.com>
github-project-automation
Bot
moved this from Backlog
to Done
in Rossoctl Issue Prioritization
…pper (#358) When a protocol mapper exists with the correct name but the wrong type (not oidc-audience-mapper), the POST returns 409 and updateAudienceMapperIfNeeded fails to find a matching mapper — entering an infinite error loop that blocks audience scope propagation. ## Root Cause The 409 Conflict from Keycloak means "a mapper with that name already exists." But updateAudienceMapperIfNeeded only looks for mappers matching BOTH Name == scopeName AND ProtocolMapper == "oidc-audience-mapper". When the existing mapper has the right name but wrong type, the loop skips it and falls through to "no matching audience mapper found." This also prevents verifyAudienceMapper (defense-in-depth from PR #350) from running, since getOrCreateAudienceClientScope returns early on the error — no self-healing is possible. ## Fix In updateAudienceMapperIfNeeded, after failing to find an oidc-audience-mapper, perform a second pass looking for any mapper with a matching name (regardless of type). If found, DELETE it via the Keycloak Admin API, then re-POST the correct oidc-audience-mapper. This is the minimal targeted fix — it handles the exact broken state (wrong-type name collision) without restructuring the flow. ## Observed Symptoms - Operator logs: "ensure audience mapper for existing scope ... no matching audience mapper found" repeating every few seconds - Agent tokens lack the correct audience claim - AuthBridge/Envoy rejects requests with 401 Unauthorized - Affects fresh installs with operator v0.2.0-rc.4 Fixes #358 Signed-off-by: cwiklik <cwiklik@users.noreply.github.com> Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: cwiklik <cwiklikj@gmail.com>
…pper (#358) When a protocol mapper exists with the correct name but the wrong type (not oidc-audience-mapper), the POST returns 409 and updateAudienceMapperIfNeeded fails to find a matching mapper — entering an infinite error loop that blocks audience scope propagation. ## Root Cause The 409 Conflict from Keycloak means "a mapper with that name already exists." But updateAudienceMapperIfNeeded only looks for mappers matching BOTH Name == scopeName AND ProtocolMapper == "oidc-audience-mapper". When the existing mapper has the right name but wrong type, the loop skips it and falls through to "no matching audience mapper found." This also prevents verifyAudienceMapper (defense-in-depth from PR #350) from running, since getOrCreateAudienceClientScope returns early on the error — no self-healing is possible. ## Fix In updateAudienceMapperIfNeeded, after failing to find an oidc-audience-mapper, perform a second pass looking for any mapper with a matching name (regardless of type). If found, DELETE it via the Keycloak Admin API, then re-POST the correct oidc-audience-mapper. This is the minimal targeted fix — it handles the exact broken state (wrong-type name collision) without restructuring the flow. ## Observed Symptoms - Operator logs: "ensure audience mapper for existing scope ... no matching audience mapper found" repeating every few seconds - Agent tokens lack the correct audience claim - AuthBridge/Envoy rejects requests with 401 Unauthorized - Affects fresh installs with operator v0.2.0-rc.4 Fixes #358 Signed-off-by: cwiklik <cwiklik@users.noreply.github.com> Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: cwiklik <cwiklikj@gmail.com>
2 participants
Summary
ensureAudienceMappererrors instead of silently discarding them with_ =, so the controller sees failures and can act on themverifyAudienceMapperdefense-in-depth check that runs on every reconcile — GETs the scope's protocol mappers and re-creates theoidc-audience-mapperif missing (catches scopes broken by prior transient failures)updateAudienceMapperIfNeededreturn an error when no matching mapper is found (was a silent no-op)Fixes #348
Changes
internal/keycloak/audience.goverifyAudienceMapper; remove unusedslogimportinternal/keycloak/audience_test.goMapperFailurePropagatedtest (mapper 500 → error surfaces); addVerifyRecreatesMissingMappertest (missing mapper detected and re-created); update existing tests for verify GET callTest plan
Unit tests — all 6 pass
Kind cluster — local image build + deploy
Built operator image from this branch, loaded into Kind cluster via
ctr import, rolled outkagenti-controller-managerdeployment withimagePullPolicy: Never.Kind cluster — new agent deploy validates fix
Deployed a new agent (
weather-service-777) inteam1namespace via Sandbox CR with source build. The operator reconciled the new pod and created the audience scope with the correct mapper:Both agents have correct audience scopes with mappers — no silent failures, no missing mappers.
Assisted-By: Claude Code