Add configuration for MCP JSON Rest bridge filter#43400
Add configuration for MCP JSON Rest bridge filter#43400paulhong01 wants to merge 7 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Shen-fong (Paul) Hung <paulhung@google.com>
|
Hi @paulhong01, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
|
CC @envoyproxy/coverage-shephards: FYI only for changes made to |
Signed-off-by: Shen-fong (Paul) Hung <paulhung@google.com>
Signed-off-by: Shen-fong (Paul) Hung <paulhung@google.com>
botengyao
left a comment
There was a problem hiding this comment.
Nice, thanks for raising this PR, exciting!
/wait
api/envoy/extensions/filters/http/mcp_json_rest_bridge/v3/mcp_json_rest_bridge.proto
Show resolved
Hide resolved
api/envoy/extensions/filters/http/mcp_json_rest_bridge/v3/mcp_json_rest_bridge.proto
Outdated
Show resolved
Hide resolved
Signed-off-by: Shen-fong (Paul) Hung <paulhung@google.com>
Signed-off-by: Shen-fong (Paul) Hung <paulhung@google.com>
Signed-off-by: Shen-fong (Paul) Hung <paulhung@google.com>
Signed-off-by: Shen-fong (Paul) Hung <paulhung@google.com>
tyxia
left a comment
There was a problem hiding this comment.
Thanks for your contribution! Left a high-level comment
api/envoy/extensions/filters/http/mcp_json_rest_bridge/v3/mcp_json_rest_bridge.proto
Show resolved
Hide resolved
| oneof pattern { | ||
| // Maps to HTTP GET. | ||
| string get = 1; | ||
|
|
||
| // Maps to HTTP PUT. | ||
| string put = 2; | ||
|
|
||
| // Maps to HTTP POST. | ||
| string post = 3; | ||
|
|
||
| // Maps to HTTP DELETE. | ||
| string delete = 4; | ||
|
|
||
| // Maps to HTTP PATCH. | ||
| string patch = 5; | ||
| } |
There was a problem hiding this comment.
We don't prefer oneof. And this is more like an enum, right?
There was a problem hiding this comment.
This is a tagged union, not an enum. These fields must carry a payload: the URL Path Template.
The structure get: "/v1/foo" concisely defines both the HTTP Method (via the field ID) and the Path (via the string value) in a single entry.
There was a problem hiding this comment.
Get it. Thanks for the explanation.
Although we still could use an enum + string/path to complete same feature?
I personally think the API is fine. But our API style may inclined to no oneof and check it manually in the source code.
But if no oneof, for such many fields, it's not friendly to check them.
Cc @adisuissa WDYT?
There was a problem hiding this comment.
Yes. An enum + string path can also work. I personally think enum + string path is little bit verbose. Also, The oneof approach structurally guarantees that if a rule exists, the method is known.
That being said, I'm open to change if the reviewer/style guide has any preference. Let me know what you think. Thanks!
wbpcode
left a comment
There was a problem hiding this comment.
Thanks for this contribution. Could you add mark this API as WIP, then it's possible to change it with some feedback from actual practices?
Thanks for the review! |
Extension for supporting existing REST backends to function as MCP servers without native MCP support.
Part of #39174
Risk Level: none
Testing: n/a
Docs Changes: yes
Release Notes: n/a
Platform Specific Features: n/a