wgengine/magicsock: do not send TSMP disco when connected (#19497)

When there is an active connection between devices, do not send new
disco keys via TSMP.

Updates #12639

Signed-off-by: Claus Lensbøl <claus@tailscale.com>

wgengine/magicsock/magicsock.go

@@ -4430,11 +4430,8 @@ 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 only emitted if we have not yet contacted that endpoint since
+// The event is only emitted if we are not already communicating directly and
-// the DiscoKey changed.
+// more than 60 seconds has passed since the last DiscoKey was sent.
-//
-// This condition is most likely met only once per endpoint, after the start of
-// tailscaled, but not until we contact the endpoint for the first time.
 //
 // We do not need the Conn to be locked, but the endpoint should be.
 func (c *Conn) maybeSendTSMPDiscoAdvert(de *endpoint) {
@@ -4444,11 +4441,16 @@ func (c *Conn) maybeSendTSMPDiscoAdvert(de *endpoint) {
 
 	de.mu.Lock()
 	defer de.mu.Unlock()
-	if mono.Now().Sub(de.lastDiscoKeyAdvertisement) > discoKeyAdvertisementInterval {
+
-		de.lastDiscoKeyAdvertisement = mono.Now()
+	now := mono.Now()
-		c.tsmpDiscoKeyAvailablePub.Publish(NewDiscoKeyAvailable{
+	if now.Sub(de.lastDiscoKeyAdvertisement) <= discoKeyAdvertisementInterval ||
-			NodeFirstAddr: de.nodeAddr,
+		(!de.lastDiscoKeyAdvertisement.IsZero() && de.bestAddr.isDirect()) {
-			NodeID:        de.nodeID,
+		return
-		})
 	}
+
+	de.lastDiscoKeyAdvertisement = now
+	c.tsmpDiscoKeyAvailablePub.Publish(NewDiscoKeyAvailable{
+		NodeFirstAddr: de.nodeAddr,
+		NodeID:        de.nodeID,
+	})
 }

wgengine/magicsock/magicsock_test.go

@@ -4443,7 +4443,6 @@ func TestReceiveTSMPDiscoKeyAdvertisement(t *testing.T) {
 }
 
 func TestSendingTSMPDiscoTimer(t *testing.T) {
-	t.Setenv("TS_USE_CACHED_NETMAP", "1")
 	conn := newTestConn(t)
 	tw := eventbustest.NewWatcher(t, conn.eventBus)
 	t.Cleanup(func() { conn.Close() })
@@ -4476,12 +4475,36 @@ func TestSendingTSMPDiscoTimer(t *testing.T) {
 		t.Errorf("Original disco key %s, does not match %s", discoKey.ShortString(), ep.discoShort())
 	}
 
+	// Only one gets through, second is rate limited.
 	conn.maybeSendTSMPDiscoAdvert(ep)
 	conn.maybeSendTSMPDiscoAdvert(ep)
-	eventbustest.ExpectExactly(tw, eventbustest.Type[NewDiscoKeyAvailable]())
+	if err := eventbustest.ExpectExactly(tw, eventbustest.Type[NewDiscoKeyAvailable]()); err != nil {
+		t.Errorf("expected only one event, got: %s", err)
+	}
+
+	// Reset to get the event firing again.
 	ep.mu.Lock()
 	ep.lastDiscoKeyAdvertisement = 0
 	ep.mu.Unlock()
 	conn.maybeSendTSMPDiscoAdvert(ep)
-	eventbustest.Expect(tw, eventbustest.Type[NewDiscoKeyAvailable]())
+	if err := eventbustest.Expect(tw, eventbustest.Type[NewDiscoKeyAvailable]()); err != nil {
+		t.Errorf("expected only one event, got: %s", err)
+	}
+
+	// With a direct bestAddr and a non-zero lastDiscoKeyAdvertisement past the
+	// rate-limit interval. No advert should be sent due to the active bestAddr.
+	ep.mu.Lock()
+	ep.lastDiscoKeyAdvertisement = mono.Now().Add(-discoKeyAdvertisementInterval - time.Second)
+	ep.bestAddr = addrQuality{epAddr: epAddr{ap: netip.MustParseAddrPort("1.2.3.4:567")}}
+	ep.mu.Unlock()
+	conn.maybeSendTSMPDiscoAdvert(ep)
+
+	// Simulating restart should send an advert.
+	ep.mu.Lock()
+	ep.lastDiscoKeyAdvertisement = 0
+	ep.mu.Unlock()
+	conn.maybeSendTSMPDiscoAdvert(ep)
+	if err := eventbustest.ExpectExactly(tw, eventbustest.Type[NewDiscoKeyAvailable]()); err != nil {
+		t.Errorf("expected only one event, got: %s", err)
+	}
 }