Forward MCPServerEntry headerForward to vMCP outbound requests#5239
Open
ChrisJBurns wants to merge 8 commits intomainfrom
Open
Forward MCPServerEntry headerForward to vMCP outbound requests#5239ChrisJBurns wants to merge 8 commits intomainfrom
ChrisJBurns wants to merge 8 commits intomainfrom
Conversation
MCPServerEntry.spec.headerForward.{addPlaintextHeaders,addHeadersFromSecret}
was accepted by the CRD but never sent on outbound requests when the entry
was consumed as a static backend of a VirtualMCPServer. Only MCPRemoteProxy
forwarded the field; vMCP requests to remoteUrl arrived without the
configured headers — breaking use cases like GitHub Copilot's X-MCP-Toolsets
multi-toolset selection.
Closes #4996.
Mirror the MCPRemoteProxy pattern using pod env vars as the operator-to-vMCP
delivery channel. Zero changes to pkg/vmcp/config and zero CRD/docs diff —
the per-backend HeaderForward data never enters the ConfigMap or any
CRD-reachable type:
- Operator: extend buildHeaderForwardEnvVarsForEntries to emit one env var
per (entry, header) for both plaintext and secret-backed headers. Plaintext
values land as literal env vars named TOOLHIVE_HEADER_PLAINTEXT_<H>_<E>;
secret-backed values keep the existing TOOLHIVE_SECRET_HEADER_FORWARD_<H>_<E>
via valueFrom.secretKeyRef. Header normalization is shared via the new
normalizeHeaderForEnvVar helper so the secret and plaintext branches
cannot diverge.
- Runtime: walk os.Environ at vMCP startup in static mode (readHeaderForwardFromEnv)
to reconstruct map[backendName]*HeaderForwardConfig. Plaintext entries
land in AddPlaintextHeaders; secret entries land in AddHeadersFromSecret
carrying only the identifier (resolved later via secrets.EnvironmentProvider
inside resolveHeaderForward at request time). Stray env vars whose decoded
entry name doesn't match a known static backend are dropped.
- Discoverer: NewUnifiedBackendDiscovererWithStaticBackends gains a
headerForwardByBackend parameter; discoverFromStaticConfig attaches the
matching map entry to Backend.HeaderForward.
- Round tripper (pkg/vmcp/client/header_forward.go): inserted between
identity (outer) and auth (inner). Resolves headers once at client-factory
time via secrets.EnvironmentProvider; rejects restricted headers via the
shared pkg/transport/middleware.RestrictedHeaders set; auth always wins
over user-supplied headers because it runs after this tripper.
- Health monitor: BackendTarget construction now carries HeaderForward,
CABundlePath, and CABundleData so health probes hit backends with the
same TLS trust and header injection as list/call traffic.
- MCPServerEntry reconciler: HeaderSecretRefsValidated condition flips the
entry to Failed when a referenced Secret is missing, matching
MCPRemoteProxy's HeaderSecretNotFound reason. GenerateHeaderForwardSecretEnvVarName
now takes ownerName rather than proxyName so both MCPRemoteProxy and
MCPServerEntry share one source of truth.
- Dynamic mode (K8s API discovery): pkg/vmcp/workloads/k8s.go::mcpServerEntryToBackend
is unchanged — it reads headerForward directly from the MCPServerEntry CRD
at backend-construction time, so no env-var path is needed there.
Acceptance gates verified: zero diff under deploy/charts/operator-crds/ and
docs/operator/crd-api.md after task operator-manifests + task operator-generate
+ task crdref-gen. Zero non-test changes under pkg/vmcp/config/.
Co-Authored-By: Claude Opus 4.7 (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}
Contributor
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 #5239 +/- ##
==========================================
+ Coverage 67.91% 67.92% +0.01%
==========================================
Files 610 615 +5
Lines 62522 63001 +479
==========================================
+ Hits 42464 42796 +332
- Misses 16879 16997 +118
- Partials 3179 3208 +29 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Manual verification on a Kind cluster surfaced a real bug in the env-var encoding: the operator was emitting one TOOLHIVE_HEADER_PLAINTEXT_<H>_<E> env var per (entry, header) pair. The runtime parsed the env-var name back into (header, entry) segments, but the env-var name encoding upper-snakes the header name (X-MCP-Toolsets becomes X_MCP_TOOLSETS), and there's no way to recover the original casing/punctuation from the env-var name alone. The fix shipped headers — but with the WRONG names, which would silently break GitHub Copilot's X-MCP-Toolsets selector (the actual reproducer in #4996). Replace the per-(entry, header) plaintext env vars with one JSON-encoded manifest env var per backend named TOOLHIVE_HEADER_FORWARD_<entry>. The JSON value carries every configured header with original user-authored names preserved. Plaintext values appear inline; secret-backed entries carry only the secret identifier — the actual Secret value still rides the existing TOOLHIVE_SECRET_HEADER_FORWARD_<H>_<E> env var via valueFrom.secretKeyRef and never enters the operator's view of the world. Operator-side: - Drop GenerateHeaderForwardPlaintextEnvVarName. - Add GenerateHeaderForwardManifestEnvVarName + HeaderForwardManifestEnvVarPrefix. - Rewrite buildHeaderForwardEnvVarsForEntries to emit one JSON manifest per backend (json.Marshal sorts map keys alphabetically, giving deterministic Deployment-spec rendering). - New private headerForwardManifest type mirrors vmcp.HeaderForwardConfig at the wire level so the runtime can json.Unmarshal directly. - Update the test fixtures to assert the new env-var shape with JSONEq. Runtime-side: - Replace prefix-walk + suffix-split logic in readHeaderForwardFromEnv with a focused walk over TOOLHIVE_HEADER_FORWARD_* env vars + json.Unmarshal into vmcp.HeaderForwardConfig directly. - Drop splitHeaderEntrySuffix (no longer relevant — the env-var name carries only the entry segment). - Update test fixtures with the new JSON shape; new case for malformed manifest skipping; new case for secret-env-var-only-no-manifest defensive path. Manual verification on Kind: - Echo backend now records X-MCP-Toolsets, X-Trace-Id, X-Api-Key with ORIGINAL casing/punctuation on every outbound request. - Acceptance gates still green: zero diff under deploy/charts/operator-crds/ and docs/operator/crd-api.md after task operator-manifests + task operator-generate + task crdref-gen. Add docs/manual-verification/headerforward-kind.md and a co-located manifest.yaml so anyone can reproduce the integration test locally end-to-end on Kind. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
size/XL
Two parallel architectural reviews (code-reviewer + go-architect) flagged
five concrete cuts that don't change behavior. Apply them in one commit:
(1) Export ctrlutil.NormalizeHeaderForEnvVar and delete the duplicate
normalizeForEnvSegment in pkg/vmcp/cli/header_forward_env.go. The
"avoiding ctrlutil's regexp surface" justification was wrong: ctrlutil
is already imported. Saves ~19 lines plus the duplication risk that
.claude/rules/go-style.md "Avoid parallel types that drift" warns
about.
(2) Delete headerForwardManifest in virtualmcpserver_deployment.go.
It mirrored vmcp.HeaderForwardConfig at the wire level but was
structurally identical — exactly the parallel-types-drift trap. The
operator now imports vmcp directly and marshals
*vmcp.HeaderForwardConfig, removing the manual field-copy loop too.
(3) Delete HeaderForwardSecretEnvVarPrefix and the !HasPrefix
disambiguation in the runtime walker. Phantom check: the secret
env-var prefix is "TOOLHIVE_SECRET_HEADER_FORWARD_" and the manifest
prefix is "TOOLHIVE_HEADER_FORWARD_"; HasPrefix(manifest) is mutually
exclusive with the secret prefix, so the negative check was
reasoning about an unreachable state.
(4) Drop the staticBackendNames filter from readHeaderForwardFromEnv.
Defensive coding without a threat model — the operator is the sole
writer of these env vars; if a stray manifest env var exists, that's
an operator bug and the discoverer's existing backend-resolution
path handles it. The walker now returns a map keyed by the
normalized entry segment from the env-var suffix; the discoverer
normalizes Backend.Name through ctrlutil.NormalizeHeaderForEnvVar
before indexing. Removes a parameter, simplifies the serve.go
plumbing, drops one defensive subtest.
(5) Test cleanups:
- TestReadHeaderForwardFromEnv "secret env var alone" subtest
replaced with a richer "secret env var must not be parsed as a
manifest" subtest that's now a positive assertion on the manifest
walker (not testing an impossible code path).
- Drop TestBuildHeaderForwardEnvVarsForEntries deterministic-across-
reconciles subtest (~33 lines): asserts that json.Marshal sorts
map keys, which is testing the stdlib, not our code. Existing
plaintext-headers case already pins the JSON output exactly.
- Drop TestHeaderForwardRoundTripper_EndToEndHTTPTestServer (~40
lines): captureTripper IS the receiver in the existing tests, so
a real httptest.Server adds no coverage.
- Replace `_ = fmt.Errorf(...)` no-op in readHeaderForwardFromEnv
with slog.Warn so malformed manifests are actually surfaced.
Net: ~185 lines removed, ~55 lines added (the directly-marshalled
HeaderForwardConfig plus the slog.Warn). Behavior identical end-to-end:
verified on Kind that all three headers (X-MCP-Toolsets, X-Trace-Id,
X-Api-Key) still arrive at the echo backend with original casing.
Acceptance gates still green: zero diff under deploy/charts/operator-crds/
and docs/operator/crd-api.md after task operator-manifests +
task operator-generate + task crdref-gen.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
size/XL
The reproduction guide is local-only — it's a personal scratch artefact, not something the project wants checked in. Kept on disk at ~/Documents/toolhive-headerforward-kind-test/ for follow-on iterations. The PR itself stays focused on the code change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
size/XL
Trailing newline left over after removing the EndToEndHTTPTestServer test in the previous commit. No semantic change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
size/XL
Single follow-up commit closing nine review findings on the MCPServerEntry headerForward → vMCP wiring. F7 (parallel HeaderForwardConfig types) is intentionally deferred. - F1: Move env-var wire-format helpers from cmd/thv-operator/pkg/ controllerutil to a neutral pkg/vmcp/headerforward/wirefmt package so the producer (operator) and consumer (vMCP runtime) share one contract without inverting the cmd/ → pkg/ layering rule. - F2: Wire HeaderForward through dynamic-mode mcpServerEntryToBackend; parity test asserts the JSON marshal of the discovered config equals the static-mode manifest emitted by the operator. - F3: Add a Secret get/list/watch RBAC marker on the MCPServerEntry reconciler so the in-cluster role permits the new validator's reads. - F4 + F12: Sort env vars deterministically before applying to the Deployment to avoid the informer-cache update loop hazard, with a shuffled-input determinism test. - F6: Thread context.Context through buildHeaderForwardTripper and resolveHeaderForward so secret-provider lookups participate in cancellation and tracing. - F8 + F9 + F15: Watch referenced Secrets via a field index, validate that the named key exists inside the Secret (not just that the Secret itself exists), and aggregate all per-ref failures into one condition message instead of returning on the first failure. - F10: Iterate the full middleware.RestrictedHeaders set in the rejection test (was a hardcoded subset) and fix a misleading subtest name. - F11: Extend the test mockProvider with per-key error injection and verify non-NotFound provider errors propagate with %w. - F16: Warn loudly when readHeaderForwardFromEnv encounters two manifest env vars sharing the same normalized owner segment, and document the collision domain on wirefmt.NormalizeForEnvVar. Also adds an end-to-end test through NewHTTPBackendClient → defaultClientFactory with httptest.Server that resolves a secret-backed header via secrets.EnvironmentProvider and asserts the test server received both the plaintext and resolved-secret headers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
size/XL
github-actions
Bot
added
size/XL
github-actions
Bot
added
size/XL
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #4996.
MCPServerEntry.spec.headerForward.{addPlaintextHeaders,addHeadersFromSecret}was accepted by the CRD but never sent on outbound requests when the entry was consumed as a static backend of aVirtualMCPServer. OnlyMCPRemoteProxyforwarded the field — vMCP requests toremoteUrlarrived without the configured headers, breaking use cases like GitHub Copilot'sX-MCP-Toolsetsmulti-toolset selection.This PR fixes the bug without touching
pkg/vmcp/configby mirroringMCPRemoteProxy's existing pattern: the operator emits per-(entry, header) env vars on the vMCP pod (literal values for plaintext,valueFrom.secretKeyReffor secrets), and the vMCP runtime walks the well-known prefixes at startup to reconstructBackend.HeaderForwardin static mode.TOOLHIVE_OTEL_HEADER_*already establishes plaintext-header-via-env in this codebase, so the convention isn't new.Zero CRD/docs diff. Zero non-test changes under
pkg/vmcp/config/.Medium level
buildHeaderForwardEnvVarsForEntriesnow emits literal-value env vars foraddPlaintextHeadersalongside the existingvalueFrom.secretKeyRefenv vars foraddHeadersFromSecret. Header normalization is extracted into a privatenormalizeHeaderForEnvVarhelper that bothGenerateHeaderForwardSecretEnvVarNameand the newGenerateHeaderForwardPlaintextEnvVarNameshare, so the secret and plaintext branches can never diverge on a header that round-trips through one and not the other.readHeaderForwardFromEnv(pkg/vmcp/cli/header_forward_env.go) walksos.Environ()for theTOOLHIVE_HEADER_PLAINTEXT_*andTOOLHIVE_SECRET_HEADER_FORWARD_*prefixes at startup, parses each(entry, header)pair via the inverse of the operator's normalization, and buildsmap[backendName]*vmcp.HeaderForwardConfig. Stray env vars whose decoded entry segment doesn't match a known static backend are dropped.NewUnifiedBackendDiscovererWithStaticBackendsgains aheaderForwardByBackendparameter.discoverFromStaticConfigattaches the matching map entry toBackend.HeaderForwardby backend name.pkg/vmcp/client/header_forward.go): inserted betweenidentityPropagatingRoundTripper(outer) andauthRoundTripper(inner). Resolves plaintext + secret headers once at client-factory time viasecrets.EnvironmentProvider. Rejects restricted headers via the sharedpkg/transport/middleware.RestrictedHeadersset. Auth always wins over user-supplied headers because it runs after this tripper.BackendTargetconstruction inpkg/vmcp/health/monitor.go::performHealthChecknow carriesHeaderForward,CABundlePath, andCABundleDataso health probes hit backends with the same TLS trust and header injection as list/call traffic.HeaderSecretRefsValidatedcondition (reusingMCPRemoteProxy'sHeaderSecretNotFoundreason) flips the entry toFailedwhen a referenced Secret is missing.GenerateHeaderForwardSecretEnvVarNamenow takesownerNamerather thanproxyNameso bothMCPRemoteProxyandMCPServerEntryshare one helper.pkg/vmcp/workloads/k8s.go::mcpServerEntryToBackendis unchanged — it readsheaderForwarddirectly from the MCPServerEntry CRD at backend-construction time, so no env-var path is needed there.Low level
pkg/vmcp/types.goHeaderForwardConfig(no kubebuilder markers — runtime-only); addHeaderForwardfield onBackendandBackendTarget.pkg/vmcp/registry.goBackendToTargetcopiesHeaderForward.pkg/vmcp/health/monitor.goHeaderForward,CABundlePath,CABundleDatainto the health-checkBackendTarget.pkg/vmcp/client/header_forward.goheaderForwardRoundTripper,buildHeaderForwardTripper,resolveHeaderForward.pkg/vmcp/client/header_forward_test.gohttptest.Servertest.pkg/vmcp/client/client.gosecretsProviderfield.pkg/vmcp/cli/header_forward_env.goreadHeaderForwardFromEnvplus header-name suffix splitter.pkg/vmcp/cli/header_forward_env_test.gopkg/vmcp/cli/serve.gopkg/vmcp/aggregator/discoverer.goheaderForwardByBackendfield onbackendDiscoverer; constructor parameter; lookup indiscoverFromStaticConfig.pkg/vmcp/aggregator/discoverer_test.gonilfor new parameter at four existing call sites.cmd/thv-operator/api/v1beta1/mcpserverentry_types.goHeaderSecretRefsValidatedcondition + reasons.cmd/thv-operator/controllers/mcpserverentry_controller.govalidateHeaderForwardSecretRefswired into reconcile.cmd/thv-operator/controllers/virtualmcpserver_deployment.gobuildHeaderForwardEnvVarsForEntriesto emit plaintext env vars in sorted order alongside secret refs.cmd/thv-operator/pkg/controllerutil/externalauth.gonormalizeHeaderForEnvVar; addGenerateHeaderForwardPlaintextEnvVarName; renameproxyName→ownerNameso MCPRemoteProxy and MCPServerEntry share one helper.Type of change
Test plan
task buildpassestask testpasses for all touched packagesTestBuildHeaderForwardEnvVarsForEntries— operator-side plaintext + secret + mixed + sort-determinismTestReadHeaderForwardFromEnv— runtime-side env walk, stray-var drop, multi-underscore header splittingTestHeaderForwardRoundTripper_*,TestResolveHeaderForward_*,TestBuildHeaderForwardTripper_*, end-to-endhttptest.Servertest (cherry-picked from PR Forward MCPServerEntry headerForward to vMCP outbound requests #5013)HeaderSecretsValid/HeaderSecretNotFoundcases (cherry-picked from PR Forward MCPServerEntry headerForward to vMCP outbound requests #5013)task operator-manifests,task operator-generate,task crdref-genproduce zero diff underdeploy/charts/operator-crds/anddocs/operator/crd-api.mdpkg/vmcp/config/Special notes for reviewers
runtime.Configso future operator-resolved sidecar fields could ship via the ConfigMap without leaking into the CRD. After investigation, env vars are a better fit forheaderForwardspecifically —MCPRemoteProxyalready uses env vars for the single-backend version of the same problem, andTOOLHIVE_OTEL_HEADER_*already establishes plaintext-header-via-env in this codebase. The wrapper stays empty for a future field that genuinely benefits from YAML co-location with user-authored vMCP config.kubectl describe podoutput instead ofkubectl get configmap. Both are RBAC-gated under similar verbs. Truly sensitive values still ridevalueFrom.secretKeyRefand never enter the operator's view of the world. Plaintext header values are by definition non-secret — that's why the user chose plaintext.http.Header.Set(which canonicalizes regardless).DESIGN.mdduring development, removed before final commit since.claude/rules/pr-creation.mdkeeps planning artifacts out of the PR diff).Generated with Claude Code