From 95efb8cef1efa8d470a1aa453be44079aaaac43b Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 17 Oct 2026 08:58:37 +0000 Subject: [PATCH] state: assign and persist unique node names in the NodeStore Reserve extra DNS names, fit collision suffixes within 63 bytes, and repair duplicate startup names in memory. Persist registration names without losing a created node on rename failure; remove unused DB renaming. --- hscontrol/app.go | 4 + hscontrol/db/node.go | 31 ----- hscontrol/poll_test.go | 62 ++++++++- hscontrol/servertest/names_compat_test.go | 34 +---- hscontrol/state/node_store.go | 98 ++++++++++--- hscontrol/state/node_store_hostname_test.go | 76 ++++++++++ hscontrol/state/persist_test.go | 147 +++++++++++--------- hscontrol/state/rename_test.go | 76 ++++++++++ hscontrol/state/state.go | 56 ++++++-- hscontrol/types/config.go | 15 +- hscontrol/types/node.go | 2 +- 11 files changed, 437 insertions(+), 164 deletions(-) diff --git a/hscontrol/app.go b/hscontrol/app.go index 708a066e5..ab1c9cd10 100644 --- a/hscontrol/app.go +++ b/hscontrol/app.go @@ -451,6 +451,7 @@ func (h *Headscale) scheduledTasks(ctx context.Context) { } h.cfg.SetExtraRecords(records) + h.state.WarnNodesHoldingExtraRecords() h.Change(change.ExtraRecords()) @@ -654,6 +655,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()) diff --git a/hscontrol/db/node.go b/hscontrol/db/node.go index 38f85bb68..5b791afb6 100644 --- a/hscontrol/db/node.go +++ b/hscontrol/db/node.go @@ -28,9 +28,6 @@ const ( defaultTestNodePrefix = "testnode" ) -// ErrNodeNameNotUnique is returned when a node name is not unique. -var ErrNodeNameNotUnique = errors.New("node name is not unique") - // preloadNode returns a session that eager-loads a node's AuthKey, the // AuthKey's User, and the node's User. The AuthKey is filtered to pre-auth // keys: auth_key_id references the shared credentials table, and only that @@ -184,34 +181,6 @@ func SetLastSeen(tx *gorm.DB, nodeID types.NodeID, lastSeen time.Time) error { return tx.Model(&types.Node{}).Where("id = ?", nodeID).Update("last_seen", lastSeen).Error } -// RenameNode takes a [types.Node] struct and a new [types.Node.GivenName] for the nodes -// and renames it. Validation should be done in the state layer before calling this function. -func RenameNode(tx *gorm.DB, - nodeID types.NodeID, newName string, -) error { - err := dnsname.ValidLabel(newName) - if err != nil { - return fmt.Errorf("renaming node: %w", err) - } - - // Check if the new name is unique - var count int64 - - if err := tx.Model(&types.Node{}).Where("given_name = ? AND id != ?", newName, nodeID).Count(&count).Error; err != nil { //nolint:noinlineerr - return fmt.Errorf("checking name uniqueness: %w", err) - } - - if count > 0 { - return ErrNodeNameNotUnique - } - - if err := tx.Model(&types.Node{}).Where("id = ?", nodeID).Update("given_name", newName).Error; err != nil { //nolint:noinlineerr - return fmt.Errorf("renaming node in database: %w", err) - } - - return nil -} - func (hsdb *HSDatabase) NodeSetExpiry(nodeID types.NodeID, expiry *time.Time) error { return hsdb.Write(func(tx *gorm.DB) error { return NodeSetExpiry(tx, nodeID, expiry) diff --git a/hscontrol/poll_test.go b/hscontrol/poll_test.go index 620356046..8d2e144ff 100644 --- a/hscontrol/poll_test.go +++ b/hscontrol/poll_test.go @@ -27,7 +27,67 @@ import ( "tailscale.com/types/views" ) -var errRegistrationGCPolicy = errors.New("node policy update failed") +var ( + errRegistrationGCPolicy = errors.New("node policy update failed") + errRegistrationGCName = errors.New("node name persistence failed") +) + +func TestEphemeralGCArmedAfterNamePersistFailure(t *testing.T) { + t.Parallel() + + app := createTestApp(t) + app.cfg.Node.Ephemeral.InactivityTimeout = time.Hour + app.StartEphemeralGCForTest(t) + user := app.state.CreateUserForTest("gc-name-user") + pak, err := app.state.CreatePreAuthKey(user.TypedID(), true, true, nil, nil) + require.NoError(t, err) + + req := tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: pak.Key}, + NodeKey: key.NewNode().Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "shared"}, + } + _, err = app.handleRegisterWithAuthKey(req, key.NewMachine().Public()) + require.NoError(t, err) + + first, ok := app.state.GetNodeByNodeKey(req.NodeKey) + require.True(t, ok) + + require.NoError(t, app.state.DB().DB.Callback().Update().Before("gorm:update"). + Register("fail_gc_name_persist", func(tx *gorm.DB) { + if tx.Statement.Table == "nodes" { + _ = tx.AddError(errRegistrationGCName) + } + })) + t.Cleanup(func() { + _ = app.state.DB().DB.Callback().Update().Remove("fail_gc_name_persist") + }) + + app.cfg.Node.Ephemeral.InactivityTimeout = 200 * time.Millisecond + req.NodeKey = key.NewNode().Public() + _, err = app.handleRegisterWithAuthKey(req, key.NewMachine().Public()) + require.ErrorIs(t, err, errRegistrationGCName) + + second, ok := app.state.GetNodeByNodeKey(req.NodeKey) + require.True(t, ok, "registration committed before the name UPDATE failed") + require.Equal(t, "shared-1", second.GivenName()) + assert.True(t, app.ephemeralGC.IsScheduled(second.ID())) + row, err := app.state.DB().GetNodeByID(second.ID()) + require.NoError(t, err) + assert.Equal(t, "shared", row.GivenName) + + require.Eventually(t, func() bool { + _, ok := app.state.GetNodeByID(second.ID()) + return !ok + }, 5*time.Second, 10*time.Millisecond, + "a committed registration must be collected after a name persistence error") + + _, err = app.state.DB().GetNodeByID(second.ID()) + require.ErrorIs(t, err, gorm.ErrRecordNotFound) + + _, ok = app.state.GetNodeByID(first.ID()) + require.True(t, ok) +} type registrationGCPolicyManager struct { policy.PolicyManager diff --git a/hscontrol/servertest/names_compat_test.go b/hscontrol/servertest/names_compat_test.go index 9762504ac..da3bf98b8 100644 --- a/hscontrol/servertest/names_compat_test.go +++ b/hscontrol/servertest/names_compat_test.go @@ -31,19 +31,6 @@ import ( // ///. A listed check that // matches fails, so a fix must delete its entry. var nameSkipReasons = map[string]string{ - "names/N1/n63b/label": labelSuffixOverflow, - "names/N2/n63b/label": labelSuffixOverflow, - "names/N3/n63b/label": labelSuffixOverflow, - "names/N4/n63b/label": labelSuffixOverflow, - "names/N5/n63b/label": labelSuffixOverflow, - "names/N6/n63b/label": labelSuffixOverflow, - "names/N7/n63b/label": labelSuffixOverflow, - "names/N8/n63b/label": labelSuffixOverflow, - "names/N9a/n63b/label": labelSuffixOverflow, - "names/N9b/n63b/label": labelSuffixOverflow, - "names/N10/n63b/label": labelSuffixOverflow, - "names/N11/n63b/label": labelSuffixOverflow, - "names/N1/domain": v2NameBare, "names/N2/domain": v2NameBare, "names/N3/domain": v2NameBare, @@ -74,28 +61,15 @@ 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" ) diff --git a/hscontrol/state/node_store.go b/hscontrol/state/node_store.go index 3bd44f6c7..27d810664 100644 --- a/hscontrol/state/node_store.go +++ b/hscontrol/state/node_store.go @@ -1,6 +1,7 @@ package state import ( + "cmp" "errors" "fmt" "maps" @@ -13,8 +14,11 @@ import ( "time" "github.com/juanfont/headscale/hscontrol/types" + "github.com/juanfont/headscale/hscontrol/util" + "github.com/juanfont/headscale/hscontrol/util/zlog/zf" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promauto" + "github.com/rs/zerolog/log" "tailscale.com/net/tsaddr" "tailscale.com/types/key" "tailscale.com/types/views" @@ -114,6 +118,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 @@ -126,10 +133,7 @@ type NodeStore struct { } func NewNodeStore(allNodes types.Nodes, peersFunc PeersFunc, batchSize int, batchTimeout time.Duration) *NodeStore { - nodes := make(map[types.NodeID]types.Node, len(allNodes)) - for _, n := range allNodes { - nodes[n.ID] = *n - } + nodes := uniqueGivenNames(allNodes) snap := snapshotFromNodes(nodes, peersFunc, nil, false, false) @@ -147,6 +151,47 @@ func NewNodeStore(allNodes types.Nodes, peersFunc PeersFunc, batchSize int, batc return store } +// uniqueGivenNames bumps duplicate names as nodes load; the lowest ID keeps a +// shared name. Nothing is written back, and extra record names are not checked. +func uniqueGivenNames(allNodes types.Nodes) map[types.NodeID]types.Node { + ordered := slices.SortedFunc(slices.Values(allNodes), func(a, b *types.Node) int { + return cmp.Compare(a.ID, b.ID) + }) + nodes := make(map[types.NodeID]types.Node, len(allNodes)) + taken := make(map[string]bool, len(allNodes)) + + var duplicates types.Nodes + + // Keep distinct stored labels before bumping duplicates so a label with + // a suffix already has its holder when the duplicate chooses a new name. + for _, n := range ordered { + name := strings.ToLower(n.GivenName) + if name != "" && taken[name] { + duplicates = append(duplicates, n) + continue + } + + nodes[n.ID] = *n + + if name != "" { + taken[name] = true + } + } + + for _, duplicate := range duplicates { + n := *duplicate + n.GivenName = resolveGivenName(nodes, n.ID, n.GivenName, nil) + nodes[n.ID] = n + log.Warn(). + Uint64(zf.NodeID, n.ID.Uint64()). + Str(zf.OldGivenName, duplicate.GivenName). + Str(zf.NewGivenName, n.GivenName). + Msg("renaming duplicate node name in memory") + } + + return nodes +} + // Snapshot is the representation of the current state of the [NodeStore]. // It contains all nodes and their relationships. // It is a copy-on-write structure, meaning that when a write occurs, @@ -522,7 +567,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 +588,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 +628,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) @@ -670,10 +707,16 @@ func (s *NodeStore) applyBatch(batch []work) { // keeps the current label. // // On collision the label is bumped as base, base-1, base-2, …, first -// 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 { +// unused wins; the base is cut so base-N stays a valid label, as Tailscale +// does. Called before the [NodeStore] writer starts when loading nodes, +// or from its 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, + reservedLabel func(string) bool, +) string { if base == "" { base = fallbackGivenName } @@ -690,14 +733,23 @@ 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 } - candidate = base + "-" + strconv.Itoa(i) + candidate = bumpedLabel(base, i) } } +// bumpedLabel is base with a -n suffix, base cut so the result stays a valid +// label, as Tailscale does. +func bumpedLabel(base string, n int) string { + suffix := "-" + strconv.Itoa(n) + + return base[:min(len(base), util.LabelHostnameLength-len(suffix))] + suffix +} + // snapshotFromNodes builds a Snapshot from nodes. With reusePeers the // previous peer-ID adjacency is carried over unchanged; with reuseRoutes // the previous route election is. ListPeers resolves adjacency IDs diff --git a/hscontrol/state/node_store_hostname_test.go b/hscontrol/state/node_store_hostname_test.go index f95c546e2..f9a01d88d 100644 --- a/hscontrol/state/node_store_hostname_test.go +++ b/hscontrol/state/node_store_hostname_test.go @@ -2,13 +2,68 @@ package state import ( "fmt" + "strings" "sync" "testing" "github.com/juanfont/headscale/hscontrol/types" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "tailscale.com/tailcfg" ) +func TestNewNodeStoreBumpsDuplicateNames(t *testing.T) { + nodes := types.Nodes{ + {ID: 4, GivenName: "DB"}, + {ID: 2, GivenName: "db"}, + {ID: 1, GivenName: "db"}, + {ID: 3, GivenName: "db-1"}, + {ID: 5, GivenName: ""}, + {ID: 6, GivenName: ""}, + } + want := map[types.NodeID]string{ + 1: "db", 2: "db-2", 3: "db-1", 4: "DB-3", 5: "", 6: "", + } + + for range 2 { + store := NewNodeStore(nodes, allowAllPeersFunc, TestBatchSize, TestBatchTimeout) + for id, name := range want { + node, ok := store.GetNode(id) + require.True(t, ok) + assert.Equal(t, name, node.GivenName()) + } + } + + assert.Equal(t, "db", nodes[1].GivenName, "loading must not mutate the input rows") +} + +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, @@ -33,6 +88,27 @@ func TestPutNodeGivenNameCollisionBumps(t *testing.T) { require.Equal(t, "laptop-2", got3.GivenName(), "third registration bumps to -2") } +func TestPutNodeBumpCutsLabelToFitSuffix(t *testing.T) { + store := NewNodeStore(nil, allowAllPeersFunc, TestBatchSize, TestBatchTimeout) + + store.Start() + defer store.Stop() + + long := strings.Repeat("a", 63) + + got := make([]string, 0, 12) + for id := range types.NodeID(12) { + got = append(got, store.PutNode(createTestNode(id+1, 1, "alice", long)).GivenName()) + } + + require.Equal(t, long, got[0]) + require.Equal(t, long[:61]+"-1", got[1]) + require.Equal(t, long[:60]+"-10", got[10]) + require.Equal(t, long[:60]+"-11", got[11]) + require.True(t, isAutoDerivedGivenName(got[10], long), + "a cut label still follows its hostname") +} + // TestPutNodeEmptyGivenNameFallsBackToNode covers the SaaS rule that // an empty sanitised label becomes the literal "node". Subsequent // empty-label registrations bump as usual. diff --git a/hscontrol/state/persist_test.go b/hscontrol/state/persist_test.go index 9216b8d34..0ec6cd64e 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -99,6 +99,87 @@ func persistTestConfig(dbPath string) *types.Config { } } +func TestLoadedDuplicateNamesAreResolvedInMemory(t *testing.T) { + dbPath := t.TempDir() + "/headscale.db" + cfg := persistTestConfig(dbPath) + cfg.TailcfgDNSConfig = &tailcfg.DNSConfig{ExtraRecords: []tailcfg.DNSRecord{ + {Name: "grafana." + cfg.BaseDomain, Type: "A", Value: "100.64.0.100"}, + }} + database, err := db.NewHeadscaleDatabase(cfg) + require.NoError(t, err) + + user := database.CreateUserForTest("loaded-name-user") + long := strings.Repeat("a", 63) + stored := []string{"db", "db", "DB", "", "grafana", "db-1", "x", "x", "x-1", long, strings.ToUpper(long)} + want := []string{"db", "db-2", "DB-3", "", "grafana", "db-1", "x", "x-2", "x-1", long, strings.ToUpper(long[:61]) + "-1"} + + ids := make([]types.NodeID, 0, len(stored)) + for _, name := range stored { + node := database.CreateNodeForTest(user) + require.NoError(t, database.DB.Model(&types.Node{}).Where("id = ?", node.ID). + Update("given_name", name).Error) + ids = append(ids, node.ID) + } + + require.NoError(t, database.Close()) + + for range 2 { + s, err := NewState(cfg) + require.NoError(t, err) + + for i, id := range ids { + node, ok := s.GetNodeByID(id) + require.True(t, ok) + assert.Equal(t, want[i], node.GivenName()) + + row, err := s.db.GetNodeByID(id) + require.NoError(t, err) + assert.Equal(t, stored[i], row.GivenName, + "startup must not write the repair") + } + + require.NoError(t, s.Close()) + } +} + +func TestRegistrationPersistsResolvedGivenName(t *testing.T) { + dbPath := t.TempDir() + "/headscale.db" + s, err := NewState(persistTestConfig(dbPath)) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close() }) + + user := s.CreateUserForTest("registration-name-user") + pak, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil) + require.NoError(t, err) + + want := []string{"db", "db-1", "node"} + ids := make([]types.NodeID, 0, len(want)) + + for i, hostname := range []string{"db", "db", "!!!"} { + 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) + + ids = append(ids, node.ID()) + require.Equal(t, want[i], node.GivenName()) + row, err := s.db.GetNodeByID(node.ID()) + require.NoError(t, err) + assert.Equal(t, want[i], row.GivenName) + } + + require.NoError(t, s.Close()) + + reopened := persistTestReopen(t, dbPath) + for i, id := range ids { + node, ok := reopened.GetNodeByID(id) + require.True(t, ok) + assert.Equal(t, want[i], node.GivenName()) + } +} + // TestPersistEmptyApprovedRoutes covers the State.SetApprovedRoutes // path. The gRPC handler builds the slice via append from a nil // declaration, so when the operator passes `-r ""` the persist layer @@ -1615,58 +1696,6 @@ func TestBackfillNodeIPsSurvivesConcurrentPersist(t *testing.T) { // differs from SaaS, keyed ///. // A listed check that matches fails, so a fix must delete its entry. var nameSkipReasons = map[string]string{ - "names/N1/db-b/reopen": labelNotSaved, - "names/N5/e2/reopen": labelNotSaved, - "names/N5/e1/reopen": labelNotSaved, - "names/N6/e2/reopen": labelNotSaved, - "names/N6/e1/reopen": labelNotSaved, - "names/N7/e2/reopen": labelNotSaved, - "names/N7/e1/reopen": labelNotSaved, - "names/N8/e2/reopen": labelNotSaved, - "names/N8/e1/reopen": labelNotSaved, - "names/N9a/e2/reopen": labelNotSaved, - "names/N9a/e1/reopen": labelNotSaved, - "names/N9b/e2/reopen": labelNotSaved, - "names/N9b/e1/reopen": labelNotSaved, - "names/N10/e2/reopen": labelNotSaved, - "names/N10/e1/reopen": labelNotSaved, - "names/N11/e2/reopen": labelNotSaved, - "names/N11/e1/reopen": labelNotSaved, - "names/N11b/e2/reopen": labelNotSaved, - "names/N11b/e1/reopen": labelNotSaved, - "names/N12a/e2/reopen": labelNotSaved, - "names/N12a/e1/reopen": labelNotSaved, - "names/N12b/e2/reopen": labelNotSaved, - "names/N12b/e1/reopen": labelNotSaved, - "names/N13/e2/reopen": labelNotSaved, - "names/N13/e1/reopen": labelNotSaved, - - "names/N1/n63b/label": labelSuffixOverflow, - "names/N2/n63b/label": labelSuffixOverflow, - "names/N3/n63b/label": labelSuffixOverflow, - "names/N4/n63b/label": labelSuffixOverflow, - "names/N5/n63b/label": labelSuffixOverflow, - "names/N6/n63b/label": labelSuffixOverflow, - "names/N7/n63b/label": labelSuffixOverflow, - "names/N8/n63b/label": labelSuffixOverflow, - "names/N9a/n63b/label": labelSuffixOverflow, - "names/N9b/n63b/label": labelSuffixOverflow, - "names/N10/n63b/label": labelSuffixOverflow, - "names/N11/n63b/label": labelSuffixOverflow, - - "names/N1/n63b/reopen": labelNotSavedOverflow, - "names/N2/n63b/reopen": labelNotSavedOverflow, - "names/N3/n63b/reopen": labelNotSavedOverflow, - "names/N4/n63b/reopen": labelNotSavedOverflow, - "names/N5/n63b/reopen": labelNotSavedOverflow, - "names/N6/n63b/reopen": labelNotSavedOverflow, - "names/N7/n63b/reopen": labelNotSavedOverflow, - "names/N8/n63b/reopen": labelNotSavedOverflow, - "names/N9a/n63b/reopen": labelNotSavedOverflow, - "names/N9b/n63b/reopen": labelNotSavedOverflow, - "names/N10/n63b/reopen": labelNotSavedOverflow, - "names/N11/n63b/reopen": labelNotSavedOverflow, - "names/N11/pdb/label": adminNameFollowsHostname, "names/N11/pdb/reopen": adminNameFollowsHostname, "names/N11b/pdb/label": adminNameFollowsHostname, @@ -1677,24 +1706,10 @@ var nameSkipReasons = map[string]string{ "names/N12b/pdb/reopen": adminNameFollowsHostname, "names/N13/pdb/label": adminNameFollowsHostname, "names/N13/pdb/reopen": adminNameFollowsHostname, - - "names/N7/m1/reopen": labelNotSaved, - "names/N8/m1/reopen": labelNotSaved, - "names/N9a/m1/reopen": labelNotSaved, - "names/N9b/m1/reopen": labelNotSaved, - "names/N10/m1/reopen": labelNotSaved, - "names/N11/m1/reopen": labelNotSaved, - "names/N11b/m1/reopen": labelNotSaved, - "names/N12a/m1/reopen": labelNotSaved, - "names/N12b/m1/reopen": labelNotSaved, - "names/N13/m1/reopen": labelNotSaved, } 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" - labelNotSaved = "LABEL_NOT_SAVED: headscale writes a new node's row with its hostname's label before the NodeStore resolves a collision or an empty label, and saves the result only on a later change; SaaS keeps the label it gave" - labelNotSavedOverflow = "LABEL_NOT_SAVED+LABEL_SUFFIX_OVERFLOW: the row keeps the unsuffixed 63-byte label, and the label the NodeStore resolves is 65 bytes; SaaS cuts the label so the suffix fits" ) // TestNodeNameReplayCompat replays the names capture through State: joins by diff --git a/hscontrol/state/rename_test.go b/hscontrol/state/rename_test.go index b141147be..551a02de9 100644 --- a/hscontrol/state/rename_test.go +++ b/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 diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 77f374dbf..c0f920d9b 100644 --- a/hscontrol/state/state.go +++ b/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 @@ -2676,7 +2700,18 @@ func (s *State) createAndSaveNewNode(params newNodeParams) (types.NodeView, erro } // Add to [NodeStore] after database creates the ID - return s.nodeStore.PutNode(*savedNode), nil + node := s.nodeStore.PutNode(*savedNode) + if node.Valid() && node.GivenName() != savedNode.GivenName { + fresh, err := s.persistNode(node) + if err != nil { + // The node is already committed; callers still need to schedule cleanup. + return node, err + } + + return fresh, nil + } + + return node, nil } // validateRequestTags validates that the requested tags are permitted for the node. @@ -3375,7 +3410,12 @@ func (s *State) HandleNodeFromPreAuthKey( ExistingNodeForNetinfo: differentUserNode, }) if err != nil { - return types.NodeView{}, s.policyChangeSince(genBefore), fmt.Errorf("creating new node: %w", err) + c := s.policyChangeSince(genBefore) + if finalNode.Valid() { + c = nodeWriteFailed(finalNode.ID(), c) + } + + return finalNode, c, fmt.Errorf("creating new node: %w", err) } } @@ -3860,23 +3900,23 @@ func (s *State) PingDB(ctx context.Context) error { // isAutoDerivedGivenName reports whether given matches what // dnsname.SanitizeHostname(hostname) would produce, optionally with a -// [NodeStore] collision-bump "-N" suffix. It is used to detect whether a -// GivenName has been admin-renamed (in which case it must not be -// overwritten by client-side hostname changes). +// [NodeStore] collision-bump "-N" suffix ([bumpedLabel]). It is used to +// detect whether a GivenName has been admin-renamed (in which case it must +// not be overwritten by client-side hostname changes). func isAutoDerivedGivenName(given, hostname string) bool { base := dnsname.SanitizeHostname(hostname) if given == base { return true } - suffix, ok := strings.CutPrefix(given, base+"-") + _, suffix, ok := strings.CutLast(given, "-") if !ok { return false } - _, err := strconv.Atoi(suffix) + n, err := strconv.Atoi(suffix) - return err == nil + return err == nil && given == bumpedLabel(base, n) } // UpdateNodeFromMapRequest is the sync point where Hostinfo changes, diff --git a/hscontrol/types/config.go b/hscontrol/types/config.go index f105d2eca..63de779b2 100644 --- a/hscontrol/types/config.go +++ b/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 } } diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index 681b5d489..d3a329b15 100644 --- a/hscontrol/types/node.go +++ b/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 }