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 }