diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 1841de270..64fc5f210 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -562,7 +562,7 @@ func NewLocalBackend(logf logger.Logf, logID logid.PublicID, sys *tsd.System, lo 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.interfaceState, false, netMon.TailscaleInterfaceName()) + cd := netmon.NewChangeDelta(nil, b.interfaceState, false, netMon.TailscaleInterfaceName(), false) b.linkChange(&cd) if buildfeatures.HasPeerAPIServer { diff --git a/net/netmon/loghelper_test.go b/net/netmon/loghelper_test.go index 0f60a040d..70c3a0706 100644 --- a/net/netmon/loghelper_test.go +++ b/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") diff --git a/net/netmon/netmon.go b/net/netmon/netmon.go index 3cccf0841..9e61a5c89 100644 --- a/net/netmon/netmon.go +++ b/net/netmon/netmon.go @@ -116,12 +116,12 @@ type ChangeDelta struct { // 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) + 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 ofcases where a rebind @@ -131,7 +131,9 @@ type ChangeDelta struct { } // 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, @@ -148,10 +150,10 @@ func NewChangeDelta(old, new *State, timeJumped bool, tsIfName string) ChangeDel 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() } @@ -160,18 +162,16 @@ func NewChangeDelta(old, new *State, timeJumped bool, tsIfName string) ChangeDel cd.DefaultRouteInterface = new.DefaultRouteInterface 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() || cd.DefaultRouteInterface == 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 - // subsumes InterfaceIPsChanged, IsExpensive likely does not change without a new interface - // appearing, but we'll keep them all for clarity and testability. - cd.RebindLikelyRequired = (cd.New == nil || // Do we need to rebind if there is no current state? - cd.Old == nil || + // Compute rebind requirement. The default interface needs to be viable and + // one of the other conditions needs to be true. + cd.RebindLikelyRequired = (cd.Old == nil || cd.TimeJumped || cd.DefaultInterfaceChanged || cd.InterfaceIPsChanged || @@ -180,11 +180,6 @@ 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 } @@ -193,14 +188,6 @@ func (cd *ChangeDelta) StateDesc() string { return fmt.Sprintf("old: %v new: %v", cd.Old, cd.New) } -// HasPAC reports whether the new state has a PAC configured. -func (cd *ChangeDelta) HasPAC() bool { - if cd.New == nil { - return false - } - return cd.New.PAC != "" -} - // 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 { @@ -236,6 +223,10 @@ func (cd *ChangeDelta) AnyInterfaceUp() bool { // 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 @@ -259,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] @@ -618,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 diff --git a/net/netmon/netmon_test.go b/net/netmon/netmon_test.go index 60345bf14..ddffb061a 100644 --- a/net/netmon/netmon_test.go +++ b/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,31 +515,28 @@ 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("2001:db8::1/64"), // global IPv6 (documentation block) - netip.MustParsePrefix("fe80::1/64"), // link-local IPv6 - netip.MustParsePrefix("::1/128"), // loopback IPv6 - netip.MustParsePrefix("::/128"), // unspecified IPv6 - netip.MustParsePrefix("224.0.0.1/32"), // multicast IPv4 - netip.MustParsePrefix("127.0.0.1/32"), // loopback IPv4 + netip.MustParsePrefix("fd15:dead:beef::1/64"), // IPv6 ULA + netip.MustParsePrefix("2001:db8::1/64"), // global IPv6 } - got := filterRoutableIPs(addrs) - - want := []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 - netip.MustParsePrefix("2001:db8::1/64"), + 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 + netip.MustParsePrefix("224.0.0.1/32"), // multicast IPv4 + netip.MustParsePrefix("127.0.0.1/32"), // loopback IPv4 } + got, want := filterRoutableIPs( + append(nonRoutable, routable...), + ), routable + if !reflect.DeepEqual(got, want) { t.Fatalf("filterRoutableIPs returned %v; want %v", got, want) }