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

if r.podVolumesNeedsUpdate(ctx, deployment, vmcp, telemetryCfg, typedWorkloads) {
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 @@ -1802,6 +1806,42 @@ func (r *VirtualMCPServerReconciler) imagePullSecretsNeedsUpdate(
return deployment.Annotations[imagePullRefsHashAnnotation] != expectedHash
}

// podVolumesNeedsUpdate detects drift on the desired pod volumes and volumeMounts
// by comparing a hash of the desired set against podVolumesHashAnnotation. We
// cannot compare deployment.Spec.Template.Spec.Volumes directly because the
// live list includes K8s-defaulted fields (defaultMode, etc.) that would cause
// spurious drift or mask real changes when only the backing secret/configmap
// ref changes (see #5619).
func (r *VirtualMCPServerReconciler) podVolumesNeedsUpdate(
ctx context.Context,
deployment *appsv1.Deployment,
vmcp *mcpv1beta1.VirtualMCPServer,
telemetryCfg *mcpv1beta1.MCPTelemetryConfig,
typedWorkloads []workloads.TypedWorkload,
) bool {
if deployment == nil || vmcp == nil {
return true
}

volumeMounts, volumes, err := r.buildPodVolumesForVmcp(ctx, vmcp, telemetryCfg, typedWorkloads)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F3 — MEDIUM] API calls inside the drift-detection chain (Consensus: 8/10)

Every other function in the deploymentNeedsUpdate chain (containerNeedsUpdate, imagePullSecretsNeedsUpdate, podTemplateSpecNeedsUpdate) is a pure function of already-fetched state. podVolumesNeedsUpdate calls buildPodVolumesForVmcp here, which transitively makes API calls (listMCPServerEntriesAsMap, potentially GetOIDCConfigForServer). This breaks the "read once, compute, converge" pattern from the operator rules and adds per-reconcile API load proportional to OIDC and MCPServerEntry configuration.

The data is already available from earlier reconcile steps — typedWorkloads and telemetryCfg already flow through. The remaining hidden call is the OIDC config Get inside buildVolumesForVmcp. Pre-fetching it in the main reconcile loop (as is done for telemetryCfg) and threading it as a parameter would make this function pure. Consider tracking as a follow-up issue.

Raised by: Kubernetes operator patterns agent, General code quality agent

if err != nil {
log.FromContext(ctx).Error(err, "Failed to build pod volumes for drift check, assuming update needed")
return true
}

expectedHash, err := podVolumesHash(volumeMounts, volumes)
if err != nil {
log.FromContext(ctx).Error(err, "Failed to hash pod volumes, assuming update needed")
return true
}

_, present := deployment.Annotations[podVolumesHashAnnotation]
if expectedHash == "" {
return present

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F4 — MEDIUM] Annotation removal blocked by MergeAnnotations — latent update loop (Consensus: 7/10)

When expectedHash == "" (no volumes), this returns present — signalling drift if the annotation exists on the live deployment. However, the update path uses MergeAnnotations(newDeployment.Annotations, deployment.Annotations) where deployment.Annotations acts as an override (takes precedence for keys not in newDeployment.Annotations). If newDeployment.Annotations omits the key (because the hash is empty), the live annotation is preserved through the merge and never removed — so drift is detected again on the next reconcile. Same latent bug exists for imagePullRefsHashAnnotation.

In practice this is unreachable today because buildVolumesForVmcp always adds the vmcp-config volume, so the hash is never empty. But it's worth a defensive comment here and a fix in the update path to explicitly delete the key when the new deployment omits it. Consider tracking alongside the existing imagePullRefsHashAnnotation issue.

Raised by: Kubernetes operator patterns agent

}
return deployment.Annotations[podVolumesHashAnnotation] != expectedHash
}

// serviceNeedsUpdate checks if the service needs to be updated
func (*VirtualMCPServerReconciler) serviceNeedsUpdate(
service *corev1.Service,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1905,6 +1905,9 @@ func TestVirtualMCPServerDeploymentNeedsUpdate(t *testing.T) {
expectedLabels, expectedAnnotations := reconciler.buildPodTemplateMetadata(
labelsForVirtualMCPServer(vmcp.Name), vmcp, vmcpConfigChecksum,
)
_, expectedDeploymentAnnotations := reconciler.buildDeploymentMetadataForVmcp(
context.Background(), labelsForVirtualMCPServer(vmcp.Name), vmcp, nil, []workloads.TypedWorkload{},
)

tests := []struct {
name string
Expand Down Expand Up @@ -2014,7 +2017,7 @@ func TestVirtualMCPServerDeploymentNeedsUpdate(t *testing.T) {
deployment: &appsv1.Deployment{
ObjectMeta: metav1.ObjectMeta{
Labels: labelsForVirtualMCPServer(vmcp.Name),
Annotations: make(map[string]string),
Annotations: expectedDeploymentAnnotations,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F5 — LOW] "Expected update" test cases trigger drift for the wrong reason (Consensus: 8/10)

This line correctly fixes the "no changes — no update needed" case to use expectedDeploymentAnnotations. However, the other expectedUpdate: true table cases ("deployment metadata changed", "pod template metadata changed", "container changed") still use Annotations: make(map[string]string) — which lacks podVolumesHashAnnotation. With the new drift check wired in, podVolumesNeedsUpdate fires first and returns true before the test's declared drift cause is even reached. Tests still pass (since expectedUpdate: true), but each test is now proving the wrong thing.

Update the sibling expectedUpdate: true cases to also use expectedDeploymentAnnotations as the base, then mutate only the field the test declares (e.g., wrong image, wrong labels). This ensures each case isolates and exercises exactly the drift check it claims to test.

Raised by: Test coverage agent, Kubernetes operator patterns agent, General code quality agent

},
Spec: appsv1.DeploymentSpec{
Template: corev1.PodTemplateSpec{
Expand Down
187 changes: 161 additions & 26 deletions cmd/thv-operator/controllers/virtualmcpserver_deployment.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,14 @@ const (
// detect every input that influences the deployed PodSpec.ImagePullSecrets.
imagePullRefsHashAnnotation = "toolhive.stacklok.io/imagepullsecrets-hash"

// podVolumesHashAnnotation tracks the SHA256 hash of the desired pod
// volumes and volumeMounts list. Mirrors the imagePullRefsHashAnnotation
// pattern to detect drift on volume-only inputs (auth-server signing keys,
// CA-bundle refs, telemetry CA bundles) without comparing live
// PodSpec.Volumes directly, which trips on K8s-defaulted fields like
// defaultMode.
podVolumesHashAnnotation = "toolhive.stacklok.io/podvolumes-hash"

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

Expand Down Expand Up @@ -147,7 +155,7 @@ func (r *VirtualMCPServerReconciler) deploymentForVirtualMCPServer(

// Build deployment components using helper functions
args := r.buildContainerArgsForVmcp(vmcp)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F2 — MEDIUM] Double computation of buildPodVolumesForVmcp per deployment build (Consensus: 9/10)

buildPodVolumesForVmcp is called here (line 157) to populate the pod spec, and then again inside buildDeploymentMetadataForVmcp (called a few lines below) to compute the hash annotation. Since buildPodVolumesForVmcp calls buildCABundleVolumesForEntrieslistMCPServerEntriesAsMap (a Kubernetes API List), every deployment build now makes that API call twice with identical inputs. There's also a narrow TOCTOU window: if an MCPServerEntry list changes between the two calls, the annotation hash won't match the volumes actually written to the spec.

Suggested approach: compute volumes once here, pass the pre-computed slices (or just the pre-computed hash) into buildDeploymentMetadataForVmcp so both uses come from the same result.

Raised by: Kubernetes operator patterns agent, Go code quality agent, General code quality agent

volumeMounts, volumes, err := r.buildVolumesForVmcp(ctx, vmcp)
volumeMounts, volumes, 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 @@ -158,31 +166,9 @@ 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 := ctrlutil.GenerateAuthServerVolumes(vmcp.Spec.AuthServerConfig)
volumes = append(volumes, authServerVolumes...)
volumeMounts = append(volumeMounts, authServerMounts...)
}
deploymentLabels, deploymentAnnotations := r.buildDeploymentMetadataForVmcp(ls, vmcp)
deploymentLabels, deploymentAnnotations := r.buildDeploymentMetadataForVmcp(
ctx, ls, vmcp, telemetryCfg, typedWorkloads,
)
deploymentTemplateLabels, deploymentTemplateAnnotations := r.buildPodTemplateMetadata(ls, vmcp, vmcpConfigChecksum)
podSecurityContext, containerSecurityContext := r.buildSecurityContextsForVmcp(ctx, vmcp)
serviceAccountName := r.serviceAccountNameForVmcp(vmcp)
Expand Down Expand Up @@ -329,6 +315,44 @@ func (r *VirtualMCPServerReconciler) buildVolumesForVmcp(
return volumeMounts, volumes, nil
}

// buildPodVolumesForVmcp assembles the full desired pod volumes and volumeMounts
// for a VirtualMCPServer Deployment. Shared by deploymentForVirtualMCPServer and
// the podVolumesNeedsUpdate drift check so both paths hash the same inputs.
func (r *VirtualMCPServerReconciler) buildPodVolumesForVmcp(
ctx context.Context,
vmcp *mcpv1beta1.VirtualMCPServer,
telemetryCfg *mcpv1beta1.MCPTelemetryConfig,
typedWorkloads []workloads.TypedWorkload,
) ([]corev1.VolumeMount, []corev1.Volume, 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, 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...)
}

// The matching env vars are injected by buildEnvVarsForVmcp so the drift
// check stays symmetric with what is built here (see #5616).
if vmcp.Spec.AuthServerConfig != nil {
authServerVolumes, authServerMounts := ctrlutil.GenerateAuthServerVolumes(vmcp.Spec.AuthServerConfig)
volumes = append(volumes, authServerVolumes...)
volumeMounts = append(volumeMounts, authServerMounts...)
}

return volumeMounts, volumes, nil
}

// buildEnvVarsForVmcp builds environment variables for the vmcp container.
// telemetryCfg is the already-fetched MCPTelemetryConfig (nil when not referenced).
func (r *VirtualMCPServerReconciler) buildEnvVarsForVmcp(
Expand Down Expand Up @@ -891,8 +915,11 @@ func (r *VirtualMCPServerReconciler) getExternalAuthConfigSecretEnvVars(

// buildDeploymentMetadataForVmcp builds deployment-level labels and annotations
func (r *VirtualMCPServerReconciler) buildDeploymentMetadataForVmcp(
ctx context.Context,
baseLabels map[string]string,
vmcp *mcpv1beta1.VirtualMCPServer,
telemetryCfg *mcpv1beta1.MCPTelemetryConfig,
typedWorkloads []workloads.TypedWorkload,
) (map[string]string, map[string]string) {
deploymentLabels := baseLabels
deploymentAnnotations := make(map[string]string)
Expand All @@ -918,6 +945,16 @@ func (r *VirtualMCPServerReconciler) buildDeploymentMetadataForVmcp(
deploymentAnnotations[imagePullRefsHashAnnotation] = hash
}

// Store hash of the desired pod volumes and volumeMounts so
// deploymentNeedsUpdate can detect volume-only drift (auth-server secret
// refs, CA-bundle refs, telemetry CA bundles) without comparing live
// PodSpec.Volumes, which trips on K8s-defaulted fields (see #5619).
if volumeMounts, volumes, err := r.buildPodVolumesForVmcp(ctx, vmcp, telemetryCfg, typedWorkloads); err == nil {
if hash, err := podVolumesHash(volumeMounts, volumes); err == nil && hash != "" {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F1 — MEDIUM] Silent error discard in buildDeploymentMetadataForVmcp (Consensus: 8/10)

Both errors (buildPodVolumesForVmcp and podVolumesHash) are silently discarded with no log call. Unlike imagePullSecretsHash (pure, cannot fail transiently), buildPodVolumesForVmcp makes real API calls (listMCPServerEntriesAsMap) that can fail transiently. When they do, the annotation is never set, and podVolumesNeedsUpdate returns true on every reconcile until the annotation is written — a quiet update loop for the duration of the outage. The operator rule allows advisory enrichment to skip but requires a log call with a comment explaining why skipping is safe.

Suggested change
if hash, err := podVolumesHash(volumeMounts, volumes); err == nil && hash != "" {
if volumeMounts, volumes, err := r.buildPodVolumesForVmcp(ctx, vmcp, telemetryCfg, typedWorkloads); err != nil {
// Advisory enrichment — skipping is safe: podVolumesNeedsUpdate will return true
// until the annotation is set, causing at most one extra rollout per transient error.
log.FromContext(ctx).Error(err, "Failed to compute pod volumes hash annotation, skipping")
} else if hash, err := podVolumesHash(volumeMounts, volumes); err != nil {
log.FromContext(ctx).Error(err, "Failed to hash pod volumes, skipping annotation")
} else if hash != "" {
deploymentAnnotations[podVolumesHashAnnotation] = hash
}

Raised by: Go code quality agent, General code quality agent

deploymentAnnotations[podVolumesHashAnnotation] = hash
}
}

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

return deploymentLabels, deploymentAnnotations
Expand All @@ -944,6 +981,104 @@ func imagePullSecretsHash(secrets []corev1.LocalObjectReference) (string, error)
return hex.EncodeToString(h[:]), nil
}

// podVolumeHashEntry captures drift-relevant volume fields without K8s-defaulted noise.
type podVolumeHashEntry struct {
Name string `json:"name"`
Source string `json:"source"`
}

// podVolumeMountHashEntry captures drift-relevant volumeMount fields.
type podVolumeMountHashEntry struct {
Name string `json:"name"`
MountPath string `json:"mountPath"`
SubPath string `json:"subPath,omitempty"`
ReadOnly bool `json:"readOnly,omitempty"`
}

// podVolumesHashPayload is the canonical form hashed by podVolumesHash.
type podVolumesHashPayload struct {
Volumes []podVolumeHashEntry `json:"volumes"`
VolumeMounts []podVolumeMountHashEntry `json:"volumeMounts"`
}

// volumeSourceFingerprint returns a stable identifier for the backing resource
// referenced by a volume, ignoring K8s-defaulted fields like defaultMode.
func volumeSourceFingerprint(vol corev1.Volume) string {
switch {
case vol.ConfigMap != nil:
src := "configmap:" + vol.ConfigMap.Name
if len(vol.ConfigMap.Items) > 0 {
items := make([]string, len(vol.ConfigMap.Items))
for i, item := range vol.ConfigMap.Items {
items[i] = item.Key + "=" + item.Path
}
sort.Strings(items)
src += ":" + strings.Join(items, ",")
}
return src
case vol.Secret != nil:
src := "secret:" + vol.Secret.SecretName
if len(vol.Secret.Items) > 0 {
items := make([]string, len(vol.Secret.Items))
for i, item := range vol.Secret.Items {
items[i] = item.Key + "=" + item.Path
}
sort.Strings(items)
src += ":" + strings.Join(items, ",")
}
return src
case vol.EmptyDir != nil:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F7 — LOW] EmptyDir fingerprint ignores Medium and SizeLimit (Consensus: 7/10)

Two EmptyDir volumes with different Medium (e.g. StorageMediumMemory vs default) or SizeLimit would produce identical fingerprints if they share a name. Since volume names are unique per pod, the name in the hash payload still distinguishes different EmptyDirs — but a change to an existing volume's Medium or SizeLimit (without a name change) would go undetected. This is inconsistent with the specificity applied to ConfigMap and Secret fingerprints.

Suggested change
case vol.EmptyDir != nil:
case vol.EmptyDir != nil:
sizeStr := "0"
if vol.EmptyDir.SizeLimit != nil {
sizeStr = vol.EmptyDir.SizeLimit.String()
}
return fmt.Sprintf("emptydir:medium=%s:limit=%s", vol.EmptyDir.Medium, sizeStr)

Raised by: Go code quality agent, General code quality agent

return "emptydir"
default:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F6 — LOW] volumeSourceFingerprint default branch misses backing-ref changes for unhandled volume types (Consensus: 7/10)

The default returns "unknown:" + vol.Name, capturing identity (name) but not the backing resource reference. If a new volume type is added to buildPodVolumesForVmcp without a corresponding case here — e.g. a Projected volume combining multiple secrets — a change to that volume's backing refs won't change the fingerprint and drift will be silently missed. Given this file's history with silent drift bugs (#5616, #5619), consider adding a defensive comment:

Suggested change
default:
default:
// WARNING: This volume type is not fingerprinted by backing resource ref.
// Adding a new volume source to buildPodVolumesForVmcp MUST add a corresponding
// case here; this fallback detects the volume by name only and will not detect
// backing-ref changes (e.g. rotating a secret name).
return "unknown:" + vol.Name

Raised by: Kubernetes operator patterns agent, General code quality agent

return "unknown:" + vol.Name
}
}

// podVolumesHash returns a deterministic SHA256 hash of the given volumes and
// volumeMounts. Returns an empty string with no error when both lists are empty.
func podVolumesHash(mounts []corev1.VolumeMount, volumes []corev1.Volume) (string, error) {
if len(mounts) == 0 && len(volumes) == 0 {
return "", nil
}

payload := podVolumesHashPayload{
Volumes: make([]podVolumeHashEntry, len(volumes)),
VolumeMounts: make([]podVolumeMountHashEntry, len(mounts)),
}

for i, vol := range volumes {
payload.Volumes[i] = podVolumeHashEntry{
Name: vol.Name,
Source: volumeSourceFingerprint(vol),
}
}
sort.Slice(payload.Volumes, func(i, j int) bool {
return payload.Volumes[i].Name < payload.Volumes[j].Name
})

for i, mount := range mounts {
payload.VolumeMounts[i] = podVolumeMountHashEntry{
Name: mount.Name,
MountPath: mount.MountPath,
SubPath: mount.SubPath,
ReadOnly: mount.ReadOnly,
}
}
sort.Slice(payload.VolumeMounts, func(i, j int) bool {
if payload.VolumeMounts[i].Name != payload.VolumeMounts[j].Name {
return payload.VolumeMounts[i].Name < payload.VolumeMounts[j].Name
}
return payload.VolumeMounts[i].MountPath < payload.VolumeMounts[j].MountPath
})

canonical, err := json.Marshal(payload)
if err != nil {
return "", fmt.Errorf("failed to marshal pod volumes for hashing: %w", err)
}
h := sha256.Sum256(canonical)
return hex.EncodeToString(h[:]), nil
}

// buildPodTemplateMetadata builds pod template labels and annotations for vmcp
func (*VirtualMCPServerReconciler) buildPodTemplateMetadata(
baseLabels map[string]string,
Expand Down
Loading
Loading