Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4778 +/- ##
==========================================
- Coverage 54.49% 49.65% -4.85%
==========================================
Files 134 294 +160
Lines 12329 24205 +11876
==========================================
+ Hits 6719 12019 +5300
- Misses 5116 11109 +5993
- Partials 494 1077 +583
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@abhisheksheth28 can you review this? |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
There was a problem hiding this comment.
🔵 Needs a closer look
The operation predicate test must cover the omitted MutationController non-consumer case.
Review details
Suppressed comments (1)
pkg/operations/operations_test.go:65
- This table omits the
MutationControlleroperation, even though mutation-controller-only processes are also non-consumers covered by this predicate. Add that case so the helper's intended classification of every operation—and a future accidental inclusion of the mutation controller—is tested.
"mutation-status only": {assigned: map[Operation]bool{MutationStatus: true}, expected: false},
"mutation-webhook only": {assigned: map[Operation]bool{MutationWebhook: true}, expected: false},
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The controller test must isolate operation state and restore the exact prior configuration.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
abhisheksheth28
left a comment
abhisheksheth28
left a comment
There was a problem hiding this comment.
part of #4773 still appears unaddressed: setupControllers constructs and injects an expansion system into every process. Could we limit construction to audit/webhook processes? Those callers still need an empty system when expansion is disabled because they call Expand unconditionally. Please include tests for both cases.
There was a problem hiding this comment.
🟢 Approval recommended
The gating is consistent across construction, controller registration, and readiness, with focused tests covering the operation predicate.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Fixed — |
|
@abhisheksheth28 I've pushed changes addressing your review — the branch is now at |
abhisheksheth28
left a comment
abhisheksheth28
left a comment
There was a problem hiding this comment.
Thanks for the updates. lgtm
There was a problem hiding this comment.
🟢 Approval recommended
The implementation satisfies the linked issue’s acceptance criteria with focused coverage and no unresolved correctness concerns.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…ions Motivation: setupControllers registered the ExpansionTemplate ingestion controller in every process where --enable-generator-resource-expansion is true (the default), even in status-only, generate-only, mutation-status-only, and mutation-webhook-only processes that never evaluate expanded resources. Only audit and the validating webhook call expansion.System.Expand. Beyond the wasted controller/watch registration, a non-consumer pod's readiness tracker was also unconditionally required to observe every ExpansionTemplate in the cluster, even though no reconciler ran on that pod to satisfy that expectation, so its readiness probe could get stuck permanently unsatisfied whenever any ExpansionTemplate objects exist. Approach: added operations.HasExpansionConsumerOperations(), mirroring the existing operations.HasValidationOperations()/mutation.Enabled() pattern already used to scope controllers by assigned --operation flags. expansion.Adder.Add now also skips registering the ingestion controller when this predicate is false, and main.go's readiness.SetupTracker call scopes its expansion-tracking bool by the same predicate, matching how mutationEnabled/externalDataEnabled are already gated in that file. Default (all-operations) behavior is unchanged since the default set includes both audit and webhook; pkg/controller/expansionstatus already gates independently on the status operation and is unaffected. Validation: go build ./... and go vet ./... are clean. golangci-lint run . (local install, not the exact Docker-pinned CI version) reports 0 issues. KUBEBUILDER_ASSETS=<abs path> go test ./pkg/controller/expansion/... ./pkg/operations/... ./pkg/readiness/... all pass. Added Test_HasExpansionConsumerOperations (table-driven, all operations) and TestAdd_RequiresExpansionConsumerOperation. Manually verified the failing-before/passing-after property: temporarily reverting only the new gate in expansion_controller.go makes TestAdd_RequiresExpansionConsumerOperation panic with a nil-pointer dereference (newReconciler calling mgr.GetClient() on a nil manager); restoring the gate makes it pass. That nil-manager panic is a synthetic test scenario proving the gate is load-bearing, not a real production path, since the real binary always passes a live manager. Did not run make native-test/native-race-test across the full repo, nor a live multi-pod e2e reproduction of the stuck-readiness scenario; the readiness-tracker interaction is validated by reading ready_tracker.go (every t.expansions access is already guarded by the same expansionEnabled bool that gates its allocation, mirroring the pre-existing mutationEnabled/externalDataEnabled pattern) plus the existing pkg/readiness suite passing unchanged. Searched open PRs and the issue's timeline for competing work: none found for this issue; a different open PR for a sibling issue touches unrelated files. Report: open-policy-agent#4773 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-sonnet-5 (via Claude Code)
…ration-flag test isolation Only construct expansion.System for processes that evaluate expanded resources (audit, webhook), per maintainer feedback that setupControllers was still constructing/injecting it for every process. Add an operations.AssignForTest helper so tests can swap the assigned-operations set safely under the package mutex instead of relying on the --operation flag's additive Set() semantics, and use it to fix a mutex-less data race in Test_HasExpansionConsumerOperations and add the missing mutation-controller-only case. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
17a702d to
3f506f7
Compare
What this PR does / why we need it:
setupControllersregisters the ExpansionTemplate ingestion controller (pkg/controller/expansion.Adder) in every process where--enable-generator-resource-expansionis true, which is the default. That means status-only, generate-only, mutation-status-only, and mutation-webhook-only processes register a controller and watches for a feature they never evaluate: only the audit and validating-webhook paths actually callexpansion.System.Expand(...)(pkg/audit/manager.go,pkg/webhook/policy.go). Beyond the wasted registration, a non-consumer pod's readiness tracker was also unconditionally required to observe everyExpansionTemplatein the cluster (pkg/readiness/ready_tracker.go'sSatisfied()gates on the rawexpansionEnabledbool), even though no reconciler ran on that pod to ever satisfy that expectation — so a status-only/generate-only/mutation-only pod's/readyzcould get stuck permanently unsatisfied whenever anyExpansionTemplateobjects exist in the cluster.This PR scopes both to the operations that actually consume expanded resources (audit and webhook):
operations.HasExpansionConsumerOperations(), mirroring the existingoperations.HasValidationOperations()/mutation.Enabled()pattern already used in this codebase to scope controllers by assigned--operationflags.expansion.Adder.Addnow also skips registering the ingestion controller when!operations.HasExpansionConsumerOperations(), in addition to the existing enable-flag check.main.go'sreadiness.SetupTrackercall now passes*expansion.ExpansionEnabled && operations.HasExpansionConsumerOperations()instead of the raw flag, so non-consumer pods stop tracking expansion-template expectations entirely, exactly likemutationEnabled/externalDataEnabledare already scoped in that file.Default (all-operations) behavior is unchanged, since the default operation set includes both
auditandwebhook.pkg/controller/expansionstatus(status aggregation) already gates onoperations.Statusindependently and is unaffected.Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Fixes #
Special notes for your reviewer:
Validation performed:
go build ./...andgo vet ./...: clean.golangci-lint run .(whole module, locally installed golangci-lint, not the exact Docker-pinned versionmake lintuses in CI): 0 issues on the full repo, including the changed files.KUBEBUILDER_ASSETS=<abs path to setup-envtest 1.33.0 assets> go test ./pkg/controller/expansion/... ./pkg/operations/... ./pkg/readiness/...: all pass (envtest-backed suites for the two directly-touched packages plus the readiness package, whoseSatisfied()/tracking logic this change also affects).pkg/operations.Test_HasExpansionConsumerOperations(table-driven, all seven operations plus none-assigned) andpkg/controller/expansion.TestAdd_RequiresExpansionConsumerOperation. I manually confirmed the failing-before/passing-after property required for this class of fix: temporarily reverting only the new gate inexpansion_controller.go(keeping the new test) makesTestAdd_RequiresExpansionConsumerOperationpanic with a nil-pointer dereference (newReconcilercallingmgr.GetClient()on a nil manager, sinceAdd(nil)no longer returns early); restoring the gate makes it pass. That nil-manager panic is a synthetic test scenario to prove the gate is load-bearing, not a real production path — the real binary always passes a live, non-nil manager toAdd.make native-test/make native-race-testacross the full repo, nor a live multi-pod e2e deployment reproducing the stuck-/readyzscenario end-to-end. The readiness-tracker interaction is validated by readingready_tracker.go(everyt.expansionsaccess is already guarded by the sameexpansionEnabledbool that gates its allocation, exactly mirroring the pre-existingmutationEnabled/externalDataEnabledpattern in the same file) plus the existingpkg/readinesstest suite passing unchanged with this change in place.main.go's mutation-system construction andpkg/controller/mutators/instances/mutator_controllers.go— no file overlap with this change.Report: #4773
Fixes #4773