fix and test mcptoolconfig for vmcp

Add proper reconciliation, dynamic updates and overrides support
Add tests to validate it

Author: taskbot

cmd/thv-operator/controllers/virtualmcpserver_controller.go

@@ -834,7 +834,7 @@ func (r *VirtualMCPServerReconciler) podTemplateSpecNeedsUpdate(
 	}
 
 	// Generate a fresh deployment with PodTemplateSpec applied
-	expectedDeployment := r.deploymentForVirtualMCPServer(ctx, vmcp, vmcpConfigChecksum)
+	expectedDeployment := r.deploymentForVirtualMCPServer(ctx, vmcp, vmcpConfigChecksum, []string{})
 	if expectedDeployment == nil {
 		// If we can't generate expected deployment, assume update is needed
 		return true

cmd/thv-operator/controllers/virtualmcpserver_podtemplatespec_reconcile_test.go

@@ -89,8 +89,8 @@ func TestVirtualMCPServerPodTemplateSpecDeterministic(t *testing.T) {
 	}
 
 	// Generate deployment twice with same input
-	dep1 := reconciler.deploymentForVirtualMCPServer(context.Background(), vmcp, "test-checksum")
-	dep2 := reconciler.deploymentForVirtualMCPServer(context.Background(), vmcp, "test-checksum")
+	dep1 := reconciler.deploymentForVirtualMCPServer(context.Background(), vmcp, "test-checksum", []string{})
+	dep2 := reconciler.deploymentForVirtualMCPServer(context.Background(), vmcp, "test-checksum", []string{})
 
 	// Both should be non-nil
 	assert.NotNil(t, dep1, "First deployment should not be nil")
@@ -219,7 +219,7 @@ func TestVirtualMCPServerPodTemplateSpecNeedsUpdate(t *testing.T) {
 			}
 
 			// Generate existing deployment using the reconciler (this ensures we have a real deployment structure)
-			existingDeployment := reconciler.deploymentForVirtualMCPServer(context.Background(), initialVmcp, "test-checksum")
+			existingDeployment := reconciler.deploymentForVirtualMCPServer(context.Background(), initialVmcp, "test-checksum", []string{})
 			assert.NotNil(t, existingDeployment, "Should generate existing deployment")
 
 			// Create VirtualMCPServer with new PodTemplateSpec

cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig.go

@@ -31,8 +31,8 @@ func (r *VirtualMCPServerReconciler) ensureVmcpConfigConfigMap(
 	// 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)
+	// Convert CRD to vmcp config using converter with OIDC resolver and k8s client
+	converter, err := vmcpconfig.NewConverter(oidcResolver, r.Client)
 	if err != nil {
 		return fmt.Errorf("failed to create vmcp converter: %w", err)
 	}

cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go

@@ -47,7 +47,11 @@ func newNoOpMockResolver(t *testing.T) *oidcmocks.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)
+	// Create a fake k8s client for testing
+	scheme := runtime.NewScheme()
+	_ = mcpv1alpha1.AddToScheme(scheme)
+	fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build()
+	converter, err := vmcpconfig.NewConverter(resolver, fakeClient)
 	require.NoError(t, err)
 	return converter
 }

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

@@ -7,6 +7,9 @@ import (
 	"fmt"
 	"time"
 
+	"github.com/go-logr/logr"
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 
 	mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1"
@@ -29,18 +32,24 @@ const (
 // Converter converts VirtualMCPServer CRD specs to vmcp Config
 type Converter struct {
 	oidcResolver oidc.Resolver
+	k8sClient    client.Client
 }
 
 // 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) {
+// k8sClient is required for resolving MCPToolConfig references.
+// Returns an error if oidcResolver or k8sClient is nil.
+func NewConverter(oidcResolver oidc.Resolver, k8sClient client.Client) (*Converter, error) {
 	if oidcResolver == nil {
 		return nil, fmt.Errorf("oidcResolver is required")
 	}
+	if k8sClient == nil {
+		return nil, fmt.Errorf("k8sClient is required")
+	}
 	return &Converter{
 		oidcResolver: oidcResolver,
+		k8sClient:    k8sClient,
 	}, nil
 }
 
@@ -243,12 +252,23 @@ func (*Converter) convertBackendAuthConfig(
 }
 
 // convertAggregation converts AggregationConfig from CRD to vmcp config
-func (*Converter) convertAggregation(
-	_ context.Context,
+func (c *Converter) convertAggregation(
+	ctx context.Context,
 	vmcp *mcpv1alpha1.VirtualMCPServer,
 ) *vmcpconfig.AggregationConfig {
 	agg := &vmcpconfig.AggregationConfig{}
 
+	c.convertConflictResolution(vmcp, agg)
+	c.convertToolConfigs(ctx, vmcp, agg)
+
+	return agg
+}
+
+// convertConflictResolution converts conflict resolution strategy and config
+func (*Converter) convertConflictResolution(
+	vmcp *mcpv1alpha1.VirtualMCPServer,
+	agg *vmcpconfig.AggregationConfig,
+) {
 	// Convert conflict resolution strategy
 	switch vmcp.Spec.Aggregation.ConflictResolution {
 	case mcpv1alpha1.ConflictResolutionPrefix:
@@ -273,32 +293,131 @@ func (*Converter) convertAggregation(
 			PrefixFormat: "{workload}_",
 		}
 	}
+}
 
-	// Convert per-workload tool configs
-	if len(vmcp.Spec.Aggregation.Tools) > 0 {
-		agg.Tools = make([]*vmcpconfig.WorkloadToolConfig, 0, len(vmcp.Spec.Aggregation.Tools))
-		for _, toolConfig := range vmcp.Spec.Aggregation.Tools {
-			wtc := &vmcpconfig.WorkloadToolConfig{
-				Workload: toolConfig.Workload,
-				Filter:   toolConfig.Filter,
-			}
+// convertToolConfigs converts per-workload tool configurations
+func (c *Converter) convertToolConfigs(
+	ctx context.Context,
+	vmcp *mcpv1alpha1.VirtualMCPServer,
+	agg *vmcpconfig.AggregationConfig,
+) {
+	if len(vmcp.Spec.Aggregation.Tools) == 0 {
+		return
+	}
 
-			// Convert overrides
-			if len(toolConfig.Overrides) > 0 {
-				wtc.Overrides = make(map[string]*vmcpconfig.ToolOverride)
-				for toolName, override := range toolConfig.Overrides {
-					wtc.Overrides[toolName] = &vmcpconfig.ToolOverride{
-						Name:        override.Name,
-						Description: override.Description,
-					}
-				}
+	ctxLogger := log.FromContext(ctx)
+	agg.Tools = make([]*vmcpconfig.WorkloadToolConfig, 0, len(vmcp.Spec.Aggregation.Tools))
+
+	for _, toolConfig := range vmcp.Spec.Aggregation.Tools {
+		wtc := &vmcpconfig.WorkloadToolConfig{
+			Workload: toolConfig.Workload,
+			Filter:   toolConfig.Filter,
+		}
+
+		c.applyToolConfigRef(ctx, ctxLogger, vmcp, toolConfig, wtc)
+		c.applyInlineOverrides(toolConfig, wtc)
+
+		agg.Tools = append(agg.Tools, wtc)
+	}
+}
+
+// applyToolConfigRef resolves and applies MCPToolConfig reference
+func (c *Converter) applyToolConfigRef(
+	ctx context.Context,
+	ctxLogger logr.Logger,
+	vmcp *mcpv1alpha1.VirtualMCPServer,
+	toolConfig mcpv1alpha1.WorkloadToolConfig,
+	wtc *vmcpconfig.WorkloadToolConfig,
+) {
+	if toolConfig.ToolConfigRef == nil {
+		return
+	}
+
+	resolvedConfig, err := c.resolveMCPToolConfig(ctx, vmcp.Namespace, toolConfig.ToolConfigRef.Name)
+	if err != nil {
+		ctxLogger.Error(err, "failed to resolve MCPToolConfig reference",
+			"workload", toolConfig.Workload,
+			"toolConfigRef", toolConfig.ToolConfigRef.Name)
+		return
+	}
+
+	if resolvedConfig == nil {
+		return
+	}
+
+	c.mergeToolConfigFilter(wtc, resolvedConfig)
+	c.mergeToolConfigOverrides(wtc, resolvedConfig)
+}
+
+// mergeToolConfigFilter merges filter from MCPToolConfig
+func (*Converter) mergeToolConfigFilter(
+	wtc *vmcpconfig.WorkloadToolConfig,
+	resolvedConfig *mcpv1alpha1.MCPToolConfig,
+) {
+	if len(wtc.Filter) == 0 && len(resolvedConfig.Spec.ToolsFilter) > 0 {
+		wtc.Filter = resolvedConfig.Spec.ToolsFilter
+	}
+}
+
+// mergeToolConfigOverrides merges overrides from MCPToolConfig
+func (*Converter) mergeToolConfigOverrides(
+	wtc *vmcpconfig.WorkloadToolConfig,
+	resolvedConfig *mcpv1alpha1.MCPToolConfig,
+) {
+	if len(resolvedConfig.Spec.ToolsOverride) == 0 {
+		return
+	}
+
+	if wtc.Overrides == nil {
+		wtc.Overrides = make(map[string]*vmcpconfig.ToolOverride)
+	}
+
+	for toolName, override := range resolvedConfig.Spec.ToolsOverride {
+		if _, exists := wtc.Overrides[toolName]; !exists {
+			wtc.Overrides[toolName] = &vmcpconfig.ToolOverride{
+				Name:        override.Name,
+				Description: override.Description,
 			}
+		}
+	}
+}
+
+// applyInlineOverrides applies inline tool overrides
+func (*Converter) applyInlineOverrides(
+	toolConfig mcpv1alpha1.WorkloadToolConfig,
+	wtc *vmcpconfig.WorkloadToolConfig,
+) {
+	if len(toolConfig.Overrides) == 0 {
+		return
+	}
 
-			agg.Tools = append(agg.Tools, wtc)
+	if wtc.Overrides == nil {
+		wtc.Overrides = make(map[string]*vmcpconfig.ToolOverride)
+	}
+
+	for toolName, override := range toolConfig.Overrides {
+		wtc.Overrides[toolName] = &vmcpconfig.ToolOverride{
+			Name:        override.Name,
+			Description: override.Description,
 		}
 	}
+}
 
-	return agg
+// resolveMCPToolConfig fetches an MCPToolConfig resource by name and namespace
+func (c *Converter) resolveMCPToolConfig(
+	ctx context.Context,
+	namespace string,
+	name string,
+) (*mcpv1alpha1.MCPToolConfig, error) {
+	toolConfig := &mcpv1alpha1.MCPToolConfig{}
+	err := c.k8sClient.Get(ctx, types.NamespacedName{
+		Name:      name,
+		Namespace: namespace,
+	}, toolConfig)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get MCPToolConfig %s/%s: %w", namespace, name, err)
+	}
+	return toolConfig, nil
 }
 
 // convertCompositeTools converts CompositeToolSpec from CRD to vmcp config

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

@@ -13,6 +13,8 @@ import (
 	"go.uber.org/mock/gomock"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 
 	mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1"
@@ -36,10 +38,19 @@ func newNoOpMockResolver(t *testing.T) *oidcmocks.MockResolver {
 	return mockResolver
 }
 
+// newTestK8sClient creates a fake Kubernetes client for testing.
+func newTestK8sClient(t *testing.T, objects ...client.Object) client.Client {
+	t.Helper()
+	scheme := runtime.NewScheme()
+	require.NoError(t, mcpv1alpha1.AddToScheme(scheme))
+	return fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build()
+}
+
 // newTestConverter creates a Converter with the given resolver, failing the test if creation fails.
 func newTestConverter(t *testing.T, resolver oidc.Resolver) *Converter {
 	t.Helper()
-	converter, err := NewConverter(resolver)
+	k8sClient := newTestK8sClient(t)
+	converter, err := NewConverter(resolver, k8sClient)
 	require.NoError(t, err)
 	return converter
 }