refactor(iroh-net)!: Rework relay-server binary, more configurable, reverse-proxy support #2341

flub · 2024-06-04T07:48:32Z

Description

This re-architects the relay-server binary. There is now a struct with detailed configuration which runs the entire server and aborts the server on drop. This simplifies running the server in various situations, including tests. The configuration is now done using a declarative struct, which supports more control over how it runs so it can be more easily used behind a reverse proxy, without TLS etc.

This is aiming to fix #2177, #2179 and #2178.

Breaking Changes

The configuration file format has changed, deployments will need to updated. For the full format see struct Config in iroh-net/src/bin/iroh-relay.rs. Here a summary:

The 3 parts of the server now have an independent enable setting: enable_relay, enable_stun and enable_metrics. If omitted they default to true.
The way to specify which addresses the server listens on has changed: http_bind_addr is for the relay server, stun_bind_addr for the STUN server, metrics_bind_addr is for the optional metrics server and tls.https_bind_addr is for when TLS is enabled. Note these are now all full socket addresses. All have sensible defaults if omitted.
There are new options in tls.cert_path and tls.key_path which allow more control over where the manual TLS keys are to be read from.
iroh_net::defaults::DEFAULT_RELAY_STUN_PORT has been renamed to iroh_net::defaults::DEFAULT_STUN_PORT.

TBD: some APIs changed as well. Why are they not all private?

Notes & open questions

The iroh_net::relay::iroh_relay crate name is a bit weird. But iroh_net::relay::server is already taken. Maybe iroh_net::relay::bin could work, but that would be weird when using it from code in other places.
The ServerConfig struct is a declarative way of controlling the new server interface. It's kind of nice to use. Bu it is a public API that will be a breaking change every time it changes, and it will change. Maybe it's worth creating a builder API for this. But maybe that's something to only tackle when it is a real demand. I feel like the iroh_net::relay::server builders are an attempt at doing this earlier than needed.

Change checklist

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

iroh-net/src/relay/iroh_relay.rs

link2xt · 2024-06-04T15:10:53Z

Related issue for reference: #2179

still a lot of cleanup and some bug fixing

There it's also available to other code

The server itself doesn't need this, only the Let's Encrypt config. So push it into there and remove the need for it otherwise.

an empty join_set will return None right away. but this set will be empty most of the time

flub · 2024-06-10T17:39:51Z

@Arqu probably time for you to give some feedback on the config file changes I'm proposing. Check src/bin/iroh-relay.rs' Config which should have sufficient docs, the changes are highlighted in the pr description

flub · 2024-06-10T17:42:45Z

@link2xt I think this is in a reasonable shape now. For you the important changes are probably also described in the Config struct of the binary.

(can't seem to ask you as reviewer :( )

Arqu · 2024-06-11T13:54:33Z

@Arqu probably time for you to give some feedback on the config file changes I'm proposing. Check src/bin/iroh-relay.rs' Config which should have sufficient docs, the changes are highlighted in the pr description

Just went over this, the changes are nice and I'm fine with this moving forward. Once this lands I'll update the configuration files for the deployments.

iroh-net/src/bin/iroh-relay.rs

dignifiedquire · 2024-06-18T07:28:00Z

iroh-net/src/bin/iroh-relay.rs

+    fn key_path(&self) -> PathBuf {
+        self.manual_key_path
+            .clone()
+            .unwrap_or_else(|| self.cert_dir().join("default.key"))


should these file names be constants?

They used to use the hostname field. But it seemed weird to require the hostname for manual TLS, it was only used to pick up a default name here. I think a constant default is fine, I honestly expect this to be customised most of the time.

iroh-net/src/bin/iroh-relay.rs

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

dignifiedquire · 2024-06-18T07:33:37Z

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

@@ -320,13 +378,17 @@ impl ServerState {
                    }
                }
            }
+            if let Some(server) = server {
+                // TODO: if the task this is running in is aborted this server is not shut


what about this todo?

I decided to leave that in because I felt like if I had to turn all of the relay server actors into structured concurrency the rabbit hole was too deep. This is basically the status quo so not a regression. It would be nice to do this, but as a separate PR probably.

Plus, would need to decide on how much I like tokio-graceful-shutdown possibly :)

iroh-net/src/relay/iroh_relay.rs

dignifiedquire

some small notes, otherwise lgtm

…everse-proxy support (n0-computer#2341) ## Description This re-architects the relay-server binary. There is now a struct with detailed configuration which runs the entire server and aborts the server on drop. This simplifies running the server in various situations, including tests. The configuration is now done using a declarative struct, which supports more control over how it runs so it can be more easily used behind a reverse proxy, without TLS etc. This is aiming to fix n0-computer#2177, n0-computer#2179 and n0-computer#2178. ## Breaking Changes The configuration file format has changed, deployments will need to updated. For the full format see `struct Config` in `iroh-net/src/bin/iroh-relay.rs`. Here a summary: - The 3 parts of the server now have an independent enable setting: `enable_relay`, `enable_stun` and `enable_metrics`. If omitted they default to `true`. - The way to specify which addresses the server listens on has changed: `http_bind_addr` is for the relay server, `stun_bind_addr` for the STUN server, `metrics_bind_addr` is for the optional metrics server and `tls.https_bind_addr` is for when TLS is enabled. Note these are now all full socket addresses. All have sensible defaults if omitted. - There are new options in `tls.cert_path` and `tls.key_path` which allow more control over where the manual TLS keys are to be read from. - `iroh_net::defaults::DEFAULT_RELAY_STUN_PORT` has been renamed to `iroh_net::defaults::DEFAULT_STUN_PORT`. TBD: some APIs changed as well. Why are they not all private? ## Notes & open questions * The `iroh_net::relay::iroh_relay` crate name is a bit weird. But `iroh_net::relay::server` is already taken. Maybe `iroh_net::relay::bin` could work, but that would be weird when using it from code in other places. * The `ServerConfig` struct is a declarative way of controlling the new server interface. It's kind of nice to use. Bu it is a public API that will be a breaking change every time it changes, and it will change. Maybe it's worth creating a builder API for this. But maybe that's something to only tackle when it is a real demand. I feel like the `iroh_net::relay::server` builders are an attempt at doing this earlier than needed. ## Change checklist - [x] Self-review. - [x] Documentation updates if relevant. - [x] Tests if relevant. - [x] All breaking changes documented.

flub added 2 commits June 3, 2024 12:12

Basic relaly server

769a7b9

Convert most code to new server, not the bin yet

6f179a5

dignifiedquire reviewed Jun 4, 2024

View reviewed changes

iroh-net/src/relay/iroh_relay.rs Show resolved Hide resolved

dignifiedquire reviewed Jun 4, 2024

View reviewed changes

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

flub self-assigned this Jun 4, 2024

dignifiedquire added this to the v0.19.0 milestone Jun 6, 2024

flub added 11 commits June 6, 2024 22:35

almost make things work

0fac430

still a lot of cleanup and some bug fixing

tests pass again

2ee232f

move the defaults to the defaults module

f4bd06d

There it's also available to other code

Add tests, some small fixes and cleanup

295414b

this field is feature-flagged

2cd2e70

clippy

2c0a07e

docs

16cf5c0

Clean up the use of hostname

981c500

The server itself doesn't need this, only the Let's Encrypt config. So push it into there and remove the need for it otherwise.

Write stun server in structured-concurrency style

de8f716

fixup docs

554a9a0

do not poll JoinSet future when empty

bbdb013

an empty join_set will return None right away. but this set will be empty most of the time

flub requested a review from Arqu June 10, 2024 17:38

flub added 2 commits June 10, 2024 18:44

Merge branch 'main' into flub/relay-server

95b37a6

a few more doc comments

6cabd1f

flub marked this pull request as ready for review June 11, 2024 15:20

dignifiedquire changed the title ~~refactor(iroh-net): Rework relay-server binary, more configurable, reverse-proxy support~~ refactor(iroh-net)!: Rework relay-server binary, more configurable, reverse-proxy support Jun 18, 2024