-
Notifications
You must be signed in to change notification settings - Fork 76
Event for user's NAT Device Type: Tell user if the node is behind an Easy or Hard NAT #173
Conversation
network/nattype.go
Outdated
// NATDeviceTypeUnknown indicates that the type of the NAT device is unknown. | ||
NATDeviceTypeUnknown NATDeviceType = iota | ||
|
||
// NATDeviceTypeEasy indicates that the NAT device is an Easy NAT i.e. it supports consistent endpoint translation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we provide the precise definitions of
(a) the heuristic used to determine this
and
(b) what we mean by 'consistent endpoint translation' - do you mean that the externally bound host:port is the same for all remote hosts, stable for different ports of a single remote host, or once mapped allows inbound from a specifically negotiated remote host?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the docs to use the terms used in scientific literature and explain consistent endpoint translation in relation to those terms.
package network | ||
|
||
// NATDeviceType indicates the type of the NAT device i.e. whether it is a Hard or an Easy NAT. | ||
type NATDeviceType int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to have this definition of nat type be merged with the existing network.Reachability
this seems to be a sub-division of what's currently used for network.ReachabilityPrivate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that we need this event on a per transport protocol basis, it'll be extremely tricky to combine this with the Reachability
event. I've updated the docs to inform the users that they should ALSO consume the Reachability event and use this NAT type event ONLY if Reachability=Private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does "consistent endpoint translation" mean the following: For connection originating from IP:port A
, the NAT will always use B
as the source 2-tuple on outgoing packets?
That would also mean that if you dial form A'
, you'd get a different address B'
, i.e. users have to use SO_REUSEPORT / SO_REUSEADDR in TCP / multiplex connections in QUIC, right?
event/nattype.go
Outdated
NATDeviceUDP NATDeviceProtocol = iota | ||
// NATDeviceTCP means that the NAT Device Type has been determined for the TCP Protocol. | ||
NATDeviceTCP | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this in package event
, but NATDeviceType
is in network
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because event. NATDeviceType
dosen't make sense. We might have references to just the NATDeviceType in the code elsewhere and storing it as network.NATDeviceType
is more readable.
We do something similar with network.Reachability
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. But what about network.NATTransportProtocol
?
Not sure about our package structure here tbh, just asking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On further thought, I agree with you. Have fixed this by moving the Transport Protocol to the network
package as well.
network/nattype.go
Outdated
) | ||
|
||
func (r NATDeviceType) String() string { | ||
str := [...]string{"Unknown", "Easy", "Hard"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allocates, doesn't it? A switch
statement would be free in terms of memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Are "Hard NAT" and "Easy NAT" the terms used in the scientific literature regarding NATs? |
I've updated the PR to use the NAT terms used in scientific literature and have made the documentation juicier to explain what those NATs mean and how they relate to hole punching. Please let me know what you think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation is A LOT better now!
network/nattype.go
Outdated
// to the same IP address and port irrespective of the destination address. | ||
// With Regards to Internet Society RFC 3489, this could be either a Full Cone NAT, a Restricted Cone NAT or a | ||
// Port Restricted Cone NAT. However, we do NOT differentiate between them here and simply classify all such NATs as a Cone NAT. | ||
// NAT traversal with hole punching is possible with a Cone NAT if the remote peer is ALSO behind a Cone NAT. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// NAT traversal with hole punching is possible with a Cone NAT if the remote peer is ALSO behind a Cone NAT. | |
// NAT traversal with hole punching is possible with a Cone NAT, even if the remote peer is also behind a Cone NAT. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the comment to reflect that hole punching will work ONLY if the remote peer is also behind a Cone NAT and fail if the remote peer is behind a Symmetric NAT.
I've addressed all your comments. Thanks ! |
Hey @marten-seemann Please approve if happy ! |
cc @vyzo for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a small objection on the name of the event.
Co-authored-by: Marten Seemann <[email protected]>
Co-authored-by: Marten Seemann <[email protected]>
Co-authored-by: Marten Seemann <[email protected]>
7f4262e
to
de360ba
Compare
@vyzo Please can you approve the PR ? |
For libp2p/go-libp2p#1039.
Emit event to inform user of the NAT Device Type wrt BAT traversal i.e. Easy NAT(supports consistent endpoint translation) or Hard NAT(does NOT support consistent endpoint translation).