From 15539426c87dd3b6afa9e024c428ca2ecc2e56c5 Mon Sep 17 00:00:00 2001 From: Trey Date: Wed, 20 May 2026 11:37:34 -0700 Subject: [PATCH 1/5] Mirror MCPExternalAuthConfig Valid=False onto consumer CR conditions Closes the Status Condition Parity gap noted in #5347. Before this change, an obo-typed MCPExternalAuthConfig surfaced Valid=False/EnterpriseRequired only on the referenced resource. The consumer CR (MCPServer, MCPRemoteProxy, VirtualMCPServer) showed only the generic dispatch failure, leaving users to inspect the referenced config to understand why their workload would not deploy. Each consumer reconciler now inspects the referenced config's Valid condition. When it is False, the reconciler mirrors the source's reason and message onto its own ExternalAuthConfigValidated condition (or per-backend DiscoveredAuthConfig/BackendAuthConfig condition for vMCP). This must merge before #5329 lights up apiserver-level admission of obo-typed configs so production users see the failure on the resource they applied. The envtest integration test for this propagation is deferred to #5329 to avoid adding a setup-envtest dependency here; once #5329 admits "obo" at the CRD layer, the test no longer needs a CRD-enum bypass. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../controllers/mcpremoteproxy_controller.go | 34 +++++ .../mcpremoteproxy_controller_test.go | 43 ++++++ .../controllers/mcpserver_controller.go | 37 +++++ .../mcpserver_externalauth_test.go | 123 +++++++++++++++ .../virtualmcpserver_controller.go | 132 +++++++++++++--- .../virtualmcpserver_externalauth_test.go | 143 ++++++++++++++++++ .../virtualmcpserver_vmcpconfig_test.go | 64 ++++++++ 7 files changed, 554 insertions(+), 22 deletions(-) diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go index b832fa17ee..c783d93d4c 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go @@ -735,6 +735,14 @@ func (r *MCPRemoteProxyReconciler) handleExternalAuthConfig(ctx context.Context, return fmt.Errorf("failed to fetch MCPExternalAuthConfig: %w", err) } + // Mirror the referenced MCPExternalAuthConfig's Valid=False condition onto + // the MCPRemoteProxy so the failure is visible on the consumer CR (e.g. + // obo-typed configs surface Valid=False/EnterpriseRequired here without the + // user having to inspect the referenced MCPExternalAuthConfig). + if mirrored, err := mirrorExternalAuthConfigInvalidForRemoteProxy(proxy, externalAuthConfig); mirrored { + return err + } + // MCPRemoteProxy supports only single-upstream embedded auth server configs. // Multi-upstream requires VirtualMCPServer. if embeddedCfg := externalAuthConfig.Spec.EmbeddedAuthServer; embeddedCfg != nil && len(embeddedCfg.UpstreamProviders) > 1 { @@ -777,6 +785,32 @@ func (r *MCPRemoteProxyReconciler) handleExternalAuthConfig(ctx context.Context, return nil } +// mirrorExternalAuthConfigInvalidForRemoteProxy inspects the referenced +// MCPExternalAuthConfig's Valid condition and, when it is False, mirrors the +// reason+message onto the MCPRemoteProxy's ExternalAuthConfigValidated +// condition. Returns (true, err) when a mirror was written so callers can +// short-circuit; (false, nil) otherwise. See the equivalent helper on +// MCPServerReconciler for the propagation rationale. +func mirrorExternalAuthConfigInvalidForRemoteProxy( + proxy *mcpv1beta1.MCPRemoteProxy, + externalAuthConfig *mcpv1beta1.MCPExternalAuthConfig, +) (bool, error) { + validCond := meta.FindStatusCondition(externalAuthConfig.Status.Conditions, mcpv1beta1.ConditionTypeValid) + if validCond == nil || validCond.Status != metav1.ConditionFalse { + return false, nil + } + meta.SetStatusCondition(&proxy.Status.Conditions, metav1.Condition{ + Type: mcpv1beta1.ConditionTypeMCPRemoteProxyExternalAuthConfigValidated, + Status: metav1.ConditionFalse, + Reason: validCond.Reason, + Message: validCond.Message, + ObservedGeneration: proxy.Generation, + }) + return true, fmt.Errorf( + "referenced MCPExternalAuthConfig %s/%s is invalid (%s): %s", + proxy.Namespace, externalAuthConfig.Name, validCond.Reason, validCond.Message) +} + // handleAuthServerRef validates and tracks the hash of the referenced authServerRef config. // It updates the MCPRemoteProxy status when the auth server configuration changes and sets // the AuthServerRefValidated condition. diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go b/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go index 5a03e5c397..6f23372533 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go @@ -671,6 +671,49 @@ func TestHandleExternalAuthConfig(t *testing.T) { expectedCondStatus: metav1.ConditionFalse, expectedCondReason: mcpv1beta1.ConditionReasonMCPRemoteProxyExternalAuthConfigFetchError, }, + { + // Mirror added for #5347: when the referenced MCPExternalAuthConfig + // has Status.Conditions[Valid]=False (e.g. obo-typed configs that + // the default OBO handler rejected with Reason=EnterpriseRequired + // in upstream-only builds), the proxy reconciler must surface a + // parallel ExternalAuthConfigValidated=False with the same reason + // and message. + name: "referenced config Valid=False is mirrored onto the proxy", + proxy: &mcpv1beta1.MCPRemoteProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "obo-mirror-proxy", + Namespace: "default", + }, + Spec: mcpv1beta1.MCPRemoteProxySpec{ + RemoteURL: "https://mcp.example.com", + ExternalAuthConfigRef: &mcpv1beta1.ExternalAuthConfigRef{ + Name: "obo-config", + }, + }, + }, + externalAuth: &mcpv1beta1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "obo-config", + Namespace: "default", + }, + Spec: mcpv1beta1.MCPExternalAuthConfigSpec{ + Type: mcpv1beta1.ExternalAuthTypeOBO, + }, + Status: mcpv1beta1.MCPExternalAuthConfigStatus{ + Conditions: []metav1.Condition{{ + Type: mcpv1beta1.ConditionTypeValid, + Status: metav1.ConditionFalse, + Reason: mcpv1beta1.ConditionReasonEnterpriseRequired, + Message: "on-behalf-of (OBO) external auth type requires an enterprise build", + }}, + }, + }, + expectError: true, + errContains: "EnterpriseRequired", + expectCondition: true, + expectedCondStatus: metav1.ConditionFalse, + expectedCondReason: mcpv1beta1.ConditionReasonEnterpriseRequired, + }, { name: "embedded auth server with multiple upstreams rejected", proxy: &mcpv1beta1.MCPRemoteProxy{ diff --git a/cmd/thv-operator/controllers/mcpserver_controller.go b/cmd/thv-operator/controllers/mcpserver_controller.go index eec776c05e..b63f0b41ca 100644 --- a/cmd/thv-operator/controllers/mcpserver_controller.go +++ b/cmd/thv-operator/controllers/mcpserver_controller.go @@ -2036,6 +2036,14 @@ func (r *MCPServerReconciler) handleExternalAuthConfig(ctx context.Context, m *m return fmt.Errorf("MCPExternalAuthConfig %s not found", m.Spec.ExternalAuthConfigRef.Name) } + // Mirror the referenced MCPExternalAuthConfig's Valid=False condition onto + // the MCPServer so the failure is visible on the consumer CR (e.g. obo-typed + // configs surface Valid=False/EnterpriseRequired here without the user + // having to inspect the referenced MCPExternalAuthConfig). + if mirrored, err := r.mirrorExternalAuthConfigInvalid(m, externalAuthConfig); mirrored { + return err + } + // MCPServer supports only single-upstream embedded auth server configs. // Multi-upstream requires VirtualMCPServer. if embeddedCfg := externalAuthConfig.Spec.EmbeddedAuthServer; embeddedCfg != nil && len(embeddedCfg.UpstreamProviders) > 1 { @@ -2076,6 +2084,35 @@ func (r *MCPServerReconciler) handleExternalAuthConfig(ctx context.Context, m *m return nil } +// mirrorExternalAuthConfigInvalid inspects the referenced MCPExternalAuthConfig's +// Valid condition and, when it is False, mirrors the reason+message onto the +// MCPServer's ExternalAuthConfigValidated condition. Returns (true, err) when a +// mirror was written so callers can short-circuit; (false, nil) otherwise. +// +// This propagates failures like Reason=EnterpriseRequired (set by the +// MCPExternalAuthConfig reconciler for obo-typed configs in upstream-only +// builds) onto the consumer CR so users see the failure on the resource they +// applied, not buried in the referenced config's status. +func (*MCPServerReconciler) mirrorExternalAuthConfigInvalid( + m *mcpv1beta1.MCPServer, + externalAuthConfig *mcpv1beta1.MCPExternalAuthConfig, +) (bool, error) { + validCond := meta.FindStatusCondition(externalAuthConfig.Status.Conditions, mcpv1beta1.ConditionTypeValid) + if validCond == nil || validCond.Status != metav1.ConditionFalse { + return false, nil + } + meta.SetStatusCondition(&m.Status.Conditions, metav1.Condition{ + Type: mcpv1beta1.ConditionTypeExternalAuthConfigValidated, + Status: metav1.ConditionFalse, + Reason: validCond.Reason, + Message: validCond.Message, + ObservedGeneration: m.Generation, + }) + return true, fmt.Errorf( + "referenced MCPExternalAuthConfig %s/%s is invalid (%s): %s", + m.Namespace, externalAuthConfig.Name, validCond.Reason, validCond.Message) +} + // handleAuthServerRef validates and tracks the hash of the referenced authServerRef config. // It updates the MCPServer status when the auth server configuration changes and sets // the AuthServerRefValidated condition. diff --git a/cmd/thv-operator/controllers/mcpserver_externalauth_test.go b/cmd/thv-operator/controllers/mcpserver_externalauth_test.go index 9fa8e0ffc3..8947da4afe 100644 --- a/cmd/thv-operator/controllers/mcpserver_externalauth_test.go +++ b/cmd/thv-operator/controllers/mcpserver_externalauth_test.go @@ -22,6 +22,7 @@ import ( "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" @@ -463,3 +464,125 @@ func TestMCPServerReconciler_handleExternalAuthConfig_NoHashInConfig(t *testing. assert.Empty(t, mcpServer.Status.ExternalAuthConfigHash, "Hash should be empty when external auth config has no hash") } + +// TestMCPServerReconciler_handleExternalAuthConfig_MirrorsInvalidCondition verifies +// the Status Condition Parity mirror added for #5347: when the referenced +// MCPExternalAuthConfig has Status.Conditions[Valid]=False (e.g. an obo-typed +// config that the default OBO handler rejected with Reason=EnterpriseRequired +// in upstream-only builds), the MCPServer reconciler must surface a parallel +// ExternalAuthConfigValidated=False condition that carries the same reason and +// message — instead of silently propagating the dispatch error through the +// generic phase-Failed path. +func TestMCPServerReconciler_handleExternalAuthConfig_MirrorsInvalidCondition(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + t.Cleanup(cancel) + + const ( + authName = "obo-config" + namespace = "default" + reason = mcpv1beta1.ConditionReasonEnterpriseRequired + message = "on-behalf-of (OBO) external auth type requires an enterprise build" + ) + + tests := []struct { + name string + sourceValid *metav1.Condition + wantMirrored bool + wantReason string + wantMessage string + }{ + { + name: "source Valid=False/EnterpriseRequired is mirrored", + sourceValid: &metav1.Condition{ + Type: mcpv1beta1.ConditionTypeValid, + Status: metav1.ConditionFalse, + Reason: reason, + Message: message, + }, + wantMirrored: true, + wantReason: reason, + wantMessage: message, + }, + { + name: "source Valid=False/InvalidConfig is mirrored verbatim", + sourceValid: &metav1.Condition{ + Type: mcpv1beta1.ConditionTypeValid, + Status: metav1.ConditionFalse, + Reason: mcpv1beta1.ConditionReasonInvalidConfig, + Message: "custom OBO handler rejected the spec", + }, + wantMirrored: true, + wantReason: mcpv1beta1.ConditionReasonInvalidConfig, + wantMessage: "custom OBO handler rejected the spec", + }, + { + name: "source Valid=True does not produce a mirror", + sourceValid: &metav1.Condition{ + Type: mcpv1beta1.ConditionTypeValid, + Status: metav1.ConditionTrue, + Reason: "ValidationSucceeded", + }, + wantMirrored: false, + }, + { + name: "source with no Valid condition does not produce a mirror", + sourceValid: nil, + wantMirrored: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + require.NoError(t, mcpv1beta1.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + + externalAuthConfig := &mcpv1beta1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{Name: authName, Namespace: namespace}, + Spec: mcpv1beta1.MCPExternalAuthConfigSpec{ + Type: mcpv1beta1.ExternalAuthTypeOBO, + }, + } + if tt.sourceValid != nil { + externalAuthConfig.Status.Conditions = []metav1.Condition{*tt.sourceValid} + } + + mcpServer := &mcpv1beta1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{Name: "test-server", Namespace: namespace}, + Spec: mcpv1beta1.MCPServerSpec{ + Image: "test-image", + ExternalAuthConfigRef: &mcpv1beta1.ExternalAuthConfigRef{Name: authName}, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithRuntimeObjects(externalAuthConfig, mcpServer). + WithStatusSubresource(&mcpv1beta1.MCPServer{}). + Build() + + reconciler := newTestMCPServerReconciler(fakeClient, scheme, kubernetes.PlatformKubernetes) + + err := reconciler.handleExternalAuthConfig(ctx, mcpServer) + + cond := meta.FindStatusCondition( + mcpServer.Status.Conditions, mcpv1beta1.ConditionTypeExternalAuthConfigValidated) + + if !tt.wantMirrored { + assert.NoError(t, err, "no error expected when source is valid") + assert.Nil(t, cond, "no mirror condition expected when source is valid") + return + } + + require.Error(t, err, "handler must surface the mirrored failure so callers mark Phase=Failed") + require.NotNil(t, cond, "mirror condition must be set on MCPServer.Status.Conditions") + assert.Equal(t, metav1.ConditionFalse, cond.Status) + assert.Equal(t, tt.wantReason, cond.Reason) + assert.Equal(t, tt.wantMessage, cond.Message) + }) + } +} diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller.go b/cmd/thv-operator/controllers/virtualmcpserver_controller.go index 0307f14218..35721a642d 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_controller.go @@ -75,6 +75,11 @@ type AuthConfigError struct { BackendName string // Error is the underlying error that occurred during conversion Error error + // Reason, when non-empty, overrides the default "ConversionFailed" condition reason. + // Used to mirror upstream MCPExternalAuthConfig.Status.Conditions[Valid].Reason + // (e.g. "EnterpriseRequired") onto the per-backend auth config condition so the + // failure surfaces with the same taxonomy on the consumer CR. + Reason string } // SpecValidationError represents a spec validation failure that the user must fix. @@ -88,6 +93,58 @@ func (e *SpecValidationError) Error() string { return e.Message } +// mirroredInvalidExternalAuthConfigError signals that a referenced +// MCPExternalAuthConfig had Status.Conditions[Valid]=False and the source's +// reason+message should be mirrored onto the consumer's per-backend condition. +type mirroredInvalidExternalAuthConfigError struct { + Reason string + Message string +} + +func (e *mirroredInvalidExternalAuthConfigError) Error() string { + return fmt.Sprintf("referenced MCPExternalAuthConfig is invalid (%s): %s", e.Reason, e.Message) +} + +// mirroredExternalAuthConfigInvalid inspects a fetched MCPExternalAuthConfig +// and returns (reason, error) when its Valid condition is False, or ("", nil) +// when Valid is True/absent. The error carries the source's reason and message +// so callers can surface them on the consumer's status without re-fetching the +// object. +func mirroredExternalAuthConfigInvalid( + externalAuthConfig *mcpv1beta1.MCPExternalAuthConfig, +) (string, error) { + validCond := meta.FindStatusCondition(externalAuthConfig.Status.Conditions, mcpv1beta1.ConditionTypeValid) + if validCond == nil || validCond.Status != metav1.ConditionFalse { + return "", nil + } + return validCond.Reason, &mirroredInvalidExternalAuthConfigError{ + Reason: validCond.Reason, + Message: validCond.Message, + } +} + +// mirroredReasonFromError returns the mirrored source reason embedded in err +// (via mirroredInvalidExternalAuthConfigError) or "" if err does not carry one. +// Used by buildOutgoingAuthConfig to propagate the source's reason onto the +// per-backend AuthConfigError without re-fetching the MCPExternalAuthConfig. +func mirroredReasonFromError(err error) string { + var mirrored *mirroredInvalidExternalAuthConfigError + if stderrors.As(err, &mirrored) { + return mirrored.Reason + } + return "" +} + +// authConfigErrorReason returns the reason string to use when surfacing an +// auth-config error via SetAuthConfigCondition: the mirrored source reason +// when present, otherwise the generic "ConversionFailed". +func authConfigErrorReason(authErr *AuthConfigError) string { + if authErr != nil && authErr.Reason != "" { + return authErr.Reason + } + return "ConversionFailed" +} + // VirtualMCPServerReconciler reconciles a VirtualMCPServer object // // Resource Cleanup Strategy: @@ -2137,6 +2194,12 @@ func (r *VirtualMCPServerReconciler) convertBackendAuthConfigToVMCP( return nil, fmt.Errorf("failed to get MCPExternalAuthConfig %s: %w", crdConfig.ExternalAuthConfigRef.Name, err) } + // Mirror the source's Valid=False condition before attempting conversion + // so the per-backend condition surfaces with the same reason taxonomy. + if _, mirrorErr := mirroredExternalAuthConfigInvalid(externalAuthConfig); mirrorErr != nil { + return nil, mirrorErr + } + // Convert the external auth config to strategy return r.convertExternalAuthConfigToStrategy(externalAuthConfig) } @@ -2252,6 +2315,19 @@ func (r *VirtualMCPServerReconciler) discoverExternalAuthConfigs( continue } + // Mirror the source's Valid=False condition (e.g. EnterpriseRequired for + // obo-typed configs in upstream-only builds) onto the per-backend + // condition so the failure surfaces with the same reason taxonomy. + if mirrorReason, mirrorErr := mirroredExternalAuthConfigInvalid(externalAuthConfig); mirrorErr != nil { + authErrors = append(authErrors, AuthConfigError{ + Context: fmt.Sprintf("%s%s", authContextDiscoveredPrefix, workloadInfo.Name), + BackendName: workloadInfo.Name, + Error: mirrorErr, + Reason: mirrorReason, + }) + continue + } + // Convert MCPExternalAuthConfig to BackendAuthStrategy strategy, err := r.convertExternalAuthConfigToStrategy(externalAuthConfig) if err != nil { @@ -2349,6 +2425,7 @@ func (r *VirtualMCPServerReconciler) buildOutgoingAuthConfig( Context: authContextDefault, BackendName: "", Error: fmt.Errorf("failed to convert default auth config: %w", err), + Reason: mirroredReasonFromError(err), }) } else { outgoing.Default = injectSubjectProviderIfNeeded(defaultStrategy, vmcp.Spec.AuthServerConfig) @@ -2374,6 +2451,7 @@ func (r *VirtualMCPServerReconciler) buildOutgoingAuthConfig( Context: fmt.Sprintf("%s%s", authContextBackendPrefix, backendName), BackendName: backendName, Error: fmt.Errorf("failed to convert backend auth config: %w", err), + Reason: mirroredReasonFromError(err), }) } else { outgoing.Backends[backendName] = injectSubjectProviderIfNeeded(strategy, vmcp.Spec.AuthServerConfig) @@ -3129,27 +3207,32 @@ func setAuthConfigConditions( allAuthErrors []AuthConfigError, ) { // Build error maps by context for quick lookup - var defaultAuthError error - backendAuthErrors := make(map[string]error) - discoveredAuthErrors := make(map[string]error) + var defaultAuthError *AuthConfigError + backendAuthErrors := make(map[string]*AuthConfigError) + discoveredAuthErrors := make(map[string]*AuthConfigError) - for _, authError := range allAuthErrors { - if authError.Context == authContextDefault { - defaultAuthError = authError.Error - } else if strings.HasPrefix(authError.Context, authContextBackendPrefix) { - backendAuthErrors[authError.BackendName] = authError.Error - } else if strings.HasPrefix(authError.Context, authContextDiscoveredPrefix) { - discoveredAuthErrors[authError.BackendName] = authError.Error + for i := range allAuthErrors { + authError := &allAuthErrors[i] + switch { + case authError.Context == authContextDefault: + defaultAuthError = authError + case strings.HasPrefix(authError.Context, authContextBackendPrefix): + backendAuthErrors[authError.BackendName] = authError + case strings.HasPrefix(authError.Context, authContextDiscoveredPrefix): + discoveredAuthErrors[authError.BackendName] = authError } } // Handle DefaultAuthConfig condition if defaultAuthError != nil { - // Default auth has error - set False condition + // Default auth has error - set False condition. When the source's + // MCPExternalAuthConfig surfaced a Valid=False condition we propagate + // the source's reason (e.g. EnterpriseRequired); otherwise we report + // ConversionFailed. statusManager.SetAuthConfigCondition( "DefaultAuthConfig", - "ConversionFailed", - fmt.Sprintf("Failed to convert default auth config: %v", defaultAuthError), + authConfigErrorReason(defaultAuthError), + fmt.Sprintf("Failed to convert default auth config: %v", defaultAuthError.Error), metav1.ConditionFalse, ) } else if hasValidDefaultAuth { @@ -3188,12 +3271,15 @@ func setAuthConfigConditions( for _, backendName := range backendsWithAuthConfig { conditionType := fmt.Sprintf("DiscoveredAuthConfig-%s", backendName) - if err, hasError := discoveredAuthErrors[backendName]; hasError { - // Backend has discovered auth config error - set False condition + if authErr, hasError := discoveredAuthErrors[backendName]; hasError { + // Backend has discovered auth config error - set False condition. + // Propagate the source's reason when the underlying + // MCPExternalAuthConfig surfaced Valid=False; otherwise report + // ConversionFailed. statusManager.SetAuthConfigCondition( conditionType, - "ConversionFailed", - fmt.Sprintf("Failed to convert discovered auth config: %v", err), + authConfigErrorReason(authErr), + fmt.Sprintf("Failed to convert discovered auth config: %v", authErr.Error), metav1.ConditionFalse, ) } else { @@ -3207,14 +3293,16 @@ func setAuthConfigConditions( } } - // Set BackendAuthConfig conditions for inline backend-specific auth configs - // First, set error conditions - for backendName, err := range backendAuthErrors { + // Set BackendAuthConfig conditions for inline backend-specific auth configs. + // First, set error conditions. Propagate the source's reason when the + // underlying MCPExternalAuthConfig surfaced Valid=False; otherwise report + // ConversionFailed. + for backendName, authErr := range backendAuthErrors { conditionType := fmt.Sprintf("BackendAuthConfig-%s", backendName) statusManager.SetAuthConfigCondition( conditionType, - "ConversionFailed", - fmt.Sprintf("Failed to convert backend auth config: %v", err), + authConfigErrorReason(authErr), + fmt.Sprintf("Failed to convert backend auth config: %v", authErr.Error), metav1.ConditionFalse, ) } diff --git a/cmd/thv-operator/controllers/virtualmcpserver_externalauth_test.go b/cmd/thv-operator/controllers/virtualmcpserver_externalauth_test.go index 7e3b8945ea..80392792cc 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_externalauth_test.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_externalauth_test.go @@ -850,6 +850,149 @@ func TestConvertBackendAuthConfigToVMCP(t *testing.T) { } } +// TestConvertBackendAuthConfigToVMCP_MirrorsInvalidExternalAuthConfig verifies +// the mirror added for #5347: when the referenced MCPExternalAuthConfig has +// Status.Conditions[Valid]=False (e.g. obo-typed configs in upstream-only +// builds), the conversion must short-circuit before reaching the converter and +// return a typed error that carries the source's reason+message so callers +// (buildOutgoingAuthConfig) can propagate the reason onto the per-backend +// AuthConfigError. +func TestConvertBackendAuthConfigToVMCP_MirrorsInvalidExternalAuthConfig(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + require.NoError(t, mcpv1beta1.AddToScheme(scheme)) + + invalidSource := &mcpv1beta1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "obo-source", Namespace: "default"}, + Spec: mcpv1beta1.MCPExternalAuthConfigSpec{ + Type: mcpv1beta1.ExternalAuthTypeOBO, + }, + Status: mcpv1beta1.MCPExternalAuthConfigStatus{ + Conditions: []metav1.Condition{{ + Type: mcpv1beta1.ConditionTypeValid, + Status: metav1.ConditionFalse, + Reason: mcpv1beta1.ConditionReasonEnterpriseRequired, + Message: "obo enterprise required", + }}, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(invalidSource). + Build() + + r := &VirtualMCPServerReconciler{ + Client: fakeClient, + Scheme: scheme, + PlatformDetector: ctrlutil.NewSharedPlatformDetector(), + } + + strategy, err := r.convertBackendAuthConfigToVMCP(context.Background(), "default", &mcpv1beta1.BackendAuthConfig{ + Type: mcpv1beta1.BackendAuthTypeExternalAuthConfigRef, + ExternalAuthConfigRef: &mcpv1beta1.ExternalAuthConfigRef{Name: "obo-source"}, + }) + + require.Error(t, err, "must short-circuit when source Valid=False") + require.Nil(t, strategy) + assert.Equal(t, mcpv1beta1.ConditionReasonEnterpriseRequired, mirroredReasonFromError(err), + "buildOutgoingAuthConfig depends on this reason flowing through mirroredReasonFromError") +} + +// TestMirroredExternalAuthConfigInvalid verifies the source-condition probe +// returns the typed error + reason exactly when Status.Conditions[Valid] is +// False, and ("", nil) otherwise. +func TestMirroredExternalAuthConfigInvalid(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + conditions []metav1.Condition + wantReason string + wantErr bool + }{ + { + name: "Valid=False/EnterpriseRequired returns mirrored error and reason", + conditions: []metav1.Condition{{ + Type: mcpv1beta1.ConditionTypeValid, + Status: metav1.ConditionFalse, + Reason: mcpv1beta1.ConditionReasonEnterpriseRequired, + Message: "obo enterprise required", + }}, + wantReason: mcpv1beta1.ConditionReasonEnterpriseRequired, + wantErr: true, + }, + { + name: "Valid=True returns no mirror", + conditions: []metav1.Condition{{ + Type: mcpv1beta1.ConditionTypeValid, + Status: metav1.ConditionTrue, + Reason: "ValidationSucceeded", + }}, + }, + { + name: "no Valid condition returns no mirror", + conditions: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + cfg := &mcpv1beta1.MCPExternalAuthConfig{ + Status: mcpv1beta1.MCPExternalAuthConfigStatus{Conditions: tt.conditions}, + } + reason, err := mirroredExternalAuthConfigInvalid(cfg) + assert.Equal(t, tt.wantReason, reason) + if tt.wantErr { + require.Error(t, err) + assert.Equal(t, tt.wantReason, mirroredReasonFromError(err)) + } else { + assert.NoError(t, err) + assert.Empty(t, mirroredReasonFromError(err)) + } + }) + } +} + +// TestAuthConfigErrorReason verifies the conversion of an AuthConfigError into +// the reason string used by setAuthConfigConditions: the mirrored source +// reason when present, otherwise the generic "ConversionFailed". +func TestAuthConfigErrorReason(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in *AuthConfigError + want string + }{ + { + name: "nil falls back to ConversionFailed", + in: nil, + want: "ConversionFailed", + }, + { + name: "empty Reason falls back to ConversionFailed", + in: &AuthConfigError{}, + want: "ConversionFailed", + }, + { + name: "non-empty Reason is propagated verbatim", + in: &AuthConfigError{Reason: mcpv1beta1.ConditionReasonEnterpriseRequired}, + want: mcpv1beta1.ConditionReasonEnterpriseRequired, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, authConfigErrorReason(tt.in)) + }) + } +} + // TestGenerateUniqueTokenExchangeEnvVarName tests the generateUniqueTokenExchangeEnvVarName function func TestGenerateUniqueTokenExchangeEnvVarName(t *testing.T) { t.Parallel() diff --git a/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go b/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go index 2da5a24a45..0da03cefa3 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go @@ -931,6 +931,70 @@ func TestSetAuthConfigConditions(t *testing.T) { Times(1) }, }, + { + // Mirror added for #5347: when an AuthConfigError carries a + // non-empty Reason (set by buildOutgoingAuthConfig/ + // discoverExternalAuthConfigs when the referenced + // MCPExternalAuthConfig surfaced Valid=False), the per-backend + // condition must use that reason instead of the generic + // ConversionFailed. Covers default + discovered + inline paths. + name: "mirrored Reason propagates to default/discovered/inline conditions", + backendsWithAuthConfig: []string{"discovered-backend"}, + inlineBackendNames: []string{"inline-backend"}, + allAuthErrors: []AuthConfigError{ + { + Context: "default", + BackendName: "", + Error: fmt.Errorf("on-behalf-of (OBO) external auth type requires an enterprise build"), + Reason: mcpv1beta1.ConditionReasonEnterpriseRequired, + }, + { + Context: "discovered:discovered-backend", + BackendName: "discovered-backend", + Error: fmt.Errorf("obo enterprise required"), + Reason: mcpv1beta1.ConditionReasonEnterpriseRequired, + }, + { + Context: "backend:inline-backend", + BackendName: "inline-backend", + Error: fmt.Errorf("obo enterprise required"), + Reason: mcpv1beta1.ConditionReasonEnterpriseRequired, + }, + }, + validate: func(t *testing.T, mock *statusmocks.MockStatusManager) { + t.Helper() + mock.EXPECT(). + RemoveConditionsWithPrefix("DiscoveredAuthConfig-", []string{"DiscoveredAuthConfig-discovered-backend"}). + Times(1) + mock.EXPECT(). + RemoveConditionsWithPrefix("BackendAuthConfig-", []string{"BackendAuthConfig-inline-backend"}). + Times(1) + mock.EXPECT(). + SetAuthConfigCondition( + "DefaultAuthConfig", + mcpv1beta1.ConditionReasonEnterpriseRequired, + gomock.Any(), + metav1.ConditionFalse, + ). + Times(1) + mock.EXPECT(). + SetAuthConfigCondition( + "DiscoveredAuthConfig-discovered-backend", + mcpv1beta1.ConditionReasonEnterpriseRequired, + gomock.Any(), + metav1.ConditionFalse, + ). + Times(1) + mock.EXPECT(). + SetAuthConfigCondition( + "BackendAuthConfig-inline-backend", + mcpv1beta1.ConditionReasonEnterpriseRequired, + gomock.Any(), + metav1.ConditionFalse, + ). + Times(1) + }, + }, } for _, tt := range tests { From 7446d2a184b200ea8060bd4eb41c33c8d287f856 Mon Sep 17 00:00:00 2001 From: Trey Date: Wed, 20 May 2026 12:39:55 -0700 Subject: [PATCH 2/5] Address code review feedback on consumer-CR mirror MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two changes from the code review on the previous commit: 1. (correctness) MCPServer.handleExternalAuthConfig now clears any stale ExternalAuthConfigValidated=False the mirror previously wrote when the referenced source has healed (Valid=True or absent). Without this, the False condition outlived its cause: the user fixed their spec but the mirror stayed sticky because MCPServer has no downstream True setter on this condition. Adds a regression-guard test case. 2. (parity) The mirror probe, typed error, and reason-extraction helper now live in a shared external_auth_mirror.go file instead of being duplicated across the three consumer reconcilers. All three reconcilers (MCPServer, MCPRemoteProxy, VirtualMCPServer) call the same primitive and surface the same typed error so downstream consumers like buildOutgoingAuthConfig can react without string-matching. MCPRemoteProxy already sets ExternalAuthConfigValidated=True downstream and so does not need the heal-clearing branch — its existing True writer overwrites any stale mirror. VirtualMCPServer heals naturally because setAuthConfigConditions removes stale per-backend conditions on each reconcile. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../controllers/external_auth_mirror.go | 68 +++++++++++++++++++ .../controllers/mcpremoteproxy_controller.go | 21 +++--- .../controllers/mcpserver_controller.go | 27 +++++--- .../mcpserver_externalauth_test.go | 39 +++++++++-- .../virtualmcpserver_controller.go | 42 ------------ 5 files changed, 133 insertions(+), 64 deletions(-) create mode 100644 cmd/thv-operator/controllers/external_auth_mirror.go diff --git a/cmd/thv-operator/controllers/external_auth_mirror.go b/cmd/thv-operator/controllers/external_auth_mirror.go new file mode 100644 index 0000000000..684b1998a6 --- /dev/null +++ b/cmd/thv-operator/controllers/external_auth_mirror.go @@ -0,0 +1,68 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package controllers + +import ( + stderrors "errors" + "fmt" + + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1" +) + +// mirroredInvalidExternalAuthConfigError signals that a referenced +// MCPExternalAuthConfig had Status.Conditions[Valid]=False and the source's +// reason+message should be mirrored onto the consumer's status. Callers may +// extract the reason via mirroredReasonFromError when they need to propagate +// it through layers that only carry a generic error (notably the +// VirtualMCPServer AuthConfigError aggregator). +type mirroredInvalidExternalAuthConfigError struct { + Reason string + Message string +} + +func (e *mirroredInvalidExternalAuthConfigError) Error() string { + return fmt.Sprintf("referenced MCPExternalAuthConfig is invalid (%s): %s", e.Reason, e.Message) +} + +// mirroredExternalAuthConfigInvalid inspects a fetched MCPExternalAuthConfig +// and returns (reason, error) when its Valid condition is False, or ("", nil) +// when Valid is True/absent. The returned error is a +// *mirroredInvalidExternalAuthConfigError that carries the source's reason and +// message so callers can surface them on the consumer's status without +// re-fetching the object. +// +// Consumer reconcilers (MCPServer, MCPRemoteProxy, VirtualMCPServer) use this +// probe to mirror the source's Valid=False condition onto the consumer CR, +// closing the Status Condition Parity gap described in #5347. Without the +// mirror, an obo-typed MCPExternalAuthConfig in an upstream-only build would +// surface EnterpriseRequired only on the referenced resource, leaving the +// consumer's status to report only the generic dispatch failure. +func mirroredExternalAuthConfigInvalid( + externalAuthConfig *mcpv1beta1.MCPExternalAuthConfig, +) (string, error) { + validCond := meta.FindStatusCondition(externalAuthConfig.Status.Conditions, mcpv1beta1.ConditionTypeValid) + if validCond == nil || validCond.Status != metav1.ConditionFalse { + return "", nil + } + return validCond.Reason, &mirroredInvalidExternalAuthConfigError{ + Reason: validCond.Reason, + Message: validCond.Message, + } +} + +// mirroredReasonFromError returns the mirrored source reason embedded in err +// (via *mirroredInvalidExternalAuthConfigError) or "" if err does not carry +// one. Used by the VirtualMCPServer buildOutgoingAuthConfig pipeline to +// propagate the source's reason onto the per-backend AuthConfigError without +// re-fetching the MCPExternalAuthConfig. +func mirroredReasonFromError(err error) string { + var mirrored *mirroredInvalidExternalAuthConfigError + if stderrors.As(err, &mirrored) { + return mirrored.Reason + } + return "" +} diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go index c783d93d4c..480727da99 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go @@ -789,26 +789,31 @@ func (r *MCPRemoteProxyReconciler) handleExternalAuthConfig(ctx context.Context, // MCPExternalAuthConfig's Valid condition and, when it is False, mirrors the // reason+message onto the MCPRemoteProxy's ExternalAuthConfigValidated // condition. Returns (true, err) when a mirror was written so callers can -// short-circuit; (false, nil) otherwise. See the equivalent helper on -// MCPServerReconciler for the propagation rationale. +// short-circuit; (false, nil) otherwise. The healed path is not handled here +// because handleExternalAuthConfig sets ExternalAuthConfigValidated=True +// downstream, which overwrites any stale False this helper previously wrote. +// See the equivalent helper on MCPServerReconciler for the propagation +// rationale. func mirrorExternalAuthConfigInvalidForRemoteProxy( proxy *mcpv1beta1.MCPRemoteProxy, externalAuthConfig *mcpv1beta1.MCPExternalAuthConfig, ) (bool, error) { - validCond := meta.FindStatusCondition(externalAuthConfig.Status.Conditions, mcpv1beta1.ConditionTypeValid) - if validCond == nil || validCond.Status != metav1.ConditionFalse { + reason, mirrorErr := mirroredExternalAuthConfigInvalid(externalAuthConfig) + if mirrorErr == nil { return false, nil } + var mirrored *mirroredInvalidExternalAuthConfigError + stderrors.As(mirrorErr, &mirrored) meta.SetStatusCondition(&proxy.Status.Conditions, metav1.Condition{ Type: mcpv1beta1.ConditionTypeMCPRemoteProxyExternalAuthConfigValidated, Status: metav1.ConditionFalse, - Reason: validCond.Reason, - Message: validCond.Message, + Reason: reason, + Message: mirrored.Message, ObservedGeneration: proxy.Generation, }) return true, fmt.Errorf( - "referenced MCPExternalAuthConfig %s/%s is invalid (%s): %s", - proxy.Namespace, externalAuthConfig.Name, validCond.Reason, validCond.Message) + "referenced MCPExternalAuthConfig %s/%s is invalid: %w", + proxy.Namespace, externalAuthConfig.Name, mirrorErr) } // handleAuthServerRef validates and tracks the hash of the referenced authServerRef config. diff --git a/cmd/thv-operator/controllers/mcpserver_controller.go b/cmd/thv-operator/controllers/mcpserver_controller.go index b63f0b41ca..552cbda0aa 100644 --- a/cmd/thv-operator/controllers/mcpserver_controller.go +++ b/cmd/thv-operator/controllers/mcpserver_controller.go @@ -8,6 +8,7 @@ package controllers import ( "context" "encoding/json" + stderrors "errors" "fmt" "maps" "os" @@ -2086,8 +2087,11 @@ func (r *MCPServerReconciler) handleExternalAuthConfig(ctx context.Context, m *m // mirrorExternalAuthConfigInvalid inspects the referenced MCPExternalAuthConfig's // Valid condition and, when it is False, mirrors the reason+message onto the -// MCPServer's ExternalAuthConfigValidated condition. Returns (true, err) when a -// mirror was written so callers can short-circuit; (false, nil) otherwise. +// MCPServer's ExternalAuthConfigValidated condition. When the source is +// Valid=True (i.e. has healed since a previous False), the helper clears any +// stale mirror it previously wrote so the condition doesn't outlive the cause. +// Returns (true, err) when a mirror was written so callers can short-circuit; +// (false, nil) otherwise. // // This propagates failures like Reason=EnterpriseRequired (set by the // MCPExternalAuthConfig reconciler for obo-typed configs in upstream-only @@ -2097,20 +2101,27 @@ func (*MCPServerReconciler) mirrorExternalAuthConfigInvalid( m *mcpv1beta1.MCPServer, externalAuthConfig *mcpv1beta1.MCPExternalAuthConfig, ) (bool, error) { - validCond := meta.FindStatusCondition(externalAuthConfig.Status.Conditions, mcpv1beta1.ConditionTypeValid) - if validCond == nil || validCond.Status != metav1.ConditionFalse { + reason, mirrorErr := mirroredExternalAuthConfigInvalid(externalAuthConfig) + if mirrorErr == nil { + // Source healed (Valid=True or absent). Clear any stale mirror so it + // doesn't persist after the cause is gone. Downstream branches in + // handleExternalAuthConfig (e.g. multi-upstream) re-set False if they + // still apply on this reconcile. + meta.RemoveStatusCondition(&m.Status.Conditions, mcpv1beta1.ConditionTypeExternalAuthConfigValidated) return false, nil } + var mirrored *mirroredInvalidExternalAuthConfigError + stderrors.As(mirrorErr, &mirrored) meta.SetStatusCondition(&m.Status.Conditions, metav1.Condition{ Type: mcpv1beta1.ConditionTypeExternalAuthConfigValidated, Status: metav1.ConditionFalse, - Reason: validCond.Reason, - Message: validCond.Message, + Reason: reason, + Message: mirrored.Message, ObservedGeneration: m.Generation, }) return true, fmt.Errorf( - "referenced MCPExternalAuthConfig %s/%s is invalid (%s): %s", - m.Namespace, externalAuthConfig.Name, validCond.Reason, validCond.Message) + "referenced MCPExternalAuthConfig %s/%s is invalid: %w", + m.Namespace, externalAuthConfig.Name, mirrorErr) } // handleAuthServerRef validates and tracks the hash of the referenced authServerRef config. diff --git a/cmd/thv-operator/controllers/mcpserver_externalauth_test.go b/cmd/thv-operator/controllers/mcpserver_externalauth_test.go index 8947da4afe..2d9f1dd835 100644 --- a/cmd/thv-operator/controllers/mcpserver_externalauth_test.go +++ b/cmd/thv-operator/controllers/mcpserver_externalauth_test.go @@ -487,11 +487,13 @@ func TestMCPServerReconciler_handleExternalAuthConfig_MirrorsInvalidCondition(t ) tests := []struct { - name string - sourceValid *metav1.Condition - wantMirrored bool - wantReason string - wantMessage string + name string + sourceValid *metav1.Condition + preexisting []metav1.Condition + wantMirrored bool + wantReason string + wantMessage string + wantPreexistingCleared bool }{ { name: "source Valid=False/EnterpriseRequired is mirrored", @@ -531,6 +533,26 @@ func TestMCPServerReconciler_handleExternalAuthConfig_MirrorsInvalidCondition(t sourceValid: nil, wantMirrored: false, }, + { + // Regression guard: once the source heals, a stale mirror left from + // a previous reconcile must be cleared so the condition does not + // outlive its cause. Without the heal path, the False sticks + // forever even after the user fixes the spec. + name: "stale mirror is cleared when source has healed", + sourceValid: &metav1.Condition{ + Type: mcpv1beta1.ConditionTypeValid, + Status: metav1.ConditionTrue, + Reason: "ValidationSucceeded", + }, + preexisting: []metav1.Condition{{ + Type: mcpv1beta1.ConditionTypeExternalAuthConfigValidated, + Status: metav1.ConditionFalse, + Reason: mcpv1beta1.ConditionReasonEnterpriseRequired, + Message: "stale mirror from a previous reconcile", + }}, + wantMirrored: false, + wantPreexistingCleared: true, + }, } for _, tt := range tests { @@ -557,6 +579,7 @@ func TestMCPServerReconciler_handleExternalAuthConfig_MirrorsInvalidCondition(t Image: "test-image", ExternalAuthConfigRef: &mcpv1beta1.ExternalAuthConfigRef{Name: authName}, }, + Status: mcpv1beta1.MCPServerStatus{Conditions: tt.preexisting}, } fakeClient := fake.NewClientBuilder(). @@ -574,7 +597,11 @@ func TestMCPServerReconciler_handleExternalAuthConfig_MirrorsInvalidCondition(t if !tt.wantMirrored { assert.NoError(t, err, "no error expected when source is valid") - assert.Nil(t, cond, "no mirror condition expected when source is valid") + if tt.wantPreexistingCleared { + assert.Nil(t, cond, "stale mirror condition must be cleared once source has healed") + } else { + assert.Nil(t, cond, "no mirror condition expected when source is valid") + } return } diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller.go b/cmd/thv-operator/controllers/virtualmcpserver_controller.go index 35721a642d..32eccde415 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_controller.go @@ -93,48 +93,6 @@ func (e *SpecValidationError) Error() string { return e.Message } -// mirroredInvalidExternalAuthConfigError signals that a referenced -// MCPExternalAuthConfig had Status.Conditions[Valid]=False and the source's -// reason+message should be mirrored onto the consumer's per-backend condition. -type mirroredInvalidExternalAuthConfigError struct { - Reason string - Message string -} - -func (e *mirroredInvalidExternalAuthConfigError) Error() string { - return fmt.Sprintf("referenced MCPExternalAuthConfig is invalid (%s): %s", e.Reason, e.Message) -} - -// mirroredExternalAuthConfigInvalid inspects a fetched MCPExternalAuthConfig -// and returns (reason, error) when its Valid condition is False, or ("", nil) -// when Valid is True/absent. The error carries the source's reason and message -// so callers can surface them on the consumer's status without re-fetching the -// object. -func mirroredExternalAuthConfigInvalid( - externalAuthConfig *mcpv1beta1.MCPExternalAuthConfig, -) (string, error) { - validCond := meta.FindStatusCondition(externalAuthConfig.Status.Conditions, mcpv1beta1.ConditionTypeValid) - if validCond == nil || validCond.Status != metav1.ConditionFalse { - return "", nil - } - return validCond.Reason, &mirroredInvalidExternalAuthConfigError{ - Reason: validCond.Reason, - Message: validCond.Message, - } -} - -// mirroredReasonFromError returns the mirrored source reason embedded in err -// (via mirroredInvalidExternalAuthConfigError) or "" if err does not carry one. -// Used by buildOutgoingAuthConfig to propagate the source's reason onto the -// per-backend AuthConfigError without re-fetching the MCPExternalAuthConfig. -func mirroredReasonFromError(err error) string { - var mirrored *mirroredInvalidExternalAuthConfigError - if stderrors.As(err, &mirrored) { - return mirrored.Reason - } - return "" -} - // authConfigErrorReason returns the reason string to use when surfacing an // auth-config error via SetAuthConfigCondition: the mirrored source reason // when present, otherwise the generic "ConversionFailed". From 2cbc121e063cce28fb12993804dfaeab22d6bc77 Mon Sep 17 00:00:00 2001 From: Trey Date: Wed, 20 May 2026 13:04:14 -0700 Subject: [PATCH 3/5] Tighten mirror probe API; trim duplicated error prefix Two changes from the second round of code review feedback on the consumer-CR mirror: 1. mirroredExternalAuthConfigInvalid now returns the typed *mirroredInvalidExternalAuthConfigError directly (nil when the source is healed) instead of a (reason, error) tuple. Callers no longer need errors.As to recover the message field, and the pointer-or-nil shape removes the awkward "discard the bool" pattern at the MCPServer and MCPRemoteProxy call sites. The typed value still satisfies the error interface so vMCP's convertBackendAuthConfigToVMCP can keep passing it through opaque error-returning APIs and recover the reason later via mirroredReasonFromError. 2. MCPServer and MCPRemoteProxy's outer error wrap no longer repeats the typed error's "referenced MCPExternalAuthConfig is invalid" prefix, so messages render as a single sentence instead of a doubled phrase. The namespaced name is preserved through %w. Updates the mirror probe's unit test to the new return shape and adds a round-trip assertion that the typed pointer flows through mirroredReasonFromError. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../controllers/external_auth_mirror.go | 22 ++++++---- .../controllers/mcpremoteproxy_controller.go | 12 ++---- .../controllers/mcpserver_controller.go | 13 ++---- .../virtualmcpserver_controller.go | 10 ++--- .../virtualmcpserver_externalauth_test.go | 42 ++++++++++--------- 5 files changed, 50 insertions(+), 49 deletions(-) diff --git a/cmd/thv-operator/controllers/external_auth_mirror.go b/cmd/thv-operator/controllers/external_auth_mirror.go index 684b1998a6..2da2ecd074 100644 --- a/cmd/thv-operator/controllers/external_auth_mirror.go +++ b/cmd/thv-operator/controllers/external_auth_mirror.go @@ -29,11 +29,17 @@ func (e *mirroredInvalidExternalAuthConfigError) Error() string { } // mirroredExternalAuthConfigInvalid inspects a fetched MCPExternalAuthConfig -// and returns (reason, error) when its Valid condition is False, or ("", nil) -// when Valid is True/absent. The returned error is a -// *mirroredInvalidExternalAuthConfigError that carries the source's reason and -// message so callers can surface them on the consumer's status without -// re-fetching the object. +// and returns a non-nil *mirroredInvalidExternalAuthConfigError when its Valid +// condition is False, or nil when Valid is True/absent. The returned value +// implements error and carries the source's reason and message so callers can +// surface them on the consumer's status without re-fetching the object. +// +// Returning the concrete pointer (rather than an (error, string) tuple) keeps +// callers from needing errors.As to recover the reason/message fields when +// they have the value in hand. The error path still satisfies the standard +// error interface, so callers that propagate it through layers that only +// carry error (notably convertBackendAuthConfigToVMCP -> buildOutgoingAuthConfig) +// can recover the same pointer via mirroredReasonFromError below. // // Consumer reconcilers (MCPServer, MCPRemoteProxy, VirtualMCPServer) use this // probe to mirror the source's Valid=False condition onto the consumer CR, @@ -43,12 +49,12 @@ func (e *mirroredInvalidExternalAuthConfigError) Error() string { // consumer's status to report only the generic dispatch failure. func mirroredExternalAuthConfigInvalid( externalAuthConfig *mcpv1beta1.MCPExternalAuthConfig, -) (string, error) { +) *mirroredInvalidExternalAuthConfigError { validCond := meta.FindStatusCondition(externalAuthConfig.Status.Conditions, mcpv1beta1.ConditionTypeValid) if validCond == nil || validCond.Status != metav1.ConditionFalse { - return "", nil + return nil } - return validCond.Reason, &mirroredInvalidExternalAuthConfigError{ + return &mirroredInvalidExternalAuthConfigError{ Reason: validCond.Reason, Message: validCond.Message, } diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go index 480727da99..2f866062b8 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go @@ -798,22 +798,18 @@ func mirrorExternalAuthConfigInvalidForRemoteProxy( proxy *mcpv1beta1.MCPRemoteProxy, externalAuthConfig *mcpv1beta1.MCPExternalAuthConfig, ) (bool, error) { - reason, mirrorErr := mirroredExternalAuthConfigInvalid(externalAuthConfig) - if mirrorErr == nil { + mirrored := mirroredExternalAuthConfigInvalid(externalAuthConfig) + if mirrored == nil { return false, nil } - var mirrored *mirroredInvalidExternalAuthConfigError - stderrors.As(mirrorErr, &mirrored) meta.SetStatusCondition(&proxy.Status.Conditions, metav1.Condition{ Type: mcpv1beta1.ConditionTypeMCPRemoteProxyExternalAuthConfigValidated, Status: metav1.ConditionFalse, - Reason: reason, + Reason: mirrored.Reason, Message: mirrored.Message, ObservedGeneration: proxy.Generation, }) - return true, fmt.Errorf( - "referenced MCPExternalAuthConfig %s/%s is invalid: %w", - proxy.Namespace, externalAuthConfig.Name, mirrorErr) + return true, fmt.Errorf("MCPExternalAuthConfig %s/%s: %w", proxy.Namespace, externalAuthConfig.Name, mirrored) } // handleAuthServerRef validates and tracks the hash of the referenced authServerRef config. diff --git a/cmd/thv-operator/controllers/mcpserver_controller.go b/cmd/thv-operator/controllers/mcpserver_controller.go index 552cbda0aa..f1f6fd4ddc 100644 --- a/cmd/thv-operator/controllers/mcpserver_controller.go +++ b/cmd/thv-operator/controllers/mcpserver_controller.go @@ -8,7 +8,6 @@ package controllers import ( "context" "encoding/json" - stderrors "errors" "fmt" "maps" "os" @@ -2101,8 +2100,8 @@ func (*MCPServerReconciler) mirrorExternalAuthConfigInvalid( m *mcpv1beta1.MCPServer, externalAuthConfig *mcpv1beta1.MCPExternalAuthConfig, ) (bool, error) { - reason, mirrorErr := mirroredExternalAuthConfigInvalid(externalAuthConfig) - if mirrorErr == nil { + mirrored := mirroredExternalAuthConfigInvalid(externalAuthConfig) + if mirrored == nil { // Source healed (Valid=True or absent). Clear any stale mirror so it // doesn't persist after the cause is gone. Downstream branches in // handleExternalAuthConfig (e.g. multi-upstream) re-set False if they @@ -2110,18 +2109,14 @@ func (*MCPServerReconciler) mirrorExternalAuthConfigInvalid( meta.RemoveStatusCondition(&m.Status.Conditions, mcpv1beta1.ConditionTypeExternalAuthConfigValidated) return false, nil } - var mirrored *mirroredInvalidExternalAuthConfigError - stderrors.As(mirrorErr, &mirrored) meta.SetStatusCondition(&m.Status.Conditions, metav1.Condition{ Type: mcpv1beta1.ConditionTypeExternalAuthConfigValidated, Status: metav1.ConditionFalse, - Reason: reason, + Reason: mirrored.Reason, Message: mirrored.Message, ObservedGeneration: m.Generation, }) - return true, fmt.Errorf( - "referenced MCPExternalAuthConfig %s/%s is invalid: %w", - m.Namespace, externalAuthConfig.Name, mirrorErr) + return true, fmt.Errorf("MCPExternalAuthConfig %s/%s: %w", m.Namespace, externalAuthConfig.Name, mirrored) } // handleAuthServerRef validates and tracks the hash of the referenced authServerRef config. diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller.go b/cmd/thv-operator/controllers/virtualmcpserver_controller.go index 32eccde415..04e66f9a3d 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_controller.go @@ -2154,8 +2154,8 @@ func (r *VirtualMCPServerReconciler) convertBackendAuthConfigToVMCP( // Mirror the source's Valid=False condition before attempting conversion // so the per-backend condition surfaces with the same reason taxonomy. - if _, mirrorErr := mirroredExternalAuthConfigInvalid(externalAuthConfig); mirrorErr != nil { - return nil, mirrorErr + if mirrored := mirroredExternalAuthConfigInvalid(externalAuthConfig); mirrored != nil { + return nil, mirrored } // Convert the external auth config to strategy @@ -2276,12 +2276,12 @@ func (r *VirtualMCPServerReconciler) discoverExternalAuthConfigs( // Mirror the source's Valid=False condition (e.g. EnterpriseRequired for // obo-typed configs in upstream-only builds) onto the per-backend // condition so the failure surfaces with the same reason taxonomy. - if mirrorReason, mirrorErr := mirroredExternalAuthConfigInvalid(externalAuthConfig); mirrorErr != nil { + if mirrored := mirroredExternalAuthConfigInvalid(externalAuthConfig); mirrored != nil { authErrors = append(authErrors, AuthConfigError{ Context: fmt.Sprintf("%s%s", authContextDiscoveredPrefix, workloadInfo.Name), BackendName: workloadInfo.Name, - Error: mirrorErr, - Reason: mirrorReason, + Error: mirrored, + Reason: mirrored.Reason, }) continue } diff --git a/cmd/thv-operator/controllers/virtualmcpserver_externalauth_test.go b/cmd/thv-operator/controllers/virtualmcpserver_externalauth_test.go index 80392792cc..96eec1fc0c 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_externalauth_test.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_externalauth_test.go @@ -901,30 +901,33 @@ func TestConvertBackendAuthConfigToVMCP_MirrorsInvalidExternalAuthConfig(t *test } // TestMirroredExternalAuthConfigInvalid verifies the source-condition probe -// returns the typed error + reason exactly when Status.Conditions[Valid] is -// False, and ("", nil) otherwise. +// returns the typed pointer exactly when Status.Conditions[Valid] is False, +// and nil otherwise. Also asserts that the typed value satisfies the error +// interface so callers can pass it through error-returning APIs (notably +// convertBackendAuthConfigToVMCP -> buildOutgoingAuthConfig) and recover the +// reason via mirroredReasonFromError. func TestMirroredExternalAuthConfigInvalid(t *testing.T) { t.Parallel() tests := []struct { - name string - conditions []metav1.Condition - wantReason string - wantErr bool + name string + conditions []metav1.Condition + wantReason string + wantMessage string }{ { - name: "Valid=False/EnterpriseRequired returns mirrored error and reason", + name: "Valid=False/EnterpriseRequired returns mirrored pointer", conditions: []metav1.Condition{{ Type: mcpv1beta1.ConditionTypeValid, Status: metav1.ConditionFalse, Reason: mcpv1beta1.ConditionReasonEnterpriseRequired, Message: "obo enterprise required", }}, - wantReason: mcpv1beta1.ConditionReasonEnterpriseRequired, - wantErr: true, + wantReason: mcpv1beta1.ConditionReasonEnterpriseRequired, + wantMessage: "obo enterprise required", }, { - name: "Valid=True returns no mirror", + name: "Valid=True returns nil", conditions: []metav1.Condition{{ Type: mcpv1beta1.ConditionTypeValid, Status: metav1.ConditionTrue, @@ -932,7 +935,7 @@ func TestMirroredExternalAuthConfigInvalid(t *testing.T) { }}, }, { - name: "no Valid condition returns no mirror", + name: "no Valid condition returns nil", conditions: nil, }, } @@ -944,15 +947,16 @@ func TestMirroredExternalAuthConfigInvalid(t *testing.T) { cfg := &mcpv1beta1.MCPExternalAuthConfig{ Status: mcpv1beta1.MCPExternalAuthConfigStatus{Conditions: tt.conditions}, } - reason, err := mirroredExternalAuthConfigInvalid(cfg) - assert.Equal(t, tt.wantReason, reason) - if tt.wantErr { - require.Error(t, err) - assert.Equal(t, tt.wantReason, mirroredReasonFromError(err)) - } else { - assert.NoError(t, err) - assert.Empty(t, mirroredReasonFromError(err)) + mirrored := mirroredExternalAuthConfigInvalid(cfg) + if tt.wantReason == "" { + assert.Nil(t, mirrored) + return } + require.NotNil(t, mirrored) + assert.Equal(t, tt.wantReason, mirrored.Reason) + assert.Equal(t, tt.wantMessage, mirrored.Message) + // Round-trips through error-typed APIs. + assert.Equal(t, tt.wantReason, mirroredReasonFromError(mirrored)) }) } } From 73e089ae55b0926815a18ac8698d676abf3f10e6 Mon Sep 17 00:00:00 2001 From: Trey Date: Thu, 21 May 2026 08:26:45 -0700 Subject: [PATCH 4/5] Clear stale consumer-CR mirror across all heal paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses stacklok/toolhive#5354 review comments: - HIGH cmd/thv-operator/controllers/mcpserver_controller.go (F1): the previous mirror added in this branch handled the source-Valid-flips-True heal path, but the no-ref early-return in handleExternalAuthConfig never reached the helper, so removing the spec.externalAuthConfigRef left a stale Valid=False/EnterpriseRequired on the consumer indefinitely. Clear the condition in the no-ref branch and in both NotFound branches so the mirror does not outlive its cause. - MEDIUM cmd/thv-operator/controllers/mcpremoteproxy_controller.go (F2): mirror helper now defensively clears its own condition on the healed path, matching MCPServer. The downstream True-writer at the end of handleExternalAuthConfig still sets the ConditionReasonMCPRemoteProxyExternalAuthConfigValid reason on success; the defensive clear closes the gap if a future control-flow change adds an early return between this helper and that writer. - LOW cmd/thv-operator/controllers/mcpserver_externalauth_test.go (F10): two new regression-guard tests for the heal paths uncovered by F1 — spec.externalAuthConfigRef removed, referenced source NotFound. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../controllers/mcpremoteproxy_controller.go | 14 +-- .../controllers/mcpserver_controller.go | 10 +- .../mcpserver_externalauth_test.go | 92 +++++++++++++++++++ 3 files changed, 109 insertions(+), 7 deletions(-) diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go index 2f866062b8..613a35f71f 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go @@ -788,18 +788,20 @@ func (r *MCPRemoteProxyReconciler) handleExternalAuthConfig(ctx context.Context, // mirrorExternalAuthConfigInvalidForRemoteProxy inspects the referenced // MCPExternalAuthConfig's Valid condition and, when it is False, mirrors the // reason+message onto the MCPRemoteProxy's ExternalAuthConfigValidated -// condition. Returns (true, err) when a mirror was written so callers can -// short-circuit; (false, nil) otherwise. The healed path is not handled here -// because handleExternalAuthConfig sets ExternalAuthConfigValidated=True -// downstream, which overwrites any stale False this helper previously wrote. -// See the equivalent helper on MCPServerReconciler for the propagation -// rationale. +// condition. When the source is healed (Valid=True or absent), the helper +// also clears any stale mirror it previously wrote so the heal contract is +// robust to control-flow changes between this site and the downstream True +// writer at the end of handleExternalAuthConfig. Returns (true, err) when a +// mirror was written so callers can short-circuit; (false, nil) otherwise. +// See the equivalent helper for MCPServer for the propagation rationale. func mirrorExternalAuthConfigInvalidForRemoteProxy( proxy *mcpv1beta1.MCPRemoteProxy, externalAuthConfig *mcpv1beta1.MCPExternalAuthConfig, ) (bool, error) { mirrored := mirroredExternalAuthConfigInvalid(externalAuthConfig) if mirrored == nil { + meta.RemoveStatusCondition( + &proxy.Status.Conditions, mcpv1beta1.ConditionTypeMCPRemoteProxyExternalAuthConfigValidated) return false, nil } meta.SetStatusCondition(&proxy.Status.Conditions, metav1.Condition{ diff --git a/cmd/thv-operator/controllers/mcpserver_controller.go b/cmd/thv-operator/controllers/mcpserver_controller.go index f1f6fd4ddc..e70fedd9fb 100644 --- a/cmd/thv-operator/controllers/mcpserver_controller.go +++ b/cmd/thv-operator/controllers/mcpserver_controller.go @@ -2016,7 +2016,9 @@ func getToolhiveRunnerImage() string { func (r *MCPServerReconciler) handleExternalAuthConfig(ctx context.Context, m *mcpv1beta1.MCPServer) error { ctxLogger := log.FromContext(ctx) if m.Spec.ExternalAuthConfigRef == nil { - // No MCPExternalAuthConfig referenced, clear any stored hash + // No MCPExternalAuthConfig referenced. Clear any stale mirror written + // while the ref was set so the condition doesn't outlive its cause. + meta.RemoveStatusCondition(&m.Status.Conditions, mcpv1beta1.ConditionTypeExternalAuthConfigValidated) if m.Status.ExternalAuthConfigHash != "" { m.Status.ExternalAuthConfigHash = "" if err := r.Status().Update(ctx, m); err != nil { @@ -2029,10 +2031,16 @@ func (r *MCPServerReconciler) handleExternalAuthConfig(ctx context.Context, m *m // Get the referenced MCPExternalAuthConfig externalAuthConfig, err := GetExternalAuthConfigForMCPServer(ctx, r.Client, m) if err != nil { + // Source lookup failed (e.g. NotFound). Clear any stale mirror — the + // referenced source no longer exists, so the previous mirror is no + // longer load-bearing. Pre-existing behavior surfaces the lookup + // error through Phase=Failed at the caller. + meta.RemoveStatusCondition(&m.Status.Conditions, mcpv1beta1.ConditionTypeExternalAuthConfigValidated) return err } if externalAuthConfig == nil { + meta.RemoveStatusCondition(&m.Status.Conditions, mcpv1beta1.ConditionTypeExternalAuthConfigValidated) return fmt.Errorf("MCPExternalAuthConfig %s not found", m.Spec.ExternalAuthConfigRef.Name) } diff --git a/cmd/thv-operator/controllers/mcpserver_externalauth_test.go b/cmd/thv-operator/controllers/mcpserver_externalauth_test.go index 2d9f1dd835..7817ae880c 100644 --- a/cmd/thv-operator/controllers/mcpserver_externalauth_test.go +++ b/cmd/thv-operator/controllers/mcpserver_externalauth_test.go @@ -613,3 +613,95 @@ func TestMCPServerReconciler_handleExternalAuthConfig_MirrorsInvalidCondition(t }) } } + +// TestMCPServerReconciler_handleExternalAuthConfig_ClearsMirrorOnRefRemoved is +// a regression guard for the no-ref heal path: handleExternalAuthConfig's +// early-return branch (m.Spec.ExternalAuthConfigRef == nil) never reaches the +// mirror helper, so if a stale ExternalAuthConfigValidated=False sits on the +// status from a previous reconcile, the user removing the ref must still +// clear it — otherwise the condition outlives its cause. +func TestMCPServerReconciler_handleExternalAuthConfig_ClearsMirrorOnRefRemoved(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + t.Cleanup(cancel) + + scheme := runtime.NewScheme() + require.NoError(t, mcpv1beta1.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + + mcpServer := &mcpv1beta1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{Name: "test-server", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerSpec{Image: "test-image" /* no ExternalAuthConfigRef */}, + Status: mcpv1beta1.MCPServerStatus{ + Conditions: []metav1.Condition{{ + Type: mcpv1beta1.ConditionTypeExternalAuthConfigValidated, + Status: metav1.ConditionFalse, + Reason: mcpv1beta1.ConditionReasonEnterpriseRequired, + Message: "stale mirror from when the ref pointed at an obo-typed config", + }}, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithRuntimeObjects(mcpServer). + WithStatusSubresource(&mcpv1beta1.MCPServer{}). + Build() + + reconciler := newTestMCPServerReconciler(fakeClient, scheme, kubernetes.PlatformKubernetes) + + require.NoError(t, reconciler.handleExternalAuthConfig(ctx, mcpServer)) + assert.Nil(t, + meta.FindStatusCondition(mcpServer.Status.Conditions, mcpv1beta1.ConditionTypeExternalAuthConfigValidated), + "stale mirror must be cleared in the no-ref early-return branch") +} + +// TestMCPServerReconciler_handleExternalAuthConfig_ClearsMirrorOnSourceNotFound +// is the second F10 regression guard: when the referenced MCPExternalAuthConfig +// is deleted between reconciles, handleExternalAuthConfig returns the lookup +// error and previously left any stale mirror in place. The fix clears the +// mirror in both NotFound branches so the condition does not outlive the +// source. +func TestMCPServerReconciler_handleExternalAuthConfig_ClearsMirrorOnSourceNotFound(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + t.Cleanup(cancel) + + scheme := runtime.NewScheme() + require.NoError(t, mcpv1beta1.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + + mcpServer := &mcpv1beta1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{Name: "test-server", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerSpec{ + Image: "test-image", + ExternalAuthConfigRef: &mcpv1beta1.ExternalAuthConfigRef{Name: "gone"}, + }, + Status: mcpv1beta1.MCPServerStatus{ + Conditions: []metav1.Condition{{ + Type: mcpv1beta1.ConditionTypeExternalAuthConfigValidated, + Status: metav1.ConditionFalse, + Reason: mcpv1beta1.ConditionReasonEnterpriseRequired, + Message: "stale mirror — source has since been deleted", + }}, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithRuntimeObjects(mcpServer). + WithStatusSubresource(&mcpv1beta1.MCPServer{}). + Build() + + reconciler := newTestMCPServerReconciler(fakeClient, scheme, kubernetes.PlatformKubernetes) + + err := reconciler.handleExternalAuthConfig(ctx, mcpServer) + require.Error(t, err, "NotFound lookup must still surface an error") + assert.Contains(t, err.Error(), "not found", + "the pre-existing NotFound error contract is unchanged by the mirror-clear") + assert.Nil(t, + meta.FindStatusCondition(mcpServer.Status.Conditions, mcpv1beta1.ConditionTypeExternalAuthConfigValidated), + "stale mirror must be cleared when the referenced source is NotFound") +} From c9d0a70834714860352219852cf80c264c26888c Mon Sep 17 00:00:00 2001 From: Trey Date: Thu, 21 May 2026 08:34:47 -0700 Subject: [PATCH 5/5] Consolidate mirror helpers; tighten error+test contract Addresses stacklok/toolhive#5354 review comments: - MEDIUM cmd/thv-operator/controllers/virtualmcpserver_externalauth_test.go (F3): add a wrapped-error round-trip assertion so a future %w -> %v regression in buildOutgoingAuthConfig can't silently break per-backend reason propagation. - MEDIUM cmd/thv-operator/controllers/mcpserver_controller.go (F4): move both consumer mirror functions to free functions in external_auth_mirror.go (mirrorInvalidOnMCPServer / mirrorInvalidOnRemoteProxy) instead of one method with an unused receiver and one suffixed free function. Single canonical home. - LOW cmd/thv-operator/controllers/mcpserver_controller.go (F5): drop the "referenced MCPExternalAuthConfig is invalid" prefix from the typed error's Error(); the outer wrap on each consumer carries the namespaced name. Rendered messages now read "MCPExternalAuthConfig ns/name: invalid (Reason): Message". - LOW cmd/thv-operator/controllers/external_auth_mirror.go (F6): substitute a fallback reason when the source surfaces Valid=False with an empty Reason so the apiserver patch cannot fail validation on a corrupt/partial source status. Add a TestMirroredExternalAuthConfigInvalid subtest for the empty-Reason input. - LOW cmd/thv-operator/controllers/external_auth_mirror.go (F7): trim the probe doc; package-level comment carries the #5347 motivation once, the per-function docs are now contract-only. - LOW cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go (F8): add expectedCondMessage to the test table and assert it on the OBO mirror row. - LOW cmd/thv-operator/controllers/mcpserver_externalauth_test.go (F9): set non-zero Generation on the test fixtures and assert ObservedGeneration on the mirrored condition. Same gap covered on the MCPRemoteProxy row. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../controllers/external_auth_mirror.go | 113 +++++++++++++----- .../controllers/mcpremoteproxy_controller.go | 31 +---- .../mcpremoteproxy_controller_test.go | 45 ++++--- .../controllers/mcpserver_controller.go | 37 +----- .../mcpserver_externalauth_test.go | 12 +- .../virtualmcpserver_externalauth_test.go | 28 +++++ 6 files changed, 152 insertions(+), 114 deletions(-) diff --git a/cmd/thv-operator/controllers/external_auth_mirror.go b/cmd/thv-operator/controllers/external_auth_mirror.go index 2da2ecd074..7c8de7ae46 100644 --- a/cmd/thv-operator/controllers/external_auth_mirror.go +++ b/cmd/thv-operator/controllers/external_auth_mirror.go @@ -13,40 +13,37 @@ import ( mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1" ) +// Status Condition Parity machinery for #5347. The probe reads a referenced +// MCPExternalAuthConfig's Valid condition; the per-consumer mirror functions +// transcribe its reason+message onto the consumer CR's parallel condition. +// Without this, an obo-typed MCPExternalAuthConfig in an upstream-only build +// surfaces EnterpriseRequired only on the referenced resource and the consumer +// status reports only the generic dispatch failure. + +// fallbackInvalidReason is substituted when a source surfaces Valid=False with +// an empty Reason. metav1.Condition requires Reason to be non-empty and the +// apiserver rejects empty-Reason patches, so the mirror cannot copy an empty +// Reason verbatim without trapping the consumer in a noisy reconcile loop. +const fallbackInvalidReason = "InvalidExternalAuthConfig" + // mirroredInvalidExternalAuthConfigError signals that a referenced -// MCPExternalAuthConfig had Status.Conditions[Valid]=False and the source's -// reason+message should be mirrored onto the consumer's status. Callers may -// extract the reason via mirroredReasonFromError when they need to propagate -// it through layers that only carry a generic error (notably the -// VirtualMCPServer AuthConfigError aggregator). +// MCPExternalAuthConfig had Status.Conditions[Valid]=False. Carries the +// source's reason+message so callers can surface them on the consumer's +// status without re-fetching the object, and satisfies the error interface +// so it can travel through error-returning APIs (notably +// convertBackendAuthConfigToVMCP -> buildOutgoingAuthConfig). type mirroredInvalidExternalAuthConfigError struct { Reason string Message string } func (e *mirroredInvalidExternalAuthConfigError) Error() string { - return fmt.Sprintf("referenced MCPExternalAuthConfig is invalid (%s): %s", e.Reason, e.Message) + return fmt.Sprintf("invalid (%s): %s", e.Reason, e.Message) } -// mirroredExternalAuthConfigInvalid inspects a fetched MCPExternalAuthConfig -// and returns a non-nil *mirroredInvalidExternalAuthConfigError when its Valid -// condition is False, or nil when Valid is True/absent. The returned value -// implements error and carries the source's reason and message so callers can -// surface them on the consumer's status without re-fetching the object. -// -// Returning the concrete pointer (rather than an (error, string) tuple) keeps -// callers from needing errors.As to recover the reason/message fields when -// they have the value in hand. The error path still satisfies the standard -// error interface, so callers that propagate it through layers that only -// carry error (notably convertBackendAuthConfigToVMCP -> buildOutgoingAuthConfig) -// can recover the same pointer via mirroredReasonFromError below. -// -// Consumer reconcilers (MCPServer, MCPRemoteProxy, VirtualMCPServer) use this -// probe to mirror the source's Valid=False condition onto the consumer CR, -// closing the Status Condition Parity gap described in #5347. Without the -// mirror, an obo-typed MCPExternalAuthConfig in an upstream-only build would -// surface EnterpriseRequired only on the referenced resource, leaving the -// consumer's status to report only the generic dispatch failure. +// mirroredExternalAuthConfigInvalid returns a non-nil typed error when the +// source's Valid condition is False, or nil otherwise. Use +// [mirroredReasonFromError] to recover the reason from a wrapped chain. func mirroredExternalAuthConfigInvalid( externalAuthConfig *mcpv1beta1.MCPExternalAuthConfig, ) *mirroredInvalidExternalAuthConfigError { @@ -54,17 +51,18 @@ func mirroredExternalAuthConfigInvalid( if validCond == nil || validCond.Status != metav1.ConditionFalse { return nil } - return &mirroredInvalidExternalAuthConfigError{ - Reason: validCond.Reason, - Message: validCond.Message, + reason := validCond.Reason + if reason == "" { + reason = fallbackInvalidReason } + return &mirroredInvalidExternalAuthConfigError{Reason: reason, Message: validCond.Message} } // mirroredReasonFromError returns the mirrored source reason embedded in err // (via *mirroredInvalidExternalAuthConfigError) or "" if err does not carry -// one. Used by the VirtualMCPServer buildOutgoingAuthConfig pipeline to -// propagate the source's reason onto the per-backend AuthConfigError without -// re-fetching the MCPExternalAuthConfig. +// one. Walks the wrap chain via errors.As so it remains correct when callers +// wrap the typed error with fmt.Errorf("...: %w", err) before passing it on +// (notably buildOutgoingAuthConfig in the VirtualMCPServer pipeline). func mirroredReasonFromError(err error) string { var mirrored *mirroredInvalidExternalAuthConfigError if stderrors.As(err, &mirrored) { @@ -72,3 +70,56 @@ func mirroredReasonFromError(err error) string { } return "" } + +// mirrorInvalidOnMCPServer mirrors the source's Valid=False condition onto the +// MCPServer's ExternalAuthConfigValidated condition. When the source is healed +// (Valid=True or absent), it clears any stale mirror so the condition does not +// outlive its cause. Returns (true, err) when a False mirror was written so +// the caller can mark Phase=Failed; (false, nil) otherwise. +// +// See package-level Status Condition Parity comment for the #5347 motivation. +func mirrorInvalidOnMCPServer( + m *mcpv1beta1.MCPServer, + externalAuthConfig *mcpv1beta1.MCPExternalAuthConfig, +) (bool, error) { + mirrored := mirroredExternalAuthConfigInvalid(externalAuthConfig) + if mirrored == nil { + meta.RemoveStatusCondition(&m.Status.Conditions, mcpv1beta1.ConditionTypeExternalAuthConfigValidated) + return false, nil + } + meta.SetStatusCondition(&m.Status.Conditions, metav1.Condition{ + Type: mcpv1beta1.ConditionTypeExternalAuthConfigValidated, + Status: metav1.ConditionFalse, + Reason: mirrored.Reason, + Message: mirrored.Message, + ObservedGeneration: m.Generation, + }) + return true, fmt.Errorf("MCPExternalAuthConfig %s/%s: %w", m.Namespace, externalAuthConfig.Name, mirrored) +} + +// mirrorInvalidOnRemoteProxy mirrors the source's Valid=False condition onto +// the MCPRemoteProxy's ExternalAuthConfigValidated condition. When the source +// is healed, it clears any stale mirror defensively — the downstream True +// writer in handleExternalAuthConfig also sets the success reason, but a +// future early return between this site and that writer would otherwise leak +// a stale False. Returns (true, err) when a False mirror was written so the +// caller can short-circuit; (false, nil) otherwise. +func mirrorInvalidOnRemoteProxy( + proxy *mcpv1beta1.MCPRemoteProxy, + externalAuthConfig *mcpv1beta1.MCPExternalAuthConfig, +) (bool, error) { + mirrored := mirroredExternalAuthConfigInvalid(externalAuthConfig) + if mirrored == nil { + meta.RemoveStatusCondition( + &proxy.Status.Conditions, mcpv1beta1.ConditionTypeMCPRemoteProxyExternalAuthConfigValidated) + return false, nil + } + meta.SetStatusCondition(&proxy.Status.Conditions, metav1.Condition{ + Type: mcpv1beta1.ConditionTypeMCPRemoteProxyExternalAuthConfigValidated, + Status: metav1.ConditionFalse, + Reason: mirrored.Reason, + Message: mirrored.Message, + ObservedGeneration: proxy.Generation, + }) + return true, fmt.Errorf("MCPExternalAuthConfig %s/%s: %w", proxy.Namespace, externalAuthConfig.Name, mirrored) +} diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go index 613a35f71f..b9da1ea202 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go @@ -739,7 +739,7 @@ func (r *MCPRemoteProxyReconciler) handleExternalAuthConfig(ctx context.Context, // the MCPRemoteProxy so the failure is visible on the consumer CR (e.g. // obo-typed configs surface Valid=False/EnterpriseRequired here without the // user having to inspect the referenced MCPExternalAuthConfig). - if mirrored, err := mirrorExternalAuthConfigInvalidForRemoteProxy(proxy, externalAuthConfig); mirrored { + if mirrored, err := mirrorInvalidOnRemoteProxy(proxy, externalAuthConfig); mirrored { return err } @@ -785,35 +785,6 @@ func (r *MCPRemoteProxyReconciler) handleExternalAuthConfig(ctx context.Context, return nil } -// mirrorExternalAuthConfigInvalidForRemoteProxy inspects the referenced -// MCPExternalAuthConfig's Valid condition and, when it is False, mirrors the -// reason+message onto the MCPRemoteProxy's ExternalAuthConfigValidated -// condition. When the source is healed (Valid=True or absent), the helper -// also clears any stale mirror it previously wrote so the heal contract is -// robust to control-flow changes between this site and the downstream True -// writer at the end of handleExternalAuthConfig. Returns (true, err) when a -// mirror was written so callers can short-circuit; (false, nil) otherwise. -// See the equivalent helper for MCPServer for the propagation rationale. -func mirrorExternalAuthConfigInvalidForRemoteProxy( - proxy *mcpv1beta1.MCPRemoteProxy, - externalAuthConfig *mcpv1beta1.MCPExternalAuthConfig, -) (bool, error) { - mirrored := mirroredExternalAuthConfigInvalid(externalAuthConfig) - if mirrored == nil { - meta.RemoveStatusCondition( - &proxy.Status.Conditions, mcpv1beta1.ConditionTypeMCPRemoteProxyExternalAuthConfigValidated) - return false, nil - } - meta.SetStatusCondition(&proxy.Status.Conditions, metav1.Condition{ - Type: mcpv1beta1.ConditionTypeMCPRemoteProxyExternalAuthConfigValidated, - Status: metav1.ConditionFalse, - Reason: mirrored.Reason, - Message: mirrored.Message, - ObservedGeneration: proxy.Generation, - }) - return true, fmt.Errorf("MCPExternalAuthConfig %s/%s: %w", proxy.Namespace, externalAuthConfig.Name, mirrored) -} - // handleAuthServerRef validates and tracks the hash of the referenced authServerRef config. // It updates the MCPRemoteProxy status when the auth server configuration changes and sets // the AuthServerRefValidated condition. diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go b/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go index 6f23372533..90cb758a48 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go @@ -499,15 +499,16 @@ func TestHandleExternalAuthConfig(t *testing.T) { t.Parallel() tests := []struct { - name string - proxy *mcpv1beta1.MCPRemoteProxy - externalAuth *mcpv1beta1.MCPExternalAuthConfig - interceptorFuncs *interceptor.Funcs - expectError bool - errContains string - expectCondition bool - expectedCondStatus metav1.ConditionStatus - expectedCondReason string + name string + proxy *mcpv1beta1.MCPRemoteProxy + externalAuth *mcpv1beta1.MCPExternalAuthConfig + interceptorFuncs *interceptor.Funcs + expectError bool + errContains string + expectCondition bool + expectedCondStatus metav1.ConditionStatus + expectedCondReason string + expectedCondMessage string // when set, asserts the condition's Message verbatim }{ { name: "no external auth reference", @@ -681,8 +682,9 @@ func TestHandleExternalAuthConfig(t *testing.T) { name: "referenced config Valid=False is mirrored onto the proxy", proxy: &mcpv1beta1.MCPRemoteProxy{ ObjectMeta: metav1.ObjectMeta{ - Name: "obo-mirror-proxy", - Namespace: "default", + Name: "obo-mirror-proxy", + Namespace: "default", + Generation: 7, }, Spec: mcpv1beta1.MCPRemoteProxySpec{ RemoteURL: "https://mcp.example.com", @@ -708,11 +710,12 @@ func TestHandleExternalAuthConfig(t *testing.T) { }}, }, }, - expectError: true, - errContains: "EnterpriseRequired", - expectCondition: true, - expectedCondStatus: metav1.ConditionFalse, - expectedCondReason: mcpv1beta1.ConditionReasonEnterpriseRequired, + expectError: true, + errContains: "EnterpriseRequired", + expectCondition: true, + expectedCondStatus: metav1.ConditionFalse, + expectedCondReason: mcpv1beta1.ConditionReasonEnterpriseRequired, + expectedCondMessage: "on-behalf-of (OBO) external auth type requires an enterprise build", }, { name: "embedded auth server with multiple upstreams rejected", @@ -795,6 +798,16 @@ func TestHandleExternalAuthConfig(t *testing.T) { "Condition status should match expected") assert.Equal(t, tt.expectedCondReason, cond.Reason, "Condition reason should match expected") + if tt.expectedCondMessage != "" { + assert.Equal(t, tt.expectedCondMessage, cond.Message, + "Condition message should match expected") + } + // F9: when the test fixture sets a non-zero Generation, + // the mirror must stamp ObservedGeneration with it. + if tt.proxy.Generation != 0 { + assert.Equal(t, tt.proxy.Generation, cond.ObservedGeneration, + "Condition.ObservedGeneration must match proxy.Generation") + } } } } else { diff --git a/cmd/thv-operator/controllers/mcpserver_controller.go b/cmd/thv-operator/controllers/mcpserver_controller.go index e70fedd9fb..c78126d7a6 100644 --- a/cmd/thv-operator/controllers/mcpserver_controller.go +++ b/cmd/thv-operator/controllers/mcpserver_controller.go @@ -2048,7 +2048,7 @@ func (r *MCPServerReconciler) handleExternalAuthConfig(ctx context.Context, m *m // the MCPServer so the failure is visible on the consumer CR (e.g. obo-typed // configs surface Valid=False/EnterpriseRequired here without the user // having to inspect the referenced MCPExternalAuthConfig). - if mirrored, err := r.mirrorExternalAuthConfigInvalid(m, externalAuthConfig); mirrored { + if mirrored, err := mirrorInvalidOnMCPServer(m, externalAuthConfig); mirrored { return err } @@ -2092,41 +2092,6 @@ func (r *MCPServerReconciler) handleExternalAuthConfig(ctx context.Context, m *m return nil } -// mirrorExternalAuthConfigInvalid inspects the referenced MCPExternalAuthConfig's -// Valid condition and, when it is False, mirrors the reason+message onto the -// MCPServer's ExternalAuthConfigValidated condition. When the source is -// Valid=True (i.e. has healed since a previous False), the helper clears any -// stale mirror it previously wrote so the condition doesn't outlive the cause. -// Returns (true, err) when a mirror was written so callers can short-circuit; -// (false, nil) otherwise. -// -// This propagates failures like Reason=EnterpriseRequired (set by the -// MCPExternalAuthConfig reconciler for obo-typed configs in upstream-only -// builds) onto the consumer CR so users see the failure on the resource they -// applied, not buried in the referenced config's status. -func (*MCPServerReconciler) mirrorExternalAuthConfigInvalid( - m *mcpv1beta1.MCPServer, - externalAuthConfig *mcpv1beta1.MCPExternalAuthConfig, -) (bool, error) { - mirrored := mirroredExternalAuthConfigInvalid(externalAuthConfig) - if mirrored == nil { - // Source healed (Valid=True or absent). Clear any stale mirror so it - // doesn't persist after the cause is gone. Downstream branches in - // handleExternalAuthConfig (e.g. multi-upstream) re-set False if they - // still apply on this reconcile. - meta.RemoveStatusCondition(&m.Status.Conditions, mcpv1beta1.ConditionTypeExternalAuthConfigValidated) - return false, nil - } - meta.SetStatusCondition(&m.Status.Conditions, metav1.Condition{ - Type: mcpv1beta1.ConditionTypeExternalAuthConfigValidated, - Status: metav1.ConditionFalse, - Reason: mirrored.Reason, - Message: mirrored.Message, - ObservedGeneration: m.Generation, - }) - return true, fmt.Errorf("MCPExternalAuthConfig %s/%s: %w", m.Namespace, externalAuthConfig.Name, mirrored) -} - // handleAuthServerRef validates and tracks the hash of the referenced authServerRef config. // It updates the MCPServer status when the auth server configuration changes and sets // the AuthServerRefValidated condition. diff --git a/cmd/thv-operator/controllers/mcpserver_externalauth_test.go b/cmd/thv-operator/controllers/mcpserver_externalauth_test.go index 7817ae880c..f059b73452 100644 --- a/cmd/thv-operator/controllers/mcpserver_externalauth_test.go +++ b/cmd/thv-operator/controllers/mcpserver_externalauth_test.go @@ -573,8 +573,13 @@ func TestMCPServerReconciler_handleExternalAuthConfig_MirrorsInvalidCondition(t externalAuthConfig.Status.Conditions = []metav1.Condition{*tt.sourceValid} } + const serverGeneration int64 = 11 mcpServer := &mcpv1beta1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{Name: "test-server", Namespace: namespace}, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-server", + Namespace: namespace, + Generation: serverGeneration, + }, Spec: mcpv1beta1.MCPServerSpec{ Image: "test-image", ExternalAuthConfigRef: &mcpv1beta1.ExternalAuthConfigRef{Name: authName}, @@ -610,6 +615,11 @@ func TestMCPServerReconciler_handleExternalAuthConfig_MirrorsInvalidCondition(t assert.Equal(t, metav1.ConditionFalse, cond.Status) assert.Equal(t, tt.wantReason, cond.Reason) assert.Equal(t, tt.wantMessage, cond.Message) + // F9: the mirror must stamp ObservedGeneration with the consumer's + // Generation (not the source's), so the condition reflects the + // generation of the spec it was computed for. + assert.Equal(t, serverGeneration, cond.ObservedGeneration, + "Condition.ObservedGeneration must match MCPServer.Generation") }) } } diff --git a/cmd/thv-operator/controllers/virtualmcpserver_externalauth_test.go b/cmd/thv-operator/controllers/virtualmcpserver_externalauth_test.go index 96eec1fc0c..96551c73df 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_externalauth_test.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_externalauth_test.go @@ -5,6 +5,7 @@ package controllers import ( "context" + "fmt" "regexp" "strings" "testing" @@ -898,6 +899,16 @@ func TestConvertBackendAuthConfigToVMCP_MirrorsInvalidExternalAuthConfig(t *test require.Nil(t, strategy) assert.Equal(t, mcpv1beta1.ConditionReasonEnterpriseRequired, mirroredReasonFromError(err), "buildOutgoingAuthConfig depends on this reason flowing through mirroredReasonFromError") + + // Wrap the error exactly as buildOutgoingAuthConfig does in production + // (fmt.Errorf("...: %w", err)) and assert the reason still survives the + // errors.As walk. A future refactor that drops %w in favour of %v or + // errors.New(fmt.Sprintf(...)) would silently break per-backend reason + // propagation and surface ConversionFailed instead of EnterpriseRequired. + wrapped := fmt.Errorf("failed to convert backend auth config: %w", err) + assert.Equal(t, mcpv1beta1.ConditionReasonEnterpriseRequired, mirroredReasonFromError(wrapped), + "buildOutgoingAuthConfig wraps the err once before extracting the reason; "+ + "the contract must survive that wrap") } // TestMirroredExternalAuthConfigInvalid verifies the source-condition probe @@ -938,6 +949,23 @@ func TestMirroredExternalAuthConfigInvalid(t *testing.T) { name: "no Valid condition returns nil", conditions: nil, }, + { + // F6 defense-in-depth: metav1.Condition requires Reason to be + // non-empty (apiserver rejects empty-Reason patches). If a source + // ever surfaces Valid=False with an empty Reason (corrupt status + // or a bug in the source reconciler), the mirror must substitute + // a fallback rather than copy the empty string through and trap + // the consumer in a noisy reconcile loop. + name: "Valid=False with empty Reason gets a fallback reason", + conditions: []metav1.Condition{{ + Type: mcpv1beta1.ConditionTypeValid, + Status: metav1.ConditionFalse, + Reason: "", + Message: "source surfaced an empty Reason", + }}, + wantReason: fallbackInvalidReason, + wantMessage: "source surfaced an empty Reason", + }, } for _, tt := range tests {