From ad5e04249baa60b47ce544887789cfd6ab83e161 Mon Sep 17 00:00:00 2001 From: Josh Bleecher Snyder Date: Fri, 19 Nov 2021 17:04:35 -0800 Subject: [PATCH] wgengine/monitor: ignore adding/removing uninteresting IPs One of the most common "unexpected" log lines is: "network state changed, but stringification didn't" One way that this can occur is if an interesting interface (non-Tailscale, has interesting IP address) gains or loses an uninteresting IP address (link local or loopback). The fact that the interface is interesting is enough for EqualFiltered to inspect it. The fact that an IP address changed is enough for EqualFiltered to declare that the interfaces are not equal. But the State.String method reasonably declines to print any uninteresting IP addresses. As a result, the network state appears to have changed, but the stringification did not. The String method is correct; nothing interesting happened. This change fixes this by adding an IP address filter to EqualFiltered in addition to the interface filter. This lets the network monitor ignore the addition/removal of uninteresting IP addresses. Signed-off-by: Josh Bleecher Snyder --- net/interfaces/interfaces.go | 56 ++++++++++++++++++++++++------- net/interfaces/interfaces_test.go | 38 ++++++++++++++++++++- wgengine/monitor/monitor.go | 2 +- wgengine/monitor/polling.go | 2 +- 4 files changed, 83 insertions(+), 15 deletions(-) diff --git a/net/interfaces/interfaces.go b/net/interfaces/interfaces.go index 1de76a15f..8dacc59a7 100644 --- a/net/interfaces/interfaces.go +++ b/net/interfaces/interfaces.go @@ -345,9 +345,18 @@ func (s *State) String() string { return sb.String() } +// An InterfaceFilter indicates whether EqualFiltered should use i when deciding whether two States are equal. +// ips are all the IPPrefixes associated with i. +type InterfaceFilter func(i Interface, ips []netaddr.IPPrefix) bool + +// An IPFilter indicates whether EqualFiltered should use ip when deciding whether two States are equal. +// ip is an ip address associated with some interface under consideration. +type IPFilter func(ip netaddr.IP) bool + // EqualFiltered reports whether s and s2 are equal, -// considering only interfaces in s for which filter returns true. -func (s *State) EqualFiltered(s2 *State, filter func(i Interface, ips []netaddr.IPPrefix) bool) bool { +// considering only interfaces in s for which filter returns true, +// and considering only IPs for those interfaces for which filterIP returns true. +func (s *State) EqualFiltered(s2 *State, useInterface InterfaceFilter, useIP IPFilter) bool { if s == nil && s2 == nil { return true } @@ -364,7 +373,7 @@ func (s *State) EqualFiltered(s2 *State, filter func(i Interface, ips []netaddr. } for iname, i := range s.Interface { ips := s.InterfaceIPs[iname] - if !filter(i, ips) { + if !useInterface(i, ips) { continue } i2, ok := s2.Interface[iname] @@ -375,7 +384,7 @@ func (s *State) EqualFiltered(s2 *State, filter func(i Interface, ips []netaddr. if !ok { return false } - if !interfacesEqual(i, i2) || !prefixesEqual(ips, ips2) { + if !interfacesEqual(i, i2) || !prefixesEqualFiltered(ips, ips2, useIP) { return false } } @@ -390,6 +399,21 @@ func interfacesEqual(a, b Interface) bool { bytes.Equal([]byte(a.HardwareAddr), []byte(b.HardwareAddr)) } +func filteredIPPs(ipps []netaddr.IPPrefix, useIP IPFilter) []netaddr.IPPrefix { + // TODO: rewrite prefixesEqualFiltered to avoid making copies + x := make([]netaddr.IPPrefix, 0, len(ipps)) + for _, ipp := range ipps { + if useIP(ipp.IP()) { + x = append(x, ipp) + } + } + return x +} + +func prefixesEqualFiltered(a, b []netaddr.IPPrefix, useIP IPFilter) bool { + return prefixesEqual(filteredIPPs(a, useIP), filteredIPPs(b, useIP)) +} + func prefixesEqual(a, b []netaddr.IPPrefix) bool { if len(a) != len(b) { return false @@ -402,13 +426,24 @@ func prefixesEqual(a, b []netaddr.IPPrefix) bool { return true } -// FilterInteresting reports whether i is an interesting non-Tailscale interface. -func FilterInteresting(i Interface, ips []netaddr.IPPrefix) bool { +// UseInterestingInterfaces is an InterfaceFilter that reports whether i is an interesting interface. +// An interesting interface if it is (a) not owned by Tailscale and (b) routes interesting IP addresses. +// See UseInterestingIPs for the defition of an interesting IP address. +func UseInterestingInterfaces(i Interface, ips []netaddr.IPPrefix) bool { return !isTailscaleInterface(i.Name, ips) && anyInterestingIP(ips) } -// FilterAll always returns true, to use EqualFiltered against all interfaces. -func FilterAll(i Interface, ips []netaddr.IPPrefix) bool { return true } +// UseInterestingIPs is an IPFilter that reports whether ip is an interesting IP address. +// An IP address is interesting if it is neither a lopback not a link local unicast IP address. +func UseInterestingIPs(ip netaddr.IP) bool { + return isInterestingIP(ip) +} + +// UseAllInterfaces is an InterfaceFilter that includes all interfaces. +func UseAllInterfaces(i Interface, ips []netaddr.IPPrefix) bool { return true } + +// UseAllIPs is an IPFilter that includes all all IPs. +func UseAllIPs(ips netaddr.IP) bool { return true } func (s *State) HasPAC() bool { return s != nil && s.PAC != "" } @@ -594,10 +629,7 @@ func anyInterestingIP(pfxs []netaddr.IPPrefix) bool { // should log in interfaces.State logging. We don't need to show // localhost or link-local addresses. func isInterestingIP(ip netaddr.IP) bool { - if ip.IsLoopback() || ip.IsLinkLocalUnicast() { - return false - } - return true + return !ip.IsLoopback() && !ip.IsLinkLocalUnicast() } var altNetInterfaces func() ([]Interface, error) diff --git a/net/interfaces/interfaces_test.go b/net/interfaces/interfaces_test.go index cd7fff204..4ef46db28 100644 --- a/net/interfaces/interfaces_test.go +++ b/net/interfaces/interfaces_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" + "net" "testing" "inet.af/netaddr" @@ -28,7 +29,7 @@ func TestGetState(t *testing.T) { t.Fatal(err) } - if !st.EqualFiltered(st2, FilterAll) { + if !st.EqualFiltered(st2, UseAllInterfaces, UseAllIPs) { // let's assume nobody was changing the system network interfaces between // the two GetState calls. t.Fatal("two States back-to-back were not equal") @@ -68,3 +69,38 @@ func TestIsUsableV6(t *testing.T) { } } } + +func TestStateEqualFilteredIPFilter(t *testing.T) { + // s1 and s2 are identical, except that an "interesting" interface + // has gained an "uninteresting" IP address. + + s1 := &State{ + InterfaceIPs: map[string][]netaddr.IPPrefix{"x": { + netaddr.MustParseIPPrefix("42.0.0.0/8"), + netaddr.MustParseIPPrefix("169.254.0.0/16"), // link local unicast + }}, + Interface: map[string]Interface{"x": {Interface: &net.Interface{Name: "x"}}}, + } + + s2 := &State{ + InterfaceIPs: map[string][]netaddr.IPPrefix{"x": { + netaddr.MustParseIPPrefix("42.0.0.0/8"), + netaddr.MustParseIPPrefix("169.254.0.0/16"), // link local unicast + netaddr.MustParseIPPrefix("127.0.0.0/8"), // loopback (added) + }}, + Interface: map[string]Interface{"x": {Interface: &net.Interface{Name: "x"}}}, + } + + // s1 and s2 are different... + if s1.EqualFiltered(s2, UseAllInterfaces, UseAllIPs) { + t.Errorf("%+v != %+v", s1, s2) + } + // ...and they look different if you only restrict to interesting interfaces... + if s1.EqualFiltered(s2, UseInterestingInterfaces, UseAllIPs) { + t.Errorf("%+v != %+v when restricting to interesting interfaces _but not_ IPs", s1, s2) + } + // ...but because the additional IP address is uninteresting, we should treat them as the same. + if !s1.EqualFiltered(s2, UseInterestingInterfaces, UseInterestingIPs) { + t.Errorf("%+v == %+v when restricting to interesting interfaces and IPs", s1, s2) + } +} diff --git a/wgengine/monitor/monitor.go b/wgengine/monitor/monitor.go index 1f13ec5ce..4895c390e 100644 --- a/wgengine/monitor/monitor.go +++ b/wgengine/monitor/monitor.go @@ -310,7 +310,7 @@ func (m *Mon) debounce() { } oldState := m.ifState - ifChanged := !curState.EqualFiltered(oldState, interfaces.FilterInteresting) + ifChanged := !curState.EqualFiltered(oldState, interfaces.UseInterestingInterfaces, interfaces.UseInterestingIPs) if ifChanged { m.gwValid = false m.ifState = curState diff --git a/wgengine/monitor/polling.go b/wgengine/monitor/polling.go index d68cba205..2b2d6d99d 100644 --- a/wgengine/monitor/polling.go +++ b/wgengine/monitor/polling.go @@ -77,7 +77,7 @@ func (pm *pollingMon) Receive() (message, error) { defer ticker.Stop() base := pm.m.InterfaceState() for { - if cur, err := pm.m.interfaceStateUncached(); err == nil && !cur.EqualFiltered(base, interfaces.FilterInteresting) { + if cur, err := pm.m.interfaceStateUncached(); err == nil && !cur.EqualFiltered(base, interfaces.UseInterestingInterfaces, interfaces.UseInterestingIPs) { return unspecifiedMessage{}, nil } select {