From afec2d41b4f54158897b8dccf49dfd19eb8cbf10 Mon Sep 17 00:00:00 2001
From: Jordan Whited <jordan@tailscale.com>
Date: Fri, 13 Sep 2024 10:51:30 -0700
Subject: [PATCH] wgengine/magicsock: remove redundant deadline from netcheck
 report call (#13395)

netcheck.Client.GetReport() applies its own deadlines. This 2s deadline
was causing GetReport() to never fall back to HTTPS/ICMP measurements
as it was shorter than netcheck.stunProbeTimeout, leaving no time
for fallbacks.

Updates #13394
Updates #6187

Signed-off-by: Jordan Whited <jordan@tailscale.com>
---
 net/netcheck/netcheck.go        | 17 +++++++++++++----
 net/netcheck/netcheck_test.go   | 12 ++++++++++++
 wgengine/magicsock/magicsock.go |  3 ---
 3 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/net/netcheck/netcheck.go b/net/netcheck/netcheck.go
index 8eb50a61d..3dc160f90 100644
--- a/net/netcheck/netcheck.go
+++ b/net/netcheck/netcheck.go
@@ -52,9 +52,9 @@
 
 // The various default timeouts for things.
 const (
-	// overallProbeTimeout is the maximum amount of time netcheck will
+	// ReportTimeout is the maximum amount of time netcheck will
 	// spend gathering a single report.
-	overallProbeTimeout = 5 * time.Second
+	ReportTimeout = 5 * time.Second
 	// stunTimeout is the maximum amount of time netcheck will spend
 	// probing with STUN packets without getting a reply before
 	// switching to HTTP probing, on the assumption that outbound UDP
@@ -63,6 +63,11 @@
 	// icmpProbeTimeout is the maximum amount of time netcheck will spend
 	// probing with ICMP packets.
 	icmpProbeTimeout = 1 * time.Second
+	// httpsProbeTimeout is the maximum amount of time netcheck will spend
+	// probing over HTTPS. This is set equal to ReportTimeout to allow HTTPS
+	// whatever time is left following STUN, which precedes it in a netcheck
+	// report.
+	httpsProbeTimeout = ReportTimeout
 	// defaultActiveRetransmitTime is the retransmit interval we use
 	// for STUN probes when we're in steady state (not in start-up),
 	// but don't have previous latency information for a DERP
@@ -731,6 +736,10 @@ func (o *GetReportOpts) getLastDERPActivity(region int) time.Time {
 }
 
 // GetReport gets a report. The 'opts' argument is optional and can be nil.
+// Callers are discouraged from passing a ctx with an arbitrary deadline as this
+// may cause GetReport to return prematurely before all reporting methods have
+// executed. ReportTimeout is the maximum amount of time GetReport will spend
+// gathering a report.
 //
 // It may not be called concurrently with itself.
 func (c *Client) GetReport(ctx context.Context, dm *tailcfg.DERPMap, opts *GetReportOpts) (_ *Report, reterr error) {
@@ -743,7 +752,7 @@ func (c *Client) GetReport(ctx context.Context, dm *tailcfg.DERPMap, opts *GetRe
 	// Mask user context with ours that we guarantee to cancel so
 	// we can depend on it being closed in goroutines later.
 	// (User ctx might be context.Background, etc)
-	ctx, cancel := context.WithTimeout(ctx, overallProbeTimeout)
+	ctx, cancel := context.WithTimeout(ctx, ReportTimeout)
 	defer cancel()
 
 	ctx = sockstats.WithSockStats(ctx, sockstats.LabelNetcheckClient, c.logf)
@@ -1044,7 +1053,7 @@ func (c *Client) runHTTPOnlyChecks(ctx context.Context, last *Report, rs *report
 func (c *Client) measureHTTPSLatency(ctx context.Context, reg *tailcfg.DERPRegion) (time.Duration, netip.Addr, error) {
 	metricHTTPSend.Add(1)
 	var result httpstat.Result
-	ctx, cancel := context.WithTimeout(httpstat.WithHTTPStat(ctx, &result), overallProbeTimeout)
+	ctx, cancel := context.WithTimeout(httpstat.WithHTTPStat(ctx, &result), httpsProbeTimeout)
 	defer cancel()
 
 	var ip netip.Addr
diff --git a/net/netcheck/netcheck_test.go b/net/netcheck/netcheck_test.go
index 2f1870576..02076f8d4 100644
--- a/net/netcheck/netcheck_test.go
+++ b/net/netcheck/netcheck_test.go
@@ -860,3 +860,15 @@ func TestNodeAddrResolve(t *testing.T) {
 		})
 	}
 }
+
+func TestReportTimeouts(t *testing.T) {
+	if ReportTimeout < stunProbeTimeout {
+		t.Errorf("ReportTimeout (%v) cannot be less than stunProbeTimeout (%v)", ReportTimeout, stunProbeTimeout)
+	}
+	if ReportTimeout < icmpProbeTimeout {
+		t.Errorf("ReportTimeout (%v) cannot be less than icmpProbeTimeout (%v)", ReportTimeout, icmpProbeTimeout)
+	}
+	if ReportTimeout < httpsProbeTimeout {
+		t.Errorf("ReportTimeout (%v) cannot be less than httpsProbeTimeout (%v)", ReportTimeout, httpsProbeTimeout)
+	}
+}
diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go
index de6b13fc1..c9f032371 100644
--- a/wgengine/magicsock/magicsock.go
+++ b/wgengine/magicsock/magicsock.go
@@ -688,9 +688,6 @@ func (c *Conn) updateNetInfo(ctx context.Context) (*netcheck.Report, error) {
 		return new(netcheck.Report), nil
 	}
 
-	ctx, cancel := context.WithTimeout(ctx, 2*time.Second)
-	defer cancel()
-
 	report, err := c.netChecker.GetReport(ctx, dm, &netcheck.GetReportOpts{
 		// Pass information about the last time that we received a
 		// frame from a DERP server to our netchecker to help avoid