refactor: conditionally create controller dependencies based on operations
What this PR does / why we need it:
Controllers were initializing all dependencies regardless of assigned operations, causing failures when pods ran with limited operation sets (e.g., mutation-only pods failing to initialize OPA client).
Changes:
-
main.go: Conditionally create dependencies based on operations using
operations.IsAssigned()directly:- OPA client: audit or webhook operations OR external data enabled
- MutationSystem: mutation operations OR expansion enabled
- ExpansionSystem: expansion enabled
- ProviderCache: external data enabled
- CacheManager: audit or webhook operations enabled
- WebhookConfigCache/CtEvents: generate operation assigned
- CertWatcher: shared between OPA and mutation (no duplication)
- Preserved original code comments for documentation
- controller.go: Nil-check dependencies before injection
- audit/controller.go: Added nil check for OPA client before initializing audit manager
- webhook/webhook.go: Added nil check for OPA client and mutation system before initializing webhooks
Example:
// Before: always created
client, err := constraintclient.NewClient(cfArgs...)
mutationSystem := mutation.NewSystem(mutationOpts)
// After: conditional
var client *constraintclient.Client
if operations.IsAssigned(operations.Audit) || operations.IsAssigned(operations.Webhook) || *externaldata.ExternalDataEnabled {
client, err = constraintclient.NewClient(cfArgs...)
}
var mutationSystem *mutation.System
if operations.IsAssigned(operations.MutationWebhook) || operations.IsAssigned(operations.MutationController) || operations.IsAssigned(operations.MutationStatus) || *expansion.ExpansionEnabled {
mutationSystem = mutation.NewSystem(mutationOpts)
}
Which issue(s) this PR fixes: Fixes #4051 Fixes #3964
Special notes for your reviewer:
- Default behavior (all operations enabled) unchanged - backward compatible
- Controllers retain operation checks as defense in depth
- Export system always created (no operation flag)
- Uses
operations.IsAssigned()directly for cleaner, more readable code - Nil checks added at multiple layers (controller, audit, webhook) for robustness
- Status operation does NOT require OPA client or cache manager - status controllers only aggregate status CRDs without interacting with the OPA constraint framework
- Original code comments preserved for documentation and maintainability
Original prompt
Look at the issues - #4051 and #3964, the problem currently is
in main.go controller dependencies are getting injected regardless of the operations and then all controllers are spun up. For exmaple, for operation mutation and validation OPA client dependency is getting injected, however OPA client dependency is only required for validaiton. So pod running with only operation mutation will error out saying there is no OPA client initialize. SImilarly, the same problem exists for pod running only with operation=generate. The goal is to reorganize controller dependencies with operations and only inject those dependencies in controller when operations are enabled.
for example all mutation related dependencies only be added when mutation operation is enabled. While fixing this change make sure to -
- Scan deep through all the code to understand what each operation is intended for and what controllers and/or section of gatekeeper code (mutation, validation, audit, generate, status etc) is dependent on these operations
- Do not update or remove any operations or any functionality, this is purely refactoring change.
- Add missing tests for adding controller deps based on operations if there are any missing changes
- Explain refactoring in the PR comments if you can and also explain reason behind your decisions on the PR comments if you can.
- Make minimum code comments while raising this PR.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
@copilot 1. changes in operations.go are not need, use IsAssigned whenever you want to check for presence of an operations. 2. Audit opreration also required opaclient since it invokes constraint template framework, make sure that audit controller dependencies are injected properly. 3. Use nil check similarly to controller.AddToManager in audit.addtomanager and webhook.addtomanager.
Re-analyze and think hard about whole operation and controller dependencies integration, double check you changes so far and the future changes as well to make sure all the dependencies are properly initiated for only required operaitons. Focus on code readbility, clean and optimized code. Do not add unnecessary comments. Remove not needed tests.
@copilot 1. changes in operations.go are not need, use IsAssigned whenever you want to check for presence of an operations. 2. Audit opreration also required opaclient since it invokes constraint temp...
Done in commit c9ef9e9:
- Removed helper functions from operations.go - now using
IsAssigned()directly throughout - Audit operation correctly uses OPA client (included in audit/webhook/status checks)
- Added nil checks in
audit.AddToManager()andwebhook.AddToManager()similar tocontroller.AddToManager() - Removed unnecessary comments and cleaned up code
All dependencies are now properly initialized only for required operations.
@copilot main.go:385 does operation status require opaclient?
@copilot main.go:385 does operation status require opaclient?
No, status operation does not require OPA client. Fixed in commit 1a6834b.
Status controllers (constraintstatus, configstatus, mutatorstatus, etc.) only aggregate status CRDs - they don't interact with the OPA constraint framework. The CFClient field in some Adder structs is injected but never used in reconciliation logic. Removed status from both OPA client and cache manager dependencies.
@copilot please address these changes.
Addressed in commit 094d605 - restored all original comments as requested.