NAT Auto Discovery #1

vyzo · 2018-05-06T08:25:33Z

Provides an ambient NAT auto-discovery service.

TBD:

doscstrings
README
tests
CI

fix bug introduced in last commit

connection state can be determined through the network interface on demand

addresses service TODO

Kubuxu · 2018-05-07T08:59:35Z

svc.go

+		log.Debugf("error dialing %s: %s", pi.ID.Pretty(), err.Error())
+		// wait for the context to timeout to avoid leaking timing information
+		// this renders the service ineffective as a port scanner
+		select {


You don't have to select here.
<-ctx.Done() is enough.

yes, indeed; habbit.

Kubuxu · 2018-05-07T09:02:35Z

autonat.go

+type NATStatus int
+
+const (
+	NATStatusUnknown = iota


I would give those constants explicit type of NATStatus.

sure, will do.

Kubuxu · 2018-05-07T09:16:12Z

autonat.go

+func (as *AutoNATState) background() {
+	// wait a bit for the node to come online and establish some connections
+	// before starting autodetection
+	time.Sleep(10 * time.Second)


10s might be a bit short here. Automatic port forwarding has about 10s timeout.
From another hand, if it is timing out we won't get and forwarding either way.

For testing, it might be useful for it to be configurable.

We can bump it a bit, say to 15s.

Re: testing
I don't think it will be useful, as we won't be able to unit-test this aspect with go test.
I plan to run some test programs and try it in the wild.

Then again we probably do want to test the autodiscovery without waiting for this time, so maybe we can make it a variable.

We can revisit when I have the test suite ready.

whyrusleeping · 2018-05-08T09:52:25Z

autonat.go

+func (as *AutoNATState) background() {
+	// wait a bit for the node to come online and establish some connections
+	// before starting autodetection
+	time.Sleep(15 * time.Second)


I would pull these durations out into named constants at least.

ok, will do.

Lifted them and turned them into variables, per @Kubuxu's suggestion in 6d4bc41

whyrusleeping · 2018-05-08T12:11:11Z

autonat.go

+	PublicAddr() (ma.Multiaddr, error)
+}
+
+type AutoNATState struct {


the naming here is a bit weird. Its not clear from reading that AutoNATState is an implementation of AutoNAT

hrm, good point. Easy to fix, should I call it AutoNATImpl? boring.

I like AmbientAutoNAT:

it conveys that it is an AutoNAT instance

it conveys that it is ambient, meaning that the user doesn't have anything to do other than creating an instance.

renamed to AmbientAutoNAT in 1562e1b

whyrusleeping · 2018-05-08T16:54:33Z

autonat.go

+	shufflePeers(peers)
+
+	for _, p := range peers {
+		cli := NewAutoNATClient(as.host, p)


creating a single use thingy here feels a bit weird. Maybe just make the dial method take the host and peer?

we can also reuse the client objects.

on the other hand it's a very simple object, with no state. Not sure it's worth the trouble to cache it.

It might make sense to make it take a peer so that we can reuse the object for all peers in the interaction; no need to hold it across function calls i think.

fixed in bb5cad4

The client object is reused for dialing all peers as needed.

whyrusleeping · 2018-05-08T16:55:00Z

autonat.go

+
+	for _, p := range peers {
+		cli := NewAutoNATClient(as.host, p)
+		ctx, cancel := context.WithTimeout(as.ctx, 60*time.Second)


magic number

we can give a name to the incantation.

named it and made it a variable so that we can unit test; fa14117

whyrusleeping · 2018-05-08T16:55:28Z

autonat.go

+		return
+	}
+
+	as.mx.Lock()


probably want to put this locked section into a separate method so we can use defers nicely

sure, will do.

done in d16ca79

svc.go

Stebalien

This was a lot simpler than I thought it would be. Nice!

Stebalien · 2018-09-17T23:28:04Z

autonat.go

+func (as *AmbientAutoNAT) background() {
+	// wait a bit for the node to come online and establish some connections
+	// before starting autodetection
+	time.Sleep(AutoNATBootDelay)


Not that important but this should probably select in a context and a time.After.

Stebalien · 2018-09-17T23:30:02Z

autonat.go

+
+	peers := make([]peer.ID, 0, len(as.peers))
+	for p := range as.peers {
+		if len(as.host.Network().ConnsToPeer(p)) > 0 {


Nit: Network().Connectedness() == inet.Connected avoids allocating.

Stebalien · 2018-09-28T23:52:19Z

autonat.go

+	// NAT status is publicly dialable
+	NATStatusPublic
+	// NAT status is private network
+	NATStatusPrivate


What about "no nat"? Do we need that state?

What does that state mean though? We have Uknown and Public -- no nat is equivalent to public.

Ah, I was thinking:

NatStatusPrivate -> Behind a nat and undiablable.

NatStatusPublic -> Behind a nat and dialable.

(although we may not need to track the undialable case.

It's more of "dialable" or not "dialable".
Do we gain anything by knowing that there is no NAT whatsoever?
Note that the inference might be hard to make.

Stebalien · 2018-09-28T23:56:00Z

client.go

+// AutoNATClient is a stateless client interface to AutoNAT peers
+type AutoNATClient interface {
+	// Dial requests from a peer providing AutoNAT services to test dial back
+	Dial(ctx context.Context, p peer.ID) (ma.Multiaddr, error)


Can we call this something else? When I see "Dial" I think "establish a connection". When I saw this function used in the code, I had absolutely no idea why dialing a peer would tell us anything about our NAT status.

Ok, will rename.

Called it DialBack.

svc.go

Stebalien · 2018-09-29T00:05:47Z

svc.go

+		log.Debugf("error dialing %s: %s", pi.ID.Pretty(), err.Error())
+		// wait for the context to timeout to avoid leaking timing information
+		// this renders the service ineffective as a port scanner
+		<-ctx.Done()


Stebalien · 2018-09-29T00:07:33Z

svc.go

+	}
+
+	ra := conns[0].RemoteMultiaddr()
+	conns[0].Close()


Probably better to call as.dialer.Network.ClosePeer. We can, unfortunately, successfully open multiple connections.

Stebalien · 2018-09-29T00:08:35Z

svc.go

+// NewAutoNATService creates a new AutoNATService instance attached to a host
+func NewAutoNATService(ctx context.Context, h host.Host, opts ...libp2p.Option) (*AutoNATService, error) {
+	opts = append(opts, libp2p.NoListenAddrs)
+	dialer, err := libp2p.New(ctx, opts...)


I wonder if we could configure libp2p with a custom connection manager that just kills all connections after 30s. Just to be extra safe :wink.

Stebalien · 2018-09-29T01:20:01Z

notify.go

+func (as *AmbientAutoNAT) OpenedStream(net inet.Network, s inet.Stream) {}
+func (as *AmbientAutoNAT) ClosedStream(net inet.Network, s inet.Stream) {}
+
+func (as *AmbientAutoNAT) Connected(net inet.Network, c inet.Conn) {


So, we really don't need a large set of peers that support this protocol. Instead of testing every one, How about we:

a. Keep a list of known autonat peers (discovered as we try to use them, not when we first connect).
b. Keep a list of known non-autonat peers.

Then, periodically*, we can:

Try every connected peer in the known good set.

Try every open connection not in the bad set, adding peers to the good set and bad set as we try them.

That way we aren't unnecessarily noisy.

*later, we can get even fancier and set the period to be "time since last inbound connection from a public address", or something like that.

I think we can reduce the noise by simply checking on the protocols reported by identify.

I changed it to look at the protocols reported by identify through the peerstore in 46d352f
I don't quite like the delay for identify, but it seems to be necessary.

I don't quite like the delay for identify, but it seems to be necessary.

Yeah... that annoys me to me to no end as well.

Identify keeps the stream open after identification. If we made it close the stream, we could hook onto the ClosedStream event to know when identify was over deterministically.

Let's really not. I know it should work, but that's just horrible.

It is, but that's all we have now 😅

Should we think about setting up some kind of "in-mem event bus" so that different layers of libp2p can emit and react to events? Identify would then emit a protocols:identify/1.0.0:complete event when it finished, or a ...:error one if it failed.

Yeah... Ideally services would just hook into identify (or the peerstore? that doesn't seem right) and get called when we connect to a peer supporting protocol X.

Made the delay configurable, with an initial value of 5 sec (per @magik6k's suggestion)

…tion

Stebalien · 2018-10-02T20:07:19Z

notify.go

+
+	go func() {
+		// add some delay for identify
+		time.Sleep(250 * time.Millisecond)


Why can't we just walk over each connected peer and check lazily? That way, we don't even need this check.

I don't think we gain anything by the lazy check.
We'll have to iterate through the list of peers, which also adds complexity for recognizing new autonat servers after we have iterated once.
So we might as well do it at connect time, the goroutine cost is marginal.

I'm not worried about the goroutine cost, I'm worried about the race. However, this is probably fine as an initial implementation.

The race is probably benign, as we are just collecting peer IDs of peers that implement the protocol.

Yeah, I'm just worried that if the peer supported AutoNAT –but their Identify took longer than 250ms– we will have missed that peer forever, no?

We can increase a tad... but how much is too much?

raulk · 2018-10-03T15:22:35Z

autonat.go

+		}
+	}
+
+	if len(peers) == 0 {


I'm afraid this black or white decision could yield erratic results, e.g. if you only have 1 active connection to an autonat peer, we're going to restrict our query to a single peer. I much rather have a minimum threshold we strive for, e.g. 5 peers, for resilience purposes, starting with peers we hold a connection to.

As it is, the autodetect does a round-robin, so no risk of establishing redundant connections if we shuffle the connected and non-connected sublists separately, i.e.

A..G: connected U..Z: not connected A B C D E F G || U V W X Y Z <- shuffle -> <- shuffle ->

I am not sure I follow. The code tries to use an already existing connection purely to avoid creating unnecessary new connections.
Do you want to try multiple peers? And each peer multiple times?

The code tries to use an already existing connection purely to avoid creating unnecessary new connections.

Currently, if we happen to be connected to 1 autonat peer only, we'll restrict ourselves to it. If it fails, we're out of luck. This makes us fragile, especially because we expect scarcity in autonat peers.

What I'm proposing is to target N peers (e.g. 5), preferring connected peers, and falling back to disconnected ones to fill up the slice. To avoid connected and unconnected peers getting mixed up in the shuffle, we keep track of the pivot index and shuffle both sublists separately.

Since autodetect is round-robin, it'll only resort to disconnected peers if the connected ones fail. This makes us more resilient overall.

That's fine, but do we want to dial more than one peers when we get a DIAL_ERROR?

That's a good question. I'm not sure I have the answer. Right now we flag NATStatusPrivate on the first dial err, and abort. However, what if that peer is behind some kind of firewall (corporate, geographical, etc.)? Wouldn't it be better to corroborate with more observations?

I guess we could do a few more tries if we have more known autonat peers, but accept the failure if we don't have enough.
I would arbitrarily go for "3 times is enemy action".

I guess we could do a few more tries if we have more known autonat peers, but accept the failure if we don't have enough.

Yep. If we don't have enough, we'd defer to the next iteration. If by then we've found more autonat peers, with this new logic we'll query them even if not connected, and hence have a chance to improve our connectivity.

I would arbitrarily go for "3 times is enemy action".

We detect "enemy action" on the receiving side through the throttling, no? (3 is fine for that)

That makes me realise that we should probably move peers who have sent us E_DIAL_REFUSED or E_INTERNAL_ERROR to a blacklist, to avoid dwelling on them, and to be well-behaved.

That's probably too much complexity for marginal improvement :)

Also, I think I want some slightly more clever strategy for making multiple dial attempts -- if our nat status was unknown or public, then try 3 times.
If it was private, then a single failure should be enough to convince us.

Implemented the "3 times is enemy action" strategy in aadb8db, with memory of past failures so that it stops asking multiple peers once it has enough confidence we are NATed.

In d7f55b0 we ensure that we have at least 3 autonat peers in the candidate set, even when we are connected to less than that.
This uses the strategy you suggested for ordering.

raulk · 2018-10-03T15:35:43Z

notify.go

+func (as *AmbientAutoNAT) OpenedStream(net inet.Network, s inet.Stream) {}
+func (as *AmbientAutoNAT) ClosedStream(net inet.Network, s inet.Stream) {}
+
+func (as *AmbientAutoNAT) Connected(net inet.Network, c inet.Conn) {


Identify keeps the stream open after identification. If we made it close the stream, we could hook onto the ClosedStream event to know when identify was over deterministically.

raulk · 2018-10-03T15:48:52Z

svc.go

+	// rate limit check
+	as.mx.Lock()
+	_, ok := as.peers[pi.ID]
+	if ok {


Two comments:

Perhaps this n=1 limiting is too optimistic. The first attempt could've failed due to an intermittent issue, not because the address is unroutable. How about allowing n=3 (or configurable) grace attempts per peer?

It's confusing that AutoNATService and AmbientAutoNAT both have a peers field, and they're both referred to by as.peers in code. I suggest renaming this one to dialedPeers to reduce ambiguity.

On a related note, both peers maps are never cleaned, so they'll keep growing forever. Any thoughts on adding a cleanup routine?

On a related note, both peers maps are never cleaned, so they'll keep growing forever. Any thoughts on adding a cleanup routine?

The number of autonat peers is expected to be relatively small for the network size and we'd like to know about our peers even if we are not connected to them any more.
If this becomes a problem, we can add some cleanup logic.

How about allowing n=3 (or configurable) grace attempts per peer?

ok, although we should probably have some backoff.

It's confusing that AutoNATService and AmbientAutoNAT both have a peers field, and they're both referred to by as.peers in code.

They serve different purposes, but they are both sequences of peer ids -- hence the name.

We could add a comment in the struct though.

Implemented configurable throttle in 8ea9f1b

They serve different purposes, but they are both sequences of peer ids -- hence the name.

It took me several reads to understand that as.peers referred to different things in the same package. Let's be nice to future maintainers and rename one of them to contextualise it.

ok, fine. I'll be nice to future self and call the limiter ~~requests~~ reqs.

3 times is enemy action; this is more resilient to transient dial errors.

magik6k

Looks good for the most part, there are some points in other discussions which seem to need adressing

autonat.go

notify.go

magik6k · 2018-10-16T09:09:06Z

package.json

+      "hash": "QmPL3AKtiaQyYpchZceXBZhZ3MSnoGqJvLZrc7fzDTTQdJ",
+      "name": "go-libp2p",
+      "version": "6.0.19"
+    }


Seems to be missing a few deps, but since this needs to be split anyways I guess it's fine

yeah, the rationale was that all the deps get pulled by go-libp2p, but it will be split and not depend on that at all.

Extract service implementation from go-libp2p-autonat

vyzo added 5 commits May 5, 2018 16:12

protobuf

32e8ab9

basic client

70f7dd8

add E_BAD_REQUEST to protobuf

f3d9a24

service implementation

cc058d6

NAT autodetection

ef097b5

vyzo requested a review from whyrusleeping May 6, 2018 08:25

vyzo added 8 commits May 6, 2018 11:28

remove left-over design notes

6efad8f

don't delete autonat peers on disconnect, just mark them as disconnected

2aa66e5

we only track autonat peers

ea43bf5

fix bug introduced in last commit

fix autonat peer tracking

00fb7e7

connection state can be determined through the network interface on demand

add TODO in service about skipping private network addresses

0377627

add network notifee

7fad996

don't try to dial private network addresses

9af8715

addresses service TODO

add localhost to private addr ranges

bc41c7a

Kubuxu reviewed May 7, 2018

View reviewed changes

vyzo added 2 commits May 7, 2018 12:03

no need to select; it's a one shot sync

dcbcfce

typed NATStatus constants

9efd0ec

Kubuxu reviewed May 7, 2018

View reviewed changes

bump initial autodiscovery delay to 15s

aaaa90e

whyrusleeping reviewed May 8, 2018

View reviewed changes

vyzo added 2 commits May 8, 2018 16:11

AutoNATState is AmbientAutoNAT

1562e1b

variables for background delays

6d4bc41

vyzo mentioned this pull request May 8, 2018

NAT Autodetection ipfs/kubo#4993

Closed

whyrusleeping reviewed May 8, 2018

View reviewed changes

svc.go Show resolved Hide resolved

named magic number incantations

fa14117

vyzo mentioned this pull request Sep 13, 2018

Integrate nat testing tool into ipfs daemon ipfs/kubo#4352

Open

Stebalien self-requested a review September 15, 2018 05:46

raulk self-requested a review September 26, 2018 22:52

Stebalien reviewed Sep 29, 2018

View reviewed changes

vyzo added 2 commits September 29, 2018 11:09

address review comments

0fdf1b0

use the protocol list by identify, don't emit chatter on every connec…

46d352f

…tion

Stebalien reviewed Oct 2, 2018

View reviewed changes

vyzo added 3 commits October 3, 2018 16:15

add observed address to the dialback set

0a4e215

ensure test hosts are only on loopback

91c209c

add /libp2p prefix in protocol string

00d2fea

raulk reviewed Oct 3, 2018

View reviewed changes

vyzo added 4 commits October 3, 2018 19:41

configurable throttle for service rate limiter

8ea9f1b

call AutoNATService.peers something else (reqs)

d9a0d1a

use more peer dial errors for increased confidence in NATPrivate state

aadb8db

3 times is enemy action; this is more resilient to transient dial errors.

use more peers if less than 3 are connected

d7f55b0

raulk mentioned this pull request Oct 4, 2018

Segregate routable and non-routable networks libp2p/go-libp2p#436

Open

vyzo added 6 commits October 4, 2018 16:57

adjust AutoNATRetryInterval in autonat tests

852f4e0

increase identify delay to 500ms

9c8ee52

jenkins file

b2c65b0

add README

8d2e2ae

reduce depgraph to just go-libp2p, update to 6.0.19

9ef3734

Add configurable Identify delay, with default value of 5 secs

6a3a9cb

magik6k reviewed Oct 16, 2018

View reviewed changes

add docstring for confidence

67bccae

magik6k approved these changes Oct 16, 2018

View reviewed changes

vyzo merged commit 35a0832 into master Oct 16, 2018

ghost removed the status/in-progress In progress label Oct 16, 2018

vyzo deleted the implementation branch October 16, 2018 10:39

willscott pushed a commit that referenced this pull request Mar 13, 2020

Merge pull request #1 from libp2p/implementation

1e10777

Extract service implementation from go-libp2p-autonat

NAT Auto Discovery #1

NAT Auto Discovery #1

Conversation

vyzo commented May 6, 2018 • edited Loading

Kubuxu May 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo May 7, 2018 • edited Loading

Choose a reason for hiding this comment

vyzo May 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulk Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulk Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulk Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulk Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo commented May 6, 2018 •

edited

Loading

Kubuxu May 7, 2018 •

edited

Loading

vyzo May 7, 2018 •

edited

Loading

vyzo May 7, 2018 •

edited

Loading

raulk Oct 3, 2018 •

edited

Loading

raulk Oct 5, 2018 •

edited

Loading

raulk Oct 3, 2018 •

edited

Loading

raulk Oct 3, 2018 •

edited

Loading