add new configmap flag in proxy runner to read config from there (#1789)

Initially it will read the configmap and validate that it exists

Related-to: #1638

Co-authored-by: taskbot <[email protected]>

Author: yrobla

cmd/thv-operator/controllers/mcpserver_controller.go

@@ -71,13 +71,21 @@ var defaultRBACRules = []rbacv1.PolicyRule{
 		Resources: []string{"pods/attach"},
 		Verbs:     []string{"create", "get"},
 	},
+	{
+		APIGroups: []string{""},
+		Resources: []string{"configmaps"},
+		Verbs:     []string{"get", "list", "watch"},
+	},
 }
 
 var ctxLogger = log.FromContext(context.Background())
 
 // mcpContainerName is the name of the mcp container used in pod templates
 const mcpContainerName = "mcp"
 
+// trueValue is the string value "true" used for environment variable comparisons
+const trueValue = "true"
+
 // Authorization ConfigMap label constants
 const (
 	// authzLabelKey is the label key for authorization configuration type
@@ -463,38 +471,51 @@ func (r *MCPServerReconciler) deploymentForMCPServer(ctx context.Context, m *mcp
 
 	// Prepare container args
 	args := []string{"run", "--foreground=true"}
-	args = append(args, fmt.Sprintf("--proxy-port=%d", m.Spec.Port))
-	args = append(args, fmt.Sprintf("--name=%s", m.Name))
-	args = append(args, fmt.Sprintf("--transport=%s", m.Spec.Transport))
-	args = append(args, fmt.Sprintf("--host=%s", getProxyHost()))
 
-	if m.Spec.TargetPort != 0 {
-		args = append(args, fmt.Sprintf("--target-port=%d", m.Spec.TargetPort))
-	}
-
-	// Generate pod template patch for secrets and merge with user-provided patch
-
-	finalPodTemplateSpec := NewMCPServerPodTemplateSpecBuilder(m.Spec.PodTemplateSpec).
-		WithServiceAccount(m.Spec.ServiceAccount).
-		WithSecrets(m.Spec.Secrets).
-		Build()
-	// Add pod template patch if we have one
-	if finalPodTemplateSpec != nil {
-		podTemplatePatch, err := json.Marshal(finalPodTemplateSpec)
-		if err != nil {
-			logger.Errorf("Failed to marshal pod template spec: %v", err)
-		} else {
-			args = append(args, fmt.Sprintf("--k8s-pod-patch=%s", string(podTemplatePatch)))
+	// Check if global ConfigMap mode is enabled via environment variable
+	useConfigMap := os.Getenv("TOOLHIVE_USE_CONFIGMAP") == trueValue
+	if useConfigMap {
+		// Use the operator-created ConfigMap (format: {name}-runconfig)
+		configMapName := fmt.Sprintf("%s-runconfig", m.Name)
+		configMapRef := fmt.Sprintf("%s/%s", m.Namespace, configMapName)
+		args = append(args, fmt.Sprintf("--from-configmap=%s", configMapRef))
+	} else {
+		// Use individual configuration flags (existing behavior)
+		args = append(args, fmt.Sprintf("--proxy-port=%d", m.Spec.Port))
+		args = append(args, fmt.Sprintf("--name=%s", m.Name))
+		args = append(args, fmt.Sprintf("--transport=%s", m.Spec.Transport))
+		args = append(args, fmt.Sprintf("--host=%s", getProxyHost()))
+		if m.Spec.TargetPort != 0 {
+			args = append(args, fmt.Sprintf("--target-port=%d", m.Spec.TargetPort))
+		}
+	}
+
+	// Add pod template patch and permission profile only if not using ConfigMap
+	// When using ConfigMap, these are included in the runconfig.json
+	if !useConfigMap {
+		// Generate pod template patch for secrets and merge with user-provided patch
+		finalPodTemplateSpec := NewMCPServerPodTemplateSpecBuilder(m.Spec.PodTemplateSpec).
+			WithServiceAccount(m.Spec.ServiceAccount).
+			WithSecrets(m.Spec.Secrets).
+			Build()
+		// Add pod template patch if we have one
+		if finalPodTemplateSpec != nil {
+			podTemplatePatch, err := json.Marshal(finalPodTemplateSpec)
+			if err != nil {
+				logger.Errorf("Failed to marshal pod template spec: %v", err)
+			} else {
+				args = append(args, fmt.Sprintf("--k8s-pod-patch=%s", string(podTemplatePatch)))
+			}
 		}
-	}
 
-	// Add permission profile args
-	if m.Spec.PermissionProfile != nil {
-		switch m.Spec.PermissionProfile.Type {
-		case mcpv1alpha1.PermissionProfileTypeBuiltin:
-			args = append(args, fmt.Sprintf("--permission-profile=%s", m.Spec.PermissionProfile.Name))
-		case mcpv1alpha1.PermissionProfileTypeConfigMap:
-			args = append(args, fmt.Sprintf("--permission-profile-path=/etc/toolhive/profiles/%s", m.Spec.PermissionProfile.Key))
+		// Add permission profile args
+		if m.Spec.PermissionProfile != nil {
+			switch m.Spec.PermissionProfile.Type {
+			case mcpv1alpha1.PermissionProfileTypeBuiltin:
+				args = append(args, fmt.Sprintf("--permission-profile=%s", m.Spec.PermissionProfile.Name))
+			case mcpv1alpha1.PermissionProfileTypeConfigMap:
+				args = append(args, fmt.Sprintf("--permission-profile-path=/etc/toolhive/profiles/%s", m.Spec.PermissionProfile.Key))
+			}
 		}
 	}
 
@@ -526,15 +547,18 @@ func (r *MCPServerReconciler) deploymentForMCPServer(ctx context.Context, m *mcp
 		args = append(args, "--enable-audit")
 	}
 
-	// Add environment variables as --env flags for the MCP server
-	for _, e := range m.Spec.Env {
-		args = append(args, fmt.Sprintf("--env=%s=%s", e.Name, e.Value))
-	}
+	// Add environment variables and tools filter only if not using ConfigMap
+	if !useConfigMap {
+		// Add environment variables as --env flags for the MCP server
+		for _, e := range m.Spec.Env {
+			args = append(args, fmt.Sprintf("--env=%s=%s", e.Name, e.Value))
+		}
 
-	// Add tools filter args
-	if len(m.Spec.ToolsFilter) > 0 {
-		slices.Sort(m.Spec.ToolsFilter)
-		args = append(args, fmt.Sprintf("--tools=%s", strings.Join(m.Spec.ToolsFilter, ",")))
+		// Add tools filter args
+		if len(m.Spec.ToolsFilter) > 0 {
+			slices.Sort(m.Spec.ToolsFilter)
+			args = append(args, fmt.Sprintf("--tools=%s", strings.Join(m.Spec.ToolsFilter, ",")))
+		}
 	}
 
 	// Add OpenTelemetry configuration args
@@ -550,11 +574,13 @@ func (r *MCPServerReconciler) deploymentForMCPServer(ctx context.Context, m *mcp
 		}
 	}
 
-	// Add the image
+	// Always add the image as it's required by proxy runner command signature
+	// When using ConfigMap, the image from ConfigMap takes precedence, but we still need
+	// to provide this as a positional argument to satisfy the command requirements
 	args = append(args, m.Spec.Image)
 
-	// Add additional args
-	if len(m.Spec.Args) > 0 {
+	// Add additional args only if not using ConfigMap
+	if !useConfigMap && len(m.Spec.Args) > 0 {
 		args = append(args, "--")
 		args = append(args, m.Spec.Args...)
 	}

cmd/thv-operator/controllers/mcpserver_runconfig.go

@@ -216,8 +216,7 @@ func (*MCPServerReconciler) createRunConfigFromMCPServer(m *mcpv1alpha1.MCPServe
 		}
 	}
 
-	// Use the RunConfigBuilder for operator context with full builder pattern
-	config, err := runner.NewOperatorRunConfigBuilder().
+	builder := runner.NewOperatorRunConfigBuilder().
 		WithName(m.Name).
 		WithImage(m.Spec.Image).
 		WithCmdArgs(m.Spec.Args).
@@ -227,8 +226,21 @@ func (*MCPServerReconciler) createRunConfigFromMCPServer(m *mcpv1alpha1.MCPServe
 		WithEnvVars(envVars).
 		WithVolumes(volumes).
 		WithSecrets(secrets).
-		WithK8sPodPatch(k8sPodPatch).
-		BuildForOperator()
+		WithK8sPodPatch(k8sPodPatch)
+
+	// Add permission profile if specified
+	if m.Spec.PermissionProfile != nil {
+		switch m.Spec.PermissionProfile.Type {
+		case mcpv1alpha1.PermissionProfileTypeBuiltin:
+			builder = builder.WithPermissionProfileNameOrPath(m.Spec.PermissionProfile.Name)
+		case mcpv1alpha1.PermissionProfileTypeConfigMap:
+			// For ConfigMap-based permission profiles, we store the path
+			builder = builder.WithPermissionProfileNameOrPath(fmt.Sprintf("/etc/toolhive/profiles/%s", m.Spec.PermissionProfile.Key))
+		}
+	}
+
+	// Use the RunConfigBuilder for operator context with full builder pattern
+	config, err := builder.BuildForOperator()
 
 	if err != nil {
 		return nil, err

cmd/thv-proxyrunner/app/run.go

@@ -1,11 +1,18 @@
 package app
 
 import (
+	"context"
+	"encoding/json"
 	"fmt"
 	"net"
 	"os"
+	"strings"
 
 	"github.com/spf13/cobra"
+	"github.com/spf13/pflag"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/kubernetes"
+	"k8s.io/client-go/rest"
 
 	"github.com/stacklok/toolhive/pkg/container"
 	"github.com/stacklok/toolhive/pkg/environment"
@@ -17,6 +24,11 @@ import (
 	"github.com/stacklok/toolhive/pkg/workloads"
 )
 
+// NewRunCmd creates a new run command for testing
+func NewRunCmd() *cobra.Command {
+	return runCmd
+}
+
 var runCmd = &cobra.Command{
 	Use:   "run [flags] SERVER_OR_IMAGE_OR_PROTOCOL [-- ARGS...]",
 	Short: "Run an MCP server",
@@ -90,6 +102,9 @@ var (
 
 	// Environment file processing
 	runEnvFileDir string
+
+	// ConfigMap reference flag (for identification only)
+	runFromConfigMap string
 )
 
 func init() {
@@ -219,11 +234,76 @@ func init() {
 		"",
 		"Load environment variables from all files in a directory",
 	)
+	runCmd.Flags().StringVar(
+		&runFromConfigMap,
+		"from-configmap",
+		"",
+		"[Experimental] Load configuration from a Kubernetes ConfigMap (format: namespace/configmap-name). "+
+			"This flag is mutually exclusive with other configuration flags.",
+	)
+}
+
+// validateConfigMapOnlyMode validates that no conflicting flags are used with --from-configmap
+// It dynamically finds all flags that could conflict by checking which ones are changed
+func validateConfigMapOnlyMode(cmd *cobra.Command) error {
+	// If --from-configmap is not set, no validation needed
+	fromConfigMapFlag := cmd.Flag("from-configmap")
+	if fromConfigMapFlag == nil || !fromConfigMapFlag.Changed {
+		return nil
+	}
+
+	var conflictingFlags []string
+
+	// Check all flags that are explicitly set by the user
+	cmd.Flags().VisitAll(func(flag *pflag.Flag) {
+		// Skip the from-configmap flag itself and flags that weren't changed
+		if flag.Name == "from-configmap" || !flag.Changed {
+			return
+		}
+
+		// Skip flags that are safe to use with ConfigMap (like help, debug, etc.)
+		if isSafeFlagWithConfigMap(flag.Name) {
+			return
+		}
+
+		// All other changed flags are considered conflicting
+		conflictingFlags = append(conflictingFlags, "--"+flag.Name)
+	})
+
+	if len(conflictingFlags) > 0 {
+		return fmt.Errorf("cannot use --from-configmap with the following flags (configuration should be provided by ConfigMap): %s",
+			strings.Join(conflictingFlags, ", "))
+	}
+
+	return nil
+}
+
+// isSafeFlagWithConfigMap returns true for flags that are safe to use with --from-configmap
+// These are typically operational flags that don't affect the RunConfig
+func isSafeFlagWithConfigMap(flagName string) bool {
+	safeFlagsWithConfigMap := map[string]bool{
+		"help":  true,
+		"debug": true,
+		// Add other safe flags here if needed
+	}
+	return safeFlagsWithConfigMap[flagName]
 }
 
 func runCmdFunc(cmd *cobra.Command, args []string) error {
 	ctx := cmd.Context()
 
+	// Handle ConfigMap identification if specified
+	if runFromConfigMap != "" {
+		// Validate that conflicting flags are not used with --from-configmap first
+		if err := validateConfigMapOnlyMode(cmd); err != nil {
+			return err
+		}
+
+		if err := identifyAndReadConfigMap(ctx, runFromConfigMap); err != nil {
+			return fmt.Errorf("failed to identify ConfigMap: %w", err)
+		}
+	}
+
 	// Validate the host flag and default resolving to IP in case hostname is provided
 	validatedHost, err := ValidateAndNormaliseHostFlag(runHost)
 	if err != nil {
@@ -353,3 +433,69 @@ func ValidateAndNormaliseHostFlag(host string) (string, error) {
 
 	return "", fmt.Errorf("could not resolve host: %s", host)
 }
+
+// parseConfigMapRef parses a ConfigMap reference in the format "namespace/configmap-name"
+func parseConfigMapRef(ref string) (namespace, name string, err error) {
+	parts := strings.SplitN(ref, "/", 2)
+	if len(parts) != 2 {
+		return "", "", fmt.Errorf("expected format 'namespace/configmap-name', got '%s'", ref)
+	}
+
+	namespace = strings.TrimSpace(parts[0])
+	name = strings.TrimSpace(parts[1])
+
+	if namespace == "" {
+		return "", "", fmt.Errorf("namespace cannot be empty")
+	}
+	if name == "" {
+		return "", "", fmt.Errorf("configmap name cannot be empty")
+	}
+
+	return namespace, name, nil
+}
+
+// identifyAndReadConfigMap identifies and reads a ConfigMap to validate its existence and contents
+// This function only validates the ConfigMap but does not use it for configuration
+func identifyAndReadConfigMap(ctx context.Context, configMapRef string) error {
+	// Parse namespace and ConfigMap name
+	namespace, configMapName, err := parseConfigMapRef(configMapRef)
+	if err != nil {
+		return fmt.Errorf("invalid --from-configmap format: %w", err)
+	}
+
+	logger.Infof("Identifying ConfigMap '%s/%s'", namespace, configMapName)
+
+	// Create in-cluster Kubernetes client
+	config, err := rest.InClusterConfig()
+	if err != nil {
+		return fmt.Errorf("failed to create in-cluster config: %w", err)
+	}
+
+	clientset, err := kubernetes.NewForConfig(config)
+	if err != nil {
+		return fmt.Errorf("failed to create Kubernetes client: %w", err)
+	}
+
+	// Get the ConfigMap
+	configMap, err := clientset.CoreV1().ConfigMaps(namespace).Get(ctx, configMapName, metav1.GetOptions{})
+	if err != nil {
+		return fmt.Errorf("failed to get ConfigMap '%s/%s': %w", namespace, configMapName, err)
+	}
+
+	// Validate that runconfig.json exists in the ConfigMap
+	runConfigJSON, ok := configMap.Data["runconfig.json"]
+	if !ok {
+		return fmt.Errorf("ConfigMap '%s/%s' does not contain 'runconfig.json' key", namespace, configMapName)
+	}
+
+	// Validate that the JSON is parseable (but don't use it)
+	var runConfig runner.RunConfig
+	if err := json.Unmarshal([]byte(runConfigJSON), &runConfig); err != nil {
+		return fmt.Errorf("ConfigMap '%s/%s' contains invalid runconfig.json: %w", namespace, configMapName, err)
+	}
+
+	logger.Infof("Successfully identified and validated ConfigMap '%s/%s' (contains %d bytes of runconfig.json)",
+		namespace, configMapName, len(runConfigJSON))
+
+	return nil
+}