RFC: Decouple the VirtualMCPServer CRD schema from the vMCP config model#78
Open
ChrisJBurns wants to merge 6 commits into
Open
RFC: Decouple the VirtualMCPServer CRD schema from the vMCP config model#78ChrisJBurns wants to merge 6 commits into
ChrisJBurns wants to merge 6 commits into
Conversation
Proposes decoupling the VirtualMCPServer CRD schema from the internal pkg/vmcp/config model via an operator-owned mirror type and a converter seam, delivered as an incremental, zero-diff, drift-tested migration. Phase 1 implementation is toolhive#5238. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The coupling this RFC addresses was a deliberate decision in THV-0023 (toolhive-rfcs#27), which recommended unifying CRD spec types with application config types. Add that lineage to the Summary, Problem Statement, References, and metadata, expand Alternative 2 into a balanced treatment of unified types, and state that this RFC supersedes that section's recommendation — keeping its single-source-of-truth goal while removing the API coupling via enforced-equivalence tests instead of type identity. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Incorporate review feedback (#2–#7): - #2: soften the equivalence claim — the tests catch structural and value-loss drift but NOT marker/CEL/enum/default/doc-comment drift; name that as accepted residual risk with the mirror's markers as SoT, and promote code-gen (Alt 3) to the recommended end-state that closes it. - #3: stop "superseding" a Draft — revisits THV-0023 (still Draft) and the implementation that adopted it. - #4: "categorical" is an end-state property; the no-leak boundary widens one package per PR (after 1a it bars only pkg/vmcp/config). - #5: lead the precedent with the in-repo spectoconfig converter; demote the k8s internal-vs-versioned analogy to "loosely analogous". - #6: add operator-generate (deepcopy) to the zero-diff gate; clarify it proves schema + generated-code, not converter behaviour; note order sensitivity. - #7: make RateLimitConfig a single cross-CRD cutover (MCPServer + vMCP) to avoid transient two-source divergence per-CRD tests can't catch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Thanks for the thorough review — all six points addressed in
|
PR #4923 adds an operator-resolved CA-bundle path to config.OIDCConfig and, because that type is embedded in the CRD, leaks 10 lines into the public VirtualMCPServer CRD schema for a value no user sets by hand — a live instance of the "can't add operator-resolved fields without them hitting the CRD" problem. Add it to the Problem Statement and References. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Capture the THV-0023 author's review proposal: share config component types across the CRD and internal config, differing only at the assembling structs. Present it fairly (minimal translation, less duplication, no marker gap for shared parts) with its honest tradeoff (shared leaves stay reachable from the CRD, so not categorical) and as largely compatible with this RFC, pointing at a likely hybrid end-state. Add the central open question it forces: zero coupling vs. zero unintentional coupling. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Per review feedback: config.Config is itself a public surface (the on-disk config.yaml schema and the type library consumers configure directly), not an internal implementation detail. Stop calling it internal in the title, Summary, Problem Statement, and Alternative 2; frame the coupling as welding two public contracts (CRD + library/runtime config API) into one Go type. Add library consumers as a stakeholder, note the CRD is not 1:1 with the on-disk config, and add a goal to document/version config.Config as a first-class public schema (cf. RunConfig). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Proposes decoupling the
VirtualMCPServerCRD'sspec.configfrom the internalpkg/vmcp/configmodel. Today they are the same Go type, socontroller-genwalks the internal config tree into the public CRD — meaning any internal config
change leaks into the public API. This RFC introduces an operator-owned mirror
type plus a converter seam, delivered as an incremental, provably
non-breaking migration guarded by drift tests.
Why an RFC
The full change is large and hard to review as one diff. This RFC frames the
direction — and, importantly, the two-phase, one-subtree-per-PR migration
so reviewers can see that the implementation PR is just step 1a of a deliberate,
gated sequence, not a big bang.
Highlights
enforced by a zero-diff gate on every PR).
fuzz, and a categorical no-leak boundary test.
telemetry path (
spectoconfig) is the existing in-repo template.the actual API evolution (Kubernetes-native refs, deprecating duplicate fields,
v1beta2if needed) — the only place real user-facing change happens.Implementation
Phase 1a is implemented in toolhive#5238.
🤖 Generated with Claude Code