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) } }