feat(iroh-net): Implement websocket protocol upgrade in iroh-relay
#2387
Conversation
matheus23
changed the title
websocket protocol upgrade in iroh-relaywebsocket protocol upgrade in iroh-relay
|
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. |
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
force-pushed
the
matheus23/relay-websockets
branch
from
4a18318 to
861fc74
Compare
|
Rebased this on main & flipped this to ready for review. Seeking FeedbackI'm still not happy with some things in this PR:
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? |
My understanding is that these are for different reasons, websocket pings are keep alive pings, where as the |
please make it a separate PR |
|
On what I wrote above about seeking feedback:
And it seems like I can just let the semver check sit there on failing. I've documented the breaking changes. |
iroh-net/src/relay/codec.rs
Outdated
| 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>> { |
There was a problem hiding this comment.
Why are these four not TryFrom impls?
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
given they are fallible, maybe call them try_from_ws_message
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
… `/derp` route to `/relay` (#2419) ## Description - Removes `relay_endpoint` config option from `ServerBuilder` - Makes `iroh-relay` accept both `/derp` and `/relay` as paths for relaying - Makes `/relay` the default path used in the iroh-net client TODO: - [x] Test both URLs ## Breaking Changes - `iroh_net::relay::http::ServerBuilder`: Removed `relay_endpoint` function ## Notes & open questions Closes #2378 ✅ Merge this after #2387 ## Change checklist - [X] Self-review. - [X] Documentation updates if relevant. - [X] Tests if relevant. - [X] All breaking changes documented.
…2387) 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: - [x] Enumify the client so it's possible to keep using the old relay protocol - [x] Cleanup! - [x] Some perf TODOs in the server, less copying. - [x] Update documentation (e.g. `local_addr` having another case being `None`) - [x] Add metrics for websocket-accept & derp-accept counts - [x] 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 - [x] Self-review. - [x] Documentation updates if relevant. - [x] Tests if relevant. - [x] All breaking changes documented.
Implements the
websocketprotocol upgrade in iroh-relay and changesiroh-netclients to connect withwebsocketinstead ofiroh derp http.State of this is:
cargo test -p iroh-netruns greenMaybeTlsStream.)ws(s)://vs.http(s)://)TODO:
local_addrhaving another case beingNone)parse_hexdump) for "new" wire formatDescription
Binarymsgs.Upgrade: websocketheaders instead ofUpgrade: iroh derp httpappropriately.ClientBuilderto dial via websockets, if the relay URL is set with aws/wssURL scheme.Breaking Changes
Client::local_addrwill now also returnNone, if one connected using websockets.iroh_net::relay::http::ClientError: Added a variantWebsocketErrorfor errors when establishing a connection using websockets.Notes & open questions
Closes #2370
Change checklist