state: log nodes with map-breaking data at startup

Scan a node-health check registry at boot and log each node whose name
can't form a valid FQDN, with the rename fix. Log-only, no mutation.

Updates #3346

Author: kradalby

hscontrol/state/node_health.go

@@ -0,0 +1,92 @@
+package state
+
+import (
+	"fmt"
+
+	"github.com/juanfont/headscale/hscontrol/types"
+	"github.com/juanfont/headscale/hscontrol/util/zlog/zf"
+	"github.com/rs/zerolog/log"
+)
+
+// nodeHealthCheck names a class of stored-node-data defect that breaks normal
+// operation and explains how to fix it. ok == true means the node passes the
+// check. This is the extension point for node-data validation: add a check
+// here as new corrupt-data classes surface (nil hostinfo, invalid IPs,
+// tags-XOR-user violations, ...) and both the boot scan and any future caller
+// run the whole set.
+type nodeHealthCheck struct {
+	name  string
+	check func(nv types.NodeView, cfg *types.Config) (problem, fixHint string, ok bool)
+}
+
+// nodeHealthChecks is the registry of node-data health checks. Today it carries
+// the one issue #3346 needs; append to it rather than reshaping callers.
+var nodeHealthChecks = []nodeHealthCheck{givenNameMapsToValidFQDN}
+
+// givenNameMapsToValidFQDN flags a node whose stored GivenName cannot produce a
+// valid FQDN (empty, or longer than MaxHostnameLength once base_domain is
+// applied). Such a node cannot be rendered into a netmap — neither its own nor
+// any peer's — so it must be renamed to recover.
+var givenNameMapsToValidFQDN = nodeHealthCheck{
+	name: "given-name-maps-to-valid-fqdn",
+	check: func(nv types.NodeView, cfg *types.Config) (string, string, bool) {
+		err := types.ValidateGivenName(nv.GivenName(), cfg.BaseDomain)
+		if err != nil {
+			return err.Error(), fmt.Sprintf("headscale nodes rename %d <name>", nv.ID()), false
+		}
+
+		return "", "", true
+	},
+}
+
+// nodeHealthFinding is a single failed check for a single node.
+type nodeHealthFinding struct {
+	nodeID   types.NodeID
+	hostname string
+	check    string
+	problem  string
+	fixHint  string
+}
+
+// scanNodeHealth runs every registered check against every node in the store
+// and returns one finding per failure. It only reports — it never mutates a
+// node — so an operator can repair the underlying data without the server
+// silently rewriting a user-visible name.
+func (s *State) scanNodeHealth() []nodeHealthFinding {
+	var findings []nodeHealthFinding
+
+	for _, nv := range s.nodeStore.ListNodes().All() {
+		for _, c := range nodeHealthChecks {
+			problem, fixHint, ok := c.check(nv, s.cfg)
+			if ok {
+				continue
+			}
+
+			findings = append(findings, nodeHealthFinding{
+				nodeID:   nv.ID(),
+				hostname: nv.Hostname(),
+				check:    c.name,
+				problem:  problem,
+				fixHint:  fixHint,
+			})
+		}
+	}
+
+	return findings
+}
+
+// logNodeHealth scans the store once and logs an actionable warning per
+// finding. Called at startup so an operator learns — by node id and fix
+// command — about stored data that will break map generation, without the
+// server changing anything itself.
+func (s *State) logNodeHealth() {
+	for _, f := range s.scanNodeHealth() {
+		log.Warn().
+			Uint64(zf.NodeID, f.nodeID.Uint64()).
+			Str(zf.NodeHostname, f.hostname).
+			Str("check", f.check).
+			Str("problem", f.problem).
+			Str("fix", f.fixHint).
+			Msg("node has invalid data that breaks map generation; rename it to restore connectivity")
+	}
+}

hscontrol/state/node_health_test.go

@@ -0,0 +1,67 @@
+package state
+
+import (
+	"testing"
+
+	"github.com/juanfont/headscale/hscontrol/db"
+	"github.com/juanfont/headscale/hscontrol/types"
+	"github.com/stretchr/testify/require"
+)
+
+func TestGivenNameMapsToValidFQDNCheck(t *testing.T) {
+	cfg := &types.Config{BaseDomain: "example.com"}
+
+	_, _, ok := givenNameMapsToValidFQDN.check((&types.Node{ID: 1, GivenName: "valid"}).View(), cfg)
+	require.True(t, ok, "a valid given name must pass the check")
+
+	problem, fixHint, ok := givenNameMapsToValidFQDN.check((&types.Node{ID: 7, GivenName: ""}).View(), cfg)
+	require.False(t, ok, "an empty given name must fail the check")
+	require.NotEmpty(t, problem)
+	require.Contains(t, fixHint, "rename 7", "fix hint must name the offending node")
+}
+
+// TestScanNodeHealthReportsInvalidNameWithoutMutating proves the boot scan
+// reports a node whose stored name would break map generation (issue #3346)
+// with an actionable fix, and that it never rewrites the stored name — the
+// maintainer's decision is log-only, no silent mutation.
+func TestScanNodeHealthReportsInvalidNameWithoutMutating(t *testing.T) {
+	dbPath := t.TempDir() + "/headscale.db"
+	cfg := persistTestConfig(dbPath)
+
+	database, err := db.NewHeadscaleDatabase(cfg)
+	require.NoError(t, err)
+
+	user := database.CreateUserForTest("scan-user")
+	bad := database.CreateRegisteredNodeForTest(user, "scan-bad")
+	good := database.CreateRegisteredNodeForTest(user, "scan-good")
+
+	require.NoError(t, database.DB.
+		Model(&types.Node{}).
+		Where("id = ?", bad.ID).
+		Update("given_name", "").Error)
+	require.NoError(t, database.Close())
+
+	s, err := NewState(cfg)
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = s.Close() })
+
+	findings := s.scanNodeHealth()
+
+	var badFinding *nodeHealthFinding
+
+	for i := range findings {
+		require.NotEqual(t, good.ID, findings[i].nodeID, "a valid node must not be reported")
+
+		if findings[i].nodeID == bad.ID {
+			badFinding = &findings[i]
+		}
+	}
+
+	require.NotNil(t, badFinding, "a node with an invalid name must be reported")
+	require.Contains(t, badFinding.fixHint, "rename", "finding must carry an actionable fix")
+
+	// Log-only: neither the scan nor boot may rewrite the stored name.
+	nv, ok := s.GetNodeByID(bad.ID)
+	require.True(t, ok)
+	require.Empty(t, nv.GivenName(), "boot scan must not mutate the stored name")
+}

hscontrol/state/state.go

@@ -277,7 +277,7 @@ func NewState(cfg *types.Config) (*State, error) {
 	)
 	nodeStore.Start()
 
-	return &State{
+	s := &State{
 		cfg: cfg,
 
 		db:        db,
@@ -289,7 +289,14 @@ func NewState(cfg *types.Config) (*State, error) {
 
 		sshCheckAuth:  make(map[sshCheckPair]time.Time),
 		registerLocks: xsync.NewMap[key.MachinePublic, *sync.Mutex](),
-	}, nil
+	}
+
+	// Surface nodes whose stored data would break map generation (e.g. an
+	// invalid given name from a legacy row) so an operator can fix them. This
+	// only logs; it never mutates a node's stored name at boot.
+	s.logNodeHealth()
+
+	return s, nil
 }
 
 // Close gracefully shuts down the [State] instance and releases all resources.