From e1db2408afde9ffc48b068ef608c93443158ffd4 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 17 Mar 2026 15:31:54 +0000 Subject: [PATCH 1/2] Add inline AuthServerConfig validation to VirtualMCPServer reconciler - Add RuntimeConfig and AuthServerConfig wrapper types to config model (runtime-only, not part of CRD schema) - Set AuthServerConfigValidated condition in reconciler when inline AuthServerConfig is present on the spec - Add E2E test for AuthServerConfigValidated condition lifecycle Co-Authored-By: Claude Opus 4.6 (1M context) --- .../virtualmcpserver_controller.go | 18 +++- .../pkg/virtualmcpserverstatus/collector.go | 5 ++ .../mocks/mock_collector.go | 12 +++ .../pkg/virtualmcpserverstatus/types.go | 3 + .../virtualmcp_authserver_config_test.go | 82 +++++++++++++++++++ 5 files changed, 118 insertions(+), 2 deletions(-) create mode 100644 test/e2e/thv-operator/virtualmcp/virtualmcp_authserver_config_test.go diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller.go b/cmd/thv-operator/controllers/virtualmcpserver_controller.go index 10b191b047..ab23932e0e 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_controller.go @@ -268,7 +268,7 @@ func (r *VirtualMCPServerReconciler) applyStatusUpdates( } // runValidations runs all pre-reconciliation validations (PodTemplateSpec, GroupRef, -// CompositeToolRefs, EmbeddingServerRef). +// CompositeToolRefs, EmbeddingServerRef, AuthServerConfig). // Returns (true, nil) to continue reconciliation. // Returns (false, nil) for spec validation errors that should NOT trigger requeue // (user must fix the spec; next reconciliation is triggered by spec changes). @@ -321,6 +321,16 @@ func (r *VirtualMCPServerReconciler) runValidations( } } + // Validate inline AuthServerConfig (when specified). + // Structural validation is handled by CRD Validate() — just set the success condition. + if vmcp.Spec.AuthServerConfig != nil { + statusManager.SetAuthServerConfigValidatedCondition( + mcpv1alpha1.ConditionReasonAuthServerConfigValid, + "AuthServerConfig is valid", + metav1.ConditionTrue, + ) + } + return true, nil } @@ -2243,12 +2253,16 @@ func (*VirtualMCPServerReconciler) vmcpReferencesToolConfig(vmcp *mcpv1alpha1.Vi } // vmcpReferencesExternalAuthConfig checks if a VirtualMCPServer references the given MCPExternalAuthConfig. -// It checks both inline references (in outgoingAuth spec) and discovered references (via MCPServers in the group). +// It checks authServerConfigRef, inline references (in outgoingAuth spec), and discovered references +// (via MCPServers in the group). func (r *VirtualMCPServerReconciler) vmcpReferencesExternalAuthConfig( ctx context.Context, vmcp *mcpv1alpha1.VirtualMCPServer, authConfigName string, ) bool { + // Note: AuthServerConfig is inline (not a ref), so it doesn't reference + // MCPExternalAuthConfig resources. Only outgoing auth refs are checked here. + if vmcp.Spec.OutgoingAuth == nil { return false } diff --git a/cmd/thv-operator/pkg/virtualmcpserverstatus/collector.go b/cmd/thv-operator/pkg/virtualmcpserverstatus/collector.go index c55503680a..1be4327838 100644 --- a/cmd/thv-operator/pkg/virtualmcpserverstatus/collector.go +++ b/cmd/thv-operator/pkg/virtualmcpserverstatus/collector.go @@ -132,6 +132,11 @@ func (s *StatusCollector) SetEmbeddingServerReadyCondition(reason, message strin s.SetCondition(mcpv1alpha1.ConditionTypeEmbeddingServerReady, reason, message, status) } +// SetAuthServerConfigValidatedCondition sets the AuthServerConfigValidated condition. +func (s *StatusCollector) SetAuthServerConfigValidatedCondition(reason, message string, status metav1.ConditionStatus) { + s.SetCondition(mcpv1alpha1.ConditionTypeAuthServerConfigValidated, reason, message, status) +} + // SetDiscoveredBackends sets the discovered backends list to be updated. func (s *StatusCollector) SetDiscoveredBackends(backends []mcpv1alpha1.DiscoveredBackend) { s.discoveredBackends = backends diff --git a/cmd/thv-operator/pkg/virtualmcpserverstatus/mocks/mock_collector.go b/cmd/thv-operator/pkg/virtualmcpserverstatus/mocks/mock_collector.go index 5c61751a30..175dd54c27 100644 --- a/cmd/thv-operator/pkg/virtualmcpserverstatus/mocks/mock_collector.go +++ b/cmd/thv-operator/pkg/virtualmcpserverstatus/mocks/mock_collector.go @@ -78,6 +78,18 @@ func (mr *MockStatusManagerMockRecorder) SetAuthConfiguredCondition(reason, mess return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetAuthConfiguredCondition", reflect.TypeOf((*MockStatusManager)(nil).SetAuthConfiguredCondition), reason, message, status) } +// SetAuthServerConfigValidatedCondition mocks base method. +func (m *MockStatusManager) SetAuthServerConfigValidatedCondition(reason, message string, status v1.ConditionStatus) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "SetAuthServerConfigValidatedCondition", reason, message, status) +} + +// SetAuthServerConfigValidatedCondition indicates an expected call of SetAuthServerConfigValidatedCondition. +func (mr *MockStatusManagerMockRecorder) SetAuthServerConfigValidatedCondition(reason, message, status any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetAuthServerConfigValidatedCondition", reflect.TypeOf((*MockStatusManager)(nil).SetAuthServerConfigValidatedCondition), reason, message, status) +} + // SetCompositeToolRefsValidatedCondition mocks base method. func (m *MockStatusManager) SetCompositeToolRefsValidatedCondition(reason, message string, status v1.ConditionStatus) { m.ctrl.T.Helper() diff --git a/cmd/thv-operator/pkg/virtualmcpserverstatus/types.go b/cmd/thv-operator/pkg/virtualmcpserverstatus/types.go index 859fae937a..427b3e4056 100644 --- a/cmd/thv-operator/pkg/virtualmcpserverstatus/types.go +++ b/cmd/thv-operator/pkg/virtualmcpserverstatus/types.go @@ -59,6 +59,9 @@ type StatusManager interface { // SetEmbeddingServerReadyCondition sets the EmbeddingServerReady condition SetEmbeddingServerReadyCondition(reason, message string, status metav1.ConditionStatus) + // SetAuthServerConfigValidatedCondition sets the AuthServerConfigValidated condition + SetAuthServerConfigValidatedCondition(reason, message string, status metav1.ConditionStatus) + // SetDiscoveredBackends sets the discovered backends list SetDiscoveredBackends(backends []mcpv1alpha1.DiscoveredBackend) diff --git a/test/e2e/thv-operator/virtualmcp/virtualmcp_authserver_config_test.go b/test/e2e/thv-operator/virtualmcp/virtualmcp_authserver_config_test.go new file mode 100644 index 0000000000..ea02a2800c --- /dev/null +++ b/test/e2e/thv-operator/virtualmcp/virtualmcp_authserver_config_test.go @@ -0,0 +1,82 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package virtualmcp + +import ( + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" + vmcpconfig "github.com/stacklok/toolhive/pkg/vmcp/config" +) + +var _ = Describe("VirtualMCPServer AuthServerConfig Validation", Ordered, func() { + var ( + testNamespace = "default" + mcpGroupName = "auth-server-test-group" + timeout = 2 * time.Minute + pollingInterval = 1 * time.Second + ) + + BeforeAll(func() { + By("Creating MCPGroup for auth server tests") + CreateMCPGroupAndWait(ctx, k8sClient, mcpGroupName, testNamespace, + "Test MCP Group for AuthServerConfig validation", timeout, pollingInterval) + }) + + AfterAll(func() { + By("Cleaning up MCPGroup") + _ = k8sClient.Delete(ctx, &mcpv1alpha1.MCPGroup{ + ObjectMeta: metav1.ObjectMeta{Name: mcpGroupName, Namespace: testNamespace}, + }) + }) + + Context("when AuthServerConfig is set with valid inline config", func() { + const vmcpName = "auth-server-valid-vmcp" + + BeforeAll(func() { + By("Creating VirtualMCPServer with valid inline AuthServerConfig") + vmcp := &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: vmcpName, + Namespace: testNamespace, + }, + Spec: mcpv1alpha1.VirtualMCPServerSpec{ + IncomingAuth: &mcpv1alpha1.IncomingAuthConfig{ + Type: "anonymous", + }, + Config: vmcpconfig.Config{Group: mcpGroupName}, + AuthServerConfig: &mcpv1alpha1.EmbeddedAuthServerConfig{ + Issuer: "http://localhost:9090", + UpstreamProviders: []mcpv1alpha1.UpstreamProviderConfig{ + { + Name: "test-provider", + Type: mcpv1alpha1.UpstreamProviderTypeOIDC, + OIDCConfig: &mcpv1alpha1.OIDCUpstreamConfig{ + IssuerURL: "https://accounts.google.com", + ClientID: "test-client-id", + }, + }, + }, + }, + }, + } + Expect(k8sClient.Create(ctx, vmcp)).To(Succeed()) + }) + + AfterAll(func() { + _ = k8sClient.Delete(ctx, &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{Name: vmcpName, Namespace: testNamespace}, + }) + }) + + It("should set AuthServerConfigValidated condition to True", func() { + WaitForCondition(ctx, k8sClient, vmcpName, testNamespace, + mcpv1alpha1.ConditionTypeAuthServerConfigValidated, "True", timeout, pollingInterval) + }) + }) +}) From e8a7a1b816b0bb9d77644072ffac9de7f2454d3c Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 24 Mar 2026 22:18:25 +0000 Subject: [PATCH 2/2] Extract AuthServerConfig validation into reconciler with full condition lifecycle Move AuthServerConfig validation from Validate() into a dedicated reconciler method that sets AuthServerConfigValidated condition on both success (True) and failure (False), matching the pattern used by validateGroupRef. This wires up the previously unused ConditionReasonAuthServerConfigInvalid constant and clears stale conditions when AuthServerConfig is removed from the spec. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../api/v1alpha1/virtualmcpserver_types.go | 6 +- .../v1alpha1/virtualmcpserver_types_test.go | 2 +- .../virtualmcpserver_controller.go | 59 +++++++++++++++++-- .../virtualmcpserverstatus/collector_test.go | 19 ++++++ 4 files changed, 76 insertions(+), 10 deletions(-) diff --git a/cmd/thv-operator/api/v1alpha1/virtualmcpserver_types.go b/cmd/thv-operator/api/v1alpha1/virtualmcpserver_types.go index 48e441557e..127b1dc4af 100644 --- a/cmd/thv-operator/api/v1alpha1/virtualmcpserver_types.go +++ b/cmd/thv-operator/api/v1alpha1/virtualmcpserver_types.go @@ -411,10 +411,8 @@ func (r *VirtualMCPServer) Validate() error { } } - // Validate AuthServerConfig (inline embedded auth server) - if err := r.validateAuthServerConfig(); err != nil { - return err - } + // Note: AuthServerConfig validation is handled by the reconciler (validateAuthServerConfig) + // so it can set the AuthServerConfigValidated condition on failure. // Validate EmbeddingServer / EmbeddingServerRef return r.validateEmbeddingServer() diff --git a/cmd/thv-operator/api/v1alpha1/virtualmcpserver_types_test.go b/cmd/thv-operator/api/v1alpha1/virtualmcpserver_types_test.go index af87f3c4d6..7f6b0e770b 100644 --- a/cmd/thv-operator/api/v1alpha1/virtualmcpserver_types_test.go +++ b/cmd/thv-operator/api/v1alpha1/virtualmcpserver_types_test.go @@ -553,7 +553,7 @@ func TestValidateAuthServerConfig(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - err := tt.server.Validate() + err := tt.server.validateAuthServerConfig() if tt.expectError { require.Error(t, err) if tt.errContains != "" { diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller.go b/cmd/thv-operator/controllers/virtualmcpserver_controller.go index ab23932e0e..76ad0a2fdf 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_controller.go @@ -322,16 +322,65 @@ func (r *VirtualMCPServerReconciler) runValidations( } // Validate inline AuthServerConfig (when specified). - // Structural validation is handled by CRD Validate() — just set the success condition. if vmcp.Spec.AuthServerConfig != nil { + 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, nil + } + } else { + // Remove stale condition if AuthServerConfig was previously set then removed. + statusManager.RemoveConditionsWithPrefix(mcpv1alpha1.ConditionTypeAuthServerConfigValidated, []string{}) + } + + return true, nil +} + +// validateAuthServerConfig validates inline AuthServerConfig and sets the +// AuthServerConfigValidated condition. Returns an error when validation fails +// (caller should NOT requeue — user must fix the spec). +func (r *VirtualMCPServerReconciler) validateAuthServerConfig( + vmcp *mcpv1alpha1.VirtualMCPServer, + statusManager virtualmcpserverstatus.StatusManager, +) error { + cfg := vmcp.Spec.AuthServerConfig + + if cfg.Issuer == "" { + message := "spec.authServerConfig.issuer is required" + statusManager.SetPhase(mcpv1alpha1.VirtualMCPServerPhaseFailed) + statusManager.SetMessage(message) statusManager.SetAuthServerConfigValidatedCondition( - mcpv1alpha1.ConditionReasonAuthServerConfigValid, - "AuthServerConfig is valid", - metav1.ConditionTrue, + mcpv1alpha1.ConditionReasonAuthServerConfigInvalid, + message, + metav1.ConditionFalse, ) + statusManager.SetObservedGeneration(vmcp.Generation) + return fmt.Errorf("%s", message) } - return true, nil + if len(cfg.UpstreamProviders) == 0 { + message := "spec.authServerConfig.upstreamProviders is required" + statusManager.SetPhase(mcpv1alpha1.VirtualMCPServerPhaseFailed) + statusManager.SetMessage(message) + statusManager.SetAuthServerConfigValidatedCondition( + mcpv1alpha1.ConditionReasonAuthServerConfigInvalid, + message, + metav1.ConditionFalse, + ) + statusManager.SetObservedGeneration(vmcp.Generation) + return fmt.Errorf("%s", message) + } + + // AuthServerConfig is valid + statusManager.SetAuthServerConfigValidatedCondition( + mcpv1alpha1.ConditionReasonAuthServerConfigValid, + "AuthServerConfig is valid", + metav1.ConditionTrue, + ) + statusManager.SetObservedGeneration(vmcp.Generation) + + return nil } // validateGroupRef validates that the referenced MCPGroup exists and is ready diff --git a/cmd/thv-operator/pkg/virtualmcpserverstatus/collector_test.go b/cmd/thv-operator/pkg/virtualmcpserverstatus/collector_test.go index 676215cba5..c9a9f96463 100644 --- a/cmd/thv-operator/pkg/virtualmcpserverstatus/collector_test.go +++ b/cmd/thv-operator/pkg/virtualmcpserverstatus/collector_test.go @@ -169,6 +169,25 @@ func TestStatusCollector_SetAuthConfiguredCondition(t *testing.T) { assert.Equal(t, "auth is configured", status.Conditions[0].Message) } +func TestStatusCollector_SetAuthServerConfigValidatedCondition(t *testing.T) { + t.Parallel() + + vmcp := &mcpv1alpha1.VirtualMCPServer{} + collector := NewStatusManager(vmcp) + + collector.SetAuthServerConfigValidatedCondition("AuthServerConfigValid", "AuthServerConfig is valid", metav1.ConditionTrue) + + status := &mcpv1alpha1.VirtualMCPServerStatus{} + hasUpdates := collector.UpdateStatus(context.Background(), status) + + assert.True(t, hasUpdates) + assert.Len(t, status.Conditions, 1) + assert.Equal(t, mcpv1alpha1.ConditionTypeAuthServerConfigValidated, status.Conditions[0].Type) + assert.Equal(t, metav1.ConditionTrue, status.Conditions[0].Status) + assert.Equal(t, "AuthServerConfigValid", status.Conditions[0].Reason) + assert.Equal(t, "AuthServerConfig is valid", status.Conditions[0].Message) +} + func TestStatusCollector_MultipleConditions(t *testing.T) { t.Parallel()