mcp_json_rest_bridge: reword and restructure filter dynamic metadata statuses - #46791
mcp_json_rest_bridge: reword and restructure filter dynamic metadata statuses#46791nahurfonseca wants to merge 4 commits into
Conversation
|
/assign mkbehr Envoy Prechecks seem to be failing for an independent reason. Can you review the other files while I figure out why do_ci.sh deps is not working? |
Signed-off-by: Nahur Fonseca <nahur@google.com>
Signed-off-by: Nahur Fonseca <nahur@google.com>
Signed-off-by: Nahur Fonseca <nahur@google.com>
Signed-off-by: Nahur Fonseca <nahur@google.com>
d114fc0 to
03e1138
Compare
| mcp_operation_ = McpOperation::Unspecified; | ||
| request_body_str_ = json_rpc.dump(); | ||
| status_ = BridgeStatus::RequestPassthrough; | ||
| status_ = BridgeStatus::InternalToolsListMissingConfig; |
There was a problem hiding this comment.
I don't understand this name. We tried to do an internal tools list but we were missing config? What's an internal tools list? We're not doing a local tools/list reply, since we just checked that above. Or if "internal" means that the status is internally-generated, how does that differ from other statuses? And "missing" config sounds like an error state, but we're passing the request through, not throwing an error.
There was a problem hiding this comment.
Thanks for the feedback, Michael.
The statuses roughly follow this pattern:
- error source: request, response or internal (not caused by the request or response, but an internal condition)
- mcp method, if method specific, e.g. tools/call, initialize.
- what the condition is
So, in this case: INTERNAL => not caused by the request or response, but an internal condition
TOOLS_LIST => the method that this condition is about
MISSING_CONFIG => as you noticed, we tried to do a local reply and could not find the config.
So, this is not an error condition (or maybe it is), but it is definitively a possible code path which we want to track with a status.
Would you have a more descriptive name for the status?
| sendErrorResponse( | ||
| Http::Code::OK, BridgeStatus::RequestMethodNotSupported, | ||
| Http::Code::OK, BridgeStatus::RequestMcpMethodNotSupported, | ||
| generateErrorJsonResponse(-32601, absl::StrCat("Method ", method, " is not supported")) |
There was a problem hiding this comment.
If we're changing the status to say that we mean MCP method and not e.g. HTTP method, then let's reflect that in the error response too.
| generateErrorJsonResponse(-32602, "Invalid tool arguments").dump(), nullptr, | ||
| BridgeStatus bridge_status = BridgeStatus::RequestToolsCallMissingRequiredArg; | ||
| std::string error_msg = "Missing required argument"; | ||
| if (absl::StrContains(http_request.status().message(), "path traversal")) { |
There was a problem hiding this comment.
String comparison on the status message is a bad way to pass error information: it's inefficient and fragile. If we want to check the return value this way, then either we should be checking the status type, or buildHttpRequest should be returning some other kind of error value that we can check here.
There was a problem hiding this comment.
Thanks, Michael. I was not sure about this piece. I will think of something else. The status type is the same for all three conditions. I may have to create an intermediary type, or pass the bridge status by reference, or something like that. Any suggestions?
3 participants
Commit Message: mcp_json_rest_bridge: reword and restructure filter dynamic metadata statuses
Additional Description:
Aligns the
BridgeStatusenum and dynamic metadata string values with the observability design specification for the Edge Agent Gateway proxy log:- Group malformed MCP method statuses (missing and non-string method) into
REQUEST_MCP_METHOD_MALFORMED(
mcp_json_rest_bridge_request_mcp_method_malformed).- Group tool name missing and tool params missing into
REQUEST_TOOLS_CALL_TOOL_NAME_MISSING(
mcp_json_rest_bridge_request_tools_call_tool_name_missing).- Ungroup HTTP request transcoding failures into distinct statuses:
-
REQUEST_TOOLS_CALL_MISSING_REQUIRED_ARG(mcp_json_rest_bridge_request_tools_call_missing_required_arg)-
REQUEST_TOOLS_CALL_PATH_TRAVERSAL_REJECTED(mcp_json_rest_bridge_request_tools_call_path_traversal_rejected)-
INTERNAL_TOOLS_CALL_INVALID_HTTP_RULE(mcp_json_rest_bridge_internal_tools_call_invalid_http_rule)- Rename statuses for clarity and consistency:
-
REQUEST_NOT_POST->HTTP_REQUEST_METHOD_NOT_POST(mcp_json_rest_bridge_http_request_method_not_post)-
REQUEST_UNSUPPORTED_PROTOCOL_VERSION->REQUEST_UNSUPPORTED_MCP_VERSION(
mcp_json_rest_bridge_request_unsupported_mcp_version)-
REQUEST_METHOD_NOT_SUPPORTED->REQUEST_MCP_METHOD_NOT_SUPPORTED(mcp_json_rest_bridge_request_mcp_method_not_supported)-
REQUEST_UNKNOWN_TOOL->REQUEST_TOOLS_CALL_TOOL_NAME_UNKNOWN(mcp_json_rest_bridge_request_tools_call_tool_name_unknown)-
REQUEST_TOOL_ARGUMENTS_INVALID->REQUEST_TOOLS_CALL_ARGUMENTS_MALFORMED(
mcp_json_rest_bridge_request_tools_call_arguments_malformed)-
REQUEST_PASSTHROUGH->INTERNAL_TOOLS_LIST_MISSING_CONFIG(mcp_json_rest_bridge_internal_tools_list_missing_config)-
RESPONSE_INVALID_UTF8->RESPONSE_TOOLS_CALL_INVALID_UTF8(mcp_json_rest_bridge_response_tools_call_invalid_utf8)-
RESPONSE_BACKEND_ERROR->RESPONSE_HTTP_STATUS_ERROR(mcp_json_rest_bridge_response_http_status_error)-
RESPONSE_FAILED_TO_PARSE_JSON->RESPONSE_FAILED_TO_PARSE_JSON_RPC(
mcp_json_rest_bridge_response_failed_to_parse_json_rpc)Risk Level: Low
Testing: Added unit tests covering newly ungrouped transcoding errors; updated existing filter and integration tests under
//test/extensions/filters/http/mcp_json_rest_bridge/....Docs Changes: N/A
Release Notes: Updated
changelogs/current/new_features/mcp_transcoder__add_status_to_dynamic_metadata.Generative AI was used to assist in preparing this PR, and I fully understand the changes.