Merge tag 'v1.88.4' into sunos-1.88

Release 1.88.4

Author: nshalman

VERSION.txt

@@ -1 +1 @@
-1.88.3
+1.88.4

wgengine/magicsock/magicsock.go

@@ -1565,6 +1565,7 @@ func (c *Conn) maybeRebindOnError(err error) {
 
 	if c.lastErrRebind.Load().Before(time.Now().Add(-5 * time.Second)) {
 		c.logf("magicsock: performing rebind due to %q", reason)
+		c.lastErrRebind.Store(time.Now())
 		c.Rebind()
 		go c.ReSTUN(reason)
 	} else {

wgengine/magicsock/magicsock_test.go

@@ -27,6 +27,7 @@ import (
 	"sync/atomic"
 	"syscall"
 	"testing"
+	"testing/synctest"
 	"time"
 	"unsafe"
 
@@ -3116,18 +3117,35 @@ func TestMaybeRebindOnError(t *testing.T) {
 	}
 
 	t.Run("no-frequent-rebind", func(t *testing.T) {
-		if runtime.GOOS != "plan9" {
-			err := fmt.Errorf("outer err: %w", syscall.EPERM)
-			conn := newTestConn(t)
-			defer conn.Close()
-			conn.lastErrRebind.Store(time.Now().Add(-1 * time.Second))
-			before := metricRebindCalls.Value()
-			conn.maybeRebindOnError(err)
-			after := metricRebindCalls.Value()
-			if before != after {
-				t.Errorf("should not rebind within 5 seconds of last")
+		synctest.Test(t, func(t *testing.T) {
+			if runtime.GOOS != "plan9" {
+				err := fmt.Errorf("outer err: %w", syscall.EPERM)
+				conn := newTestConn(t)
+				defer conn.Close()
+				lastRebindTime := time.Now().Add(-1 * time.Second)
+				conn.lastErrRebind.Store(lastRebindTime)
+				before := metricRebindCalls.Value()
+				conn.maybeRebindOnError(err)
+				after := metricRebindCalls.Value()
+				if before != after {
+					t.Errorf("should not rebind within 5 seconds of last")
+				}
+
+				// ensure that rebinds are performed and store an updated last
+				// rebind time.
+				time.Sleep(6 * time.Second)
+
+				conn.maybeRebindOnError(err)
+				newTime := conn.lastErrRebind.Load()
+				if newTime == lastRebindTime {
+					t.Errorf("expected a rebind to occur")
+				}
+				if newTime.Sub(lastRebindTime) < 5*time.Second {
+					t.Errorf("expected at least 5 seconds between %s and %s", lastRebindTime, newTime)
+				}
 			}
-		}
+
+		})
 	})
 }
 

wgengine/magicsock/relaymanager.go

@@ -758,7 +758,10 @@ func (r *relayManager) handleNewServerEndpointRunLoop(newServerEndpoint newRelay
 		ctx:          ctx,
 		cancel:       cancel,
 	}
-	if byServerDisco == nil {
+	// We must look up byServerDisco again. The previous value may have been
+	// deleted from the outer map when cleaning up duplicate work.
+	byServerDisco, ok = r.handshakeWorkByServerDiscoByEndpoint[newServerEndpoint.wlb.ep]
+	if !ok {
 		byServerDisco = make(map[key.DiscoPublic]*relayHandshakeWork)
 		r.handshakeWorkByServerDiscoByEndpoint[newServerEndpoint.wlb.ep] = byServerDisco
 	}

wgengine/magicsock/relaymanager_test.go

@@ -7,6 +7,7 @@ import (
 	"testing"
 
 	"tailscale.com/disco"
+	udprelay "tailscale.com/net/udprelay/endpoint"
 	"tailscale.com/types/key"
 	"tailscale.com/util/set"
 )
@@ -78,3 +79,41 @@ func TestRelayManagerGetServers(t *testing.T) {
 		t.Errorf("got %v != want %v", got, servers)
 	}
 }
+
+// Test for http://go/corp/32978
+func TestRelayManager_handleNewServerEndpointRunLoop(t *testing.T) {
+	rm := relayManager{}
+	rm.init()
+	<-rm.runLoopStoppedCh // prevent runLoop() from starting, we will inject/handle events in the test
+	ep := &endpoint{}
+	conn := newConn(t.Logf)
+	ep.c = conn
+	serverDisco := key.NewDisco().Public()
+	rm.handleNewServerEndpointRunLoop(newRelayServerEndpointEvent{
+		wlb: endpointWithLastBest{
+			ep: ep,
+		},
+		se: udprelay.ServerEndpoint{
+			ServerDisco: serverDisco,
+			LamportID:   1,
+			VNI:         1,
+		},
+	})
+	rm.handleNewServerEndpointRunLoop(newRelayServerEndpointEvent{
+		wlb: endpointWithLastBest{
+			ep: ep,
+		},
+		se: udprelay.ServerEndpoint{
+			ServerDisco: serverDisco,
+			LamportID:   2,
+			VNI:         2,
+		},
+	})
+	rm.stopWorkRunLoop(ep)
+	if len(rm.handshakeWorkByServerDiscoByEndpoint) != 0 ||
+		len(rm.handshakeWorkByServerDiscoVNI) != 0 ||
+		len(rm.handshakeWorkAwaitingPong) != 0 ||
+		len(rm.addrPortVNIToHandshakeWork) != 0 {
+		t.Fatal("stranded relayHandshakeWork state")
+	}
+}