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
10 changes: 10 additions & 0 deletions cmd/thv-operator/controllers/virtualmcpserver_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -1797,6 +1797,15 @@ func (r *VirtualMCPServerReconciler) deploymentNeedsUpdate(
return true
}

_, _, expectedVolumesHash, err := r.buildPodVolumesForVmcp(ctx, vmcp, telemetryCfg, typedWorkloads)
if err != nil {
log.FromContext(ctx).Error(err, "Failed to build volumes, assuming update needed")
return true
}
if deployment.Annotations[podVolumesHashAnnotation] != expectedVolumesHash {
return true
}

// Check if spec.replicas has changed. Only compare when spec.replicas is non-nil;
// nil means hands-off mode (HPA or external controller manages replicas) and the live count is authoritative.
if vmcp.Spec.Replicas != nil {
Expand Down Expand Up @@ -1960,6 +1969,7 @@ func mergeDeploymentAnnotations(desired, live map[string]string) map[string]stri
for _, key := range []string{
imagePullRefsHashAnnotation,
podTemplateSpecHashAnnotation,
podVolumesHashAnnotation,
ctrlutil.AuthServerCABundleChecksumAnnotation,
} {
if _, want := desired[key]; !want {
Expand Down
177 changes: 174 additions & 3 deletions cmd/thv-operator/controllers/virtualmcpserver_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1908,6 +1908,10 @@ func TestVirtualMCPServerDeploymentNeedsUpdate(t *testing.T) {
expectedLabels, expectedAnnotations := reconciler.buildPodTemplateMetadata(
labelsForVirtualMCPServer(vmcp.Name), vmcp, vmcpConfigChecksum, "",
)
desiredVolumeMounts, desiredVolumes, desiredVolumesHash, err := reconciler.buildPodVolumesForVmcp(
context.Background(), vmcp, nil, nil,
)
require.NoError(t, err)

tests := []struct {
name string
Expand Down Expand Up @@ -2081,7 +2085,7 @@ func TestVirtualMCPServerDeploymentNeedsUpdate(t *testing.T) {
deployment: &appsv1.Deployment{
ObjectMeta: metav1.ObjectMeta{
Labels: labelsForVirtualMCPServer(vmcp.Name),
Annotations: make(map[string]string),
Annotations: map[string]string{podVolumesHashAnnotation: desiredVolumesHash},
Comment thread
jstar0 marked this conversation as resolved.
},
Spec: appsv1.DeploymentSpec{
Template: corev1.PodTemplateSpec{
Expand All @@ -2097,10 +2101,12 @@ func TestVirtualMCPServerDeploymentNeedsUpdate(t *testing.T) {
Ports: []corev1.ContainerPort{
{ContainerPort: 4483},
},
Args: reconciler.buildContainerArgsForVmcp(vmcp),
Env: mustBuildEnvVarsForVmcp(reconciler, vmcp),
Args: reconciler.buildContainerArgsForVmcp(vmcp),
Env: mustBuildEnvVarsForVmcp(reconciler, vmcp),
VolumeMounts: desiredVolumeMounts,
},
},
Volumes: desiredVolumes,
ServiceAccountName: vmcpServiceAccountName(vmcp.Name),
},
},
Expand Down Expand Up @@ -2146,6 +2152,12 @@ func TestMergeDeploymentAnnotations(t *testing.T) {
live: map[string]string{podTemplateSpecHashAnnotation: "stale-hash"},
expected: map[string]string{},
},
{
name: "prunes stale pod volumes hash annotation when desired no longer wants it",
desired: map[string]string{},
live: map[string]string{podVolumesHashAnnotation: "stale-hash"},
expected: map[string]string{},
},
{
name: "keeps hash annotations desired still wants",
desired: map[string]string{imagePullRefsHashAnnotation: "new-hash", podTemplateSpecHashAnnotation: "new-hash"},
Expand Down Expand Up @@ -2637,6 +2649,165 @@ func TestVirtualMCPServerEnsureDeployment_NoUpdateNeeded(t *testing.T) {
assert.Equal(t, ctrl.Result{}, result)
}

func TestVirtualMCPServerEnsureDeployment_BackfillsPodVolumesHashOnce(t *testing.T) {
t.Parallel()

ctx := t.Context()
scheme := testutil.NewScheme(t)
vmcp := v1beta1test.NewVirtualMCPServer(testVmcpName, "default",
v1beta1test.WithVMCPGroupRef(testGroupName),
)
configMap := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: vmcpConfigMapName(vmcp.Name),
Namespace: vmcp.Namespace,
Annotations: map[string]string{
checksum.ContentChecksumAnnotation: "test-checksum",
},
},
Data: map[string]string{"config.yaml": "test-config"},
}

deploymentUpdates := 0
k8sClient := fake.NewClientBuilder().
WithScheme(scheme).
WithObjects(vmcp, configMap).
WithInterceptorFuncs(interceptor.Funcs{
Update: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.UpdateOption) error {
if _, ok := obj.(*appsv1.Deployment); ok {
deploymentUpdates++
}
return c.Update(ctx, obj, opts...)
},
}).
Build()
reconciler := &VirtualMCPServerReconciler{
Client: k8sClient,
Scheme: scheme,
PlatformDetector: ctrlutil.NewSharedPlatformDetector(),
}

deployment := reconciler.deploymentForVirtualMCPServer(ctx, vmcp, "test-checksum", "", nil, nil)
require.NotNil(t, deployment)
require.NotEmpty(t, deployment.Annotations[podVolumesHashAnnotation])
delete(deployment.Annotations, podVolumesHashAnnotation)
require.NoError(t, k8sClient.Create(ctx, deployment))

result, err := reconciler.ensureDeployment(ctx, vmcp, nil, nil)
require.NoError(t, err)
assert.Equal(t, ctrl.Result{}, result)
assert.Equal(t, 1, deploymentUpdates, "a missing hash annotation must cause exactly one update")

updated := &appsv1.Deployment{}
require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: vmcp.Name, Namespace: vmcp.Namespace}, updated))
require.NotEmpty(t, updated.Annotations[podVolumesHashAnnotation])
resourceVersion := updated.ResourceVersion

result, err = reconciler.ensureDeployment(ctx, vmcp, nil, nil)
require.NoError(t, err)
assert.Equal(t, ctrl.Result{}, result)
assert.Equal(t, 1, deploymentUpdates, "the rebuilt deployment must be steady state")

updated = &appsv1.Deployment{}
require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: vmcp.Name, Namespace: vmcp.Namespace}, updated))
assert.Equal(t, resourceVersion, updated.ResourceVersion, "steady-state reconcile must not write again")
}

func TestVirtualMCPServerEnsureDeployment_UpdatesMCPServerEntryCABundleVolume(t *testing.T) {
t.Parallel()

ctx := t.Context()
scheme := testutil.NewScheme(t)
vmcp := v1beta1test.NewVirtualMCPServer(testVmcpName, "default",
v1beta1test.WithVMCPGroupRef(testGroupName),
)
configMap := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: vmcpConfigMapName(vmcp.Name),
Namespace: vmcp.Namespace,
Annotations: map[string]string{
checksum.ContentChecksumAnnotation: "test-checksum",
},
},
Data: map[string]string{"config.yaml": "test-config"},
}
entry := &mcpv1beta1.MCPServerEntry{
ObjectMeta: metav1.ObjectMeta{Name: "remote-entry", Namespace: vmcp.Namespace},
Spec: mcpv1beta1.MCPServerEntrySpec{
RemoteURL: "https://mcp.example.com",
Transport: "streamable-http",
GroupRef: &mcpv1beta1.MCPGroupRef{Name: testGroupName},
CABundleRef: &mcpv1beta1.CABundleSource{ConfigMapRef: &corev1.ConfigMapKeySelector{
LocalObjectReference: corev1.LocalObjectReference{Name: "ca-bundle-v1"},
Key: "ca.crt",
}},
},
}
typedWorkloads := []workloads.TypedWorkload{{
Name: entry.Name,
Type: workloads.WorkloadTypeMCPServerEntry,
}}

deploymentUpdates := 0
k8sClient := fake.NewClientBuilder().
WithScheme(scheme).
WithObjects(vmcp, configMap, entry).
WithInterceptorFuncs(interceptor.Funcs{
Update: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.UpdateOption) error {
if _, ok := obj.(*appsv1.Deployment); ok {
deploymentUpdates++
}
return c.Update(ctx, obj, opts...)
},
}).
Build()
reconciler := &VirtualMCPServerReconciler{
Client: k8sClient,
Scheme: scheme,
PlatformDetector: ctrlutil.NewSharedPlatformDetector(),
}

deployment := reconciler.deploymentForVirtualMCPServer(
ctx, vmcp, "test-checksum", "", nil, typedWorkloads,
)
require.NotNil(t, deployment)
initialHash := deployment.Annotations[podVolumesHashAnnotation]
require.NotEmpty(t, initialHash)
require.NoError(t, k8sClient.Create(ctx, deployment))

updatedEntry := &mcpv1beta1.MCPServerEntry{}
require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: entry.Name, Namespace: entry.Namespace}, updatedEntry))
updatedEntry.Spec.CABundleRef.ConfigMapRef.Name = "ca-bundle-v2"
require.NoError(t, k8sClient.Update(ctx, updatedEntry))

result, err := reconciler.ensureDeployment(ctx, vmcp, nil, typedWorkloads)
require.NoError(t, err)
assert.Equal(t, ctrl.Result{}, result)
assert.Equal(t, 1, deploymentUpdates, "changing only the ConfigMap volume source must update the deployment")

updated := &appsv1.Deployment{}
require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: vmcp.Name, Namespace: vmcp.Namespace}, updated))
assert.NotEqual(t, initialHash, updated.Annotations[podVolumesHashAnnotation])
var caBundleConfigMapName string
for _, volume := range updated.Spec.Template.Spec.Volumes {
if volume.Name == caBundleVolumeName(entry.Name) && volume.ConfigMap != nil {
caBundleConfigMapName = volume.ConfigMap.Name
break
}
}
assert.Equal(t, "ca-bundle-v2", caBundleConfigMapName)
resourceVersion := updated.ResourceVersion

result, err = reconciler.ensureDeployment(ctx, vmcp, nil, typedWorkloads)
require.NoError(t, err)
assert.Equal(t, ctrl.Result{}, result)
assert.Equal(t, 1, deploymentUpdates, "the updated CA bundle reference must reach steady state")

updated = &appsv1.Deployment{}
require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: vmcp.Name, Namespace: vmcp.Namespace}, updated))
assert.Equal(t, resourceVersion, updated.ResourceVersion, "steady-state reconcile must not write again")
}

// TestVirtualMCPServerEnsureDeployment_RemovesStaleHashAnnotation is a regression test
// for #5817/#5818: a stale operator-owned hash annotation left over from a prior
// reconcile (when the corresponding field was non-empty) must be removed once that
Expand Down
119 changes: 89 additions & 30 deletions cmd/thv-operator/controllers/virtualmcpserver_deployment.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ const (
// detect every input that influences the deployed PodSpec.ImagePullSecrets.
imagePullRefsHashAnnotation = "toolhive.stacklok.io/imagepullsecrets-hash"

// podVolumesHashAnnotation tracks the SHA256 hash of the desired vMCP
// container volume mounts and PodSpec volumes. The hash is stored on the
// Deployment so changes to referenced Secrets or ConfigMaps trigger a
// rollout without comparing API-server-defaulted live PodSpec fields.
podVolumesHashAnnotation = "toolhive.stacklok.io/podvolumes-hash"

// Log level configuration
logLevelDebug = "debug" // Debug log level value

Expand Down Expand Up @@ -144,7 +150,7 @@ func (r *VirtualMCPServerReconciler) deploymentForVirtualMCPServer(

// Build deployment components using helper functions
args := r.buildContainerArgsForVmcp(vmcp)
volumeMounts, volumes, err := r.buildVolumesForVmcp(ctx, vmcp)
volumeMounts, volumes, volumesHash, err := r.buildPodVolumesForVmcp(ctx, vmcp, telemetryCfg, typedWorkloads)
if err != nil {
log.FromContext(ctx).Error(err, "Failed to build volumes for VirtualMCPServer")
return nil
Expand All @@ -155,35 +161,7 @@ func (r *VirtualMCPServerReconciler) deploymentForVirtualMCPServer(
return nil
}

// Add CA bundle volumes for MCPServerEntry backends with caBundleRef
caVolumes, caMounts, err := r.buildCABundleVolumesForEntries(ctx, vmcp.Namespace, typedWorkloads)
if err != nil {
log.FromContext(ctx).Error(err, "Failed to build CA bundle volumes for MCPServerEntries")
return nil
}
volumes = append(volumes, caVolumes...)
volumeMounts = append(volumeMounts, caMounts...)

// Add telemetry CA bundle volumes from the pre-fetched MCPTelemetryConfig
if telemetryCfg != nil {
telVolumes, telMounts := ctrlutil.AddTelemetryCABundleVolumes(telemetryCfg)
volumes = append(volumes, telVolumes...)
volumeMounts = append(volumeMounts, telMounts...)
}

// Add embedded auth server volumes if configured (inline config). The matching
// env vars are injected by buildEnvVarsForVmcp above so the drift check stays
// symmetric with what is built here (see #5616).
if vmcp.Spec.AuthServerConfig != nil {
authServerVolumes, authServerMounts, err := ctrlutil.GenerateAuthServerVolumes(vmcp.Spec.AuthServerConfig)
if err != nil {
log.FromContext(ctx).Error(err, "Failed to build embedded auth server CA volumes")
return nil
}
volumes = append(volumes, authServerVolumes...)
volumeMounts = append(volumeMounts, authServerMounts...)
}
deploymentLabels, deploymentAnnotations := r.buildDeploymentMetadataForVmcp(ls, vmcp)
deploymentLabels, deploymentAnnotations := r.buildDeploymentMetadataForVmcp(ls, vmcp, volumesHash)
deploymentTemplateLabels, deploymentTemplateAnnotations := r.buildPodTemplateMetadata(
ls, vmcp, vmcpConfigChecksum, caBundleChecksum)
podSecurityContext, containerSecurityContext := r.buildSecurityContextsForVmcp(ctx, vmcp)
Expand Down Expand Up @@ -263,6 +241,84 @@ func (r *VirtualMCPServerReconciler) deploymentForVirtualMCPServer(
return dep
}

// buildPodVolumesForVmcp builds the complete desired volume state for the vmcp
// container and computes its stable hash in the same pass. Keeping all volume
// sources here ensures the PodSpec and Deployment annotation use one consistent
// snapshot of referenced Kubernetes objects.
func (r *VirtualMCPServerReconciler) buildPodVolumesForVmcp(
ctx context.Context,
vmcp *mcpv1beta1.VirtualMCPServer,
telemetryCfg *mcpv1beta1.MCPTelemetryConfig,
typedWorkloads []workloads.TypedWorkload,
) ([]corev1.VolumeMount, []corev1.Volume, string, error) {
volumeMounts, volumes, err := r.buildVolumesForVmcp(ctx, vmcp)
if err != nil {
return nil, nil, "", err
}

caVolumes, caMounts, err := r.buildCABundleVolumesForEntries(ctx, vmcp.Namespace, typedWorkloads)
if err != nil {
return nil, nil, "", fmt.Errorf("failed to build CA bundle volumes for MCPServerEntries: %w", err)
}
volumes = append(volumes, caVolumes...)
volumeMounts = append(volumeMounts, caMounts...)

if telemetryCfg != nil {
telVolumes, telMounts := ctrlutil.AddTelemetryCABundleVolumes(telemetryCfg)
volumes = append(volumes, telVolumes...)
volumeMounts = append(volumeMounts, telMounts...)
}

if vmcp.Spec.AuthServerConfig != nil {
authServerVolumes, authServerMounts, err := ctrlutil.GenerateAuthServerVolumes(vmcp.Spec.AuthServerConfig)
if err != nil {
return nil, nil, "", fmt.Errorf("failed to build embedded auth server CA volumes: %w", err)
}
volumes = append(volumes, authServerVolumes...)
volumeMounts = append(volumeMounts, authServerMounts...)
}

hash, err := podVolumesHash(volumes, volumeMounts)
if err != nil {
return nil, nil, "", err
}
return volumeMounts, volumes, hash, nil
}

// podVolumesHash returns a deterministic hash of the complete desired volume
// and volume-mount objects. Sorting by identity makes ordering-only changes a
// no-op while hashing the full Kubernetes structs preserves all drift-relevant
// source fields, including future VolumeSource additions.
func podVolumesHash(volumes []corev1.Volume, volumeMounts []corev1.VolumeMount) (string, error) {
normalizedVolumes := append([]corev1.Volume(nil), volumes...)
sort.SliceStable(normalizedVolumes, func(i, j int) bool {
return normalizedVolumes[i].Name < normalizedVolumes[j].Name
})
normalizedMounts := append([]corev1.VolumeMount(nil), volumeMounts...)
sort.SliceStable(normalizedMounts, func(i, j int) bool {
if normalizedMounts[i].Name != normalizedMounts[j].Name {
return normalizedMounts[i].Name < normalizedMounts[j].Name
}
if normalizedMounts[i].MountPath != normalizedMounts[j].MountPath {
return normalizedMounts[i].MountPath < normalizedMounts[j].MountPath
}
return normalizedMounts[i].SubPath < normalizedMounts[j].SubPath
})

canonical, err := json.Marshal(struct {
Volumes []corev1.Volume
VolumeMounts []corev1.VolumeMount
}{
Volumes: normalizedVolumes,
VolumeMounts: normalizedMounts,
})
if err != nil {
return "", fmt.Errorf("failed to marshal pod volumes for hashing: %w", err)
}
hash := sha256.Sum256(canonical)
return hex.EncodeToString(hash[:]), nil
}

// buildContainerArgsForVmcp builds the container arguments for vmcp
func (*VirtualMCPServerReconciler) buildContainerArgsForVmcp(
vmcp *mcpv1beta1.VirtualMCPServer,
Expand Down Expand Up @@ -930,6 +986,7 @@ func xaaSecretEnvVars(externalAuthConfig *mcpv1beta1.MCPExternalAuthConfig, conf
func (r *VirtualMCPServerReconciler) buildDeploymentMetadataForVmcp(
baseLabels map[string]string,
vmcp *mcpv1beta1.VirtualMCPServer,
podVolumesHash string,
) (map[string]string, map[string]string) {
deploymentLabels := baseLabels
deploymentAnnotations := make(map[string]string)
Expand All @@ -955,6 +1012,8 @@ func (r *VirtualMCPServerReconciler) buildDeploymentMetadataForVmcp(
deploymentAnnotations[imagePullRefsHashAnnotation] = hash
}

deploymentAnnotations[podVolumesHashAnnotation] = podVolumesHash

// TODO: Add support for ResourceOverrides if needed in the future

return deploymentLabels, deploymentAnnotations
Expand Down
Loading
Loading