From e151b74f93c5669dadce10524276952ec5bcac74 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Wed, 25 Aug 2021 23:33:46 -0700 Subject: [PATCH] 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 --- wgengine/magicsock/magicsock.go | 33 ++---------------- wgengine/magicsock/magicsock_test.go | 51 ++++++++++++++++++++++++---- 2 files changed, 47 insertions(+), 37 deletions(-) diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index 3881a631c..3a1e387ea 100644 --- a/wgengine/magicsock/magicsock.go +++ b/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, host netaddr.IP, port uint16) (net.PacketConn, error) { +func (c *Conn) listenPacket(network string, 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. - var s string - if !host.IsZero() { - s = host.String() - } - addr := net.JoinHostPort(s, fmt.Sprint(port)) + addr := net.JoinHostPort("", 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 diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go index d16eaed43..c63dbc7d1 100644 --- a/wgengine/magicsock/magicsock_test.go +++ b/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, - Port: port, + Logf: t.Logf, + Port: port, + TestOnlyPacketListener: localhostListener{}, EndpointsFunc: func(eps []tailcfg.Endpoint) { t.Logf("endpoints: %q", eps) },