Add dynamic/static mode support to VirtualMCPServer operator#3235
Add dynamic/static mode support to VirtualMCPServer operator#3235
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3235 +/- ##
==========================================
+ Coverage 64.74% 64.85% +0.11%
==========================================
Files 370 372 +2
Lines 36076 36292 +216
==========================================
+ Hits 23356 23537 +181
- Misses 10881 10906 +25
- Partials 1839 1849 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
There was a problem hiding this comment.
Pull request overview
This PR adds support for dynamic and static operational modes to the VirtualMCPServer operator. In dynamic mode (source: discovered), the vMCP server discovers backends at runtime via Kubernetes API with RBAC permissions and a minimal ConfigMap. In static mode (source: inline), backends are pre-configured in the ConfigMap with no RBAC resources created, eliminating the need for Kubernetes API access.
Key changes:
- Added mode-based RBAC resource management (create in dynamic, skip/cleanup in static)
- Modified ConfigMap generation to produce minimal or full content based on mode
- Updated deployment to use appropriate ServiceAccount based on mode
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/thv-operator/controllers/virtualmcpserver_controller.go | Adds ensureRBACResources logic for mode-based RBAC creation/cleanup and getServiceAccountNameForVmcp helper |
| cmd/thv-operator/controllers/virtualmcpserver_deployment.go | Updates RBAC rules to include mcptoolconfigs and virtualmcpservers/status permissions, adds service account determination |
| cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig.go | Modifies ConfigMap generation to conditionally include backend details based on source mode |
| cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go | Adds unit tests for dynamic and static mode ConfigMap content |
| test/e2e/thv-operator/virtualmcp/virtualmcp_lifecycle_test.go | Adds E2E tests for dynamic mode RBAC creation, static mode RBAC absence, and mode switching behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go
Outdated
Show resolved
Hide resolved
cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_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. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml
Show resolved
Hide resolved
deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jhrozek
left a comment
There was a problem hiding this comment.
Code Review Summary
This PR implements dynamic/static mode support for VirtualMCPServer. The overall implementation is solid with good test coverage. I have a few suggestions and questions.
Key observations:
- Good: Helper function
outgoingAuthSource()centralizes mode detection - Good: Proper error handling in static mode (fails fast when backends unavailable)
- Good: Test coverage for both modes
Suggestion: Consider MCPRegistry RBAC pattern
The EnsureRBACResource helper in pkg/controllerutil/rbac.go only creates RBAC resources but never updates them. The MCPRegistry controller uses a better pattern with controllerutil.CreateOrUpdate and retry.RetryOnConflict that automatically updates RBAC rules on operator upgrades.
See cmd/thv-operator/pkg/registryapi/rbac.go for the reference implementation. Consider adopting this pattern for VirtualMCPServer RBAC in a follow-up.
Additional notes:
Nil pointer dereference risk in discoverer.go: If NewUnifiedBackendDiscovererWithStaticBackends() is called with an empty staticBackends slice, the code falls through to dynamic mode where d.groupsManager is nil. Consider adding a defensive check.
URL validation suggestion for config.go: Consider adding +kubebuilder:validation:Pattern=^https?:// to the URL field, similar to MCPRemoteProxy.RemoteURL.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Update the VirtualMCPServer operator to generate different ConfigMap content and RBAC resources based on the outgoingAuth.source field, enabling two distinct operational modes:
Dynamic mode (source: discovered):
Static mode (source: inline):
Closes: #3003
Large PR Justification
This is an atomic PR that covers a single functionality. It includes structure changes and comprehensive testing. Cannot be split.