From d7f8d3940f57337af52b3df6d9d3d84fc0949b81 Mon Sep 17 00:00:00 2001 From: Anastassios Nanos Date: Fri, 9 Oct 2026 13:06:09 +0300 Subject: [PATCH 1/2] test(netgw): Draw another port when the probed one is taken The forward tests take a port from a throwaway listener on 127.0.0.1, close it, and expect Expose to bind it a moment later. That is not what the probe promises. The kernel's search for a free port is exact-address for a SO_REUSEADDR socket, which every Go listener is, so a port another process holds on 0.0.0.0 is still handed out on 127.0.0.1; the probe is TCP, so a UDP holder is never seen at all; and the other packages' tests run beside this one on the CI Mac and take ports as they go. TestExposeLeavesAnOverlappingAddressToTheKernel, which binds 0.0.0.0 first, failed a run that way: Expose(0.0.0.0): a forward already listens there: 0.0.0.0:63935 is held outside this gateway: listen tcp 0.0.0.0:63935: bind: address already in use A port held outside the gateway says nothing about the gateway. Every test that publishes on a fresh port now draws again on that one refusal and fails on any other: exposeFresh for a set of forwards that have to share one port, withdrawing what it had installed before it redraws, and startupNetworkFresh for the forward New installs, which binds the same way and refuses the same way. Refs: https://github.com/brig-sh/hull/actions/runs/37910806016/job/113755213031 Signed-off-by: Anastassios Nanos --- internal/netgw/forward_test.go | 141 +++++++++++++++++++++++++-------- 1 file changed, 108 insertions(+), 33 deletions(-) diff --git a/internal/netgw/forward_test.go b/internal/netgw/forward_test.go index 8e57d60d..cae9801a 100644 --- a/internal/netgw/forward_test.go +++ b/internal/netgw/forward_test.go @@ -20,6 +20,7 @@ import ( "reflect" "strconv" "strings" + "syscall" "testing" ) @@ -89,7 +90,8 @@ func forwardNetwork(t *testing.T, start ...Forward) *Network { } // freePort takes a port from the host and gives it straight back, so the -// number is one nothing else on this machine is listening on. +// number is one nothing on this machine was listening on at 127.0.0.1 just +// now. That is all it promises: see exposeFresh for what it does not. func freePort(t *testing.T) string { t.Helper() l, err := net.Listen("tcp", "127.0.0.1:0") @@ -100,17 +102,103 @@ func freePort(t *testing.T) string { return strconv.Itoa(l.Addr().(*net.TCPAddr).Port) } +// exposeFresh publishes the forwards on one port the binds accepted, and +// returns what was installed along with the port. Each Local is a host with +// no port -- "127.0.0.1", "0.0.0.0", or "" for the empty host -- and the +// forwards must differ in protocol: whether two addresses can share a port +// within one protocol is the kernel's answer, refused as ErrForwardExists +// without the errno this helper redraws on, and a test asking that question +// has to ask it itself. +// +// A port freePort hands back can be in use by the time Expose binds it. The +// kernel's search for a free port is exact-address for a SO_REUSEADDR socket, +// which every Go listener is, so a port another process holds on 0.0.0.0 is +// still handed out on 127.0.0.1; the probe is TCP, so a UDP holder is never +// seen at all; and the other packages' tests run beside this one and take +// ports as they go. Either way the bind fails with the port held outside this +// gateway, which says nothing about the gateway and failed a CI run over a +// number. The test draws again instead, withdrawing whatever of the set it +// had already installed, so the forwards end up on one port together. +func exposeFresh(t *testing.T, n *Network, forwards ...Forward) ([]Forward, string) { + t.Helper() + // As Expose reads them: the empty protocol is tcp, and case is ignored. + protocols := map[string]bool{} + for _, f := range forwards { + protocol, err := parseProtocol(f.Protocol) + if err != nil { + t.Fatalf("exposeFresh: %v", err) + } + if protocols[protocol] { + t.Fatalf("exposeFresh: two %s forwards cannot share a port through this helper", protocol) + } + protocols[protocol] = true + } + var err error + for range 10 { + port := freePort(t) + var installed []Forward + for _, f := range forwards { + f.Local += ":" + port + var got Forward + got, err = n.Expose(f) + if err != nil { + break + } + installed = append(installed, got) + } + if err == nil { + return installed, port + } + for _, got := range installed { + if _, uerr := n.Unexpose(got.Protocol, got.Local); uerr != nil { + t.Fatalf("Unexpose after a redraw: %v", uerr) + } + } + if !errors.Is(err, syscall.EADDRINUSE) { + break + } + } + t.Fatalf("Expose on a fresh port: %v", err) + return nil, "" +} + +// startupNetworkFresh is forwardNetwork for a startup forward on a fresh +// port, drawing again the way exposeFresh does: New installs the forward +// through the same bind and refuses the same way. +func startupNetworkFresh(t *testing.T, f Forward) (*Network, string) { + t.Helper() + var err error + for range 10 { + port := freePort(t) + f := f + f.Local += ":" + port + var n *Network + n, err = New(Config{ + MTU: 1500, + Subnet: testSubnet, + GatewayIP: testGatewayIP, + GatewayMacAddress: testGatewayMA, + Forwards: []Forward{f}, + }) + if err == nil { + return n, port + } + if !errors.Is(err, syscall.EADDRINUSE) { + break + } + } + t.Fatalf("New with a startup forward on a fresh port: %v", err) + return nil, "" +} + func TestExposeAndUnexposeChangeARunningGateway(t *testing.T) { n := forwardNetwork(t) if got := n.Forwards(); len(got) != 0 { t.Fatalf("a new gateway has %d forwards", len(got)) } - local := "127.0.0.1:" + freePort(t) - installed, err := n.Expose(Forward{Local: local, Remote: "10.87.0.2:80"}) - if err != nil { - t.Fatalf("Expose: %v", err) - } + got, port := exposeFresh(t, n, Forward{Local: "127.0.0.1", Remote: "10.87.0.2:80"}) + installed, local := got[0], "127.0.0.1:"+port // What was installed, with the defaults filled in -- this is what the API // answers a POST with, and a caller that omitted the protocol must not be // told it is empty when a later read says tcp. @@ -142,10 +230,8 @@ func TestExposeAndUnexposeChangeARunningGateway(t *testing.T) { func TestExposeRefusesALocalAddressAlreadyPublished(t *testing.T) { n := forwardNetwork(t) - local := "127.0.0.1:" + freePort(t) - if _, err := n.Expose(Forward{Local: local, Remote: "10.87.0.2:80"}); err != nil { - t.Fatalf("Expose: %v", err) - } + _, port := exposeFresh(t, n, Forward{Local: "127.0.0.1", Remote: "10.87.0.2:80"}) + local := "127.0.0.1:" + port _, err := n.Expose(Forward{Local: local, Remote: "10.87.0.3:80"}) if !errors.Is(err, ErrForwardExists) { t.Fatalf("second Expose: %v is not ErrForwardExists", err) @@ -161,12 +247,8 @@ func TestExposeRefusesALocalAddressAlreadyPublished(t *testing.T) { // same collision as publishing either twice. func TestExposeReadsAnEmptyHostAsAllInterfaces(t *testing.T) { n := forwardNetwork(t) - port := freePort(t) - got, err := n.Expose(Forward{Local: ":" + port, Remote: "10.87.0.2:80"}) - if err != nil { - t.Fatalf("Expose: %v", err) - } - if got.Local != "0.0.0.0:"+port { + installed, port := exposeFresh(t, n, Forward{Local: "", Remote: "10.87.0.2:80"}) + if got := installed[0]; got.Local != "0.0.0.0:"+port { t.Fatalf("Expose installed %q, want 0.0.0.0:%s", got.Local, port) } if _, err := n.Expose(Forward{Local: "0.0.0.0:" + port, Remote: "10.87.0.3:80"}); !errors.Is(err, ErrForwardExists) { @@ -188,10 +270,7 @@ func TestExposeLeavesAnOverlappingAddressToTheKernel(t *testing.T) { {"127.0.0.1", "0.0.0.0"}, } { n := forwardNetwork(t) - port := freePort(t) - if _, err := n.Expose(Forward{Local: order[0] + ":" + port, Remote: "10.87.0.2:80"}); err != nil { - t.Fatalf("Expose(%s): %v", order[0], err) - } + _, port := exposeFresh(t, n, Forward{Local: order[0], Remote: "10.87.0.2:80"}) _, err := n.Expose(Forward{Local: order[1] + ":" + port, Remote: "10.87.0.3:80"}) if err != nil && !errors.Is(err, ErrForwardExists) { t.Fatalf("Expose(%s) after %s: %v is neither installed nor ErrForwardExists", @@ -223,10 +302,7 @@ func TestExposeReportsAPortHeldOutsideTheGatewayAsPublished(t *testing.T) { // hosts: the set's keys differ, so only the bind refuses. func TestExposeNamesTheForwardHoldingAPortItCannotBind(t *testing.T) { n := forwardNetwork(t) - port := freePort(t) - if _, err := n.Expose(Forward{Protocol: "udp", Local: "0.0.0.0:" + port, Remote: "10.87.0.2:53"}); err != nil { - t.Fatalf("Expose: %v", err) - } + _, port := exposeFresh(t, n, Forward{Protocol: "udp", Local: "0.0.0.0", Remote: "10.87.0.2:53"}) _, err := n.Expose(Forward{Protocol: "udp", Local: "127.0.0.1:" + port, Remote: "10.87.0.3:53"}) if !errors.Is(err, ErrForwardExists) { t.Fatalf("udp beside a published wildcard: %v is not ErrForwardExists", err) @@ -272,13 +348,12 @@ func TestUnexposeTellsAnUnreadableTargetFromAnAbsentOne(t *testing.T) { // addresses on one port are two forwards. func TestExposeAllowsOnePortOnTwoSpecificAddresses(t *testing.T) { n := forwardNetwork(t) - port := freePort(t) - if _, err := n.Expose(Forward{Local: "127.0.0.1:" + port, Remote: "10.87.0.2:80"}); err != nil { - t.Fatalf("Expose: %v", err) - } - if _, err := n.Expose(Forward{Protocol: "udp", Local: "127.0.0.1:" + port, Remote: "10.87.0.3:80"}); err != nil { - t.Fatalf("udp beside tcp on one address: %v", err) - } + // Both through the helper: the probe is TCP, so the port it hands out is + // unverified for UDP, and a UDP holder elsewhere would fail the second + // bind with no way to draw again from here. + exposeFresh(t, n, + Forward{Local: "127.0.0.1", Remote: "10.87.0.2:80"}, + Forward{Protocol: "udp", Local: "127.0.0.1", Remote: "10.87.0.3:80"}) if got := n.Forwards(); len(got) != 2 { t.Fatalf("want two forwards, got %+v", got) } @@ -292,8 +367,8 @@ func TestUnexposeRefusesAPortNothingPublished(t *testing.T) { } func TestStartupForwardsAreListedLikeAnyOther(t *testing.T) { - local := "127.0.0.1:" + freePort(t) - n := forwardNetwork(t, Forward{Protocol: "tcp", Local: local, Remote: "10.87.0.2:8080"}) + n, port := startupNetworkFresh(t, Forward{Protocol: "tcp", Local: "127.0.0.1", Remote: "10.87.0.2:8080"}) + local := "127.0.0.1:" + port want := []Forward{{"tcp", local, "10.87.0.2:8080"}} if got := n.Forwards(); !reflect.DeepEqual(got, want) { t.Fatalf("Forwards = %+v, want %+v", got, want) From d7ded41d50fbeef8feef4835015593f4dec57acd Mon Sep 17 00:00:00 2001 From: Anastassios Nanos Date: Fri, 9 Oct 2026 13:06:09 +0300 Subject: [PATCH 2/2] test(netgw): Widen the reply wait for the shared CI Mac A member waits three seconds for the gateway to answer a frame. Every answer is computed in-process -- the netstack's ICMP reply, the ARP exchange it needs first, a dial the forwarder makes -- so the wait is a liveness bound, not a budget the gateway is held to, and it only has to be wide enough that a slow machine cannot cross it. The CI Mac runs these under the race detector, beside the e2e job's VMs and whatever the other runners on it are doing, and crossed it once: --- FAIL: TestICMPEchoIsAnsweredNotForwarded (4.92s) forwarder_test.go:321: timed out waiting for an ICMP echo reply Make it ten seconds, named, and use it for every bound of the same kind: the forwarder's dial wait, the poll for a stack counter, and the deadline on a pipe write, which completes when the gateway reads the frame and so waits for the same thing. A reply that never comes still fails, later. Refs: https://github.com/brig-sh/hull/actions/runs/37910806016/job/113758751765 Signed-off-by: Anastassios Nanos --- internal/netgw/egress_test.go | 4 ++-- internal/netgw/forward_test.go | 4 +++- internal/netgw/forwarder_test.go | 4 ++-- internal/netgw/network_test.go | 20 +++++++++++++++++--- 4 files changed, 24 insertions(+), 8 deletions(-) diff --git a/internal/netgw/egress_test.go b/internal/netgw/egress_test.go index e0240823..e1ef9947 100644 --- a/internal/netgw/egress_test.go +++ b/internal/netgw/egress_test.go @@ -396,7 +396,7 @@ func TestWatchHostsResolvesBeforeTheFirstTick(t *testing.T) { go policy.WatchHosts(ctx, res, time.Hour) guest, dst := addr(t, testGuestIP), addr(t, "93.184.216.34") - deadline := time.Now().Add(3 * time.Second) + deadline := time.Now().Add(replyWait) for !policy.AllowsConnection(guest, dst) { if time.Now().After(deadline) { t.Fatal("WatchHosts did not resolve before its first tick") @@ -415,7 +415,7 @@ func TestWatchHostsStopsWithTheContext(t *testing.T) { cancel() select { case <-done: - case <-time.After(3 * time.Second): + case <-time.After(replyWait): t.Fatal("WatchHosts outlived its context") } } diff --git a/internal/netgw/forward_test.go b/internal/netgw/forward_test.go index cae9801a..6c6bc439 100644 --- a/internal/netgw/forward_test.go +++ b/internal/netgw/forward_test.go @@ -164,7 +164,9 @@ func exposeFresh(t *testing.T, n *Network, forwards ...Forward) ([]Forward, stri // startupNetworkFresh is forwardNetwork for a startup forward on a fresh // port, drawing again the way exposeFresh does: New installs the forward -// through the same bind and refuses the same way. +// through the same bind and refuses the same way. A refused New leaves its +// stack and goroutines running, as every test's network does -- the package +// has no Close -- so a redraw here costs one more of those. func startupNetworkFresh(t *testing.T, f Forward) (*Network, string) { t.Helper() var err error diff --git a/internal/netgw/forwarder_test.go b/internal/netgw/forwarder_test.go index 6136e10c..dc3026fe 100644 --- a/internal/netgw/forwarder_test.go +++ b/internal/netgw/forwarder_test.go @@ -69,7 +69,7 @@ func awaitDial(t *testing.T, dials chan dialed) dialed { select { case d := <-dials: return d - case <-time.After(3 * time.Second): + case <-time.After(replyWait): t.Fatal("the forwarder never dialed out") return dialed{} } @@ -245,7 +245,7 @@ func TestIPv6NeverEgresses(t *testing.T) { func awaitUnknownL3(t *testing.T, n *Network, protocol tcpip.NetworkProtocolNumber) { t.Helper() - deadline := time.Now().Add(2 * time.Second) + deadline := time.Now().Add(replyWait) for { counter, ok := n.stack.Stats().NICs.UnknownL3ProtocolRcvdPacketCounts.Get(uint64(protocol)) if ok && counter.Value() > 0 { diff --git a/internal/netgw/network_test.go b/internal/netgw/network_test.go index 38e2dda5..4fd6f888 100644 --- a/internal/netgw/network_test.go +++ b/internal/netgw/network_test.go @@ -129,16 +129,30 @@ func (m *member) arpReplyTo(frame []byte) []byte { func (m *member) send(t *testing.T, frame []byte) { t.Helper() - _ = m.conn.SetWriteDeadline(time.Now().Add(2 * time.Second)) + // A pipe write completes when the gateway reads the frame, which is the + // same in-process answer await waits for, under the same load. + _ = m.conn.SetWriteDeadline(time.Now().Add(replyWait)) if _, err := m.conn.Write(frame); err != nil { t.Fatalf("send frame: %v", err) } } -// await returns the first frame matching pred, or fails after the deadline. +// replyWait bounds how long a test waits for the gateway to answer. +// +// Every answer is computed in-process -- the netstack's ICMP reply, an ARP +// exchange with the member, a dial the forwarder makes, a counter it bumps, +// the read that completes a pipe write, a resolver tick -- so this is a +// liveness bound, not a budget the gateway is held to. It only has to be +// wide enough that a slow machine cannot cross it: the CI Mac runs these +// under the race detector beside the e2e job's VMs and the other runners' +// work, and three seconds was crossed there once, by an echo reply that +// arrived late rather than not at all. +const replyWait = 10 * time.Second + +// await returns the first frame matching pred, or fails after replyWait. func (m *member) await(t *testing.T, what string, pred func([]byte) bool) []byte { t.Helper() - deadline := time.After(3 * time.Second) + deadline := time.After(replyWait) for { select { case frame := <-m.frames: