wgengine{,/netstack}: remove AddNetworkMapCallback from Engine interface

It had exactly one user: netstack. Just have LocalBackend notify
netstack when here's a new netmap instead, simplifying the bloated
Engine interface that has grown a bunch of non-Engine-y things.
(plenty of rando stuff remains after this, but it's a start)

Updates #cleanup

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

cmd/tailscaled/tailscaled.go

@@ -496,6 +496,7 @@ func getLocalBackend(ctx context.Context, logf logger.Logf, logID logid.PublicID
 	if err != nil {
 		return nil, fmt.Errorf("newNetstack: %w", err)
 	}
+	sys.Set(ns)
 	ns.ProcessLocalIPs = onlyNetstack
 	ns.ProcessSubnets = onlyNetstack || handleSubnetsInNetstack()
 

ipn/ipnlocal/local.go

@@ -4077,6 +4077,9 @@ func hasCapability(nm *netmap.NetworkMap, cap string) bool {
 // Tailscale is turned off.
 func (b *LocalBackend) setNetMapLocked(nm *netmap.NetworkMap) {
 	b.dialer.SetNetMap(nm)
+	if ns, ok := b.sys.Netstack.GetOK(); ok {
+		ns.UpdateNetstackIPs(nm)
+	}
 	var login string
 	if nm != nil {
 		login = cmpx.Or(nm.UserProfiles[nm.User()].LoginName, "<missing-profile>")

tsd/tsd.go

@@ -27,6 +27,7 @@
 	"tailscale.com/net/netmon"
 	"tailscale.com/net/tsdial"
 	"tailscale.com/net/tstun"
+	"tailscale.com/types/netmap"
 	"tailscale.com/wgengine"
 	"tailscale.com/wgengine/magicsock"
 	"tailscale.com/wgengine/router"
@@ -43,9 +44,17 @@ type System struct {
 	Router         SubSystem[router.Router]
 	Tun            SubSystem[*tstun.Wrapper]
 	StateStore     SubSystem[ipn.StateStore]
+	Netstack       SubSystem[NetstackImpl] // actually a *netstack.Impl
 	controlKnobs   controlknobs.Knobs
 }
 
+// NetstackImpl is the interface that *netstack.Impl implements.
+// It's an interface for circular dependency reasons: netstack.Impl
+// references LocalBackend, and LocalBackend has a tsd.System.
+type NetstackImpl interface {
+	UpdateNetstackIPs(*netmap.NetworkMap)
+}
+
 // Set is a convenience method to set a subsystem value.
 // It panics if the type is unknown or has that type
 // has already been set.
@@ -67,6 +76,8 @@ func (s *System) Set(v any) {
 		s.MagicSock.Set(v)
 	case ipn.StateStore:
 		s.StateStore.Set(v)
+	case NetstackImpl:
+		s.Netstack.Set(v)
 	default:
 		panic(fmt.Sprintf("unknown type %T", v))
 	}

tsnet/tsnet.go

@@ -513,6 +513,7 @@ func (s *Server) start() (reterr error) {
 	if err != nil {
 		return fmt.Errorf("netstack.Create: %w", err)
 	}
+	sys.Set(ns)
 	ns.ProcessLocalIPs = true
 	ns.GetTCPHandlerForFlow = s.getTCPHandlerForFlow
 	ns.GetUDPHandlerForFlow = s.getUDPHandlerForFlow

tstest/integration/integration_test.go

@@ -41,6 +41,7 @@
 	"tailscale.com/tstest/integration/testcontrol"
 	"tailscale.com/types/key"
 	"tailscale.com/types/logger"
+	"tailscale.com/util/rands"
 )
 
 var (
@@ -876,7 +877,9 @@ func newTestNode(t *testing.T, env *testEnv) *testNode {
 	dir := t.TempDir()
 	sockFile := filepath.Join(dir, "tailscale.sock")
 	if len(sockFile) >= 104 {
-		t.Fatalf("sockFile path %q (len %v) is too long, must be < 104", sockFile, len(sockFile))
+		// Maximum length for a unix socket on darwin. Try something else.
+		sockFile = filepath.Join(os.TempDir(), rands.HexString(8)+".sock")
+		t.Cleanup(func() { os.Remove(sockFile) })
 	}
 	return &testNode{
 		env:       env,

wgengine/netstack/netstack.go

@@ -44,6 +44,7 @@
 	"tailscale.com/net/tsdial"
 	"tailscale.com/net/tstun"
 	"tailscale.com/syncs"
+	"tailscale.com/tailcfg"
 	"tailscale.com/types/ipproto"
 	"tailscale.com/types/logger"
 	"tailscale.com/types/netmap"
@@ -253,7 +254,6 @@ func (ns *Impl) Start(lb *ipnlocal.LocalBackend) error {
 		panic("nil LocalBackend")
 	}
 	ns.lb = lb
-	ns.e.AddNetworkMapCallback(ns.updateIPs)
 	// size = 0 means use default buffer size
 	const tcpReceiveBufferSize = 0
 	const maxInFlightConnectionAttempts = 1024
@@ -310,8 +310,19 @@ func ipPrefixToAddressWithPrefix(ipp netip.Prefix) tcpip.AddressWithPrefix {
 
 var v4broadcast = netaddr.IPv4(255, 255, 255, 255)
 
-func (ns *Impl) updateIPs(nm *netmap.NetworkMap) {
+// UpdateNetstackIPs updates the set of local IPs that netstack should handle
-	ns.atomicIsLocalIPFunc.Store(tsaddr.NewContainsIPFunc(nm.Addresses))
+// from nm.
+//
+// TODO(bradfitz): don't pass the whole netmap here; just pass the two
+// address slice views.
+func (ns *Impl) UpdateNetstackIPs(nm *netmap.NetworkMap) {
+	var selfNode tailcfg.NodeView
+	if nm != nil {
+		ns.atomicIsLocalIPFunc.Store(tsaddr.NewContainsIPFunc(nm.Addresses))
+		selfNode = nm.SelfNode
+	} else {
+		ns.atomicIsLocalIPFunc.Store(tsaddr.NewContainsIPFunc(nil))
+	}
 
 	oldIPs := make(map[tcpip.AddressWithPrefix]bool)
 	for _, protocolAddr := range ns.ipstack.AllAddresses()[nicID] {
@@ -328,14 +339,14 @@ func (ns *Impl) updateIPs(nm *netmap.NetworkMap) {
 	newIPs := make(map[tcpip.AddressWithPrefix]bool)
 
 	isAddr := map[netip.Prefix]bool{}
-	if nm.SelfNode.Valid() {
+	if selfNode.Valid() {
-		for i := range nm.SelfNode.Addresses().LenIter() {
+		for i := range selfNode.Addresses().LenIter() {
-			ipp := nm.SelfNode.Addresses().At(i)
+			ipp := selfNode.Addresses().At(i)
 			isAddr[ipp] = true
 			newIPs[ipPrefixToAddressWithPrefix(ipp)] = true
 		}
-		for i := range nm.SelfNode.AllowedIPs().LenIter() {
+		for i := range selfNode.AllowedIPs().LenIter() {
-			ipp := nm.SelfNode.AllowedIPs().At(i)
+			ipp := selfNode.AllowedIPs().At(i)
 			if !isAddr[ipp] && ns.ProcessSubnets {
 				newIPs[ipPrefixToAddressWithPrefix(ipp)] = true
 			}

wgengine/userspace.go

@@ -126,15 +126,14 @@ type userspaceEngine struct {
 	statusBufioReader   *bufio.Reader // reusable for UAPI
 	lastStatusPollTime  mono.Time     // last time we polled the engine status
 
-	mu                  sync.Mutex         // guards following; see lock order comment below
+	mu             sync.Mutex         // guards following; see lock order comment below
-	netMap              *netmap.NetworkMap // or nil
+	netMap         *netmap.NetworkMap // or nil
-	closing             bool               // Close was called (even if we're still closing)
+	closing        bool               // Close was called (even if we're still closing)
-	statusCallback      StatusCallback
+	statusCallback StatusCallback
-	peerSequence        []key.NodePublic
+	peerSequence   []key.NodePublic
-	endpoints           []tailcfg.Endpoint
+	endpoints      []tailcfg.Endpoint
-	pendOpen            map[flowtrack.Tuple]*pendingOpenFlow // see pendopen.go
+	pendOpen       map[flowtrack.Tuple]*pendingOpenFlow // see pendopen.go
-	networkMapCallbacks set.HandleSet[NetworkMapCallback]
+	tsIPByIPPort   map[netip.AddrPort]netip.Addr        // allows registration of IP:ports as belonging to a certain Tailscale IP for whois lookups
-	tsIPByIPPort        map[netip.AddrPort]netip.Addr // allows registration of IP:ports as belonging to a certain Tailscale IP for whois lookups
 
 	// pongCallback is the map of response handlers waiting for disco or TSMP
 	// pong callbacks. The map key is a random slice of bytes.
@@ -1158,20 +1157,6 @@ func (e *userspaceEngine) linkChange(delta *netmon.ChangeDelta) {
 	e.magicConn.ReSTUN(why)
 }
 
-func (e *userspaceEngine) AddNetworkMapCallback(cb NetworkMapCallback) func() {
-	e.mu.Lock()
-	defer e.mu.Unlock()
-	if e.networkMapCallbacks == nil {
-		e.networkMapCallbacks = make(set.HandleSet[NetworkMapCallback])
-	}
-	h := e.networkMapCallbacks.Add(cb)
-	return func() {
-		e.mu.Lock()
-		defer e.mu.Unlock()
-		delete(e.networkMapCallbacks, h)
-	}
-}
-
 func (e *userspaceEngine) SetNetInfoCallback(cb NetInfoCallback) {
 	e.magicConn.SetNetInfoCallback(cb)
 }
@@ -1184,14 +1169,7 @@ func (e *userspaceEngine) SetNetworkMap(nm *netmap.NetworkMap) {
 	e.magicConn.SetNetworkMap(nm)
 	e.mu.Lock()
 	e.netMap = nm
-	callbacks := make([]NetworkMapCallback, 0, 4)
-	for _, fn := range e.networkMapCallbacks {
-		callbacks = append(callbacks, fn)
-	}
 	e.mu.Unlock()
-	for _, fn := range callbacks {
-		fn(nm)
-	}
 }
 
 func (e *userspaceEngine) DiscoPublicKey() key.DiscoPublic {

wgengine/watchdog.go

@@ -149,11 +149,6 @@ func (e *watchdogEngine) SetDERPMap(m *tailcfg.DERPMap) {
 func (e *watchdogEngine) SetNetworkMap(nm *netmap.NetworkMap) {
 	e.watchdog("SetNetworkMap", func() { e.wrap.SetNetworkMap(nm) })
 }
-func (e *watchdogEngine) AddNetworkMapCallback(callback NetworkMapCallback) func() {
-	var fn func()
-	e.watchdog("AddNetworkMapCallback", func() { fn = e.wrap.AddNetworkMapCallback(callback) })
-	return func() { e.watchdog("RemoveNetworkMapCallback", fn) }
-}
 func (e *watchdogEngine) DiscoPublicKey() (k key.DiscoPublic) {
 	e.watchdog("DiscoPublicKey", func() { k = e.wrap.DiscoPublicKey() })
 	return k

wgengine/wgengine.go

@@ -125,12 +125,6 @@ type Engine interface {
 	// The network map should only be read from.
 	SetNetworkMap(*netmap.NetworkMap)
 
-	// AddNetworkMapCallback adds a function to a list of callbacks
-	// that are called when the network map updates. It returns a
-	// function that when called would remove the function from the
-	// list of callbacks.
-	AddNetworkMapCallback(NetworkMapCallback) (removeCallback func())
-
 	// SetNetInfoCallback sets the function to call when a
 	// new NetInfo summary is available.
 	SetNetInfoCallback(NetInfoCallback)