Refactor Converter to require OIDC resolver for all OIDC types

Previously, the Converter only handled inline OIDC configuration directly and had a TODO for kubernetes and configMap types. This change: - Makes oidcResolver a required parameter for NewConverter (returns error if nil) - Uses the resolver for all OIDC types (kubernetes, configMap, inline) - Adds mapResolvedOIDCToVmcpConfig() to map resolver output to vmcp config - Handles ClientSecretEnv for all OIDC types that may have client secrets - Updates controller to pass OIDC resolver to converter - Adds comprehensive tests using gomock for OIDC resolution - Adds compile-time interface assertion for VirtualMCPServer This ensures fail-closed behavior: if OIDC is configured but resolution fails, the converter returns an error rather than silently deploying without authentication.

Author: jhrozek

cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig.go

@@ -13,6 +13,7 @@ import (
 "sigs.k8s.io/controller-runtime/pkg/log"
 
 mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1"
+"github.com/stacklok/toolhive/cmd/thv-operator/pkg/oidc"
 "github.com/stacklok/toolhive/cmd/thv-operator/pkg/runconfig/configmap/checksum"
 "github.com/stacklok/toolhive/cmd/thv-operator/pkg/vmcpconfig"
 "github.com/stacklok/toolhive/pkg/vmcp/workloads"
@@ -25,8 +26,14 @@ func (r *VirtualMCPServerReconciler) ensureVmcpConfigConfigMap(
 ) error {
 ctxLogger := log.FromContext(ctx)
 
-// Convert CRD to vmcp config using converter
-converter := vmcpconfig.NewConverter()
+// Create OIDC resolver to handle all OIDC types (kubernetes, configMap, inline)
+oidcResolver := oidc.NewResolver(r.Client)
+
+// Convert CRD to vmcp config using converter with OIDC resolver
+converter, err := vmcpconfig.NewConverter(oidcResolver)
+if err != nil {
+return fmt.Errorf("failed to create vmcp converter: %w", err)
+}
 config, err := converter.Convert(ctx, vmcp)
 if err != nil {
 return fmt.Errorf("failed to create vmcp Config from VirtualMCPServer: %w", err)

cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go

@@ -20,6 +20,7 @@ import (
 
 "github.com/stretchr/testify/assert"
 "github.com/stretchr/testify/require"
+"go.uber.org/mock/gomock"
 "gopkg.in/yaml.v3"
 corev1 "k8s.io/api/core/v1"
 metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -28,10 +29,29 @@ import (
 "sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1"
+oidcmocks "github.com/stacklok/toolhive/cmd/thv-operator/pkg/oidc/mocks"
 "github.com/stacklok/toolhive/cmd/thv-operator/pkg/vmcpconfig"
 "github.com/stacklok/toolhive/pkg/vmcp"
 )
 
+// newNoOpMockResolver creates a mock resolver that returns (nil, nil) for all calls.
+// Use this in tests that don't care about OIDC configuration.
+func newNoOpMockResolver(t *testing.T) *oidcmocks.MockResolver {
+t.Helper()
+ctrl := gomock.NewController(t)
+mockResolver := oidcmocks.NewMockResolver(ctrl)
+mockResolver.EXPECT().Resolve(gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes()
+return mockResolver
+}
+
+// newTestConverter creates a Converter with the given resolver, failing the test if creation fails.
+func newTestConverter(t *testing.T, resolver *oidcmocks.MockResolver) *vmcpconfig.Converter {
+t.Helper()
+converter, err := vmcpconfig.NewConverter(resolver)
+require.NoError(t, err)
+return converter
+}
+
 // TestCreateVmcpConfigFromVirtualMCPServer tests vmcp config generation
 func TestCreateVmcpConfigFromVirtualMCPServer(t *testing.T) {
 t.Parallel()
@@ -65,7 +85,7 @@ func TestCreateVmcpConfigFromVirtualMCPServer(t *testing.T) {
 t.Run(tt.name, func(t *testing.T) {
 t.Parallel()
 
-converter := vmcpconfig.NewConverter()
+converter := newTestConverter(t, newNoOpMockResolver(t))
 config, err := converter.Convert(context.Background(), tt.vmcp)
 
 require.NoError(t, err)
@@ -138,7 +158,7 @@ func TestConvertOutgoingAuth(t *testing.T) {
 },
 }
 
-converter := vmcpconfig.NewConverter()
+converter := newTestConverter(t, newNoOpMockResolver(t))
 config, err := converter.Convert(context.Background(), vmcpServer)
 require.NoError(t, err)
 
@@ -198,7 +218,7 @@ func TestConvertBackendAuthConfig(t *testing.T) {
 },
 }
 
-converter := vmcpconfig.NewConverter()
+converter := newTestConverter(t, newNoOpMockResolver(t))
 config, err := converter.Convert(context.Background(), vmcpServer)
 require.NoError(t, err)
 
@@ -292,7 +312,7 @@ func TestConvertAggregation(t *testing.T) {
 },
 }
 
-converter := vmcpconfig.NewConverter()
+converter := newTestConverter(t, newNoOpMockResolver(t))
 config, err := converter.Convert(context.Background(), vmcpServer)
 require.NoError(t, err)
 
@@ -385,7 +405,7 @@ func TestConvertCompositeTools(t *testing.T) {
 },
 }
 
-converter := vmcpconfig.NewConverter()
+converter := newTestConverter(t, newNoOpMockResolver(t))
 config, err := converter.Convert(context.Background(), vmcpServer)
 require.NoError(t, err)
 
@@ -564,7 +584,7 @@ func TestYAMLMarshalingDeterminism(t *testing.T) {
 },
 }
 
-converter := vmcpconfig.NewConverter()
+converter := newTestConverter(t, newNoOpMockResolver(t))
 
 // Marshal the config 10 times to ensure deterministic output
 const iterations = 10

cmd/thv-operator/pkg/vmcpconfig/converter.go

@@ -4,11 +4,13 @@ package vmcpconfig
 import (
 "context"
 "encoding/json"
+"fmt"
 "time"
 
 "sigs.k8s.io/controller-runtime/pkg/log"
 
 mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1"
+"github.com/stacklok/toolhive/cmd/thv-operator/pkg/oidc"
 authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types"
 vmcpconfig "github.com/stacklok/toolhive/pkg/vmcp/config"
 )
@@ -18,19 +20,31 @@ const (
 authzLabelValueInline = "inline"
 // conflictResolutionPrefix is the string value for prefix conflict resolution strategy
 conflictResolutionPrefix = "prefix"
+// vmcpOIDCClientSecretEnvVar is the environment variable name for the OIDC client secret.
+// The deployment controller mounts secrets as environment variables with this name.
+//nolint:gosec // This is an environment variable name, not a credential
+vmcpOIDCClientSecretEnvVar = "VMCP_OIDC_CLIENT_SECRET"
 )
 
 // Converter converts VirtualMCPServer CRD specs to vmcp Config
-type Converter struct{}
+type Converter struct {
+oidcResolver oidc.Resolver
+}
 
-// NewConverter creates a new Converter instance
-func NewConverter() *Converter {
-return &Converter{}
+// NewConverter creates a new Converter instance.
+// oidcResolver is required and used to resolve OIDC configuration from various sources
+// (kubernetes, configMap, inline). Use a mock resolver in tests.
+// Returns an error if oidcResolver is nil.
+func NewConverter(oidcResolver oidc.Resolver) (*Converter, error) {
+if oidcResolver == nil {
+return nil, fmt.Errorf("oidcResolver is required")
+}
+return &Converter{
+oidcResolver: oidcResolver,
+}, nil
 }
 
 // Convert converts VirtualMCPServer CRD spec to vmcp Config
-//
-//nolint:unparam // error return reserved for future reference resolution
 func (c *Converter) Convert(
 ctx context.Context,
 vmcp *mcpv1alpha1.VirtualMCPServer,
@@ -42,7 +56,11 @@ func (c *Converter) Convert(
 
 // Convert IncomingAuth
 if vmcp.Spec.IncomingAuth != nil {
-config.IncomingAuth = c.convertIncomingAuth(ctx, vmcp)
+incomingAuth, err := c.convertIncomingAuth(ctx, vmcp)
+if err != nil {
+return nil, fmt.Errorf("failed to convert incoming auth: %w", err)
+}
+config.IncomingAuth = incomingAuth
 }
 
 // Convert OutgoingAuth - always set with defaults if not specified
@@ -84,45 +102,34 @@ func (c *Converter) Convert(
 return config, nil
 }
 
-// convertIncomingAuth converts IncomingAuthConfig from CRD to vmcp config
-func (*Converter) convertIncomingAuth(
-_ context.Context,
+// convertIncomingAuth converts IncomingAuthConfig from CRD to vmcp config.
+func (c *Converter) convertIncomingAuth(
+ctx context.Context,
 vmcp *mcpv1alpha1.VirtualMCPServer,
-) *vmcpconfig.IncomingAuthConfig {
+) (*vmcpconfig.IncomingAuthConfig, error) {
+ctxLogger := log.FromContext(ctx)
+
 incoming := &vmcpconfig.IncomingAuthConfig{
 Type: vmcp.Spec.IncomingAuth.Type,
 }
 
 // Convert OIDC configuration if present
 if vmcp.Spec.IncomingAuth.OIDCConfig != nil {
-// Handle inline OIDC configuration
-if vmcp.Spec.IncomingAuth.OIDCConfig.Type == authzLabelValueInline && vmcp.Spec.IncomingAuth.OIDCConfig.Inline != nil {
-inline := vmcp.Spec.IncomingAuth.OIDCConfig.Inline
-oidcConfig := &vmcpconfig.OIDCConfig{
-Issuer: inline.Issuer,
-ClientID: inline.ClientID, // Note: API uses clientId (camelCase) but config uses ClientID
-Audience: inline.Audience,
-Resource: vmcp.Spec.IncomingAuth.OIDCConfig.ResourceURL,
-Scopes: nil, // TODO: Add scopes if needed
-ProtectedResourceAllowPrivateIP: inline.ProtectedResourceAllowPrivateIP,
-InsecureAllowHTTP: inline.InsecureAllowHTTP,
-}
-
-// Handle client secret - always use environment variable reference for security
-// Both ClientSecretRef (reference to existing secret) and ClientSecret (literal value)
-// are mounted as environment variables by the deployment controller
-if inline.ClientSecretRef != nil || inline.ClientSecret != "" {
-// Generate environment variable name that will be mounted in the deployment
-// The deployment controller will mount the secret (either from ClientSecretRef or
-// from a generated secret for ClientSecret literal values)
-oidcConfig.ClientSecretEnv = "VMCP_OIDC_CLIENT_SECRET"
-}
-
-incoming.OIDC = oidcConfig
-} else {
-// TODO: Handle configMap and kubernetes types
-// For now, create empty config to avoid nil pointer
-incoming.OIDC = &vmcpconfig.OIDCConfig{}
+// Use the OIDC resolver to handle all OIDC types (kubernetes, configMap, inline)
+// VirtualMCPServer implements OIDCConfigurable, so the resolver can work with it directly
+resolvedConfig, err := c.oidcResolver.Resolve(ctx, vmcp)
+if err != nil {
+ctxLogger.Error(err, "failed to resolve OIDC config",
+"vmcp", vmcp.Name,
+"namespace", vmcp.Namespace,
+"oidcType", vmcp.Spec.IncomingAuth.OIDCConfig.Type)
+// Fail closed: return error when OIDC is configured but resolution fails
+// This prevents deploying without authentication when OIDC is explicitly requested
+return nil, fmt.Errorf("OIDC resolution failed for type %q: %w",
+vmcp.Spec.IncomingAuth.OIDCConfig.Type, err)
+}
+if resolvedConfig != nil {
+incoming.OIDC = mapResolvedOIDCToVmcpConfig(resolvedConfig, vmcp.Spec.IncomingAuth.OIDCConfig)
 }
 }
 
@@ -146,7 +153,53 @@ func (*Converter) convertIncomingAuth(
 // TODO: Load policies from ConfigMap if Type is "configMap"
 }
 
-return incoming
+return incoming, nil
+}
+
+// mapResolvedOIDCToVmcpConfig maps from oidc.OIDCConfig (resolved by the OIDC resolver)
+// to vmcpconfig.OIDCConfig (used by the vmcp runtime).
+// This keeps the vmcp config types separate from the operator's OIDC resolver types,
+// maintaining clean architectural boundaries while enabling unified OIDC resolution.
+func mapResolvedOIDCToVmcpConfig(
+resolved *oidc.OIDCConfig,
+oidcConfigRef *mcpv1alpha1.OIDCConfigRef,
+) *vmcpconfig.OIDCConfig {
+if resolved == nil {
+return nil
+}
+
+config := &vmcpconfig.OIDCConfig{
+Issuer: resolved.Issuer,
+ClientID: resolved.ClientID,
+Audience: resolved.Audience,
+Resource: resolved.ResourceURL,
+ProtectedResourceAllowPrivateIP: resolved.JWKSAllowPrivateIP,
+InsecureAllowHTTP: resolved.InsecureAllowHTTP,
+// Scopes are not currently in oidc.OIDCConfig - should be added later
+}
+
+// Handle client secret - the deployment controller mounts secrets as environment variables
+// We need to set ClientSecretEnv for all OIDC config types that may have a client secret
+if oidcConfigRef != nil {
+switch oidcConfigRef.Type {
+case mcpv1alpha1.OIDCConfigTypeInline:
+// Inline config: check if ClientSecretRef or ClientSecret is set
+if oidcConfigRef.Inline != nil {
+if oidcConfigRef.Inline.ClientSecretRef != nil || oidcConfigRef.Inline.ClientSecret != "" {
+config.ClientSecretEnv = vmcpOIDCClientSecretEnvVar
+}
+}
+case mcpv1alpha1.OIDCConfigTypeConfigMap:
+// ConfigMap config: check if the resolved config has a client secret
+// Note: Storing secrets in ConfigMaps is not recommended; use inline with SecretRef instead
+if resolved.ClientSecret != "" {
+config.ClientSecretEnv = vmcpOIDCClientSecretEnvVar
+}
+// OIDCConfigTypeKubernetes does not use client secrets (uses service account tokens)
+}
+}
+
+return config
 }
 
 // convertOutgoingAuth converts OutgoingAuthConfig from CRD to vmcp config