Make netcheck handle v6-only interfaces better, faster.

Also:

* add -verbose flag to cmd/tailscale netcheck
* remove some API from the interfaces package
* convert some of the interfaces package to netaddr.IP
* don't even send IPv4 probes on machines with no IPv4 (or only v4
  loopback)
* and once three regions have replied, stop waiting for other probes
  at 2x the slowest duration.

Updates #376

cmd/tailscale/netcheck.go

@@ -31,6 +31,7 @@ var netcheckCmd = &ffcli.Command{
 		fs := flag.NewFlagSet("netcheck", flag.ExitOnError)
 		fs.StringVar(&netcheckArgs.format, "format", "", `output format; empty (for human-readable), "json" or "json-line"`)
 		fs.DurationVar(&netcheckArgs.every, "every", 0, "if non-zero, do an incremental report with the given frequency")
+		fs.BoolVar(&netcheckArgs.verbose, "verbose", false, "verbose logs")
 		return fs
 	})(),
 }
@@ -38,20 +39,28 @@ var netcheckCmd = &ffcli.Command{
 var netcheckArgs struct {
 	format  string
 	every   time.Duration
+	verbose bool
 }
 
 func runNetcheck(ctx context.Context, args []string) error {
 	c := &netcheck.Client{
-		Logf:     logger.WithPrefix(log.Printf, "netcheck: "),
 		DNSCache: dnscache.Get(),
 	}
-	if netcheckArgs.every != 0 {
+	if netcheckArgs.verbose {
+		c.Logf = logger.WithPrefix(log.Printf, "netcheck: ")
+		c.Verbose = true
+	} else {
 		c.Logf = logger.Discard
 	}
 
 	dm := derpmap.Prod()
 	for {
+		t0 := time.Now()
 		report, err := c.GetReport(ctx, dm)
+		d := time.Since(t0)
+		if netcheckArgs.verbose {
+			c.Logf("GetReport took %v; err=%v", d.Round(time.Millisecond), err)
+		}
 		if err != nil {
 			log.Fatalf("netcheck: %v", err)
 		}

net/interfaces/interfaces.go

@@ -10,6 +10,8 @@ import (
 	"net"
 	"reflect"
 	"strings"
+
+	"inet.af/netaddr"
 )
 
 // Tailscale returns the current machine's Tailscale interface, if any.
@@ -37,39 +39,6 @@ func Tailscale() (net.IP, *net.Interface, error) {
 	return nil, nil, nil
 }
 
-// HaveIPv6GlobalAddress reports whether the machine appears to have a
-// global scope unicast IPv6 address.
-//
-// It only returns an error if there's a problem querying the system
-// interfaces.
-func HaveIPv6GlobalAddress() (bool, error) {
-	ifs, err := net.Interfaces()
-	if err != nil {
-		return false, err
-	}
-	for i := range ifs {
-		iface := &ifs[i]
-		if !isUp(iface) || isLoopback(iface) {
-			continue
-		}
-		addrs, err := iface.Addrs()
-		if err != nil {
-			continue
-		}
-		for _, a := range addrs {
-			ipnet, ok := a.(*net.IPNet)
-			if !ok {
-				continue
-			}
-			if ipnet.IP.To4() != nil || !ipnet.IP.IsGlobalUnicast() {
-				continue
-			}
-			return true, nil
-		}
-	}
-	return false, nil
-}
-
 // maybeTailscaleInterfaceName reports whether s is an interface
 // name that might be used by Tailscale.
 func maybeTailscaleInterfaceName(s string) bool {
@@ -82,7 +51,8 @@ func maybeTailscaleInterfaceName(s string) bool {
 // IsTailscaleIP reports whether ip is an IP in a range used by
 // Tailscale virtual network interfaces.
 func IsTailscaleIP(ip net.IP) bool {
-	return cgNAT.Contains(ip)
+	nip, _ := netaddr.FromStdIP(ip) // TODO: push this up to caller, change func signature
+	return cgNAT.Contains(nip)
 }
 
 func isUp(nif *net.Interface) bool       { return nif.Flags&net.FlagUp != 0 }
@@ -111,10 +81,13 @@ func LocalAddresses() (regular, loopback []string, err error) {
 		for _, a := range addrs {
 			switch v := a.(type) {
 			case *net.IPNet:
+				ip, ok := netaddr.FromStdIP(v.IP)
+				if !ok {
+					continue
+				}
+				if ip.Is6() {
 					// TODO(crawshaw): IPv6 support.
 					// Easy to do here, but we need good endpoint ordering logic.
-				ip := v.IP.To4()
-				if ip == nil {
 					continue
 				}
 				// TODO(apenwarr): don't special case cgNAT.
@@ -148,7 +121,7 @@ func (i Interface) IsLoopback() bool { return isLoopback(i.Interface) }
 func (i Interface) IsUp() bool       { return isUp(i.Interface) }
 
 // ForeachInterfaceAddress calls fn for each interface's address on the machine.
-func ForeachInterfaceAddress(fn func(Interface, net.IP)) error {
+func ForeachInterfaceAddress(fn func(Interface, netaddr.IP)) error {
 	ifaces, err := net.Interfaces()
 	if err != nil {
 		return err
@@ -162,7 +135,9 @@ func ForeachInterfaceAddress(fn func(Interface, net.IP)) error {
 		for _, a := range addrs {
 			switch v := a.(type) {
 			case *net.IPNet:
-				fn(Interface{iface}, v.IP)
+				if ip, ok := netaddr.FromStdIP(v.IP); ok {
+					fn(Interface{iface}, ip)
+				}
 			}
 		}
 	}
@@ -173,9 +148,16 @@ func ForeachInterfaceAddress(fn func(Interface, net.IP)) error {
 // routing table, and other network configuration.
 // For now it's pretty basic.
 type State struct {
-	InterfaceIPs map[string][]net.IP
+	InterfaceIPs map[string][]netaddr.IP
 	InterfaceUp  map[string]bool
 
+	// HaveV6Global is whether this machine has an IPv6 global address
+	// on some interface.
+	HaveV6Global bool
+
+	// HaveV4 is whether the machine has some non-localhost IPv4 address.
+	HaveV4 bool
+
 	// IsExpensive is whether the current network interface is
 	// considered "expensive", which currently means LTE/etc
 	// instead of Wifi. This field is not populated by GetState.
@@ -204,12 +186,14 @@ func (s *State) RemoveTailscaleInterfaces() {
 // It does not set the returned State.IsExpensive. The caller can populate that.
 func GetState() (*State, error) {
 	s := &State{
-		InterfaceIPs: make(map[string][]net.IP),
+		InterfaceIPs: make(map[string][]netaddr.IP),
 		InterfaceUp:  make(map[string]bool),
 	}
-	if err := ForeachInterfaceAddress(func(ni Interface, ip net.IP) {
+	if err := ForeachInterfaceAddress(func(ni Interface, ip netaddr.IP) {
 		s.InterfaceIPs[ni.Name] = append(s.InterfaceIPs[ni.Name], ip)
 		s.InterfaceUp[ni.Name] = ni.IsUp()
+		s.HaveV6Global = s.HaveV6Global || isGlobalV6(ip)
+		s.HaveV4 = s.HaveV4 || (ip.Is4() && !ip.IsLoopback())
 	}); err != nil {
 		return nil, err
 	}
@@ -227,7 +211,7 @@ func HTTPOfListener(ln net.Listener) string {
 
 	var goodIP string
 	var privateIP string
-	ForeachInterfaceAddress(func(i Interface, ip net.IP) {
+	ForeachInterfaceAddress(func(i Interface, ip netaddr.IP) {
 		if isPrivateIP(ip) {
 			if privateIP == "" {
 				privateIP = ip.String()
@@ -246,16 +230,20 @@ func HTTPOfListener(ln net.Listener) string {
 
 }
 
-func isPrivateIP(ip net.IP) bool {
+func isPrivateIP(ip netaddr.IP) bool {
 	return private1.Contains(ip) || private2.Contains(ip) || private3.Contains(ip)
 }
 
-func mustCIDR(s string) *net.IPNet {
-	_, ipNet, err := net.ParseCIDR(s)
+func isGlobalV6(ip netaddr.IP) bool {
+	return v6Global1.Contains(ip)
+}
+
+func mustCIDR(s string) netaddr.IPPrefix {
+	prefix, err := netaddr.ParseIPPrefix(s)
 	if err != nil {
 		panic(err)
 	}
-	return ipNet
+	return prefix
 }
 
 var (
@@ -264,4 +252,5 @@ var (
 	private3      = mustCIDR("192.168.0.0/16")
 	cgNAT         = mustCIDR("100.64.0.0/10")
 	linkLocalIPv4 = mustCIDR("169.254.0.0/16")
+	v6Global1     = mustCIDR("2000::/3")
 )

net/netcheck/netcheck.go

@@ -74,6 +74,9 @@ type Client struct {
 	// If nil, a DNS cache is not used.
 	DNSCache *dnscache.Resolver
 
+	// Verbose enables verbose logging.
+	Verbose bool
+
 	// Logf optionally specifies where to log to.
 	// If nil, log.Printf is used.
 	Logf logger.Logf
@@ -112,6 +115,12 @@ func (c *Client) logf(format string, a ...interface{}) {
 	}
 }
 
+func (c *Client) vlogf(format string, a ...interface{}) {
+	if c.Verbose {
+		c.logf(format, a...)
+	}
+}
+
 // handleHairSTUN reports whether pkt (from src) was our magic hairpin
 // probe packet that we sent to ourselves.
 func (c *Client) handleHairSTUNLocked(pkt []byte, src *net.UDPAddr) bool {
@@ -250,11 +259,16 @@ const numIncrementalRegions = 3
 
 // makeProbePlan generates the probe plan for a DERPMap, given the most
 // recent report and whether IPv6 is configured on an interface.
-func makeProbePlan(dm *tailcfg.DERPMap, have6if bool, last *Report) (plan probePlan) {
+func makeProbePlan(dm *tailcfg.DERPMap, ifState *interfaces.State, last *Report) (plan probePlan) {
 	if last == nil || len(last.RegionLatency) == 0 {
-		return makeProbePlanInitial(dm, have6if)
+		return makeProbePlanInitial(dm, ifState)
 	}
+	have6if := ifState.HaveV6Global
+	have4if := ifState.HaveV4
 	plan = make(probePlan)
+	if !have4if && !have6if {
+		return plan
+	}
 	had4 := len(last.RegionV4Latency) > 0
 	had6 := len(last.RegionV6Latency) > 0
 	hadBoth := have6if && had4 && had6
@@ -263,7 +277,7 @@ func makeProbePlan(dm *tailcfg.DERPMap, have6if bool, last *Report) (plan probeP
 			break
 		}
 		var p4, p6 []probe
-		do4 := true
+		do4 := have4if
 		do6 := have6if
 
 		// By default, each node only gets one STUN packet sent,
@@ -317,7 +331,7 @@ func makeProbePlan(dm *tailcfg.DERPMap, have6if bool, last *Report) (plan probeP
 	return plan
 }
 
-func makeProbePlanInitial(dm *tailcfg.DERPMap, have6if bool) (plan probePlan) {
+func makeProbePlanInitial(dm *tailcfg.DERPMap, ifState *interfaces.State) (plan probePlan) {
 	plan = make(probePlan)
 
 	// initialSTUNTimeout is only 100ms because some extra retransmits
@@ -330,10 +344,10 @@ func makeProbePlanInitial(dm *tailcfg.DERPMap, have6if bool) (plan probePlan) {
 		for try := 0; try < 3; try++ {
 			n := reg.Nodes[try%len(reg.Nodes)]
 			delay := time.Duration(try) * initialSTUNTimeout
-			if nodeMight4(n) {
+			if ifState.HaveV4 && nodeMight4(n) {
 				p4 = append(p4, probe{delay: delay, node: n.Name, proto: probeIPv4})
 			}
-			if have6if && nodeMight6(n) {
+			if ifState.HaveV6Global && nodeMight6(n) {
 				p6 = append(p6, probe{delay: delay, node: n.Name, proto: probeIPv6})
 			}
 		}
@@ -416,12 +430,15 @@ type reportState struct {
 	pc4         STUNConn
 	pc6         STUNConn
 	pc4Hair     net.PacketConn
+	incremental bool // doing a lite, follow-up netcheck
+	stopProbeCh chan struct{}
 
 	mu            sync.Mutex
 	sentHairCheck bool
 	report        *Report                            // to be returned by GetReport
 	inFlight      map[stun.TxID]func(netaddr.IPPort) // called without c.mu held
 	gotEP4        string
+	timers        []*time.Timer
 }
 
 func (rs *reportState) anyUDP() bool {
@@ -472,21 +489,29 @@ func (rs *reportState) probeWouldHelp(probe probe, node *tailcfg.DERPNode) bool
 }
 
 func (rs *reportState) startHairCheckLocked(dst netaddr.IPPort) {
-	if rs.sentHairCheck {
+	if rs.sentHairCheck || rs.incremental {
 		return
 	}
 	rs.sentHairCheck = true
-	rs.pc4Hair.WriteTo(stun.Request(rs.hairTX), dst.UDPAddr())
+	ua := dst.UDPAddr()
+	rs.pc4Hair.WriteTo(stun.Request(rs.hairTX), ua)
+	rs.c.vlogf("sent haircheck to %v", ua)
 	time.AfterFunc(500*time.Millisecond, func() { close(rs.hairTimeout) })
 }
 
 func (rs *reportState) waitHairCheck(ctx context.Context) {
 	rs.mu.Lock()
 	defer rs.mu.Unlock()
+	ret := rs.report
+	if rs.incremental {
+		if rs.c.last != nil {
+			ret.HairPinning = rs.c.last.HairPinning
+		}
+		return
+	}
 	if !rs.sentHairCheck {
 		return
 	}
-	ret := rs.report
 
 	select {
 	case <-rs.gotHairSTUN:
@@ -504,6 +529,14 @@ func (rs *reportState) waitHairCheck(ctx context.Context) {
 	}
 }
 
+func (rs *reportState) stopTimers() {
+	rs.mu.Lock()
+	defer rs.mu.Unlock()
+	for _, t := range rs.timers {
+		t.Stop()
+	}
+}
+
 // addNodeLatency updates rs to note that node's latency is d. If ipp
 // is non-zero (for all but HTTPS replies), it's recorded as our UDP
 // IP:port.
@@ -520,6 +553,19 @@ func (rs *reportState) addNodeLatency(node *tailcfg.DERPNode, ipp netaddr.IPPort
 	ret.UDP = true
 	updateLatency(ret.RegionLatency, node.RegionID, d)
 
+	// Once we've heard from 3 regions, start a timer to give up
+	// on the other ones.  The timer's duration is a function of
+	// whether this is our initial full probe or an incremental
+	// one. For incremental ones, wait for the duration of the
+	// slowest region. For initial ones, double that.
+	if len(ret.RegionLatency) == 3 {
+		timeout := maxDurationValue(ret.RegionLatency)
+		if !rs.incremental {
+			timeout *= 2
+		}
+		rs.timers = append(rs.timers, time.AfterFunc(timeout, rs.stopProbes))
+	}
+
 	switch {
 	case ipp.IP.Is6():
 		updateLatency(ret.RegionV6Latency, node.RegionID, d)
@@ -543,6 +589,13 @@ func (rs *reportState) addNodeLatency(node *tailcfg.DERPNode, ipp netaddr.IPPort
 	}
 }
 
+func (rs *reportState) stopProbes() {
+	select {
+	case rs.stopProbeCh <- struct{}{}:
+	default:
+	}
+}
+
 func newReport() *Report {
 	return &Report{
 		RegionLatency:   make(map[int]time.Duration),
@@ -577,6 +630,7 @@ func (c *Client) GetReport(ctx context.Context, dm *tailcfg.DERPMap) (*Report, e
 		hairTX:      stun.NewTxID(), // random payload
 		gotHairSTUN: make(chan *net.UDPAddr, 1),
 		hairTimeout: make(chan struct{}),
+		stopProbeCh: make(chan struct{}, 1),
 	}
 	c.curState = rs
 	last := c.last
@@ -586,6 +640,7 @@ func (c *Client) GetReport(ctx context.Context, dm *tailcfg.DERPMap) (*Report, e
 		c.nextFull = false
 		c.lastFull = now
 	}
+	rs.incremental = last != nil
 	c.mu.Unlock()
 
 	defer func() {
@@ -594,9 +649,10 @@ func (c *Client) GetReport(ctx context.Context, dm *tailcfg.DERPMap) (*Report, e
 		c.curState = nil
 	}()
 
-	v6iface, err := interfaces.HaveIPv6GlobalAddress()
+	ifState, err := interfaces.GetState()
 	if err != nil {
 		c.logf("interfaces: %v", err)
+		return nil, err
 	}
 
 	// Create a UDP4 socket used for sending to our discovered IPv4 address.
@@ -619,7 +675,7 @@ func (c *Client) GetReport(ctx context.Context, dm *tailcfg.DERPMap) (*Report, e
 		go c.readPackets(ctx, u4)
 	}
 
-	if v6iface {
+	if ifState.HaveV6Global {
 		if f := c.GetSTUNConn6; f != nil {
 			rs.pc6 = f()
 		} else {
@@ -633,7 +689,7 @@ func (c *Client) GetReport(ctx context.Context, dm *tailcfg.DERPMap) (*Report, e
 		}
 	}
 
-	plan := makeProbePlan(dm, v6iface, last)
+	plan := makeProbePlan(dm, ifState, last)
 
 	wg := syncs.NewWaitGroupChan()
 	wg.Add(len(plan))
@@ -651,9 +707,13 @@ func (c *Client) GetReport(ctx context.Context, dm *tailcfg.DERPMap) (*Report, e
 	select {
 	case <-ctx.Done():
 	case <-wg.DoneChan():
+	case <-rs.stopProbeCh:
+		// Saw enough regions.
+		c.vlogf("saw enough regions; not waiting for rest")
 	}
 
 	rs.waitHairCheck(ctx)
+	rs.stopTimers()
 
 	// Try HTTPS latency check if all STUN probes failed due to UDP presumably being blocked.
 	if !rs.anyUDP() {
@@ -882,6 +942,7 @@ func (rs *reportState) runProbe(ctx context.Context, dm *tailcfg.DERPMap, probe
 	default:
 		panic("bad probe proto " + fmt.Sprint(probe.proto))
 	}
+	c.vlogf("sent to %v", addr)
 }
 
 // proto is 4 or 6
@@ -933,3 +994,12 @@ func regionHasDERPNode(r *tailcfg.DERPRegion) bool {
 	}
 	return false
 }
+
+func maxDurationValue(m map[int]time.Duration) (max time.Duration) {
+	for _, v := range m {
+		if v > max {
+			max = v
+		}
+	}
+	return max
+}

net/netcheck/netcheck_test.go

@@ -15,6 +15,7 @@ import (
 	"testing"
 	"time"
 
+	"tailscale.com/net/interfaces"
 	"tailscale.com/net/stun"
 	"tailscale.com/net/stun/stuntest"
 	"tailscale.com/tailcfg"
@@ -256,6 +257,7 @@ func TestMakeProbePlan(t *testing.T) {
 		name    string
 		dm      *tailcfg.DERPMap
 		have6if bool
+		no4     bool // no IPv4
 		last    *Report
 		want    probePlan
 	}{
@@ -371,10 +373,27 @@ func TestMakeProbePlan(t *testing.T) {
 				"region-3-v4": []probe{p("3a", 4)},
 			},
 		},
+		{
+			name:    "only_v6_initial",
+			have6if: true,
+			no4:     true,
+			dm:      basicMap,
+			want: probePlan{
+				"region-1-v6": []probe{p("1a", 6), p("1a", 6, 100*ms), p("1a", 6, 200*ms)},
+				"region-2-v6": []probe{p("2a", 6), p("2b", 6, 100*ms), p("2a", 6, 200*ms)},
+				"region-3-v6": []probe{p("3a", 6), p("3b", 6, 100*ms), p("3c", 6, 200*ms)},
+				"region-4-v6": []probe{p("4a", 6), p("4b", 6, 100*ms), p("4c", 6, 200*ms)},
+				"region-5-v6": []probe{p("5a", 6), p("5b", 6, 100*ms), p("5c", 6, 200*ms)},
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := makeProbePlan(tt.dm, tt.have6if, tt.last)
+			ifState := &interfaces.State{
+				HaveV6Global: tt.have6if,
+				HaveV4:       !tt.no4,
+			}
+			got := makeProbePlan(tt.dm, ifState, tt.last)
 			if !reflect.DeepEqual(got, tt.want) {
 				t.Errorf("unexpected plan; got:\n%v\nwant:\n%v\n", got, tt.want)
 			}