diff --git a/cmd/thv-operator/api/v1alpha1/virtualmcpserver_types.go b/cmd/thv-operator/api/v1alpha1/virtualmcpserver_types.go index 48e441557e..5a452fdf9b 100644 --- a/cmd/thv-operator/api/v1alpha1/virtualmcpserver_types.go +++ b/cmd/thv-operator/api/v1alpha1/virtualmcpserver_types.go @@ -411,32 +411,13 @@ 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() } -// validateAuthServerConfig validates inline AuthServerConfig. -// Rules: -// - issuer must be non-empty when config is provided -// - at least one upstream provider must be configured -func (r *VirtualMCPServer) validateAuthServerConfig() error { - if r.Spec.AuthServerConfig == nil { - return nil - } - if r.Spec.AuthServerConfig.Issuer == "" { - return fmt.Errorf("spec.authServerConfig.issuer is required") - } - if len(r.Spec.AuthServerConfig.UpstreamProviders) == 0 { - return fmt.Errorf("spec.authServerConfig.upstreamProviders is required") - } - return nil -} - // validateEmbeddingServer validates EmbeddingServerRef and Optimizer configuration. // Rules: // - embeddingServerRef.name must be non-empty when ref is provided diff --git a/cmd/thv-operator/api/v1alpha1/virtualmcpserver_types_test.go b/cmd/thv-operator/api/v1alpha1/virtualmcpserver_types_test.go index af87f3c4d6..900e204326 100644 --- a/cmd/thv-operator/api/v1alpha1/virtualmcpserver_types_test.go +++ b/cmd/thv-operator/api/v1alpha1/virtualmcpserver_types_test.go @@ -479,89 +479,3 @@ func TestValidateEmbeddingServer(t *testing.T) { }) } } - -func TestValidateAuthServerConfig(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - server *VirtualMCPServer - expectError bool - errContains string - }{ - { - name: "nil_config_passes", - server: &VirtualMCPServer{ - Spec: VirtualMCPServerSpec{ - Config: config.Config{Group: "test-group"}, - }, - }, - }, - { - name: "valid_config_passes", - server: &VirtualMCPServer{ - Spec: VirtualMCPServerSpec{ - Config: config.Config{Group: "test-group"}, - AuthServerConfig: &EmbeddedAuthServerConfig{ - Issuer: "http://localhost:9090", - UpstreamProviders: []UpstreamProviderConfig{ - { - Name: "test", - Type: UpstreamProviderTypeOIDC, - OIDCConfig: &OIDCUpstreamConfig{ - IssuerURL: "https://accounts.google.com", - ClientID: "test-client", - }, - }, - }, - }, - }, - }, - }, - { - name: "empty_issuer_errors", - server: &VirtualMCPServer{ - Spec: VirtualMCPServerSpec{ - Config: config.Config{Group: "test-group"}, - AuthServerConfig: &EmbeddedAuthServerConfig{ - Issuer: "", - UpstreamProviders: []UpstreamProviderConfig{ - {Name: "test", Type: UpstreamProviderTypeOIDC}, - }, - }, - }, - }, - expectError: true, - errContains: "spec.authServerConfig.issuer is required", - }, - { - name: "no_upstreams_errors", - server: &VirtualMCPServer{ - Spec: VirtualMCPServerSpec{ - Config: config.Config{Group: "test-group"}, - AuthServerConfig: &EmbeddedAuthServerConfig{ - Issuer: "http://localhost:9090", - }, - }, - }, - expectError: true, - errContains: "spec.authServerConfig.upstreamProviders is required", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - err := tt.server.Validate() - if tt.expectError { - require.Error(t, err) - if tt.errContains != "" { - assert.Contains(t, err.Error(), tt.errContains) - } - return - } - require.NoError(t, err) - }) - } -} diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller.go b/cmd/thv-operator/controllers/virtualmcpserver_controller.go index 10b191b047..e7423de19c 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,9 +321,68 @@ func (r *VirtualMCPServerReconciler) runValidations( } } + // Validate inline AuthServerConfig (when specified). + 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 (*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.ConditionReasonAuthServerConfigInvalid, + message, + metav1.ConditionFalse, + ) + statusManager.SetObservedGeneration(vmcp.Generation) + return fmt.Errorf("%s", message) + } + + 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 func (r *VirtualMCPServerReconciler) validateGroupRef( ctx context.Context, @@ -2243,12 +2302,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/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() 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) + }) + }) +})