From 67926ede39f0cb740e6fa55ae75bce07cfeb1f1f Mon Sep 17 00:00:00 2001
From: Val <valerie@tailscale.com>
Date: Wed, 20 Sep 2023 14:44:10 +0200
Subject: [PATCH] wgengine/magicsock: add MTU to addrLatency and rename to
 addrQuality

Add a field to record the wire MTU of the path to this address to the
addrLatency struct and rename it addrQuality.

Updates #311

Signed-off-by: Val <valerie@tailscale.com>
---
 wgengine/magicsock/endpoint.go       | 27 ++++++++++++++++++---------
 wgengine/magicsock/magicsock_test.go | 18 +++++++++++++-----
 2 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/wgengine/magicsock/endpoint.go b/wgengine/magicsock/endpoint.go
index ccfa4bfae..6268500ec 100644
--- a/wgengine/magicsock/endpoint.go
+++ b/wgengine/magicsock/endpoint.go
@@ -65,7 +65,7 @@ type endpoint struct {
 	lastFullPing   mono.Time      // last time we pinged all disco or wireguard only endpoints
 	derpAddr       netip.AddrPort // fallback/bootstrap path, if non-zero (non-zero for well-behaved clients)
 
-	bestAddr           addrLatency // best non-DERP path; zero if none
+	bestAddr           addrQuality // best non-DERP path; zero if none
 	bestAddrAt         mono.Time   // time best address re-confirmed
 	trustBestAddrUntil mono.Time   // time when bestAddr expires
 	sentPing           map[stun.TxID]sentPing
@@ -207,7 +207,7 @@ func (de *endpoint) deleteEndpointLocked(why string, ep netip.AddrPort) {
 			What: "deleteEndpointLocked-bestAddr-" + why,
 			From: de.bestAddr,
 		})
-		de.bestAddr = addrLatency{}
+		de.bestAddr = addrQuality{}
 	}
 }
 
@@ -906,7 +906,7 @@ func (de *endpoint) addCandidateEndpoint(ep netip.AddrPort, forRxPingTxID stun.T
 //
 // de.mu must be held.
 func (de *endpoint) clearBestAddrLocked() {
-	de.bestAddr = addrLatency{}
+	de.bestAddr = addrQuality{}
 	de.bestAddrAt = 0
 	de.trustBestAddrUntil = 0
 }
@@ -996,7 +996,7 @@ func (de *endpoint) handlePongConnLocked(m *disco.Pong, di *discoInfo, src netip
 	// Promote this pong response to our current best address if it's lower latency.
 	// TODO(bradfitz): decide how latency vs. preference order affects decision
 	if !isDerp {
-		thisPong := addrLatency{sp.to, latency}
+		thisPong := addrQuality{sp.to, latency, 0}
 		if betterAddr(thisPong, de.bestAddr) {
 			de.c.logf("magicsock: disco: node %v %v now using %v", de.publicKey.ShortString(), de.discoShort(), sp.to)
 			de.debugUpdates.Add(EndpointChange{
@@ -1022,19 +1022,28 @@ func (de *endpoint) handlePongConnLocked(m *disco.Pong, di *discoInfo, src netip
 	return
 }
 
-// addrLatency is an IPPort with an associated latency.
-type addrLatency struct {
+// addrQuality is an IPPort with an associated latency and path mtu.
+type addrQuality struct {
 	netip.AddrPort
 	latency time.Duration
+	wireMTU tstun.WireMTU
 }
 
-func (a addrLatency) String() string {
-	return a.AddrPort.String() + "@" + a.latency.String()
+func (a addrQuality) String() string {
+	return fmt.Sprintf("%v@%v+%v", a.AddrPort, a.latency, a.wireMTU)
 }
 
 // betterAddr reports whether a is a better addr to use than b.
-func betterAddr(a, b addrLatency) bool {
+func betterAddr(a, b addrQuality) bool {
 	if a.AddrPort == b.AddrPort {
+		if a.wireMTU > b.wireMTU {
+			// TODO(val): Think harder about the case of lower
+			// latency and smaller or unknown MTU, and higher
+			// latency but larger MTU. Probably in most cases the
+			// largest MTU will also be the lowest latency but we
+			// can't depend on that.
+			return true
+		}
 		return false
 	}
 	if !b.IsValid() {
diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go
index de22882df..eaeb28c01 100644
--- a/wgengine/magicsock/magicsock_test.go
+++ b/wgengine/magicsock/magicsock_test.go
@@ -1648,10 +1648,13 @@ func TestEndpointSetsEqual(t *testing.T) {
 
 func TestBetterAddr(t *testing.T) {
 	const ms = time.Millisecond
-	al := func(ipps string, d time.Duration) addrLatency {
-		return addrLatency{netip.MustParseAddrPort(ipps), d}
+	al := func(ipps string, d time.Duration) addrQuality {
+		return addrQuality{AddrPort: netip.MustParseAddrPort(ipps), latency: d}
 	}
-	zero := addrLatency{}
+	almtu := func(ipps string, d time.Duration, mtu tstun.WireMTU) addrQuality {
+		return addrQuality{AddrPort: netip.MustParseAddrPort(ipps), latency: d, wireMTU: mtu}
+	}
+	zero := addrQuality{}
 
 	const (
 		publicV4   = "1.2.3.4:555"
@@ -1662,7 +1665,7 @@ func TestBetterAddr(t *testing.T) {
 	)
 
 	tests := []struct {
-		a, b addrLatency
+		a, b addrQuality
 		want bool // whether a is better than b
 	}{
 		{a: zero, b: zero, want: false},
@@ -1724,7 +1727,12 @@ func TestBetterAddr(t *testing.T) {
 			b:    al(publicV6, 100*ms),
 			want: true,
 		},
-
+		// If addresses are equal, prefer larger MTU
+		{
+			a:    almtu(publicV4, 30*ms, 1500),
+			b:    almtu(publicV4, 30*ms, 0),
+			want: true,
+		},
 		// Private IPs are preferred over public IPs even if the public
 		// IP is IPv6.
 		{