Enable multi-upstream for MCPServer, MCPRemoteProxy, and proxy runner#4322
Open
Enable multi-upstream for MCPServer, MCPRemoteProxy, and proxy runner#4322
Conversation
Move multi-upstream restrictions from the authserver library to consumer layers. The library now accepts multi-upstream configs but enforces name semantics: single-upstream defaults empty names to "default", while multi-upstream requires explicit non-"default" names with distinct error messages for empty vs reserved names. Validate upstream names against a DNS-label regex (no leading/trailing hyphens, lowercase alphanumeric only) to prevent delimiter injection in storage keys. Add test coverage for invalid name formats (uppercase, underscores, leading/trailing hyphens). Remove the GetUpstream() convenience method (no callers remain after Phase 2). Add a cardinality guard in the proxy runner's Run() that rejects len(Upstreams) > 1 with an actionable error pointing to VirtualMCPServer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove the len > 1 guard from MCPExternalAuthConfig.validateEmbeddedAuthServer() so the CRD accepts multi-upstream configs. Add multi-upstream rejection to MCPServer and MCPRemoteProxy controllers in handleExternalAuthConfig(), setting a ConditionFalse status with reason MultiUpstreamNotSupported and an actionable error directing users to VirtualMCPServer. Add duplicate upstream name validation in the CRD webhook so conflicts are caught at admission time rather than Pod startup. Tighten the Name field pattern to disallow trailing hyphens and add MaxLength=63 for RFC 1123 compliance. VirtualMCPServer remains unrestricted as the intended multi-upstream consumer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The embedded auth server has supported sequential multi-upstream chains since the Phase 2 work, but every consumer layer (MCPServer controller, MCPRemoteProxy controller, proxy runner) blocked configs with more than one upstream provider. This commit lifts all those restrictions and fixes the two bugs that would have broken multi-upstream even without the guards. Guard removal: - MCPServer controller: remove len > 1 check in handleExternalAuthConfig - MCPRemoteProxy controller: same removal - Proxy runner Run(): remove len > 1 early-return - Remove associated condition type/reason constants and rejection tests Converter fix (authserver.go): - GenerateAuthServerEnvVars now iterates all providers and emits name-keyed env vars (TOOLHIVE_UPSTREAM_CLIENT_SECRET_OKTA, _GITHUB, …) derived from each provider's Name field, instead of only reading UpstreamProviders[0] into a single unindexed var. Name-keyed bindings are position-independent, so reordering providers in the CRD does not change the secret-to-provider mapping. - buildEmbeddedAuthServerRunnerConfig now builds Upstreams from all providers instead of only the first; buildUpstreamRunConfig gains an envVarName parameter to match the env var naming Middleware fix (middleware.go): - Restore ProviderName auto-derivation in addUpstreamSwapMiddleware, defaulting to the first upstream's name for single-upstream configs. Multi-upstream ProviderName selection will be addressed when a CRD field is added in a follow-up. Observability (upstreamswap/middleware.go): - Add provider field to all upstreamswap log lines (injection success, empty-token warning, get-tokens error) to make confused-deputy debugging tractable when multiple providers are in play. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jhrozek
requested review from
ChrisJBurns,
JAORMX,
jerm-dro and
yrobla
as code owners
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4322 +/- ##
==========================================
- Coverage 68.61% 68.45% -0.16%
==========================================
Files 478 478
Lines 48450 48491 +41
==========================================
- Hits 33243 33196 -47
- Misses 12367 12394 +27
- Partials 2840 2901 +61 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
The embedded auth server has supported sequential multi-upstream authorization chains since Phase 2, but every consumer layer (MCPServer controller, MCPRemoteProxy controller, proxy runner) blocked configs with more than one upstream provider. This PR lifts those restrictions and fixes the env var naming scheme to support multiple providers correctly.
TOOLHIVE_UPSTREAM_CLIENT_SECRETto name-keyedTOOLHIVE_UPSTREAM_CLIENT_SECRET_<PROVIDER>, making bindings position-independent across provider reorderingFixes #4138
Type of change
Test plan
task test)task lint-fix)Changes
cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_types.gocmd/thv-operator/pkg/controllerutil/authserver.goupstreamSecretBinding, multi-provider iterationpkg/authserver/config.goGetUpstream()pkg/auth/upstreamswap/middleware.godeploy/charts/operator-crds/docs/operator/crd-api.mdSpecial notes for reviewers
upstreamSecretBindingstruct is the single source of truth for env var naming, shared betweenGenerateAuthServerEnvVars(Pod env) andbuildUpstreamRunConfig(runtime config) to prevent desync.Name(uppercased, hyphens→underscores). Since the CRD regex forbids underscores, the transform is injective — no two valid names can produce the same suffix.Upstreams[0]for now; explicit per-backendProviderNameselection is addressed in the follow-up branch (authserver-multi-upstream/4-tokens-in-identity).Generated with Claude Code