From c821c9c9966dd8a58f0fb03aca30743f151e57de Mon Sep 17 00:00:00 2001
From: Dean Sheather <dean@deansheather.com>
Date: Thu, 21 Sep 2023 11:37:00 -0700
Subject: [PATCH] chore: avoid logging netcheck send errors if STUN is disabled
 (#40)

---
 tailcfg/derpmap.go              |  12 ++++
 tailcfg/tailcfg_test.go         | 118 ++++++++++++++++++++++++++++++++
 wgengine/magicsock/magicsock.go |   9 ++-
 3 files changed, 138 insertions(+), 1 deletion(-)

diff --git a/tailcfg/derpmap.go b/tailcfg/derpmap.go
index 583379c210318..d7daed18b2294 100644
--- a/tailcfg/derpmap.go
+++ b/tailcfg/derpmap.go
@@ -28,6 +28,18 @@ type DERPMap struct {
 	OmitDefaultRegions bool `json:"omitDefaultRegions,omitempty"`
 }
 
+// HasSTUN returns true if there are any STUN servers in the DERPMap.
+func (m *DERPMap) HasSTUN() bool {
+	for _, r := range m.Regions {
+		for _, n := range r.Nodes {
+			if n.STUNPort >= 0 {
+				return true
+			}
+		}
+	}
+	return false
+}
+
 // / RegionIDs returns the sorted region IDs.
 func (m *DERPMap) RegionIDs() []int {
 	ret := make([]int, 0, len(m.Regions))
diff --git a/tailcfg/tailcfg_test.go b/tailcfg/tailcfg_test.go
index 2db6cb864b1e2..e38faee9f8edf 100644
--- a/tailcfg/tailcfg_test.go
+++ b/tailcfg/tailcfg_test.go
@@ -729,3 +729,121 @@ func TestCurrentCapabilityVersion(t *testing.T) {
 		t.Errorf("CurrentCapabilityVersion = %d; want %d", CurrentCapabilityVersion, max)
 	}
 }
+
+func TestDERPMapHasSTUN(t *testing.T) {
+	cases := []struct {
+		name    string
+		derpMap *DERPMap
+		want    bool
+	}{
+		{
+			name: "None",
+			derpMap: &DERPMap{
+				Regions: map[int]*DERPRegion{
+					1: {
+						RegionID: 1,
+						Nodes: []*DERPNode{
+							{
+								RegionID: 1,
+								Name:     "1a",
+								STUNPort: -1,
+							},
+						},
+					},
+					2: {
+						RegionID: 2,
+						Nodes: []*DERPNode{
+							{
+								RegionID: 2,
+								Name:     "2a",
+								STUNPort: -1,
+							},
+						},
+					},
+				},
+			},
+			want: false,
+		},
+		{
+			name: "One",
+			derpMap: &DERPMap{
+				Regions: map[int]*DERPRegion{
+					1: {
+						RegionID: 1,
+						Nodes: []*DERPNode{
+							{
+								RegionID: 1,
+								Name:     "1a",
+								STUNPort: -1,
+							},
+						},
+					},
+					2: {
+						RegionID: 2,
+						Nodes: []*DERPNode{
+							{
+								RegionID: 2,
+								Name:     "2a",
+								STUNPort: -1,
+							},
+							{
+								RegionID: 2,
+								Name:     "2b",
+								STUNPort: 0, // default
+							},
+						},
+					},
+				},
+			},
+			want: true,
+		},
+		{
+			name: "Some",
+			derpMap: &DERPMap{
+				Regions: map[int]*DERPRegion{
+					1: {
+						RegionID: 1,
+						Nodes: []*DERPNode{
+							{
+								RegionID: 1,
+								Name:     "1a",
+								STUNPort: -1,
+							},
+							{
+								RegionID: 1,
+								Name:     "1b",
+								STUNPort: 1234,
+							},
+						},
+					},
+					2: {
+						RegionID: 2,
+						Nodes: []*DERPNode{
+							{
+								RegionID: 2,
+								Name:     "2a",
+								STUNPort: -1,
+							},
+							{
+								RegionID: 2,
+								Name:     "2b",
+								STUNPort: 4321,
+							},
+						},
+					},
+				},
+			},
+			want: true,
+		},
+	}
+
+	for _, c := range cases {
+		c := c
+		t.Run(c.name, func(t *testing.T) {
+			got := c.derpMap.HasSTUN()
+			if got != c.want {
+				t.Errorf("got %v; want %v", got, c.want)
+			}
+		})
+	}
+}
diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go
index 0d46a0424bb65..b2971b290a27d 100644
--- a/wgengine/magicsock/magicsock.go
+++ b/wgengine/magicsock/magicsock.go
@@ -536,7 +536,7 @@ func (c *Conn) updateEndpoints(why string) {
 		c.muCond.Broadcast()
 	}()
 	c.dlogf("[v1] magicsock: starting endpoint update (%s)", why)
-	if c.noV4Send.Load() && runtime.GOOS != "js" {
+	if c.noV4Send.Load() && c.derpMapHasSTUNNodes() && runtime.GOOS != "js" {
 		c.mu.Lock()
 		closed := c.closed
 		c.mu.Unlock()
@@ -560,6 +560,13 @@ func (c *Conn) updateEndpoints(why string) {
 	}
 }
 
+// c.mu must NOT be held.
+func (c *Conn) derpMapHasSTUNNodes() bool {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+	return c.derpMap != nil && c.derpMap.HasSTUN()
+}
+
 // setEndpoints records the new endpoints, reporting whether they're changed.
 // It takes ownership of the slice.
 func (c *Conn) setEndpoints(endpoints []tailcfg.Endpoint) (changed bool) {