From f6dc47efe4e925e433229f3f9c3f31fe4c367492 Mon Sep 17 00:00:00 2001
From: Brad Fitzpatrick <bradfitz@tailscale.com>
Date: Mon, 17 Aug 2020 12:56:17 -0700
Subject: [PATCH] tailcfg, controlclient, magicsock: add control feature flag
 to enable DRPO

Updates #150
---
 control/controlclient/direct.go | 21 +++++++++++++++++++--
 tailcfg/tailcfg.go              |  7 +++++++
 wgengine/magicsock/magicsock.go | 18 ++++++++++++++++--
 3 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/control/controlclient/direct.go b/control/controlclient/direct.go
index a5b5ca203..a7d399daf 100644
--- a/control/controlclient/direct.go
+++ b/control/controlclient/direct.go
@@ -26,6 +26,7 @@ import (
 	"strconv"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"time"
 
 	"github.com/tailscale/wireguard-go/wgcfg"
@@ -37,6 +38,7 @@ import (
 	"tailscale.com/net/tlsdial"
 	"tailscale.com/tailcfg"
 	"tailscale.com/types/logger"
+	"tailscale.com/types/opt"
 	"tailscale.com/types/structs"
 	"tailscale.com/version"
 )
@@ -621,8 +623,14 @@ func (c *Direct) PollNetMap(ctx context.Context, maxPolls int, cb func(*NetworkM
 			vlogf("netmap: new map contains DERP map")
 			lastDERPMap = resp.DERPMap
 		}
-		if resp.Debug != nil && resp.Debug.LogHeapPprof {
-			go logheap.LogHeap(resp.Debug.LogHeapURL)
+		if resp.Debug != nil {
+			if resp.Debug.LogHeapPprof {
+				go logheap.LogHeap(resp.Debug.LogHeapURL)
+			}
+			newv := resp.Debug.DERPRoute
+			if old, ok := controlUseDERPRoute.Load().(opt.Bool); !ok || old != newv {
+				controlUseDERPRoute.Store(newv)
+			}
 		}
 		// Temporarily (2020-06-29) support removing all but
 		// discovery-supporting nodes during development, for
@@ -953,3 +961,12 @@ func cloneNodes(v1 []*tailcfg.Node) []*tailcfg.Node {
 	}
 	return v2
 }
+
+var controlUseDERPRoute atomic.Value
+
+// DERPRouteFlag reports the last reported value from control for whether
+// DERP route optimization (Issue 150) should be enabled.
+func DERPRouteFlag() opt.Bool {
+	v, _ := controlUseDERPRoute.Load().(opt.Bool)
+	return v
+}
diff --git a/tailcfg/tailcfg.go b/tailcfg/tailcfg.go
index 0d073a17d..21a6eb4e6 100644
--- a/tailcfg/tailcfg.go
+++ b/tailcfg/tailcfg.go
@@ -563,6 +563,13 @@ type Debug struct {
 	// always do its background STUN queries (see magicsock's
 	// periodicReSTUN), regardless of inactivity.
 	ForceBackgroundSTUN bool `json:",omitempty"`
+
+	// DERPRoute controls whether the DERP reverse path
+	// optimization (see Issue 150) should be enabled or
+	// disabled. The environment variable in magicsock is the
+	// highest priority (if set), then this (if set), then the
+	// binary default value.
+	DERPRoute opt.Bool `json:",omitempty"`
 }
 
 func (k MachineKey) String() string                   { return fmt.Sprintf("mkey:%x", k[:]) }
diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go
index 67b2ae9b2..d20389a10 100644
--- a/wgengine/magicsock/magicsock.go
+++ b/wgengine/magicsock/magicsock.go
@@ -70,7 +70,8 @@ var (
 	// debugUseDerpRoute temporarily (2020-03-22) controls whether DERP
 	// reverse routing is enabled (Issue 150). It will become always true
 	// later.
-	debugUseDerpRoute, _ = strconv.ParseBool(os.Getenv("TS_DEBUG_ENABLE_DERP_ROUTE"))
+	debugUseDerpRouteEnv = os.Getenv("TS_DEBUG_ENABLE_DERP_ROUTE")
+	debugUseDerpRoute, _ = strconv.ParseBool(debugUseDerpRouteEnv)
 	// logDerpVerbose logs all received DERP packets, including their
 	// full payload.
 	logDerpVerbose, _ = strconv.ParseBool(os.Getenv("TS_DEBUG_DERP"))
@@ -81,6 +82,19 @@ var (
 	debugReSTUNStopOnIdle, _ = strconv.ParseBool(os.Getenv("TS_DEBUG_RESTUN_STOP_ON_IDLE"))
 )
 
+// useDerpRoute reports whether magicsock should enable the DERP
+// return path optimization (Issue 150).
+func useDerpRoute() bool {
+	if debugUseDerpRouteEnv != "" {
+		return debugUseDerpRoute
+	}
+	ob := controlclient.DERPRouteFlag()
+	if v, ok := ob.Get(); ok {
+		return v
+	}
+	return false
+}
+
 // inTest reports whether the running program is a test that set the
 // IN_TS_TEST environment variable.
 //
@@ -1111,7 +1125,7 @@ func (c *Conn) derpWriteChanOfAddr(addr netaddr.IPPort, peer key.Public) chan<-
 	// perhaps peer's home is Frankfurt, but they dialed our home DERP
 	// node in SF to reach us, so we can reply to them using our
 	// SF connection rather than dialing Frankfurt. (Issue 150)
-	if !peer.IsZero() && debugUseDerpRoute {
+	if !peer.IsZero() && useDerpRoute() {
 		if r, ok := c.derpRoute[peer]; ok {
 			if ad, ok := c.activeDerp[r.derpID]; ok && ad.c == r.dc {
 				c.setPeerLastDerpLocked(peer, r.derpID, regionID)