From ea6b0bf0b7122ddab67b1c09b372d7f1255bd85b Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 2 Oct 2026 10:51:16 +0000 Subject: [PATCH] mapper: reconcile self on policy recomputes Self renders from the same state as peers, so leaving it out of policy responses hid an exit node's approved routes until it reconnected. Fixes #3502 --- hscontrol/mapper/batcher.go | 7 +- hscontrol/mapper/batcher_test.go | 58 ++++++++ hscontrol/mapper/mapper.go | 14 +- hscontrol/mapper/node_conn.go | 25 +++- hscontrol/servertest/issues_test.go | 179 +++++++++++++++++++++--- hscontrol/servertest/via_compat_test.go | 5 + integration/route_test.go | 5 + 7 files changed, 259 insertions(+), 34 deletions(-) diff --git a/hscontrol/mapper/batcher.go b/hscontrol/mapper/batcher.go index 321814212..c70a4197f 100644 --- a/hscontrol/mapper/batcher.go +++ b/hscontrol/mapper/batcher.go @@ -129,9 +129,7 @@ func generateMapResponse( } removed = nc.computePeerDiff(currentPeerIDs) - // Include self node when this is a self-update (e.g., node's own tags changed) - // so the node sees its updated self info along with new packet filters. - resp, err = mapper.policyChangeResponse(nodeID, version, currentPeers, isSelfUpdate) + resp, err = mapper.policyChangeResponse(nodeID, version, currentPeers) } else if isSelfUpdate { // Non-policy self-update: just send the self node info resp, err = mapper.selfMapResponse(nodeID, version) @@ -386,6 +384,9 @@ func (b *Batcher) AddNode( nodeConn.workMu.Unlock() + // Still pendingInitial, so no broadcast can race this. + newEntry.lastSelf.Store(initialMap.Node) + // Open the connection for broadcast sends now that the initial // map is the stream's first frame; send() requeued any changes // that arrived in the meantime. diff --git a/hscontrol/mapper/batcher_test.go b/hscontrol/mapper/batcher_test.go index 0ff1a5de6..75c417ec5 100644 --- a/hscontrol/mapper/batcher_test.go +++ b/hscontrol/mapper/batcher_test.go @@ -2715,3 +2715,61 @@ func TestDNSConfigOnlyWithSelfRefresh(t *testing.T) { assert.Nil(t, sent[0].DNSConfig, "policy response must not carry DNSConfig") assert.NotNil(t, sent[1].DNSConfig, "self refresh must carry DNSConfig") } + +// TestSelfSentOnlyWhenChanged pins that a policy recompute carries the +// node's own self to each connection that does not hold it yet: self renders +// from the same state as peers (issue #3502), and a Node forces a full client +// netmap rebuild. +func TestSelfSentOnlyWhenChanged(t *testing.T) { + testData, cleanup := setupBatcherWithTestData(t, NewBatcherAndMapper, 1, 2, normalBufferSize) + defer cleanup() + + b := testData.Batcher.Batcher + self := &testData.Nodes[0] + + rename := func(name string) { + _, _, err := testData.State.RenameNode(self.n.ID, name) + require.NoError(t, err) + } + + require.NoError(t, b.AddNode(self.n.ID, self.ch, tailcfg.CapabilityVersion(100), nil)) + require.NotNil(t, expectReceive(t, self.ch, "initial map").Node) + + nc, ok := b.nodes.Load(self.n.ID) + require.True(t, ok) + + policyFrame := func(chs ...chan *tailcfg.MapResponse) []*tailcfg.MapResponse { + nc.workMu.Lock() + defer nc.workMu.Unlock() + + require.NoError(t, handleNodeChange(nc, b.mapper, change.PolicyChange())) + + frames := make([]*tailcfg.MapResponse, 0, len(chs)) + for _, ch := range chs { + frames = append(frames, expectReceive(t, ch, "policy frame")) + } + + return frames + } + + assert.Nil(t, policyFrame(self.ch)[0].Node, "unchanged self must be dropped") + + rename("renamed") + + frame := policyFrame(self.ch)[0] + require.NotNil(t, frame.Node, "moved self must be sent") + assert.Contains(t, frame.Node.Name, "renamed") + + // A connection joining after self moved gets the new self in its + // initial map; the next recompute must still reach the older one. + rename("renamed-again") + + ch2 := make(chan *tailcfg.MapResponse, normalBufferSize) + require.NoError(t, b.AddNode(self.n.ID, ch2, tailcfg.CapabilityVersion(100), nil)) + require.NotNil(t, expectReceive(t, ch2, "second connection's initial map").Node) + + frames := policyFrame(self.ch, ch2) + require.NotNil(t, frames[0].Node, "first connection still holds the old self") + assert.Contains(t, frames[0].Node.Name, "renamed-again") + assert.Nil(t, frames[1].Node, "second connection already holds the new self") +} diff --git a/hscontrol/mapper/mapper.go b/hscontrol/mapper/mapper.go index f031839cb..eb96090fd 100644 --- a/hscontrol/mapper/mapper.go +++ b/hscontrol/mapper/mapper.go @@ -309,7 +309,8 @@ func (m *mapper) selfMapResponse( // - PeersChanged for remaining peers (their AllowedIPs may have changed due to policy) // - Updated PacketFilters // - Updated SSHPolicy (SSH rules may reference users/groups that changed) -// - Optionally, the node's own self info (when includeSelf is true) +// - The node's own self info, which renders from the same state as peers; +// dropped per connection when unchanged, see [connectionEntry.withSelfDelta] // // DNSConfig is left out: it forces clients into a full netmap rebuild, and // the node's DNS config inputs arrive with its own [change.SelfUpdate], see @@ -317,24 +318,17 @@ func (m *mapper) selfMapResponse( // // This avoids the issue where an empty Peers slice is interpreted by Tailscale // clients as "no change" rather than "no peers". -// When includeSelf is true, the node's self info is included so that a node -// whose own attributes changed (e.g., tags via admin API) sees its updated -// self info along with the new packet filters. func (m *mapper) policyChangeResponse( nodeID types.NodeID, capVer tailcfg.CapabilityVersion, currentPeers views.Slice[types.NodeView], - includeSelf bool, ) (*tailcfg.MapResponse, error) { builder := m.NewMapResponseBuilder(nodeID). WithDebugType(policyResponseDebug). WithCapabilityVersion(capVer). WithPacketFilters(). - WithSSHPolicy() - - if includeSelf { - builder = builder.WithSelfNode() - } + WithSSHPolicy(). + WithSelfNode() // Send remaining peers in PeersChanged - their AllowedIPs may have // changed due to the policy update (e.g., different routes allowed). diff --git a/hscontrol/mapper/node_conn.go b/hscontrol/mapper/node_conn.go index 3ef68b67b..386a6862c 100644 --- a/hscontrol/mapper/node_conn.go +++ b/hscontrol/mapper/node_conn.go @@ -50,6 +50,25 @@ type connectionEntry struct { // can never become the stream's first frame ahead of the initial // map. The zero value means the connection is ready. pendingInitial atomic.Bool + + // lastSelf is the self node last delivered to this connection's + // client, which keeps it until sent another. Every send to an + // established connection must go through [multiChannelNodeConn.send] + // to keep it current. + lastSelf atomic.Pointer[tailcfg.Node] +} + +// withSelfDelta returns data without its Node when this client already +// holds an equal one: a Node forces a full client netmap rebuild. +func (entry *connectionEntry) withSelfDelta(data *tailcfg.MapResponse) *tailcfg.MapResponse { + if data.Node == nil || !data.Node.Equal(entry.lastSelf.Load()) { + return data + } + + stripped := *data + stripped.Node = nil + + return &stripped } // multiChannelNodeConn manages multiple concurrent connections for a single node. @@ -341,7 +360,7 @@ func (mc *multiChannelNodeConn) send(data *tailcfg.MapResponse) error { ) for _, conn := range snapshot { - err := conn.send(data) + err := conn.send(conn.withSelfDelta(data)) if err != nil { lastErr = err @@ -352,6 +371,10 @@ func (mc *multiChannelNodeConn) send(data *tailcfg.MapResponse) error { Msg("send: connection failed") } else { successCount++ + + if data.Node != nil { + conn.lastSelf.Store(data.Node) + } } } diff --git a/hscontrol/servertest/issues_test.go b/hscontrol/servertest/issues_test.go index 0ac3eabc5..618789234 100644 --- a/hscontrol/servertest/issues_test.go +++ b/hscontrol/servertest/issues_test.go @@ -222,36 +222,128 @@ func TestIssuesRoutes(t *testing.T) { } }) - // When the server approves routes for a node, that node - // should receive a self-update reflecting the change. - t.Run("self_update_after_route_approval", func(t *testing.T) { + // The client derives ExitNodeOption from its own SelfNode.AllowedIPs, + // and SelfNode only changes when a MapResponse carries Node. SaaS + // captures show approved exit routes on the exit node's own SelfNode + // (routes-ea*, routes-b17/b18). Peers are the control: they must see + // the routes, proving the change was dispatched. + // https://github.com/juanfont/headscale/issues/3502 + t.Run("exit_route_approval_reaches_self", func(t *testing.T) { t.Parallel() srv := servertest.NewServer(t) - user := srv.CreateUser(t, "selfup-user") + user := srv.CreateUser(t, "selfexit-user") - c1 := servertest.NewClient(t, srv, "selfup-node1", + exit := servertest.NewClient(t, srv, "selfexit-node", servertest.WithUser(user)) - servertest.NewClient(t, srv, "selfup-node2", + obs := servertest.NewClient(t, srv, "selfexit-obs", servertest.WithUser(user)) - c1.WaitForPeers(t, 1, 10*time.Second) + exit.WaitForPeers(t, 1, 10*time.Second) + advertiseRoutes(t, exit, obs, tsaddr.ExitRoutes()) - nodeID := findNodeID(t, srv, "selfup-node1") - route := netip.MustParsePrefix("10.77.0.0/24") - - countBefore := c1.UpdateCount() - - _, routeChange, err := srv.State().SetApprovedRoutes( - nodeID, []netip.Prefix{route}) + _, c, err := srv.State().SetApprovedRoutes( + findNodeID(t, srv, "selfexit-node"), tsaddr.ExitRoutes()) require.NoError(t, err) - srv.App.Change(routeChange) + srv.App.Change(c) - c1.WaitForCondition(t, "self-update after route approval", - 10*time.Second, - func(nm *netmap.NetworkMap) bool { - return c1.UpdateCount() > countBefore - }) + obs.WaitForCondition(t, "peer offers exit node", 10*time.Second, + peerOffersExit("selfexit-node")) + exit.WaitForCondition(t, "self offers exit node", 10*time.Second, + selfOffersExit) + }) + + // Advertising an already-approved exit route goes through + // UpdateNodeFromMapRequest instead of SetApprovedRoutes. This is the + // re-advertise workaround from the issue that 0.29.4 broke. + t.Run("exit_route_advertised_after_approval_reaches_self", func(t *testing.T) { + t.Parallel() + + srv := servertest.NewServer(t) + user := srv.CreateUser(t, "preexit-user") + + exit := servertest.NewClient(t, srv, "preexit-node", + servertest.WithUser(user)) + obs := servertest.NewClient(t, srv, "preexit-obs", + servertest.WithUser(user)) + + exit.WaitForPeers(t, 1, 10*time.Second) + + _, c, err := srv.State().SetApprovedRoutes( + findNodeID(t, srv, "preexit-node"), tsaddr.ExitRoutes()) + require.NoError(t, err) + srv.App.Change(c) + + advertiseRoutes(t, exit, obs, tsaddr.ExitRoutes()) + + obs.WaitForCondition(t, "peer offers exit node", 10*time.Second, + peerOffersExit("preexit-node")) + exit.WaitForCondition(t, "self offers exit node", 10*time.Second, + selfOffersExit) + }) + + // A policy reload that auto-approves an advertised exit route goes + // through autoApproveNodes, a third producer of the same change. + t.Run("exit_route_auto_approved_on_reload_reaches_self", func(t *testing.T) { + t.Parallel() + + srv := servertest.NewServer(t) + user := srv.CreateUser(t, "reloadexit-user") + + exit := servertest.NewClient(t, srv, "reloadexit-node", + servertest.WithUser(user)) + obs := servertest.NewClient(t, srv, "reloadexit-obs", + servertest.WithUser(user)) + + exit.WaitForPeers(t, 1, 10*time.Second) + advertiseRoutes(t, exit, obs, tsaddr.ExitRoutes()) + + _, err := srv.State().SetPolicyInDB(`{ + "acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}], + "autoApprovers": {"exitNode": ["reloadexit-user@"]} + }`) + require.NoError(t, err) + + changes, err := srv.State().ReloadPolicy() + require.NoError(t, err) + srv.App.Change(changes...) + + obs.WaitForCondition(t, "peer offers exit node", 10*time.Second, + peerOffersExit("reloadexit-node")) + exit.WaitForCondition(t, "self offers exit node", 10*time.Second, + selfOffersExit) + }) + + // An auto-approver approves the route inside the advertising map + // request, a fourth producer of the same change. + t.Run("exit_route_auto_approved_on_advertise_reaches_self", func(t *testing.T) { + t.Parallel() + + srv := servertest.NewServer(t) + user := srv.CreateUser(t, "advexit-user") + + _, err := srv.State().SetPolicyInDB(`{ + "acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}], + "autoApprovers": {"exitNode": ["advexit-user@"]} + }`) + require.NoError(t, err) + + changes, err := srv.State().ReloadPolicy() + require.NoError(t, err) + srv.App.Change(changes...) + + exit := servertest.NewClient(t, srv, "advexit-node", + servertest.WithUser(user)) + obs := servertest.NewClient(t, srv, "advexit-obs", + servertest.WithUser(user)) + + exit.WaitForPeers(t, 1, 10*time.Second) + advertiseRoutes(t, exit, obs, tsaddr.ExitRoutes()) + + obs.WaitForCondition(t, "peer offers exit node", 10*time.Second, + peerOffersExit("advexit-node")) + exit.WaitForCondition(t, "self offers exit node", 10*time.Second, + selfOffersExit) }) // [tailcfg.Hostinfo] route advertisement should be stored on server. @@ -1104,3 +1196,50 @@ func findNodeID(tb testing.TB, srv *servertest.TestServer, hostname string) type return 0 } + +// advertiseRoutes pushes routes in c's Hostinfo and waits until obs sees +// them, so the server has stored the announcement. +func advertiseRoutes(tb testing.TB, c, obs *servertest.TestClient, routes []netip.Prefix) { + tb.Helper() + + c.Direct().SetHostinfo(&tailcfg.Hostinfo{ + BackendLogID: "servertest-" + c.Name, + Hostname: c.Name, + RoutableIPs: routes, + }) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + _ = c.Direct().SendUpdate(ctx) + + obs.WaitForCondition(tb, "routes in peer hostinfo", 10*time.Second, + func(nm *netmap.NetworkMap) bool { + p, ok := peerByHostname(nm, c.Name) + + return ok && p.Hostinfo().RoutableIPs().Len() == len(routes) + }) +} + +// selfOffersExit mirrors how tailscaled computes Self.ExitNodeOption. +func selfOffersExit(nm *netmap.NetworkMap) bool { + return nm.SelfNode.Valid() && tsaddr.ContainsExitRoutes(nm.SelfNode.AllowedIPs()) +} + +func peerOffersExit(hostname string) func(*netmap.NetworkMap) bool { + return func(nm *netmap.NetworkMap) bool { + p, ok := peerByHostname(nm, hostname) + + return ok && tsaddr.ContainsExitRoutes(p.AllowedIPs()) + } +} + +func peerByHostname(nm *netmap.NetworkMap, hostname string) (tailcfg.NodeView, bool) { + for _, p := range nm.Peers { + if hi := p.Hostinfo(); hi.Valid() && hi.Hostname() == hostname { + return p, true + } + } + + return tailcfg.NodeView{}, false +} diff --git a/hscontrol/servertest/via_compat_test.go b/hscontrol/servertest/via_compat_test.go index 79c31c16f..d8012ec08 100644 --- a/hscontrol/servertest/via_compat_test.go +++ b/hscontrol/servertest/via_compat_test.go @@ -255,6 +255,11 @@ func runViaMapCompat(t *testing.T, c *testcapture.Capture) { nm := cl.Netmap() require.NotNil(tt, nm, "netmap is nil") + // Routes were approved on live sessions: verify that approval + // reached SelfNode throughout the stable comparison window. + require.Equal(tt, selfOffersExit(capture.Netmap), selfOffersExit(nm), + "SelfNode exit routes should match SaaS") + compareNetmap(tt, nm, capture, clients, saasAddrs) }) }) diff --git a/integration/route_test.go b/integration/route_test.go index 0c37ea2f9..f2dd914dd 100644 --- a/integration/route_test.go +++ b/integration/route_test.go @@ -1693,6 +1693,11 @@ func TestEnablingExitRoutes(t *testing.T) { assert.Contains(c, peerStatus.AllowedIPs.AsSlice(), tsaddr.AllIPv6()) } } + + // The approved node must learn its own routes on its live + // session, not only after reconnecting (issue #3502). + assert.True(c, status.Self.ExitNodeOption, + "%s should offer itself as exit node", client.Hostname()) } }, integrationutil.ScaledTimeout(10*time.Second), integrationutil.SlowPoll, "clients should see new routes") }