Add release tests for NAT #980

pau-hedgehog · 2025-09-18T18:06:56Z

No description provided.

Copilot

Pull Request Overview

This PR adds comprehensive test coverage for NAT (Network Address Translation) functionality in gateway peering configurations. The changes extend the existing VPC peering test framework to support NAT scenarios including basic NAT peering and overlapping subnet resolution using NAT.

Extends existing gateway peering functions to accept NAT CIDR parameters
Adds two new test cases for NAT functionality with expected failure markers
Enhances test reporting infrastructure to support expected failures (XFAIL)

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

pkg/hhfab/release.go

github-actions · 2025-09-18T19:28:05Z

Test Results

13 files 52 suites 5h 19m 36s ⏱️
24 tests 23 ✅ 1 💤 0 ❌
312 runs 93 ✅ 219 💤 0 ❌

Results for commit 18a54e6.

♻️ This comment has been updated with latest results.

Copilot

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

pkg/hhfab/release.go

Copilot · 2025-09-30T20:56:44Z

pkg/hhfab/release.go

+			Subnets: map[string]*vpcapi.VPCSubnet{
+				"subnet-01": {
+					Subnet:  existingSubnetCIDR, // Same subnet as vpc-01! (10.0.1.0/24)
+					VLAN:    1020,               // Use a known available VLAN


The VLAN ID 1020 is hardcoded. Consider using a constant or dynamic allocation to avoid potential conflicts with other tests or deployments.

Copilot · 2025-09-30T20:56:44Z

pkg/hhfab/release.go

+	vpc1NATCIDR := []string{"192.168.11.0/24"}
+	vpc2NATCIDR := []string{"192.168.12.0/24"}


NAT CIDR ranges are hardcoded in multiple places (lines 2453-2454 and 2674-2675). Consider defining these as constants to ensure consistency and make them easier to modify.

Copilot · 2025-09-30T20:56:44Z

pkg/hhfab/release.go

-func appendGwPeeringSpec(gwPeerings map[string]*gwapi.PeeringSpec, vpc1, vpc2 *vpcapi.VPC, vpc1Subnets, vpc2Subnets []string) {
+// If vpc1NATCIDR or vpc2NATCIDR are provided, NAT will be configured for the respective VPC
+// NOTE: vpc1Subnets/vpc2Subnets currently always receive nil in basic tests (for future multi-subnet suite)
+func appendGwPeeringSpec(gwPeerings map[string]*gwapi.PeeringSpec, vpc1, vpc2 *vpcapi.VPC, vpc1Subnets, vpc2Subnets, vpc1NATCIDR, vpc2NATCIDR []string) { //nolint:unparam


The function signature has grown to 7 parameters, which impacts readability and maintainability. Consider using a struct parameter to group related parameters (e.g., PeeringConfig struct with VPC1Subnets, VPC2Subnets, VPC1NATCIDR, VPC2NATCIDR fields).

github-actions · 2025-11-01T12:06:39Z

Release Tests

16 files 64 suites 7h 11m 41s ⏱️
33 tests 31 ✅ 2 💤 0 ❌
528 runs 181 ✅ 347 💤 0 ❌

Results for commit d681737.

♻️ This comment has been updated with latest results.

pau-hedgehog · 2025-11-05T19:08:02Z

@qmonnet, as you may have taken a look at this PR, could you please share which additional tests would be needed to give coverage to the 1st release? Thanks

qmonnet · 2025-11-06T16:10:37Z

Let's see, I think ideally we should cover the following things:

Stateless NAT

Set up / configuration & validation
-> I think your first test is related; configuration and validation will be more a matter of dataplane and agent unit tests anyway
Source NAT
Destination NAT
Source + destination NAT
For the above, combination of ICMP / TCP / UDP, IPv4 / IPv6
-> I think at the moment you have ICMP + TCP/UDP for IPv4, for the case source + destination NAT?
Try using IPs not from exposed or NATed ranges, should result in packets being dropped

Stateful NAT

Set up / configuration & validation
-> Same as stateless, you seem to have a test for basic setup
Source NAT (we don't support destination or source + destination at the moment)
-> You cover that (with the current issues we've been investigating pending)
Source NAT + stateless destination NAT?
-> To be confirmed, need to see with Manish if we support this
For the above, combination of ICMP / TCP / UDP, IPv4 / IPv6
-> At the moment you have ICMP + TCP (iperf3) for IPv4?
Try using IPs not from exposed or NATed ranges, should be dropped
ICMP Destination unreachable with embedded packet datagram (inner ICMP / TCP / UDP, for IPv4 and IPv6)
Ideally, path MTU discovery (related to previous item)
-> Not sure that's doable before the release, plus I think Manish mentioned an issue when we send packets with the max MTU at the moment

Additional tests

Throwing in a mix of VpcExpose blocks with stateful, stateless, or not NAT together and make sure that they don't interfere with each other
-> Not sure this is doable by release time
IP overlap
-> You have this covered for stateless NAT at least
Permutations of IPs/Nots
-> IPs and Nots are converted internally into a list of IPs only in dataplane, so as long as this conversion is correct, the presence of Nots should not have a significant impact.

Note: IPv6 should work the same as IPv4 in most cases, so dedicated tests are maybe not indispensable but would be nice to have. UDP should be the same as TCP in most cases.

edipascale

Thanks @pau-hedgehog,this was very useful for testing the latest release! However I'd like to improve some things, since we have time before it can be merged.

Note that I completely ignored the new test connectivity code as that will be thrown out regardless

pkg/hhfab/release.go

edipascale · 2025-11-21T13:31:01Z

pkg/hhfab/release.go

-	// Use all subnets from both VPCs by passing empty subnet lists
-	appendGwPeeringSpec(gwPeerings, vpc1, vpc2, nil, nil)
+	// Use all subnets from both VPCs, no NAT
+	appendGwPeeringSpec(gwPeerings, vpc1, vpc2, nil, nil, nil, nil, nil)


nit: for cases like these where you have a basic use-case with only a subset of parameters and that is called in a bunch of places, I would keep the original function signature and make it a wrapper around the call to the new signature, i.e.:

func appendGwPeeringSpec(gwPeerings map[string]*gwapi.PeeringSpec, vpc1, vpc2 *vpcapi.VPC, vpc1Subnets, vpc2Subnets []string) { appendGwPeeringSpecWithNAT(gwPeerings, vpc1, vpc2, vpc1Subnets, vpc2Subnets, nil, nil, nil) } func appendGwPeeringSpecWithNAT(...) {

The struct alternative is also a valid option

pkg/hhfab/release.go

edipascale · 2025-11-21T13:53:43Z

pkg/hhfab/release.go

+	}
+
+	// Wait for NAT gateway peering to take effect
+	time.Sleep(15 * time.Second)


should we do a waitReady instead?

pkg/hhfab/release.go

add expected fail capability to release tests add Stateful NAT GW Peering test temporary until TestConnectivity updated: add function to calculate NAT offsets add function to validate NAT connectivity Signed-off-by: Pau Capdevila <[email protected]>

gatewayPeeringStatelessSourceNATTest gatewayPeeringStatelessDestinationNATTest gatewayPeeringStatelessNATWithIPExclusionTest gatewayPeeringStatelessNATWithNATPoolExclusionTest Signed-off-by: Pau Capdevila <[email protected]>

also removes redundant VPC removals Signed-off-by: Pau Capdevila <[email protected]>

Signed-off-by: Pau Capdevila <[email protected]>

pau-hedgehog requested a review from Copilot September 18, 2025 18:06

pau-hedgehog self-assigned this Sep 18, 2025

pau-hedgehog added ci:-upgrade Disable VLAB upgrade tests ci:+release Enable VLAB release tests labels Sep 18, 2025

Copilot AI reviewed Sep 18, 2025

View reviewed changes

pkg/hhfab/release.go Show resolved Hide resolved

pkg/hhfab/release.go Show resolved Hide resolved

pkg/hhfab/release.go Show resolved Hide resolved

pkg/hhfab/release.go Show resolved Hide resolved

pau-hedgehog added ci:+hlab Enable hybrid VLAB tests and removed ci:-upgrade Disable VLAB upgrade tests labels Sep 19, 2025

Frostman requested a review from Copilot September 30, 2025 20:55

Copilot AI reviewed Sep 30, 2025

View reviewed changes

pau-hedgehog force-pushed the pau/gw_nat_test branch from 18a54e6 to 0a23879 Compare November 1, 2025 08:46

pau-hedgehog force-pushed the pau/gw_nat_test branch from e7639b5 to 0ee3b2f Compare November 2, 2025 15:49

pau-hedgehog force-pushed the pau/gw_nat_test branch from 0ee3b2f to 7ed9102 Compare November 6, 2025 12:03

pau-hedgehog force-pushed the pau/gw_nat_test branch from 7ed9102 to 4555fc4 Compare November 6, 2025 18:12

qmonnet mentioned this pull request Nov 7, 2025

More bug fixes for stateful NAT githedgehog/dataplane#991

Merged

pau-hedgehog force-pushed the pau/gw_nat_test branch from e1c34d3 to 59ae446 Compare November 7, 2025 20:51

pau-hedgehog force-pushed the pau/gw_nat_test branch 3 times, most recently from 6816981 to 45a5c1a Compare November 20, 2025 14:25

edipascale requested changes Nov 21, 2025

View reviewed changes

pau-hedgehog force-pushed the pau/gw_nat_test branch from 45a5c1a to ee4a470 Compare November 26, 2025 22:53

pau-hedgehog added 3 commits November 28, 2025 09:43

chore(testing): add new NAT GW Peering tests

7dd43ca

gatewayPeeringStatelessSourceNATTest gatewayPeeringStatelessDestinationNATTest gatewayPeeringStatelessNATWithIPExclusionTest gatewayPeeringStatelessNATWithNATPoolExclusionTest Signed-off-by: Pau Capdevila <[email protected]>

chore(testing): refactor appendGwPeeringSpec

f8f6349

also removes redundant VPC removals Signed-off-by: Pau Capdevila <[email protected]>

chore(testing): add proper waitReady and use boolean for NAT mode

d681737

Signed-off-by: Pau Capdevila <[email protected]>

pau-hedgehog force-pushed the pau/gw_nat_test branch from 0492ddd to d681737 Compare November 28, 2025 14:24

		vpc1NATCIDR := []string{"192.168.11.0/24"}
		vpc2NATCIDR := []string{"192.168.12.0/24"}

Add release tests for NAT #980

Are you sure you want to change the base?

Add release tests for NAT #980

Uh oh!

Conversation

pau-hedgehog commented Sep 18, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release Tests

Uh oh!

pau-hedgehog commented Nov 5, 2025

Uh oh!

qmonnet commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Stateless NAT

Stateful NAT

Additional tests

Uh oh!

edipascale left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

edipascale Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

edipascale Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

pau-hedgehog Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Sep 18, 2025 •

edited

Loading

github-actions bot commented Nov 1, 2025 •

edited

Loading

qmonnet commented Nov 6, 2025 •

edited

Loading