diff --git a/kagenti-operator/internal/keycloak/audience.go b/kagenti-operator/internal/keycloak/audience.go index 4e3d1cee..27184ba5 100644 --- a/kagenti-operator/internal/keycloak/audience.go +++ b/kagenti-operator/internal/keycloak/audience.go @@ -275,7 +275,25 @@ func (a *Admin) updateAudienceMapperIfNeeded(ctx context.Context, token, realm, mappers[i].Config["included.custom.audience"] = audience return a.putAudienceMapper(ctx, token, realm, scopeID, mappers[i]) } - return fmt.Errorf("no matching audience mapper found for scope %q (scopeID %s)", scopeName, scopeID) + + // No oidc-audience-mapper found. A mapper with the same name but a different + // protocolMapper type caused the 409 conflict. Delete it and recreate correctly. + for i := range mappers { + if mappers[i].Name != scopeName { + continue + } + if err := a.deleteMapper(ctx, token, realm, scopeID, mappers[i].ID); err != nil { + return fmt.Errorf("delete stale mapper %q (id %s): %w", scopeName, mappers[i].ID, err) + } + return a.ensureAudienceMapper(ctx, token, realm, scopeID, scopeName, audience) + } + + // Scope has no mappers matching the expected name. The 409 may be a Keycloak + // realm-level name collision (another scope once held this mapper) or a concurrent + // reconcile that hasn't committed. Either way, the mapper doesn't exist here and + // we can't create it — return nil and let verifyAudienceMapper handle it on the + // next reconcile (it calls ensureAudienceMapper without the 409 loop). + return nil } func (a *Admin) putAudienceMapper(ctx context.Context, token, realm, scopeID string, mapper protocolMapperRep) error { @@ -304,6 +322,27 @@ func (a *Admin) putAudienceMapper(ctx context.Context, token, realm, scopeID str return fmt.Errorf("keycloak update audience mapper: status %d: %s", resp.StatusCode, truncate(body, 256)) } +func (a *Admin) deleteMapper(ctx context.Context, token, realm, scopeID, mapperID string) error { + base := trimBaseURL(a.BaseURL) + endpoint := base + "/admin/realms/" + url.PathEscape(realm) + "/client-scopes/" + + url.PathEscape(scopeID) + "/protocol-mappers/models/" + url.PathEscape(mapperID) + req, err := http.NewRequestWithContext(ctx, http.MethodDelete, endpoint, nil) + if err != nil { + return err + } + req.Header.Set("Authorization", "Bearer "+token) + resp, err := a.httpc().Do(req) + if err != nil { + return err + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode == http.StatusNoContent || resp.StatusCode == http.StatusNotFound { + return nil + } + body, _ := io.ReadAll(resp.Body) + return fmt.Errorf("keycloak delete mapper: status %d: %s", resp.StatusCode, truncate(body, 256)) +} + // verifyAudienceMapper is a defense-in-depth check that runs on every reconcile. // It GETs the mappers for a scope and ensures the oidc-audience-mapper exists with the // correct audience. If the mapper is missing (e.g. due to a prior transient failure), diff --git a/kagenti-operator/internal/keycloak/audience_test.go b/kagenti-operator/internal/keycloak/audience_test.go index 42b27e97..70d70839 100644 --- a/kagenti-operator/internal/keycloak/audience_test.go +++ b/kagenti-operator/internal/keycloak/audience_test.go @@ -353,6 +353,208 @@ func TestEnsureAudienceScope_VerifyRecreatesMissingMapper(t *testing.T) { } } +// TestEnsureAudienceScope_DeletesWrongTypeMapper verifies that when a mapper with the +// correct name exists but has the wrong protocolMapper type (not oidc-audience-mapper), +// the operator deletes it and recreates the correct mapper. This is the fix for #358. +func TestEnsureAudienceScope_DeletesWrongTypeMapper(t *testing.T) { + var deleteMapperCalls, recreatePostCalls int + spiffeURI := "spiffe://example.org/ns/ns/sa/wl" + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + path := r.URL.Path + switch { + case path == testMasterRealmTokenPath: + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{"access_token": "tok"}) + + // Scope already exists + case path == "/admin/realms/kagenti/client-scopes" && r.Method == http.MethodGet: + _ = json.NewEncoder(w).Encode([]clientScopeListItem{{ID: "scope-123", Name: "agent-ns-wl-aud"}}) + + // ensureAudienceMapper POST — 409 conflict (mapper name taken) + case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodPost: + if deleteMapperCalls > 0 { + // After delete, the POST succeeds + recreatePostCalls++ + w.WriteHeader(http.StatusCreated) + } else { + w.WriteHeader(http.StatusConflict) + } + + // GET mappers — returns a mapper with wrong type (e.g. "oidc-hardcoded-claim-mapper") + case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodGet: + if deleteMapperCalls > 0 { + // After delete+recreate, verify sees the correct mapper + _ = json.NewEncoder(w).Encode([]protocolMapperRep{{ + ID: "m-new", Name: "agent-ns-wl-aud", Protocol: "openid-connect", + ProtocolMapper: "oidc-audience-mapper", + Config: map[string]string{"included.custom.audience": spiffeURI}, + }}) + } else { + _ = json.NewEncoder(w).Encode([]protocolMapperRep{{ + ID: "mapper-stale", + Name: "agent-ns-wl-aud", + Protocol: "openid-connect", + ProtocolMapper: "oidc-hardcoded-claim-mapper", // WRONG TYPE + Config: map[string]string{"claim.value": "something"}, + }}) + } + + // DELETE the stale mapper + case strings.Contains(path, "/protocol-mappers/models/mapper-stale") && r.Method == http.MethodDelete: + deleteMapperCalls++ + w.WriteHeader(http.StatusNoContent) + + // Realm default scope + case path == "/admin/realms/kagenti/default-default-client-scopes/scope-123" && r.Method == http.MethodPut: + w.WriteHeader(http.StatusNoContent) + + default: + t.Fatalf("unexpected %s %s", r.Method, path) + } + })) + defer srv.Close() + + a := Admin{BaseURL: srv.URL, HTTPClient: srv.Client()} + token, err := a.PasswordGrantToken(context.Background(), "u", "p") + if err != nil { + t.Fatal(err) + } + + err = a.EnsureAudienceScope(context.Background(), token, AudienceParams{ + Realm: "kagenti", + ClientName: "ns/wl", + AudienceClientID: spiffeURI, + AudienceScopeEnabled: true, + }) + if err != nil { + t.Fatal(err) + } + if deleteMapperCalls != 1 { + t.Fatalf("expected 1 DELETE call for stale mapper, got %d", deleteMapperCalls) + } + if recreatePostCalls != 1 { + t.Fatalf("expected 1 POST call to recreate mapper after delete, got %d", recreatePostCalls) + } +} + +// TestEnsureAudienceScope_PhantomConflict409 verifies that when the mapper POST +// returns 409 but the scope's mapper list is empty (phantom Keycloak conflict from +// concurrent reconcile or realm-level name collision), the function returns nil +// instead of entering an error loop. The verifyAudienceMapper defense-in-depth will +// retry on the next reconcile. +func TestEnsureAudienceScope_PhantomConflict409(t *testing.T) { + var postCalls, getCalls int + spiffeURI := "spiffe://example.org/ns/ns/sa/wl" + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + path := r.URL.Path + switch { + case path == testMasterRealmTokenPath: + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{"access_token": "tok"}) + + // Scope already exists + case path == "/admin/realms/kagenti/client-scopes" && r.Method == http.MethodGet: + _ = json.NewEncoder(w).Encode([]clientScopeListItem{{ID: "scope-123", Name: "agent-ns-wl-aud"}}) + + // POST mapper always returns 409 (persistent realm-level collision) + case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodPost: + postCalls++ + w.WriteHeader(http.StatusConflict) + + // GET mappers — empty (the mapper doesn't actually exist in this scope) + // Second GET (from verifyAudienceMapper) also empty — triggers ensureAudienceMapper + // which hits 409 again and returns nil + case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodGet: + getCalls++ + _ = json.NewEncoder(w).Encode([]protocolMapperRep{}) + + // Realm default scope + case path == "/admin/realms/kagenti/default-default-client-scopes/scope-123" && r.Method == http.MethodPut: + w.WriteHeader(http.StatusNoContent) + + default: + t.Fatalf("unexpected %s %s", r.Method, path) + } + })) + defer srv.Close() + + a := Admin{BaseURL: srv.URL, HTTPClient: srv.Client()} + token, err := a.PasswordGrantToken(context.Background(), "u", "p") + if err != nil { + t.Fatal(err) + } + + err = a.EnsureAudienceScope(context.Background(), token, AudienceParams{ + Realm: "kagenti", + ClientName: "ns/wl", + AudienceClientID: spiffeURI, + AudienceScopeEnabled: true, + }) + if err != nil { + t.Fatalf("expected nil error (phantom 409 should not loop), got: %v", err) + } + if postCalls < 1 { + t.Fatalf("expected at least 1 POST attempt, got %d", postCalls) + } +} + +// TestEnsureAudienceScope_ConcurrentCreate409 verifies that when both the initial +// and retry POST return 409 (concurrent reconcile created the mapper), the operator +// treats it as success rather than entering an error loop. +func TestEnsureAudienceScope_ConcurrentCreate409(t *testing.T) { + spiffeURI := "spiffe://example.org/ns/ns/sa/wl" + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + path := r.URL.Path + switch { + case path == testMasterRealmTokenPath: + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{"access_token": "tok"}) + + case path == "/admin/realms/kagenti/client-scopes" && r.Method == http.MethodGet: + _ = json.NewEncoder(w).Encode([]clientScopeListItem{{ID: "scope-123", Name: "agent-ns-wl-aud"}}) + + // All POSTs return 409 (concurrent reconcile already created it) + case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodPost: + w.WriteHeader(http.StatusConflict) + + // GET mappers — empty during updateAudienceMapperIfNeeded (race: mapper not yet visible) + // but present during verifyAudienceMapper (transaction committed) + case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodGet: + _ = json.NewEncoder(w).Encode([]protocolMapperRep{{ + ID: "m-concurrent", Name: "agent-ns-wl-aud", Protocol: "openid-connect", + ProtocolMapper: "oidc-audience-mapper", + Config: map[string]string{"included.custom.audience": spiffeURI}, + }}) + + case path == "/admin/realms/kagenti/default-default-client-scopes/scope-123" && r.Method == http.MethodPut: + w.WriteHeader(http.StatusNoContent) + + default: + t.Fatalf("unexpected %s %s", r.Method, path) + } + })) + defer srv.Close() + + a := Admin{BaseURL: srv.URL, HTTPClient: srv.Client()} + token, err := a.PasswordGrantToken(context.Background(), "u", "p") + if err != nil { + t.Fatal(err) + } + + err = a.EnsureAudienceScope(context.Background(), token, AudienceParams{ + Realm: "kagenti", + ClientName: "ns/wl", + AudienceClientID: spiffeURI, + AudienceScopeEnabled: true, + }) + if err != nil { + t.Fatalf("expected success when concurrent reconcile created mapper, got: %v", err) + } +} + func TestEnsureAudienceScope_Disabled(t *testing.T) { a := Admin{} err := a.EnsureAudienceScope(context.Background(), "t", AudienceParams{AudienceScopeEnabled: false})