state: treat extra DNS record names as taken node names

hscontrol/app.go

@@ -443,6 +443,7 @@ func (h *Headscale) scheduledTasks(ctx context.Context) {
 			}
 
 			h.cfg.SetExtraRecords(records)
+			h.state.WarnNodesHoldingExtraRecords()
 
 			h.Change(change.ExtraRecords())
 
@@ -646,6 +647,9 @@ func (h *Headscale) Serve() error {
 		defer h.extraRecordMan.Close()
 	}
 
+	// Covers the records from the config too.
+	h.state.WarnNodesHoldingExtraRecords()
+
 	// Start all scheduled tasks, e.g. expiring nodes, derp updates and
 	// records updates
 	scheduleCtx, scheduleCancel := context.WithCancel(context.Background())

hscontrol/servertest/names_compat_test.go

@@ -74,28 +74,16 @@ var nameSkipReasons = map[string]string{
 	"names/N12b/pdb/label": adminNameFollowsHostname,
 	"names/N13/pdb/label":  adminNameFollowsHostname,
 
-	"names/N9a/0/message": caseTwinTakenText,
-	"names/N9b/0/message": caseTwinTakenText,
+	"names/N9a/0/message": nameTakenText,
+	"names/N9b/0/message": nameTakenText,
 
-	"names-svc/S3/g1/label":        extraRecordNotTaken,
-	"names-svc/S4/g1/label":        extraRecordNotTaken,
-	"names-svc/S4/svc-other/label": extraRecordNotTaken,
-	"names-svc/S5/g1/label":        extraRecordNotTaken,
-	"names-svc/S5/svc-other/label": extraRecordNotTaken,
-	"names-svc/S5/0/status":        extraRecordNotTaken,
-	"names-svc/S5/0/message":       recordNameTakenText,
-	"names-svc/S6/g1/label":        extraRecordNotTaken,
-	"names-svc/S6/svc-other/label": extraRecordNotTaken,
-	"names-svc/S7/g1/label":        extraRecordNotTaken,
-	"names-svc/S7/svc-other/label": extraRecordNotTaken,
+	"names-svc/S5/0/message": nameTakenText,
 }
 
 const (
 	labelSuffixOverflow      = "LABEL_SUFFIX_OVERFLOW: headscale appends -N to a 63-byte label, giving a 65-byte one; SaaS cuts the label so the suffix fits in 63 bytes"
 	adminNameFollowsHostname = "ADMIN_NAME_FOLLOWS_HOSTNAME: an admin name equal to the hostname's label counts as derived from it, so a hostname change replaces it; SaaS keeps an admin name across hostname changes"
-	extraRecordNotTaken      = "EXTRA_RECORD_NOT_TAKEN: headscale gives a device the label an extra DNS record holds and accepts a rename to it; SaaS bumps a device past a Service's name and refuses the rename with 400"
-	caseTwinTakenText        = "NAME_TAKEN_TEXT: headscale refuses a rename to a case twin saying the node name is not unique, where SaaS says the name is already taken"
-	recordNameTakenText      = "EXTRA_RECORD_NOT_TAKEN+NAME_TAKEN_TEXT: headscale accepts a rename to an extra record's name; refused, its text says the node name is not unique, where SaaS says the name is already taken"
+	nameTakenText            = "NAME_TAKEN_TEXT: headscale's v2 message for a rename refused as taken is the operation, with the reason only in data; SaaS's message says the name is already taken"
 	v2NameBare               = "V2_NAME_BARE: headscale's v2 device name is the bare label; SaaS's is the FQDN under the tailnet domain"
 )
 

hscontrol/state/node_store.go

@@ -114,6 +114,9 @@ type NodeStore struct {
 
 	peersFunc  PeersFunc
 	writeQueue chan work
+	// reservedLabel reports labels held outside the NodeStore; nil reserves
+	// none. Set before Start: the writer reads it without a lock.
+	reservedLabel func(string) bool
 
 	// stopped is closed once by Stop to signal the writer goroutine to exit
 	// and to let in-flight writes return cleanly instead of panicking with
@@ -522,7 +525,7 @@ func (s *NodeStore) applyBatch(batch []work) {
 		switch w.op {
 		case put:
 			n := w.node
-			n.GivenName = resolveGivenName(nodes, n.ID, n.GivenName)
+			n.GivenName = resolveGivenName(nodes, n.ID, n.GivenName, s.reservedLabel)
 
 			nodes[w.nodeID] = n
 			if w.nodeResult != nil {
@@ -543,7 +546,9 @@ func (s *NodeStore) applyBatch(batch []work) {
 				fn(&n)
 
 				if n.GivenName != oldGivenName {
-					n.GivenName = resolveGivenName(nodes, n.ID, n.GivenName)
+					n.GivenName = resolveGivenName(
+						nodes, n.ID, n.GivenName, s.reservedLabel,
+					)
 				}
 
 				nodes[id] = n
@@ -581,17 +586,7 @@ func (s *NodeStore) applyBatch(batch []work) {
 				continue
 			}
 
-			taken := false
-
-			// DNS is case-insensitive: a legacy "Laptop" already holds "laptop".
-			for id, other := range nodes {
-				if id != w.nodeID && strings.EqualFold(other.GivenName, w.name) {
-					taken = true
-					break
-				}
-			}
-
-			if taken {
+			if resolveGivenName(nodes, w.nodeID, w.name, s.reservedLabel) != w.name {
 				setErrResults[w] = ErrGivenNameTaken
 				nodeResultRequests[w.nodeID] = append(nodeResultRequests[w.nodeID], w)
 
@@ -673,7 +668,12 @@ func (s *NodeStore) applyBatch(batch []work) {
 // unused wins. Must be called from the [NodeStore] writer goroutine
 // (inside [NodeStore.applyBatch]) so the nodes map reflects all earlier
 // ops in the batch and no other writer can interleave.
-func resolveGivenName(nodes map[types.NodeID]types.Node, self types.NodeID, base string) string {
+func resolveGivenName(
+	nodes map[types.NodeID]types.Node,
+	self types.NodeID,
+	base string,
+	reservedLabel func(string) bool,
+) string {
 	if base == "" {
 		base = fallbackGivenName
 	}
@@ -690,7 +690,8 @@ func resolveGivenName(nodes map[types.NodeID]types.Node, self types.NodeID, base
 
 	candidate := base
 	for i := 1; ; i++ {
-		if _, busy := taken[strings.ToLower(candidate)]; !busy {
+		_, busy := taken[strings.ToLower(candidate)]
+		if !busy && (reservedLabel == nil || !reservedLabel(candidate)) {
 			return candidate
 		}
 

hscontrol/state/node_store_hostname_test.go

@@ -7,8 +7,36 @@ import (
 
 	"github.com/juanfont/headscale/hscontrol/types"
 	"github.com/stretchr/testify/require"
+	"tailscale.com/tailcfg"
 )
 
+func TestPutNodeBumpsPastReservedLabel(t *testing.T) {
+	cfg := persistTestConfig(t.TempDir() + "/headscale.db")
+	cfg.TailcfgDNSConfig = &tailcfg.DNSConfig{ExtraRecords: []tailcfg.DNSRecord{
+		{Name: "laptop." + cfg.BaseDomain},
+		{Name: "laptop-1." + cfg.BaseDomain},
+	}}
+	s, err := NewState(cfg)
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = s.Close() })
+
+	node := s.nodeStore.PutNode(createTestNode(1, 1, "alice", "laptop"))
+	require.Equal(t, "laptop-2", node.GivenName())
+}
+
+func TestSetGivenNameRejectsReservedLabel(t *testing.T) {
+	cfg := persistTestConfig(t.TempDir() + "/headscale.db")
+	cfg.TailcfgDNSConfig = &tailcfg.DNSConfig{ExtraRecords: []tailcfg.DNSRecord{
+		{Name: "laptop." + cfg.BaseDomain},
+	}}
+	s, err := NewState(cfg)
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = s.Close() })
+	s.nodeStore.PutNode(createTestNode(1, 1, "alice", "phone"))
+	_, err = s.nodeStore.SetGivenName(1, "laptop")
+	require.ErrorIs(t, err, ErrGivenNameTaken)
+}
+
 // TestPutNodeGivenNameCollisionBumps is the reproduction for #3188.
 // Before the hostname-cleanroom rewrite, two nodes could be written
 // with the same GivenName, producing duplicates. After the rewrite,

hscontrol/state/rename_test.go

@@ -5,9 +5,85 @@ import (
 	"testing"
 
 	"github.com/juanfont/headscale/hscontrol/db"
+	"github.com/juanfont/headscale/hscontrol/types"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"tailscale.com/tailcfg"
+	"tailscale.com/types/key"
 )
 
+func TestNodeNameCannotShadowExtraRecord(t *testing.T) {
+	cfg := persistTestConfig(t.TempDir() + "/headscale.db")
+	cfg.TailcfgDNSConfig = &tailcfg.DNSConfig{ExtraRecords: []tailcfg.DNSRecord{
+		{Name: "grafana." + cfg.BaseDomain, Type: "A", Value: "100.64.0.100"},
+	}}
+	s, err := NewState(cfg)
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = s.Close() })
+
+	user := s.CreateUserForTest("extra-record-user")
+	pak, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil)
+	require.NoError(t, err)
+
+	register := func(hostname string) types.NodeView {
+		t.Helper()
+
+		node, _, err := s.HandleNodeFromPreAuthKey(tailcfg.RegisterRequest{
+			Auth:     &tailcfg.RegisterResponseAuth{AuthKey: pak.Key},
+			NodeKey:  key.NewNode().Public(),
+			Hostinfo: &tailcfg.Hostinfo{Hostname: hostname},
+		}, key.NewMachine().Public())
+		require.NoError(t, err)
+
+		return node
+	}
+	static := register("grafana")
+	assert.Equal(t, "grafana-1", static.GivenName())
+
+	existing := register("existing")
+
+	cfg.SetExtraRecords([]tailcfg.DNSRecord{
+		{Name: "SVC." + cfg.BaseDomain + ".", Type: "A", Value: "100.64.0.101"},
+		{Name: "existing." + cfg.BaseDomain, Type: "A", Value: "100.64.0.102"},
+		{Name: "child.parent." + cfg.BaseDomain, Type: "A", Value: "100.64.0.103"},
+	})
+
+	fresh, _ := s.GetNodeByID(existing.ID())
+	assert.Equal(
+		t,
+		"existing",
+		fresh.GivenName(),
+		"a records reload keeps existing holders",
+	)
+	assert.Equal(t, []types.NodeID{existing.ID()}, s.WarnNodesHoldingExtraRecords())
+
+	dynamic := register("svc")
+	assert.Equal(t, "svc-1", dynamic.GivenName())
+
+	_, _, err = s.RenameNode(existing.ID(), "Svc")
+	require.ErrorIs(t, err, ErrNodeNameNotUnique)
+	_, err = s.UpdateNodeFromMapRequest(existing.ID(), tailcfg.MapRequest{
+		NodeKey: existing.NodeKey(), Hostinfo: &tailcfg.Hostinfo{Hostname: "svc"},
+	})
+	require.NoError(t, err)
+
+	fresh, _ = s.GetNodeByID(existing.ID())
+	assert.Equal(t, "svc-2", fresh.GivenName())
+
+	parent := register("parent")
+	assert.Equal(
+		t,
+		"parent",
+		parent.GivenName(),
+		"only exact record names are reserved",
+	)
+	cfg.SetExtraRecords(nil)
+
+	renamed, _, err := s.RenameNode(existing.ID(), "Svc")
+	require.NoError(t, err)
+	assert.Equal(t, "svc", renamed.GivenName())
+}
+
 // TestRenameNodeRejectsNameExceedingFQDNLimit proves RenameNode rejects a name
 // that is a valid DNS label but whose FQDN, under the configured base_domain,
 // exceeds MaxHostnameLength. Without the FQDN-length gate such a name persists

hscontrol/state/state.go

@@ -397,6 +397,9 @@ func NewState(cfg *types.Config) (*State, error) {
 
 	nodeStore := NewNodeStore(nodes, policyPeersFunc(polMan), batchSize, batchTimeout)
 	polMan.SetRouterSession(nodeStore.RouterSession)
+	nodeStore.reservedLabel = func(label string) bool {
+		return cfg.BaseDomain != "" && cfg.HasExtraRecord(label+"."+cfg.BaseDomain)
+	}
 	nodeStore.Start()
 
 	s := &State{
@@ -1394,6 +1397,27 @@ func (s *State) RenameNode(nodeID types.NodeID, newName string) (types.NodeView,
 	return nodeView, c, nil
 }
 
+// WarnNodesHoldingExtraRecords logs, and returns, the nodes whose name is also
+// an extra DNS record. They keep it: only an operator adding the record can
+// cause this, so the operator picks which one to rename.
+func (s *State) WarnNodesHoldingExtraRecords() []types.NodeID {
+	var ids []types.NodeID
+
+	for _, n := range s.nodeStore.ListNodes().All() {
+		if !s.nodeStore.reservedLabel(n.GivenName()) {
+			continue
+		}
+
+		ids = append(ids, n.ID())
+		log.Warn().
+			Uint64(zf.NodeID, n.ID().Uint64()).
+			Str("record", n.GivenName()+"."+s.cfg.BaseDomain).
+			Msg("node keeps a name that is also an extra DNS record")
+	}
+
+	return ids
+}
+
 // BackfillNodeIPs assigns IP addresses to nodes that don't have them. The
 // returned changes tell clients about the new addresses.
 // Like the other writes, it returns the changes alongside an error once the

hscontrol/types/config.go

@@ -2021,10 +2021,17 @@ func (c *Config) CloneTailcfgDNSConfig() *tailcfg.DNSConfig {
 	return c.TailcfgDNSConfig.Clone()
 }
 
-// extraRecordAtOrBelow reports whether an extra DNS record is name or a
-// subdomain of it. Case is ignored, as in TLS name matching; name is
+// HasExtraRecord reports whether an extra DNS record is exactly name,
+// ignoring case and a trailing dot. Safe for concurrent use with
+// [Config.SetExtraRecords].
+func (c *Config) HasExtraRecord(name string) bool {
+	return c.extraRecordAt(strings.ToLower(strings.TrimSuffix(name, ".")), false)
+}
+
+// extraRecordAt reports whether an extra DNS record is name or, with orBelow,
+// a subdomain of it. Case is ignored, as in TLS name matching; name is
 // lowercase.
-func (c *Config) extraRecordAtOrBelow(name string) bool {
+func (c *Config) extraRecordAt(name string, orBelow bool) bool {
 	tailcfgDNSMu.RLock()
 	defer tailcfgDNSMu.RUnlock()
 
@@ -2034,7 +2041,7 @@ func (c *Config) extraRecordAtOrBelow(name string) bool {
 
 	for _, r := range c.TailcfgDNSConfig.ExtraRecords {
 		rn := strings.ToLower(strings.TrimSuffix(r.Name, "."))
-		if rn == name || strings.HasSuffix(rn, "."+name) {
+		if rn == name || (orBelow && strings.HasSuffix(rn, "."+name)) {
 			return true
 		}
 	}

hscontrol/types/node.go

@@ -926,7 +926,7 @@ func (nv NodeView) CertDomain(cfg *Config) (string, bool) {
 	// certificate for a name the operator gave to a service. A wildcard
 	// order shares its challenge record with the bare name, so names
 	// below the node's are covered too.
-	if cfg.extraRecordAtOrBelow(domain) {
+	if cfg.extraRecordAt(domain, true) {
 		return "", false
 	}