Skip to content

Commit e37e336

Browse files
tgrunnagleclaude
tgrunnagle
and
claude
authored
Mirror MCPExternalAuthConfig Valid=False onto consumer CR conditions (#5354)
* 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) <noreply@anthropic.com> * Address code review feedback on consumer-CR mirror 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * Clear stale consumer-CR mirror across all heal paths Addresses #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) <noreply@anthropic.com> * Consolidate mirror helpers; tighten error+test contract Addresses #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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 686eb1b commit e37e336

8 files changed

Lines changed: 774 additions & 32 deletions

‎cmd/thv-operator/controllers/external_auth_mirror.go‎

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
package controllers
5+
6+
import (
7+
stderrors "errors"
8+
"fmt"
9+
10+
"k8s.io/apimachinery/pkg/api/meta"
11+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
12+
13+
mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1"
14+
)
15+
16+
// Status Condition Parity machinery for #5347. The probe reads a referenced
17+
// MCPExternalAuthConfig's Valid condition; the per-consumer mirror functions
18+
// transcribe its reason+message onto the consumer CR's parallel condition.
19+
// Without this, an obo-typed MCPExternalAuthConfig in an upstream-only build
20+
// surfaces EnterpriseRequired only on the referenced resource and the consumer
21+
// status reports only the generic dispatch failure.
22+
23+
// fallbackInvalidReason is substituted when a source surfaces Valid=False with
24+
// an empty Reason. metav1.Condition requires Reason to be non-empty and the
25+
// apiserver rejects empty-Reason patches, so the mirror cannot copy an empty
26+
// Reason verbatim without trapping the consumer in a noisy reconcile loop.
27+
const fallbackInvalidReason = "InvalidExternalAuthConfig"
28+
29+
// mirroredInvalidExternalAuthConfigError signals that a referenced
30+
// MCPExternalAuthConfig had Status.Conditions[Valid]=False. Carries the
31+
// source's reason+message so callers can surface them on the consumer's
32+
// status without re-fetching the object, and satisfies the error interface
33+
// so it can travel through error-returning APIs (notably
34+
// convertBackendAuthConfigToVMCP -> buildOutgoingAuthConfig).
35+
type mirroredInvalidExternalAuthConfigError struct {
36+
Reason string
37+
Message string
38+
}
39+
40+
func (e *mirroredInvalidExternalAuthConfigError) Error() string {
41+
return fmt.Sprintf("invalid (%s): %s", e.Reason, e.Message)
42+
}
43+
44+
// mirroredExternalAuthConfigInvalid returns a non-nil typed error when the
45+
// source's Valid condition is False, or nil otherwise. Use
46+
// [mirroredReasonFromError] to recover the reason from a wrapped chain.
47+
func mirroredExternalAuthConfigInvalid(
48+
externalAuthConfig *mcpv1beta1.MCPExternalAuthConfig,
49+
) *mirroredInvalidExternalAuthConfigError {
50+
validCond := meta.FindStatusCondition(externalAuthConfig.Status.Conditions, mcpv1beta1.ConditionTypeValid)
51+
if validCond == nil || validCond.Status != metav1.ConditionFalse {
52+
return nil
53+
}
54+
reason := validCond.Reason
55+
if reason == "" {
56+
reason = fallbackInvalidReason
57+
}
58+
return &mirroredInvalidExternalAuthConfigError{Reason: reason, Message: validCond.Message}
59+
}
60+
61+
// mirroredReasonFromError returns the mirrored source reason embedded in err
62+
// (via *mirroredInvalidExternalAuthConfigError) or "" if err does not carry
63+
// one. Walks the wrap chain via errors.As so it remains correct when callers
64+
// wrap the typed error with fmt.Errorf("...: %w", err) before passing it on
65+
// (notably buildOutgoingAuthConfig in the VirtualMCPServer pipeline).
66+
func mirroredReasonFromError(err error) string {
67+
var mirrored *mirroredInvalidExternalAuthConfigError
68+
if stderrors.As(err, &mirrored) {
69+
return mirrored.Reason
70+
}
71+
return ""
72+
}
73+
74+
// mirrorInvalidOnMCPServer mirrors the source's Valid=False condition onto the
75+
// MCPServer's ExternalAuthConfigValidated condition. When the source is healed
76+
// (Valid=True or absent), it clears any stale mirror so the condition does not
77+
// outlive its cause. Returns (true, err) when a False mirror was written so
78+
// the caller can mark Phase=Failed; (false, nil) otherwise.
79+
//
80+
// See package-level Status Condition Parity comment for the #5347 motivation.
81+
func mirrorInvalidOnMCPServer(
82+
m *mcpv1beta1.MCPServer,
83+
externalAuthConfig *mcpv1beta1.MCPExternalAuthConfig,
84+
) (bool, error) {
85+
mirrored := mirroredExternalAuthConfigInvalid(externalAuthConfig)
86+
if mirrored == nil {
87+
meta.RemoveStatusCondition(&m.Status.Conditions, mcpv1beta1.ConditionTypeExternalAuthConfigValidated)
88+
return false, nil
89+
}
90+
meta.SetStatusCondition(&m.Status.Conditions, metav1.Condition{
91+
Type: mcpv1beta1.ConditionTypeExternalAuthConfigValidated,
92+
Status: metav1.ConditionFalse,
93+
Reason: mirrored.Reason,
94+
Message: mirrored.Message,
95+
ObservedGeneration: m.Generation,
96+
})
97+
return true, fmt.Errorf("MCPExternalAuthConfig %s/%s: %w", m.Namespace, externalAuthConfig.Name, mirrored)
98+
}
99+
100+
// mirrorInvalidOnRemoteProxy mirrors the source's Valid=False condition onto
101+
// the MCPRemoteProxy's ExternalAuthConfigValidated condition. When the source
102+
// is healed, it clears any stale mirror defensively — the downstream True
103+
// writer in handleExternalAuthConfig also sets the success reason, but a
104+
// future early return between this site and that writer would otherwise leak
105+
// a stale False. Returns (true, err) when a False mirror was written so the
106+
// caller can short-circuit; (false, nil) otherwise.
107+
func mirrorInvalidOnRemoteProxy(
108+
proxy *mcpv1beta1.MCPRemoteProxy,
109+
externalAuthConfig *mcpv1beta1.MCPExternalAuthConfig,
110+
) (bool, error) {
111+
mirrored := mirroredExternalAuthConfigInvalid(externalAuthConfig)
112+
if mirrored == nil {
113+
meta.RemoveStatusCondition(
114+
&proxy.Status.Conditions, mcpv1beta1.ConditionTypeMCPRemoteProxyExternalAuthConfigValidated)
115+
return false, nil
116+
}
117+
meta.SetStatusCondition(&proxy.Status.Conditions, metav1.Condition{
118+
Type: mcpv1beta1.ConditionTypeMCPRemoteProxyExternalAuthConfigValidated,
119+
Status: metav1.ConditionFalse,
120+
Reason: mirrored.Reason,
121+
Message: mirrored.Message,
122+
ObservedGeneration: proxy.Generation,
123+
})
124+
return true, fmt.Errorf("MCPExternalAuthConfig %s/%s: %w", proxy.Namespace, externalAuthConfig.Name, mirrored)
125+
}

‎cmd/thv-operator/controllers/mcpremoteproxy_controller.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -735,6 +735,14 @@ func (r *MCPRemoteProxyReconciler) handleExternalAuthConfig(ctx context.Context,
735735
return fmt.Errorf("failed to fetch MCPExternalAuthConfig: %w", err)
736736
}
737737

738+
// Mirror the referenced MCPExternalAuthConfig's Valid=False condition onto
739+
// the MCPRemoteProxy so the failure is visible on the consumer CR (e.g.
740+
// obo-typed configs surface Valid=False/EnterpriseRequired here without the
741+
// user having to inspect the referenced MCPExternalAuthConfig).
742+
if mirrored, err := mirrorInvalidOnRemoteProxy(proxy, externalAuthConfig); mirrored {
743+
return err
744+
}
745+
738746
// MCPRemoteProxy supports only single-upstream embedded auth server configs.
739747
// Multi-upstream requires VirtualMCPServer.
740748
if embeddedCfg := externalAuthConfig.Spec.EmbeddedAuthServer; embeddedCfg != nil && len(embeddedCfg.UpstreamProviders) > 1 {

‎cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go‎

Lines changed: 65 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -499,15 +499,16 @@ func TestHandleExternalAuthConfig(t *testing.T) {
499499
t.Parallel()
500500

501501
tests := []struct {
502-
name string
503-
proxy *mcpv1beta1.MCPRemoteProxy
504-
externalAuth *mcpv1beta1.MCPExternalAuthConfig
505-
interceptorFuncs *interceptor.Funcs
506-
expectError bool
507-
errContains string
508-
expectCondition bool
509-
expectedCondStatus metav1.ConditionStatus
510-
expectedCondReason string
502+
name string
503+
proxy *mcpv1beta1.MCPRemoteProxy
504+
externalAuth *mcpv1beta1.MCPExternalAuthConfig
505+
interceptorFuncs *interceptor.Funcs
506+
expectError bool
507+
errContains string
508+
expectCondition bool
509+
expectedCondStatus metav1.ConditionStatus
510+
expectedCondReason string
511+
expectedCondMessage string // when set, asserts the condition's Message verbatim
511512
}{
512513
{
513514
name: "no external auth reference",
@@ -671,6 +672,51 @@ func TestHandleExternalAuthConfig(t *testing.T) {
671672
expectedCondStatus: metav1.ConditionFalse,
672673
expectedCondReason: mcpv1beta1.ConditionReasonMCPRemoteProxyExternalAuthConfigFetchError,
673674
},
675+
{
676+
// Mirror added for #5347: when the referenced MCPExternalAuthConfig
677+
// has Status.Conditions[Valid]=False (e.g. obo-typed configs that
678+
// the default OBO handler rejected with Reason=EnterpriseRequired
679+
// in upstream-only builds), the proxy reconciler must surface a
680+
// parallel ExternalAuthConfigValidated=False with the same reason
681+
// and message.
682+
name: "referenced config Valid=False is mirrored onto the proxy",
683+
proxy: &mcpv1beta1.MCPRemoteProxy{
684+
ObjectMeta: metav1.ObjectMeta{
685+
Name: "obo-mirror-proxy",
686+
Namespace: "default",
687+
Generation: 7,
688+
},
689+
Spec: mcpv1beta1.MCPRemoteProxySpec{
690+
RemoteURL: "https://mcp.example.com",
691+
ExternalAuthConfigRef: &mcpv1beta1.ExternalAuthConfigRef{
692+
Name: "obo-config",
693+
},
694+
},
695+
},
696+
externalAuth: &mcpv1beta1.MCPExternalAuthConfig{
697+
ObjectMeta: metav1.ObjectMeta{
698+
Name: "obo-config",
699+
Namespace: "default",
700+
},
701+
Spec: mcpv1beta1.MCPExternalAuthConfigSpec{
702+
Type: mcpv1beta1.ExternalAuthTypeOBO,
703+
},
704+
Status: mcpv1beta1.MCPExternalAuthConfigStatus{
705+
Conditions: []metav1.Condition{{
706+
Type: mcpv1beta1.ConditionTypeValid,
707+
Status: metav1.ConditionFalse,
708+
Reason: mcpv1beta1.ConditionReasonEnterpriseRequired,
709+
Message: "on-behalf-of (OBO) external auth type requires an enterprise build",
710+
}},
711+
},
712+
},
713+
expectError: true,
714+
errContains: "EnterpriseRequired",
715+
expectCondition: true,
716+
expectedCondStatus: metav1.ConditionFalse,
717+
expectedCondReason: mcpv1beta1.ConditionReasonEnterpriseRequired,
718+
expectedCondMessage: "on-behalf-of (OBO) external auth type requires an enterprise build",
719+
},
674720
{
675721
name: "embedded auth server with multiple upstreams rejected",
676722
proxy: &mcpv1beta1.MCPRemoteProxy{
@@ -752,6 +798,16 @@ func TestHandleExternalAuthConfig(t *testing.T) {
752798
"Condition status should match expected")
753799
assert.Equal(t, tt.expectedCondReason, cond.Reason,
754800
"Condition reason should match expected")
801+
if tt.expectedCondMessage != "" {
802+
assert.Equal(t, tt.expectedCondMessage, cond.Message,
803+
"Condition message should match expected")
804+
}
805+
// F9: when the test fixture sets a non-zero Generation,
806+
// the mirror must stamp ObservedGeneration with it.
807+
if tt.proxy.Generation != 0 {
808+
assert.Equal(t, tt.proxy.Generation, cond.ObservedGeneration,
809+
"Condition.ObservedGeneration must match proxy.Generation")
810+
}
755811
}
756812
}
757813
} else {

‎cmd/thv-operator/controllers/mcpserver_controller.go‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2016,7 +2016,9 @@ func getToolhiveRunnerImage() string {
20162016
func (r *MCPServerReconciler) handleExternalAuthConfig(ctx context.Context, m *mcpv1beta1.MCPServer) error {
20172017
ctxLogger := log.FromContext(ctx)
20182018
if m.Spec.ExternalAuthConfigRef == nil {
2019-
// No MCPExternalAuthConfig referenced, clear any stored hash
2019+
// No MCPExternalAuthConfig referenced. Clear any stale mirror written
2020+
// while the ref was set so the condition doesn't outlive its cause.
2021+
meta.RemoveStatusCondition(&m.Status.Conditions, mcpv1beta1.ConditionTypeExternalAuthConfigValidated)
20202022
if m.Status.ExternalAuthConfigHash != "" {
20212023
m.Status.ExternalAuthConfigHash = ""
20222024
if err := r.Status().Update(ctx, m); err != nil {
@@ -2029,13 +2031,27 @@ func (r *MCPServerReconciler) handleExternalAuthConfig(ctx context.Context, m *m
20292031
// Get the referenced MCPExternalAuthConfig
20302032
externalAuthConfig, err := GetExternalAuthConfigForMCPServer(ctx, r.Client, m)
20312033
if err != nil {
2034+
// Source lookup failed (e.g. NotFound). Clear any stale mirror — the
2035+
// referenced source no longer exists, so the previous mirror is no
2036+
// longer load-bearing. Pre-existing behavior surfaces the lookup
2037+
// error through Phase=Failed at the caller.
2038+
meta.RemoveStatusCondition(&m.Status.Conditions, mcpv1beta1.ConditionTypeExternalAuthConfigValidated)
20322039
return err
20332040
}
20342041

20352042
if externalAuthConfig == nil {
2043+
meta.RemoveStatusCondition(&m.Status.Conditions, mcpv1beta1.ConditionTypeExternalAuthConfigValidated)
20362044
return fmt.Errorf("MCPExternalAuthConfig %s not found", m.Spec.ExternalAuthConfigRef.Name)
20372045
}
20382046

2047+
// Mirror the referenced MCPExternalAuthConfig's Valid=False condition onto
2048+
// the MCPServer so the failure is visible on the consumer CR (e.g. obo-typed
2049+
// configs surface Valid=False/EnterpriseRequired here without the user
2050+
// having to inspect the referenced MCPExternalAuthConfig).
2051+
if mirrored, err := mirrorInvalidOnMCPServer(m, externalAuthConfig); mirrored {
2052+
return err
2053+
}
2054+
20392055
// MCPServer supports only single-upstream embedded auth server configs.
20402056
// Multi-upstream requires VirtualMCPServer.
20412057
if embeddedCfg := externalAuthConfig.Spec.EmbeddedAuthServer; embeddedCfg != nil && len(embeddedCfg.UpstreamProviders) > 1 {

0 commit comments

Comments
 (0)