Skip to content

gateway: pass websocket connection_init payload to subgraphs#2508

Merged
tomhoule merged 2 commits intomainfrom
tomhoule-tlsmknkunvro
Jan 18, 2025
Merged

gateway: pass websocket connection_init payload to subgraphs#2508
tomhoule merged 2 commits intomainfrom
tomhoule-tlsmknkunvro

Conversation

@tomhoule
Copy link
Contributor

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 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:

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:

[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

@tomhoule tomhoule force-pushed the tomhoule-tlsmknkunvro branch from 8dcef14 to 770462c Compare January 17, 2025 19:36
@github-actions
Copy link

github-actions bot commented Jan 17, 2025

TestsPassed ✅Skipped ⏭️Failed ❌
Federation Audit Report179 ran168 passed0 skipped11 failed

url.set_path("/ws");
url.set_scheme("ws").unwrap();

// FIXME: we should shut down the server when the stream is dropped.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pub struct InitPayload(pub(crate) serde_json::Map<String, serde_json::Value>);

impl InitPayload {
pub(crate) fn to_headers(&self) -> http::HeaderMap {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is best effort. Maybe trying a bit too hard, and too optimistic (ignores errors). I'm very open to better ideas.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with completely removing that part of the logic, and passing empty headers to the request context constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that sounds best.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@tomhoule tomhoule force-pushed the tomhoule-tlsmknkunvro branch from 770462c to 94490da Compare January 17, 2025 19:44
@tomhoule tomhoule marked this pull request as ready for review January 17, 2025 19:52
@tomhoule tomhoule requested a review from a team as a code owner January 17, 2025 19:52
@tomhoule tomhoule requested a review from Finistere January 17, 2025 19:52
@tomhoule tomhoule force-pushed the tomhoule-tlsmknkunvro branch from 94490da to 9b7c9d1 Compare January 17, 2025 20:01
Copy link
Contributor

@Finistere Finistere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work

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()))?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't the default be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@tomhoule tomhoule force-pushed the tomhoule-tlsmknkunvro branch from 9b7c9d1 to 189c7cd Compare January 17, 2025 20:56
@tomhoule tomhoule force-pushed the tomhoule-tlsmknkunvro branch from e439795 to d52ab1e Compare January 18, 2025 11:53
@tomhoule tomhoule merged commit 50d9b6c into main Jan 18, 2025
16 checks passed
@tomhoule tomhoule deleted the tomhoule-tlsmknkunvro branch January 18, 2025 13:47
tomhoule added a commit that referenced this pull request Jan 20, 2025
## 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants