changes from review

Author: taskbot

cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_webhook.go

@@ -24,14 +24,26 @@ var _ webhook.CustomValidator = &MCPExternalAuthConfig{}
 
 // ValidateCreate implements webhook.CustomValidator
 func (r *MCPExternalAuthConfig) ValidateCreate(_ context.Context, _ runtime.Object) (admission.Warnings, error) {
-	return nil, r.validate()
+	var warnings admission.Warnings
+	if r.Spec.Type == ExternalAuthTypeUnauthenticated {
+		warnings = append(warnings,
+			"'unauthenticated' type disables authentication to the backend. "+
+				"Only use for backends on trusted networks or when authentication is handled by network-level security.")
+	}
+	return warnings, r.validate()
 }
 
 // ValidateUpdate implements webhook.CustomValidator
 func (r *MCPExternalAuthConfig) ValidateUpdate(
 	_ context.Context, _ runtime.Object, _ runtime.Object,
 ) (admission.Warnings, error) {
-	return nil, r.validate()
+	var warnings admission.Warnings
+	if r.Spec.Type == ExternalAuthTypeUnauthenticated {
+		warnings = append(warnings,
+			"'unauthenticated' type disables authentication to the backend. "+
+				"Only use for backends on trusted networks or when authentication is handled by network-level security.")
+	}
+	return warnings, r.validate()
 }
 
 // ValidateDelete implements webhook.CustomValidator

cmd/thv-operator/controllers/virtualmcpserver_controller.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"maps"
 	"reflect"
+	"strings"
 	"time"
 
 	appsv1 "k8s.io/api/apps/v1"
@@ -64,10 +65,6 @@ type VirtualMCPServerReconciler struct {
 	PlatformDetector *ctrlutil.SharedPlatformDetector
 }
 
-var (
-	envVarSanitizeRegex = regexp.MustCompile(`[^A-Z0-9_]`)
-)
-
 // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=virtualmcpservers,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=virtualmcpservers/status,verbs=get;update;patch
 // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpgroups,verbs=get;list;watch

cmd/thv-operator/pkg/controllerutil/externalauth.go

@@ -7,6 +7,10 @@ import (
 	"strings"
 )
 
+var (
+	envVarSanitizer = regexp.MustCompile(`[^A-Z0-9_]`)
+)
+
 // GenerateUniqueTokenExchangeEnvVarName generates a unique environment variable name for token exchange
 // client secrets, incorporating the ExternalAuthConfig name to ensure uniqueness.
 // This function is used by both the converter and deployment controller to ensure consistent
@@ -18,7 +22,7 @@ func GenerateUniqueTokenExchangeEnvVarName(configName string) string {
 	// Sanitize config name for use in env var (uppercase, replace invalid chars with underscore)
 	sanitized := strings.ToUpper(strings.ReplaceAll(configName, "-", "_"))
 	// Remove any remaining invalid characters (keep only alphanumeric and underscore)
-	sanitized = regexp.MustCompile(`[^A-Z0-9_]`).ReplaceAllString(sanitized, "_")
+	sanitized = envVarSanitizer.ReplaceAllString(sanitized, "_")
 	return fmt.Sprintf("TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET_%s", sanitized)
 }
 
@@ -33,6 +37,6 @@ func GenerateUniqueHeaderInjectionEnvVarName(configName string) string {
 	// Sanitize config name for use in env var (uppercase, replace invalid chars with underscore)
 	sanitized := strings.ToUpper(strings.ReplaceAll(configName, "-", "_"))
 	// Remove any remaining invalid characters (keep only alphanumeric and underscore)
-	sanitized = regexp.MustCompile(`[^A-Z0-9_]`).ReplaceAllString(sanitized, "_")
+	sanitized = envVarSanitizer.ReplaceAllString(sanitized, "_")
 	return fmt.Sprintf("TOOLHIVE_HEADER_INJECTION_VALUE_%s", sanitized)
 }

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

@@ -18,6 +18,7 @@ import (
 	"github.com/stacklok/toolhive/cmd/thv-operator/pkg/controllerutil"
 	"github.com/stacklok/toolhive/cmd/thv-operator/pkg/oidc"
 	"github.com/stacklok/toolhive/cmd/thv-operator/pkg/spectoconfig"
+	"github.com/stacklok/toolhive/pkg/vmcp/auth/converters"
 	authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types"
 	vmcpconfig "github.com/stacklok/toolhive/pkg/vmcp/config"
 )
@@ -300,53 +301,37 @@ func (c *Converter) convertBackendAuthConfig(
 	return nil, fmt.Errorf("backend %s: unknown auth type %q", backendName, crdConfig.Type)
 }
 
-// convertExternalAuthConfigToStrategy converts MCPExternalAuthConfig to BackendAuthStrategy
+// convertExternalAuthConfigToStrategy converts MCPExternalAuthConfig to BackendAuthStrategy.
+// This uses the converter registry to consolidate conversion logic and apply token type normalization consistently.
+// The registry pattern makes adding new auth types easier and ensures conversion happens in one place.
 func (*Converter) convertExternalAuthConfigToStrategy(
 	_ context.Context,
 	externalAuthConfig *mcpv1alpha1.MCPExternalAuthConfig,
 ) (*authtypes.BackendAuthStrategy, error) {
-	strategy := &authtypes.BackendAuthStrategy{}
-
-	switch externalAuthConfig.Spec.Type {
-	case mcpv1alpha1.ExternalAuthTypeUnauthenticated:
-		strategy.Type = authtypes.StrategyTypeUnauthenticated
-
-	case mcpv1alpha1.ExternalAuthTypeHeaderInjection:
-		if externalAuthConfig.Spec.HeaderInjection == nil {
-			return nil, fmt.Errorf("headerInjection config is required when type is headerInjection")
-		}
-
-		strategy.Type = authtypes.StrategyTypeHeaderInjection
-		strategy.HeaderInjection = &authtypes.HeaderInjectionConfig{
-			HeaderName: externalAuthConfig.Spec.HeaderInjection.HeaderName,
-			// The secret value will be mounted as an environment variable by the deployment controller
-			// Use the same env var naming convention as the deployment controller
-			HeaderValueEnv: controllerutil.GenerateUniqueHeaderInjectionEnvVarName(externalAuthConfig.Name),
-		}
-
-	case mcpv1alpha1.ExternalAuthTypeTokenExchange:
-		if externalAuthConfig.Spec.TokenExchange == nil {
-			return nil, fmt.Errorf("tokenExchange config is required when type is tokenExchange")
-		}
-
-		strategy.Type = authtypes.StrategyTypeTokenExchange
-		strategy.TokenExchange = &authtypes.TokenExchangeConfig{
-			TokenURL:         externalAuthConfig.Spec.TokenExchange.TokenURL,
-			ClientID:         externalAuthConfig.Spec.TokenExchange.ClientID,
-			Audience:         externalAuthConfig.Spec.TokenExchange.Audience,
-			Scopes:           externalAuthConfig.Spec.TokenExchange.Scopes,
-			SubjectTokenType: externalAuthConfig.Spec.TokenExchange.SubjectTokenType,
-		}
+	// Use the converter registry to convert to typed strategy
+	registry := converters.DefaultRegistry()
+	converter, err := registry.GetConverter(externalAuthConfig.Spec.Type)
+	if err != nil {
+		return nil, err
+	}
 
-		// If client secret ref is set, use an environment variable
-		if externalAuthConfig.Spec.TokenExchange.ClientSecretRef != nil {
-			// The secret value will be mounted as an environment variable by the deployment controller
-			// Use the same env var naming convention as the deployment controller
-			strategy.TokenExchange.ClientSecretEnv = controllerutil.GenerateUniqueTokenExchangeEnvVarName(externalAuthConfig.Name)
-		}
+	// Convert to typed BackendAuthStrategy (applies token type normalization)
+	strategy, err := converter.ConvertToStrategy(externalAuthConfig)
+	if err != nil {
+		return nil, fmt.Errorf("failed to convert external auth config to strategy: %w", err)
+	}
 
-	default:
-		return nil, fmt.Errorf("unknown external auth type: %s", externalAuthConfig.Spec.Type)
+	// Enrich with unique env var names per ExternalAuthConfig to avoid conflicts
+	// when multiple configs of the same type reference different secrets
+	if strategy.TokenExchange != nil &&
+		externalAuthConfig.Spec.TokenExchange != nil &&
+		externalAuthConfig.Spec.TokenExchange.ClientSecretRef != nil {
+		strategy.TokenExchange.ClientSecretEnv = controllerutil.GenerateUniqueTokenExchangeEnvVarName(externalAuthConfig.Name)
+	}
+	if strategy.HeaderInjection != nil &&
+		externalAuthConfig.Spec.HeaderInjection != nil &&
+		externalAuthConfig.Spec.HeaderInjection.ValueSecretRef != nil {
+		strategy.HeaderInjection.HeaderValueEnv = controllerutil.GenerateUniqueHeaderInjectionEnvVarName(externalAuthConfig.Name)
 	}
 
 	return strategy, nil