Remove referencingWorkloads/referenceCount from config CRD statuses - #5631
Remove referencingWorkloads/referenceCount from config CRD statuses#5631ChrisJBurns wants to merge 6 commits into
Conversation
Remove the ReferencingWorkloads and ReferenceCount status fields from the six config CRDs (MCPOIDCConfig, MCPAuthzConfig, MCPExternalAuthConfig, MCPTelemetryConfig, MCPWebhookConfig, MCPToolConfig) and the machinery that maintained them. After the indexer + stale-scan-removal work, that denormalized list was kept fresh only to power a kubectl printer column — it no longer backs deletion protection, which the finalizer recomputes live via findReferencingWorkloads. - Drop both fields (and the "References" printcolumn markers) from the v1beta1 status structs and the v1alpha1 type markers; regenerate deepcopy. - Remove the status-population paths in Reconcile, the workload watches and their map handlers (which existed only to refresh the list), and the workloadReferenceCount helper. Keep findReferencingWorkloads + the field indexes for the on-demand deletion check, and the DeletionBlocked condition. - Update unit tests and the stale comments in the workload controllers. BREAKING CHANGE: status.referencingWorkloads and status.referenceCount are removed from all six config CRDs. Anything reading those fields (automation, scripts, the kubectl "References" column) will no longer find them. Deletion protection is unchanged. See #5607. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drop the envtest specs that asserted Status.ReferencingWorkloads / ReferenceCount tracking and pruning (those fields are gone). Deletion-protection coverage is retained, rewritten to assert via the DeletionBlocked condition and object existence rather than the removed list. See #5607. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Regenerate the six config CRDs and crd-api.md after removing the referencingWorkloads/referenceCount status fields and the "References" printer column. See #5607. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5631 +/- ##
==========================================
+ Coverage 70.27% 70.32% +0.04%
==========================================
Files 648 647 -1
Lines 66011 65738 -273
==========================================
- Hits 46391 46231 -160
+ Misses 16279 16159 -120
- Partials 3341 3348 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
WorkloadRefsEqual was the "did the ref list change?" guard before the config status writes removed in this PR. With those callers gone it had no production references (only its own test), the symmetric leftover to the deleted workloadReferenceCount helper. Remove it and its test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The config-CRD sections still described a referencingWorkloads status field. Reword the three references to drop it and note that referrers are recomputed on demand at deletion time (finalizer + field-indexed lookup). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cover the full create-ref -> deletion blocked (DeletionBlocked condition) -> remove ref -> deleted transition for MCPExternalAuthConfig, mirroring the ToolConfig/Telemetry suites. With the workload watches removed, unblock now rides the controller's 30s requeue, so the spec allows up to 45s for the config to be garbage-collected after the reference is gone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the review findings (commits on top of the original three):
|
JAORMX
left a comment
There was a problem hiding this comment.
Re-reviewed the current head (b837630) since my July approval. The core change is sound — I verified the invariant holds across all six controllers: handleDeletion still recomputes referrers live via the field indexers, fails closed on query error, blocks with DeletionBlocked=True + 30s requeue, and only removes the finalizer at zero refs. The removed Watches(...) calls were purely status-list maintenance; the indexers survive and are still queried; RBAC markers unchanged; CRD YAMLs (both files/crds and templates) and docs/operator/crd-api.md are consistently regenerated; existing objects with populated status fields drop them silently on read (fields were optional, conversion None, v1alpha1 embeds v1beta1 status). Net −3688 lines of genuinely dead machinery.
Two things to address before merge:
1. Stale chainsaw test asserts a removed status field (will fail e2e).
test/e2e/chainsaw/operator/single-tenancy/test-scenarios/mcpwebhookconfig/chainsaw-test.yaml:127-129 still asserts:
status:
referencingWorkloads:
- kind: MCPServer
name: missing-webhook-serverv1alpha1 MCPWebhookConfig embeds v1beta1.MCPWebhookConfigStatus, from which this field was removed — this scenario will fail against the new schema. It's the only remaining reference in test/.
2. api-break-allowed label not applied.
This PR removes fields from served v1beta1 CRD status — exactly what CONTRIBUTING.md's API-stability section prohibits without the escape hatch, and the api-compat CI workflow will fail without the api-break-allowed label. The PR body already carries the required break + migration notes, so it just needs the label (and the jq replacement one-liner would be worth preserving in docs/operator/ — it'll be lost once this merges, since there's no upgrade-guide entry for it).
Non-blocking observations from the panel:
- Pre-existing gap worth a follow-up issue:
MCPExternalAuthConfigReconciler.findReferencingWorkloadsonly checks MCPServer + MCPRemoteProxy — a VirtualMCPServer referencing an MCPExternalAuthConfig viaoutgoingAuth.backends[].externalAuthConfigRefisn't counted, so the finalizer won't block that deletion. Pre-existing on main, not a regression here, but this PR touches everyfindReferencingWorkloadsso it's the right moment to notice. - Residual
watchRBAC verbs on workload CRDs for the config controllers that no longer watch anything — cosmetic, could be dropped in a follow-up. DeletionBlockedclearing is asymmetric: the webhook controller clears it on unblock, the other five don't (object vanishes with a lingeringTruecondition). Inherited, low-impact.- Behavior note: unblock is now up to ~30s slower (requeue-driven rather than watch-driven) — the new integration test already accounts for this with the 45s timeout. Reasonable trade for the deleted machinery.
|
Not sure whether to close this or leave open. The fix is sound but it is a breaking change on the |
|
I think we can still fix this @ChrisJBurns |
Summary
Removes the
status.referencingWorkloadsandstatus.referenceCountfields (and theReferencesprinter column) from the six config CRDs —MCPOIDCConfig,MCPAuthzConfig,MCPExternalAuthConfig,MCPTelemetryConfig,MCPWebhookConfig,MCPToolConfig— and deletes the watch/rebuild machinery that maintained them.After the field-indexer rollout (#5599) and stale-scan removal (#5626), that denormalized list was kept fresh only to populate a
kubectlprinter column. It no longer backs deletion protection: the finalizer recomputes referrers live (findReferencingWorkloads) before allowing deletion. So the stored list was pure overhead — every workload change reconciled the config purely to refresh a count — and this removes it.Completes the cleanup tracked in #5607.
status.referencingWorkloadsandstatus.referenceCountare removed from all six config CRDs. Anyone relying on these status fields for automated workflows — scripts, GitOps assertions, dashboards, or thekubectl get"References" column — will no longer find them. There is no replacement field; if you need the set of referrers, list the workloads and filter by their config-ref. For example: