Skip to content
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

Re-create forwardings after externalIP change #2014

Merged
merged 1 commit into from Apr 19, 2019

Conversation

5 participants
@frennkie
Copy link
Contributor

commented Oct 7, 2018

I noticed that the nat=true feature works really well to detect when the external IP address of a NAT device has changed (in my case using UPnP). But this requires that lnd also takes over the creation of the port forwarding (e.g. 9735). In my case after every IP change the port forwardings were gone. I assume my NAT device removes them when the line is disconnected and re-established.

In lnd I found no code that would re-create the port forwardings in this case. Port forwardings are only setup in newServer at server start (configurePortForwarding in line 405).

This PR make sure that the forwardings are re-created after a change in the external IP is detected.

@frennkie frennkie force-pushed the frennkie:re-create-port-mappings branch from 1e7d694 to 07197d4 Oct 7, 2018

@Roasbeef Roasbeef requested a review from wpaulino Oct 8, 2018

@Roasbeef Roasbeef added this to the 0.5.1 milestone Oct 18, 2018

Show resolved Hide resolved server.go Outdated
@wpaulino
Copy link
Collaborator

left a comment

Thanks for the PR! As is, this PR seems a bit hacky to me. I think a better approach would be to have an additional config flag that assumes port forwarding is already set up manually by the user so that we don't attempt to do so (i.e., --nat-watch-only or another name if you have a better one!). Then, if this flag is active, we can just simply watch for IP address changes without dealing with any port forwarding logic.

@frennkie

This comment has been minimized.

Copy link
Contributor Author

commented Oct 27, 2018

@wpaulino I fully agree with you. I will try to find some time to work on adding the --nat-watch-only (or something like this) flag in a separate PR.

@frennkie

This comment has been minimized.

Copy link
Contributor Author

commented Oct 27, 2018

Regarding the port forwarding logic I think the code really doesn't need to be very complicated. I checked the RfCs and both UPnP and NAT-PMP do not mind if a port forwarding that is already present is requested again.

Actually on the contrary: NAT-PMP requires port forwardings to have a lifetime set. This value can be 0 but the recommendation is 7200 seconds (2 hours) and the client is expected to renew it in time. https://tools.ietf.org/rfc/rfc6886.txt

@frennkie

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2018

I (significantly) simplified this PR. Now all this does is to re-create the port forwardings after a change of the external IP has been detected.

The u.forwardedPorts[port]; exists check assumes that it has knowledge about the actual status of the port forwarding on the NAT device. But (at least for me) this is not the case as all UPnP forwardings are removed when the external IP changes.

Therefore I removed the check. Should the forwardings still be in place then re-creating them doesn't actually do any harm. This is even the recommended behavior described in the PCP RfC.

PR has been tested and is working on my node.

@halseth halseth added P3 and removed P2 labels Nov 28, 2018

@wpaulino
Copy link
Collaborator

left a comment

Confirmed that the additional forwarding attempts are not an issue. LGTM after squashing the commits into two: one for the server and another for the nat package.

Show resolved Hide resolved server.go Outdated

@Roasbeef Roasbeef added this to Blocked in High Priority Dec 18, 2018

@frennkie frennkie force-pushed the frennkie:re-create-port-mappings branch from f78e209 to 6c795a7 Jan 4, 2019

@frennkie

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2019

@wpaulino Thanks for your support. I now now squashed this PR as requested.

@wpaulino
Copy link
Collaborator

left a comment

LGTM ⚡️

@frennkie

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2019

Damn... I applied this PR, compiled it and deployed it to my Lightning node on Friday. On Saturday morning it was ok, this morning it was not ok (no UPNP port forwardings on my router). I re-connected my internet line and lnd successfully re-created the forwards.

I checked the logs and there seems to be a race condition.

The normal flow is

  1. ISP disconnects line
  2. Router reconnects
  3. Router cleans port forwardings
  4. lnd notices the new IP and re-creates the port forwardings (this runs every 15 minutes)

According to the logs what happened this morning was:

  1. ISP disconnects line
  2. Router reconnects
  3. lnd notices the new IP and re-creates the port forwardings (this runs every 15 minutes)
  4. Router cleans port forwardings

My guess is that the Router either also has a scheduled job or that it waited with cleaning the port forwardings because there were still connections in a local NAT table.

I see two solutions:

  • A. Always schedule a 2nd re-create task (may be 5-10 minutes) later when a new IP is detected.
  • B. Always re-create the forwardings (even if the IP hasn't changed.

My suggestions would be option B. Because:

  • Simple
  • There is not much load/traffic (~40 packets with 4Kbytes in total)
  • This also catches other situations when the NAT devices loses the port forwardings (but may still have the same external IP)
  • This would comply with the NAT-PMP RFC6886

@frennkie frennkie force-pushed the frennkie:re-create-port-mappings branch from 6c795a7 to a9f1080 Jan 15, 2019

High Priority automation moved this from Blocked to Needs review Jan 15, 2019

@frennkie

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

@wpaulino I implemented the change suggested as option B. last week and it's been running stable since. I just force pushed this into this PR.

With this change lnd (configuration nat=true) will now try to recreate/refresh the port forwarding for port 9735 every 15 minutes.

Without this my connection was completely unreliable - lnd losing inbound connectivity every other day.

@Roasbeef Roasbeef removed this from the 0.5.2 milestone Jan 16, 2019

@frennkie

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2019

Is there anything I can or have to do to get this merged?

@halseth

This comment has been minimized.

Copy link
Collaborator

commented Feb 19, 2019

@frennkie A bit short on review cycles atm!

@frennkie

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

@halseth @Roasbeef Anything I can do to help get this into master? I don't really enjoy building my own patched lnd binary everytime you do a new (beta) release.

@halseth
Copy link
Collaborator

left a comment

Seems simple enough :) A few comments.

Show resolved Hide resolved server.go Outdated
Show resolved Hide resolved server.go Outdated
Show resolved Hide resolved server.go Outdated
Show resolved Hide resolved server.go Outdated
@frennkie

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

@halseth I made the requested changes. Let me know when/if I should do another squash. I squashed all commits into one now.

server: add periodic renewal of port forwarding
The check prevented the creation of port forwardings which were assumed
to be present already. After this change the port forwardings which
might have been removed from the NAT device can be re-created.

@frennkie frennkie force-pushed the frennkie:re-create-port-mappings branch from e92e073 to a27ac66 Apr 13, 2019

@halseth
Copy link
Collaborator

left a comment

LGTM :)

@halseth halseth requested a review from wpaulino Apr 15, 2019

High Priority automation moved this from Needs review to Final Testing -- Ready For Merge Apr 19, 2019

@wpaulino wpaulino merged commit 5173ef6 into lightningnetwork:master Apr 19, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on re-create-port-mappings at 59.798%
Details

High Priority automation moved this from Final Testing -- Ready For Merge to Done Apr 19, 2019

@frennkie frennkie referenced this pull request Apr 20, 2019

Closed

NAT=true not working #2008

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.