New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

upnp port mapping #43

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@jgautier

jgautier commented May 29, 2018

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:

  1. 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.

  2. 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).

  3. 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.

  4. 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

This comment has been minimized.

jgautier commented May 29, 2018

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

This comment has been minimized.

Contributor

pfrazee commented May 30, 2018

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.

@karissa

This comment has been minimized.

Collaborator

karissa commented May 30, 2018

@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.

@karissa

This comment has been minimized.

Collaborator

karissa commented May 30, 2018

great work @jgautier !

@martinheidegger

This comment has been minimized.

Contributor

martinheidegger commented May 30, 2018

It seems like there is no unmapping done on close?

@jgautier

This comment has been minimized.

jgautier commented May 30, 2018

@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!

@karissa

This comment has been minimized.

Collaborator

karissa commented May 31, 2018

@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!

@karissa

This comment has been minimized.

Collaborator

karissa commented May 31, 2018

@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

This comment has been minimized.

Contributor

pfrazee commented Jun 1, 2018

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

This comment has been minimized.

jwerle commented Sep 14, 2018

🙌 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment