From 12fb144440c4fb78867571c6a8867d237000db49 Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Thu, 18 Jun 2026 22:58:37 +0100 Subject: [PATCH 1/3] Wire MCPAuthzConfig references into the MCPServer controller An MCPServer that sets spec.authzConfigRef now resolves and enforces the referenced MCPAuthzConfig at runtime, mirroring the OIDCConfigRef pattern and building on the Stage-1 controllerutil helpers. Backend-agnostic: the proxy runner's authz factory handles cedarv1 and httpv1 alike. - handleAuthzConfig: fetch + validate-ready, track AuthzConfigHash, set the AuthzConfigRefValidated condition; on nil-ref it clears the hash AND removes the condition so a stale "valid" signal does not linger (unlike the OIDC version). Wired into Reconcile after handleOIDCConfig. - mapAuthzConfigToServers watch (extracted as a named method) + Watches on MCPAuthzConfig so a config change re-reconciles referencing servers. - Runtime resolution via AddAuthzConfigRefOptions in the runconfig builder; ConfigMap materialization via EnsureAuthzConfigMapFromRef and a mounted volume via GenerateAuthzVolumeConfigFromRef. Inline spec.authzConfig is untouched and remains mutually exclusive (CRD XValidation). - Add the mcpauthzconfigs get;list;watch RBAC marker. - Unit tests for handleAuthzConfig and an envtest integration suite proving both backends, the watch re-reconcile, ConfigMap materialization, and the invalid case. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../controllers/mcpserver_authzconfig_test.go | 164 +++++++++++++++ .../controllers/mcpserver_controller.go | 194 +++++++++++++++++- .../controllers/mcpserver_runconfig.go | 7 + ...pserver_authzconfigref_integration_test.go | 164 +++++++++++++++ 4 files changed, 526 insertions(+), 3 deletions(-) create mode 100644 cmd/thv-operator/controllers/mcpserver_authzconfig_test.go create mode 100644 cmd/thv-operator/test-integration/mcp-server/mcpserver_authzconfigref_integration_test.go diff --git a/cmd/thv-operator/controllers/mcpserver_authzconfig_test.go b/cmd/thv-operator/controllers/mcpserver_authzconfig_test.go new file mode 100644 index 0000000000..c83867d365 --- /dev/null +++ b/cmd/thv-operator/controllers/mcpserver_authzconfig_test.go @@ -0,0 +1,164 @@ +// 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" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1" + "github.com/stacklok/toolhive/pkg/container/kubernetes" +) + +// authzConfigForTest builds an MCPAuthzConfig with the given validity and hash. +func authzConfigForTest(name string, valid bool, hash string) *mcpv1beta1.MCPAuthzConfig { + status := metav1.ConditionFalse + if valid { + status = metav1.ConditionTrue + } + return &mcpv1beta1.MCPAuthzConfig{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "default"}, + Spec: mcpv1beta1.MCPAuthzConfigSpec{ + Type: "cedarv1", + Config: runtime.RawExtension{Raw: []byte(`{"policies":["permit(principal, action, resource);"],"entities_json":"[]"}`)}, + }, + Status: mcpv1beta1.MCPAuthzConfigStatus{ + ConfigHash: hash, + Conditions: []metav1.Condition{{ + Type: mcpv1beta1.ConditionTypeAuthzConfigValid, + Status: status, + Reason: "Test", + }}, + }, + } +} + +func TestMCPServerReconciler_handleAuthzConfig(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + mcpServer *mcpv1beta1.MCPServer + authzConfig *mcpv1beta1.MCPAuthzConfig + expectError bool + expectHash string + expectHashCleared bool + expectConditionStatus *metav1.ConditionStatus + expectConditionReason string + expectConditionGone bool + }{ + { + name: "no ref clears stored hash and condition", + mcpServer: &mcpv1beta1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{Name: "s", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerSpec{Image: "img"}, + Status: mcpv1beta1.MCPServerStatus{ + AuthzConfigHash: "old", + Conditions: []metav1.Condition{{ + Type: mcpv1beta1.ConditionAuthzConfigRefValidated, + Status: metav1.ConditionTrue, + Reason: mcpv1beta1.ConditionReasonAuthzConfigRefValid, + }}, + }, + }, + expectHashCleared: true, + expectConditionGone: true, + }, + { + name: "referenced config not found sets NotFound condition", + mcpServer: &mcpv1beta1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{Name: "s", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerSpec{ + Image: "img", + AuthzConfigRef: &mcpv1beta1.MCPAuthzConfigReference{Name: "missing"}, + }, + }, + expectError: true, + expectConditionStatus: conditionStatusPtr(metav1.ConditionFalse), + expectConditionReason: mcpv1beta1.ConditionReasonAuthzConfigRefNotFound, + }, + { + name: "referenced config not valid sets NotValid condition", + mcpServer: &mcpv1beta1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{Name: "s", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerSpec{ + Image: "img", + AuthzConfigRef: &mcpv1beta1.MCPAuthzConfigReference{Name: "bad"}, + }, + }, + authzConfig: authzConfigForTest("bad", false, ""), + expectError: true, + expectConditionStatus: conditionStatusPtr(metav1.ConditionFalse), + expectConditionReason: mcpv1beta1.ConditionReasonAuthzConfigRefNotValid, + }, + { + name: "valid ref sets condition True and tracks hash", + mcpServer: &mcpv1beta1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{Name: "s", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerSpec{ + Image: "img", + AuthzConfigRef: &mcpv1beta1.MCPAuthzConfigReference{Name: "ok"}, + }, + }, + authzConfig: authzConfigForTest("ok", true, "hash-123"), + expectHash: "hash-123", + expectConditionStatus: conditionStatusPtr(metav1.ConditionTrue), + expectConditionReason: mcpv1beta1.ConditionReasonAuthzConfigRefValid, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := t.Context() + + scheme := runtime.NewScheme() + require.NoError(t, mcpv1beta1.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + + objs := []runtime.Object{tt.mcpServer} + if tt.authzConfig != nil { + objs = append(objs, tt.authzConfig) + } + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithRuntimeObjects(objs...). + WithStatusSubresource(&mcpv1beta1.MCPServer{}, &mcpv1beta1.MCPAuthzConfig{}). + Build() + + reconciler := newTestMCPServerReconciler(fakeClient, scheme, kubernetes.PlatformKubernetes) + + err := reconciler.handleAuthzConfig(ctx, tt.mcpServer) + if tt.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + + if tt.expectHash != "" { + assert.Equal(t, tt.expectHash, tt.mcpServer.Status.AuthzConfigHash) + } + if tt.expectHashCleared { + assert.Empty(t, tt.mcpServer.Status.AuthzConfigHash) + } + + cond := meta.FindStatusCondition(tt.mcpServer.Status.Conditions, mcpv1beta1.ConditionAuthzConfigRefValidated) + if tt.expectConditionGone { + assert.Nil(t, cond, "AuthzConfigRefValidated condition must be cleared when the ref is removed") + } + if tt.expectConditionStatus != nil { + require.NotNil(t, cond, "expected AuthzConfigRefValidated condition") + assert.Equal(t, *tt.expectConditionStatus, cond.Status) + assert.Equal(t, tt.expectConditionReason, cond.Reason) + } + }) + } +} diff --git a/cmd/thv-operator/controllers/mcpserver_controller.go b/cmd/thv-operator/controllers/mcpserver_controller.go index 73b17fd0e9..60f482e17c 100644 --- a/cmd/thv-operator/controllers/mcpserver_controller.go +++ b/cmd/thv-operator/controllers/mcpserver_controller.go @@ -138,6 +138,10 @@ const ( // authzLabelValueInline is the label value for inline authorization configuration authzLabelValueInline = "inline" + + // authzLabelValueRef is the label value for a ConfigMap materialized from a + // referenced MCPAuthzConfig (spec.authzConfigRef) + authzLabelValueRef = "ref" ) const defaultTerminationGracePeriodSeconds = int64(30) @@ -155,6 +159,7 @@ func (r *MCPServerReconciler) detectPlatform(ctx context.Context) (kubernetes.Pl // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpservers/finalizers,verbs=update // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcptoolconfigs,verbs=get;list;watch // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpoidcconfigs,verbs=get;list;watch +// +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpauthzconfigs,verbs=get;list;watch // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcptelemetryconfigs,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=configmaps,verbs=create;delete;get;list;patch;update;watch // +kubebuilder:rbac:groups="",resources=services,verbs=create;delete;get;list;patch;update;watch @@ -317,6 +322,17 @@ func (r *MCPServerReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return ctrl.Result{}, err } + // Check if MCPAuthzConfig is referenced and handle it + if err := r.handleAuthzConfig(ctx, mcpServer); err != nil { + ctxLogger.Error(err, "Failed to handle MCPAuthzConfig") + mcpServer.Status.Phase = mcpv1beta1.MCPServerPhaseFailed + setReadyCondition(mcpServer, metav1.ConditionFalse, mcpv1beta1.ConditionReasonNotReady, err.Error()) + if statusErr := r.Status().Update(ctx, mcpServer); statusErr != nil { + ctxLogger.Error(statusErr, "Failed to update MCPServer status after MCPAuthzConfig error") + } + return ctrl.Result{}, err + } + // Update the MCPServer status with the pod status if err := r.updateMCPServerStatus(ctx, mcpServer); err != nil { ctxLogger.Error(err, "Failed to update MCPServer status") @@ -1242,13 +1258,22 @@ func (r *MCPServerReconciler) deploymentForMCPServer( }) } - // Add volume mounts for authorization configuration + // Add volume mounts for authorization configuration (inline spec.authzConfig). authzVolumeMount, authzVolume := ctrlutil.GenerateAuthzVolumeConfig(m.Spec.AuthzConfig, m.Name) if authzVolumeMount != nil { volumeMounts = append(volumeMounts, *authzVolumeMount) volumes = append(volumes, *authzVolume) } + // Add the volume mount for a referenced MCPAuthzConfig (spec.authzConfigRef). + // Mutually exclusive with the inline volume above (CRD XValidation), and the + // volume name/path are shared, so at most one authz volume is ever mounted. + if m.Spec.AuthzConfigRef != nil { + refMount, refVolume := ctrlutil.GenerateAuthzVolumeConfigFromRef(m.Name) + volumeMounts = append(volumeMounts, *refMount) + volumes = append(volumes, *refVolume) + } + // Add OIDC CA bundle volume if configured via MCPOIDCConfigRef if m.Spec.OIDCConfigRef != nil { oidcCfg, err := ctrlutil.GetOIDCConfigForServer(ctx, r.Client, m.Namespace, m.Spec.OIDCConfigRef) @@ -2103,6 +2128,14 @@ func labelsForInlineAuthzConfig(name string) map[string]string { return labels } +// labelsForAuthzConfigRef returns the labels for ConfigMaps materialized from a +// referenced MCPAuthzConfig. +func labelsForAuthzConfigRef(name string) map[string]string { + labels := labelsForMCPServer(name) + labels[authzLabelKey] = authzLabelValueRef + return labels +} + // getToolhiveRunnerImage returns the image to use for the toolhive runner container func getToolhiveRunnerImage() string { // Get the image from the environment variable or use a default @@ -2421,6 +2454,109 @@ func setOIDCConfigRefCondition(m *mcpv1beta1.MCPServer, status metav1.ConditionS }) } +// handleAuthzConfig validates the referenced MCPAuthzConfig, tracks its hash on +// the MCPServer status, and sets the AuthzConfigRefValidated condition. When the +// ref is cleared it removes both the hash and the condition so a stale "valid" +// signal does not linger. ReferencingWorkloads on the MCPAuthzConfig is owned by +// the MCPAuthzConfig controller (#5511); this controller never writes it. +func (r *MCPServerReconciler) handleAuthzConfig(ctx context.Context, m *mcpv1beta1.MCPServer) error { + if m.Spec.AuthzConfigRef == nil { + // No MCPAuthzConfig referenced: clear any stored hash and remove the + // condition so it does not remain stale-True after the ref is removed. + changed := false + if m.Status.AuthzConfigHash != "" { + m.Status.AuthzConfigHash = "" + changed = true + } + if meta.RemoveStatusCondition(&m.Status.Conditions, mcpv1beta1.ConditionAuthzConfigRefValidated) { + changed = true + } + if changed { + if err := r.Status().Update(ctx, m); err != nil { + return fmt.Errorf("failed to clear MCPAuthzConfig status: %w", err) + } + } + return nil + } + + authzConfig, err := r.fetchAndValidateAuthzConfig(ctx, m) + if err != nil { + return err + } + + prevCondition := meta.FindStatusCondition(m.Status.Conditions, mcpv1beta1.ConditionAuthzConfigRefValidated) + needsUpdate := prevCondition == nil || prevCondition.Status != metav1.ConditionTrue + + setAuthzConfigRefCondition(m, metav1.ConditionTrue, + mcpv1beta1.ConditionReasonAuthzConfigRefValid, + fmt.Sprintf("MCPAuthzConfig %s is valid and ready", m.Spec.AuthzConfigRef.Name)) + + if m.Status.AuthzConfigHash != authzConfig.Status.ConfigHash { + m.Status.AuthzConfigHash = authzConfig.Status.ConfigHash + needsUpdate = true + } + + if needsUpdate { + if err := r.Status().Update(ctx, m); err != nil { + return fmt.Errorf("failed to update MCPAuthzConfig status: %w", err) + } + } + + return nil +} + +// fetchAndValidateAuthzConfig fetches the referenced MCPAuthzConfig, validates it +// is ready, and sets the appropriate failure condition on the MCPServer if not. +func (r *MCPServerReconciler) fetchAndValidateAuthzConfig( + ctx context.Context, m *mcpv1beta1.MCPServer, +) (*mcpv1beta1.MCPAuthzConfig, error) { + ctxLogger := log.FromContext(ctx) + + authzConfig, err := ctrlutil.GetAuthzConfigForWorkload(ctx, r.Client, m.Namespace, m.Spec.AuthzConfigRef) + if err != nil { + setAuthzConfigRefCondition(m, metav1.ConditionFalse, + mcpv1beta1.ConditionReasonAuthzConfigRefNotFound, + fmt.Sprintf("MCPAuthzConfig %s not found: %v", m.Spec.AuthzConfigRef.Name, err)) + if statusErr := r.Status().Update(ctx, m); statusErr != nil { + ctxLogger.Error(statusErr, "Failed to update status after MCPAuthzConfig lookup error") + } + return nil, err + } + + if authzConfig == nil { + setAuthzConfigRefCondition(m, metav1.ConditionFalse, + mcpv1beta1.ConditionReasonAuthzConfigRefNotFound, + fmt.Sprintf("MCPAuthzConfig %s not found", m.Spec.AuthzConfigRef.Name)) + if statusErr := r.Status().Update(ctx, m); statusErr != nil { + ctxLogger.Error(statusErr, "Failed to update status after MCPAuthzConfig not found") + } + return nil, fmt.Errorf("MCPAuthzConfig %s not found", m.Spec.AuthzConfigRef.Name) + } + + if err := ctrlutil.ValidateAuthzConfigReady(authzConfig); err != nil { + setAuthzConfigRefCondition(m, metav1.ConditionFalse, + mcpv1beta1.ConditionReasonAuthzConfigRefNotValid, + fmt.Sprintf("MCPAuthzConfig %s is not valid: %v", m.Spec.AuthzConfigRef.Name, err)) + if statusErr := r.Status().Update(ctx, m); statusErr != nil { + ctxLogger.Error(statusErr, "Failed to update status after MCPAuthzConfig validation check") + } + return nil, err + } + + return authzConfig, nil +} + +// setAuthzConfigRefCondition sets the AuthzConfigRefValidated status condition +func setAuthzConfigRefCondition(m *mcpv1beta1.MCPServer, status metav1.ConditionStatus, reason, message string) { + meta.SetStatusCondition(&m.Status.Conditions, metav1.Condition{ + Type: mcpv1beta1.ConditionAuthzConfigRefValidated, + Status: status, + Reason: reason, + Message: message, + ObservedGeneration: m.Generation, + }) +} + // handleWebhookConfig validates and tracks the hash of the referenced MCPWebhookConfig. func (r *MCPServerReconciler) handleWebhookConfig(ctx context.Context, m *mcpv1beta1.MCPServer) error { ctxLogger := log.FromContext(ctx) @@ -2463,10 +2599,26 @@ func (r *MCPServerReconciler) handleWebhookConfig(ctx context.Context, m *mcpv1b return nil } -// ensureAuthzConfigMap ensures the authorization ConfigMap exists for inline configuration +// ensureAuthzConfigMap ensures the authorization ConfigMap exists for inline +// configuration (spec.authzConfig) and for a referenced MCPAuthzConfig +// (spec.authzConfigRef). The two are mutually exclusive (CRD XValidation), so at +// most one ConfigMap is materialized per reconcile. func (r *MCPServerReconciler) ensureAuthzConfigMap(ctx context.Context, m *mcpv1beta1.MCPServer) error { - return ctrlutil.EnsureAuthzConfigMap( + if err := ctrlutil.EnsureAuthzConfigMap( ctx, r.Client, r.Scheme, m, m.Namespace, m.Name, m.Spec.AuthzConfig, labelsForInlineAuthzConfig(m.Name), + ); err != nil { + return err + } + + if m.Spec.AuthzConfigRef == nil { + return nil + } + authzConfig, err := ctrlutil.GetAuthzConfigForWorkload(ctx, r.Client, m.Namespace, m.Spec.AuthzConfigRef) + if err != nil { + return err + } + return ctrlutil.EnsureAuthzConfigMapFromRef( + ctx, r.Client, r.Scheme, m, m.Namespace, m.Name, authzConfig, labelsForAuthzConfigRef(m.Name), ) } @@ -2632,6 +2784,38 @@ func (r *MCPServerReconciler) validateRateLimitConfig(ctx context.Context, mcpSe } } +// mapAuthzConfigToServers maps MCPAuthzConfig changes to reconciliation requests +// for the MCPServers that reference it via spec.authzConfigRef. +func (r *MCPServerReconciler) mapAuthzConfigToServers( + ctx context.Context, obj client.Object, +) []reconcile.Request { + authzConfig, ok := obj.(*mcpv1beta1.MCPAuthzConfig) + if !ok { + return nil + } + + mcpServerList := &mcpv1beta1.MCPServerList{} + if err := r.List(ctx, mcpServerList, client.InNamespace(authzConfig.Namespace)); err != nil { + log.FromContext(ctx).Error(err, "Failed to list MCPServers for MCPAuthzConfig watch") + return nil + } + + var requests []reconcile.Request + for _, server := range mcpServerList.Items { + if server.Spec.AuthzConfigRef != nil && + server.Spec.AuthzConfigRef.Name == authzConfig.Name { + requests = append(requests, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: server.Name, + Namespace: server.Namespace, + }, + }) + } + } + + return requests +} + // mapWebhookConfigToServers maps MCPWebhookConfig changes to MCPServer reconciliation requests. func (r *MCPServerReconciler) mapWebhookConfigToServers( ctx context.Context, obj client.Object, @@ -2733,6 +2917,9 @@ func (r *MCPServerReconciler) SetupWithManager(mgr ctrl.Manager) error { }, ) + // Create a handler that maps MCPAuthzConfig changes to MCPServer reconciliation requests + authzConfigHandler := handler.EnqueueRequestsFromMapFunc(r.mapAuthzConfigToServers) + telemetryConfigHandler := handler.EnqueueRequestsFromMapFunc(r.mapTelemetryConfigToServers) webhookConfigHandler := handler.EnqueueRequestsFromMapFunc(r.mapWebhookConfigToServers) @@ -2742,6 +2929,7 @@ func (r *MCPServerReconciler) SetupWithManager(mgr ctrl.Manager) error { Owns(&corev1.Service{}). Watches(&mcpv1beta1.MCPExternalAuthConfig{}, externalAuthConfigHandler). Watches(&mcpv1beta1.MCPOIDCConfig{}, oidcConfigHandler). + Watches(&mcpv1beta1.MCPAuthzConfig{}, authzConfigHandler). Watches(&mcpv1beta1.MCPTelemetryConfig{}, telemetryConfigHandler). Watches(&mcpv1alpha1.MCPWebhookConfig{}, webhookConfigHandler). Complete(r) diff --git a/cmd/thv-operator/controllers/mcpserver_runconfig.go b/cmd/thv-operator/controllers/mcpserver_runconfig.go index cdd19bf373..7bb2d785d7 100644 --- a/cmd/thv-operator/controllers/mcpserver_runconfig.go +++ b/cmd/thv-operator/controllers/mcpserver_runconfig.go @@ -204,6 +204,13 @@ func (r *MCPServerReconciler) createRunConfigFromMCPServer(m *mcpv1beta1.MCPServ return nil, fmt.Errorf("failed to process AuthzConfig: %w", err) } + // Resolve a referenced MCPAuthzConfig (spec.authzConfigRef) into runtime authz. + // Mutually exclusive with the inline spec.authzConfig handled above. Backend- + // agnostic: the proxy runner's authz factory dispatches on type (cedarv1, httpv1). + if err := ctrlutil.AddAuthzConfigRefOptions(ctx, r.Client, m.Namespace, m.Spec.AuthzConfigRef, &options); err != nil { + return nil, fmt.Errorf("failed to process AuthzConfigRef: %w", err) + } + // Resolve OIDC configuration from either legacy OIDCConfig or new MCPOIDCConfigRef. // Resolve once and reuse for both RunConfig options and embedded auth server config. var resolvedOIDCConfig *oidc.OIDCConfig diff --git a/cmd/thv-operator/test-integration/mcp-server/mcpserver_authzconfigref_integration_test.go b/cmd/thv-operator/test-integration/mcp-server/mcpserver_authzconfigref_integration_test.go new file mode 100644 index 0000000000..229e3dc449 --- /dev/null +++ b/cmd/thv-operator/test-integration/mcp-server/mcpserver_authzconfigref_integration_test.go @@ -0,0 +1,164 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package controllers + +import ( + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + + mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1" +) + +// The MCPAuthzConfig controller is not registered in this suite, so we pre-seed +// the config's Valid condition + ConfigHash directly; the MCPServer controller +// (which is registered) only reads them. +var _ = Describe("MCPServer AuthzConfigRef Integration Tests", func() { + const ( + timeout = time.Second * 30 + interval = time.Millisecond * 250 + ) + + // seedAuthzConfig creates an MCPAuthzConfig and stamps its status (Valid + // condition + ConfigHash) as the MCPAuthzConfig controller would. + seedAuthzConfig := func(name, namespace, typ, rawConfig, hash string, valid bool) *mcpv1beta1.MCPAuthzConfig { + cfg := &mcpv1beta1.MCPAuthzConfig{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}, + Spec: mcpv1beta1.MCPAuthzConfigSpec{ + Type: typ, + Config: runtime.RawExtension{Raw: []byte(rawConfig)}, + }, + } + Expect(k8sClient.Create(ctx, cfg)).To(Succeed()) + + status := metav1.ConditionFalse + if valid { + status = metav1.ConditionTrue + } + cfg.Status.ConfigHash = hash + meta.SetStatusCondition(&cfg.Status.Conditions, metav1.Condition{ + Type: mcpv1beta1.ConditionTypeAuthzConfigValid, + Status: status, + Reason: "Test", + Message: "seeded by integration test", + }) + Expect(k8sClient.Status().Update(ctx, cfg)).To(Succeed()) + return cfg + } + + newServer := func(name, namespace, authzRefName string) *mcpv1beta1.MCPServer { + return &mcpv1beta1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}, + Spec: mcpv1beta1.MCPServerSpec{ + Image: "example/mcp-server:v1.0.0", + Transport: "streamable-http", + AuthzConfigRef: &mcpv1beta1.MCPAuthzConfigReference{Name: authzRefName}, + }, + } + } + + const ( + cedarConfig = `{"policies":["permit(principal, action, resource);"],"entities_json":"[]"}` + httpConfig = `{"http":{"url":"https://pdp.example.com"},"claim_mapping":"standard"}` + ) + + DescribeTable("a valid referenced MCPAuthzConfig is validated and hash-tracked, for any backend", + func(nsName, cfgName, srvName, typ, rawConfig string) { + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: nsName}} + _ = k8sClient.Create(ctx, ns) + + seedAuthzConfig(cfgName, nsName, typ, rawConfig, "hash-1", true) + Expect(k8sClient.Create(ctx, newServer(srvName, nsName, cfgName))).To(Succeed()) + + By("setting AuthzConfigRefValidated=True and tracking the hash") + Eventually(func(g Gomega) { + var got mcpv1beta1.MCPServer + g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: srvName, Namespace: nsName}, &got)).To(Succeed()) + cond := meta.FindStatusCondition(got.Status.Conditions, mcpv1beta1.ConditionAuthzConfigRefValidated) + g.Expect(cond).NotTo(BeNil()) + g.Expect(cond.Status).To(Equal(metav1.ConditionTrue)) + g.Expect(got.Status.AuthzConfigHash).To(Equal("hash-1")) + }, timeout, interval).Should(Succeed()) + + By("materializing the authz ConfigMap the proxy mounts (any backend)") + Eventually(func(g Gomega) { + var cm corev1.ConfigMap + g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: srvName + "-authz-ref", Namespace: nsName}, &cm)).To(Succeed()) + g.Expect(cm.Data).To(HaveKey("authz.json")) + g.Expect(cm.Data["authz.json"]).To(ContainSubstring(typ)) + }, timeout, interval).Should(Succeed()) + }, + Entry("cedarv1", "authzref-cedar", "authz-cedar", "srv-cedar", "cedarv1", cedarConfig), + Entry("httpv1", "authzref-http", "authz-http", "srv-http", "httpv1", httpConfig), + ) + + Context("when the referenced MCPAuthzConfig changes", Ordered, func() { + const ( + namespace = "authzref-watch" + cfgName = "authz-watch" + srvName = "srv-watch" + ) + BeforeAll(func() { + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespace}} + _ = k8sClient.Create(ctx, ns) + seedAuthzConfig(cfgName, namespace, "cedarv1", cedarConfig, "hash-1", true) + Expect(k8sClient.Create(ctx, newServer(srvName, namespace, cfgName))).To(Succeed()) + }) + + It("re-reconciles the MCPServer via the watch and updates the tracked hash", func() { + Eventually(func(g Gomega) { + var got mcpv1beta1.MCPServer + g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: srvName, Namespace: namespace}, &got)).To(Succeed()) + g.Expect(got.Status.AuthzConfigHash).To(Equal("hash-1")) + }, timeout, interval).Should(Succeed()) + + By("bumping the config hash") + var cfg mcpv1beta1.MCPAuthzConfig + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: cfgName, Namespace: namespace}, &cfg)).To(Succeed()) + cfg.Status.ConfigHash = "hash-2" + meta.SetStatusCondition(&cfg.Status.Conditions, metav1.Condition{ + Type: mcpv1beta1.ConditionTypeAuthzConfigValid, Status: metav1.ConditionTrue, Reason: "Test", + }) + Expect(k8sClient.Status().Update(ctx, &cfg)).To(Succeed()) + + By("observing the MCPServer pick up the new hash through the watch") + Eventually(func(g Gomega) { + var got mcpv1beta1.MCPServer + g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: srvName, Namespace: namespace}, &got)).To(Succeed()) + g.Expect(got.Status.AuthzConfigHash).To(Equal("hash-2")) + }, timeout, interval).Should(Succeed()) + }) + }) + + Context("when the referenced MCPAuthzConfig is not valid", Ordered, func() { + const ( + namespace = "authzref-invalid" + cfgName = "authz-invalid" + srvName = "srv-invalid" + ) + BeforeAll(func() { + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespace}} + _ = k8sClient.Create(ctx, ns) + seedAuthzConfig(cfgName, namespace, "cedarv1", cedarConfig, "", false) + Expect(k8sClient.Create(ctx, newServer(srvName, namespace, cfgName))).To(Succeed()) + }) + + It("sets AuthzConfigRefValidated=False with reason NotValid", func() { + Eventually(func(g Gomega) { + var got mcpv1beta1.MCPServer + g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: srvName, Namespace: namespace}, &got)).To(Succeed()) + cond := meta.FindStatusCondition(got.Status.Conditions, mcpv1beta1.ConditionAuthzConfigRefValidated) + g.Expect(cond).NotTo(BeNil()) + g.Expect(cond.Status).To(Equal(metav1.ConditionFalse)) + g.Expect(cond.Reason).To(Equal(mcpv1beta1.ConditionReasonAuthzConfigRefNotValid)) + }, timeout, interval).Should(Succeed()) + }) + }) +}) From b788d53cfdd05261644c071c2cfdad831f3bf37a Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Thu, 18 Jun 2026 23:18:24 +0100 Subject: [PATCH 2/3] Guard ref authz volume against duplicate volume name Belt-and-suspenders for review feedback: only add the spec.authzConfigRef volume when the inline authz volume was not added. Inline and ref share the "authz-config" volume name and are mutually exclusive via CRD XValidation, so a hypothetical CEL regression now degrades to "inline wins" rather than producing an invalid pod spec with a duplicate volume name. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd/thv-operator/controllers/mcpserver_controller.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/cmd/thv-operator/controllers/mcpserver_controller.go b/cmd/thv-operator/controllers/mcpserver_controller.go index 60f482e17c..1de10cd22f 100644 --- a/cmd/thv-operator/controllers/mcpserver_controller.go +++ b/cmd/thv-operator/controllers/mcpserver_controller.go @@ -1266,9 +1266,11 @@ func (r *MCPServerReconciler) deploymentForMCPServer( } // Add the volume mount for a referenced MCPAuthzConfig (spec.authzConfigRef). - // Mutually exclusive with the inline volume above (CRD XValidation), and the - // volume name/path are shared, so at most one authz volume is ever mounted. - if m.Spec.AuthzConfigRef != nil { + // Inline and ref are mutually exclusive (CRD XValidation) and share the + // "authz-config" volume name, so only add the ref volume when the inline one + // was not added. This keeps a hypothetical CEL regression degrading to "one + // volume wins" rather than an invalid pod spec with a duplicate volume name. + if m.Spec.AuthzConfigRef != nil && authzVolumeMount == nil { refMount, refVolume := ctrlutil.GenerateAuthzVolumeConfigFromRef(m.Name) volumeMounts = append(volumeMounts, *refMount) volumes = append(volumes, *refVolume) From d5ca499cb6d42f7349aa68a1f45a9acda0b8be85 Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Thu, 18 Jun 2026 23:29:04 +0100 Subject: [PATCH 3/3] Address review feedback on MCPServer authz wiring - Document fail-stale-on-revocation semantics on handleAuthzConfig: a previously-valid ref that later becomes invalid/missing leaves an already- running workload enforcing its last-applied policy (fail-stale, not fail-open) while the MCPServer is marked Failed/Ready=False. (#2) - Note on GenerateAuthzVolumeConfigFromRef that the mounted ConfigMap is not the active enforcement path in operator deployments (the proxy reads authz from the RunConfig-embedded config); the mount is for inline parity / future file-based consumption. (#3) - Reword the status-write error messages to name the MCPServer (the object being written), not the MCPAuthzConfig. (#4) - Add a transition unit test (valid -> invalid -> valid) covering the needsUpdate bookkeeping, plus an integration It asserting the valid -> invalid flip propagates to the MCPServer via the watch. (#1) - Soften the watch test's "via the watch" phrasing to claim only the observable outcome. (#5) - Tighten the unit not-found/not-valid assertions with ErrorContains. (#6) Co-Authored-By: Claude Opus 4.8 (1M context) --- .../controllers/mcpserver_authzconfig_test.go | 64 +++++++++++++++++++ .../controllers/mcpserver_controller.go | 12 +++- .../pkg/controllerutil/authz_ref.go | 7 ++ ...pserver_authzconfigref_integration_test.go | 27 +++++++- 4 files changed, 106 insertions(+), 4 deletions(-) diff --git a/cmd/thv-operator/controllers/mcpserver_authzconfig_test.go b/cmd/thv-operator/controllers/mcpserver_authzconfig_test.go index c83867d365..0307c9918c 100644 --- a/cmd/thv-operator/controllers/mcpserver_authzconfig_test.go +++ b/cmd/thv-operator/controllers/mcpserver_authzconfig_test.go @@ -49,6 +49,7 @@ func TestMCPServerReconciler_handleAuthzConfig(t *testing.T) { mcpServer *mcpv1beta1.MCPServer authzConfig *mcpv1beta1.MCPAuthzConfig expectError bool + expectErrContains string expectHash string expectHashCleared bool expectConditionStatus *metav1.ConditionStatus @@ -82,6 +83,7 @@ func TestMCPServerReconciler_handleAuthzConfig(t *testing.T) { }, }, expectError: true, + expectErrContains: "not found", expectConditionStatus: conditionStatusPtr(metav1.ConditionFalse), expectConditionReason: mcpv1beta1.ConditionReasonAuthzConfigRefNotFound, }, @@ -96,6 +98,7 @@ func TestMCPServerReconciler_handleAuthzConfig(t *testing.T) { }, authzConfig: authzConfigForTest("bad", false, ""), expectError: true, + expectErrContains: "not valid", expectConditionStatus: conditionStatusPtr(metav1.ConditionFalse), expectConditionReason: mcpv1beta1.ConditionReasonAuthzConfigRefNotValid, }, @@ -139,6 +142,9 @@ func TestMCPServerReconciler_handleAuthzConfig(t *testing.T) { err := reconciler.handleAuthzConfig(ctx, tt.mcpServer) if tt.expectError { assert.Error(t, err) + if tt.expectErrContains != "" { + assert.ErrorContains(t, err, tt.expectErrContains) + } } else { assert.NoError(t, err) } @@ -162,3 +168,61 @@ func TestMCPServerReconciler_handleAuthzConfig(t *testing.T) { }) } } + +// TestMCPServerReconciler_handleAuthzConfig_Transitions exercises the stateful +// bookkeeping (needsUpdate from the prior condition, and recovery) that the +// static single-state cases above do not: valid -> invalid -> valid. +func TestMCPServerReconciler_handleAuthzConfig_Transitions(t *testing.T) { + t.Parallel() + ctx := t.Context() + + scheme := runtime.NewScheme() + require.NoError(t, mcpv1beta1.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + + authzConfig := authzConfigForTest("cfg", true, "h1") + server := &mcpv1beta1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{Name: "s", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerSpec{ + Image: "img", + AuthzConfigRef: &mcpv1beta1.MCPAuthzConfigReference{Name: "cfg"}, + }, + } + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(server, authzConfig). + WithStatusSubresource(&mcpv1beta1.MCPServer{}, &mcpv1beta1.MCPAuthzConfig{}). + Build() + r := newTestMCPServerReconciler(fakeClient, scheme, kubernetes.PlatformKubernetes) + + condStatus := func() *metav1.Condition { + return meta.FindStatusCondition(server.Status.Conditions, mcpv1beta1.ConditionAuthzConfigRefValidated) + } + + // valid -> condition True + hash tracked + require.NoError(t, r.handleAuthzConfig(ctx, server)) + require.NotNil(t, condStatus()) + assert.Equal(t, metav1.ConditionTrue, condStatus().Status) + assert.Equal(t, "h1", server.Status.AuthzConfigHash) + + // flip the referenced config to invalid -> condition transitions to False/NotValid + authzConfig.Status.Conditions = []metav1.Condition{{ + Type: mcpv1beta1.ConditionTypeAuthzConfigValid, Status: metav1.ConditionFalse, Reason: "Invalidated", + }} + require.NoError(t, fakeClient.Status().Update(ctx, authzConfig)) + + assert.Error(t, r.handleAuthzConfig(ctx, server)) + require.NotNil(t, condStatus()) + assert.Equal(t, metav1.ConditionFalse, condStatus().Status) + assert.Equal(t, mcpv1beta1.ConditionReasonAuthzConfigRefNotValid, condStatus().Reason) + + // recover the config -> condition transitions back to True + authzConfig.Status.Conditions = []metav1.Condition{{ + Type: mcpv1beta1.ConditionTypeAuthzConfigValid, Status: metav1.ConditionTrue, Reason: "Test", + }} + require.NoError(t, fakeClient.Status().Update(ctx, authzConfig)) + + require.NoError(t, r.handleAuthzConfig(ctx, server)) + require.NotNil(t, condStatus()) + assert.Equal(t, metav1.ConditionTrue, condStatus().Status) +} diff --git a/cmd/thv-operator/controllers/mcpserver_controller.go b/cmd/thv-operator/controllers/mcpserver_controller.go index 1de10cd22f..5b4ae11ffc 100644 --- a/cmd/thv-operator/controllers/mcpserver_controller.go +++ b/cmd/thv-operator/controllers/mcpserver_controller.go @@ -2461,6 +2461,14 @@ func setOIDCConfigRefCondition(m *mcpv1beta1.MCPServer, status metav1.ConditionS // ref is cleared it removes both the hash and the condition so a stale "valid" // signal does not linger. ReferencingWorkloads on the MCPAuthzConfig is owned by // the MCPAuthzConfig controller (#5511); this controller never writes it. +// +// Revocation semantics (fail-stale, not fail-open): if a previously-valid ref +// later becomes invalid or missing, this returns an error and Reconcile stops +// before updating the deployment, so an already-running workload keeps enforcing +// its last-applied authz policy while the MCPServer is marked Failed/Ready=False. +// It is not torn down and does not revert to no-authz. This matches the +// OIDC/ExternalAuth/Telemetry ref handlers; hard fail-closed-on-revocation would +// require a separate, product-signed-off mechanism. func (r *MCPServerReconciler) handleAuthzConfig(ctx context.Context, m *mcpv1beta1.MCPServer) error { if m.Spec.AuthzConfigRef == nil { // No MCPAuthzConfig referenced: clear any stored hash and remove the @@ -2475,7 +2483,7 @@ func (r *MCPServerReconciler) handleAuthzConfig(ctx context.Context, m *mcpv1bet } if changed { if err := r.Status().Update(ctx, m); err != nil { - return fmt.Errorf("failed to clear MCPAuthzConfig status: %w", err) + return fmt.Errorf("failed to clear MCPAuthzConfig hash from MCPServer status: %w", err) } } return nil @@ -2500,7 +2508,7 @@ func (r *MCPServerReconciler) handleAuthzConfig(ctx context.Context, m *mcpv1bet if needsUpdate { if err := r.Status().Update(ctx, m); err != nil { - return fmt.Errorf("failed to update MCPAuthzConfig status: %w", err) + return fmt.Errorf("failed to update MCPServer status after validating MCPAuthzConfig: %w", err) } } diff --git a/cmd/thv-operator/pkg/controllerutil/authz_ref.go b/cmd/thv-operator/pkg/controllerutil/authz_ref.go index 12c4fe9927..d2b3a24c9b 100644 --- a/cmd/thv-operator/pkg/controllerutil/authz_ref.go +++ b/cmd/thv-operator/pkg/controllerutil/authz_ref.go @@ -208,6 +208,13 @@ func EnsureAuthzConfigMapFromRef( // GenerateAuthzVolumeConfigFromRef returns the volume mount + volume for the // ConfigMap materialized by EnsureAuthzConfigMapFromRef, mirroring // GenerateAuthzVolumeConfig for the inline path. +// +// Note: in operator-managed deployments this mounted file is NOT the active +// enforcement path — the proxy builds its authz middleware from the authz config +// embedded in the RunConfig (via AddAuthzConfigRefOptions → WithAuthzConfig). The +// mount exists for parity with the inline path and for potential future +// file-based consumption (config.AuthzConfigPath); the operator does not set +// that path today. func GenerateAuthzVolumeConfigFromRef(resourceName string) (*corev1.VolumeMount, *corev1.Volume) { volumeMount := &corev1.VolumeMount{ Name: "authz-config", diff --git a/cmd/thv-operator/test-integration/mcp-server/mcpserver_authzconfigref_integration_test.go b/cmd/thv-operator/test-integration/mcp-server/mcpserver_authzconfigref_integration_test.go index 229e3dc449..ab1d888e3a 100644 --- a/cmd/thv-operator/test-integration/mcp-server/mcpserver_authzconfigref_integration_test.go +++ b/cmd/thv-operator/test-integration/mcp-server/mcpserver_authzconfigref_integration_test.go @@ -112,7 +112,7 @@ var _ = Describe("MCPServer AuthzConfigRef Integration Tests", func() { Expect(k8sClient.Create(ctx, newServer(srvName, namespace, cfgName))).To(Succeed()) }) - It("re-reconciles the MCPServer via the watch and updates the tracked hash", func() { + It("reflects a config hash change on the referencing MCPServer", func() { Eventually(func(g Gomega) { var got mcpv1beta1.MCPServer g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: srvName, Namespace: namespace}, &got)).To(Succeed()) @@ -128,13 +128,36 @@ var _ = Describe("MCPServer AuthzConfigRef Integration Tests", func() { }) Expect(k8sClient.Status().Update(ctx, &cfg)).To(Succeed()) - By("observing the MCPServer pick up the new hash through the watch") + // The MCPServer controller watches MCPAuthzConfig, so the change is + // picked up without an external nudge. (This asserts the observable + // outcome; it does not attempt to prove the watch is the sole trigger.) + By("observing the MCPServer eventually reflect the new hash") Eventually(func(g Gomega) { var got mcpv1beta1.MCPServer g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: srvName, Namespace: namespace}, &got)).To(Succeed()) g.Expect(got.Status.AuthzConfigHash).To(Equal("hash-2")) }, timeout, interval).Should(Succeed()) }) + + It("transitions AuthzConfigRefValidated to False when the config becomes invalid", func() { + By("flagging the referenced config invalid") + var cfg mcpv1beta1.MCPAuthzConfig + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: cfgName, Namespace: namespace}, &cfg)).To(Succeed()) + meta.SetStatusCondition(&cfg.Status.Conditions, metav1.Condition{ + Type: mcpv1beta1.ConditionTypeAuthzConfigValid, Status: metav1.ConditionFalse, Reason: "Invalidated", + }) + Expect(k8sClient.Status().Update(ctx, &cfg)).To(Succeed()) + + By("observing the MCPServer condition flip to False/NotValid") + Eventually(func(g Gomega) { + var got mcpv1beta1.MCPServer + g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: srvName, Namespace: namespace}, &got)).To(Succeed()) + cond := meta.FindStatusCondition(got.Status.Conditions, mcpv1beta1.ConditionAuthzConfigRefValidated) + g.Expect(cond).NotTo(BeNil()) + g.Expect(cond.Status).To(Equal(metav1.ConditionFalse)) + g.Expect(cond.Reason).To(Equal(mcpv1beta1.ConditionReasonAuthzConfigRefNotValid)) + }, timeout, interval).Should(Succeed()) + }) }) Context("when the referenced MCPAuthzConfig is not valid", Ordered, func() {