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)