CM-976: Update the RBAC reconcile logic for istio-csr - #388
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughReconciler now aligns desired RBAC resources with live objects' exact names before in-place updates; ClusterRoleBinding RoleRef immutable changes are detected and trigger delete+recreate. Tests expanded with new roleRef reconciliation cases and assertions on client calls. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (1)
pkg/controller/istiocsr/rbacs_test.go (1)
166-170: Assert the new update/replace behavior, not just the setup.These additions only let the reconciler reach the new branch. They still do not verify the behavior this PR is protecting:
UpdateWithRetryshould see the fetched live name withGenerateNamecleared, and aRoleRefmismatch should go throughDelete+Createrather thanUpdateWithRetry. With the current fakes, the pre-fix behavior would still pass.Also applies to: 187-188, 197-201, 211-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/istiocsr/rbacs_test.go` around lines 166 - 170, The tests currently only exercise the new branch without asserting the new update/replace semantics: update the rbacs_test.go cases that use testClusterRole() (the ones setting name "cert-manager-istio-csr") to assert that UpdateWithRetry is invoked with the live object whose GenerateName has been cleared (i.e., capture the object passed into the fake client's UpdateWithRetry and assert obj.GenerateName == ""), and for the RoleRef-mismatch scenario arrange the fake Update to fail so the reconciler must call Delete followed by Create (assert Delete and Create were called on the fake client in that order). Locate helpers/tests referencing testClusterRole(), cert-manager-istio-csr, UpdateWithRetry, GenerateName, and RoleRef to add these expectations and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controller/istiocsr/rbacs.go`:
- Around line 195-203: When you detect an immutable RoleRef change
(rbacRoleBindingRefModified) and delete the old binding (r.Delete(fetched)),
also clear the stored ClusterRoleBinding name in the controller's status (the
field that holds roleBindingName) and persist that status update before
returning so the next reconcile won't look up the stale name and attempt
label-adoption; keep the rest of the flow (set exist = false) but add a status
clear/write step referencing the same status field that holds roleBindingName so
recreation via GenerateName remains idempotent.
---
Nitpick comments:
In `@pkg/controller/istiocsr/rbacs_test.go`:
- Around line 166-170: The tests currently only exercise the new branch without
asserting the new update/replace semantics: update the rbacs_test.go cases that
use testClusterRole() (the ones setting name "cert-manager-istio-csr") to assert
that UpdateWithRetry is invoked with the live object whose GenerateName has been
cleared (i.e., capture the object passed into the fake client's UpdateWithRetry
and assert obj.GenerateName == ""), and for the RoleRef-mismatch scenario
arrange the fake Update to fail so the reconciler must call Delete followed by
Create (assert Delete and Create were called on the fake client in that order).
Locate helpers/tests referencing testClusterRole(), cert-manager-istio-csr,
UpdateWithRetry, GenerateName, and RoleRef to add these expectations and
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4eec910f-c9e7-4d4d-a741-490af193e158
📒 Files selected for processing (2)
pkg/controller/istiocsr/rbacs.gopkg/controller/istiocsr/rbacs_test.go
| // ClusterRoleBinding.RoleRef is immutable; a new ClusterRole name (e.g. after delete/recreate | ||
| // with GenerateName) cannot be applied via Update. | ||
| if exist && rbacRoleBindingRefModified(desired, fetched) { | ||
| r.log.V(1).Info("clusterrolebinding roleRef changed, deleting for recreation (roleRef is immutable)", "name", roleBindingName) | ||
| if err := r.Delete(r.ctx, fetched); err != nil { | ||
| return common.FromClientError(err, "failed to delete %s clusterrolebinding to replace roleRef", roleBindingName) | ||
| } | ||
| exist = false | ||
| } |
There was a problem hiding this comment.
Keep the immutable RoleRef replacement path idempotent.
This deletes the old binding and recreates the replacement with GenerateName. If the following status update fails, the next reconcile still looks up the stale old name from status, skips label-based adoption, and creates another generated ClusterRoleBinding. That turns a transient status-write failure into duplicate cluster-scoped RBAC objects.
Suggested fix
if exist && rbacRoleBindingRefModified(desired, fetched) {
r.log.V(1).Info("clusterrolebinding roleRef changed, deleting for recreation (roleRef is immutable)", "name", roleBindingName)
if err := r.Delete(r.ctx, fetched); err != nil {
return common.FromClientError(err, "failed to delete %s clusterrolebinding to replace roleRef", roleBindingName)
}
+ // Reuse the old name so a retry after a status-write failure reconciles
+ // the same binding instead of minting another generated one.
+ desired.SetName(fetched.GetName())
+ desired.SetGenerateName("")
exist = false
}Also applies to: 210-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controller/istiocsr/rbacs.go` around lines 195 - 203, When you detect an
immutable RoleRef change (rbacRoleBindingRefModified) and delete the old binding
(r.Delete(fetched)), also clear the stored ClusterRoleBinding name in the
controller's status (the field that holds roleBindingName) and persist that
status update before returning so the next reconcile won't look up the stale
name and attempt label-adoption; keep the rest of the flow (set exist = false)
but add a status clear/write step referencing the same status field that holds
roleBindingName so recreation via GenerateName remains idempotent.
| // desired is built with GenerateName for create; for update the name must match the live object. | ||
| desired.SetName(fetched.GetName()) | ||
| desired.SetGenerateName("") |
There was a problem hiding this comment.
@bharath-b-rh, since we are reseting the resource name all the time, cannot we hardcode the name instead of using generateName? Why it has to be dynamic?
There was a problem hiding this comment.
istio-csr is designed for multi-tenant, hence the dynamic naming for cluster scoped resources.
|
Pre-merge Validations:
/label qe-approved |
openshift-ci
Bot
added
the
qe-approved
1dad031 to
21affd7
Compare
openshift-ci
Bot
added
the
needs-rebase
Signed-off-by: chiragkyal <ckyal@redhat.com>
21affd7 to
3107367
Compare
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controller/istiocsr/rbacs_test.go (1)
727-776: Tighten these helpers to assert exact CRB replacement counts.They currently prove only that at least one CRB delete/create happened. A regression that issues duplicate replacements in a single reconcile would still pass. Since the fake client already records
DeleteandCreateargs, assert exactly one CRB delete and one CRB create, and validate the deleted binding name too.As per coding guidelines, focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/istiocsr/rbacs_test.go` around lines 727 - 776, The helpers (clusterRoleBindingDeleteCount, assertClusterRoleBindingRoleRefReplaceNoUpdate, assertClusterRoleBindingRoleRefReplaceUsesDeleteCreate) only assert "at least one" delete/create; change them to assert exactly one CRB Delete and exactly one CRB Create and validate the deleted binding name: use m.DeleteCallCount() and iterate DeleteArgsForCall to ensure exactly one delete target is a *rbacv1.ClusterRoleBinding and its GetName() equals the expected binding name, and assert m.CreateCallCount() == 1 and inspect the single CreateArgsForCall to validate created ClusterRoleBinding metadata.name, empty generateName, and RoleRef.Name == wantRoleRefName; update assertClusterRoleBindingRoleRefReplaceNoUpdate to fail if DeleteCallCount() != 1 and assertClusterRoleBindingRoleRefReplaceUsesDeleteCreate to check both counts == 1 and validate the deleted binding name as well as the created object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/controller/istiocsr/rbacs_test.go`:
- Around line 727-776: The helpers (clusterRoleBindingDeleteCount,
assertClusterRoleBindingRoleRefReplaceNoUpdate,
assertClusterRoleBindingRoleRefReplaceUsesDeleteCreate) only assert "at least
one" delete/create; change them to assert exactly one CRB Delete and exactly one
CRB Create and validate the deleted binding name: use m.DeleteCallCount() and
iterate DeleteArgsForCall to ensure exactly one delete target is a
*rbacv1.ClusterRoleBinding and its GetName() equals the expected binding name,
and assert m.CreateCallCount() == 1 and inspect the single CreateArgsForCall to
validate created ClusterRoleBinding metadata.name, empty generateName, and
RoleRef.Name == wantRoleRefName; update
assertClusterRoleBindingRoleRefReplaceNoUpdate to fail if DeleteCallCount() != 1
and assertClusterRoleBindingRoleRefReplaceUsesDeleteCreate to check both counts
== 1 and validate the deleted binding name as well as the created object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56d130dd-647b-4ade-a48c-2b28bc70c432
📒 Files selected for processing (2)
pkg/controller/istiocsr/rbacs.gopkg/controller/istiocsr/rbacs_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controller/istiocsr/rbacs.go`:
- Around line 451-457: When deleting the existing clusterrolebinding in the
block that checks rbacRoleBindingRefModified(desired, fetched), handle the
benign NotFound case returned by r.Delete(r.ctx, fetched) by checking
apierrors.IsNotFound(err) (or equivalent helper) and treating it as success
instead of returning an error; keep other errors wrapped with
common.FromClientError as before. Ensure you reference the same variables:
r.Delete(r.ctx, fetched), fetched, roleBindingName, and recreate flag, so if err
is NotFound set recreate=true and return recreate, nil (or simply continue)
rather than returning a failure.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a11351e-33dd-4a2d-af46-7bd92b094994
📒 Files selected for processing (2)
pkg/controller/istiocsr/rbacs.gopkg/controller/istiocsr/rbacs_test.go
|
/retitle NO-JIRA: Update the RBAC reconcile logic for istio-csr |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@chiragkyal: This pull request explicitly references no jira issue. DetailsIn response to this:
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. |
|
/retitle CM-976: Update the RBAC reconcile logic for istio-csr |
openshift-ci
Bot
changed the title
|
@chiragkyal: This pull request references CM-976 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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. |
Signed-off-by: chiragkyal <ckyal@redhat.com>
|
/lgtm |
openshift-ci
Bot
added
docs-approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bharath-b-rh, chiragkyal 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
|
@chiragkyal: all tests passed! 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. |
3 participants
Summary
Fix ClusterRole and ClusterRoleBinding update path to copy the live object's name onto the desired spec before calling UpdateWithRetry, since the desired object is built with GenerateName (for creation) and would otherwise fail the update with a mismatched name.
Extract ClusterRoleBinding modification logic into
handleClusterRoleBindingModificationto correctly handle the immutable RoleRef field.Add comprehensive unit tests covering the new RoleRef-change code path, including delete failure, successful delete-and-recreate, and assertions that the live object name is propagated correctly during updates.