feat: test-connectivity refactor with matrix - #1761
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors hhfab connectivity testing from a set of ad-hoc, imperative checks into a declarative, matrix-based model that can be repopulated from live peering state and then overlaid with additional expectations (notably for NAT and port-forward scenarios).
Changes:
- Introduces a new
ConnectivityMatrixmodel and a matrix-driven connectivity runner (TestConnectivityWithMatrix/DoVLABTestConnectivityWithMatrix). - Updates VPC setup to return discovered per-server endpoints (with IP/HostBGP metadata) to seed the matrix.
- Refactors multiple runtime test suites (VPC peerings, NAT, externals, failover helpers) to accept an optional matrix and use matrix-based connectivity where appropriate.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/hhfab/vlabrunner.go | Updates SetupVPCs callsites to ignore the new returned endpoints. |
| pkg/hhfab/testing.go | Adds server IP discovery helper; changes SetupVPCs to return endpoints; refactors connectivity prelude and ping/iperf pair execution. |
| pkg/hhfab/rt_utils.go | Routes failover connectivity checks through matrix-based runner when a matrix is provided. |
| pkg/hhfab/rt_static_external.go | Updates test function signature to accept an optional matrix parameter. |
| pkg/hhfab/rt_single_vpc_suite.go | Updates suite test signatures and forwards matrix into connectivity checks where supported. |
| pkg/hhfab/rt_no_vpc_suite.go | Updates suite test signatures (matrix parameter unused for these cases). |
| pkg/hhfab/rt_nat_tests.go | Refactors NAT tests to a spec+overlay pattern that annotates the connectivity matrix with NAT expectations. |
| pkg/hhfab/rt_nat_external_tests.go | Moves NAT external validation to matrix overlays plus a retained stability ping helper. |
| pkg/hhfab/rt_multi_vpc_single_subnet_suite.go | Converts multiple peering scenarios to run via the matrix-based connectivity runner. |
| pkg/hhfab/rt_multi_vpc_multi_subnet_suite.go | Uses matrix-based connectivity for subnet filtering test; updates signatures elsewhere. |
| pkg/hhfab/rt_base.go | Extends TestFunc signature to accept a matrix and builds an initial matrix during suite setup. |
| pkg/hhfab/matrix.go | Adds the matrix data model, repopulation logic, NAT overlay helpers, and the matrix-driven connectivity runner. |
| pkg/hhfab/cmdvlab.go | Changes DoVLABSetupVPCs to return endpoints from SetupVPCs. |
| cmd/hhfab/main.go | Updates CLI setup-vpcs path to ignore returned endpoints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
edipascale
added
ci:+release
Release Tests 12 files 48 suites 3h 59m 18s ⏱️ Results for commit 709bff4. ♻️ This comment has been updated with latest results. |
9add36d to
45674d6
Compare
|
@pau-hedgehog I plan on doing another pass to remove some of the verbose comments and simplify the code in a few places, but this should be ready to be reviewed, at least in terms of the approach used |
39fff37 to
32b0ed1
Compare
f88e533 to
c44067e
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ConnectivityMatrix domain and probe phases, implements SSH-based server endpoint discovery over VPC attachments and concurrent IP matching, changes SetupVPCs to return collected endpoints, introduces connectivity test helpers (reachCheckUnsupported sentinel, prepareConnectivityTest, populateConnectivityMatrix), and wires a shared ConnectivityMatrix through VLAB test harness and all test suites (including NAT test conversion) to execute matrix-driven connectivity checks instead of legacy queries. ChangesConnectivity Matrix–Driven Testing
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/hhfab/rt_nat_external_tests.go (1)
362-374:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
As4()against non-IPv4 NAT pools.Both inverted port-forward paths call
prefix.Masked().Addr().As4()after checking only the prefix length. If the annotation is malformed or IPv6, this panics the runner instead of returning a normal test error.Suggested fix
prefix, err := netip.ParsePrefix(bgpNATCIDR) if err != nil { return false, nil, fmt.Errorf("parsing BGP NAT CIDR %s: %w", bgpNATCIDR, err) } +if !prefix.Addr().Is4() { + return false, nil, fmt.Errorf("BGP NAT CIDR %s must be IPv4", bgpNATCIDR) //nolint:goerr113 +} if prefix.Bits() != 24 { return false, nil, fmt.Errorf("BGP NAT CIDR %s must be a /24 for the .200 inverted NAT address to be valid", bgpNATCIDR) //nolint:goerr113 }Apply the same guard in
staticExternalPortForwardNATGatewayTest.Also applies to: 797-809
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/hhfab/rt_nat_external_tests.go` around lines 362 - 374, The code calls prefix.Masked().Addr().As4() without verifying the address is IPv4, which panics for malformed or IPv6 NAT pools; update the parsing in the function containing the shown snippet and likewise in staticExternalPortForwardNATGatewayTest to check prefix.Addr().Is4() (or inspect the Addr returned by prefix.Masked().Addr()) before calling As4(), and return a formatted error (instead of panicking) when the address is not IPv4 so the test runner reports a normal error; ensure you replace direct As4() usage around the prefix.Masked().Addr().As4() calls with this guarded check and error path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/hhfab/endpoints.go`:
- Around line 241-260: The loop can append multiple Endpoints for the same
attachment because it doesn't skip attachments already claimed in the used
slice; modify the block that picks att := atts[bestIdx] to first check if
used[bestIdx] is true and continue if so, then mark used[bestIdx] = true and
only then append the new Endpoint (affecting the loop over p.ips, the used
slice, and the creation of Endpoint for serverName/vpc/subnet); this ensures
each attachment index produces at most one Endpoint and preserves
ReplaceServerEndpoints' (vpc, subnet) reconciliation contract.
In `@pkg/hhfab/matrix.go`:
- Around line 543-576: The deduplication map extTargets currently keys only on
pfTargetKey{ip,port} which collapses multiple distinct (src,dst) probes; update
the key to include the source (e.g., add a source identifier like
src.Server.Name or src endpoint id) so uniqueness is per (src,ip,port).
Concretely, extend pfTargetKey to include a source field, update where keys are
constructed (the place using pfTargetKey{ip: e.NAT.DestinationIP, port:
e.NAT.DestinationPort}) and any lookups/insertions into extTargets, and keep
using reachabilityFromExpectation(matrix.Lookup(...)) and pfTargetVal as before;
this preserves per-source probe tracking and avoids skipping other sources to
the same external target.
In `@pkg/hhfab/rt_nat_tests.go`:
- Around line 617-629: Do not restore the cached origEndpoints snapshot on
revert; instead re-discover the moved server's current endpoints from
matrix.AllEndpoints at revert time to avoid re-installing stale IPs. Replace
usage of the origEndpoints slice (and any re-install logic that uses those
*Endpoint pointers after ReplaceServerEndpoints) with a fresh lookup that
filters matrix.AllEndpoints for ep.Server != nil && ep.Server.Name ==
targetServer (or call a dedicated helper like FindEndpointsForServer if present)
and use that fresh set when reattaching the server so DHCP-changed leases are
respected.
In `@pkg/hhfab/testing.go`:
- Around line 1888-1940: populateConnectivityMatrix is calling IsServerReachable
and IsExternalSubnetReachable with only server names, which ORs across all
attachments and incorrectly marks every (server, vpc, subnet) EndpointPair as
allowed; update the population to perform attachment-scoped reachability checks:
add/introduce a new reachability API (or overload) that accepts both endpoint
attachment identities (server, vpc, subnet) for source and destination, then
replace calls to IsServerReachable and IsExternalSubnetReachable inside
populateConnectivityMatrix so that for each src and dst Endpoint (from
m.AllEndpoints) you pass the specific attachment info and use that
attachment-scoped response to set ConnectivityExpectation on the corresponding
EndpointPair; ensure you preserve the same error handling (including
reachCheckUnsupported) and set Reason/Peering from the new result.
---
Outside diff comments:
In `@pkg/hhfab/rt_nat_external_tests.go`:
- Around line 362-374: The code calls prefix.Masked().Addr().As4() without
verifying the address is IPv4, which panics for malformed or IPv6 NAT pools;
update the parsing in the function containing the shown snippet and likewise in
staticExternalPortForwardNATGatewayTest to check prefix.Addr().Is4() (or inspect
the Addr returned by prefix.Masked().Addr()) before calling As4(), and return a
formatted error (instead of panicking) when the address is not IPv4 so the test
runner reports a normal error; ensure you replace direct As4() usage around the
prefix.Masked().Addr().As4() calls with this guarded check and error path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ca5fe06-03ef-4c89-bff8-d64305ff05b2
📒 Files selected for processing (16)
cmd/hhfab/main.gopkg/hhfab/cmdvlab.gopkg/hhfab/endpoints.gopkg/hhfab/endpoints_test.gopkg/hhfab/matrix.gopkg/hhfab/rt_base.gopkg/hhfab/rt_multi_vpc_multi_subnet_suite.gopkg/hhfab/rt_multi_vpc_single_subnet_suite.gopkg/hhfab/rt_nat_external_tests.gopkg/hhfab/rt_nat_tests.gopkg/hhfab/rt_no_vpc_suite.gopkg/hhfab/rt_single_vpc_suite.gopkg/hhfab/rt_static_external.gopkg/hhfab/rt_utils.gopkg/hhfab/testing.gopkg/hhfab/vlabrunner.go
c44067e to
4ee922e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/hhfab/rt_nat_tests.go (1)
538-589:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPick an unused valid VLAN instead of
originalVLAN + 100.Line 538 assumes
originalVLAN + 100is always safe, but this test runs against whatever VLANs the suite already has. If that VLAN is already allocated, ororiginalVLANis near the top of the valid range, the overlap VPC create/readiness path will fail for reasons unrelated to NAT. Choose a free VLAN from the current VPC inventory and validate it before buildingoverlapVPC.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/hhfab/rt_nat_tests.go` around lines 538 - 589, The test currently sets newVLAN := originalVLAN + 100 which can collide or exceed valid VLAN range; instead, before creating overlapNS/overlapVPC, scan existing VPCs (via testCtx.kube.List or the inventory used in these tests) to build a set of used VLANs, pick the first unused VLAN within the valid range (e.g., 1–4094) and validate it, then assign that value to newVLAN; use that newVLAN when constructing overlapVPC (referenced by overlapVPC, overlapNS, originalVLAN) and fail the test early with a clear error if no free VLAN is found.
🧹 Nitpick comments (1)
pkg/hhfab/rt_on_ready_suite.go (1)
170-170: 💤 Low valueRemove the commented variable declaration.
The commented variable
staticExtProxyNameis never used. If this is intended for future NAT testing (as suggested by the TODO on line 675), consider either removing it entirely or adding an explanatory TODO comment.🧹 Proposed cleanup
var bgpExtName, staticExtNonProxyName string - // var staticExtProxyName string for _, ext := range extList.Items {Alternatively, if this is planned for future use:
- // var staticExtProxyName string + // TODO: var staticExtProxyName string - will be used for NAT proxy peering tests🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/hhfab/rt_on_ready_suite.go` at line 170, Remove the unused commented variable declaration "// var staticExtProxyRemoteIP string" from the file (pkg/hhfab/rt_on_ready_suite.go) or, if you intend to use it for future NAT testing, replace the commented declaration with a clear TODO comment that explains its planned purpose (referencing staticExtProxyRemoteIP) so the intent is explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/hhfab/rt_nat_tests.go`:
- Around line 60-62: The skip-path in runNATTest currently returns an error when
vpcs.Items < 2; change it to mark the case as skipped without an error by
returning skip=true and nil for both result and error (i.e., return true, nil,
nil) so it matches gatewayPeeringOverlapNATTest and lets callers treat it as a
skip rather than a failure; update the return at the vpcs.Items length check
inside runNATTest accordingly.
---
Outside diff comments:
In `@pkg/hhfab/rt_nat_tests.go`:
- Around line 538-589: The test currently sets newVLAN := originalVLAN + 100
which can collide or exceed valid VLAN range; instead, before creating
overlapNS/overlapVPC, scan existing VPCs (via testCtx.kube.List or the inventory
used in these tests) to build a set of used VLANs, pick the first unused VLAN
within the valid range (e.g., 1–4094) and validate it, then assign that value to
newVLAN; use that newVLAN when constructing overlapVPC (referenced by
overlapVPC, overlapNS, originalVLAN) and fail the test early with a clear error
if no free VLAN is found.
---
Nitpick comments:
In `@pkg/hhfab/rt_on_ready_suite.go`:
- Line 170: Remove the unused commented variable declaration "// var
staticExtProxyRemoteIP string" from the file (pkg/hhfab/rt_on_ready_suite.go)
or, if you intend to use it for future NAT testing, replace the commented
declaration with a clear TODO comment that explains its planned purpose
(referencing staticExtProxyRemoteIP) so the intent is explicit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b04026b9-4dbd-4b10-811d-448c45e2b9c7
📒 Files selected for processing (17)
cmd/hhfab/main.gopkg/hhfab/cmdvlab.gopkg/hhfab/endpoints.gopkg/hhfab/endpoints_test.gopkg/hhfab/matrix.gopkg/hhfab/rt_base.gopkg/hhfab/rt_multi_vpc_multi_subnet_suite.gopkg/hhfab/rt_multi_vpc_single_subnet_suite.gopkg/hhfab/rt_nat_external_tests.gopkg/hhfab/rt_nat_tests.gopkg/hhfab/rt_no_vpc_suite.gopkg/hhfab/rt_on_ready_suite.gopkg/hhfab/rt_single_vpc_suite.gopkg/hhfab/rt_static_external.gopkg/hhfab/rt_utils.gopkg/hhfab/testing.gopkg/hhfab/vlabrunner.go
🚧 Files skipped from review as they are similar to previous changes (15)
- pkg/hhfab/rt_static_external.go
- pkg/hhfab/cmdvlab.go
- pkg/hhfab/vlabrunner.go
- pkg/hhfab/rt_utils.go
- pkg/hhfab/rt_no_vpc_suite.go
- cmd/hhfab/main.go
- pkg/hhfab/rt_single_vpc_suite.go
- pkg/hhfab/rt_multi_vpc_multi_subnet_suite.go
- pkg/hhfab/rt_base.go
- pkg/hhfab/endpoints.go
- pkg/hhfab/rt_multi_vpc_single_subnet_suite.go
- pkg/hhfab/endpoints_test.go
- pkg/hhfab/rt_nat_external_tests.go
- pkg/hhfab/matrix.go
- pkg/hhfab/testing.go
fb7fe92 to
479c1b1
Compare
479c1b1 to
3d9ba10
Compare
|
Filed the DHCP static lease / stale connectivity matrix issue we found while triaging this branch's failures separately: #1876 |
cd3dc5b to
e47a567
Compare
e47a567 to
ce28f3b
Compare
ce28f3b to
acb7a48
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
pkg/hhfab/matrix.go:467
deps.wgis a*sync.WaitGroup, which does not have aGomethod. This won’t compile; useAdd/Donewith a plain goroutine (and keepWait()as-is).
deps.wg.Go(func() {
for _, e := range runPingIperfPair(ctx, opts, args) {
deps.errChan <- e
}
})
pkg/hhfab/matrix.go:522
deps.wgis a*sync.WaitGroup, so callingdeps.wg.Go(...)won’t compile. Replace this withAdd(1)+go func(){ defer Done(); ... }().
deps.wg.Go(func() {
logArgs := []any{"from", name, "expected", expected.Reachable}
if expected.Reachable {
logArgs = append(logArgs, "reason", expected.Reason)
if expected.Peering != "" {
pkg/hhfab/matrix.go:587
deps.wgis a*sync.WaitGroup(noGomethod), so this won’t compile. UseAdd/Doneand a goroutine.
deps.wg.Go(func() {
if ie := runMatrixIperfPortForward(ctx, opts, deps.iperfs, fromName, deps.sshByServer[fromName], target, port, expected); ie != nil {
deps.errChan <- ie
}
})
pkg/hhfab/matrix.go:596
deps.wgis a*sync.WaitGroup, which doesn’t haveGo. This currently won’t compile; useAdd/Donewith a goroutine.
deps.wg.Go(func() {
if ie := runMatrixIperfPortForward(ctx, opts, deps.iperfs, key.from, deps.sshByServer[key.from], key.ip, key.port, val); ie != nil {
deps.errChan <- ie
}
})
pkg/hhfab/matrix.go:758
- This port-forward iperf3 path runs via
toolbox ... iperf3, but the normal iperf3 checks run inside the always-oniperf3container (sudo docker exec iperf3 ...). Using toolbox here can reintroduce the memory/ENOMEM behavior the container approach avoids, and it bypasses the existing iperf parsing/min-speed logic. Consider switching tosudo docker exec iperf3 ... -p <port>and (ideally) reusing the same result parsing/min-speed enforcement ascheckIPerf/runIPerf3Test.
secs := opts.IPerfsSeconds
cmd := fmt.Sprintf("toolbox -E LD_PRELOAD=/lib/x86_64-linux-gnu/libgcc_s.so.1 -q timeout %d iperf3 -J -c %s -p %d -t %d",
secs+25, toIP.String(), toPort, secs)
if _, _, iperfErr := retrySSHCmd(ctx, ssh, cmd, from); iperfErr != nil {
return &IperfError{Source: from, Destination: target, ClientMsg: iperfErr.Error()}
pau-hedgehog
left a comment
pau-hedgehog
left a comment
There was a problem hiding this comment.
Sorry it took me to long. Working through the rest of my review
| } | ||
| r, err := IsServerReachable(ctx, kube, src.Server.Name, dst.Server.Name, gatewayEnabled) | ||
| if err != nil { | ||
| if errors.Is(err, reachCheckUnsupported) { |
There was a problem hiding this comment.
This skip writes no entry, and Lookup treats missing pairs as Deny, so a pair populate can't evaluate silently becomes "expected unreachable".
On master the same condition fails the run loudly. Same pattern: discovery drops endpoints on
warning, and noSetup returns an empty non-nil matrix (rt_base.go:425). All of these pass green while testing nothing.
Can we check the matrix after populate/overlay before leaning on Deny verdicts?
There was a problem hiding this comment.
A golden-file test pinning populate's output for a fixed topology could catch most of this class too
There was a problem hiding this comment.
I've attempted to address most of these comments, with this being the most prominent one, in #1910
Specifically for this, the idea is to have a list of "dropped endpoints" which have IPs that do not match any subnet or attachments with no matching IPs. These are part of the matrix and will fail a validate call that runs as the first step of the test connectivity. Nodes with no IPs are still silently glossed over to be compatible with the eslag l3vni issue/workaround
| if e.Verdict != VerdictAllow { | ||
| continue | ||
| } | ||
| if e.NAT != nil && !e.NAT.SourcePool.IsValid() { |
There was a problem hiding this comment.
SourcePool decides whether to curl, but the observed source address is never asserted. Same on master, so not blocking, just for follow-up
There was a problem hiding this comment.
agreed to leave it for a follow-up
| continue | ||
| } | ||
| e := matrix.Lookup(src, dst, ProtoPort{}) | ||
| if e.Verdict != VerdictAllow || e.NAT == nil { |
There was a problem hiding this comment.
Deny port-forward entries are never probed, so negative coverage doesn't reach port-forwards yet. The Deny entries already keep their NAT details, so the data is there when the runner is ready. Worth a follow-up
There was a problem hiding this comment.
There should now be infra to handle this, although I did not extend the existing tests to make use of it yet
| if matrix == nil { | ||
| connCheckErr = DoVLABTestConnectivity(ctx, testCtx.vlabCfg.WorkDir, testCtx.vlabCfg.CacheDir, testCtx.tcOpts) | ||
| } else { | ||
| connCheckErr = DoVLABTestConnectivityWithMatrix(ctx, testCtx.vlabCfg.WorkDir, testCtx.vlabCfg.CacheDir, testCtx.tcOpts, matrix) |
There was a problem hiding this comment.
Spine, gateway and mesh failover receive the matrix but still call the legacy path, while link failover already uses it here. Do you plan to migrate them later?
There was a problem hiding this comment.
migrated in the branch mentioned above
pau-hedgehog
left a comment
pau-hedgehog
left a comment
There was a problem hiding this comment.
This was huge. It improves our coverage significantly, especially the NAT tests.
There are some suggested follow-ups in the comments, the matrix guard being the main one.
Next thing would be figuring out migration next steps.
| return fmt.Errorf("matrix entry %s→%s (vpc %s/%s) has no valid target IP", src.Server.Name, dst.Server.Name, dst.Server.VPC, dst.Server.Subnet) //nolint:goerr113 | ||
| } | ||
|
|
||
| expected := reachabilityFromExpectation(entry) |
There was a problem hiding this comment.
When a probe fails, the error says what was expected but not why the matrix expected it. The entry knows (Reason, Peering) but that's dropped here and only shows at debug level. Including it in the failure message would make errors easier to debug
There was a problem hiding this comment.
a "why" including reason and peering (or whichever of the two are available if we only have one) has been added
| // Resolve the SSH config and a toolbox mutex for every unique server | ||
| // name referenced by the matrix. | ||
| sshByServer := map[string]*sshutil.Config{} | ||
| toolboxMutexes := map[string]*sync.Mutex{} |
| matrix.Add(ConnectivityExpectation{ | ||
| Pair: EndpointPair{Source: src, Destination: dst}, | ||
| Verdict: VerdictAllow, | ||
| Reason: ReachabilityReasonGatewayPeering, |
There was a problem hiding this comment.
nit: hardcoding reason to GatewayPeering
There was a problem hiding this comment.
an existing reason is now preserved; otherwise we use gateway peering as default, which is the one thing we use it for currently e.g. in NAT tests
replace a number of ad-hoc testing function with a declarative, matrix-based test. Reachability checks that are currently supported by Fabric are preserved as is; additional checks are manually overlayed on the matrix, e.g. for NAT tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Emanuele Di Pascale <emanuele@githedgehog.com>
Move endpoint collection out of SetupVPCs into a standalone, inspection- based helper (CollectServerEndpoints in pkg/hhfab/endpoints.go) so the matrix can model servers with multiple (vpc, subnet) attachments, be built against pre-existing topologies, and be refreshed per-server after a runtime config change. SetupVPCs now delegates to the collector after configuring servers and keeps its ([]*Endpoint, error) signature. BuildConnectivityMatrixFromCluster covers the build-from-scratch case. ReplaceServerEndpoints on the matrix swaps endpoint pointers and wipes stale entries; rebindMatrixServerEndpoint in the overlap-NAT test now reads (vpc, subnet) from the cluster instead of being told. The single-IP discoverServerIP is replaced by discoverServerIPs; the legacy non-matrix TestConnectivity asserts len==1 itself. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Emanuele Di Pascale <emanuele@githedgehog.com>
ensure that followup failover test have up-to-date information Signed-off-by: Emanuele Di Pascale <emanuele@githedgehog.com>
acb7a48 to
709bff4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
pkg/hhfab/testing.go:2904
expose.Asis a slice, but the error uses%s, which will format as%!s(...)at runtime. Use%v(or%qif you stringify) so the message is readable while still wrappingreachCheckUnsupportedforerrors.Ischecks.
if len(expose.As) > 0 {
return false, fmt.Errorf("%w: gw peering with non-empty expose 'As' %s", reachCheckUnsupported, expose.As)
}
pkg/hhfab/matrix.go:645
toolboxMutexesis populated but never used (it only serves as a dedupe set). This adds confusing dead code; you can dedupe usingsshByServerdirectly and drop the mutex map entirely.
sshByServer := map[string]*sshutil.Config{}
toolboxMutexes := map[string]*sync.Mutex{}
for _, ep := range matrix.AllEndpoints {
replace a number of ad-hoc testing function with a declarative, matrix-based test. Reachability checks that are currently supported by Fabric are preserved as is; additional checks are manually overlayed on the matrix, e.g. for NAT tests.