types/netmap: move some mutations earlier, remove, document some fields

And optimize the Persist setting a bit, allocating later and only mutating
fields when there's been a Node change.

Updates #1909

Change-Id: Iaddfd9e88ef76e1d18e8d0a41926eb44d0955312
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

control/controlclient/direct.go

@@ -54,6 +54,7 @@ import (
 	"tailscale.com/types/ptr"
 	"tailscale.com/types/tkatype"
 	"tailscale.com/util/clientmetric"
+	"tailscale.com/util/cmpx"
 	"tailscale.com/util/multierr"
 	"tailscale.com/util/singleflight"
 	"tailscale.com/util/systemd"
@@ -1098,6 +1099,9 @@ func (c *Direct) sendMapRequest(ctx context.Context, isStreaming bool, nu Netmap
 			setControlKnobsFromNodeAttrs(resp.Node.Capabilities)
 		}
 
+		// Call Node.InitDisplayNames on any changed nodes.
+		initDisplayNames(cmpx.Or(resp.Node, sess.lastNode).View(), &resp)
+
 		nm := sess.netmapForResponse(&resp)
 		if nm.SelfNode == nil {
 			c.logf("MapResponse lacked node")
@@ -1122,14 +1126,16 @@ func (c *Direct) sendMapRequest(ctx context.Context, isStreaming bool, nu Netmap
 			c.lastPrintMap = now
 			c.logf("[v1] new network map[%d]:\n%s", i, nm.VeryConcise())
 		}
-		newPersist := persist.AsStruct()
-		newPersist.NodeID = nm.SelfNode.StableID
-		newPersist.UserProfile = nm.UserProfiles[nm.User]
 
 		c.mu.Lock()
 		// If we are the ones who last updated persist, then we can update it
-		// again. Otherwise, we should not touch it.
+		// again. Otherwise, we should not touch it. Also, it's only worth
-		if persist == c.persist {
+		// change it if the Node info changed.
+		if persist == c.persist && resp.Node != nil {
+			newPersist := persist.AsStruct()
+			newPersist.NodeID = nm.SelfNode.StableID
+			newPersist.UserProfile = nm.UserProfiles[nm.User()]
+
 			c.persist = newPersist.View()
 			persist = c.persist
 		}
@@ -1144,6 +1150,28 @@ func (c *Direct) sendMapRequest(ctx context.Context, isStreaming bool, nu Netmap
 	return nil
 }
 
+// initDisplayNames mutates any tailcfg.Nodes in resp to populate their display names,
+// calling InitDisplayNames on each.
+//
+// The magicDNSSuffix used is based on selfNode.
+func initDisplayNames(selfNode tailcfg.NodeView, resp *tailcfg.MapResponse) {
+	if resp.Node == nil && len(resp.Peers) == 0 && len(resp.PeersChanged) == 0 {
+		// Fast path for a common case (delta updates). No need to compute
+		// magicDNSSuffix.
+		return
+	}
+	magicDNSSuffix := netmap.MagicDNSSuffixOfNodeName(selfNode.Name())
+	if resp.Node != nil {
+		resp.Node.InitDisplayNames(magicDNSSuffix)
+	}
+	for _, n := range resp.Peers {
+		n.InitDisplayNames(magicDNSSuffix)
+	}
+	for _, n := range resp.PeersChanged {
+		n.InitDisplayNames(magicDNSSuffix)
+	}
+}
+
 // decode JSON decodes the res.Body into v. If serverNoiseKey is not specified,
 // it uses the serverKey and mkey to decode the message from the NaCl-crypto-box.
 func decode(res *http.Response, v any, serverKey, serverNoiseKey key.MachinePublic, mkey key.MachinePrivate) error {

control/controlclient/map.go

@@ -67,6 +67,9 @@ func newMapSession(privateNodeKey key.NodePrivate) *mapSession {
 }
 
 func (ms *mapSession) addUserProfile(userID tailcfg.UserID) {
+	if userID == 0 {
+		return
+	}
 	nm := ms.netMapBuilding
 	if _, dup := nm.UserProfiles[userID]; dup {
 		// Already populated it from a previous peer.
@@ -184,7 +187,6 @@ func (ms *mapSession) netmapForResponse(resp *tailcfg.MapResponse) *netmap.Netwo
 		nm.Expiry = node.KeyExpiry
 		nm.Name = node.Name
 		nm.Addresses = filterSelfAddresses(node.Addresses)
-		nm.User = node.User
 		if node.Hostinfo.Valid() {
 			nm.Hostinfo = *node.Hostinfo.AsStruct()
 		}
@@ -195,16 +197,9 @@ func (ms *mapSession) netmapForResponse(resp *tailcfg.MapResponse) *netmap.Netwo
 		}
 	}
 
-	ms.addUserProfile(nm.User)
+	ms.addUserProfile(nm.User())
-	magicDNSSuffix := nm.MagicDNSSuffix()
-	if nm.SelfNode != nil {
-		nm.SelfNode.InitDisplayNames(magicDNSSuffix)
-	}
 	for _, peer := range resp.Peers {
-		peer.InitDisplayNames(magicDNSSuffix)
+		ms.addUserProfile(peer.Sharer)
-		if !peer.Sharer.IsZero() {
-			ms.addUserProfile(peer.Sharer)
-		}
 		ms.addUserProfile(peer.User)
 	}
 	if DevKnob.ForceProxyDNS() {

control/controlclient/map_test.go

@@ -446,10 +446,11 @@ func TestNetmapForResponse(t *testing.T) {
 			ComputedNameWithHost: "foo",
 		}
 		ms := newTestMapSession(t)
-
+		mapRes := &tailcfg.MapResponse{
-		nm1 := ms.netmapForResponse(&tailcfg.MapResponse{
 			Node: someNode,
-		})
+		}
+		initDisplayNames(mapRes.Node.View(), mapRes)
+		nm1 := ms.netmapForResponse(mapRes)
 		if nm1.SelfNode == nil {
 			t.Fatal("nil Node in 1st netmap")
 		}

ipn/ipnlocal/local.go

@@ -703,7 +703,7 @@ func (b *LocalBackend) updateStatus(sb *ipnstate.StatusBuilder, extraLocked func
 			ss.InNetworkMap = true
 			ss.HostName = b.netMap.Hostinfo.Hostname
 			ss.DNSName = b.netMap.Name
-			ss.UserID = b.netMap.User
+			ss.UserID = b.netMap.User()
 			if sn := b.netMap.SelfNode; sn != nil {
 				peerStatusFromNode(ss, sn.View())
 				if c := sn.Capabilities; len(c) > 0 {
@@ -2781,7 +2781,7 @@ func (b *LocalBackend) setPrefsLockedOnEntry(caller string, newp *ipn.Prefs) ipn
 		}
 	}
 	if netMap != nil {
-		newProfile := netMap.UserProfiles[netMap.User]
+		newProfile := netMap.UserProfiles[netMap.User()]
 		if newLoginName := newProfile.LoginName; newLoginName != "" {
 			if !oldp.Persist().Valid() {
 				b.logf("active login: %s", newLoginName)
@@ -3972,7 +3972,7 @@ func (b *LocalBackend) setNetMapLocked(nm *netmap.NetworkMap) {
 	b.dialer.SetNetMap(nm)
 	var login string
 	if nm != nil {
-		login = cmpx.Or(nm.UserProfiles[nm.User].LoginName, "<missing-profile>")
+		login = cmpx.Or(nm.UserProfiles[nm.User()].LoginName, "<missing-profile>")
 	}
 	b.netMap = nm
 	if login != b.activeLogin {
@@ -4323,7 +4323,7 @@ func (b *LocalBackend) peerIsTaildropTargetLocked(p tailcfg.NodeView) bool {
 	if b.netMap == nil || !p.Valid() {
 		return false
 	}
-	if b.netMap.User == p.User() {
+	if b.netMap.User() == p.User() {
 		return true
 	}
 	if p.Addresses().Len() > 0 &&

types/netmap/netmap.go

@@ -30,12 +30,22 @@ type NetworkMap struct {
 	PrivateKey key.NodePrivate
 	Expiry     time.Time
 	// Name is the DNS name assigned to this node.
-	Name          string
+	// It is the MapResponse.Node.Name value and ends with a period.
-	Addresses     []netip.Prefix // same as tailcfg.Node.Addresses (IP addresses of this Node directly)
+	Name string
+
+	// Addresses is SelfNode.Addresses. (IP addresses of this Node directly)
+	//
+	// TODO(bradfitz): remove this field and make this a method.
+	Addresses []netip.Prefix
+
+	// MachineStatus is either tailcfg.MachineAuthorized or tailcfg.MachineUnauthorized,
+	// depending on SelfNode.MachineAuthorized.
+	// TODO(bradfitz): remove this field and make it a method.
 	MachineStatus tailcfg.MachineStatus
-	MachineKey    key.MachinePublic
+
-	Peers         []tailcfg.NodeView // sorted by Node.ID
+	MachineKey key.MachinePublic
-	DNS           tailcfg.DNSConfig
+	Peers      []tailcfg.NodeView // sorted by Node.ID
+	DNS        tailcfg.DNSConfig
 	// TODO(maisem) : replace with View.
 	Hostinfo          tailcfg.Hostinfo
 	PacketFilter      []filter.Match
@@ -66,10 +76,6 @@ type NetworkMap struct {
 	// hash of the latest update message to tick through TKA).
 	TKAHead tka.AUMHash
 
-	// ACLs
-
-	User tailcfg.UserID
-
 	// Domain is the current Tailnet name.
 	Domain string
 
@@ -81,6 +87,15 @@ type NetworkMap struct {
 	UserProfiles map[tailcfg.UserID]tailcfg.UserProfile
 }
 
+// User returns nm.SelfNode.User if nm.SelfNode is non-nil, otherwise it returns
+// 0.
+func (nm *NetworkMap) User() tailcfg.UserID {
+	if nm.SelfNode != nil {
+		return nm.SelfNode.User
+	}
+	return 0
+}
+
 // AnyPeersAdvertiseRoutes reports whether any peer is advertising non-exit node routes.
 func (nm *NetworkMap) AnyPeersAdvertiseRoutes() bool {
 	for _, p := range nm.Peers {
@@ -111,18 +126,26 @@ func (nm *NetworkMap) PeerByTailscaleIP(ip netip.Addr) (peer tailcfg.NodeView, o
 	return tailcfg.NodeView{}, false
 }
 
-// MagicDNSSuffix returns the domain's MagicDNS suffix (even if
+// MagicDNSSuffix returns the domain's MagicDNS suffix (even if MagicDNS isn't
-// MagicDNS isn't necessarily in use).
+// necessarily in use) of the provided Node.Name value.
 //
 // It will neither start nor end with a period.
-func (nm *NetworkMap) MagicDNSSuffix() string {
+func MagicDNSSuffixOfNodeName(nodeName string) string {
-	name := strings.Trim(nm.Name, ".")
+	name := strings.Trim(nodeName, ".")
 	if _, rest, ok := strings.Cut(name, "."); ok {
 		return rest
 	}
 	return name
 }
 
+// MagicDNSSuffix returns the domain's MagicDNS suffix (even if
+// MagicDNS isn't necessarily in use).
+//
+// It will neither start nor end with a period.
+func (nm *NetworkMap) MagicDNSSuffix() string {
+	return MagicDNSSuffixOfNodeName(nm.Name)
+}
+
 // SelfCapabilities returns SelfNode.Capabilities if nm and nm.SelfNode are
 // non-nil. This is a method so we can use it in envknob/logknob without a
 // circular dependency.
@@ -171,12 +194,12 @@ func (nm *NetworkMap) PeerWithStableID(pid tailcfg.StableNodeID) (_ tailcfg.Node
 func (nm *NetworkMap) printConciseHeader(buf *strings.Builder) {
 	fmt.Fprintf(buf, "netmap: self: %v auth=%v",
 		nm.NodeKey.ShortString(), nm.MachineStatus)
-	login := nm.UserProfiles[nm.User].LoginName
+	login := nm.UserProfiles[nm.User()].LoginName
 	if login == "" {
-		if nm.User.IsZero() {
+		if nm.User().IsZero() {
 			login = "?"
 		} else {
-			login = fmt.Sprint(nm.User)
+			login = fmt.Sprint(nm.User())
 		}
 	}
 	fmt.Fprintf(buf, " u=%s", login)
@@ -189,7 +212,7 @@ func (nm *NetworkMap) printConciseHeader(buf *strings.Builder) {
 func (a *NetworkMap) equalConciseHeader(b *NetworkMap) bool {
 	if a.NodeKey != b.NodeKey ||
 		a.MachineStatus != b.MachineStatus ||
-		a.User != b.User ||
+		a.User() != b.User() ||
 		len(a.Addresses) != len(b.Addresses) {
 		return false
 	}