wgengine/magicsock: stop retaining *netmap.NetworkMap

We're trying to start using that monster type less and eventually get
rid of it.

Updates #1909

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

3 files changed, 56 insertions, 27 deletions

diff --git a/wgengine/magicsock/debughttp.go b/wgengine/magicsock/debughttp.go
index a1f17b560..f26b50044 100644
--- a/wgengine/magicsock/debughttp.go
+++ b/wgengine/magicsock/debughttp.go
@@ -102,10 +102,9 @@ func (c *Conn) ServeHTTPDebug(w http.ResponseWriter, r *http.Request) {
 		sort.Slice(ent, func(i, j int) bool { return ent[i].pub.Less(ent[j].pub) })
 
 		peers := map[key.NodePublic]tailcfg.NodeView{}
-		if c.netMap != nil {
-			for _, p := range c.netMap.Peers {
-				peers[p.Key()] = p
-			}
+		for i := range c.peers.LenIter() {
+			p := c.peers.At(i)
+			peers[p.Key()] = p
 		}
 
 		for _, e := range ent {
diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go
index ae35cef0d..e1f6eda55 100644
--- a/wgengine/magicsock/magicsock.go
+++ b/wgengine/magicsock/magicsock.go
@@ -51,6 +51,7 @@ import (
 	"tailscale.com/types/logger"
 	"tailscale.com/types/netmap"
 	"tailscale.com/types/nettype"
+	"tailscale.com/types/views"
 	"tailscale.com/util/clientmetric"
 	"tailscale.com/util/mak"
 	"tailscale.com/util/ringbuffer"
@@ -256,14 +257,16 @@ type Conn struct {
 	// magicsock could do with any complexity reduction it can get.
 	netInfoLast *tailcfg.NetInfo
 
-	derpMap     *tailcfg.DERPMap // nil (or zero regions/nodes) means DERP is disabled
-	netMap      *netmap.NetworkMap
-	privateKey  key.NodePrivate    // WireGuard private key for this node
-	everHadKey  bool               // whether we ever had a non-zero private key
-	myDerp      int                // nearest DERP region ID; 0 means none/unknown
-	derpStarted chan struct{}      // closed on first connection to DERP; for tests & cleaner Close
-	activeDerp  map[int]activeDerp // DERP regionID -> connection to a node in that region
-	prevDerp    map[int]*syncs.WaitGroupChan
+	derpMap          *tailcfg.DERPMap              // nil (or zero regions/nodes) means DERP is disabled
+	peers            views.Slice[tailcfg.NodeView] // from last SetNetworkMap update
+	lastFlags        debugFlags                    // at time of last SetNetworkMap
+	firstAddrForTest netip.Addr                    // from last SetNetworkMap update; for tests only
+	privateKey       key.NodePrivate               // WireGuard private key for this node
+	everHadKey       bool                          // whether we ever had a non-zero private key
+	myDerp           int                           // nearest DERP region ID; 0 means none/unknown
+	derpStarted      chan struct{}                 // closed on first connection to DERP; for tests & cleaner Close
+	activeDerp       map[int]activeDerp            // DERP regionID -> connection to a node in that region
+	prevDerp         map[int]*syncs.WaitGroupChan
 
 	// derpRoute contains optional alternate routes to use as an
 	// optimization instead of contacting a peer via their home
@@ -1737,12 +1740,12 @@ func (c *Conn) UpdatePeers(newPeers set.Set[key.NodePublic]) {
 	}
 }
 
-func nodesEqual(x, y []tailcfg.NodeView) bool {
-	if len(x) != len(y) {
+func nodesEqual(x, y views.Slice[tailcfg.NodeView]) bool {
+	if x.Len() != y.Len() {
 		return false
 	}
-	for i := range x {
-		if !x[i].Equal(y[i]) {
+	for i := range x.LenIter() {
+		if !x.At(i).Equal(y.At(i)) {
 			return false
 		}
 	}
@@ -1773,6 +1776,18 @@ func debugRingBufferSize(numPeers int) int {
 	return max(defaultVal, maxRingBufferSize/(averageRingBufferElemSize*numPeers))
 }
 
+// debugFlags are the debug flags in use by the magicsock package.
+// They might be set by envknob and/or controlknob.
+// The value is comparable.
+type debugFlags struct {
+	heartbeatDisabled bool
+}
+
+func (c *Conn) debugFlagsLocked() (f debugFlags) {
+	f.heartbeatDisabled = debugEnableSilentDisco() // TODO(bradfitz): controlknobs too, later
+	return
+}
+
 // SetNetworkMap is called when the control client gets a new network
 // map from the control server. It must always be non-nil.
 //
@@ -1786,21 +1801,30 @@ func (c *Conn) SetNetworkMap(nm *netmap.NetworkMap) {
 		return
 	}
 
-	priorNetmap := c.netMap
+	priorPeers := c.peers
 	metricNumPeers.Set(int64(len(nm.Peers)))
 
 	// Update c.netMap regardless, before the following early return.
-	c.netMap = nm
+	curPeers := views.SliceOf(nm.Peers)
+	c.peers = curPeers
 
-	if priorNetmap != nil && nodesEqual(priorNetmap.Peers, nm.Peers) {
+	flags := c.debugFlagsLocked()
+	if len(nm.Addresses) > 0 {
+		c.firstAddrForTest = nm.Addresses[0].Addr()
+	} else {
+		c.firstAddrForTest = netip.Addr{}
+	}
+
+	if nodesEqual(priorPeers, curPeers) && c.lastFlags == flags {
 		// The rest of this function is all adjusting state for peers that have
 		// changed. But if the set of peers is equal and the debug flags (for
 		// silent disco) haven't changed, no need to do anything else.
 		return
 	}
 
+	c.lastFlags = flags
+
 	c.logf("[v1] magicsock: got updated network map; %d peers", len(nm.Peers))
-	heartbeatDisabled := debugEnableSilentDisco()
 
 	entriesPerBuffer := debugRingBufferSize(len(nm.Peers))
 
@@ -1845,7 +1869,7 @@ func (c *Conn) SetNetworkMap(nm *netmap.NetworkMap) {
 			if epDisco := ep.disco.Load(); epDisco != nil {
 				oldDiscoKey = epDisco.key
 			}
-			ep.updateFromNode(n, heartbeatDisabled)
+			ep.updateFromNode(n, flags.heartbeatDisabled)
 			c.peerMap.upsertEndpoint(ep, oldDiscoKey) // maybe update discokey mappings in peerMap
 			continue
 		}
@@ -1878,7 +1902,7 @@ func (c *Conn) SetNetworkMap(nm *netmap.NetworkMap) {
 			publicKeyHex:      n.Key().UntypedHexString(),
 			sentPing:          map[stun.TxID]sentPing{},
 			endpointState:     map[netip.AddrPort]*endpointState{},
-			heartbeatDisabled: heartbeatDisabled,
+			heartbeatDisabled: flags.heartbeatDisabled,
 			isWireguardOnly:   n.IsWireGuardOnly(),
 		}
 		if n.Addresses().Len() > 0 {
@@ -1898,7 +1922,7 @@ func (c *Conn) SetNetworkMap(nm *netmap.NetworkMap) {
 			c.logEndpointCreated(n)
 		}
 
-		ep.updateFromNode(n, heartbeatDisabled)
+		ep.updateFromNode(n, flags.heartbeatDisabled)
 		c.peerMap.upsertEndpoint(ep, key.DiscoPublic{})
 	}
 
diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go
index b60777d2d..7700f6ca8 100644
--- a/wgengine/magicsock/magicsock_test.go
+++ b/wgengine/magicsock/magicsock_test.go
@@ -36,9 +36,11 @@ import (
 	"golang.org/x/net/ipv4"
 	"golang.org/x/net/ipv6"
 	"tailscale.com/cmd/testwrapper/flakytest"
+	"tailscale.com/control/controlknobs"
 	"tailscale.com/derp"
 	"tailscale.com/derp/derphttp"
 	"tailscale.com/disco"
+	"tailscale.com/envknob"
 	"tailscale.com/ipn/ipnstate"
 	"tailscale.com/net/connstats"
 	"tailscale.com/net/netaddr"
@@ -244,10 +246,10 @@ func (s *magicStack) Status() *ipnstate.Status {
 func (s *magicStack) IP() netip.Addr {
 	for deadline := time.Now().Add(5 * time.Second); time.Now().Before(deadline); time.Sleep(10 * time.Millisecond) {
 		s.conn.mu.Lock()
-		nm := s.conn.netMap
+		addr := s.conn.firstAddrForTest
 		s.conn.mu.Unlock()
-		if nm != nil && len(nm.Addresses) > 0 {
-			return nm.Addresses[0].Addr()
+		if addr.IsValid() {
+			return addr
 		}
 	}
 	panic("timed out waiting for magicstack to get an IP assigned")
@@ -1941,13 +1943,17 @@ func TestRebindingUDPConn(t *testing.T) {
 // peers didn't change, but the netmap has non-peer info in it too we shouldn't discard)
 func TestSetNetworkMapWithNoPeers(t *testing.T) {
 	var c Conn
+	knobs := &controlknobs.Knobs{}
 	c.logf = logger.Discard
+	c.controlKnobs = knobs // TODO(bradfitz): move silent disco bool to controlknobs
 
 	for i := 1; i <= 3; i++ {
+		v := !debugEnableSilentDisco()
+		envknob.Setenv("TS_DEBUG_ENABLE_SILENT_DISCO", fmt.Sprint(v))
 		nm := &netmap.NetworkMap{}
 		c.SetNetworkMap(nm)
 		t.Logf("ptr %d: %p", i, nm)
-		if c.netMap != nm {
+		if c.lastFlags.heartbeatDisabled != v {
 			t.Fatalf("call %d: didn't store netmap", i)
 		}
 	}