ipn/ipnlocal, net/netmon: remove netmon.State dependencies

updates tailscale/corp#33891

This further reduces (but does not completely eliminate) the
need to pass around netmon.State and leans on the
netmon.ChangeDelta and its precomputed fields.

Signed-off-by: Jonathan Nobels <jonathan@tailscale.com>

ipn/ipnlocal/local.go

@@ -294,7 +294,7 @@ type LocalBackend struct {
 	authURLTime       time.Time     // when the authURL was received from the control server; TODO(nickkhyl): move to nodeBackend
 	authActor         ipnauth.Actor // an actor who called [LocalBackend.StartLoginInteractive] last, or nil; TODO(nickkhyl): move to nodeBackend
 	egg               bool
-	prevIfState       *netmon.State
+	interfaceState    *netmon.State  // latest network interface state or nil
 	peerAPIServer     *peerAPIServer // or nil
 	peerAPIListeners  []*peerAPIListener
 	loginFlags        controlclient.LoginFlags
@@ -559,10 +559,10 @@ func NewLocalBackend(logf logger.Logf, logID logid.PublicID, sys *tsd.System, lo
 
 	b.e.SetStatusCallback(b.setWgengineStatus)
 
-	b.prevIfState = netMon.InterfaceState()
+	b.interfaceState = netMon.InterfaceState()
 	// Call our linkChange code once with the current state.
 	// Following changes are triggered via the eventbus.
-	cd := netmon.NewChangeDelta(nil, b.prevIfState, false, netMon.TailscaleInterfaceName())
+	cd := netmon.NewChangeDelta(nil, b.interfaceState, false, netMon.TailscaleInterfaceName(), false)
 	b.linkChange(&cd)
 
 	if buildfeatures.HasPeerAPIServer {
@@ -935,7 +935,7 @@ func (b *LocalBackend) pauseOrResumeControlClientLocked() {
 	if b.cc == nil {
 		return
 	}
-	networkUp := b.prevIfState.AnyInterfaceUp()
+	networkUp := b.interfaceState.AnyInterfaceUp()
 	pauseForNetwork := (b.state == ipn.Stopped && b.NetMap() != nil) || (!networkUp && !testenv.InTest() && !assumeNetworkUpdateForTest())
 
 	prefs := b.pm.CurrentPrefs()
@@ -962,9 +962,8 @@ func (b *LocalBackend) linkChange(delta *netmon.ChangeDelta) {
 	b.mu.Lock()
 	defer b.mu.Unlock()
 
-	ifst := delta.New
+	b.interfaceState = delta.New
-	hadPAC := b.prevIfState.HasPAC()
+
-	b.prevIfState = ifst
 	b.pauseOrResumeControlClientLocked()
 	prefs := b.pm.CurrentPrefs()
 	if delta.RebindLikelyRequired && prefs.AutoExitNode().IsSet() {
@@ -978,8 +977,8 @@ func (b *LocalBackend) linkChange(delta *netmon.ChangeDelta) {
 		needReconfig = true
 	}
 	// If the PAC-ness of the network changed, reconfig wireguard+route to add/remove subnets.
-	if hadPAC != ifst.HasPAC() {
+	if delta.HasPACOrProxyConfigChanged {
-		b.logf("linkChange: in state %v; PAC changed from %v->%v", b.state, hadPAC, ifst.HasPAC())
+		b.logf("linkChange: in state %v; PAC or proxyConfig changed; updating routes", b.state)
 		needReconfig = true
 	}
 	if needReconfig {
@@ -5053,7 +5052,7 @@ func (b *LocalBackend) authReconfigLocked() {
 	}
 
 	prefs := b.pm.CurrentPrefs()
-	hasPAC := b.prevIfState.HasPAC()
+	hasPAC := b.interfaceState.HasPAC()
 	disableSubnetsIfPAC := cn.SelfHasCap(tailcfg.NodeAttrDisableSubnetsIfPAC)
 	dohURL, dohURLOK := cn.exitNodeCanProxyDNS(prefs.ExitNodeID())
 	dcfg := cn.dnsConfigForNetmap(prefs, b.keyExpired, version.OS())
@@ -5299,7 +5298,7 @@ func (b *LocalBackend) initPeerAPIListenerLocked() {
 		var err error
 		skipListen := i > 0 && isNetstack
 		if !skipListen {
-			ln, err = ps.listen(a.Addr(), b.prevIfState)
+			ln, err = ps.listen(a.Addr(), b.interfaceState.TailscaleInterfaceIndex)
 			if err != nil {
 				if peerAPIListenAsync {
 					b.logf("[v1] possibly transient peerapi listen(%q) error, will try again on linkChange: %v", a.Addr(), err)

ipn/ipnlocal/peerapi.go

@@ -41,7 +41,7 @@ import (
 	"tailscale.com/wgengine/filter"
 )
 
-var initListenConfig func(*net.ListenConfig, netip.Addr, *netmon.State, string) error
+var initListenConfig func(config *net.ListenConfig, addr netip.Addr, tunIfIndex int) error
 
 // peerDNSQueryHandler is implemented by tsdns.Resolver.
 type peerDNSQueryHandler interface {
@@ -53,7 +53,7 @@ type peerAPIServer struct {
 	resolver peerDNSQueryHandler
 }
 
-func (s *peerAPIServer) listen(ip netip.Addr, ifState *netmon.State) (ln net.Listener, err error) {
+func (s *peerAPIServer) listen(ip netip.Addr, tunIfIndex int) (ln net.Listener, err error) {
 	// Android for whatever reason often has problems creating the peerapi listener.
 	// But since we started intercepting it with netstack, it's not even important that
 	// we have a real kernel-level listener. So just create a dummy listener on Android
@@ -69,7 +69,7 @@ func (s *peerAPIServer) listen(ip netip.Addr, ifState *netmon.State) (ln net.Lis
 		// On iOS/macOS, this sets the lc.Control hook to
 		// setsockopt the interface index to bind to, to get
 		// out of the network sandbox.
-		if err := initListenConfig(&lc, ip, ifState, s.b.dialer.TUNName()); err != nil {
+		if err := initListenConfig(&lc, ip, tunIfIndex); err != nil {
 			return nil, err
 		}
 		if runtime.GOOS == "darwin" || runtime.GOOS == "ios" {

ipn/ipnlocal/peerapi_macios_ext.go

@@ -6,11 +6,9 @@
 package ipnlocal
 
 import (
-	"fmt"
 	"net"
 	"net/netip"
 
-	"tailscale.com/net/netmon"
 	"tailscale.com/net/netns"
 )
 
@@ -21,10 +19,6 @@ func init() {
 // initListenConfigNetworkExtension configures nc for listening on IP
 // through the iOS/macOS Network/System Extension (Packet Tunnel
 // Provider) sandbox.
-func initListenConfigNetworkExtension(nc *net.ListenConfig, ip netip.Addr, st *netmon.State, tunIfName string) error {
+func initListenConfigNetworkExtension(nc *net.ListenConfig, ip netip.Addr, ifaceIndex int) error {
-	tunIf, ok := st.Interface[tunIfName]
+	return netns.SetListenConfigInterfaceIndex(nc, ifaceIndex)
-	if !ok {
-		return fmt.Errorf("no interface with name %q", tunIfName)
-	}
-	return netns.SetListenConfigInterfaceIndex(nc, tunIf.Index)
 }

ipn/ipnlocal/serve.go

@@ -167,7 +167,7 @@ func (s *localListener) Run() {
 			// required by the network sandbox to allow binding to
 			// a specific interface. Without this hook, the system
 			// chooses a default interface to bind to.
-			if err := initListenConfig(&lc, ip, s.b.prevIfState, s.b.dialer.TUNName()); err != nil {
+			if err := initListenConfig(&lc, ip, s.b.interfaceState.TailscaleInterfaceIndex); err != nil {
 				s.logf("localListener failed to init listen config %v, backing off: %v", s.ap, err)
 				s.bo.BackOff(s.ctx, err)
 				continue

logtail/logtail.go

@@ -437,7 +437,7 @@ func (lg *Logger) internetUp() bool {
 // [netmon.ChangeDelta] events to detect whether the Internet is expected to be
 // reachable.
 func (lg *Logger) onChangeDelta(delta *netmon.ChangeDelta) {
-	if delta.New.AnyInterfaceUp() {
+	if delta.AnyInterfaceUp() {
 		fmt.Fprintf(lg.stderr, "logtail: internet back up\n")
 		lg.networkIsUp.Set()
 	} else {
@@ -456,7 +456,7 @@ func (lg *Logger) awaitInternetUp(ctx context.Context) {
 	}
 	upc := make(chan bool, 1)
 	defer lg.netMonitor.RegisterChangeCallback(func(delta *netmon.ChangeDelta) {
-		if delta.New.AnyInterfaceUp() {
+		if delta.AnyInterfaceUp() {
 			select {
 			case upc <- true:
 			default:

net/netmon/loghelper_test.go

@@ -64,7 +64,7 @@ func syncTestLinkChangeLogLimiter(t *testing.T) {
 	// InjectEvent doesn't work because it's not a major event, so we
 	// instead inject the event ourselves.
 	injector := eventbustest.NewInjector(t, bus)
-	eventbustest.Inject(injector, NewChangeDelta(nil, nil, true, ""))
+	eventbustest.Inject(injector, NewChangeDelta(nil, nil, true, "", false))
 	synctest.Wait()
 
 	logf("hello %s", "world")

net/netmon/netmon.go

@@ -87,7 +87,10 @@ type ChangeFunc func(*ChangeDelta)
 
 // ChangeDelta describes the difference between two network states.
 //
-// Use NewChangeDelta to construct one and compute the cached fields.
+// Use NewChangeDelta to construct a delta and compute the cached fields.
+//
+// TODO (barnstar): make new and old (and netmon.State) private once all consumers are updated
+// to use the accessor methods.
 type ChangeDelta struct {
 	// Old is the old interface state, if known.
 	// It's nil if the old state is unknown.
@@ -108,26 +111,29 @@ type ChangeDelta struct {
 	// platforms know this or set it.  Copied from netmon.Monitor.tsIfName.
 	TailscaleIfaceName string
 
+	DefaultRouteInterface string
+
 	// Computed Fields
 
 	DefaultInterfaceChanged     bool // whether default route interface changed
-	IsLessExpensive             bool // whether new state is less expensive than old
+	IsLessExpensive             bool // whether new state's default interface is less expensive than old.
 	HasPACOrProxyConfigChanged  bool // whether PAC/HTTP proxy config changed
 	InterfaceIPsChanged         bool // whether any interface IPs changed in a meaningful way
 	AvailableProtocolsChanged   bool // whether we have seen a change in available IPv4/IPv6
 	DefaultInterfaceMaybeViable bool // whether the default interface is potentially viable (has usable IPs, is up and is not the tunnel itself)
+	IsInitialState              bool // whether this is the initial state (old == nil, new != nil)
 
 	// RebindLikelyRequired combines the various fields above to report whether this change likely requires us
-	// to rebind sockets.  This is a very conservative estimate and covers a number of
+	// to rebind sockets.  This is a very conservative estimate and covers a number ofcases where a rebind
-	// cases where a rebind is not strictly necessary.  Consumers of the ChangeDelta should
+	// may not be strictly necessary.  Consumers of the ChangeDelta should consider checking the individual fields
-	// use this as a hint only.  If in doubt, rebind.
+	// above or the state of their sockets.
 	RebindLikelyRequired bool
 }
 
-var skipRebindIfNoDefaultRouteChange = true
-
 // NewChangeDelta builds a ChangeDelta and eagerly computes the cached fields.
-func NewChangeDelta(old, new *State, timeJumped bool, tsIfName string) ChangeDelta {
+// forceViability, if true, forces DefaultInterfaceMaybeViable to be true regardless of the
+// actual state of the default interface.  This is useful in testing.
+func NewChangeDelta(old, new *State, timeJumped bool, tsIfName string, forceViability bool) ChangeDelta {
 	cd := ChangeDelta{
 		Old:                old,
 		New:                new,
@@ -142,29 +148,30 @@ func NewChangeDelta(old, new *State, timeJumped bool, tsIfName string) ChangeDel
 		cd.IsLessExpensive = false
 		cd.HasPACOrProxyConfigChanged = true
 		cd.InterfaceIPsChanged = true
+		cd.IsInitialState = true
 	} else {
-		cd.AvailableProtocolsChanged = cd.Old.HaveV4 != cd.New.HaveV4 || cd.Old.HaveV6 != cd.New.HaveV6
+		cd.AvailableProtocolsChanged = (cd.Old.HaveV4 != cd.New.HaveV4) || (cd.Old.HaveV6 != cd.New.HaveV6)
 		cd.DefaultInterfaceChanged = cd.Old.DefaultRouteInterface != cd.New.DefaultRouteInterface
 		cd.IsLessExpensive = cd.Old.IsExpensive && !cd.New.IsExpensive
-		cd.HasPACOrProxyConfigChanged = cd.Old.PAC != cd.New.PAC || cd.Old.HTTPProxy != cd.New.HTTPProxy
+		cd.HasPACOrProxyConfigChanged = (cd.Old.PAC != cd.New.PAC) || (cd.Old.HTTPProxy != cd.New.HTTPProxy)
 		cd.InterfaceIPsChanged = cd.isInterestingIntefaceChange()
 	}
 
 	// If the default route interface is populated, but it's not up this event signifies that we're in
 	// the process of tearing it down.  Rebinds are going to fail so it's flappy to try.
-	defIfName := new.DefaultRouteInterface
+	cd.DefaultRouteInterface = new.DefaultRouteInterface
-	defIf := new.Interface[defIfName]
+	defIf := new.Interface[cd.DefaultRouteInterface]
-	cd.DefaultInterfaceMaybeViable = true
+
 	// The default interface is not viable if is down or is the Tailscale interface itself.
-	if !defIf.IsUp() || defIfName == tsIfName {
+	if !forceViability && (!defIf.IsUp() || cd.DefaultRouteInterface == tsIfName) {
 		cd.DefaultInterfaceMaybeViable = false
+	} else {
+		cd.DefaultInterfaceMaybeViable = true
 	}
 
-	// Compute rebind requirement.  A number of these checks are redundant - HaveSomeAddressChanged
+	// Compute rebind requirement.   The default interface needs to be viable and
-	// subsumes InterfaceIPsChanged, IsExpensive likely does not change without a new interface
+	// one of the other conditions needs to be true.
-	// appearing, but we'll keep them all for clarity and testability.
+	cd.RebindLikelyRequired = (cd.Old == nil ||
-	cd.RebindLikelyRequired = (cd.New == nil || // Do we need to rebind if there is no current state?
-		cd.Old == nil ||
 		cd.TimeJumped ||
 		cd.DefaultInterfaceChanged ||
 		cd.InterfaceIPsChanged ||
@@ -173,18 +180,53 @@ func NewChangeDelta(old, new *State, timeJumped bool, tsIfName string) ChangeDel
 		cd.AvailableProtocolsChanged) &&
 		cd.DefaultInterfaceMaybeViable
 
-	// (barnstar) TODO: There are likely a number of optimizations we can do here to avoid
-	// rebinding in cases where it is not necessary but we really need to leave that to the
-	// upstream component.  If it's sockets are happy, then it probably doesn't need to rebind,
-	// but it may want to if any of these fields are true.
-
 	return cd
 }
 
+// StateDesc returns a description of the old and new states for logging
+func (cd *ChangeDelta) StateDesc() string {
+	return fmt.Sprintf("old: %v new: %v", cd.Old, cd.New)
+}
+
+// InterfaceIPAppeared reports whether the given IP address exists on any interface
+// in the old state, but not in the new state.
+func (cd *ChangeDelta) InterfaceIPDisppeared(ip netip.Addr) bool {
+	if cd.Old == nil {
+		return false
+	}
+	if cd.New == nil && cd.Old.HasIP(ip) {
+		return true
+	}
+	return cd.New.HasIP(ip) && !cd.Old.HasIP(ip)
+}
+
+// InterfaceIPDisappeared reports whether the given IP address existed on any interface
+// in the old state, but not in the new state.
+func (cd *ChangeDelta) InterfaceIPDisappeared(ip netip.Addr) bool {
+	return !cd.New.HasIP(ip) && cd.Old.HasIP(ip)
+}
+
+// AnyInterfaceUp reports whether any interfaces are up in the new state.
+func (cd *ChangeDelta) AnyInterfaceUp() bool {
+	if cd.New == nil {
+		return false
+	}
+	for _, ifi := range cd.New.Interface {
+		if ifi.IsUp() {
+			return true
+		}
+	}
+	return false
+}
+
 // isInterestingIntefaceChange reports whether any interfaces have changed in a meaninful way.
 // This excludes interfaces that are not interesting per IsInterestingInterface and
 // filters out changes to interface IPs that that are uninteresting (e.g. link-local addresses).
 func (cd *ChangeDelta) isInterestingIntefaceChange() bool {
+	if cd.Old == nil && cd.New == nil {
+		return false
+	}
+
 	// If either side is nil treat as changed.
 	if cd.Old == nil || cd.New == nil {
 		return true
@@ -208,7 +250,7 @@ func (cd *ChangeDelta) isInterestingIntefaceChange() bool {
 		}
 
 		// The old interface doesn't exist in the new interface set and it has
-		// an a global unicast IP.  That's considered a change from the perspective
+		// a global unicast IP.  That's considered a change from the perspective
 		// of anything that may have been bound to it.  If it didn't have a global
 		// unicast IP, it's not interesting.
 		newInterface, ok := cd.New.Interface[iname]
@@ -567,7 +609,7 @@ func (m *Monitor) handlePotentialChange(newState *State, forceCallbacks bool) {
 		return
 	}
 
-	delta := NewChangeDelta(oldState, newState, timeJumped, m.tsIfName)
+	delta := NewChangeDelta(oldState, newState, timeJumped, m.tsIfName, false)
 
 	if delta.RebindLikelyRequired {
 		m.gwValid = false

net/netmon/netmon_test.go

@@ -498,7 +498,7 @@ func TestRebindRequired(t *testing.T) {
 				}
 			}
 
-			cd := NewChangeDelta(tt.s1, tt.s2, false, tt.tsIfName)
+			cd := NewChangeDelta(tt.s1, tt.s2, false, tt.tsIfName, true)
 			_ = cd // in case we need it later
 			if got := cd.RebindLikelyRequired; got != tt.want {
 				t.Errorf("RebindRequired = %v; want %v", got, tt.want)
@@ -515,13 +515,17 @@ func withIsInterestingInterface(t *testing.T, fn func(Interface, []netip.Prefix)
 }
 
 func TestIncludesRoutableIP(t *testing.T) {
-	addrs := []netip.Prefix{
+	routable := []netip.Prefix{
 		netip.MustParsePrefix("1.2.3.4/32"),
 		netip.MustParsePrefix("10.0.0.1/24"),          // RFC1918 IPv4 (private)
 		netip.MustParsePrefix("172.16.0.1/12"),        // RFC1918 IPv4 (private)
 		netip.MustParsePrefix("192.168.1.1/24"),       // RFC1918 IPv4 (private)
-		netip.MustParsePrefix("fd15:dead:beef::1/64"), // IPv6 ULA (should be filtered)
+		netip.MustParsePrefix("fd15:dead:beef::1/64"), // IPv6 ULA
-		netip.MustParsePrefix("2001:db8::1/64"),       // global IPv6 (documentation block)
+		netip.MustParsePrefix("2001:db8::1/64"),       // global IPv6
+	}
+
+	nonRoutable := []netip.Prefix{
+		netip.MustParsePrefix("ff00::/8"),     // multicast IPv6 (should be filtered)
 		netip.MustParsePrefix("fe80::1/64"),   // link-local IPv6
 		netip.MustParsePrefix("::1/128"),      // loopback IPv6
 		netip.MustParsePrefix("::/128"),       // unspecified IPv6
@@ -529,16 +533,9 @@ func TestIncludesRoutableIP(t *testing.T) {
 		netip.MustParsePrefix("127.0.0.1/32"), // loopback IPv4
 	}
 
-	got := filterRoutableIPs(addrs)
+	got, want := filterRoutableIPs(
-
+		append(nonRoutable, routable...),
-	want := []netip.Prefix{
+	), routable
-		netip.MustParsePrefix("1.2.3.4/32"),
-		netip.MustParsePrefix("10.0.0.1/24"),          // RFC1918 IPv4 (private)
-		netip.MustParsePrefix("172.16.0.1/12"),        // RFC1918 IPv4 (private)
-		netip.MustParsePrefix("192.168.1.1/24"),       // RFC1918 IPv4 (private)
-		netip.MustParsePrefix("fd15:dead:beef::1/64"), // IPv6 ULA
-		netip.MustParsePrefix("2001:db8::1/64"),
-	}
 
 	if !reflect.DeepEqual(got, want) {
 		t.Fatalf("filterRoutableIPs returned %v; want %v", got, want)
@@ -642,8 +639,3 @@ func TestForeachInterface(t *testing.T) {
 		})
 	}
 }
-
-type testOSMon struct {
-	osMon
-	Interesting func(name string) bool
-}

net/netmon/state.go

@@ -287,6 +287,9 @@ type State struct {
 
 	// PAC is the URL to the Proxy Autoconfig URL, if applicable.
 	PAC string
+
+	// TailscaleInterfaceIndex is the index of the Tailscale interface
+	TailscaleInterfaceIndex int
 }
 
 func (s *State) String() string {
@@ -507,6 +510,10 @@ func getState(optTSInterfaceName string) (*State, error) {
 			return
 		}
 
+		if isTailscaleInterface(ni.Name, pfxs) {
+			s.TailscaleInterfaceIndex = ni.Index
+		}
+
 		if !ifUp || isTSInterfaceName || isTailscaleInterface(ni.Name, pfxs) {
 			return
 		}

net/tsdial/tsdial.go

@@ -264,7 +264,7 @@ var (
 
 func (d *Dialer) linkChanged(delta *netmon.ChangeDelta) {
 	// Track how often we see ChangeDeltas with no DefaultRouteInterface.
-	if delta.New.DefaultRouteInterface == "" {
+	if delta.DefaultRouteInterface == "" {
 		metricChangeDeltaNoDefaultRoute.Add(1)
 	}
 
@@ -294,22 +294,23 @@ func changeAffectsConn(delta *netmon.ChangeDelta, conn net.Conn) bool {
 	}
 	lip, rip := la.AddrPort().Addr(), ra.AddrPort().Addr()
 
-	if delta.Old == nil {
+	if delta.IsInitialState {
 		return false
 	}
-	if delta.Old.DefaultRouteInterface != delta.New.DefaultRouteInterface ||
+
-		delta.Old.HTTPProxy != delta.New.HTTPProxy {
+	if delta.DefaultInterfaceChanged ||
+		delta.HasPACOrProxyConfigChanged {
 		return true
 	}
 
 	// In a few cases, we don't have a new DefaultRouteInterface (e.g. on
-	// Android; see tailscale/corp#19124); if so, pessimistically assume
+	// Android and macOS/iOS; see tailscale/corp#19124); if so, pessimistically assume
 	// that all connections are affected.
-	if delta.New.DefaultRouteInterface == "" && runtime.GOOS != "plan9" {
+	if delta.DefaultRouteInterface == "" && runtime.GOOS != "plan9" {
 		return true
 	}
 
-	if !delta.New.HasIP(lip) && delta.Old.HasIP(lip) {
+	if delta.InterfaceIPDisappeared(lip) {
 		// Our interface with this source IP went away.
 		return true
 	}

wgengine/userspace.go

@@ -387,6 +387,7 @@ func NewUserspaceEngine(logf logger.Logf, conf Config) (_ Engine, reterr error)
 	conf.Dialer.SetTUNName(tunName)
 	conf.Dialer.SetNetMon(e.netMon)
 	conf.Dialer.SetBus(e.eventBus)
+
 	e.dns = dns.NewManager(logf, conf.DNS, e.health, conf.Dialer, fwdDNSLinkSelector{e, tunName}, conf.ControlKnobs, runtime.GOOS)
 
 	// TODO: there's probably a better place for this
@@ -1333,12 +1334,11 @@ func (e *userspaceEngine) Done() <-chan struct{} {
 
 func (e *userspaceEngine) linkChange(delta *netmon.ChangeDelta) {
 
-	cur := delta.New
+	up := delta.AnyInterfaceUp()
-	up := cur.AnyInterfaceUp()
 	if !up {
-		e.logf("LinkChange: all links down; pausing: %v", cur)
+		e.logf("LinkChange: all links down; pausing: %v", delta.StateDesc())
 	} else if delta.RebindLikelyRequired {
-		e.logf("LinkChange: major, rebinding. New state: %v OldState: %v", cur, delta.Old)
+		e.logf("LinkChange: major, rebinding: %v", delta.StateDesc())
 	} else {
 		e.logf("[v1] LinkChange: minor")
 	}