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..7c8de7ae46 --- /dev/null +++ b/cmd/thv-operator/controllers/external_auth_mirror.go @@ -0,0 +1,125 @@ +// 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" +) + +// 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. 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("invalid (%s): %s", e.Reason, e.Message) +} + +// 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 { + validCond := meta.FindStatusCondition(externalAuthConfig.Status.Conditions, mcpv1beta1.ConditionTypeValid) + if validCond == nil || validCond.Status != metav1.ConditionFalse { + return nil + } + 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. 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) { + return mirrored.Reason + } + 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 b832fa17ee..b9da1ea202 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 := mirrorInvalidOnRemoteProxy(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 { diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go b/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go index 5a03e5c397..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", @@ -671,6 +672,51 @@ 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", + Generation: 7, + }, + 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, + expectedCondMessage: "on-behalf-of (OBO) external auth type requires an enterprise build", + }, { name: "embedded auth server with multiple upstreams rejected", proxy: &mcpv1beta1.MCPRemoteProxy{ @@ -752,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 eec776c05e..c78126d7a6 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,13 +2031,27 @@ 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) } + // 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 := mirrorInvalidOnMCPServer(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 { diff --git a/cmd/thv-operator/controllers/mcpserver_externalauth_test.go b/cmd/thv-operator/controllers/mcpserver_externalauth_test.go index 9fa8e0ffc3..f059b73452 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,254 @@ 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 + preexisting []metav1.Condition + wantMirrored bool + wantReason string + wantMessage string + wantPreexistingCleared bool + }{ + { + 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, + }, + { + // 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 { + 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} + } + + const serverGeneration int64 = 11 + mcpServer := &mcpv1beta1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-server", + Namespace: namespace, + Generation: serverGeneration, + }, + Spec: mcpv1beta1.MCPServerSpec{ + Image: "test-image", + ExternalAuthConfigRef: &mcpv1beta1.ExternalAuthConfigRef{Name: authName}, + }, + Status: mcpv1beta1.MCPServerStatus{Conditions: tt.preexisting}, + } + + 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") + 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 + } + + 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) + // 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") + }) + } +} + +// 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") +} diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller.go b/cmd/thv-operator/controllers/virtualmcpserver_controller.go index 0307f14218..04e66f9a3d 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,16 @@ func (e *SpecValidationError) Error() string { return e.Message } +// 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 +2152,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 mirrored := mirroredExternalAuthConfigInvalid(externalAuthConfig); mirrored != nil { + return nil, mirrored + } + // Convert the external auth config to strategy return r.convertExternalAuthConfigToStrategy(externalAuthConfig) } @@ -2252,6 +2273,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 mirrored := mirroredExternalAuthConfigInvalid(externalAuthConfig); mirrored != nil { + authErrors = append(authErrors, AuthConfigError{ + Context: fmt.Sprintf("%s%s", authContextDiscoveredPrefix, workloadInfo.Name), + BackendName: workloadInfo.Name, + Error: mirrored, + Reason: mirrored.Reason, + }) + continue + } + // Convert MCPExternalAuthConfig to BackendAuthStrategy strategy, err := r.convertExternalAuthConfigToStrategy(externalAuthConfig) if err != nil { @@ -2349,6 +2383,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 +2409,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 +3165,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 +3229,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 +3251,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..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" @@ -850,6 +851,180 @@ 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") + + // 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 +// 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 + wantMessage string + }{ + { + 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, + wantMessage: "obo enterprise required", + }, + { + name: "Valid=True returns nil", + conditions: []metav1.Condition{{ + Type: mcpv1beta1.ConditionTypeValid, + Status: metav1.ConditionTrue, + Reason: "ValidationSucceeded", + }}, + }, + { + 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 { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + cfg := &mcpv1beta1.MCPExternalAuthConfig{ + Status: mcpv1beta1.MCPExternalAuthConfigStatus{Conditions: tt.conditions}, + } + 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)) + }) + } +} + +// 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 {