fix and test mcptoolconfig for vmcp (#2891)

* fix and test mcptoolconfig for vmcp

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

* changes from review

* fixes from review

---------

Co-authored-by: taskbot <taskbot@users.noreply.github.com>
Co-authored-by: Don Browne <dmjb@users.noreply.github.com>

Author: 3 people

cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go

@@ -52,9 +52,7 @@ func newTestConverter(t *testing.T, resolver *oidcmocks.MockResolver) *vmcpconfi
 	t.Helper()
 	scheme := runtime.NewScheme()
 	_ = mcpv1alpha1.AddToScheme(scheme)
-	fakeClient := fake.NewClientBuilder().
-		WithScheme(scheme).
-		Build()
+	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,7 @@ import (
 	"fmt"
 	"time"
 
+	"github.com/go-logr/logr"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
@@ -40,7 +41,8 @@ type Converter struct {
 // 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.
-// k8sClient is required and used to fetch referenced VirtualMCPCompositeToolDefinition resources.
+// k8sClient is required for resolving MCPToolConfig references and fetching referenced
+// VirtualMCPCompositeToolDefinition resources.
 // Returns an error if oidcResolver or k8sClient is nil.
 func NewConverter(oidcResolver oidc.Resolver, k8sClient client.Client) (*Converter, error) {
 	if oidcResolver == nil {
@@ -86,7 +88,11 @@ func (c *Converter) Convert(
 
 	// Convert Aggregation - always set with defaults if not specified
 	if vmcp.Spec.Aggregation != nil {
-		config.Aggregation = c.convertAggregation(ctx, vmcp)
+		agg, err := c.convertAggregation(ctx, vmcp)
+		if err != nil {
+			return nil, fmt.Errorf("failed to convert aggregation config: %w", err)
+		}
+		config.Aggregation = agg
 	} else {
 		// Provide default aggregation config with prefix conflict resolution
 		config.Aggregation = &vmcpconfig.AggregationConfig{
@@ -260,12 +266,25 @@ 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 {
+) (*vmcpconfig.AggregationConfig, error) {
 	agg := &vmcpconfig.AggregationConfig{}
 
+	c.convertConflictResolution(vmcp, agg)
+	if err := c.convertToolConfigs(ctx, vmcp, agg); err != nil {
+		return nil, err
+	}
+
+	return agg, nil
+}
+
+// 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:
@@ -290,32 +309,137 @@ 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,
+) error {
+	if len(vmcp.Spec.Aggregation.Tools) == 0 {
+		return nil
+	}
 
-			// 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,
+		}
+
+		if err := c.applyToolConfigRef(ctx, ctxLogger, vmcp, toolConfig, wtc); err != nil {
+			return err
+		}
+		c.applyInlineOverrides(toolConfig, wtc)
+
+		agg.Tools = append(agg.Tools, wtc)
+	}
+	return nil
+}
+
+// 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,
+) error {
+	if toolConfig.ToolConfigRef == nil {
+		return nil
+	}
+
+	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)
+		// Fail closed: return error when MCPToolConfig is configured but resolution fails
+		// This prevents deploying without tool filtering when explicit configuration is requested
+		return fmt.Errorf("MCPToolConfig resolution failed for %q: %w",
+			toolConfig.ToolConfigRef.Name, err)
+	}
+
+	// Note: resolveMCPToolConfig never returns (nil, nil) - it either succeeds with
+	// (toolConfig, nil) or fails with (nil, error), so no nil check needed here
+
+	c.mergeToolConfigFilter(wtc, resolvedConfig)
+	c.mergeToolConfigOverrides(wtc, resolvedConfig)
+	return nil
+}
+
+// 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
+	}
+
+	if wtc.Overrides == nil {
+		wtc.Overrides = make(map[string]*vmcpconfig.ToolOverride)
+	}
 
-			agg.Tools = append(agg.Tools, wtc)
+	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