nat: add HasNAT method for checking NAT environments #2346
Conversation
|
Turns out I could add tests. A mocknet would have been nicer though. |
|
I’ll review this tomorrow. |
|
I have to admit I don't understand this PR. None of the three issues listed. If these are bugs, we should have issues for them (this applies to all but the most trivial fixes). If these are separate bugs, it would be helpful to have separate PRs for them. |
|
Is this fixing a regression from v0.27? @sukunrt Can you add more context, and create an issue that would highlight the problem such that someone could find it if they run into this issue, and thus find this fix? |
| @@ -21,6 +21,7 @@ import ( | |||
| // and tries to obtain port mappings for those. | |||
| type NATManager interface { | |||
| GetMapping(ma.Multiaddr) ma.Multiaddr | |||
| HasNAT() bool | |||
There was a problem hiding this comment.
I don't think we should call this HasNAT because this checks if we can communicate with a NAT device. If we have a NAT, but are unable to communicate with it (e.g. via upnp) then, this would return false.
Maybe call it HasDiscoveredNAT instead?
|
this explains the first issue, #2357 |
|
@MarcoPolo, yes this is a regression from v0.27 |
|
Let's just merge the fix for the regression for now. |
|
|
||
| for _, addr := range resolved { | ||
| addrs = append(addrs, listen) | ||
| for _, addr := range addrs { |
There was a problem hiding this comment.
I'm a little suspicious about this code in general. Why do we trust the NAT device to give us the correct external port, but not the IP address? Or, in other words, how likely is it that the NAT device gives us the correct port mapping but the incorrect public IP address?
There was a problem hiding this comment.
This is not about the NAT giving the IP address, this is about this line returning the correct addresses for the listen address.
#2346
| @@ -904,6 +904,19 @@ func (h *BasicHost) AllAddrs() []ma.Multiaddr { | |||
| } | |||
| } | |||
| } | |||
| // Add observed addresses for umapped addresses | |||
| for _, listen := range unmapped { | |||
There was a problem hiding this comment.
This didn't exist before. What did this fix for you? This codepath gets hit in the case we discover a NAT device, but fail to register a mapping, is that correct?
There was a problem hiding this comment.
Yes. If you are unable to do a nat mapping after discovering a NAT device this code path fixes it.
|
Let's just take this commit: bed6dca for now. |
Do you want to create a separate PR? |
|
closing in favour of #2358 |
This PR fixes 1 major and 2 minor issues with address inference in NATed environments.
NATPortMapoption and we are unable to do a NAT mapping, we should do address inference using addresses obtained from observed address manager. This is the same behaviour as 0.27