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. 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,