diff --git a/pkg/hhfab/matrix.go b/pkg/hhfab/matrix.go index 7abb6c4c1..2e4dcb894 100644 --- a/pkg/hhfab/matrix.go +++ b/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) + }() } }) } diff --git a/pkg/hhfab/rt_gw_nat_acl_suite.go b/pkg/hhfab/rt_gw_nat_acl_suite.go index 7f5667da8..67bcbb75a 100644 --- a/pkg/hhfab/rt_gw_nat_acl_suite.go +++ b/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 diff --git a/pkg/hhfab/rt_nat_tests.go b/pkg/hhfab/rt_nat_tests.go index 4b8312aac..d9a7e31d5 100644 --- a/pkg/hhfab/rt_nat_tests.go +++ b/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, diff --git a/pkg/hhfab/testing_test.go b/pkg/hhfab/testing_test.go index 6a120323f..8cab21e63 100644 --- a/pkg/hhfab/testing_test.go +++ b/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 `