Fix runtime_config API semantics and validation

Author: slyt3

pkg/api/v1/workload_service.go

@@ -7,17 +7,20 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"log/slog"
+	"strings"
 	"time"
 
-	regtypes "github.com/stacklok/toolhive-core/registry/types"
+	nameref "github.com/google/go-containerregistry/pkg/name"
 	groupval "github.com/stacklok/toolhive-core/validation/group"
 	httpval "github.com/stacklok/toolhive-core/validation/http"
 	"github.com/stacklok/toolhive/pkg/auth/remote"
 	"github.com/stacklok/toolhive/pkg/config"
 	"github.com/stacklok/toolhive/pkg/container/runtime"
+	"github.com/stacklok/toolhive/pkg/container/templates"
 	"github.com/stacklok/toolhive/pkg/groups"
+	"github.com/stacklok/toolhive/pkg/logger"
 	"github.com/stacklok/toolhive/pkg/networking"
+	regtypes "github.com/stacklok/toolhive/pkg/registry/registry"
 	"github.com/stacklok/toolhive/pkg/runner"
 	"github.com/stacklok/toolhive/pkg/runner/retriever"
 	"github.com/stacklok/toolhive/pkg/secrets"
@@ -32,6 +35,24 @@ const (
 	imageRetrievalTimeout = 10 * time.Minute
 )
 
+func isValidRuntimePackageName(pkg string) bool {
+	if pkg == "" {
+		return false
+	}
+	for i, r := range pkg {
+		switch {
+		case r >= 'a' && r <= 'z':
+		case r >= 'A' && r <= 'Z':
+		case r >= '0' && r <= '9':
+		case r == '.', r == '_':
+		case (r == '+' || r == '-') && i > 0:
+		default:
+			return false
+		}
+	}
+	return true
+}
+
 // WorkloadService handles business logic for workload operations
 type WorkloadService struct {
 	workloadManager  workloads.Manager
@@ -73,13 +94,13 @@ func (s *WorkloadService) CreateWorkloadFromRequest(ctx context.Context, req *cr
 
 	// Save the workload state
 	if err := runConfig.SaveState(ctx); err != nil {
-		slog.Error("failed to save workload config", "error", err)
+		logger.Errorf("Failed to save workload config: %v", err)
 		return nil, fmt.Errorf("failed to save workload config: %w", err)
 	}
 
 	// Start workload
 	if err := s.workloadManager.RunWorkloadDetached(ctx, runConfig); err != nil {
-		slog.Error("failed to start workload", "error", err)
+		logger.Errorf("Failed to start workload: %v", err)
 		return nil, fmt.Errorf("failed to start workload: %w", err)
 	}
 
@@ -91,7 +112,7 @@ func (s *WorkloadService) UpdateWorkloadFromRequest(ctx context.Context, name st
 	// If ProxyPort is 0, reuse the existing port
 	if req.ProxyPort == 0 && existingPort > 0 {
 		req.ProxyPort = existingPort
-		slog.Debug("reusing existing port", "port", existingPort, "name", name)
+		logger.Debugf("Reusing existing port %d for workload %s", existingPort, name)
 	}
 
 	// Build the full run config
@@ -162,7 +183,11 @@ func (s *WorkloadService) BuildFullRunConfig(
 	var imageURL string
 	var imageMetadata *regtypes.ImageMetadata
 	var serverMetadata regtypes.ServerMetadata
-	var registryProxyPort int
+	runtimeConfigOverride := runtimeConfigFromRequest(req)
+	retrievalRuntimeConfig, err := runtimeConfigForImageBuild(req, runtimeConfigOverride)
+	if err != nil {
+		return nil, fmt.Errorf("%w: %w", retriever.ErrInvalidRunConfig, err)
+	}
 
 	if req.URL != "" {
 		// Configure remote authentication if OAuth config is provided
@@ -181,8 +206,8 @@ func (s *WorkloadService) BuildFullRunConfig(
 			req.Image,
 			"", // We do not let the user specify a CA cert path here.
 			retriever.VerifyImageWarn,
-			"",  // TODO Add support for registry groups lookups for API
-			nil, // No runtime override from API (yet)
+			"", // TODO Add support for registry groups lookups for API
+			retrievalRuntimeConfig,
 		)
 		if err != nil {
 			// Check if the error is due to context timeout
@@ -193,12 +218,7 @@ func (s *WorkloadService) BuildFullRunConfig(
 			return nil, fmt.Errorf("failed to retrieve MCP server image: %w", err)
 		}
 
-		if remoteServerMetadata, ok := serverMetadata.(*regtypes.RemoteServerMetadata); ok && remoteServerMetadata != nil {
-			// Use registry proxy port if not set by request
-			if req.ProxyPort == 0 && remoteServerMetadata.ProxyPort > 0 {
-				registryProxyPort = remoteServerMetadata.ProxyPort
-			}
-
+		if remoteServerMetadata, ok := serverMetadata.(*regtypes.RemoteServerMetadata); ok {
 			if remoteServerMetadata.OAuthConfig != nil {
 				// Default resource: user-provided > registry metadata > derived from remote URL
 				resource := req.OAuthConfig.Resource
@@ -234,11 +254,8 @@ func (s *WorkloadService) BuildFullRunConfig(
 				}
 			}
 		}
-		// Handle server metadata - API only supports container servers.
-		// Use type assertion with nil check to guard against typed nil pointers.
-		if md, ok := serverMetadata.(*regtypes.ImageMetadata); ok && md != nil {
-			imageMetadata = md
-		}
+		// Handle server metadata - API only supports container servers
+		imageMetadata, _ = serverMetadata.(*regtypes.ImageMetadata)
 	}
 
 	// Build RunConfig
@@ -281,6 +298,11 @@ func (s *WorkloadService) BuildFullRunConfig(
 		runner.WithTelemetryConfigFromFlags("", false, false, false, "", 0.0, nil, false, nil, false),
 	}
 
+	// Runtime overrides only apply to protocol-scheme image builds.
+	if runtimeConfigOverride != nil && req.URL == "" {
+		options = append(options, runner.WithRuntimeConfig(runtimeConfigOverride))
+	}
+
 	// Add header forward configuration if specified
 	if req.HeaderForward != nil {
 		if len(req.HeaderForward.AddPlaintextHeaders) > 0 {
@@ -291,11 +313,6 @@ func (s *WorkloadService) BuildFullRunConfig(
 		}
 	}
 
-	// Use registry proxy port for remote servers if not set by request
-	if registryProxyPort > 0 {
-		options = append(options, runner.WithRegistryProxyPort(registryProxyPort))
-	}
-
 	// Add existing port if provided (for update operations)
 	if existingPort > 0 {
 		options = append(options, runner.WithExistingPort(existingPort))
@@ -305,10 +322,8 @@ func (s *WorkloadService) BuildFullRunConfig(
 	transportType := "streamable-http"
 	if req.Transport != "" {
 		transportType = req.Transport
-	} else if md, ok := serverMetadata.(*regtypes.ImageMetadata); ok && md != nil {
-		if t := md.GetTransport(); t != "" {
-			transportType = t
-		}
+	} else if serverMetadata != nil {
+		transportType = serverMetadata.GetTransport()
 	}
 
 	// Configure middleware from flags
@@ -330,7 +345,7 @@ func (s *WorkloadService) BuildFullRunConfig(
 
 	runConfig, err := runner.NewRunConfigBuilder(ctx, imageMetadata, req.EnvVars, &runner.DetachedEnvVarValidator{}, options...)
 	if err != nil {
-		slog.Error("failed to build run config", "error", err)
+		logger.Errorf("Failed to build run config: %v", err)
 		return nil, fmt.Errorf("%w: Failed to build run config: %w", retriever.ErrInvalidRunConfig, err)
 	}
 
@@ -377,6 +392,80 @@ func createRequestToRemoteAuthConfig(
 	return remoteAuthConfig
 }
 
+func runtimeConfigFromRequest(req *createRequest) *templates.RuntimeConfig {
+	if req == nil || req.RuntimeConfig == nil {
+		return nil
+	}
+
+	runtimeConfig := &templates.RuntimeConfig{}
+	if builderImage := strings.TrimSpace(req.RuntimeConfig.BuilderImage); builderImage != "" {
+		runtimeConfig.BuilderImage = builderImage
+	}
+	if len(req.RuntimeConfig.AdditionalPackages) > 0 {
+		for _, pkg := range req.RuntimeConfig.AdditionalPackages {
+			if trimmedPkg := strings.TrimSpace(pkg); trimmedPkg != "" {
+				runtimeConfig.AdditionalPackages = append(runtimeConfig.AdditionalPackages, trimmedPkg)
+			}
+		}
+	}
+	if runtimeConfig.BuilderImage == "" && len(runtimeConfig.AdditionalPackages) == 0 {
+		return nil
+	}
+
+	return runtimeConfig
+}
+
+func validateRuntimeConfig(runtimeConfig *templates.RuntimeConfig) error {
+	if runtimeConfig == nil {
+		return nil
+	}
+
+	if runtimeConfig.BuilderImage != "" {
+		if _, err := nameref.ParseReference(runtimeConfig.BuilderImage); err != nil {
+			return fmt.Errorf("runtime_config.builder_image must be a valid container image reference")
+		}
+	}
+
+	for _, pkg := range runtimeConfig.AdditionalPackages {
+		if !isValidRuntimePackageName(pkg) {
+			return fmt.Errorf("runtime_config.additional_packages contains invalid package name %q", pkg)
+		}
+	}
+
+	return nil
+}
+
+func runtimeConfigForImageBuild(req *createRequest, runtimeConfigOverride *templates.RuntimeConfig) (*templates.RuntimeConfig, error) {
+	if runtimeConfigOverride == nil || req == nil {
+		return nil, nil
+	}
+	if err := validateRuntimeConfig(runtimeConfigOverride); err != nil {
+		return nil, err
+	}
+	if req.URL != "" || !runner.IsImageProtocolScheme(req.Image) {
+		return nil, fmt.Errorf("runtime_config is only supported for protocol-scheme images")
+	}
+
+	transportType, _, err := runner.ParseProtocolScheme(req.Image)
+	if err != nil {
+		return nil, err
+	}
+
+	baseConfig := runner.GetBaseRuntimeConfig(transportType)
+	merged := &templates.RuntimeConfig{
+		BuilderImage:       baseConfig.BuilderImage,
+		AdditionalPackages: append([]string{}, baseConfig.AdditionalPackages...),
+	}
+	if runtimeConfigOverride.BuilderImage != "" {
+		merged.BuilderImage = runtimeConfigOverride.BuilderImage
+	}
+	if len(runtimeConfigOverride.AdditionalPackages) > 0 {
+		merged.AdditionalPackages = append(merged.AdditionalPackages, runtimeConfigOverride.AdditionalPackages...)
+	}
+
+	return merged, nil
+}
+
 // GetWorkloadNamesFromRequest gets workload names from either the names field or group
 func (s *WorkloadService) GetWorkloadNamesFromRequest(ctx context.Context, req bulkOperationRequest) ([]string, error) {
 	if len(req.Names) > 0 {