netcheck: work behind UDP-blocked networks again, add tests

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

cmd/tailscale/netcheck.go

@@ -9,7 +9,6 @@ import (
 	"fmt"
 	"log"
 	"sort"
-	"time"
 
 	"tailscale.com/derp/derpmap"
 	"tailscale.com/net/dnscache"
@@ -17,9 +16,6 @@ import (
 )
 
 func runNetcheck(ctx context.Context, args []string) error {
-	ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
-	defer cancel()
-
 	c := &netcheck.Client{
 		DERP:     derpmap.Prod(),
 		Logf:     log.Printf,

netcheck/netcheck.go

@@ -142,7 +142,7 @@ func (c *Client) GetReport(ctx context.Context) (*Report, error) {
 	// Mask user context with ours that we guarantee to cancel so
 	// we can depend on it being closed in goroutines later.
 	// (User ctx might be context.Background, etc)
-	ctx, cancel := context.WithCancel(ctx)
+	ctx, cancel := context.WithTimeout(ctx, 3*time.Second)
 	defer cancel()
 
 	if c.DERP == nil {
@@ -222,6 +222,16 @@ func (c *Client) GetReport(ctx context.Context) (*Report, error) {
 		gotEP4          string
 		bestDerpLatency time.Duration
 	)
+	anyV6 := func() bool {
+		mu.Lock()
+		defer mu.Unlock()
+		return ret.IPv6
+	}
+	anyV4 := func() bool {
+		mu.Lock()
+		defer mu.Unlock()
+		return gotEP4 != ""
+	}
 	add := func(server, ipPort string, d time.Duration) {
 		c.logf("%s says we are %s (in %v)", server, ipPort, d)
 
@@ -331,14 +341,10 @@ func (c *Client) GetReport(ctx context.Context) (*Report, error) {
 
 	grp.Go(func() error {
 		err := s4.Run(ctx)
-		if err == nil {
-			return nil
-		}
-		mu.Lock()
-		defer mu.Unlock()
-		// If we got at least one IPv4 endpoint, treat that as
-		// good enough.
-		if gotEP4 != "" {
+		if errors.Is(err, context.DeadlineExceeded) {
+			if !anyV4() {
+				c.logf("netcheck: no IPv4 UDP STUN replies")
+			}
 			return nil
 		}
 		return err
@@ -362,12 +368,17 @@ func (c *Client) GetReport(ctx context.Context) (*Report, error) {
 		c.mu.Unlock()
 
 		grp.Go(func() error {
-			if err := s6.Run(ctx); err != nil {
-				// IPv6 seemed like it was configured, but actually failed.
-				// Just log and return a nil error.
-				c.logf("netcheck: ignoring IPv6 failure: %v", err)
+			err := s6.Run(ctx)
+			if errors.Is(err, context.DeadlineExceeded) {
+				if !anyV6() {
+					// IPv6 seemed like it was configured, but actually failed.
+					// Just log and return a nil error.
+					c.logf("netcheck: IPv6 seemed configured, but no UDP STUN replies")
+				}
+				return nil
 			}
-			return nil
+			// Otherwise must be some invalid use of Stunner.
+			return err //
 		})
 		if c.GetSTUNConn6 == nil {
 			go reader(s6, pc6)

netcheck/netcheck_test.go

@@ -5,10 +5,16 @@
 package netcheck
 
 import (
+	"context"
 	"net"
+	"reflect"
+	"strings"
 	"testing"
+	"time"
 
+	"tailscale.com/derp/derpmap"
 	"tailscale.com/stun"
+	"tailscale.com/stun/stuntest"
 )
 
 func TestHairpinSTUN(t *testing.T) {
@@ -29,3 +35,66 @@ func TestHairpinSTUN(t *testing.T) {
 		t.Fatal("expected value")
 	}
 }
+
+func TestBasic(t *testing.T) {
+	stunAddr, cleanup := stuntest.Serve(t)
+	defer cleanup()
+
+	c := &Client{
+		DERP: derpmap.NewTestWorld(stunAddr),
+		Logf: t.Logf,
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
+	defer cancel()
+
+	r, err := c.GetReport(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !r.UDP {
+		t.Error("want UDP")
+	}
+	if len(r.DERPLatency) != 1 {
+		t.Errorf("expected 1 key in DERPLatency; got %+v", r.DERPLatency)
+	}
+	if _, ok := r.DERPLatency[stunAddr]; !ok {
+		t.Errorf("expected key %q in DERPLatency; got %+v", stunAddr, r.DERPLatency)
+	}
+	if r.GlobalV4 == "" {
+		t.Error("expected GlobalV4 set")
+	}
+	if r.PreferredDERP != 1 {
+		t.Errorf("PreferredDERP = %v; want 1", r.PreferredDERP)
+	}
+}
+
+func TestWorksWhenUDPBlocked(t *testing.T) {
+	blackhole, err := net.ListenPacket("udp4", ":0")
+	if err != nil {
+		t.Fatalf("failed to open blackhole STUN listener: %v", err)
+	}
+	defer blackhole.Close()
+
+	stunAddr := blackhole.LocalAddr().String()
+	stunAddr = strings.Replace(stunAddr, "0.0.0.0:", "127.0.0.1:", 1)
+
+	c := &Client{
+		DERP: derpmap.NewTestWorld(stunAddr),
+		Logf: t.Logf,
+	}
+	ctx, cancel := context.WithTimeout(context.Background(), 250*time.Millisecond)
+	defer cancel()
+
+	r, err := c.GetReport(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+	want := &Report{
+		DERPLatency: map[string]time.Duration{},
+	}
+
+	if !reflect.DeepEqual(r, want) {
+		t.Errorf("mismatch\n got: %+v\nwant: %+v\n", r, want)
+	}
+}

stun/stuntest/stuntest.go

@@ -0,0 +1,81 @@
+// Copyright (c) 2020 Tailscale Inc & AUTHORS All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Package stuntest provides a STUN test server.
+package stuntest
+
+import (
+	"net"
+	"strings"
+	"sync"
+	"testing"
+
+	"tailscale.com/stun"
+)
+
+type stunStats struct {
+	mu       sync.Mutex
+	readIPv4 int
+	readIPv6 int
+}
+
+func Serve(t *testing.T) (addr string, cleanupFn func()) {
+	t.Helper()
+
+	// TODO(crawshaw): use stats to test re-STUN logic
+	var stats stunStats
+
+	pc, err := net.ListenPacket("udp4", ":0")
+	if err != nil {
+		t.Fatalf("failed to open STUN listener: %v", err)
+	}
+
+	stunAddr := pc.LocalAddr().String()
+	stunAddr = strings.Replace(stunAddr, "0.0.0.0:", "127.0.0.1:", 1)
+
+	doneCh := make(chan struct{})
+	go runSTUN(t, pc, &stats, doneCh)
+	return stunAddr, func() {
+		pc.Close()
+		<-doneCh
+	}
+}
+
+func runSTUN(t *testing.T, pc net.PacketConn, stats *stunStats, done chan<- struct{}) {
+	defer close(done)
+
+	var buf [64 << 10]byte
+	for {
+		n, addr, err := pc.ReadFrom(buf[:])
+		if err != nil {
+			if strings.Contains(err.Error(), "closed network connection") {
+				t.Logf("STUN server shutdown")
+				return
+			}
+			continue
+		}
+		ua := addr.(*net.UDPAddr)
+		pkt := buf[:n]
+		if !stun.Is(pkt) {
+			continue
+		}
+		txid, err := stun.ParseBindingRequest(pkt)
+		if err != nil {
+			continue
+		}
+
+		stats.mu.Lock()
+		if ua.IP.To4() != nil {
+			stats.readIPv4++
+		} else {
+			stats.readIPv6++
+		}
+		stats.mu.Unlock()
+
+		res := stun.Response(txid, ua.IP, uint16(ua.Port))
+		if _, err := pc.WriteTo(res, addr); err != nil {
+			t.Logf("STUN server write failed: %v", err)
+		}
+	}
+}

wgengine/magicsock/magicsock_test.go

@@ -15,7 +15,6 @@ import (
 	"net/http/httptest"
 	"os"
 	"strings"
-	"sync"
 	"testing"
 	"time"
 
@@ -26,7 +25,7 @@ import (
 	"tailscale.com/derp"
 	"tailscale.com/derp/derphttp"
 	"tailscale.com/derp/derpmap"
-	"tailscale.com/stun"
+	"tailscale.com/stun/stuntest"
 	"tailscale.com/types/key"
 	"tailscale.com/types/logger"
 )
@@ -40,7 +39,7 @@ func TestListen(t *testing.T) {
 		}
 	}
 
-	stunAddr, stunCleanupFn := serveSTUN(t)
+	stunAddr, stunCleanupFn := stuntest.Serve(t)
 	defer stunCleanupFn()
 
 	port := pickPort(t)
@@ -138,72 +137,6 @@ func TestPickDERPFallback(t *testing.T) {
 	}
 }
 
-type stunStats struct {
-	mu       sync.Mutex
-	readIPv4 int
-	readIPv6 int
-}
-
-func serveSTUN(t *testing.T) (addr string, cleanupFn func()) {
-	t.Helper()
-
-	// TODO(crawshaw): use stats to test re-STUN logic
-	var stats stunStats
-
-	pc, err := net.ListenPacket("udp4", ":3478")
-	if err != nil {
-		t.Fatalf("failed to open STUN listener: %v", err)
-	}
-
-	stunAddr := pc.LocalAddr().String()
-	stunAddr = strings.Replace(stunAddr, "0.0.0.0:", "127.0.0.1:", 1)
-
-	doneCh := make(chan struct{})
-	go runSTUN(t, pc, &stats, doneCh)
-	return stunAddr, func() {
-		pc.Close()
-		<-doneCh
-	}
-}
-
-func runSTUN(t *testing.T, pc net.PacketConn, stats *stunStats, done chan<- struct{}) {
-	defer close(done)
-
-	var buf [64 << 10]byte
-	for {
-		n, addr, err := pc.ReadFrom(buf[:])
-		if err != nil {
-			if strings.Contains(err.Error(), "closed network connection") {
-				t.Logf("STUN server shutdown")
-				return
-			}
-			continue
-		}
-		ua := addr.(*net.UDPAddr)
-		pkt := buf[:n]
-		if !stun.Is(pkt) {
-			continue
-		}
-		txid, err := stun.ParseBindingRequest(pkt)
-		if err != nil {
-			continue
-		}
-
-		stats.mu.Lock()
-		if ua.IP.To4() != nil {
-			stats.readIPv4++
-		} else {
-			stats.readIPv6++
-		}
-		stats.mu.Unlock()
-
-		res := stun.Response(txid, ua.IP, uint16(ua.Port))
-		if _, err := pc.WriteTo(res, addr); err != nil {
-			t.Logf("STUN server write failed: %v", err)
-		}
-	}
-}
-
 func makeConfigs(t *testing.T, ports []uint16) []wgcfg.Config {
 	t.Helper()
 
@@ -330,7 +263,7 @@ func TestTwoDevicePing(t *testing.T) {
 	derpServer, derpAddr, derpCleanupFn := runDERP(t)
 	defer derpCleanupFn()
 
-	stunAddr, stunCleanupFn := serveSTUN(t)
+	stunAddr, stunCleanupFn := stuntest.Serve(t)
 	defer stunCleanupFn()
 
 	derps := derpmap.NewTestWorldWith(&derpmap.Server{