From b2cd13146c3650ef05fba06712465a4301196479 Mon Sep 17 00:00:00 2001 From: Maisem Ali Date: Mon, 23 Oct 2023 14:20:15 -0700 Subject: [PATCH] ipn/ipnlocal: move tailcfg.Netinfo ownership to LocalBackend The only thing place which owned the copy of Netinfo was the controlclient, which meant that we had another place where we were modifying the hostinfo and blending fields in. This moves all of that logic to now occur solely in LocalBackend. Updates #cleanup Signed-off-by: Maisem Ali --- control/controlclient/auto.go | 12 ---------- control/controlclient/client.go | 6 ----- control/controlclient/direct.go | 41 ++++----------------------------- ipn/ipnlocal/local.go | 20 ++++++++-------- 4 files changed, 13 insertions(+), 66 deletions(-) diff --git a/control/controlclient/auto.go b/control/controlclient/auto.go index fa5e2e106..10f5a3781 100644 --- a/control/controlclient/auto.go +++ b/control/controlclient/auto.go @@ -564,18 +564,6 @@ func (c *Auto) SetHostinfo(hi *tailcfg.Hostinfo) { c.updateControl() } -func (c *Auto) SetNetInfo(ni *tailcfg.NetInfo) { - if ni == nil { - panic("nil NetInfo") - } - if !c.direct.SetNetInfo(ni) { - return - } - - // Send new NetInfo to server - c.updateControl() -} - // SetTKAHead updates the TKA head hash that map-request infrastructure sends. func (c *Auto) SetTKAHead(headHash string) { if !c.direct.SetTKAHead(headHash) { diff --git a/control/controlclient/client.go b/control/controlclient/client.go index ef5af68c6..1bd087e29 100644 --- a/control/controlclient/client.go +++ b/control/controlclient/client.go @@ -65,12 +65,6 @@ type Client interface { // in a separate http request. It has nothing to do with the rest of // the state machine. SetHostinfo(*tailcfg.Hostinfo) - // SetNetinfo changes the NetIinfo structure that will be sent in - // subsequent node registration requests. - // TODO: a server-side change would let us simply upload this - // in a separate http request. It has nothing to do with the rest of - // the state machine. - SetNetInfo(*tailcfg.NetInfo) // SetTKAHead changes the TKA head hash value that will be sent in // subsequent netmap requests. SetTKAHead(headHash string) diff --git a/control/controlclient/direct.go b/control/controlclient/direct.go index 80f6e919b..1b1ed78f5 100644 --- a/control/controlclient/direct.go +++ b/control/controlclient/direct.go @@ -20,7 +20,6 @@ import ( "net/netip" "net/url" "os" - "reflect" "runtime" "slices" "strings" @@ -50,7 +49,6 @@ import ( "tailscale.com/types/logger" "tailscale.com/types/netmap" "tailscale.com/types/persist" - "tailscale.com/types/ptr" "tailscale.com/types/tkatype" "tailscale.com/util/clientmetric" "tailscale.com/util/multierr" @@ -93,8 +91,7 @@ type Direct struct { authKey string tryingNewKey key.NodePrivate expiry time.Time // or zero value if none/unknown - hostinfo *tailcfg.Hostinfo // always non-nil - netinfo *tailcfg.NetInfo + hostinfo *tailcfg.Hostinfo // always non-nil, never mutated only replaced endpoints []tailcfg.Endpoint tkaHead string lastPingURL string // last PingRequest.URL received, for dup suppression @@ -289,9 +286,6 @@ func NewDirect(opts Options) (*Direct, error) { c.SetHostinfo(hostinfo.New()) } else { c.SetHostinfo(opts.Hostinfo) - if ni := opts.Hostinfo.NetInfo; ni != nil { - c.SetNetInfo(ni) - } } if opts.NoiseTestClient != nil { c.noiseClient = &NoiseClient{ @@ -321,8 +315,6 @@ func (c *Direct) SetHostinfo(hi *tailcfg.Hostinfo) bool { if hi == nil { panic("nil Hostinfo") } - hi = ptr.To(*hi) - hi.NetInfo = nil c.mu.Lock() defer c.mu.Unlock() @@ -335,24 +327,7 @@ func (c *Direct) SetHostinfo(hi *tailcfg.Hostinfo) bool { return true } -// SetNetInfo clones the provided NetInfo and remembers it for the -// next update. It reports whether the NetInfo has changed. -func (c *Direct) SetNetInfo(ni *tailcfg.NetInfo) bool { - if ni == nil { - panic("nil NetInfo") - } - c.mu.Lock() - defer c.mu.Unlock() - - if reflect.DeepEqual(ni, c.netinfo) { - return false - } - c.netinfo = ni.Clone() - c.logf("NetInfo: %v", ni) - return true -} - -// SetNetInfo stores a new TKA head value for next update. +// SetTKAHead stores a new TKA head value for next update. // It reports whether the TKA head changed. func (c *Direct) SetTKAHead(tkaHead string) bool { c.mu.Lock() @@ -445,14 +420,6 @@ type httpClient interface { Do(req *http.Request) (*http.Response, error) } -// hostInfoLocked returns a Clone of c.hostinfo and c.netinfo. -// It must only be called with c.mu held. -func (c *Direct) hostInfoLocked() *tailcfg.Hostinfo { - hi := c.hostinfo.Clone() - hi.NetInfo = c.netinfo.Clone() - return hi -} - func (c *Direct) doLogin(ctx context.Context, opt loginOpt) (mustRegen bool, newURL string, nks tkatype.MarshaledSignature, err error) { c.mu.Lock() persist := c.persist.AsStruct() @@ -460,7 +427,7 @@ func (c *Direct) doLogin(ctx context.Context, opt loginOpt) (mustRegen bool, new serverKey := c.serverKey serverNoiseKey := c.serverNoiseKey authKey, isWrapped, wrappedSig, wrappedKey := decodeWrappedAuthkey(c.authKey, c.logf) - hi := c.hostInfoLocked() + hi := c.hostinfo backendLogID := hi.BackendLogID expired := !c.expiry.IsZero() && c.expiry.Before(c.clock.Now()) c.mu.Unlock() @@ -849,7 +816,7 @@ func (c *Direct) sendMapRequest(ctx context.Context, isStreaming bool, nu Netmap serverURL := c.serverURL serverKey := c.serverKey serverNoiseKey := c.serverNoiseKey - hi := c.hostInfoLocked() + hi := c.hostinfo backendLogID := hi.BackendLogID var epStrs []string var eps []netip.AddrPort diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index d78106114..16282c564 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -19,6 +19,7 @@ import ( "os" "os/user" "path/filepath" + "reflect" "runtime" "slices" "sort" @@ -202,15 +203,13 @@ type LocalBackend struct { // 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 + netinfo *tailcfg.NetInfo conf *conffile.Config // latest parsed config, or nil if not in declarative mode pm *profileManager // mu guards access @@ -3122,9 +3121,6 @@ func (b *LocalBackend) peerAPIServicesLocked() (ret []tailcfg.Service) { // doSetHostinfoFilterServices calls SetHostinfo on the controlclient, // possibly after mangling the given hostinfo. -// -// TODO(danderson): we shouldn't be mangling hostinfo here after -// painstakingly constructing it in twelvety other places. func (b *LocalBackend) doSetHostinfoFilterServices() { b.mu.Lock() if b.cc == nil { @@ -3150,6 +3146,7 @@ func (b *LocalBackend) generateHostinfoForControlLocked() *tailcfg.Hostinfo { return nil } hi := b.hostinfo.AsStruct() + hi.NetInfo = b.netinfo hi.Services = b.peerAPIServicesLocked() if b.shouldUploadServicesLocked() { @@ -4200,13 +4197,14 @@ func (b *LocalBackend) assertClientLocked() { // setNetInfo sets the provided NetInfo on the control client, if any. func (b *LocalBackend) setNetInfo(ni *tailcfg.NetInfo) { b.mu.Lock() - cc := b.cc - b.mu.Unlock() - - if cc == nil { + if b.netinfo != nil && ni != nil && reflect.DeepEqual(ni, b.netinfo) { + b.mu.Unlock() return } - cc.SetNetInfo(ni) + b.netinfo = ni.Clone() + b.mu.Unlock() + + b.doSetHostinfoFilterServices() } func hasCapability(nm *netmap.NetworkMap, cap tailcfg.NodeCapability) bool {