From 9be21088f42f5a8beca72480cfc3ea56ed1a7343 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Claus=20Lensb=C3=B8l?= Date: Wed, 27 May 2026 13:05:12 -0400 Subject: [PATCH] wgengine/{,magicsock},tstest/natlab/vmtest: send disco on cached netmap (#19878) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Originally found when adding tests for working with cached netmaps, and finding the added tests to be flakey. When working off of a cached netmap, if a node exists in the cached netmap but does not yet have any endpoints, DERP connections are available but not direct ones. By sending callMeMaybe to nodes without endpoints in the cached netmap, we can establish direct connections for this edge case. Aditionally, ensure that TSMP disco advert messages are not sent if the endpoint does not have a valid address yet. Fixes #19843 Updates #19597 Signed-off-by: Claus Lensbøl --- tstest/natlab/vmtest/vmtest_test.go | 97 ++++++++++++++++++++++++++++- wgengine/magicsock/endpoint.go | 9 ++- wgengine/magicsock/magicsock.go | 4 ++ wgengine/userspace.go | 3 + 4 files changed, 110 insertions(+), 3 deletions(-) diff --git a/tstest/natlab/vmtest/vmtest_test.go b/tstest/natlab/vmtest/vmtest_test.go index 98b0ad670..ec6bfd86b 100644 --- a/tstest/natlab/vmtest/vmtest_test.go +++ b/tstest/natlab/vmtest/vmtest_test.go @@ -1061,7 +1061,97 @@ func TestCachedNetmapAfterRestart(t *testing.T) { // WireGuard tunnel after one is restarted while the control server is // unreachable. After restart the node must use only its on-disk cached // netmaps to re-connect and ping the other (still online) node. +// +// The test has two modes, pinging from the offline node to the online node, +// and pinging from the online node to the offline node. func TestDirectConnectionWithCachedNetmapOnOneNode(t *testing.T) { + for _, testPingFrom := range []string{"offline", "online"} { + t.Run(fmt.Sprintf("ping_from_%s", testPingFrom), func(t *testing.T) { + env := vmtest.New(t) + + aNet := env.AddNetwork("1.0.0.1", "192.168.1.1/24", vnet.EasyNAT) + bNet := env.AddNetwork("2.0.0.1", "192.168.2.1/24", vnet.EasyNAT) + + // Node "a" is the offline peer, node "b" is the online peer. + a := env.AddNode("a", aNet, + vmtest.OS(vmtest.Gokrazy), + tailcfg.NodeCapMap{tailcfg.NodeAttrCacheNetworkMaps: nil}) + b := env.AddNode("b", bNet, + vmtest.OS(vmtest.Gokrazy), + tailcfg.NodeCapMap{tailcfg.NodeAttrCacheNetworkMaps: nil}) + + pStr := "Ping a → b" + if testPingFrom == "online" { + pStr = "Ping b → a" + } + checkInitialMetrics := env.AddStep("Check initial client metrics") + cutControlStep := env.AddStep("Cut control server access from a") + restartStep := env.AddStep("Restart tailscaled on a") + tsmpPingStep := env.AddStep(fmt.Sprintf("%s TSMP (cached netmap, no control)", pStr)) + discoPingStep := env.AddStep(fmt.Sprintf("%s Disco (want Direct)", pStr)) + checkFinalMetrics := env.AddStep("Check final client metrics") + + env.Start() + + // Before: Verify that we have not recorded any cached contacts. + checkInitialMetrics.Begin() + checkClientMetrics(t, "Node A", env.ClientMetrics(a), map[string]int64{ + "magicsock_cached_peer_contact_derp": 0, + "magicsock_cached_peer_contact_direct": 0, + }) + checkInitialMetrics.End(nil) + + cutControlStep.Begin() + a.DropControlTraffic() + env.ControlServer().SetOnMapRequest(func(nk key.NodePublic) { + if env.ControlServer().Node(nk).Name == a.Name() { + panic(fmt.Sprintf("got connection from %v", a.Name())) + } + }) + cutControlStep.End(nil) + + restartStep.Begin() + env.RestartTailscaled(a) + restartStep.End(nil) + + // Set the direction of the ping. + pFrom, pTo := a, b + if testPingFrom == "online" { + pFrom, pTo = b, a + } + + tsmpPingStep.Begin() + if err := env.Ping(pFrom, pTo, tailcfg.PingTSMP, 30*time.Second); err != nil { + tsmpPingStep.Fatal(err) + } + tsmpPingStep.End(nil) + + discoPingStep.Begin() + if err := env.PingExpect(pFrom, pTo, vmtest.PingRouteDirect, 90*time.Second); err != nil { + discoPingStep.Fatal(err) + } + // Ping back, mostly to give time for the metrics to be set. + if err := env.PingExpect(pTo, pFrom, vmtest.PingRouteDirect, 30*time.Second); err != nil { + discoPingStep.Fatal(err) + } + discoPingStep.End(nil) + + // After: Verify that we recorded a direct contact on the disconnected node. + checkFinalMetrics.Begin() + checkClientMetrics(t, "Node A", env.ClientMetrics(a), map[string]int64{ + "magicsock_cached_peer_contact_direct": 1, + }) + checkFinalMetrics.End(nil) + }) + } +} + +// TestDirectWithCachedNetmapOnTwoNodes verifies that two nodes with netmap +// caching enabled (NodeAttrCacheNetworkMaps) can re-establish a direct +// WireGuard tunnel after both restarted while the control server is +// unreachable. After restart one node must use only its on-disk cached +// netmaps to re-connect and ping the other node. +func TestDirectConnectionWithCachedNetmapOnTwoNodes(t *testing.T) { env := vmtest.New(t) aNet := env.AddNetwork("1.0.0.1", "192.168.1.1/24", vnet.EasyNAT) @@ -1093,15 +1183,18 @@ func TestDirectConnectionWithCachedNetmapOnOneNode(t *testing.T) { cutControlStep.Begin() a.DropControlTraffic() + b.DropControlTraffic() env.ControlServer().SetOnMapRequest(func(nk key.NodePublic) { - if env.ControlServer().Node(nk).Name == a.Name() { - panic(fmt.Sprintf("got connection from %v", a.Name())) + nodeName := env.ControlServer().Node(nk).Name + if nodeName == a.Name() || nodeName == b.Name() { + panic(fmt.Sprintf("got connection from %v", nodeName)) } }) cutControlStep.End(nil) restartStep.Begin() env.RestartTailscaled(a) + env.RestartTailscaled(b) restartStep.End(nil) tsmpPingStep.Begin() diff --git a/wgengine/magicsock/endpoint.go b/wgengine/magicsock/endpoint.go index d831a9032..d1430ca4e 100644 --- a/wgengine/magicsock/endpoint.go +++ b/wgengine/magicsock/endpoint.go @@ -1381,12 +1381,19 @@ func (de *endpoint) sendDiscoPingsLocked(now mono.Time, sendCallMeMaybe bool) { de.startDiscoPingLocked(epAddr{ap: ep}, now, pingDiscovery, 0, nil) } derpAddr := de.derpAddr - if sentAny && sendCallMeMaybe && derpAddr.IsValid() { + if sendCallMeMaybe && derpAddr.IsValid() && (sentAny || de.c.usingCachedNetmap.Load()) { // Have our magicsock.Conn figure out its STUN endpoint (if // it doesn't know already) and then send a CallMeMaybe // message to our peer via DERP informing them that we've // sent so our firewall ports are probably open and now // would be a good time for them to connect. + // + // When working off of a cached netmap, send out a CallMeMaybe + // even if we don't know about any peer endpoints. + // Since we cannot rely on control to transfer endpoints for us, + // this makes establishing direct connections more reliable + // as the peer will respond with its own message and initiate + // the connection. go de.c.enqueueCallMeMaybe(derpAddr, de) } } diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index 442e26579..8de8e85f6 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -4444,6 +4444,10 @@ func (c *Conn) maybeSendTSMPDiscoAdvert(de *endpoint) { de.mu.Lock() defer de.mu.Unlock() + if !de.nodeAddr.IsValid() { + return + } + now := mono.Now() if now.Sub(de.lastDiscoKeyAdvertisement) <= discoKeyAdvertisementInterval || (!de.lastDiscoKeyAdvertisement.IsZero() && de.bestAddr.isDirect()) { diff --git a/wgengine/userspace.go b/wgengine/userspace.go index 222df1bc8..bf6d97ddd 100644 --- a/wgengine/userspace.go +++ b/wgengine/userspace.go @@ -614,6 +614,9 @@ func NewUserspaceEngine(logf logger.Logf, conf Config) (_ Engine, reterr error) }) var tsmpRequestGroup singleflight.Group[netip.Addr, struct{}] eventbus.SubscribeFunc(ec, func(req magicsock.NewDiscoKeyAvailable) { + if !req.NodeFirstAddr.IsValid() { + return + } go tsmpRequestGroup.Do(req.NodeFirstAddr, func() (struct{}, error) { e.sendTSMPDiscoAdvertisement(req.NodeFirstAddr) e.logf("wgengine: sending TSMP disco key advertisement to %v", req.NodeFirstAddr)