CM-1039: Thread context.Context from Reconcile() through controller helpers#419
CM-1039: Thread context.Context from Reconcile() through controller helpers#419sebrandon1 wants to merge 1 commit intoopenshift:masterfrom
Conversation
…elpers Both istiocsr and trustmanager controllers stored a context.Context field on their Reconciler struct, initialized once in New(). The Reconcile() method receives a request-scoped context from controller-runtime but all helper methods used the stale struct field instead. This defeats cancellation and deadline propagation from the framework. Remove the ctx field from both Reconciler structs and thread the context parameter from Reconcile() through every helper method call chain.
openshift-ci-robot
added
the
jira/valid-reference
|
@sebrandon1: This pull request references CM-1039 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 "5.0.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. |
WalkthroughThis PR propagates Go context throughout IstioCSR and TrustManager controller reconciliation flows by adding ChangesContext Propagation Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sebrandon1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/istiocsr/utils.go (1)
481-486:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the original reconciliation error in the aggregate.
When
updateStatusfails, this branch currently returns the raw status-update error twice and dropsprependErr. That hides the actual root cause whenever both operations fail.Proposed fix
func (r *Reconciler) updateCondition(ctx context.Context, istiocsr *v1alpha1.IstioCSR, prependErr error) error { if err := r.updateStatus(ctx, istiocsr); err != nil { errUpdate := fmt.Errorf("failed to update %s/%s status: %w", istiocsr.GetNamespace(), istiocsr.GetName(), err) if prependErr != nil { - return utilerrors.NewAggregate([]error{err, errUpdate}) + return utilerrors.NewAggregate([]error{prependErr, errUpdate}) } return errUpdate } return prependErr }🤖 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/controller/istiocsr/utils.go` around lines 481 - 486, The updateCondition function currently aggregates the status-update error twice and discards the original reconciliation error (prependErr); change the aggregation so that when updateStatus fails you return an aggregate containing the original prependErr and the constructed errUpdate (i.e., utilerrors.NewAggregate([]error{prependErr, errUpdate})) so the root reconciliation error is preserved; keep the existing behavior when prependErr is nil (return errUpdate or the single status error as before). Reference: function updateCondition, call to r.updateStatus, variable prependErr and errUpdate.
🧹 Nitpick comments (2)
pkg/controller/istiocsr/install_instiocsr_test.go (1)
116-125: ⚡ Quick winAssert that the reconcile context is the one reaching client calls.
This only exercises the new signature today. If
reconcileIstioCSRDeploymentor one of its helpers regresses tocontext.Background(), these tests still pass because every fake ignoresctx. Passing a sentinel context here and checking it in one mockedGet/Create/UpdateWithRetrycallback would lock in the behavior this PR is meant to preserve.🤖 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/controller/istiocsr/install_instiocsr_test.go` around lines 116 - 125, The test must assert the actual context passed into controller client calls: create a sentinel context (e.g., ctx := context.WithValue(context.Background(), "sentinel", true)) in the subtest before calling r.reconcileIstioCSRDeployment, pass that ctx into the call instead of context.Background(), and configure one of the fake client callbacks on fakes.FakeCtrlClient (Get or Create or UpdateWithRetry) to check that the incoming ctx equals the sentinel (or contains the sentinel value) and fail the test if not; update testReconciler usage in the subtest to use this ctx and reference reconcileIstioCSRDeployment, FakeCtrlClient, and istiocsr so the assertion locks the intended behavior.pkg/controller/trustmanager/webhooks_test.go (1)
227-242: ⚡ Quick winVerify that the provided context is forwarded to
Exists/Patch.Right now this just compiles against the new API. A future fallback to
context.Background()insidecreateOrApplyValidatingWebhookConfigurationwould still pass. Using a sentinel context here and asserting it inside one fake client callback would make the context-threading change testable.🤖 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/controller/trustmanager/webhooks_test.go` around lines 227 - 242, The test should verify the context is forwarded to the client methods: create a sentinel context (e.g., ctxSentinel := context.WithValue(context.Background(), "sentinel", struct{}{})), call r.createOrApplyValidatingWebhookConfiguration with that ctxSentinel instead of context.Background(), and configure the fake client (fakes.FakeCtrlClient) inside the test (via mock.ExistsStub and/or mock.PatchStub) to assert the incoming ctx equals ctxSentinel (fail the test if not) before returning; update the test case that exercises createOrApplyValidatingWebhookConfiguration to set those stubs so Exists/Patch receive and validate the sentinel context.
🤖 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.
Outside diff comments:
In `@pkg/controller/istiocsr/utils.go`:
- Around line 481-486: The updateCondition function currently aggregates the
status-update error twice and discards the original reconciliation error
(prependErr); change the aggregation so that when updateStatus fails you return
an aggregate containing the original prependErr and the constructed errUpdate
(i.e., utilerrors.NewAggregate([]error{prependErr, errUpdate})) so the root
reconciliation error is preserved; keep the existing behavior when prependErr is
nil (return errUpdate or the single status error as before). Reference: function
updateCondition, call to r.updateStatus, variable prependErr and errUpdate.
---
Nitpick comments:
In `@pkg/controller/istiocsr/install_instiocsr_test.go`:
- Around line 116-125: The test must assert the actual context passed into
controller client calls: create a sentinel context (e.g., ctx :=
context.WithValue(context.Background(), "sentinel", true)) in the subtest before
calling r.reconcileIstioCSRDeployment, pass that ctx into the call instead of
context.Background(), and configure one of the fake client callbacks on
fakes.FakeCtrlClient (Get or Create or UpdateWithRetry) to check that the
incoming ctx equals the sentinel (or contains the sentinel value) and fail the
test if not; update testReconciler usage in the subtest to use this ctx and
reference reconcileIstioCSRDeployment, FakeCtrlClient, and istiocsr so the
assertion locks the intended behavior.
In `@pkg/controller/trustmanager/webhooks_test.go`:
- Around line 227-242: The test should verify the context is forwarded to the
client methods: create a sentinel context (e.g., ctxSentinel :=
context.WithValue(context.Background(), "sentinel", struct{}{})), call
r.createOrApplyValidatingWebhookConfiguration with that ctxSentinel instead of
context.Background(), and configure the fake client (fakes.FakeCtrlClient)
inside the test (via mock.ExistsStub and/or mock.PatchStub) to assert the
incoming ctx equals ctxSentinel (fail the test if not) before returning; update
the test case that exercises createOrApplyValidatingWebhookConfiguration to set
those stubs so Exists/Patch receive and validate the sentinel context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 23714fe0-8a1a-4c99-a548-c1d17e7437d7
📒 Files selected for processing (37)
pkg/controller/istiocsr/certificates.gopkg/controller/istiocsr/certificates_test.gopkg/controller/istiocsr/controller.gopkg/controller/istiocsr/controller_test.gopkg/controller/istiocsr/deployments.gopkg/controller/istiocsr/deployments_test.gopkg/controller/istiocsr/install_instiocsr_test.gopkg/controller/istiocsr/install_istiocsr.gopkg/controller/istiocsr/networkpolicies.gopkg/controller/istiocsr/rbacs.gopkg/controller/istiocsr/rbacs_test.gopkg/controller/istiocsr/serviceaccounts.gopkg/controller/istiocsr/serviceaccounts_test.gopkg/controller/istiocsr/services.gopkg/controller/istiocsr/services_test.gopkg/controller/istiocsr/test_utils.gopkg/controller/istiocsr/utils.gopkg/controller/trustmanager/certificates.gopkg/controller/trustmanager/certificates_test.gopkg/controller/trustmanager/configmaps.gopkg/controller/trustmanager/configmaps_test.gopkg/controller/trustmanager/controller.gopkg/controller/trustmanager/controller_test.gopkg/controller/trustmanager/deployments.gopkg/controller/trustmanager/deployments_test.gopkg/controller/trustmanager/install_trustmanager.gopkg/controller/trustmanager/install_trustmanager_test.gopkg/controller/trustmanager/rbacs.gopkg/controller/trustmanager/rbacs_test.gopkg/controller/trustmanager/serviceaccounts.gopkg/controller/trustmanager/serviceaccounts_test.gopkg/controller/trustmanager/services.gopkg/controller/trustmanager/services_test.gopkg/controller/trustmanager/test_utils.gopkg/controller/trustmanager/utils.gopkg/controller/trustmanager/webhooks.gopkg/controller/trustmanager/webhooks_test.go
💤 Files with no reviewable changes (2)
- pkg/controller/trustmanager/test_utils.go
- pkg/controller/istiocsr/test_utils.go
|
@sebrandon1: 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
ctx context.Contextfield from bothistiocsrandtrustmanagerReconciler structsReconcile(ctx context.Context, ...)through every helper method call chain (37 files, ~90 call sites)context.Background()explicitly (semantically correct for test entrypoints)Motivation
Both controllers stored a
context.Contextinitialized once tocontext.Background()inNew(). TheReconcile()method receives a proper request-scoped context from controller-runtime, but all helper methods used the stale struct field. This defeats cancellation and deadline propagation from the framework.This supersedes #242, which attempted to standardize context usage by replacing
context.Background()withcontext.TODO()everywhere — semantically backwards sincecontext.TODO()signals "placeholder, haven't decided yet" whilecontext.Background()is correct for top-level contexts.Test plan
go test ./pkg/controller/istiocsr/...passesgo test ./pkg/controller/trustmanager/...passesgo build ./...succeedsmake lintshows only pre-existing issues (none introduced)r.ctxreferences in either packageSummary by CodeRabbit