Repository navigation
fix(keycloak): keep KeycloakRealmUser roles when spec.roles is unset - #3531
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughChangesRole list semantics
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant KeycloakRealmUserSpec
participant RoleSynchronization
participant Keycloak
KeycloakRealmUserSpec->>RoleSynchronization: provide realm and client role lists
alt role list is omitted or null
RoleSynchronization-->>Keycloak: skip role operations
else role list is explicitly empty
RoleSynchronization->>Keycloak: remove existing role mappings
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size/L
7eb8f54 to
4b4ad55
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/system/keycloak-operator/images/keycloak-operator/patches/keycloakrealmuser-unset-roles.diff`:
- Around line 115-119: Update the packaged KeycloakRealmUser CRD descriptions
for both clientRoles and roles to document the omitted-versus-empty role-list
behavior, matching the descriptions in keycloakrealmuser-unset-roles.diff.
Regenerate the CRD if supported; otherwise patch the chart CRD directly while
preserving its YAML formatting.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 70a9f6d7-3ad3-4997-8833-122980aa4407
📒 Files selected for processing (1)
packages/system/keycloak-operator/images/keycloak-operator/patches/keycloakrealmuser-unset-roles.diff
| clientRoles: | ||
| - description: ClientRoles is a list of client roles assigned to user. | ||
| + description: |- | ||
| + ClientRoles is a list of client roles assigned to user. | ||
| + The same rule as for Roles applies per client: omitting an entry's roles list leaves that client's roles untouched, an empty list removes all of them. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg 'keycloakrealmuser|keycloak-operator/crds|keycloakrealmusers\.yaml|role' || true
echo
echo "Target diff lines:"
sed -n '1,220p' packages/system/keycloak-operator/images/keycloak-operator/patches/keycloakrealmuser-unset-roles.diff
echo
echo "Installed CRD excerpt around ClientRoles/Roles:"
if [ -f packages/system/keycloak-operator/charts/keycloak-operator/crds/v1.edp.epam.com_keycloakrealmusers.yaml ]; then
rg -n -C 3 'clientRoles:|description:' packages/system/keycloak-operator/charts/keycloak-operator/crds/v1.edp.epam.com_keycloakrealmusers.yaml
fi
echo
echo "All generated/source CRDs with keycloakrealmusers names:"
git ls-files | rg 'keycloakrealmusers\.ya?ml$'Repository: cozystack/cozystack
Length of output: 42051
Update the packaged KeycloakRealmUser CRD descriptions.
packages/system/keycloak-operator/charts/keycloak-operator/crds/v1.edp.epam.com_keycloakrealmusers.yaml still uses the old clientRoles and roles descriptions, so installed API docs do not expose the omitted-vs-empty role contract. Regenerate or patch this CRD.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/system/keycloak-operator/images/keycloak-operator/patches/keycloakrealmuser-unset-roles.diff`
around lines 115 - 119, Update the packaged KeycloakRealmUser CRD descriptions
for both clientRoles and roles to document the omitted-versus-empty role-list
behavior, matching the descriptions in keycloakrealmuser-unset-roles.diff.
Regenerate the CRD if supported; otherwise patch the chart CRD directly while
preserving its YAML formatting.
4b4ad55 to
d3286ae
Compare
The operator reconciles user roles as an authoritative set. Under the default `full` strategy it deletes every realm role mapping missing from spec.roles, and every client role mapping missing from the matching spec.clientRoles entry. A consumer that manages only the user record and leaves those fields out therefore has all of its roles stripped on the first reconcile, `default-roles-<realm>` included, which costs the user offline_access, uma_authorization and the account client roles. Treat an omitted list as "not managed" and leave Keycloak untouched for that field, the same convention spec.identityProviders already uses. An empty but non-nil list keeps its meaning, so revoking every role is still possible with an explicit `roles: []`. Drop omitempty from the two json tags as well. Without that an explicit empty list is erased the first time the operator writes the object back, adding its own finalizer is enough, and returns indistinguishable from an omitted field, so the distinction does not survive a round trip. The patch mirrors epam/edp-keycloak-operator#372 file for file, so it can be dropped in one move once COMMIT_REF moves past the upstream merge. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
d3286ae to
5f8a820
Compare
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
Patch applies clean against the pinned COMMIT_REF, the tree builds, the full upstream suite stays green, and all three mutations of the preservation logic turn the new tests red.
I ran git apply exactly as the Dockerfile does, against COMMIT_REF=facbc36 fetched from the same URL, exit 0. The two patch files touch disjoint file sets, so the shared atomic apply cannot conflict between them. CGO_ENABLED=0 go build ./cmd succeeds and go vet is clean on both patched packages.
Mutations. Dropping the realm guard reddens unset roles keep existing realm roles, and it fails for the right reason, mock: I don't know what to return because the method call was unexpected. GetUserRealmRoleMappings(...), which is the threat rather than a proxy for it. Dropping only the client-roles guard reddens exactly one test with no collateral, so the two guards are independently pinned. Restoring omitempty on both tags reddens the serialization test at both the realm level and nested in clientRoles[].
All three spellings of unset are covered, and I decoded real JSON through the patched types rather than reasoning about it. Absent key and explicit null both land on Roles == nil, so one guard covers both and the second needs no separate assertion. Empty list decodes non-nil with len 0 and means remove everything, covered by its own case. All three are accepted by the apiserver too, roles and clientRoles[].roles are nullable: true in the shipped CRD, so the "roles": null the operator will now emit is stored rather than rejected.
Two things worth writing down.
Nothing in cozystack CI runs these tests. The Dockerfile does go mod download and go build and never go test, the package Makefile has no test: target, and cozystack's own go-unit-tests covers a different module. So roughly 140 lines of the patch are carried for upstream's benefit and never execute here. The real regression guard is that git apply is atomic, so a COMMIT_REF bump that moves the hunks fails the build loudly. Worth a line in the patch header, so the next person to bump the ref knows which protection they have and which they do not.
The tag change touches three CRDs, not one. UserClientRole is shared by keycloakrealmuser_types.go:55, keycloakclient_types.go:213 and keycloakrealmgroup_types.go:57. It is safe, all three shipped CRDs mark that field nullable: true and the Go values are identical, only the wire form moves. But if any of them had not been nullable the failure would have been ugly: the group controller adds its finalizer, client.Update serializes "roles": null, the apiserver rejects it, and every KeycloakRealmGroup carrying a clientRoles entry wedges. One sentence in the commit message saves the next reader deriving that.
Same bug class survives for KeycloakRealmGroup, which has the identical field and no nil guard. Upstream scoped to users and this mirrors it faithfully, so not this PR's job, but worth an issue so it is not lost when the patch is dropped.
2 participants
What this PR does
The vendored keycloak-operator image is built from a pinned upstream commit with
images/keycloak-operator/patches/*.diffapplied at build time. This patch is a backport of epam/edp-keycloak-operator#372 onto that pin, so it is dropped in the same commit that movesCOMMIT_REFpast the upstream merge. The bug is epam/edp-keycloak-operator#371.Under the default
fullstrategy the operator treatsspec.rolesandspec.clientRoles[].rolesas authoritative sets: after adding what is missing, it deletes every mapping the spec does not list. A resource that manages the user record and leaves those fields out therefore loses all of its realm roles on the first reconcile,default-roles-<realm>included, and with itoffline_access,uma_authorizationand theaccountclient roles. The user keeps logging in and stops being able to do anything.Upstream fixes this in two halves that only work together. The controller returns early when the desired list is nil, so an omitted field means "not managed" while an explicit
roles: []keeps its meaning of "remove every role". The json tags onRolesandClientRoles[].Rolesloseomitempty, because with it an explicit empty list is erased the first time the operator writes the object back, which adding its own finalizer already does, and comes back indistinguishable from an omitted field. Take that half away and the distinction holds only until the operator next touches the object.All eight files from the upstream PR are here. Six apply to the pin unchanged.
sync_user_roles.goand its test are rewritten against the pinned context, where the generated client package is still namedkeycloakv2(upstream renamed it tokeycloakapiafter the pin, which is the only reason those two do not apply as-is). Tests match upstream: four controller cases for unset and empty realm and client roles, two inapi/v1for the serialization round trip.Checked by extracting the pinned tarball, applying every
.diffin the patches directory in the order the Dockerfile globs them, thengo build ./...andgo test ./internal/controller/keycloakrealmuser/... ./api/..., all green. Reverting each of the three conditions in turn fails exactly the tests that cover it, and nothing else.One caveat a reader deserves up front: this patch does not reach any cluster yet. No CI path rebuilds this package, so
hack/build-matrix.shreturns an empty matrix for a change confined to it, andvalues.yamlkeeps pointing at an image built by hand in March. The same happened to9c5fbb69aand5b9b420ff, which edited this Dockerfile and left the pin alone, so the running binary is already older than the tree says. That gap is tracked in #3416 and is deliberately not addressed here: the fix for it is a choice between adding these packages to the build list and dropping the in-repo build, and the first option wakes a known mirror gap for split-host references. Landing the patch now means it is in place the moment that is resolved.Screenshots
Not a UI change.
Downstream repositories
Walked the trigger map against the diff. It touches one build-time patch file under
packages/system/keycloak-operator/images/, adds no package underpackages/apps/orpackages/extra/, and changes no values key, schema, enum, default, CRD,ApplicationDefinition, namespace, label, annotation, metric, release asset, variant or node prerequisite. Nothing in the map fires.Release note
Summary by CodeRabbit