feat(upnp): add implementation based on IGD protocol #4156

jxs · 2023-07-04T15:34:33Z

Description

Implements UPnP via the IGD protocol. The usage of IGD is an implementation detail and is planned to be extended to support NATpnp.

Resolves: #3903.

Notes & open questions

As discussed, this hasn't yet any type of tests, since we need a network with a gateway with igd support. Feel free to suggest any ideas for testing.

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

thomaseizinger

Great work! I left some comments :)

protocols/upnp/Cargo.toml

protocols/upnp/src/lib.rs

protocols/upnp/src/provider.rs

protocols/upnp/src/behaviour.rs

protocols/upnp/src/lib.rs

thomaseizinger · 2023-07-04T18:20:46Z

You will need to exclude this from WASM to have CI pass.

dariusc93 · 2023-07-04T21:59:47Z

As discussed, this hasn't yet any type of tests, since we need a network with a gateway with igd support. Feel free to suggest any ideas for testing.

Something i was going to do in the future for testing was write a small server that would emulate a gateway for igd. Maybe this is something you could do is have a server and run test against that?

protocols/upnp/Cargo.toml

protocols/upnp/src/behaviour.rs

jxs · 2023-07-05T17:29:46Z

You will need to exclude this from WASM to have CI pass.

there's also the libssl-dev dependency on ubuntu, should we add it to the ci?

thomaseizinger · 2023-07-05T19:40:41Z

You will need to exclude this from WASM to have CI pass.

there's also the libssl-dev dependency on ubuntu, should we add it to the ci?

Is there an option to use a different SSL stack?

for a channel that polls the futures on an async task.

jxs · 2023-07-06T22:44:51Z

You will need to exclude this from WASM to have CI pass.

there's also the libssl-dev dependency on ubuntu, should we add it to the ci?

Is there an option to use a different SSL stack?

unfortunately the async-std upstream library doesn't expose that. surf exposes rustls as a feature thou

mxinden

Excited for this to land. Only drive-by comments.

Cargo.lock

libp2p/Cargo.toml

protocols/upnp/src/behaviour.rs

…protocol

thomaseizinger

Some more questions!

protocols/upnp/src/gateway.rs

protocols/upnp/src/behaviour.rs

protocols/upnp/src/gateway.rs

protocols/upnp/src/lib.rs

mergify · 2023-07-09T23:07:08Z

This pull request has merge conflicts. Could you please resolve them @jxs? 🙏

…protocol

mergify · 2023-09-04T09:45:56Z

This pull request has merge conflicts. Could you please resolve them @jxs? 🙏

…protocol

jxs · 2023-09-04T17:48:57Z

surf and consequently The async-std version is super flaky, running 10 times the example, in 6 I get this error:

Could this be the result of some form of garbage-collection? Are we dropping something somewhere and thus get connection resets?

hum, how could that be possible? We spawn the search_gateway task on the async-std executor and then wait for its output, it should be self contained no?

thomaseizinger · 2023-09-04T23:44:26Z

surf and consequently The async-std version is super flaky, running 10 times the example, in 6 I get this error:

Could this be the result of some form of garbage-collection? Are we dropping something somewhere and thus get connection resets?

hum, how could that be possible? We spawn the search_gateway task on the async-std executor and then wait for its output, it should be self contained no?

Yeah that is what I would expect. I know that async-std does implicitly spawn new worker threads and also the reactor but I'd be surprised if that causes flakiness.

mergify · 2023-09-05T13:37:47Z

This pull request has merge conflicts. Could you please resolve them @jxs? 🙏

…protocol

mxinden

I am fine moving forward here, i.e. merge. In case there is anything we can do to reduce the dependency footprint, that would be very much appreciated.

Cargo.lock

examples/upnp/src/main.rs

protocols/upnp/src/behaviour.rs

…protocol

mergify · 2023-09-06T17:50:51Z

This pull request has merge conflicts. Could you please resolve them @jxs? 🙏

…protocol

thomaseizinger

Thank you for your patience @jxs! I think this is the right path forward :)

Cargo.lock

libp2p/Cargo.toml

protocols/upnp/Cargo.toml

protocols/upnp/src/lib.rs

examples/upnp/src/main.rs

Cargo.lock

protocols/upnp/Cargo.toml

thomaseizinger

Looks great! Well done and sorry for so much back and forth :)

thomaseizinger · 2023-09-11T23:19:48Z

Let's go!

thomaseizinger · 2023-09-12T00:34:52Z

@jxs can you go through the open conversations and resolve them if they are addressed? Github won't let us merge otherwise :)

jxs · 2023-09-12T15:07:25Z

@jxs can you go through the open conversations and resolve them if they are addressed? Github won't let us merge otherwise :)

done Thomas, thanks! Merged 🎉

thomaseizinger · 2023-09-17T23:06:04Z

@jxs Mind updating the roadmap? https://github.com/libp2p/rust-libp2p/blob/master/ROADMAP.md#automate-port-forwarding-eg-via-upnp

jxs · 2023-09-20T13:51:15Z

btw, can we release libp2p-upnp 0.1?

jxs requested a review from thomaseizinger July 4, 2023 15:34

thomaseizinger reviewed Jul 4, 2023

View reviewed changes

dariusc93 reviewed Jul 4, 2023

View reviewed changes

protocols/upnp/Cargo.toml Outdated Show resolved Hide resolved

protocols/upnp/src/behaviour.rs Outdated Show resolved Hide resolved

jxs added 5 commits July 5, 2023 16:53

feat(protocols): Add initial UPnP protocol

bddebad

cargo clippy

ba6ad5a

fix docs

3638fd9

review: address Thomas review

6671af3

review: address Thomas review

7edc88b

jxs force-pushed the add-upnp-protocol branch from cea2d5f to 7edc88b Compare July 5, 2023 15:54

review: address Thomas review

5c59216

jxs added 3 commits July 6, 2023 22:14

update copyright

cb2dbbc

rename Provider trait to Gateway

4172755

replace UnoderedFutures on the main behaviour

6fc4cfc

for a channel that polls the futures on an async task.

jxs force-pushed the add-upnp-protocol branch from 5553164 to 6fc4cfc Compare July 6, 2023 22:31

mxinden reviewed Jul 7, 2023

View reviewed changes

Cargo.lock Show resolved Hide resolved

libp2p/Cargo.toml Outdated Show resolved Hide resolved

protocols/upnp/src/behaviour.rs Outdated Show resolved Hide resolved

jxs added 4 commits July 7, 2023 16:12

update Cargo.toml formatting

0eb75da

Merge branch 'master' of github.com:libp2p/rust-libp2p into add-upnp-…

3f3f745

…protocol

revert Box<dyn Error> to String conversion.

7d679c8

clippy

28e0fde

jxs force-pushed the add-upnp-protocol branch from 87f58c3 to 28e0fde Compare July 8, 2023 12:13

thomaseizinger reviewed Jul 9, 2023

View reviewed changes

thomaseizinger changed the title ~~feat(protocols): Add initial UPnP protocol~~ feat(upnp): add implementation based on IGD protocol Jul 9, 2023

jxs added 2 commits July 10, 2023 14:45

Merge branch 'master' of github.com:libp2p/rust-libp2p into add-upnp-…

bdb0818

…protocol

address Thomas and Max review

293133b

Merge branch 'master' of github.com:libp2p/rust-libp2p into add-upnp-…

7c8c534

…protocol

Merge branch 'master' of github.com:libp2p/rust-libp2p into add-upnp-…

e5cb61d

…protocol

mxinden approved these changes Sep 6, 2023

View reviewed changes

Cargo.lock Outdated Show resolved Hide resolved

examples/upnp/src/main.rs Outdated Show resolved Hide resolved

protocols/upnp/src/behaviour.rs Show resolved Hide resolved

jxs added 2 commits September 6, 2023 18:20

Merge branch 'master' of github.com:libp2p/rust-libp2p into add-upnp-…

316b524

…protocol

replace example with tokio

ebc4fdb

jxs added 2 commits September 6, 2023 21:09

Merge branch 'master' of github.com:libp2p/rust-libp2p into add-upnp-…

98ba415

…protocol

deprecate async-std support

b642cd9

jxs force-pushed the add-upnp-protocol branch from b948bbb to b642cd9 Compare September 7, 2023 12:59

thomaseizinger reviewed Sep 8, 2023

View reviewed changes

jxs added 2 commits September 8, 2023 09:08

address review

4c18dab

address missing remark on review

1de13e8

thomaseizinger reviewed Sep 11, 2023

View reviewed changes

Cargo.lock Show resolved Hide resolved

thomaseizinger reviewed Sep 11, 2023

View reviewed changes

protocols/upnp/Cargo.toml Show resolved Hide resolved

thomaseizinger approved these changes Sep 11, 2023

View reviewed changes

add publish false to overcome cargo semver

4b68a48

thomaseizinger added the send-it label Sep 11, 2023

jxs and others added 2 commits September 12, 2023 10:45

Merge branch 'master' into add-upnp-protocol

98108f7

Merge branch 'master' into add-upnp-protocol

1a36609

mergify bot merged commit 411a049 into libp2p:master Sep 12, 2023
68 of 69 checks passed

mxinden mentioned this pull request Sep 21, 2023

chore: prepare non-breaking releases #4537

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(upnp): add implementation based on IGD protocol #4156

feat(upnp): add implementation based on IGD protocol #4156

jxs commented Jul 4, 2023 •

edited by thomaseizinger

Loading

thomaseizinger left a comment

thomaseizinger commented Jul 4, 2023

dariusc93 commented Jul 4, 2023

jxs commented Jul 5, 2023

thomaseizinger commented Jul 5, 2023

jxs commented Jul 6, 2023

mxinden left a comment

thomaseizinger left a comment

mergify bot commented Jul 9, 2023

mergify bot commented Sep 4, 2023

jxs commented Sep 4, 2023

thomaseizinger commented Sep 4, 2023

mergify bot commented Sep 5, 2023

mxinden left a comment

mergify bot commented Sep 6, 2023

thomaseizinger left a comment

thomaseizinger left a comment

thomaseizinger commented Sep 11, 2023

thomaseizinger commented Sep 12, 2023

jxs commented Sep 12, 2023

thomaseizinger commented Sep 17, 2023

jxs commented Sep 20, 2023

feat(upnp): add implementation based on IGD protocol #4156

feat(upnp): add implementation based on IGD protocol #4156

Conversation

jxs commented Jul 4, 2023 • edited by thomaseizinger Loading

Description

Notes & open questions

Change checklist

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger commented Jul 4, 2023

dariusc93 commented Jul 4, 2023

jxs commented Jul 5, 2023

thomaseizinger commented Jul 5, 2023

jxs commented Jul 6, 2023

mxinden left a comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

mergify bot commented Jul 9, 2023

mergify bot commented Sep 4, 2023

jxs commented Sep 4, 2023

thomaseizinger commented Sep 4, 2023

mergify bot commented Sep 5, 2023

mxinden left a comment

Choose a reason for hiding this comment

mergify bot commented Sep 6, 2023

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger commented Sep 11, 2023

thomaseizinger commented Sep 12, 2023

jxs commented Sep 12, 2023

thomaseizinger commented Sep 17, 2023

jxs commented Sep 20, 2023

jxs commented Jul 4, 2023 •

edited by thomaseizinger

Loading