From 47507b2c9add047a7814f024267c9b302f0b47c5 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 17 Oct 2026 09:04:55 +0000 Subject: [PATCH] state: bump duplicate node names loaded from the database --- hscontrol/state/node_store.go | 55 ++++++++++++++++++--- hscontrol/state/node_store_hostname_test.go | 26 ++++++++++ hscontrol/state/persist_test.go | 43 ++++++++++++++++ 3 files changed, 117 insertions(+), 7 deletions(-) diff --git a/hscontrol/state/node_store.go b/hscontrol/state/node_store.go index 3549c8056..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" @@ -14,8 +15,10 @@ import ( "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" @@ -130,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) @@ -151,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, @@ -667,9 +708,9 @@ func (s *NodeStore) applyBatch(batch []work) { // // On collision the label is bumped as base, base-1, base-2, …, first // unused wins; the base is cut so base-N stays a valid label, as Tailscale -// does. 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. +// 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, diff --git a/hscontrol/state/node_store_hostname_test.go b/hscontrol/state/node_store_hostname_test.go index 827bf56a1..f9a01d88d 100644 --- a/hscontrol/state/node_store_hostname_test.go +++ b/hscontrol/state/node_store_hostname_test.go @@ -7,10 +7,36 @@ import ( "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{ diff --git a/hscontrol/state/persist_test.go b/hscontrol/state/persist_test.go index 8c8a6b4c1..dde52f770 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -99,6 +99,49 @@ 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))