upnp port mapping - Githubissues

jgautier commented 6 years ago

Not sure if this fixes #1 but it does use upnp to open ports. If the router supports it this allows clients behind NATs to talk over TCP to each other. Couple things to point out:

I'm a little unsure about the impliedPort vs publicPort when joining the swarm. I basically made it so if the mapping fails it falls back to the impleidPort logic otherwise sets the publicPort to the port that was mapped.
It has to wait for the 'listen' event before the mapping since the port is not always known until after that event, and I made sure to try to open the port before the join is called (not sure if this part matters).
The ttl for the part mapping is set to 0 which keeps the port open forever which might be annoying to have your router have so many open ports. Maybe a different approach would be to set the ttl to lower and then re-map on an interval. That way the ports will close eventually after the program has exited.
I have not written a test. I am open to writing one just not sure what a good case would be. I tested this manually by running the example with data-swarm-defaults, utp: false and running the 2 swarms behind a NAT.

Thanks!

jgautier commented 6 years ago

also not sure whatsup with the tests. They all pass but the process does not exit after the tests finish. I tried on master and it has the same problem.

pfrazee commented 6 years ago

Good idea to check into this (upnp) and see if it can help improve connection rates. Need to double check that there's not a downside, like corporate/university LANs seeing it as bad activity.

okdistribute commented 6 years ago

@pfrazee I think it's worth trying out since this could improve connections for a large chunk of people -- orgs with high security could just outright disable upnp if they don't want it.

okdistribute commented 6 years ago

great work @jgautier !

martinheidegger commented 6 years ago

It seems like there is no unmapping done on close?

jgautier commented 6 years ago

@martinheidegger right here is where the unmapping happens on close: https://github.com/mafintosh/discovery-swarm/pull/43/files#diff-168726dbe96b3ce427e7fedce31bb0bcR107 is there an additional place I should add an unmapping? This only works if you properly close/destroy the swarm. If you exit a program without calling destroy on the swarm then the port stays mapped forever. This is actually pretty annoying since now I have tons of ports mapped on my router while doing testing :P. I think the better approach is probably to set the ttl like for an hour or a configurable time and then have a interval timer that renews the port mapping.

@pfrazee @karissa What do you guys think is a good way to test whether this would work well in a corporate/university setting?

What do you guys think needs to be done to get this merged? For me I think this is my checklist

[ ] Implement the better ttl strategy. What do you guys think about what I mentioned?
[ ] Fix the tests not exiting. Are other people having issues with this or is it just me?
[ ] Determine if there are any issues with corporate/university LANs
[ ] Write a new test that verifies the port is mapped correctly.
[ ] Resolve package.json conflict.

One idea for the test would be to import the nat upnp module in the test script and the use it to get the mappings and check that the port was actually opened. I don't know how well this would work because the test would likely fail if you have upnp turned off on your network and I am guessing wherever travis is running the tests also would not have upnp. Other then that the only other way I could test to think of is to mockout the nat-upnp object and verify the functions are being called.

Thanks everyone!

okdistribute commented 6 years ago

@jgautier I think getting this to work well in a corporate/university setting is difficult and I don't have enough expertise in this to tell you how. @maxogden might be the right one for this!

okdistribute commented 6 years ago

@jgautier as for the ttl, it sounds good but perhaps if the process is still running could we renew the ttl? people could have their dats replicating indefinitely.

pfrazee commented 6 years ago

If you exit a program without calling destroy on the swarm then the port stays mapped forever. This is actually pretty annoying since now I have tons of ports mapped on my router while doing testing :P. I think the better approach is probably to set the ttl like for an hour or a configurable time and then have a interval timer that renews the port mapping.

That sounds right to me. (I haven't gotten a chance to look at the protocol or your implementation, but--) Is it possible to make the port mappings idempotent so that multiple calls will overwrite the previous instead of accumulating?

@mafintosh should definitely take a look at this

jwerle commented 5 years ago

:raised_hands: :pray:

mafintosh / discovery-swarm

upnp port mapping #43