factor out dial policy (#62)

factor out dial policy

autonat.go

@@ -63,6 +63,7 @@ func New(ctx context.Context, h host.Host, options ...Option) (AutoNAT, error) {
 	var err error
 	conf := new(config)
 	conf.host = h
+	conf.dialPolicy.host = h
 
 	if err = defaults(conf); err != nil {
 		return nil, err
@@ -154,44 +155,6 @@ func ipInList(candidate ma.Multiaddr, list []ma.Multiaddr) bool {
 	return false
 }
 
-// skipDial indicates that a multiaddress isn't worth attempted dialing.
-// The same logic is used when the autonat client is considering if
-// a remote peer is worth using as a server, and when the server is
-// considering if a requested client is worth dialing back.
-func (c *config) skipDial(addr ma.Multiaddr) bool {
-	// skip relay addresses
-	_, err := addr.ValueForProtocol(ma.P_CIRCUIT)
-	if err == nil {
-		return true
-	}
-
-	if c.allowSelfDials {
-		return false
-	}
-
-	// skip private network (unroutable) addresses
-	if !manet.IsPublicAddr(addr) {
-		return true
-	}
-	candidateIP, err := manet.ToIP(addr)
-	if err != nil {
-		return true
-	}
-
-	// Skip dialing addresses we believe are the local node's
-	for _, localAddr := range c.host.Addrs() {
-		localIP, err := manet.ToIP(localAddr)
-		if err != nil {
-			continue
-		}
-		if localIP.Equal(candidateIP) {
-			return true
-		}
-	}
-
-	return false
-}
-
 func (as *AmbientAutoNAT) background() {
 	// wait a bit for the node to come online and establish some connections
 	// before starting autodetection
@@ -375,21 +338,8 @@ func (as *AmbientAutoNAT) probeNextPeer() {
 			continue
 		}
 
-		goodAddr := false
-		for _, a := range info.Addrs {
-			if !as.config.skipDial(a) {
-				goodAddr = true
-				// if a public IP of the peer is one of ours: skip the peer.
-				aIP, _ := manet.ToIP(a)
-				aHost, _ := manet.FromIP(aIP)
-				if len(manet.AddrMatch(aHost, as.host.Addrs())) > 0 {
-					goodAddr = false
-					break
-				}
-			}
-		}
-		if !goodAddr {
-			continue
+		if !as.config.dialPolicy.skipPeer(info.Addrs) {
+			addrs = append(addrs, info)
 		}
 		addrs = append(addrs, info)
 	}

autonat_test.go

@@ -55,7 +55,7 @@ func makeAutoNAT(ctx context.Context, t *testing.T, ash host.Host) (host.Host, A
 	h.Peerstore().AddAddrs(ash.ID(), ash.Addrs(), time.Minute)
 	h.Peerstore().AddProtocols(ash.ID(), AutoNATProto)
 	a, _ := New(ctx, h, WithSchedule(100*time.Millisecond, time.Second), WithoutStartupDelay())
-	a.(*AmbientAutoNAT).config.allowSelfDials = true
+	a.(*AmbientAutoNAT).config.dialPolicy.allowSelfDials = true
 	return h, a
 }
 

dialpolicy.go

+package autonat
+
+import (
+	"net"
+
+	"github.com/libp2p/go-libp2p-core/host"
+	ma "github.com/multiformats/go-multiaddr"
+	manet "github.com/multiformats/go-multiaddr-net"
+)
+
+type dialPolicy struct {
+	allowSelfDials bool
+	host           host.Host
+}
+
+// skipDial indicates that a multiaddress isn't worth attempted dialing.
+// The same logic is used when the autonat client is considering if
+// a remote peer is worth using as a server, and when the server is
+// considering if a requested client is worth dialing back.
+func (d *dialPolicy) skipDial(addr ma.Multiaddr) bool {
+	// skip relay addresses
+	_, err := addr.ValueForProtocol(ma.P_CIRCUIT)
+	if err == nil {
+		return true
+	}
+
+	if d.allowSelfDials {
+		return false
+	}
+
+	// skip private network (unroutable) addresses
+	if !manet.IsPublicAddr(addr) {
+		return true
+	}
+	candidateIP, err := manet.ToIP(addr)
+	if err != nil {
+		return true
+	}
+
+	// Skip dialing addresses we believe are the local node's
+	for _, localAddr := range d.host.Addrs() {
+		localIP, err := manet.ToIP(localAddr)
+		if err != nil {
+			continue
+		}
+		if localIP.Equal(candidateIP) {
+			return true
+		}
+	}
+
+	return false
+}
+
+// skipPeer indicates that the collection of multiaddresses representing a peer
+// isn't worth attempted dialing. Addresses are dialed individually, and while
+// individual addresses for a peer may be worth considering, there are some
+// factors, like the presence of the same public address as the local host,
+// that may make the peer undesirable to dial as a whole.
+func (d *dialPolicy) skipPeer(addrs []ma.Multiaddr) bool {
+	localAddrs := d.host.Addrs()
+	localHosts := make([]net.IP, 0)
+	for _, lAddr := range localAddrs {
+		if _, err := lAddr.ValueForProtocol(ma.P_CIRCUIT); err != nil && manet.IsPublicAddr(lAddr) {
+			lIP, err := manet.ToIP(lAddr)
+			if err != nil {
+				continue
+			}
+			localHosts = append(localHosts, lIP)
+		}
+	}
+
+	// if a public IP of the peer is one of ours: skip the peer.
+	goodPublic := false
+	for _, addr := range addrs {
+		if _, err := addr.ValueForProtocol(ma.P_CIRCUIT); err != nil && manet.IsPublicAddr(addr) {
+			aIP, err := manet.ToIP(addr)
+			if err != nil {
+				continue
+			}
+
+			for _, lIP := range localHosts {
+				if lIP.Equal(aIP) {
+					return true
+				}
+			}
+			goodPublic = true
+		}
+	}
+
+	if d.allowSelfDials {
+		return false
+	}
+
+	return !goodPublic
+}

dialpolicy_test.go

+package autonat
+
+import (
+	"context"
+	"errors"
+	"net"
+	"testing"
+
+	blankhost "github.com/libp2p/go-libp2p-blankhost"
+	"github.com/libp2p/go-libp2p-core/peer"
+	"github.com/libp2p/go-libp2p-core/transport"
+	swarmt "github.com/libp2p/go-libp2p-swarm/testing"
+	"github.com/multiformats/go-multiaddr"
+)
+
+func makeMA(a string) multiaddr.Multiaddr {
+	addr, err := multiaddr.NewMultiaddr(a)
+	if err != nil {
+		panic(err)
+	}
+	return addr
+}
+
+type mockT struct {
+	ctx  context.Context
+	addr multiaddr.Multiaddr
+}
+
+func (m *mockT) Dial(ctx context.Context, a multiaddr.Multiaddr, p peer.ID) (transport.CapableConn, error) {
+	return nil, nil
+}
+func (m *mockT) CanDial(_ multiaddr.Multiaddr) bool { return true }
+func (m *mockT) Listen(a multiaddr.Multiaddr) (transport.Listener, error) {
+	return &mockL{m.ctx, m.addr}, nil
+}
+func (m *mockT) Protocols() []int { return []int{multiaddr.P_IP4} }
+func (m *mockT) Proxy() bool      { return false }
+func (m *mockT) String() string   { return "mock-tcp-ipv4" }
+
+type mockL struct {
+	ctx  context.Context
+	addr multiaddr.Multiaddr
+}
+
+func (l *mockL) Accept() (transport.CapableConn, error) {
+	select {
+	case <-l.ctx.Done():
+	}
+	return nil, errors.New("expected in mocked test")
+}
+func (l *mockL) Close() error                   { return nil }
+func (l *mockL) Addr() net.Addr                 { return nil }
+func (l *mockL) Multiaddr() multiaddr.Multiaddr { return l.addr }
+
+func TestSkipDial(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	s := swarmt.GenSwarm(t, ctx)
+	d := dialPolicy{host: blankhost.NewBlankHost(s)}
+	if d.skipDial(makeMA("/ip4/8.8.8.8")) != false {
+		t.Fatal("failed dialing a valid public addr")
+	}
+
+	if d.skipDial(makeMA("/ip6/2607:f8b0:400a::1")) != false {
+		t.Fatal("failed dialing a valid public addr")
+	}
+
+	if d.skipDial(makeMA("/ip4/192.168.0.1")) != true {
+		t.Fatal("didn't skip dialing an internal addr")
+	}
+
+	s.AddTransport(&mockT{ctx, makeMA("/ip4/8.8.8.8")})
+	err := s.AddListenAddr(makeMA("/ip4/8.8.8.8"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if d.skipDial(makeMA("/ip4/8.8.8.8")) != true {
+		t.Fatal("failed dialing a valid host address")
+	}
+}
+
+func TestSkipPeer(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	s := swarmt.GenSwarm(t, ctx)
+	d := dialPolicy{host: blankhost.NewBlankHost(s)}
+	if d.skipPeer([]multiaddr.Multiaddr{makeMA("/ip4/8.8.8.8")}) != false {
+		t.Fatal("failed dialing a valid public addr")
+	}
+	if d.skipPeer([]multiaddr.Multiaddr{makeMA("/ip4/8.8.8.8"), makeMA("/ip4/192.168.0.1")}) != false {
+		t.Fatal("failed dialing a valid public addr")
+	}
+	if d.skipPeer([]multiaddr.Multiaddr{makeMA("/ip4/192.168.0.1")}) != true {
+		t.Fatal("succeeded with no public addr")
+	}
+
+	s.AddTransport(&mockT{ctx, makeMA("/ip4/8.8.8.8")})
+	err := s.AddListenAddr(makeMA("/ip4/8.8.8.8"))
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if d.skipPeer([]multiaddr.Multiaddr{makeMA("/ip4/8.8.8.8"), makeMA("/ip4/192.168.0.1")}) != true {
+		t.Fatal("succeeded dialing host address")
+	}
+	if d.skipPeer([]multiaddr.Multiaddr{makeMA("/ip4/8.8.8.8"), makeMA("/ip4/9.9.9.9")}) != true {
+		t.Fatal("succeeded dialing host address when other public")
+	}
+	if d.skipPeer([]multiaddr.Multiaddr{makeMA("/ip4/9.9.9.9")}) != false {
+		t.Fatal("succeeded dialing host address when other public")
+	}
+}
+
+func TestSkipLocalPeer(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	s := swarmt.GenSwarm(t, ctx)
+	d := dialPolicy{host: blankhost.NewBlankHost(s)}
+	s.AddTransport(&mockT{ctx, makeMA("/ip4/192.168.0.1")})
+	err := s.AddListenAddr(makeMA("/ip4/192.168.0.1"))
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if d.skipPeer([]multiaddr.Multiaddr{makeMA("/ip4/8.8.8.8")}) != false {
+		t.Fatal("failed dialing a valid public addr")
+	}
+	if d.skipPeer([]multiaddr.Multiaddr{makeMA("/ip4/8.8.8.8"), makeMA("/ip4/192.168.0.1")}) != false {
+		t.Fatal("failed dialing a valid public addr")
+	}
+	if d.skipPeer([]multiaddr.Multiaddr{makeMA("/ip4/192.168.0.1")}) != true {
+		t.Fatal("succeeded with no public addr")
+	}
+}

options.go

@@ -13,11 +13,10 @@ type config struct {
 	host host.Host
 
 	addressFunc       AddrFunc
+	dialPolicy        dialPolicy
 	dialer            network.Network
 	forceReachability bool
 	reachability      network.Reachability
-	// Don't try to skip dials that might be within the lan
-	allowSelfDials bool
 
 	// client
 	bootDelay       time.Duration

svc.go

@@ -110,7 +110,7 @@ func (as *autoNATService) handleDial(p peer.ID, obsaddr ma.Multiaddr, mpi *pb.Me
 
 	// add observed addr to the list of addresses to dial
 	var obsHost net.IP
-	if !as.config.skipDial(obsaddr) {
+	if !as.config.dialPolicy.skipDial(obsaddr) {
 		addrs = append(addrs, obsaddr)
 		seen[obsaddr.String()] = struct{}{}
 		obsHost, _ = manet.ToIP(obsaddr)
@@ -123,7 +123,7 @@ func (as *autoNATService) handleDial(p peer.ID, obsaddr ma.Multiaddr, mpi *pb.Me
 			continue
 		}
 
-		if as.config.skipDial(addr) {
+		if as.config.dialPolicy.skipDial(addr) {
 			continue
 		}
 

svc_test.go

@@ -20,7 +20,7 @@ func makeAutoNATConfig(ctx context.Context, t *testing.T) *config {
 	c := config{host: h, dialer: dh.Network()}
 	_ = defaults(&c)
 	c.forceReachability = true
-	c.allowSelfDials = true
+	c.dialPolicy.allowSelfDials = true
 	return &c
 }
 
@@ -47,7 +47,7 @@ func TestAutoNATServiceDialError(t *testing.T) {
 
 	c := makeAutoNATConfig(ctx, t)
 	c.dialTimeout = 1 * time.Second
-	c.allowSelfDials = false
+	c.dialPolicy.allowSelfDials = false
 	_ = makeAutoNATService(ctx, t, c)
 	hc, ac := makeAutoNATClient(ctx, t)
 	connect(t, c.host, hc)
@@ -179,7 +179,7 @@ func TestAutoNATServiceStartup(t *testing.T) {
 	h := bhost.NewBlankHost(swarmt.GenSwarm(t, ctx))
 	dh := bhost.NewBlankHost(swarmt.GenSwarm(t, ctx))
 	an, err := New(ctx, h, EnableService(dh.Network()))
-	an.(*AmbientAutoNAT).config.allowSelfDials = true
+	an.(*AmbientAutoNAT).config.dialPolicy.allowSelfDials = true
 	if err != nil {
 		t.Fatal(err)
 	}