Watch authz ConfigMaps from VirtualMCPServer

blkt · blkt · commit 98ee5591722c · 2026-05-14T10:54:41.000+02:00
The VirtualMCPServer controller did not watch ConfigMaps referenced by spec.incomingAuth.authzConfig.configMap, so changes to a referenced authz ConfigMap were never observed: the converter was not re-run, the rendered vmcp config.yaml retained stale policies, and the running pod enforced them indefinitely. This matters in particular for the enterprise flow where an external controller rewrites the authz ConfigMap when Cedar policies change. Add a Watches entry on corev1.ConfigMap with: * a mapper that lists VirtualMCPServers in the ConfigMap's namespace and enqueues those whose AuthzConfig.Type is "configMap" and whose ConfigMap.Name matches the changed object * a predicate that admits Create and Delete events unconditionally and Update events only when .Data or .BinaryData actually changed, so metadata-only updates (labels, annotations, resourceVersion bumps) do not trigger reconciliation The watch is functionally meaningful only once #5208 lands and the converter actually resolves ConfigMap-sourced policies; until then the re-reconcile still produces an empty-policy config. Wiring it now unblocks that follow-up without an extra round trip. Closes #5270
diff --git a/cmd/thv-operator/controllers/virtualmcpserver_authz_configmap.go b/cmd/thv-operator/controllers/virtualmcpserver_authz_configmap.go
@@ -0,0 +1,94 @@
+// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package controllers
+
+import (
+	"context"
+	"reflect"
+
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/event"
+	"sigs.k8s.io/controller-runtime/pkg/log"
+	"sigs.k8s.io/controller-runtime/pkg/predicate"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+
+	mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1"
+)
+
+// 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
+// and roll out a pod with the new config.
+//
+// The mapper lists VirtualMCPServers in the ConfigMap's namespace and enqueues any that
+// reference this ConfigMap. ConfigMaps are cluster-wide objects but authz references are
+// namespace-scoped, so the lookup is bounded to a single namespace.
+func (r *VirtualMCPServerReconciler) mapAuthzConfigMapToVirtualMCPServer(
+	ctx context.Context, obj client.Object,
+) []reconcile.Request {
+	cm, ok := obj.(*corev1.ConfigMap)
+	if !ok {
+		return nil
+	}
+
+	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")
+		return nil
+	}
+
+	var requests []reconcile.Request
+	for _, vmcp := range vmcpList.Items {
+		if !vmcpReferencesAuthzConfigMap(&vmcp, cm.Name) {
+			continue
+		}
+		requests = append(requests, reconcile.Request{
+			NamespacedName: types.NamespacedName{
+				Name:      vmcp.Name,
+				Namespace: vmcp.Namespace,
+			},
+		})
+	}
+
+	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
+}
+
+// configMapDataChangedPredicate admits ConfigMap events that may affect a VirtualMCPServer's
+// 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
+// newly-created ConfigMap or surface a deletion as a status error.
+func configMapDataChangedPredicate() predicate.Predicate {
+	return predicate.Funcs{
+		UpdateFunc: func(e event.UpdateEvent) bool {
+			oldCM, ok := e.ObjectOld.(*corev1.ConfigMap)
+			if !ok {
+				return false
+			}
+			newCM, ok := e.ObjectNew.(*corev1.ConfigMap)
+			if !ok {
+				return false
+			}
+			return !reflect.DeepEqual(oldCM.Data, newCM.Data) ||
+				!reflect.DeepEqual(oldCM.BinaryData, newCM.BinaryData)
+		},
+		CreateFunc:  func(_ event.CreateEvent) bool { return true },
+		DeleteFunc:  func(_ event.DeleteEvent) bool { return true },
+		GenericFunc: func(_ event.GenericEvent) bool { return false },
+	}
+}
diff --git a/cmd/thv-operator/controllers/virtualmcpserver_authz_configmap_test.go b/cmd/thv-operator/controllers/virtualmcpserver_authz_configmap_test.go
@@ -0,0 +1,223 @@
+// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package controllers
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/event"
+
+	mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1"
+)
+
+func TestMapAuthzConfigMapToVirtualMCPServer(t *testing.T) {
+	t.Parallel()
+
+	const ns = "default"
+	const cmName = "shared-authz"
+
+	// vmcpWithConfigMapRef references cmName via configMap.
+	vmcpWithConfigMapRef := &mcpv1beta1.VirtualMCPServer{
+		ObjectMeta: metav1.ObjectMeta{Name: "vmcp-cm", Namespace: ns},
+		Spec: mcpv1beta1.VirtualMCPServerSpec{
+			IncomingAuth: &mcpv1beta1.IncomingAuthConfig{
+				Type: "oidc",
+				AuthzConfig: &mcpv1beta1.AuthzConfigRef{
+					Type:      mcpv1beta1.AuthzConfigTypeConfigMap,
+					ConfigMap: &mcpv1beta1.ConfigMapAuthzRef{Name: cmName, Key: "authz.json"},
+				},
+			},
+		},
+	}
+	// vmcpDifferentCM references a different ConfigMap by the same type.
+	vmcpDifferentCM := &mcpv1beta1.VirtualMCPServer{
+		ObjectMeta: metav1.ObjectMeta{Name: "vmcp-other-cm", Namespace: ns},
+		Spec: mcpv1beta1.VirtualMCPServerSpec{
+			IncomingAuth: &mcpv1beta1.IncomingAuthConfig{
+				Type: "oidc",
+				AuthzConfig: &mcpv1beta1.AuthzConfigRef{
+					Type:      mcpv1beta1.AuthzConfigTypeConfigMap,
+					ConfigMap: &mcpv1beta1.ConfigMapAuthzRef{Name: "other-authz"},
+				},
+			},
+		},
+	}
+	// vmcpInline uses inline authz — same name on Inline.something should not match.
+	vmcpInline := &mcpv1beta1.VirtualMCPServer{
+		ObjectMeta: metav1.ObjectMeta{Name: "vmcp-inline", Namespace: ns},
+		Spec: mcpv1beta1.VirtualMCPServerSpec{
+			IncomingAuth: &mcpv1beta1.IncomingAuthConfig{
+				Type: "oidc",
+				AuthzConfig: &mcpv1beta1.AuthzConfigRef{
+					Type:   mcpv1beta1.AuthzConfigTypeInline,
+					Inline: &mcpv1beta1.InlineAuthzConfig{Policies: []string{"permit(principal, action, resource);"}},
+				},
+			},
+		},
+	}
+	// vmcpNoIncomingAuth has no incoming auth configured.
+	vmcpNoIncomingAuth := &mcpv1beta1.VirtualMCPServer{
+		ObjectMeta: metav1.ObjectMeta{Name: "vmcp-no-auth", Namespace: ns},
+		Spec:       mcpv1beta1.VirtualMCPServerSpec{},
+	}
+	// vmcpInOtherNamespace references the same name in a different namespace.
+	vmcpInOtherNamespace := &mcpv1beta1.VirtualMCPServer{
+		ObjectMeta: metav1.ObjectMeta{Name: "vmcp-other-ns", Namespace: "other"},
+		Spec: mcpv1beta1.VirtualMCPServerSpec{
+			IncomingAuth: &mcpv1beta1.IncomingAuthConfig{
+				Type: "oidc",
+				AuthzConfig: &mcpv1beta1.AuthzConfigRef{
+					Type:      mcpv1beta1.AuthzConfigTypeConfigMap,
+					ConfigMap: &mcpv1beta1.ConfigMapAuthzRef{Name: cmName},
+				},
+			},
+		},
+	}
+
+	scheme := runtime.NewScheme()
+	require.NoError(t, mcpv1beta1.AddToScheme(scheme))
+	require.NoError(t, corev1.AddToScheme(scheme))
+
+	fakeClient := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(
+			vmcpWithConfigMapRef,
+			vmcpDifferentCM,
+			vmcpInline,
+			vmcpNoIncomingAuth,
+			vmcpInOtherNamespace,
+		).
+		Build()
+
+	reconciler := &VirtualMCPServerReconciler{
+		Client: fakeClient,
+		Scheme: scheme,
+	}
+
+	tests := []struct {
+		name     string
+		obj      client.Object
+		expected []types.NamespacedName
+	}{
+		{
+			name: "configMap matched by name in same namespace",
+			obj: &corev1.ConfigMap{
+				ObjectMeta: metav1.ObjectMeta{Name: cmName, Namespace: ns},
+			},
+			expected: []types.NamespacedName{{Name: "vmcp-cm", Namespace: ns}},
+		},
+		{
+			name: "configMap in other namespace only matches vmcp in that namespace",
+			obj: &corev1.ConfigMap{
+				ObjectMeta: metav1.ObjectMeta{Name: cmName, Namespace: "other"},
+			},
+			expected: []types.NamespacedName{{Name: "vmcp-other-ns", Namespace: "other"}},
+		},
+		{
+			name: "configMap not referenced by any vmcp",
+			obj: &corev1.ConfigMap{
+				ObjectMeta: metav1.ObjectMeta{Name: "unreferenced", Namespace: ns},
+			},
+			expected: nil,
+		},
+		{
+			name:     "non-configMap object returns nil",
+			obj:      &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: cmName, Namespace: ns}},
+			expected: nil,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			requests := reconciler.mapAuthzConfigMapToVirtualMCPServer(t.Context(), tt.obj)
+			require.Len(t, requests, len(tt.expected))
+			got := make([]types.NamespacedName, 0, len(requests))
+			for _, r := range requests {
+				got = append(got, r.NamespacedName)
+			}
+			assert.ElementsMatch(t, tt.expected, got)
+		})
+	}
+}
+
+func TestConfigMapDataChangedPredicate(t *testing.T) {
+	t.Parallel()
+
+	p := configMapDataChangedPredicate()
+
+	baseCM := &corev1.ConfigMap{
+		ObjectMeta: metav1.ObjectMeta{Name: "cm", Namespace: "default"},
+		Data:       map[string]string{"authz.json": `{"policies":[]}`},
+	}
+
+	t.Run("create event is admitted", func(t *testing.T) {
+		t.Parallel()
+		assert.True(t, p.Create(event.CreateEvent{Object: baseCM}))
+	})
+
+	t.Run("delete event is admitted", func(t *testing.T) {
+		t.Parallel()
+		assert.True(t, p.Delete(event.DeleteEvent{Object: baseCM}))
+	})
+
+	t.Run("generic event is rejected", func(t *testing.T) {
+		t.Parallel()
+		assert.False(t, p.Generic(event.GenericEvent{Object: baseCM}))
+	})
+
+	t.Run("update event with data change is admitted", func(t *testing.T) {
+		t.Parallel()
+		oldCM := baseCM.DeepCopy()
+		newCM := baseCM.DeepCopy()
+		newCM.Data = map[string]string{"authz.json": `{"policies":["permit(principal, action, resource);"]}`}
+		assert.True(t, p.Update(event.UpdateEvent{ObjectOld: oldCM, ObjectNew: newCM}))
+	})
+
+	t.Run("update event with binary data change is admitted", func(t *testing.T) {
+		t.Parallel()
+		oldCM := baseCM.DeepCopy()
+		newCM := baseCM.DeepCopy()
+		newCM.BinaryData = map[string][]byte{"blob": []byte("x")}
+		assert.True(t, p.Update(event.UpdateEvent{ObjectOld: oldCM, ObjectNew: newCM}))
+	})
+
+	t.Run("update event with label-only change is rejected", func(t *testing.T) {
+		t.Parallel()
+		oldCM := baseCM.DeepCopy()
+		newCM := baseCM.DeepCopy()
+		newCM.Labels = map[string]string{"updated": "true"}
+		assert.False(t, p.Update(event.UpdateEvent{ObjectOld: oldCM, ObjectNew: newCM}))
+	})
+
+	t.Run("update event with annotation-only change is rejected", func(t *testing.T) {
+		t.Parallel()
+		oldCM := baseCM.DeepCopy()
+		newCM := baseCM.DeepCopy()
+		newCM.Annotations = map[string]string{"updated": "true"}
+		assert.False(t, p.Update(event.UpdateEvent{ObjectOld: oldCM, ObjectNew: newCM}))
+	})
+
+	t.Run("update event with no changes is rejected", func(t *testing.T) {
+		t.Parallel()
+		oldCM := baseCM.DeepCopy()
+		newCM := baseCM.DeepCopy()
+		assert.False(t, p.Update(event.UpdateEvent{ObjectOld: oldCM, ObjectNew: newCM}))
+	})
+
+	t.Run("update event on non-configMap object is rejected", func(t *testing.T) {
+		t.Parallel()
+		oldObj := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "s", Namespace: "default"}}
+		newObj := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "s", Namespace: "default"}}
+		assert.False(t, p.Update(event.UpdateEvent{ObjectOld: oldObj, ObjectNew: newObj}))
+	})
+}
diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller.go b/cmd/thv-operator/controllers/virtualmcpserver_controller.go
@@ -27,6 +27,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/tools/events"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
@@ -2594,6 +2595,15 @@ func (r *VirtualMCPServerReconciler) SetupWithManager(mgr ctrl.Manager) error {
 			&mcpv1beta1.MCPTelemetryConfig{},
 			handler.EnqueueRequestsFromMapFunc(r.mapTelemetryConfigToVirtualMCPServer),
 		).
+		// Watch ConfigMaps referenced via spec.incomingAuth.authzConfig.configMap so that
+		// policy changes trigger reconciliation. The predicate filters out metadata-only
+		// updates; the mapper narrows to VirtualMCPServers that actually reference the
+		// changed ConfigMap. See #5270.
+		Watches(
+			&corev1.ConfigMap{},
+			handler.EnqueueRequestsFromMapFunc(r.mapAuthzConfigMapToVirtualMCPServer),
+			builder.WithPredicates(configMapDataChangedPredicate()),
+		).
 		Complete(r)
 }
 
