fix: scope expansion ingestion controller to audit and webhook operations - #4778
Open
pujitha24 wants to merge 7 commits into
Open
fix: scope expansion ingestion controller to audit and webhook operations#4778pujitha24 wants to merge 7 commits into
pujitha24 wants to merge 7 commits into
Conversation
…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)
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #4778 +/- ##
==========================================
- Coverage 54.49% 49.25% -5.25%
==========================================
Files 134 291 +157
Lines 12329 23396 +11067
==========================================
+ Hits 6719 11524 +4805
- Misses 5116 10824 +5708
- Partials 494 1048 +554
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:
|
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
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.
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