Conversation
1028307 to
88cb6e0
Compare
88cb6e0 to
a5513cd
Compare
a5513cd to
bf331f2
Compare
| session.logger.InfoContext(s.cfg.ParentContext, "Started handling stdio to SSE session", "base_url", logutils.StringerAttr(baseURL)) | ||
| defer session.logger.InfoContext(s.cfg.ParentContext, "Completed handling stdio to SSE session") |
There was a problem hiding this comment.
How come logs in this function are using the parent context instead of ctx?
There was a problem hiding this comment.
i've added a comment that input context can be canceled by connection monitor so we use parent context here to make we see the defer log.
There was a problem hiding this comment.
Why does that matter for logging though? Cancellation is not a concept that is respected by slog.Handlers. Changing the context will result in a loss of request scoped attributes. I suggest using the request context for all logs emitted.
There was a problem hiding this comment.
This also speaks to whether storing a parent context is the correct design choice, see https://go.dev/blog/context-and-structs.
There was a problem hiding this comment.
Thanks for pointing out!! i always thought the log won't happen if cantext is canceled. i see now it's mentioned in the slog handler godoc. I will revert all the parent ctx (maybe except the end session event).
| // handleStdioToSSE proxies a stdio client connection to an SSE server. | ||
| func (s *Server) handleStdioToSSE(ctx context.Context, sessionCtx *SessionCtx) error { | ||
| // Prep. | ||
| ctx, cancel := context.WithCancel(ctx) |
There was a problem hiding this comment.
Under what conditions is this context to be used instead of the parent context and vice versa?
| testCtx := setupTestContext(t, withAdminRole(t), withApp(app)) | ||
| handleDoneCh := make(chan struct{}, 1) | ||
| defer close(handleDoneCh) | ||
| go func() { |
There was a problem hiding this comment.
How is this goroutine cleaned up if HandleSession never returns?
There was a problem hiding this comment.
it uses t.Context as input. i've modified a little bit to make it safer in case go routine returns after cleanup starts.
|
@gabrielcorado PTAL 🙏 |
gabrielcorado
left a comment
There was a problem hiding this comment.
LGTM, some minor comments. I've tested with a few MCP servers and worked nicely, great work.
| } | ||
| if resp.StatusCode != http.StatusOK { | ||
| resp.Body.Close() | ||
| return nil, nil, trace.Errorf("unexpected status code: %d", resp.StatusCode) |
There was a problem hiding this comment.
What about having a more detailed error message here? The error here indicates that the path or URI is wrong or the server doesn't support SSE (for example, it only supports Streamable HTTP). What about a message like: "Unexpected status code: %d. Ensure the server URL is reachable, and is serving an MCP SSE server on the specified path.".
What do you think?
| // endpoint used for posting client requests. If successful, the transport | ||
| // reader and message writer are returned. | ||
| func ConnectSSEServer(ctx context.Context, baseURL *url.URL) (*SSEResponseReader, *SSERequestWriter, error) { | ||
| httpClient, err := defaults.HTTPClient() |
There was a problem hiding this comment.
Should we support the same configuration for HTTP transport from regular HTTP apps?
teleport/lib/srv/app/transport.go
Lines 99 to 109 in 57ed41b
# Conflicts: # api/types/app_test.go # api/types/events/events.pb.go # integration/appaccess/appaccess_test.go # lib/srv/mcp/memory.go # lib/srv/mcp/server.go # lib/srv/mcp/stdio_test.go # lib/utils/mcputils/protocol.go # lib/utils/mcputils/stdio.go
� Conflicts: � lib/srv/mcp/server.go
dd2559a to
e35a8b4
Compare
d23e50e to
407e7df
Compare
* MCP access part 12: server-side SSE support # Conflicts: # api/types/app_test.go # api/types/events/events.pb.go # integration/appaccess/appaccess_test.go # lib/srv/mcp/memory.go # lib/srv/mcp/server.go # lib/srv/mcp/stdio_test.go # lib/utils/mcputils/protocol.go # lib/utils/mcputils/stdio.go * parse uri for determining transport type * fix pointer, atomic, and parse error � Conflicts: � lib/srv/mcp/server.go * fix schema, etc * switch to golang internal mcp sse parsing * remove ParentCtx from logging * fix build and address comments
* MCP access part 12: server-side SSE support * parse uri for determining transport type * fix pointer, atomic, and parse error � Conflicts: � lib/srv/mcp/server.go * fix schema, etc * switch to golang internal mcp sse parsing * remove ParentCtx from logging * fix build and address comments
* [MCP] server-side SSE support (#56051) * MCP access part 12: server-side SSE support * parse uri for determining transport type * fix pointer, atomic, and parse error � Conflicts: � lib/srv/mcp/server.go * fix schema, etc * switch to golang internal mcp sse parsing * remove ParentCtx from logging * fix build and address comments * [mcp] refactor jwt token and app header rewrite logic (#58601) * [mcp] mcputils for streamable http (#58764) * [mcp] mcputils for streamable http * fix flaky test * use utils.ReadAtMost and fmt.Appendf * add a marshal function for event * fix spell * [mcp] update audit events for streamable HTTP transport (#59155) * [mcp] update audit events for streamable HTTP transport * nolint for unused functions for now, they will be used in next PR * [mcp] server handler for streamable HTTP transport (#59499) * [mcp] server handler for streamable hTTP transport * review comments round 1 * add comments and fix flaky test * [mcp] bump mcp-go version (#59500) * [mcp] bump mcp-go version * fix IO transport by explicit start * [mcp] add server prometheus metrics (#59773) * [mcp] add server prometheus metrics * remove TODO and nolint * use counter where possible and limit known methods * move reporting test to individual tests * nolint for "cancelled" * Fix an issue docker container launched by MCP commands are not removed sometimes (#59879) * Fix an issue docker container launched by MCP commands are not removed sometimes * switch to math/rand/v2 * add "tsh proxy mcp" command (#59968) * [refactor] client.NewMCPServerDialer (#60020) * [refactor] client.NewMCPServerDialer * TestVerifyTLSCertLeafExpiry * TestMatchResourcesByFilters * fix typo * fix lint * mcputils for streamable HTTP transport conversion (#60024) * mcputils for streamable HTTP transport conversion * remove need of context from mcptest functions * add test for notification * [mcp] "tsh mcp connect" support for streamable HTTP (#60120) * implement "tsh mcp connect" for streamable HTTP * wait for 5s just to be conservative * [mcp] Web UI and Teleport Connect adjustments for SSE and Streamable HTTP MCP servers (#60281) * [mcp] Web UI and Teleport Connect adjustments for SSE and Streamable HTTP MCP servers * review comments * [mcp] fix some edge cases for streamable HTTP (#60286) * [mcp] add JWT and rewrite headers support for SSE MCP servers (#60320) * fix go.mod to match master * fix lint and ut
related:
changelog: added SSE support for MCP access. Note that SSE transport is currently only used between Teleport agent and the remote MCP server, and SSE is not used on client side, yet.
Flow: Claude ----(stdio)---> Teleport ------(sse)----> sse server
tsh:
server log:
(notice
sse:stdoutvsstdio:stdin)audit with external

mcp_session_id: