chore(rt): address review nits from #1968 and #1974

pkg/hhfab/matrix.go

@@ -572,7 +572,10 @@ type matrixTestDeps struct {
 	pings       *semaphore.Weighted
 	iperfs      *semaphore.Weighted
 	// probes is fixed at 1: overlapping proto-port probes made one probe's
-	// congestion look like another's ACL drop (#1937).
+	// congestion look like another's ACL drop (#1937). It only covers the
+	// proto-port phase; the server-to-server phase is started into the same
+	// WaitGroup and keeps running its pings concurrently under the wider pings
+	// semaphore.
 	probes         *semaphore.Weighted
 	curls          *semaphore.Weighted
 	inSources      func(string) bool
@@ -881,24 +884,26 @@ func runMatrixProtoPortPhase(ctx context.Context, opts TestConnectivityOpts, mat
 						return
 					}
 
-					switch pp.Protocol {
-					case "icmp":
-						if pe := checkPing(ctx, opts.PingsCount, deps.pings, fromName, toName, fromSSH, toIP, nil, expected); pe != nil {
-							deps.errChan <- pe
+					func() {
+						defer deps.probes.Release(1)
+
+						switch pp.Protocol {
+						case "icmp":
+							if pe := checkPing(ctx, opts.PingsCount, deps.pings, fromName, toName, fromSSH, toIP, nil, expected); pe != nil {
+								deps.errChan <- pe
+							}
+						case "tcp":
+							if ie := checkTCPPort(ctx, deps.iperfs, fromName, fromSSH, toIP, pp.Port, expected); ie != nil {
+								deps.errChan <- ie
+							}
+						case "udp":
+							if ie := checkUDPPort(ctx, opts, deps.iperfs, fromName, fromSSH, toIP, pp.Port, expected); ie != nil {
+								deps.errChan <- ie
+							}
+						default:
+							deps.errChan <- fmt.Errorf("matrix proto entry %s→%s has unsupported protocol %q", fromName, toName, pp.Protocol) //nolint:goerr113
 						}
-					case "tcp":
-						if ie := checkTCPPort(ctx, deps.iperfs, fromName, fromSSH, toIP, pp.Port, expected); ie != nil {
-							deps.errChan <- ie
-						}
-					case "udp":
-						if ie := checkUDPPort(ctx, opts, deps.iperfs, fromName, fromSSH, toIP, pp.Port, expected); ie != nil {
-							deps.errChan <- ie
-						}
-					default:
-						deps.errChan <- fmt.Errorf("matrix proto entry %s→%s has unsupported protocol %q", fromName, toName, pp.Protocol) //nolint:goerr113
-					}
-
-					deps.probes.Release(1)
+					}()
 				}
 			})
 		}

pkg/hhfab/rt_gw_nat_acl_suite.go

@@ -14,6 +14,16 @@ func makeGatewayNATACLSuite() *JUnitTestSuite {
 	suite.TestCases = append(suite.TestCases, getNATTestCases()...)
 	suite.TestCases = append(suite.TestCases, getExternalNATTestCases()...)
 	suite.TestCases = append(suite.TestCases, getACLTestCases()...)
+	// Last on purpose: unlike the others it creates an IPv4Namespace and a VPC and
+	// re-attaches a server, so without a wipe between tests anything its reverts
+	// miss would be inherited by every test after it.
+	suite.TestCases = append(suite.TestCases, JUnitTestCase{
+		Name: "Gateway Peering Overlap NAT",
+		F:    gatewayPeeringOverlapNATTest,
+		SkipFlags: SkipFlags{
+			NoGateway: true,
+		},
+	})
 	suite.Tests = len(suite.TestCases)
 
 	return suite

pkg/hhfab/rt_nat_tests.go

@@ -84,6 +84,11 @@ func (testCtx *VPCPeeringTestCtx) runNATTest(ctx context.Context, matrix *Connec
 
 	tcOpts := testCtx.tcOpts
 	tcOpts.Sources = natTestProbeServers(matrix, vpc1.Name, vpc2.Name)
+	if len(tcOpts.Sources) == 0 {
+		// An empty source list means "no filter" downstream, which would silently
+		// probe the whole matrix instead of the VPCs under test.
+		return false, nil, fmt.Errorf("%s: no servers to probe in VPCs %s and %s", spec.Name, vpc1.Name, vpc2.Name) //nolint:goerr113
+	}
 	tcOpts.Destinations = tcOpts.Sources
 	slog.Debug("Probing the VPCs under test", "test", spec.Name, "servers", tcOpts.Sources)
 
@@ -97,7 +102,9 @@ func (testCtx *VPCPeeringTestCtx) runNATTest(ctx context.Context, matrix *Connec
 // natTestProbeServers lists the servers a VPC-to-VPC test should probe: those in
 // the two peered VPCs, plus one outside them as an isolation control. Probing
 // the rest of the matrix only restates that unpeered VPCs cannot talk, at the
-// cost of concurrent probes on the paths under test.
+// cost of concurrent probes on the paths under test. The control is a single
+// server, picked deterministically, so the test proves isolation from that one
+// VPC and not from every unpeered VPC in the topology.
 func natTestProbeServers(matrix *ConnectivityMatrix, vpc1, vpc2 string) []string {
 	underTest := map[string]bool{}
 	var outside []string
@@ -116,6 +123,8 @@ func natTestProbeServers(matrix *ConnectivityMatrix, vpc1, vpc2 string) []string
 	servers := slices.Sorted(maps.Keys(underTest))
 	slices.Sort(outside)
 	for _, name := range outside {
+		// AllEndpoints is keyed per server and VPC, so a server attached to both
+		// an under-test and an outside VPC lands in both lists.
 		if !underTest[name] {
 			servers = append(servers, name)
 
@@ -980,7 +989,8 @@ func gatewayPeeringMasqueradePortForwardNATTest(ctx context.Context, testCtx *VP
 	})
 }
 
-// getNATTestCases returns the NAT test cases to be added to the multi-VPC single-subnet suite
+// getNATTestCases returns the NAT test cases to be added to the multi-VPC single-subnet suite.
+// Gateway Peering Overlap NAT is not here: makeGatewayNATACLSuite appends it last.
 func getNATTestCases() []JUnitTestCase {
 	return []JUnitTestCase{
 		{
@@ -1004,13 +1014,6 @@ func getNATTestCases() []JUnitTestCase {
 				NoGateway: true,
 			},
 		},
-		{
-			Name: "Gateway Peering Overlap NAT",
-			F:    gatewayPeeringOverlapNATTest,
-			SkipFlags: SkipFlags{
-				NoGateway: true,
-			},
-		},
 		{
 			Name: "Gateway Peering Port Forward NAT",
 			F:    gatewayPeeringPortForwardNATTest,

pkg/hhfab/testing_test.go

@@ -362,8 +362,8 @@ rtt min/avg/max/mdev = 0.611/0.912/1.308/0.251 ms
 [1782458023.423317] 64 bytes from 10.20.1.4: icmp_seq=1 ttl=62 time=0.253 ms
 [1782458023.927269] 64 bytes from 10.20.1.4: icmp_seq=2 ttl=62 time=0.400 ms
 [1782458024.935112] no answer yet for icmp_seq=3
-[1782458024.431201] 64 bytes from 10.20.1.4: icmp_seq=4 ttl=62 time=0.425 ms
-[1782458024.935112] 64 bytes from 10.20.1.4: icmp_seq=5 ttl=62 time=0.478 ms
+[1782458024.935634] 64 bytes from 10.20.1.4: icmp_seq=4 ttl=62 time=0.425 ms
+[1782458025.439488] 64 bytes from 10.20.1.4: icmp_seq=5 ttl=62 time=0.478 ms
 --- 10.20.1.4 ping statistics ---
 5 packets transmitted, 4 received, 20% packet loss, time 2016ms
 `