From e9b83094c2f341baafbd1ea4730d905e2dd7bd9b Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Wed, 24 Jun 2026 00:52:32 +0100 Subject: [PATCH 1/2] Index MCPOIDCConfig referencing-workload lookups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit findReferencingWorkloads rebuilt the MCPOIDCConfig reference list — on every reconcile and on every deletion check — by listing all MCPServers, VirtualMCPServers, and MCPRemoteProxies in the namespace and filtering them in memory. That is O(all workloads) and scales poorly as a namespace fills up. Register field indexes on the three workloads' oidcConfigRef fields and query them with client.MatchingFields, so each lookup returns only the workloads that actually reference the config. This mirrors the existing spec.groupRef indexes (setupGroupRefFieldIndexes, mcpserverentry_controller) and is registered in SetupWithManager, so it applies in production and the integration suite alike. Behavior is unchanged — the computed reference list is identical, just produced by an indexed lookup instead of a full in-memory scan. This is the template for the same change across the other config controllers (see #5607). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../controllers/mcpoidcconfig_controller.go | 108 +++++++++++++----- .../mcpoidcconfig_controller_test.go | 6 +- .../reconciler_test_helpers_test.go | 21 +++- 3 files changed, 103 insertions(+), 32 deletions(-) diff --git a/cmd/thv-operator/controllers/mcpoidcconfig_controller.go b/cmd/thv-operator/controllers/mcpoidcconfig_controller.go index 9f15db04cb..cfe89ca089 100644 --- a/cmd/thv-operator/controllers/mcpoidcconfig_controller.go +++ b/cmd/thv-operator/controllers/mcpoidcconfig_controller.go @@ -272,46 +272,83 @@ func (r *MCPOIDCConfigReconciler) handleDeletion( return ctrl.Result{}, nil } +// Field-index keys backing findReferencingWorkloads. MCPServer and MCPRemoteProxy +// both reference the config via spec.oidcConfigRef; VirtualMCPServer nests it +// under spec.incomingAuth. The indexes are registered in SetupWithManager. +const ( + oidcConfigRefIndexKey = "spec.oidcConfigRef" + vmcpOIDCConfigRefIndexKey = "spec.incomingAuth.oidcConfigRef" +) + +// indexMCPServerByOIDCConfigRef extracts the MCPOIDCConfig name an MCPServer +// references, for the field index. Returns nil when there is no reference so +// unreferencing servers are not indexed under the empty key. +func indexMCPServerByOIDCConfigRef(obj client.Object) []string { + server, ok := obj.(*mcpv1beta1.MCPServer) + if !ok || server.Spec.OIDCConfigRef == nil || server.Spec.OIDCConfigRef.Name == "" { + return nil + } + return []string{server.Spec.OIDCConfigRef.Name} +} + +// indexVirtualMCPServerByOIDCConfigRef extracts the MCPOIDCConfig name a +// VirtualMCPServer references via spec.incomingAuth, for the field index. +func indexVirtualMCPServerByOIDCConfigRef(obj client.Object) []string { + vmcp, ok := obj.(*mcpv1beta1.VirtualMCPServer) + if !ok || vmcp.Spec.IncomingAuth == nil || + vmcp.Spec.IncomingAuth.OIDCConfigRef == nil || vmcp.Spec.IncomingAuth.OIDCConfigRef.Name == "" { + return nil + } + return []string{vmcp.Spec.IncomingAuth.OIDCConfigRef.Name} +} + +// indexMCPRemoteProxyByOIDCConfigRef extracts the MCPOIDCConfig name an +// MCPRemoteProxy references, for the field index. +func indexMCPRemoteProxyByOIDCConfigRef(obj client.Object) []string { + proxy, ok := obj.(*mcpv1beta1.MCPRemoteProxy) + if !ok || proxy.Spec.OIDCConfigRef == nil || proxy.Spec.OIDCConfigRef.Name == "" { + return nil + } + return []string{proxy.Spec.OIDCConfigRef.Name} +} + // findReferencingWorkloads returns the workload resources (MCPServer, VirtualMCPServer, and MCPRemoteProxy) // that reference this MCPOIDCConfig via their OIDCConfigRef field. +// +// Each lookup is served by a field index (registered in SetupWithManager) so the +// query returns only the referencing workloads instead of listing every workload +// in the namespace and filtering in memory. func (r *MCPOIDCConfigReconciler) findReferencingWorkloads( ctx context.Context, oidcConfig *mcpv1beta1.MCPOIDCConfig, ) ([]mcpv1beta1.WorkloadReference, error) { - // Find referencing MCPServers - refs, err := ctrlutil.FindWorkloadRefsFromMCPServers(ctx, r.Client, oidcConfig.Namespace, oidcConfig.Name, - func(server *mcpv1beta1.MCPServer) *string { - if server.Spec.OIDCConfigRef != nil { - return &server.Spec.OIDCConfigRef.Name - } - return nil - }) - if err != nil { - return nil, err + var refs []mcpv1beta1.WorkloadReference + + serverList := &mcpv1beta1.MCPServerList{} + if err := r.List(ctx, serverList, client.InNamespace(oidcConfig.Namespace), + client.MatchingFields{oidcConfigRefIndexKey: oidcConfig.Name}); err != nil { + return nil, fmt.Errorf("failed to list MCPServers by oidcConfigRef: %w", err) + } + for i := range serverList.Items { + refs = append(refs, mcpv1beta1.WorkloadReference{Kind: mcpv1beta1.WorkloadKindMCPServer, Name: serverList.Items[i].Name}) } - // Also check VirtualMCPServers vmcpList := &mcpv1beta1.VirtualMCPServerList{} - if err := r.List(ctx, vmcpList, client.InNamespace(oidcConfig.Namespace)); err != nil { - return nil, fmt.Errorf("failed to list VirtualMCPServers: %w", err) + if err := r.List(ctx, vmcpList, client.InNamespace(oidcConfig.Namespace), + client.MatchingFields{vmcpOIDCConfigRefIndexKey: oidcConfig.Name}); err != nil { + return nil, fmt.Errorf("failed to list VirtualMCPServers by oidcConfigRef: %w", err) } - for _, vmcp := range vmcpList.Items { - if vmcp.Spec.IncomingAuth != nil && - vmcp.Spec.IncomingAuth.OIDCConfigRef != nil && - vmcp.Spec.IncomingAuth.OIDCConfigRef.Name == oidcConfig.Name { - refs = append(refs, mcpv1beta1.WorkloadReference{Kind: mcpv1beta1.WorkloadKindVirtualMCPServer, Name: vmcp.Name}) - } + for i := range vmcpList.Items { + refs = append(refs, mcpv1beta1.WorkloadReference{Kind: mcpv1beta1.WorkloadKindVirtualMCPServer, Name: vmcpList.Items[i].Name}) } - // Check MCPRemoteProxies proxyList := &mcpv1beta1.MCPRemoteProxyList{} - if err := r.List(ctx, proxyList, client.InNamespace(oidcConfig.Namespace)); err != nil { - return nil, fmt.Errorf("failed to list MCPRemoteProxies: %w", err) + if err := r.List(ctx, proxyList, client.InNamespace(oidcConfig.Namespace), + client.MatchingFields{oidcConfigRefIndexKey: oidcConfig.Name}); err != nil { + return nil, fmt.Errorf("failed to list MCPRemoteProxies by oidcConfigRef: %w", err) } - for _, proxy := range proxyList.Items { - if proxy.Spec.OIDCConfigRef != nil && proxy.Spec.OIDCConfigRef.Name == oidcConfig.Name { - refs = append(refs, mcpv1beta1.WorkloadReference{Kind: mcpv1beta1.WorkloadKindMCPRemoteProxy, Name: proxy.Name}) - } + for i := range proxyList.Items { + refs = append(refs, mcpv1beta1.WorkloadReference{Kind: mcpv1beta1.WorkloadKindMCPRemoteProxy, Name: proxyList.Items[i].Name}) } ctrlutil.SortWorkloadRefs(refs) @@ -321,6 +358,25 @@ func (r *MCPOIDCConfigReconciler) findReferencingWorkloads( // SetupWithManager sets up the controller with the Manager. // Watches MCPServer, VirtualMCPServer, and MCPRemoteProxy changes to maintain accurate ReferencingWorkloads status. func (r *MCPOIDCConfigReconciler) SetupWithManager(mgr ctrl.Manager) error { + // Field indexes backing findReferencingWorkloads: each lets the controller + // query only the workloads referencing a given config rather than listing + // every workload in the namespace and filtering in memory. + if err := mgr.GetFieldIndexer().IndexField( + context.Background(), &mcpv1beta1.MCPServer{}, oidcConfigRefIndexKey, indexMCPServerByOIDCConfigRef, + ); err != nil { + return fmt.Errorf("failed to set up MCPServer oidcConfigRef index: %w", err) + } + if err := mgr.GetFieldIndexer().IndexField( + context.Background(), &mcpv1beta1.VirtualMCPServer{}, vmcpOIDCConfigRefIndexKey, indexVirtualMCPServerByOIDCConfigRef, + ); err != nil { + return fmt.Errorf("failed to set up VirtualMCPServer oidcConfigRef index: %w", err) + } + if err := mgr.GetFieldIndexer().IndexField( + context.Background(), &mcpv1beta1.MCPRemoteProxy{}, oidcConfigRefIndexKey, indexMCPRemoteProxyByOIDCConfigRef, + ); err != nil { + return fmt.Errorf("failed to set up MCPRemoteProxy oidcConfigRef index: %w", err) + } + // Watch MCPServer changes to update ReferencingWorkloads on referenced MCPOIDCConfigs. // This handler enqueues both the currently-referenced MCPOIDCConfig AND any // MCPOIDCConfig that still lists this server in ReferencingWorkloads (covers the diff --git a/cmd/thv-operator/controllers/mcpoidcconfig_controller_test.go b/cmd/thv-operator/controllers/mcpoidcconfig_controller_test.go index 8c3c032679..dd570e8255 100644 --- a/cmd/thv-operator/controllers/mcpoidcconfig_controller_test.go +++ b/cmd/thv-operator/controllers/mcpoidcconfig_controller_test.go @@ -291,8 +291,7 @@ func TestMCPOIDCConfigReconciler_handleDeletion(t *testing.T) { objs := []client.Object{tt.oidcConfig} - fakeClient := fake.NewClientBuilder(). - WithScheme(scheme). + fakeClient := withOIDCConfigRefIndexes(fake.NewClientBuilder().WithScheme(scheme)). WithObjects(objs...). Build() @@ -706,8 +705,7 @@ func TestMCPOIDCConfigReconciler_ConcurrentForeignConditionSurvivesMergePatch(t }, } - fakeClient := fake.NewClientBuilder(). - WithScheme(scheme). + fakeClient := withOIDCConfigRefIndexes(fake.NewClientBuilder().WithScheme(scheme)). WithObjects(oidcConfig). WithStatusSubresource(&mcpv1beta1.MCPOIDCConfig{}). WithInterceptorFuncs(inject). diff --git a/cmd/thv-operator/controllers/reconciler_test_helpers_test.go b/cmd/thv-operator/controllers/reconciler_test_helpers_test.go index cdad1926e4..46f7655322 100644 --- a/cmd/thv-operator/controllers/reconciler_test_helpers_test.go +++ b/cmd/thv-operator/controllers/reconciler_test_helpers_test.go @@ -115,12 +115,29 @@ func newTestMCPExternalAuthConfigReconciler( }, fakeClient } +// withOIDCConfigRefIndexes registers the field indexes that +// MCPOIDCConfigReconciler.findReferencingWorkloads relies on, so fake-client +// MatchingFields lookups (which the real cache populates via SetupWithManager) +// work in unit tests. Without these, the fake client returns "no index with +// name ... has been registered" for MCPServer/VirtualMCPServer/MCPRemoteProxy. +func withOIDCConfigRefIndexes(b *fake.ClientBuilder) *fake.ClientBuilder { + return b. + WithIndex(&mcpv1beta1.MCPServer{}, oidcConfigRefIndexKey, indexMCPServerByOIDCConfigRef). + WithIndex(&mcpv1beta1.VirtualMCPServer{}, vmcpOIDCConfigRefIndexKey, indexVirtualMCPServerByOIDCConfigRef). + WithIndex(&mcpv1beta1.MCPRemoteProxy{}, oidcConfigRefIndexKey, indexMCPRemoteProxyByOIDCConfigRef) +} + // newTestMCPOIDCConfigReconciler builds an MCPOIDCConfigReconciler backed by a -// fake client seeded with objs, with the MCPOIDCConfig status subresource enabled. +// fake client seeded with objs, with the MCPOIDCConfig status subresource enabled +// and the OIDC config-ref field indexes registered (see withOIDCConfigRefIndexes). // See newTestMCPExternalAuthConfigReconciler for the Recorder convention. func newTestMCPOIDCConfigReconciler(t *testing.T, objs ...client.Object) (*MCPOIDCConfigReconciler, client.Client) { t.Helper() - fakeClient, scheme := newTestFakeClient(t, &mcpv1beta1.MCPOIDCConfig{}, objs...) + scheme := testutil.NewScheme(t) + fakeClient := withOIDCConfigRefIndexes(fake.NewClientBuilder().WithScheme(scheme)). + WithObjects(objs...). + WithStatusSubresource(&mcpv1beta1.MCPOIDCConfig{}). + Build() return &MCPOIDCConfigReconciler{ Client: fakeClient, Scheme: scheme, From e4f79a42a3e398b658f852ed6c7dced7408cdd4a Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Wed, 24 Jun 2026 01:01:13 +0100 Subject: [PATCH 2/2] Add reverse-reference controller conventions to operator rules Codify the field-indexer + level-triggered reverse-reference pattern (and the anti-patterns it replaces) so future controller code does not regress to list-all-and-filter lookups, hand-rolled stale-ref scans, or unnecessary denormalized reference lists in status. Auto-loads on cmd/thv-operator/**. See #5607 / #5608 for the worked example. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/rules/operator.md | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/.claude/rules/operator.md b/.claude/rules/operator.md index c805b8036f..4a4b0a8387 100644 --- a/.claude/rules/operator.md +++ b/.claude/rules/operator.md @@ -103,3 +103,42 @@ the guard doing its job, not a bug. Status-subresource patching uses the sibling helper `controllerutil.MutateAndPatchStatus` (see the "Status Writes" section above). + +## Reverse References (who-references-me lookups) + +When a controller tracks which other objects reference it — e.g. a config CRD +referenced by workloads through a `*Ref` spec field — follow these rules. They +are the level-triggered, indexed pattern controller-runtime is built for; +deviating re-implements (usually less correctly) what the framework already +gives you. See #5607 / #5608 for the worked example. + +- **Use a field index for the reverse lookup.** To find "which objects reference + X", register `mgr.GetFieldIndexer().IndexField(...)` on the referrer's ref + field and query with `client.MatchingFields`. Do **not** `List` every object + and filter in memory — that is O(all objects) on the hot path. Mirror the + existing `setupGroupRefFieldIndexes` / `mcpserverentry_controller` indexes. The + extractor func must return `nil` for objects with no ref so they aren't indexed + under the empty key. + +- **Don't hand-roll stale-reference detection in watch handlers.** + `handler.EnqueueRequestsFromMapFunc` already runs the map function on **both** + the old and new object on update events (and on the object for create/delete), + so a map function that just returns "the object this referrer points at" + automatically enqueues both the newly- *and* previously-referenced object — you + do not need to scan every object's status to find the one holding a now-stale + entry. Pair the handler with a predicate so it only fires when the ref actually + changes (avoids status churn). + +- **Reconcile is level-triggered.** Rebuild reverse-reference state from current + cluster truth on every reconcile; never treat stored status as authoritative. + Watch handlers are latency hints, not correctness — missed events are healed by + periodic resync. + +- **Don't store a denormalized reverse-reference list** (e.g. + `Status.ReferencingWorkloads`) unless a concrete consumer needs it materialized + on the object itself, such as a `kubectl` printer column. For deletion + protection, have the finalizer recompute referrers **on demand** (the + Kubernetes PVC-protection model: `pkg/controller/volume/pvcprotection`). A + finalizer that re-queries live makes a stored list redundant for protection, + and maintaining the list means watching every referrer purely to keep a count + fresh.