Add MCPServerEntry validation controller and MCPGroup integration#4664
Add MCPServerEntry validation controller and MCPGroup integration#4664
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4664 +/- ##
==========================================
- Coverage 68.66% 68.60% -0.07%
==========================================
Files 509 515 +6
Lines 52987 53518 +531
==========================================
+ Hits 36384 36715 +331
- Misses 13782 13961 +179
- Partials 2821 2842 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
94d87f1 to
a57d60d
Compare
ChrisJBurns
left a comment
There was a problem hiding this comment.
Agent-Assisted Code Review
This review was performed by three specialized agents: kubernetes-expert, code-reviewer, and security-advisor. Each finding is attributed to the agent(s) that identified it.
Summary
| Priority | Count | Description |
|---|---|---|
| Must fix | 3 | Integration test nesting bug, AllowInsecureAnnotation dead code, overly broad RBAC |
| Should fix/defer | 4 | Failed phase never used, unused constant, condition naming inconsistency, missing ConfigMap watch |
| Medium | 4 | SSRF protection, client-side filtering, all-or-nothing status clearing, error info leak |
| Minor | 1 | Test field indexer duplication |
Positive Aspects
- Clean validate-then-update-status pattern with no infrastructure side effects
- Good use of
RequeueAfter: 500msfor conflict handling (consistent with existing controllers) - Correct finalizer decision (none needed for validation-only controller)
setupGroupRefFieldIndexesextraction is a nice improvement tomain.go- MCPGroup integration faithfully follows established patterns for MCPServer and MCPRemoteProxy
- CRD YAML, Helm charts, RBAC, and API docs are consistent and complete
- Short name
mseis well-chosen
Generated with Claude Code
cmd/thv-operator/test-integration/mcp-oidc-config/suite_test.go
Outdated
Show resolved
Hide resolved
cmd/thv-operator/test-integration/mcp-remote-proxy/suite_test.go
Outdated
Show resolved
Hide resolved
6fcd0e8 to
3062bb6
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.
jhrozek
left a comment
There was a problem hiding this comment.
looks good but there are conflicts
Introduce a validation-only controller for MCPServerEntry that checks referenced resources (MCPGroup, MCPExternalAuthConfig, CA bundle ConfigMap) exist and sets status conditions accordingly. The controller never creates infrastructure or probes remote URLs. Integrate MCPServerEntry into the MCPGroup controller so groups track entry membership in Entries/EntryCount status fields, handle entry deletion notifications, and watch for entry changes. Wire up field indexing for MCPServerEntry.Spec.GroupRef and register the controller in the operator's server controller setup. Extract field indexer setup into setupGroupRefFieldIndexes to keep setupServerControllers under the cyclomatic complexity limit. Refs: #4656 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…uites The Chainsaw E2E tests assert on the exact ClusterRole contents, and the integration test suites need field indexers for all types that reference MCPGroup via spec.groupRef. Add mcpserverentries and mcpserverentries/status to the RBAC golden files for both multi-tenancy and single-tenancy Chainsaw test setups. Register MCPServerEntry field indexer in all five integration test suite BeforeSuite blocks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix RBAC: reduce mcpserverentries verbs to get;list;watch (validation-only) - Fix field indexer nesting bug in 4 integration test suites where MCPServerEntry indexer was unreachable on the happy path - Add MCPGroup Ready phase check in validateGroupRef, matching MCPServer/MCPRemoteProxy behavior - Return transient errors for requeue instead of persisting misleading NotFound conditions; sanitize error messages in status conditions - Add watches for MCPGroup and ConfigMap changes to re-validate entries - Use field indexes for auth config and CA bundle ConfigMap lookups - Use Failed phase when validations fail (Pending is initial-only) - Remove unused AllowInsecureAnnotation constant and misleading doc - Align condition type names with shared constants from mcpserver_types - Add unit tests for MCPServerEntry controller and MCPGroup entry methods - Regenerate RBAC manifests and CRD API docs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3062bb6 to
c66df7c
Compare
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
Large PR justification has been provided. Thank you!
Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
The MCPGroup controller references Entries and EntryCount on MCPGroupStatus for tracking MCPServerEntry membership, but the fields were not defined on the type. This caused compilation failures across lint, tests, vuln check, helm, and E2E CI jobs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The chainsaw E2E setup test expected mcpserverentries in the full CRUD resource list, but the generated role correctly places it in a separate read-only rule alongside virtualmcpcompositetooldefinitions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The MCPServerEntry controller needs only read access (get/list/watch) to mcpserverentries, not full CRUD. The generated role.yaml correctly groups mcpserverentries with virtualmcpcompositetooldefinitions in a read-only rule, but the chainsaw assertion files were out of sync: multi-tenancy was missing mcpserverentries entirely, and single-tenancy had it in the full-CRUD block. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
MCPServerEntry needs a controller to validate that referenced resources exist and to integrate with MCPGroup for discovery. This is the second PR in the RFC-55 implementation series, building on #4662 (CRD types).
Failedphase (notPending) when validations fail, matching the semantic distinction documented in the typesRefs #4656
Type of change
Large PR Justification
mcpserverentry_types.goand both CRD YAML files that Add MCPServerEntry CRD types and MCPGroup status fields #4662 had already merged — those appear as new additions here but are restorations, not new codeTest plan
task test) — operator unit tests pass, including 8 table-driven subtests for MCPServerEntryReconciler and MCPGroup entry integration/deletion teststask lint-fix) — 0 issuestask operator-manifestsChanges
mcpserverentry_types.gomcpserverentry_controller.gomcpgroup_controller.gomain.gosetupGroupRefFieldIndexes, add MCPServerEntry controller setupmcpserverentry_controller_test.gomcpgroup_controller_test.gotest-integration/*/suite_test.gorole.yaml, CRD YAMLs,crd-api.mdchainsaw/*/assert-rbac-clusterrole.yamlSpecial notes for reviewers
Getcalls are returned to controller-runtime for requeue rather than persisting a potentially misleading condition. Detailed errors are logged server-side only.RemoteURLis tracked separately in Add URL validation to reject internal/metadata endpoints in RemoteURL fields #4695 — the existing^https?://CEL pattern matches what MCPRemoteProxy already uses today.Generated with Claude Code