ipn/ipnlocal: evict stale node indexes when a delta upsert replaces a peer

When a node is renamed in the admin console, control sends peers a
single MapResponse delta: a PeersChanged entry carrying the full
updated node with its new Name, and no new DNSConfig (MagicDNS
records are computed client-side from peer names). That arrives as a
NodeMutationUpsert, but nodeBackend's upsert path only added the new
node's index entries and never removed the replaced node's, so
nodeByName retained the old name, and nodeByAddr, nodeByKey, and
nodeByStableID could likewise go stale if those fields changed.

Since 7e609b258 the quad-100 resolver serves MagicDNS answers on
demand from those live indexes, so a renamed peer's old name kept
resolving until something rebuilt the indexes from a full netmap,
such as toggling Tailscale off and on.

Evict the replaced node's index entries before adding the new ones.
Also consolidate the natlab DNS coverage into a single TestMagicDNS
that boots one VM and exercises extra records, search domains, and
peer add/rename/remove end to end, injecting the same MapResponse
shapes that production control sends.

Updates tailscale/corp#45631

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

ipn/ipnlocal/node_backend.go

@@ -1100,6 +1100,22 @@ func (nb *nodeBackend) UpdateNetmapDelta(muts []netmap.NodeMutation) (res netmap
 				} else {
 					delete(nb.tsmpLearnedDisco, old.Key())
 				}
+				// Evict index entries derived from the old node value
+				// before re-adding them from the new one below, so a
+				// changed name, address, or key doesn't leave a stale
+				// entry behind. Notably, a node rename in the admin
+				// console arrives as an upsert with a new Name, and a
+				// stale nodeByName entry would keep serving MagicDNS
+				// answers for the old name (tailscale/corp#45631).
+				for _, ipp := range old.Addresses().All() {
+					if ipp.IsSingleIP() {
+						delete(nb.nodeByAddr, ipp.Addr())
+					}
+				}
+				delete(nb.nodeByKey, old.Key())
+				delete(nb.nodeByWGString, old.Key().WireGuardGoString())
+				delete(nb.nodeByStableID, old.StableID())
+				nb.removeNodeNameLocked(old.Name())
 			}
 			mak.Set(&nb.peers, nid, m.Node)
 			for _, ipp := range m.Node.Addresses().All() {

ipn/ipnlocal/node_backend_test.go

@@ -641,4 +641,18 @@ func TestNodeBackendMagicDNSHosts(t *testing.T) {
 		t.Fatal("UpdateNetmapDelta not handled")
 	}
 	wantHost("p3.example.ts.net.", netip.MustParseAddr("100.64.0.3"))
+
+	// Renaming a peer arrives as an upsert of the full node with a
+	// new Name. The old name must stop resolving and the new one
+	// must start (tailscale/corp#45631).
+	p3renamed := p3.Clone()
+	p3renamed.Name = "p3-renamed.example.ts.net."
+	if _, handled := nb.UpdateNetmapDelta([]netmap.NodeMutation{netmap.NodeMutationUpsert{Node: p3renamed.View()}}); !handled {
+		t.Fatal("UpdateNetmapDelta not handled")
+	}
+	wantHost("p3-renamed.example.ts.net.", netip.MustParseAddr("100.64.0.3"))
+	wantHost("p3.example.ts.net.")
+	if fqdn, ok := nb.magicDNSPTR(netip.MustParseAddr("100.64.0.3")); !ok || fqdn != "p3-renamed.example.ts.net." {
+		t.Errorf("magicDNSPTR(100.64.0.3) after rename = %q, %v; want p3's new name", fqdn, ok)
+	}
 }

tstest/natlab/vmtest/dns_test.go

@@ -5,6 +5,7 @@ package vmtest_test
 
 import (
 	"fmt"
+	"net/netip"
 	"strings"
 	"testing"
 	"time"
@@ -14,19 +15,38 @@ import (
 	"tailscale.com/tstest/natlab/vmtest"
 	"tailscale.com/tstest/natlab/vnet"
 	"tailscale.com/types/dnstype"
+	"tailscale.com/types/key"
 )
 
-// TestDNSExtraRecordsSearchDomains verifies that control-plane DNS config
+// TestMagicDNS verifies that control-plane DNS config makes it all the
-// makes it all the way into an Ubuntu guest's OS resolver: an ExtraRecords
+// way into an Ubuntu guest's OS resolver, and that MagicDNS answers
-// entry ("extratest.record" → 1.2.3.4) resolves via libc (getent), and the
+// track netmap changes. Booting a VM is expensive, so this one test
-// "record" search domain lets the bare name "extratest" resolve too. That
+// covers several scenarios in sequence:
-// requires tailscaled to have plumbed MagicDNS routes and search domains
+//
-// into systemd-resolved.
+//   - DNSConfig.ExtraRecords resolve via libc (getent), and search
-func TestDNSExtraRecordsSearchDomains(t *testing.T) {
+//     domains let bare names resolve, which requires tailscaled to
+//     have plumbed MagicDNS routes and search domains into
+//     systemd-resolved.
+//   - A peer added by control resolves by FQDN and by short name,
+//     and its IP reverse-resolves (PTR) to its name.
+//   - A peer renamed by control resolves under its new name only:
+//     the old name must stop resolving, and PTR must track the new
+//     name. When a node is renamed in the admin console, the control
+//     plane sends peers a single MapResponse delta: a PeersChanged
+//     entry containing the full updated Node with the new Name, and
+//     notably no new DNSConfig (MagicDNS records are computed
+//     client-side from peer names). This test injects deltas of
+//     exactly that shape. It reproduces tailscale/corp#45631, where
+//     a renamed node's old name kept resolving.
+//   - A peer removed by control stops resolving.
+func TestMagicDNS(t *testing.T) {
 	env := vmtest.New(t, vmtest.ControlDNS("tailnet.test", &tailcfg.DNSConfig{
 		Proxied: true,
-		Domains: []string{"record"},
+		Domains: []string{"tailnet.test", "record"},
-		Routes:  map[string][]*dnstype.Resolver{"record": nil},
+		Routes: map[string][]*dnstype.Resolver{
+			"tailnet.test": nil,
+			"record":       nil,
+		},
 		ExtraRecords: []tailcfg.DNSRecord{
 			{Name: "extratest.record", Type: "A", Value: "1.2.3.4"},
 		},
@@ -36,21 +56,110 @@ func TestDNSExtraRecordsSearchDomains(t *testing.T) {
 		vmtest.OS(vmtest.Ubuntu2404))
 	env.Start()
 
-	for _, name := range []string{"extratest.record", "extratest"} {
+	dt := &dnsTester{t: t, env: env, node: node}
-		// Retry for a bit: tailscaled applies the DNS config to
+
-		// systemd-resolved asynchronously after coming up.
+	// ExtraRecords, by full name and via the "record" search domain.
-		if err := tstest.WaitFor(30*time.Second, func() error {
+	dt.wantResolves("extratest.record", "1.2.3.4")
-			out, err := env.SSHExec(node, "getent hosts "+name)
+	dt.wantResolves("extratest", "1.2.3.4")
-			if err != nil {
+
-				return fmt.Errorf("getent hosts %s: %v (%s)", name, err, strings.TrimSpace(out))
+	nodeKey := env.Status(node).Self.PublicKey
-			}
+	cs := env.ControlServer()
-			if !strings.Contains(out, "1.2.3.4") {
+
-				return fmt.Errorf("getent hosts %s = %q, want it to contain 1.2.3.4", name, strings.TrimSpace(out))
+	// Add a peer the way control does, as a PeersChanged delta
-			}
+	// carrying the full node.
-			return nil
+	peerAddr := netip.MustParsePrefix("100.64.7.7/32")
-		}); err != nil {
+	peer := &tailcfg.Node{
-			out, _ := env.SSHExec(node, "resolvectl status; cat /etc/resolv.conf")
+		ID:                7777,
-			t.Fatalf("%v\nresolver state:\n%s", err, out)
+		StableID:          "peer1renamed",
+		Name:              "renamee.tailnet.test.",
+		User:              7777,
+		Key:               key.NewNode().Public(),
+		Machine:           key.NewMachine().Public(),
+		DiscoKey:          key.NewDisco().Public(),
+		Addresses:         []netip.Prefix{peerAddr},
+		AllowedIPs:        []netip.Prefix{peerAddr},
+		Hostinfo:          (&tailcfg.Hostinfo{OS: "linux", Hostname: "renamee"}).View(),
+		Cap:               tailcfg.CurrentCapabilityVersion,
+		MachineAuthorized: true,
+	}
+	if !cs.AddRawMapResponse(nodeKey, &tailcfg.MapResponse{
+		PeersChanged: []*tailcfg.Node{peer},
+	}) {
+		t.Fatal("AddRawMapResponse(add peer): node not connected")
+	}
+	dt.wantResolves("renamee.tailnet.test", "100.64.7.7")
+	dt.wantResolves("renamee", "100.64.7.7") // via search domain
+	dt.wantResolves("100.64.7.7", "renamee.tailnet.test")
+
+	// Rename the peer, sending the same delta shape production
+	// control sends: the full node again with only the Name changed.
+	renamed := peer.Clone()
+	renamed.Name = "renamed.tailnet.test."
+	if !cs.AddRawMapResponse(nodeKey, &tailcfg.MapResponse{
+		PeersChanged: []*tailcfg.Node{renamed},
+	}) {
+		t.Fatal("AddRawMapResponse(rename peer): node not connected")
+	}
+	dt.wantResolves("renamed.tailnet.test", "100.64.7.7")
+	dt.wantResolves("renamed", "100.64.7.7")
+	dt.wantResolves("100.64.7.7", "renamed.tailnet.test")
+	dt.wantNXDOMAIN("renamee.tailnet.test")
+	dt.wantNXDOMAIN("renamee")
+
+	// Remove the peer; its name must stop resolving.
+	if !cs.AddRawMapResponse(nodeKey, &tailcfg.MapResponse{
+		PeersRemoved: []tailcfg.NodeID{peer.ID},
+	}) {
+		t.Fatal("AddRawMapResponse(remove peer): node not connected")
+	}
+	dt.wantNXDOMAIN("renamed.tailnet.test")
+}
+
+// dnsTester asserts DNS state in a guest via libc lookups (getent),
+// retrying for a bit because tailscaled applies netmap and DNS config
+// changes asynchronously.
+type dnsTester struct {
+	t    *testing.T
+	env  *vmtest.Env
+	node *vmtest.Node
+}
+
+// wantResolves waits until name resolves and its answer contains want.
+// With an IP address as name, getent does a reverse (PTR) lookup and
+// want is the expected hostname. It flushes systemd-resolved's cache
+// before each attempt so it tests tailscaled's resolver rather than a
+// previously cached answer.
+func (dt *dnsTester) wantResolves(name, want string) {
+	dt.t.Helper()
+	if err := tstest.WaitFor(30*time.Second, func() error {
+		dt.env.SSHExec(dt.node, "resolvectl flush-caches")
+		out, err := dt.env.SSHExec(dt.node, "getent hosts "+name)
+		if err != nil {
+			return fmt.Errorf("getent hosts %s: %v (%s)", name, err, strings.TrimSpace(out))
 		}
+		if !strings.Contains(out, want) {
+			return fmt.Errorf("getent hosts %s = %q, want it to contain %q", name, strings.TrimSpace(out), want)
+		}
+		return nil
+	}); err != nil {
+		out, _ := dt.env.SSHExec(dt.node, "resolvectl status; cat /etc/resolv.conf")
+		dt.t.Fatalf("%v\nresolver state:\n%s", err, out)
+	}
+}
+
+// wantNXDOMAIN waits until name no longer resolves. It flushes
+// systemd-resolved's cache before each attempt so it tests
+// tailscaled's resolver rather than a previously cached answer.
+func (dt *dnsTester) wantNXDOMAIN(name string) {
+	dt.t.Helper()
+	if err := tstest.WaitFor(30*time.Second, func() error {
+		dt.env.SSHExec(dt.node, "resolvectl flush-caches")
+		out, err := dt.env.SSHExec(dt.node, "getent hosts "+name)
+		if err == nil {
+			return fmt.Errorf("%s still resolves: %q", name, strings.TrimSpace(out))
+		}
+		return nil
+	}); err != nil {
+		dt.t.Fatal(err)
 	}
 }