CM-1039: Thread context.Context from Reconcile() through controller helpers#419
Open
sebrandon1 wants to merge 1 commit into
Open
CM-1039: Thread context.Context from Reconcile() through controller helpers#419sebrandon1 wants to merge 1 commit into
sebrandon1 wants to merge 1 commit into
Conversation
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (37)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (25)
WalkthroughPropagates Go contexts through IstioCSR and TrustManager controllers: removes stored reconciler context, adds ChangesContext propagation through controller reconciliation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[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
force-pushed
the
ctx-consistency-rebase
branch
from
df73b9f to
0dd33b8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/controller/trustmanager/configmaps_test.go (1)
321-321: ⚡ Quick winAdd one explicit context-propagation assertion in this test path.
Using
context.Background()updates the call site, but it still doesn’t verify that the same context reachesGet/Exists/Patch. Add a case withcontext.WithValue(...)and assert the fake client callbacks receive that value to lock in the PR’s core behavior.🤖 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/configmaps_test.go` at line 321, Add a test case that verifies context propagation to the client callbacks by replacing the call using context.Background() with a variant that uses a context created by context.WithValue(...) carrying a unique key/value and asserting the fake client's Get/Exists/Patch callbacks see that same value; specifically, update the test that calls r.createOrApplyDefaultCAPackageConfigMap(...) to invoke it with the new ctx and modify the fake client callbacks (used in the test harness) to check ctx.Value(...) matches the injected value so the assertion ensures createOrApplyDefaultCAPackageConfigMap and its internal Get/Exists/Patch calls propagate the exact context.pkg/controller/istiocsr/install_instiocsr_test.go (1)
124-124: ⚡ Quick winAssert context propagation here, not just the new signature.
This still passes if
reconcileIstioCSRDeploymentfalls back tocontext.Background()somewhere downstream. Pass a sentinel context and assert one of the fake client callbacks sees it.Suggested test shape
+ type ctxKey struct{} + reqCtx := context.WithValue(context.Background(), ctxKey{}, "reconcile-test") + + mock.CreateCalls(func(ctx context.Context, obj client.Object, option ...client.CreateOption) error { + if ctx.Value(ctxKey{}) != "reconcile-test" { + t.Fatalf("request context was not propagated") + } + return nil + }) + - err := r.reconcileIstioCSRDeployment(context.Background(), istiocsr, true) + err := r.reconcileIstioCSRDeployment(reqCtx, istiocsr, true)🤖 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` at line 124, Replace the use of context.Background() in the test call to r.reconcileIstioCSRDeployment with a sentinel context (e.g., ctx := context.WithValue(context.Background(), "sentinelKey", "sentinel")) and update the fake client(s) used by reconcileIstioCSRDeployment to capture the incoming ctx in their callback(s) (the fake Create/Update/Delete/Get hooks or whatever fake client method you already use) and assert the captured ctx carries the sentinel value; ensure the test calls r.reconcileIstioCSRDeployment(ctx, true) and fails if the fake client callback sees a different or nil context so you detect any fallback to context.Background().
🤖 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/controller/istiocsr/install_instiocsr_test.go`:
- Line 124: Replace the use of context.Background() in the test call to
r.reconcileIstioCSRDeployment with a sentinel context (e.g., ctx :=
context.WithValue(context.Background(), "sentinelKey", "sentinel")) and update
the fake client(s) used by reconcileIstioCSRDeployment to capture the incoming
ctx in their callback(s) (the fake Create/Update/Delete/Get hooks or whatever
fake client method you already use) and assert the captured ctx carries the
sentinel value; ensure the test calls r.reconcileIstioCSRDeployment(ctx, true)
and fails if the fake client callback sees a different or nil context so you
detect any fallback to context.Background().
In `@pkg/controller/trustmanager/configmaps_test.go`:
- Line 321: Add a test case that verifies context propagation to the client
callbacks by replacing the call using context.Background() with a variant that
uses a context created by context.WithValue(...) carrying a unique key/value and
asserting the fake client's Get/Exists/Patch callbacks see that same value;
specifically, update the test that calls
r.createOrApplyDefaultCAPackageConfigMap(...) to invoke it with the new ctx and
modify the fake client callbacks (used in the test harness) to check
ctx.Value(...) matches the injected value so the assertion ensures
createOrApplyDefaultCAPackageConfigMap and its internal Get/Exists/Patch calls
propagate the exact context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1eba83f8-e38a-4ef9-9e0e-8f29e2e077e3
📒 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
✅ Files skipped from review due to trivial changes (3)
- pkg/controller/trustmanager/webhooks_test.go
- pkg/controller/istiocsr/services_test.go
- pkg/controller/trustmanager/deployments_test.go
🚧 Files skipped from review as they are similar to previous changes (21)
- pkg/controller/trustmanager/certificates_test.go
- pkg/controller/trustmanager/rbacs_test.go
- pkg/controller/istiocsr/certificates.go
- pkg/controller/trustmanager/services_test.go
- pkg/controller/trustmanager/webhooks.go
- pkg/controller/trustmanager/services.go
- pkg/controller/istiocsr/serviceaccounts.go
- pkg/controller/istiocsr/install_istiocsr.go
- pkg/controller/istiocsr/networkpolicies.go
- pkg/controller/istiocsr/controller_test.go
- pkg/controller/trustmanager/deployments.go
- pkg/controller/trustmanager/utils.go
- pkg/controller/istiocsr/utils.go
- pkg/controller/istiocsr/services.go
- pkg/controller/trustmanager/serviceaccounts.go
- pkg/controller/trustmanager/controller.go
- pkg/controller/istiocsr/controller.go
- pkg/controller/istiocsr/rbacs.go
- pkg/controller/trustmanager/certificates.go
- pkg/controller/istiocsr/certificates_test.go
- pkg/controller/trustmanager/install_trustmanager.go
Member
Author
|
/retest |
…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.
sebrandon1
force-pushed
the
ctx-consistency-rebase
branch
from
0dd33b8 to
5074109
Compare
Member
Author
|
/retest |
Contributor
|
@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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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