tailcfg: update node display name fields and methods (#1207)

Signed-off-by: Sonia Appasamy <sonia@tailscale.com>

Consolidates the node display name logic from each of the clients into
tailcfg.Node. UI clients can use these names directly, rather than computing
them independently.

control/controlclient/direct.go

@@ -770,12 +770,12 @@ func (c *Direct) sendMapRequest(ctx context.Context, maxPolls int, cb func(*Netw
 		c.mu.Unlock()
 
 		nm := &NetworkMap{
+			SelfNode:        resp.Node,
 			NodeKey:         tailcfg.NodeKey(persist.PrivateNodeKey.Public()),
 			PrivateKey:      persist.PrivateNodeKey,
 			MachineKey:      machinePubKey,
 			Expiry:          resp.Node.KeyExpiry,
 			Name:            resp.Node.Name,
-			DisplayName:     resp.Node.DisplayName,
 			Addresses:       resp.Node.Addresses,
 			Peers:           resp.Peers,
 			LocalPort:       localPort,
@@ -799,10 +799,10 @@ func (c *Direct) sendMapRequest(ctx context.Context, maxPolls int, cb func(*Netw
 			}
 		}
 		addUserProfile(nm.User)
+		magicDNSSuffix := nm.MagicDNSSuffix()
+		nm.SelfNode.InitDisplayNames(magicDNSSuffix)
 		for _, peer := range resp.Peers {
-			if peer.DisplayName == "" {
+			peer.InitDisplayNames(magicDNSSuffix)
-				peer.DisplayName = peer.DefaultDisplayName()
-			}
 			if !peer.Sharer.IsZero() {
 				if c.keepSharerAndUserSplit {
 					addUserProfile(peer.Sharer)
@@ -812,9 +812,6 @@ func (c *Direct) sendMapRequest(ctx context.Context, maxPolls int, cb func(*Netw
 			}
 			addUserProfile(peer.User)
 		}
-		if resp.Node.DisplayName == "" {
-			nm.DisplayName = resp.Node.DefaultDisplayName()
-		}
 		if resp.Node.MachineAuthorized {
 			nm.MachineStatus = tailcfg.MachineAuthorized
 		} else {

control/controlclient/netmap.go

@@ -24,13 +24,12 @@
 type NetworkMap struct {
 	// Core networking
 
+	SelfNode   *tailcfg.Node
 	NodeKey    tailcfg.NodeKey
 	PrivateKey wgkey.Private
 	Expiry     time.Time
 	// Name is the DNS name assigned to this node.
 	Name          string
-	// DisplayName is the title to show for the node in client UIs.
-	DisplayName   string
 	Addresses     []netaddr.IPPrefix
 	LocalPort     uint16 // used for debugging
 	MachineStatus tailcfg.MachineStatus

tailcfg/tailcfg.go

@@ -161,12 +161,6 @@ type Node struct {
 	StableID StableNodeID
 	Name     string // DNS
 
-	// DisplayName is the title to show for the node in client
-	// UIs. This field is assigned by default in controlclient,
-	// but can be overriden by providing this field non-empty
-	// in a MapResponse.
-	DisplayName string `json:",omitempty"`
-
 	// User is the user who created the node. If ACL tags are in
 	// use for the node then it doesn't reflect the ACL identity
 	// that the node is running as.
@@ -190,21 +184,98 @@ type Node struct {
 	KeepAlive bool `json:",omitempty"` // open and keep open a connection to this peer
 
 	MachineAuthorized bool `json:",omitempty"` // TODO(crawshaw): replace with MachineStatus
+
+	// The following three computed fields hold the various names that can
+	// be used for this node in UIs. They are populated from controlclient
+	// (not from control) by calling node.InitDisplayNames. These can be
+	// used directly or accessed via node.DisplayName or node.DisplayNames.
+
+	ComputedName            string `json:",omitempty"` // MagicDNS base name (for normal non-shared-in nodes), FQDN (without trailing dot, for shared-in nodes), or Hostname (if no MagicDNS)
+	computedHostIfDifferent string // hostname, if different than ComputedName, otherwise empty
+	ComputedNameWithHost    string `json:",omitempty"` // either "ComputedName" or "ComputedName (computedHostIfDifferent)", if computedHostIfDifferent is set
 }
 
-// DefaultDisplayName returns a value suitable
+// DisplayName returns the user-facing name for a node which should
-// for using as the default value for n.DisplayName.
+// be shown in client UIs.
-func (n *Node) DefaultDisplayName() string {
+//
-	if n.Name != "" {
+// Parameter forOwner specifies whether the name is requested by
-		// Use the Magic DNS prefix as the default display name.
+// the owner of the node. When forOwner is false, the hostname is
-		return dnsname.ToBaseName(n.Name)
+// never included in the return value.
+//
+// Return value is either either "Name" or "Name (Hostname)", where
+// Name is the node's MagicDNS base name (for normal non-shared-in
+// nodes), FQDN (without trailing dot, for shared-in nodes), or
+// Hostname (if no MagicDNS). Hostname is only included in the
+// return value if it varies from Name and forOwner is provided true.
+//
+// DisplayName is only valid if InitDisplayNames has been called.
+func (n *Node) DisplayName(forOwner bool) string {
+	if forOwner {
+		return n.ComputedNameWithHost
 	}
-	if n.Hostinfo.Hostname != "" {
+	return n.ComputedName
-		// When no Magic DNS name is present, use the hostname.
+}
-		return n.Hostinfo.Hostname
+
+// DisplayName returns the decomposed user-facing name for a node.
+//
+// Parameter forOwner specifies whether the name is requested by
+// the owner of the node. When forOwner is false, hostIfDifferent
+// is always returned empty.
+//
+// Return value name is the node's primary name, populated with the
+// node's MagicDNS base name (for normal non-shared-in nodes), FQDN
+// (without trailing dot, for shared-in nodes), or Hostname (if no
+// MagicDNS).
+//
+// Return value hostIfDifferent, when non-empty, is the node's
+// hostname. hostIfDifferent is only populated when the hostname
+// varies from name and forOwner is provided as true.
+//
+// DisplayNames is only valid if InitDisplayNames has been called.
+func (n *Node) DisplayNames(forOwner bool) (name, hostIfDifferent string) {
+	if forOwner {
+		return n.ComputedName, n.computedHostIfDifferent
 	}
-	// When we've exhausted all other name options, use the node's ID.
+	return n.ComputedName, ""
-	return n.ID.String()
+}
+
+// InitDisplayNames computes and populates n's display name
+// fields: n.ComputedName, n.computedHostIfDifferent, and
+// n.ComputedNameWithHost.
+func (n *Node) InitDisplayNames(networkMagicDNSSuffix string) {
+	dnsName := n.Name
+	if dnsName != "" {
+		dnsName = strings.TrimRight(dnsName, ".")
+		if i := strings.Index(dnsName, "."); i != -1 && dnsname.HasSuffix(dnsName, networkMagicDNSSuffix) {
+			dnsName = dnsName[:i]
+		}
+	}
+
+	name := dnsName
+	hostIfDifferent := n.Hostinfo.Hostname
+
+	if strings.EqualFold(name, hostIfDifferent) {
+		hostIfDifferent = ""
+	}
+	if name == "" {
+		if hostIfDifferent != "" {
+			name = hostIfDifferent
+			hostIfDifferent = ""
+		} else {
+			name = n.Key.String()
+		}
+	}
+
+	var nameWithHost string
+	if hostIfDifferent != "" {
+		nameWithHost = fmt.Sprintf("%s (%s)", name, hostIfDifferent)
+	} else {
+		nameWithHost = name
+	}
+
+	n.ComputedName = name
+	n.computedHostIfDifferent = hostIfDifferent
+	n.ComputedNameWithHost = nameWithHost
 }
 
 type MachineStatus int
@@ -818,7 +889,6 @@ func (n *Node) Equal(n2 *Node) bool {
 		n.ID == n2.ID &&
 		n.StableID == n2.StableID &&
 		n.Name == n2.Name &&
-		n.DisplayName == n2.DisplayName &&
 		n.User == n2.User &&
 		n.Sharer == n2.Sharer &&
 		n.Key == n2.Key &&
@@ -832,7 +902,10 @@ func (n *Node) Equal(n2 *Node) bool {
 		n.Hostinfo.Equal(&n2.Hostinfo) &&
 		n.Created.Equal(n2.Created) &&
 		eqTimePtr(n.LastSeen, n2.LastSeen) &&
-		n.MachineAuthorized == n2.MachineAuthorized
+		n.MachineAuthorized == n2.MachineAuthorized &&
+		n.ComputedName == n2.ComputedName &&
+		n.computedHostIfDifferent == n2.computedHostIfDifferent &&
+		n.ComputedNameWithHost == n2.ComputedNameWithHost
 }
 
 func eqStrings(a, b []string) bool {

tailcfg/tailcfg_clone.go

@@ -64,7 +64,6 @@ func (src *Node) Clone() *Node {
 	ID                      NodeID
 	StableID                StableNodeID
 	Name                    string
-	DisplayName       string
 	User                    UserID
 	Sharer                  UserID
 	Key                     NodeKey
@@ -80,6 +79,9 @@ func (src *Node) Clone() *Node {
 	LastSeen                *time.Time
 	KeepAlive               bool
 	MachineAuthorized       bool
+	ComputedName            string
+	computedHostIfDifferent string
+	ComputedNameWithHost    string
 }{})
 
 // Clone makes a deep copy of Hostinfo.

tailcfg/tailcfg_test.go

@@ -189,10 +189,11 @@ func TestHostinfoEqual(t *testing.T) {
 
 func TestNodeEqual(t *testing.T) {
 	nodeHandles := []string{
-		"ID", "StableID", "Name", "DisplayName", "User", "Sharer",
+		"ID", "StableID", "Name", "User", "Sharer",
 		"Key", "KeyExpiry", "Machine", "DiscoKey",
 		"Addresses", "AllowedIPs", "Endpoints", "DERP", "Hostinfo",
 		"Created", "LastSeen", "KeepAlive", "MachineAuthorized",
+		"ComputedName", "computedHostIfDifferent", "ComputedNameWithHost",
 	}
 	if have := fieldsOf(reflect.TypeOf(Node{})); !reflect.DeepEqual(have, nodeHandles) {
 		t.Errorf("Node.Equal check might be out of sync\nfields: %q\nhandled: %q\n",

util/dnsname/dnsname.go

@@ -17,11 +17,3 @@ func HasSuffix(name, suffix string) bool {
 	nameBase := strings.TrimSuffix(name, suffix)
 	return len(nameBase) < len(name) && strings.HasSuffix(nameBase, ".")
 }
-
-// ToBaseName removes the domain ending from a DNS name of a node.
-func ToBaseName(name string) string {
-	if i := strings.Index(name, "."); i != -1 {
-		return name[:i]
-	}
-	return name
-}

util/dnsname/dnsname_test.go

@@ -26,21 +26,3 @@ func TestHasSuffix(t *testing.T) {
 		}
 	}
 }
-
-func TestToBaseName(t *testing.T) {
-	tests := []struct {
-		name string
-		want string
-	}{
-		{"foo", "foo"},
-		{"foo.com", "foo"},
-		{"foo.example.com.beta.tailscale.net", "foo"},
-		{"computer-a.test.gmail.com.beta.tailscale.net", "computer-a"},
-	}
-	for _, tt := range tests {
-		got := ToBaseName(tt.name)
-		if got != tt.want {
-			t.Errorf("ToBaseName(%q) = %q; want %q", tt.name, got, tt.want)
-		}
-	}
-}