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
27 changes: 15 additions & 12 deletions cmd/thv-operator/controllers/virtualmcpserver_deployment.go
Original file line number Diff line number Diff line change
Expand Up @@ -684,36 +684,39 @@ func (*VirtualMCPServerReconciler) applyPodTemplateSpecToDeployment(
) error {
ctxLogger := log.FromContext(ctx)

// Build user-provided PodTemplateSpec
// Early return if no PodTemplateSpec provided
if vmcp.Spec.PodTemplateSpec == nil || len(vmcp.Spec.PodTemplateSpec.Raw) == 0 {
return nil
}

// Validate the PodTemplateSpec and check if there are meaningful customizations
builder, err := ctrlutil.NewPodTemplateSpecBuilder(vmcp.Spec.PodTemplateSpec, "vmcp")
if err != nil {
return fmt.Errorf("failed to build PodTemplateSpec: %w", err)
}

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

// Marshal both the original and user-provided PodTemplateSpec
// Use the raw user JSON directly for strategic merge patch.
// This avoids the nil-slice-becomes-empty-array problem that occurs when
// we parse JSON into Go structs and re-marshal - Go's json.Marshal converts
// nil slices to [], which strategic merge patch interprets as "replace with empty".
// By using the raw JSON, we preserve exactly what the user specified.
userJSON := vmcp.Spec.PodTemplateSpec.Raw

originalJSON, err := json.Marshal(deployment.Spec.Template)
if err != nil {
return fmt.Errorf("failed to marshal original PodTemplateSpec: %w", err)
}

userJSON, err := json.Marshal(userPodTemplateSpec)
if err != nil {
return fmt.Errorf("failed to marshal user PodTemplateSpec: %w", err)
}

// Apply strategic merge patch to combine controller defaults with user customizations
patchedJSON, err := strategicpatch.StrategicMergePatch(originalJSON, userJSON, corev1.PodTemplateSpec{})
if err != nil {
return fmt.Errorf("failed to apply strategic merge patch: %w", err)
}

// Unmarshal the patched result back into the deployment
var patchedPodTemplateSpec corev1.PodTemplateSpec
if err := json.Unmarshal(patchedJSON, &patchedPodTemplateSpec); err != nil {
return fmt.Errorf("failed to unmarshal patched PodTemplateSpec: %w", err)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,11 @@ import (
"github.com/stacklok/toolhive/cmd/thv-operator/pkg/runconfig/configmap/checksum"
)

// TestVirtualMCPServerReconcile_WithPodTemplateSpec tests are complex and require envtest
// The key functionality is tested in:
// - TestVirtualMCPServerPodTemplateSpecBuilder: builder validation
// - TestVirtualMCPServerPodTemplateSpecValidation: parsing validation
// - TestVirtualMCPServerPodTemplateSpecNeedsUpdate: change detection
// - TestVirtualMCPServerPodTemplateSpecDeterministic: deterministic generation
// - Integration tests in test/e2e/

func TestVirtualMCPServerReconcile_WithValidPodTemplateSpec(t *testing.T) {
t.Parallel()
t.Skip("Complex reconciliation tests require envtest - see TestVirtualMCPServerPodTemplateSpec* for unit tests")
}
const (
testPodTemplateNamespace = "test-namespace"
testPodTemplateVmcpName = "test-vmcp"
testPodTemplateGroupName = "test-group"
)

// TestVirtualMCPServerPodTemplateSpecDeterministic verifies that generating a deployment
// twice with the same PodTemplateSpec produces identical results (no spurious updates)
Expand All @@ -38,9 +31,9 @@ func TestVirtualMCPServerPodTemplateSpecDeterministic(t *testing.T) {
_ = corev1.AddToScheme(scheme)
_ = appsv1.AddToScheme(scheme)

namespace := "test-namespace"
vmcpName := "test-vmcp"
groupName := "test-group"
namespace := testPodTemplateNamespace
vmcpName := testPodTemplateVmcpName
groupName := testPodTemplateGroupName

mcpGroup := &mcpv1alpha1.MCPGroup{
ObjectMeta: metav1.ObjectMeta{
Expand Down Expand Up @@ -107,9 +100,77 @@ func TestVirtualMCPServerPodTemplateSpecDeterministic(t *testing.T) {
"Generating deployment twice with same PodTemplateSpec should produce identical results")
}

func TestVirtualMCPServerDeploymentUpdate_WithPodTemplateSpecChange(t *testing.T) {
// TestVirtualMCPServerPodTemplateSpecPreservesContainer verifies that when a user provides
// a PodTemplateSpec with only pod-level settings (like nodeSelector), the controller-generated
// vmcp container is preserved and not wiped out by the strategic merge patch.
// This is a regression test for the nil-slice-becomes-empty-array bug.
func TestVirtualMCPServerPodTemplateSpecPreservesContainer(t *testing.T) {
t.Parallel()
t.Skip("Complex reconciliation tests require envtest - see TestVirtualMCPServerPodTemplateSpecNeedsUpdate for change detection tests")
scheme := runtime.NewScheme()
_ = mcpv1alpha1.AddToScheme(scheme)
_ = corev1.AddToScheme(scheme)
_ = appsv1.AddToScheme(scheme)

namespace := testPodTemplateNamespace
vmcpName := testPodTemplateVmcpName
groupName := testPodTemplateGroupName

mcpGroup := &mcpv1alpha1.MCPGroup{
ObjectMeta: metav1.ObjectMeta{
Name: groupName,
Namespace: namespace,
},
Status: mcpv1alpha1.MCPGroupStatus{
Phase: mcpv1alpha1.MCPGroupPhaseReady,
},
}

// Use raw JSON directly (simulating real user input) - only nodeSelector, no containers
// This is the exact scenario that triggered the original bug
vmcp := &mcpv1alpha1.VirtualMCPServer{
ObjectMeta: metav1.ObjectMeta{
Name: vmcpName,
Namespace: namespace,
},
Spec: mcpv1alpha1.VirtualMCPServerSpec{
GroupRef: mcpv1alpha1.GroupRef{
Name: groupName,
},
PodTemplateSpec: &runtime.RawExtension{
Raw: []byte(`{"spec":{"nodeSelector":{"disktype":"ssd"}}}`),
},
},
}

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

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

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

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

// Verify deployment was created
assert.NotNil(t, dep, "Deployment should not be nil")

// Verify the vmcp container is preserved (not wiped out by strategic merge)
assert.Len(t, dep.Spec.Template.Spec.Containers, 1, "Should have exactly one container")
assert.Equal(t, "vmcp", dep.Spec.Template.Spec.Containers[0].Name, "Container should be named 'vmcp'")

// Verify the nodeSelector was applied
assert.Equal(t, "ssd", dep.Spec.Template.Spec.NodeSelector["disktype"],
"nodeSelector should be applied from PodTemplateSpec")
}

func TestVirtualMCPServerPodTemplateSpecNeedsUpdate(t *testing.T) {
Expand Down Expand Up @@ -168,9 +229,9 @@ func TestVirtualMCPServerPodTemplateSpecNeedsUpdate(t *testing.T) {
_ = corev1.AddToScheme(scheme)
_ = appsv1.AddToScheme(scheme)

namespace := "test-namespace"
vmcpName := "test-vmcp"
groupName := "test-group"
namespace := testPodTemplateNamespace
vmcpName := testPodTemplateVmcpName
groupName := testPodTemplateGroupName

// Create MCPGroup (required for deployment generation)
mcpGroup := &mcpv1alpha1.MCPGroup{
Expand Down
Loading
Loading