From 41c388f45a1a760514d2bc51e59f5764ed9c0662 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 17 Oct 2026 08:58:37 +0000 Subject: [PATCH] state: save the name NodeStore picks for a new node The row is committed before the name UPDATE, so a failed UPDATE still returns the node and registration arms ephemeral cleanup. --- hscontrol/poll_test.go | 62 +++++++++++++++++++- hscontrol/state/persist_test.go | 101 ++++++++++++++++---------------- hscontrol/state/state.go | 20 ++++++- 3 files changed, 129 insertions(+), 54 deletions(-) diff --git a/hscontrol/poll_test.go b/hscontrol/poll_test.go index d1e9dc643..c541f2cd9 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/state/persist_test.go b/hscontrol/state/persist_test.go index 9216b8d34..b13581aa5 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -99,6 +99,44 @@ func persistTestConfig(dbPath string) *types.Config { } } +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,32 +1653,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, @@ -1654,18 +1666,18 @@ var nameSkipReasons = map[string]string{ "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/N1/n63b/reopen": labelSuffixOverflow, + "names/N2/n63b/reopen": labelSuffixOverflow, + "names/N3/n63b/reopen": labelSuffixOverflow, + "names/N4/n63b/reopen": labelSuffixOverflow, + "names/N5/n63b/reopen": labelSuffixOverflow, + "names/N6/n63b/reopen": labelSuffixOverflow, + "names/N7/n63b/reopen": labelSuffixOverflow, + "names/N8/n63b/reopen": labelSuffixOverflow, + "names/N9a/n63b/reopen": labelSuffixOverflow, + "names/N9b/n63b/reopen": labelSuffixOverflow, + "names/N10/n63b/reopen": labelSuffixOverflow, + "names/N11/n63b/reopen": labelSuffixOverflow, "names/N11/pdb/label": adminNameFollowsHostname, "names/N11/pdb/reopen": adminNameFollowsHostname, @@ -1677,24 +1689,11 @@ 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/state.go b/hscontrol/state/state.go index 77f374dbf..1c342c52d 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -2676,7 +2676,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 +3386,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) } }