types/netmap, ipn/ipnlocal, control/controlclient: rename NodeMutationAdd to NodeMutationUpsert

NodeMutationAdd was a misleading name: a PeersChanged entry in a
MapResponse can represent either a truly new peer or a full
replacement for an existing peer that couldn't be expressed as a
PeerChangedPatch. Calling it "Add" implied it was always a completely
new node, which is wrong.  (I'd changed my mind on the design of
mapping add/delete events to NodeMutations halfway through #19607 and
forgot to update the name, even though I'd updated half the docs)

Rename it to NodeMutationUpsert to reflect the actual semantics: the
node should be inserted or replaced in the peer map regardless of
whether it already existed.

Updates #19607
Updates #12542

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

types/netmap/nodemut.go

@@ -68,15 +68,17 @@ func (m NodeMutationLastSeen) Apply(n *tailcfg.Node) {
 	n.LastSeen = new(m.LastSeen)
 }
 
-// NodeMutationAdd is a NodeMutation that says a new peer has been added.
-// Apply is a no-op: consumers of NodeMutationAdd must type-switch to handle
-// adds by inserting Node into their peer map.
-type NodeMutationAdd struct {
+// NodeMutationUpsert is a NodeMutation that says a peer's full Node value
+// should be inserted or replaced.
+//
+// Apply is a no-op: consumers of NodeMutationUpsert must type-switch to handle
+// upserts by storing Node in their peer map.
+type NodeMutationUpsert struct {
 	Node tailcfg.NodeView
 }
 
-func (m NodeMutationAdd) NodeIDBeingMutated() tailcfg.NodeID { return m.Node.ID() }
-func (m NodeMutationAdd) Apply(*tailcfg.Node)                {}
+func (m NodeMutationUpsert) NodeIDBeingMutated() tailcfg.NodeID { return m.Node.ID() }
+func (m NodeMutationUpsert) Apply(*tailcfg.Node)                {}
 
 // NodeMutationRemove is a NodeMutation that says a peer has been removed.
 // Apply is a no-op: consumers of NodeMutationRemove must type-switch to handle
@@ -132,9 +134,11 @@ func NodeMutationsFromPatch(p *tailcfg.PeerChange) (_ []NodeMutation, ok bool) {
 // by res. It returns ok=false if res contains any non-delta field as defined
 // by mapResponseContainsNonPatchFields.
 //
-// Adds and removes (from res.PeersChanged / res.PeersRemoved) are emitted as
-// NodeMutationAdd / NodeMutationRemove entries. Callers must type-switch to
-// handle those alongside field mutations.
+// Upserts and removes (from res.PeersChanged / res.PeersRemoved) are emitted
+// as NodeMutationUpsert / NodeMutationRemove entries. A PeersChanged entry can
+// be either a new peer or a full replacement for an existing peer that couldn't
+// be represented as PeerChangedPatch. Callers must type-switch to handle those
+// alongside field mutations.
 func MutationsFromMapResponse(res *tailcfg.MapResponse, now time.Time) (ret []NodeMutation, ok bool) {
 	if now.IsZero() {
 		now = time.Now()
@@ -149,7 +153,7 @@ func MutationsFromMapResponse(res *tailcfg.MapResponse, now time.Time) (ret []No
 	for _, n := range res.PeersChanged {
 		// Any n still in PeersChanged after patchifyPeersChanged is a
 		// truly-new (or replaced) peer.
-		ret = append(ret, NodeMutationAdd{Node: n.View()})
+		ret = append(ret, NodeMutationUpsert{Node: n.View()})
 	}
 	for _, p := range res.PeersChangedPatch {
 		deltas, ok := NodeMutationsFromPatch(p)
@@ -174,7 +178,7 @@ func MutationsFromMapResponse(res *tailcfg.MapResponse, now time.Time) (ret []No
 
 // mapResponseContainsNonPatchFields reports whether res contains any field
 // that can't be expressed as a per-peer NodeMutation (including the new
-// NodeMutationAdd / NodeMutationRemove variants) or via the sibling narrow
+// NodeMutationUpsert / NodeMutationRemove variants) or via the sibling narrow
 // setter methods on the map-session backend (e.g. UpdatePacketFilter).
 //
 // When this returns true, the caller must fall back to rebuilding and
@@ -182,7 +186,7 @@ func MutationsFromMapResponse(res *tailcfg.MapResponse, now time.Time) (ret []No
 // handled incrementally.
 //
 // PeersChanged, PeersRemoved, and PacketFilter(s) are intentionally not in
-// this list: new/removed peers ride NodeMutationAdd/Remove, packet
+// this list: upserted/removed peers ride NodeMutationUpsert/Remove, packet
 // filter updates are delivered via the backend's UpdatePacketFilter
 // method, and UserProfile updates ride the backend's UpdateUserProfiles
 // method.

types/netmap/nodemut_test.go

@@ -55,7 +55,7 @@ func TestMapResponseContainsNonPatchFields(t *testing.T) {
 			// The three legacy delta fields handled via NodeMutation patches.
 			want = false
 		case "PeersChanged", "PeersRemoved":
-			// Now carried as NodeMutationAdd / NodeMutationRemove entries.
+			// Now carried as NodeMutationUpsert / NodeMutationRemove entries.
 			want = false
 		case "PacketFilter", "PacketFilters":
 			// Now delivered separately via PacketFilterUpdater.
@@ -196,7 +196,7 @@ func TestMutationsFromMapResponse(t *testing.T) {
 			mr: &tailcfg.MapResponse{
 				PeersChanged: []*tailcfg.Node{{ID: 7}},
 			},
-			want: muts(NodeMutationAdd{Node: (&tailcfg.Node{ID: 7}).View()}),
+			want: muts(NodeMutationUpsert{Node: (&tailcfg.Node{ID: 7}).View()}),
 		},
 		{
 			name: "add-and-remove-mixed-with-patch",
@@ -211,7 +211,7 @@ func TestMutationsFromMapResponse(t *testing.T) {
 			want: muts(
 				NodeMutationRemove{3},
 				NodeMutationDERPHome{5, 2},
-				NodeMutationAdd{Node: (&tailcfg.Node{ID: 7}).View()},
+				NodeMutationUpsert{Node: (&tailcfg.Node{ID: 7}).View()},
 			),
 		},
 	}