Skip to content

Commit eb5430a

Browse files
taskbottaskbot
committed
changes from review
1 parent d59dd3b commit eb5430a

File tree

2 files changed

+305
-1
lines changed

2 files changed

+305
-1
lines changed

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

Lines changed: 304 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1070,3 +1070,307 @@ func TestConverter_IncomingAuthRequired(t *testing.T) {
10701070
})
10711071
}
10721072
}
1073+
1074+
// Test helpers for MCPToolConfig tests
1075+
func newMCPToolConfig(name, namespace string, filter []string, overrides map[string]mcpv1alpha1.ToolOverride) *mcpv1alpha1.MCPToolConfig {
1076+
return &mcpv1alpha1.MCPToolConfig{
1077+
ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace},
1078+
Spec: mcpv1alpha1.MCPToolConfigSpec{ToolsFilter: filter, ToolsOverride: overrides},
1079+
}
1080+
}
1081+
1082+
func toolOverride(name, desc string) mcpv1alpha1.ToolOverride {
1083+
return mcpv1alpha1.ToolOverride{Name: name, Description: desc}
1084+
}
1085+
1086+
func vmcpToolOverride(name, desc string) *vmcpconfig.ToolOverride {
1087+
return &vmcpconfig.ToolOverride{Name: name, Description: desc}
1088+
}
1089+
1090+
func TestResolveMCPToolConfig(t *testing.T) {
1091+
t.Parallel()
1092+
1093+
ns := "test-ns"
1094+
tests := []struct {
1095+
name string
1096+
configName string
1097+
existing *mcpv1alpha1.MCPToolConfig
1098+
expectError bool
1099+
}{
1100+
{
1101+
name: "successfully resolve existing MCPToolConfig",
1102+
configName: "test-config",
1103+
existing: newMCPToolConfig("test-config", ns, []string{"tool1", "tool2"}, nil),
1104+
},
1105+
{
1106+
name: "error when MCPToolConfig not found",
1107+
configName: "nonexistent",
1108+
expectError: true,
1109+
},
1110+
{
1111+
name: "successfully resolve with overrides",
1112+
configName: "config-with-overrides",
1113+
existing: newMCPToolConfig("config-with-overrides", ns, []string{"fetch"},
1114+
map[string]mcpv1alpha1.ToolOverride{"fetch": toolOverride("renamed_fetch", "Renamed tool")}),
1115+
},
1116+
}
1117+
1118+
for _, tt := range tests {
1119+
t.Run(tt.name, func(t *testing.T) {
1120+
t.Parallel()
1121+
1122+
var k8sClient client.Client
1123+
if tt.existing != nil {
1124+
k8sClient = newTestK8sClient(t, tt.existing)
1125+
} else {
1126+
k8sClient = newTestK8sClient(t)
1127+
}
1128+
1129+
converter := newTestConverter(t, newNoOpMockResolver(t))
1130+
converter.k8sClient = k8sClient
1131+
1132+
result, err := converter.resolveMCPToolConfig(context.Background(), ns, tt.configName)
1133+
1134+
if tt.expectError {
1135+
assert.Error(t, err)
1136+
assert.Nil(t, result)
1137+
} else {
1138+
assert.NoError(t, err)
1139+
assert.NotNil(t, result)
1140+
assert.Equal(t, tt.existing.Spec, result.Spec)
1141+
}
1142+
})
1143+
}
1144+
}
1145+
1146+
func TestMergeToolConfigFilter(t *testing.T) {
1147+
t.Parallel()
1148+
1149+
tests := []struct {
1150+
name string
1151+
existing []string
1152+
config *mcpv1alpha1.MCPToolConfig
1153+
expected []string
1154+
}{
1155+
{
1156+
name: "merge when workload has none",
1157+
existing: nil,
1158+
config: newMCPToolConfig("", "", []string{"tool1", "tool2"}, nil),
1159+
expected: []string{"tool1", "tool2"},
1160+
},
1161+
{
1162+
name: "inline takes precedence",
1163+
existing: []string{"inline_tool"},
1164+
config: newMCPToolConfig("", "", []string{"config_tool"}, nil),
1165+
expected: []string{"inline_tool"},
1166+
},
1167+
{
1168+
name: "no change when config has no filter",
1169+
existing: []string{"existing_tool"},
1170+
config: newMCPToolConfig("", "", nil, nil),
1171+
expected: []string{"existing_tool"},
1172+
},
1173+
{
1174+
name: "empty filter from config",
1175+
existing: nil,
1176+
config: newMCPToolConfig("", "", []string{}, nil),
1177+
expected: nil,
1178+
},
1179+
}
1180+
1181+
for _, tt := range tests {
1182+
t.Run(tt.name, func(t *testing.T) {
1183+
t.Parallel()
1184+
1185+
wtc := &vmcpconfig.WorkloadToolConfig{Filter: tt.existing}
1186+
(&Converter{}).mergeToolConfigFilter(wtc, tt.config)
1187+
1188+
assert.Equal(t, tt.expected, wtc.Filter)
1189+
})
1190+
}
1191+
}
1192+
1193+
func TestMergeToolConfigOverrides(t *testing.T) {
1194+
t.Parallel()
1195+
1196+
tests := []struct {
1197+
name string
1198+
existing map[string]*vmcpconfig.ToolOverride
1199+
config *mcpv1alpha1.MCPToolConfig
1200+
expected map[string]*vmcpconfig.ToolOverride
1201+
}{
1202+
{
1203+
name: "merge when workload has none",
1204+
existing: nil,
1205+
config: newMCPToolConfig("", "", nil, map[string]mcpv1alpha1.ToolOverride{"tool1": toolOverride("renamed_tool1", "Renamed description")}),
1206+
expected: map[string]*vmcpconfig.ToolOverride{"tool1": vmcpToolOverride("renamed_tool1", "Renamed description")},
1207+
},
1208+
{
1209+
name: "inline takes precedence",
1210+
existing: map[string]*vmcpconfig.ToolOverride{"tool1": vmcpToolOverride("inline_name", "Inline description")},
1211+
config: newMCPToolConfig("", "", nil, map[string]mcpv1alpha1.ToolOverride{"tool1": toolOverride("config_name", "Config description")}),
1212+
expected: map[string]*vmcpconfig.ToolOverride{"tool1": vmcpToolOverride("inline_name", "Inline description")},
1213+
},
1214+
{
1215+
name: "merge non-conflicting",
1216+
existing: map[string]*vmcpconfig.ToolOverride{"tool1": vmcpToolOverride("inline_tool1", "Inline description")},
1217+
config: newMCPToolConfig("", "", nil, map[string]mcpv1alpha1.ToolOverride{"tool2": toolOverride("config_tool2", "Config description")}),
1218+
expected: map[string]*vmcpconfig.ToolOverride{
1219+
"tool1": vmcpToolOverride("inline_tool1", "Inline description"),
1220+
"tool2": vmcpToolOverride("config_tool2", "Config description"),
1221+
},
1222+
},
1223+
{
1224+
name: "no change when config has no overrides",
1225+
existing: map[string]*vmcpconfig.ToolOverride{"tool1": vmcpToolOverride("existing_name", "")},
1226+
config: newMCPToolConfig("", "", nil, nil),
1227+
expected: map[string]*vmcpconfig.ToolOverride{"tool1": vmcpToolOverride("existing_name", "")},
1228+
},
1229+
}
1230+
1231+
for _, tt := range tests {
1232+
t.Run(tt.name, func(t *testing.T) {
1233+
t.Parallel()
1234+
1235+
wtc := &vmcpconfig.WorkloadToolConfig{Overrides: tt.existing}
1236+
(&Converter{}).mergeToolConfigOverrides(wtc, tt.config)
1237+
1238+
assert.Equal(t, tt.expected, wtc.Overrides)
1239+
})
1240+
}
1241+
}
1242+
1243+
func TestApplyInlineOverrides(t *testing.T) {
1244+
t.Parallel()
1245+
1246+
tests := []struct {
1247+
name string
1248+
inline map[string]mcpv1alpha1.ToolOverride
1249+
existing map[string]*vmcpconfig.ToolOverride
1250+
expected map[string]*vmcpconfig.ToolOverride
1251+
}{
1252+
{
1253+
name: "apply to empty workload",
1254+
inline: map[string]mcpv1alpha1.ToolOverride{"tool1": toolOverride("renamed_tool1", "Inline description")},
1255+
existing: nil,
1256+
expected: map[string]*vmcpconfig.ToolOverride{"tool1": vmcpToolOverride("renamed_tool1", "Inline description")},
1257+
},
1258+
{
1259+
name: "replace existing",
1260+
inline: map[string]mcpv1alpha1.ToolOverride{"tool1": toolOverride("new_name", "New description")},
1261+
existing: map[string]*vmcpconfig.ToolOverride{"tool1": vmcpToolOverride("old_name", "Old description")},
1262+
expected: map[string]*vmcpconfig.ToolOverride{"tool1": vmcpToolOverride("new_name", "New description")},
1263+
},
1264+
{
1265+
name: "no change when no inline",
1266+
inline: nil,
1267+
existing: map[string]*vmcpconfig.ToolOverride{"tool1": vmcpToolOverride("existing_name", "")},
1268+
expected: map[string]*vmcpconfig.ToolOverride{"tool1": vmcpToolOverride("existing_name", "")},
1269+
},
1270+
{
1271+
name: "multiple overrides",
1272+
inline: map[string]mcpv1alpha1.ToolOverride{
1273+
"tool1": toolOverride("renamed_tool1", "Description 1"),
1274+
"tool2": toolOverride("renamed_tool2", "Description 2"),
1275+
},
1276+
existing: nil,
1277+
expected: map[string]*vmcpconfig.ToolOverride{
1278+
"tool1": vmcpToolOverride("renamed_tool1", "Description 1"),
1279+
"tool2": vmcpToolOverride("renamed_tool2", "Description 2"),
1280+
},
1281+
},
1282+
}
1283+
1284+
for _, tt := range tests {
1285+
t.Run(tt.name, func(t *testing.T) {
1286+
t.Parallel()
1287+
1288+
wtc := &vmcpconfig.WorkloadToolConfig{Overrides: tt.existing}
1289+
toolConfig := mcpv1alpha1.WorkloadToolConfig{Overrides: tt.inline}
1290+
(&Converter{}).applyInlineOverrides(toolConfig, wtc)
1291+
1292+
assert.Equal(t, tt.expected, wtc.Overrides)
1293+
})
1294+
}
1295+
}
1296+
1297+
func TestConvertToolConfigs(t *testing.T) {
1298+
t.Parallel()
1299+
1300+
tests := []struct {
1301+
name string
1302+
tools []mcpv1alpha1.WorkloadToolConfig
1303+
existingConfig *mcpv1alpha1.MCPToolConfig
1304+
expectedWorkload string
1305+
expectedFilter []string
1306+
expectedOverride map[string]*vmcpconfig.ToolOverride
1307+
}{
1308+
{
1309+
name: "inline config only",
1310+
tools: []mcpv1alpha1.WorkloadToolConfig{{
1311+
Workload: "backend1",
1312+
Filter: []string{"tool1", "tool2"},
1313+
Overrides: map[string]mcpv1alpha1.ToolOverride{"tool1": toolOverride("renamed_tool1", "Renamed")},
1314+
}},
1315+
expectedWorkload: "backend1",
1316+
expectedFilter: []string{"tool1", "tool2"},
1317+
expectedOverride: map[string]*vmcpconfig.ToolOverride{"tool1": vmcpToolOverride("renamed_tool1", "Renamed")},
1318+
},
1319+
{
1320+
name: "with MCPToolConfig reference",
1321+
tools: []mcpv1alpha1.WorkloadToolConfig{{
1322+
Workload: "backend1",
1323+
ToolConfigRef: &mcpv1alpha1.ToolConfigRef{Name: "test-config"},
1324+
}},
1325+
existingConfig: newMCPToolConfig("test-config", "default", []string{"fetch"},
1326+
map[string]mcpv1alpha1.ToolOverride{"fetch": toolOverride("renamed_fetch", "Renamed fetch")}),
1327+
expectedWorkload: "backend1",
1328+
expectedFilter: []string{"fetch"},
1329+
expectedOverride: map[string]*vmcpconfig.ToolOverride{"fetch": vmcpToolOverride("renamed_fetch", "Renamed fetch")},
1330+
},
1331+
{
1332+
name: "inline takes precedence",
1333+
tools: []mcpv1alpha1.WorkloadToolConfig{{
1334+
Workload: "backend1",
1335+
Filter: []string{"inline_tool"},
1336+
ToolConfigRef: &mcpv1alpha1.ToolConfigRef{Name: "test-config"},
1337+
Overrides: map[string]mcpv1alpha1.ToolOverride{"fetch": toolOverride("inline_fetch", "Inline override")},
1338+
}},
1339+
existingConfig: newMCPToolConfig("test-config", "default", []string{"config_tool"},
1340+
map[string]mcpv1alpha1.ToolOverride{"fetch": toolOverride("config_fetch", "Config override")}),
1341+
expectedWorkload: "backend1",
1342+
expectedFilter: []string{"inline_tool"},
1343+
expectedOverride: map[string]*vmcpconfig.ToolOverride{"fetch": vmcpToolOverride("inline_fetch", "Inline override")},
1344+
},
1345+
}
1346+
1347+
for _, tt := range tests {
1348+
t.Run(tt.name, func(t *testing.T) {
1349+
t.Parallel()
1350+
1351+
ctx := log.IntoContext(context.Background(), logr.Discard())
1352+
var k8sClient client.Client
1353+
if tt.existingConfig != nil {
1354+
k8sClient = newTestK8sClient(t, tt.existingConfig)
1355+
} else {
1356+
k8sClient = newTestK8sClient(t)
1357+
}
1358+
1359+
converter := newTestConverter(t, newNoOpMockResolver(t))
1360+
converter.k8sClient = k8sClient
1361+
1362+
vmcp := &mcpv1alpha1.VirtualMCPServer{
1363+
ObjectMeta: metav1.ObjectMeta{Name: "test-vmcp", Namespace: "default"},
1364+
Spec: mcpv1alpha1.VirtualMCPServerSpec{Aggregation: &mcpv1alpha1.AggregationConfig{Tools: tt.tools}},
1365+
}
1366+
1367+
agg := &vmcpconfig.AggregationConfig{}
1368+
converter.convertToolConfigs(ctx, vmcp, agg)
1369+
1370+
require.Len(t, agg.Tools, 1)
1371+
assert.Equal(t, tt.expectedWorkload, agg.Tools[0].Workload)
1372+
assert.Equal(t, tt.expectedFilter, agg.Tools[0].Filter)
1373+
assert.Equal(t, tt.expectedOverride, agg.Tools[0].Overrides)
1374+
})
1375+
}
1376+
}

‎test/e2e/thv-operator/virtualmcp/virtualmcp_toolconfig_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ var _ = Describe("VirtualMCPServer Tool Filtering via MCPToolConfig", Ordered, f
173173
GinkgoWriter.Printf(" Exposed tool: %s - %s\n", tool.Name, tool.Description)
174174
}
175175

176-
// Verify filtering: should only have echo tool from backend1 (renamed to renamed_echo)
176+
// Verify filtering: should only have fetch tool from backend1 (renamed to renamed_fetch)
177177
toolNames := make([]string, len(tools.Tools))
178178
for i, tool := range tools.Tools {
179179
toolNames[i] = tool.Name

0 commit comments

Comments
 (0)