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) } }