Add release tests for NAT #980

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

No description provided.

Signed-off-by: Pau Capdevila <pau@githedgehog.com>

Until testing.go gets NAT support, the following tests are expected to fail: - gatewayPeeringNATTest - gatewayPeeringOverlapNATTest Signed-off-by: Pau Capdevila <pau@githedgehog.com>

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.}

Copilot · 2025-09-18T18:08:00Z

pkg/hhfab/release.go

+		return false, nil, fmt.Errorf("listing VPCs: %w", err)
+	}
+	if len(vpcs.Items) < 2 {
+		return true, nil, fmt.Errorf("not enough VPCs for NAT gateway peering test") //nolint:goerr113


Returning true (skip) with a non-nil error is inconsistent. When skipping due to insufficient resources, return true, nil, nil and log the reason instead of returning an error.

Suggested change

return true, nil, fmt.Errorf("not enough VPCs for NAT gateway peering test") //nolint:goerr113

slog.Info("Skipping NAT gateway peering test: not enough VPCs")

return true, nil, nil

Copilot · 2025-09-18T18:08:00Z

pkg/hhfab/release.go

+
+	vpcPeerings := make(map[string]*vpcapi.VPCPeeringSpec, 0)
+	externalPeerings := make(map[string]*vpcapi.ExternalPeeringSpec, 0)
+	gwPeerings := make(map[string]*gwapi.PeeringSpec, 1)


[nitpick] The capacity parameter for map initialization should be consistent. Either use 0 for all maps or use meaningful capacities based on expected usage. Mixed capacities (0, 0, 1) suggest inconsistent initialization patterns.

Suggested change

gwPeerings := make(map[string]*gwapi.PeeringSpec, 1)

gwPeerings := make(map[string]*gwapi.PeeringSpec, 0)

Copilot · 2025-09-18T18:08:01Z

pkg/hhfab/release.go

+	// Set up NAT gateway peering between the overlapping VPCs
+	vpcPeerings := make(map[string]*vpcapi.VPCPeeringSpec, 0)
+	externalPeerings := make(map[string]*vpcapi.ExternalPeeringSpec, 0)
+	gwPeerings := make(map[string]*gwapi.PeeringSpec, 1)


[nitpick] Same inconsistent map capacity initialization pattern as in the previous test function. Consider standardizing the capacity values across all test functions.

Suggested change

gwPeerings := make(map[string]*gwapi.PeeringSpec, 1)

gwPeerings := make(map[string]*gwapi.PeeringSpec, 0)

Copilot · 2025-09-18T18:08:01Z

pkg/hhfab/release.go

+	time.Sleep(5 * time.Second)
+
+	// Create new overlapping VPC with vpc-01's subnet CIDR in the separate namespace
+	newVPCName := "vpc-ovrlp" // VPC names must be ≤11 chars because they map to Linux VRF interface names


[nitpick] The comment explains the 11-character limit but 'vpc-ovrlp' is only 9 characters. Consider clarifying whether this is the actual limit or if there's additional context needed for the constraint.

Suggested change

newVPCName := "vpc-ovrlp" // VPC names must be ≤11 chars because they map to Linux VRF interface names

newVPCName := "vpc-ovrlp" // Example name is 9 chars (≤11 char limit for VPC names, as they map to Linux VRF interface names)

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.}

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

pkg/hhfab/release.go

+	time.Sleep(5 * time.Second)
+
+	// Create new overlapping VPC with vpc-01's subnet CIDR in the separate namespace
+	newVPCName := "vpc-ovrlp" // VPC names must be ≤11 chars because they map to Linux VRF interface names


The magic number 11 is hardcoded without clear validation. Consider adding a constant or validation function to ensure VPC names don't exceed the VRF interface name limit.

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).

pau-hedgehog added 3 commits September 17, 2025 21:56

chore(testing): add NAT GW Peering tests

bc62e41

Signed-off-by: Pau Capdevila <pau@githedgehog.com>

tbs

bf20562

Signed-off-by: Pau Capdevila <pau@githedgehog.com>

chore(testing): add skip fail capability to release tests

18a54e6

Until testing.go gets NAT support, the following tests are expected to fail: - gatewayPeeringNATTest - gatewayPeeringOverlapNATTest Signed-off-by: Pau Capdevila <pau@githedgehog.com>

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add release tests for NAT #980

Add release tests for NAT #980

Uh oh!

pau-hedgehog commented Sep 18, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Sep 18, 2025

Uh oh!

Copilot AI Sep 18, 2025

Uh oh!

Copilot AI Sep 18, 2025

Uh oh!

Copilot AI Sep 18, 2025

Uh oh!

github-actions bot commented Sep 18, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Sep 30, 2025

Uh oh!

Copilot AI Sep 30, 2025

Uh oh!

Copilot AI Sep 30, 2025

Uh oh!

Copilot AI Sep 30, 2025

Uh oh!

Uh oh!

	return true, nil, fmt.Errorf("not enough VPCs for NAT gateway peering test") //nolint:goerr113
	slog.Info("Skipping NAT gateway peering test: not enough VPCs")
	return true, nil, nil

	gwPeerings := make(map[string]*gwapi.PeeringSpec, 1)
	gwPeerings := make(map[string]*gwapi.PeeringSpec, 0)

	newVPCName := "vpc-ovrlp" // VPC names must be ≤11 chars because they map to Linux VRF interface names
	newVPCName := "vpc-ovrlp" // Example name is 9 chars (≤11 char limit for VPC names, as they map to Linux VRF interface names)

		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!

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

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!

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!

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Sep 18, 2025 •

edited

Loading