state: send PolicyChange from SetApprovedRoutes only when visibility moved

Otherwise NodeAdded. Any SubnetRoutes/ExitRoutes change still bumps
NodesGeneration, so in practice only unannounced approvals narrow.
This commit is contained in:
Kristoffer Dalby
2026-09-25 17:36:26 +00:00
parent c26f6e5255
commit 9526d74d61
4 changed files with 115 additions and 8 deletions
+86
View File
@@ -1272,3 +1272,89 @@ func TestMapRequestWithdrawingRoutesClearsUnhealthy(t *testing.T) {
assert.Empty(t, nv.AllApprovedRoutes())
assert.False(t, nv.Unhealthy(), "a node with no approved routes is no HA candidate")
}
// TestSetApprovedRoutesReportsVisibility pins which change an admin route
// approval reports: a whole-peer update when nothing peers or the policy
// read moved, a policy change otherwise.
func TestSetApprovedRoutesReportsVisibility(t *testing.T) {
subnetA := netip.MustParsePrefix("10.55.1.0/24")
subnetB := netip.MustParsePrefix("10.55.2.0/24")
exit := []netip.Prefix{netip.MustParsePrefix("0.0.0.0/0"), netip.MustParsePrefix("::/0")}
tests := []struct {
name string
prepare func(nodes []*types.Node)
approve []netip.Prefix
// wantType is the returned change's [change.Change.Type].
wantType string
// wantNewPeer is the index of a node that must become a peer.
wantNewPeer int
}{
{
name: "unannounced route moves nothing peers see",
prepare: func(nodes []*types.Node) {
nodes[0].Hostinfo = &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{subnetA}}
nodes[0].ApprovedRoutes = []netip.Prefix{subnetA}
},
approve: []netip.Prefix{subnetA, netip.MustParsePrefix("10.99.0.0/24")},
wantType: "peers",
wantNewPeer: -1,
},
{
// No primary moves and no peer is gained, but the node's
// approved subnets are a policy input (wildcard sources, via
// grants and its own reduced filter read them), so the
// policy manager reports it.
name: "second subnet as HA standby is a policy input",
prepare: func(nodes []*types.Node) {
nodes[1].Hostinfo = &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{subnetB}}
nodes[1].ApprovedRoutes = []netip.Prefix{subnetB}
nodes[0].Hostinfo = &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{subnetA, subnetB}}
nodes[0].ApprovedRoutes = []netip.Prefix{subnetA}
},
approve: []netip.Prefix{subnetA, subnetB},
wantType: "policy",
wantNewPeer: -1,
},
{
name: "first exit approval makes the node visible to a new peer",
prepare: func(nodes []*types.Node) {
nodes[0].Hostinfo = &tailcfg.Hostinfo{RoutableIPs: exit}
},
approve: exit,
wantType: "policy",
wantNewPeer: 3,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
s, ids, _ := newAutoApproveTestState(t, tt.prepare)
primariesBefore := s.nodeStore.PrimaryRoutes()
peersBefore := s.nodeStore.ListPeerIDs(ids[0])
_, c, err := s.SetApprovedRoutes(ids[0], tt.approve)
require.NoError(t, err)
assert.Equal(t, tt.wantType, c.Type())
if c.Type() == "peers" {
assert.Equal(t, ids[0], c.OriginNode)
assert.Contains(t, c.PeersChanged, ids[0])
}
peersAfter := s.nodeStore.ListPeerIDs(ids[0])
if tt.wantNewPeer < 0 {
assert.Equal(t, peersBefore, peersAfter, "scenario must keep the node's peers")
assert.Equal(t, primariesBefore, s.nodeStore.PrimaryRoutes(), "scenario must keep every primary")
} else {
assert.NotContains(t, peersBefore, ids[tt.wantNewPeer])
assert.Contains(t, peersAfter, ids[tt.wantNewPeer])
}
checkAutoApproveAdjacency(t, s)
})
}
}
+6
View File
@@ -973,6 +973,12 @@ func (s *NodeStore) ListPeers(id types.NodeID) views.Slice[types.NodeView] {
return views.SliceOf(peers)
}
// ListPeerIDs returns the sorted IDs of id's peers as a copy the caller
// may keep across later writes.
func (s *NodeStore) ListPeerIDs(id types.NodeID) []types.NodeID {
return slices.Sorted(slices.Values(s.data.Load().peersByNode[id]))
}
// PrimaryRouteFor returns the current primary advertiser for prefix.
func (s *NodeStore) PrimaryRouteFor(prefix netip.Prefix) (types.NodeID, bool) {
id, ok := s.data.Load().routes[prefix]
+4 -4
View File
@@ -807,7 +807,7 @@ func TestPersistCallerChangeDecisions(t *testing.T) {
wantPeersChanged: true,
},
{
name: "SetApprovedRoutes keeps reporting a policy change (Task 6 narrows this)",
name: "SetApprovedRoutes of an unannounced route resends only the node",
run: func(t *testing.T, s *State, nodeID types.NodeID) change.Change {
t.Helper()
@@ -816,9 +816,9 @@ func TestPersistCallerChangeDecisions(t *testing.T) {
return c
},
wantType: "policy",
wantOriginNode: false,
wantPeersChanged: false,
wantType: "peers",
wantOriginNode: true,
wantPeersChanged: true,
},
{
name: "SaveNode reports no change for a payload-only save",
+19 -4
View File
@@ -1053,11 +1053,14 @@ func (s *State) SetNodeTags(nodeID types.NodeID, tags []string) (types.NodeView,
}
// SetApprovedRoutes sets the network routes that a node is approved to advertise.
// It returns a PolicyChange when a primary moved, the policy manager saw a
// policy input change, or the node's peers changed; otherwise NodeAdded.
func (s *State) SetApprovedRoutes(nodeID types.NodeID, routes []netip.Prefix) (types.NodeView, change.Change, error) {
// TODO(kradalby): In principle we should call the AutoApprove logic here
// because even if the CLI removes an auto-approved route, it will be added
// back automatically.
prevRoutes := s.nodeStore.PrimaryRoutes()
prevPeers := s.nodeStore.ListPeerIDs(nodeID)
genBefore := s.polMan.NodesGeneration()
n, ok := s.nodeStore.UpdateNode(nodeID, func(node *types.Node) {
@@ -1076,15 +1079,27 @@ func (s *State) SetApprovedRoutes(nodeID types.NodeID, routes []netip.Prefix) (t
// Persist the node changes to the database
nodeView, c, err := s.persistNodeAndRefreshPolicy(n, genBefore)
// The reads around the write can also see concurrent writers; that
// only turns a whole-peer update into a policy change, never back.
routeChange := !maps.Equal(prevRoutes, s.nodeStore.PrimaryRoutes())
peersChanged := !slices.Equal(prevPeers, s.nodeStore.ListPeerIDs(nodeID))
if err != nil {
// The NodeStore holds the new routes either way.
if routeChange || peersChanged {
c = c.Merge(change.PolicyChange())
}
return nodeView, c, err
}
// PolicyChange fans out a fresh netmap whenever the new approved
// set shifted a primary advertiser.
routeChange := !maps.Equal(prevRoutes, s.nodeStore.PrimaryRoutes())
if routeChange || !c.IsFull() {
if routeChange || peersChanged || !c.IsEmpty() {
c = change.PolicyChange()
} else {
// No visibility or effective route moved; resend the node so
// peers hold its current state.
c = change.NodeAdded(nodeID)
}
return nodeView, c, nil