wgengine/magicsock: remove opts.SimulatedNetwork.

It only existed to override one test-only behavior with a
different test-only behavior, in both cases working around
an annoying feature of our CI environments. Instead, handle
that weirdness entirely in the test code, with a tweaked
TestOnlyPacketListener that gets injected.

Signed-off-by: David Anderson <danderson@tailscale.com>

wgengine/magicsock/magicsock.go

@@ -281,7 +281,6 @@ type Conn struct {
 	idleFunc               func() time.Duration // nil means unknown
 	testOnlyPacketListener nettype.PacketListener
 	noteRecvActivity       func(tailcfg.DiscoKey) // or nil, see Options.NoteRecvActivity
-	simulatedNetwork       bool
 
 	// ================================================================
 	// No locking required to access these fields, either because
@@ -541,13 +540,6 @@ type Options struct {
 	// should not hold Conn.mu while calling it.
 	NoteRecvActivity func(tailcfg.DiscoKey)
 
-	// SimulatedNetwork can be set true in tests to signal that
-	// the network is simulated and thus it's okay to bind on the
-	// unspecified address (which we'd normally avoid to avoid
-	// triggering macOS and Windows firwall dialog boxes during
-	// "go test").
-	SimulatedNetwork bool
-
 	// LinkMonitor is the link monitor to use.
 	// With one, the portmapper won't be used.
 	LinkMonitor *monitor.Mon
@@ -605,7 +597,6 @@ func NewConn(opts Options) (*Conn, error) {
 	c.idleFunc = opts.IdleFunc
 	c.testOnlyPacketListener = opts.TestOnlyPacketListener
 	c.noteRecvActivity = opts.NoteRecvActivity
-	c.simulatedNetwork = opts.SimulatedNetwork
 	c.portMapper = portmapper.NewClient(logger.WithPrefix(c.logf, "portmapper: "), c.onPortMapChanged)
 	if opts.LinkMonitor != nil {
 		c.portMapper.SetGatewayLookupFunc(opts.LinkMonitor.GatewayAndSelfIP)
@@ -2679,15 +2670,9 @@ func (c *Conn) initialBind() error {
 
 // listenPacket opens a packet listener.
 // The network must be "udp4" or "udp6".
-// Host is the (local) IP address to listen on; use the zero IP to leave unspecified.
+func (c *Conn) listenPacket(network string, port uint16) (net.PacketConn, error) {
-func (c *Conn) listenPacket(network string, host netaddr.IP, port uint16) (net.PacketConn, error) {
 	ctx := context.Background() // unused without DNS name to resolve
-	// Translate host to package net: "" for the zero value, the IP address string otherwise.
+	addr := net.JoinHostPort("", fmt.Sprint(port))
-	var s string
-	if !host.IsZero() {
-		s = host.String()
-	}
-	addr := net.JoinHostPort(s, fmt.Sprint(port))
 	if c.testOnlyPacketListener != nil {
 		return c.testOnlyPacketListener.ListenPacket(ctx, network, addr)
 	}
@@ -2701,18 +2686,6 @@ func (c *Conn) listenPacket(network string, host netaddr.IP, port uint16) (net.P
 // If curPortFate is set to dropCurrentPort, no attempt is made to reuse
 // the current port.
 func (c *Conn) bindSocket(rucPtr **RebindingUDPConn, network string, curPortFate currentPortFate) error {
-	var host netaddr.IP
-	if inTest() && !c.simulatedNetwork {
-		switch network {
-		case "udp4":
-			host = netaddr.MustParseIP("127.0.0.1")
-		case "udp6":
-			host = netaddr.MustParseIP("::1")
-		default:
-			panic("unrecognized network in bindSocket: " + network)
-		}
-	}
-
 	if *rucPtr == nil {
 		*rucPtr = new(RebindingUDPConn)
 	}
@@ -2753,7 +2726,7 @@ func (c *Conn) bindSocket(rucPtr **RebindingUDPConn, network string, curPortFate
 			c.logf("magicsock: bindSocket %v close failed: %v", network, err)
 		}
 		// Open a new one with the desired port.
-		pconn, err = c.listenPacket(network, host, port)
+		pconn, err = c.listenPacket(network, port)
 		if err != nil {
 			c.logf("magicsock: unable to bind %v port %d: %v", network, port, err)
 			continue

wgengine/magicsock/magicsock_test.go

@@ -150,7 +150,6 @@ func newMagicStack(t testing.TB, logf logger.Logf, l nettype.PacketListener, der
 		EndpointsFunc: func(eps []tailcfg.Endpoint) {
 			epCh <- eps
 		},
-		SimulatedNetwork: l != nettype.Std{},
 	})
 	if err != nil {
 		t.Fatalf("constructing magicsock: %v", err)
@@ -557,8 +556,45 @@ func makeNestable(t *testing.T) (logf logger.Logf, setT func(t *testing.T)) {
 	return logf, setT
 }
 
+// localhostOnlyListener is a nettype.PacketListener that listens on
+// localhost (127.0.0.1 or ::1, depending on the requested network)
+// when asked to listen on the unspecified address.
+//
+// It's used in tests where we set up localhost-to-localhost
+// communication, because if you listen on the unspecified address on
+// macOS and Windows, you get an interactive firewall consent prompt
+// to allow the binding, which breaks our CIs.
+type localhostListener struct{}
+
+func (localhostListener) ListenPacket(ctx context.Context, network, address string) (net.PacketConn, error) {
+	host, port, err := net.SplitHostPort(address)
+	if err != nil {
+		return nil, err
+	}
+	switch network {
+	case "udp4":
+		switch host {
+		case "", "0.0.0.0":
+			host = "127.0.0.1"
+		case "127.0.0.1":
+		default:
+			return nil, fmt.Errorf("localhostListener cannot be asked to listen on %q", address)
+		}
+	case "udp6":
+		switch host {
+		case "", "::":
+			host = "::1"
+		case "::1":
+		default:
+			return nil, fmt.Errorf("localhostListener cannot be asked to listen on %q", address)
+		}
+	}
+	var conf net.ListenConfig
+	return conf.ListenPacket(ctx, network, net.JoinHostPort(host, port))
+}
+
 func TestTwoDevicePing(t *testing.T) {
-	l, ip := nettype.Std{}, netaddr.IPv4(127, 0, 0, 1)
+	l, ip := localhostListener{}, netaddr.IPv4(127, 0, 0, 1)
 	n := &devices{
 		m1:     l,
 		m1IP:   ip,
@@ -577,12 +613,12 @@ func TestNoDiscoKey(t *testing.T) {
 	tstest.PanicOnLog()
 	tstest.ResourceCheck(t)
 
-	derpMap, cleanup := runDERPAndStun(t, t.Logf, nettype.Std{}, netaddr.IPv4(127, 0, 0, 1))
+	derpMap, cleanup := runDERPAndStun(t, t.Logf, localhostListener{}, netaddr.IPv4(127, 0, 0, 1))
 	defer cleanup()
 
-	m1 := newMagicStack(t, t.Logf, nettype.Std{}, derpMap)
+	m1 := newMagicStack(t, t.Logf, localhostListener{}, derpMap)
 	defer m1.Close()
-	m2 := newMagicStack(t, t.Logf, nettype.Std{}, derpMap)
+	m2 := newMagicStack(t, t.Logf, localhostListener{}, derpMap)
 	defer m2.Close()
 
 	removeDisco := func(idx int, nm *netmap.NetworkMap) {
@@ -1110,8 +1146,9 @@ func newTestConn(t testing.TB) *Conn {
 	t.Helper()
 	port := pickPort(t)
 	conn, err := NewConn(Options{
-		Logf: t.Logf,
+		Logf:                   t.Logf,
-		Port: port,
+		Port:                   port,
+		TestOnlyPacketListener: localhostListener{},
 		EndpointsFunc: func(eps []tailcfg.Endpoint) {
 			t.Logf("endpoints: %q", eps)
 		},