Remove enforceServers and image validation from MCPRegistry#4776
Remove enforceServers and image validation from MCPRegistry#4776ChrisJBurns merged 4 commits intomainfrom
Conversation
The enforceServers feature and its image validation code have been broken since PR #2568 removed the data sources from the operator. The {name}-registry-storage ConfigMap that image validation reads from is never created — nothing populates it. As a result, enforceServers has silently done nothing: the ConfigMap lookup returns NotFound and checkImageInRegistry returns false for every check. Remove the entire feature rather than attempting to fix it: - Remove EnforceServers field from MCPRegistrySpec CRD - Remove GetStorageName() helper (dead since sources removal) - Delete image_validation.go and its tests - Remove image validation call sites from MCPServer and EmbeddingServer controllers - Remove ConditionImageValidated and related status constants - Remove getConfigMapName from registryapi - Delete the enforcing example - Regenerate CRD manifests Fixes #4717 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
24b4e80 to
6c51146
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4776 +/- ##
==========================================
+ Coverage 68.90% 68.93% +0.03%
==========================================
Files 518 517 -1
Lines 54921 54741 -180
==========================================
- Hits 37843 37736 -107
+ Misses 14172 14099 -73
Partials 2906 2906 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟣
docs/arch/06-registry-system.md:655-664— This PR correctly removes the stale storage_manager.go reference from docs/arch/06-registry-system.md, but the companion file docs/arch/09-operator-architecture.md (not touched by this PR) still contains at least two references to the deleted file cmd/thv-operator/pkg/sources/storage_manager.go (lines 556 and 566). Since this PR explicitly addressed the same type of stale reference in a neighboring doc, it would be good to clean up 09-operator-architecture.md in the same pass.Extended reasoning...
What the bug is and how it manifests
docs/arch/09-operator-architecture.md contains multiple references to cmd/thv-operator/pkg/sources/storage_manager.go, a file that was deleted in PR #2568. Specifically, verifiers found at least four stale references in that file: a diagram reference around line 531, a bold link around line 556, a section header 'Storage Manager' around line 562, and an implementation reference around line 566. The file does not exist anywhere in the repository.
The specific code path that triggers it
This PR correctly removed the storage section from docs/arch/06-registry-system.md, which previously contained 'Implementation: cmd/thv-operator/pkg/sources/storage_manager.go'. The same cleanup was not applied to docs/arch/09-operator-architecture.md, which references the same deleted path.
Why existing code does not prevent it
There is no automated check that validates documentation references against actual file paths in the repository. The stale references survived PR #2568 (which deleted the file) and continued to survive until now because neither that PR nor this one updated 09-operator-architecture.md.
What the impact would be
Readers following the architecture documentation will find references to a file that does not exist, which is misleading and undermines the accuracy of the documentation. This is especially confusing given that 06-registry-system.md was explicitly cleaned up in this PR.
How to fix it
Remove or update the references to cmd/thv-operator/pkg/sources/storage_manager.go in docs/arch/09-operator-architecture.md, replacing the 'Storage Manager' section and diagram node with accurate descriptions of how storage is now handled (i.e., the registry server fetches and stores data at runtime, as described in the updated 06-registry-system.md).
Step-by-step proof
- PR #2568 deleted cmd/thv-operator/pkg/sources/storage_manager.go.
- Neither PR #2568 nor any subsequent PR updated docs/arch/09-operator-architecture.md.
- This PR updates docs/arch/06-registry-system.md to remove 'Implementation: cmd/thv-operator/pkg/sources/storage_manager.go' (visible in the diff).
- docs/arch/09-operator-architecture.md still contains references to the same deleted file path at multiple lines (confirmed by verifiers via Glob search showing the file does not exist on disk).
- A reader consulting the operator architecture doc after this PR would still find broken references that contradict the now-accurate registry system doc.
Update REGISTRY.md, architecture docs, and regenerate CRD API docs to remove all references to the removed enforceServers feature and the stale registry-storage ConfigMap. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
6c51146 to
e06ad30
Compare
- Remove stale image validation troubleshooting from REGISTRY.md - Remove stale image validation log reference from REGISTRY.md Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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. |
- Remove "Provides image validation" from README.md feature list - Replace fictional syncStatus/apiStatus status fields in REGISTRY.md with actual MCPRegistryStatus fields (phase, message, url, readyReplicas, conditions) - Fix stale jsonpath examples in troubleshooting section Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
The
enforceServersfeature has been broken since PR #2568 removed data sources from the operator. The{name}-registry-storageConfigMap that image validation reads from is never created — theStorageManagerthat populated it was deleted. As a result,enforceServers: truesilently does nothing. This PR removes the entire feature rather than attempting to fix it.Fixes #4717
Medium level
EnforceServersfield fromMCPRegistrySpecand theGetStorageName()helperimage_validation.go, its tests, and theImageValidatorinterface)ConditionImageValidatedand related status condition constantsmain.go(removedimageValidationvariable and unusedenableRegistryparameter)getConfigMapNamedead code from registryapiLow level
cmd/thv-operator/api/v1alpha1/mcpregistry_types.goEnforceServersfield andGetStorageName()cmd/thv-operator/api/v1alpha1/mcpserver_types.goConditionImageValidatedandConditionReasonImageValidation*constantscmd/thv-operator/api/v1alpha1/embeddingserver_types.gocmd/thv-operator/controllers/mcpserver_controller.gocmd/thv-operator/controllers/embeddingserver_controller.goImageValidationfield,validateImagemethod, call sitecmd/thv-operator/controllers/embeddingserver_controller_test.goTestValidateImageandImageValidationfieldscmd/thv-operator/controllers/mcpserver_test_helpers_test.goImageValidationfieldcmd/thv-operator/main.goimageValidation,enableRegistryparam, validation importcmd/thv-operator/pkg/validation/image_validation.gocmd/thv-operator/pkg/validation/image_validation_test.gocmd/thv-operator/pkg/validation/cedar_validation.gocmd/thv-operator/pkg/registryapi/manager.gogetConfigMapNamecmd/thv-operator/pkg/registryapi/types_test.gocmd/thv-operator/test-integration/embedding-server/suite_test.goImageValidationfield and importdeploy/charts/operator-crds/*/toolhive.stacklok.dev_mcpregistries.yamlexamples/operator/mcp-registries/mcpregistry-enforcing.yamlcmd/thv-operator/REGISTRY.mddocs/arch/06-registry-system.mddocs/operator/crd-api.mdType of change
Test plan
task lint-fix) — 0 issuesgo build ./cmd/thv-operator/...) — passesDoes this introduce a user-facing change?
The
enforceServersfield is removed from the MCPRegistry CRD. This is a no-op in practice — the feature has been silently broken since #2568, so no existing deployment depends on it.Special notes for reviewers
The regenerated
docs/operator/crd-api.mdadds ~470 lines of type documentation unrelated toenforceServers. This is expected behavior fromcrd-ref-docs— it picked up types that were previously missing from the generated output. The diff is noisy but mechanical.Large PR Justification
Generated with Claude Code