fix(rbac): handle immutable roleRef in RoleBinding reconciliation#3321
Conversation
RoleBinding.roleRef is immutable in Kubernetes. When the referenced Role name changes (e.g. during upgrade from v6.5 to v6.6), delete and recreate the RoleBinding instead of attempting to update. Signed-off-by: Vitalii Parfonov <vparfono@redhat.com>
📝 WalkthroughWalkthroughRoleBinding reconciliation in internal/reconcile/rbac.go was rewritten from a controllerutil.CreateOrUpdate-based approach to an explicit Get-then-manage flow: create if missing, delete/recreate if RoleRef changed, or update Subjects/OwnerReferences otherwise. A new test suite validates these three scenarios. ChangesRoleBinding Reconciliation
Estimated code review effort: 2 (Simple) | ~12 minutes Related PRs: None identified. Suggested labels: None Suggested reviewers: None 🐰 A binding once bound, now torn and remade, 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/assign @xperimental |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/reconcile/rbac.go (2)
30-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffDuplicate logic with
ClusterRoleBinding.This Get/Create/Delete-recreate/Update flow (lines 30-54) mirrors
ClusterRoleBinding(lines 56-80) almost line-for-line, differing only in namespaced vs. cluster scope and OwnerReferences handling. Worth extracting a shared helper (e.g. keyed on aRoleRef-getter interface) to avoid the two implementations drifting apart over time.🤖 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 `@internal/reconcile/rbac.go` around lines 30 - 54, The RoleBinding reconciliation flow duplicates the ClusterRoleBinding logic almost exactly, so factor the shared Get/Create/Delete-recreate/Update sequence into a common helper to keep both paths aligned. Keep the scope-specific differences in separate wrappers such as RoleBinding and ClusterRoleBinding, and pass in the object-specific details like namespace handling and OwnerReferences updates via a small interface or callback. Use the existing RoleBinding and ClusterRoleBinding functions as the call sites for the shared helper so future changes only need to be made in one place.
50-53: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUpdate is called unconditionally even when nothing changed.
Unlike the previous
controllerutil.CreateOrUpdate(which only writes when the mutate callback actually changes the object), this path always callsUpdateeven ifSubjects/OwnerReferencesalready matchdesired. That's an unnecessary API write (and resourceVersion bump) on every reconcile.♻️ Proposed fix to skip no-op updates
+ if reflect.DeepEqual(existing.Subjects, desired.Subjects) && reflect.DeepEqual(existing.OwnerReferences, desired.OwnerReferences) { + return nil + } existing.Subjects = desired.Subjects existing.OwnerReferences = desired.OwnerReferences log.V(3).Info("Updating roleBinding", "name", desired.Name, "namespace", desired.Namespace) return k8Client.Update(context.TODO(), existing)Requires adding
"reflect"to the 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 `@internal/reconcile/rbac.go` around lines 50 - 53, The role binding reconcile path always performs an Update even when the object is already in the desired state, causing unnecessary API writes and resourceVersion bumps. In internal/reconcile/rbac.go, update the logic around the existing.Subjects and existing.OwnerReferences assignment in the roleBinding handling so it first compares them to desired using reflect.DeepEqual and skips k8Client.Update when nothing changed. Keep the existing log.V(3).Info and Update call only for real mutations, and add reflect to the imports.
🤖 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 `@internal/reconcile/rbac.go`:
- Around line 30-54: The RoleBinding reconciliation flow duplicates the
ClusterRoleBinding logic almost exactly, so factor the shared
Get/Create/Delete-recreate/Update sequence into a common helper to keep both
paths aligned. Keep the scope-specific differences in separate wrappers such as
RoleBinding and ClusterRoleBinding, and pass in the object-specific details like
namespace handling and OwnerReferences updates via a small interface or
callback. Use the existing RoleBinding and ClusterRoleBinding functions as the
call sites for the shared helper so future changes only need to be made in one
place.
- Around line 50-53: The role binding reconcile path always performs an Update
even when the object is already in the desired state, causing unnecessary API
writes and resourceVersion bumps. In internal/reconcile/rbac.go, update the
logic around the existing.Subjects and existing.OwnerReferences assignment in
the roleBinding handling so it first compares them to desired using
reflect.DeepEqual and skips k8Client.Update when nothing changed. Keep the
existing log.V(3).Info and Update call only for real mutations, and add reflect
to the imports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d29ba02e-90e4-4d18-b962-d5cb8e291cb0
📒 Files selected for processing (2)
internal/reconcile/rbac.gointernal/reconcile/rbac_test.go
|
/assign @jcantrill |
|
@vparfonov: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jcantrill, vparfonov The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
openshift-merge-bot
Bot
merged commit c02ef14
into
openshift:master
3 participants
Description
RoleBinding.roleRefis immutable in Kubernetes. When the referencedRolename changes (e.g. during upgrade from v6.5 to v6.6), delete and recreate theRoleBindinginstead of attempting to update./cc
/assign
Links
Summary by CodeRabbit