{state,db}: preserve node expiry on MapRequest updates #2875
Conversation
2f2adb6 to
404e762
Compare
404e762 to
d88c39c
Compare
nblock
left a comment
There was a problem hiding this comment.
I built d88c39c and noticed that Expiration is purged when tailscaled is restarted.
After auth:
$ headscale nodes list
ID | Hostname | Name | MachineKey | NodeKey | User | IP addresses | Ephemeral | Last seen | Expiration | Connected | Expired
1 | n1 | n1 | [hG2IT] | [Ydo2r] | user1 | 100.64.0.1, fd7a:115c:a1e0::1 | false | 2025-11-11 05:47:13 | 2025-11-14 05:47:11 | online | no $ headscale nodes list
ID | Hostname | Name | MachineKey | NodeKey | User | IP addresses | Ephemeral | Last seen | Expiration | Connected | Expired
1 | n1 | n1 | [hG2IT] | [Ydo2r] | user1 | 100.64.0.1, fd7a:115c:a1e0::1 | false | 2025-11-11 05:49:44 | 0001-01-01 00:00:00 | online | no There was a problem hiding this comment.
Pull Request Overview
This PR fixes a regression introduced in v0.27.0 where node expiry times were being reset to zero when tailscaled restarts and sends a MapRequest. The fix changes GORM database operations from Save() to Updates() to preserve database fields that are not explicitly set in the struct being saved.
- Replaced
Save()withUpdates()in multiple database persistence operations to prevent overwriting NULL database values with zero values from nil pointer fields - Added a specialized update pattern in
BackfillNodeIPsusingUpdates()withSelect()to explicitly control which fields are updated - Added comprehensive regression tests to validate expiry preservation and user field preservation
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| hscontrol/state/state.go | Changed Save() to Updates() in UpdateUser, persistNodeToDB, HandleNodeFromAuthPath, and HandleNodeFromPreAuthKey to preserve unchanged database fields |
| hscontrol/state/expiry_test.go | Added regression test reproducing the expiry reset bug when MapRequest updates a node |
| hscontrol/db/users.go | Changed RenameUser to use Updates() instead of Save() |
| hscontrol/db/user_update_test.go | Added tests validating that Updates() preserves unchanged user fields |
| hscontrol/db/preauth_keys.go | Changed UsePreAuthKey to use Model().Update() for single field updates |
| hscontrol/db/node.go | Removed deprecated NodeSave() function |
| hscontrol/db/ip.go | Changed BackfillNodeIPs to use Updates() with Select() for targeted IP field updates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d88c39c to
b652f93
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Use Omit("expiry") to prevent overwriting expiry during MapRequest updates. | ||
| // Expiry should only be updated through explicit SetNodeExpiry calls or re-registration. | ||
| // See: https://github.com/juanfont/headscale/issues/2862 | ||
| err := s.db.DB.Omit("expiry").Updates(nodePtr).Error |
There was a problem hiding this comment.
Using Omit(\"expiry\") in persistNodeToDB will break SetNodeExpiry functionality. When SetNodeExpiry updates a node's expiry and calls persistNodeToDB, the expiry field will be omitted from the database update, making it impossible to update expiry times. Consider using a parameter or context to distinguish between MapRequest updates (where expiry should be omitted) and explicit expiry updates (where expiry must be included).
| _, err = hsdb.Write(s.db.DB, func(tx *gorm.DB) (*types.Node, error) { | ||
| if err := tx.Save(updatedNodeView.AsStruct()).Error; err != nil { | ||
| // Use Updates() to preserve fields not modified by UpdateNode. | ||
| err := tx.Updates(updatedNodeView.AsStruct()).Error | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to save node: %w", err) | ||
| } | ||
| return nil, nil |
There was a problem hiding this comment.
Using Updates() without Select() may not properly update the expiry field when it's set in lines 1185-1189. GORM's Updates() skips zero values, and since Expiry is a pointer, setting it to a non-nil pointer should work, but this is inconsistent with the Omit(\"expiry\") approach used in persistNodeToDB. Consider using Select() to explicitly include the expiry field for this re-registration path, or using a direct update of the expiry field separately.
| // Use Updates() to preserve fields not modified by UpdateNode. | ||
| err := tx.Updates(updatedNodeView.AsStruct()).Error | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to save node: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
Similar to HandleNodeFromAuthPath, this code sets node.Expiry at line 1409 but then uses Updates() which may not properly update pointer fields depending on their values. This is inconsistent with the Omit(\"expiry\") approach used in persistNodeToDB and could lead to expiry not being saved correctly during preauth key registration.
| // Use Updates() to preserve fields not modified by UpdateNode. | |
| err := tx.Updates(updatedNodeView.AsStruct()).Error | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to save node: %w", err) | |
| } | |
| // Use Updates() to preserve fields not modified by UpdateNode, but omit expiry. | |
| err := tx.Omit("expiry").Updates(updatedNodeView.AsStruct()).Error | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to save node: %w", err) | |
| } | |
| // Explicitly update expiry, even if it is nil. | |
| err = tx.Model(updatedNodeView.AsStruct()).Update("expiry", updatedNodeView.Expiry).Error | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to update expiry: %w", err) | |
| } |
Signed-off-by: Kristoffer Dalby <[email protected]>
Fixes a regression introduced in v0.27.0 where node expiry times were being reset to zero when tailscaled restarts and sends a MapRequest. The issue was caused by using GORM's Save() method in persistNodeToDB(), which overwrites ALL fields including zero values. When a MapRequest updates a node (without including expiry information), Save() would overwrite the database expiry field with a zero value. Changed to use Updates() which only updates non-zero values, preserving existing database values when struct pointer fields are nil. In BackfillNodeIPs, we need to explicitly update IPv4/IPv6 fields even when nil (to remove IPs), so we use Select() to specify those fields. Added regression test that validates expiry is preserved after MapRequest. Fixes juanfont#2862
Changed UpdateUser and re-registration flows to use Updates() which only writes modified fields, preventing unintended overwrites of unchanged fields. Also updated UsePreAuthKey to use Model().Update() for single field updates and removed unused NodeSave wrapper.
RenameUser only modifies Name field, should use Updates() not Save().
When tailscaled restarts, it sends RegisterRequest with Auth=nil and
Expiry=zero. Previously this was treated as a logout because
time.Time{}.Before(time.Now()) returns true.
Add early return in handleRegister() to detect this case and preserve
the existing node state without modification.
Fixes juanfont#2862
b652f93 to
997accf
Compare
Fixes a regression introduced in v0.27.0 where node expiry times were being reset to zero when tailscaled restarts and sends a MapRequest.
The issue was caused by using GORM's Save() method in persistNodeToDB(), which overwrites ALL fields including zero values. When a MapRequest updates a node (without including expiry information), Save() would overwrite the database expiry field with a zero value.
Changed to use Updates() which only updates non-zero values, preserving existing database values when struct pointer fields are nil.
In BackfillNodeIPs, we need to explicitly update IPv4/IPv6 fields even when nil (to remove IPs), so we use Select() to specify those fields.
Added regression test that validates expiry is preserved after MapRequest.
Fixes #2862
claude was used in this PR.