From c60cbca371195583110b691342909e593cc92973 Mon Sep 17 00:00:00 2001 From: Maisem Ali Date: Tue, 3 May 2022 15:07:30 -0700 Subject: [PATCH] control/controlclient: store netinfo and hostinfo separately Currently, when SetNetInfo is called it sets the value on hostinfo.NetInfo. However, when SetHostInfo is called it overwrites the hostinfo field which may mean it also clears out the NetInfo it had just received. This commit stores NetInfo separately and combines it into Hostinfo as needed so that control is always notified of the latest values. Also, remove unused copies of Hostinfo from ipn.Status and controlclient.Auto. Updates #tailscale/corp#4824 (maybe fixes) Signed-off-by: Maisem Ali --- control/controlclient/auto.go | 12 +++---- control/controlclient/controlclient_test.go | 2 +- control/controlclient/direct.go | 36 +++++++++++++-------- control/controlclient/status.go | 7 ++-- ipn/ipnlocal/local.go | 6 +--- 5 files changed, 31 insertions(+), 32 deletions(-) diff --git a/control/controlclient/auto.go b/control/controlclient/auto.go index 87066d628..96b5808f8 100644 --- a/control/controlclient/auto.go +++ b/control/controlclient/auto.go @@ -61,10 +61,9 @@ type Auto struct { loggedIn bool // true if currently logged in loginGoal *LoginGoal // non-nil if some login activity is desired synced bool // true if our netmap is up-to-date - hostinfo *tailcfg.Hostinfo - inPollNetMap bool // true if currently running a PollNetMap - inLiteMapUpdate bool // true if a lite (non-streaming) map request is outstanding - inSendStatus int // number of sendStatus calls currently in progress + inPollNetMap bool // true if currently running a PollNetMap + inLiteMapUpdate bool // true if a lite (non-streaming) map request is outstanding + inSendStatus int // number of sendStatus calls currently in progress state State authCtx context.Context // context used for auth requests @@ -555,9 +554,8 @@ func (c *Auto) SetNetInfo(ni *tailcfg.NetInfo) { if !c.direct.SetNetInfo(ni) { return } - c.logf("NetInfo: %v", ni) - // Send new Hostinfo (which includes NetInfo) to server + // Send new NetInfo to server c.sendNewMapRequest() } @@ -567,7 +565,6 @@ func (c *Auto) sendStatus(who string, err error, url string, nm *netmap.NetworkM loggedIn := c.loggedIn synced := c.synced statusFunc := c.statusFunc - hi := c.hostinfo c.inSendStatus++ c.mu.Unlock() @@ -595,7 +592,6 @@ func (c *Auto) sendStatus(who string, err error, url string, nm *netmap.NetworkM URL: url, Persist: p, NetMap: nm, - Hostinfo: hi, State: state, Err: err, } diff --git a/control/controlclient/controlclient_test.go b/control/controlclient/controlclient_test.go index d7a19d498..a203d3f18 100644 --- a/control/controlclient/controlclient_test.go +++ b/control/controlclient/controlclient_test.go @@ -22,7 +22,7 @@ func fieldsOf(t reflect.Type) (fields []string) { func TestStatusEqual(t *testing.T) { // Verify that the Equal method stays in sync with reality - equalHandles := []string{"LoginFinished", "LogoutFinished", "Err", "URL", "NetMap", "State", "Persist", "Hostinfo"} + equalHandles := []string{"LoginFinished", "LogoutFinished", "Err", "URL", "NetMap", "State", "Persist"} if have := fieldsOf(reflect.TypeOf(Status{})); !reflect.DeepEqual(have, equalHandles) { t.Errorf("Status.Equal check might be out of sync\nfields: %q\nhandled: %q\n", have, equalHandles) diff --git a/control/controlclient/direct.go b/control/controlclient/direct.go index cf77b2a6e..7d9671dfb 100644 --- a/control/controlclient/direct.go +++ b/control/controlclient/direct.go @@ -80,12 +80,12 @@ type Direct struct { sfGroup singleflight.Group // protects noiseClient creation. noiseClient *noiseClient - persist persist.Persist - authKey string - tryingNewKey key.NodePrivate - expiry *time.Time - // hostinfo is mutated in-place while mu is held. + persist persist.Persist + authKey string + tryingNewKey key.NodePrivate + expiry *time.Time hostinfo *tailcfg.Hostinfo // always non-nil + netinfo *tailcfg.NetInfo endpoints []tailcfg.Endpoint everEndpoints bool // whether we've ever had non-empty endpoints localPort uint16 // or zero to mean auto @@ -208,7 +208,12 @@ func NewDirect(opts Options) (*Direct, error) { if opts.Hostinfo == nil { c.SetHostinfo(hostinfo.New()) } else { + ni := opts.Hostinfo.NetInfo + opts.Hostinfo.NetInfo = nil c.SetHostinfo(opts.Hostinfo) + if ni != nil { + c.SetNetInfo(ni) + } } return c, nil } @@ -253,14 +258,11 @@ func (c *Direct) SetNetInfo(ni *tailcfg.NetInfo) bool { c.mu.Lock() defer c.mu.Unlock() - if c.hostinfo == nil { - c.logf("[unexpected] SetNetInfo called with no HostInfo; ignoring NetInfo update: %+v", ni) - return false - } - if reflect.DeepEqual(ni, c.hostinfo.NetInfo) { + if reflect.DeepEqual(ni, c.netinfo) { return false } - c.hostinfo.NetInfo = ni.Clone() + c.netinfo = ni.Clone() + c.logf("NetInfo: %v", ni) return true } @@ -337,6 +339,14 @@ 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, err error) { c.mu.Lock() persist := c.persist @@ -344,7 +354,7 @@ func (c *Direct) doLogin(ctx context.Context, opt loginOpt) (mustRegen bool, new serverKey := c.serverKey serverNoiseKey := c.serverNoiseKey authKey := c.authKey - hi := c.hostinfo.Clone() + hi := c.hostInfoLocked() backendLogID := hi.BackendLogID expired := c.expiry != nil && !c.expiry.IsZero() && c.expiry.Before(c.timeNow()) c.mu.Unlock() @@ -646,7 +656,7 @@ func (c *Direct) sendMapRequest(ctx context.Context, maxPolls int, cb func(*netm serverURL := c.serverURL serverKey := c.serverKey serverNoiseKey := c.serverNoiseKey - hi := c.hostinfo.Clone() + hi := c.hostInfoLocked() backendLogID := hi.BackendLogID localPort := c.localPort var epStrs []string diff --git a/control/controlclient/status.go b/control/controlclient/status.go index 3c137ac98..74a4e57e2 100644 --- a/control/controlclient/status.go +++ b/control/controlclient/status.go @@ -9,7 +9,6 @@ import ( "fmt" "reflect" - "tailscale.com/tailcfg" "tailscale.com/types/empty" "tailscale.com/types/netmap" "tailscale.com/types/persist" @@ -75,9 +74,8 @@ type Status struct { // package, but we have some automated tests elsewhere that need to // use them. Please don't use these fields. // TODO(apenwarr): Unexport or remove these. - State State - Persist *persist.Persist // locally persisted configuration - Hostinfo *tailcfg.Hostinfo // current Hostinfo data + State State + Persist *persist.Persist // locally persisted configuration } // Equal reports whether s and s2 are equal. @@ -92,7 +90,6 @@ func (s *Status) Equal(s2 *Status) bool { s.URL == s2.URL && reflect.DeepEqual(s.Persist, s2.Persist) && reflect.DeepEqual(s.NetMap, s2.NetMap) && - reflect.DeepEqual(s.Hostinfo, s2.Hostinfo) && s.State == s2.State } diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index f5356148e..38c646a7b 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -935,8 +935,7 @@ func (b *LocalBackend) Start(opts ipn.Options) error { httpTestClient := b.httpTestClient if b.hostinfo != nil { - hostinfo.Services = b.hostinfo.Services // keep any previous session and netinfo - hostinfo.NetInfo = b.hostinfo.NetInfo + hostinfo.Services = b.hostinfo.Services // keep any previous services } b.hostinfo = hostinfo b.state = ipn.NoState @@ -2870,9 +2869,6 @@ func (b *LocalBackend) assertClientLocked() { func (b *LocalBackend) setNetInfo(ni *tailcfg.NetInfo) { b.mu.Lock() cc := b.cc - if b.hostinfo != nil { - b.hostinfo.NetInfo = ni.Clone() - } b.mu.Unlock() if cc == nil {