fix: honor MCPServerRegistration targetRef namespace - #1139
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMCPServerRegistration now resolves HTTPRoutes using ChangesCross-namespace HTTPRoute lookup
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
the
review-effort/medium
|
Hey @jasonmadigan fixed a small controller bug where MCPServerRegistration.spec.targetRef.namespace was ignored during HTTPRoute lookup....added a regression test and verified with go test ./internal/controller. |
|
Thanks for this @avinxshKD, it's exactly the I'd actually filed #1199 for this same bug just before David pointed me here.., could you add A few notes while it's still draft, no rush:
And whenever your primary PR merges (and if you've nothing else queued), would be great to bring this out of draft. happy to re-review and help get it over the line 🙂 |
…/skill) addresses david-martin's review on Kuadrant#1114: - Q2: target v1.0 (v1.0.1 current); version surface isolated, body refs being migrated - (5) reject oldest-wins prefix tiebreak; recommend namespace-qualified /a2a/{namespace}/{prefix} (pending confirm) - (4) cross-namespace allowed — controller honors targetRef.namespace (Kuadrant#1139, from Kuadrant#1199) - (6) skill-level filtering is not applicable (message/send names no skill) - (8) fix tenant line: tenant carries enforcement via header (x-mcp-toolname pattern); path's edge is attachment - (3) status mirrors main — Ready = configured, drop AgentCardDiscovered/discoveredSkills - add pluggable card-store backend seam (in-memory PoC, shared store a future option) - update signed-card note for the v1.0 target + route-by-tenant/verbatim direction (open dependency flagged) Signed-off-by: Aman-Cool <aman017102007@gmail.com>
… (review #4) david-martin confirmed: assume Kuadrant#1139 lands and copy its cross-namespace targetRef handling rather than reimplementing it. Signed-off-by: Aman-Cool <aman017102007@gmail.com>
f58aa32 to
f378443
Compare
|
Hey @Aman-Cool thanks for reviewing. Updated. Added the same-name cross-namespace case and pulled the fallback into targetRefNamespace() so lookup/status/index use the same path now. Left ReferenceGrant out for this one to keep the PR focused. Pls take a look. |
maleck13
left a comment
maleck13
left a comment
There was a problem hiding this comment.
We can't allow a cross namespace reference without a reference grant. The original code enforced same namespace specifically to stop cross namespace references. Being able to create an MCPServerRegistration is not enough permission wise. You need to have permission to expose that server on the HTTPRoute you are targeting. It being in the same ns is the first check. If it is not in the same namespace it must use a reference grant just like the MCPGatewayExtension does
f378443 to
752d432
Compare
|
@maleck13 Updated. |
coderabbitai
Bot
added
the
high-risk
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/controller/mcpserverregistration_controller.go (1)
351-353: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid copying
ReferenceGrantobjects in the loop.Use index iteration and pass the slice element address.
Proposed change
- for _, rg := range refGrantList.Items { - if referenceGrantAllowsMCPServerRegistrationHTTPRoute(&rg, mcpsr) { + for i := range refGrantList.Items { + if referenceGrantAllowsMCPServerRegistrationHTTPRoute(&refGrantList.Items[i], mcpsr) { return true, nil } }As per coding guidelines, "Use
for i := rangenotfor _, v := rangeon large structs."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/mcpserverregistration_controller.go` around lines 351 - 353, The loop in the MCP server registration check is copying each ReferenceGrant object, which is unnecessary for a large struct. Update the iteration in the function that calls referenceGrantAllowsMCPServerRegistrationHTTPRoute to use index-based traversal over refGrantList.Items and pass the address of the slice element instead of the ranged value, keeping the existing return logic unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/mcpserverregistration_controller.go`:
- Around line 617-620: The deletion cleanup path in updateHTTPRouteStatus can
update a cross-namespace HTTPRoute without going through the ReferenceGrant
check because it is invoked before getTargetHTTPRoute. Move the grant validation
into updateHTTPRouteStatus itself, or ensure the finalizer cleanup path skips
cross-namespace targets unless the ReferenceGrant has already been verified,
using the targetRef namespace handling and getTargetHTTPRoute as the key
locations to adjust.
---
Nitpick comments:
In `@internal/controller/mcpserverregistration_controller.go`:
- Around line 351-353: The loop in the MCP server registration check is copying
each ReferenceGrant object, which is unnecessary for a large struct. Update the
iteration in the function that calls
referenceGrantAllowsMCPServerRegistrationHTTPRoute to use index-based traversal
over refGrantList.Items and pass the address of the slice element instead of the
ranged value, keeping the existing return logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 24102982-09da-471c-beb3-9f31bc095084
📒 Files selected for processing (2)
internal/controller/mcpserverregistration_controller.gointernal/controller/mcpserverregistration_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/mcpserverregistration_controller_test.go
752d432 to
e46cce3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/controller/mcpserverregistration_controller_test.go (2)
80-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a same-namespace (empty
TargetRef.Namespace) subtest.Good coverage for the cross-namespace grant/no-grant paths and the same-name-route collision case. Missing a subtest for the default fallback (
TargetRef.Namespace == ""), which was specifically requested in review comments as coverage fortargetRefNamespace()'s default path. If it's not already covered by an existing test earlier in this file, please add it.🧪 Suggested subtest
{ name: "falls back to registration namespace when targetRef namespace is empty", objects: []client.Object{ testHTTPRoute("target-route", "registrations"), }, wantNamespace: "registrations", },Note this would require the outer test to vary
mcpsr.Spec.TargetRef.Namespaceper case rather than hardcoding"routes".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/mcpserverregistration_controller_test.go` around lines 80 - 104, Add a same-namespace case to TestGetTargetHTTPRouteUsesTargetRefNamespace so targetRefNamespace() is covered when TargetRef.Namespace is empty. Update the existing test table and setup so mcpsr.Spec.TargetRef.Namespace can vary per case instead of being hardcoded to "routes", and add a subtest that asserts the registration namespace is used as the fallback. Keep the existing ReferenceGrant and collision coverage intact in the same test.
152-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing positive test: cleanup succeeds when a valid ReferenceGrant is present.
This test only covers the "no grant → early return" branch. It doesn't verify that
updateHTTPRouteStatusactually removes theProgrammedcondition on deletion when a validReferenceGrantexists in the target namespace — the actual "cleanup" behavior the test name references. Since this is new ReferenceGrant-gated logic, a bug in grant matching (e.g., always returning false) wouldn't be caught by the current test.🧪 Suggested additional case
// seed testMCPServerReferenceGrant(...) alongside the HTTPRoute, then assert // httpRoute.Status.Parents[0].Conditions is empty after calling updateHTTPRouteStatus // with mcpsr.DeletionTimestamp set.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/mcpserverregistration_controller_test.go` around lines 152 - 209, The test for updateHTTPRouteStatus currently only covers the no-ReferenceGrant early-return path and misses the successful cleanup case. Add a positive test alongside TestUpdateHTTPRouteStatusRequiresReferenceGrantForCrossNamespaceCleanup that seeds a valid ReferenceGrant in the target namespace, then calls MCPReconciler.updateHTTPRouteStatus with a deleting MCPServerRegistration and verifies the HTTPRoute status no longer contains the Programmed condition. Use the existing test helpers like testHTTPRoute and testMCPServerReferenceGrant to keep the setup aligned with the new grant-gated logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/controller/mcpserverregistration_controller_test.go`:
- Around line 80-104: Add a same-namespace case to
TestGetTargetHTTPRouteUsesTargetRefNamespace so targetRefNamespace() is covered
when TargetRef.Namespace is empty. Update the existing test table and setup so
mcpsr.Spec.TargetRef.Namespace can vary per case instead of being hardcoded to
"routes", and add a subtest that asserts the registration namespace is used as
the fallback. Keep the existing ReferenceGrant and collision coverage intact in
the same test.
- Around line 152-209: The test for updateHTTPRouteStatus currently only covers
the no-ReferenceGrant early-return path and misses the successful cleanup case.
Add a positive test alongside
TestUpdateHTTPRouteStatusRequiresReferenceGrantForCrossNamespaceCleanup that
seeds a valid ReferenceGrant in the target namespace, then calls
MCPReconciler.updateHTTPRouteStatus with a deleting MCPServerRegistration and
verifies the HTTPRoute status no longer contains the Programmed condition. Use
the existing test helpers like testHTTPRoute and testMCPServerReferenceGrant to
keep the setup aligned with the new grant-gated logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e6f25811-05dd-4464-898a-b0a1f9832a78
📒 Files selected for processing (2)
internal/controller/mcpserverregistration_controller.gointernal/controller/mcpserverregistration_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/mcpserverregistration_controller.go
e46cce3 to
c20b9bc
Compare
|
@avinxshKD. Agreeing with @maleck13... creating a registration shouldn't be enough to expose another namespace's server, so same-namespace as the first check and a grant otherwise is the right boundary. The updated fix looks right too; grant check before the lookup, and Two things I'd flag though: there's no |
c20b9bc to
6797135
Compare
|
@Aman-Cool Thanks for the review and agreed. Updated this to watch ReferenceGrants as well, and the no-grant path now removes the MCPServer config so revokes take effect. Also filled in the fallback/cleanup test cases. Pls take a look when get chance. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/mcpserverregistration_controller.go (1)
627-648: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAllow status cleanup without a ReferenceGrant.
Revoking the grant leaves the targetHTTPRoutewith a staleProgrammed/InUseByMCPServerRegistrationcondition becauseupdateHTTPRouteStatusreturns before removing it. Let cleanup proceed even when the grant is gone, while still requiring the grant to set the condition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/mcpserverregistration_controller.go` around lines 627 - 648, The cleanup path in updateHTTPRouteStatus is returning too early when hasValidHTTPRouteReferenceGrant is false, which prevents stale Programmed/InUseByMCPServerRegistration conditions from being removed. Update the logic around targetRefNamespace, hasValidHTTPRouteReferenceGrant, and the HTTPRoute fetch so that a missing ReferenceGrant only blocks setting the condition, but still allows the code to continue and clear existing status on the target HTTPRoute.
🧹 Nitpick comments (3)
internal/controller/mcpserverregistration_controller.go (2)
877-884: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRange large struct by index.
MCPServerRegistrationis a large struct; iterating by value copies each item. Use the index form (as done inhasValidHTTPRouteReferenceGrantat Line 361).♻️ Proposed change
- for _, mcpsr := range mcpsrList.Items { - nn := client.ObjectKeyFromObject(&mcpsr) + for i := range mcpsrList.Items { + nn := client.ObjectKeyFromObject(&mcpsrList.Items[i]) if _, ok := seen[nn]; ok { continue }As per coding guidelines: "Use
for i := rangenotfor _, v := rangeon large structs".Source: Coding guidelines
868-875: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPer-iteration List error discards already-collected requests. Returning
nilon a transient list failure for onefromentry drops enqueues gathered from earlier entries, silently skipping reconciliation. Prefercontinue.♻️ Proposed change
if err := r.List(ctx, mcpsrList, client.MatchingFields{MCPServerRegistrationReferenceGrantIndex: refGrantFromToMCPServerRegistrationIndexValue(from)}, ); err != nil { logf.FromContext(ctx).Error(err, "Failed to list MCPServerRegistrations for ReferenceGrant") - return nil + continue }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/mcpserverregistration_controller.go` around lines 868 - 875, The per-iteration list failure in the ReferenceGrant processing loop drops all previously collected requests because the code returns nil inside the ref.Spec.From loop. Update the error handling in the MCPServerRegistration controller’s ReferenceGrant path to log the failure for the current from entry and continue to the next iteration instead of exiting early. Keep the existing request collection logic intact so earlier enqueues are preserved when one r.List call fails.internal/controller/mcpserverregistration_controller_test.go (1)
120-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared scheme setup to reduce duplication.
The
runtime.NewScheme()+AddToScheme/Installtriple is repeated across four test functions. A smallnewTestScheme(t *testing.T) *runtime.Schemehelper would remove the repetition.♻️ Proposed helper
func newTestScheme(t *testing.T) *runtime.Scheme { t.Helper() scheme := runtime.NewScheme() if err := mcpv1alpha1.AddToScheme(scheme); err != nil { t.Fatalf("AddToScheme: %v", err) } if err := gatewayv1.Install(scheme); err != nil { t.Fatalf("gatewayv1.Install: %v", err) } if err := gatewayv1beta1.Install(scheme); err != nil { t.Fatalf("gatewayv1beta1.Install: %v", err) } return scheme }Also applies to: 205-208, 249-251, 288-291
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/mcpserverregistration_controller_test.go` around lines 120 - 123, The scheme setup is duplicated across multiple controller tests; extract it into a shared helper such as newTestScheme(t *testing.T) in mcpserverregistration_controller_test.go. Move the runtime.NewScheme() plus mcpv1alpha1.AddToScheme, gatewayv1.Install, and gatewayv1beta1.Install calls into that helper, use t.Helper() and fail fast on any error, then replace the repeated blocks in each test function with a single call to newTestScheme.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/controller/mcpserverregistration_controller.go`:
- Around line 627-648: The cleanup path in updateHTTPRouteStatus is returning
too early when hasValidHTTPRouteReferenceGrant is false, which prevents stale
Programmed/InUseByMCPServerRegistration conditions from being removed. Update
the logic around targetRefNamespace, hasValidHTTPRouteReferenceGrant, and the
HTTPRoute fetch so that a missing ReferenceGrant only blocks setting the
condition, but still allows the code to continue and clear existing status on
the target HTTPRoute.
---
Nitpick comments:
In `@internal/controller/mcpserverregistration_controller_test.go`:
- Around line 120-123: The scheme setup is duplicated across multiple controller
tests; extract it into a shared helper such as newTestScheme(t *testing.T) in
mcpserverregistration_controller_test.go. Move the runtime.NewScheme() plus
mcpv1alpha1.AddToScheme, gatewayv1.Install, and gatewayv1beta1.Install calls
into that helper, use t.Helper() and fail fast on any error, then replace the
repeated blocks in each test function with a single call to newTestScheme.
In `@internal/controller/mcpserverregistration_controller.go`:
- Around line 868-875: The per-iteration list failure in the ReferenceGrant
processing loop drops all previously collected requests because the code returns
nil inside the ref.Spec.From loop. Update the error handling in the
MCPServerRegistration controller’s ReferenceGrant path to log the failure for
the current from entry and continue to the next iteration instead of exiting
early. Keep the existing request collection logic intact so earlier enqueues are
preserved when one r.List call fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e962a17c-758c-44ba-8382-bbb17ea093af
📒 Files selected for processing (2)
internal/controller/mcpserverregistration_controller.gointernal/controller/mcpserverregistration_controller_test.go
6797135 to
bb5a802
Compare
|
This now covers the full cross-namespace permission path, not just the namespace lookup: ReferenceGrant gate, revoke requeue, stale config cleanup, and regression tests around each path. |
|
Suggestion: when Using the same reason here would keep status reasons consistent across controllers and let users distinguish "missing ReferenceGrant" from other NotReady causes when inspecting MCPServerRegistration status conditions. |
Signed-off-by: Avinash Kumar Deepak <avinash8655279@gmail.com>
bb5a802 to
f325bab
Compare
|
@maleck13 good call, updated this to use ConditionReasonRefGrantRequired for the missing grant path and added a status assertion for it. |
|
@Aman-Cool @jasonmadigan ptal when get chance and lmk if changes needed |
Patryk-Stefanski
left a comment
Patryk-Stefanski
left a comment
There was a problem hiding this comment.
Thanks for the fix — the core approach is right and the ReferenceGrant enforcement is the correct security model for cross-namespace references. A few things to address before this merges (see inline comments).
| @@ -41,6 +43,8 @@ const ( | |||
There was a problem hiding this comment.
Blocking: errReferenceGrantRequired falls through to return ctrl.Result{}, fmt.Errorf("reconcile failed %w", err), causing a tight requeue loop.
When a ReferenceGrant is missing, the ReferenceGrant watcher already handles re-triggering reconciliation once the grant appears. Returning an error here causes the controller to requeue immediately with exponential backoff — burning cycles for nothing, identical to any transient error.
The right behaviour is to update status (already done above) and return ctrl.Result{}, nil, trusting the watch to re-trigger. Suggested fix:
if errors.Is(err, errReferenceGrantRequired) {
statusReason = mcpv1alpha1.ConditionReasonRefGrantRequired
if removeErr := r.ConfigReaderWriter.RemoveMCPServer(ctx, mcpServerName(mcpsr)); removeErr != nil {
return ctrl.Result{}, removeErr
}
if updateErr := r.updateStatus(ctx, mcpsr, false, statusReason, err.Error()); updateErr != nil {
if apierrors.IsConflict(updateErr) {
return ctrl.Result{RequeueAfter: defaultRequeueTime}, nil
}
return ctrl.Result{}, fmt.Errorf("reconcile failed: status update failed %w", updateErr)
}
return ctrl.Result{}, nil // ReferenceGrant watch re-triggers when grant is created
}Compare with how the deletion finalizer path (line ~128) returns nil cleanly after completing its work.
| } | ||
| } | ||
|
|
||
| httpRoute := &gatewayv1.HTTPRoute{} |
There was a problem hiding this comment.
Nit: silent no-op when ReferenceGrant is revoked on a live registration should at least log.
If a ReferenceGrant is revoked while the MCPServerRegistration is not being deleted, this returns nil silently — the stale route status stays on the HTTPRoute. That's a safe default, but without a log line an operator has no way to know why route status isn't being cleaned up. Consider:
if !hasGrant && mcpsr.DeletionTimestamp == nil {
logger.Info("skipping HTTPRoute status update: ReferenceGrant missing", "targetNamespace", targetNamespace)
return nil
}| func refGrantFromToMCPServerRegistrationIndexValue(from gatewayv1beta1.ReferenceGrantFrom) string { | ||
| return fmt.Sprintf("%s/%s/%s", from.Group, from.Kind, from.Namespace) | ||
| } | ||
|
|
There was a problem hiding this comment.
Nit: the two index-key functions form a matched pair but their symmetry is non-obvious.
mcpServerRegistrationToRefGrantIndexValue produces <group>/MCPServerRegistration/<namespace> while refGrantFromToMCPServerRegistrationIndexValue produces <group>/<kind>/<namespace>. They match when the ReferenceGrant's from.Kind == "MCPServerRegistration" and from.Group == mcp.kuadrant.io/v1alpha1, which is correct — but nothing in the code makes this pairing visible to a future reader.
A short comment on either function linking them would help:
// produces a key matching refGrantFromToMCPServerRegistrationIndexValue for an MCPServerRegistration from
func mcpServerRegistrationToRefGrantIndexValue(mcpsr mcpv1alpha1.MCPServerRegistration) string {| }, | ||
| Spec: mcpv1alpha1.MCPServerRegistrationSpec{ | ||
| TargetRef: mcpv1alpha1.TargetReference{ | ||
| Name: "target-route", |
There was a problem hiding this comment.
Nit: test name is misleading.
TestUpdateHTTPRouteStatusRequiresReferenceGrantForCrossNamespaceCleanup implies that a ReferenceGrant is required for cleanup to occur, but the test actually proves the opposite: status is removed even without a grant (during deletion). The name should reflect what is actually under test, e.g. TestUpdateHTTPRouteStatusCrossNamespaceCleanupOnDeletion.
| scheme := testScheme(t) | ||
|
|
||
| mcpsr := &mcpv1alpha1.MCPServerRegistration{ | ||
| ObjectMeta: metav1.ObjectMeta{ |
There was a problem hiding this comment.
Nit: this test asserts the current (broken) error-return behaviour.
Lines 532–537 assert err != nil and that the error message contains "ReferenceGrant required". If the blocking issue above is fixed (Reconcile returns nil when the grant is missing), this assertion will need to flip to if err != nil { t.Fatal(...) }. Worth updating both together so the test documents the intended behaviour, not the current one.
5 participants
Fixes MCPServerRegistration reconciliation when
spec.targetRef.namespacepoints at an HTTPRoute outside the registration namespace.The API and docs allow this, and the field index/status update paths already account for it, but
getTargetHTTPRoutealways read the route from the MCPServerRegistration namespace.This updates route lookup to use
targetRef.namespacewhen set, with a regression test for cross-namespace refs.Fixes: #1199
Tested:
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Tests