gateway: pass websocket connection_init payload to subgraphs#2508
gateway: pass websocket connection_init payload to subgraphs#2508
Conversation
8dcef14 to
770462c
Compare
|
| url.set_path("/ws"); | ||
| url.set_scheme("ws").unwrap(); | ||
|
|
||
| // FIXME: we should shut down the server when the stream is dropped. |
There was a problem hiding this comment.
Haven't spent time on this, but should do before merging. Current idea would be a wrapper for the stream that contains a channel, then select on the channel in the spawned task. That way when the stream is dropped, the task exits.
crates/engine/src/websocket.rs
Outdated
| pub struct InitPayload(pub(crate) serde_json::Map<String, serde_json::Value>); | ||
|
|
||
| impl InitPayload { | ||
| pub(crate) fn to_headers(&self) -> http::HeaderMap { |
There was a problem hiding this comment.
This is best effort. Maybe trying a bit too hard, and too optimistic (ignores errors). I'm very open to better ideas.
There was a problem hiding this comment.
I just wonder how useful it is, sounds like something that would be worth dedicated rules/hook when asked for rather than trying to guess headers
There was a problem hiding this comment.
I'm happy with completely removing that part of the logic, and passing empty headers to the request context constructor.
There was a problem hiding this comment.
If that sounds best.
There was a problem hiding this comment.
Not entirely sure to be honest, but it just feels arbitrary. Think we should probably just treat them separately and provide similar looking rules/hooks in the future to better handle it. Like on_gateway_http_request & on_gateway_websocket_request for example. So would remove it yeah
770462c to
94490da
Compare
94490da to
9b7c9d1
Compare
| let (connection, _) = async_tungstenite::tokio::connect_async(request).await.unwrap(); | ||
|
|
||
| let (client, actor) = graphql_ws_client::Client::build(connection) | ||
| .payload(self.init_payload.unwrap_or_else(|| serde_json::Map::default().into()))? |
There was a problem hiding this comment.
shouldn't the default be null?
There was a problem hiding this comment.
Yeah, can be null (https://github.com/enisdenjo/graphql-ws/blob/master/PROTOCOL.md#connectioninit), will change to null when I deal with the fixmes, before merging.
The gateway implements the [graphql-transport-ws protocol](https://github.com/graphql/graphql-over-http/blob/main/rfcs/GraphQLOverWebSocket.md). In that protocol, the first message on a websocket connection will be `connection_init`. That message contains an optional `payload` field that is a JSON object with arbitrary values. Typically, information that would go in HTTP headers for queries and mutations flows instead in that `connection_init` message's payload. This is because browsers can't send custom headers with the websocket HTTP upgrade request. So headers like `Authorization` are passed there instead. Until this PR, the gateway expected a payload of the shape: ```ts type ConnectionInit = { type: 'connection_init', payload: { headers: Map<string, string> } } ``` It would then handle the contents of the `headers` key like headers, passing them through subgraph header rules, extracting JWTs from Authorization if so configured, etc. Eventually, these headers did land in the connection_init payload of requests made to subgraph, at the top level. This is limiting, because both from the perspective of the client and the subgraphs, the payload is an arbitrary JSON object. We want subscriptions that work from a given client to a given subgraph to also work from the same client to the same subgraph with the Grafbase gateway in the middle. This PR makes the gateway pass on the connection_init payload verbatim by default, with an option to disable it under: ```toml [websockets] forward_connection_init_payload = false ``` The gateway still attempts to extract headers for itself from the root of the connection_init payload and from its `headers` key if present. This PR also contains new infrastructure to test websocket subscriptions in integration-tests, since the existing websocket tests were deleted with the CLI test suite. closes GB-8244 closes GB-8262
9b7c9d1 to
189c7cd
Compare
e439795 to
d52ab1e
Compare
## Features ### Websocket connection init payload forwarding The gateway implements the [graphql-transport-ws protocol](https://github.com/graphql/graphql-over-http/blob/main/rfcs/GraphQLOverWebSocket.md). In that protocol, the first message on a websocket connection will be connection_init. That message contains an optional payload field that is a JSON object with arbitrary values. Typically, information that would go in HTTP headers for queries and mutations flows instead in that connection_init message's payload. Data like tokens that would otherwise go into the `Authorization` header are passed there instead. Before this release, the ConnectionInit payload was not forwarded to the subgraph resolving the subscription. It is now forwarded by default. That behaviour can be disabled with the following configuration: ```toml [websockets] forward_connection_init_payload = false ``` Docs are available at https://grafbase.com/docs/reference/gateway/configuration/websockets Implemented in #2508 ## Improvements ### Trusted document incremental adoption When `trusted_documents.enabled = true` and `trusted_documents.enforced = false`, the gateway fetches the trusted document whenever a trusted document id is present in the request. In 0.25.0 and 0.25.1, the gateway would return a hard error when no trusted document exists with that id, even when an inline document was in the query. This is not in the spirit of audit mode: there is the `document_id_unknown_log_level` setting to log, potentailly with error level, to detect this problem, but it should not block the request. We now allow for a soft transition: when no trusted document is found, but an inline document is present, we will use the inline document to execute the query. This is useful for organizations who want to gradually adopt trusted documents. Docs: https://grafbase.com/docs/reference/gateway/configuration/trusted-documents Implemented in #2512
The gateway implements the graphql-transport-ws protocol. In that protocol, the first message on a websocket connection will be
connection_init. That message contains an optionalpayloadfield that is a JSON object with arbitrary values. Typically, information that would go in HTTP headers for queries and mutations flows instead in thatconnection_initmessage's payload. This is because browsers can't send custom headers with the websocket HTTP upgrade request. So headers likeAuthorizationare passed there instead.Until this PR, the gateway expected a payload of the shape:
It would then handle the contents of the
headerskey like headers, passing them through subgraph header rules, extracting JWTs from Authorization if so configured, etc. Eventually, these headers did land in the connection_init payload of requests made to subgraph, at the top level.This is limiting, because both from the perspective of the client and the subgraphs, the payload is an arbitrary JSON object. We want subscriptions that work from a given client to a given subgraph to also work from the same client to the same subgraph with the Grafbase gateway in the middle.
This PR makes the gateway pass on the connection_init payload verbatim by default, with an option to disable it under:
The gateway still attempts to extract headers for itself from the root of the connection_init payload and from its
headerskey if present.This PR also contains new infrastructure to test websocket subscriptions in integration-tests, since the existing websocket tests were deleted with the CLI test suite.
closes GB-8244
closes GB-8262