Add MCPServerEntry backend discovery to VirtualMCPServer#4698
Add MCPServerEntry backend discovery to VirtualMCPServer#4698ChrisJBurns merged 5 commits intomainfrom
Conversation
JAORMX
requested review from
ChrisJBurns,
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 #4698 +/- ##
==========================================
- Coverage 68.68% 68.63% -0.05%
==========================================
Files 515 516 +1
Lines 53567 54113 +546
==========================================
+ Hits 36792 37142 +350
- Misses 13932 14106 +174
- Partials 2843 2865 +22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
VirtualMCPServer needs to discover MCPServerEntry resources (zero-infrastructure catalog entries) and include them as backends. This enables vMCP to route traffic to remote MCP servers declared as MCPServerEntry without proxy pods. - Add WorkloadTypeMCPServerEntry constant and discoverer logic that lists MCPServerEntries by groupRef, converts them to vmcp.Backend using the remote URL directly from the spec (no K8s Service needed) - Extend ConfigMap generation to include entry-type backends with remoteURL, transport, auth config, and CA bundle mount paths - Mount CA bundle ConfigMaps as read-only volumes at /etc/toolhive/ca-bundles/<entry-name>/ when caBundleRef is configured - Add mcpserverentries to VirtualMCPServer RBAC rules (get/list/watch) - Add MCPServerEntry watch with optimized mapper (via MCPGroup Status.Entries) - Add CABundlePath field to StaticBackendConfig for TLS verification - Fix pre-existing missing mcpremoteproxies RBAC marker on controller - Extract metadata key constants to fix goconst lint violations Closes #4657 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Run task operator-manifests, operator-generate, and crdref-gen to pick up the new CABundlePath field on StaticBackendConfig and the updated RBAC markers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
JAORMX
force-pushed
the
worktree-issue-4657-vmcp-entry-discovery
branch
from
e94dc73 to
6a67884
Compare
github-actions
bot
added
size/XL
github-actions
bot
dismissed
their stale review
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. |
github-actions
bot
added
size/XL
ChrisJBurns
left a comment
ChrisJBurns
left a comment
There was a problem hiding this comment.
Multi-Agent Consensus Review
Agents consulted: kubernetes-expert, code-quality-reviewer, security-advisor, toolhive-expert
Consensus Summary
| # | Finding | Consensus | Severity | Action |
|---|---|---|---|---|
| 1 | CABundlePath plumbed but never consumed by vMCP runtime | 10/10 | HIGH | Document or implement |
| 2 | caBundleVolumeName truncation collision risk | 10/10 | MEDIUM | Fix |
| 3 | listMCPServerEntriesAsMap called 5x per reconciliation | 8/10 | MEDIUM | Track as tech debt |
| 4 | metadataKeyWorkloadStat truncated name | 7/10 | LOW | Fix |
| 5 | caBundleMountPath nil dereference risk | 7/10 | LOW | Fix |
| 6 | RemoteURL lacks runtime validation | 7/10 | MEDIUM | Verify CRD validation |
| 7 | buildCABundlePathMap silently swallows errors | 7/10 | MEDIUM | Fix |
Overall
This PR cleanly extends the VirtualMCPServer workload type system with MCPServerEntry as a third backend type. The implementation closely follows existing patterns for MCPServer and MCPRemoteProxy across all integration points — discovery, ConfigMap generation, RBAC, watches, and auth config. Test coverage is thorough with ~1200 lines covering edge cases, auth flows, and the new CA bundle plumbing.
The most significant concern is that the CABundlePath field is fully plumbed through the operator (volumes mounted, paths computed, config populated) but the vMCP runtime never reads it — vmcp.Backend has no CABundlePath field, and no custom TLS config is created. This means CA bundles for MCPServerEntry backends don't actually work yet. If this is intentional staged delivery, the PR description should state this explicitly and link to a follow-up issue. The caBundleVolumeName truncation collision is a concrete bug that should be fixed with a hash suffix before merge.
The remaining findings are improvements rather than blockers: a truncated constant name, a missing nil guard, and an error-swallowing pattern inconsistency.
Documentation
The CRD API docs and manifests are correctly regenerated. If CABundlePath consumption is deferred, a follow-up issue should be created and referenced.
Generated with Claude Code
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
Review comments from automated PR review. 3 findings flagged for discussion.
- Rename metadataKeyWorkloadStat to metadataKeyWorkloadStatus (F4) - Add nil guard to caBundleMountPath for defensive safety (F5) - Fix volume name truncation with hash suffix to avoid collisions and trailing hyphens in DNS labels (F2) - Add url.Parse validation for RemoteURL as defense-in-depth (F6) - Propagate errors from buildCABundlePathMap and buildCABundleVolumesForEntries instead of silently swallowing (F7) - Add explicit phase check to skip non-Valid MCPServerEntry backends - Use path.Join instead of fmt.Sprintf for path construction - Add tests for volume name truncation, collision avoidance, and phase-based filtering Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
bot
added
size/XL
ChrisJBurns
left a comment
ChrisJBurns
left a comment
There was a problem hiding this comment.
All review feedback from both reviewers has been addressed in 07d0e17:
- F2/jhrozek: Volume name truncation → hash suffix + trailing hyphen trim + 4 new tests
- F4:
metadataKeyWorkloadStat→ renamed tometadataKeyWorkloadStatus - F5:
caBundleMountPathnil guard → added, plus switched topath.Join - F6: RemoteURL runtime validation →
url.Parsedefense-in-depth added - F7/jhrozek: Error handling consistency →
buildCABundlePathMapandbuildCABundleVolumesForEntriesnow propagate errors - jhrozek: Explicit phase check → non-Valid entries skipped with new test coverage
F1 (CABundlePath not consumed by runtime) remains as intentional staged delivery — acceptable for this PR.
F3 (repeated list calls) acknowledged as tech debt deferred to follow-up.
LGTM.
testify does not provide NotHasSuffix; use assert.False with strings.HasSuffix instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
bot
added
size/XL
Rename local `url` variables to `serverURL` and `proxyURL` to avoid shadowing the `net/url` import, which revive flags as import-shadowing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
bot
added
size/XL
Main gained a CABundlePath field (after Metadata) via #4698 while this branch added one (before Metadata, alongside Type). Remove the duplicate to fix the build. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* Add vMCP MCPServerEntry static backend support Enable vMCP static mode to parse and connect to MCPServerEntry-type backends from ConfigMap configuration, routing MCP traffic directly to remote servers without proxy pods. - Add BackendType constants (container, proxy, entry) to vmcp types - Extend StaticBackendConfig with Type and CABundlePath fields - Propagate entry backend fields through discoverer to Backend - Add custom CA bundle support to HTTP client transport (per-backend) - Add config validation for entry backend type and CA bundle paths - Add unit tests for all new functionality (19 test cases) Closes #4658 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Address review feedback: harden path validation and TLS config - Strengthen CA bundle path validation with null byte check, path traversal rejection, and absolute path requirement to match existing patterns in pkg/git/client.go and pkg/server/discovery/ - Clone TLS config instead of replacing it to preserve existing settings (e.g. NextProtos for HTTP/2 ALPN) from the cloned transport - Add backward-compatibility comments to unused BackendType constants - Add test cases for null bytes and relative paths in CA bundle path Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Remove duplicate CABundlePath from StaticBackendConfig Main gained a CABundlePath field (after Metadata) via #4698 while this branch added one (before Metadata, alongside Type). Remove the duplicate to fix the build. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Regenerate CRD manifests and API docs Update VirtualMCPServer CRD schema to include the new Type field on StaticBackendConfig. Regenerated with task operator-manifests and task crdref-gen. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 participants
Summary
Closes #4657
Type of change
Test plan
task test)task lint-fix)Changes
pkg/vmcp/workloads/discoverer.goWorkloadTypeMCPServerEntryconstantpkg/vmcp/workloads/k8s.gocmd/thv-operator/controllers/virtualmcpserver_controller.golistMCPServerEntriesAsMap,mapMCPServerEntryToVirtualMCPServerwatch mapper, MCPServerEntry support in auth config discovery, RBAC markerscmd/thv-operator/controllers/virtualmcpserver_deployment.gomcpserverentriesto discovered RBAC rules, CA bundle volume helpers,buildCABundleVolumesForEntriescmd/thv-operator/controllers/virtualmcpserver_vmcpconfig.gobuildTransportMapfor entry type, addbuildCABundlePathMap, pass CA bundle paths to static configpkg/vmcp/config/config.goCABundlePathfield toStaticBackendConfigdeploy/charts/operator-crds/...docs/operator/crd-api.mdLarge PR Justification
Does this introduce a user-facing change?
VirtualMCPServer now discovers MCPServerEntry resources via groupRef and includes them as backends in the generated ConfigMap. Users can declare remote MCP server endpoints as MCPServerEntry CRDs and have them automatically routed through vMCP.
Special notes for reviewers
spec.remoteURLdirectly (unlike MCPServer/MCPRemoteProxy which usestatus.URLset after K8s Service creation), since it creates no infrastructure./etc/toolhive/ca-bundles/<entry-name>/with the key defaulting toca.crt.mcpremoteproxiesRBAC marker on the VirtualMCPServer controller.listMCPServerEntriesAsMapcalls (one per helper function) mirror the existing pattern for MCPServer/MCPRemoteProxy; a caching pass is deferred to a follow-up optimization.Generated with Claude Code