groupmapper: do not delete groups synced by another IdP

[kchawlani19]

Summary

• Fix multi-IdP group deletion where an IdP could delete a sole-member generated group it did not sync.
• Add regression coverage for issue Groups gets deleted on multi-IdP setup #240 while preserving legitimate cleanup when the current IdP synced the group.

Fixes #240

Test plan

• go test ./pkg/groupmapper/ -count=1
• Confirmed Groups gets deleted on multi-IdP setup #240 regression fails against the old check order and passes with the fix

Summary by CodeRabbit

• Bug Fixes
  • Corrected group removal behavior based on identity-provider synchronization status.
  • Prevented unsynced groups or groups managed by another identity provider from being deleted.
  • Groups synchronized by the current identity provider are now removed as expected.

[openshift-merge-bot]

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 26f39786-c66c-48f6-a930-0517bae602b2

📥 Commits

Reviewing files that changed from the base of the PR and between 1600eaf and 49e9145.

📒 Files selected for processing (2)

• pkg/groupmapper/groupmapper.go
• pkg/groupmapper/groupmapper_test.go

Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review.

---

Walkthrough

The group mapper now checks the current identity provider’s sync annotation before deleting generated single-user groups. Regression tests cover unsynced groups, groups synced by another identity provider, and groups synced by both providers.

Changes

Group cleanup behavior

Layer / File(s)	Summary
Sync guard and regression coverage pkg/groupmapper/groupmapper.go, pkg/groupmapper/groupmapper_test.go	removeUserFromGroup skips deletion when the group lacks the current identity provider’s synced annotation. Tests cover the supported sync states and watch events. Test helpers now create groups and sync annotations for specified identity providers.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 49e91

This change narrows group deletion to groups synced by the current IdP and adds regression coverage for both protected and legitimate cleanup paths; no actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 15

✅ Passed checks (15 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the primary fix: preventing deletion of groups synced by another identity provider.
Linked Issues check	✅ Passed	The change addresses issue #240 by checking IdP sync ownership before deleting sole-member generated groups and adds regression coverage.
Out of Scope Changes check	✅ Passed	The code and test changes directly support the multi-IdP group deletion fix and contain no unrelated changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Stable And Deterministic Test Names	✅ Passed	The PR adds only static string subtest names for Go t.Run; no Ginkgo titles or dynamic values such as IDs, timestamps, namespaces, nodes, or IPs appear in the changed tests.
Test Structure And Quality	✅ Passed	The changed tests use standard testing.T, not Ginkgo. Added cases each cover one behavior, use fake resources, and run under the existing bounded watch context with watcher cleanup.
Microshift Test Compatibility	✅ Passed	The patch changes only groupmapper code and standard Go Test* unit cases; it adds no Ginkgo e2e tests, so the MicroShift compatibility check does not apply.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	The diff changes only pkg/groupmapper code and standard Go table tests; it adds no Ginkgo e2e tests or multi-node/HA assumptions.
Topology-Aware Scheduling Compatibility	✅ Passed	The diff only changes group cleanup logic and unit tests in pkg/groupmapper; it adds no manifests, controllers, replicas, affinities, topology spread, node selectors, tolerations, or PDBs.
Ote Binary Stdout Contract	✅ Passed	The PR changes only groupmapper logic and unit tests; added lines contain no process-level stdout writes, and no main/init/suite setup code changed.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	The PR adds standard Go table-driven unit cases, not Ginkgo e2e tests; they use fake clients and watches, with no IPv4 assumptions or external connectivity.
No-Weak-Crypto	✅ Passed	The diff only reorders group-sync checks and adds annotation-based regression tests; it introduces no weak crypto, custom crypto, or secret/token comparisons.
Container-Privileges	✅ Passed	The commit changes only Go source and tests. The exact diff adds no container/Kubernetes manifests or privileged, host namespace, SYS_ADMIN, or allowPrivilegeEscalation settings.
No-Sensitive-Data-In-Logs	✅ Passed	The diff adds no production logging or sensitive-data output. Added tests use only placeholders such as user1, test-group, and other-idp; existing t.Errorf calls are unchanged.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kchawlani19
Once this PR has been reviewed and has the lgtm label, please assign flavianmissi for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[openshift-ci]

@kchawlani19: 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.