state: bump duplicate node names loaded from the database

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,

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{

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