OCPBUGS-63228: groupmapper: avoid mutating original group users slice when removing user

[liouk]

When removing a user from a group, removeUserFromGroup gets a pointer to the group object living in the informer cache via the lister, then builds the new user list using append on a sub-slice of it:

  newUsers = append(updatedGroup.Users[0:userIdx], updatedGroup.Users[userIdx+1:]...)

Because the sub-slice shares the backing array with the cached object, append mutates the cached object's user list in place -- the removed user disappears and the last user gets duplicated. When the informer re-indexes the group after an update event, it diffs the corrupted old object against the new one. Since the removed user is no longer in the corrupted old object, its index entry is never cleaned up and becomes a permanent phantom.

On subsequent logins, GroupsFor(user) returns the phantom group from the index. groupsDiff sees that the cache already includes the group, matches the provider's desired state, and computes no changes needed -- so the user is never actually added to the group.

addUserToGroup has a similar issue.

This PR fixes this issue by creating a slice with a new backing array to store the updated user list. addUserToGroup can simply use the deep-copied object as this already uses a new backing array.

[openshift-ci-robot]

@liouk: This pull request references Jira Issue OCPBUGS-63228, which is invalid:

• expected the bug to target either version "4.22." or "openshift-4.22.", but it targets "4.21" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

When removing a user from a group, removeUserFromGroup gets a pointer to the group object living in the informer cache via the lister, then builds the new user list using append on a sub-slice of it:

 newUsers = append(updatedGroup.Users[0:userIdx], updatedGroup.Users[userIdx+1:]...)

Because the sub-slice shares the backing array with the cached object, append mutates the cached object's user list in place -- the removed user disappears and the last user gets duplicated. When the informer re-indexes the group after an update event, it diffs the corrupted old object against the new one. Since the removed user is no longer in the corrupted old object, its index entry is never cleaned up and becomes a permanent phantom.

On subsequent logins, GroupsFor(user) returns the phantom group from the index. groupsDiff sees that the cache already includes the group, matches the provider's desired state, and computes no changes needed -- so the user is never actually added to the group.

addUserToGroup has a similar issue.

This PR fixes this issue by creating a slice with a new backing array to store the updated user list. addUserToGroup can simply use the deep-copied object as this already uses a new backing array.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[liouk]

/jira refresh

[openshift-ci-robot]

@liouk: This pull request references Jira Issue OCPBUGS-63228, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (4.22.0) matches configured target version for branch (4.22.0)
• bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @xingxingxia

Details

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[liouk]

Since we do not currently have any unit tests that cover this issue, I will create a proof PR to demonstrate the bug and then incorporate the appropriate unit tests in this one.

/hold

[liouk]

Proof PR: #212
Failing unit tests: https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_oauth-server/212/pull-ci-openshift-oauth-server-master-unit/2026678658705395712

These tests have also been added in this PR, and must succeed for the fix to be proven.

[openshift-ci]

@liouk: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[liouk]

Unit tests on proof PR (#212) failed as expected, while the ones in this PR are passing.

/hold cancel

[ibihim]

/approve

[everettraven]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: everettraven, ibihim, liouk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [ibihim]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[liouk]

Verified via:

• targeted unit tests in this PR and the proof PR (DO-NOT-MERGE: Add unit tests that prove bug OCPBUGS-63228 #212); discussed above
• I've also been reproducing the issue using this script -- eventually after ~100 iterations we hit this problem. With the fix in place, I've never been able to reproduce this, even after running the script for hours (2k+ iterations)

Marking as verified based on the above.

/verified by @liouk

cc @xingxingxia

[openshift-ci-robot]

@liouk: This PR has been marked as verified by @liouk.

Details

In response to this:

Verified via:

• targeted unit tests in this PR and the proof PR (DO-NOT-MERGE: Add unit tests that prove bug OCPBUGS-63228 #212); discussed above
• I've also been reproducing the issue using this script -- eventually after ~100 iterations we hit this problem. With the fix in place, I've never been able to reproduce this, even after running the script for hours (2k+ iterations)

Marking as verified based on the above.

/verified by @liouk

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[liouk]

/cherrypick release-4.21 release-4.20

[openshift-cherrypick-robot]

@liouk: once the present PR merges, I will cherry-pick it on top of release-4.21 in a new PR and assign it to you.

Details

In response to this:

/cherrypick release-4.21 release-4.20

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[openshift-ci-robot]

@liouk: Jira Issue Verification Checks: Jira Issue OCPBUGS-63228
✔️ This pull request was pre-merge verified.
✔️ All associated pull requests have merged.
✔️ All associated, merged pull requests were pre-merge verified.

Jira Issue OCPBUGS-63228 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓

Details

In response to this:

When removing a user from a group, removeUserFromGroup gets a pointer to the group object living in the informer cache via the lister, then builds the new user list using append on a sub-slice of it:

 newUsers = append(updatedGroup.Users[0:userIdx], updatedGroup.Users[userIdx+1:]...)

Because the sub-slice shares the backing array with the cached object, append mutates the cached object's user list in place -- the removed user disappears and the last user gets duplicated. When the informer re-indexes the group after an update event, it diffs the corrupted old object against the new one. Since the removed user is no longer in the corrupted old object, its index entry is never cleaned up and becomes a permanent phantom.

On subsequent logins, GroupsFor(user) returns the phantom group from the index. groupsDiff sees that the cache already includes the group, matches the provider's desired state, and computes no changes needed -- so the user is never actually added to the group.

addUserToGroup has a similar issue.

This PR fixes this issue by creating a slice with a new backing array to store the updated user list. addUserToGroup can simply use the deep-copied object as this already uses a new backing array.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[openshift-cherrypick-robot]

@liouk: new pull request created: #213

Details

In response to this:

/cherrypick release-4.21 release-4.20

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.