feat: federate MCP resources/templates (list + read) with scheme-pref…#853
Closed
aryan7081 wants to merge 2 commits intoKuadrant:mainfrom
Closed
feat: federate MCP resources/templates (list + read) with scheme-pref…#853aryan7081 wants to merge 2 commits intoKuadrant:mainfrom
aryan7081 wants to merge 2 commits intoKuadrant:mainfrom
Conversation
…ix URIs (Kuadrant#788) Signed-off-by: aryan7081 <haryan458@gmail.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe PR implements end-to-end MCP resources federation through the gateway. It adds resource discovery/listing in the upstream manager, broker-side resource aggregation with metadata filtering, router support for resource read requests with URI rewriting, CRD status tracking, and comprehensive tests across all layers. ChangesMCP Resources Federation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 35 minutes and 48 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/broker/upstream/manager.go (1)
252-255:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t return before the resource pass.
Once tools are already cached and the upstream supports
notifications/tools/list_changed, timer events exit here. That meansmanageResources(...)never runs, so servers that rely on timer-based resource refresh stop updating resources/templates entirely.Possible fix shape
- if !man.shouldFetchTools(event) { - man.logger.Debug("not fetching tools", "event", event, "upstream mcp server", man.MCP.ID(), "waiting for notification", notificationToolsListChanged) - return - } - - man.logger.Debug("fetching tools", "upstream mcp server", man.MCP.ID()) - current, fetched, err := man.getTools(ctx) - ... + if man.shouldFetchTools(event) { + man.logger.Debug("fetching tools", "upstream mcp server", man.MCP.ID()) + current, fetched, err := man.getTools(ctx) + ... + } else { + man.logger.Debug("not fetching tools", "event", event, "upstream mcp server", man.MCP.ID(), "waiting for notification", notificationToolsListChanged) + } + + resourceCount, templateCount, resourceErr := man.manageResources(ctx, event) + ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/broker/upstream/manager.go` around lines 252 - 255, The early return in the block that checks man.shouldFetchTools prevents the resource refresh pass from running; instead of returning immediately when shouldFetchTools is false, ensure manageResources still executes. Modify the branch around man.shouldFetchTools (and the debug log referencing "notificationToolsListChanged" and man.MCP.ID()) so that fetching of tools is skipped but man.manageResources(event) is always invoked (or invoked before any return), preserving the existing debug log context.
🧹 Nitpick comments (2)
docs/design/resource-federation.md (1)
133-133: ⚡ Quick winPrefer
> [!NOTE]admonition syntax.The repo uses GitHub-style admonitions (
> [!NOTE]) which the mkdocs plugin converts automatically.> **Note**:works but is inconsistent with the convention.♻️ Proposed fix
-> **Note**: mcp-go does **not** expose a public `ListResources()` on the server (matching the prompts gap noted in [prompts-federation.md](prompts-federation.md)). The manager maintains its own `resourcesMap` and `servedResourcesMap` for lookups, the same way it does for tools. +> [!NOTE] +> mcp-go does **not** expose a public `ListResources()` on the server (matching the prompts gap noted in [prompts-federation.md](prompts-federation.md)). The manager maintains its own `resourcesMap` and `servedResourcesMap` for lookups, the same way it does for tools.Based on learnings: "In this repo's GitHub-flavored docs, prefer the GitHub-style
> [!NOTE]syntax."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/resource-federation.md` at line 133, Replace the inline admonition style "Note" with the repository's preferred GitHub-style admonition; change the line that currently reads "Note: mcp-go does not expose a public `ListResources()`..." to use the GitHub-flavored admonition marker (`> [!NOTE]`) instead, keeping the same content and references to `mcp-go`, `ListResources()`, `resourcesMap`, and `servedResourcesMap` (and the link to `prompts-federation.md`) so the mkdocs plugin will render it consistently.internal/broker/filtered_resources_handler_test.go (1)
58-76: ⚡ Quick winMissing nil-normalization test for
FilterResourceTemplates.
TestFilterResources_NormalisesNilSlicecovers the nil→[]normalization for resources, but there's no equivalent for templates.FilterResourceTemplateshas the same nil-to-empty branch (perfiltered_resources_handler.go) that would silently regress without coverage.✅ Suggested test
+func TestFilterResourceTemplates_NormalisesNilSlice(t *testing.T) { + b := newTestBroker(t) + res := &mcp.ListResourceTemplatesResult{ResourceTemplates: nil} + b.FilterResourceTemplates(context.Background(), nil, &mcp.ListResourceTemplatesRequest{}, res) + assert.NotNil(t, res.ResourceTemplates, "nil slice must be replaced with empty slice so wire format is [] not null") + assert.Empty(t, res.ResourceTemplates) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/broker/filtered_resources_handler_test.go` around lines 58 - 76, Add a unit test to assert FilterResourceTemplates normalizes a nil ResourceTemplates slice to an empty slice (same pattern as TestFilterResources_NormalisesNilSlice). Create a test (e.g. TestFilterResourceTemplates_NormalisesNilSlice) that constructs a broker with newTestBroker(t), sets res := &mcp.ListResourceTemplatesResult{ResourceTemplates: nil}, calls b.FilterResourceTemplates(context.Background(), nil, &mcp.ListResourceTemplatesRequest{}, res) and then asserts that res.ResourceTemplates is non-nil and has length 0; reference the FilterResourceTemplates method and the mcp.ListResourceTemplatesResult.ResourceTemplates field to locate where to add the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/broker/upstream/manager.go`:
- Around line 382-398: The current logic in getResourceTemplates ->
templateToServerTemplate then gatewayResources.AddResourceTemplates only ever
adds templates, so removed upstream templates remain advertised; change the
publish behavior in manager.go (around man.getResourceTemplates,
man.templateToServerTemplate, and the gatewayResources.AddResourceTemplates
call) to perform a full replace: after acquiring man.resourcesLock and building
serverTemplates, compute which previously-published template IDs are no longer
present (compare man.resourceTemplates keys/IDs vs current templates), call
gatewayResources.RemoveResourceTemplates for those removed IDs, and then call
AddResourceTemplates for the current set (or use a
gatewayResources.ReplaceResourceTemplates API if available); also ensure
removeAllResources() clears published templates by invoking
RemoveResourceTemplates for all previously published IDs.
- Around line 773-776: The MCPManager.findResourceConflicts function currently
returns nil and must instead detect and reject duplicate-prefix / duplicate-URI
collisions before registration: iterate the provided candidates slice and
compare each server.ServerResource's prefix/URI against other candidates and the
manager's existing registered resources (the MCPManager's in-memory resource
registry/state), and if any duplicate prefix or URI is found return a non-nil
error describing the conflicting resource(s); ensure you perform this check
under the same lock/coordination used for mutations so the check is race-free
and return a clear error from findResourceConflicts when a conflict is detected
so resources/read rejects the registration.
In `@internal/mcp-router/resource_handlers.go`:
- Around line 37-38: Wrap the unconditional span.SetAttributes(...) calls with
an if span.IsRecording() check to avoid needless attribute serialization on
non-recording spans: keep the existing span.SetStatus(codes.Error, ...) calls
as-is but move each attribute write (span.SetAttributes(...)) inside a guard
like if span.IsRecording() { span.SetAttributes(...) }; do this for the
span.SetAttributes invocations shown (the one after span.SetStatus("no resource
uri set") and the other occurrences noted in the review) so all attribute writes
in resource_handlers.go use span.IsRecording() before calling
span.SetAttributes.
- Around line 71-84: The SSE JSON-RPC error payload currently omits the request
"id"; update the payload passed to
calculatedResponse.WithImmediateJSONRPCResponse so the JSON includes the
original request id (use mcpReq.ID or mcpReq.GetId()/GetID() as available)
alongside the error object and jsonrpc fields. Locate the call to
calculatedResponse.WithImmediateJSONRPCResponse and change the inline data
string to include `"id": <the-request-id>` (ensuring proper string
quoting/escaping) so clients can correlate the -32002 "Resource not found" error
with the original MCP request; keep the existing headers (mcpReq.GetSessionID())
and other fields unchanged.
---
Outside diff comments:
In `@internal/broker/upstream/manager.go`:
- Around line 252-255: The early return in the block that checks
man.shouldFetchTools prevents the resource refresh pass from running; instead of
returning immediately when shouldFetchTools is false, ensure manageResources
still executes. Modify the branch around man.shouldFetchTools (and the debug log
referencing "notificationToolsListChanged" and man.MCP.ID()) so that fetching of
tools is skipped but man.manageResources(event) is always invoked (or invoked
before any return), preserving the existing debug log context.
---
Nitpick comments:
In `@docs/design/resource-federation.md`:
- Line 133: Replace the inline admonition style "Note" with the repository's
preferred GitHub-style admonition; change the line that currently reads "Note:
mcp-go does not expose a public `ListResources()`..." to use the GitHub-flavored
admonition marker (`> [!NOTE]`) instead, keeping the same content and references
to `mcp-go`, `ListResources()`, `resourcesMap`, and `servedResourcesMap` (and
the link to `prompts-federation.md`) so the mkdocs plugin will render it
consistently.
In `@internal/broker/filtered_resources_handler_test.go`:
- Around line 58-76: Add a unit test to assert FilterResourceTemplates
normalizes a nil ResourceTemplates slice to an empty slice (same pattern as
TestFilterResources_NormalisesNilSlice). Create a test (e.g.
TestFilterResourceTemplates_NormalisesNilSlice) that constructs a broker with
newTestBroker(t), sets res :=
&mcp.ListResourceTemplatesResult{ResourceTemplates: nil}, calls
b.FilterResourceTemplates(context.Background(), nil,
&mcp.ListResourceTemplatesRequest{}, res) and then asserts that
res.ResourceTemplates is non-nil and has length 0; reference the
FilterResourceTemplates method and the
mcp.ListResourceTemplatesResult.ResourceTemplates field to locate where to add
the test.
🪄 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: cdfeb98d-ba43-4cbf-98f6-70cf5ded63a3
⛔ Files ignored due to path filters (2)
charts/mcp-gateway/crds/mcp.kuadrant.io_mcpserverregistrations.yamlis excluded by!charts/mcp-gateway/crds/**config/crd/mcp.kuadrant.io_mcpserverregistrations.yamlis excluded by!config/crd/mcp.kuadrant.io_*.yaml
📒 Files selected for processing (21)
api/v1alpha1/types.gobundle/manifests/mcp.kuadrant.io_mcpserverregistrations.yamldocs/design/resource-federation.mddocs/reference/mcpserverregistration.mdgo.modinternal/broker/broker.gointernal/broker/filtered_resources_handler.gointernal/broker/filtered_resources_handler_test.gointernal/broker/filtered_tools_handler_test.gointernal/broker/status_test.gointernal/broker/upstream/manager.gointernal/broker/upstream/manager_resources_test.gointernal/broker/upstream/manager_test.gointernal/broker/upstream/mcp.gointernal/controller/mcpserverregistration_controller.gointernal/mcp-router/headers.gointernal/mcp-router/request_handlers.gointernal/mcp-router/resource_handlers.gointernal/mcp-router/resource_handlers_test.gointernal/mcp-router/response_handlers_test.gotests/e2e/test_cases.md
Signed-off-by: aryan7081 <haryan458@gmail.com>
Member
|
Thanks for the work here, but resource federation is something we need to design and sequence internally — it touches CRDs and has broader implications for how the broker handles non-tool primitives. We have related work planned (#787, #788) and want to make sure the approach is cohesive. Going to close this one, but don't let it discourage you from picking up smaller scoped issues. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ref #788
Adds MCP resources and resource templates federation: upstream discovery in the broker,
<toolPrefix>+<scheme>:URI namespacing, aggregatedresources/listandresources/templates/listwith gateway-internal_metastripped, router handling forresources/read(routing + unprefixed upstream URI), andMCPServerRegistration.status.discoveredResourcesplus broker/statusresource counts. Design:docs/design/resource-federation.md.Known limitations
findResourceConflictsis a stub; duplicate-prefix registrations are not rejected for overlapping federated URIs the way tools are—routing can be nondeterministic until broker-side validation exists.AddResourceTemplatesis merge-only—stale templates are possible when upstream removes or clears templates; documented in the design doc.Clarifications I need
resources/list,resources/read, andresources/templates/listin this PR, or merge with unit tests and add E2E next?SetResourceTemplates) or mcp-go gains proper removal/replace?notifications/resources/list_changedto clients, and JWT/virtual-server resource auth remain explicit non-goals for this PR.Summary by CodeRabbit
Release Notes
resources/readrequests allow clients to read resources through the gateway.discoveredResourcesstatus field in server registrations reports discovered resource counts.