feat: add MagicEndpoint to iroh-net #1133

Frando · 2023-06-26T10:57:51Z

This is a first pass at adding a MagicEndpoint that combines a magicsock and a quinn endpoint. The API is minimalistic so far and allows to both accept and create connections. I also added two utility functions to extract the ALPN and PeerId on incoming connections, and a maximally-simple example to test things.

An endpoint-like struct that can create and accept connections will be needed for peer-to-peer sockets as e.g. in the upcoming gossipswarm impl. (It could also work with functions only but would involve more code copying I think).

I mostly copied code around from the existing listen and dial functions. No tests or other code is yet converted over to actually use this. Sharing now to get feedback on the API.

flub

I've also been wondering about simplifying the use of the magicsocket. But I was initially thinking of adding a bit of functionality to Conn (or maybe a new MagicEndpoint, this is a pretty good name) that would have a method to give you a new quinn::Endpoint which is configured to use this magicsocket.

That way you'd have to call .add_know_addrs() and .get_mapping_addr() before you can call .connect() on a quinn::Endpoint making the relation between both ~~sockets~~ endpoints a bit more explicit. But it would not put us in the game of having to wrap all quinn::Endpoint APIs.

flub · 2023-06-26T11:37:57Z

iroh-net/src/defaults.rs

+        host_name: "derp.iroh.network".into(),
+        stun_only: false,
+        stun_port: 3478,
+        ipv4: UseIpv4::Some("35.175.99.113".parse().unwrap()),


FWIW I tend to slightly prefer one of the non-.parse().unwrap() variants, e.g. (35.175.99.133).into() or similar (writing this from memory after a holiday - so that's probably wrong!).

More interestingly though: are we fine with hardcoding an IP address here?

Ack to the first comment. Wrt to the second, I don't know ;-) Copied it from iroh/config.rs.

More interestingly though: are we fine with hardcoding an IP address here?

It is not great, but okay for the moment I would say

iroh-net/src/endpoint.rs

flub · 2023-06-26T12:00:18Z

iroh-net/src/endpoint.rs

+            client_config
+        };
+
+        debug!("connecting to {}: (via {} - {:?})", peer_id, addr, addrs);


i wonder if the addrs is really correct here? it could have even more addresses than we have here in the addrs variable i think?

Very much possible, I copied this from iroh-net/src/client.rs:132 and did not check it at all

iroh-net/src/endpoint.rs

Frando · 2023-06-26T12:55:07Z

I've also been wondering about simplifying the use of the magicsocket. But I was initially thinking of adding a bit of functionality to Conn (or maybe a new MagicEndpoint, this is a pretty good name) that would have a method to give you a new quinn::Endpoint which is configured to use this magicsocket.

Aha, yeah that would work as well. Somehow, the notion of connect(peer_id) is quite appealing to me though.

That way you'd have to call .add_know_addrs() and .get_mapping_addr() before you can call .connect() on a quinn::Endpoint making the relation between both ~~sockets~~ endpoints a bit more explicit. But it would not put us in the game of having to wrap all quinn::Endpoint APIs.

That is true. However, the API surface of quinn::Endpoint is really small when omitting the constructors which are taken care of in either case. Method-wise, it's just:

accept, connect: already implemented here
local_addr is mostly useless unless coupled with get_mapping_addr I think? So the wrapping would make sense here IMO
set_server_config and rebind: not sure if exposing those in the magicsock setup is needed / makes sense
close and wait_idle: those should be exposed

iroh-net/src/endpoint.rs

iroh-net/examples/magic.rs

Frando · 2023-06-27T16:23:41Z

Thanks for the reviews! I added a MagicEndpointBuilder and also more docs.

dignifiedquire · 2023-06-27T17:38:49Z

@Frando would be great if you could do the integration/usage of it in iroh itself, so we can see how and if it works there as needed for the current use case.

iroh-net/src/endpoint.rs

dignifiedquire · 2023-06-27T19:09:31Z

iroh-net/src/endpoint.rs

+    pub async fn bind(self, bind_addr: SocketAddr) -> anyhow::Result<MagicEndpoint> {
+        let keypair = self.keypair.unwrap_or_else(|| Keypair::generate());
+        let tls_server_config =
+            tls::make_server_config(&keypair, self.alpn_protocols, self.keylog)?;


does this error if alpn_protocols is empty?

Frando · 2023-06-28T10:58:46Z

@Frando would be great if you could do the integration/usage of it in iroh itself, so we can see how and if it works there as needed for the current use case.

Allright, I took a stab at this, the latest commit uses MagicEndpoint throughout iroh.

dignifiedquire · 2023-06-28T14:36:20Z

Allright, I took a stab at this, the latest commit uses MagicEndpoint throughout iroh.

Very nice and clean :) really like having this abstraction.

dignifiedquire · 2023-06-28T14:37:06Z

@Frando the last integration I would love to see is replacing the structure in hp/magicsock/conn.rs tests

Frando · 2023-06-28T20:32:11Z

@Frando the last integration I would love to see is replacing the structure in hp/magicsock/conn.rs tests

I just had a look at this. The tests access more internals of the magic socket than the current API exposes. Most seems to me for validation of those internals.

they access the tracked_endpoints() of a magic socket to check if some peer id is in there.
they also update the network map completely whereas the currently-exposed API on the MagicEndpoint only allows to add_known_addr(peer_id, socket_addrs)

I haven't worked with the internals of the magic socket much yet so not sure if those are needed outside of testing. So, should I ..

expose tracked_endpoints and set_network_map on the MagicEndpoint or
allow access to the magic socket Conn on the MagicEndpint through pub (or pub(crate) methods magicsocket() and magicsocket_mut(), and keeps tests mostly the same, or
restructure some of the tests to use less internals and work more with PeerIds only (likely possible, but not sure if the current setup is intended)?

Frando · 2023-06-28T20:34:29Z

And CI passed on the last commit, including CLI tests! Only netsim failures, but seems unrelated (github bucket something something)

dignifiedquire · 2023-06-28T20:38:31Z

I think a first pass just exposing things that are needed using cfg(test) is fine. I can take a look in a second round if it makes sense to restructure the tests.

iroh-net/src/client.rs

iroh-net/src/endpoint.rs

dignifiedquire

some smaller nits, other than that this is looking good :)

Frando · 2023-06-29T09:28:34Z

I addressed the last comments and rebased onto main. Let's see what CI says.

One thing I'm not super sure about is the netmap managment. But I think this can be refined later.

iroh-net/examples/magic.rs

dignifiedquire · 2023-06-30T13:18:37Z

iroh-net/src/hp/magicsock/conn.rs

@@ -2649,7 +2658,7 @@ mod tests {
                    nodes: vec![DerpNode {
                        name: "t1".into(),
                        region_id: 1,
-                        host_name: "http://test-node.invalid".parse().unwrap(),
+                        host_name: "https://test-node.invalid".parse().unwrap(),


does this need to be https?

probably does, just wondering why this change was necessary now

good question. I can't tell for sure, but this way the tests passed.

dignifiedquire · 2023-06-30T13:20:00Z

iroh-net/src/lib.rs

 pub mod hp;
+pub mod magic_endpoint;


can we make this private, if we reexport the MagicEndpoint?

There's also the MagicEndpointBuilder struct and accept_conn functions in there (and possible get_alpn and get_peer_id, see below). Not sure if they should all be at the top level?
I think I'd defer this to later, currently pretty much everything in iroh-net is public anyway - and once things stabilize a bit we might want to change this overall and actually define the public API surface for iroh-net?

iroh-net/src/magic_endpoint.rs

dignifiedquire · 2023-06-30T13:21:31Z

iroh-net/src/magic_endpoint.rs

+}
+
+/// Extract the [`PeerId`] from the peer's TLS certificate.
+pub async fn get_peer_id(connection: &quinn::Connection) -> anyhow::Result<PeerId> {


does this need to be public?

So they are used in the accept_conn above, which definitely should be public. I left them public, because get_alpn already works after the first handshake phase (on quinn::Connecting) whereas get_peer_id only works on the established quinn::Connection. So I wasn't sure if maybe you'd want to do the alpn extraction earlier, potentially abort if it's not what you expected, and then to get_peer_id afterwards. However I don't think it makes much of a difference, so also fine to make them non-public for now and advise to use accept_conn.

lets leave it as is for now then

* move dial_peer onto MagicEndpoint * some renames for clarity * remove iroh_net::client module

flub

Nice! Most of my comments are about the public docs from a user point of view. But nothing is blocking.

iroh-net/src/magic_endpoint.rs

flub · 2023-06-30T16:24:25Z

iroh-net/src/magic_endpoint.rs

+    /// Connect to a remote endpoint, creating an endpoint on the fly.
+    ///
+    /// The PeerId and the ALPN protocol are required. If you happen to know dialable addresses of
+    /// the remote endpoint, they can be specified and will be added to the endpoint's peer map.


Is it possible to describe what the "endpoint's peer map" means?

I removed the term for now and took over the description you proposed in connect

flub · 2023-06-30T16:47:33Z

iroh-net/src/magic_endpoint.rs

+            .conn
+            .get_mapping_addr(&node_key)
+            .await
+            .expect("just inserted");


There is an .await involved... so who knows?

True! Made it an error instead. Will have to check in practice if this occurs, but not panicing sounds like the right thing to do.

iroh-net/src/magic_endpoint.rs

flub · 2023-06-30T17:03:07Z

iroh-net/src/magic_endpoint.rs

+}
+
+/// Accept an incoming connection and extract the client-provided [`PeerId`] and ALPN protocol.
+pub async fn accept_conn(


I'd be tempted to experiment with an extension trait or a wrapper for Connecting (and I guess Accept). But we can do that later.

Yup I thought about that too. However we'd either have to use async_trait or write the futures by hand. I'd defer this to post-merge I think.

flub · 2023-06-30T17:21:58Z

iroh-net/src/hp/magicsock/conn.rs

@@ -2826,7 +2820,7 @@ mod tests {
        })
    }

-    fn setup_logging() {
+    pub fn setup_logging() {


I kind of wish you copied in iroh-bytes/src/test_utils.rs to iroh-net/src/test_utils.rs and used it's setup_logging call for your functions instead of making this pub... But I tried to clean this up earlier and utterly failed failed. For new #[tokio::test] tests it should just work though, since they're single-threaded runtimes. Just don't try change existing tests.

But if you don't want to do that now that's fine, it's my fault for not managing to clean this up before.

Yes this sounds good to me too. I also want to expose run_derp_and_stun in a test_util module also to other crates (i.e. iroh-gossip) that use iroh-net and need to test things with a local derper.
However I'd prefer to do this after the merge in a new PR.

flub reviewed Jun 26, 2023

View reviewed changes

flub previously approved these changes Jun 26, 2023

View reviewed changes

iroh-net/src/endpoint.rs Outdated Show resolved Hide resolved

dignifiedquire reviewed Jun 27, 2023

View reviewed changes

iroh-net/examples/magic.rs Show resolved Hide resolved

dignifiedquire reviewed Jun 27, 2023

View reviewed changes

iroh-net/examples/magic.rs Outdated Show resolved Hide resolved

Frando dismissed flub’s stale review via 89e6b10 June 27, 2023 16:20

dignifiedquire reviewed Jun 27, 2023

View reviewed changes

iroh-net/src/endpoint.rs Outdated Show resolved Hide resolved

dignifiedquire reviewed Jun 27, 2023

View reviewed changes

Frando force-pushed the Frando/magic-endpoint branch from dd39d43 to a9a6693 Compare June 28, 2023 09:35

dignifiedquire reviewed Jun 29, 2023

View reviewed changes

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

dignifiedquire reviewed Jun 29, 2023

View reviewed changes

iroh-net/src/endpoint.rs Outdated Show resolved Hide resolved

dignifiedquire reviewed Jun 29, 2023

View reviewed changes

iroh-net/src/endpoint.rs Outdated Show resolved Hide resolved

dignifiedquire reviewed Jun 29, 2023

View reviewed changes

iroh-net/src/endpoint.rs Outdated Show resolved Hide resolved

dignifiedquire previously approved these changes Jun 29, 2023

View reviewed changes

Frando dismissed dignifiedquire’s stale review via 6d83ceb June 29, 2023 09:11

Frando force-pushed the Frando/magic-endpoint branch from ff41242 to 0c6bfe0 Compare June 29, 2023 09:27

dignifiedquire reviewed Jun 30, 2023

View reviewed changes

iroh-net/examples/magic.rs Show resolved Hide resolved

dignifiedquire reviewed Jun 30, 2023

View reviewed changes

iroh-net/src/magic_endpoint.rs Outdated Show resolved Hide resolved

dignifiedquire reviewed Jun 30, 2023

View reviewed changes

Frando added 20 commits June 30, 2023 17:27

test: magic endpoint end-to-end

f087d8d

feat: add MagicEndpoint::close and test

5deca05

examples(iroh-net): set derp addr from cli

57e6c37

change(net): add MagicEndpointBuilder

36f1978

chore: fmt

203dcf9

fix: MagicEndpoint rebase and cleanup

929f65c

fix: typos

d7da7cb

fix: typos

f263e05

feat: convert iroh to use the MagicEndpoint

d5b6fe9

chore: clippy

b0a800e

fix: derp url in tests

0f50598

change: use default derp region from iroh-net

fa0fec0

fix(iroh): actually pass keypair into magic endpoint

7b75589

tests: use magic endpoint in magicsock tests

787b97f

change(net): cleanups and renames

4d1158f

* move dial_peer onto MagicEndpoint * some renames for clarity * remove iroh_net::client module

chore: fmt

dc88c85

fix: adjust after rebase

bc2c44f

fix: make MagicEndpoint::add_known_addrs additive

699374d

docs: improve

015c4ed

chore: cleanup

0129484

Frando dismissed dignifiedquire’s stale review via 0129484 June 30, 2023 15:30

Frando force-pushed the Frando/magic-endpoint branch from 08c9154 to 0129484 Compare June 30, 2023 15:30

chore: fmt

64fe3fc

flub previously approved these changes Jun 30, 2023

View reviewed changes

docs: improve MagicEndpoint docs

f309d09

Frando dismissed flub’s stale review via f309d09 July 3, 2023 09:30

change(net): MagicEndpoint::bind takes port, not addr

87253d2

flub approved these changes Jul 3, 2023

View reviewed changes

dignifiedquire merged commit 4597cb3 into n0-computer:main Jul 3, 2023
14 of 15 checks passed

divagant-martian mentioned this pull request Aug 23, 2024

Allow customizing the quinn::TransportConfig in iroh::node::Builder #2592

Open

feat: add MagicEndpoint to iroh-net #1133

feat: add MagicEndpoint to iroh-net #1133

Conversation

Frando commented Jun 26, 2023 • edited Loading

flub left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frando commented Jun 26, 2023 • edited Loading

Frando commented Jun 27, 2023

dignifiedquire commented Jun 27, 2023

Choose a reason for hiding this comment

Frando commented Jun 28, 2023

dignifiedquire commented Jun 28, 2023

dignifiedquire commented Jun 28, 2023

Frando commented Jun 28, 2023 • edited Loading

Frando commented Jun 28, 2023

dignifiedquire commented Jun 28, 2023 • edited Loading

dignifiedquire left a comment

Choose a reason for hiding this comment

Frando commented Jun 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frando Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frando commented Jun 26, 2023 •

edited

Loading

flub left a comment •

edited

Loading

Frando commented Jun 26, 2023 •

edited

Loading

Frando commented Jun 28, 2023 •

edited

Loading

dignifiedquire commented Jun 28, 2023 •

edited

Loading

Frando Jul 3, 2023 •

edited

Loading