fix: Update audience scope mapper when included.custom.audience changes - #331
Conversation
|
This seems like a corner case that we have not tested. If I understand it correctly, the scenario is that an agent was initially provisioned without SPIRE enabled, and later, it was restarted with SPIRE enabled. Because error 409 is treated as success, client registration doesn't proceed to update Keycloak with the updated audience scope mapper. Is this right? I'm curious what is your use case that you needed to restart an agent from no-spire to with-spire. Can't you delete the agent and create a new one? |
|
That's correct @huang195 . My use case is just that I am running a sequence of tests and I first try token exchange in non-spiffe mode. And then, I enable spiffe and expect token exchange to work as well. For regular kagentiusers; when they use the kagenti ui; that should correspond to checking unchecking the "Enable SPIFFE" checkbox in the UI. They should certainly hit the same issue. |
pdettori
left a comment
There was a problem hiding this comment.
Clean, well-scoped fix for a real production issue (toggling SPIFFE on/off leaves stale audience mappers). The GET+compare+PUT logic on 409 Conflict is the right approach. Tests cover both the update and no-op paths. All CI green.
A few non-blocking suggestions inline.
fc6774a to
95947c6
Compare
|
/retest |
When an audience scope mapper already exists (409 Conflict), fetch the existing mapper and compare its included.custom.audience value. If it differs from the desired value (e.g. short-form "ns/wl" vs SPIFFE URI), update it via PUT. Previously, 409 was treated as success without checking the audience value, causing stale mappers when clients transitioned from short-form IDs to SPIFFE URIs. Fixes rossoctl#330 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Akram <akram.benaissi@gmail.com>
- Drop omitempty from protocolMapperRep.ID to fail explicitly on empty ID - Add nil guard for Config map to prevent potential panic - Add slog.Debug log when no matching audience mapper is found Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Akram <akram.benaissi@gmail.com>
95947c6 to
082e541
Compare
Summary
Fixes #330
When an audience scope mapper already exists (HTTP 409 Conflict),
ensureAudienceMappernow fetches the existing mapper and compares itsincluded.custom.audiencevalue. If it differs from the desired value, the mapper is updated via PUT.Previously, 409 was treated as success without checking the audience value, causing stale mappers when clients transitioned from short-form IDs (
namespace/workload) to SPIFFE URIs (spiffe://domain/ns/namespace/sa/workload) or vice versa.Changes
IDfield toprotocolMapperRep(needed for PUT by mapper ID)ensureAudienceMapper, call newupdateAudienceMapperIfNeededto check and fix the audienceputAudienceMapperfor updating an existing mapper via PUTRoot Cause
The
EnsureAudienceScopecaller inclientregistration_controller.gocorrectly passes the resolved SPIFFE URI asAudienceClientID:But if the scope was initially created with the short-form audience (e.g., before SPIFFE was enabled), the mapper POST returns 409 and the stale audience was never corrected.
Test plan
TestEnsureAudienceScope_UpdatesStaleMapper— verifies stale short-form audience is updated to SPIFFE URI via GET + PUTTestEnsureAudienceScope_SkipsUpdateWhenCorrect— verifies no PUT is issued when audience already matchesTestEnsureAudienceScope*tests passgo test ./internal/keycloak/ -v)Assisted-By: Claude (Anthropic AI) noreply@anthropic.com