Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions cmd/thv-operator/controllers/virtualmcpserver_deployment.go
Original file line number Diff line number Diff line change
Expand Up @@ -1220,17 +1220,15 @@ func (*VirtualMCPServerReconciler) applyPodTemplateSpecToDeployment(
return nil
}

// Validate the PodTemplateSpec and check if there are meaningful customizations
builder, err := ctrlutil.NewPodTemplateSpecBuilder(vmcp.Spec.PodTemplateSpec, "vmcp")
if err != nil {
// Validate the user-provided PodTemplateSpec is well-formed.
// We don't check builder.Build() == nil for "empty" customizations: that helper
// only enumerates a subset of PodSpec fields and would skip the patch for
// fields like runtimeClassName or topologySpreadConstraints. Strategic merge
// patch is a no-op for `{}` anyway, so always running it is safe.
if _, err := ctrlutil.NewPodTemplateSpecBuilder(vmcp.Spec.PodTemplateSpec, "vmcp"); err != nil {
return fmt.Errorf("failed to build PodTemplateSpec: %w", err)
}

if builder.Build() == nil {
// No meaningful customizations to apply
return nil
}

merged, err := ctrlutil.ApplyPodTemplateSpecPatch(deployment.Spec.Template, vmcp.Spec.PodTemplateSpec.Raw)
if err != nil {
return err
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,114 @@ func TestVirtualMCPServerPodTemplateSpecNeedsUpdate(t *testing.T) {
}
}

// TestVirtualMCPServerPodTemplateSpecPreservesUndetectedFields is a regression test for
// https://github.com/stacklok/toolhive/issues/5110. PodTemplateSpecBuilder.isEmpty()
// only enumerated a subset of PodSpec fields, so applyPodTemplateSpecToDeployment
// skipped the strategic merge patch when a user set only fields outside that list
// (runtimeClassName, topologySpreadConstraints, hostNetwork, dnsConfig, readinessGates).
func TestVirtualMCPServerPodTemplateSpecPreservesUndetectedFields(t *testing.T) {
t.Parallel()

tests := []struct {
name string
userJSON string
assert func(t *testing.T, podSpec corev1.PodSpec)
}{
{
name: "runtimeClassName is preserved",
userJSON: `{"spec":{"runtimeClassName":"kata"}}`,
assert: func(t *testing.T, podSpec corev1.PodSpec) {
t.Helper()
require.NotNil(t, podSpec.RuntimeClassName)
assert.Equal(t, "kata", *podSpec.RuntimeClassName)
},
},
{
name: "topologySpreadConstraints are preserved",
userJSON: `{"spec":{"topologySpreadConstraints":[{"maxSkew":1,"topologyKey":"topology.kubernetes.io/zone","whenUnsatisfiable":"DoNotSchedule","labelSelector":{"matchLabels":{"app":"vmcp"}}}]}}`,
assert: func(t *testing.T, podSpec corev1.PodSpec) {
t.Helper()
require.Len(t, podSpec.TopologySpreadConstraints, 1)
assert.Equal(t, int32(1), podSpec.TopologySpreadConstraints[0].MaxSkew)
assert.Equal(t, "topology.kubernetes.io/zone", podSpec.TopologySpreadConstraints[0].TopologyKey)
},
},
{
name: "hostNetwork is preserved",
userJSON: `{"spec":{"hostNetwork":true}}`,
assert: func(t *testing.T, podSpec corev1.PodSpec) {
t.Helper()
assert.True(t, podSpec.HostNetwork)
},
},
{
name: "dnsConfig is preserved",
userJSON: `{"spec":{"dnsConfig":{"searches":["svc.cluster.local"]}}}`,
assert: func(t *testing.T, podSpec corev1.PodSpec) {
t.Helper()
require.NotNil(t, podSpec.DNSConfig)
assert.Equal(t, []string{"svc.cluster.local"}, podSpec.DNSConfig.Searches)
},
},
{
name: "readinessGates are preserved",
userJSON: `{"spec":{"readinessGates":[{"conditionType":"cloud.google.com/load-balancer-healthy"}]}}`,
assert: func(t *testing.T, podSpec corev1.PodSpec) {
t.Helper()
require.Len(t, podSpec.ReadinessGates, 1)
assert.Equal(t, corev1.PodConditionType("cloud.google.com/load-balancer-healthy"), podSpec.ReadinessGates[0].ConditionType)
},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
scheme := testutil.NewScheme(t)

mcpGroup := &mcpv1beta1.MCPGroup{
ObjectMeta: metav1.ObjectMeta{
Name: testPodTemplateGroupName,
Namespace: testPodTemplateNamespace,
},
Status: mcpv1beta1.MCPGroupStatus{
Phase: mcpv1beta1.MCPGroupPhaseReady,
},
}

vmcp := v1beta1test.NewVirtualMCPServer(testPodTemplateVmcpName, testPodTemplateNamespace,
v1beta1test.WithVMCPGroupRef(testPodTemplateGroupName),
v1beta1test.WithVMCPPodTemplateSpec(&runtime.RawExtension{Raw: []byte(tt.userJSON)}),
)

configMap := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: vmcpConfigMapName(testPodTemplateVmcpName),
Namespace: testPodTemplateNamespace,
},
}

fakeClient := fake.NewClientBuilder().
WithScheme(scheme).
WithObjects(mcpGroup, vmcp, configMap).
Build()

reconciler := &VirtualMCPServerReconciler{
Client: fakeClient,
Scheme: scheme,
}

dep := reconciler.deploymentForVirtualMCPServer(
context.Background(), vmcp, "test-checksum", nil, []workloads.TypedWorkload{})

require.NotNil(t, dep, "Deployment should not be nil")
assert.Len(t, dep.Spec.Template.Spec.Containers, 1, "vmcp container should be preserved")
assert.Equal(t, "vmcp", dep.Spec.Template.Spec.Containers[0].Name)
tt.assert(t, dep.Spec.Template.Spec)
})
}
}

// TestVirtualMCPServerPodTemplateSpecResourceOverride verifies that a user can override
// the default resource requirements via PodTemplateSpec using strategic merge patch.
func TestVirtualMCPServerPodTemplateSpecResourceOverride(t *testing.T) {
Expand Down
Loading