Add `--create-nat` gke-deployer flag #307

Qqkyu · 2025-10-21T13:48:40Z

Added logic similar to the kubetest(1) --gke-create-nat flag (crucial for tests running in private clusters).

k8s-ci-robot · 2025-10-21T13:48:50Z

Hi @Qqkyu. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Qqkyu · 2025-10-21T13:50:47Z

cc: @BenTheElder @aojea

k8s-triage-robot · 2025-10-21T15:53:55Z

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

aojea · 2025-10-22T05:33:42Z

kubetest2-gke/deployer/down.go

+	if errNat != nil {
+		return errNat
 	}
-	if err := d.DeleteSubnets(d.retryCount); err != nil {
-		return err
+	if errNetwork != nil {
+		return errNetwork
+	}
+	if errSubnets != nil {
+		return errSubnets
 	}


you can now aggregate in the std library
errs := errors.Join(errNat, errNetwork, errSubnets)
if errs != nil {
return errs
}

thanks, done

aojea · 2025-10-22T05:34:31Z

/ok-to-test

aojea · 2025-10-28T16:58:31Z

kubetest2-gke/deployer/nat.go

+	}
+	region := regionFromLocation(d.Regions, d.Zones, d.retryCount)
+	nat := d.getNatName()
+	hostProject := d.Projects[0]


is it always in the first project?

This is a common assumption throughout the codebase, for example in firewall.go ensureFirewallRulesForMultiProjects:

func (d *Deployer) ensureFirewallRulesForMultiProjects() error { hostProject := d.Projects[0] ...

But also in multiple places in network.go

aojea · 2025-10-28T17:08:16Z

kubetest2-gke/deployer/down.go

+	errNat := d.CleanupNat()
+	errNetwork := d.TeardownNetwork()
+	errSubnets := d.DeleteSubnets(d.retryCount)


keep the check on the error to just logging the error as above, that is useful

aojea · 2025-10-29T10:14:57Z

better to have a clean rebase without the merge commit, there are some comments also , then it lgtm

aojea · 2025-10-29T13:28:47Z

kubetest2-gke/deployer/down.go

+	if err := d.CleanupNat(); err != nil {
+		klog.Errorf("Error cleaning-up nat: %v", err)
+	}
+
 	if err := d.TeardownNetwork(); err != nil {
-		return err
+		klog.Errorf("Error tearing-down network: %v", err)
 	}
+
 	if err := d.DeleteSubnets(d.retryCount); err != nil {
-		return err
+		klog.Errorf("Error deleting subnets: %v", err)
 	}
+
 	return d.DeleteNetwork()


don't we need to aggregate the errors and return the final error?
I suggested to log but does not affect if we swallow the error?

Yeah, I misinterpreted previous comment. Changed it so that we log and aggregate errors now. Does this work?

aojea · 2025-10-29T14:37:02Z

kubetest2-gke/deployer/nat.go

+	if d.Network == "default" {
+		return fmt.Errorf("NAT router should be set manually for the default network")


why is this?

So my understanding is that using custom networks for test environments is best practice as it provides isolation and control over resources, like nat and firewalls (reason why in EnsureFirewallRules we've got the same check). Cleaning up test-specific resources on the default network could become difficult/messy

the EnsureFirewallRules has

// Do not modify the firewall rules for the default network if d.Network == "default" { return nil }

so, it is better to fail on validation of the flags that at runtime, specially for these options.

Do we have more clear the use case for enabling the nat creation or are we just trying to map 1 to 1 options?

Qqkyu · 2025-10-30T10:29:44Z

/retest

Qqkyu · 2025-11-07T10:46:27Z

/approve

k8s-ci-robot · 2025-11-07T10:46:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Qqkyu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~kubetest2-gke/OWNERS~~ [Qqkyu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Qqkyu · 2025-11-07T10:47:44Z

/lgtm

k8s-ci-robot · 2025-11-07T10:47:46Z

@Qqkyu: you cannot LGTM your own PR.

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Qqkyu · 2025-11-07T10:48:40Z

@aojea any other comments/suggestions? If not, can you give an lgtm here and we can merge this?

Qqkyu · 2025-12-08T10:26:24Z

Bump @aojea @upodroid

k8s-ci-robot requested review from aojea and upodroid October 21, 2025 13:48

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 21, 2025

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 21, 2025

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 21, 2025

aojea reviewed Oct 22, 2025

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 22, 2025

aojea reviewed Oct 28, 2025

View reviewed changes

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 28, 2025

aojea reviewed Oct 29, 2025

View reviewed changes

Qqkyu mentioned this pull request Nov 4, 2025

REQUEST: New membership for Qqkyu kubernetes/org#5969

Closed

11 tasks

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2025

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 13, 2025

Qqkyu added 2 commits November 13, 2025 08:13

Add --create-nat flag

cf9f1e6

Update copyright year

e903782

Qqkyu added 4 commits November 13, 2025 08:13

aggregate errors

cbc3658

Log errors in Down() instead of returning

e4f9e89

Remove unused import

4e79900

Log and aggregate errors

2ee9260

Qqkyu force-pushed the gke-deployer-add-create-nat-flag branch from c4847c0 to 2ee9260 Compare November 13, 2025 08:14

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 13, 2025

		if d.Network == "default" {
		return fmt.Errorf("NAT router should be set manually for the default network")

Add --create-nat gke-deployer flag #307

Are you sure you want to change the base?

Add --create-nat gke-deployer flag #307

Conversation

Qqkyu commented Oct 21, 2025

Uh oh!

k8s-ci-robot commented Oct 21, 2025

Uh oh!

Qqkyu commented Oct 21, 2025

Uh oh!

k8s-triage-robot commented Oct 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aojea commented Oct 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aojea commented Oct 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Qqkyu commented Oct 30, 2025

Uh oh!

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

Uh oh!

Uh oh!

k8s-ci-robot commented Nov 7, 2025

Uh oh!

Qqkyu commented Nov 7, 2025

Uh oh!

k8s-ci-robot commented Nov 7, 2025

Uh oh!

Qqkyu commented Nov 7, 2025

Uh oh!

Qqkyu commented Dec 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add `--create-nat` gke-deployer flag #307

Add `--create-nat` gke-deployer flag #307

Qqkyu commented Nov 7, 2025 •

edited

Loading