Skip to content

Commit b5d52b6

Browse files
authored
Preserve config-level outgoing auth across backend watcher reconciles (#6513)
The Kubernetes backend watcher rebuilt each backend from its resource references alone, so outgoingAuth.default and outgoingAuth.backends entries applied at startup were silently dropped on the first reconcile. The resolution logic moves to config.OutgoingAuthConfig.ApplyToBackend and the reconciler now applies it before every upsert, mirroring startup discovery exactly. NewBackendWatcher accepts the outgoing auth configuration and NewKubernetesBackendRegistry exposes it through the WithOutgoingAuth option.
1 parent 30e832a commit b5d52b6

8 files changed

Lines changed: 254 additions & 63 deletions

File tree

pkg/vmcp/aggregator/discoverer.go

Lines changed: 4 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -220,49 +220,11 @@ func (d *backendDiscoverer) Discover(ctx context.Context, groupRef string) (back
220220
// applyAuthConfigToBackend applies authentication configuration to a backend based on the source mode.
221221
// It determines whether to use discovered auth from the MCPServer or auth from the vMCP config.
222222
//
223-
// Auth resolution logic:
224-
// - "discovered" mode: Use discovered auth if available, otherwise fall back to Default or backend-specific config
225-
// - "inline" mode (or ""): Always use config-based auth, ignore discovered auth
226-
// - unknown mode: Default to config-based auth for safety
227-
//
228-
// When useDiscoveredAuth is false, ResolveForBackend is called which handles:
229-
// 1. Backend-specific config (d.authConfig.Backends[backendName])
230-
// 2. Default config fallback (d.authConfig.Default)
231-
// 3. No auth if neither is configured
223+
// The resolution logic lives in config.OutgoingAuthConfig.ApplyToBackend so the
224+
// Kubernetes backend watcher's reconciler applies the exact same semantics when it
225+
// rebuilds a backend (see pkg/vmcp/k8s).
232226
func (d *backendDiscoverer) applyAuthConfigToBackend(backend *vmcp.Backend, backendName string) {
233-
if d.authConfig == nil {
234-
return
235-
}
236-
237-
// Determine if we should use discovered auth or config-based auth
238-
var useDiscoveredAuth bool
239-
switch d.authConfig.Source {
240-
case "discovered":
241-
// In discovered mode, use auth discovered from MCPServer (if any exists)
242-
// If no auth is discovered, fall back to config-based auth via ResolveForBackend
243-
// which will use backend-specific config, then Default, then no auth
244-
useDiscoveredAuth = backend.AuthConfig != nil
245-
case "inline", "":
246-
// For inline mode or empty source, always use config-based auth
247-
// Ignore any discovered auth from backends
248-
useDiscoveredAuth = false
249-
default:
250-
// Unknown source mode - default to config-based auth for safety
251-
slog.Warn("unknown auth source mode, defaulting to config-based auth", "source", d.authConfig.Source)
252-
useDiscoveredAuth = false
253-
}
254-
255-
if useDiscoveredAuth {
256-
// Keep the auth discovered from MCPServer (already populated in backend)
257-
slog.Debug("backend using discovered auth strategy", "backend", backendName, "strategy", backend.AuthConfig.Type)
258-
} else {
259-
// Use auth from config (inline mode)
260-
authConfig := d.authConfig.ResolveForBackend(backendName)
261-
if authConfig != nil {
262-
backend.AuthConfig = authConfig
263-
slog.Debug("backend configured with auth strategy from config", "backend", backendName, "strategy", authConfig.Type)
264-
}
265-
}
227+
d.authConfig.ApplyToBackend(backend, backendName)
266228
}
267229

268230
// discoverFromStaticConfig converts pre-configured static backends into vmcp.Backend objects

pkg/vmcp/backendregistry/registry.go

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,13 +28,15 @@ import (
2828
"k8s.io/client-go/rest"
2929

3030
"github.com/stacklok/toolhive/pkg/vmcp"
31+
"github.com/stacklok/toolhive/pkg/vmcp/config"
3132
"github.com/stacklok/toolhive/pkg/vmcp/k8s"
3233
"github.com/stacklok/toolhive/pkg/vmcp/server"
3334
)
3435

3536
// options holds the optional settings for NewKubernetesBackendRegistry.
3637
type options struct {
37-
restConfig *rest.Config
38+
restConfig *rest.Config
39+
outgoingAuth *config.OutgoingAuthConfig
3840
}
3941

4042
// Option configures NewKubernetesBackendRegistry.
@@ -51,6 +53,17 @@ func WithRESTConfig(cfg *rest.Config) Option {
5153
}
5254
}
5355

56+
// WithOutgoingAuth supplies the vMCP config-level outgoing auth configuration.
57+
// The watcher applies it to every reconciled backend with the same precedence
58+
// startup discovery uses (discovered CR-side auth first, then backends[<name>],
59+
// then Default). The default (no option) is nil: reconciled backends keep only
60+
// the auth discovered from their own resource references.
61+
func WithOutgoingAuth(authConfig *config.OutgoingAuthConfig) Option {
62+
return func(o *options) {
63+
o.outgoingAuth = authConfig
64+
}
65+
}
66+
5467
// NewKubernetesBackendRegistry builds a live, Kubernetes-populated backend
5568
// registry for an embedder, hiding the pkg/vmcp/k8s watch substrate.
5669
//
@@ -127,7 +140,7 @@ func NewKubernetesBackendRegistry(
127140
// Start empty; the watcher's initial informer sync populates the registry.
128141
dynamicRegistry := vmcp.NewDynamicRegistry(nil)
129142

130-
watcher, err := k8s.NewBackendWatcher(restConfig, namespace, group, dynamicRegistry)
143+
watcher, err := k8s.NewBackendWatcher(restConfig, namespace, group, dynamicRegistry, o.outgoingAuth)
131144
if err != nil {
132145
return nil, nil, fmt.Errorf("failed to create backend watcher: %w", err)
133146
}

pkg/vmcp/cli/serve.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,7 @@ func Serve(ctx context.Context, cfg ServeConfig) error {
230230
return fmt.Errorf("VMCP_NAMESPACE environment variable not set")
231231
}
232232

233-
backendWatcher, err = k8s.NewBackendWatcher(restConfig, namespace, vmcpCfg.Group, dynamicRegistry)
233+
backendWatcher, err = k8s.NewBackendWatcher(restConfig, namespace, vmcpCfg.Group, dynamicRegistry, vmcpCfg.OutgoingAuth)
234234
if err != nil {
235235
return fmt.Errorf("failed to create backend watcher: %w", err)
236236
}

pkg/vmcp/config/config.go

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ package config
1111
import (
1212
"encoding/json"
1313
"fmt"
14+
"log/slog"
1415
"time"
1516

1617
"github.com/stacklok/toolhive/pkg/audit"
@@ -427,6 +428,54 @@ func (c *OutgoingAuthConfig) ResolveForBackend(backendID string) *authtypes.Back
427428
return nil
428429
}
429430

431+
// ApplyToBackend applies this outgoing auth configuration to a backend, choosing
432+
// between the backend's own discovered auth and config-based auth. It is the single
433+
// implementation shared by startup discovery (pkg/vmcp/aggregator) and the Kubernetes
434+
// backend watcher's reconciler (pkg/vmcp/k8s), so both paths resolve auth identically.
435+
//
436+
// Auth resolution logic:
437+
// - "discovered" mode: keep discovered auth if the backend has any, otherwise fall
438+
// back to config-based auth via ResolveForBackend (backend-specific entry, then
439+
// Default, then no auth)
440+
// - "inline" mode (or ""): always use config-based auth, ignore discovered auth
441+
// - unknown mode: default to config-based auth for safety
442+
//
443+
// A nil receiver is a no-op: the backend keeps whatever auth it already carries.
444+
func (c *OutgoingAuthConfig) ApplyToBackend(backend *vmcp.Backend, backendName string) {
445+
if c == nil {
446+
return
447+
}
448+
449+
// Determine if we should use discovered auth or config-based auth
450+
var useDiscoveredAuth bool
451+
switch c.Source {
452+
case "discovered":
453+
// In discovered mode, use auth discovered from MCPServer (if any exists)
454+
// If no auth is discovered, fall back to config-based auth via ResolveForBackend
455+
// which will use backend-specific config, then Default, then no auth
456+
useDiscoveredAuth = backend.AuthConfig != nil
457+
case "inline", "":
458+
// For inline mode or empty source, always use config-based auth
459+
// Ignore any discovered auth from backends
460+
useDiscoveredAuth = false
461+
default:
462+
// Unknown source mode - default to config-based auth for safety
463+
slog.Warn("unknown auth source mode, defaulting to config-based auth", "source", c.Source)
464+
useDiscoveredAuth = false
465+
}
466+
467+
if useDiscoveredAuth {
468+
// Keep the auth discovered from MCPServer (already populated in backend)
469+
slog.Debug("backend using discovered auth strategy", "backend", backendName, "strategy", backend.AuthConfig.Type)
470+
} else {
471+
// Use auth from config (inline mode)
472+
if authConfig := c.ResolveForBackend(backendName); authConfig != nil {
473+
backend.AuthConfig = authConfig
474+
slog.Debug("backend configured with auth strategy from config", "backend", backendName, "strategy", authConfig.Type)
475+
}
476+
}
477+
}
478+
430479
// AggregationConfig defines tool aggregation, filtering, and conflict resolution strategies.
431480
//
432481
// Tool Visibility vs Routing:

pkg/vmcp/k8s/backend_reconciler.go

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818

1919
mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1"
2020
"github.com/stacklok/toolhive/pkg/vmcp"
21+
"github.com/stacklok/toolhive/pkg/vmcp/config"
2122
"github.com/stacklok/toolhive/pkg/vmcp/workloads"
2223
)
2324

@@ -55,7 +56,8 @@ const (
5556
// 3. If groupRef doesn't match → Remove from registry (moved to different group)
5657
// 4. Convert to vmcp.Backend using discoverer
5758
// 5. If conversion fails or returns nil (auth failed) → Remove from registry
58-
// 6. Upsert backend to registry (triggers version increment + cache invalidation)
59+
// 6. Apply config-level outgoing auth (same semantics as startup discovery)
60+
// 7. Upsert backend to registry (triggers version increment + cache invalidation)
5961
type BackendReconciler struct {
6062
client.Client
6163

@@ -70,6 +72,13 @@ type BackendReconciler struct {
7072

7173
// Discoverer converts K8s resources to vmcp.Backend (reuses existing code)
7274
Discoverer workloads.Discoverer
75+
76+
// OutgoingAuth is the vMCP config-level outgoing auth configuration. It is
77+
// applied to every reconciled backend with the same precedence startup
78+
// discovery uses (discovered CR-side auth first, then backends[<name>],
79+
// then Default), so a reconcile does not strip auth that only exists in
80+
// the config. May be nil: backends then keep only their discovered auth.
81+
OutgoingAuth *config.OutgoingAuthConfig
7382
}
7483

7584
// SetupIndexes registers field indexes required by the reconciler's watch handlers.
@@ -283,6 +292,13 @@ func (r *BackendReconciler) convertAndUpsertBackend(
283292
return r.removeBackendFromRegistry(ctx, backendID, "Auth failure or no URL")
284293
}
285294

295+
// Apply config-level outgoing auth with the same precedence startup discovery
296+
// uses (discovered CR-side auth first, then backends[<name>], then Default).
297+
// The discoverer above only resolves auth from the resource's own references,
298+
// so without this step a reconcile would strip auth that exists only in the
299+
// vMCP config (outgoingAuth.default / outgoingAuth.backends).
300+
r.OutgoingAuth.ApplyToBackend(backend, resourceInfo.Name)
301+
286302
// Upsert backend to registry (triggers version increment + cache invalidation)
287303
if err := r.Registry.Upsert(*backend); err != nil {
288304
ctxLogger.Error(err, "Failed to upsert backend to registry", "backendID", backend.ID)

pkg/vmcp/k8s/backend_reconciler_test.go

Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@ import (
2020
mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1"
2121
"github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1/v1beta1test"
2222
"github.com/stacklok/toolhive/pkg/vmcp"
23+
authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types"
24+
"github.com/stacklok/toolhive/pkg/vmcp/config"
2325
"github.com/stacklok/toolhive/pkg/vmcp/k8s"
2426
"github.com/stacklok/toolhive/pkg/vmcp/workloads"
2527
)
@@ -645,3 +647,140 @@ func TestMapAuthConfigToEntries(t *testing.T) {
645647
})
646648
}
647649
}
650+
651+
// newOutgoingAuthTestFixture creates the fake client, discoverer, and registry
652+
// shared by the outgoing-auth reconciliation tests below. The MCPServer carries
653+
// no ExternalAuthConfigRef, so any auth on the upserted backend must come from
654+
// the reconciler's OutgoingAuth config (or from discoveredAuth when non-nil,
655+
// simulating auth resolved from the resource's own references).
656+
func newOutgoingAuthTestFixture(
657+
t *testing.T,
658+
discoveredAuth *authtypes.BackendAuthStrategy,
659+
) (client.Client, *mockDiscoverer, *mockRegistry) {
660+
t.Helper()
661+
662+
scheme := runtime.NewScheme()
663+
require.NoError(t, mcpv1beta1.AddToScheme(scheme))
664+
665+
mcpServer := &mcpv1beta1.MCPServer{
666+
ObjectMeta: metav1.ObjectMeta{
667+
Name: "test-server",
668+
Namespace: "default",
669+
},
670+
Spec: mcpv1beta1.MCPServerSpec{
671+
GroupRef: &mcpv1beta1.MCPGroupRef{Name: "test-group"},
672+
},
673+
}
674+
675+
k8sClient := fake.NewClientBuilder().
676+
WithScheme(scheme).
677+
WithObjects(mcpServer).
678+
Build()
679+
680+
mockBackend := &vmcp.Backend{
681+
ID: "test-server",
682+
Name: "test-server",
683+
BaseURL: "http://test-server:8080",
684+
AuthConfig: discoveredAuth,
685+
}
686+
687+
return k8sClient, &mockDiscoverer{backend: mockBackend}, &mockRegistry{}
688+
}
689+
690+
// reconcileTestServer runs one reconcile of the "test-server" MCPServer and
691+
// returns the single upserted backend.
692+
func reconcileTestServer(t *testing.T, reconciler *k8s.BackendReconciler, mockReg *mockRegistry) vmcp.Backend {
693+
t.Helper()
694+
695+
req := ctrl.Request{
696+
NamespacedName: types.NamespacedName{
697+
Name: "test-server",
698+
Namespace: "default",
699+
},
700+
}
701+
702+
result, err := reconciler.Reconcile(context.Background(), req)
703+
require.NoError(t, err)
704+
assert.Equal(t, ctrl.Result{}, result)
705+
require.Len(t, mockReg.upsertedBackends, 1, "Backend should be upserted to registry")
706+
return mockReg.upsertedBackends[0]
707+
}
708+
709+
// TestReconcile_OutgoingAuthBackendsEntry verifies that a config-level
710+
// outgoingAuth.backends.<name> entry survives reconciliation for a backend whose
711+
// own resource carries no discovered auth. Regression test for the watcher path
712+
// silently dropping config-level outgoing auth (#6454).
713+
func TestReconcile_OutgoingAuthBackendsEntry(t *testing.T) {
714+
t.Parallel()
715+
716+
k8sClient, mockDisc, mockReg := newOutgoingAuthTestFixture(t, nil)
717+
718+
wantStrategy := &authtypes.BackendAuthStrategy{Type: authtypes.StrategyTypeUpstreamInject}
719+
reconciler := newTestReconciler(k8sClient, "default", "test-group", mockReg, mockDisc)
720+
reconciler.OutgoingAuth = &config.OutgoingAuthConfig{
721+
Source: "discovered",
722+
Backends: map[string]*authtypes.BackendAuthStrategy{
723+
"test-server": wantStrategy,
724+
},
725+
}
726+
727+
upserted := reconcileTestServer(t, reconciler, mockReg)
728+
require.NotNil(t, upserted.AuthConfig, "backends[<name>] auth config must survive reconciliation")
729+
assert.Equal(t, authtypes.StrategyTypeUpstreamInject, upserted.AuthConfig.Type)
730+
}
731+
732+
// TestReconcile_OutgoingAuthDefault verifies that the config-level
733+
// outgoingAuth.default survives reconciliation for a backend whose own resource
734+
// carries no discovered auth. Regression test for #6454.
735+
func TestReconcile_OutgoingAuthDefault(t *testing.T) {
736+
t.Parallel()
737+
738+
k8sClient, mockDisc, mockReg := newOutgoingAuthTestFixture(t, nil)
739+
740+
reconciler := newTestReconciler(k8sClient, "default", "test-group", mockReg, mockDisc)
741+
reconciler.OutgoingAuth = &config.OutgoingAuthConfig{
742+
Source: "discovered",
743+
Default: &authtypes.BackendAuthStrategy{Type: authtypes.StrategyTypeHeaderInjection},
744+
}
745+
746+
upserted := reconcileTestServer(t, reconciler, mockReg)
747+
require.NotNil(t, upserted.AuthConfig, "default auth config must survive reconciliation")
748+
assert.Equal(t, authtypes.StrategyTypeHeaderInjection, upserted.AuthConfig.Type)
749+
}
750+
751+
// TestReconcile_OutgoingAuthDiscoveredWins verifies that in discovered mode the
752+
// auth resolved from the backend's own resource references takes precedence over
753+
// a conflicting config-level backends entry — the same precedence startup
754+
// discovery applies.
755+
func TestReconcile_OutgoingAuthDiscoveredWins(t *testing.T) {
756+
t.Parallel()
757+
758+
discovered := &authtypes.BackendAuthStrategy{Type: authtypes.StrategyTypeUpstreamInject}
759+
k8sClient, mockDisc, mockReg := newOutgoingAuthTestFixture(t, discovered)
760+
761+
reconciler := newTestReconciler(k8sClient, "default", "test-group", mockReg, mockDisc)
762+
reconciler.OutgoingAuth = &config.OutgoingAuthConfig{
763+
Source: "discovered",
764+
Backends: map[string]*authtypes.BackendAuthStrategy{
765+
"test-server": {Type: authtypes.StrategyTypeHeaderInjection},
766+
},
767+
}
768+
769+
upserted := reconcileTestServer(t, reconciler, mockReg)
770+
require.NotNil(t, upserted.AuthConfig)
771+
assert.Equal(t, authtypes.StrategyTypeUpstreamInject, upserted.AuthConfig.Type,
772+
"discovered auth must win over the config-level entry in discovered mode")
773+
}
774+
775+
// TestReconcile_OutgoingAuthNilConfig verifies that a nil OutgoingAuth leaves
776+
// the reconciled backend untouched (the pre-existing behavior).
777+
func TestReconcile_OutgoingAuthNilConfig(t *testing.T) {
778+
t.Parallel()
779+
780+
k8sClient, mockDisc, mockReg := newOutgoingAuthTestFixture(t, nil)
781+
782+
reconciler := newTestReconciler(k8sClient, "default", "test-group", mockReg, mockDisc)
783+
784+
upserted := reconcileTestServer(t, reconciler, mockReg)
785+
assert.Nil(t, upserted.AuthConfig, "backend without discovered auth stays unauthenticated when no config is supplied")
786+
}

0 commit comments

Comments
 (0)