More connectivity matrix fixes

[edipascale]

No description provided.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Changes

The PR centralizes connectivity expectation ownership, updates TCP and UDP probe handling, expands ACL coverage, improves reachability diagnostics, and adjusts ESLAG endpoint and multihomed leaf selection.

Connectivity matrix validation and probing

Layer / File(s)	Summary
Expectation ownership and validation pkg/hhfab/matrix.go, pkg/hhfab/matrix_test.go	entryOwner classifies expectations as unknown or unreadable. Validation and probe-enable checks cover server, external, NAT, port-forward, and filtered entries.
Shared probe selection and dispatch pkg/hhfab/matrix.go	Probe phases share ownership and server-pair filtering. Listener cleanup matches exact ports. Disabled required probes fail early.
Reachability-aware TCP and UDP checks pkg/hhfab/testing.go, pkg/hhfab/rt_nat_tests.go	TCP and UDP checks preserve reachability details, apply nested deadlines, retry unparseable UDP results, and classify inconclusive loss.
Probe command and diagnostic coverage pkg/hhfab/testing_test.go	Tests cover BGP commands, UDP timing, and reachability explanation formatting.

ACL protocol and NAT coverage

Layer / File(s)	Summary
ACL test cases and documentation pkg/hhfab/rt_acl_tests.go, pkg/hhfab/rt_acl_tests.md	The ACL suite and coverage documentation cover numeric ICMP matching, pre-NAT destination evaluation, flow-scope NAT restrictions, and external-peering limitations.

ESLAG and multihoming support

Layer / File(s)	Summary
ESLAG and L3VNI endpoint handling pkg/hhfab/endpoints.go, pkg/hhfab/endpoints_test.go	Endpoint collection tracks ESLAG and VPC mode metadata. Missing-IP handling skips only ESLAG/L3VNI-only servers.
ESLAG fallback and candidate ordering pkg/hhfab/rt_eslag_fallback.go, pkg/hhfab/vlabbuilder.go, cmd/hhfab/main.go	ESLAG fallback refreshes the matrix endpoint after bond restoration. Candidate construction prioritizes orphan leaves and appends ESLAG leaves when required. The CLI description permits any two different leaves.

Possibly related PRs

• githedgehog/fabricator#1761: Introduced the matrix-based connectivity validation and probing refined here.
• githedgehog/fabricator#1808: Overlaps with the ESLAG fallback test changes.
• githedgehog/fabricator#1913: Shares reachability handling in testing.go and connectivity-matrix code.

Suggested reviewers: copilot, frostman, pau-hedgehog

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 23.08% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	❓ Inconclusive	No pull request description was provided, so the changeset has no author-provided summary to assess.	Add a concise description that summarizes the connectivity matrix, ACL, ESLAG, and diagnostic changes.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies connectivity matrix fixes, which are the primary focus of the changeset.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

Pull request overview

This PR continues tightening the connectivity-matrix-driven test harness by making matrix validation stricter (rejecting expectations that no probe phase can assert), improving probe error attribution, and expanding ACL test coverage to cover additional dataplane behaviors.

Changes:

• Adjust multihomed server leaf candidate selection to prefer orphan leaves and only “top up” with ESLAG leaves when needed.
• Refactor connectivity matrix validation/probing by introducing a single “entry owner” (phase) classifier, adding explicit validation for unprobeable expectations, and improving TCP/UDP probe error context via Reachability.
• Extend gateway ACL test suite/docs with numeric-protocol ICMP coverage and pre-NAT destination matching coverage; add unit tests for getServerHostBGPCmd.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/hhfab/vlabbuilder.go	Ensures orphan leaves are preferred for multihomed server placement, with ESLAG leaves appended only when necessary.
pkg/hhfab/testing.go	Updates TCP/UDP port probes to accept Reachability and improves IperfError context (Why).
pkg/hhfab/testing_test.go	Adds unit tests for host BGP command generation from server connections.
pkg/hhfab/rt_acl_tests.md	Documents new ACL test cases/coverage and clarifies limitations.
pkg/hhfab/rt_acl_tests.go	Adds new ACL test cases (numeric ICMP protocol; pre-NAT destination match) and reuses the persistent iperf3 port constant.
pkg/hhfab/matrix.go	Adds phase ownership classification, validates unprobeable entries, anchors listener teardown pkill, and aligns phases with entryOwner.
pkg/hhfab/endpoints_test.go	Updates validation tests and adds coverage for “no phase can read this entry” failures.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

[github-actions]

Release Tests

 12 files  ± 0   48 suites  ±0   4h 53m 17s ⏱️ - 21m 11s
 57 tests + 2   55 ✅ + 2    2 💤 ±0  0 ❌ ±0 
684 runs  +24  311 ✅ +20  373 💤 +4  0 ❌ ±0 

Results for commit 48c69e9. ± Comparison against base commit 1d27ea7.

♻️ This comment has been updated with latest results.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

pkg/hhfab/endpoints_test.go (1)

319-326: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the required unprobed classification.

The endpoint label also appears in the unknown-entry error. A regression that returns could not be evaluated instead of no probe phase can read still passes this case. Store the result and assert the required classification.

Proposed fix

-		require.ErrorContains(t, m.Validate(), "server-1(vpc-1/default) → server-2(vpc-2/default)")
+		err = m.Validate()
+		require.ErrorContains(t, err, "no probe phase can read")
+		require.ErrorContains(t, err, "server-1(vpc-1/default) → server-2(vpc-2/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_test.go` around lines 319 - 326, Update the final
m.Validate assertion in the TranslatedAddress case to store the returned error
and assert that it contains both the endpoint label and the required “no probe
phase can read” unprobed classification, rather than relying only on the
endpoint text.

🤖 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 319-321: Add an ownership check to the condition that applies the
external Allow shortcut. The existing condition checks e.Pair.Destination,
e.Pair.Destination.External, and extAllowBySource[e.Pair.Source], but it should
also verify that the entry owner equals matrixPhaseCurl before continuing. This
ensures the shortcut only suppresses validation for curl-owned entries with
VerdictUnknown, not for unrelated external DNAT entries that happen to share a
source with an unscoped external Allow.
- Around line 373-376: Update Validate() to reject entries that simultaneously
define ProtoPort and a nonzero NAT.DestinationPort, including the inherited NAT
overlay case; return the existing validation error format for invalid
combinations. Preserve entryOwner() precedence only for valid, non-overlapping
entries.

---

Nitpick comments:
In `@pkg/hhfab/endpoints_test.go`:
- Around line 319-326: Update the final m.Validate assertion in the
TranslatedAddress case to store the returned error and assert that it contains
both the endpoint label and the required “no probe phase can read” unprobed
classification, rather than relying only on the endpoint text.

🪄 Autofix

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: b7b6827a-1c23-4575-b4fe-33ebe46b8b66

📥 Commits

Reviewing files that changed from the base of the PR and between 01803ea and ca75c69.

📒 Files selected for processing (8)

• pkg/hhfab/endpoints_test.go
• pkg/hhfab/matrix.go
• pkg/hhfab/rt_acl_tests.go
• pkg/hhfab/rt_acl_tests.md
• pkg/hhfab/rt_eslag_fallback.go
• pkg/hhfab/testing.go
• pkg/hhfab/testing_test.go
• pkg/hhfab/vlabbuilder.go

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (1)

pkg/hhfab/matrix.go:410

• hasEntriesOwnedBy only scans explicitly added expectations in m.entries, but the probe phases also act on implicit default expectations returned by Lookup (e.g., default-deny server→server and server→external pairs). As a result, TestConnectivityWithMatrix can allow pings/iperfs/curls to be disabled even though those phases would still have work (and would otherwise pass those implicit expectations unasserted).

func (m *ConnectivityMatrix) hasEntriesOwnedBy(p matrixPhase) bool {
	for _, byPP := range m.entries {
		for _, e := range byPP {
			if m.entryOwner(e) == p {
				return true

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

[coderabbitai]

Actionable comments posted: 1

🤖 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 201-202: Update the HostBGP attachment deduplication flow around
the attachment collection and metadata construction so duplicate attachments
preserve metadata from every raw VPCAttachment: merge l3vni with logical AND
(and retain the ESLAG exemption only when appropriate) before applying the no-IP
suppression at the line-250 path. Ensure the resulting behavior is independent
of VPCAttachmentList.Items order, and add regression coverage for both
attachment orders.

🪄 Autofix

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: 7733d4ca-a1a5-41e8-b774-869a6a44266c

📥 Commits

Reviewing files that changed from the base of the PR and between b9c51d5 and e6df485.

📒 Files selected for processing (2)

• pkg/hhfab/endpoints.go
• pkg/hhfab/endpoints_test.go

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (3)

pkg/hhfab/matrix.go:537

• probesServerPair() claims to match what the server-to-server phases will probe, but it doesn’t exclude self-pairs or same-node pairs. Those are skipped by entryOwner(), so this helper can return true for pairs that will never be probed (and may lead to extra iteration / future inconsistencies).

// probesServerPair reports whether the server-to-server phases will probe this
// pair, so listener setup covers exactly what gets probed.
func (d *matrixTestDeps) probesServerPair(src, dst *Endpoint) bool {
	return src.Server != nil && dst.Server != nil &&
		d.inSources(src.Server.Name) && d.inDestinations(dst.Server.Name)

pkg/hhfab/matrix.go:855

• The “phase has no enabled probe” guards use matrix.hasEntriesOwnedBy(...), which doesn’t consider opts.Sources/opts.Destinations. This can incorrectly fail runs where the caller filters out all server-to-server (or curl) entries, even though nothing in that phase would actually be probed.

	// checkPing/checkIPerf/checkCurl no-op when their count is off, so a phase
	// left with no enabled probe would pass its entries unasserted
	if opts.PingsCount <= 0 && opts.IPerfsSeconds <= 0 && matrix.hasEntriesOwnedBy(matrixPhaseServerServer) {
		return fmt.Errorf("matrix has server-to-server entries but both pings and iperfs are disabled") //nolint:goerr113
	}

pkg/hhfab/endpoints.go:110

• The new naming is misleading: serverAttachment.l3vni is set via vpc.Spec.Mode != vpcapi.VPCModeL2VNI, which includes both l3vni and l3flat modes, and eslagL3VNIOnly() similarly tests “non-L2VNI” rather than L3VNI specifically. Consider renaming to reflect the actual semantics (e.g., l3Mode / eslagNonL2VNIOnly) to avoid confusion as more VPC modes exist.

	eslag      bool
	l3vni      bool
	attachName string // for diagnostics
}

// eslagL3VNIOnly reports whether every one of the server's attachments is an
// ESLAG connection into a non-L2VNI VPC. Those servers never run hhnet, so
// discovering no addresses on them is expected rather than a failure.
func eslagL3VNIOnly(atts []serverAttachment) bool {
	if len(atts) == 0 {
		return false
	}

	return !slices.ContainsFunc(atts, func(a serverAttachment) bool {
		return !a.eslag || !a.l3vni
	})
}

[pau-hedgehog]

Nice work, this closes almost everything from the last round plus the two issues that came out of reviewing it.

One defect worth fixing: the new disabled-probe guard counts stored entries while the server-to-server phase probes synthesized ones, so it can pass while asserting nothing. One question on the UDP connect budget depending on the expectation. The rest are comments and one-liners.

I ran two mutations on env-1 to check the ACL suite fails when it should: breaking the ACL config fails Default Deny through all three probe types both ways, and flipping one tcp verdict fails exactly one probe in one direction. Worth knowing it has teeth, and maybe worth making permanent.

The red v-mesh-gw job looks like the #1929/#1937/#1924 single-packet-loss family: one lost ping on an expected-allow switch-peering pair, nothing to do with the ACL or matrix paths. Worth rebasing onto master before merge so CI re-runs and we get a clean board rather than merging with a red check.

Nits, batched here since several sit on lines this PR doesn't touch:

• overlayExternalNoEgress still writes a bare {Pair, Verdict: VerdictDeny} with no Reason, Peering or Detail, so a curl failure on one of those pairs prints "because: no reason recorded" in the branch whose parent added the because clause. One Detail line.
• The --multihomed-servers usage string still says "2 connections to 2 different orphan leaves" while 94fe6f2 makes ESLAG leaves the documented fallback. The struct comment was fixed in #1910, this one was missed.
• TestValidate is still in endpoints_test.go and this PR adds 40 more lines to it. Cheaper to move to matrix_test.go now than later.
• checkUDPPort's dead band: "UDP traffic was not blocked" is a real improvement over claiming delivery, but it still doesn't say the result was inconclusive or print the two thresholds.
• The UDP retry doubles the pathological worst case to 2 x outer, ~96s for a deny probe at release defaults, serialized behind the weight-1 semaphore. The expected case is far better and the retry is the right call; just noting the tail.

[edipascale]

@pau-hedgehog I rebased on master and pushed two more commits on top of the old ones; the first addresses your comments (except for the UDP connection timeout, until we decide what to do), the second improves the because clause for ACLs. I want to avoid a fourth round of matrix fixes if we can, so let's try and fix any remaining issue here

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (2)

pkg/hhfab/matrix.go:351

• The doc comment on entryOwner is misleading: Validate calls entryOwner before it reports VerdictUnknown entries, and entryOwner itself has VerdictUnknown-specific logic (e.g. external Curl allow short-circuit). The comment currently states Validate has already rejected VerdictUnknown, which isn’t true and may confuse future changes (e.g. someone removing the VerdictUnknown branch as “dead”).

// entryOwner assumes Validate has already rejected VerdictUnknown: the phases
// carry no verdict guard of their own, and reachabilityFromExpectation maps
// Unknown to Reachable:false, so a phase reached without Validate would assert
// deny on an entry explicitly marked unevaluable.

pkg/hhfab/endpoints.go:110

• Function/field naming doesn’t match the behavior described in the comment: eslagL3VNIOnly returns true for any attachment where VPC mode is “non-L2VNI” (endpoints.go:202 sets l3vni as Mode != VPCModeL2VNI), which includes L3Flat as well as L3VNI (see other code paths checking VPCModeL3VNI || VPCModeL3Flat). Consider renaming the boolean (e.g. l3Mode) and the helper (e.g. eslagL3ModeOnly) to avoid implying it only applies to L3VNI.

// eslagL3VNIOnly reports whether every one of the server's attachments is an
// ESLAG connection into a non-L2VNI VPC. Those servers never run hhnet, so
// discovering no addresses on them is expected rather than a failure.
func eslagL3VNIOnly(atts []serverAttachment) bool {
	if len(atts) == 0 {
		return false
	}

	return !slices.ContainsFunc(atts, func(a serverAttachment) bool {
		return !a.eslag || !a.l3vni
	})
}

[pau-hedgehog]

Rebase looks good, and thanks for turning these around so fast.

checkProbesEnabled closes the guard properly: it walks the same pairs the phases do, including the Deny that Lookup synthesizes where populate stored nothing, and honouring the source/destination filters covers the ownership-is-not-probed gap as well. TestCheckProbesEnabled asserting the guard rather than the walk is the right shape.

Also reviewed 78e59df on top. expectationWhy keeps the allow path byte-identical and only changes the deny phrasing, TestExpectationWhy covers every branch of it, and each current caller of setVPCToVPCProtoVerdict is an ACL context so labelling a proto-scoped deny "ACL" holds for all of them. "ACL on gateway-peering ..." is a real improvement on a failure line, that phrasing bothered me in the mutation runs too.

UDP connect budget settled in-thread: keeping 5s, revisit if we ever see a deny probe time out on a path we expect to be open.

Nothing else from me.