diff --git a/hscontrol/state/maprequest_test.go b/hscontrol/state/maprequest_test.go index 3eb3685e9..daf01a059 100644 --- a/hscontrol/state/maprequest_test.go +++ b/hscontrol/state/maprequest_test.go @@ -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) + }) + } +} diff --git a/hscontrol/state/node_store.go b/hscontrol/state/node_store.go index 617a50078..1fa7db3d8 100644 --- a/hscontrol/state/node_store.go +++ b/hscontrol/state/node_store.go @@ -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] diff --git a/hscontrol/state/persist_test.go b/hscontrol/state/persist_test.go index 4a082fe3e..f2b532384 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -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", diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 926df8bf0..f41fa4298 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -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