From 7a20db9f4963cf366d0dce1872be3d181fb4d651 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 13 May 2026 09:53:01 +0000 Subject: [PATCH] types: persist Node JSON slices via named IsZero types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Endpoints, Tags and ApprovedRoutes serialize as JSON on Node. GORM's struct Updates path skips fields it considers zero, and reflect treats a nil slice as zero — clearing any of these columns via the State persist path would leave the previous value in the database. Introduce Strings, Prefixes and AddrPorts as named slice types whose IsZero() always reports false, so GORM keeps the column in the UPDATE regardless of the slice being nil or empty. JSON marshalling is unchanged: nil serializes to null, empty to []. List() returns the underlying unnamed slice for callers (mainly testify assertions over reflect.DeepEqual) that distinguish the named type from its base. Regenerated types_clone.go and types_view.go follow the field-type swap. Test assertions across hscontrol/{db,state,servertest} updated to call .List() where reflect.DeepEqual previously matched the raw slice type. Fixes #3110 --- hscontrol/db/db_test.go | 2 +- hscontrol/db/users_test.go | 2 +- hscontrol/servertest/issues_test.go | 2 +- hscontrol/state/node_store_test.go | 12 ++++---- hscontrol/types/node.go | 6 ++-- hscontrol/types/slices.go | 46 +++++++++++++++++++++++++++++ hscontrol/types/types_clone.go | 6 ++-- hscontrol/types/types_view.go | 6 ++-- 8 files changed, 64 insertions(+), 18 deletions(-) create mode 100644 hscontrol/types/slices.go diff --git a/hscontrol/db/db_test.go b/hscontrol/db/db_test.go index ee88d4ae..20e95e7b 100644 --- a/hscontrol/db/db_test.go +++ b/hscontrol/db/db_test.go @@ -122,7 +122,7 @@ func TestSQLiteMigrationAndDataValidation(t *testing.T) { // Expected: tags = ["tag:server"] (no duplicates) node4 := findNode("node4") require.NotNil(t, node4, "node4 should exist") - assert.Equal(t, []string{"tag:server"}, node4.Tags, "node4 should have tag:server without duplicates") + assert.Equal(t, []string{"tag:server"}, node4.Tags.List(), "node4 should have tag:server without duplicates") //nolint:goconst // descriptive test assertions read better with the literal inline // Node 5: user2 has no RequestTags // Expected: tags = [] (unchanged) diff --git a/hscontrol/db/users_test.go b/hscontrol/db/users_test.go index cbb1c3ce..36d836f9 100644 --- a/hscontrol/db/users_test.go +++ b/hscontrol/db/users_test.go @@ -123,7 +123,7 @@ func TestDestroyUserErrors(t *testing.T) { result := db.DB.First(&survivingNode, "id = ?", node.ID) require.NoError(t, result.Error) assert.Nil(t, survivingNode.UserID) - assert.Equal(t, []string{"tag:server"}, survivingNode.Tags) + assert.Equal(t, []string{"tag:server"}, survivingNode.Tags.List()) }, }, { diff --git a/hscontrol/servertest/issues_test.go b/hscontrol/servertest/issues_test.go index 4859f794..b687d512 100644 --- a/hscontrol/servertest/issues_test.go +++ b/hscontrol/servertest/issues_test.go @@ -563,7 +563,7 @@ func TestIssuesNodeStoreConsistency(t *testing.T) { require.NoError(t, err, "node should be in database") nsRoutes := nsView.ApprovedRoutes().AsSlice() - dbRoutes := dbNode.ApprovedRoutes + dbRoutes := dbNode.ApprovedRoutes.List() assert.Equal(t, nsRoutes, dbRoutes, "NodeStore and DB should agree on approved routes") diff --git a/hscontrol/state/node_store_test.go b/hscontrol/state/node_store_test.go index 6b421754..745a8714 100644 --- a/hscontrol/state/node_store_test.go +++ b/hscontrol/state/node_store_test.go @@ -684,7 +684,7 @@ func TestNodeStoreOperations(t *testing.T) { finalNode := snapshot.nodesByID[1] assert.Equal(t, "multi-update-hostname", finalNode.Hostname) assert.Equal(t, "multi-update-givenname", finalNode.GivenName) - assert.Equal(t, []string{"tag1", "tag2"}, finalNode.Tags) + assert.Equal(t, []string{"tag1", "tag2"}, finalNode.Tags.List()) }, }, }, @@ -722,14 +722,14 @@ func TestNodeStoreOperations(t *testing.T) { assert.NotNil(t, nodePtr) assert.Equal(t, "db-save-hostname", nodePtr.Hostname) assert.Equal(t, "db-save-given", nodePtr.GivenName) - assert.Equal(t, []string{"db-tag1", "db-tag2"}, nodePtr.Tags) + assert.Equal(t, []string{"db-tag1", "db-tag2"}, nodePtr.Tags.List()) // Verify the snapshot also reflects the same state snapshot := store.data.Load() storedNode := snapshot.nodesByID[1] assert.Equal(t, "db-save-hostname", storedNode.Hostname) assert.Equal(t, "db-save-given", storedNode.GivenName) - assert.Equal(t, []string{"db-tag1", "db-tag2"}, storedNode.Tags) + assert.Equal(t, []string{"db-tag1", "db-tag2"}, storedNode.Tags.List()) }, }, { @@ -792,15 +792,15 @@ func TestNodeStoreOperations(t *testing.T) { // All should have the complete final state assert.Equal(t, "concurrent-db-hostname", nodePtr1.Hostname) assert.Equal(t, "concurrent-db-given", nodePtr1.GivenName) - assert.Equal(t, []string{"concurrent-tag"}, nodePtr1.Tags) + assert.Equal(t, []string{"concurrent-tag"}, nodePtr1.Tags.List()) assert.Equal(t, "concurrent-db-hostname", nodePtr2.Hostname) assert.Equal(t, "concurrent-db-given", nodePtr2.GivenName) - assert.Equal(t, []string{"concurrent-tag"}, nodePtr2.Tags) + assert.Equal(t, []string{"concurrent-tag"}, nodePtr2.Tags.List()) assert.Equal(t, "concurrent-db-hostname", nodePtr3.Hostname) assert.Equal(t, "concurrent-db-given", nodePtr3.GivenName) - assert.Equal(t, []string{"concurrent-tag"}, nodePtr3.Tags) + assert.Equal(t, []string{"concurrent-tag"}, nodePtr3.Tags.List()) // Verify consistency with stored state snapshot := store.data.Load() diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index 60512ebe..63cdffbb 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -120,7 +120,7 @@ type Node struct { NodeKey key.NodePublic `gorm:"serializer:text"` DiscoKey key.DiscoPublic `gorm:"serializer:text"` - Endpoints []netip.AddrPort `gorm:"serializer:json"` + Endpoints AddrPorts `gorm:"serializer:json"` Hostinfo *tailcfg.Hostinfo `gorm:"column:host_info;serializer:json"` @@ -150,7 +150,7 @@ type Node struct { // When non-empty, the node is "tagged" and tags define its identity. // Empty for user-owned nodes. // Tags cannot be removed once set (one-way transition). - Tags []string `gorm:"column:tags;serializer:json"` + Tags Strings `gorm:"column:tags;serializer:json"` // When a node has been created with a PreAuthKey, we need to // prevent the preauthkey from being deleted before the node. @@ -169,7 +169,7 @@ type Node struct { // as a subnet router. They are not necessarily the routes that the node // announces at the moment. // See [Node.Hostinfo] - ApprovedRoutes []netip.Prefix `gorm:"column:approved_routes;serializer:json"` + ApprovedRoutes Prefixes `gorm:"column:approved_routes;serializer:json"` CreatedAt time.Time UpdatedAt time.Time diff --git a/hscontrol/types/slices.go b/hscontrol/types/slices.go new file mode 100644 index 00000000..faf29d1b --- /dev/null +++ b/hscontrol/types/slices.go @@ -0,0 +1,46 @@ +package types + +import "net/netip" + +// The named slice types below are used for GORM-persisted Node columns +// that serialise as JSON. GORM v2's struct-based Updates skips fields +// it considers zero — for unnamed slice types that is nil — and the +// default reflect.Value.IsZero treats a nil slice as zero. By giving +// each slice an IsZero() that always returns false, the column is +// always included in UPDATE statements regardless of whether the +// caller is clearing the field. JSON marshalling is unchanged: a nil +// value serialises to null and an empty value serialises to []. +// +// The .List() helpers return the underlying unnamed slice for the +// places (mainly testify assertions over reflect.DeepEqual) where the +// distinction between the named and unnamed type matters. + +// Strings is a []string with a GORM-friendly IsZero. +type Strings []string + +// IsZero implements GORM's zeroer interface to keep the column in the +// UPDATE set even when the slice is nil or empty. +func (Strings) IsZero() bool { return false } + +// List returns the underlying []string. +func (s Strings) List() []string { return []string(s) } + +// Prefixes is a []netip.Prefix with a GORM-friendly IsZero. +type Prefixes []netip.Prefix + +// IsZero implements GORM's zeroer interface to keep the column in the +// UPDATE set even when the slice is nil or empty. +func (Prefixes) IsZero() bool { return false } + +// List returns the underlying []netip.Prefix. +func (s Prefixes) List() []netip.Prefix { return []netip.Prefix(s) } + +// AddrPorts is a []netip.AddrPort with a GORM-friendly IsZero. +type AddrPorts []netip.AddrPort + +// IsZero implements GORM's zeroer interface to keep the column in the +// UPDATE set even when the slice is nil or empty. +func (AddrPorts) IsZero() bool { return false } + +// List returns the underlying []netip.AddrPort. +func (s AddrPorts) List() []netip.AddrPort { return []netip.AddrPort(s) } diff --git a/hscontrol/types/types_clone.go b/hscontrol/types/types_clone.go index 6739beed..a14dc6fd 100644 --- a/hscontrol/types/types_clone.go +++ b/hscontrol/types/types_clone.go @@ -86,7 +86,7 @@ var _NodeCloneNeedsRegeneration = Node(struct { MachineKey key.MachinePublic NodeKey key.NodePublic DiscoKey key.DiscoPublic - Endpoints []netip.AddrPort + Endpoints AddrPorts Hostinfo *tailcfg.Hostinfo IPv4 *netip.Addr IPv6 *netip.Addr @@ -95,12 +95,12 @@ var _NodeCloneNeedsRegeneration = Node(struct { UserID *uint User *User RegisterMethod string - Tags []string + Tags Strings AuthKeyID *uint64 AuthKey *PreAuthKey Expiry *time.Time LastSeen *time.Time - ApprovedRoutes []netip.Prefix + ApprovedRoutes Prefixes CreatedAt time.Time UpdatedAt time.Time DeletedAt *time.Time diff --git a/hscontrol/types/types_view.go b/hscontrol/types/types_view.go index 6684ecd6..f8beb9a9 100644 --- a/hscontrol/types/types_view.go +++ b/hscontrol/types/types_view.go @@ -275,7 +275,7 @@ var _NodeViewNeedsRegeneration = Node(struct { MachineKey key.MachinePublic NodeKey key.NodePublic DiscoKey key.DiscoPublic - Endpoints []netip.AddrPort + Endpoints AddrPorts Hostinfo *tailcfg.Hostinfo IPv4 *netip.Addr IPv6 *netip.Addr @@ -284,12 +284,12 @@ var _NodeViewNeedsRegeneration = Node(struct { UserID *uint User *User RegisterMethod string - Tags []string + Tags Strings AuthKeyID *uint64 AuthKey *PreAuthKey Expiry *time.Time LastSeen *time.Time - ApprovedRoutes []netip.Prefix + ApprovedRoutes Prefixes CreatedAt time.Time UpdatedAt time.Time DeletedAt *time.Time