diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index 7d856f3e..3609e3a9 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -1094,9 +1094,18 @@ func equalUnordered[E comparable](a, b []E, cmp func(E, E) int) bool { // HasPolicyChange reports whether the node has changes that affect // policy evaluation. Includes approved subnet routes because they act -// as source identity in [Node.CanAccess] for subnet-to-subnet ACLs. +// as source identity in [Node.CanAccess] for subnet-to-subnet ACLs, +// and enabled exit routes because autogroup:internet and exit-node +// reduction depend on which exit nodes are advertised-and-approved. func (nv NodeView) HasPolicyChange(other NodeView) bool { - if nv.UserID() != other.UserID() { + if nv.TypedUserID() != other.TypedUserID() { + return true + } + + // The policy resolves ownership through the loaded association, so + // compare it as well as the raw foreign key. + if nv.User().Valid() != other.User().Valid() || + (nv.User().Valid() && nv.User().ID() != other.User().ID()) { return true } @@ -1112,6 +1121,10 @@ func (nv NodeView) HasPolicyChange(other NodeView) bool { return true } + if !equalPrefixesUnordered(nv.ExitRoutes(), other.ExitRoutes()) { + return true + } + return false } diff --git a/hscontrol/types/node_test.go b/hscontrol/types/node_test.go index 6a23fc9f..c723e5dc 100644 --- a/hscontrol/types/node_test.go +++ b/hscontrol/types/node_test.go @@ -5,11 +5,13 @@ import ( "net/netip" "strings" "testing" + "time" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/juanfont/headscale/hscontrol/policy/matcher" "github.com/juanfont/headscale/hscontrol/util" + "github.com/stretchr/testify/require" "tailscale.com/tailcfg" "tailscale.com/types/key" ) @@ -889,3 +891,155 @@ func TestHasNetworkChanges(t *testing.T) { }) } } + +func policyChangeTestNode() Node { + ipv4 := netip.MustParseAddr("100.64.0.1") + now := time.Now() + + return Node{ + ID: 1, + MachineKey: key.NewMachine().Public(), + NodeKey: key.NewNode().Public(), + DiscoKey: key.NewDisco().Public(), + Hostname: "node", + GivenName: "node", + IPv4: &ipv4, + CreatedAt: now, + UpdatedAt: now, + Hostinfo: &tailcfg.Hostinfo{Hostname: "node"}, + } +} + +// TestHasPolicyChangeUserIDPointerIdentity ensures two distinct *uint +// values holding the same user ID do not register as a policy change. +func TestHasPolicyChangeUserIDPointerIdentity(t *testing.T) { + userID := uint(7) + + a := policyChangeTestNode() + a.UserID = &userID + + b := policyChangeTestNode() + otherPtr := new(uint) + *otherPtr = 7 + b.UserID = otherPtr + + // Different pointers, same value. + require.NotSame(t, a.UserID, b.UserID) + require.False(t, a.View().HasPolicyChange(b.View()), + "same UserID value via distinct pointers must not register as policy change") + + // And a real change must register. + c := policyChangeTestNode() + otherVal := uint(8) + c.UserID = &otherVal + require.True(t, a.View().HasPolicyChange(c.View()), + "different UserID value must register as policy change") +} + +// TestHasPolicyChangeUserIDValidity covers the nil vs non-nil transition. +func TestHasPolicyChangeUserIDValidity(t *testing.T) { + a := policyChangeTestNode() + // a.UserID nil + b := policyChangeTestNode() + v := uint(1) + b.UserID = &v + + require.True(t, a.View().HasPolicyChange(b.View()), + "nil -> non-nil UserID must register as policy change") +} + +// TestHasPolicyChangeExitRoutes covers the missing ExitRoutes comparison. +func TestHasPolicyChangeExitRoutes(t *testing.T) { + exitV4 := netip.MustParsePrefix("0.0.0.0/0") + exitV6 := netip.MustParsePrefix("::/0") + + base := policyChangeTestNode() + base.Hostinfo.RoutableIPs = []netip.Prefix{exitV4, exitV6} + base.ApprovedRoutes = nil // not approved -> no exit routes + + b := policyChangeTestNode() + b.Hostinfo.RoutableIPs = []netip.Prefix{exitV4, exitV6} + b.ApprovedRoutes = []netip.Prefix{exitV4, exitV6} // approved -> exit routes live + + require.True(t, base.View().HasPolicyChange(b.View()), + "enabling exit routes must register as policy change") + + // Reverse: approved on both, no change. + c := policyChangeTestNode() + c.Hostinfo.RoutableIPs = []netip.Prefix{exitV4, exitV6} + c.ApprovedRoutes = []netip.Prefix{exitV4, exitV6} + + require.False(t, b.View().HasPolicyChange(c.View()), + "identical exit-route state must not register as policy change") +} + +func TestHasPolicyChangeFields(t *testing.T) { + subnet := netip.MustParsePrefix("10.0.0.0/24") + exit := netip.MustParsePrefix("0.0.0.0/0") + + tests := []struct { + name string + mutate func(*Node) + want bool + }{ + {name: "no change", mutate: func(*Node) {}, want: false}, + {name: "last seen", mutate: func(n *Node) { n.LastSeen = new(time.Now()) }, want: false}, + {name: "expiry", mutate: func(n *Node) { n.Expiry = new(time.Now()) }, want: false}, + {name: "hostname", mutate: func(n *Node) { n.Hostname = "other" }, want: false}, + {name: "tags", mutate: func(n *Node) { n.Tags = []string{"tag:x"} }, want: true}, + { + name: "ipv4", + mutate: func(n *Node) { + ip := netip.MustParseAddr("100.64.0.2") + n.IPv4 = &ip + }, + want: true, + }, + { + name: "announced but unapproved subnet", + mutate: func(n *Node) { n.Hostinfo.RoutableIPs = []netip.Prefix{subnet} }, + want: false, + }, + { + name: "approved and announced subnet", + mutate: func(n *Node) { + n.Hostinfo.RoutableIPs = []netip.Prefix{subnet} + n.ApprovedRoutes = []netip.Prefix{subnet} + }, + want: true, + }, + { + name: "approved and announced exit", + mutate: func(n *Node) { + n.Hostinfo.RoutableIPs = []netip.Prefix{exit} + n.ApprovedRoutes = []netip.Prefix{exit} + }, + want: true, + }, + { + name: "user association cleared", + mutate: func(n *Node) { n.User = nil }, + want: true, + }, + { + name: "user association moved", + mutate: func(n *Node) { n.User = &User{ID: 8} }, + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + userID := uint(7) + + base := policyChangeTestNode() + base.UserID = &userID + base.User = &User{ID: userID} + + other := *base.Clone() + tt.mutate(&other) + + require.Equal(t, tt.want, other.View().HasPolicyChange(base.View())) + }) + } +}