From ea4425d8a979cd381e38bf545869d8ed0b3aa930 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Wed, 23 Aug 2023 12:13:38 -0700 Subject: [PATCH] ipn/ipnlocal, wgengine/magicsock: move UpdateStatus stuff around Upcoming work on incremental netmap change handling will require some replumbing of which subsystems get notified about what. Done naively, it could break "tailscale status --json" visibility later. To make sure I understood the flow of all the updates I was rereading the status code and realized parts of ipnstate.Status were being populated by the wrong subsystems. The engine (wireguard) and magicsock (data plane, NAT traveral) should only populate the stuff that they uniquely know. The WireGuard bits were fine but magicsock was populating stuff stuff that LocalBackend could've better handled, so move it there. Updates #1909 Change-Id: I6d1b95d19a2d1b70fbb3c875fac8ea1e169e8cb0 Signed-off-by: Brad Fitzpatrick --- ipn/ipnlocal/local.go | 18 ++++++++++++++ wgengine/magicsock/magicsock.go | 35 +++++++--------------------- wgengine/magicsock/magicsock_test.go | 10 +++++--- 3 files changed, 34 insertions(+), 29 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index c0dd70528..caac3977c 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -651,6 +651,7 @@ func (b *LocalBackend) UpdateStatus(sb *ipnstate.StatusBuilder) { func (b *LocalBackend) updateStatus(sb *ipnstate.StatusBuilder, extraLocked func(*ipnstate.StatusBuilder)) { b.mu.Lock() defer b.mu.Unlock() + sb.MutateStatus(func(s *ipnstate.Status) { s.Version = version.Long() s.TUN = !b.sys.IsNetstack() @@ -701,7 +702,19 @@ func (b *LocalBackend) updateStatus(sb *ipnstate.StatusBuilder, extraLocked func } } }) + + var tailscaleIPs []netip.Addr + if b.netMap != nil { + for _, addr := range b.netMap.Addresses { + if addr.IsSingleIP() { + sb.AddTailscaleIP(addr.Addr()) + tailscaleIPs = append(tailscaleIPs, addr.Addr()) + } + } + } + sb.MutateSelfStatus(func(ss *ipnstate.PeerStatus) { + ss.OS = version.OS() ss.Online = health.GetInPollNetMap() if b.netMap != nil { ss.InNetworkMap = true @@ -716,6 +729,10 @@ func (b *LocalBackend) updateStatus(sb *ipnstate.StatusBuilder, extraLocked func ss.Capabilities = c.AsSlice() } } + for _, addr := range tailscaleIPs { + ss.TailscaleIPs = append(ss.TailscaleIPs, addr) + } + } else { ss.HostName, _ = os.Hostname() } @@ -783,6 +800,7 @@ func (b *LocalBackend) populatePeerStatusLocked(sb *ipnstate.StatusBuilder) { // peerStatusFromNode copies fields that exist in the Node struct for // current node and peers into the provided PeerStatus. func peerStatusFromNode(ps *ipnstate.PeerStatus, n tailcfg.NodeView) { + ps.PublicKey = n.Key() ps.ID = n.StableID() ps.Created = n.Created() ps.ExitNodeOption = tsaddr.ContainsExitRoutes(n.AllowedIPs()) diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index 7cea3877c..65eb564a8 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -54,7 +54,6 @@ import ( "tailscale.com/util/mak" "tailscale.com/util/ringbuffer" "tailscale.com/util/uniq" - "tailscale.com/version" "tailscale.com/wgengine/capture" ) @@ -2508,54 +2507,38 @@ func simpleDur(d time.Duration) time.Duration { return d.Round(time.Minute) } +// UpdateStatus implements the interface nede by ipnstate.StatusBuilder. +// +// This method adds in the magicsock-specific information only. Most +// of the status is otherwise populated by LocalBackend. func (c *Conn) UpdateStatus(sb *ipnstate.StatusBuilder) { c.mu.Lock() defer c.mu.Unlock() - var tailscaleIPs []netip.Addr - if c.netMap != nil { - tailscaleIPs = make([]netip.Addr, 0, len(c.netMap.Addresses)) - for _, addr := range c.netMap.Addresses { - if !addr.IsSingleIP() { - continue - } - sb.AddTailscaleIP(addr.Addr()) - tailscaleIPs = append(tailscaleIPs, addr.Addr()) - } - } - sb.MutateSelfStatus(func(ss *ipnstate.PeerStatus) { - if !c.privateKey.IsZero() { - ss.PublicKey = c.privateKey.Public() - } else { - ss.PublicKey = key.NodePublic{} - } ss.Addrs = make([]string, 0, len(c.lastEndpoints)) for _, ep := range c.lastEndpoints { ss.Addrs = append(ss.Addrs, ep.Addr.String()) } - ss.OS = version.OS() if c.derpMap != nil { - derpRegion, ok := c.derpMap.Regions[c.myDerp] - if ok { - ss.Relay = derpRegion.RegionCode + if reg, ok := c.derpMap.Regions[c.myDerp]; ok { + ss.Relay = reg.RegionCode } } - ss.TailscaleIPs = tailscaleIPs }) if sb.WantPeers { c.peerMap.forEachEndpoint(func(ep *endpoint) { ps := &ipnstate.PeerStatus{InMagicSock: true} - //ps.Addrs = append(ps.Addrs, n.Endpoints...) ep.populatePeerStatus(ps) sb.AddPeer(ep.publicKey, ps) }) } c.foreachActiveDerpSortedLocked(func(node int, ad activeDerp) { - // TODO(bradfitz): add to ipnstate.StatusBuilder - //f("
  • derp-%v: cr%v,wr%v
    • ", node, simpleDur(now.Sub(ad.createTime)), simpleDur(now.Sub(*ad.lastWrite))) + // TODO(bradfitz): add a method to ipnstate.StatusBuilder + // to include all the DERP connections we have open + // and add it here. See the other caller of foreachActiveDerpSortedLocked. }) } diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go index bd5a07624..ac6a89aa1 100644 --- a/wgengine/magicsock/magicsock_test.go +++ b/wgengine/magicsock/magicsock_test.go @@ -226,6 +226,8 @@ func (s *magicStack) Public() key.NodePublic { return s.privateKey.Public() } +// Status returns a subset of the ipnstate.Status, only involving +// the magicsock-specific parts. func (s *magicStack) Status() *ipnstate.Status { var sb ipnstate.StatusBuilder sb.WantPeers = true @@ -240,9 +242,11 @@ func (s *magicStack) Status() *ipnstate.Status { // address. See meshStacks for one possible source of netmaps and IPs. func (s *magicStack) IP() netip.Addr { for deadline := time.Now().Add(5 * time.Second); time.Now().Before(deadline); time.Sleep(10 * time.Millisecond) { - st := s.Status() - if len(st.TailscaleIPs) > 0 { - return st.TailscaleIPs[0] + s.conn.mu.Lock() + nm := s.conn.netMap + s.conn.mu.Unlock() + if nm != nil && len(nm.Addresses) > 0 { + return nm.Addresses[0].Addr() } } panic("timed out waiting for magicstack to get an IP assigned")