wgengine/{,magicsock},tstest/natlab/vmtest: send disco on cached netmap (#19878)

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 <claus@tailscale.com>

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()

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)
 	}
 }

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()) {

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)