From 82cf5591dc1198eef30a50a9a19fa5776b3190db Mon Sep 17 00:00:00 2001
From: Alex Chan <alexc@tailscale.com>
Date: Mon, 15 Sep 2025 06:53:41 +0100
Subject: [PATCH] wgengine/magicsock: send a valid payload in
 TestNetworkDownSendErrors

This test ostensibly checks whether we record an error metric if a packet
is dropped because the network is down, but the network connectivity is
irrelevant -- the send error is actually because the arguments to Send()
are invalid:

    RebindingUDPConn.WriteWireGuardBatchTo:
    [unexpected] offset (0) != Geneve header length (8)

This patch changes the test so we try to send a valid packet, and we
verify this by sending it once before taking the network down.  The new
error is:

    magicsock: network down

which is what we're trying to test.

We then test sending an invalid payload as a separate test case.

Updates tailscale/corp#22075

Signed-off-by: Alex Chan <alexc@tailscale.com>
---
 wgengine/magicsock/magicsock_test.go | 75 +++++++++++++++++++++++-----
 1 file changed, 63 insertions(+), 12 deletions(-)

diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go
index 5774432d5..5b27e7a1b 100644
--- a/wgengine/magicsock/magicsock_test.go
+++ b/wgengine/magicsock/magicsock_test.go
@@ -3131,32 +3131,83 @@ func TestMaybeRebindOnError(t *testing.T) {
 	})
 }
 
-func TestNetworkDownSendErrors(t *testing.T) {
+func TestNetworkSendErrors(t *testing.T) {
 	bus := eventbus.New()
 	defer bus.Close()
 
 	netMon := must.Get(netmon.New(bus, t.Logf))
 	defer netMon.Close()
 
-	reg := new(usermetric.Registry)
 	conn := must.Get(NewConn(Options{
 		DisablePortMapper: true,
 		Logf:              t.Logf,
 		NetMon:            netMon,
-		Metrics:           reg,
 		EventBus:          bus,
+		// We need a Registry to create the connection, but we
+		// create a clean instance in each test case so we get
+		// isolated metrics -- this Registry is never used.
+		Metrics: new(usermetric.Registry),
 	}))
 	defer conn.Close()
 
-	conn.SetNetworkUp(false)
-	if err := conn.Send([][]byte{{00}}, &lazyEndpoint{}, 0); err == nil {
-		t.Error("expected error, got nil")
-	}
-	resp := httptest.NewRecorder()
-	reg.Handler(resp, new(http.Request))
-	if !strings.Contains(resp.Body.String(), `tailscaled_outbound_dropped_packets_total{reason="error"} 1`) {
-		t.Errorf("expected NetworkDown to increment packet dropped metric; got %q", resp.Body.String())
-	}
+	t.Run("network-down", func(t *testing.T) {
+		// TODO(alexc): This test case fails on Windows because it never
+		// successfully sends the first packet:
+		//
+		// 	   expected successful Send, got err: "write udp4 0.0.0.0:57516->127.0.0.1:9999:
+		// 	   wsasendto: The requested address is not valid in its context."
+		//
+		// It would be nice to run this test on Windows, but I was already
+		// on a side quest and it was unclear if this test has ever worked
+		// correctly on Windows.
+		if runtime.GOOS == "windows" {
+			t.Skipf("skipping on %s", runtime.GOOS)
+		}
+
+		reg := new(usermetric.Registry)
+		conn.metrics = registerMetrics(reg)
+
+		buffs := [][]byte{{00, 00, 00, 00, 00, 00, 00, 00}}
+		ep := &lazyEndpoint{
+			src: epAddr{ap: netip.MustParseAddrPort("127.0.0.1:9999")},
+		}
+		offset := 8
+
+		// Check this is a valid payload to send when the network is up
+		conn.SetNetworkUp(true)
+		if err := conn.Send(buffs, ep, offset); err != nil {
+			t.Errorf("expected successful Send, got err: %q", err)
+		}
+
+		// Now we know the payload would be sent if the network is up,
+		// send it again when the network is down
+		conn.SetNetworkUp(false)
+		err := conn.Send(buffs, ep, offset)
+		if err == nil {
+			t.Error("expected error, got nil")
+		}
+		resp := httptest.NewRecorder()
+		reg.Handler(resp, new(http.Request))
+		if !strings.Contains(resp.Body.String(), `tailscaled_outbound_dropped_packets_total{reason="error"} 1`) {
+			t.Errorf("expected NetworkDown to increment packet dropped metric; got %q", resp.Body.String())
+		}
+	})
+
+	t.Run("invalid-payload", func(t *testing.T) {
+		reg := new(usermetric.Registry)
+		conn.metrics = registerMetrics(reg)
+
+		conn.SetNetworkUp(false)
+		err := conn.Send([][]byte{{00}}, &lazyEndpoint{}, 0)
+		if err == nil {
+			t.Error("expected error, got nil")
+		}
+		resp := httptest.NewRecorder()
+		reg.Handler(resp, new(http.Request))
+		if !strings.Contains(resp.Body.String(), `tailscaled_outbound_dropped_packets_total{reason="error"} 1`) {
+			t.Errorf("expected invalid payload to increment packet dropped metric; got %q", resp.Body.String())
+		}
+	})
 }
 
 func Test_packetLooksLike(t *testing.T) {