From 5f1f96747bbae11352aa9817d5ee5eb55579b777 Mon Sep 17 00:00:00 2001 From: Kevin Kugler Date: Thu, 24 Sep 2026 11:41:33 +0200 Subject: [PATCH] fix: add drift detection to ProtocolMapper, Role and ClientScope These controllers PUT their definition to Keycloak on every sync (5m) even when nothing changed. Each protocol mapper PUT invalidates the owning client in Keycloak's realm cache on all nodes, so the next token/userinfo requests reload from the DB. On prod (40 mapper CRs) this meant ~11.6k no-op writes per day and 2-13s token latencies, pushing the pace-id SLA (v1) from ~99.98% to 99.74%. Compare the desired definition against the current representation with the existing definitionsMatch (subset, unordered string arrays) and skip the PUT when in sync, as the client controller already does: - ProtocolMapper: list mappers raw and keep the matched representation - Role: compare via Get{Realm,Client}RoleRaw; composites are stripped beforehand and still synced through the composites endpoint - ClientScope: compare via GetClientScopeRaw A failed fetch falls through to the update, so behaviour on errors is unchanged. Verified against live prod state: all 40 mappers compare as in sync. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../keycloakclientscope_controller.go | 15 +++-- .../keycloakprotocolmapper_controller.go | 52 +++++++++------ .../keycloakprotocolmapper_drift_test.go | 64 +++++++++++++++++++ .../controller/keycloakrole_controller.go | 30 ++++++--- 4 files changed, 127 insertions(+), 34 deletions(-) create mode 100644 internal/controller/keycloakprotocolmapper_drift_test.go diff --git a/internal/controller/keycloakclientscope_controller.go b/internal/controller/keycloakclientscope_controller.go index e390ca6..7a1efc5 100644 --- a/internal/controller/keycloakclientscope_controller.go +++ b/internal/controller/keycloakclientscope_controller.go @@ -141,12 +141,17 @@ func (r *KeycloakClientScopeReconciler) Reconcile(ctx context.Context, req ctrl. scopeID = *existingScope.ID definition = mergeIDIntoDefinition(definition, existingScope.ID) - log.Info("updating client scope", "name", scopeDef.Name, "realm", realmName) - if err := kc.UpdateClientScope(ctx, realmName, scopeID, definition); err != nil { - RecordError(controllerName, "keycloak_api_error") - return r.updateStatus(ctx, clientScope, false, "UpdateFailed", fmt.Sprintf("Failed to update client scope: %v", err), scopeID) + currentRaw, fetchErr := kc.GetClientScopeRaw(ctx, realmName, scopeID) + if fetchErr == nil && definitionsMatch(definition, currentRaw) { + log.V(1).Info("client scope already in sync, skipping update", "name", scopeDef.Name) + } else { + log.Info("updating client scope", "name", scopeDef.Name, "realm", realmName) + if err := kc.UpdateClientScope(ctx, realmName, scopeID, definition); err != nil { + RecordError(controllerName, "keycloak_api_error") + return r.updateStatus(ctx, clientScope, false, "UpdateFailed", fmt.Sprintf("Failed to update client scope: %v", err), scopeID) + } + log.Info("client scope updated successfully", "name", scopeDef.Name) } - log.Info("client scope updated successfully", "name", scopeDef.Name) } // Update status diff --git a/internal/controller/keycloakprotocolmapper_controller.go b/internal/controller/keycloakprotocolmapper_controller.go index 88e14e4..a50e6af 100644 --- a/internal/controller/keycloakprotocolmapper_controller.go +++ b/internal/controller/keycloakprotocolmapper_controller.go @@ -122,28 +122,19 @@ func (r *KeycloakProtocolMapperReconciler) Reconcile(ctx context.Context, req ct return r.updateStatus(ctx, mapper, false, "ConfigSecretError", err.Error(), "", "", "", "") } - // Find existing mapper by name + // Find existing mapper by name. The raw representation is kept for drift + // detection so that an unchanged mapper is not re-PUT on every sync. var mapperID string + var currentRaw json.RawMessage + var existingMappers []json.RawMessage + var listErr error if parentType == "client" { - existingMappers, err := kc.GetClientProtocolMappers(ctx, realmName, parentID) - if err == nil { - for _, m := range existingMappers { - if m.Name != nil && *m.Name == mapperName { - mapperID = *m.ID - break - } - } - } + existingMappers, listErr = kc.GetClientProtocolMappersRaw(ctx, realmName, parentID) } else { - existingMappers, err := kc.GetClientScopeProtocolMappers(ctx, realmName, parentID) - if err == nil { - for _, m := range existingMappers { - if m.Name != nil && *m.Name == mapperName { - mapperID = *m.ID - break - } - } - } + existingMappers, listErr = kc.GetClientScopeProtocolMappersRaw(ctx, realmName, parentID) + } + if listErr == nil { + mapperID, currentRaw = findMapperByName(existingMappers, mapperName) } if mapperID == "" { @@ -160,6 +151,11 @@ func (r *KeycloakProtocolMapperReconciler) Reconcile(ctx context.Context, req ct return r.updateStatus(ctx, mapper, false, "CreateFailed", fmt.Sprintf("Failed to create protocol mapper: %v", err), "", "", parentType, parentID) } log.Info("protocol mapper created successfully", "name", mapperName, "id", mapperID) + } else if definitionsMatch(definition, currentRaw) { + // Every PUT on a protocol mapper invalidates the owning client in + // Keycloak's realm cache on all nodes, so skipping no-op updates keeps + // token endpoint latency flat in steady state. + log.V(1).Info("protocol mapper already in sync, skipping update", "name", mapperName) } else { // Update mapper definition = mergeIDIntoDefinition(definition, &mapperID) @@ -345,3 +341,21 @@ func (r *KeycloakProtocolMapperReconciler) findMappersForSecret(ctx context.Cont return o.(*keycloakv1beta1.KeycloakProtocolMapper).Spec.ConfigSecretRef }) } + +// findMapperByName returns the ID and raw representation of the mapper with +// the given name, or empty values if none matches. +func findMapperByName(mappers []json.RawMessage, name string) (string, json.RawMessage) { + for _, raw := range mappers { + var m struct { + ID string `json:"id"` + Name string `json:"name"` + } + if err := json.Unmarshal(raw, &m); err != nil { + continue + } + if m.Name == name && m.ID != "" { + return m.ID, raw + } + } + return "", nil +} diff --git a/internal/controller/keycloakprotocolmapper_drift_test.go b/internal/controller/keycloakprotocolmapper_drift_test.go new file mode 100644 index 0000000..33a97fe --- /dev/null +++ b/internal/controller/keycloakprotocolmapper_drift_test.go @@ -0,0 +1,64 @@ +package controller + +import ( + "encoding/json" + "testing" +) + +// Shape taken from a live Keycloak 26 GET .../protocol-mappers/models: the +// server adds "id" and reorders keys, which must not count as drift. +const liveMapperList = `[ + {"id":"eb2a593f-8dc6-4791-8d7d-5c576a203fd4","name":"realm roles","protocol":"openid-connect","protocolMapper":"oidc-usermodel-realm-role-mapper","consentRequired":false,"config":{"access.token.claim":"true","introspection.token.claim":"true","claim.name":"realm_access.roles","jsonType.label":"String","multivalued":"true"}}, + {"id":"9544248a-c141-43fd-97a7-b5a918fcf9c9","name":"email","protocol":"openid-connect","protocolMapper":"oidc-usermodel-property-mapper","consentRequired":false,"config":{"user.attribute":"email","claim.name":"email"}} +]` + +func liveMappers(t *testing.T) []json.RawMessage { + t.Helper() + var items []json.RawMessage + if err := json.Unmarshal([]byte(liveMapperList), &items); err != nil { + t.Fatal(err) + } + return items +} + +func TestFindMapperByName(t *testing.T) { + id, raw := findMapperByName(liveMappers(t), "realm roles") + if id != "eb2a593f-8dc6-4791-8d7d-5c576a203fd4" || raw == nil { + t.Fatalf("got id=%q raw=%s", id, raw) + } + if id, raw := findMapperByName(liveMappers(t), "missing"); id != "" || raw != nil { + t.Fatalf("expected no match, got id=%q", id) + } +} + +func TestProtocolMapperDrift_InSyncSkipsUpdate(t *testing.T) { + desired := json.RawMessage(`{"config":{"access.token.claim":"true","claim.name":"realm_access.roles","introspection.token.claim":"true","jsonType.label":"String","multivalued":"true"},"consentRequired":false,"name":"realm roles","protocol":"openid-connect","protocolMapper":"oidc-usermodel-realm-role-mapper"}`) + _, current := findMapperByName(liveMappers(t), "realm roles") + if !definitionsMatch(desired, current) { + t.Fatal("unchanged mapper reported as drift; would re-PUT on every sync") + } +} + +func TestProtocolMapperDrift_ConfigChangeDetected(t *testing.T) { + desired := json.RawMessage(`{"name":"realm roles","protocolMapper":"oidc-usermodel-realm-role-mapper","config":{"claim.name":"roles"}}`) + _, current := findMapperByName(liveMappers(t), "realm roles") + if definitionsMatch(desired, current) { + t.Fatal("changed config.claim.name not detected") + } +} + +func TestProtocolMapperDrift_NoCurrentForcesUpdate(t *testing.T) { + if definitionsMatch(json.RawMessage(`{"name":"x"}`), nil) { + t.Fatal("missing current representation must fall through to update") + } +} + +func TestRoleDrift_CompositesStrippedBeforeCompare(t *testing.T) { + // Role GET does not return composites; the controller strips them from the + // definition and syncs them via the composites endpoint instead. + desired := removeFieldFromDefinition(json.RawMessage(`{"name":"default-roles-x","composite":true,"composites":{"realm":["offline_access"]}}`), "composites") + current := json.RawMessage(`{"id":"1","name":"default-roles-x","composite":true,"clientRole":false,"containerId":"x","attributes":{}}`) + if !definitionsMatch(desired, current) { + t.Fatal("unchanged role reported as drift") + } +} diff --git a/internal/controller/keycloakrole_controller.go b/internal/controller/keycloakrole_controller.go index f75c170..e1d6972 100644 --- a/internal/controller/keycloakrole_controller.go +++ b/internal/controller/keycloakrole_controller.go @@ -130,12 +130,17 @@ func (r *KeycloakRoleReconciler) Reconcile(ctx context.Context, req ctrl.Request } else { roleID = *existingRole.ID definition = mergeIDIntoDefinition(definition, existingRole.ID) - log.Info("updating client role", "name", roleName, "realm", realmName, "client", clientUUID) - if err := kc.UpdateClientRole(ctx, realmName, clientUUID, roleName, definition); err != nil { - RecordError(controllerName, "keycloak_api_error") - return r.updateStatus(ctx, role, false, "UpdateFailed", fmt.Sprintf("Failed to update client role: %v", err), roleID, roleName, true, clientUUID) + currentRaw, fetchErr := kc.GetClientRoleRaw(ctx, realmName, clientUUID, roleName) + if fetchErr == nil && definitionsMatch(definition, currentRaw) { + log.V(1).Info("client role already in sync, skipping update", "name", roleName) + } else { + log.Info("updating client role", "name", roleName, "realm", realmName, "client", clientUUID) + if err := kc.UpdateClientRole(ctx, realmName, clientUUID, roleName, definition); err != nil { + RecordError(controllerName, "keycloak_api_error") + return r.updateStatus(ctx, role, false, "UpdateFailed", fmt.Sprintf("Failed to update client role: %v", err), roleID, roleName, true, clientUUID) + } + log.Info("client role updated successfully", "name", roleName) } - log.Info("client role updated successfully", "name", roleName) } } else { existingRole, err := kc.GetRealmRole(ctx, realmName, roleName) @@ -150,12 +155,17 @@ func (r *KeycloakRoleReconciler) Reconcile(ctx context.Context, req ctrl.Request } else { roleID = *existingRole.ID definition = mergeIDIntoDefinition(definition, existingRole.ID) - log.Info("updating realm role", "name", roleName, "realm", realmName) - if err := kc.UpdateRealmRole(ctx, realmName, roleName, definition); err != nil { - RecordError(controllerName, "keycloak_api_error") - return r.updateStatus(ctx, role, false, "UpdateFailed", fmt.Sprintf("Failed to update realm role: %v", err), roleID, roleName, false, "") + currentRaw, fetchErr := kc.GetRealmRoleRaw(ctx, realmName, roleName) + if fetchErr == nil && definitionsMatch(definition, currentRaw) { + log.V(1).Info("realm role already in sync, skipping update", "name", roleName) + } else { + log.Info("updating realm role", "name", roleName, "realm", realmName) + if err := kc.UpdateRealmRole(ctx, realmName, roleName, definition); err != nil { + RecordError(controllerName, "keycloak_api_error") + return r.updateStatus(ctx, role, false, "UpdateFailed", fmt.Sprintf("Failed to update realm role: %v", err), roleID, roleName, false, "") + } + log.Info("realm role updated successfully", "name", roleName) } - log.Info("realm role updated successfully", "name", roleName) } }