Skip to content

Added spec.podTemplateSpec support to MCPRemoteProxy #5531

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Sanskarzz wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Added spec.podTemplateSpec support to MCPRemoteProxy #5531

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 20 additions & 0 deletions cmd/thv-operator/api/v1beta1/mcpremoteproxy_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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"
)
Expand Down Expand Up @@ -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"

Expand Down
5 changes: 5 additions & 0 deletions cmd/thv-operator/api/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

191 changes: 186 additions & 5 deletions cmd/thv-operator/controllers/mcpremoteproxy_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}

Expand All @@ -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
}

Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
}
Expand All @@ -1429,19 +1570,59 @@ 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
// defaults with spec.resourceOverrides.proxyDeployment.imagePullSecrets. Uses
// 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
Expand Down
Loading
Loading