dynamic port allocation for cloud nat #5784

rabun788 · 2022-03-03T20:32:06Z

Using dynamic port allocation lets the NAT gateway allocate different numbers of ports to each VM based on usage.

fixes hashicorp/terraform-provider-google#11052

If this PR is for Terraform, I acknowledge that I have:

Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

compute: supported `enable_dynamic_port_allocation` in `google_compute_router_nat` to let the NAT gateway allocate different numbers of ports to each VM based on usage.

Original commentary: hashicorp/terraform-provider-google#11214

Using dynamic port allocation lets the NAT gateway allocate different numbers of ports to each VM based on usage.

google-cla · 2022-03-03T20:32:10Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

modular-magician · 2022-03-03T20:32:22Z

Oops! It looks like you're using an unknown release-note type in your changelog entries:

REPLACEME

Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md.

modular-magician · 2022-03-03T20:33:05Z

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review.

Thanks for your contribution! A human will be with you soon.

@shuyama1, please review this PR or find an appropriate assignee.

modular-magician · 2022-03-03T20:45:27Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 35 insertions(+))
Terraform Beta: Diff ( 2 files changed, 35 insertions(+))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

rabun788 · 2022-03-03T20:51:22Z

mention: hashicorp/terraform-provider-google#11052

modular-magician · 2022-03-03T21:01:27Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 35 insertions(+))
Terraform Beta: Diff ( 2 files changed, 35 insertions(+))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

shuyama1

Hi @rabun788 Thanks for your contribution! Would you mind adding a new test/ modifying an existing test to test this field, as we try to cover all the features tested. Please let me know if you have questions! Also, please re-request my review after you're done adding the test. Many thanks!

modular-magician · 2022-03-07T18:54:14Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 121 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 3 files changed, 121 insertions(+), 1 deletion(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

shuyama1 · 2022-03-07T19:09:35Z

/gcbrun

modular-magician · 2022-03-07T19:22:26Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 121 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 3 files changed, 121 insertions(+), 1 deletion(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2022-03-07T21:13:56Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDataSourceSqlDatabaseInstance_basic|TestAccBigqueryReservationAssignment_BasicHandWritten|TestAccCGCSnippet_sqlMysqlInstanceBackupExample|TestAccCGCSnippet_sqlPostgresInstanceBackupExample|TestAccCGCSnippet_sqlSqlserverInstanceBackupExample|TestAccCGCSnippet_sqlSqlserverInstanceAuthorizedNetworkExample|TestAccCloudBuildTrigger_cloudbuildTriggerPubsubConfigExample|TestAccCloudBuildTrigger_cloudbuildTriggerWebhookConfigExample|TestAccCloudBuildTrigger_cloudbuildTriggerManualExample|TestAccCloudBuildTrigger_cloudbuildTriggerFilenameExample|TestAccCloudBuildTrigger_cloudbuildTriggerBuildExample|TestAccCloudBuildTrigger_basic|TestAccCloudBuildTrigger_available_secrets_config|TestAccCloudBuildTrigger_cloudbuildTriggerServiceAccountExample|TestAccCloudBuildTrigger_disable|TestAccCloudBuildTrigger_fullStep|TestAccCloudFunctionsFunction_secretEnvVar|TestAccComputeRouterNat_withDisabledDynamicPortAllocation|TestAccComputeRouterNat_basic|TestAccComputeRouterNat_update|TestAccComputeRouterNat_removeLogConfig|TestAccComputeRouterNat_withManualIpAndSubnetConfiguration|TestAccComputeRouterNat_withDisabledIndependentEndpointMapping|TestAccComputeRouterNat_withNatIpsAndDrainNatIps|TestAccContainerCluster_withAuthenticatorGroupsConfig|TestAccDataprocCluster_nonPreemptibleSecondary|TestAccDataprocCluster_updatable|TestAccDataprocCluster_withConfigOverrides|TestAccServiceNetworkingPeeredDNSDomain_basic|TestAccVPCAccessConnector_cloudrunVPCAccessConnectorExample You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=265417

mmv1/third_party/terraform/tests/resource_compute_router_nat_test.go.erb

mmv1/products/compute/api.yaml

mmv1/third_party/terraform/tests/resource_compute_router_nat_test.go.erb

mmv1/products/compute/api.yaml

FalconerTC · 2022-03-21T20:19:48Z

When Dynamic Port Allocation is enabled for a NAT router, you can also configure the maximum ports per VM. Can something like max_ports_per_vm be added as part of this?

modular-magician · 2022-03-22T14:20:00Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 112 insertions(+))
Terraform Beta: Diff ( 3 files changed, 112 insertions(+))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

updating

…into patch-14

modular-magician · 2022-03-22T14:41:55Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 107 insertions(+))
Terraform Beta: Diff ( 3 files changed, 107 insertions(+))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2022-03-22T15:36:53Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 153 insertions(+))
Terraform Beta: Diff ( 3 files changed, 153 insertions(+))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

shuyama1 · 2022-03-22T16:21:43Z

/gcbrun

modular-magician · 2022-03-22T16:40:42Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 153 insertions(+))
Terraform Beta: Diff ( 3 files changed, 153 insertions(+))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

shuyama1 · 2022-03-22T19:56:48Z

/gcbrun

modular-magician · 2022-03-22T20:10:58Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 153 insertions(+))
Terraform Beta: Diff ( 3 files changed, 153 insertions(+))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

shuyama1 · 2022-03-23T18:01:40Z

/gcbrun

modular-magician · 2022-03-23T18:16:34Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 153 insertions(+))
Terraform Beta: Diff ( 3 files changed, 153 insertions(+))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

shuyama1 · 2022-03-24T16:58:36Z

mmv1/products/compute/api.yaml

@@ -13355,6 +13355,10 @@ objects:
        name: minPortsPerVm
        description: |
          Minimum number of ports allocated to a VM from this NAT.
+      - !ruby/object:Api::Type::Integer


Would you mind adding a test on this? Simply adding this field in the existing test, such as TestAccComputeRouterNat_withDisabledDynamicPortAllocation, would be fine.

shuyama1 · 2022-03-24T16:59:53Z

mmv1/third_party/terraform/tests/resource_compute_router_nat_test.go.erb

+  })
+}
+
+func TestAccComputeRouterNat_withDisabledDynamicPortAllocation(t *testing.T) {


Shall we also add an update step for this test. We can simply follow the pattern of TestAccComputeRouterNat_withDisabledIndependentEndpointMapping

shuyama1 · 2022-03-24T17:23:35Z

mmv1/third_party/terraform/tests/resource_compute_router_nat_test.go.erb

@@ -2,343 +2,366 @@
 package google

 import (
-	"fmt"
+  "fmt"


nit: the spacing in this resource is tabbed rather than 2 spaces

inkel

I really hope this gets merged and released soon!

inkel · 2022-04-06T15:07:40Z

mmv1/third_party/terraform/tests/resource_compute_router_nat_test.go.erb

+func testAccComputeRouterNatWithDisabledDynamicPortAllocation(routerName string, disabled bool, enabled bool) string {
+  return fmt.Sprintf(`
+resource "google_compute_network" "foobar" {
+  name                    = "%s-net"
+  auto_create_subnetworks = "false"
+}
+
+resource "google_compute_subnetwork" "foobar" {
+  name          = "%s-subnet"
+  network       = google_compute_network.foobar.self_link
+  ip_cidr_range = "10.0.0.0/16"
+  region        = "us-central1"
+}
+
+resource "google_compute_address" "foobar" {
+  name   = "router-nat-%s-addr"
+  region = google_compute_subnetwork.foobar.region
+}
+
+resource "google_compute_router" "foobar" {
+  name    = "%s"
+  region  = google_compute_subnetwork.foobar.region
+  network = google_compute_network.foobar.self_link
+  bgp {
+    asn = 64514
+  }
+}
+
+resource "google_compute_router_nat" "foobar" {
+  name                               = "%s"
+  router                             = google_compute_router.foobar.name
+  region                             = google_compute_router.foobar.region
+  nat_ip_allocate_option             = "MANUAL_ONLY"
+  nat_ips                            = [google_compute_address.foobar.self_link]
+  source_subnetwork_ip_ranges_to_nat = "LIST_OF_SUBNETWORKS"
+  subnetwork {
+    name                    = google_compute_subnetwork.foobar.name
+    source_ip_ranges_to_nat = ["ALL_IP_RANGES"]
+  }
+  enable_endpoint_independent_mapping = %t
+  enable_dynamic_port_allocation      = %t
+}
+`, routerName, routerName, routerName, routerName, routerName, disabled, enabled)


You can use explicit argument indexes instead and reduce the repetition:

Suggested change

func testAccComputeRouterNatWithDisabledDynamicPortAllocation(routerName string, disabled bool, enabled bool) string {

return fmt.Sprintf(`

resource "google_compute_network" "foobar" {

name = "%s-net"

auto_create_subnetworks = "false"

}

resource "google_compute_subnetwork" "foobar" {

name = "%s-subnet"

network = google_compute_network.foobar.self_link

ip_cidr_range = "10.0.0.0/16"

region = "us-central1"

}

resource "google_compute_address" "foobar" {

name = "router-nat-%s-addr"

region = google_compute_subnetwork.foobar.region

}

resource "google_compute_router" "foobar" {

name = "%s"

region = google_compute_subnetwork.foobar.region

network = google_compute_network.foobar.self_link

bgp {

asn = 64514

}

}

resource "google_compute_router_nat" "foobar" {

name = "%s"

router = google_compute_router.foobar.name

region = google_compute_router.foobar.region

nat_ip_allocate_option = "MANUAL_ONLY"

nat_ips = [google_compute_address.foobar.self_link]

source_subnetwork_ip_ranges_to_nat = "LIST_OF_SUBNETWORKS"

subnetwork {

name = google_compute_subnetwork.foobar.name

source_ip_ranges_to_nat = ["ALL_IP_RANGES"]

}

enable_endpoint_independent_mapping = %t

enable_dynamic_port_allocation = %t

}

`, routerName, routerName, routerName, routerName, routerName, disabled, enabled)

func testAccComputeRouterNatWithDisabledDynamicPortAllocation(routerName string, disabled bool, enabled bool) string {

return fmt.Sprintf(`

resource "google_compute_network" "foobar" {

name = "%[1]s-net"

auto_create_subnetworks = "false"

}

resource "google_compute_subnetwork" "foobar" {

name = "%[1]s-subnet"

network = google_compute_network.foobar.self_link

ip_cidr_range = "10.0.0.0/16"

region = "us-central1"

}

resource "google_compute_address" "foobar" {

name = "router-nat-%[1]s-addr"

region = google_compute_subnetwork.foobar.region

}

resource "google_compute_router" "foobar" {

name = "%[1]s"

region = google_compute_subnetwork.foobar.region

network = google_compute_network.foobar.self_link

bgp {

asn = 64514

}

}

resource "google_compute_router_nat" "foobar" {

name = "%[1]s"

router = google_compute_router.foobar.name

region = google_compute_router.foobar.region

nat_ip_allocate_option = "MANUAL_ONLY"

nat_ips = [google_compute_address.foobar.self_link]

source_subnetwork_ip_ranges_to_nat = "LIST_OF_SUBNETWORKS"

subnetwork {

name = google_compute_subnetwork.foobar.name

source_ip_ranges_to_nat = ["ALL_IP_RANGES"]

}

enable_endpoint_independent_mapping = %t

enable_dynamic_port_allocation = %t

}

`, routerName, disabled, enabled)

Note that the %t placeholders do not use an index, as they will be indexed as one would expect. See this example.

melinath · 2022-06-22T15:26:36Z

I think this is now obsolete - this functionality has been implemented in the provider at this point.

dynamic port allocation for cloud nat

d1a0da3

Using dynamic port allocation lets the NAT gateway allocate different numbers of ports to each VM based on usage.

modular-magician requested a review from shuyama1 March 3, 2022 20:33

rabun788 closed this Mar 3, 2022

rabun788 reopened this Mar 3, 2022

shuyama1 requested changes Mar 7, 2022

View reviewed changes

rabun788 and others added 3 commits March 7, 2022 13:19

adding test for enableDynamicPortAllocation

dc40e68

Update resource_compute_router_nat_test.go.erb

b11308d

Update resource_compute_router_nat_test.go.erb

4d78a23

rabun788 requested a review from shuyama1 March 7, 2022 18:54

shuyama1 requested changes Mar 8, 2022

View reviewed changes

shuyama1 reviewed Mar 8, 2022

View reviewed changes

mmv1/products/compute/api.yaml Outdated Show resolved Hide resolved

rabun788 added 4 commits March 22, 2022 09:58

Update api.yaml

835bf77

Update api.yaml

4d4cd78

Update resource_compute_router_nat_test.go.erb

2648d86

Update resource_compute_router_nat_test.go.erb

b08c1e1

rabun788 added 3 commits March 22, 2022 10:21

Merge pull request #1 from GoogleCloudPlatform/main

ee96d16

updating

Merge pull request #3 from rabun788/main

0b9893f

updating

Update api.yaml

c806950

rabun788 added 2 commits March 22, 2022 10:32

Merge branch 'patch-14' of https://github.com/rabun788/magic-modules …

fb532b7

…into patch-14

adding disabled for test

e6d797c

adding max ports per vm

884800a

rabun788 requested a review from shuyama1 March 22, 2022 15:53

shuyama1 requested changes Mar 24, 2022

View reviewed changes

inkel reviewed Apr 6, 2022

View reviewed changes

melinath closed this Jun 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dynamic port allocation for cloud nat #5784

dynamic port allocation for cloud nat #5784

rabun788 commented Mar 3, 2022 •

edited by shuyama1

Loading

google-cla bot commented Mar 3, 2022

modular-magician commented Mar 3, 2022

modular-magician commented Mar 3, 2022

modular-magician commented Mar 3, 2022

rabun788 commented Mar 3, 2022

modular-magician commented Mar 3, 2022

shuyama1 left a comment

modular-magician commented Mar 7, 2022

shuyama1 commented Mar 7, 2022

modular-magician commented Mar 7, 2022

modular-magician commented Mar 7, 2022

FalconerTC commented Mar 21, 2022

modular-magician commented Mar 22, 2022

modular-magician commented Mar 22, 2022

modular-magician commented Mar 22, 2022

shuyama1 commented Mar 22, 2022

modular-magician commented Mar 22, 2022

shuyama1 commented Mar 22, 2022

modular-magician commented Mar 22, 2022

shuyama1 commented Mar 23, 2022

modular-magician commented Mar 23, 2022

shuyama1 Mar 24, 2022

shuyama1 Mar 24, 2022

shuyama1 Mar 24, 2022

inkel left a comment

inkel Apr 6, 2022

melinath commented Jun 22, 2022

dynamic port allocation for cloud nat #5784

dynamic port allocation for cloud nat #5784

Conversation

rabun788 commented Mar 3, 2022 • edited by shuyama1 Loading

google-cla bot commented Mar 3, 2022

modular-magician commented Mar 3, 2022

modular-magician commented Mar 3, 2022

modular-magician commented Mar 3, 2022

Diff report:

rabun788 commented Mar 3, 2022

modular-magician commented Mar 3, 2022

Diff report:

shuyama1 left a comment

Choose a reason for hiding this comment

modular-magician commented Mar 7, 2022

Diff report:

shuyama1 commented Mar 7, 2022

modular-magician commented Mar 7, 2022

Diff report:

modular-magician commented Mar 7, 2022

FalconerTC commented Mar 21, 2022

modular-magician commented Mar 22, 2022

Diff report:

modular-magician commented Mar 22, 2022

Diff report:

modular-magician commented Mar 22, 2022

Diff report:

shuyama1 commented Mar 22, 2022

modular-magician commented Mar 22, 2022

Diff report:

shuyama1 commented Mar 22, 2022

modular-magician commented Mar 22, 2022

Diff report:

shuyama1 commented Mar 23, 2022

modular-magician commented Mar 23, 2022

Diff report:

shuyama1 Mar 24, 2022

Choose a reason for hiding this comment

shuyama1 Mar 24, 2022

Choose a reason for hiding this comment

shuyama1 Mar 24, 2022

Choose a reason for hiding this comment

inkel left a comment

Choose a reason for hiding this comment

inkel Apr 6, 2022

Choose a reason for hiding this comment

melinath commented Jun 22, 2022

rabun788 commented Mar 3, 2022 •

edited by shuyama1

Loading