feat(iroh-net): Implement `websocket` protocol upgrade in iroh-relay #2387

matheus23 · 2024-06-19T13:25:48Z

Implements the websocket protocol upgrade in iroh-relay and changes iroh-net clients to connect with websocket instead of iroh derp http.

State of this is:

cargo test -p iroh-net runs green
websocket on the relay is supported
backwards compatibility is tested
the client also supports connecting using websockets
- (but we're forced to use tungstenites connection establishment, no custom MaybeTlsStream.)
- It decides this via the URL scheme (ws(s):// vs. http(s)://)

TODO:

Enumify the client so it's possible to keep using the old relay protocol
Cleanup!
Some perf TODOs in the server, less copying.
Update documentation (e.g. local_addr having another case being None)
Add metrics for websocket-accept & derp-accept counts
Snapshot tests (using parse_hexdump) for "new" wire format

Description

Supports clients connecting to iroh-relay with websockets & running the derp protocol over websocket Binary msgs.
Adds support for the relay answering Upgrade: websocket headers instead of Upgrade: iroh derp http appropriately.
Adds support for ClientBuilder to dial via websockets, if the relay URL is set with a ws/wss URL scheme.

Breaking Changes

Not in the protocol: The old HTTP upgrade & protocol should still be supported. There's tests to ensure this.
Client::local_addr will now also return None, if one connected using websockets.
iroh_net::relay::http::ClientError: Added a variant WebsocketError for errors when establishing a connection using websockets.

Notes & open questions

Closes #2370

Change checklist

Self-review.
Documentation updates if relevant.
Tests if relevant.
All breaking changes documented.

iroh-net/src/relay/client.rs

iroh-blobs/src/downloader/test.rs

matheus23 · 2024-06-20T12:06:31Z

FWIW, I'm still hacking on stuff over here. There's some module visibility changes that are probably not necessary. I'll do a pass over the diff at the end - making sure it's minimal.
Just working in the open - that's why the PRs here (and thanks for the comments already - that's why I opened it :) ).

And change `iroh-net` clients to connect with `websocket` instead of `iroh derp http`. Very WIP! TODO: - Enumify the client so it's possible to keep using the old relay protocol - Cleanup! - Some perf TODOs in the server, less copying.

matheus23 · 2024-06-21T15:10:16Z

Rebased this on main & flipped this to ready for review.

Seeking Feedback

I'm still not happy with some things in this PR:

We have both Frame::Ping and tungstenite::Message::Ping, so sometimes you ping-pong on the websocket level, sometimes on the "derp" level. That's werid.
- this is fixable, but would require quite a bit of work. The Ping messages don't match exactly - and we'd need to think about how to handle this in the client's loop (how does it decide to send websocket ping messages instead of derp pings? and similar issues with close frames).
I'm using tokio_tungstenite_wasm::WebSocketStream in the client, in preparation for making this browser-ready. However, this means two things:
- we lose control over the connection handshake, i.e. we're not using our own MaybeTlsStreamReader etc., which in turn means we can't mock the WebSocketStream in tests the same way, and the HTTP proxy support doesn't work (perhaps the proxy stuff is fixable?)
- we have an annoying BiLock somewhere inside our SplitSink and SplitStream wrappers, that split the WebSocketStream into parts. Instead, it'd be nice to eventually just separate the sink and stream completely, but that means we'll need to reimplement some parts of the websocket protocol (when to respond to pings & when to create close frames), but also, this is completely irrelevant for the browser case, since there it's not fixable.

Would love feedback on these things.

Also: There's the cargo semver check failure. How do I fix that? Do I bump the version within the PR?

dignifiedquire · 2024-06-21T15:24:02Z

We have both Frame::Ping and tungstenite::Message::Ping

My understanding is that these are for different reasons, websocket pings are keep alive pings, where as the Frame::Ping is mostly used for testing/measuring

dignifiedquire · 2024-06-21T15:25:09Z

Also: There's the cargo semver check failure. How do I fix that? Do I bump the version within the PR?

please make it a separate PR

Cargo.lock

iroh-net/src/relay/codec.rs

iroh-net/src/relay/http/client.rs

iroh-net/src/relay/http/server.rs

iroh-net/src/relay/iroh_relay.rs

matheus23 · 2024-06-26T15:15:40Z

On what I wrote above about seeking feedback:

Seems like it's fine to keep both.
Yes, we want to do this, but I'll do it in a PR afterwards - bringing back some more testing & proxy support, tracked in Support HTTP proxies in clients connecting to relays over websockets #2418

And it seems like I can just let the semver check sit there on failing. I've documented the breaking changes.

divagant-martian

We have been trying to move away from the DERP naming in general, but I guess coming up with a better name than "custom relaying protocol", which quite correctly is used here to describe what it is, requires some bikeshedding

iroh-net/src/relay/client.rs

divagant-martian · 2024-06-26T17:44:09Z

iroh-net/src/relay/codec.rs

+    pub(crate) fn into_ws_message(self) -> std::io::Result<tungstenite::Message> {
+        Ok(tungstenite::Message::binary(self.into_ws_vec()))
+    }
+
+    /// Serializes this to bytes and wraps it in a websocket binary message for use in wasm.
+    pub(crate) fn into_wasm_ws_message(self) -> std::io::Result<tokio_tungstenite_wasm::Message> {
+        Ok(tokio_tungstenite_wasm::Message::binary(self.into_ws_vec()))
+    }
+
+    /// Unwraps any binary messages received via websockets and parses them into `Frame`s.
+    ///
+    /// Ignores any non-binary websocket messages with a warning.
+    pub(crate) fn from_ws_message(
+        msg: tungstenite::Result<tungstenite::Message>,
+    ) -> Option<anyhow::Result<Self>> {
+        match msg {
+            Ok(tungstenite::Message::Binary(vec)) => Some(Self::from_ws_vec(vec)),
+            Ok(msg) => {
+                tracing::warn!(?msg, "Got websocket message of unsupported type, skipping.");
+                None
+            }
+            Err(e) => Some(Err(e.into())),
+        }
+    }
+
+    /// Unwraps any binary messages received via websockets (in wasm) and parses them into `Frame`s.
+    ///
+    /// Ignores any non-binary websocket messages with a warning.
+    pub(crate) fn from_wasm_ws_message(
+        msg: tokio_tungstenite_wasm::Result<tokio_tungstenite_wasm::Message>,
+    ) -> Option<anyhow::Result<Self>> {


Why are these four not TryFrom impls?

I'm used to just not do *From traits except for errors.
I agree this is one of the cases where using TryFrom really is appropriate 👍

Looking at this again - the from_ws_message and from_wasm_ws_message functions actually return an Option<Result<...>>, so you can't implement TryFrom for them. We could write From functions, but IMO that's now getting really far away from how From and TryFrom are used usually.

Perhaps it makes sense to bikeshed the function names?

given they are fallible, maybe call them try_from_ws_message

Meh - ended up moving these helpers out of Frame and just inline their logic into the Stream and Sink implementations.
That also uncovered some weird, unnecessary ? from the into_ws_message call-site. I think the code just belongs closer to the Stream implementation, the logic was quite intertwined anyways (effectively some nested matches, this is now flattened).

Instead, it's only two new functions on Frame remaining: Frame::encode_for_ws_msg and Frame::decode_from_ws_msg.

Feels way better IMHO.

iroh-net/src/relay/codec.rs

iroh-net/src/relay/http/client.rs

iroh-net/src/relay/http/server.rs

ramfox

This looks great. Wooo!

My only note, which diva mentioned, is I don't love doubling down on the name "derp", since we have been trying to step away from calling the protocol "derp" since we've technically broken away from it multiple times now 😂

However, I think this is a problem best left for our "wire protocol" review in anticipation of our mythical "one final breaking change" plan. Gotta love that we still ping the /derp endpoint and use 'iroh derp http' in our headers 🤣

So I say merge without worrying about the derp naming, and leave that to a more general "relay protocol" follow up later.

iroh-net/src/relay/codec.rs

matheus23 changed the title ~~feat: Implement websocket protocol upgrade in iroh-relay~~ feat(iroh-net): Implement websocket protocol upgrade in iroh-relay Jun 19, 2024

dignifiedquire reviewed Jun 20, 2024

View reviewed changes

iroh-net/src/relay/client.rs Outdated Show resolved Hide resolved

dignifiedquire reviewed Jun 20, 2024

View reviewed changes

iroh-net/src/relay/client.rs Outdated Show resolved Hide resolved

matheus23 commented Jun 20, 2024

View reviewed changes

iroh-blobs/src/downloader/test.rs Show resolved Hide resolved

dignifiedquire added this to the v0.20.0 milestone Jun 20, 2024

matheus23 self-assigned this Jun 21, 2024

matheus23 marked this pull request as ready for review June 21, 2024 14:59

matheus23 added 16 commits June 21, 2024 16:59

chore: Replace path dep with git dep for fastwebsockets

06c77b3

feat: Use tokio-tungstenite-wasm for client ws connecting

72ef2e3

feat: Add back codepath for derp protocol on the client

805136f

chore: Remove unused imports & fastwebsockets lib

88af1db

chore: Prefer Pin::new over _unpin()

ddc932c

feat: Determine ws/relay protocl based on relay url scheme

3feb916

fix: Send Connection: upgrade header in ws response, reintroduce tests

b6af9b2

feat: Break the websocket wire protocol to avoid copies

5a7f33f

refactor: Move the RelayIo definition

8d02bfa

chore: Make clippy happy

97f2be2

feat: Add websocket_accepts and similar rough metrics

6314213

fix: Don't stop at websocket control messages

8803fdd

refactor: Adjust mod exposure, rename & cleanup

fdd1214

docs: Document Protocol struct

7def324

chore: Write (prop)tests for new codec.rs code

861fc74

matheus23 force-pushed the matheus23/relay-websockets branch from 4a18318 to 861fc74 Compare June 21, 2024 14:59