Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4777 +/- ##
==========================================
- Coverage 54.49% 49.19% -5.30%
==========================================
Files 134 292 +158
Lines 12329 24027 +11698
==========================================
+ Hits 6719 11821 +5102
- Misses 5116 11138 +6022
- Partials 494 1068 +574
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:
|
| func TestAddSkipsRegistrationWhenMutationDisabled(t *testing.T) { | ||
| if err := flag.CommandLine.Set("operation", string(operations.Audit)); err != nil { | ||
| t.Fatalf("setting operation flag: %v", err) | ||
| } | ||
|
|
||
| a := &Adder{} | ||
| if err := a.Add(nil); err != nil { |
There was a problem hiding this comment.
🟡 Changes recommended
Focused coverage does not verify conditional mutation-system construction or mutation-enabled operation sets.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| var mutationSystem *mutation.System | ||
| if mutation.Enabled() { | ||
| mutationSystem = mutation.NewSystem(mutationOpts) | ||
| } |
There was a problem hiding this comment.
This is a valid concern.
11e51c8 to
728a4ce
Compare
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 3 out of 3 changed files in this pull request and generated 2 comments.
|
@abhisheksheth28 can you review this? |
There was a problem hiding this comment.
🟡 Changes recommended
Focused coverage for mutation-system construction and injection remains missing, as noted in the unresolved review thread.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The acknowledged focused-test gap for the main.go construction branch remains unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The tests do not verify that production setup uses the conditional constructor, leaving a stated acceptance criterion uncovered.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
| } | ||
|
|
||
| mutationSystem := mutation.NewSystem(mutationOpts) | ||
| mutationSystem := newMutationSystem(mutationOpts) |
There was a problem hiding this comment.
🟢 Approval recommended
The operation guards are consistent with existing mutation semantics and the affected paths have focused regression coverage.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Added |
|
@abhisheksheth28 I've pushed changes addressing your review — the branch is now at |
There was a problem hiding this comment.
lgtm. thanks! please pick up the CI fix from #4779 before merging so the root package tests run automatically.
There was a problem hiding this comment.
🟡 Changes recommended
The root-level construction tests are excluded from the standard CI unit-test target.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
| // setupControllers unconditionally constructing a *mutation.System: when only | ||
| // a non-mutation operation (e.g. audit) is assigned, newMutationSystem must | ||
| // return nil. | ||
| func TestNewMutationSystemDisabledWhenNoMutationOperation(t *testing.T) { |
…assigned
Motivation:
setupControllers in main.go always calls mutation.NewSystem(...), and
pkg/controller/mutators/instances.Adder.Add always creates the mutator
conflict-routing channels and registers the routeConflictEvents runnable
with the manager, regardless of which --operation values are assigned. A
pod running e.g. --operation=audit or --operation=generate only therefore
still constructs an unused mutation system and starts an unused runnable
and goroutine, even though it never ingests, applies, or reports on any
mutator.
Approach:
Gate both call sites on mutation.Enabled() (pkg/mutation/mutation.go),
which is already the single source of truth used by
pkg/controller/mutators/core.Adder.add for the same purpose:
- main.go only constructs mutationSystem via mutation.NewSystem when
mutation.Enabled() is true; otherwise it stays nil.
expansion.NewSystem already supports a nil mutation system
(pkg/expansion/system.go short-circuits Expand's mutation step when
s.mutationSystem == nil), so expansion of generated resources is
unaffected: mutators still apply when a mutation operation is present,
and are simply skipped when one isn't.
- instances.Adder.Add now returns nil immediately when mutation.Enabled()
is false, before creating the conflict-routing channels or registering
routeConflictEvents.
Traced every remaining consumer of the (now possibly nil) mutation system
to confirm none can dereference nil: pkg/webhook/mutation.go's
AddMutatingWebhook already gates on operations.IsAssigned(MutationWebhook)
before use; pkg/webhook/policy.go stores the system but never dereferences
it; pkg/controller/mutators/core/adder.go already gated on
mutation.Enabled(); and instances.Adder is the only implementer of
pkg/controller/controller.go's MutationSystemInjector, now self-gating.
This is a minimal subset of the fuller scope described in the issue; it
does not restructure how the external-data provider cache / client cert
watcher options are wired into mutationOpts, since those remain unused
(harmless) when mutation.NewSystem is never called.
Validation:
- go build ./..., go vet ./..., and golangci-lint run (local v2.12.2,
repo pins v2.9.0 via Docker which was unavailable here) on the changed
packages and the whole repo: clean, 0 issues.
- Added a targeted unit test, TestAddSkipsRegistrationWhenMutationDisabled,
in pkg/controller/mutators/instances/mutator_controllers_test.go. It
narrows the process --operation flag to audit only and calls
(&Adder{}).Add(nil): against the pre-fix code this panics with a nil
pointer dereference on mgr.GetScheme(); against the fix it passes.
Verified both directions, plus stability under
`go test -run TestAddSkipsRegistrationWhenMutationDisabled -count=5`
and `go test -race -count=3` for the whole package.
- go test ./pkg/controller/mutators/... ./pkg/webhook/... ./pkg/expansion/...
and the broader ./pkg/controller/... (with KUBEBUILDER_ASSETS from
setup-envtest) all pass.
- Not run here: the repo's full make native-test/native-race-test across
./pkg/... ./apis/... ./cmd/gator/..., and any live e2e/dev-stack
scenario. No Kubernetes API/CRD types changed, so make generate/manifests
should be a no-op.
Report: open-policy-agent#4772
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Assisted-by: claude-sonnet-5 (via Claude Code)
…operation flag state Adds TestAddProceedsWhenMutationEnabled alongside the existing mutation-disabled test, and runs each in a re-exec'd subprocess so their process-wide "operation" flag mutations no longer leak into each other or future tests in the package. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
…peration classes Extracts the mutation.Enabled() guard around mutation.NewSystem in setupControllers into newMutationSystem, and adds subprocess-isolated tests exercising it for both a non-mutation operation (audit) and a mutation operation, addressing the main.go:516 review comment. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
The subprocess isolation added for the new operation-flag-narrowing tests re-execs the current test binary (os.Args[0]) with a compile-time-constant -test.run filter, which gosec's G204 flags as a "tainted input" false positive since neither argument is externally controlled. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
e552dd6 to
db8d818
Compare
What this PR does / why we need it:
Gatekeeper unconditionally constructs a
mutation.SysteminsetupControllersand unconditionally creates the mutator conflict-routing channels plus registers therouteConflictEventsrunnable inpkg/controller/mutators/instances.Adder.Add, even for pods where no mutation operation (mutation-webhook,mutation-controller,mutation-status) is assigned (e.g.--operation=auditor--operation=generateonly). That means a status-only, audit-only, or generate-only process still pays for an unused mutation system and an unused runnable/goroutine.This PR gates both of those on the existing
mutation.Enabled()single source of truth (already used bypkg/controller/mutators/core.Adder.add), so:mutationSysteminmain.gostaysnilunless a mutation operation is assigned.expansion.NewSystemalready supports anilmutation system (seepkg/expansion/system.go,s.mutationSystem == nilshort-circuitsexpand), so expansion of generated resources continues to work unchanged: it applies mutators when a mutation operation is present, and simply skips mutation when one isn't.instances.Adder.Addnow returns immediately whenmutation.Enabled()is false, before creating the conflict-routing channels or registeringrouteConflictEventswith the manager.This is a minimal, surgical subset of the full scope described in the issue (it does not additionally restructure how the external-data provider cache / client cert watcher options are wired into
mutationOpts, since those are simply unused whenmutation.NewSystemis never called — no behavior change needed there).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:
I traced every consumer of
MutationSystem/mutationSystemthat could now receivenil:pkg/webhook/mutation.goAddMutatingWebhookalready returns early unlessoperations.IsAssigned(operations.MutationWebhook)is true, before touchingdeps.MutationSystem.pkg/webhook/policy.gostores amutationSystemfield but never dereferences it.pkg/controller/mutators/core/adder.go(*Adder).addalready guarded onmutation.Enabled()before this change (pre-existing).pkg/controller/controller.go'sMutationSystemInjectorinterface is implemented only byinstances.Adder, which now guards onmutation.Enabled()itself, per this diff.pkg/expansion/system.goalready nil-checkss.mutationSystembefore callingMutate.So there is no remaining path where a
nil*mutation.Systemgets dereferenced.Validation:
go build ./...,go vet ./..., andgolangci-lint run ./pkg/controller/mutators/instances/... .(locally installed golangci-lint v2.12.2; repo pins v2.9.0 via Docker inmake lint, which I could not run here because Docker wasn't available in this environment — the local binary run against the same config found 0 issues on both the changed packages and the whole repo) are all clean. I added a targeted unit test,TestAddSkipsRegistrationWhenMutationDisabled, that narrows the process--operationflag toauditand calls(&Adder{}).Add(nil); it fails with a nil-pointer panic onmgr.GetScheme()against the pre-fix code and passes against the fix (verified both ways, including repeated runs with-count=5and-race -count=3to confirm the flag-mutating test itself is stable). I also ran the full existing suites for the directly affected packages with envtest:go test ./pkg/controller/mutators/... ./pkg/webhook/... ./pkg/expansion/...(KUBEBUILDER_ASSETS viasetup-envtest) and the broadergo test ./pkg/controller/...— all pass. I could not run the fullmake native-test/make native-race-testtargets across the entire repo (./pkg/... ./apis/... ./cmd/gator/...) or any live e2e/dev-stack scenario in this environment; I believe this change is safe based on the nil-safety trace above plus the passing targeted and package-level tests.Fixes #4772