mcp: jsonrpc response fix - #1997
Conversation
Signed-off-by: Hritik003 <hritik.raj@nutanix.com>
dosubot
Bot
added
the
size:L
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1997 +/- ##
==========================================
- Coverage 84.40% 84.38% -0.02%
==========================================
Files 130 130
Lines 18104 18113 +9
==========================================
+ Hits 15280 15284 +4
- Misses 1877 1883 +6
+ Partials 947 946 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if len(msgs) == 0 { | ||
| return fmt.Errorf("no JSON-RPC messages in MCP response body") | ||
| } | ||
| for _, mmsg := range msgs { |
There was a problem hiding this comment.
Instead of duplicating this logic here, can't we just reuse the SSE logic that already exists?
Something like changing the decodeJSONRPCMessagesFromBackendBody method to something that tries to decode the full JSON body, and returns something that tells if it succeeded or if it is an SSE, then enter the SSE handling block below, instead of duplicating the logic.
I think it could be cleaner to have a tryDecodeJSON, and fallback to SSE, based on what that returns
| return []jsonrpc.Message{msg}, nil | ||
| } | ||
| firstErr := err | ||
| if len(body) >= 2 && body[0] == 0x1f && body[1] == 0x8b { |
There was a problem hiding this comment.
Does Slack return gzip? I'm wondering if this is a case we wanna handle here. There are other encryption such as br, that are also supported by our MCP server that we're not handling here...
I'm wondering what concrete cases you've found that need this very specific logic here, as I think we're starting to spread the logic about dealing with compressed payloads around, and that is going to become very hard to maintain.
Signed-off-by: Hritik003 <hritik.raj@nutanix.com>
d035a3f to
c6c96c2
Compare
Signed-off-by: Hritik003 <hritik.raj@nutanix.com>
Signed-off-by: Hritik003 <hritik.raj@nutanix.com>
nacx
left a comment
nacx
left a comment
There was a problem hiding this comment.
Overall LGTM. Just one comment left.
|
|
||
| func mcpProxyBackendName(mcpRoute *aigv1a1.MCPRoute) string { | ||
| return fmt.Sprintf("%s-%s-mcp-proxy", mcpRoute.Namespace, mcpRoute.Name) | ||
| return fmt.Sprintf("%s%s-%s-mcp-proxy", internalapi.MCPGeneratedResourceCommonPrefix, mcpRoute.Namespace, mcpRoute.Name) |
There was a problem hiding this comment.
Would this change have an impact on upgrades?
There was a problem hiding this comment.
yes on controller restart, a new proxy Backend is created, and the old one becomes an orphan. Traffic will flow through the new Backend once the HTTPRoute is updated
There was a problem hiding this comment.
Should we add a cleanup logic for the same?
There was a problem hiding this comment.
I have a question first. How is this change related to the feature this PR is about? If it is not related, I'd take taht out of this PR.
Signed-off-by: Hritik003 <hritik.raj@nutanix.com>
Signed-off-by: Hritik003 <hritik.raj@nutanix.com>
|
/retest |
3 participants
Description
the problem is that MCP backends are wildly inconsistent in how they format their responses:
Some return plain JSON (application/json Content-Type with a raw JSON-RPC body).
Some return gzip-compressed JSON without setting the Content-Encoding header properly (so the proxy can't rely on headers alone).
Some return SSE-framed data (lines like data: {...}) while incorrectly advertising Content-Type: application/json (e.g. Slack MCP).
decodeJSONRPCMessagesFromBackendBody handles all three cases with a cascading fallback strategy:
Try raw JSON decode first.
If that fails, sniff for the gzip magic bytes (0x1f 0x8b) and try decompressing.
If that also fails, check for SSE framing (data: prefix) and parse it as SSE.
Related Issues/PRs (if applicable)
Closes #1996
cc: @nacx