From 868998a81b7f46578803909fcb3361a1b576342d Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 31 Oct 2026 14:18:06 +0000 Subject: [PATCH] state: let a node's name follow a new hostname at login As SaaS. It stayed old, and the mismatch then read as an admin-set name, which stopped later hostname changes from renaming the node too. --- hscontrol/servertest/lifecycle_compat_test.go | 14 ++++++ hscontrol/servertest/lifecycle_test.go | 44 +++++++++++++++++++ hscontrol/state/state.go | 33 ++++++++------ 3 files changed, 78 insertions(+), 13 deletions(-) diff --git a/hscontrol/servertest/lifecycle_compat_test.go b/hscontrol/servertest/lifecycle_compat_test.go index 9fb597eba..da7934db9 100644 --- a/hscontrol/servertest/lifecycle_compat_test.go +++ b/hscontrol/servertest/lifecycle_compat_test.go @@ -187,6 +187,14 @@ func runRouteLifecycleCompat(t *testing.T, id, observer string, skips map[string {"filter-routes", filterRouteSources(step.Captures[observer].Netmap), func() string { return filterRouteSources(r.clients[observer].Netmap()) }}, + {"name", firstLabel(dev.Name), func() string { + nodes := r.machineNodes(router) + if len(nodes) != 1 { + return fmt.Sprintf("devices %d", len(nodes)) + } + + return nodes[0].GivenName() + }}, {"device", deviceState( countDevicesOnMachine(t, step, dev.MachineKey), dev.ID == join.saasID, sortedStrings(dev.Addresses) == join.saasAddrs, @@ -390,6 +398,12 @@ func filterRouteSources(nm *netmap.NetworkMap) string { return fmt.Sprint(slices.Sorted(maps.Keys(seen))) } +func firstLabel(name string) string { + label, _, _ := strings.Cut(name, ".") + + return label +} + // saasDevice is the part of a SaaS device-list entry the checks read. // Keys and IDs are the capture's labels. type saasDevice struct { diff --git a/hscontrol/servertest/lifecycle_test.go b/hscontrol/servertest/lifecycle_test.go index adcd7aab8..20b9e9186 100644 --- a/hscontrol/servertest/lifecycle_test.go +++ b/hscontrol/servertest/lifecycle_test.go @@ -679,6 +679,50 @@ func TestNodeExpiryRouteFailover(t *testing.T) { } } +// TestReloginFollowsNewHostname: a node's name follows a new hostname at a login +// as at a map request, and later hostname changes still rename it. An admin-set +// name stays. +func TestReloginFollowsNewHostname(t *testing.T) { + t.Parallel() + + hostinfo := func(name string) *tailcfg.Hostinfo { + return &tailcfg.Hostinfo{Hostname: name, BackendLogID: "servertest"} + } + + srv := servertest.NewServer(t) + owner := srv.CreateUser(t, "owner") + id := types.UserID(owner.ID) + client := servertest.NewClient(t, srv, "first", servertest.WithUser(owner)) + + client.WaitForCondition(t, "a first map", 20*time.Second, func(nm *netmap.NetworkMap) bool { + return nm != nil && nm.SelfNode.Valid() + }) + + givenName := func() string { + nodes := srv.State().ListNodes() + require.Equal(t, 1, nodes.Len()) + + return nodes.At(0).GivenName() + } + + require.NoError(t, client.LogoutAndDisconnect(t.Context())) + require.NoError(t, client.Relogin(t.Context(), srv.CreatePreAuthKey(t, id), false, hostinfo("second"))) + assert.Equal(t, "second", givenName()) + + client.Direct().SetHostinfo(hostinfo("third")) + require.NoError(t, client.Direct().SendUpdate(t.Context())) + assert.EventuallyWithT(t, func(c *assert.CollectT) { + assert.Equal(c, "third", givenName()) + }, 10*time.Second, 100*time.Millisecond, "a later hostname change renames too") + + _, _, err := srv.State().RenameNode(srv.State().ListNodes().At(0).ID(), "chosen") + require.NoError(t, err) + + require.NoError(t, client.LogoutAndDisconnect(t.Context())) + require.NoError(t, client.Relogin(t.Context(), srv.CreatePreAuthKey(t, id), false, hostinfo("fourth"))) + assert.Equal(t, "chosen", givenName(), "a login keeps an admin-set name") +} + // TestLogoutOfDeletedDeviceRegistersNothing: a client sends its auth key with a // logout, and that must not bring back the device an admin just deleted. func TestLogoutOfDeletedDeviceRegistersNothing(t *testing.T) { diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 174ba8b04..dd10878c2 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -2578,7 +2578,7 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView node.DiscoKey = regData.DiscoKey } - node.Hostname = params.Hostname + setHostname(node, params.Hostname) node.Hostinfo = hostinfo // Preserve the node's live endpoints when the register request carried @@ -3699,7 +3699,7 @@ func (s *State) reregisterNodeWithPAK( return } - node.Hostname = hostname + setHostname(node, hostname) // TODO(kradalby): We should ensure we use the same hostinfo and node merge semantics // when a node re-registers as we do when it sends a map request (UpdateNodeFromMapRequest). @@ -4114,6 +4114,23 @@ func (s *State) PingDB(ctx context.Context) error { return s.db.PingDB(ctx) } +// setHostname stores a node's hostname, and its name follows, as on SaaS. An +// admin-set name stays: the name only follows while it is still what the old +// hostname gives (possibly with a "-N" collision bump). [NodeStore.UpdateNode] +// bumps a name that collides. +func setHostname(node *types.Node, hostname string) { + if hostname == node.Hostname { + return + } + + autoDerived := isAutoDerivedGivenName(node.GivenName, node.Hostname) + + node.Hostname = hostname + if autoDerived { + node.GivenName = dnsname.SanitizeHostname(hostname) + } +} + // isAutoDerivedGivenName reports whether given matches what // dnsname.SanitizeHostname(hostname) would produce, optionally with a // [NodeStore] collision-bump "-N" suffix ([bumpedLabel]). It is used to @@ -4271,17 +4288,7 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest // Only a real hostname change may re-derive GivenName: it is peer // visible, so the whole node is resent and peers learn the name. if hasReportedHostnameChange(currentNode.Hostname, newHostinfo) { - // Preserve an admin-renamed GivenName: only auto-derive - // when the current GivenName is still what - // SanitizeHostname of the old Hostname would produce - // (possibly with a "-N" collision bump). - autoDerived := isAutoDerivedGivenName(currentNode.GivenName, currentNode.Hostname) - - currentNode.Hostname = newHostinfo.Hostname - if autoDerived { - currentNode.GivenName = dnsname.SanitizeHostname(newHostinfo.Hostname) - // [NodeStore.UpdateNode] auto-bumps GivenName on collision. - } + setHostname(currentNode, newHostinfo.Hostname) } // Approval follows the announced set, only when it changes: any