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 <maisem@tailscale.com>

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,
 	}

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)

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)
+	if reflect.DeepEqual(ni, c.netinfo) {
 		return false
 	}
-	if reflect.DeepEqual(ni, c.hostinfo.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

control/controlclient/status.go

@@ -9,7 +9,6 @@
 	"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
 }
 

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 {