From cc43f4d1de78267dd2eb6734c2a89f05a3cc0372 Mon Sep 17 00:00:00 2001 From: Maisem Ali Date: Mon, 23 Oct 2023 11:43:45 -0700 Subject: [PATCH] ipn/ipnlocal: store HostinfoView on LocalBackend We were storing a `*tailcfg.Hostinfo` which would get mutated all over the place, store a view and store the fields being mutated separately. Updates #cleanup Signed-off-by: Maisem Ali --- ipn/ipnlocal/local.go | 208 ++++++++++++++++++---------------- ipn/ipnlocal/loglines_test.go | 2 +- ipn/ipnlocal/state_test.go | 2 +- 3 files changed, 110 insertions(+), 102 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index ad36fd37b..d78106114 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -157,7 +157,6 @@ type LocalBackend struct { e wgengine.Engine // non-nil; TODO(bradfitz): remove; use sys store ipn.StateStore // non-nil; TODO(bradfitz): remove; use sys dialer *tsdial.Dialer // non-nil; TODO(bradfitz): remove; use sys - pushDeviceToken syncs.AtomicValue[string] backendLogID logid.PublicID unregisterNetMon func() unregisterHealthWatch func() @@ -197,8 +196,22 @@ type LocalBackend struct { shouldInterceptTCPPortAtomic syncs.AtomicValue[func(uint16) bool] numClientStatusCalls atomic.Uint32 - // The mutex protects the following elements. - mu sync.Mutex + // The mutex protects all the following elements. + mu sync.Mutex + + // hostinfo is the up-to-date hostinfo, it is only replaced never mutated in + // place. This contains all values except for those listed below. To get the + // fully populated hostinfo, use b.generateHostinfoForControlLocked(). + // + // TODO(maisem): tailcfg.NetInfo is owned by cc and blended into hostinfo, + // we should move it here too. + hostinfo tailcfg.HostinfoView + // The following values are plugged into a copy of b.hostinfo before it is + // sent to control, they are not set on b.hostinfo itself. + wantIngress bool + services views.Slice[tailcfg.Service] + pushDeviceToken string + conf *conffile.Config // latest parsed config, or nil if not in declarative mode pm *profileManager // mu guards access filterHash deephash.Sum @@ -213,8 +226,6 @@ type LocalBackend struct { state ipn.State capFileSharing bool // whether netMap contains the file sharing capability capTailnetLock bool // whether netMap contains the tailnet lock capability - // hostinfo is mutated in-place while mu is held. - hostinfo *tailcfg.Hostinfo // netMap is the most recently set full netmap from the controlclient. // It can't be mutated in place once set. Because it can't be mutated in place, // delta updates from the control server don't apply to it. Instead, use @@ -1444,8 +1455,8 @@ func (b *LocalBackend) startIsNoopLocked(opts ipn.Options) bool { // * UpdatePrefs // * AuthKey return b.state == ipn.Running && - b.hostinfo != nil && - b.hostinfo.FrontendLogID == opts.FrontendLogID && + b.hostinfo.Valid() && + b.hostinfo.FrontendLogID() == opts.FrontendLogID && opts.LegacyMigrationPrefs == nil && opts.UpdatePrefs == nil && opts.AuthKey == "" @@ -1518,14 +1529,6 @@ func (b *LocalBackend) Start(opts ipn.Options) error { return nil } - hostinfo := hostinfo.New() - applyConfigToHostinfo(hostinfo, b.conf) - hostinfo.BackendLogID = b.backendLogID.String() - hostinfo.FrontendLogID = opts.FrontendLogID - hostinfo.Userspace.Set(b.sys.IsNetstack()) - hostinfo.UserspaceRouter.Set(b.sys.IsNetstackRouter()) - b.logf.JSON(1, "Hostinfo", hostinfo) - // TODO(apenwarr): avoid the need to reinit controlclient. // This will trigger a full relogin/reconfigure cycle every // time a Handle reconnects to the backend. Ideally, we @@ -1536,10 +1539,6 @@ func (b *LocalBackend) Start(opts ipn.Options) error { prevCC := b.resetControlClientLocked() httpTestClient := b.httpTestClient - if b.hostinfo != nil { - hostinfo.Services = b.hostinfo.Services // keep any previous services - } - b.hostinfo = hostinfo b.state = ipn.NoState if err := b.migrateStateLocked(opts.LegacyMigrationPrefs); err != nil { @@ -1557,6 +1556,9 @@ func (b *LocalBackend) Start(opts ipn.Options) error { } b.setAtomicValuesFromPrefsLocked(pv) } + b.initHostinfoLocked(opts) + hiForClient := b.generateHostinfoForControlLocked() + b.logf.JSON(1, "Hostinfo", hiForClient) prefs := b.pm.CurrentPrefs() wantRunning := prefs.WantRunning() @@ -1573,7 +1575,6 @@ func (b *LocalBackend) Start(opts ipn.Options) error { if inServerMode := prefs.ForceDaemon(); inServerMode || runtime.GOOS == "windows" { b.logf("Start: serverMode=%v", inServerMode) } - b.applyPrefsToHostinfoLocked(hostinfo, prefs) b.setNetMapLocked(nil) persistv := prefs.Persist().AsStruct() @@ -1581,6 +1582,7 @@ func (b *LocalBackend) Start(opts ipn.Options) error { persistv = new(persist.Persist) } b.updateFilterLocked(nil, ipn.PrefsView{}) + b.mu.Unlock() if b.portpoll != nil { @@ -1627,7 +1629,7 @@ func (b *LocalBackend) Start(opts ipn.Options) error { Persist: *persistv, ServerURL: serverURL, AuthKey: opts.AuthKey, - Hostinfo: hostinfo, + Hostinfo: hiForClient, HTTPTestClient: httpTestClient, DiscoPublicKey: discoPublic, DebugFlags: debugFlags, @@ -2021,10 +2023,7 @@ func (b *LocalBackend) readPoller() { } b.mu.Lock() - if b.hostinfo == nil { - b.hostinfo = new(tailcfg.Hostinfo) - } - b.hostinfo.Services = sl + b.services = views.SliceOf(sl) b.mu.Unlock() b.doSetHostinfoFilterServices() @@ -2038,28 +2037,25 @@ func (b *LocalBackend) readPoller() { // GetPushDeviceToken returns the push notification device token. func (b *LocalBackend) GetPushDeviceToken() string { - return b.pushDeviceToken.Load() + b.mu.Lock() + defer b.mu.Unlock() + return b.pushDeviceToken } // SetPushDeviceToken sets the push notification device token and informs the // controlclient of the new value. func (b *LocalBackend) SetPushDeviceToken(tk string) { - old := b.pushDeviceToken.Swap(tk) + b.mu.Lock() + old := b.pushDeviceToken + b.pushDeviceToken = tk + b.mu.Unlock() + if old == tk { return } b.doSetHostinfoFilterServices() } -func applyConfigToHostinfo(hi *tailcfg.Hostinfo, c *conffile.Config) { - if c == nil { - return - } - if c.Parsed.Hostname != nil { - hi.Hostname = *c.Parsed.Hostname - } -} - // WatchNotifications subscribes to the ipn.Notify message bus notification // messages. // @@ -2707,14 +2703,11 @@ func (b *LocalBackend) parseWgStatusLocked(s *wgengine.Status) (ret ipn.EngineSt return ret } -// shouldUploadServices reports whether this node should include services -// in Hostinfo. When the user preferences currently request "shields up" -// mode, all inbound connections are refused, so services are not reported. -// Otherwise, shouldUploadServices respects NetMap.CollectServices. -func (b *LocalBackend) shouldUploadServices() bool { - b.mu.Lock() - defer b.mu.Unlock() - +// shouldUploadServicesLocked reports whether this node should include services +// in Hostinfo. When the user preferences currently request "shields up" mode, +// all inbound connections are refused, so services are not reported. Otherwise, +// shouldUploadServices respects NetMap.CollectServices. +func (b *LocalBackend) shouldUploadServicesLocked() bool { p := b.pm.CurrentPrefs() if !p.Valid() || b.netMap == nil { return false // default to safest setting @@ -2940,20 +2933,6 @@ func (b *LocalBackend) SetPrefs(newp *ipn.Prefs) { b.setPrefsLockedOnEntry("SetPrefs", newp) } -// wantIngressLocked reports whether this node has ingress configured. This bool -// is sent to the coordination server (in Hostinfo.WireIngress) as an -// optimization hint to know primarily which nodes are NOT using ingress, to -// avoid doing work for regular nodes. -// -// Even if the user's ServeConfig.AllowFunnel map was manually edited in raw -// mode and contains map entries with false values, sending true (from Len > 0) -// is still fine. This is only an optimization hint for the control plane and -// doesn't affect security or correctness. And we also don't expect people to -// modify their ServeConfig in raw mode. -func (b *LocalBackend) wantIngressLocked() bool { - return b.serveConfig.Valid() && b.serveConfig.HasAllowFunnel() -} - // setPrefsLockedOnEntry requires b.mu be held to call it, but it // unlocks b.mu when done. newp ownership passes to this function. // It returns a readonly copy of the new prefs. @@ -2971,13 +2950,6 @@ func (b *LocalBackend) setPrefsLockedOnEntry(caller string, newp *ipn.Prefs) ipn setExitNodeID(newp, netMap) // We do this to avoid holding the lock while doing everything else. - oldHi := b.hostinfo - newHi := oldHi.Clone() - b.applyPrefsToHostinfoLocked(newHi, newp.View()) - b.hostinfo = newHi - hostInfoChanged := !oldHi.Equal(newHi) - cc := b.cc - // [GRINDER STATS LINE] - please don't remove (used for log parsing) if caller == "SetPrefs" { b.logf("SetPrefs: %v", newp.Pretty()) @@ -3010,6 +2982,14 @@ func (b *LocalBackend) setPrefsLockedOnEntry(caller string, newp *ipn.Prefs) ipn b.logf("failed to save new controlclient state: %v", err) } b.lastProfileID = b.pm.CurrentProfile().ID + + oldHI, newHI := b.hostinfo, b.hostinfo.AsStruct() + b.applyPrefsToHostinfoLocked(newHI) + b.hostinfo = newHI.View() + + hostInfoChanged := !oldHI.Equal(b.hostinfo) + + cc := b.cc b.mu.Unlock() if oldp.ShieldsUp() != newp.ShieldsUp || hostInfoChanged { @@ -3111,6 +3091,9 @@ func (b *LocalBackend) TCPHandlerForDst(src, dst netip.AddrPort) (handler func(c return nil, nil } +// peerAPIServicesLocked returns the list of services that the peer API +// is currently listening on. +// b.mu must be held. func (b *LocalBackend) peerAPIServicesLocked() (ret []tailcfg.Service) { for _, pln := range b.peerAPIListeners { proto := tailcfg.PeerAPI4 @@ -3131,6 +3114,9 @@ func (b *LocalBackend) peerAPIServicesLocked() (ret []tailcfg.Service) { Port: 1, // version }) } + if b.egg { + ret = append(ret, tailcfg.Service{Proto: "egg", Port: 1}) + } return ret } @@ -3141,37 +3127,45 @@ func (b *LocalBackend) peerAPIServicesLocked() (ret []tailcfg.Service) { // painstakingly constructing it in twelvety other places. func (b *LocalBackend) doSetHostinfoFilterServices() { b.mu.Lock() - cc := b.cc - if cc == nil { + if b.cc == nil { // Control client isn't up yet. b.mu.Unlock() return } - if b.hostinfo == nil { - b.mu.Unlock() + cc := b.cc + hi := b.generateHostinfoForControlLocked() + b.mu.Unlock() + if hi == nil { b.logf("[unexpected] doSetHostinfoFilterServices with nil hostinfo") return } - peerAPIServices := b.peerAPIServicesLocked() - if b.egg { - peerAPIServices = append(peerAPIServices, tailcfg.Service{Proto: "egg", Port: 1}) + cc.SetHostinfo(hi) +} + +// generateHostinfoForControlLocked generates a Hostinfo struct for sending to +// control, based on the current state of the backend. +// b.mu must be held. +func (b *LocalBackend) generateHostinfoForControlLocked() *tailcfg.Hostinfo { + if !b.hostinfo.Valid() { + return nil + } + hi := b.hostinfo.AsStruct() + + hi.Services = b.peerAPIServicesLocked() + if b.shouldUploadServicesLocked() { + hi.Services = b.services.AppendTo(hi.Services) } - // TODO(maisem,bradfitz): store hostinfo as a view, not as a mutable struct. - hi := *b.hostinfo // shallow copy - b.mu.Unlock() + // The Hostinfo.WantIngress field tells control whether this node wants to + // be wired up for ingress connections. If harmless if it's accidentally + // true; the actual policy is controlled in tailscaled by ServeConfig. But + // if this is accidentally false, then control may not configure DNS + // properly. This exists as an optimization to control to program fewer DNS + // records that have ingress enabled but are not actually being used. + hi.WireIngress = b.wantIngress - // Make a shallow copy of hostinfo so we can mutate - // at the Service field. - if !b.shouldUploadServices() { - hi.Services = []tailcfg.Service{} - } - // Don't mutate hi.Service's underlying array. Append to - // the slice with no free capacity. - c := len(hi.Services) - hi.Services = append(hi.Services[:c:c], peerAPIServices...) - hi.PushDeviceToken = b.pushDeviceToken.Load() - cc.SetHostinfo(&hi) + hi.PushDeviceToken = b.pushDeviceToken + return hi } // NetMap returns the latest cached network map received from @@ -3827,8 +3821,29 @@ func unmapIPPrefixes(ippsList ...[]netip.Prefix) (ret []netip.Prefix) { return ret } +// initHostinfoLocked constructs a hostinfo struct and assigns it to +// b.hostinfo. // b.mu must be held. -func (b *LocalBackend) applyPrefsToHostinfoLocked(hi *tailcfg.Hostinfo, prefs ipn.PrefsView) { +func (b *LocalBackend) initHostinfoLocked(opts ipn.Options) { + hi := hostinfo.New() + hi.BackendLogID = b.backendLogID.String() + hi.FrontendLogID = opts.FrontendLogID + hi.Userspace.Set(b.sys.IsNetstack()) + hi.UserspaceRouter.Set(b.sys.IsNetstackRouter()) + + if b.conf != nil && b.conf.Parsed.Hostname != nil { + hi.Hostname = *b.conf.Parsed.Hostname + } + b.applyPrefsToHostinfoLocked(hi) + + b.hostinfo = hi.View() +} + +// applyPrefsToHostinfoLocked updates the given hostinfo with the +// current prefs. +// b.mu must be held. +func (b *LocalBackend) applyPrefsToHostinfoLocked(hi *tailcfg.Hostinfo) { + prefs := b.pm.CurrentPrefs() if h := prefs.Hostname(); h != "" { hi.Hostname = h } @@ -3845,14 +3860,6 @@ func (b *LocalBackend) applyPrefsToHostinfoLocked(hi *tailcfg.Hostinfo, prefs ip sshHostKeys = b.getSSHHostKeyPublicStrings() } hi.SSH_HostKeys = sshHostKeys - - // The Hostinfo.WantIngress field tells control whether this node wants to - // be wired up for ingress connections. If harmless if it's accidentally - // true; the actual policy is controlled in tailscaled by ServeConfig. But - // if this is accidentally false, then control may not configure DNS - // properly. This exists as an optimization to control to program fewer DNS - // records that have ingress enabled but are not actually being used. - hi.WireIngress = b.wantIngressLocked() } // enterState transitions the backend into newState, updating internal @@ -4190,8 +4197,7 @@ func (b *LocalBackend) assertClientLocked() { } } -// setNetInfo sets b.hostinfo.NetInfo to ni, and passes ni along to the -// controlclient, if one exists. +// setNetInfo sets the provided NetInfo on the control client, if any. func (b *LocalBackend) setNetInfo(ni *tailcfg.NetInfo) { b.mu.Lock() cc := b.cc @@ -4371,6 +4377,7 @@ func (b *LocalBackend) setTCPPortsInterceptedFromNetmapAndPrefsLocked(prefs ipn. } b.reloadServeConfigLocked(prefs) + var wireIngress bool if b.serveConfig.Valid() { servePorts := make([]uint16, 0, 3) b.serveConfig.RangeOverTCPs(func(port uint16, _ ipn.TCPPortHandlerView) bool { @@ -4387,11 +4394,12 @@ func (b *LocalBackend) setTCPPortsInterceptedFromNetmapAndPrefsLocked(prefs ipn. if !b.sys.IsNetstack() { b.updateServeTCPPortNetMapAddrListenersLocked(servePorts) } + wireIngress = b.serveConfig.HasAllowFunnel() } // Kick off a Hostinfo update to control if WireIngress changed. - if wire := b.wantIngressLocked(); b.hostinfo != nil && b.hostinfo.WireIngress != wire { - b.logf("Hostinfo.WireIngress changed to %v", wire) - b.hostinfo.WireIngress = wire + if b.wantIngress != wireIngress { + b.wantIngress = wireIngress + b.logf("Hostinfo.WireIngress changed to %v", wireIngress) go b.doSetHostinfoFilterServices() } diff --git a/ipn/ipnlocal/loglines_test.go b/ipn/ipnlocal/loglines_test.go index 361791858..41af881e7 100644 --- a/ipn/ipnlocal/loglines_test.go +++ b/ipn/ipnlocal/loglines_test.go @@ -64,7 +64,7 @@ func TestLocalLogLines(t *testing.T) { } defer lb.Shutdown() - lb.hostinfo = &tailcfg.Hostinfo{} + lb.hostinfo = (&tailcfg.Hostinfo{}).View() // hacky manual override of the usual log-on-change behaviour of keylogf lb.keyLogf = logListen.Logf diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index 0ac687bb5..f3c610cfe 100644 --- a/ipn/ipnlocal/state_test.go +++ b/ipn/ipnlocal/state_test.go @@ -915,7 +915,7 @@ func TestEditPrefsHasNoKeys(t *testing.T) { if err != nil { t.Fatalf("NewLocalBackend: %v", err) } - b.hostinfo = &tailcfg.Hostinfo{OS: "testos"} + b.hostinfo = (&tailcfg.Hostinfo{OS: "testos"}).View() b.pm.SetPrefs((&ipn.Prefs{ Persist: &persist.Persist{ PrivateNodeKey: key.NewNode(),