Skip to content

Simplify config-CRD reference tracking (indexers, framework dedup, on-demand deletion) #5607

Description

@ChrisJBurns

Summary

The six config-CRD controllers (MCPOIDCConfig, MCPAuthzConfig, MCPExternalAuthConfig, MCPTelemetryConfig, MCPWebhookConfig, MCPToolConfig) each track which workloads reference them in Status.ReferencingWorkloads. The way this is maintained is more complex and less efficient than it needs to be, and the stored list itself is arguably not worth keeping. This issue proposes three incremental, mostly-independent improvements — from a safe drop-in win to a longer-term simplification.

Related: refactor work started in #5599 (sharing the stale-ref scan) — this issue reframes that as "the scan can largely be deleted, not deduplicated."

TL;DR

  1. Use field indexers instead of list-all-and-filter when computing referencing workloads (cheaper on every reconcile, idiomatic, already used for spec.groupRef).
  2. Delete the hand-rolled "stale ref" scan — controller-runtime already wakes the previously-referenced config for free.
  3. (Longer term) Drop Status.ReferencingWorkloads — it's effectively only there for a printer column, it's expensive to maintain at scale, and it doesn't help deletion protection (which re-queries live anyway).

What's in place today
  • Each config stores Status.ReferencingWorkloads []{Kind,Name} + a ReferenceCount (surfaced as the kubectl "References" printer column).
  • Each controller watches the referring workload types (MCPServer / VirtualMCPServer / MCPRemoteProxy) via EnqueueRequestsFromMapFunc.
  • The watch map function does two things: enqueue the config the workload references now, and a "stale ref" scan — list all configs of that type and check each one's Status.ReferencingWorkloads to find configs that still list a workload that no longer references them, so they reconcile and prune the stale entry.
  • On reconcile, findReferencingWorkloads rebuilds the list by listing every workload in the namespace and filtering in memory.
  • Deletion protection uses a finalizer: handleDeletion calls findReferencingWorkloads live and blocks while the count > 0.
The redundancy / inefficiency
  • The stale-ref scan is redundant. controller-runtime's EnqueueRequestsFromMapFunc runs the map function on both the old and new object on every update (verified in our version, v0.23.3 — pkg/handler/enqueue_mapped.go enqueues ObjectOld and ObjectNew). So when a workload switches its ref from A→B (or drops it, or is deleted), the framework already enqueues A via the old object. The hand-written scan is re-solving a problem the framework solves — and it's copy-pasted across all six controllers.
  • The rebuild is O(all workloads). findReferencingWorkloads pulls every workload in the namespace from the cache and filters in memory, on every reconcile. At hundreds of workloads this is wasteful CPU/allocations on the hot path.
  • The stored list earns almost nothing. It is read by nothing except the printer column; the deletion decision recomputes live and does not trust it.
What we get from the framework for free

EnqueueRequestsFromMapFunc already maps old + new on updates (and the object on create/delete). So the map function only needs to return "the config this workload references", and the config a workload just left is woken automatically — no status scan required. Pair it with a predicate so it only fires on real ref changes (avoids status churn). Missed-event edge cases (e.g. a delete while the controller was down) are healed by the level-triggered reconcile + periodic resync, same as today.

Better way: field indexers for the rebuild

Register a field index on each workload's ref field and query it directly:

mgr.GetFieldIndexer().IndexField(ctx, &MCPServer{}, "spec.oidcConfigRef", extractRefName)
// ...
r.List(ctx, &servers, client.InNamespace(ns), client.MatchingFields{"spec.oidcConfigRef": cfg.Name})

This returns only the matching workloads instead of scanning all of them. It's the standard controller-runtime/kubebuilder pattern and is already used in this repo for spec.groupRef (setupGroupRefFieldIndexes, mcpserverentry_controller). Same output, much cheaper at scale, and it benefits both the reconcile rebuild and the deletion check.

Longer term: compute references on-demand, drop the stored list

Status.ReferencingWorkloads only exists to back the printer column. It does not protect deletion — handleDeletion already re-runs the lookup live before removing the finalizer, so the stored list is pure display overhead. Maintaining it means watching every workload type and rewriting config status on each change, which scales poorly (hundreds of workloads → constant status writes for a count nobody reads programmatically).

The conventional pattern (e.g. Kubernetes PVC protection: pkg/controller/volume/pvcprotection) is finalizer + on-demand check at deletion time, no stored list. We could drop ReferencingWorkloads (and the watches/index that feed it), keep just the finalizer + live check, and either drop the "References" column or keep a lightweight count if the UX is worth it. This removes the entire watch + rebuild + store + scan apparatus.


Efficiency / outcome

  • Reconcile rebuild: O(all workloads) in-memory scan → O(matching workloads) indexed lookup.
  • Watch path: delete the per-controller stale-ref scan (and its shared helper + getters) — net code removal, not addition.
  • At scale (hundreds of workloads/configs): far fewer wasted reconciles and status writes; if we go all the way to on-demand, we stop watching workloads purely to maintain a display count.
  • No behavior change for steps 1–2: Status.ReferencingWorkloads, the count column, and finalizer-based deletion protection all stay identical — just computed more efficiently.

Suggested order

  1. Field indexer for findReferencingWorkloads (safe, drop-in, no behavior change).
  2. Delete the stale-ref scan; lean on framework old+new enqueue + a predicate (verify with the operator integration suite).
  3. Discuss whether to drop Status.ReferencingWorkloads and move fully to on-demand deletion checks.

Metadata

Metadata

Assignees

No one assigned

    Labels

    needs-triageIssue needs initial triage by a maintainer

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions