From 9365a26ce1691dc0a36204dcd3ee08b6a308ce8a Mon Sep 17 00:00:00 2001 From: Sanskarzz Date: Tue, 16 Jun 2026 02:07:13 +0530 Subject: [PATCH] Add spec.podTemplateSpec support to MCPRemoteProxy Signed-off-by: Sanskarzz --- .../api/v1beta1/mcpremoteproxy_types.go | 20 ++ .../api/v1beta1/zz_generated.deepcopy.go | 5 + .../controllers/mcpremoteproxy_controller.go | 191 +++++++++++++- .../controllers/mcpremoteproxy_deployment.go | 45 +++- .../mcpremoteproxy_podtemplatespec_test.go | 240 ++++++++++++++++++ .../controllers/podtemplatespec_constants.go | 11 + .../virtualmcpserver_deployment.go | 4 - ...oolhive.stacklok.dev_mcpremoteproxies.yaml | 18 ++ ...oolhive.stacklok.dev_mcpremoteproxies.yaml | 18 ++ docs/operator/crd-api.md | 1 + 10 files changed, 543 insertions(+), 10 deletions(-) create mode 100644 cmd/thv-operator/controllers/mcpremoteproxy_podtemplatespec_test.go create mode 100644 cmd/thv-operator/controllers/podtemplatespec_constants.go diff --git a/cmd/thv-operator/api/v1beta1/mcpremoteproxy_types.go b/cmd/thv-operator/api/v1beta1/mcpremoteproxy_types.go index 5ba87691aa..69171cd7ea 100644 --- a/cmd/thv-operator/api/v1beta1/mcpremoteproxy_types.go +++ b/cmd/thv-operator/api/v1beta1/mcpremoteproxy_types.go @@ -5,6 +5,7 @@ package v1beta1 import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" ) // HeaderForwardConfig defines header forward configuration for remote servers. @@ -133,6 +134,16 @@ type MCPRemoteProxySpec struct { // +optional ServiceAccount *string `json:"serviceAccount,omitempty"` + // PodTemplateSpec defines the pod template to use for the MCPRemoteProxy + // This allows for customizing the pod configuration beyond what is provided by the other fields. + // Note that to modify the specific container the remote proxy runs in, you must specify + // the `toolhive` container name in the PodTemplateSpec. + // This field accepts a PodTemplateSpec object as JSON/YAML. + // +optional + // +kubebuilder:pruning:PreserveUnknownFields + // +kubebuilder:validation:Type=object + PodTemplateSpec *runtime.RawExtension `json:"podTemplateSpec,omitempty"` + // TrustProxyHeaders indicates whether to trust X-Forwarded-* headers from reverse proxies // When enabled, the proxy will use X-Forwarded-Proto, X-Forwarded-Host, X-Forwarded-Port, // and X-Forwarded-Prefix headers to construct endpoint URLs @@ -258,6 +269,9 @@ const ( // ConditionTypeMCPRemoteProxyAuthServerRefValidated indicates whether the AuthServerRef is valid ConditionTypeMCPRemoteProxyAuthServerRefValidated = "AuthServerRefValidated" + // ConditionTypeMCPRemoteProxyPodTemplateValid indicates whether the PodTemplateSpec is valid + ConditionTypeMCPRemoteProxyPodTemplateValid = "PodTemplateValid" + // ConditionTypeConfigurationValid indicates whether the proxy spec has passed all pre-deployment validation checks ConditionTypeConfigurationValid = "ConfigurationValid" ) @@ -346,6 +360,12 @@ const ( // ConditionReasonMCPRemoteProxyAuthServerRefMultiUpstream indicates multi-upstream is not supported ConditionReasonMCPRemoteProxyAuthServerRefMultiUpstream = "MultiUpstreamNotSupported" + // ConditionReasonMCPRemoteProxyPodTemplateValid indicates PodTemplateSpec validation succeeded + ConditionReasonMCPRemoteProxyPodTemplateValid = "ValidPodTemplateSpec" + + // ConditionReasonMCPRemoteProxyPodTemplateInvalid indicates PodTemplateSpec validation failed + ConditionReasonMCPRemoteProxyPodTemplateInvalid = "InvalidPodTemplateSpec" + // ConditionReasonConfigurationValid indicates all configuration validations passed ConditionReasonConfigurationValid = "ConfigurationValid" diff --git a/cmd/thv-operator/api/v1beta1/zz_generated.deepcopy.go b/cmd/thv-operator/api/v1beta1/zz_generated.deepcopy.go index 14a4e22dcd..c269d443ac 100644 --- a/cmd/thv-operator/api/v1beta1/zz_generated.deepcopy.go +++ b/cmd/thv-operator/api/v1beta1/zz_generated.deepcopy.go @@ -1447,6 +1447,11 @@ func (in *MCPRemoteProxySpec) DeepCopyInto(out *MCPRemoteProxySpec) { *out = new(string) **out = **in } + if in.PodTemplateSpec != nil { + in, out := &in.PodTemplateSpec, &out.PodTemplateSpec + *out = new(runtime.RawExtension) + (*in).DeepCopyInto(*out) + } if in.ResourceOverrides != nil { in, out := &in.ResourceOverrides, &out.ResourceOverrides *out = new(ResourceOverrides) diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go index ce18e84019..8a45ebada0 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go @@ -48,6 +48,8 @@ type MCPRemoteProxyReconciler struct { ImagePullSecretsDefaults imagepullsecrets.Defaults } +var errInvalidMCPRemoteProxyPodTemplateSpec = stderrors.New("invalid MCPRemoteProxy PodTemplateSpec") + // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpremoteproxies,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpremoteproxies/status,verbs=get;update;patch // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcptoolconfigs,verbs=get;list;watch @@ -83,6 +85,9 @@ func (r *MCPRemoteProxyReconciler) Reconcile(ctx context.Context, req ctrl.Reque // Validate and handle configurations if err := r.validateAndHandleConfigs(ctx, proxy); err != nil { + if stderrors.Is(err, errInvalidMCPRemoteProxyPodTemplateSpec) { + return ctrl.Result{}, nil + } return ctrl.Result{}, err } @@ -121,6 +126,11 @@ func (r *MCPRemoteProxyReconciler) validateAndHandleConfigs(ctx context.Context, return err } + // Validate PodTemplateSpec early - before creating or updating resources. + if !r.validateAndUpdatePodTemplateStatus(ctx, proxy) { + return errInvalidMCPRemoteProxyPodTemplateSpec + } + // Validate the GroupRef if specified r.validateGroupRef(ctx, proxy) @@ -180,6 +190,65 @@ func (r *MCPRemoteProxyReconciler) validateAndHandleConfigs(ctx context.Context, return nil } +// validateAndUpdatePodTemplateStatus validates the PodTemplateSpec and updates status conditions. +// It returns false when the PodTemplateSpec is invalid and reconciliation should stop until the user fixes it. +func (r *MCPRemoteProxyReconciler) validateAndUpdatePodTemplateStatus( + ctx context.Context, + proxy *mcpv1beta1.MCPRemoteProxy, +) bool { + ctxLogger := log.FromContext(ctx) + + if proxy.Spec.PodTemplateSpec == nil || proxy.Spec.PodTemplateSpec.Raw == nil { + return true + } + + _, err := ctrlutil.NewPodTemplateSpecBuilder(proxy.Spec.PodTemplateSpec, mcpRemoteProxyContainerName) + if err != nil { + if r.Recorder != nil { + r.Recorder.Eventf(proxy, nil, corev1.EventTypeWarning, "InvalidPodTemplateSpec", "ValidatePodTemplateSpec", + "Failed to parse PodTemplateSpec: %v. Deployment blocked until PodTemplateSpec is fixed.", err) + } + + proxy.Status.Phase = mcpv1beta1.MCPRemoteProxyPhaseFailed + proxy.Status.Message = fmt.Sprintf("Invalid PodTemplateSpec: %v", err) + meta.SetStatusCondition(&proxy.Status.Conditions, metav1.Condition{ + Type: mcpv1beta1.ConditionTypeMCPRemoteProxyPodTemplateValid, + Status: metav1.ConditionFalse, + ObservedGeneration: proxy.Generation, + Reason: mcpv1beta1.ConditionReasonMCPRemoteProxyPodTemplateInvalid, + Message: fmt.Sprintf("Failed to parse PodTemplateSpec: %v. Deployment blocked until fixed.", err), + }) + meta.SetStatusCondition(&proxy.Status.Conditions, metav1.Condition{ + Type: mcpv1beta1.ConditionTypeReady, + Status: metav1.ConditionFalse, + ObservedGeneration: proxy.Generation, + Reason: mcpv1beta1.ConditionReasonDeploymentNotReady, + Message: fmt.Sprintf("Invalid PodTemplateSpec: %v", err), + }) + + if statusErr := r.Status().Update(ctx, proxy); statusErr != nil { + ctxLogger.Error(statusErr, "Failed to update MCPRemoteProxy status with PodTemplateSpec validation") + return false + } + + ctxLogger.Error(err, "PodTemplateSpec validation failed") + return false + } + + meta.SetStatusCondition(&proxy.Status.Conditions, metav1.Condition{ + Type: mcpv1beta1.ConditionTypeMCPRemoteProxyPodTemplateValid, + Status: metav1.ConditionTrue, + ObservedGeneration: proxy.Generation, + Reason: mcpv1beta1.ConditionReasonMCPRemoteProxyPodTemplateValid, + Message: "PodTemplateSpec is valid", + }) + if statusErr := r.Status().Update(ctx, proxy); statusErr != nil { + ctxLogger.Error(statusErr, "Failed to update MCPRemoteProxy status with PodTemplateSpec validation") + } + + return true +} + // ensureAllResources ensures all Kubernetes resources for the proxy func (r *MCPRemoteProxyReconciler) ensureAllResources(ctx context.Context, proxy *mcpv1beta1.MCPRemoteProxy) error { ctxLogger := log.FromContext(ctx) @@ -305,6 +374,9 @@ func (r *MCPRemoteProxyReconciler) ensureDeployment( deployment.Spec.Template = newDeployment.Spec.Template deployment.Labels = newDeployment.Labels deployment.Annotations = ctrlutil.MergeAnnotations(newDeployment.Annotations, deployment.Annotations) + if proxy.Spec.PodTemplateSpec == nil || len(proxy.Spec.PodTemplateSpec.Raw) == 0 { + delete(deployment.Annotations, podTemplateSpecHashAnnotation) + } ctxLogger.Info("Updating Deployment", "Deployment.Namespace", deployment.Namespace, "Deployment.Name", deployment.Name) if err := r.Update(ctx, deployment); err != nil { @@ -1284,7 +1356,7 @@ func (r *MCPRemoteProxyReconciler) deploymentNeedsUpdate( return true } - if r.containerNeedsUpdate(ctx, deployment, proxy) { + if r.containerNeedsUpdate(ctx, deployment, proxy, runConfigChecksum) { return true } @@ -1296,7 +1368,11 @@ func (r *MCPRemoteProxyReconciler) deploymentNeedsUpdate( return true } - if r.podSpecNeedsUpdate(deployment, proxy) { + if r.podTemplateSpecNeedsUpdate(ctx, deployment, proxy) { + return true + } + + if r.podSpecNeedsUpdate(ctx, deployment, proxy, runConfigChecksum) { return true } @@ -1311,11 +1387,45 @@ func (r *MCPRemoteProxyReconciler) containerNeedsUpdate( ctx context.Context, deployment *appsv1.Deployment, proxy *mcpv1beta1.MCPRemoteProxy, + runConfigChecksum string, ) bool { if deployment == nil || proxy == nil || len(deployment.Spec.Template.Spec.Containers) == 0 { return true } + if proxy.Spec.PodTemplateSpec != nil && len(proxy.Spec.PodTemplateSpec.Raw) > 0 { + return r.containerNeedsUpdateWithPodTemplate(ctx, deployment, proxy, runConfigChecksum) + } + + return r.generatedContainerNeedsUpdate(ctx, deployment, proxy) +} + +func (r *MCPRemoteProxyReconciler) containerNeedsUpdateWithPodTemplate( + ctx context.Context, + deployment *appsv1.Deployment, + proxy *mcpv1beta1.MCPRemoteProxy, + runConfigChecksum string, +) bool { + expectedDeployment := r.deploymentForMCPRemoteProxy(ctx, proxy, runConfigChecksum) + if expectedDeployment == nil { + return true + } + currentContainer, ok := findContainerByName(deployment.Spec.Template.Spec.Containers, mcpRemoteProxyContainerName) + if !ok { + return true + } + expectedContainer, ok := findContainerByName(expectedDeployment.Spec.Template.Spec.Containers, mcpRemoteProxyContainerName) + if !ok { + return true + } + return remoteProxyContainerFieldsNeedUpdate(currentContainer, expectedContainer) +} + +func (r *MCPRemoteProxyReconciler) generatedContainerNeedsUpdate( + ctx context.Context, + deployment *appsv1.Deployment, + proxy *mcpv1beta1.MCPRemoteProxy, +) bool { container := deployment.Spec.Template.Spec.Containers[0] // Check if runner image has changed @@ -1353,7 +1463,7 @@ func (r *MCPRemoteProxyReconciler) containerNeedsUpdate( } // Check if service account has changed - expectedServiceAccountName := proxyRunnerServiceAccountNameForRemoteProxy(proxy.Name) + expectedServiceAccountName := serviceAccountNameForRemoteProxy(proxy) currentServiceAccountName := deployment.Spec.Template.Spec.ServiceAccountName if currentServiceAccountName != "" && currentServiceAccountName != expectedServiceAccountName { return true @@ -1362,6 +1472,32 @@ func (r *MCPRemoteProxyReconciler) containerNeedsUpdate( return false } +func findContainerByName(containers []corev1.Container, name string) (*corev1.Container, bool) { + for i := range containers { + if containers[i].Name == name { + return &containers[i], true + } + } + return nil, false +} + +func remoteProxyContainerFieldsNeedUpdate(current, expected *corev1.Container) bool { + if current == nil || expected == nil { + return true + } + + return current.Image != expected.Image || + !equality.Semantic.DeepEqual(current.Args, expected.Args) || + !equality.Semantic.DeepEqual(current.Env, expected.Env) || + !equality.Semantic.DeepEqual(current.VolumeMounts, expected.VolumeMounts) || + !equality.Semantic.DeepEqual(current.Resources, expected.Resources) || + !equality.Semantic.DeepEqual(current.Ports, expected.Ports) || + !equality.Semantic.DeepEqual(current.StartupProbe, expected.StartupProbe) || + !equality.Semantic.DeepEqual(current.LivenessProbe, expected.LivenessProbe) || + !equality.Semantic.DeepEqual(current.ReadinessProbe, expected.ReadinessProbe) || + !equality.Semantic.DeepEqual(current.SecurityContext, expected.SecurityContext) +} + // deploymentMetadataNeedsUpdate checks if deployment-level metadata has changed. // // Compares deployment labels and annotations, including any user-specified overrides @@ -1418,6 +1554,11 @@ func (r *MCPRemoteProxyReconciler) podTemplateMetadataNeedsUpdate( labelsForMCPRemoteProxy(proxy.Name), proxy, runConfigChecksum, ) + if proxy.Spec.PodTemplateSpec != nil && len(proxy.Spec.PodTemplateSpec.Raw) > 0 { + return !ctrlutil.MapIsSubset(expectedPodTemplateLabels, deployment.Spec.Template.Labels) || + !ctrlutil.MapIsSubset(expectedPodTemplateAnnotations, deployment.Spec.Template.Annotations) + } + if !maps.Equal(deployment.Spec.Template.Labels, expectedPodTemplateLabels) { return true } @@ -1429,6 +1570,29 @@ func (r *MCPRemoteProxyReconciler) podTemplateMetadataNeedsUpdate( return false } +// podTemplateSpecNeedsUpdate checks whether the user-provided PodTemplateSpec raw input changed. +func (*MCPRemoteProxyReconciler) podTemplateSpecNeedsUpdate( + ctx context.Context, + deployment *appsv1.Deployment, + proxy *mcpv1beta1.MCPRemoteProxy, +) bool { + if deployment == nil || proxy == nil { + return true + } + + if proxy.Spec.PodTemplateSpec == nil || proxy.Spec.PodTemplateSpec.Raw == nil { + _, hadPrevious := deployment.Annotations[podTemplateSpecHashAnnotation] + return hadPrevious + } + + expectedHash, err := checksum.HashRawJSON(proxy.Spec.PodTemplateSpec.Raw) + if err != nil { + log.FromContext(ctx).Error(err, "Failed to hash PodTemplateSpec, assuming update needed") + return true + } + return deployment.Annotations[podTemplateSpecHashAnnotation] != expectedHash +} + // podSpecNeedsUpdate checks if pod-level fields (not container fields) have drifted. // // Currently compares ImagePullSecrets — the merge of cluster-wide chart @@ -1436,12 +1600,29 @@ func (r *MCPRemoteProxyReconciler) podTemplateMetadataNeedsUpdate( // equality.Semantic.DeepEqual so nil and empty slices are treated as equal, // which matches Kubernetes' own serialization semantics. func (r *MCPRemoteProxyReconciler) podSpecNeedsUpdate( + ctx context.Context, deployment *appsv1.Deployment, proxy *mcpv1beta1.MCPRemoteProxy, + runConfigChecksum string, ) bool { + if proxy.Spec.PodTemplateSpec != nil && len(proxy.Spec.PodTemplateSpec.Raw) > 0 { + expectedDeployment := r.deploymentForMCPRemoteProxy(ctx, proxy, runConfigChecksum) + if expectedDeployment == nil { + return true + } + return deployment.Spec.Template.Spec.ServiceAccountName != expectedDeployment.Spec.Template.Spec.ServiceAccountName || + !equality.Semantic.DeepEqual( + deployment.Spec.Template.Spec.ImagePullSecrets, + expectedDeployment.Spec.Template.Spec.ImagePullSecrets, + ) + } + + if deployment.Spec.Template.Spec.ServiceAccountName != serviceAccountNameForRemoteProxy(proxy) { + return true + } + expected := r.imagePullSecretsForRemoteProxy(proxy) - current := deployment.Spec.Template.Spec.ImagePullSecrets - return !equality.Semantic.DeepEqual(current, expected) + return !equality.Semantic.DeepEqual(deployment.Spec.Template.Spec.ImagePullSecrets, expected) } // serviceNeedsUpdate checks if the service needs to be updated diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go b/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go index 67e195575c..4bf8659e24 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go @@ -21,6 +21,8 @@ import ( "github.com/stacklok/toolhive/pkg/vmcp/headerforward/wirefmt" ) +const mcpRemoteProxyContainerName = "toolhive" + // deploymentForMCPRemoteProxy returns a MCPRemoteProxy Deployment object func (r *MCPRemoteProxyReconciler) deploymentForMCPRemoteProxy( ctx context.Context, proxy *mcpv1beta1.MCPRemoteProxy, runConfigChecksum string, @@ -76,7 +78,7 @@ func (r *MCPRemoteProxyReconciler) deploymentForMCPRemoteProxy( ImagePullSecrets: r.imagePullSecretsForRemoteProxy(proxy), Containers: []corev1.Container{{ Image: getToolhiveRunnerImage(), - Name: "toolhive", + Name: mcpRemoteProxyContainerName, Args: args, Env: env, VolumeMounts: volumeMounts, @@ -94,6 +96,13 @@ func (r *MCPRemoteProxyReconciler) deploymentForMCPRemoteProxy( }, } + if err := r.applyPodTemplateSpecToDeployment(ctx, proxy, dep); err != nil { + log.FromContext(ctx).Error(err, "Failed to apply PodTemplateSpec to Deployment", + "mcpremoteproxy", proxy.Name, + "namespace", proxy.Namespace) + return nil + } + if err := controllerutil.SetControllerReference(proxy, dep, r.Scheme); err != nil { ctxLogger := log.FromContext(ctx) ctxLogger.Error(err, "Failed to set controller reference for Deployment") @@ -305,6 +314,13 @@ func (*MCPRemoteProxyReconciler) buildDeploymentMetadata( deploymentLabels := baseLabels deploymentAnnotations := make(map[string]string) + if proxy.Spec.PodTemplateSpec != nil && len(proxy.Spec.PodTemplateSpec.Raw) > 0 { + hash, err := checksum.HashRawJSON(proxy.Spec.PodTemplateSpec.Raw) + if err == nil { + deploymentAnnotations[podTemplateSpecHashAnnotation] = hash + } + } + if proxy.Spec.ResourceOverrides != nil && proxy.Spec.ResourceOverrides.ProxyDeployment != nil { if proxy.Spec.ResourceOverrides.ProxyDeployment.Labels != nil { deploymentLabels = ctrlutil.MergeLabels(baseLabels, proxy.Spec.ResourceOverrides.ProxyDeployment.Labels) @@ -319,6 +335,33 @@ func (*MCPRemoteProxyReconciler) buildDeploymentMetadata( return deploymentLabels, deploymentAnnotations } +// applyPodTemplateSpecToDeployment applies user-provided PodTemplateSpec customizations +// to the generated MCPRemoteProxy Deployment using Kubernetes strategic merge semantics. +func (*MCPRemoteProxyReconciler) applyPodTemplateSpecToDeployment( + ctx context.Context, + proxy *mcpv1beta1.MCPRemoteProxy, + deployment *appsv1.Deployment, +) error { + if proxy.Spec.PodTemplateSpec == nil || len(proxy.Spec.PodTemplateSpec.Raw) == 0 { + return nil + } + + if _, err := ctrlutil.NewPodTemplateSpecBuilder(proxy.Spec.PodTemplateSpec, mcpRemoteProxyContainerName); err != nil { + return fmt.Errorf("failed to build PodTemplateSpec: %w", err) + } + + merged, err := ctrlutil.ApplyPodTemplateSpecPatch(deployment.Spec.Template, proxy.Spec.PodTemplateSpec.Raw) + if err != nil { + return err + } + deployment.Spec.Template = merged + + log.FromContext(ctx).V(1).Info("Applied PodTemplateSpec customizations to deployment", + "mcpremoteproxy", proxy.Name, + "namespace", proxy.Namespace) + return nil +} + // buildPodTemplateMetadata builds pod template labels and annotations. // // The runConfigChecksum parameter must be a non-empty SHA256 hash of the RunConfig. diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_podtemplatespec_test.go b/cmd/thv-operator/controllers/mcpremoteproxy_podtemplatespec_test.go new file mode 100644 index 0000000000..e4594ade58 --- /dev/null +++ b/cmd/thv-operator/controllers/mcpremoteproxy_podtemplatespec_test.go @@ -0,0 +1,240 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package controllers + +import ( + "encoding/json" + stderrors "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/events" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1" + ctrlutil "github.com/stacklok/toolhive/cmd/thv-operator/pkg/controllerutil" +) + +func rawPodTemplateSpecJSON(t *testing.T, raw string) *runtime.RawExtension { + t.Helper() + var obj map[string]any + require.NoError(t, json.Unmarshal([]byte(raw), &obj)) + return &runtime.RawExtension{Raw: []byte(raw)} +} + +func TestMCPRemoteProxyPodTemplateSpecValidation(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + podTemplateSpec *runtime.RawExtension + expectError bool + expectedPhase mcpv1beta1.MCPRemoteProxyPhase + expectedCondition metav1.ConditionStatus + expectedReason string + expectedStatusMessage string + }{ + { + name: "valid PodTemplateSpec", + podTemplateSpec: rawPodTemplateSpecJSON(t, `{"spec":{"nodeSelector":{"disk":"ssd"}}}`), + expectedCondition: metav1.ConditionTrue, + expectedReason: mcpv1beta1.ConditionReasonMCPRemoteProxyPodTemplateValid, + }, + { + name: "invalid PodTemplateSpec", + podTemplateSpec: &runtime.RawExtension{ + Raw: []byte(`{"spec":{"containers":"not-a-container-list"}}`), + }, + expectError: true, + expectedPhase: mcpv1beta1.MCPRemoteProxyPhaseFailed, + expectedCondition: metav1.ConditionFalse, + expectedReason: mcpv1beta1.ConditionReasonMCPRemoteProxyPodTemplateInvalid, + expectedStatusMessage: "Invalid PodTemplateSpec", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + proxy := &mcpv1beta1.MCPRemoteProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "template-proxy", + Namespace: "default", + Generation: 3, + }, + Spec: mcpv1beta1.MCPRemoteProxySpec{ + RemoteURL: "https://mcp.example.com", + PodTemplateSpec: tt.podTemplateSpec, + }, + } + scheme := createRunConfigTestScheme() + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithRuntimeObjects(proxy). + WithStatusSubresource(&mcpv1beta1.MCPRemoteProxy{}). + Build() + reconciler := &MCPRemoteProxyReconciler{ + Client: fakeClient, + Scheme: scheme, + Recorder: events.NewFakeRecorder(10), + } + + err := reconciler.validateAndHandleConfigs(t.Context(), proxy) + if tt.expectError { + require.Error(t, err) + assert.True(t, stderrors.Is(err, errInvalidMCPRemoteProxyPodTemplateSpec)) + } else { + require.NoError(t, err) + } + + updated := &mcpv1beta1.MCPRemoteProxy{} + require.NoError(t, fakeClient.Get(t.Context(), types.NamespacedName{ + Name: proxy.Name, + Namespace: proxy.Namespace, + }, updated)) + + condition := meta.FindStatusCondition( + updated.Status.Conditions, + mcpv1beta1.ConditionTypeMCPRemoteProxyPodTemplateValid, + ) + require.NotNil(t, condition) + assert.Equal(t, tt.expectedCondition, condition.Status) + assert.Equal(t, tt.expectedReason, condition.Reason) + assert.Equal(t, proxy.Generation, condition.ObservedGeneration) + + if tt.expectedPhase != "" { + assert.Equal(t, tt.expectedPhase, updated.Status.Phase) + } + if tt.expectedStatusMessage != "" { + assert.Contains(t, updated.Status.Message, tt.expectedStatusMessage) + } + }) + } +} + +func TestMCPRemoteProxyPodTemplateSpecMerge(t *testing.T) { + t.Parallel() + + proxy := &mcpv1beta1.MCPRemoteProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "template-proxy", + Namespace: "default", + }, + Spec: mcpv1beta1.MCPRemoteProxySpec{ + RemoteURL: "https://mcp.example.com", + PodTemplateSpec: rawPodTemplateSpecJSON(t, `{ + "metadata": { + "labels": {"custom-template-label": "enabled"}, + "annotations": {"custom-template-annotation": "enabled"} + }, + "spec": { + "nodeSelector": {"disk": "ssd"}, + "tolerations": [{ + "key": "dedicated", + "operator": "Equal", + "value": "toolhive", + "effect": "NoSchedule" + }], + "containers": [{ + "name": "toolhive", + "env": [{"name": "EXTRA_PROXY_ENV", "value": "enabled"}] + }, { + "name": "sidecar", + "image": "busybox:latest", + "args": ["sleep", "3600"] + }] + } + }`), + }, + } + + scheme := createRunConfigTestScheme() + reconciler := &MCPRemoteProxyReconciler{ + Scheme: scheme, + PlatformDetector: ctrlutil.NewSharedPlatformDetector(), + } + + deployment := reconciler.deploymentForMCPRemoteProxy(t.Context(), proxy, "test-checksum") + require.NotNil(t, deployment) + + assert.Equal(t, "ssd", deployment.Spec.Template.Spec.NodeSelector["disk"]) + require.Len(t, deployment.Spec.Template.Spec.Tolerations, 1) + assert.Equal(t, "enabled", deployment.Spec.Template.Labels["custom-template-label"]) + assert.Equal(t, "enabled", deployment.Spec.Template.Annotations["custom-template-annotation"]) + assert.Contains(t, deployment.Annotations, podTemplateSpecHashAnnotation) + + toolhiveContainer, ok := findContainerByName(deployment.Spec.Template.Spec.Containers, mcpRemoteProxyContainerName) + require.True(t, ok) + assert.Equal(t, getToolhiveRunnerImage(), toolhiveContainer.Image) + assert.Contains(t, toolhiveContainer.Args, "run") + assert.Contains(t, toolhiveContainer.Env, corev1.EnvVar{Name: "EXTRA_PROXY_ENV", Value: "enabled"}) + + sidecar, ok := findContainerByName(deployment.Spec.Template.Spec.Containers, "sidecar") + require.True(t, ok) + assert.Equal(t, "busybox:latest", sidecar.Image) +} + +func TestMCPRemoteProxyPodTemplateSpecMergeAppliesFieldsOutsideBuilderEmptinessCheck(t *testing.T) { + t.Parallel() + + proxy := &mcpv1beta1.MCPRemoteProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "template-proxy", + Namespace: "default", + }, + Spec: mcpv1beta1.MCPRemoteProxySpec{ + RemoteURL: "https://mcp.example.com", + PodTemplateSpec: rawPodTemplateSpecJSON(t, `{"spec":{"runtimeClassName":"gvisor"}}`), + }, + } + + scheme := createRunConfigTestScheme() + reconciler := &MCPRemoteProxyReconciler{ + Scheme: scheme, + PlatformDetector: ctrlutil.NewSharedPlatformDetector(), + } + + deployment := reconciler.deploymentForMCPRemoteProxy(t.Context(), proxy, "test-checksum") + require.NotNil(t, deployment) + require.NotNil(t, deployment.Spec.Template.Spec.RuntimeClassName) + assert.Equal(t, "gvisor", *deployment.Spec.Template.Spec.RuntimeClassName) +} + +func TestMCPRemoteProxyPodTemplateSpecDriftDetection(t *testing.T) { + t.Parallel() + + proxy := &mcpv1beta1.MCPRemoteProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "template-proxy", + Namespace: "default", + }, + Spec: mcpv1beta1.MCPRemoteProxySpec{ + RemoteURL: "https://mcp.example.com", + PodTemplateSpec: rawPodTemplateSpecJSON(t, `{"spec":{"nodeSelector":{"disk":"ssd"}}}`), + }, + } + + scheme := createRunConfigTestScheme() + reconciler := &MCPRemoteProxyReconciler{ + Scheme: scheme, + PlatformDetector: ctrlutil.NewSharedPlatformDetector(), + } + + deployment := reconciler.deploymentForMCPRemoteProxy(t.Context(), proxy, "test-checksum") + require.NotNil(t, deployment) + assert.False(t, reconciler.deploymentNeedsUpdate(t.Context(), deployment, proxy, "test-checksum")) + + proxy.Spec.PodTemplateSpec = rawPodTemplateSpecJSON(t, `{"spec":{"nodeSelector":{"disk":"nvme"}}}`) + assert.True(t, reconciler.deploymentNeedsUpdate(t.Context(), deployment, proxy, "test-checksum")) + + proxy.Spec.PodTemplateSpec = nil + assert.True(t, reconciler.deploymentNeedsUpdate(t.Context(), deployment, proxy, "test-checksum")) +} diff --git a/cmd/thv-operator/controllers/podtemplatespec_constants.go b/cmd/thv-operator/controllers/podtemplatespec_constants.go new file mode 100644 index 0000000000..bffaf9f7c3 --- /dev/null +++ b/cmd/thv-operator/controllers/podtemplatespec_constants.go @@ -0,0 +1,11 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package controllers + +const ( + // podTemplateSpecHashAnnotation tracks the SHA256 hash of a user-provided PodTemplateSpec. + // It is used by workload reconcilers to detect changes without comparing full rendered + // templates, which may include Kubernetes-defaulted fields. + podTemplateSpecHashAnnotation = "toolhive.stacklok.io/podtemplatespec-hash" +) diff --git a/cmd/thv-operator/controllers/virtualmcpserver_deployment.go b/cmd/thv-operator/controllers/virtualmcpserver_deployment.go index 6cf5061d4b..b002a4def0 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_deployment.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_deployment.go @@ -35,10 +35,6 @@ import ( ) const ( - // podTemplateSpecHashAnnotation tracks the SHA256 hash of the user-provided PodTemplateSpec. - // Used to detect changes without comparing full rendered templates (which include K8s-defaulted fields). - podTemplateSpecHashAnnotation = "toolhive.stacklok.io/podtemplatespec-hash" - // imagePullRefsHashAnnotation tracks the SHA256 hash of the desired // imagePullSecrets list — chart-level defaults merged with // vmcp.Spec.ImagePullSecrets — used by buildDeploymentMetadataForVmcp. diff --git a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpremoteproxies.yaml b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpremoteproxies.yaml index fe5e8a8a22..9abd50c5f7 100644 --- a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpremoteproxies.yaml +++ b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpremoteproxies.yaml @@ -350,6 +350,15 @@ spec: - audience - name type: object + podTemplateSpec: + description: |- + PodTemplateSpec defines the pod template to use for the MCPRemoteProxy + This allows for customizing the pod configuration beyond what is provided by the other fields. + Note that to modify the specific container the remote proxy runs in, you must specify + the `toolhive` container name in the PodTemplateSpec. + This field accepts a PodTemplateSpec object as JSON/YAML. + type: object + x-kubernetes-preserve-unknown-fields: true proxyPort: default: 8080 description: ProxyPort is the port to expose the MCP proxy on @@ -1002,6 +1011,15 @@ spec: - audience - name type: object + podTemplateSpec: + description: |- + PodTemplateSpec defines the pod template to use for the MCPRemoteProxy + This allows for customizing the pod configuration beyond what is provided by the other fields. + Note that to modify the specific container the remote proxy runs in, you must specify + the `toolhive` container name in the PodTemplateSpec. + This field accepts a PodTemplateSpec object as JSON/YAML. + type: object + x-kubernetes-preserve-unknown-fields: true proxyPort: default: 8080 description: ProxyPort is the port to expose the MCP proxy on diff --git a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpremoteproxies.yaml b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpremoteproxies.yaml index 537607ff2b..6a4d02d011 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpremoteproxies.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpremoteproxies.yaml @@ -353,6 +353,15 @@ spec: - audience - name type: object + podTemplateSpec: + description: |- + PodTemplateSpec defines the pod template to use for the MCPRemoteProxy + This allows for customizing the pod configuration beyond what is provided by the other fields. + Note that to modify the specific container the remote proxy runs in, you must specify + the `toolhive` container name in the PodTemplateSpec. + This field accepts a PodTemplateSpec object as JSON/YAML. + type: object + x-kubernetes-preserve-unknown-fields: true proxyPort: default: 8080 description: ProxyPort is the port to expose the MCP proxy on @@ -1005,6 +1014,15 @@ spec: - audience - name type: object + podTemplateSpec: + description: |- + PodTemplateSpec defines the pod template to use for the MCPRemoteProxy + This allows for customizing the pod configuration beyond what is provided by the other fields. + Note that to modify the specific container the remote proxy runs in, you must specify + the `toolhive` container name in the PodTemplateSpec. + This field accepts a PodTemplateSpec object as JSON/YAML. + type: object + x-kubernetes-preserve-unknown-fields: true proxyPort: default: 8080 description: ProxyPort is the port to expose the MCP proxy on diff --git a/docs/operator/crd-api.md b/docs/operator/crd-api.md index f55c8d4384..7ac8cdfd84 100644 --- a/docs/operator/crd-api.md +++ b/docs/operator/crd-api.md @@ -2281,6 +2281,7 @@ _Appears in:_ | `telemetryConfigRef` _[api.v1beta1.MCPTelemetryConfigReference](#apiv1beta1mcptelemetryconfigreference)_ | TelemetryConfigRef references an MCPTelemetryConfig resource for shared telemetry configuration.
The referenced MCPTelemetryConfig must exist in the same namespace as this MCPRemoteProxy.
Cross-namespace references are not supported for security and isolation reasons. | | Optional: \{\}
| | `resources` _[api.v1beta1.ResourceRequirements](#apiv1beta1resourcerequirements)_ | Resources defines the resource requirements for the proxy container | | Optional: \{\}
| | `serviceAccount` _string_ | ServiceAccount is the name of an already existing service account to use by the proxy.
If not specified, a ServiceAccount will be created automatically and used by the proxy. | | Optional: \{\}
| +| `podTemplateSpec` _[RawExtension](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.27/#rawextension-runtime-pkg)_ | PodTemplateSpec defines the pod template to use for the MCPRemoteProxy
This allows for customizing the pod configuration beyond what is provided by the other fields.
Note that to modify the specific container the remote proxy runs in, you must specify
the `toolhive` container name in the PodTemplateSpec.
This field accepts a PodTemplateSpec object as JSON/YAML. | | Type: object
Optional: \{\}
| | `trustProxyHeaders` _boolean_ | TrustProxyHeaders indicates whether to trust X-Forwarded-* headers from reverse proxies
When enabled, the proxy will use X-Forwarded-Proto, X-Forwarded-Host, X-Forwarded-Port,
and X-Forwarded-Prefix headers to construct endpoint URLs | false | Optional: \{\}
| | `endpointPrefix` _string_ | EndpointPrefix is the path prefix to prepend to SSE endpoint URLs.
This is used to handle path-based ingress routing scenarios where the ingress
strips a path prefix before forwarding to the backend. | | Optional: \{\}
| | `resourceOverrides` _[api.v1beta1.ResourceOverrides](#apiv1beta1resourceoverrides)_ | ResourceOverrides allows overriding annotations and labels for resources created by the operator | | Optional: \{\}
|