diff --git a/cmd/thv-operator/api/v1beta1/mcpserverentry_types.go b/cmd/thv-operator/api/v1beta1/mcpserverentry_types.go index 6c5043d6e7..04c989230b 100644 --- a/cmd/thv-operator/api/v1beta1/mcpserverentry_types.go +++ b/cmd/thv-operator/api/v1beta1/mcpserverentry_types.go @@ -101,6 +101,11 @@ const ( // ConditionTypeMCPServerEntryRemoteURLValidated indicates whether the RemoteURL passes // format and SSRF safety checks. ConditionTypeMCPServerEntryRemoteURLValidated = "RemoteURLValidated" + + // ConditionTypeMCPServerEntryHeaderSecretRefsValidated indicates whether every Kubernetes + // Secret referenced by spec.headerForward.addHeadersFromSecret exists. Absent when the + // entry declares no header Secret refs. + ConditionTypeMCPServerEntryHeaderSecretRefsValidated = "HeaderSecretRefsValidated" ) // Condition reasons for MCPServerEntry. @@ -146,6 +151,14 @@ const ( // ConditionReasonMCPServerEntryRemoteURLInvalid indicates the RemoteURL is malformed or // targets a blocked internal/metadata endpoint. ConditionReasonMCPServerEntryRemoteURLInvalid = ConditionReasonRemoteURLInvalid + + // ConditionReasonMCPServerEntryHeaderSecretsValid indicates every referenced header Secret exists. + ConditionReasonMCPServerEntryHeaderSecretsValid = "HeaderSecretsValid" + + // ConditionReasonMCPServerEntryHeaderSecretNotFound indicates a Secret referenced by + // spec.headerForward.addHeadersFromSecret was not found in the entry's namespace. + // Reuses the string value used by MCPRemoteProxy for parity. + ConditionReasonMCPServerEntryHeaderSecretNotFound = ConditionReasonHeaderSecretNotFound ) //+kubebuilder:object:root=true diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go b/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go index bff48386a3..591cdc04a1 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go @@ -18,6 +18,7 @@ import ( ctrlutil "github.com/stacklok/toolhive/cmd/thv-operator/pkg/controllerutil" "github.com/stacklok/toolhive/cmd/thv-operator/pkg/runconfig/configmap/checksum" "github.com/stacklok/toolhive/pkg/container/kubernetes" + "github.com/stacklok/toolhive/pkg/vmcp/headerforward/wirefmt" ) // deploymentForMCPRemoteProxy returns a MCPRemoteProxy Deployment object @@ -278,7 +279,7 @@ func buildHeaderForwardSecretEnvVars(proxy *mcpv1beta1.MCPRemoteProxy) []corev1. } // Generate env var name following the TOOLHIVE_SECRET_ pattern - envVarName, _ := ctrlutil.GenerateHeaderForwardSecretEnvVarName(proxy.Name, headerSecret.HeaderName) + envVarName, _ := wirefmt.SecretEnvVarName(proxy.Name, headerSecret.HeaderName) envVars = append(envVars, corev1.EnvVar{ Name: envVarName, diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_runconfig.go b/cmd/thv-operator/controllers/mcpremoteproxy_runconfig.go index 65cf8cb81f..0059404fb1 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_runconfig.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_runconfig.go @@ -21,6 +21,7 @@ import ( "github.com/stacklok/toolhive/cmd/thv-operator/pkg/runconfig/configmap/checksum" "github.com/stacklok/toolhive/pkg/runner" transporttypes "github.com/stacklok/toolhive/pkg/transport/types" + "github.com/stacklok/toolhive/pkg/vmcp/headerforward/wirefmt" ) // ensureRunConfigConfigMap ensures the RunConfig ConfigMap exists and is up to date for MCPRemoteProxy @@ -291,7 +292,7 @@ func addHeaderForwardConfigOptions(proxy *mcpv1beta1.MCPRemoteProxy, options *[] continue } // Get the secret identifier (not the full env var name) - _, secretIdentifier := ctrlutil.GenerateHeaderForwardSecretEnvVarName(proxy.Name, headerSecret.HeaderName) + _, secretIdentifier := wirefmt.SecretEnvVarName(proxy.Name, headerSecret.HeaderName) headerSecrets[headerSecret.HeaderName] = secretIdentifier } *options = append(*options, runner.WithHeaderForwardSecrets(headerSecrets)) diff --git a/cmd/thv-operator/controllers/mcpserverentry_controller.go b/cmd/thv-operator/controllers/mcpserverentry_controller.go index c34281470f..0ad7a1c5c5 100644 --- a/cmd/thv-operator/controllers/mcpserverentry_controller.go +++ b/cmd/thv-operator/controllers/mcpserverentry_controller.go @@ -6,6 +6,7 @@ package controllers import ( "context" "fmt" + "strings" "time" corev1 "k8s.io/api/core/v1" @@ -32,6 +33,10 @@ const ( // mcpServerEntryCABundleRefField is the field index key for CABundleRef ConfigMap lookups. mcpServerEntryCABundleRefField = "spec.caBundleRef.configMapRef.name" + + // mcpServerEntryHeaderSecretRefField is the field index key for + // spec.headerForward.addHeadersFromSecret[*].valueSecretRef.name lookups. + mcpServerEntryHeaderSecretRefField = "spec.headerForward.addHeadersFromSecret.valueSecretRef.name" ) // MCPServerEntryReconciler reconciles a MCPServerEntry object. @@ -46,6 +51,7 @@ type MCPServerEntryReconciler struct { // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpgroups,verbs=get;list;watch // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpexternalauthconfigs,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch +// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch // Reconcile validates referenced resources and updates status conditions. func (r *MCPServerEntryReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { @@ -85,6 +91,12 @@ func (r *MCPServerEntryReconciler) Reconcile(ctx context.Context, req ctrl.Reque } allValid = valid && allValid + valid, err = r.validateHeaderForwardSecretRefs(ctx, entry) + if err != nil { + return ctrl.Result{}, err + } + allValid = valid && allValid + // Compute overall phase and Valid condition r.updateOverallStatus(entry, allValid) @@ -137,6 +149,38 @@ func (r *MCPServerEntryReconciler) SetupWithManager(mgr ctrl.Manager) error { mcpServerEntryCABundleRefField, err) } + // Set up field index for headerForward Secret refs. Used by the Secret + // watch below so a Secret create/update/delete reconciles every entry that + // references it — without this index we would have to list all entries on + // every Secret event. + if err := mgr.GetFieldIndexer().IndexField( + context.Background(), + &mcpv1beta1.MCPServerEntry{}, + mcpServerEntryHeaderSecretRefField, + func(obj client.Object) []string { + entry := obj.(*mcpv1beta1.MCPServerEntry) + if entry.Spec.HeaderForward == nil { + return nil + } + seen := make(map[string]struct{}, len(entry.Spec.HeaderForward.AddHeadersFromSecret)) + names := make([]string, 0, len(entry.Spec.HeaderForward.AddHeadersFromSecret)) + for _, ref := range entry.Spec.HeaderForward.AddHeadersFromSecret { + if ref.ValueSecretRef == nil { + continue + } + if _, ok := seen[ref.ValueSecretRef.Name]; ok { + continue + } + seen[ref.ValueSecretRef.Name] = struct{}{} + names = append(names, ref.ValueSecretRef.Name) + } + return names + }, + ); err != nil { + return fmt.Errorf("unable to create field index for MCPServerEntry %s: %w", + mcpServerEntryHeaderSecretRefField, err) + } + return ctrl.NewControllerManagedBy(mgr). For(&mcpv1beta1.MCPServerEntry{}). Watches( @@ -151,6 +195,10 @@ func (r *MCPServerEntryReconciler) SetupWithManager(mgr ctrl.Manager) error { &corev1.ConfigMap{}, handler.EnqueueRequestsFromMapFunc(r.findEntriesForConfigMap), ). + Watches( + &corev1.Secret{}, + handler.EnqueueRequestsFromMapFunc(r.findEntriesForHeaderSecret), + ). Complete(r) } @@ -300,6 +348,81 @@ func (r *MCPServerEntryReconciler) validateCABundleRef( return true, nil } +// validateHeaderForwardSecretRefs checks that every Secret referenced by +// spec.headerForward.addHeadersFromSecret exists AND that the named key +// inside it is present in the entry's namespace. Skipped (condition removed) +// when no Secret-backed headers are declared. +// +// All ref-level failures are aggregated into one condition message — a user +// fixing two missing secrets at once should see both surface together rather +// than one-per-reconcile. +// +// Returns (valid, error) where a non-nil error means a transient API +// failure that should be requeued. Key-not-found and Secret-not-found are +// surfaced via the condition (valid=false, err=nil). +func (r *MCPServerEntryReconciler) validateHeaderForwardSecretRefs( + ctx context.Context, + entry *mcpv1beta1.MCPServerEntry, +) (bool, error) { + ctxLogger := log.FromContext(ctx) + + if entry.Spec.HeaderForward == nil || len(entry.Spec.HeaderForward.AddHeadersFromSecret) == 0 { + meta.RemoveStatusCondition(&entry.Status.Conditions, + mcpv1beta1.ConditionTypeMCPServerEntryHeaderSecretRefsValidated) + return true, nil + } + + var failures []string + for _, ref := range entry.Spec.HeaderForward.AddHeadersFromSecret { + if ref.ValueSecretRef == nil { + continue + } + secret := &corev1.Secret{} + secretKey := types.NamespacedName{ + Namespace: entry.Namespace, + Name: ref.ValueSecretRef.Name, + } + if err := r.Get(ctx, secretKey, secret); err != nil { + if errors.IsNotFound(err) { + failures = append(failures, fmt.Sprintf( + "Secret %q referenced for header %q not found", + ref.ValueSecretRef.Name, ref.HeaderName)) + continue + } + // Transient API failure (rate-limit, network, etc.) — requeue + // rather than condition-flip. The next reconcile will re-validate. + ctxLogger.Error(err, "Failed to get referenced header Secret", + "secret", ref.ValueSecretRef.Name, "header", ref.HeaderName) + return false, err + } + if _, ok := secret.Data[ref.ValueSecretRef.Key]; !ok { + failures = append(failures, fmt.Sprintf( + "Secret %q has no key %q referenced for header %q", + ref.ValueSecretRef.Name, ref.ValueSecretRef.Key, ref.HeaderName)) + } + } + + if len(failures) > 0 { + meta.SetStatusCondition(&entry.Status.Conditions, metav1.Condition{ + Type: mcpv1beta1.ConditionTypeMCPServerEntryHeaderSecretRefsValidated, + Status: metav1.ConditionFalse, + Reason: mcpv1beta1.ConditionReasonMCPServerEntryHeaderSecretNotFound, + Message: strings.Join(failures, "; "), + ObservedGeneration: entry.Generation, + }) + return false, nil + } + + meta.SetStatusCondition(&entry.Status.Conditions, metav1.Condition{ + Type: mcpv1beta1.ConditionTypeMCPServerEntryHeaderSecretRefsValidated, + Status: metav1.ConditionTrue, + Reason: mcpv1beta1.ConditionReasonMCPServerEntryHeaderSecretsValid, + Message: "All referenced header Secrets exist with required keys", + ObservedGeneration: entry.Generation, + }) + return true, nil +} + // validateRemoteURL checks that the RemoteURL is well-formed and does not target // a blocked internal or metadata endpoint (SSRF protection). func (*MCPServerEntryReconciler) validateRemoteURL( @@ -421,6 +544,44 @@ func (r *MCPServerEntryReconciler) findEntriesForGroup( return requests } +// findEntriesForHeaderSecret maps Secret changes to MCPServerEntry reconcile +// requests for entries that reference the Secret in +// spec.headerForward.addHeadersFromSecret. This ensures that creating the +// referenced Secret (or rotating its contents) flips the validation condition +// from false → true within one reconcile, instead of waiting for the resync. +func (r *MCPServerEntryReconciler) findEntriesForHeaderSecret( + ctx context.Context, + obj client.Object, +) []reconcile.Request { + ctxLogger := log.FromContext(ctx) + + secret, ok := obj.(*corev1.Secret) + if !ok { + ctxLogger.Error(nil, "Object is not a Secret", "object", obj.GetName()) + return nil + } + + entryList := &mcpv1beta1.MCPServerEntryList{} + if err := r.List(ctx, entryList, + client.InNamespace(secret.Namespace), + client.MatchingFields{mcpServerEntryHeaderSecretRefField: secret.Name}, + ); err != nil { + ctxLogger.Error(err, "Failed to list MCPServerEntries for header Secret change") + return nil + } + + requests := make([]reconcile.Request, len(entryList.Items)) + for i, entry := range entryList.Items { + requests[i] = reconcile.Request{ + NamespacedName: types.NamespacedName{ + Namespace: entry.Namespace, + Name: entry.Name, + }, + } + } + return requests +} + // findEntriesForConfigMap maps ConfigMap changes to MCPServerEntry reconcile requests // for entries that reference the ConfigMap as a CA bundle. func (r *MCPServerEntryReconciler) findEntriesForConfigMap( diff --git a/cmd/thv-operator/controllers/mcpserverentry_controller_test.go b/cmd/thv-operator/controllers/mcpserverentry_controller_test.go index 97b01014fd..42d0e7c48c 100644 --- a/cmd/thv-operator/controllers/mcpserverentry_controller_test.go +++ b/cmd/thv-operator/controllers/mcpserverentry_controller_test.go @@ -137,6 +137,10 @@ func TestMCPServerEntryReconciler_Reconcile(t *testing.T) { status metav1.ConditionStatus reason string } + // extraCheck runs additional assertions on the post-reconcile entry + // state — used when matching condition reasons isn't enough (e.g. + // asserting message content for aggregated failures). + extraCheck func(t *testing.T, entry *mcpv1beta1.MCPServerEntry) }{ { name: "happy path - all refs valid", @@ -330,6 +334,149 @@ func TestMCPServerEntryReconciler_Reconcile(t *testing.T) { {mcpv1beta1.ConditionTypeMCPServerEntryValid, metav1.ConditionTrue, mcpv1beta1.ConditionReasonMCPServerEntryValid}, }, }, + { + name: "headerForward plaintext only - no secret validation needed", + entry: func() *mcpv1beta1.MCPServerEntry { + e := newMCPServerEntry(testEntryGroupRef, nil, nil) + e.Spec.HeaderForward = &mcpv1beta1.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{"X-MCP-Toolsets": "projects,issues"}, + } + return e + }(), + objects: []client.Object{newMCPGroup(mcpv1beta1.MCPGroupPhaseReady)}, + wantPhase: mcpv1beta1.MCPServerEntryPhaseValid, + conditions: []struct { + condType string + status metav1.ConditionStatus + reason string + }{ + {mcpv1beta1.ConditionTypeMCPServerEntryValid, metav1.ConditionTrue, mcpv1beta1.ConditionReasonMCPServerEntryValid}, + }, + }, + { + name: "headerForward secret ref exists", + entry: func() *mcpv1beta1.MCPServerEntry { + e := newMCPServerEntry(testEntryGroupRef, nil, nil) + e.Spec.HeaderForward = &mcpv1beta1.HeaderForwardConfig{ + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + { + HeaderName: "X-API-Key", + ValueSecretRef: &mcpv1beta1.SecretKeyRef{Name: "api-key-secret", Key: "token"}, + }, + }, + } + return e + }(), + objects: []client.Object{ + newMCPGroup(mcpv1beta1.MCPGroupPhaseReady), + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "api-key-secret", Namespace: testEntryNS}, + Data: map[string][]byte{"token": []byte("secret-value")}, + }, + }, + wantPhase: mcpv1beta1.MCPServerEntryPhaseValid, + conditions: []struct { + condType string + status metav1.ConditionStatus + reason string + }{ + {mcpv1beta1.ConditionTypeMCPServerEntryHeaderSecretRefsValidated, metav1.ConditionTrue, mcpv1beta1.ConditionReasonMCPServerEntryHeaderSecretsValid}, + {mcpv1beta1.ConditionTypeMCPServerEntryValid, metav1.ConditionTrue, mcpv1beta1.ConditionReasonMCPServerEntryValid}, + }, + }, + { + name: "headerForward secret ref missing flips entry to Failed", + entry: func() *mcpv1beta1.MCPServerEntry { + e := newMCPServerEntry(testEntryGroupRef, nil, nil) + e.Spec.HeaderForward = &mcpv1beta1.HeaderForwardConfig{ + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + { + HeaderName: "X-API-Key", + ValueSecretRef: &mcpv1beta1.SecretKeyRef{Name: "missing-secret", Key: "token"}, + }, + }, + } + return e + }(), + objects: []client.Object{newMCPGroup(mcpv1beta1.MCPGroupPhaseReady)}, + wantPhase: mcpv1beta1.MCPServerEntryPhaseFailed, + conditions: []struct { + condType string + status metav1.ConditionStatus + reason string + }{ + {mcpv1beta1.ConditionTypeMCPServerEntryHeaderSecretRefsValidated, metav1.ConditionFalse, mcpv1beta1.ConditionReasonMCPServerEntryHeaderSecretNotFound}, + {mcpv1beta1.ConditionTypeMCPServerEntryValid, metav1.ConditionFalse, mcpv1beta1.ConditionReasonMCPServerEntryInvalid}, + }, + }, + { + name: "headerForward secret exists but key is missing flips entry to Failed", + entry: func() *mcpv1beta1.MCPServerEntry { + e := newMCPServerEntry(testEntryGroupRef, nil, nil) + e.Spec.HeaderForward = &mcpv1beta1.HeaderForwardConfig{ + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + { + HeaderName: "X-API-Key", + ValueSecretRef: &mcpv1beta1.SecretKeyRef{Name: "api-key-secret", Key: "absent-key"}, + }, + }, + } + return e + }(), + objects: []client.Object{ + newMCPGroup(mcpv1beta1.MCPGroupPhaseReady), + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "api-key-secret", Namespace: testEntryNS}, + Data: map[string][]byte{"token": []byte("secret-value")}, + }, + }, + wantPhase: mcpv1beta1.MCPServerEntryPhaseFailed, + conditions: []struct { + condType string + status metav1.ConditionStatus + reason string + }{ + {mcpv1beta1.ConditionTypeMCPServerEntryHeaderSecretRefsValidated, metav1.ConditionFalse, mcpv1beta1.ConditionReasonMCPServerEntryHeaderSecretNotFound}, + {mcpv1beta1.ConditionTypeMCPServerEntryValid, metav1.ConditionFalse, mcpv1beta1.ConditionReasonMCPServerEntryInvalid}, + }, + }, + { + name: "headerForward multiple secret refs failures are aggregated", + entry: func() *mcpv1beta1.MCPServerEntry { + e := newMCPServerEntry(testEntryGroupRef, nil, nil) + e.Spec.HeaderForward = &mcpv1beta1.HeaderForwardConfig{ + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + { + HeaderName: "X-API-Key", + ValueSecretRef: &mcpv1beta1.SecretKeyRef{Name: "missing-1", Key: "token"}, + }, + { + HeaderName: "X-Other", + ValueSecretRef: &mcpv1beta1.SecretKeyRef{Name: "missing-2", Key: "value"}, + }, + }, + } + return e + }(), + objects: []client.Object{newMCPGroup(mcpv1beta1.MCPGroupPhaseReady)}, + wantPhase: mcpv1beta1.MCPServerEntryPhaseFailed, + conditions: []struct { + condType string + status metav1.ConditionStatus + reason string + }{ + {mcpv1beta1.ConditionTypeMCPServerEntryHeaderSecretRefsValidated, metav1.ConditionFalse, mcpv1beta1.ConditionReasonMCPServerEntryHeaderSecretNotFound}, + {mcpv1beta1.ConditionTypeMCPServerEntryValid, metav1.ConditionFalse, mcpv1beta1.ConditionReasonMCPServerEntryInvalid}, + }, + extraCheck: func(t *testing.T, entry *mcpv1beta1.MCPServerEntry) { + t.Helper() + cond := meta.FindStatusCondition(entry.Status.Conditions, + mcpv1beta1.ConditionTypeMCPServerEntryHeaderSecretRefsValidated) + require.NotNil(t, cond) + assert.Contains(t, cond.Message, "missing-1") + assert.Contains(t, cond.Message, "missing-2") + }, + }, } for _, tt := range tests { @@ -388,6 +535,9 @@ func TestMCPServerEntryReconciler_Reconcile(t *testing.T) { for _, c := range tt.conditions { assertCondition(t, updatedEntry.Status.Conditions, c.condType, c.status, c.reason) } + if tt.extraCheck != nil { + tt.extraCheck(t, &updatedEntry) + } }) } } diff --git a/cmd/thv-operator/controllers/virtualmcpserver_deployment.go b/cmd/thv-operator/controllers/virtualmcpserver_deployment.go index ea476512b2..6f93594319 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_deployment.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_deployment.go @@ -28,7 +28,9 @@ import ( ctrlutil "github.com/stacklok/toolhive/cmd/thv-operator/pkg/controllerutil" "github.com/stacklok/toolhive/cmd/thv-operator/pkg/runconfig/configmap/checksum" "github.com/stacklok/toolhive/pkg/container/kubernetes" + vmcptypes "github.com/stacklok/toolhive/pkg/vmcp" vmcpconfig "github.com/stacklok/toolhive/pkg/vmcp/config" + "github.com/stacklok/toolhive/pkg/vmcp/headerforward/wirefmt" "github.com/stacklok/toolhive/pkg/vmcp/workloads" ) @@ -358,6 +360,17 @@ func (r *VirtualMCPServerReconciler) buildEnvVarsForVmcp( // Mount outgoing auth secrets env = append(env, r.buildOutgoingAuthEnvVars(ctx, vmcp, typedWorkloads)...) + // Mount per-(entry, header) env vars for MCPServerEntry headerForward. + // Plaintext entries are emitted as literal-value env vars; secret entries + // as valueFrom.secretKeyRef. The vMCP runtime walks the well-known prefixes + // at startup to reconstruct the per-backend HeaderForwardConfig in static + // mode (issue #4996). + headerEnv, err := r.buildHeaderForwardEnvVarsForEntries(ctx, vmcp.Namespace, typedWorkloads) + if err != nil { + return nil, fmt.Errorf("failed to build header forward env vars: %w", err) + } + env = append(env, headerEnv...) + // Always mount HMAC secret for session token binding. env = append(env, r.buildHMACSecretEnvVar(vmcp)) @@ -493,6 +506,152 @@ func (r *VirtualMCPServerReconciler) buildOutgoingAuthEnvVars( return env } +// buildHeaderForwardEnvVarsForEntries iterates entry-type workloads and emits +// header-forward env vars on the vMCP pod for each entry that declares +// spec.headerForward. +// +// One JSON-encoded manifest env var per backend carries the full +// HeaderForwardConfig with original header names preserved +// (TOOLHIVE_HEADER_FORWARD_). Plaintext header values +// appear inline in the JSON; secret-backed entries carry only the secret +// identifier — the Secret value rides a sibling +// TOOLHIVE_SECRET_HEADER_FORWARD_
_ env var emitted via +// valueFrom.secretKeyRef so the value never enters the operator's view of +// the world and resolves at request time inside resolveHeaderForward via +// secrets.EnvironmentProvider. +// +// Scoping by entry name prevents collisions when multiple entries in the +// group declare the same header name. +// +// Returns (nil, nil) when no entries declare any HeaderForward — the common +// case, producing no env-var churn on the Deployment spec. +func (r *VirtualMCPServerReconciler) buildHeaderForwardEnvVarsForEntries( + ctx context.Context, + namespace string, + typedWorkloads []workloads.TypedWorkload, +) ([]corev1.EnvVar, error) { + // Early return if no MCPServerEntry workloads to avoid unnecessary API calls. + hasEntries := false + for _, workload := range typedWorkloads { + if workload.Type == workloads.WorkloadTypeMCPServerEntry { + hasEntries = true + break + } + } + if !hasEntries { + return nil, nil + } + + entryMap, err := r.listMCPServerEntriesAsMap(ctx, namespace) + if err != nil { + return nil, fmt.Errorf("failed to list MCPServerEntries: %w", err) + } + + var envVars []corev1.EnvVar + for _, workload := range typedWorkloads { + if workload.Type != workloads.WorkloadTypeMCPServerEntry { + continue + } + entry, found := entryMap[workload.Name] + if !found || entry.Spec.HeaderForward == nil { + continue + } + + manifest, err := buildHeaderForwardManifestForEntry(entry) + if err != nil { + return nil, fmt.Errorf("failed to build header-forward manifest for entry %q: %w", entry.Name, err) + } + if manifest != "" { + manifestVarName, _ := wirefmt.ManifestEnvVarName(entry.Name) + envVars = append(envVars, corev1.EnvVar{ + Name: manifestVarName, + Value: manifest, + }) + } + + // Secret-backed headers — emit valueFrom.secretKeyRef env vars so + // the resolved Secret value never enters the operator's view of the + // world. AddHeadersFromSecret is a slice in the v1beta1 API so the + // order is already deterministic from the user's manifest. + for _, ref := range entry.Spec.HeaderForward.AddHeadersFromSecret { + if ref.ValueSecretRef == nil { + continue + } + envVarName, _ := wirefmt.SecretEnvVarName(entry.Name, ref.HeaderName) + envVars = append(envVars, corev1.EnvVar{ + Name: envVarName, + ValueFrom: &corev1.EnvVarSource{ + SecretKeyRef: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: ref.ValueSecretRef.Name, + }, + Key: ref.ValueSecretRef.Key, + }, + }, + }) + } + } + + // Sort the resulting env vars by Name. The Kubernetes informer cache + // returns items in non-deterministic order (Go map iteration), so + // without sorting the env vars appear in a different sequence on each + // reconcile. reflect.DeepEqual in containerNeedsUpdate is order- + // sensitive, so non-deterministic ordering causes a continuous + // deployment update loop. Mirrors the pattern in + // discoverExternalAuthConfigSecrets / discoverInlineExternalAuthConfigSecrets. + sort.Slice(envVars, func(i, j int) bool { + return envVars[i].Name < envVars[j].Name + }) + + return envVars, nil +} + +// buildHeaderForwardManifestForEntry builds the JSON-encoded manifest for +// a single MCPServerEntry. AddHeadersFromSecret values carry the secret +// identifier (HEADER_FORWARD__) the runtime hands to +// secrets.EnvironmentProvider at request time; the resolved Secret value +// itself never appears in the JSON. +// +// Returns the empty string when the entry has no plaintext or +// secret-backed entries — the caller skips emitting the env var in that +// case to keep Deployment specs minimal. +func buildHeaderForwardManifestForEntry(entry *mcpv1beta1.MCPServerEntry) (string, error) { + if entry == nil || entry.Spec.HeaderForward == nil { + return "", nil + } + src := entry.Spec.HeaderForward + + // Build vmcp.HeaderForwardConfig directly so the JSON shape is the + // runtime contract — there is no intermediate operator-side type to + // drift away from it. Header keys preserve original casing. + manifest := vmcptypes.HeaderForwardConfig{ + AddPlaintextHeaders: src.AddPlaintextHeaders, + } + for _, ref := range src.AddHeadersFromSecret { + if ref.ValueSecretRef == nil { + continue + } + _, identifier := wirefmt.SecretEnvVarName(entry.Name, ref.HeaderName) + if manifest.AddHeadersFromSecret == nil { + manifest.AddHeadersFromSecret = make(map[string]string) + } + manifest.AddHeadersFromSecret[ref.HeaderName] = identifier + } + + if len(manifest.AddPlaintextHeaders) == 0 && len(manifest.AddHeadersFromSecret) == 0 { + return "", nil + } + + // json.Marshal sorts map keys alphabetically by default, giving + // deterministic Deployment-spec rendering across reconciles regardless + // of Go's randomized map iteration. + out, err := json.Marshal(manifest) + if err != nil { + return "", fmt.Errorf("marshal manifest: %w", err) + } + return string(out), nil +} + // discoverExternalAuthConfigSecrets discovers ExternalAuthConfigs from workloads in the group // and returns environment variables for their client secrets. This is used for discovered mode. func (r *VirtualMCPServerReconciler) discoverExternalAuthConfigSecrets( diff --git a/cmd/thv-operator/controllers/virtualmcpserver_deployment_test.go b/cmd/thv-operator/controllers/virtualmcpserver_deployment_test.go index 6f1a792abc..95047a5ce5 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_deployment_test.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_deployment_test.go @@ -1121,3 +1121,302 @@ func TestImagePullSecretsHash(t *testing.T) { assert.NotEqual(t, a, b) }) } + +// TestBuildHeaderForwardEnvVarsForEntries verifies the operator emits one env +// var per (entry, header) declared on an MCPServerEntry.spec.headerForward, +// using literal values for plaintext and valueFrom.secretKeyRef for +// secret-backed headers. The map iteration is sorted for determinism so that +// two reconciles with the same input produce byte-identical Deployment specs. +func TestBuildHeaderForwardEnvVarsForEntries(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + entries []mcpv1beta1.MCPServerEntry + workloads []workloads.TypedWorkload + validate func(t *testing.T, env []corev1.EnvVar) + }{ + { + name: "no MCPServerEntry workloads yields no env vars", + entries: nil, + workloads: []workloads.TypedWorkload{ + {Name: "server1", Type: workloads.WorkloadTypeMCPServer}, + }, + validate: func(t *testing.T, env []corev1.EnvVar) { + t.Helper() + assert.Empty(t, env) + }, + }, + { + name: "entry without headerForward yields no env vars", + entries: []mcpv1beta1.MCPServerEntry{ + { + ObjectMeta: metav1.ObjectMeta{Name: "entry-noop", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + RemoteURL: "https://mcp.example.com", + Transport: "streamable-http", + GroupRef: &mcpv1beta1.MCPGroupRef{Name: "test-group"}, + }, + }, + }, + workloads: []workloads.TypedWorkload{ + {Name: "entry-noop", Type: workloads.WorkloadTypeMCPServerEntry}, + }, + validate: func(t *testing.T, env []corev1.EnvVar) { + t.Helper() + assert.Empty(t, env) + }, + }, + { + name: "entry with plaintext headers emits one JSON manifest env var preserving original casing", + entries: []mcpv1beta1.MCPServerEntry{ + { + ObjectMeta: metav1.ObjectMeta{Name: "github-copilot", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + RemoteURL: "https://api.githubcopilot.com/mcp/", + Transport: "streamable-http", + GroupRef: &mcpv1beta1.MCPGroupRef{Name: "test-group"}, + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{ + "X-MCP-Toolsets": "projects,issues,pull_requests", + "X-Trace-Id": "abc123", + }, + }, + }, + }, + }, + workloads: []workloads.TypedWorkload{ + {Name: "github-copilot", Type: workloads.WorkloadTypeMCPServerEntry}, + }, + validate: func(t *testing.T, env []corev1.EnvVar) { + t.Helper() + require.Len(t, env, 1) + assert.Equal(t, "TOOLHIVE_HEADER_FORWARD_GITHUB_COPILOT", env[0].Name) + assert.Nil(t, env[0].ValueFrom) + // json.Marshal sorts map keys alphabetically. + assert.JSONEq(t, + `{"addPlaintextHeaders":{"X-MCP-Toolsets":"projects,issues,pull_requests","X-Trace-Id":"abc123"}}`, + env[0].Value, + ) + }, + }, + { + name: "entry with secret-backed headers emits both manifest and valueFrom.secretKeyRef env var", + entries: []mcpv1beta1.MCPServerEntry{ + { + ObjectMeta: metav1.ObjectMeta{Name: "stripe", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + RemoteURL: "https://api.stripe.example/mcp/", + Transport: "streamable-http", + GroupRef: &mcpv1beta1.MCPGroupRef{Name: "test-group"}, + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + { + HeaderName: "X-API-Key", + ValueSecretRef: &mcpv1beta1.SecretKeyRef{ + Name: "stripe-key", + Key: "token", + }, + }, + }, + }, + }, + }, + }, + workloads: []workloads.TypedWorkload{ + {Name: "stripe", Type: workloads.WorkloadTypeMCPServerEntry}, + }, + validate: func(t *testing.T, env []corev1.EnvVar) { + t.Helper() + require.Len(t, env, 2) + + assert.Equal(t, "TOOLHIVE_HEADER_FORWARD_STRIPE", env[0].Name) + assert.JSONEq(t, + `{"addHeadersFromSecret":{"X-API-Key":"HEADER_FORWARD_X_API_KEY_STRIPE"}}`, + env[0].Value, + ) + + assert.Equal(t, "TOOLHIVE_SECRET_HEADER_FORWARD_X_API_KEY_STRIPE", env[1].Name) + assert.Empty(t, env[1].Value) + require.NotNil(t, env[1].ValueFrom) + require.NotNil(t, env[1].ValueFrom.SecretKeyRef) + assert.Equal(t, "stripe-key", env[1].ValueFrom.SecretKeyRef.Name) + assert.Equal(t, "token", env[1].ValueFrom.SecretKeyRef.Key) + }, + }, + { + name: "mixed plaintext + secret across multiple entries are scoped per entry", + entries: []mcpv1beta1.MCPServerEntry{ + { + ObjectMeta: metav1.ObjectMeta{Name: "alpha", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + RemoteURL: "https://alpha.example/mcp/", + Transport: "streamable-http", + GroupRef: &mcpv1beta1.MCPGroupRef{Name: "test-group"}, + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{"X-Trace": "alpha-trace"}, + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + {HeaderName: "X-Token", ValueSecretRef: &mcpv1beta1.SecretKeyRef{Name: "alpha-secret", Key: "tok"}}, + }, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "beta", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + RemoteURL: "https://beta.example/mcp/", + Transport: "streamable-http", + GroupRef: &mcpv1beta1.MCPGroupRef{Name: "test-group"}, + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{"X-Trace": "beta-trace"}, + }, + }, + }, + }, + workloads: []workloads.TypedWorkload{ + {Name: "alpha", Type: workloads.WorkloadTypeMCPServerEntry}, + {Name: "beta", Type: workloads.WorkloadTypeMCPServerEntry}, + }, + validate: func(t *testing.T, env []corev1.EnvVar) { + t.Helper() + require.Len(t, env, 3) + + // After sort.Slice by Name: + // TOOLHIVE_HEADER_FORWARD_ALPHA + // TOOLHIVE_HEADER_FORWARD_BETA + // TOOLHIVE_SECRET_HEADER_FORWARD_X_TOKEN_ALPHA + assert.Equal(t, "TOOLHIVE_HEADER_FORWARD_ALPHA", env[0].Name) + assert.JSONEq(t, + `{"addPlaintextHeaders":{"X-Trace":"alpha-trace"},"addHeadersFromSecret":{"X-Token":"HEADER_FORWARD_X_TOKEN_ALPHA"}}`, + env[0].Value, + ) + + assert.Equal(t, "TOOLHIVE_HEADER_FORWARD_BETA", env[1].Name) + assert.JSONEq(t, + `{"addPlaintextHeaders":{"X-Trace":"beta-trace"}}`, + env[1].Value, + ) + + assert.Equal(t, "TOOLHIVE_SECRET_HEADER_FORWARD_X_TOKEN_ALPHA", env[2].Name) + require.NotNil(t, env[2].ValueFrom) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + require.NoError(t, mcpv1beta1.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + + objs := make([]client.Object, 0, len(tt.entries)) + for i := range tt.entries { + objs = append(objs, &tt.entries[i]) + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(objs...). + Build() + + r := &VirtualMCPServerReconciler{ + Client: fakeClient, + Scheme: scheme, + } + + env, err := r.buildHeaderForwardEnvVarsForEntries(t.Context(), "default", tt.workloads) + require.NoError(t, err) + tt.validate(t, env) + }) + } +} + +// TestBuildHeaderForwardEnvVarsForEntries_ShuffledInputDeterministic verifies +// that the env-var emission is byte-identical regardless of the order +// typedWorkloads arrives in. The function sorts internally; this test pins +// that contract so a future refactor that drops the sort is caught +// immediately. Together with the comment block in the function, this +// closes the deployment-update-loop hazard from informer-cache ordering. +func TestBuildHeaderForwardEnvVarsForEntries_ShuffledInputDeterministic(t *testing.T) { + t.Parallel() + + entries := []mcpv1beta1.MCPServerEntry{ + { + ObjectMeta: metav1.ObjectMeta{Name: "alpha", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + RemoteURL: "https://alpha.example/", + Transport: "streamable-http", + GroupRef: &mcpv1beta1.MCPGroupRef{Name: "g"}, + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{"X-Trace": "alpha-trace"}, + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + {HeaderName: "X-Token", ValueSecretRef: &mcpv1beta1.SecretKeyRef{Name: "alpha-secret", Key: "tok"}}, + }, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "beta", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + RemoteURL: "https://beta.example/", + Transport: "streamable-http", + GroupRef: &mcpv1beta1.MCPGroupRef{Name: "g"}, + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{"X-Trace": "beta-trace"}, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "gamma", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + RemoteURL: "https://gamma.example/", + Transport: "streamable-http", + GroupRef: &mcpv1beta1.MCPGroupRef{Name: "g"}, + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + {HeaderName: "X-Key", ValueSecretRef: &mcpv1beta1.SecretKeyRef{Name: "gamma-secret", Key: "key"}}, + }, + }, + }, + }, + } + + // Two workload orderings — natural and reversed. + natural := []workloads.TypedWorkload{ + {Name: "alpha", Type: workloads.WorkloadTypeMCPServerEntry}, + {Name: "beta", Type: workloads.WorkloadTypeMCPServerEntry}, + {Name: "gamma", Type: workloads.WorkloadTypeMCPServerEntry}, + } + reversed := []workloads.TypedWorkload{ + {Name: "gamma", Type: workloads.WorkloadTypeMCPServerEntry}, + {Name: "beta", Type: workloads.WorkloadTypeMCPServerEntry}, + {Name: "alpha", Type: workloads.WorkloadTypeMCPServerEntry}, + } + + scheme := runtime.NewScheme() + require.NoError(t, mcpv1beta1.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + + objs := make([]client.Object, 0, len(entries)) + for i := range entries { + objs = append(objs, &entries[i]) + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(objs...). + Build() + + r := &VirtualMCPServerReconciler{Client: fakeClient, Scheme: scheme} + + envNatural, err := r.buildHeaderForwardEnvVarsForEntries(t.Context(), "default", natural) + require.NoError(t, err) + + envReversed, err := r.buildHeaderForwardEnvVarsForEntries(t.Context(), "default", reversed) + require.NoError(t, err) + + assert.Equal(t, envNatural, envReversed, + "buildHeaderForwardEnvVarsForEntries must produce byte-identical output regardless of input workload order") +} diff --git a/cmd/thv-operator/pkg/controllerutil/externalauth.go b/cmd/thv-operator/pkg/controllerutil/externalauth.go index 576b38014f..0412f7d3b3 100644 --- a/cmd/thv-operator/pkg/controllerutil/externalauth.go +++ b/cmd/thv-operator/pkg/controllerutil/externalauth.go @@ -44,30 +44,7 @@ func GenerateUniqueHeaderInjectionEnvVarName(configName string) string { return fmt.Sprintf("TOOLHIVE_HEADER_INJECTION_VALUE_%s", sanitized) } -// GenerateHeaderForwardSecretEnvVarName generates the environment variable name for a header forward secret. -// The generated name follows the TOOLHIVE_SECRET_ pattern expected by the EnvironmentProvider. -// -// Parameters: -// - proxyName: The name of the MCPRemoteProxy resource -// - headerName: The HTTP header name (e.g., "X-API-Key") -// -// Returns the full environment variable name (e.g., "TOOLHIVE_SECRET_HEADER_FORWARD_X_API_KEY_MY_PROXY") -// and the secret identifier portion (e.g., "HEADER_FORWARD_X_API_KEY_MY_PROXY") for use in RunConfig. -func GenerateHeaderForwardSecretEnvVarName(proxyName, headerName string) (envVarName, secretIdentifier string) { - // Sanitize header name for use in env var (uppercase, replace hyphens with underscore) - sanitizedHeader := strings.ToUpper(strings.ReplaceAll(headerName, "-", "_")) - sanitizedHeader = envVarSanitizer.ReplaceAllString(sanitizedHeader, "_") - - // Sanitize proxy name for use in env var - sanitizedProxy := strings.ToUpper(strings.ReplaceAll(proxyName, "-", "_")) - sanitizedProxy = envVarSanitizer.ReplaceAllString(sanitizedProxy, "_") - - // Build the secret identifier (what gets stored in RunConfig.AddHeadersFromSecret) - secretIdentifier = fmt.Sprintf("HEADER_FORWARD_%s_%s", sanitizedHeader, sanitizedProxy) - - // Build the full env var name (TOOLHIVE_SECRET_ prefix + identifier) - // This follows the pattern expected by secrets.EnvironmentProvider - envVarName = fmt.Sprintf("TOOLHIVE_SECRET_%s", secretIdentifier) - - return envVarName, secretIdentifier -} +// Header-forward env-var helpers (constants + name generators + the shared +// header-name normalizer) moved to pkg/vmcp/headerforward/wirefmt so the +// runtime can consume them without inverting Go layering. Operator code +// imports them from there. diff --git a/cmd/thv-operator/pkg/controllerutil/externalauth_test.go b/cmd/thv-operator/pkg/controllerutil/externalauth_test.go index 7cffbeabb5..638ba82dce 100644 --- a/cmd/thv-operator/pkg/controllerutil/externalauth_test.go +++ b/cmd/thv-operator/pkg/controllerutil/externalauth_test.go @@ -104,69 +104,5 @@ func TestGenerateUniqueHeaderInjectionEnvVarName(t *testing.T) { } } -// TestGenerateHeaderForwardSecretEnvVarName tests the GenerateHeaderForwardSecretEnvVarName function -func TestGenerateHeaderForwardSecretEnvVarName(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - proxyName string - headerName string - expectedEnvVarName string - expectedSecretIdentifier string - }{ - { - name: "simple names", - proxyName: "my-proxy", - headerName: "X-API-Key", - expectedEnvVarName: "TOOLHIVE_SECRET_HEADER_FORWARD_X_API_KEY_MY_PROXY", - expectedSecretIdentifier: "HEADER_FORWARD_X_API_KEY_MY_PROXY", - }, - { - name: "lowercase header", - proxyName: "test-proxy", - headerName: "authorization", - expectedEnvVarName: "TOOLHIVE_SECRET_HEADER_FORWARD_AUTHORIZATION_TEST_PROXY", - expectedSecretIdentifier: "HEADER_FORWARD_AUTHORIZATION_TEST_PROXY", - }, - { - name: "multiple hyphens", - proxyName: "my-remote-proxy", - headerName: "X-Custom-Header", - expectedEnvVarName: "TOOLHIVE_SECRET_HEADER_FORWARD_X_CUSTOM_HEADER_MY_REMOTE_PROXY", - expectedSecretIdentifier: "HEADER_FORWARD_X_CUSTOM_HEADER_MY_REMOTE_PROXY", - }, - { - name: "special characters in proxy name", - proxyName: "proxy.name@123", - headerName: "X-Token", - expectedEnvVarName: "TOOLHIVE_SECRET_HEADER_FORWARD_X_TOKEN_PROXY_NAME_123", - expectedSecretIdentifier: "HEADER_FORWARD_X_TOKEN_PROXY_NAME_123", - }, - } - - envVarPattern := regexp.MustCompile(`^[A-Z0-9_]+$`) - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - envVarName, secretIdentifier := GenerateHeaderForwardSecretEnvVarName(tt.proxyName, tt.headerName) - - // Verify expected values - assert.Equal(t, tt.expectedEnvVarName, envVarName, "envVarName should match expected") - assert.Equal(t, tt.expectedSecretIdentifier, secretIdentifier, "secretIdentifier should match expected") - - // Verify env var name starts with TOOLHIVE_SECRET_ prefix - assert.True(t, regexp.MustCompile("^TOOLHIVE_SECRET_").MatchString(envVarName), - "envVarName should start with TOOLHIVE_SECRET_ prefix") - - // Verify env var name is valid - assert.Regexp(t, envVarPattern, envVarName, "envVarName should be a valid environment variable name") - assert.Regexp(t, envVarPattern, secretIdentifier, "secretIdentifier should be a valid identifier") - - // Verify relationship: envVarName = "TOOLHIVE_SECRET_" + secretIdentifier - assert.Equal(t, "TOOLHIVE_SECRET_"+secretIdentifier, envVarName, - "envVarName should equal TOOLHIVE_SECRET_ prefix + secretIdentifier") - }) - } -} +// Tests for the header-forward env var generators moved with the +// implementation to pkg/vmcp/headerforward/wirefmt — see wirefmt_test.go. diff --git a/pkg/vmcp/aggregator/discoverer.go b/pkg/vmcp/aggregator/discoverer.go index 4a26b9937a..d50f409ed7 100644 --- a/pkg/vmcp/aggregator/discoverer.go +++ b/pkg/vmcp/aggregator/discoverer.go @@ -21,6 +21,7 @@ import ( "github.com/stacklok/toolhive/pkg/groups" "github.com/stacklok/toolhive/pkg/vmcp" "github.com/stacklok/toolhive/pkg/vmcp/config" + "github.com/stacklok/toolhive/pkg/vmcp/headerforward/wirefmt" "github.com/stacklok/toolhive/pkg/vmcp/workloads" workloadsmgr "github.com/stacklok/toolhive/pkg/workloads" ) @@ -33,6 +34,15 @@ type backendDiscoverer struct { authConfig *config.OutgoingAuthConfig staticBackends []config.StaticBackendConfig // Pre-configured backends for static mode groupRef string // Group reference for static mode metadata + + // headerForwardByBackend is keyed by the NORMALIZED backend name (the + // suffix the operator emits in TOOLHIVE_HEADER_FORWARD_). The + // canonical backend name from the static config is normalized on + // lookup via wirefmt.NormalizeForEnvVar so the keying matches + // the operator-side env-var encoding. Nil/empty when no entry declared + // headerForward. Only populated in static mode — dynamic mode reads + // headerForward directly from the MCPServerEntry CRD. + headerForwardByBackend map[string]*vmcp.HeaderForwardConfig } // NewUnifiedBackendDiscoverer creates a unified backend discoverer that works with both @@ -55,17 +65,24 @@ func NewUnifiedBackendDiscoverer( // NewUnifiedBackendDiscovererWithStaticBackends creates a backend discoverer for static mode // with pre-configured backends, eliminating the need for K8s API access. +// +// headerForwardByBackend carries per-backend HeaderForwardConfig (keyed by +// backend name) constructed at startup from the operator-emitted env vars. +// Pass nil when no entry in the group declares headerForward — the discoverer +// will simply leave Backend.HeaderForward nil for every backend. func NewUnifiedBackendDiscovererWithStaticBackends( staticBackends []config.StaticBackendConfig, authConfig *config.OutgoingAuthConfig, groupRef string, + headerForwardByBackend map[string]*vmcp.HeaderForwardConfig, ) BackendDiscoverer { return &backendDiscoverer{ - workloadsManager: nil, // Not needed in static mode - groupsManager: nil, // Not needed in static mode - authConfig: authConfig, - staticBackends: staticBackends, - groupRef: groupRef, + workloadsManager: nil, // Not needed in static mode + groupsManager: nil, // Not needed in static mode + authConfig: authConfig, + staticBackends: staticBackends, + groupRef: groupRef, + headerForwardByBackend: headerForwardByBackend, } } @@ -272,6 +289,7 @@ func (d *backendDiscoverer) discoverFromStaticConfig() []vmcp.Backend { Type: vmcp.BackendType(staticBackend.Type), CABundlePath: staticBackend.CABundlePath, HealthStatus: vmcp.BackendHealthy, // Assume healthy, actual health check happens later + HeaderForward: d.headerForwardByBackend[wirefmt.NormalizeForEnvVar(staticBackend.Name)], Metadata: staticBackend.Metadata, } diff --git a/pkg/vmcp/aggregator/discoverer_test.go b/pkg/vmcp/aggregator/discoverer_test.go index 6321662fa5..1e3bc50369 100644 --- a/pkg/vmcp/aggregator/discoverer_test.go +++ b/pkg/vmcp/aggregator/discoverer_test.go @@ -1186,6 +1186,7 @@ func TestStaticBackendDiscoverer_EmptyBackendList(t *testing.T) { []config.StaticBackendConfig{}, // Empty slice, not nil nil, // No auth config "test-group", + nil, // No headerForward map ) // This should return empty list without panicking @@ -1281,6 +1282,7 @@ func TestStaticBackendDiscoverer_MetadataGroupOverride(t *testing.T) { tt.staticBackends, nil, // No auth config needed for this test tt.groupRef, + nil, // No headerForward map for this test ) backends, err := discoverer.Discover(ctx, tt.groupRef) @@ -1400,6 +1402,7 @@ func TestStaticBackendDiscoverer_EntryBackendFields(t *testing.T) { tt.staticBackends, nil, // No auth config needed for this test tt.groupRef, + nil, // No headerForward map for this test ) backends, err := discoverer.Discover(ctx, tt.groupRef) @@ -1466,6 +1469,7 @@ func TestBackendDiscoverer_Discover_DeterministicOrdering(t *testing.T) { tc.staticBackends, nil, // No auth config needed for this test "test-group", + nil, // No headerForward map for this test ) backends, err := discoverer.Discover(ctx, "test-group") diff --git a/pkg/vmcp/cli/header_forward_env.go b/pkg/vmcp/cli/header_forward_env.go new file mode 100644 index 0000000000..1e5a84a285 --- /dev/null +++ b/pkg/vmcp/cli/header_forward_env.go @@ -0,0 +1,74 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package cli + +import ( + "encoding/json" + "log/slog" + "strings" + + "github.com/stacklok/toolhive/pkg/vmcp" + "github.com/stacklok/toolhive/pkg/vmcp/headerforward/wirefmt" +) + +// readHeaderForwardFromEnv reconstructs the per-backend +// vmcp.HeaderForwardConfig map by walking environment variables emitted by +// the operator on the vMCP pod. +// +// The operator emits one JSON-encoded manifest env var per backend named +// "TOOLHIVE_HEADER_FORWARD_". The JSON value carries +// every configured header for that backend with original (un-normalized) +// names preserved: +// +// { +// "addPlaintextHeaders": {"X-MCP-Toolsets":"projects,issues"}, +// "addHeadersFromSecret": {"X-Api-Key":"HEADER_FORWARD_X_API_KEY_"} +// } +// +// AddHeadersFromSecret values are secret IDENTIFIERS, not values. Secret +// values resolve later inside resolveHeaderForward via +// secrets.EnvironmentProvider, which reads TOOLHIVE_SECRET_ +// env vars (delivered separately via valueFrom.secretKeyRef so the value +// never enters the operator's view of the world). +// +// The map key is the normalized entry segment from the env-var suffix — +// the SAME value the operator's GenerateHeaderForwardManifestEnvVarName +// produces for that backend's name. Callers look up by passing the +// original backend name through ctrlutil.NormalizeHeaderForEnvVar before +// indexing. +func readHeaderForwardFromEnv(envEntries []string) map[string]*vmcp.HeaderForwardConfig { + out := make(map[string]*vmcp.HeaderForwardConfig) + for _, entry := range envEntries { + name, value, ok := strings.Cut(entry, "=") + if !ok || !strings.HasPrefix(name, wirefmt.ManifestEnvVarPrefix) { + continue + } + ownerSegment := strings.TrimPrefix(name, wirefmt.ManifestEnvVarPrefix) + + var cfg vmcp.HeaderForwardConfig + if err := json.Unmarshal([]byte(value), &cfg); err != nil { + // A malformed manifest is a programmer error in the operator; + // log and skip rather than fail the whole startup. The backend + // will simply have no headerForward attached. + slog.Warn("invalid headerForward manifest from env, skipping", + "envvar", name, "error", err) + continue + } + // wirefmt.NormalizeForEnvVar maps two distinct entry names with the + // same uppercased+sanitized form to the same env-var suffix (e.g. + // "github-copilot" and "github_copilot"). DNS-1123 forbids + // underscores in entry names, so this collision is unreachable in + // production today — but a future relaxation, a migration that + // allows mixed casing, or a third-party producing manifests can all + // land us here. Surface the collision loudly: the second config + // silently overwriting the first would mask a misconfiguration that + // is otherwise extremely hard to debug. + if _, dup := out[ownerSegment]; dup { + slog.Warn("duplicate headerForward manifest env var; later value overrides earlier", + "envvar", name, "ownerSegment", ownerSegment) + } + out[ownerSegment] = &cfg + } + return out +} diff --git a/pkg/vmcp/cli/header_forward_env_test.go b/pkg/vmcp/cli/header_forward_env_test.go new file mode 100644 index 0000000000..4683c08335 --- /dev/null +++ b/pkg/vmcp/cli/header_forward_env_test.go @@ -0,0 +1,151 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package cli + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/vmcp" +) + +// TestReadHeaderForwardFromEnv covers reconstructing the per-backend +// HeaderForwardConfig from the JSON-encoded TOOLHIVE_HEADER_FORWARD_ +// env vars the operator emits on the vMCP pod. Original header casing / +// punctuation is preserved by the JSON value, so the runtime reconstructs +// "X-MCP-Toolsets" rather than "X_MCP_TOOLSETS". +// +// The map is keyed by the normalized entry segment from the env-var +// suffix; the discoverer normalizes Backend.Name through +// ctrlutil.NormalizeHeaderForEnvVar before indexing. +func TestReadHeaderForwardFromEnv(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + env []string + validate func(t *testing.T, m map[string]*vmcp.HeaderForwardConfig) + }{ + { + name: "no env vars yields empty map", + env: []string{"HOME=/root", "PATH=/bin"}, + validate: func(t *testing.T, m map[string]*vmcp.HeaderForwardConfig) { + t.Helper() + assert.Empty(t, m) + }, + }, + { + name: "plaintext header decodes preserving original casing and hyphens", + env: []string{ + `TOOLHIVE_HEADER_FORWARD_GITHUB_COPILOT={"addPlaintextHeaders":{"X-MCP-Toolsets":"projects,issues"}}`, + "PATH=/bin", + }, + validate: func(t *testing.T, m map[string]*vmcp.HeaderForwardConfig) { + t.Helper() + cfg := m["GITHUB_COPILOT"] + require.NotNil(t, cfg) + assert.Equal(t, map[string]string{"X-MCP-Toolsets": "projects,issues"}, cfg.AddPlaintextHeaders) + assert.Empty(t, cfg.AddHeadersFromSecret) + }, + }, + { + name: "secret header decodes to identifier (value not in manifest)", + env: []string{ + `TOOLHIVE_HEADER_FORWARD_STRIPE={"addHeadersFromSecret":{"X-Api-Key":"HEADER_FORWARD_X_API_KEY_STRIPE"}}`, + // The secret-value env var carries the resolved value via + // secretKeyRef; the walker must NOT treat it as a manifest. + "TOOLHIVE_SECRET_HEADER_FORWARD_X_API_KEY_STRIPE=resolved-secret-value", + }, + validate: func(t *testing.T, m map[string]*vmcp.HeaderForwardConfig) { + t.Helper() + assert.NotContains(t, m, "X_API_KEY_STRIPE", "secret env var must not be parsed as a manifest") + cfg := m["STRIPE"] + require.NotNil(t, cfg) + assert.Empty(t, cfg.AddPlaintextHeaders) + assert.Equal(t, + map[string]string{"X-Api-Key": "HEADER_FORWARD_X_API_KEY_STRIPE"}, + cfg.AddHeadersFromSecret, + ) + }, + }, + { + name: "malformed manifest is skipped without failing other backends", + env: []string{ + `TOOLHIVE_HEADER_FORWARD_BROKEN=not-json`, + `TOOLHIVE_HEADER_FORWARD_DEMO={"addPlaintextHeaders":{"X-Trace":"ok"}}`, + }, + validate: func(t *testing.T, m map[string]*vmcp.HeaderForwardConfig) { + t.Helper() + assert.NotContains(t, m, "BROKEN") + cfg := m["DEMO"] + require.NotNil(t, cfg) + assert.Equal(t, map[string]string{"X-Trace": "ok"}, cfg.AddPlaintextHeaders) + }, + }, + { + name: "mixed plaintext + secret in one manifest scoped per backend", + env: []string{ + `TOOLHIVE_HEADER_FORWARD_ALPHA={"addPlaintextHeaders":{"X-Trace":"alpha-trace"},"addHeadersFromSecret":{"X-Token":"HEADER_FORWARD_X_TOKEN_ALPHA"}}`, + `TOOLHIVE_HEADER_FORWARD_BETA={"addPlaintextHeaders":{"X-Trace":"beta-trace"}}`, + }, + validate: func(t *testing.T, m map[string]*vmcp.HeaderForwardConfig) { + t.Helper() + + alpha := m["ALPHA"] + require.NotNil(t, alpha) + assert.Equal(t, map[string]string{"X-Trace": "alpha-trace"}, alpha.AddPlaintextHeaders) + assert.Equal(t, + map[string]string{"X-Token": "HEADER_FORWARD_X_TOKEN_ALPHA"}, + alpha.AddHeadersFromSecret, + ) + + beta := m["BETA"] + require.NotNil(t, beta) + assert.Equal(t, map[string]string{"X-Trace": "beta-trace"}, beta.AddPlaintextHeaders) + assert.Empty(t, beta.AddHeadersFromSecret) + }, + }, + { + name: "malformed env entry without '=' is skipped", + env: []string{ + "NO_EQUALS_HERE", + `TOOLHIVE_HEADER_FORWARD_DEMO={"addPlaintextHeaders":{"X-Trace":"ok"}}`, + }, + validate: func(t *testing.T, m map[string]*vmcp.HeaderForwardConfig) { + t.Helper() + cfg := m["DEMO"] + require.NotNil(t, cfg) + assert.Equal(t, map[string]string{"X-Trace": "ok"}, cfg.AddPlaintextHeaders) + }, + }, + { + // DNS-1123 forbids underscores in MCPServerEntry names today, so + // real entries with the same NormalizeForEnvVar output cannot + // coexist. We pass two manifests that share the same suffix + // directly to verify the runtime keeps "last wins" behavior and + // surfaces a warning rather than silently dropping the first. + name: "duplicate manifest suffix retains last value", + env: []string{ + `TOOLHIVE_HEADER_FORWARD_DUP={"addPlaintextHeaders":{"X-Trace":"first"}}`, + `TOOLHIVE_HEADER_FORWARD_DUP={"addPlaintextHeaders":{"X-Trace":"second"}}`, + }, + validate: func(t *testing.T, m map[string]*vmcp.HeaderForwardConfig) { + t.Helper() + cfg := m["DUP"] + require.NotNil(t, cfg) + assert.Equal(t, map[string]string{"X-Trace": "second"}, cfg.AddPlaintextHeaders, + "duplicate manifest must keep the later value (last-write-wins)") + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + tt.validate(t, readHeaderForwardFromEnv(tt.env)) + }) + } +} diff --git a/pkg/vmcp/cli/serve.go b/pkg/vmcp/cli/serve.go index ffb501115c..727146f399 100644 --- a/pkg/vmcp/cli/serve.go +++ b/pkg/vmcp/cli/serve.go @@ -566,10 +566,21 @@ func discoverBackends( if len(cfg.Backends) > 0 { // Static mode: use pre-configured backends from config. slog.Info(fmt.Sprintf("Static mode: using %d pre-configured backends", len(cfg.Backends))) + + // Reconstruct per-backend HeaderForwardConfig from env vars the + // operator emitted on this pod. Plaintext header values are inline + // in the JSON manifest; secret-backed headers carry only identifiers + // here and resolve later via secrets.EnvironmentProvider at request + // time. Map keys are the normalized entry segment from the env-var + // suffix; the discoverer normalizes Backend.Name through + // ctrlutil.NormalizeHeaderForEnvVar to look up the matching entry. + // Returns an empty map when no entry in the group declared + // headerForward — the common case. discoverer = aggregator.NewUnifiedBackendDiscovererWithStaticBackends( cfg.Backends, cfg.OutgoingAuth, cfg.Group, + readHeaderForwardFromEnv(os.Environ()), ) } else { // Dynamic mode: discover backends at runtime from the active workload manager (K8s or local). diff --git a/pkg/vmcp/client/client.go b/pkg/vmcp/client/client.go index d84018856a..c72cf11cb9 100644 --- a/pkg/vmcp/client/client.go +++ b/pkg/vmcp/client/client.go @@ -27,6 +27,7 @@ import ( "go.opentelemetry.io/otel/propagation" "github.com/stacklok/toolhive/pkg/auth" + "github.com/stacklok/toolhive/pkg/secrets" "github.com/stacklok/toolhive/pkg/versions" "github.com/stacklok/toolhive/pkg/vmcp" vmcpauth "github.com/stacklok/toolhive/pkg/vmcp/auth" @@ -64,6 +65,11 @@ type httpBackendClient struct { // registry manages authentication strategies for outgoing requests to backend MCP servers. // Must not be nil - use UnauthenticatedStrategy for no authentication. registry vmcpauth.OutgoingAuthRegistry + + // secretsProvider resolves TOOLHIVE_SECRET_ env vars at client-creation + // time for per-backend header-forward injection. Nil when no backends declare + // headerForward.AddHeadersFromSecret — plaintext-only backends do not require it. + secretsProvider secrets.Provider } // NewHTTPBackendClient creates a new HTTP-based backend client. @@ -80,7 +86,8 @@ func NewHTTPBackendClient(registry vmcpauth.OutgoingAuthRegistry) (vmcp.BackendC } c := &httpBackendClient{ - registry: registry, + registry: registry, + secretsProvider: secrets.NewEnvironmentProvider(), } c.clientFactory = c.defaultClientFactory return c, nil @@ -296,6 +303,14 @@ func (h *httpBackendClient) defaultClientFactory(ctx context.Context, target *vm target: target, } + // Inject per-backend HTTP headers from MCPServerEntry.spec.headerForward. + // Resolves plaintext + secret-backed headers once here; auth (inner) always + // wins over user-supplied headers because it runs after this tripper. + baseTransport, err = buildHeaderForwardTripper(ctx, baseTransport, target.HeaderForward, h.secretsProvider, target.WorkloadID) + if err != nil { + return nil, fmt.Errorf("failed to build header-forward transport: %w", err) + } + // Extract identity and health-check marker from context and propagate them to backend // requests. The health-check marker must be carried through to the DELETE request that // mcp-go emits when closing a streamable-HTTP session: mcp-go creates that request with diff --git a/pkg/vmcp/client/header_forward.go b/pkg/vmcp/client/header_forward.go new file mode 100644 index 0000000000..1708742dd8 --- /dev/null +++ b/pkg/vmcp/client/header_forward.go @@ -0,0 +1,160 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package client + +import ( + "context" + "errors" + "fmt" + "log/slog" + "net/http" + + "github.com/stacklok/toolhive/pkg/secrets" + "github.com/stacklok/toolhive/pkg/transport/middleware" + "github.com/stacklok/toolhive/pkg/vmcp" +) + +// headerForwardRoundTripper injects a pre-resolved set of HTTP headers onto every +// outbound request to a specific backend. Headers are resolved once at client +// creation time (plaintext verbatim + secret identifiers looked up via +// secrets.EnvironmentProvider) and stored in an immutable map. The request is +// cloned before mutation so callers retain an untouched request. +// +// This tripper applies to health checks, listTools/listResources/listPrompts, and +// tool/resource/prompt invocations — every HTTP call made by the vMCP backend +// client passes through the same transport chain. +type headerForwardRoundTripper struct { + base http.RoundTripper + headers http.Header // canonicalized header name → single value +} + +// RoundTrip injects the pre-resolved headers onto a clone of the request and +// delegates to the wrapped transport. Headers already present on the request +// are left untouched so inner transports (auth, identity, trace) can always +// override user-supplied values for the same name. +func (h *headerForwardRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + if len(h.headers) == 0 { + return h.base.RoundTrip(req) + } + reqCopy := req.Clone(req.Context()) + if reqCopy.Header == nil { + reqCopy.Header = make(http.Header) + } + for name, values := range h.headers { + if len(values) == 0 { + continue + } + // Do not clobber headers already set by earlier (outer) tripper stages. + // The vMCP runtime is authoritative for identity/trace/auth headers. + if reqCopy.Header.Get(name) != "" { + continue + } + reqCopy.Header.Set(name, values[0]) + } + return h.base.RoundTrip(reqCopy) +} + +// buildHeaderForwardTripper constructs a headerForwardRoundTripper for the +// backend's pre-resolved HeaderForwardConfig. Returns base unchanged when no +// header injection is configured or the effective header set is empty. +// +// Fails loudly (constructor validation, per go-style.md) when a secret identifier +// cannot be resolved through the provider, so a misconfigured backend surfaces +// at pod startup — not as a silent missing-header on every request. +// +// Restricted header names (matching pkg/transport/middleware.RestrictedHeaders) +// are rejected to prevent Host, Content-Length, Authorization, hop-by-hop, and +// X-Forwarded-* spoofing via user-supplied config. +func buildHeaderForwardTripper( + ctx context.Context, + base http.RoundTripper, + cfg *vmcp.HeaderForwardConfig, + provider secrets.Provider, + backendName string, +) (http.RoundTripper, error) { + if cfg == nil { + return base, nil + } + + headers, err := resolveHeaderForward(ctx, cfg, provider, backendName) + if err != nil { + return nil, err + } + if len(headers) == 0 { + return base, nil + } + return &headerForwardRoundTripper{base: base, headers: headers}, nil +} + +// resolveHeaderForward merges plaintext headers with secret-resolved headers into +// a single canonicalized http.Header. Plaintext entries are copied verbatim; +// secret identifiers are looked up via the provider (TOOLHIVE_SECRET_). +// +// Returns an error if any header name is in middleware.RestrictedHeaders or if +// any referenced secret identifier is not present in the environment. +func resolveHeaderForward( + ctx context.Context, + cfg *vmcp.HeaderForwardConfig, + provider secrets.Provider, + backendName string, +) (http.Header, error) { + if cfg == nil { + return nil, nil + } + + size := len(cfg.AddPlaintextHeaders) + len(cfg.AddHeadersFromSecret) + if size == 0 { + return nil, nil + } + out := make(http.Header, size) + + for name, value := range cfg.AddPlaintextHeaders { + canonical := http.CanonicalHeaderKey(name) + if middleware.RestrictedHeaders[canonical] { + return nil, fmt.Errorf( + "backend %q: header %q is restricted and cannot be forwarded", + backendName, canonical, + ) + } + out.Set(canonical, value) + } + + if provider == nil && len(cfg.AddHeadersFromSecret) > 0 { + return nil, fmt.Errorf( + "backend %q: secret provider required to resolve %d header secret(s)", + backendName, len(cfg.AddHeadersFromSecret), + ) + } + + for name, identifier := range cfg.AddHeadersFromSecret { + canonical := http.CanonicalHeaderKey(name) + if middleware.RestrictedHeaders[canonical] { + return nil, fmt.Errorf( + "backend %q: header %q is restricted and cannot be forwarded", + backendName, canonical, + ) + } + value, err := provider.GetSecret(ctx, identifier) + if err != nil { + // Do not include the identifier or value in any user-facing error; + // the log is the only place we record the identifier, not the value. + slog.Error("failed to resolve header-forward secret", + "backend", backendName, "header", canonical, "identifier", identifier, + "error", err) + if errors.Is(err, secrets.ErrSecretNotFound) { + return nil, fmt.Errorf( + "backend %q: header %q secret not found in pod environment", + backendName, canonical, + ) + } + return nil, fmt.Errorf( + "backend %q: failed to resolve header %q from secret provider: %w", + backendName, canonical, err, + ) + } + out.Set(canonical, value) + } + + return out, nil +} diff --git a/pkg/vmcp/client/header_forward_integration_test.go b/pkg/vmcp/client/header_forward_integration_test.go new file mode 100644 index 0000000000..45dacc88ba --- /dev/null +++ b/pkg/vmcp/client/header_forward_integration_test.go @@ -0,0 +1,134 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package client_test + +import ( + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/secrets" + "github.com/stacklok/toolhive/pkg/vmcp" + "github.com/stacklok/toolhive/pkg/vmcp/auth" + "github.com/stacklok/toolhive/pkg/vmcp/auth/strategies" + vmcpclient "github.com/stacklok/toolhive/pkg/vmcp/client" +) + +// TestHeaderForward_EndToEnd_ThroughHTTPBackendClient drives the full +// NewHTTPBackendClient → defaultClientFactory wiring. It verifies that: +// +// 1. The plaintext header reaches the backend +// 2. A secret-backed header is resolved through the EnvironmentProvider +// using the TOOLHIVE_SECRET_ convention and reaches the backend +// +// Anything below this is exercised by the round-tripper unit tests, but only +// an end-to-end test confirms the *factory wiring itself* (provider lookup, +// transport stacking, ctx threading) is correct in production code paths. +func TestHeaderForward_EndToEnd_ThroughHTTPBackendClient(t *testing.T) { + // t.Setenv forbids t.Parallel — fine here, this test is fast. + const ( + identifier = "HEADER_FORWARD_X_API_KEY_BACKEND_E2E" + envVarName = secrets.EnvVarPrefix + identifier + secretVal = "super-secret-resolved-from-env" + ) + t.Setenv(envVarName, secretVal) + + captured := newCapturingMCPServer(t) + t.Cleanup(captured.server.Close) + + registry := auth.NewDefaultOutgoingAuthRegistry() + require.NoError(t, registry.RegisterStrategy("unauthenticated", &strategies.UnauthenticatedStrategy{})) + + backendClient, err := vmcpclient.NewHTTPBackendClient(registry) + require.NoError(t, err) + + target := &vmcp.BackendTarget{ + WorkloadID: "backend-e2e", + WorkloadName: "Backend E2E", + BaseURL: captured.server.URL, + TransportType: "streamable-http", + HeaderForward: &vmcp.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{ + "X-Tenant": "acme", + }, + AddHeadersFromSecret: map[string]string{ + "X-API-Key": identifier, + }, + }, + } + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + // CallTool drives the full Initialize → tools/list flow through the + // streamable-HTTP transport. We don't care about the call result — only + // that the request reached the test server with the configured headers. + _, _ = backendClient.CallTool(ctx, target, "anything", map[string]any{}, nil) + + captured.mu.Lock() + defer captured.mu.Unlock() + require.NotEmpty(t, captured.headers, "test server received no requests") + assert.Equal(t, "acme", captured.headers.Get("X-Tenant"), + "plaintext header must be forwarded by the e2e factory wiring") + assert.Equal(t, secretVal, captured.headers.Get("X-Api-Key"), + "secret-backed header must be resolved via EnvironmentProvider and forwarded") +} + +// capturingMCPServer is a minimal HTTP server that records the headers of +// every inbound request and returns a stub MCP initialize response. It does +// not implement the full MCP protocol — just enough for the streamable-HTTP +// client to send at least one request whose headers we can inspect. +type capturingMCPServer struct { + server *httptest.Server + mu sync.Mutex + headers http.Header +} + +func newCapturingMCPServer(t *testing.T) *capturingMCPServer { + t.Helper() + c := &capturingMCPServer{} + c.server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + c.mu.Lock() + // Only capture the FIRST request — that's the one carrying the headers + // resolved at client-construction time. Subsequent requests on the same + // connection would see the same headers but recording every one makes + // the test read confusing. + if c.headers == nil { + c.headers = r.Header.Clone() + } + c.mu.Unlock() + + // Drain the body so the underlying transport can reuse the connection. + _, _ = io.Copy(io.Discard, r.Body) + _ = r.Body.Close() + + // Return a minimal JSON-RPC error response so the client unwinds + // cleanly. We don't need a full Initialize because we already have + // what we need (the headers) before the client gives up. + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(map[string]any{ + "jsonrpc": "2.0", + "id": 1, + "error": map[string]any{ + "code": -32601, + "message": "method not implemented in test server", + }, + }) + })) + // Sanity check — the test server URL must be plain HTTP (no TLS) so the + // transport doesn't try to verify a self-signed cert against system roots. + require.True(t, strings.HasPrefix(c.server.URL, "http://"), + "capturingMCPServer must serve plain HTTP") + return c +} diff --git a/pkg/vmcp/client/header_forward_test.go b/pkg/vmcp/client/header_forward_test.go new file mode 100644 index 0000000000..eddbfd21c9 --- /dev/null +++ b/pkg/vmcp/client/header_forward_test.go @@ -0,0 +1,243 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package client + +import ( + "context" + "errors" + "io" + "net/http" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/secrets" + "github.com/stacklok/toolhive/pkg/transport/middleware" + "github.com/stacklok/toolhive/pkg/vmcp" +) + +// mockProvider is an in-memory stand-in for secrets.Provider so tests don't rely +// on process env. It only implements GetSecret; other methods return errors. +// +// errors maps an identifier to a per-key error to inject — used to verify that +// resolveHeaderForward propagates non-NotFound errors (transient backend +// failures, permission errors, etc.) without translating them to NotFound. +type mockProvider struct { + values map[string]string + errors map[string]error +} + +func (m *mockProvider) GetSecret(_ context.Context, name string) (string, error) { + if m.errors != nil { + if err, ok := m.errors[name]; ok { + return "", err + } + } + if v, ok := m.values[name]; ok { + return v, nil + } + return "", secrets.ErrSecretNotFound +} + +// mockProvider needs to satisfy secrets.Provider; the other methods are unused. +func (*mockProvider) SetSecret(_ context.Context, _, _ string) error { return nil } +func (*mockProvider) DeleteSecret(_ context.Context, _ string) error { return nil } +func (*mockProvider) ListSecrets(_ context.Context) ([]secrets.SecretDescription, error) { + return nil, nil +} +func (*mockProvider) DeleteSecrets(_ context.Context, _ []string) error { return nil } +func (*mockProvider) Cleanup() error { return nil } +func (*mockProvider) Capabilities() secrets.ProviderCapabilities { + return secrets.ProviderCapabilities{CanRead: true} +} + +// captureTripper records the headers seen on the last request that reached it. +type captureTripper struct { + lastHeaders http.Header +} + +func (c *captureTripper) RoundTrip(req *http.Request) (*http.Response, error) { + c.lastHeaders = req.Header.Clone() + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader("")), + Header: make(http.Header), + }, nil +} + +func TestHeaderForwardRoundTripper_InjectsPlaintext(t *testing.T) { + t.Parallel() + + base := &captureTripper{} + rt := &headerForwardRoundTripper{ + base: base, + headers: http.Header{ + "X-Mcp-Toolsets": []string{"projects,issues"}, + "X-Tenant": []string{"acme"}, + }, + } + + req, err := http.NewRequest(http.MethodGet, "https://example.com", http.NoBody) + require.NoError(t, err) + + _, err = rt.RoundTrip(req) + require.NoError(t, err) + + assert.Equal(t, "projects,issues", base.lastHeaders.Get("X-Mcp-Toolsets")) + assert.Equal(t, "acme", base.lastHeaders.Get("X-Tenant")) + // Original request must remain unmutated. + assert.Empty(t, req.Header.Get("X-Mcp-Toolsets")) +} + +func TestHeaderForwardRoundTripper_NoHeadersIsNoOp(t *testing.T) { + t.Parallel() + + base := &captureTripper{} + rt := &headerForwardRoundTripper{base: base, headers: nil} + + req, err := http.NewRequest(http.MethodGet, "https://example.com", http.NoBody) + require.NoError(t, err) + + _, err = rt.RoundTrip(req) + require.NoError(t, err) + assert.Empty(t, base.lastHeaders.Get("X-Mcp-Toolsets")) +} + +func TestHeaderForwardRoundTripper_DoesNotClobberExistingHeader(t *testing.T) { + t.Parallel() + + base := &captureTripper{} + rt := &headerForwardRoundTripper{ + base: base, + headers: http.Header{ + "Authorization": []string{"Bearer user-provided"}, + }, + } + + req, err := http.NewRequest(http.MethodGet, "https://example.com", http.NoBody) + require.NoError(t, err) + // Pre-set the header as an outer (identity/trace/auth) tripper would. + req.Header.Set("Authorization", "Bearer real-auth") + + _, err = rt.RoundTrip(req) + require.NoError(t, err) + + assert.Equal(t, "Bearer real-auth", base.lastHeaders.Get("Authorization"), + "user-supplied header-forward value must not clobber an already-set header") +} + +func TestResolveHeaderForward_PlaintextAndSecretMerged(t *testing.T) { + t.Parallel() + + cfg := &vmcp.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{ + "X-Tenant": "acme", + }, + AddHeadersFromSecret: map[string]string{ + "X-API-Key": "HEADER_FORWARD_X_API_KEY_BACKEND_A", + }, + } + provider := &mockProvider{values: map[string]string{ + "HEADER_FORWARD_X_API_KEY_BACKEND_A": "resolved-from-env", + }} + + hdr, err := resolveHeaderForward(t.Context(), cfg, provider, "backend-a") + require.NoError(t, err) + assert.Equal(t, "acme", hdr.Get("X-Tenant")) + assert.Equal(t, "resolved-from-env", hdr.Get("X-Api-Key")) +} + +func TestResolveHeaderForward_SecretNotFoundReturnsError(t *testing.T) { + t.Parallel() + + cfg := &vmcp.HeaderForwardConfig{ + AddHeadersFromSecret: map[string]string{ + "X-API-Key": "HEADER_FORWARD_X_API_KEY_BACKEND_A", + }, + } + provider := &mockProvider{values: map[string]string{}} + + hdr, err := resolveHeaderForward(t.Context(), cfg, provider, "backend-a") + require.Error(t, err) + assert.Nil(t, hdr) + // Error must not leak the identifier or the backend's private values. + assert.NotContains(t, err.Error(), "HEADER_FORWARD_X_API_KEY_BACKEND_A") +} + +// TestResolveHeaderForward_RestrictedHeaderRejected iterates the full +// middleware.RestrictedHeaders set and verifies every entry is rejected via +// both the plaintext and secret-backed paths. Hardcoding a subset would let a +// future addition to RestrictedHeaders silently fall out of coverage. +func TestResolveHeaderForward_RestrictedHeaderRejected(t *testing.T) { + t.Parallel() + + require.NotEmpty(t, middleware.RestrictedHeaders, "guard against empty map") + + for header := range middleware.RestrictedHeaders { + t.Run("plaintext_"+header, func(t *testing.T) { + t.Parallel() + cfg := &vmcp.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{header: "x"}, + } + _, err := resolveHeaderForward(t.Context(), cfg, &mockProvider{}, "backend-x") + require.Error(t, err) + assert.Contains(t, err.Error(), "restricted") + }) + + t.Run("secret_"+header, func(t *testing.T) { + t.Parallel() + cfg := &vmcp.HeaderForwardConfig{ + AddHeadersFromSecret: map[string]string{header: "HEADER_FORWARD_RESTRICTED"}, + } + _, err := resolveHeaderForward(t.Context(), cfg, &mockProvider{}, "backend-x") + require.Error(t, err) + assert.Contains(t, err.Error(), "restricted") + }) + } +} + +// TestResolveHeaderForward_NonNotFoundProviderError verifies that a transient +// secret-provider failure (e.g. permission denied, timeout) is propagated with +// its original chain intact — distinct from the user-facing "not found" +// message produced for ErrSecretNotFound. +func TestResolveHeaderForward_NonNotFoundProviderError(t *testing.T) { + t.Parallel() + + wantErr := errors.New("permission denied calling secrets backend") + cfg := &vmcp.HeaderForwardConfig{ + AddHeadersFromSecret: map[string]string{ + "X-API-Key": "HEADER_FORWARD_X_API_KEY_BACKEND_A", + }, + } + provider := &mockProvider{ + errors: map[string]error{ + "HEADER_FORWARD_X_API_KEY_BACKEND_A": wantErr, + }, + } + + hdr, err := resolveHeaderForward(t.Context(), cfg, provider, "backend-a") + require.Error(t, err) + assert.Nil(t, hdr) + require.ErrorIs(t, err, wantErr, + "non-NotFound provider errors must be wrapped with %%w so callers can errors.Is them") + assert.NotContains(t, err.Error(), "not found", + "only ErrSecretNotFound should produce the user-facing 'not found' message") +} + +func TestResolveHeaderForward_NilCfgReturnsNil(t *testing.T) { + t.Parallel() + hdr, err := resolveHeaderForward(t.Context(), nil, nil, "x") + require.NoError(t, err) + assert.Nil(t, hdr) +} + +func TestBuildHeaderForwardTripper_NilCfgReturnsBase(t *testing.T) { + t.Parallel() + base := &captureTripper{} + got, err := buildHeaderForwardTripper(t.Context(), base, nil, nil, "x") + require.NoError(t, err) + assert.Same(t, base, got, "nil cfg must pass base through untouched") +} diff --git a/pkg/vmcp/headerforward/wirefmt/wirefmt.go b/pkg/vmcp/headerforward/wirefmt/wirefmt.go new file mode 100644 index 0000000000..3d5a3bbf2e --- /dev/null +++ b/pkg/vmcp/headerforward/wirefmt/wirefmt.go @@ -0,0 +1,92 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +// Package wirefmt is the shared wire-format contract for headerForward +// data shipped from the operator to the vMCP runtime via pod env vars. +// +// Both the operator (producer) and the vMCP runtime (consumer) import this +// package: the operator emits TOOLHIVE_HEADER_FORWARD_ +// env vars and the runtime parses them at startup. Living in pkg/ rather +// than under cmd/thv-operator preserves one-way Go layering — the +// operator binary depends on pkg/ helpers, never the reverse. +package wirefmt + +import ( + "fmt" + "regexp" + "strings" +) + +// ManifestEnvVarPrefix is the prefix the operator uses when emitting one +// JSON-encoded manifest env var per backend on a workload pod for +// MCPServerEntry / MCPRemoteProxy headerForward configuration. +// +// The full env var name is "TOOLHIVE_HEADER_FORWARD_"; +// the value is JSON-encoded vmcp.HeaderForwardConfig with original header +// names preserved (uppercased/sanitized normalization can't recover hyphens +// or original casing on the receive side, so we carry the literal names in +// the JSON value instead). Plaintext header values appear inline; secret- +// backed entries carry only the secret identifier — the actual Secret +// value rides a sibling TOOLHIVE_SECRET_HEADER_FORWARD__ env var +// emitted via valueFrom.secretKeyRef and resolved at request time by +// secrets.EnvironmentProvider. +const ManifestEnvVarPrefix = "TOOLHIVE_HEADER_FORWARD_" + +// envVarSanitizer matches any character outside the env-var-safe set +// [A-Z0-9_]. Used by NormalizeForEnvVar to collapse non-conforming chars. +var envVarSanitizer = regexp.MustCompile(`[^A-Z0-9_]`) + +// NormalizeForEnvVar applies the canonical sanitization used in every +// header-forward env-var name: uppercase, hyphens to underscores, anything +// outside [A-Z0-9_] also to underscore. The secret-ref and the manifest +// emitters MUST share this function so a header that round-trips through +// one branch never collides with the other. +// +// Collision domain: this normalization is one-way and lossy. Two distinct +// header names that differ only in non-[A-Z0-9_-] characters will collapse +// to the same normalized form (e.g. "X-A.B" and "X-A_B" both become +// "X_A_B"). For backend (owner) names this is mitigated by Kubernetes +// DNS-1123 subdomain validation, which forbids underscores in resource +// names — two K8s resources cannot present a colliding pair. Header names +// don't have that guard, so callers iterating env vars should log when +// they see the same normalized owner key twice; readers that decode a +// single-backend manifest are not affected because the JSON value carries +// the literal user-authored header names verbatim. +func NormalizeForEnvVar(s string) string { + upper := strings.ToUpper(strings.ReplaceAll(s, "-", "_")) + return envVarSanitizer.ReplaceAllString(upper, "_") +} + +// SecretEnvVarName generates the environment variable name for a header +// forward secret. The generated name follows the TOOLHIVE_SECRET_ +// pattern expected by secrets.EnvironmentProvider. +// +// Parameters: +// - ownerName: the resource owning the header forwarding configuration +// (an MCPRemoteProxy or an MCPServerEntry). +// - headerName: the HTTP header name (e.g. "X-API-Key"). +// +// Returns the full env var name (e.g. "TOOLHIVE_SECRET_HEADER_FORWARD_X_API_KEY_MY_OWNER") +// and the secret identifier (the env var name with the leading +// "TOOLHIVE_SECRET_" stripped, used by secrets.EnvironmentProvider lookups). +func SecretEnvVarName(ownerName, headerName string) (envVarName, secretIdentifier string) { + sanitizedHeader := NormalizeForEnvVar(headerName) + sanitizedOwner := NormalizeForEnvVar(ownerName) + secretIdentifier = fmt.Sprintf("HEADER_FORWARD_%s_%s", sanitizedHeader, sanitizedOwner) + envVarName = fmt.Sprintf("TOOLHIVE_SECRET_%s", secretIdentifier) + return envVarName, secretIdentifier +} + +// ManifestEnvVarName generates the environment variable name for the +// JSON-encoded headerForward manifest emitted per backend on a workload +// pod. One env var per backend; the JSON value carries every configured +// header (plaintext values inline, secret entries by identifier). +// +// Returns the full env var name (e.g. "TOOLHIVE_HEADER_FORWARD_MY_OWNER") +// and the normalized owner segment ("MY_OWNER") used when iterating env +// vars matching ManifestEnvVarPrefix. +func ManifestEnvVarName(ownerName string) (envVarName, normalizedOwner string) { + normalizedOwner = NormalizeForEnvVar(ownerName) + envVarName = ManifestEnvVarPrefix + normalizedOwner + return envVarName, normalizedOwner +} diff --git a/pkg/vmcp/headerforward/wirefmt/wirefmt_test.go b/pkg/vmcp/headerforward/wirefmt/wirefmt_test.go new file mode 100644 index 0000000000..54e095a354 --- /dev/null +++ b/pkg/vmcp/headerforward/wirefmt/wirefmt_test.go @@ -0,0 +1,132 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package wirefmt + +import ( + "regexp" + "testing" + + "github.com/stretchr/testify/assert" +) + +// envVarPattern is the character set every env var name produced by this +// package must conform to. The runtime's secrets.EnvironmentProvider lookups +// expect [A-Z0-9_], so every name we emit MUST round-trip cleanly. +var envVarPattern = regexp.MustCompile(`^[A-Z0-9_]+$`) + +func TestNormalizeForEnvVar(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in string + want string + }{ + {"hyphens become underscores", "my-proxy", "MY_PROXY"}, + {"already uppercase preserved", "FOO_BAR", "FOO_BAR"}, + {"non-alnum collapses to underscore", "proxy.name@123", "PROXY_NAME_123"}, + {"mixed case lowercased to upper", "GitHub-Copilot", "GITHUB_COPILOT"}, + {"empty stays empty", "", ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := NormalizeForEnvVar(tt.in) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestSecretEnvVarName(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + ownerName string + headerName string + expectedEnvVarName string + expectedSecretIdentifier string + }{ + { + name: "simple names", + ownerName: "my-proxy", + headerName: "X-API-Key", + expectedEnvVarName: "TOOLHIVE_SECRET_HEADER_FORWARD_X_API_KEY_MY_PROXY", + expectedSecretIdentifier: "HEADER_FORWARD_X_API_KEY_MY_PROXY", + }, + { + name: "lowercase header upcases", + ownerName: "test-proxy", + headerName: "authorization", + expectedEnvVarName: "TOOLHIVE_SECRET_HEADER_FORWARD_AUTHORIZATION_TEST_PROXY", + expectedSecretIdentifier: "HEADER_FORWARD_AUTHORIZATION_TEST_PROXY", + }, + { + name: "multiple hyphens", + ownerName: "my-remote-proxy", + headerName: "X-Custom-Header", + expectedEnvVarName: "TOOLHIVE_SECRET_HEADER_FORWARD_X_CUSTOM_HEADER_MY_REMOTE_PROXY", + expectedSecretIdentifier: "HEADER_FORWARD_X_CUSTOM_HEADER_MY_REMOTE_PROXY", + }, + { + name: "special characters in owner name", + ownerName: "proxy.name@123", + headerName: "X-Token", + expectedEnvVarName: "TOOLHIVE_SECRET_HEADER_FORWARD_X_TOKEN_PROXY_NAME_123", + expectedSecretIdentifier: "HEADER_FORWARD_X_TOKEN_PROXY_NAME_123", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + envVarName, secretIdentifier := SecretEnvVarName(tt.ownerName, tt.headerName) + + assert.Equal(t, tt.expectedEnvVarName, envVarName) + assert.Equal(t, tt.expectedSecretIdentifier, secretIdentifier) + + assert.Regexp(t, envVarPattern, envVarName) + assert.Regexp(t, envVarPattern, secretIdentifier) + assert.Equal(t, "TOOLHIVE_SECRET_"+secretIdentifier, envVarName, + "envVarName must equal TOOLHIVE_SECRET_ + secretIdentifier") + }) + } +} + +func TestManifestEnvVarName(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + ownerName string + expectedEnvVarName string + expectedNormalizedOwn string + }{ + { + name: "hyphen owner", + ownerName: "github-copilot", + expectedEnvVarName: "TOOLHIVE_HEADER_FORWARD_GITHUB_COPILOT", + expectedNormalizedOwn: "GITHUB_COPILOT", + }, + { + name: "already uppercase", + ownerName: "STRIPE", + expectedEnvVarName: "TOOLHIVE_HEADER_FORWARD_STRIPE", + expectedNormalizedOwn: "STRIPE", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + envVarName, normalizedOwner := ManifestEnvVarName(tt.ownerName) + assert.Equal(t, tt.expectedEnvVarName, envVarName) + assert.Equal(t, tt.expectedNormalizedOwn, normalizedOwner) + assert.Regexp(t, envVarPattern, envVarName) + assert.Equal(t, ManifestEnvVarPrefix+normalizedOwner, envVarName, + "envVarName must equal ManifestEnvVarPrefix + normalizedOwner") + }) + } +} diff --git a/pkg/vmcp/health/monitor.go b/pkg/vmcp/health/monitor.go index 38eafef800..82a6c72fac 100644 --- a/pkg/vmcp/health/monitor.go +++ b/pkg/vmcp/health/monitor.go @@ -427,13 +427,19 @@ func (m *Monitor) performHealthCheck(ctx context.Context, backend *vmcp.Backend) return } - // Create BackendTarget from Backend + // Create BackendTarget from Backend. Carry CA bundle and header-forward config + // so health checks hit the backend with the same TLS trust and header injection + // as list/call requests — otherwise a healthy-to-the-monitor backend could fail + // for real traffic (or vice versa). target := &vmcp.BackendTarget{ WorkloadID: backend.ID, WorkloadName: backend.Name, BaseURL: backend.BaseURL, TransportType: backend.TransportType, + CABundlePath: backend.CABundlePath, + CABundleData: backend.CABundleData, AuthConfig: backend.AuthConfig, + HeaderForward: backend.HeaderForward, HealthStatus: vmcp.BackendUnknown, // Status is determined by the health check Metadata: backend.Metadata, } diff --git a/pkg/vmcp/registry.go b/pkg/vmcp/registry.go index ba9f2c6b81..ccce6e321c 100644 --- a/pkg/vmcp/registry.go +++ b/pkg/vmcp/registry.go @@ -373,6 +373,7 @@ func BackendToTarget(backend *Backend) *BackendTarget { AuthConfig: backend.AuthConfig, SessionAffinity: false, // TODO: Add session affinity support in future phases HealthStatus: backend.HealthStatus, + HeaderForward: backend.HeaderForward, Metadata: backend.Metadata, } } diff --git a/pkg/vmcp/types.go b/pkg/vmcp/types.go index 025fd3d746..8867da1131 100644 --- a/pkg/vmcp/types.go +++ b/pkg/vmcp/types.go @@ -15,6 +15,27 @@ import ( // This file contains shared domain types used across multiple vmcp subpackages. // Following DDD principles, these are core domain concepts that cross bounded contexts. +// HeaderForwardConfig configures HTTP headers injected into outbound requests +// from the vMCP runtime to a static backend. AddPlaintextHeaders carries literal +// values; AddHeadersFromSecret maps header names to secret identifiers resolved +// via secrets.EnvironmentProvider at request time (env var TOOLHIVE_SECRET_). +// +// Secret values MUST NOT appear in this struct — only identifiers. The operator +// injects actual Secret values into the vMCP pod as env vars via +// valueFrom.secretKeyRef at Deployment rendering time. Plaintext values are +// also injected via env vars by the operator (literal values, not secret refs) +// and ingested at vMCP startup; the type is therefore purely a runtime +// representation and is not part of any CRD schema. +type HeaderForwardConfig struct { + // AddPlaintextHeaders is a map of canonical HTTP header name to literal value. + AddPlaintextHeaders map[string]string `json:"addPlaintextHeaders,omitempty" yaml:"addPlaintextHeaders,omitempty"` + + // AddHeadersFromSecret maps canonical HTTP header name to secret identifier. + // The vMCP runtime resolves each identifier via secrets.EnvironmentProvider, + // which reads TOOLHIVE_SECRET_ from the pod environment. + AddHeadersFromSecret map[string]string `json:"addHeadersFromSecret,omitempty" yaml:"addHeadersFromSecret,omitempty"` +} + // BackendTarget identifies a specific backend workload and provides // the information needed to forward requests to it. type BackendTarget struct { @@ -78,6 +99,11 @@ type BackendTarget struct { // HealthStatus indicates the current health of the backend. HealthStatus BackendHealthStatus + // HeaderForward carries per-backend HTTP header injection configuration + // applied by the vMCP client to every outbound request targeting this backend + // (list, call, health-check). Nil when no headers are configured. + HeaderForward *HeaderForwardConfig + // Metadata stores additional backend-specific information. Metadata map[string]string } @@ -328,6 +354,11 @@ type Backend struct { // +optional AuthConfigRef string + // HeaderForward carries per-backend HTTP header injection configuration. + // Only populated for entry-type backends whose MCPServerEntry declares + // spec.headerForward. Nil when the entry has no header forwarding configured. + HeaderForward *HeaderForwardConfig + // Metadata stores additional backend information. Metadata map[string]string } diff --git a/pkg/vmcp/workloads/k8s.go b/pkg/vmcp/workloads/k8s.go index 7a8ffa557e..94b92eacc1 100644 --- a/pkg/vmcp/workloads/k8s.go +++ b/pkg/vmcp/workloads/k8s.go @@ -22,6 +22,7 @@ import ( transporttypes "github.com/stacklok/toolhive/pkg/transport/types" "github.com/stacklok/toolhive/pkg/vmcp" "github.com/stacklok/toolhive/pkg/vmcp/auth/converters" + "github.com/stacklok/toolhive/pkg/vmcp/headerforward/wirefmt" "github.com/stacklok/toolhive/pkg/workloads/types" ) @@ -582,9 +583,46 @@ func (d *k8sDiscoverer) mcpServerEntryToBackend(ctx context.Context, entry *mcpv return nil } + // Per-backend HTTP header injection. Mirrors the static-mode operator + // path in cmd/thv-operator/controllers/virtualmcpserver_deployment.go::buildHeaderForwardManifestForEntry: + // plaintext values verbatim, secret refs translated to identifiers + // (HEADER_FORWARD__) the round tripper resolves at request + // time via secrets.EnvironmentProvider. + backend.HeaderForward = headerForwardFromEntry(entry) + return backend } +// headerForwardFromEntry projects MCPServerEntry.spec.headerForward onto the +// runtime *vmcp.HeaderForwardConfig the round tripper consumes. Returns nil +// when the entry has no headerForward configured. Secret values never +// appear here — only identifiers, which are dereferenced at request time. +func headerForwardFromEntry(entry *mcpv1beta1.MCPServerEntry) *vmcp.HeaderForwardConfig { + if entry == nil || entry.Spec.HeaderForward == nil { + return nil + } + src := entry.Spec.HeaderForward + + cfg := &vmcp.HeaderForwardConfig{ + AddPlaintextHeaders: src.AddPlaintextHeaders, + } + for _, ref := range src.AddHeadersFromSecret { + if ref.ValueSecretRef == nil { + continue + } + _, identifier := wirefmt.SecretEnvVarName(entry.Name, ref.HeaderName) + if cfg.AddHeadersFromSecret == nil { + cfg.AddHeadersFromSecret = make(map[string]string) + } + cfg.AddHeadersFromSecret[ref.HeaderName] = identifier + } + + if len(cfg.AddPlaintextHeaders) == 0 && len(cfg.AddHeadersFromSecret) == 0 { + return nil + } + return cfg +} + // mapMCPServerEntryPhaseToHealth converts a MCPServerEntryPhase to a backend health status. func mapMCPServerEntryPhaseToHealth(phase mcpv1beta1.MCPServerEntryPhase) vmcp.BackendHealthStatus { switch phase { diff --git a/pkg/vmcp/workloads/k8s_test.go b/pkg/vmcp/workloads/k8s_test.go index edae76e868..5c32660044 100644 --- a/pkg/vmcp/workloads/k8s_test.go +++ b/pkg/vmcp/workloads/k8s_test.go @@ -5,6 +5,7 @@ package workloads import ( "context" + "encoding/json" "testing" "github.com/stretchr/testify/assert" @@ -1829,3 +1830,175 @@ func TestFetchCABundleData(t *testing.T) { }) } } + +// TestHeaderForwardFromEntry covers the dynamic-mode projection of +// MCPServerEntry.spec.headerForward into the runtime *vmcp.HeaderForwardConfig. +// The shape MUST match what the operator emits in the static-mode JSON +// manifest env var so both code paths feed equivalent data into the round +// tripper. +func TestHeaderForwardFromEntry(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + entry *mcpv1beta1.MCPServerEntry + want *vmcp.HeaderForwardConfig + }{ + { + name: "nil entry yields nil", + entry: nil, + want: nil, + }, + { + name: "entry without headerForward yields nil", + entry: &mcpv1beta1.MCPServerEntry{ + ObjectMeta: metav1.ObjectMeta{Name: "demo", Namespace: testNamespace}, + Spec: mcpv1beta1.MCPServerEntrySpec{RemoteURL: "https://x"}, + }, + want: nil, + }, + { + name: "plaintext only — header names preserved verbatim", + entry: &mcpv1beta1.MCPServerEntry{ + ObjectMeta: metav1.ObjectMeta{Name: "github-copilot", Namespace: testNamespace}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{ + "X-MCP-Toolsets": "projects,issues", + "X-Trace-Id": "kind-test", + }, + }, + }, + }, + want: &vmcp.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{ + "X-MCP-Toolsets": "projects,issues", + "X-Trace-Id": "kind-test", + }, + }, + }, + { + name: "secret only — values become identifiers", + entry: &mcpv1beta1.MCPServerEntry{ + ObjectMeta: metav1.ObjectMeta{Name: "stripe", Namespace: testNamespace}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + { + HeaderName: "X-API-Key", + ValueSecretRef: &mcpv1beta1.SecretKeyRef{ + Name: "stripe-key", + Key: "token", + }, + }, + }, + }, + }, + }, + want: &vmcp.HeaderForwardConfig{ + AddHeadersFromSecret: map[string]string{ + "X-API-Key": "HEADER_FORWARD_X_API_KEY_STRIPE", + }, + }, + }, + { + name: "mixed plaintext + secret", + entry: &mcpv1beta1.MCPServerEntry{ + ObjectMeta: metav1.ObjectMeta{Name: "alpha", Namespace: testNamespace}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{"X-Trace": "alpha-trace"}, + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + { + HeaderName: "X-Token", + ValueSecretRef: &mcpv1beta1.SecretKeyRef{ + Name: "alpha-secret", + Key: "tok", + }, + }, + }, + }, + }, + }, + want: &vmcp.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{"X-Trace": "alpha-trace"}, + AddHeadersFromSecret: map[string]string{ + "X-Token": "HEADER_FORWARD_X_TOKEN_ALPHA", + }, + }, + }, + { + name: "secret ref with nil ValueSecretRef is skipped — empty config returns nil", + entry: &mcpv1beta1.MCPServerEntry{ + ObjectMeta: metav1.ObjectMeta{Name: "ghost", Namespace: testNamespace}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + {HeaderName: "X-Stray", ValueSecretRef: nil}, + }, + }, + }, + }, + want: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := headerForwardFromEntry(tt.entry) + assert.Equal(t, tt.want, got) + }) + } +} + +// TestHeaderForwardParity_StaticVsDynamic pins that the dynamic-mode +// projection produces a runtime-equivalent shape to what the operator's +// static-mode JSON manifest serializes. If either path drifts, the round +// tripper sees a different Backend.HeaderForward depending on mode — +// which is exactly the bug pattern PR #5239 fixes for static mode and +// this finding (F2) extends to dynamic mode. +// +// The test compares the JSON serialization of the dynamic-mode result +// to a hand-written manifest that mirrors what +// virtualmcpserver_deployment.go::buildHeaderForwardManifestForEntry +// would emit for the same entry. The two are required to be byte-equal +// after json.Marshal (which sorts map keys). +func TestHeaderForwardParity_StaticVsDynamic(t *testing.T) { + t.Parallel() + + entry := &mcpv1beta1.MCPServerEntry{ + ObjectMeta: metav1.ObjectMeta{Name: "github-copilot-fake", Namespace: testNamespace}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + RemoteURL: "https://api.example/mcp", + Transport: "streamable-http", + GroupRef: &mcpv1beta1.MCPGroupRef{Name: "g"}, + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{ + "X-MCP-Toolsets": "projects,issues,pull_requests", + "X-Trace-Id": "kind-test", + }, + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + { + HeaderName: "X-Api-Key", + ValueSecretRef: &mcpv1beta1.SecretKeyRef{ + Name: "test-secret", + Key: "token", + }, + }, + }, + }, + }, + } + + dynamic := headerForwardFromEntry(entry) + require.NotNil(t, dynamic) + + dynamicJSON, err := json.Marshal(dynamic) + require.NoError(t, err) + + const wantStaticManifestJSON = `{"addPlaintextHeaders":{"X-MCP-Toolsets":"projects,issues,pull_requests","X-Trace-Id":"kind-test"},"addHeadersFromSecret":{"X-Api-Key":"HEADER_FORWARD_X_API_KEY_GITHUB_COPILOT_FAKE"}}` + + assert.JSONEq(t, wantStaticManifestJSON, string(dynamicJSON), + "dynamic-mode HeaderForward must marshal to the same shape as the static-mode operator manifest") +}