Skip to content

Commit d644395

Browse files
taskbottaskbot
committed
fixes from review
1 parent c3fe566 commit d644395

File tree

3 files changed

+193
-16
lines changed

3 files changed

+193
-16
lines changed

‎cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,6 @@ func newNoOpMockResolver(t *testing.T) *oidcmocks.MockResolver {
5050
// newTestConverter creates a Converter with the given resolver, failing the test if creation fails.
5151
func newTestConverter(t *testing.T, resolver *oidcmocks.MockResolver) *vmcpconfig.Converter {
5252
t.Helper()
53-
// Create a fake k8s client for testing
5453
scheme := runtime.NewScheme()
5554
_ = mcpv1alpha1.AddToScheme(scheme)
5655
fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build()

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

Lines changed: 26 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99

1010
"github.com/go-logr/logr"
1111
"k8s.io/apimachinery/pkg/api/errors"
12+
"k8s.io/apimachinery/pkg/runtime"
1213
"k8s.io/apimachinery/pkg/types"
1314
"sigs.k8s.io/controller-runtime/pkg/client"
1415
"sigs.k8s.io/controller-runtime/pkg/log"
@@ -86,7 +87,11 @@ func (c *Converter) Convert(
8687

8788
// Convert Aggregation - always set with defaults if not specified
8889
if vmcp.Spec.Aggregation != nil {
89-
config.Aggregation = c.convertAggregation(ctx, vmcp)
90+
agg, err := c.convertAggregation(ctx, vmcp)
91+
if err != nil {
92+
return nil, fmt.Errorf("failed to convert aggregation config: %w", err)
93+
}
94+
config.Aggregation = agg
9095
} else {
9196
// Provide default aggregation config with prefix conflict resolution
9297
config.Aggregation = &vmcpconfig.AggregationConfig{
@@ -261,13 +266,15 @@ func (*Converter) convertBackendAuthConfig(
261266
func (c *Converter) convertAggregation(
262267
ctx context.Context,
263268
vmcp *mcpv1alpha1.VirtualMCPServer,
264-
) *vmcpconfig.AggregationConfig {
269+
) (*vmcpconfig.AggregationConfig, error) {
265270
agg := &vmcpconfig.AggregationConfig{}
266271

267272
c.convertConflictResolution(vmcp, agg)
268-
c.convertToolConfigs(ctx, vmcp, agg)
273+
if err := c.convertToolConfigs(ctx, vmcp, agg); err != nil {
274+
return nil, err
275+
}
269276

270-
return agg
277+
return agg, nil
271278
}
272279

273280
// convertConflictResolution converts conflict resolution strategy and config
@@ -306,9 +313,9 @@ func (c *Converter) convertToolConfigs(
306313
ctx context.Context,
307314
vmcp *mcpv1alpha1.VirtualMCPServer,
308315
agg *vmcpconfig.AggregationConfig,
309-
) {
316+
) error {
310317
if len(vmcp.Spec.Aggregation.Tools) == 0 {
311-
return
318+
return nil
312319
}
313320

314321
ctxLogger := log.FromContext(ctx)
@@ -320,11 +327,14 @@ func (c *Converter) convertToolConfigs(
320327
Filter: toolConfig.Filter,
321328
}
322329

323-
c.applyToolConfigRef(ctx, ctxLogger, vmcp, toolConfig, wtc)
330+
if err := c.applyToolConfigRef(ctx, ctxLogger, vmcp, toolConfig, wtc); err != nil {
331+
return err
332+
}
324333
c.applyInlineOverrides(toolConfig, wtc)
325334

326335
agg.Tools = append(agg.Tools, wtc)
327336
}
337+
return nil
328338
}
329339

330340
// applyToolConfigRef resolves and applies MCPToolConfig reference
@@ -334,25 +344,28 @@ func (c *Converter) applyToolConfigRef(
334344
vmcp *mcpv1alpha1.VirtualMCPServer,
335345
toolConfig mcpv1alpha1.WorkloadToolConfig,
336346
wtc *vmcpconfig.WorkloadToolConfig,
337-
) {
347+
) error {
338348
if toolConfig.ToolConfigRef == nil {
339-
return
349+
return nil
340350
}
341351

342352
resolvedConfig, err := c.resolveMCPToolConfig(ctx, vmcp.Namespace, toolConfig.ToolConfigRef.Name)
343353
if err != nil {
344354
ctxLogger.Error(err, "failed to resolve MCPToolConfig reference",
345355
"workload", toolConfig.Workload,
346356
"toolConfigRef", toolConfig.ToolConfigRef.Name)
347-
return
357+
// Fail closed: return error when MCPToolConfig is configured but resolution fails
358+
// This prevents deploying without tool filtering when explicit configuration is requested
359+
return fmt.Errorf("MCPToolConfig resolution failed for %q: %w",
360+
toolConfig.ToolConfigRef.Name, err)
348361
}
349362

350-
if resolvedConfig == nil {
351-
return
352-
}
363+
// Note: resolveMCPToolConfig never returns (nil, nil) - it either succeeds with
364+
// (toolConfig, nil) or fails with (nil, error), so no nil check needed here
353365

354366
c.mergeToolConfigFilter(wtc, resolvedConfig)
355367
c.mergeToolConfigOverrides(wtc, resolvedConfig)
368+
return nil
356369
}
357370

358371
// mergeToolConfigFilter merges filter from MCPToolConfig

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

Lines changed: 167 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1657,7 +1657,6 @@ func TestApplyInlineOverrides(t *testing.T) {
16571657
t.Run(tt.name, func(t *testing.T) {
16581658
t.Parallel()
16591659

1660-
16611660
wtc := &vmcpconfig.WorkloadToolConfig{Overrides: tt.existing}
16621661
toolConfig := mcpv1alpha1.WorkloadToolConfig{Overrides: tt.inline}
16631662
(&Converter{}).applyInlineOverrides(toolConfig, wtc)
@@ -1738,12 +1737,178 @@ func TestConvertToolConfigs(t *testing.T) {
17381737
}
17391738

17401739
agg := &vmcpconfig.AggregationConfig{}
1741-
converter.convertToolConfigs(ctx, vmcp, agg)
1740+
err := converter.convertToolConfigs(ctx, vmcp, agg)
17421741

1742+
require.NoError(t, err)
17431743
require.Len(t, agg.Tools, 1)
17441744
assert.Equal(t, tt.expectedWorkload, agg.Tools[0].Workload)
17451745
assert.Equal(t, tt.expectedFilter, agg.Tools[0].Filter)
17461746
assert.Equal(t, tt.expectedOverride, agg.Tools[0].Overrides)
17471747
})
17481748
}
17491749
}
1750+
1751+
// TestConvertToolConfigs_FailClosed tests that MCPToolConfig resolution errors cause conversion to fail.
1752+
// This is a security feature: if a user explicitly references an MCPToolConfig (for tool filtering or
1753+
// security policy enforcement), we should fail rather than deploy without the intended configuration.
1754+
func TestConvertToolConfigs_FailClosed(t *testing.T) {
1755+
t.Parallel()
1756+
1757+
tests := []struct {
1758+
name string
1759+
tools []mcpv1alpha1.WorkloadToolConfig
1760+
existingConfig *mcpv1alpha1.MCPToolConfig
1761+
expectError bool
1762+
expectedErrMsg string
1763+
}{
1764+
{
1765+
name: "error when MCPToolConfig reference not found (fail closed)",
1766+
tools: []mcpv1alpha1.WorkloadToolConfig{{
1767+
Workload: "backend1",
1768+
ToolConfigRef: &mcpv1alpha1.ToolConfigRef{Name: "nonexistent-config"},
1769+
}},
1770+
existingConfig: nil, // MCPToolConfig doesn't exist in cluster
1771+
expectError: true,
1772+
expectedErrMsg: "MCPToolConfig resolution failed for \"nonexistent-config\"",
1773+
},
1774+
{
1775+
name: "no error when no ToolConfigRef specified",
1776+
tools: []mcpv1alpha1.WorkloadToolConfig{{
1777+
Workload: "backend1",
1778+
Filter: []string{"tool1"},
1779+
}},
1780+
existingConfig: nil,
1781+
expectError: false,
1782+
},
1783+
{
1784+
name: "successful when MCPToolConfig exists",
1785+
tools: []mcpv1alpha1.WorkloadToolConfig{{
1786+
Workload: "backend1",
1787+
ToolConfigRef: &mcpv1alpha1.ToolConfigRef{Name: "valid-config"},
1788+
}},
1789+
existingConfig: newMCPToolConfig("valid-config", "default", []string{"fetch"}, nil),
1790+
expectError: false,
1791+
},
1792+
}
1793+
1794+
for _, tt := range tests {
1795+
t.Run(tt.name, func(t *testing.T) {
1796+
t.Parallel()
1797+
1798+
ctx := log.IntoContext(context.Background(), logr.Discard())
1799+
var k8sClient client.Client
1800+
if tt.existingConfig != nil {
1801+
k8sClient = newTestK8sClient(t, tt.existingConfig)
1802+
} else {
1803+
k8sClient = newTestK8sClient(t)
1804+
}
1805+
1806+
converter := newTestConverter(t, newNoOpMockResolver(t))
1807+
converter.k8sClient = k8sClient
1808+
1809+
vmcp := &mcpv1alpha1.VirtualMCPServer{
1810+
ObjectMeta: metav1.ObjectMeta{Name: "test-vmcp", Namespace: "default"},
1811+
Spec: mcpv1alpha1.VirtualMCPServerSpec{Aggregation: &mcpv1alpha1.AggregationConfig{Tools: tt.tools}},
1812+
}
1813+
1814+
agg := &vmcpconfig.AggregationConfig{}
1815+
err := converter.convertToolConfigs(ctx, vmcp, agg)
1816+
1817+
if tt.expectError {
1818+
require.Error(t, err)
1819+
assert.Contains(t, err.Error(), tt.expectedErrMsg)
1820+
} else {
1821+
require.NoError(t, err)
1822+
}
1823+
})
1824+
}
1825+
}
1826+
1827+
// TestConvert_MCPToolConfigFailClosed tests that MCPToolConfig resolution errors propagate through
1828+
// the full Convert() method and prevent VirtualMCPServer deployment.
1829+
func TestConvert_MCPToolConfigFailClosed(t *testing.T) {
1830+
t.Parallel()
1831+
1832+
tests := []struct {
1833+
name string
1834+
vmcp *mcpv1alpha1.VirtualMCPServer
1835+
existingConfig *mcpv1alpha1.MCPToolConfig
1836+
expectError bool
1837+
expectedErrMsg string
1838+
}{
1839+
{
1840+
name: "Convert fails when MCPToolConfig not found",
1841+
vmcp: &mcpv1alpha1.VirtualMCPServer{
1842+
ObjectMeta: metav1.ObjectMeta{Name: "test-vmcp", Namespace: "default"},
1843+
Spec: mcpv1alpha1.VirtualMCPServerSpec{
1844+
GroupRef: mcpv1alpha1.GroupRef{Name: "test-group"},
1845+
Aggregation: &mcpv1alpha1.AggregationConfig{
1846+
Tools: []mcpv1alpha1.WorkloadToolConfig{{
1847+
Workload: "backend1",
1848+
ToolConfigRef: &mcpv1alpha1.ToolConfigRef{Name: "missing-config"},
1849+
}},
1850+
},
1851+
},
1852+
},
1853+
existingConfig: nil,
1854+
expectError: true,
1855+
expectedErrMsg: "failed to convert aggregation config",
1856+
},
1857+
{
1858+
name: "Convert succeeds when MCPToolConfig exists",
1859+
vmcp: &mcpv1alpha1.VirtualMCPServer{
1860+
ObjectMeta: metav1.ObjectMeta{Name: "test-vmcp", Namespace: "default"},
1861+
Spec: mcpv1alpha1.VirtualMCPServerSpec{
1862+
GroupRef: mcpv1alpha1.GroupRef{Name: "test-group"},
1863+
Aggregation: &mcpv1alpha1.AggregationConfig{
1864+
Tools: []mcpv1alpha1.WorkloadToolConfig{{
1865+
Workload: "backend1",
1866+
ToolConfigRef: &mcpv1alpha1.ToolConfigRef{Name: "valid-config"},
1867+
}},
1868+
},
1869+
},
1870+
},
1871+
existingConfig: newMCPToolConfig("valid-config", "default", []string{"fetch"}, nil),
1872+
expectError: false,
1873+
},
1874+
{
1875+
name: "Convert succeeds when no Aggregation specified",
1876+
vmcp: &mcpv1alpha1.VirtualMCPServer{
1877+
ObjectMeta: metav1.ObjectMeta{Name: "test-vmcp", Namespace: "default"},
1878+
Spec: mcpv1alpha1.VirtualMCPServerSpec{
1879+
GroupRef: mcpv1alpha1.GroupRef{Name: "test-group"},
1880+
},
1881+
},
1882+
existingConfig: nil,
1883+
expectError: false,
1884+
},
1885+
}
1886+
1887+
for _, tt := range tests {
1888+
t.Run(tt.name, func(t *testing.T) {
1889+
t.Parallel()
1890+
1891+
ctx := log.IntoContext(context.Background(), logr.Discard())
1892+
var k8sClient client.Client
1893+
if tt.existingConfig != nil {
1894+
k8sClient = newTestK8sClient(t, tt.existingConfig)
1895+
} else {
1896+
k8sClient = newTestK8sClient(t)
1897+
}
1898+
1899+
converter := newTestConverter(t, newNoOpMockResolver(t))
1900+
converter.k8sClient = k8sClient
1901+
1902+
config, err := converter.Convert(ctx, tt.vmcp)
1903+
1904+
if tt.expectError {
1905+
require.Error(t, err)
1906+
assert.Contains(t, err.Error(), tt.expectedErrMsg)
1907+
assert.Nil(t, config)
1908+
} else {
1909+
require.NoError(t, err)
1910+
assert.NotNil(t, config)
1911+
}
1912+
})
1913+
}
1914+
}

0 commit comments

Comments
 (0)