NO-JIRA: Fix SA label reconciliation and correct exist/update/event logic for all Istio CSR resources#381
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@bharath-b-rh: 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. |
WalkthroughReconciler flows were standardized across Istio CSR controller resources: existing-object handling now detects changes and only updates when needed, emitting ResourceAlreadyExists and Reconciled events conditionally. Tests for service accounts were expanded, ServiceAccount type handling added to drift detection, and a cert-manager IssuerRef type was adjusted; an OWNERS file was removed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bharath-b-rh 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
![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/serviceaccounts_test.go (1)
83-89: Consider adding timeout to prevent test hangs.The
assertEventsfunctions read from the recorder channel without a timeout. If the event isn't emitted due to a code change or bug, the test will hang indefinitely rather than fail with a clear message.♻️ Suggested improvement using select with timeout
assertEvents: func(t *testing.T, r *Reconciler) { rec := r.eventRecorder.(*record.FakeRecorder) select { case evt := <-rec.Events: if !strings.Contains(evt, "ResourceAlreadyExists") || !strings.Contains(evt, "serviceaccount resource already exists") { t.Errorf("createOrApplyServiceAccounts() event: %q, want ResourceAlreadyExists and serviceaccount already exists", evt) } case <-time.After(time.Second): t.Error("expected ResourceAlreadyExists event but none received") } },Also applies to: 105-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/istiocsr/serviceaccounts_test.go` around lines 83 - 89, The test's assertEvents closures block on reading rec.Events and can hang; update the assertions in pkg/controller/istiocsr/serviceaccounts_test.go (the assertEvents functions used around the createOrApplyServiceAccounts tests) to use a select that waits on rec.Events and a time.After timeout (e.g., 1s) so the test fails fast with a clear t.Error when no event is received, and add the required import for time; apply the same change to the other assertEvents at the second occurrence (the block around lines 105-111).
🤖 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/serviceaccounts.go`:
- Around line 28-36: The current if/else lumps "not exist" with "exists and
unchanged" and logs "serviceaccount resource already exists" when exist==false;
split the logic so you first check exist: if exist { if
hasObjectChanged(desired, fetched) { perform UpdateWithRetry and emit reconciled
event } else { log resource exists and is in expected state } } else { create
the resource (use r.CreateWithRetry(r.ctx, desired)), handle errors like the
Update path, and emit an event indicating creation (use r.eventRecorder.Eventf
with appropriate message referencing serviceAccountName and istiocsr) }. Ensure
you reference the existing symbols hasObjectChanged, UpdateWithRetry,
CreateWithRetry (or the controller's create helper), r.eventRecorder.Eventf,
serviceAccountName and istiocsr.
---
Nitpick comments:
In `@pkg/controller/istiocsr/serviceaccounts_test.go`:
- Around line 83-89: The test's assertEvents closures block on reading
rec.Events and can hang; update the assertions in
pkg/controller/istiocsr/serviceaccounts_test.go (the assertEvents functions used
around the createOrApplyServiceAccounts tests) to use a select that waits on
rec.Events and a time.After timeout (e.g., 1s) so the test fails fast with a
clear t.Error when no event is received, and add the required import for time;
apply the same change to the other assertEvents at the second occurrence (the
block around lines 105-111).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1da0bd67-29f6-4abf-a100-f168da508873
📒 Files selected for processing (3)
pkg/controller/istiocsr/serviceaccounts.gopkg/controller/istiocsr/serviceaccounts_test.gopkg/controller/istiocsr/utils.go
Signed-off-by: Bharath B <bhb@redhat.com>
bharath-b-rh
force-pushed
the
istiocsr-sa
branch
from
d6e0301 to
8c7b23b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controller/istiocsr/rbacs.go (1)
99-113: Consider extracting the repeated exist/update/event block into a shared helper.The same reconciliation branch is duplicated six times; a helper would reduce drift risk and make future behavior changes safer.
♻️ Refactor direction
+// reconcileExistingResource centralizes: +// - optional ResourceAlreadyExists event +// - hasObjectChanged check +// - UpdateWithRetry + Reconciled event +// - unchanged-state log +func (r *Reconciler) reconcileExistingResource(...) error { + ... +}Also applies to: 195-209, 256-270, 299-313, 343-357, 386-400
🤖 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 99 - 113, The exist/update/event reconciliation block around variables and functions like exist, istioCSRCreateRecon, hasObjectChanged(desired, fetched), r.UpdateWithRetry(r.ctx, desired), r.eventRecorder.Eventf(...), r.log, roleName, desired, fetched, and istiocsr is duplicated multiple times; extract it into a single helper (e.g., reconcileResourceOrNoop) that accepts the common inputs (ctx, desired, fetched, resource identifier/name, createRecon flag, and any recorder/log) and performs the exist-check, conditional update via UpdateWithRetry, and event logging so all six occurrences call this helper (replace the repeated branches at the sites that include the same logic such as the block shown and the ones at the other listed ranges) to centralize behavior and reduce duplication and drift risk.
🤖 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.go`:
- Around line 99-113: The exist/update/event reconciliation block around
variables and functions like exist, istioCSRCreateRecon,
hasObjectChanged(desired, fetched), r.UpdateWithRetry(r.ctx, desired),
r.eventRecorder.Eventf(...), r.log, roleName, desired, fetched, and istiocsr is
duplicated multiple times; extract it into a single helper (e.g.,
reconcileResourceOrNoop) that accepts the common inputs (ctx, desired, fetched,
resource identifier/name, createRecon flag, and any recorder/log) and performs
the exist-check, conditional update via UpdateWithRetry, and event logging so
all six occurrences call this helper (replace the repeated branches at the sites
that include the same logic such as the block shown and the ones at the other
listed ranges) to centralize behavior and reduce duplication and drift risk.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d85c3886-ad0f-40e8-848d-9389fe48b0ca
📒 Files selected for processing (9)
pkg/controller/istiocsr/OWNERSpkg/controller/istiocsr/certificates.gopkg/controller/istiocsr/deployments.gopkg/controller/istiocsr/networkpolicies.gopkg/controller/istiocsr/rbacs.gopkg/controller/istiocsr/serviceaccounts.gopkg/controller/istiocsr/serviceaccounts_test.gopkg/controller/istiocsr/services.gopkg/controller/istiocsr/utils.go
💤 Files with no reviewable changes (1)
- pkg/controller/istiocsr/OWNERS
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controller/istiocsr/utils.go
- pkg/controller/istiocsr/serviceaccounts.go
bharath-b-rh
changed the title
|
@bharath-b-rh: 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. |
2 participants
Summary
Fixes Istio CSR ServiceAccount reconciliation so label (and other metadata) drift is detected and corrected, and aligns the “existing resource” reconcile path across Istio CSR-managed resources so logging and events match reality.
Problem
if exist && changed … else { log “already exists…” }structure meant the else ran when exist was false, so the controller could log “resource already exists and is in expected state” on the create path. Reconciled events could also fire even when no update ran.Changes
hasObjectChanged(metadata drift via existing metadata comparison); UpdateWithRetry when drift is detected; Reconciled only after a successful update; expanded unit tests (create, unchanged, modified/update, update error, istioCSRCreateRecon + ResourceAlreadyExists, non-blocking event asserts with timeout).