From c0c453334a5fa421134767de995c816d7db21811 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 28 Jul 2026 01:46:27 +0000 Subject: [PATCH] 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 --- ipn/ipnlocal/node_backend.go | 16 +++ ipn/ipnlocal/node_backend_test.go | 14 +++ tstest/natlab/vmtest/dns_test.go | 157 +++++++++++++++++++++++++----- 3 files changed, 163 insertions(+), 24 deletions(-) diff --git a/ipn/ipnlocal/node_backend.go b/ipn/ipnlocal/node_backend.go index 874ac04c6..28c352766 100644 --- a/ipn/ipnlocal/node_backend.go +++ b/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() { diff --git a/ipn/ipnlocal/node_backend_test.go b/ipn/ipnlocal/node_backend_test.go index 03b54fe65..991ccf299 100644 --- a/ipn/ipnlocal/node_backend_test.go +++ b/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) + } } diff --git a/tstest/natlab/vmtest/dns_test.go b/tstest/natlab/vmtest/dns_test.go index 217161357..50ad96b7d 100644 --- a/tstest/natlab/vmtest/dns_test.go +++ b/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 -// makes it all the way into an Ubuntu guest's OS resolver: an ExtraRecords -// entry ("extratest.record" → 1.2.3.4) resolves via libc (getent), and the -// "record" search domain lets the bare name "extratest" resolve too. That -// requires tailscaled to have plumbed MagicDNS routes and search domains -// into systemd-resolved. -func TestDNSExtraRecordsSearchDomains(t *testing.T) { +// TestMagicDNS verifies that control-plane DNS config makes it all the +// way into an Ubuntu guest's OS resolver, and that MagicDNS answers +// track netmap changes. Booting a VM is expensive, so this one test +// covers several scenarios in sequence: +// +// - DNSConfig.ExtraRecords resolve via libc (getent), and search +// 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"}, - Routes: map[string][]*dnstype.Resolver{"record": nil}, + Domains: []string{"tailnet.test", "record"}, + 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"} { - // Retry for a bit: tailscaled applies the DNS config to - // systemd-resolved asynchronously after coming up. - if err := tstest.WaitFor(30*time.Second, func() error { - out, err := env.SSHExec(node, "getent hosts "+name) - if err != nil { - return fmt.Errorf("getent hosts %s: %v (%s)", name, err, strings.TrimSpace(out)) - } - 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)) - } - return nil - }); err != nil { - out, _ := env.SSHExec(node, "resolvectl status; cat /etc/resolv.conf") - t.Fatalf("%v\nresolver state:\n%s", err, out) + dt := &dnsTester{t: t, env: env, node: node} + + // ExtraRecords, by full name and via the "record" search domain. + dt.wantResolves("extratest.record", "1.2.3.4") + dt.wantResolves("extratest", "1.2.3.4") + + nodeKey := env.Status(node).Self.PublicKey + cs := env.ControlServer() + + // Add a peer the way control does, as a PeersChanged delta + // carrying the full node. + peerAddr := netip.MustParsePrefix("100.64.7.7/32") + peer := &tailcfg.Node{ + ID: 7777, + 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) } }