test-connectivity follow-up + multihomed unbundled + ACLs - #1910
Merged
Conversation
edipascale
added
ci:+release
|
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:
📝 WalkthroughWalkthroughVPC setup and endpoint collection now return dropped endpoint records. Connectivity matrices track unknown and dropped states, support protocol-port probes, preserve reachability context, and add gateway peering ACL coverage. ChangesConnectivity discovery and validation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR hardens the connectivity test “oracle” by ensuring the matrix cannot silently omit unevaluated or undiscovered topology elements and still pass (via implicit default-deny lookups). It introduces explicit VerdictUnknown, tracks endpoint discovery drops, and validates the matrix before executing matrix-driven connectivity tests.
Changes:
- Add
VerdictUnknownandConnectivityMatrix.Validate()to prevent testing against incomplete/unevaluated matrices. - Extend endpoint discovery to report “dropped” attachments/addresses and propagate those through matrix construction and validation.
- Improve probe error diagnostics (“why” a verdict was expected) and expand matrix runner coverage (including Deny port-forward targets).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/hhfab/vlabrunner.go | Adapts SetupVPCs call site to new return signature. |
| pkg/hhfab/testing.go | Updates SetupVPCs to return dropped endpoints; enriches probe errors with expectation provenance. |
| pkg/hhfab/rt_single_vpc_suite.go | Switches failover/suite connectivity checks to the matrix runner and refreshes matrix after peering changes. |
| pkg/hhfab/rt_nat_tests.go | Preserves populate-established reasons in NAT overlays; updates endpoint rebind to carry discovery drops. |
| pkg/hhfab/rt_nat_external_tests.go | Updates direct ping helper usage to the new Reachability-based signature. |
| pkg/hhfab/rt_multi_vpc_multi_subnet_suite.go | Updates ping helper usage to pass Reachability. |
| pkg/hhfab/rt_base.go | Plumbs dropped endpoint info into initial matrix build in suite setup. |
| pkg/hhfab/matrix.go | Introduces VerdictUnknown, Validate, drop tracking, improved diagnostics, and expands port-forward probing semantics. |
| pkg/hhfab/endpoints.go | Adds DroppedEndpoint reporting and returns it from endpoint discovery; adds per-server drop replacement helper. |
| pkg/hhfab/endpoints_test.go | Adds tests for drop replacement and matrix validation semantics. |
| pkg/hhfab/cmdvlab.go | Updates helper wrapper to return dropped endpoints from SetupVPCs. |
| cmd/hhfab/main.go | Updates CLI call site to ignore the additional SetupVPCs return value. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
edipascale
marked this pull request as ready for review
July 29, 2026 15:26
edipascale
force-pushed
the
ema/test-connectivity-review
branch
from
July 29, 2026 16:05
e91db67 to
9abfdc8
Compare
edipascale
force-pushed
the
ema/test-connectivity-review
branch
from
July 30, 2026 09:31
9abfdc8 to
79b111b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
pkg/hhfab/matrix.go:316
- The error message has a typo/awkward pluralization ("entr(ies)") which will surface to users and also makes test assertions harder to read. Consider using a plain "entries" message.
errs = append(errs, fmt.Errorf("%d matrix entr(ies) could not be evaluated and were not overlaid by the test: %s", //nolint:goerr113
len(unknowns), strings.Join(unknowns, "; ")))
pkg/hhfab/matrix.go:299
- ConnectivityMatrix.Validate determines whether an Unknown-to-external entry can be ignored by checking for any Allow to any external from that source. This currently counts port-forward/DNAT-only Allow entries (and protocol-specific Allow entries), but runMatrixCurlPhase explicitly ignores DNAT-only externals (NAT present without a valid SourcePool) when computing the external reachability oracle. This mismatch can let Validate accept a matrix that will still behave as "external unreachable" during curl checks, masking Unknown verdicts that should fail the test.
extAllowBySource := map[*Endpoint]bool{}
for pair, byPP := range m.entries {
for _, e := range byPP {
if e.Verdict == VerdictAllow && pair.Destination != nil && pair.Destination.External != nil {
extAllowBySource[pair.Source] = true
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/hhfab/endpoints.go (1)
219-228: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winScope the zero-IP exemption or turn it into a drop. This branch skips every attachment for any server that probes with no IPs; if a non-ESLAG server ever reaches it, discovery failure is hidden instead of recorded in
DroppedEndpoint.🤖 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/endpoints.go` around lines 219 - 228, Update the zero-IP branch in the endpoint discovery loop to apply the exemption only to ESLAG servers operating in L3VNI mode; for all other servers, record each attachment as a DroppedEndpoint instead of continuing silently. Preserve the existing warning and skip behavior for the explicitly exempt ESLAG case.
🤖 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/matrix.go`:
- Around line 687-698: The deny probe in the !expected.Reachable branch should
retry the nc reachability check during a short settle window instead of relying
on retrySSHCmd alone. Update the logic around retrySSHCmd to re-probe until the
port is confirmed unreachable or the retry window expires, while preserving the
existing IperfError response when it remains reachable.
In `@pkg/hhfab/rt_single_vpc_suite.go`:
- Around line 459-467: Update the gateway-peering setup around matrix.Repopulate
to register a final matrix.Repopulate revert before the existing revert stack,
ensuring reverse-order execution refreshes the suite-level matrix after gateway
teardown. Preserve the current initial refresh and error handling, and use the
existing revert mechanism so later tests observe the cleaned topology.
---
Outside diff comments:
In `@pkg/hhfab/endpoints.go`:
- Around line 219-228: Update the zero-IP branch in the endpoint discovery loop
to apply the exemption only to ESLAG servers operating in L3VNI mode; for all
other servers, record each attachment as a DroppedEndpoint instead of continuing
silently. Preserve the existing warning and skip behavior for the explicitly
exempt ESLAG case.
🪄 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
Run ID: e50d92e7-3677-40f4-b1b0-3979cb606b87
📒 Files selected for processing (12)
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_nat_external_tests.gopkg/hhfab/rt_nat_tests.gopkg/hhfab/rt_single_vpc_suite.gopkg/hhfab/testing.gopkg/hhfab/vlabrunner.go
edipascale
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
pkg/hhfab/matrix.go:892
- When expected.Reachable is false, runMatrixIperfPortForward treats any non-nil error from
nc -zw2as a successful deny. This can create false passes if the probe can’t run (e.g.ncmissing, SSH command failure), since those cases should be reported as infrastructure failures rather than “denied as expected”.
if !expected.Reachable {
if _, _, err := retrySSHCmd(ctx, ssh, fmt.Sprintf("nc -zw2 %s %d", toIP.String(), toPort), from); err == nil {
return &IperfError{
Source: from,
Destination: target,
Why: why,
ClientMsg: "port-forward target is reachable but the matrix expects it to be denied",
}
}
return nil
}
pkg/hhfab/matrix.go:732
- TCP/UDP proto-port probe failures currently lose the diagnostic context that was added elsewhere (IperfError.Why / expectationWhy). Setting Why before emitting these errors keeps proto-port ACL failures actionable (which peering/reason produced the expectation).
port := pp.Port
deps.wg.Go(func() {
if ie := checkTCPPort(ctx, deps.iperfs, fromName, fromSSH, toIP, port, expected.Reachable); ie != nil {
deps.errChan <- ie
}
pkg/hhfab/vlabbuilder.go:380
- Typo in comment: “orphan laves” → “orphan leaves”.
// add eslag leaves to candidates for multihomed servers if there are not enough orphan laves
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 (2)
pkg/hhfab/endpoints.go (1)
233-238: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRecord zero-IP servers unless they are explicitly known ESLAG/L3VNI cases.
This branch is only safe for the intended ESLAG/L3VNI servers, but
serverAttachment/collectedcarry no ESLAG signal, so any other zero-IP server with real attachments will be skipped without enteringdroppedandValidate()won’t flag the topology as partial. Thread the ESLAG/L3VNI state through discovery or record zero-IP cases by default.🤖 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/endpoints.go` around lines 233 - 238, Update the zero-IP handling in the endpoint discovery flow around serverAttachment/collected so zero-IP servers with attachments are recorded in dropped by default and remain visible to Validate(). Preserve the skip-without-drop behavior only when the server is explicitly identified as an ESLAG/L3VNI case, threading that state through discovery as needed.pkg/hhfab/testing.go (1)
758-856: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMixed-bucket multihomed servers can fail
setup-vpcsAtpkg/hhfab/testing.go:769-855,925-935,vpc.Spec.Subnets[subnetName].HostBGPis fixed by the first server in each bucket, buthostBGPis decided per server. The genericsetup-vpcspath accepts arbitraryservers-per-subnet, so a multihomed server can land in a bucket already marked non-hostBGP and then take the hostBGP path against an unconfigured subnet. Reset the bucket on the hostBGP transition or keep multihomed servers on fresh buckets.🤖 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/testing.go` around lines 758 - 856, Prevent mixed HostBGP and non-HostBGP servers from sharing a subnet bucket in the setup-vpcs allocation flow. After determining hostBGP and useP2P for each server, move multihomed/HostBGP servers to a fresh bucket when the current subnet’s HostBGP setting differs, before deriving subnetName or configuring the subnet. Ensure vpc.Spec.Subnets[subnetName].HostBGP always matches the server path that uses it.
🧹 Nitpick comments (3)
pkg/hhfab/rt_acl_tests.go (1)
52-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTie
aclProbePorttopersistentIperf3Port. The comment asserts this port is the always-on iperf3 daemon port, but 5201 is re-hardcoded here whilematrix.godefinespersistentIperf3Port = 5201. Deriving it keeps the invariant if the daemon port changes.♻️ Proposed change
- aclProbePort uint16 = 5201 + aclProbePort uint16 = persistentIperf3Port🤖 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_acl_tests.go` around lines 52 - 57, Update the aclProbePort constant in the ACL test port declarations to derive its value from persistentIperf3Port instead of hard-coding 5201. Preserve aclAltPort unchanged and keep the existing always-on daemon behavior.pkg/hhfab/matrix_test.go (1)
82-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit coverage for
ConnectivityMatrix.Validate.
Validateis the gate that now fails whole runs (unknown verdicts, dropped discoveries, empty endpoint sets, external-allow exemption), and it is pure in-memory logic — ideal for table tests. This file covers only the proto-port helpers.🤖 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/matrix_test.go` around lines 82 - 140, Add table-driven unit tests for ConnectivityMatrix.Validate covering unknown verdicts, dropped discovery entries, empty endpoint sets, and the external-allow exemption. Build each case with the existing ConnectivityMatrix and endpoint helpers, assert validation success or failure as appropriate, and keep the current proto-port tests unchanged.pkg/hhfab/matrix.go (1)
719-732: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTCP/UDP probes lose the reachability reason.
checkPingreceives the fullexpected, whilecheckTCPPort/checkUDPPortget onlyexpected.Reachable, so their failures cannot report why the matrix expected allow/deny. Consider widening those helpers to takeReachabilityfor consistent diagnostics.🤖 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/matrix.go` around lines 719 - 732, Update the TCP/UDP probe flow around checkTCPPort and checkUDPPort to pass the full expected Reachability value instead of only expected.Reachable. Widen both helper signatures and their diagnostic handling so failures retain and report the reachability reason consistently with checkPing, while preserving the existing allow/deny behavior.
🤖 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 171-180: Update the duplicate-attachment collapse condition in
CollectServerEndpoints so it only skips matching (vpc, subnet) attachments when
subnet.HostBGP is enabled. Leave non-HostBGP duplicates on the existing
warning/drop path so they produce the expected diagnostics and DroppedEndpoint
records.
In `@pkg/hhfab/matrix.go`:
- Around line 658-673: Add a short readiness-confirmation loop in the on-demand
listener startup flow around retrySSHCmd, after launching each detached iperf3
server and before appending it to started. Probe the target host and port until
the listener is accepting connections or the bounded retry timeout expires, then
tear down and return an error on failure; preserve the existing startup and
cleanup behavior for successful listeners.
In `@pkg/hhfab/rt_acl_tests.go`:
- Around line 476-489: The expected-to-fail cases returned by getACLTestCases
are currently registered unconditionally; gate them behind an appropriate
feature/environment toggle or make each TestFunc self-skip by returning the
established skipped-result tuple with the enforcement-not-implemented reason. In
pkg/hhfab/rt_acl_tests.go lines 476-489, apply the gating or self-skip behavior
to the ACL cases; in pkg/hhfab/rt_multi_vpc_single_subnet_suite.go lines
124-125, retain the append unchanged once those cases are gated or
self-skipping.
- Around line 293-300: Replace the hardcoded "subnet-01" values in the
allow-subnet-fwd rule with names derived through a vpcFirstSubnetName helper,
mirroring the existing vpcFirstSubnetCIDR usage. Pass vpc1 and vpc2 respectively
so each endpoint matches its fixture’s actual first subnet.
---
Outside diff comments:
In `@pkg/hhfab/endpoints.go`:
- Around line 233-238: Update the zero-IP handling in the endpoint discovery
flow around serverAttachment/collected so zero-IP servers with attachments are
recorded in dropped by default and remain visible to Validate(). Preserve the
skip-without-drop behavior only when the server is explicitly identified as an
ESLAG/L3VNI case, threading that state through discovery as needed.
In `@pkg/hhfab/testing.go`:
- Around line 758-856: Prevent mixed HostBGP and non-HostBGP servers from
sharing a subnet bucket in the setup-vpcs allocation flow. After determining
hostBGP and useP2P for each server, move multihomed/HostBGP servers to a fresh
bucket when the current subnet’s HostBGP setting differs, before deriving
subnetName or configuring the subnet. Ensure
vpc.Spec.Subnets[subnetName].HostBGP always matches the server path that uses
it.
---
Nitpick comments:
In `@pkg/hhfab/matrix_test.go`:
- Around line 82-140: Add table-driven unit tests for
ConnectivityMatrix.Validate covering unknown verdicts, dropped discovery
entries, empty endpoint sets, and the external-allow exemption. Build each case
with the existing ConnectivityMatrix and endpoint helpers, assert validation
success or failure as appropriate, and keep the current proto-port tests
unchanged.
In `@pkg/hhfab/matrix.go`:
- Around line 719-732: Update the TCP/UDP probe flow around checkTCPPort and
checkUDPPort to pass the full expected Reachability value instead of only
expected.Reachable. Widen both helper signatures and their diagnostic handling
so failures retain and report the reachability reason consistently with
checkPing, while preserving the existing allow/deny behavior.
In `@pkg/hhfab/rt_acl_tests.go`:
- Around line 52-57: Update the aclProbePort constant in the ACL test port
declarations to derive its value from persistentIperf3Port instead of
hard-coding 5201. Preserve aclAltPort unchanged and keep the existing always-on
daemon behavior.
🪄 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
Run ID: 0454e1da-d86a-4520-bbc9-7aa05983ad35
📒 Files selected for processing (11)
pkg/hhfab/endpoints.gopkg/hhfab/matrix.gopkg/hhfab/matrix_test.gopkg/hhfab/rt_acl_tests.gopkg/hhfab/rt_acl_tests.mdpkg/hhfab/rt_multi_vpc_single_subnet_suite.gopkg/hhfab/rt_nat_tests.gopkg/hhfab/rt_on_ready_suite.gopkg/hhfab/rt_utils.gopkg/hhfab/testing.gopkg/hhfab/vlabbuilder.go
This was referenced Jul 30, 2026
edipascale
force-pushed
the
ema/test-connectivity-review
branch
from
July 30, 2026 11:29
0f3da43 to
9e4fba8
Compare
pau-hedgehog
force-pushed
the
ema/test-connectivity-review
branch
from
August 2, 2026 08:21
f25c945 to
3a681ce
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (3)
pkg/hhfab/vlabbuilder.go:380
- Typo in comment: "laves" → "leaves".
// add eslag leaves to candidates for multihomed servers if there are not enough orphan laves
pkg/hhfab/endpoints.go:237
- When a server has VPCAttachments but endpoint discovery finds no configured IPs, the code currently skips the server without recording any DroppedEndpoint. This undermines ConnectivityMatrix.Validate(): the matrix can silently omit attached servers and still be treated as a “sound oracle”. Recording a drop per attachment (or at least one per server) will make partial discovery fail fast and avoid false-pass connectivity runs.
if len(p.ips) == 0 {
// Expected for ESLAG servers in L3VNI mode, which never run hhnet; not recorded as a drop
slog.Warn("Server has no configured IPs, skipping endpoints", "server", p.serverName, "attachments", len(atts))
continue
pkg/hhfab/testing.go:3417
- This function uses an iperf semaphore for concurrency control, but on failure the message says "acquiring iperf3 semaphore" even though the TCP probe is implemented with
nc. This makes failures harder to interpret when debugging probe infra vs iperf3 issues.
if sem != nil {
if err := sem.Acquire(ctx, 1); err != nil {
ie.ClientMsg = fmt.Sprintf("acquiring iperf3 semaphore: %s", err)
return ie
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/hhfab/testing.go (1)
2253-2262: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider returning an empty string when no reason exists.
expectationWhyreturns the literal"no reason recorded"for a zero-valueReachability. Deny expectations built byreachabilityFromExpectationcarry an emptyReason, so every deny-pathPingError,IperfError, andCurlErrormessage gains the suffix, because: no reason recorded.whySuffixalready suppresses the suffix for an empty string.♻️ Proposed change
func expectationWhy(r Reachability) string { switch { case r.Reason != "" && r.Peering != "": return fmt.Sprintf("%s %q", r.Reason, r.Peering) case r.Reason != "": return string(r.Reason) default: - return "no reason recorded" + return "" } }🤖 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/testing.go` around lines 2253 - 2262, Update expectationWhy to return an empty string in its default branch when Reachability has no reason, while preserving the existing formatting for populated Reason and Peering values so whySuffix can omit the reason suffix for deny-path errors.pkg/hhfab/matrix.go (1)
758-764: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider using the ping semaphore for TCP connect probes.
checkTCPPortruns a singlenc -zw2connect. It is as cheap as a ping and much cheaper than an iperf3 throughput run. It currently acquiresdeps.iperfs, whose default parallelism is 1 (opts.IPerfsParalleldefaults to 1 at line 796). Every TCP proto-port probe therefore serializes, and each can block for up to the 2-secondnctimeout plus the SSH round-trip. An ACL suite with many TCP expectations will be slow.
deps.pingsdefaults to 50 and matches the cost profile better. Keepdeps.iperfsfor the UDP branch, which does run iperf3.🤖 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/matrix.go` around lines 758 - 764, Update the TCP branch in the probe dispatch to pass deps.pings to checkTCPPort instead of deps.iperfs, allowing TCP connect probes to use the higher-concurrency ping semaphore. Leave deps.iperfs assigned to the UDP/iperf3 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/matrix.go`:
- Around line 687-690: Update the pkill command constructed in the iperf3
listener teardown block to anchor the port pattern at the end of the command
line, ensuring a requested port such as 5301 cannot match superstring ports such
as 53010. Preserve the existing retrySSHCmd and warning behavior.
- Around line 752-757: Update the "icmp" branch in runMatrixProtoPortPhase to
report an error when opts.PingsCount is zero or less instead of calling
checkPing and silently accepting the expectation. Preserve the existing
checkPing behavior for positive ping counts, and send the resulting error
through deps.errChan so the matrix fails for an unexecutable ICMP expectation.
---
Nitpick comments:
In `@pkg/hhfab/matrix.go`:
- Around line 758-764: Update the TCP branch in the probe dispatch to pass
deps.pings to checkTCPPort instead of deps.iperfs, allowing TCP connect probes
to use the higher-concurrency ping semaphore. Leave deps.iperfs assigned to the
UDP/iperf3 path.
In `@pkg/hhfab/testing.go`:
- Around line 2253-2262: Update expectationWhy to return an empty string in its
default branch when Reachability has no reason, while preserving the existing
formatting for populated Reason and Peering values so whySuffix can omit the
reason suffix for deny-path errors.
🪄 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
Run ID: 3512e8d1-5495-44c1-bbd6-0860599a5112
📒 Files selected for processing (19)
cmd/hhfab/main.gopkg/hhfab/cmdvlab.gopkg/hhfab/endpoints.gopkg/hhfab/endpoints_test.gopkg/hhfab/matrix.gopkg/hhfab/matrix_test.gopkg/hhfab/rt_acl_tests.gopkg/hhfab/rt_acl_tests.mdpkg/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_on_ready_suite.gopkg/hhfab/rt_single_vpc_suite.gopkg/hhfab/rt_utils.gopkg/hhfab/testing.gopkg/hhfab/vlabbuilder.gopkg/hhfab/vlabrunner.go
🚧 Files skipped from review as they are similar to previous changes (16)
- cmd/hhfab/main.go
- pkg/hhfab/rt_multi_vpc_multi_subnet_suite.go
- pkg/hhfab/rt_multi_vpc_single_subnet_suite.go
- pkg/hhfab/vlabrunner.go
- pkg/hhfab/rt_base.go
- pkg/hhfab/cmdvlab.go
- pkg/hhfab/rt_nat_external_tests.go
- pkg/hhfab/rt_single_vpc_suite.go
- pkg/hhfab/rt_acl_tests.md
- pkg/hhfab/rt_on_ready_suite.go
- pkg/hhfab/matrix_test.go
- pkg/hhfab/rt_utils.go
- pkg/hhfab/vlabbuilder.go
- pkg/hhfab/endpoints_test.go
- pkg/hhfab/rt_nat_tests.go
- pkg/hhfab/endpoints.go
Lookup answers Deny for any pair with no entry, so anything the matrix silently lost was asserted as "expected unreachable" and passed green. Three paths did exactly that: populate skipped pairs it could not evaluate, discovery dropped endpoints on a warning, and noSetup returned an empty matrix. - add VerdictUnknown, written where populate hits reachCheckUnsupported instead of leaving the pair absent - have CollectServerEndpoints report dropped attachments/addresses, and carry them on the matrix (a server with no IPs at all stays a warning: ESLAG in L3VNI mode legitimately has none) - add ConnectivityMatrix.Validate, called at the top of TestConnectivityWithMatrix, rejecting leftover Unknowns, drops and an empty endpoint set. An all-Deny topology stays valid; an Unknown to an external is only fatal when that source has no Allow to any external, since the external oracle ORs over all of them Also from review: probe failures now say which peering (or lack of one) the expectation came from, Deny port-forward entries are probed instead of skipped, spine/gateway/mesh failover use the matrix runner, and NAT overlays keep the reason populate established instead of hardcoding gateway-peering. Signed-off-by: Emanuele Di Pascale <emanuele@githedgehog.com>
Signed-off-by: Emanuele Di Pascale <emanuele@githedgehog.com>
Replace single-connection/subnet args of getServerHostBGPCmd with a []HostBGPParams slice so the helper can compose configs for hosts attached to multiple hostBGP VPCs over multiple connections. Each entry carries VPC label, connections, VLAN, subnet, and per-server address offset. Updates the two existing call sites (SetupVPCs and the on-ready release test) to pass a single-element slice; behavior is unchanged. Signed-off-by: Emanuele Di Pascale <emanuele@githedgehog.com>
automatically opt for a hostBGP subnet if a server has multiple unbundled connections. also detect this setup in test-connectivity and treat it as a single endpoint Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Emanuele Di Pascale <emanuele@githedgehog.com>
the standard topology has 2 eslag leaves and 1 orphan, and we want to be able to test hostbgp with this. vlab gen now prefers orphan leaves for mh servers but will take eslag leaves too if there are not enough orphans Signed-off-by: Emanuele Di Pascale <emanuele@githedgehog.com>
The ConnectivityMatrix's ProtoPort field was unused scaffolding: the runner only ever probed ICMP (ping), TCP iperf3 on port 5201, and curl, and every Lookup passed the zero ProtoPort. Gateway peering ACLs need per-protocol/per-port verdicts on the same endpoint pair (e.g. TCP allow + UDP deny), which the single default verdict cannot express. Plumb ProtoPort end to end: - ProtoPortEntries/HasProtoPortEntries accessors. - runMatrixProtoPortPhase probes each non-zero ProtoPort entry (icmp via ping, tcp via nc, udp via iperf3 -u with loss parsing); the legacy server-server phase skips proto-scoped pairs so the two are disjoint. - On-demand iperf3 listeners for arbitrary (non-5201) ports. - checkTCPPort/checkUDPPort probes with no throughput floor. - setVPCToVPCProtoVerdict overlay helper for tests. Infra only; the ACL spec builder and ACL tests come next. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Emanuele Di Pascale <emanuele@githedgehog.com>
Add GwPeeringOptions.ACL so a PeeringACL can be attached to a gateway peering spec, and a rt_acl_tests.go suite exercising ACLs in isolation on the multi-VPC single-subnet suite, built on the ProtoPort matrix infra: default deny, deny-unless-exposed baseline, explicit allow, protocol scoping, direction (packet) scoping, flow scope opening the reverse direction, subnet scoping, arbitrary-port scoping, and two rule-precedence cases. Directional/precedence cases use packet scope to isolate the direction under test; a dedicated flow-scope case asserts the stateful reverse. These encode the intended behavior ahead of the dataplane ACL implementation, so they are expected to fail until enforcement lands. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Emanuele Di Pascale <emanuele@githedgehog.com>
Validate() built its "external is allowed from this source" gate from any Allow to any External across all ProtoPorts, while runMatrixCurlPhase only honors default-ProtoPort, non-DNAT-only entries. A port-forward or proto-scoped entry could therefore suppress an Unknown external entry that the curl phase goes on to assert as denied. Both now share externalCurlAllowed(). Also: - wait for on-demand iperf3 listeners to bind before the proto-port phase probes them; docker exec -d returns as soon as the process is spawned - gate the multihomed attachment collapse on subnet.HostBGP, so duplicate attachments without a shared VIP still surface as drops instead of silently losing an endpoint - derive the ACL subnet-scoping rule's subnet names rather than assuming subnet-01 - fix the "entr(ies)" plural in the validation error Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Emanuele Di Pascale <emanuele@githedgehog.com>
SetupVPCs set a subnet's HostBGP flag when the subnet was created, from the first server assigned to it, while computing hostBGP per server. With ServersPerSubnet > 1 the two could disagree, and the resulting subnet is not merely mislabeled: a hostBGP subnet has DHCP disabled and only accepts unbundled attachments, so a shared subnet either gets rejected by the API or leaves one of its servers without an address. Roll to a fresh subnet whenever a server's hostBGP requirement differs from that of the subnet it would join. This affects vlabs generated with --multihomed-servers (multihomed servers always speak hostBGP) and --hostbgp-subnet, both of which could previously produce a broken topology depending on where the server landed in the packing order. Such a vlab may now use more subnets, and hence more VPCs, than before. Also makes subnet.HostBGP an exact signal for the multihomed attachment collapse in CollectServerEndpoints. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Emanuele Di Pascale <emanuele@githedgehog.com>
Two unrelated causes of "matrix is not a sound oracle" in CI. The ACL flow-scope masquerade case left its default ProtoPort entry as populate produced it — Unknown, since the masquerade expose 'As' is not evaluable — while overlaying only the proto-scoped verdicts. That entry is read by nothing: runMatrixServerServerPhase hands proto-scoped pairs to runMatrixProtoPortPhase, which reads only the proto entries. Validate was stricter than the phases, so skip default entries shadowed that way, keeping port-forward entries (still read by runMatrixPortForwardPhase). The external port-forward cases hit a real gap. IsExternalSubnetReachable scans every external and aborts on the first unevaluable peering, so the DNAT peering under test poisons the verdict for the other external too. A port-forward grants no egress, so its Allow no longer settles the source's curl expectation, and nothing else did. State it: mark the externals the peering never mentions unreachable, which the curl phase then verifies. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Emanuele Di Pascale <emanuele@githedgehog.com>
edipascale
force-pushed
the
ema/test-connectivity-review
branch
from
August 3, 2026 07:27
3a681ce to
08fa246
Compare
pau-hedgehog
reviewed
Aug 3, 2026
pau-hedgehog
reviewed
Aug 3, 2026
pau-hedgehog
reviewed
Aug 3, 2026
pau-hedgehog
approved these changes
Aug 3, 2026
pau-hedgehog
left a comment
pau-hedgehog
left a comment
Contributor
There was a problem hiding this comment.
Nice work, the oracle part specially: Validate() plus VerdictUnknown closes the pass-by-testing-nothing failure mode, as we saw on CI last week.
I checked ACL enforcement in the dataplane repo, so the ten cases assert what they claim. The "expected to fail" in one of the commits is stale. The rest of my comments are small.
Next, multihomed/hostBGP on #1747 which Github is suggesting to stack. Do we want that?
Frostman
approved these changes
Aug 4, 2026
Merged
This was referenced Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An attempt to address most of the comments on #1761, kept as a separate branch in case we decide to merge the connectivity test refactor as it currently is while we address them.
Includes a manual pass to clean a big chunk of the AI comment slop. I kept what I thought was useful (e.g. gotchas and clarifications).
Also includes other stuff that was stacked on top of test-connectivity as separate PRs (i.e. multihomed unbundled support and ACLs release tests), since it was asked by the reviewers.
Does not include the hostbgp release test for the moment, as rebasing it on top of the connectivity refactor implies substantial changes and I want to make sure I'm not breaking it before committing to the change