fix(keycloak): propagate and verify audience mapper errors (#348)

kagenti-operator/internal/controller/indexers_test.go

@@ -111,8 +111,10 @@ var _ = Describe("mapWorkloadToAgentCards", func() {
 			sbx.SetLabels(map[string]string{LabelAgentType: LabelValueAgent})
 
 			mapFn := mapWorkloadToAgentCards(indexedClient, "agents.x-k8s.io/v1alpha1", "Sandbox", logger)
+			Eventually(func() int {
+				return len(mapFn(ctx, sbx))
+			}).Should(Equal(1))
 			requests := mapFn(ctx, sbx)
-			Expect(requests).To(HaveLen(1))
 			Expect(requests[0].Name).To(Equal("sandbox-card"))
 		})
 	})

kagenti-operator/internal/keycloak/audience.go

@@ -13,7 +13,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
-	"log/slog"
 	"net/http"
 	"net/url"
 	"strings"
@@ -66,6 +65,9 @@ func (a *Admin) EnsureAudienceScope(ctx context.Context, token string, p Audienc
 	if err != nil {
 		return err
 	}
+	if err := a.verifyAudienceMapper(ctx, token, p.Realm, scopeID, scopeName, p.AudienceClientID); err != nil {
+		return fmt.Errorf("verify audience mapper for scope %q: %w", scopeName, err)
+	}
 	_ = a.putRealmDefaultDefaultClientScope(ctx, token, p.Realm, scopeID)
 	for _, plat := range p.PlatformClientIDs {
 		plat = strings.TrimSpace(plat)
@@ -87,7 +89,9 @@ func (a *Admin) getOrCreateAudienceClientScope(ctx context.Context, token, realm
 		return "", err
 	}
 	if scopeID != "" {
-		_ = a.ensureAudienceMapper(ctx, token, realm, scopeID, scopeName, audience)
+		if err := a.ensureAudienceMapper(ctx, token, realm, scopeID, scopeName, audience); err != nil {
+			return "", fmt.Errorf("ensure audience mapper for existing scope %q: %w", scopeName, err)
+		}
 		return scopeID, nil
 	}
 
@@ -105,7 +109,9 @@ func (a *Admin) getOrCreateAudienceClientScope(ctx context.Context, token, realm
 	if scopeID == "" {
 		return "", fmt.Errorf("create client scope %q returned empty id", scopeName)
 	}
-	_ = a.ensureAudienceMapper(ctx, token, realm, scopeID, scopeName, audience)
+	if err := a.ensureAudienceMapper(ctx, token, realm, scopeID, scopeName, audience); err != nil {
+		return "", fmt.Errorf("ensure audience mapper for new scope %q: %w", scopeName, err)
+	}
 	return scopeID, nil
 }
 
@@ -219,30 +225,41 @@ func (a *Admin) ensureAudienceMapper(ctx context.Context, token, realm, scopeID,
 	return nil
 }
 
-// updateAudienceMapperIfNeeded fetches the existing mapper for the scope and updates
-// its included.custom.audience if it differs from the desired value.
-func (a *Admin) updateAudienceMapperIfNeeded(ctx context.Context, token, realm, scopeID, scopeName, audience string) error {
+// listAudienceMappers fetches all protocol mappers for a client scope.
+func (a *Admin) listAudienceMappers(ctx context.Context, token, realm, scopeID string) ([]protocolMapperRep, error) {
 	base := trimBaseURL(a.BaseURL)
 	endpoint := base + "/admin/realms/" + url.PathEscape(realm) + "/client-scopes/" + url.PathEscape(scopeID) + "/protocol-mappers/models"
 	req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil)
 	if err != nil {
-		return err
+		return nil, err
 	}
 	req.Header.Set("Authorization", "Bearer "+token)
 
 	resp, err := a.httpc().Do(req)
 	if err != nil {
-		return err
+		return nil, err
 	}
 	defer func() { _ = resp.Body.Close() }()
 	body, _ := io.ReadAll(resp.Body)
 	if resp.StatusCode != http.StatusOK {
-		return fmt.Errorf("keycloak list mappers: status %d: %s", resp.StatusCode, truncate(body, 256))
+		return nil, fmt.Errorf("keycloak list mappers: status %d: %s", resp.StatusCode, truncate(body, 256))
 	}
 
 	var mappers []protocolMapperRep
 	if err := json.Unmarshal(body, &mappers); err != nil {
-		return fmt.Errorf("keycloak list mappers decode: %w", err)
+		return nil, fmt.Errorf("keycloak list mappers decode: %w", err)
+	}
+	return mappers, nil
+}
+
+// updateAudienceMapperIfNeeded fetches the existing mapper for the scope and updates
+// its included.custom.audience if it differs from the desired value.
+// Returns an error if no matching mapper is found — this treats "no match" as a real
+// failure (e.g. Keycloak race or name mismatch) rather than silently ignoring it.
+func (a *Admin) updateAudienceMapperIfNeeded(ctx context.Context, token, realm, scopeID, scopeName, audience string) error {
+	mappers, err := a.listAudienceMappers(ctx, token, realm, scopeID)
+	if err != nil {
+		return err
 	}
 
 	for i := range mappers {
@@ -255,12 +272,10 @@ func (a *Admin) updateAudienceMapperIfNeeded(ctx context.Context, token, realm,
 		if mappers[i].Config["included.custom.audience"] == audience {
 			return nil // already correct
 		}
-		// Update the mapper with the correct audience.
 		mappers[i].Config["included.custom.audience"] = audience
 		return a.putAudienceMapper(ctx, token, realm, scopeID, mappers[i])
 	}
-	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)
 }
 
 func (a *Admin) putAudienceMapper(ctx context.Context, token, realm, scopeID string, mapper protocolMapperRep) error {
@@ -289,6 +304,34 @@ 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))
 }
 
+// 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),
+// it re-creates it. If the audience is stale, it updates it.
+// Cost: one extra GET per reconcile per audience-enabled scope; accepted tradeoff for
+// catching scopes left broken by prior transient failures.
+func (a *Admin) verifyAudienceMapper(ctx context.Context, token, realm, scopeID, scopeName, audience string) error {
+	mappers, err := a.listAudienceMappers(ctx, token, realm, scopeID)
+	if err != nil {
+		return err
+	}
+
+	for i := range mappers {
+		if mappers[i].Name != scopeName || mappers[i].ProtocolMapper != "oidc-audience-mapper" {
+			continue
+		}
+		if mappers[i].Config != nil && mappers[i].Config["included.custom.audience"] == audience {
+			return nil
+		}
+		if mappers[i].Config == nil {
+			mappers[i].Config = make(map[string]string)
+		}
+		mappers[i].Config["included.custom.audience"] = audience
+		return a.putAudienceMapper(ctx, token, realm, scopeID, mappers[i])
+	}
+	return a.ensureAudienceMapper(ctx, token, realm, scopeID, scopeName, audience)
+}
+
 func (a *Admin) putRealmDefaultDefaultClientScope(ctx context.Context, token, realm, scopeID string) error {
 	base := trimBaseURL(a.BaseURL)
 	endpoint := base + "/admin/realms/" + url.PathEscape(realm) + "/default-default-client-scopes/" + url.PathEscape(scopeID)

kagenti-operator/internal/keycloak/audience_test.go

@@ -35,6 +35,12 @@ func TestEnsureAudienceScope(t *testing.T) {
 		case strings.Contains(path, "/client-scopes/new-scope-id/protocol-mappers/models") && r.Method == http.MethodPost:
 			postMapperCalls++
 			w.WriteHeader(http.StatusCreated)
+		case strings.Contains(path, "/client-scopes/new-scope-id/protocol-mappers/models") && r.Method == http.MethodGet:
+			_ = json.NewEncoder(w).Encode([]protocolMapperRep{{
+				ID: "m1", Name: "agent-ns-wl-aud", Protocol: "openid-connect",
+				ProtocolMapper: "oidc-audience-mapper",
+				Config:         map[string]string{"included.custom.audience": "ns/wl"},
+			}})
 		case path == "/admin/realms/kagenti/default-default-client-scopes/new-scope-id" && r.Method == http.MethodPut:
 			putRealmCalls++
 			w.WriteHeader(http.StatusNoContent)
@@ -79,6 +85,7 @@ func TestEnsureAudienceScope(t *testing.T) {
 func TestEnsureAudienceScope_UpdatesStaleMapper(t *testing.T) {
 	var getMapperCalls, putMapperCalls int
 	var putMapperBody protocolMapperRep
+	spiffeURI := "spiffe://example.org/ns/ns/sa/wl"
 
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		path := r.URL.Path
@@ -95,16 +102,20 @@ func TestEnsureAudienceScope_UpdatesStaleMapper(t *testing.T) {
 		case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodPost:
 			w.WriteHeader(http.StatusConflict)
 
-		// GET mappers — returns mapper with stale audience
+		// GET mappers — first call returns stale, subsequent calls return corrected
 		case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodGet:
 			getMapperCalls++
+			aud := "ns/wl"
+			if putMapperCalls > 0 {
+				aud = spiffeURI
+			}
 			_ = json.NewEncoder(w).Encode([]protocolMapperRep{{
 				ID:             "mapper-456",
 				Name:           "agent-ns-wl-aud",
 				Protocol:       "openid-connect",
 				ProtocolMapper: "oidc-audience-mapper",
 				Config: map[string]string{
-					"included.custom.audience": "ns/wl", // stale short-form
+					"included.custom.audience": aud,
 					"id.token.claim":           "false",
 					"access.token.claim":       "true",
 					"userinfo.token.claim":     "false",
@@ -134,7 +145,6 @@ func TestEnsureAudienceScope_UpdatesStaleMapper(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	spiffeURI := "spiffe://example.org/ns/ns/sa/wl"
 	err = a.EnsureAudienceScope(context.Background(), token, AudienceParams{
 		Realm:                "kagenti",
 		ClientName:           "ns/wl",
@@ -144,8 +154,8 @@ func TestEnsureAudienceScope_UpdatesStaleMapper(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if getMapperCalls != 1 {
-		t.Fatalf("expected 1 GET mapper call, got %d", getMapperCalls)
+	if getMapperCalls != 2 {
+		t.Fatalf("expected 2 GET mapper calls (update + verify), got %d", getMapperCalls)
 	}
 	if putMapperCalls != 1 {
 		t.Fatalf("expected 1 PUT mapper call, got %d", putMapperCalls)
@@ -221,6 +231,128 @@ func TestEnsureAudienceScope_SkipsUpdateWhenCorrect(t *testing.T) {
 	}
 }
 
+// TestEnsureAudienceScope_MapperFailurePropagated verifies that when the mapper POST
+// returns a server error (e.g. 500), the error propagates to EnsureAudienceScope
+// instead of being silently swallowed (regression test for #348).
+func TestEnsureAudienceScope_MapperFailurePropagated(t *testing.T) {
+	var srv *httptest.Server
+	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 does not exist yet
+		case path == "/admin/realms/kagenti/client-scopes" && r.Method == http.MethodGet:
+			_ = json.NewEncoder(w).Encode([]clientScopeListItem{})
+
+		// Scope creation succeeds
+		case path == "/admin/realms/kagenti/client-scopes" && r.Method == http.MethodPost:
+			w.Header().Set("Location", srv.URL+"/admin/realms/kagenti/client-scopes/new-scope-id")
+			w.WriteHeader(http.StatusCreated)
+
+		// Mapper POST returns 500 (server error)
+		case strings.Contains(path, "/client-scopes/new-scope-id/protocol-mappers/models") && r.Method == http.MethodPost:
+			w.WriteHeader(http.StatusInternalServerError)
+			_, _ = w.Write([]byte(`{"error":"internal"}`))
+
+		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:     "spiffe://example.org/ns/ns/sa/wl",
+		AudienceScopeEnabled: true,
+	})
+	if err == nil {
+		t.Fatal("expected error when mapper POST fails, got nil")
+	}
+	if !strings.Contains(err.Error(), "ensure audience mapper") {
+		t.Fatalf("expected error to contain 'ensure audience mapper', got: %s", err.Error())
+	}
+}
+
+// TestEnsureAudienceScope_VerifyRecreatesMissingMapper verifies that the defense-in-depth
+// verifyAudienceMapper check detects a scope that exists without a mapper (from a prior
+// failed reconcile) and re-creates the mapper.
+func TestEnsureAudienceScope_VerifyRecreatesMissingMapper(t *testing.T) {
+	var verifyGetCalls, recreatePostCalls, verifyGetAfterRecreate 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 from prior run
+		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 — mapper created (scope exists, mapper doesn't)
+		case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodPost:
+			recreatePostCalls++
+			w.WriteHeader(http.StatusCreated)
+
+		// GET mappers — first call (verify) returns empty (mapper missing), second returns recreated
+		case strings.Contains(path, "/client-scopes/scope-123/protocol-mappers/models") && r.Method == http.MethodGet:
+			verifyGetCalls++
+			if recreatePostCalls > 0 {
+				verifyGetAfterRecreate++
+				_ = 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{})
+			}
+
+		// 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 verifyGetCalls < 1 {
+		t.Fatalf("expected at least 1 verify GET call, got %d", verifyGetCalls)
+	}
+	if recreatePostCalls < 1 {
+		t.Fatalf("expected mapper to be re-created via POST, got %d calls", recreatePostCalls)
+	}
+}
+
 func TestEnsureAudienceScope_Disabled(t *testing.T) {
 	a := Admin{}
 	err := a.EnsureAudienceScope(context.Background(), "t", AudienceParams{AudienceScopeEnabled: false})