diff --git a/hscontrol/state/maprequest.go b/hscontrol/state/maprequest.go index fb529df63..07ef695a1 100644 --- a/hscontrol/state/maprequest.go +++ b/hscontrol/state/maprequest.go @@ -10,6 +10,7 @@ import ( "github.com/rs/zerolog" "github.com/rs/zerolog/log" "tailscale.com/tailcfg" + "tailscale.com/types/views" ) // mapRequestDelta carries the classified facts extracted from one MapRequest @@ -154,30 +155,56 @@ func hostinfoEqual(oldHI, newHI *tailcfg.Hostinfo) bool { return oldCopy.Equal(&newCopy) } -// peerHostinfo keeps the Hostinfo fields another node's client reads from -// a peer: what tailscale status shows, PeerAPI services, SSH known hosts, -// exit-node location, app-connector eligibility, and the routes this server -// feeds into policy. Everything else is stored but never fanned out, and a -// peer's NetInfo is never read at all. -func peerHostinfo(hi *tailcfg.Hostinfo) *tailcfg.Hostinfo { - if hi == nil { - return nil +// peerHostinfoEqual reports whether a and b agree on the Hostinfo fields +// another node's client reads from a peer: what tailscale status shows, PeerAPI +// services, SSH known hosts, exit-node location, app-connector eligibility, and +// the routes this server feeds into policy. Everything else is stored but never +// fanned out, and a peer's NetInfo is never read at all. +func peerHostinfoEqual(a, b tailcfg.HostinfoView) bool { + if !a.Valid() || !b.Valid() { + return a.Valid() == b.Valid() } - return &tailcfg.Hostinfo{ - Hostname: hi.Hostname, - OS: hi.OS, - Services: hi.Services, - SSH_HostKeys: hi.SSH_HostKeys, - Location: hi.Location, - AppConnector: hi.AppConnector, - RoutableIPs: hi.RoutableIPs, - } + return a.Hostname() == b.Hostname() && + a.OS() == b.OS() && + servicesEqual(a.Services(), b.Services()) && + views.SliceEqual(a.SSH_HostKeys(), b.SSH_HostKeys()) && + locationEqual(a.Location(), b.Location()) && + a.AppConnector() == b.AppConnector() && + views.SliceEqual(a.RoutableIPs(), b.RoutableIPs()) } -// peerHostinfoEqual reports whether the fields peers read are unchanged. -func peerHostinfoEqual(oldHI, newHI *tailcfg.Hostinfo) bool { - return peerHostinfo(oldHI).Equal(peerHostinfo(newHI)) +// servicesEqual compares field by field, as [tailcfg.Service] is +// incomparable. +func servicesEqual(a, b views.Slice[tailcfg.Service]) bool { + if a.Len() != b.Len() { + return false + } + + for i := range a.Len() { + x, y := a.At(i), b.At(i) + if x.Proto != y.Proto || x.Port != y.Port || x.Description != y.Description { + return false + } + } + + return true +} + +// locationEqual compares field by field, as [tailcfg.LocationView] has no +// Equal. +func locationEqual(a, b tailcfg.LocationView) bool { + if !a.Valid() || !b.Valid() { + return a.Valid() == b.Valid() + } + + return a.Country() == b.Country() && + a.CountryCode() == b.CountryCode() && + a.City() == b.City() && + a.CityCode() == b.CityCode() && + a.Latitude() == b.Latitude() && + a.Longitude() == b.Longitude() && + a.Priority() == b.Priority() } // netInfoEqualIgnoringDERP compares two NetInfo values via diff --git a/hscontrol/state/maprequest_test.go b/hscontrol/state/maprequest_test.go index daf01a059..fe384abdb 100644 --- a/hscontrol/state/maprequest_test.go +++ b/hscontrol/state/maprequest_test.go @@ -3,6 +3,7 @@ package state import ( "fmt" "net/netip" + "reflect" "strings" "sync" "sync/atomic" @@ -860,12 +861,60 @@ func TestPeerHostinfoEqual(t *testing.T) { t.Run(tt.name, func(t *testing.T) { changed := base() tt.mutate(changed) - require.Equal(t, tt.want, peerHostinfoEqual(base(), changed)) + require.Equal(t, tt.want, peerHostinfoEqual(base().View(), changed.View())) }) } - require.True(t, peerHostinfoEqual(nil, nil)) - require.False(t, peerHostinfoEqual(nil, base())) + require.True(t, peerHostinfoEqual(tailcfg.HostinfoView{}, tailcfg.HostinfoView{})) + require.False(t, peerHostinfoEqual(tailcfg.HostinfoView{}, base().View())) +} + +// TestPeerHostinfoEqualSeesEveryField fails when tailscale adds a field to +// [tailcfg.Service] or [tailcfg.Location] that servicesEqual or locationEqual +// does not compare. +func TestPeerHostinfoEqualSeesEveryField(t *testing.T) { + set := func(t *testing.T, f reflect.Value) { + t.Helper() + + switch { + case f.Kind() == reflect.String: + f.SetString("x") + case f.CanInt(): + f.SetInt(1) + case f.CanUint(): + f.SetUint(1) + case f.CanFloat(): + f.SetFloat(1) + default: + t.Fatalf("field kind %s not handled; compare it", f.Kind()) + } + } + + for _, field := range reflect.VisibleFields(reflect.TypeFor[tailcfg.Service]()) { + if !field.IsExported() { + continue + } + + t.Run("Service."+field.Name, func(t *testing.T) { + var svc tailcfg.Service + set(t, reflect.ValueOf(&svc).Elem().FieldByIndex(field.Index)) + + a := &tailcfg.Hostinfo{Services: []tailcfg.Service{{}}} + b := &tailcfg.Hostinfo{Services: []tailcfg.Service{svc}} + assert.False(t, peerHostinfoEqual(a.View(), b.View())) + }) + } + + for _, field := range reflect.VisibleFields(reflect.TypeFor[tailcfg.Location]()) { + t.Run("Location."+field.Name, func(t *testing.T) { + var loc tailcfg.Location + set(t, reflect.ValueOf(&loc).Elem().FieldByIndex(field.Index)) + + a := &tailcfg.Hostinfo{Location: &tailcfg.Location{}} + b := &tailcfg.Hostinfo{Location: &loc} + assert.False(t, peerHostinfoEqual(a.View(), b.View())) + }) + } } func TestNetInfoEqualIgnoringDERP(t *testing.T) { diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 4b5747bd2..627425f94 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -3402,7 +3402,7 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest delta.hostinfoChanged = newHostinfo != nil && !hostinfoEqual(currentNode.Hostinfo, newHostinfo) delta.peerHostinfoChanged = newHostinfo != nil && - !peerHostinfoEqual(currentNode.Hostinfo, newHostinfo) + !peerHostinfoEqual(currentNode.Hostinfo.View(), newHostinfo.View()) delta.dnsMetadataChanged = newHostinfo != nil && (currentNode.Hostinfo == nil || currentNode.Hostinfo.Hostname != newHostinfo.Hostname ||