DON'T REVIEW YET: Introduce RuntimeConfig wrapper for vMCP ConfigMap surface#5238
DON'T REVIEW YET: Introduce RuntimeConfig wrapper for vMCP ConfigMap surface#5238ChrisJBurns wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5238 +/- ##
==========================================
- Coverage 67.97% 67.96% -0.01%
==========================================
Files 612 612
Lines 62723 62724 +1
==========================================
Hits 42633 42633
+ Misses 16908 16900 -8
- Partials 3182 3191 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jhrozek
left a comment
There was a problem hiding this comment.
Two small nits on comments — both easy fixes before merge. The overall design is solid and the test coverage for the seam invariants is thorough.
| // Note: when the operator writes the vMCP ConfigMap it wraps Config in | ||
| // runtime.RuntimeConfig (see pkg/vmcp/config/runtime). Today the wrapper | ||
| // adds no extra keys, so parsing into Config directly succeeds. When a | ||
| // future operator-resolved sidecar field lands on RuntimeConfig, callers | ||
| // that need it should use runtime.Load instead, which parses into the | ||
| // wrapper. |
There was a problem hiding this comment.
Nit: two stale references from before the RuntimeConfig → Config rename — runtime.RuntimeConfig on line 37 should be runtime.Config, and runtime.Load on line 40 doesn't exist anywhere in the codebase.
| // Note: when the operator writes the vMCP ConfigMap it wraps Config in | |
| // runtime.RuntimeConfig (see pkg/vmcp/config/runtime). Today the wrapper | |
| // adds no extra keys, so parsing into Config directly succeeds. When a | |
| // future operator-resolved sidecar field lands on RuntimeConfig, callers | |
| // that need it should use runtime.Load instead, which parses into the | |
| // wrapper. | |
| // Note: when the operator writes the vMCP ConfigMap it wraps Config in | |
| // runtime.Config (see pkg/vmcp/config/runtime). Today the wrapper adds no | |
| // extra keys, so parsing into Config directly succeeds. When a future | |
| // operator-resolved sidecar field lands on runtime.Config, a new | |
| // runtime.Load function will need to be created that parses into runtime.Config | |
| // instead of vmcpconfig.Config. |
| // `,inline`) handle key collisions differently — yaml.v3 errors or has | ||
| // a different precedence than encoding/json's outer-wins rule. The | ||
| // disjoint-tag test in runtime_config_test.go pins this. | ||
| type Config struct { |
There was a problem hiding this comment.
Nit / question: when the first operator-only field lands here, YAMLLoader.Load() uses KnownFields(true) and decodes into vmcpconfig.Config, so it will reject the new YAML key and the vMCP binary will fail to start. The fix at that point is to create a runtime.Load() function and update pkg/vmcp/cli/serve.go and pkg/vmcp/cli/validate.go to use it. Is there a plan to track this? A TODO comment on the struct would make the coupling visible to whoever adds the first field here.
VirtualMCPServerSpec.Config was typed as pkg/vmcp/config.Config, so controller-gen walked the entire internal config tree into the public CRD schema. Any change to the internal on-disk/runtime config model (field, tag, validation) therefore leaked into the v1beta1 CRD. Introduce an operator-owned mirror, cmd/thv-operator/pkg/vmcpcrd, a field-for-field duplicate of pkg/vmcp/config (incl. Duration and the composite-tool validation). Retype VirtualMCPServerSpec.Config and the VirtualMCPCompositeToolDefinition embed onto the mirror, and convert mirror -> config.Config in the operator's converter via a JSON transcode (crdToRuntime), keeping the existing Kubernetes-resolution overrides. The no-leak guarantee is now structural: nothing reachable from the CRD types references pkg/vmcp/config, so internal config changes cannot reach the CRD schema. Generated CRD manifests are byte-identical. Tests: - structural parity (config <-> mirror JSON leaf-set equality) - round-trip transcode fuzz (randfill) - categorical no-leak boundary (no CRD field type in pkg/vmcp/config) Scope note: external shared types embedded in config (telemetry, audit, ratelimit, auth strategy) are not yet mirrored; that and the crd-ref-docs rendering of the mirror are tracked follow-ups. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
471ae22 to
17ef19c
Compare
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.
Summary
VirtualMCPServerSpec.Configis typed aspkg/vmcp/config.Configin v1beta1, socontroller-genwalks every field reachable fromConfiginto the public CRD schema. That blocks adding operator-resolved sidecar fields (per-backend secret-identifier maps, resolved CA bundle paths, futureBackendHeaderForwardforMCPServerEntryreferences, etc.) without churning the CRD and triggering the v1beta1 stability gate.This PR introduces
pkg/vmcp/config.RuntimeConfig: a wrapper that embedsConfiginline and is the designated home for operator-resolved fields. Today the wrapper adds nothing — marshalled YAML is byte-identical, parsed YAML is identical, andtask operator-manifestsproduces zero CRD diff. Future PRs add sidecar fields ontoRuntimeConfigwithout touching the publicConfigor v1beta1.This is a foundational refactor for #4996 / #5013 (forward
MCPServerEntry.headerForwardto vMCP outbound requests). The current #5013 implementation adds 96 lines of CRD YAML and 86 lines ofcrd-api.mdbecauseHeaderForwardwas placed onStaticBackendConfig(a Config-reachable type). Once this PR lands, #5013 can be reworked to put the field onRuntimeConfiginstead — net CRD diff drops to zero.Wiring
YAMLLoader.Loadnow returns*RuntimeConfig. Callers that only need user-facingConfigfields read them through the embed (rc.Name,rc.Group, etc.); callers that consume sidecars read them off the wrapper directly. StrictKnownFields(true)validation is preserved.cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig.gowraps*ConfiginRuntimeConfig{}before YAML marshal. Single write path, single read path.loadAndValidateConfiginpkg/vmcp/cli/serve.goandvalidate.gounwrap to*Configto keep the existing serve pipeline tight. A comment marks where to thread the wrapper through when a sidecar consumer arrives.Tests pinning the seam
pkg/vmcp/config/runtime_config_test.go:TestRuntimeConfig_MarshalsIdenticallyToConfig— byte-identity vsConfigtoday.TestRuntimeConfig_Load_RoundTrip— Load through the operator's write shape.TestRuntimeConfig_DisjointTopLevelTags— reflect-based check that catches a future field onRuntimeConfigsharing a JSON or YAML key with anyConfigfield.encoding/json(anonymous-field promotion) andyaml.v3(,inline) handle key collisions differently, so a collision would silently produce divergent serialization. This is forward-looking — today the wrapper has no extras and the test is trivially green.cmd/thv-operator/pkg/spectoconfig/runtime_config_drift_test.go:RuntimeConfigis a strict superset ofConfig.runtimeOnlyLeafJustificationswith a written rationale (today empty).cmd/thv-operator/internal/testutil/is operator-internal.Acceptance gate
git diff main -- deploy/charts/operator-crds/ docs/operator/crd-api.mdis empty after running both:The wrapper is invisible to
controller-genbecauseRuntimeConfigis not field-referenced from any v1beta1 type. The doc onRuntimeConfigcalls out the only way to break that invariant (retypingVirtualMCPServerSpec.Configfromconfig.Configtoconfig.RuntimeConfig— don't).Type of change
Test plan
task buildpasses (verified viago build ./...)task testpasses for all touched packages (pkg/vmcp/...,cmd/thv-operator/pkg/spectoconfig/...)TestRuntimeConfig_MarshalsIdenticallyToConfigTestRuntimeConfig_Load_RoundTripTestRuntimeConfig_DisjointTopLevelTagsTestRuntimeConfigSeam(operator-side drift test)task operator-manifestsandtask operator-generateproduce zero diff underdeploy/charts/operator-crds/anddocs/operator/crd-api.mdSpecial notes for reviewers
Configdirectly. The point is to establish the seam now so the next operator-resolved field doesn't have to fight v1beta1 stability.Load()signature change.YAMLLoader.Load()now returns*RuntimeConfiginstead of*Config. Most existing callers are field-access only (works transparently through embed). Two callers needed&cfg.ConfigforValidator.Validate(*Config). The CLI'sloadAndValidateConfigunwraps to*Configat its return boundary to keep the serve pipeline tight; a comment documents the migration path when a sidecar consumer is added.cmd/thv-operator/pkg/spectoconfig/runtime_config_drift_test.gobecause the drift harness lives incmd/thv-operator/internal/testutil/and is operator-internal. The TYPES it tests live inpkg/vmcp/config/. This layering is acceptable; moving the harness up to a shared location is out of scope for this PR.Implementation plan
Approved plan (AI-assisted)
Three agents reviewed the design before commit:
Load()covered, single ConfigMap write path, no checksum fixtures pinned to current YAML.,inlinesemantics, strict-decode behaviour). One refinement landed in the type doc — the only way to leakRuntimeConfigfields into the CRD is retypingVirtualMCPServerSpec.Config, now explicitly called out.Load/LoadRuntimeinto oneLoad()returning*RuntimeConfig(interface pollution; lossy view would have become a latent bug); (2) added the disjoint-tag reflect test to catch the top forward-looking hazard.🤖 Generated with Claude Code