fix(rbac): implement registry rolebinding finalization for OpenShift#1641
fix(rbac): implement registry rolebinding finalization for OpenShift#1641danielpenad wants to merge 1 commit into
Conversation
danielpenad
requested review from
akurinnoy,
btjd,
dkwon17,
ibuziuk and
rohanKanojia
as code owners
|
Hi @danielpenad. Thanks for your PR. I'm waiting for a devfile member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds OpenShift-specific cleanup for the shared registry image-puller RoleBinding during workspace finalization: when no non-deleted workspaces remain the RoleBinding is deleted; otherwise the finalized workspace's ServiceAccount is removed. Includes three tests covering removal, deletion, and missing-RoleBinding cases. ChangesRegistry RBAC Finalization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/provision/workspace/rbac/finalize.go (1)
16-24: ⚡ Quick winOrganize imports per project guidelines.
The imports are not organized according to the coding guideline, which requires three groups: (1) standard library, (2) third-party + Kubernetes, (3) project-local. Currently they are organized as project-local first, then Kubernetes, then third-party.
Run
make fmtto automatically enforce the correct import organization. As per coding guidelines, imports should be organized with third-party and Kubernetes libraries before project-local imports.🤖 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 `@pkg/provision/workspace/rbac/finalize.go` around lines 16 - 24, Reorder the import block in finalize.go to follow project guidelines: group standard library imports first (none here), then third-party and Kubernetes imports (e.g., "sigs.k8s.io/controller-runtime/pkg/client" and "github.com/devfile/api/v2/..." if treated as third-party), and finally project-local imports ("github.com/devfile/devworkspace-operator/pkg/common", ".../pkg/constants", ".../pkg/infrastructure", ".../pkg/provision/sync"); after rearranging, run make fmt to enforce the correct import grouping and formatting.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@pkg/provision/workspace/rbac/finalize.go`:
- Around line 16-24: Reorder the import block in finalize.go to follow project
guidelines: group standard library imports first (none here), then third-party
and Kubernetes imports (e.g., "sigs.k8s.io/controller-runtime/pkg/client" and
"github.com/devfile/api/v2/..." if treated as third-party), and finally
project-local imports ("github.com/devfile/devworkspace-operator/pkg/common",
".../pkg/constants", ".../pkg/infrastructure", ".../pkg/provision/sync"); after
rearranging, run make fmt to enforce the correct import grouping and formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e04e5dca-8487-4ff3-9ee8-57536f10a549
📒 Files selected for processing (2)
pkg/provision/workspace/rbac/finalize.gopkg/provision/workspace/rbac/finalize_test.go
Signed-off-by: Daniel Pena Docampo <danielpdo@inditex.com>
danielpenad
force-pushed
the
bugfix/GH-1640-image-puller-rolebinding
branch
from
fbacf7b to
30866a6
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: danielpenad, rohanKanojia The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Tested and works as expected : |
|
/che-ai-assistant ok-pr-review Review is complete. Please check the review comments below. |
tolusha
left a comment
tolusha
left a comment
There was a problem hiding this comment.
Review Summary
This PR correctly fixes the OpenShift registry image-puller RoleBinding leak described in #1640. The implementation mirrors the existing finalizeSCCRBAC pattern, has strong test coverage, and handles edge cases properly. The design is sound and the RBAC security implications have been carefully considered.
Verdict: ✅ Approve - Ready to merge with two non-blocking suggestions below.
Suggestions
1. Document migration path for pre-existing leaked subjects
The fix prevents future RBAC leaks but won't clean up pre-existing stale subjects in clusters already affected by the bug. The linked issue mentions production RoleBindings with 21K+ stale ServiceAccount subjects.
Consider adding a note to the PR description or issue #1640 about manual remediation for affected clusters. For example:
Migration note: This fix prevents future leaks. For clusters with existing stale subjects, administrators can manually clean up affected RoleBindings:
oc edit rolebinding devworkspace-registry-image-puller-<namespace>-binding -n <namespace> # Remove subjects referencing non-existent ServiceAccounts
Alternatively, a follow-up issue could track a one-time cleanup controller or script.
2. Add test for Kubernetes no-op behavior
The three new tests all use infrastructure.InitializeForTesting(infrastructure.OpenShiftv4) to verify OpenShift-specific cleanup. Consider adding a test that verifies FinalizeRBAC on Kubernetes does NOT interact with registry RoleBindings, to guard against regressions where the IsOpenShift() gate is accidentally removed.
Positive Observations
- Clean, focused fix that follows existing patterns
- Strong test assertions with two-layer error checking
- Proper use of existing helpers (
countNonDeletedWorkspaces,deleteRolebinding,removeServiceAccountFromRolebinding) - Control flow refactoring correctly ensures registry cleanup runs after default RBAC cleanup
- Idempotent operations safe for concurrent deletions
Review generated by Claude Code with /ok-pr-review
3 participants
Closes #1640
What does this PR do?
This PR fixes an OpenShift-specific RBAC leak in the shared registry image-puller RoleBinding.
When a DevWorkspace is deleted, the operator now cleans up its ServiceAccount from:
devworkspace-registry-image-puller-<namespace>-bindingIf there are no remaining non-deleted DevWorkspaces in the namespace, the shared RoleBinding is deleted entirely. The fix only affects the OpenShift
system:image-pullerRoleBinding cleanup path and does not change the existing default workspace RBAC or SCC RBAC behavior.What issues does this PR fix or reference?
Fixes the shared OpenShift registry image-puller RoleBinding subject leak described in:
Related issues:
Is it tested? How?
Yes.
Local validation:
go test ./pkg/provision/workspace/rbacmake testLive validation on OpenShift (Already done by me):
devworkspace-controller-managerto use the custom image.devworkspace-registry-image-puller-<namespace>-binding.PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-pathto trigger)v8-devworkspace-operator-e2e: DevWorkspace e2e testv8-che-happy-path: Happy path for verification integration with CheSummary by CodeRabbit
Bug Fixes
Tests