diff --git a/cmd/tailscale/netcheck.go b/cmd/tailscale/netcheck.go index 93ef45423..ae5849c19 100644 --- a/cmd/tailscale/netcheck.go +++ b/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, diff --git a/netcheck/netcheck.go b/netcheck/netcheck.go index f07584027..067173b98 100644 --- a/netcheck/netcheck.go +++ b/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) diff --git a/netcheck/netcheck_test.go b/netcheck/netcheck_test.go index a28d9a974..b56a176d9 100644 --- a/netcheck/netcheck_test.go +++ b/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) + } +} diff --git a/stun/stuntest/stuntest.go b/stun/stuntest/stuntest.go new file mode 100644 index 000000000..ba244fc37 --- /dev/null +++ b/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) + } + } +} diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go index 4782c3933..8628ddedd 100644 --- a/wgengine/magicsock/magicsock_test.go +++ b/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{