Skip to content

Commit b7eb620

Browse files
taskbottaskbot
committed
fixes from review
1 parent 8d52325 commit b7eb620

File tree

2 files changed

+192
-14
lines changed

2 files changed

+192
-14
lines changed

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

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,11 @@ func (c *Converter) Convert(
8484

8585
// Convert Aggregation - always set with defaults if not specified
8686
if vmcp.Spec.Aggregation != nil {
87-
config.Aggregation = c.convertAggregation(ctx, vmcp)
87+
agg, err := c.convertAggregation(ctx, vmcp)
88+
if err != nil {
89+
return nil, fmt.Errorf("failed to convert aggregation config: %w", err)
90+
}
91+
config.Aggregation = agg
8892
} else {
8993
// Provide default aggregation config with prefix conflict resolution
9094
config.Aggregation = &vmcpconfig.AggregationConfig{
@@ -255,13 +259,15 @@ func (*Converter) convertBackendAuthConfig(
255259
func (c *Converter) convertAggregation(
256260
ctx context.Context,
257261
vmcp *mcpv1alpha1.VirtualMCPServer,
258-
) *vmcpconfig.AggregationConfig {
262+
) (*vmcpconfig.AggregationConfig, error) {
259263
agg := &vmcpconfig.AggregationConfig{}
260264

261265
c.convertConflictResolution(vmcp, agg)
262-
c.convertToolConfigs(ctx, vmcp, agg)
266+
if err := c.convertToolConfigs(ctx, vmcp, agg); err != nil {
267+
return nil, err
268+
}
263269

264-
return agg
270+
return agg, nil
265271
}
266272

267273
// convertConflictResolution converts conflict resolution strategy and config
@@ -300,9 +306,9 @@ func (c *Converter) convertToolConfigs(
300306
ctx context.Context,
301307
vmcp *mcpv1alpha1.VirtualMCPServer,
302308
agg *vmcpconfig.AggregationConfig,
303-
) {
309+
) error {
304310
if len(vmcp.Spec.Aggregation.Tools) == 0 {
305-
return
311+
return nil
306312
}
307313

308314
ctxLogger := log.FromContext(ctx)
@@ -314,11 +320,14 @@ func (c *Converter) convertToolConfigs(
314320
Filter: toolConfig.Filter,
315321
}
316322

317-
c.applyToolConfigRef(ctx, ctxLogger, vmcp, toolConfig, wtc)
323+
if err := c.applyToolConfigRef(ctx, ctxLogger, vmcp, toolConfig, wtc); err != nil {
324+
return err
325+
}
318326
c.applyInlineOverrides(toolConfig, wtc)
319327

320328
agg.Tools = append(agg.Tools, wtc)
321329
}
330+
return nil
322331
}
323332

324333
// applyToolConfigRef resolves and applies MCPToolConfig reference
@@ -328,25 +337,28 @@ func (c *Converter) applyToolConfigRef(
328337
vmcp *mcpv1alpha1.VirtualMCPServer,
329338
toolConfig mcpv1alpha1.WorkloadToolConfig,
330339
wtc *vmcpconfig.WorkloadToolConfig,
331-
) {
340+
) error {
332341
if toolConfig.ToolConfigRef == nil {
333-
return
342+
return nil
334343
}
335344

336345
resolvedConfig, err := c.resolveMCPToolConfig(ctx, vmcp.Namespace, toolConfig.ToolConfigRef.Name)
337346
if err != nil {
338347
ctxLogger.Error(err, "failed to resolve MCPToolConfig reference",
339348
"workload", toolConfig.Workload,
340349
"toolConfigRef", toolConfig.ToolConfigRef.Name)
341-
return
350+
// Fail closed: return error when MCPToolConfig is configured but resolution fails
351+
// This prevents deploying without tool filtering when explicit configuration is requested
352+
return fmt.Errorf("MCPToolConfig resolution failed for %q: %w",
353+
toolConfig.ToolConfigRef.Name, err)
342354
}
343355

344-
if resolvedConfig == nil {
345-
return
346-
}
356+
// Note: resolveMCPToolConfig never returns (nil, nil) - it either succeeds with
357+
// (toolConfig, nil) or fails with (nil, error), so no nil check needed here
347358

348359
c.mergeToolConfigFilter(wtc, resolvedConfig)
349360
c.mergeToolConfigOverrides(wtc, resolvedConfig)
361+
return nil
350362
}
351363

352364
// mergeToolConfigFilter merges filter from MCPToolConfig

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

Lines changed: 167 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1365,12 +1365,178 @@ func TestConvertToolConfigs(t *testing.T) {
13651365
}
13661366

13671367
agg := &vmcpconfig.AggregationConfig{}
1368-
converter.convertToolConfigs(ctx, vmcp, agg)
1368+
err := converter.convertToolConfigs(ctx, vmcp, agg)
13691369

1370+
require.NoError(t, err)
13701371
require.Len(t, agg.Tools, 1)
13711372
assert.Equal(t, tt.expectedWorkload, agg.Tools[0].Workload)
13721373
assert.Equal(t, tt.expectedFilter, agg.Tools[0].Filter)
13731374
assert.Equal(t, tt.expectedOverride, agg.Tools[0].Overrides)
13741375
})
13751376
}
13761377
}
1378+
1379+
// TestConvertToolConfigs_FailClosed tests that MCPToolConfig resolution errors cause conversion to fail.
1380+
// This is a security feature: if a user explicitly references an MCPToolConfig (for tool filtering or
1381+
// security policy enforcement), we should fail rather than deploy without the intended configuration.
1382+
func TestConvertToolConfigs_FailClosed(t *testing.T) {
1383+
t.Parallel()
1384+
1385+
tests := []struct {
1386+
name string
1387+
tools []mcpv1alpha1.WorkloadToolConfig
1388+
existingConfig *mcpv1alpha1.MCPToolConfig
1389+
expectError bool
1390+
expectedErrMsg string
1391+
}{
1392+
{
1393+
name: "error when MCPToolConfig reference not found (fail closed)",
1394+
tools: []mcpv1alpha1.WorkloadToolConfig{{
1395+
Workload: "backend1",
1396+
ToolConfigRef: &mcpv1alpha1.ToolConfigRef{Name: "nonexistent-config"},
1397+
}},
1398+
existingConfig: nil, // MCPToolConfig doesn't exist in cluster
1399+
expectError: true,
1400+
expectedErrMsg: "MCPToolConfig resolution failed for \"nonexistent-config\"",
1401+
},
1402+
{
1403+
name: "no error when no ToolConfigRef specified",
1404+
tools: []mcpv1alpha1.WorkloadToolConfig{{
1405+
Workload: "backend1",
1406+
Filter: []string{"tool1"},
1407+
}},
1408+
existingConfig: nil,
1409+
expectError: false,
1410+
},
1411+
{
1412+
name: "successful when MCPToolConfig exists",
1413+
tools: []mcpv1alpha1.WorkloadToolConfig{{
1414+
Workload: "backend1",
1415+
ToolConfigRef: &mcpv1alpha1.ToolConfigRef{Name: "valid-config"},
1416+
}},
1417+
existingConfig: newMCPToolConfig("valid-config", "default", []string{"fetch"}, nil),
1418+
expectError: false,
1419+
},
1420+
}
1421+
1422+
for _, tt := range tests {
1423+
t.Run(tt.name, func(t *testing.T) {
1424+
t.Parallel()
1425+
1426+
ctx := log.IntoContext(context.Background(), logr.Discard())
1427+
var k8sClient client.Client
1428+
if tt.existingConfig != nil {
1429+
k8sClient = newTestK8sClient(t, tt.existingConfig)
1430+
} else {
1431+
k8sClient = newTestK8sClient(t)
1432+
}
1433+
1434+
converter := newTestConverter(t, newNoOpMockResolver(t))
1435+
converter.k8sClient = k8sClient
1436+
1437+
vmcp := &mcpv1alpha1.VirtualMCPServer{
1438+
ObjectMeta: metav1.ObjectMeta{Name: "test-vmcp", Namespace: "default"},
1439+
Spec: mcpv1alpha1.VirtualMCPServerSpec{Aggregation: &mcpv1alpha1.AggregationConfig{Tools: tt.tools}},
1440+
}
1441+
1442+
agg := &vmcpconfig.AggregationConfig{}
1443+
err := converter.convertToolConfigs(ctx, vmcp, agg)
1444+
1445+
if tt.expectError {
1446+
require.Error(t, err)
1447+
assert.Contains(t, err.Error(), tt.expectedErrMsg)
1448+
} else {
1449+
require.NoError(t, err)
1450+
}
1451+
})
1452+
}
1453+
}
1454+
1455+
// TestConvert_MCPToolConfigFailClosed tests that MCPToolConfig resolution errors propagate through
1456+
// the full Convert() method and prevent VirtualMCPServer deployment.
1457+
func TestConvert_MCPToolConfigFailClosed(t *testing.T) {
1458+
t.Parallel()
1459+
1460+
tests := []struct {
1461+
name string
1462+
vmcp *mcpv1alpha1.VirtualMCPServer
1463+
existingConfig *mcpv1alpha1.MCPToolConfig
1464+
expectError bool
1465+
expectedErrMsg string
1466+
}{
1467+
{
1468+
name: "Convert fails when MCPToolConfig not found",
1469+
vmcp: &mcpv1alpha1.VirtualMCPServer{
1470+
ObjectMeta: metav1.ObjectMeta{Name: "test-vmcp", Namespace: "default"},
1471+
Spec: mcpv1alpha1.VirtualMCPServerSpec{
1472+
GroupRef: mcpv1alpha1.GroupRef{Name: "test-group"},
1473+
Aggregation: &mcpv1alpha1.AggregationConfig{
1474+
Tools: []mcpv1alpha1.WorkloadToolConfig{{
1475+
Workload: "backend1",
1476+
ToolConfigRef: &mcpv1alpha1.ToolConfigRef{Name: "missing-config"},
1477+
}},
1478+
},
1479+
},
1480+
},
1481+
existingConfig: nil,
1482+
expectError: true,
1483+
expectedErrMsg: "failed to convert aggregation config",
1484+
},
1485+
{
1486+
name: "Convert succeeds when MCPToolConfig exists",
1487+
vmcp: &mcpv1alpha1.VirtualMCPServer{
1488+
ObjectMeta: metav1.ObjectMeta{Name: "test-vmcp", Namespace: "default"},
1489+
Spec: mcpv1alpha1.VirtualMCPServerSpec{
1490+
GroupRef: mcpv1alpha1.GroupRef{Name: "test-group"},
1491+
Aggregation: &mcpv1alpha1.AggregationConfig{
1492+
Tools: []mcpv1alpha1.WorkloadToolConfig{{
1493+
Workload: "backend1",
1494+
ToolConfigRef: &mcpv1alpha1.ToolConfigRef{Name: "valid-config"},
1495+
}},
1496+
},
1497+
},
1498+
},
1499+
existingConfig: newMCPToolConfig("valid-config", "default", []string{"fetch"}, nil),
1500+
expectError: false,
1501+
},
1502+
{
1503+
name: "Convert succeeds when no Aggregation specified",
1504+
vmcp: &mcpv1alpha1.VirtualMCPServer{
1505+
ObjectMeta: metav1.ObjectMeta{Name: "test-vmcp", Namespace: "default"},
1506+
Spec: mcpv1alpha1.VirtualMCPServerSpec{
1507+
GroupRef: mcpv1alpha1.GroupRef{Name: "test-group"},
1508+
},
1509+
},
1510+
existingConfig: nil,
1511+
expectError: false,
1512+
},
1513+
}
1514+
1515+
for _, tt := range tests {
1516+
t.Run(tt.name, func(t *testing.T) {
1517+
t.Parallel()
1518+
1519+
ctx := log.IntoContext(context.Background(), logr.Discard())
1520+
var k8sClient client.Client
1521+
if tt.existingConfig != nil {
1522+
k8sClient = newTestK8sClient(t, tt.existingConfig)
1523+
} else {
1524+
k8sClient = newTestK8sClient(t)
1525+
}
1526+
1527+
converter := newTestConverter(t, newNoOpMockResolver(t))
1528+
converter.k8sClient = k8sClient
1529+
1530+
config, err := converter.Convert(ctx, tt.vmcp)
1531+
1532+
if tt.expectError {
1533+
require.Error(t, err)
1534+
assert.Contains(t, err.Error(), tt.expectedErrMsg)
1535+
assert.Nil(t, config)
1536+
} else {
1537+
require.NoError(t, err)
1538+
assert.NotNil(t, config)
1539+
}
1540+
})
1541+
}
1542+
}

0 commit comments

Comments
 (0)