diff --git a/cmd/thv-operator/controllers/virtualmcpserver_authz_configmap.go b/cmd/thv-operator/controllers/virtualmcpserver_authz_configmap.go index ce3eadfb68..5db9ba897d 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_authz_configmap.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_authz_configmap.go @@ -18,6 +18,44 @@ import ( mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1" ) +const virtualMCPServerConfigMapIndex = "toolhive.stacklok.dev/virtualmcpserver-configmap" + +// indexVirtualMCPServerConfigMaps returns namespace-local ConfigMap references +// whose data changes require the VirtualMCPServer to reconcile. +func indexVirtualMCPServerConfigMaps(obj client.Object) []string { + vmcp, ok := obj.(*mcpv1beta1.VirtualMCPServer) + if !ok { + return nil + } + + names := make(map[string]struct{}) + if vmcp.Spec.IncomingAuth != nil && vmcp.Spec.IncomingAuth.AuthzConfig != nil && + vmcp.Spec.IncomingAuth.AuthzConfig.Type == mcpv1beta1.AuthzConfigTypeConfigMap && + vmcp.Spec.IncomingAuth.AuthzConfig.ConfigMap != nil && + vmcp.Spec.IncomingAuth.AuthzConfig.ConfigMap.Name != "" { + names[vmcp.Spec.IncomingAuth.AuthzConfig.ConfigMap.Name] = struct{}{} + } + if vmcp.Spec.AuthServerConfig != nil { + for _, provider := range vmcp.Spec.AuthServerConfig.UpstreamProviders { + if name := providerCABundleConfigMapName(provider); name != "" { + names[name] = struct{}{} + } + } + for i := range vmcp.Spec.AuthServerConfig.TrustedIssuers { + ref := vmcp.Spec.AuthServerConfig.TrustedIssuers[i].CABundleRef + if ref != nil && ref.ConfigMapRef != nil && ref.ConfigMapRef.Name != "" { + names[ref.ConfigMapRef.Name] = struct{}{} + } + } + } + + result := make([]string, 0, len(names)) + for name := range names { + result = append(result, name) + } + return result +} + // mapAuthzConfigMapToVirtualMCPServer maps ConfigMap changes to VirtualMCPServer reconciliation // requests. Used by SetupWithManager to trigger reconciliation when a ConfigMap referenced via // spec.incomingAuth.authzConfig.configMap is updated, so the converter can re-resolve policies @@ -35,20 +73,20 @@ func (r *VirtualMCPServerReconciler) mapAuthzConfigMapToVirtualMCPServer( } vmcpList := &mcpv1beta1.VirtualMCPServerList{} - if err := r.List(ctx, vmcpList, client.InNamespace(cm.Namespace)); err != nil { - log.FromContext(ctx).Error(err, "Failed to list VirtualMCPServers for authz ConfigMap watch") + if err := r.List(ctx, vmcpList, + client.InNamespace(cm.Namespace), + client.MatchingFields{virtualMCPServerConfigMapIndex: cm.Name}, + ); err != nil { + log.FromContext(ctx).Error(err, "Failed to list VirtualMCPServers for ConfigMap watch") return nil } - var requests []reconcile.Request - for _, vmcp := range vmcpList.Items { - if !vmcpReferencesAuthzConfigMap(&vmcp, cm.Name) && !vmcpReferencesAuthServerCABundle(&vmcp, cm.Name) { - continue - } + requests := make([]reconcile.Request, 0, len(vmcpList.Items)) + for i := range vmcpList.Items { requests = append(requests, reconcile.Request{ NamespacedName: types.NamespacedName{ - Name: vmcp.Name, - Namespace: vmcp.Namespace, + Name: vmcpList.Items[i].Name, + Namespace: vmcpList.Items[i].Namespace, }, }) } @@ -56,40 +94,6 @@ func (r *VirtualMCPServerReconciler) mapAuthzConfigMapToVirtualMCPServer( return requests } -// vmcpReferencesAuthzConfigMap reports whether the VirtualMCPServer references the named -// ConfigMap via spec.incomingAuth.authzConfig. -func vmcpReferencesAuthzConfigMap(vmcp *mcpv1beta1.VirtualMCPServer, configMapName string) bool { - if vmcp.Spec.IncomingAuth == nil || - vmcp.Spec.IncomingAuth.AuthzConfig == nil || - vmcp.Spec.IncomingAuth.AuthzConfig.Type != mcpv1beta1.AuthzConfigTypeConfigMap || - vmcp.Spec.IncomingAuth.AuthzConfig.ConfigMap == nil { - return false - } - return vmcp.Spec.IncomingAuth.AuthzConfig.ConfigMap.Name == configMapName -} - -// vmcpReferencesAuthServerCABundle reports whether an inline auth-server -// configuration selects the named ConfigMap for a CA bundle. -func vmcpReferencesAuthServerCABundle(vmcp *mcpv1beta1.VirtualMCPServer, configMapName string) bool { - if vmcp.Spec.AuthServerConfig == nil { - return false - } - for i := range vmcp.Spec.AuthServerConfig.UpstreamProviders { - provider := &vmcp.Spec.AuthServerConfig.UpstreamProviders[i] - ref := provider.CABundleRef() - if ref != nil && ref.ConfigMapRef != nil && ref.ConfigMapRef.Name == configMapName { - return true - } - } - for i := range vmcp.Spec.AuthServerConfig.TrustedIssuers { - ref := vmcp.Spec.AuthServerConfig.TrustedIssuers[i].CABundleRef - if ref != nil && ref.ConfigMapRef != nil && ref.ConfigMapRef.Name == configMapName { - return true - } - } - return false -} - // resolved authz config. Update events are admitted only when .Data or .BinaryData actually // change, so metadata-only updates (labels, annotations, resourceVersion bumps) do not trigger // reconciliation. Create and Delete events are passed through so the controller can pick up a diff --git a/cmd/thv-operator/controllers/virtualmcpserver_authz_configmap_test.go b/cmd/thv-operator/controllers/virtualmcpserver_authz_configmap_test.go index dddbfb542a..d77a8d875f 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_authz_configmap_test.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_authz_configmap_test.go @@ -80,6 +80,7 @@ func TestMapAuthzConfigMapToVirtualMCPServer(t *testing.T) { vmcpNoIncomingAuth, vmcpInOtherNamespace, ). + WithIndex(&mcpv1beta1.VirtualMCPServer{}, virtualMCPServerConfigMapIndex, indexVirtualMCPServerConfigMaps). Build() reconciler := &VirtualMCPServerReconciler{ @@ -143,7 +144,8 @@ func TestMapAuthzConfigMapToVirtualMCPServer_InlineCABundle(t *testing.T) { OIDCConfig: &mcpv1beta1.OIDCUpstreamConfig{CABundleRef: &mcpv1beta1.CABundleSource{ConfigMapRef: &corev1.ConfigMapKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "ca-map"}}}}, }}} scheme := testutil.NewScheme(t) - r := &VirtualMCPServerReconciler{Client: fake.NewClientBuilder().WithScheme(scheme).WithObjects(vmcp).Build()} + r := &VirtualMCPServerReconciler{Client: fake.NewClientBuilder().WithScheme(scheme).WithObjects(vmcp). + WithIndex(&mcpv1beta1.VirtualMCPServer{}, virtualMCPServerConfigMapIndex, indexVirtualMCPServerConfigMaps).Build()} requests := r.mapAuthzConfigMapToVirtualMCPServer(t.Context(), &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "ca-map", Namespace: ns}}) require.Equal(t, []types.NamespacedName{{Name: "vmcp", Namespace: ns}}, []types.NamespacedName{requests[0].NamespacedName}) } diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller.go b/cmd/thv-operator/controllers/virtualmcpserver_controller.go index cfd1059330..de83abdd6f 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_controller.go @@ -467,8 +467,6 @@ func (r *VirtualMCPServerReconciler) runAuthValidations( vmcp *mcpv1beta1.VirtualMCPServer, statusManager virtualmcpserverstatus.StatusManager, ) (bool, error) { - ctxLogger := log.FromContext(ctx) - // Validate inline AuthServerConfig (when specified). if vmcp.Spec.AuthServerConfig != nil { // Surface the IdentitySynthesized advisory upfront, before validation. @@ -482,13 +480,13 @@ func (r *VirtualMCPServerReconciler) runAuthValidations( r.applyAuthServerIdentitySynthesizedCondition(vmcp, statusManager) if err := r.validateAuthServerConfig(vmcp, statusManager); err != nil { if applyErr := r.applyStatusUpdates(ctx, vmcp, statusManager); applyErr != nil { - ctxLogger.Error(applyErr, "Failed to apply status updates after AuthServerConfig validation error") + return false, applyErr } return false, nil } if terminal, err := r.validateAuthServerConfigCABundles(ctx, vmcp, statusManager); err != nil { if applyErr := r.applyStatusUpdates(ctx, vmcp, statusManager); applyErr != nil { - ctxLogger.Error(applyErr, "Failed to apply status updates after CA bundle validation error") + return false, applyErr } if terminal { return false, nil @@ -507,7 +505,7 @@ func (r *VirtualMCPServerReconciler) runAuthValidations( // RemoveConditionsWithPrefix call above when AuthServerConfig is nil. if err := r.validateAuthzUpstreamAvailable(ctx, vmcp, statusManager); err != nil { if applyErr := r.applyStatusUpdates(ctx, vmcp, statusManager); applyErr != nil { - ctxLogger.Error(applyErr, "Failed to apply status updates after AuthzUpstreamAvailable validation error") + return false, applyErr } return false, nil } @@ -2899,6 +2897,13 @@ func (r *VirtualMCPServerReconciler) mapEmbeddingServerToVirtualMCPServer( // SetupWithManager sets up the controller with the Manager func (r *VirtualMCPServerReconciler) SetupWithManager(mgr ctrl.Manager) error { + if err := mgr.GetFieldIndexer().IndexField( + context.Background(), &mcpv1beta1.VirtualMCPServer{}, + virtualMCPServerConfigMapIndex, indexVirtualMCPServerConfigMaps, + ); err != nil { + return fmt.Errorf("failed to set up VirtualMCPServer ConfigMap reference index: %w", err) + } + return ctrl.NewControllerManagedBy(mgr). For(&mcpv1beta1.VirtualMCPServer{}). Owns(&appsv1.Deployment{}). diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go b/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go index b41c84204d..9730939a4c 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go @@ -4465,3 +4465,45 @@ func TestVirtualMCPServer_AuthServerConfigCABundleGetErrorIsTransient(t *testing assert.NotEqual(t, mcpv1beta1.VirtualMCPServerPhaseFailed, vmcp.Status.Phase, "a transient read failure must not stamp a terminal phase") } + +// TestVirtualMCPServer_RunAuthValidations_StatusWriteFailurePropagates verifies +// that a failed status write inside runAuthValidations surfaces as an error +// (so the caller requeues with backoff) instead of being logged and swallowed +// as if the terminal validation path had completed cleanly. +func TestVirtualMCPServer_RunAuthValidations_StatusWriteFailurePropagates(t *testing.T) { + t.Parallel() + + // An empty Issuer fails validateAuthServerConfig on its first check, + // driving runAuthValidations into the applyStatusUpdates call whose + // error propagation this test targets. + vmcp := v1beta1test.NewVirtualMCPServer(testVmcpName, "default", + v1beta1test.WithVMCPAuthServerConfig(&mcpv1beta1.EmbeddedAuthServerConfig{}), + v1beta1test.MutateVMCP(func(v *mcpv1beta1.VirtualMCPServer) { + v.Generation = 1 + }), + ) + + scheme := testutil.NewScheme(t) + fakeClient := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(vmcp). + WithStatusSubresource(&mcpv1beta1.VirtualMCPServer{}). + WithInterceptorFuncs(interceptor.Funcs{ + SubResourceUpdate: func(_ context.Context, _ client.Client, _ string, _ client.Object, + _ ...client.SubResourceUpdateOption) error { + return apierrors.NewServiceUnavailable("apiserver is having a moment") + }, + }). + Build() + + reconciler := &VirtualMCPServerReconciler{ + Client: fakeClient, + Scheme: scheme, + PlatformDetector: ctrlutil.NewSharedPlatformDetector(), + } + statusManager := virtualmcpserverstatus.NewStatusManager(vmcp) + + ok, err := reconciler.runAuthValidations(t.Context(), vmcp, statusManager) + + require.Error(t, err, "a failed status write must surface so the caller requeues, not be swallowed") + assert.False(t, ok) +}