Gateway ACL release-tests - #1874
Closed
edipascale wants to merge 2 commits into
Closed
Conversation
edipascale
added
ci:-vlab
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesThe connectivity matrix now supports protocol/port-scoped expectations and probes. TCP and UDP reachability checks were added, with on-demand iperf3 listeners. Gateway peering ACL options and release tests cover multiple ACL behaviors. Gateway ACL connectivity
Possibly related issues
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
Adds a new set of “release-tests” that encode the intended behavior for gateway peering ACLs (expected to fail until dataplane enforcement is implemented), and extends the matrix-driven connectivity runner to probe protocol/port–scoped expectations.
Changes:
- Introduces protocol/port–scoped matrix expectations and a dedicated probe phase (ICMP ping, TCP connect, UDP iperf3-loss) including on-demand iperf3 listeners for non-5201 ports.
- Extends gateway peering spec helpers to accept an optional peering-scoped ACL and adds a new ACL-focused runtime test suite file wired into the multi-VPC single-subnet suite.
- Extends iperf3 JSON parsing structs to support UDP summary fields used by the new UDP probe.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/hhfab/testing.go | Adds TCP/UDP port probes and extends iperf3 JSON parsing to support UDP loss-based reachability checks. |
| pkg/hhfab/rt_utils.go | Adds optional ACL field to gateway peering options and plumbs it into the generated peering spec. |
| pkg/hhfab/rt_nat_tests.go | Adds helper to set per-protocol/per-port verdicts in the connectivity matrix. |
| pkg/hhfab/rt_multi_vpc_single_subnet_suite.go | Registers the new gateway peering ACL test cases into the suite. |
| pkg/hhfab/rt_acl_tests.go | New ACL release-tests covering default actions, protocol/port scoping, directionality (packet vs flow), subnet scoping, and rule precedence. |
| pkg/hhfab/matrix.go | Adds proto/port-scoped entry enumeration, listener management, and a dedicated proto/port probe phase to the matrix runner. |
| pkg/hhfab/matrix_test.go | New unit tests covering proto/port entry sorting/filtering and proto verdict overlay behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
edipascale
force-pushed
the
ema/prepare-acl-rts
branch
from
July 15, 2026 09:20
a370583 to
f2e846b
Compare
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 765-785: Update the protocol dispatch around checkPing,
checkTCPPort, and checkUDPPort to honor the individual enablement options: run
ICMP only when opts.PingsCount > 0, and run TCP/UDP only when opts.IPerfsSeconds
> 0. Apply the same guards to the corresponding listener startup paths
referenced by the review so L4 listeners are created only for enabled probes.
In `@pkg/hhfab/rt_acl_tests.go`:
- Around line 359-370: Update every ACL test registration returned by
getACLTestCases to include NoServers: true alongside NoGateway: true in
SkipFlags. Preserve the existing test functions, names, and NoGateway behavior
while ensuring all server-dependent connectivity probes are skipped for
no-server topologies.
In `@pkg/hhfab/testing.go`:
- Around line 3385-3399: Update the TCP probe logic around retrySSHCmd so deny
assertions pass only for a valid refused or timed-out connection, not SSH,
Docker, or probe execution failures; surface infrastructure errors instead. In
pkg/hhfab/testing.go lines 3385-3399, distinguish command execution errors from
expected connection failures. In pkg/hhfab/testing.go lines 3444-3489, restrict
blocked-UDP classification to validated control-channel denial, zero datagrams,
or near-total packet loss, and do not treat arbitrary command or JSON parse
errors as blocked traffic.
🪄 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: 285748a3-21ea-477b-8cab-e4093ab0429b
📒 Files selected for processing (7)
pkg/hhfab/matrix.gopkg/hhfab/matrix_test.gopkg/hhfab/rt_acl_tests.gopkg/hhfab/rt_multi_vpc_single_subnet_suite.gopkg/hhfab/rt_nat_tests.gopkg/hhfab/rt_utils.gopkg/hhfab/testing.go
edipascale
force-pushed
the
ema/prepare-acl-rts
branch
from
July 15, 2026 09:54
f2e846b to
6a90d16
Compare
edipascale
force-pushed
the
ema/multiple-conns-unbundled
branch
from
July 15, 2026 09:59
d99cb50 to
340084b
Compare
edipascale
force-pushed
the
ema/prepare-acl-rts
branch
from
July 15, 2026 09:59
6a90d16 to
e40a803
Compare
edipascale
force-pushed
the
ema/multiple-conns-unbundled
branch
from
July 22, 2026 06:46
340084b to
c430b6d
Compare
edipascale
force-pushed
the
ema/prepare-acl-rts
branch
3 times, most recently
from
July 22, 2026 10:49
37eb8f2 to
0051511
Compare
Contributor
Author
|
Tested with https://github.com/githedgehog/dataplane/pkgs/container/dataplane/1053558225?tag=v0-8d6531066 results of a vlab release-test run with regex "Gateway Peering": |
edipascale
force-pushed
the
ema/multiple-conns-unbundled
branch
from
July 28, 2026 14:43
c430b6d to
ef68a37
Compare
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>
edipascale
force-pushed
the
ema/prepare-acl-rts
branch
from
July 28, 2026 14:43
0051511 to
78897bf
Compare
edipascale
added
ci:+release
Release Tests 6 files 24 suites 3h 28m 32s ⏱️ Results for commit 78897bf. |
edipascale
marked this pull request as ready for review
July 29, 2026 06:13
Contributor
Author
|
closing in favor of #1910 |
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.
Fix #1873