Forward MCPServerEntry headerForward to vMCP outbound requests#5013
Forward MCPServerEntry headerForward to vMCP outbound requests#5013ChrisJBurns wants to merge 1 commit intomainfrom
Conversation
MCPServerEntry.spec.headerForward was accepted by the CRD but silently ignored when the entry was consumed as a static backend of VirtualMCPServer. Only MCPRemoteProxy honored the field, so vMCP requests to remoteUrl arrived without the configured headers (e.g. GitHub Copilot's X-MCP-Toolsets). Mirror the MCPRemoteProxy pattern so header values never enter the ConfigMap: - Surface headerForward on pkg/vmcp.Backend, BackendTarget, and vmcpconfig.StaticBackendConfig so the runtime model carries per-backend header injection alongside auth and CA bundle. - Extract headerForward in mcpServerEntryToBackend and threader the values through buildHeaderForwardMap and convertBackendsToStaticBackends to the ConfigMap. Plaintext is copied verbatim; Secret refs become only env-var identifiers via ctrlutil.GenerateHeaderForwardSecretEnvVarName. - Emit one env var per (entry, header) on the vMCP Deployment with valueFrom.secretKeyRef so the Secret value is injected at pod start. - Insert a new headerForwardRoundTripper in pkg/vmcp/client between identityPropagatingRoundTripper and authRoundTripper. Resolve headers once at client-factory time via secrets.EnvironmentProvider; reject restricted headers via the shared pkg/transport/middleware.RestrictedHeaders set. - Validate referenced Secrets in MCPServerEntry's reconciler and surface the result through a new HeaderSecretRefsValidated condition, matching MCPRemoteProxy's HeaderSecretNotFound reason. - Carry HeaderForward, CABundlePath, and CABundleData into the BackendTarget built by the health monitor so health checks hit backends with the same TLS trust and header injection as list/call. - Generalize GenerateHeaderForwardSecretEnvVarName to accept an owner name rather than a proxy name; the existing MCPRemoteProxy caller is unchanged and MCPServerEntry uses the entry name as the disambiguator. Closes #4996 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ChrisJBurns
requested review from
JAORMX,
amirejaz,
jerm-dro,
jhrozek and
yrobla
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5013 +/- ##
==========================================
+ Coverage 68.98% 69.09% +0.10%
==========================================
Files 552 555 +3
Lines 72996 73344 +348
==========================================
+ Hits 50359 50679 +320
+ Misses 19639 19633 -6
- Partials 2998 3032 +34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
Four inline comments from a review pass. All non-blocking suggestions. See individual comments for details.
| } | ||
| } | ||
|
|
||
| return envVars, nil |
There was a problem hiding this comment.
buildHeaderForwardEnvVarsForEntries returns envVars without sorting, but the two sibling functions in this same file (discoverExternalAuthConfigSecrets at line 622, discoverInlineExternalAuthConfigSecrets at line 668) both sort by name and include a comment explaining why: containerNeedsUpdate uses reflect.DeepEqual on the env list, and unsorted env vars produce a continuous deployment update loop. PR #4783 fixed exactly this bug for the sibling functions.
With ≥2 entries declaring headerForward.addHeadersFromSecret, the order that listMCPServerEntriesAsMap + inner slice iteration produces isn't guaranteed stable across reconciles, so this function can trigger the same rollout loop.
Suggest adding the same sort before return envVars, nil, mirroring the existing comment:
sort.Slice(envVars, func(i, j int) bool {
return envVars[i].Name < envVars[j].Name
})| backendName, canonical, | ||
| ) | ||
| } | ||
| value, err := provider.GetSecret(nil, identifier) //nolint:staticcheck // provider ignores ctx |
There was a problem hiding this comment.
provider.GetSecret(nil, identifier) passes a literal nil context.Context. The current EnvironmentProvider ignores ctx so this works today, but the secretsProvider field is typed secrets.Provider to allow substitution (Vault, 1Password, cloud KMS, etc.). Any provider that calls ctx.Err() or passes ctx into an outgoing HTTP/gRPC client will nil-deref on every request.
Suggest passing context.Background() here (resolution happens at client-factory construction time, so the caller's ctx isn't needed) and removing the //nolint:staticcheck suppression.
As a future improvement, it would be cleaner to thread a ctx context.Context parameter through buildHeaderForwardTripper → resolveHeaderForward so any future cancellation-aware provider gets the caller's context.
| // staticHeaderForwardFromEntry builds the ConfigMap-ready HeaderForwardConfig for | ||
| // a single MCPServerEntry. Returns nil when the entry declares no headers (e.g., | ||
| // all AddHeadersFromSecret entries have nil ValueSecretRef). | ||
| func staticHeaderForwardFromEntry(entry *mcpv1beta1.MCPServerEntry) *vmcptypes.HeaderForwardConfig { |
There was a problem hiding this comment.
staticHeaderForwardFromEntry here and buildHeaderForwardFromEntry in pkg/vmcp/workloads/k8s.go:598 are near-duplicates — same return type (*vmcp.HeaderForwardConfig), same logic (plaintext copy + secret refs → identifiers via GenerateHeaderForwardSecretEnvVarName), minor cosmetic differences (maps.Copy vs manual loop, plaintext == nil vs len(plaintext) == 0).
Both paths must produce byte-identical identifiers because the operator-side function writes to the ConfigMap and the runtime-side function writes to the vmcp.Backend registry that the runtime reads — divergence would produce a silent "header not forwarded" mismatch between what the ConfigMap declares and what the runtime looks up.
Consider extracting a single helper in cmd/thv-operator/pkg/controllerutil/ (next to GenerateHeaderForwardSecretEnvVarName, which both callers already import) so drift is impossible. Not blocking for this PR, but worth doing as a follow-up.
| // | ||
| // See individual subpackage documentation for detailed usage and examples. | ||
| // | ||
| // +groupName=toolhive.stacklok.dev |
There was a problem hiding this comment.
Adding +groupName=toolhive.stacklok.dev here (necessary for controller-gen to emit DeepCopy for the new HeaderForwardConfig) creates a new ## toolhive.stacklok.dev/vmcp section in docs/operator/crd-api.md with ~30 blank lines before the single HeaderForwardConfig entry.
The sibling packages that set +groupName (pkg/audit, pkg/telemetry, pkg/vmcp/config, pkg/vmcp/auth/types) also set +versionName=<subgroup> and have short 4-line package comments. pkg/vmcp/doc.go has a 157-line package comment, which crd-ref-docs appears to render as group description with each blank-ish line becoming a blank markdown line.
Minimal fix: add // +versionName=vmcp to match the sibling convention and regenerate. If that doesn't clean up the whitespace, filtering the group in docs/operator/crd-ref-config.yaml is an alternative.
2 participants
Summary
Closes #4996
MCPServerEntry.spec.headerForwardwas accepted by the CRD but silently ignored when the entry was consumed as a static backend ofVirtualMCPServer. OnlyMCPRemoteProxyhonored the field, so vMCP requests toremoteUrlarrived without the configured headers — breaking use cases like GitHub Copilot'sX-MCP-Toolsetsmulti-toolset selection.Medium level
pkg/vmcp.BackendandBackendTargetgain aHeaderForwardfield;vmcpconfig.StaticBackendConfiggains the same so headers ride alongside auth/CA-bundle through the whole pipeline.mcpServerEntryToBackendextractsheaderForward. A newbuildHeaderForwardMapmirrorsbuildCABundlePathMapand threads per-backend header config intoconvertBackendsToStaticBackends. Plaintext headers are copied verbatim; secret refs become only env-var identifiers — secret values never enter the ConfigMap.buildHeaderForwardEnvVarsForEntriesemits onevalueFrom.secretKeyRefenv var per (entry, header), scoped by entry name to prevent collisions across entries in the same group.headerForwardRoundTripperinpkg/vmcp/clientsits betweenidentityPropagatingRoundTripper(outer) andauthRoundTripper(inner). Headers are resolved once at client creation viasecrets.EnvironmentProvider(TOOLHIVE_SECRET_<ident>). Restricted headers (Host, hop-by-hop, X-Forwarded-*, etc.) are rejected via the existingmiddleware.RestrictedHeadersset — same list MCPRemoteProxy uses.BackendTargetconstruction now carriesHeaderForward,CABundlePath, andCABundleDataso health checks hit backends with the same TLS trust and header injection as list/call traffic.MCPServerEntrygains aHeaderSecretRefsValidatedcondition (reusing MCPRemoteProxy'sHeaderSecretNotFoundreason) that flips the entry toFailedwhen a referenced Secret is missing.GenerateHeaderForwardSecretEnvVarName(proxyName, …)renamed to(ownerName, …)so bothMCPRemoteProxyandMCPServerEntryshare one source of truth.Low level
pkg/vmcp/types.goHeaderForwardConfigtype (+gendoc, +kubebuilder:object:generate); addHeaderForwardfield toBackendandBackendTarget.pkg/vmcp/doc.go+groupNamemarker soHeaderForwardConfigrenders indocs/operator/crd-api.md.pkg/vmcp/registry.goBackendToTargetnow copiesHeaderForward.pkg/vmcp/config/config.goStaticBackendConfig.HeaderForwardfield (*vmcp.HeaderForwardConfig).pkg/vmcp/aggregator/discoverer.godiscoverFromStaticConfigpropagatesHeaderForward.pkg/vmcp/workloads/k8s.gomcpServerEntryToBackendpopulatesbackend.HeaderForward; newbuildHeaderForwardFromEntryhelper turns secret refs into identifiers via the shared helper.pkg/vmcp/health/monitor.goHeaderForward,CABundlePath,CABundleDatainto the health-checkBackendTarget.pkg/vmcp/client/header_forward.goheaderForwardRoundTripper;buildHeaderForwardTripperandresolveHeaderForwardresolve headers once at factory time with restricted-header rejection.pkg/vmcp/client/client.gosecretsProviderfield onhttpBackendClient; insert tripper in chain between identity (outer) and auth (inner).cmd/thv-operator/api/v1beta1/mcpserverentry_types.goConditionTypeMCPServerEntryHeaderSecretRefsValidatedand reason constants.cmd/thv-operator/controllers/mcpserverentry_controller.govalidateHeaderForwardSecretRefs; wired into the reconcile fan-out.cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig.gobuildHeaderForwardMap; threads through to config assembly.cmd/thv-operator/controllers/virtualmcpserver_controller.goconvertBackendsToStaticBackendsacceptsheaderForwardMapand populatesStaticBackendConfig.HeaderForward.cmd/thv-operator/controllers/virtualmcpserver_deployment.gobuildHeaderForwardEnvVarsForEntries; called frombuildEnvVarsForVmcp.cmd/thv-operator/pkg/controllerutil/externalauth.goproxyNameparameter toownerName; no behavioral change.cmd/thv-operator/Taskfile.ymlpkg/vmcpto controller-gen paths soHeaderForwardConfiggets a generated DeepCopy.docs/operator/crd-api.md,deploy/charts/operator-crds/**,pkg/vmcp/zz_generated.deepcopy.go,pkg/vmcp/config/zz_generated.deepcopy.goType of change
Test plan
task lint-fixpasses (0 issues)task buildpassestask testpasses for all touched packages (48 packages acrosspkg/vmcp/...andcmd/thv-operator/...)TestBuildHeaderForwardFromEntry(+ secret-leak sentinel) inpkg/vmcp/workloads/TestBuildHeaderForwardMap(+ per-entry identifier scoping, secret-leak sentinel) incmd/thv-operator/controllers/TestBuildHeaderForwardEnvVarsForEntriesincmd/thv-operator/controllers/TestHeaderForwardRoundTripper_*,TestResolveHeaderForward_*,TestBuildHeaderForwardTripper_*, and an end-to-endhttptest.Servertest inpkg/vmcp/client/MCPServerEntryreconciler cases coveringHeaderSecretsValid/HeaderSecretNotFoundtask operator-generate,task operator-manifests,task crdref-genrun cleanly with no residual diff after commitSpecial notes for reviewers
trace → identity → headerForward → auth → http. The new tripper refuses to clobber an already-set header, so auth/identity/trace always win. Restricted-header rejection uses the existingpkg/transport/middleware.RestrictedHeadersset to keep parity.zz_generated.deepcopy.goupdates and CRD YAML regeneration are bundled in the same commit.cmd/thv-operator/Taskfile.ymladdspkg/vmcpto the controller-gen paths (needed becauseHeaderForwardConfiglives at thepkg/vmcproot to keeppkg/vmcp/configfrom cycling back onto itself).design-mcpserverentry-headerforward.md). Not committed as it's a planning artefact, but happy to share if useful.Generated with Claude Code