wgengine/magicsock: suppress TSMP disco advert when bestAddr is peer relay

Updates #20156

Signed-off-by: Jordan Whited <jordan@tailscale.com>

wgengine/magicsock/endpoint.go

@@ -1841,6 +1841,8 @@ type addrQuality struct {
 	wireMTU          tstun.WireMTU
 }
 
+func (a addrQuality) isZero() bool { return a == addrQuality{} }
+
 func (a addrQuality) String() string {
 	// TODO(jwhited): consider including relayServerDisco
 	return fmt.Sprintf("%v@%v+%v", a.epAddr, a.latency, a.wireMTU)

wgengine/magicsock/magicsock.go

@@ -4527,9 +4527,9 @@ type NewDiscoKeyAvailable struct {
 // maybeSendTSMPDiscoAdvert conditionally emits an event indicating that we
 // should send our DiscoKey to the first node address of the magicksock endpoint.
 //
-// The event is suppressed if we are already communicating directly or
+// The event is suppressed if we are communicating over a non-DERP path, or
-// less than 60 seconds has passed since the last DiscoKey was sent, or netmap
+// less than [discoKeyAdvertisementInterval] has passed since the last DiscoKey
-// caching is disabled on this node.
+// was sent, or netmap caching is disabled on this node.
 //
 // We do not need the Conn to be locked, but the endpoint should be.
 func (c *Conn) maybeSendTSMPDiscoAdvert(de *endpoint) {
@@ -4557,7 +4557,7 @@ func (c *Conn) maybeSendTSMPDiscoAdvert(de *endpoint) {
 
 	now := mono.Now()
 	if now.Sub(de.lastDiscoKeyAdvertisement) <= discoKeyAdvertisementInterval ||
-		(!de.lastDiscoKeyAdvertisement.IsZero() && de.bestAddr.isDirect()) {
+		(!de.lastDiscoKeyAdvertisement.IsZero() && !de.bestAddr.isZero()) {
 		return
 	}
 

wgengine/magicsock/magicsock_test.go

@@ -4620,3 +4620,62 @@ func TestSendingTSMPDiscoCachingDisabled(t *testing.T) {
 		})
 	}
 }
+
+// TestSendingTSMPDiscoPeerRelaySuppressed verifies that maybeSendTSMPDiscoAdvert
+// suppresses the advert when the bestAddr is a peer relay path (a non-zero
+// addrQuality whose epAddr has a VNI set), even though such a path is not
+// direct. Suppression is observed via lastDiscoKeyAdvertisement remaining
+// unchanged, since a fired advert would overwrite it with the current time.
+func TestSendingTSMPDiscoPeerRelaySuppressed(t *testing.T) {
+	conn := newTestConn(t)
+	t.Cleanup(func() { conn.Close() })
+
+	// maybeSendTSMPDiscoAdvert only advertises when netmap caching is enabled.
+	conn.controlKnobs = new(controlknobs.Knobs)
+	conn.controlKnobs.CacheNetworkMaps.Store(true)
+
+	peerKey := key.NewNode().Public()
+	ep := &endpoint{
+		nodeID:    1,
+		publicKey: peerKey,
+		nodeAddr:  netip.MustParseAddr("100.64.0.1"),
+	}
+	discoKey := key.NewDisco().Public()
+	ep.disco.Store(&endpointDisco{
+		key:   discoKey,
+		short: discoKey.ShortString(),
+	})
+	ep.c = conn
+	conn.mu.Lock()
+	nodeView := (&tailcfg.Node{
+		Key: ep.publicKey,
+		Addresses: []netip.Prefix{
+			netip.MustParsePrefix("100.64.0.1/32"),
+		},
+	}).View()
+	conn.peersByID = map[tailcfg.NodeID]tailcfg.NodeView{nodeView.ID(): nodeView}
+	conn.mu.Unlock()
+
+	conn.peerMap.upsertEndpoint(ep, key.DiscoPublic{})
+
+	// A peer relay bestAddr: an epAddr with a VNI set. It is past the
+	// rate-limit interval with a non-zero lastDiscoKeyAdvertisement, so the
+	// only thing suppressing the advert is the active (non-zero) bestAddr.
+	var vni packet.VirtualNetworkID
+	vni.Set(7)
+	lastAdvert := mono.Now().Add(-discoKeyAdvertisementInterval - time.Second)
+	ep.mu.Lock()
+	ep.lastDiscoKeyAdvertisement = lastAdvert
+	ep.bestAddr = addrQuality{epAddr: epAddr{ap: netip.MustParseAddrPort("1.2.3.4:567"), vni: vni}}
+	ep.mu.Unlock()
+
+	conn.maybeSendTSMPDiscoAdvert(ep)
+
+	// A fired advert would have overwritten lastDiscoKeyAdvertisement with the
+	// current time; confirm it was left untouched, indicating suppression.
+	ep.mu.Lock()
+	defer ep.mu.Unlock()
+	if ep.lastDiscoKeyAdvertisement != lastAdvert {
+		t.Errorf("lastDiscoKeyAdvertisement = %v; want unchanged %v (advert should have been suppressed)", ep.lastDiscoKeyAdvertisement, lastAdvert)
+	}
+}