Remove deprecated legacy typed fields from MCPRegistry CRD#4705
Remove deprecated legacy typed fields from MCPRegistry CRD#4705ChrisJBurns merged 5 commits intomainfrom
Conversation
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 #4705 +/- ##
==========================================
+ Coverage 68.59% 68.82% +0.22%
==========================================
Files 517 515 -2
Lines 54631 54115 -516
==========================================
- Hits 37474 37242 -232
+ Misses 14268 14004 -264
+ Partials 2889 2869 -20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Remove all legacy typed config fields, config generation code, and associated tests after the decoupled configYAML path was added in #4693. The configYAML field is now the only way to configure the registry server. CRD changes: - Remove Sources, Registries, DatabaseConfig, AuthConfig, TelemetryConfig fields and all 25+ associated types - Make configYAML required - Remove CEL validation rules (configYAML is required via markers) - Delete legacy helper methods (HasDatabaseConfig, GetDatabaseConfig, GetDatabasePort, BuildPGPassSecretName) Operator code removal: - Delete config/config.go legacy ConfigManager and all build* functions (~1,100 lines) - Delete config/config_test.go (~2,170 lines) - Delete pgpass.go and pgpass_test.go (~420 lines) - Delete WithRegistrySourceMounts, WithGitAuthMount, WithPGPassMount, WithRegistryStorageMount from podtemplatespec.go - Delete reconcileLegacyPath, ensurePGPassSecret from manager.go - Delete legacy buildRegistryAPIDeployment and ensureDeployment - Rename NewPath variants to be the primary functions - Simplify controller validation (remove mutual exclusivity checks) Tests and examples: - Rewrite integration test RegistryBuilder to generate configYAML - Delete all legacy unit tests - Delete 14 legacy example YAML files - Regenerate CRD manifests and API docs Closes #4704 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
d3ed1e0 to
f430124
Compare
ChrisJBurns
left a comment
There was a problem hiding this comment.
Multi-Agent Consensus Review
Agents consulted: kubernetes-expert, code-reviewer, toolhive-expert, security-advisor
Consensus Summary
| # | Finding | Consensus | Severity | Action |
|---|---|---|---|---|
| 1 | Migration documentation needed for breaking CRD change | 9/10 | MEDIUM | Document |
| 2 | GetStorageName() / image_validation.go latent bug |
7/10 | MEDIUM | Investigate |
| 3 | Loss of operator-enforced security invariants | 8/10 | MEDIUM | Document |
| 4 | configYAML in ConfigMap could contain inline credentials | 7/10 | LOW | Document |
| 5 | Test helper builds YAML via string concatenation | 7/10 | LOW | Nit |
| 6 | config package doc misleading after cleanup | 7/10 | LOW | Fix |
Finding #2 (non-inline): GetStorageName() / image_validation.go latent bug cannot be mapped to this diff because image_validation.go is not part of the changed files. The issue: image_validation.go:206 uses GetStorageName() to look up a {name}-registry-storage ConfigMap that the legacy path created. The new configYAML path creates {name}-registry-server-config instead. With the legacy path removed, image enforcement via enforceServers will silently degrade (ConfigMap not found -> returns false). This pre-dates the PR but is now the only path. Consider filing a tracking issue.
Overall
This is a clean, well-executed removal of ~10,900 lines of legacy typed-config code from the MCPRegistry CRD. The deletion is thorough — all four agents confirmed no orphaned references to removed types, functions, or constants in production code. The integration tests are properly rewritten to use the configYAML approach with explicit volume/mount construction, maintaining equivalent coverage.
The findings are documentation and follow-up items rather than code defects. The most significant architectural concern is the validation shift: moving from typed CRD fields with CEL rules and kubebuilder validation to raw configYAML means the operator no longer enforces security-relevant configuration (auth mode, SSL mode, audience) at admission time. This is an accepted tradeoff of the "operator does NOT parse, validate, or transform" design, but should be documented clearly for users. The PGPass handling is actually a security improvement — the operator no longer reads raw database credentials during reconciliation.
Generated with Claude Code
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. |
Add security note to configYAML field warning against inlining credentials — configYAML is stored in a ConfigMap, not a Secret. Recommend using file paths with mounted Secrets via volumes. Fix stale package doc on config package after legacy code removal. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3a0b3f3 to
ed3544a
Compare
Rewrite all MCPRegistry documentation to reflect the removal of legacy typed fields. The configYAML pass-through path is now the only way to configure the registry server. REGISTRY.md: Rewrite all YAML examples (ConfigMap, Git, API, OAuth, filtering, production/dev/multi-source) to use configYAML with volumes/volumeMounts. Add CRD fields reference table, config YAML structure guide, database/pgpass section, and security guidance about not inlining credentials. 06-registry-system.md: Update architecture examples from old typed source/configMapRef patterns to configYAML blocks. Explain the pass-through model. README.md: Replace legacy MCPRegistry example with configYAML version. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
77ce2cd to
4972d27
Compare
The registry server uses in-memory git clones and PostgreSQL for all storage — the emptyDir at /data was removed in the Phase 2 cleanup. The integration test still asserted its presence. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4d8b3c1 to
531b0af
Compare
Summary
Removes all legacy typed config fields from the MCPRegistry CRD after the decoupled
configYAMLpath was added in #4693. TheconfigYAMLfield is now the only way to configure the registry server, eliminating ~10,900 net lines of coupled config transformation code.This is a breaking CRD change — existing MCPRegistry resources using the legacy typed fields (
sources,registries,databaseConfig,authConfig,telemetryConfig) must be migrated toconfigYAMLbefore upgrading the CRD.Closes #4704
Large PR Justification
This PR removes the legacy typed config path from the MCPRegistry CRD and operator. The changes span CRD types, config generation, controller logic, deployment builders, PodTemplateSpec options, tests, and examples — all tightly coupled to the same legacy types. Splitting would leave the codebase in a non-compiling intermediate state since deleting a CRD type breaks every file that references it simultaneously.
Migration Guide
Users must migrate existing MCPRegistry resources before upgrading to this CRD version.
Before (legacy typed path)
After (configYAML path)
Migration steps
spec.configYAMLspec.volumesandspec.volumeMountswith explicit mount paths matching the file paths inconfigYAMLdatabaseConfig, create a pgpass-formatted Secret and reference it viaspec.pgpassSecretRef(the operator handles the init container and chmod 0600 automatically)configYAMLis stored in a ConfigMap, not a Secret. Reference credentials via file paths and mount actual secrets usingvolumes/volumeMountsspec.sources,spec.registries,spec.databaseConfig,spec.authConfig,spec.telemetryConfigSee
examples/operator/mcp-registries/mcpregistry-configyaml-*.yamlfor complete examples covering all source types.Type of change
Test plan
task test) — all passingtask lint) — 0 issuesgo build ./cmd/thv-operator/...)task operator-generate && task operator-manifests)task crdref-gen)Changes
mcpregistry_types.gozz_generated.deepcopy.goconfig/config.goconfig/config_test.gopgpass.go+pgpass_test.godeployment.gomanager.gopodtemplatespec.gotypes.gomcpregistry_controller.godocs/operator/crd-api.mdSpecial notes for reviewers
GetStorageName()is kept — still used bypkg/validation/image_validation.go. Filed Fix image_validation.go GetStorageName() for configYAML path #4717 to investigate and fix the latent bug whereenforceServersno longer works with the configYAML path.RegistryBuilderwas completely rewritten to constructconfigYAMLYAML strings instead of typed Go structsGenerated with Claude Code