net/netmon: make ChangeFunc's signature take new ChangeDelta, not bool

Updates #9040

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

cmd/tailscaled/debug.go

@@ -82,13 +82,13 @@ func runMonitor(ctx context.Context, loop bool) error {
 	}
 	defer mon.Close()
 
-	mon.RegisterChangeCallback(func(changed bool, st *interfaces.State) {
-		if !changed {
-			log.Printf("Network monitor fired; no change")
+	mon.RegisterChangeCallback(func(delta *netmon.ChangeDelta) {
+		if !delta.Major {
+			log.Printf("Network monitor fired; not a major change")
 			return
 		}
 		log.Printf("Network monitor fired. New state:")
-		dump(st)
+		dump(delta.New)
 	})
 	if loop {
 		log.Printf("Starting link change monitor; initial state:")

ipn/ipnlocal/local.go

@@ -50,6 +50,7 @@ import (
 	"tailscale.com/net/dnscache"
 	"tailscale.com/net/dnsfallback"
 	"tailscale.com/net/interfaces"
+	"tailscale.com/net/netmon"
 	"tailscale.com/net/netns"
 	"tailscale.com/net/netutil"
 	"tailscale.com/net/tsaddr"
@@ -342,7 +343,7 @@ func NewLocalBackend(logf logger.Logf, logID logid.PublicID, sys *tsd.System, lo
 	b.prevIfState = netMon.InterfaceState()
 	// Call our linkChange code once with the current state, and
 	// then also whenever it changes:
-	b.linkChange(false, netMon.InterfaceState())
+	b.linkChange(&netmon.ChangeDelta{New: netMon.InterfaceState()})
 	b.unregisterNetMon = netMon.RegisterChangeCallback(b.linkChange)
 
 	b.unregisterHealthWatch = health.RegisterWatcher(b.onHealthChange)
@@ -508,11 +509,11 @@ func (b *LocalBackend) pauseOrResumeControlClientLocked() {
 }
 
 // linkChange is our network monitor callback, called whenever the network changes.
-// major is whether ifst is different than earlier.
-func (b *LocalBackend) linkChange(major bool, ifst *interfaces.State) {
+func (b *LocalBackend) linkChange(delta *netmon.ChangeDelta) {
 	b.mu.Lock()
 	defer b.mu.Unlock()
 
+	ifst := delta.New
 	hadPAC := b.prevIfState.HasPAC()
 	b.prevIfState = ifst
 	b.pauseOrResumeControlClientLocked()

logtail/logtail.go

@@ -22,7 +22,6 @@ import (
 	"time"
 
 	"tailscale.com/envknob"
-	"tailscale.com/net/interfaces"
 	"tailscale.com/net/netmon"
 	"tailscale.com/net/sockstats"
 	"tailscale.com/tstime"
@@ -427,8 +426,8 @@ func (l *Logger) internetUp() bool {
 
 func (l *Logger) awaitInternetUp(ctx context.Context) {
 	upc := make(chan bool, 1)
-	defer l.netMonitor.RegisterChangeCallback(func(changed bool, st *interfaces.State) {
-		if st.AnyInterfaceUp() {
+	defer l.netMonitor.RegisterChangeCallback(func(delta *netmon.ChangeDelta) {
+		if delta.New.AnyInterfaceUp() {
 			select {
 			case upc <- true:
 			default:

net/netmon/netmon.go

@@ -71,9 +71,37 @@ type Monitor struct {
 }
 
 // ChangeFunc is a callback function registered with Monitor that's called when the
-// network changed. The changed parameter is whether the network changed
-// enough for State to have changed since the last callback.
-type ChangeFunc func(changed bool, state *interfaces.State)
+// network changed.
+type ChangeFunc func(*ChangeDelta)
+
+// ChangeDelta describes the difference between two network states.
+type ChangeDelta struct {
+	// Old is the old interface state, if known.
+	// It's nil if the old state is unknown.
+	// Do not mutate it.
+	Old *interfaces.State
+
+	// New is the new network state.
+	// It is always non-nil.
+	// Do not mutate it.
+	New *interfaces.State
+
+	// Major is our legacy boolean of whether the network changed in some major
+	// way.
+	//
+	// Deprecated: do not remove. As of 2023-08-23 we're in a renewed effort to
+	// remove it and ask specific qustions of ChangeDelta instead. Look at Old
+	// and New (or add methods to ChangeDelta) instead of using Major.
+	Major bool
+
+	// TimeJumped is whether there was a big jump in wall time since the last
+	// time we checked. This is a hint that a mobile sleeping device might have
+	// come out of sleep.
+	TimeJumped bool
+
+	// TODO(bradfitz): add some lazy cached fields here as needed with methods
+	// on *ChangeDelta to let callers ask specific questions
+}
 
 // New instantiates and starts a monitoring instance.
 // The returned monitor is inactive until it's started by the Start method.
@@ -299,29 +327,33 @@ func (m *Monitor) debounce() {
 		} else {
 			m.mu.Lock()
 
-			oldState := m.ifState
-			changed := !curState.EqualFiltered(oldState, m.isInterestingInterface, interfaces.UseInterestingIPs)
-			if changed {
+			delta := &ChangeDelta{
+				Old: m.ifState,
+				New: curState,
+			}
+			delta.Major = !delta.New.EqualFiltered(delta.Old, m.isInterestingInterface, interfaces.UseInterestingIPs)
+			if delta.Major {
 				m.gwValid = false
 				m.ifState = curState
 
-				if s1, s2 := oldState.String(), curState.String(); s1 == s2 {
+				if s1, s2 := delta.Old.String(), delta.New.String(); s1 == s2 {
 					m.logf("[unexpected] network state changed, but stringification didn't: %v", s1)
-					m.logf("[unexpected] old: %s", jsonSummary(oldState))
-					m.logf("[unexpected] new: %s", jsonSummary(curState))
+					m.logf("[unexpected] old: %s", jsonSummary(delta.Old))
+					m.logf("[unexpected] new: %s", jsonSummary(delta.New))
 				}
 			}
 			// See if we have a queued or new time jump signal.
 			if shouldMonitorTimeJump && m.checkWallTimeAdvanceLocked() {
 				m.resetTimeJumpedLocked()
-				if !changed {
+				delta.TimeJumped = true
+				if !delta.Major {
 					// Only log if it wasn't an interesting change.
 					m.logf("time jumped (probably wake from sleep); synthesizing major change event")
-					changed = true
+					delta.Major = true
 				}
 			}
 			for _, cb := range m.cbs {
-				go cb(changed, m.ifState)
+				go cb(delta)
 			}
 			m.mu.Unlock()
 		}

net/netmon/netmon_test.go

@@ -8,8 +8,6 @@ import (
 	"sync/atomic"
 	"testing"
 	"time"
-
-	"tailscale.com/net/interfaces"
 )
 
 func TestMonitorStartClose(t *testing.T) {
@@ -40,7 +38,7 @@ func TestMonitorInjectEvent(t *testing.T) {
 	}
 	defer mon.Close()
 	got := make(chan bool, 1)
-	mon.RegisterChangeCallback(func(changed bool, state *interfaces.State) {
+	mon.RegisterChangeCallback(func(*ChangeDelta) {
 		select {
 		case got <- true:
 		default:
@@ -101,9 +99,9 @@ func TestMonitorMode(t *testing.T) {
 			done = t.C
 		}
 		n := 0
-		mon.RegisterChangeCallback(func(changed bool, st *interfaces.State) {
+		mon.RegisterChangeCallback(func(d *ChangeDelta) {
 			n++
-			t.Logf("cb: changed=%v, ifSt=%v", changed, st)
+			t.Logf("cb: changed=%v, ifSt=%v", d.Major, d.New)
 		})
 		mon.Start()
 		<-done

net/sockstats/sockstats_tsgo.go

@@ -266,25 +266,29 @@ func setNetMon(netMon *netmon.Monitor) {
 		sockStats.usedInterfaces[ifIndex] = 1
 	}
 
-	netMon.RegisterChangeCallback(func(changed bool, state *interfaces.State) {
-		if changed {
-			if ifName := state.DefaultRouteInterface; ifName != "" {
-				ifIndex := state.Interface[ifName].Index
-				sockStats.mu.Lock()
-				defer sockStats.mu.Unlock()
-				// Ignore changes to unknown interfaces -- it would require
-				// updating the tx/rxBytesByInterface maps and thus
-				// additional locking for every read/write. Most of the time
-				// the set of interfaces is static.
-				if _, ok := sockStats.knownInterfaces[ifIndex]; ok {
-					sockStats.currentInterface.Store(uint32(ifIndex))
-					sockStats.usedInterfaces[ifIndex] = 1
-					sockStats.currentInterfaceCellular.Store(isLikelyCellularInterface(ifName))
-				} else {
-					sockStats.currentInterface.Store(0)
-					sockStats.currentInterfaceCellular.Store(false)
-				}
-			}
+	netMon.RegisterChangeCallback(func(delta *netmon.ChangeDelta) {
+		if !delta.Major {
+			return
+		}
+		state := delta.New
+		ifName := state.DefaultRouteInterface
+		if ifName == "" {
+			return
+		}
+		ifIndex := state.Interface[ifName].Index
+		sockStats.mu.Lock()
+		defer sockStats.mu.Unlock()
+		// Ignore changes to unknown interfaces -- it would require
+		// updating the tx/rxBytesByInterface maps and thus
+		// additional locking for every read/write. Most of the time
+		// the set of interfaces is static.
+		if _, ok := sockStats.knownInterfaces[ifIndex]; ok {
+			sockStats.currentInterface.Store(uint32(ifIndex))
+			sockStats.usedInterfaces[ifIndex] = 1
+			sockStats.currentInterfaceCellular.Store(isLikelyCellularInterface(ifName))
+		} else {
+			sockStats.currentInterface.Store(0)
+			sockStats.currentInterfaceCellular.Store(false)
 		}
 	})
 }

net/tsdial/tsdial.go

@@ -18,7 +18,6 @@ import (
 	"time"
 
 	"tailscale.com/net/dnscache"
-	"tailscale.com/net/interfaces"
 	"tailscale.com/net/netknob"
 	"tailscale.com/net/netmon"
 	"tailscale.com/net/netns"
@@ -139,8 +138,8 @@ func (d *Dialer) SetNetMon(netMon *netmon.Monitor) {
 	d.netMonUnregister = d.netMon.RegisterChangeCallback(d.linkChanged)
 }
 
-func (d *Dialer) linkChanged(major bool, state *interfaces.State) {
-	if !major {
+func (d *Dialer) linkChanged(delta *netmon.ChangeDelta) {
+	if !delta.Major {
 		return
 	}
 	d.mu.Lock()

wgengine/userspace.go

@@ -25,7 +25,6 @@ import (
 	"tailscale.com/ipn/ipnstate"
 	"tailscale.com/net/dns"
 	"tailscale.com/net/flowtrack"
-	"tailscale.com/net/interfaces"
 	"tailscale.com/net/netmon"
 	"tailscale.com/net/packet"
 	"tailscale.com/net/sockstats"
@@ -304,9 +303,9 @@ func NewUserspaceEngine(logf logger.Logf, conf Config) (_ Engine, reterr error)
 
 	logf("link state: %+v", e.netMon.InterfaceState())
 
-	unregisterMonWatch := e.netMon.RegisterChangeCallback(func(changed bool, st *interfaces.State) {
+	unregisterMonWatch := e.netMon.RegisterChangeCallback(func(delta *netmon.ChangeDelta) {
 		tshttpproxy.InvalidateCache()
-		e.linkChange(changed, st)
+		e.linkChange(delta)
 	})
 	closePool.addFunc(unregisterMonWatch)
 	e.netMonUnregister = unregisterMonWatch
@@ -1099,7 +1098,9 @@ func (e *userspaceEngine) LinkChange(_ bool) {
 	e.netMon.InjectEvent()
 }
 
-func (e *userspaceEngine) linkChange(changed bool, cur *interfaces.State) {
+func (e *userspaceEngine) linkChange(delta *netmon.ChangeDelta) {
+	changed := delta.Major // TODO(bradfitz): ask more specific questions?
+	cur := delta.New
 	up := cur.AnyInterfaceUp()
 	if !up {
 		e.logf("LinkChange: all links down; pausing: %v", cur)