From 4946d1c88d25aa9c3d426437aca4520b5376a86c Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 26 Jun 2026 14:19:55 +0000 Subject: [PATCH] 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 --- hscontrol/state/node_health.go | 92 +++++++++++++++++++++++++++++ hscontrol/state/node_health_test.go | 67 +++++++++++++++++++++ hscontrol/state/state.go | 11 +++- 3 files changed, 168 insertions(+), 2 deletions(-) create mode 100644 hscontrol/state/node_health.go create mode 100644 hscontrol/state/node_health_test.go diff --git a/hscontrol/state/node_health.go b/hscontrol/state/node_health.go new file mode 100644 index 00000000..4edaf398 --- /dev/null +++ b/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 ", 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") + } +} diff --git a/hscontrol/state/node_health_test.go b/hscontrol/state/node_health_test.go new file mode 100644 index 00000000..cd61a25b --- /dev/null +++ b/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") +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 18cc00e8..ddb410eb 100644 --- a/hscontrol/state/state.go +++ b/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.