-
Notifications
You must be signed in to change notification settings - Fork 0
RFC: CRD v1beta1 Optimization and Configuration Extraction #23
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
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.
Added a bunch of comments related to the shared CRDs. I'm not sure if this proposal is not pulling in this opposite direction as @jerm-dro has been working in the vMCP config space.
| IncomingAuth *IncomingAuthConfig `json:"incomingAuth"` | ||
|
|
||
| // NOTE: THIS IS NOT ENTIRELY USED AND IS PARTIALLY DUPLICATED | ||
| Config config.Config `json:"config,omitempty"` |
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.
@jerm-dro might have the details paged in, but IIRC this was to make sure that the vMCP binary can manage the config itself? e.g. this is the desired field we're moving towards?
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.
Thanks for flagging this. I've pulled out the VirtualMCPServer Config embedding sections from this RFC - those changes will come in a follow-up PR to make sure we're aligned with THV-0014 and Jeremy's work. This RFC now focuses on the shared configuration CRDs (MCPOIDCConfig, MCPTelemetryConfig, MCPAuthzConfig) that all workload types can reference.
| ## Goals | ||
|
|
||
| - **Extract shared configuration into reusable CRDs**: Create MCPOIDCConfig, MCPTelemetryConfig, MCPAuthzConfig, and MCPAggregationToolConfig following the MCPExternalAuthConfig pattern | ||
| - **Remove VirtualMCPServer Config embedding**: Adopt CRD-first approach with runtime conversion to platform-agnostic types |
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.
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.
Yup, thanks for raising. I mentioned to Ozz I have thoughts about this, and I will be opening a PR with an alternative proposed.
Writing writing the alternative is not on the top of my list yet, I can summarize my thinking: I don't agree. I think as much as possible we should use the same config in CRDs and the servers. If there are unique-CRD spec fields that supersede or modify the config, that should be well-documented. I'll elaborate on how I've come to that conclusion in the PR.
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 RFC proposes architectural improvements to ToolHive's Kubernetes CRDs: - Extract shared config into reusable CRDs (MCPOIDCConfig, MCPTelemetryConfig, MCPAuthzConfig, MCPAggregationToolConfig) - Remove VirtualMCPServer's embedded Config field - Replace type discriminators with CEL-validated unions - Remove deprecated fields - Consolidate MCPRegistry status Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove VirtualMCPServer Config embedding sections (deferred to follow-up PR) - Split OIDC config into shareable (issuer, jwksUri, claims) vs per-server (audience, scopes) - Split Telemetry config into shareable (endpoint, settings) vs per-server (serviceName) - Add namespace to all config references - Add status size discussion to Open Questions - Clean up Go implementation details, keep architecture-focused content - Update reference types to show per-server override patterns Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
e3f65ad to
227f01e
Compare
|
LGTM now. I won't ack in case Jeremy adds his comments later so that the proposal doesn't get accidentally merged, but feel free to ping me once you need me to hit the approve button. |
Summary
This RFC proposes architectural improvements to ToolHive's Kubernetes CRDs for the v1beta1 release.
Key Changes
New Configuration CRDs:
MCPOIDCConfig- Shared OIDC authentication configMCPTelemetryConfig- Shared OpenTelemetry/Prometheus configMCPAuthzConfig- Shared Cedar authorization policiesMCPAggregationToolConfig- Per-workload tool filtering for VirtualMCPServerStructural Improvements:
Configfield (CRD-first approach)port,targetPort,toolsFilter)Quality of Life:
kubectl getoutputLocalObjectReferencetypeImplementation Phases
Related RFCs
Breaking Change
This is a breaking change from v1alpha1. Migration tooling will be provided.
🤖 Generated with Claude Code