-
Notifications
You must be signed in to change notification settings - Fork 150
Implement ExternalAuthConfig discovery for VirtualMCPServer backends #2726
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2726 +/- ##
==========================================
- Coverage 56.55% 56.46% -0.09%
==========================================
Files 320 320
Lines 30874 31097 +223
==========================================
+ Hits 17460 17559 +99
- Misses 11911 12026 +115
- Partials 1503 1512 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
implements #2704 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
00b55a4 to
a716ccc
Compare
a716ccc to
3da9d52
Compare
jhrozek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are couple of issues that need to be resolved prior to merging
| // OutgoingAuthSourceDiscovered indicates that auth configs should be automatically discovered from MCPServers | ||
| OutgoingAuthSourceDiscovered = "discovered" | ||
| // OutgoingAuthSourceInline indicates that auth configs should be explicitly specified | ||
| OutgoingAuthSourceInline = "inline" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have to solve this in this PR but the constant appears to be unused. In general I think we should codify what should the different auth sources mean and do.
|
|
||
| // For header injection, resolve secrets from Kubernetes | ||
| if externalAuthConfig.Spec.Type == mcpv1alpha1.ExternalAuthTypeHeaderInjection { | ||
| strategy, err = converter.ResolveSecrets(ctx, externalAuthConfig, r.Client, namespace, strategy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry but I think this is wrong, I think what should be happening instead is us setting header_value_env instead and mount the secrets into the deployment as env vars. The reason is that we don't want secrets in a configmap.
I think we might want to remove the option to set the values directly from both the runtime and the CRD and rely exclusively on secrets...
| } | ||
|
|
||
| // Use the standard env var name from the converter | ||
| envVarName := "TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I read it correctly that if multiple back ends use token exchange they will clobber the single value? I think we should use some back end prefix or suffix.
|
|
||
| for _, workloadName := range workloadNames { | ||
| mcpServer := &mcpv1alpha1.MCPServer{} | ||
| if err := r.Get(ctx, types.NamespacedName{Name: workloadName, Namespace: vmcp.Namespace}, mcpServer); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would be easier to do a full List and then filter by names rather than doing N Get calls for the names. But that's an optimization for layer maybe..worth a comment with a TODO now?
|
|
||
| // Get workload names from the group | ||
| workloadDiscoverer := workloads.NewK8SDiscovererWithClient(r.Client, vmcp.Namespace) | ||
| workloadNames, err := workloadDiscoverer.ListWorkloadsInGroup(ctx, vmcp.Spec.GroupRef.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could absolutely be a follow up issue, but note that listing workloads in group at 2 different moments can give you 2 different sets (here and in ensureVmcpConfigConfigMap). I would prefer to do 1 List and pass the results around
| if err != nil { | ||
| return nil, fmt.Errorf("failed to convert default auth config: %w", err) | ||
| } | ||
| outgoing.Default = defaultStrategy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think continuing in a degraded mode is fine, but it made me realize we migth want to expose conditions for observability in vMCP. Let's file an issue (I can do it if you prefer)
| if err != nil { | ||
| ctxLogger := log.FromContext(ctx) | ||
| ctxLogger.V(1).Info("Failed to get Default ExternalAuthConfig secret, continuing without it", | ||
| "error", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about conditions, continuing in a degraded mode is fine, but the admin should know about it
Summary
This PR implements automatic discovery and resolution of
MCPExternalAuthConfigresources for backend MCPServers in the VirtualMCPServer controller. The feature enables VirtualMCPServer to automatically discover and apply external authentication configurations (e.g., OAuth2 Token Exchange, Header Injection) from referenced MCPServer resources without requiring manual configuration.Changes
Core Implementation
discoverExternalAuthConfigs: DiscoversExternalAuthConfigfrom MCPServer resources in the group and adds them to the outgoing auth configurationbuildOutgoingAuthConfig: BuildsOutgoingAuthConfigby discoveringExternalAuthConfigfrom MCPServer resources when using "discovered" source modeconvertExternalAuthConfigToStrategy: ConvertsMCPExternalAuthConfigCRD resources to internalBackendAuthStrategyformat using typed structures (from PR Refactor/typed backend auth strategy #2797), handling token exchange and header injection configurationsconvertBackendAuthConfigToVMCP: Converts inlineBackendAuthConfigfrom CRD spec toBackendAuthStrategy, supporting both direct references andExternalAuthConfigRefreferencesdiscoverBackends: Now uses the resolvedOutgoingAuthConfig(including discovered external auth configs) when creating theUnifiedBackendDiscovererensureVmcpConfigConfigMap: Ensures that fully resolvedOutgoingAuthConfig(including discovered external auths) is written to the ConfigMap consumed by VirtualMCPServer podsSecret Management
discoverExternalAuthConfigSecrets: Discovers ExternalAuthConfigs from MCPServers and returns environment variables for their client secrets (used in discovered mode)discoverInlineExternalAuthConfigSecrets: Discovers ExternalAuthConfigs referenced in inline Backends and returns environment variables for their client secretsgetExternalAuthConfigSecretEnvVar: Returns an environment variable for the client secret from an ExternalAuthConfig (for token exchange with ClientSecretRef)buildOutgoingAuthEnvVars: Builds environment variables for outgoing auth secrets and mounts them in the deploymentFeatures
Two source modes supported:
discovered: Automatically discover auth configs from all referenced MCPServersinline: Use only explicitly specified auth configs (existing behavior)Inline overrides in discovered mode: When using
discoveredmode, you can still provide inline overrides in theBackendsmap, which take precedence over discovered configsToken Exchange support: Full support for OAuth2 Token Exchange (RFC 8693) including:
Header Injection support: Support for header injection authentication with:
Testing
Unit tests: Comprehensive test coverage in
virtualmcpserver_externalauth_test.go(918 lines):TestConvertExternalAuthConfigToStrategy: Tests conversion logic with various configurations (token exchange, header injection)TestBuildOutgoingAuthConfig: Tests buildingOutgoingAuthConfigin discovered and inline modesTestConvertBackendAuthConfigToVMCP: Tests inline config conversionTestDiscoverBackendsWithExternalAuthConfigIntegration: Integration test for end-to-end discovery flowE2E tests: Existing e2e tests in
test/e2e/thv-operator/virtualmcp/cover discovered mode scenariosHow It Works
When a VirtualMCPServer is created/updated, the controller discovers all referenced MCPServers from the specified groups
For each MCPServer with an
ExternalAuthConfigRef, the controller:MCPExternalAuthConfigresourceBackendAuthStrategy(usingauthtypes.BackendAuthStrategyfrom PR Refactor/typed backend auth strategy #2797)OutgoingAuthConfigfor that backendThe resolved configuration (including discovered auth configs) is written to the vmcp ConfigMap with proper nested YAML structure (e.g.,
token_exchange: { ... })Client secrets from ExternalAuthConfigs are automatically discovered and mounted as environment variables in the VirtualMCPServer deployment
The VirtualMCPServer pod consumes the ConfigMap and uses the auth strategies when communicating with backends
Example Usage
Notes
MCPExternalAuthConfigresources by logging and skipping affected backendsBackendsmap always take precedence over discovered configurations when both are presentBackendAuthStrategystructures, ensuring proper YAML marshaling that the vmcp CLI expectsRelated Issues
Fixes the issue where
ExternalAuthConfigwas not being resolved and used by the VirtualMCPServer controller to populate backend authentication configurations.Testing
Large PR Justification
This PR implements a complete, atomic feature for automatic discovery and application of ExternalAuthConfig from MCPServers. The feature requires both discovery logic and secret management to function together—they cannot work independently. The PR includes 918 lines of comprehensive tests verifying end-to-end functionality, and integrates with PR #2797's typed BackendAuthStrategy changes. Splitting would create incomplete intermediate states that don't work, require duplicate logic, and delay integration testing. The size reflects comprehensive functionality and extensive test coverage, not code bloat.