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) } }