refactor: separate Config process exclusions from validation data sync - #4784
anneheartrecord wants to merge 2 commits into
Conversation
setupControllers always built the cache-manager registrar, the sync metrics cache, the cache manager and the expectations pruner, and AddToManager always registered the cache manager runnable and the sync controller. Since open-policy-agent#4423 the Config controller tolerated a missing constraint client through a no-op CFDataClient, so mutation-webhook-only pods carried the whole validation data-sync stack for the sole purpose of updating process exclusions. Name the capabilities the wiring actually depends on in pkg/operations and let each of them decide independently: - the cache manager, its registrar, the sync controller and the expectations pruner are only built when an operation syncs validation data; - the Config reconciler takes the process excluder directly, so exclusions are applied without a cache manager behind them and syncOnly is skipped; - the Config controller is not registered at all when nothing in the pod reads the Config resource, and readiness stops waiting on it in that case; - Dependencies.Validate rejects a plan that does not match the assigned operations instead of letting it surface as a nil dereference later. NeedsValidationDataSync is derived from HasValidationOperations, so status-only pods drop the stack as soon as open-policy-agent#4770 lands. Signed-off-by: Charles Cheng <charlescheng@rezona.ai>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0c10b5500
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| func NeedsConfigReconciliation() bool { | ||
| return NeedsProcessExclusions() || | ||
| NeedsValidationDataSync() || | ||
| NeedsGenerateConfigNotifications() | ||
| } |
There was a problem hiding this comment.
When a pod is assigned only mutation-controller or mutation-status, every predicate here is false, so the Config controller is skipped even though its reconciler is also the sole consumer of Config.spec.readiness.statsEnabled. Consequently, enabling readiness stats no longer calls Tracker.EnableStats() on these pods, and the mutation expectation diagnostics in statsPrinter remain disabled precisely when their readiness is stuck. Include this Config responsibility in the reconciliation capability or handle the setting separately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed - mutation.Enabled() covers mutation-status and mutation-controller, so those pods do run a tracker, statsPrinter still reports their mutator expectations, and this reconciler is the only thing that reads spec.readiness.statsEnabled.
Fixed in 2a81529. Readiness stats is now its own capability and NeedsConfigReconciliation() includes it, so the Config controller registers again on mutation-only pods. Dependencies.Validate correspondingly requires the process excluder wherever that controller registers, since the reconciler needs somewhere to put the exclusions it parses.
Side effect worth calling out: NeedsConfigReconciliation() is now true for every operation set. The gating the issue asks for is unaffected - the cache manager, registrar, sync controller and pruner are still only built for validation data sync, and mutation-webhook-only still gets exclusions without a no-op data client - but no pod can skip the Config controller until the readiness stats switch moves out of it. I kept the predicate rather than deleting it so that stays visible; happy to drop it if you would rather not carry an always-true one.
The Config reconciler is the only consumer of Config.spec.readiness.statsEnabled, and every operation set gets a readiness tracker - mutation-only pods included, whose mutator expectations are exactly what that verbose logging reports. Skipping Config reconciliation on pods that neither exclude processes nor sync data therefore silenced the readiness diagnostics precisely where they are needed. Name the readiness stats switch as its own capability and fold it into NeedsConfigReconciliation, which makes Config reconciliation unconditional today. Dependencies.Validate now demands the process excluder wherever the Config controller registers, since the reconciler needs somewhere to put the exclusions it parses. Signed-off-by: Charles Cheng <charlescheng@rezona.ai>
There was a problem hiding this comment.
🟡 Changes recommended
Several gates remain effectively unconditional or depend on an unmerged PR, leaving stated operation-specific behavior unmet.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Separates Config process exclusions from validation data-sync infrastructure.
Changes:
- Adds operation capability predicates and dependency validation.
- Conditionally initializes data-sync components.
- Supports Config reconciliation without a cache manager and adds tests.
File summaries
| File | Description |
|---|---|
main.go |
Gates data-sync component construction. |
pkg/controller/controller.go |
Validates dependencies and conditionally registers sync components. |
pkg/controller/controller_test.go |
Tests dependency validation. |
pkg/controller/config/config_controller.go |
Separates exclusions from cache-manager synchronization. |
pkg/controller/config/config_controller_test.go |
Updates reconciler setup. |
pkg/controller/config/config_capability_test.go |
Tests cache-manager-free reconciliation. |
pkg/operations/capabilities.go |
Defines operation capability predicates. |
pkg/operations/capabilities_test.go |
Tests capability combinations. |
pkg/operations/testing.go |
Adds operation assignment test support. |
pkg/readiness/ready_tracker.go |
Gates Config readiness expectations. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| events chan event.GenericEvent | ||
| cm *cachemanager.CacheManager | ||
| ) | ||
| if operations.NeedsValidationDataSync() { |
| func NeedsValidationDataSync() bool { | ||
| return HasValidationOperations() |
| func NeedsConfigReconciliation() bool { | ||
| return NeedsProcessExclusions() || | ||
| NeedsValidationDataSync() || | ||
| NeedsGenerateConfigNotifications() || | ||
| NeedsReadinessStats() |
What this PR does / why we need it:
setupControllersalways built the cache-manager registrar, the sync metrics cache, the cache manager and the expectations pruner, andcontroller.AddToManageralways registered the cache manager runnable and the sync controller. #4423 made that stack tolerate a missing constraint client via a no-opCFDataClient, which unblocked the mutation-webhook startup fix but left mutation-only pods carrying validation data-sync infrastructure only so the Config controller could update process exclusions.This splits the two responsibilities of the Config resource apart.
pkg/operations/capabilities.gonames the four capabilities the wiring depends on:NeedsValidationDataSyncNeedsProcessExclusionsNeedsGenerateConfigNotificationsNeedsConfigReconciliationWith those in place:
setupControllersonly builds the registrar, sync metrics cache, cache manager and pruner when an operation syncs validation data. Mutation-webhook-only no longer instantiates a no-opCFDataClientor registers data watches.AddToManageronly adds the cache manager runnable and the sync controller when a cache manager exists.Dependencies.Validatefails setup with a message naming the assigned operations when the plan does not match them — a missing cache manager / sync channel / watch manager for data-sync operations, a cache manager built for operations that do not sync, or a missing process excluder. Previously a mismatch would have surfaced as a nil dereference somewhere downstream.*process.Excluderdirectly. With a cache manager it still swaps throughExcludeProcessesso the re-list scheduling is unchanged; without one it replaces the excluder in place and skipsUpsertSourceentirely.mutation-controller/mutation-status). The readiness tracker stops expecting Config for exactly that case, so--operation=mutation-statusdoes not wait forever on an observation that can no longer arrive.Audit, validating webhook and generate keep their current behavior: audit and webhook still sync referential data, generate still gets Config-triggered VAP scope reconciliation, and
ConfigPodStatusis still written for every operation set that reconciles Config. Default all-operations behavior is unchanged.NeedsValidationDataSyncis defined in terms ofHasValidationOperationsrather than re-deriving the operation list, so status-only pods drop the cache manager, sync controller and pruner as soon as #4770 / #4780 lands — no change needed here.Which issue(s) this PR fixes:
Fixes #4775
Special notes for your reviewer:
Tests:
pkg/operations/capabilities_test.gowalks each capability across the operation sets that matter (audit, validating webhook, mutation-webhook-only, mutation-status-only, mutation-controller-only, generate-only, all).pkg/controller/controller_test.gocoversDependencies.Validate, including the case where a cache manager is handed to operations that do not sync data.pkg/controller/config/config_capability_test.gopins the registration gate (mutation-status-only registers nothing even with no dependencies set; mutation-webhook still errors on a missing excluder) and reconciles a Config with a nil cache manager, asserting the mutation-webhook exclusions land andConfigPodStatusis still written. Reverting either gate fails these.I deliberately left
main_test.goalone — #4779 and #4780 are both creating it, and #4776 covers the table-drivensetupControllersmatrix.This touches
main.goalongside #4777 / #4778 / #4779 / #4780, but in a different block; happy to rebase in whatever order you merge them.