Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cmd/headscale/cli/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,14 @@ func newHeadscaleServerWithConfig() (*hscontrol.Headscale, error) {
cfg, err := types.LoadServerConfig()
if err != nil {
return nil, fmt.Errorf(
"failed to load configuration while creating headscale instance: %w",
"loading configuration: %w",
err,
)
}

app, err := hscontrol.NewHeadscale(cfg)
if err != nil {
return nil, err
return nil, fmt.Errorf("creating new headscale: %w", err)
}

return app, nil
Expand Down
4 changes: 2 additions & 2 deletions hscontrol/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ func NewHeadscale(cfg *types.Config) (*Headscale, error) {
registrationCache,
)
if err != nil {
return nil, err
return nil, fmt.Errorf("new database: %w", err)
}

app.ipAlloc, err = db.NewIPAllocator(app.db, cfg.PrefixV4, cfg.PrefixV6, cfg.IPAllocation)
Expand All @@ -160,7 +160,7 @@ func NewHeadscale(cfg *types.Config) (*Headscale, error) {
})

if err = app.loadPolicyManager(); err != nil {
return nil, fmt.Errorf("failed to load ACL policy: %w", err)
return nil, fmt.Errorf("loading ACL policy: %w", err)
}

var authProvider AuthProvider
Expand Down
19 changes: 18 additions & 1 deletion hscontrol/db/db.go
Original file line number Diff line number Diff line change
Expand Up @@ -672,7 +672,24 @@ AND auth_key_id NOT IN (
{
ID: "202502171819",
Migrate: func(tx *gorm.DB) error {
_ = tx.Migrator().DropColumn(&types.Node{}, "last_seen")
// This migration originally removed the last_seen column
// from the node table, but it was added back in
// 202505091439.
return nil
},
Rollback: func(db *gorm.DB) error { return nil },
},
// Add back last_seen column to node table.
{
ID: "202505091439",
Migrate: func(tx *gorm.DB) error {
// Add back last_seen column to node table if it does not exist.
// This is a workaround for the fact that the last_seen column
// was removed in the 202502171819 migration, but only for some
// beta testers.
if !tx.Migrator().HasColumn(&types.Node{}, "last_seen") {
_ = tx.Migrator().AddColumn(&types.Node{}, "last_seen")
}

return nil
},
Expand Down
14 changes: 14 additions & 0 deletions hscontrol/db/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,20 @@ func SetApprovedRoutes(
return nil
}

// SetLastSeen sets a node's last seen field indicating that we
// have recently communicating with this node.
func (hsdb *HSDatabase) SetLastSeen(nodeID types.NodeID, lastSeen time.Time) error {
return hsdb.Write(func(tx *gorm.DB) error {
return SetLastSeen(tx, nodeID, lastSeen)
})
}

// SetLastSeen sets a node's last seen field indicating that we
// have recently communicating with this node.
func SetLastSeen(tx *gorm.DB, nodeID types.NodeID, lastSeen time.Time) error {
return tx.Model(&types.Node{}).Where("id = ?", nodeID).Update("last_seen", lastSeen).Error
}
Comment on lines +254 to +266
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In hscontrol/db/node.go, you have duplicated documentation comments for the SetLastSeen function. Consider removing the redundancy to maintain cleaner code.


// RenameNode takes a Node struct and a new GivenName for the nodes
// and renames it. If the name is not unique, it will return an error.
func RenameNode(tx *gorm.DB,
Expand Down
4 changes: 4 additions & 0 deletions hscontrol/poll.go
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,10 @@ func (h *Headscale) updateNodeOnlineStatus(online bool, node *types.Node) {
change.LastSeen = &now
}

if node.LastSeen != nil {
h.db.SetLastSeen(node.ID, *node.LastSeen)
}
Comment on lines +412 to +414
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In hscontrol/poll.go, you're updating the LastSeen field in the database only if it's not nil. However, there's no clear validation ensuring that LastSeen gets initialized properly. Consider adding more robust handling for the case when LastSeen is unexpectedly nil.


ctx := types.NotifyCtx(context.Background(), "poll-nodeupdate-onlinestatus", node.Hostname)
h.nodeNotifier.NotifyWithIgnore(ctx, types.UpdatePeerPatch(change), node.ID)
}
Expand Down
6 changes: 1 addition & 5 deletions hscontrol/types/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,11 +98,7 @@ type Node struct {

// LastSeen is when the node was last in contact with
// headscale. It is best effort and not persisted.
LastSeen *time.Time `gorm:"-"`

// DEPRECATED: Use the ApprovedRoutes field instead.
// TODO(kradalby): remove when ApprovedRoutes is used all over the code.
// Routes []Route `gorm:"constraint:OnDelete:CASCADE;"`
LastSeen *time.Time `gorm:"column:last_seen"`
Comment on lines 98 to +101
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In hscontrol/types/node.go, the LastSeen field is now persisted to the database which is good, but consider adding validation in relevant functions to handle potential database errors when LastSeen is NULL in the database (for newly created nodes or after migrations).


// ApprovedRoutes is a list of routes that the node is allowed to announce
// as a subnet router. They are not necessarily the routes that the node
Expand Down
32 changes: 30 additions & 2 deletions integration/auth_key_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (

"slices"

v1 "github.com/juanfont/headscale/gen/go/headscale/v1"
"github.com/juanfont/headscale/integration/hsic"
"github.com/juanfont/headscale/integration/tsic"
"github.com/samber/lo"
Expand Down Expand Up @@ -44,6 +45,9 @@ func TestAuthKeyLogoutAndReloginSameUser(t *testing.T) {
allClients, err := scenario.ListTailscaleClients()
assertNoErrListClients(t, err)

allIps, err := scenario.ListTailscaleClientsIPs()
assertNoErrListClientIPs(t, err)

err = scenario.WaitForTailscaleSync()
assertNoErrSync(t, err)

Expand All @@ -66,6 +70,10 @@ func TestAuthKeyLogoutAndReloginSameUser(t *testing.T) {
nodeCountBeforeLogout := len(listNodes)
t.Logf("node count before logout: %d", nodeCountBeforeLogout)

for _, node := range listNodes {
assertLastSeenSet(t, node)
}

for _, client := range allClients {
err := client.Logout()
if err != nil {
Expand All @@ -78,6 +86,13 @@ func TestAuthKeyLogoutAndReloginSameUser(t *testing.T) {

t.Logf("all clients logged out")

listNodes, err = headscale.ListNodes()
require.Equal(t, nodeCountBeforeLogout, len(listNodes))

for _, node := range listNodes {
assertLastSeenSet(t, node)
}

// if the server is not running with HTTPS, we have to wait a bit before
// reconnection as the newest Tailscale client has a measure that will only
// reconnect over HTTPS if they saw a noise connection previously.
Comment on lines 96 to 98
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In integration/auth_key_test.go, removing the time.Sleep(time.Minute) might introduce flakiness in tests if the test relies on certain timing behaviors. Have you verified that the tests remain stable without this delay?

Expand Down Expand Up @@ -105,8 +120,9 @@ func TestAuthKeyLogoutAndReloginSameUser(t *testing.T) {
listNodes, err = headscale.ListNodes()
require.Equal(t, nodeCountBeforeLogout, len(listNodes))

allIps, err := scenario.ListTailscaleClientsIPs()
assertNoErrListClientIPs(t, err)
for _, node := range listNodes {
assertLastSeenSet(t, node)
}

allAddrs := lo.Map(allIps, func(x netip.Addr, index int) string {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In integration/auth_key_test.go, allIps is first fetched on line 48-49 but then used much later in the test on line 127. Between these points, clients log out and log back in, potentially changing the IP addresses. Consider either refreshing the IP list before use or using a more reliable identifier for nodes.

return x.String()
Expand Down Expand Up @@ -137,8 +153,20 @@ func TestAuthKeyLogoutAndReloginSameUser(t *testing.T) {
}
}
}

listNodes, err = headscale.ListNodes()
require.Equal(t, nodeCountBeforeLogout, len(listNodes))
for _, node := range listNodes {
assertLastSeenSet(t, node)
}
})
}

}

func assertLastSeenSet(t *testing.T, node *v1.Node) {
assert.NotNil(t, node)
assert.NotNil(t, node.LastSeen)
}

// This test will first log in two sets of nodes to two sets of users, then
Expand Down
Loading