feat(iroh-net): Upnp port mapping #1117

divagant-martian · 2023-06-20T17:08:36Z

This is the skateboard version of upnp mappings, which I think should cover a wide range of use cases to begin with.

Random notes on this (that depending on process will end up in one/several issues)

Tailscale differences (behaviour)

for probing, they first send a ssdp:all request (simple service discovery) this seems unnecessary to me since the type of device we want is an igd one. An ssdp:all request will be answered from many different devices, let's say for example speakers connected to the local network. I don't see this as useful since afterwards, the responses are filtered to gather the igd devices anyway. We don't do that, and instead send the igd request only.
before sending the igd request, which is technically sent to a multicast address, ts sends also a unicast request to the likely router address. This is to deal with firewalls that reject the response to the multicast address since it's coming to the unicast address. Including this sounds something that we should do, but would mean to contribute or fork igd or move the code to our codebase. The igd code is fairly simple so I don't think any of these options is out of scope.
the local address is for now set to ipv4 unspecified address. ts uses the interfaces address.
renewing and creating a mapping is done as soon as it's identified (awaiting) instead of "spawning a task" (goroutine)
no on_change when the mapping changes.
"closing" the client in tailscale mean invalidating the mappings and marking the client as closed. However the closed value is only ever used to .. prevent double closing. I think we can simply invalidate the mappings, there is no real use for the closing.

Other notes

This so far follows the og template but I'd like us to consider a couple points:

No need to register a on_change callback. We probably can instead add a tokio::sync::watch to keep track of the last changed value.
The client is clone, which is surprising to me. Do we really want multiple port mapping clients to exist and operate concurrently? Depending on this moving this to be more of a service/actor would make sense.
Updating the mapping is done in a very synchronous, reactive fashion. If we need a mapping and don't have one, we start creating it there. If the mapping is due for renewal, the renewal process starts in that instant (when we realize it's late ). We could instead add the necessary futures to renew the mapping. For creation, we would create the mapping as soon as the local port is known and not when the mapping is requested.
These are all changes that make sense to me, but it would be good to have the opinion of someone with better understanding of how port mapping fits within the code as a whole, in concept.

Further notes

I noticed that the netcheck code that calls the upnp mapping does not wait for it to complete. While this was simple to solve, I'd like to first check out the git history of the code to understand what the current code is supposed to do and adjust accordingly.
What's the standard for logging?
What kind of metrics do we want to check? How do we do metrics in general
Tests will be interesting. I've tested the relevant parts of the code in a toy project and get a mapping, but in practice this probably depends on different kinds of routers and lan setups in general

dignifiedquire · 2023-06-21T09:29:58Z

What's the standard for logging?

We use tracing crate for logging, some nice examples can be found in the netcheck.rs code.

dignifiedquire · 2023-06-21T09:31:35Z

What kind of metrics do we want to check? How do we do metrics in general

This is how we define metrics:

/~https://github.com/n0-computer/iroh/blob/main/src/metrics/netcheck.rs
you can look at netcheck.rs to see how we use it.

for a detailed walkthrough, ping @Arqu our metrics guy :D

dignifiedquire · 2023-06-21T09:32:30Z

What kind of metrics do we want to check?

I would start with the metrics that tailscale collects, other than that my general approach is to collect

success & failure rates
unexpected events

dignifiedquire · 2023-06-21T09:33:35Z

Tests will be interesting.

Yes, maybe we could add some functionality to the doctor command that tries to do a port mapping and reports how well it worked, so it is easy for folks to run manual tests.

Arqu · 2023-06-21T12:29:31Z

Dig mostly covered the bare basics of metrics. To have metrics collected run with iroh --metrics-addr 127.0.0.1:9090 provide ... then you can access the current snapshot of prometheus metrics on localhost:9090. There's a separate exporter you can run to push things up to our infra.

As for what metrics, there is no hard rule, metrics are there to serve us, the important ones will be surfaced more in grafana but most of them are useful. I usually go by add more as you need. Typically you want to cover major events, and output counters along with instrumenting main loops and interesting branches/error cases.

iroh-net/src/hp/portmapper.rs

iroh-net/src/hp/portmapper/upnp.rs

iroh-net/src/hp/portmapper.rs

dignifiedquire · 2023-06-21T19:09:02Z

No need to register a on_change callback. We probably can instead add a tokio::sync::watch to keep track of the last changed value.

Sounds good, though should probably check for what it is actually used where and if we need it.

dignifiedquire · 2023-06-21T19:10:15Z

The client is clone, which is surprising to me.

This is purely because of the structure of netcheck.rs at the moment. If we can make it work without, cool.

dignifiedquire · 2023-06-21T19:11:53Z

Updating the mapping is done in a very synchronous, reactive fashion. If we need a mapping and don't have one, we start creating it there. If the mapping is due for renewal, the renewal process starts in that instant (when we realize it's late ). We could instead add the necessary futures to renew the mapping. For creation, we would create the mapping as soon as the local port is known and not when the mapping is requested.
These are all changes that make sense to me, but it would be good to have the opinion of someone with better understanding of how port mapping fits within the code as a whole, in concept.

This sounds good to me in general, but I think it would be good to check with @flub how to best integrate this with his refactor plans for netcheck.

iroh-net/src/hp/portmapper.rs

iroh-net/src/hp/portmapper/upnp.rs

ramfox · 2023-06-21T20:17:32Z

iroh-net/src/hp/portmapper.rs

+                        tracing::debug!(
+                            "upnp gateway changed from {old_gateway_addr} to {gateway_addr}"
+                        );
+                        // TODO(@divagant-martian): tailscale does not invalidate the mappings here. Why?


If we've searched for the router and the router has changed, maybe this implies that we don't have access to the old one anymore, so release-ing the mapping is maybe not possible, or just not our responsibility?

If our local address has changed, but our router is the same, I understand we want to release the mapping because we want them to not direct the traffic to the old local address.

Anyways, I think it's fine to not invalidate the mappings in this case.

you are right about releasing the mappings, sorry for the lack of precision: I mean they don't clear the mappings. So we still have self.current_mapping as Some even if, as you pointed, the router is not there anymore

iroh-net/src/hp/portmapper.rs

dignifiedquire · 2023-07-03T08:20:06Z

iroh-net/src/hp/netcheck.rs

+    /// Updates the portmap related values based on a [`portmapper::ProbeResult`].
+    pub fn update_portmap_probe(&mut self, probe_result: Option<portmapper::ProbeOutput>) {
+        match probe_result {
+            Some(portmapper::ProbeOutput { upnp, pmp, pcp }) => {


Suggestion, lest just just store the ProbeOutput instead of these three individual fields.

done in 0846437

dignifiedquire · 2023-07-03T08:27:55Z

iroh/src/commands/doctor.rs

+    PortMap {
+        /// Local port to get a mapping.
+        local_port: NonZeroU16,
+        /// How long to wait for an external port to be ready.


nit: in seconds or sth like that is missing.

done in 552e1c7

iroh-net/src/hp/portmapper/upnp.rs

dignifiedquire · 2023-07-03T08:35:47Z

iroh-net/src/hp/portmapper/mapping.rs

+    fn poll(&mut self, cx: &mut std::task::Context<'_>) -> Poll<Event> {
+        // grab the waker if needed
+        if let Some(waker) = &self.waker {
+            if waker.will_wake(cx.waker()) {


what exactly does this check do?

an optimization to prevent switching wakers when the task has not moved threads

dignifiedquire · 2023-07-03T08:42:01Z

When I try and run the doctor command on a network where upnp is disabled it just times out, but looking at the log it should actually error. Any chance that error could be properly reported?

iroh-net/src/hp/portmapper.rs

dignifiedquire · 2023-07-03T09:09:11Z

Okay, tried to run this locally and got into the following issue. The local_addr ends up being set to 127.0.0.1 when requesting a mapping, resulting in an error, as can be seen here (using the rust-igd examples)

When I use my local lan IP 192.168.x.x things work as expected in all the examples for rust-igd.

divagant-martian · 2023-07-03T10:26:03Z

When I try and run the doctor command on a network where upnp is disabled it just times out, but looking at the log it should actually error. Any chance that error could be properly reported?

Not sure what you mean, in your screenshot there is both the log and the error.

Can you elaborate?

dignifiedquire · 2023-07-03T10:50:17Z

Can you elaborate?

The error is only reported in the logs. If I run without logs only this shows up

divagant-martian · 2023-07-03T14:09:26Z

@dignifiedquire ok I see. This is somewhat intentional and I see both approaches making sense. Right now I report whether there is an external address and if it changed on the watcher. In the doctor command, a failure would get a None report to a None report (so intentionally not really reported". This is the approach tailscale has.

If you think it makes sense, I could change this option to a result and propagate it up. The only place where this would make a difference tho is in the doctor command

dignifiedquire · 2023-07-03T17:03:31Z

I see, that makes sense. Given that we can get the errors via logs, I think we can leave it.

divagant-martian · 2023-07-03T19:29:16Z

This should be ready in any case, since I moved to using the same dep as #1157 for the local ip.
Home router is not necessary yet, since the addr in which an upnp gateway answers is a multicast addr following the spec, not its unicast addr

dignifiedquire

lets go :)

changes have been addressed and pr has been approved

* add igd * add log, use types for readability * request mapping * fix code placement + comments * filling in the gaps * some cleanup and refactor * debugging * debug cleanning * merge cleanup * easy doctor appointment * debug cleanning * use non zero to ensure port to map is positive * spelling * follow iroh code style * log when probe fails * update comment * more logs, use localhost * add portmap metrics to iroh-metrics * use portmap metrics * add client * wip: closing for the day * wip * more wip * connecting stuff * coding with the heart lol * fixes * customize timeout of portmapping doctor command * on_change for portmap * improve docs and logs * add mapping struct that handles maintenance * add waker * improve debug logs * connect with mapping thing * partially connect current mapping stream * simplify mapping handling and add a sanity test * remove hardcoded mapping half life * remove unused field * ensure external port is non zero * simplify test types, reduce test timeout * add external addr fn to mapping * rework getting a new upnp mapping to reuse a known gateway and or known external port * connect expiry and renew events * split client functions * update doctor command * add service channel capacity * remove unnecesary change * leverage derive_more and the utils mod * remove yet one more unnecesary clone * Revert "remove yet one more unnecesary clone" might be controversial since technically the probe starts before the report This reverts commit f558246. * misc spelling * misc docs * cleanup * more cleanup * simplify logic updating a probe * appease clippy * expand comment * expand metrics * self review * improve doctor docs * unflatten portmap probe * get interfaces * get interfaces using dep already here

divagant-martian added 7 commits June 12, 2023 17:07

add igd

65d3500

add log, use types for readability

6781552

request mapping

7cd2bb5

fix code placement + comments

2a29bd9

filling in the gaps

428f6a8

Merge branch 'main' into upnp-port-mapping

99234ff

some cleanup and refactor

4a8865c

divagant-martian added 4 commits June 21, 2023 09:51

debugging

49d972d

Merge branch 'main' into upnp-port-mapping

a2288be

debug cleanning

73ec440

merge cleanup

ea88b84

dignifiedquire reviewed Jun 21, 2023

View reviewed changes

iroh-net/src/hp/portmapper.rs Outdated Show resolved Hide resolved

dignifiedquire reviewed Jun 21, 2023

View reviewed changes

iroh-net/src/hp/portmapper.rs Outdated Show resolved Hide resolved

dignifiedquire reviewed Jun 21, 2023

View reviewed changes

iroh-net/src/hp/portmapper/upnp.rs Outdated Show resolved Hide resolved

dignifiedquire reviewed Jun 21, 2023

View reviewed changes

iroh-net/src/hp/portmapper.rs Outdated Show resolved Hide resolved

ramfox previously requested changes Jun 21, 2023

View reviewed changes

divagant-martian added 6 commits June 21, 2023 20:49

easy doctor appointment

a7394cf

debug cleanning

fb7f67f

use non zero to ensure port to map is positive

4d2b14b

spelling

80fd159

follow iroh code style

32d5a0c

log when probe fails

dd75325

divagant-martian requested review from dignifiedquire and flub July 2, 2023 22:33

self review

63a93a7

dignifiedquire reviewed Jul 3, 2023

View reviewed changes

iroh-net/src/hp/portmapper/upnp.rs Show resolved Hide resolved

dignifiedquire reviewed Jul 3, 2023

View reviewed changes

iroh-net/src/hp/portmapper.rs Show resolved Hide resolved

dignifiedquire reviewed Jul 3, 2023

View reviewed changes

iroh-net/src/hp/portmapper.rs Show resolved Hide resolved

divagant-martian self-assigned this Jul 3, 2023

improve doctor docs

552e1c7

divagant-martian added 3 commits July 3, 2023 12:17

unflatten portmap probe

0846437

Merge branch 'main' into upnp-port-mapping

da57bcf

get interfaces

5e9d6f7

divagant-martian requested a review from dignifiedquire July 3, 2023 18:20

get interfaces using dep already here

f1ae1fc

divagant-martian changed the title ~~feat: Upnp port mapping~~ feat(iroh-net): Upnp port mapping Jul 4, 2023

dignifiedquire approved these changes Jul 4, 2023

View reviewed changes

divagant-martian merged commit 701e9b7 into n0-computer:main Jul 4, 2023

dignifiedquire mentioned this pull request Jul 5, 2023

iroh-net: implement portmapping #910

Closed

4 tasks

divagant-martian deleted the upnp-port-mapping branch September 17, 2023 14:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(iroh-net): Upnp port mapping #1117

feat(iroh-net): Upnp port mapping #1117

divagant-martian commented Jun 20, 2023 •

edited

Loading

dignifiedquire commented Jun 21, 2023

dignifiedquire commented Jun 21, 2023

dignifiedquire commented Jun 21, 2023

dignifiedquire commented Jun 21, 2023

Arqu commented Jun 21, 2023 •

edited

Loading

dignifiedquire commented Jun 21, 2023

dignifiedquire commented Jun 21, 2023

dignifiedquire commented Jun 21, 2023

ramfox Jun 21, 2023

divagant-martian Jun 22, 2023

dignifiedquire Jul 3, 2023

divagant-martian Jul 3, 2023

dignifiedquire Jul 3, 2023

divagant-martian Jul 3, 2023

dignifiedquire Jul 3, 2023

divagant-martian Jul 3, 2023

dignifiedquire commented Jul 3, 2023

dignifiedquire commented Jul 3, 2023

divagant-martian commented Jul 3, 2023

dignifiedquire commented Jul 3, 2023

divagant-martian commented Jul 3, 2023

dignifiedquire commented Jul 3, 2023

divagant-martian commented Jul 3, 2023 •

edited

Loading

dignifiedquire left a comment

feat(iroh-net): Upnp port mapping #1117

feat(iroh-net): Upnp port mapping #1117

Conversation

divagant-martian commented Jun 20, 2023 • edited Loading

Tailscale differences (behaviour)

Other notes

Further notes

dignifiedquire commented Jun 21, 2023

dignifiedquire commented Jun 21, 2023

dignifiedquire commented Jun 21, 2023

dignifiedquire commented Jun 21, 2023

Arqu commented Jun 21, 2023 • edited Loading

dignifiedquire commented Jun 21, 2023

dignifiedquire commented Jun 21, 2023

dignifiedquire commented Jun 21, 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

dignifiedquire commented Jul 3, 2023

dignifiedquire commented Jul 3, 2023

divagant-martian commented Jul 3, 2023

dignifiedquire commented Jul 3, 2023

divagant-martian commented Jul 3, 2023

dignifiedquire commented Jul 3, 2023

divagant-martian commented Jul 3, 2023 • edited Loading

dignifiedquire left a comment

Choose a reason for hiding this comment

divagant-martian commented Jun 20, 2023 •

edited

Loading

Arqu commented Jun 21, 2023 •

edited

Loading

divagant-martian commented Jul 3, 2023 •

edited

Loading