-
Notifications
You must be signed in to change notification settings - Fork 149
fix: properly implement token exchange for discovery #2769
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
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2769 +/- ##
==========================================
- Coverage 56.48% 56.44% -0.05%
==========================================
Files 319 319
Lines 30943 31022 +79
==========================================
+ Hits 17479 17510 +31
- Misses 11960 12000 +40
- Partials 1504 1512 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull request overview
This PR fixes token exchange for discovery mode in the Virtual MCP (vmcp) system by addressing configuration format issues and adding comprehensive end-to-end testing with mock servers deployed as Kubernetes resources.
Key Changes:
- Fixes vmcp configuration YAML format conversion to use a flat
token_cache.configstructure instead of separatememory/redissections, aligning with the YAML loader's expected format - Adds discovery logic for incoming OIDC configuration from backend MCPServers, enabling vMCP to authenticate to backends that require OIDC
- Implements comprehensive E2E tests with mock HTTP, OAuth token exchange, and OIDC servers deployed as Kubernetes pods to validate authentication flows
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/thv-operator/virtualmcp/virtualmcp_auth_discovery_test.go | Replaces simple httptest mock with Kubernetes-deployed mock servers (HTTP, OAuth, OIDC) for realistic E2E testing of auth discovery and token exchange flows |
| test/e2e/thv-operator/virtualmcp/helpers.go | Adds GetPodLogs helper function to retrieve logs from specific pods for debugging test failures |
| test/e2e/oidc_mock.go | Implements proper JWKS endpoint with RSA public key export and adds client_credentials grant type support |
| pkg/vmcp/workloads/k8s.go | Adds discoverIncomingOIDCConfig method to discover and populate OIDC configuration from backend MCPServers, enabling vMCP→backend authentication |
| pkg/vmcp/types.go | Adds IncomingOIDCConfig field to Backend and BackendTarget types for storing discovered OIDC configuration |
| pkg/vmcp/registry.go | Propagates IncomingOIDCConfig field when converting Backend to BackendTarget |
| cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig.go | Implements convertToCLIFormat to transform Go config structs into vmcp CLI-compatible YAML format with flat token_cache.config structure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/e2e/thv-operator/virtualmcp/virtualmcp_auth_discovery_test.go
Outdated
Show resolved
Hide resolved
test/e2e/thv-operator/virtualmcp/virtualmcp_auth_discovery_test.go
Outdated
Show resolved
Hide resolved
test/e2e/thv-operator/virtualmcp/virtualmcp_auth_discovery_test.go
Outdated
Show resolved
Hide resolved
Large PR justification has been provided. Thank you!
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
7c3263b to
68da4c9
Compare
68da4c9 to
b3ff79c
Compare
b3ff79c to
28615db
Compare
28615db to
bfe83b2
Compare
bfe83b2 to
5c6563b
Compare
5c6563b to
03e0d32
Compare
18a756b to
d0c5bf0
Compare
when using the discovered mode on vmcp, we discovered that it was not working for several problems. Add a fix and add proper testing to validate
d0c5bf0 to
47b3af7
Compare
47b3af7 to
a27d766
Compare
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.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a27d766 to
c7327cd
Compare
c7327cd to
76eec1d
Compare
76eec1d to
7cf6767
Compare
when using the discovered mode on vmcp, we discovered that it was not working for several problems. Add a fix and add proper testing to validate
Large PR Justification
This is a complete fix of the feature, including unit tests. The fixes are atomic and need to come together to solve the issue