From be322e8ea70bd72a44106a3ac3d29f8426f0c2ed Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 9 Sep 2026 14:36:20 +0000 Subject: [PATCH] policy,types: skip recompile when the user list is unchanged SetUsers now also reports whether user-derived peer adjacency moved. Updates #3417 --- hscontrol/policy/pm.go | 5 +- hscontrol/policy/v2/policy.go | 47 +++++++++++++----- hscontrol/policy/v2/policy_test.go | 78 ++++++++++++++++++++++++++++++ hscontrol/state/persist_test.go | 21 ++++++++ hscontrol/state/state.go | 18 ++++--- hscontrol/types/users.go | 10 ++++ 6 files changed, 159 insertions(+), 20 deletions(-) diff --git a/hscontrol/policy/pm.go b/hscontrol/policy/pm.go index 99e88807..2e459b3e 100644 --- a/hscontrol/policy/pm.go +++ b/hscontrol/policy/pm.go @@ -25,7 +25,10 @@ type PolicyManager interface { // from the current policy, avoiding trust of client-provided URL params. SSHCheckParams(srcNodeID, dstNodeID types.NodeID) (time.Duration, bool) SetPolicy(pol []byte) (bool, error) - SetUsers(users []types.User) (bool, error) + // SetUsers replaces the user list. policyChanged reports whether clients + // need a policy refresh; peerMapChanged reports whether user-derived peer + // adjacency may have changed. Both are false when the list is unchanged. + SetUsers(users []types.User) (policyChanged, peerMapChanged bool, err error) SetNodes(nodes views.Slice[types.NodeView]) (bool, error) // NodeCanHaveTag reports whether the given node can have the given tag. NodeCanHaveTag(node types.NodeView, tag string) bool diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index d59f44b5..fc4c13c4 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -803,34 +803,57 @@ func (pm *PolicyManager) MatchersForNode(node types.NodeView) ([]matcher.Match, return matchers, nil } -// SetUsers updates the users in the policy manager and updates the filter rules. -func (pm *PolicyManager) SetUsers(users []types.User) (bool, error) { +// SetUsers replaces the user list and recompiles when it changed. Both results +// are false for an unchanged list, so callers can skip the peer-map rebuild +// and the client refresh that a user change would otherwise require. +func (pm *PolicyManager) SetUsers(users []types.User) (bool, bool, error) { if pm == nil { - return false, nil + return false, false, nil } pm.mu.Lock() defer pm.mu.Unlock() + if equalUsers(pm.users, users) { + return false, false, nil + } + + prev := pm.users pm.users = users - // Clear SSH policy map when users change to force SSH policy recomputation - // This ensures that if SSH policy compilation previously failed due to missing users, - // it will be retried with the new user list + // SSH policies resolve users by name, so they are recomputed on any + // user change. pm.sshPolicyMap.Clear() - changed, err := pm.updateLocked() + policyChanged, err := pm.updateLocked() if err != nil { - return false, err + // Keep the old list so a retry with the same input recompiles + // instead of being treated as unchanged. + pm.users = prev + + return false, false, err } - // If SSH policies exist, force a policy change when users are updated - // This ensures nodes get updated SSH policies even if other policy hashes didn't change + // SSH rules embed user identity, so a user change needs a client refresh + // even when the filter hash did not move. if pm.pol != nil && len(pm.pol.SSHs) > 0 { - return true, nil + policyChanged = true } - return changed, nil + return policyChanged, true, nil +} + +// equalUsers compares user lists ignoring order and every field the policy +// does not read, so a row touch such as an OIDC login is not a change. +func equalUsers(a, b []types.User) bool { + if len(a) != len(b) { + return false + } + + byID := func(l, r types.User) int { return cmp.Compare(l.ID, r.ID) } + a, b = slices.SortedFunc(slices.Values(a), byID), slices.SortedFunc(slices.Values(b), byID) + + return slices.EqualFunc(a, b, func(l, r types.User) bool { return l.PolicyEqual(&r) }) } // SetNodes updates the nodes in the policy manager and updates the filter rules. diff --git a/hscontrol/policy/v2/policy_test.go b/hscontrol/policy/v2/policy_test.go index a5219470..fc99162c 100644 --- a/hscontrol/policy/v2/policy_test.go +++ b/hscontrol/policy/v2/policy_test.go @@ -4,6 +4,7 @@ import ( "net/netip" "slices" "testing" + "time" "github.com/google/go-cmp/cmp" "github.com/juanfont/headscale/hscontrol/policy/matcher" @@ -355,6 +356,83 @@ func TestSSHCheckParamsUnhydratedUserNoPanic(t *testing.T) { }, "SSHCheckParams must not panic when a non-tagged node has an unhydrated User") } +func TestSetUsers(t *testing.T) { + const allowAll = `{"acls":[{"action":"accept","src":["*"],"dst":["*:*"]}]}` + + const sshCheck = `{ + "ssh": [ + { + "action": "check", + "src": ["user1@headscale.net"], + "dst": ["autogroup:self"], + "users": ["root"] + } + ] + }` + + tests := []struct { + name string + policy string + mutate func(*types.User) + + wantPolicyChanged bool + wantPeerMapChanged bool + }{ + { + name: "identical users without ssh", + policy: allowAll, + mutate: func(*types.User) {}, + }, + { + name: "identical users with ssh", + policy: sshCheck, + mutate: func(*types.User) {}, + }, + { + name: "timestamp bump only", + policy: sshCheck, + mutate: func(u *types.User) { u.UpdatedAt = u.UpdatedAt.Add(time.Hour) }, + }, + { + name: "display name change without ssh", + policy: allowAll, + mutate: func(u *types.User) { u.DisplayName = "Renamed" }, + }, + { + name: "email change without ssh", + policy: allowAll, + mutate: func(u *types.User) { u.Email = "other@headscale.net" }, + + wantPeerMapChanged: true, + }, + { + name: "rename with ssh", + policy: sshCheck, + mutate: func(u *types.User) { u.Name = "renamed" }, + + wantPolicyChanged: true, + wantPeerMapChanged: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + users := types.Users{{ID: 1, Name: "user1", Email: "user1@headscale.net"}} + + pm, err := NewPolicyManager([]byte(tt.policy), users, types.Nodes{}.ViewSlice()) + require.NoError(t, err) + + updated := slices.Clone(users) + tt.mutate(&updated[0]) + + policyChanged, peerMapChanged, err := pm.SetUsers(updated) + require.NoError(t, err) + require.Equal(t, tt.wantPolicyChanged, policyChanged, "policyChanged") + require.Equal(t, tt.wantPeerMapChanged, peerMapChanged, "peerMapChanged") + }) + } +} + // TestInvalidateGlobalPolicyCache tests the cache invalidation logic for global policies. func TestInvalidateGlobalPolicyCache(t *testing.T) { mustIPPtr := func(s string) *netip.Addr { diff --git a/hscontrol/state/persist_test.go b/hscontrol/state/persist_test.go index 762f1bc7..abad20cf 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -647,3 +647,24 @@ func TestConcurrentPreAuthKeyRegistrationSameMachineKey(t *testing.T) { require.Equal(t, 1, s.ListNodes().Len(), "concurrent registrations of one machine key must yield a single node") } + +// TestUpdatePolicyManagerUsersUnchangedKeepsSnapshot ensures re-sending the +// same user list does not rebuild peer adjacency, while a real user change +// does. +func TestUpdatePolicyManagerUsersUnchangedKeepsSnapshot(t *testing.T) { + _, s, _ := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + require.NoError(t, s.UpdatePolicyManagerUsersForTest()) + + before := s.nodeStore.data.Load() + + require.NoError(t, s.UpdatePolicyManagerUsersForTest()) + require.Same(t, before, s.nodeStore.data.Load(), + "unchanged users must not rebuild the peer map") + + _, _, err := s.CreateUser(types.User{Name: "second"}) + require.NoError(t, err) + require.NotSame(t, before, s.nodeStore.data.Load(), + "a user change must rebuild the peer map") +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 1fede1d6..e5adc8dc 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -2878,12 +2878,9 @@ func reauthChange(node types.NodeView, isRelogin, policyChanged bool) change.Cha } } -// updatePolicyManagerUsers updates the policy manager with current users. -// Returns true if the policy changed and notifications should be sent. -// TODO(kradalby): This is a temporary stepping stone, ultimately we should -// have the list already available so it could go much quicker. Alternatively -// the policy manager could have a remove or add list for users. -// updatePolicyManagerUsers refreshes the policy manager with current user data. +// updatePolicyManagerUsers pushes the current user list into the policy +// manager, rebuilds peer adjacency when user identity changed, and returns +// a PolicyChange when clients need a refresh. func (s *State) updatePolicyManagerUsers() (change.Change, error) { users, err := s.ListAllUsers() if err != nil { @@ -2892,13 +2889,20 @@ func (s *State) updatePolicyManagerUsers() (change.Change, error) { log.Debug().Caller().Int("user.count", len(users)).Msg("policy manager user update initiated because user list modification detected") - changed, err := s.polMan.SetUsers(users) + changed, peerMapChanged, err := s.polMan.SetUsers(users) if err != nil { return change.Change{}, fmt.Errorf("updating policy manager users: %w", err) } log.Debug().Caller().Bool("policy.changed", changed).Msg("policy manager user update completed because SetUsers operation finished") + if peerMapChanged { + // User-driven matcher state changed: rebuild candidate adjacency + // so peer visibility reflects the new policy. Without this, the + // cached peersByNode stays stale until the next node write. + s.nodeStore.RebuildPeerMaps() + } + if changed { return change.PolicyChange(), nil } diff --git a/hscontrol/types/users.go b/hscontrol/types/users.go index ea5f3de1..f44428c7 100644 --- a/hscontrol/types/users.go +++ b/hscontrol/types/users.go @@ -105,6 +105,16 @@ func (u *User) StringID() string { return strconv.FormatUint(uint64(u.ID), 10) } +// PolicyEqual reports whether the policy would resolve both users the same +// way: the same row, and the same name, email, and provider identity that +// user aliases match on. +func (u *User) PolicyEqual(o *User) bool { + return u.ID == o.ID && + u.Name == o.Name && + u.Email == o.Email && + u.ProviderIdentifier == o.ProviderIdentifier +} + // TypedID returns a pointer to the user's ID as a [UserID] type. // This is a convenience method to avoid ugly casting like ptr.To(types.UserID(user.ID)). func (u *User) TypedID() *UserID {