feat(removes nat_ip_allocate_option)!: add dynamic port mapping #73

nikhilmakhijani · 2022-06-21T10:32:46Z

This MR includes the following changes:

Adding dynamic port mapping functionality
Remove nat_ip_allocation_option variable

github-actions · 2022-08-20T23:10:29Z

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days

nikhilmakhijani · 2022-10-19T18:19:49Z

Please review @bharathkkb

alexander-verharen-qc · 2022-11-22T18:08:38Z

main.tf

@@ -57,6 +55,7 @@ resource "google_compute_router_nat" "main" {
  tcp_established_idle_timeout_sec    = var.tcp_established_idle_timeout_sec
  tcp_transitory_idle_timeout_sec     = var.tcp_transitory_idle_timeout_sec
  enable_endpoint_independent_mapping = var.enable_endpoint_independent_mapping
+  enable_dynamic_port_allocation      = var.enable_dynamic_port_allocation


This could be conditional, since the previous variable should be false if this one is true.

versions.tf

bharathkkb

Sorry for the delay @nikhilmakhijani

versions.tf

bharathkkb · 2022-11-29T02:19:36Z

variables.tf

 variable "enable_endpoint_independent_mapping" {
  type        = bool
  description = "Specifies if endpoint independent mapping is enabled."
-  default     = null
+  default     = true


Any reason for changing this default?

It is enabled by default as per the terraform documentation so I made it true here.

This is not enabled by default in Google Cloud, see https://cloud.google.com/nat/docs/overview#specs-rfcs

By default, Endpoint-Independent Mapping is disabled when you create a NAT gateway.

The terraform documentation should probably be updated to match GCP and this set to false as default.

@jcalero I took a peek at the provider and it seems like they actually default to true. It might be worth opening an issue there to point out this difference. @nikhilmakhijani Could we keep this a null for now so we can follow the provider default?

https://github.com/hashicorp/terraform-provider-google/blob/53295dc35039ca8cd66da18cc758bf7487bd8c03/google/resource_compute_router_nat.go#L238-L243

Yes, I have updated it to null for now. Can we check whether it should be enabled or disabled as per best practice then update it accordingly?

It's worth noting that Endpoint-Independent Mapping (EIM) will be disabled by default in future Terraform provider versions in order to be in sync with the Google default setting:

hashicorp/terraform-provider-google#10609

Speaking personally, I would definitely say the best practice for 99% of all deployments is to disable EIM but do enable Dynamic Port Allocation (DPA) as this will manage port allocations more efficiently and decrease the chance of Cloud NAT drops.

At a bare minimum, it should be an option to leverage DPA as the feature has been out over a year and I've yet to see any downside to using it.

I believe when it is set to null it still ends up where is EIM is enabled. This is why I've opened a separate PR to set the value to false #113

…/terraform-google-cloud-nat into fix/dynamic_port

comment-bot-dev · 2022-12-02T06:33:07Z

@nikhilmakhijani
Thanks for the PR! 🚀
✅ Lint checks have passed.

github-actions · 2023-02-01T23:09:56Z

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days

apeabody · 2023-03-24T16:05:05Z

/gcbrun

apeabody · 2023-03-24T16:08:38Z

Thanks for the contribution @nikhilmakhijani! I've triggered the CI, in the meantime can all the remaining comment threads be closed?

apeabody · 2023-03-24T16:23:39Z

Hi @nikhilmakhijani, relevant output from the INT cI:

    subnetworks_test.go:45: 
        	Error Trace:	/workspace/test/integration/subnetworks/subnetworks_test.go:45
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/blueprint-test@v0.5.0/pkg/tft/terraform.go:412
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/blueprint-test@v0.5.0/pkg/tft/terraform.go:432
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/blueprint-test@v0.5.0/pkg/utils/stages.go:31
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/blueprint-test@v0.5.0/pkg/tft/terraform.go:432
        	Error:      	Not equal: 
        	            	expected: map[string]interface {}{"enableEndpointIndependentMapping":true, "endpointTypes":[]interface {}{"ENDPOINT_TYPE_VM"}, "icmpIdleTimeoutSec":15, "minPortsPerVm":128, "name":"my-cloud-nat-cft-cloud-nat-test-RANDOM_ID", "natIpAllocateOption":"MANUAL_ONLY", "sourceSubnetworkIpRangesToNat":"LIST_OF_SUBNETWORKS", "subnetworks":[]interface {}{map[string]interface {}{"name":"https://www.googleapis.com/compute/v1/projects/PROJECT_ID/regions/us-east4/subnetworks/cft-cloud-nat-test-RANDOM_ID-a", "sourceIpRangesToNat":[]interface {}{"ALL_IP_RANGES"}}}, "tcpEstablishedIdleTimeoutSec":600, "tcpTimeWaitTimeoutSec":120, "tcpTransitoryIdleTimeoutSec":15, "udpIdleTimeoutSec":15}
        	            	actual  : map[string]interface {}{"enableDynamicPortAllocation":false, "enableEndpointIndependentMapping":true, "endpointTypes":[]interface {}{"ENDPOINT_TYPE_VM"}, "icmpIdleTimeoutSec":15, "minPortsPerVm":128, "name":"my-cloud-nat-cft-cloud-nat-test-RANDOM_ID", "natIpAllocateOption":"MANUAL_ONLY", "sourceSubnetworkIpRangesToNat":"LIST_OF_SUBNETWORKS", "subnetworks":[]interface {}{map[string]interface {}{"name":"https://www.googleapis.com/compute/v1/projects/PROJECT_ID/regions/us-east4/subnetworks/cft-cloud-nat-test-RANDOM_ID-a", "sourceIpRangesToNat":[]interface {}{"ALL_IP_RANGES"}}}, "tcpEstablishedIdleTimeoutSec":600, "tcpTimeWaitTimeoutSec":120, "tcpTransitoryIdleTimeoutSec":15, "udpIdleTimeoutSec":15}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,2 +1,3 @@
        	            	-(map[string]interface {}) (len=12) {
        	            	+(map[string]interface {}) (len=13) {
        	            	+ (string) (len=27) "enableDynamicPortAllocation": (bool) false,
        	            	  (string) (len=32) "enableEndpointIndependentMapping": (bool) true,
        	Test:       	TestSubnetworks

apeabody · 2023-04-03T21:36:55Z

/gcbrun

apeabody · 2023-04-04T16:28:09Z

/gcbrun

sclausson · 2023-05-11T08:15:53Z

@apeabody any updates on this one? Would be great to be able to add dynamic port mapping to the module.

apeabody · 2023-05-11T15:51:52Z

/gcbrun

apeabody · 2023-05-11T15:53:21Z

@apeabody any updates on this one? Would be great to be able to add dynamic port mapping to the module.

Hi @sclausson - I've retriggered the CI, but I suspect this is still relevant #73 (comment)

nikhilmakhijani · 2023-05-11T16:17:24Z

/gcbrun

nikhilmakhijani · 2023-05-11T16:21:23Z

@apeabody any updates on this one? Would be great to be able to add dynamic port mapping to the module.

Hi @sclausson - I've retriggered the CI, but I suspect this is still relevant #73 (comment)

Hi @apeabody I have updated the test case. Could you please check again?

apeabody · 2023-05-11T16:24:53Z

/gcbrun

apeabody · 2023-05-11T17:11:27Z

Hi @nikhilmakhijani here is the current INT output:

TestAdvanced 2023-05-11T16:34:02Z command.go:185: "cloud-foundation-cicd"
    advanced_test.go:45: 
        	Error Trace:	/workspace/test/integration/advanced/advanced_test.go:45
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/blueprint-test@v0.5.1/pkg/tft/terraform.go:408
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/blueprint-test@v0.5.1/pkg/tft/terraform.go:428
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/blueprint-test@v0.5.1/pkg/utils/stages.go:31
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/blueprint-test@v0.5.1/pkg/tft/terraform.go:428
        	Error:      	Not equal: 
        	            	expected: map[string]interface {}{"enableEndpointIndependentMapping":true, "endpointTypes":[]interface {}{"ENDPOINT_TYPE_VM"}, "icmpIdleTimeoutSec":15, "minPortsPerVm":128, "name":"my-cloud-nat-cft-cloud-nat-test-RANDOM_ID", "natIpAllocateOption":"MANUAL_ONLY", "sourceSubnetworkIpRangesToNat":"ALL_SUBNETWORKS_ALL_IP_RANGES", "tcpEstablishedIdleTimeoutSec":600, "tcpTimeWaitTimeoutSec":240, "tcpTransitoryIdleTimeoutSec":15, "udpIdleTimeoutSec":15}
        	            	actual  : map[string]interface {}{"enableDynamicPortAllocation":false, "enableEndpointIndependentMapping":true, "endpointTypes":[]interface {}{"ENDPOINT_TYPE_VM"}, "icmpIdleTimeoutSec":15, "minPortsPerVm":128, "name":"my-cloud-nat-cft-cloud-nat-test-RANDOM_ID", "natIpAllocateOption":"MANUAL_ONLY", "sourceSubnetworkIpRangesToNat":"ALL_SUBNETWORKS_ALL_IP_RANGES", "tcpEstablishedIdleTimeoutSec":600, "tcpTimeWaitTimeoutSec":240, "tcpTransitoryIdleTimeoutSec":15, "udpIdleTimeoutSec":15}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,2 +1,3 @@
        	            	-(map[string]interface {}) (len=11) {
        	            	+(map[string]interface {}) (len=12) {
        	            	+ (string) (len=27) "enableDynamicPortAllocation": (bool) false,
        	            	  (string) (len=32) "enableEndpointIndependentMapping": (bool) true,
        	Test:       	TestAdvanced
        	```

apeabody · 2023-05-11T17:13:56Z

/gcbrun

apeabody

Thanks @nikhilmakhijani - a few final nits.

main.tf

outputs.tf

variables.tf

versions.tf

variables.tf

apeabody · 2023-05-11T18:05:38Z

/gcbrun

Nikhil Makhijani and others added 3 commits May 21, 2022 20:50

fix: add variable type in variables.tf

f1aa365

fix: update readme.md variables for router,project & region

40f4af5

Merge branch 'terraform-google-modules:master' into master

3f5e0fc

nikhilmakhijani force-pushed the fix/dynamic_port branch from 51a3fea to 9e2b99d Compare June 21, 2022 10:36

feat: enable dynamic port allocation

902036f

nikhilmakhijani force-pushed the fix/dynamic_port branch from 9e2b99d to 902036f Compare June 21, 2022 10:41

github-actions bot added the Stale label Aug 20, 2022

Merge branch 'master' into fix/dynamic_port

3992842

nikhilmakhijani changed the title ~~Fix/dynamic port~~ feat: add dynamic port mapping Aug 27, 2022

github-actions bot removed the Stale label Aug 27, 2022

Nikhil Makhijani added 2 commits August 29, 2022 15:37

fix: remove nat_ip_allocation_option variable

3f03a1a

docs: update readme file

92a1d94

nikhilmakhijani mentioned this pull request Aug 29, 2022

Support Dynamic Port Allocation #64

Closed

alexander-verharen-qc suggested changes Nov 22, 2022

View reviewed changes

bharathkkb reviewed Nov 29, 2022

View reviewed changes

Nikhil Makhijani added 2 commits November 29, 2022 05:02

fix: bumping min version to 4.27

74b8bfc

fix: bumping min version to 4.27

4d694f1

nikhilmakhijani requested a review from a team as a code owner November 29, 2022 05:09

nikhilmakhijani and others added 3 commits November 29, 2022 18:10

Merge branch 'master' into fix/dynamic_port

0a67053

Merge branch 'master' into fix/dynamic_port

480621e

Merge branch 'master' into fix/dynamic_port

03f91a4

bharathkkb approved these changes Dec 2, 2022

View reviewed changes

Merge branch 'fix/dynamic_port' of https://github.com/nikhilmakhijani…

8c820aa

…/terraform-google-cloud-nat into fix/dynamic_port

nikhilmakhijani force-pushed the fix/dynamic_port branch from ac9980c to cecfe18 Compare December 2, 2022 06:26

fix: update default value for enable endpoint independent mapping

9707cd2

nikhilmakhijani force-pushed the fix/dynamic_port branch from cecfe18 to 9707cd2 Compare December 2, 2022 06:31

Merge branch 'master' into fix/dynamic_port

d1d3998

github-actions bot removed the Stale label Mar 24, 2023

Merge branch 'master' into fix/dynamic_port

7e1f654

Merge branch 'master' into fix/dynamic_port

2bd4e30

Merge branch 'master' into fix/dynamic_port

fd4cc76

fix: test json

1948160

Merge branch 'master' into fix/dynamic_port

fbe92ca

fix: test json for basic & advanced tests

a1b9f2b

apeabody requested changes May 11, 2023

View reviewed changes

main.tf Outdated Show resolved Hide resolved

outputs.tf Outdated Show resolved Hide resolved

variables.tf Outdated Show resolved Hide resolved

versions.tf Outdated Show resolved Hide resolved

variables.tf Show resolved Hide resolved

apeabody changed the title ~~feat: add dynamic port mapping~~ feat!: add dynamic port mapping May 11, 2023

chore: update copyright year

e66ddaf

apeabody changed the title ~~feat!: add dynamic port mapping~~ feat(removes nat_ip_allocate_option)!: add dynamic port mapping May 11, 2023

apeabody approved these changes May 11, 2023

View reviewed changes

apeabody merged commit 0cf1d69 into terraform-google-modules:master May 11, 2023

release-please bot mentioned this pull request May 11, 2023

chore(master): release 4.0.0 #115

Merged

ankurgupta-fk mentioned this pull request Jun 23, 2023

max_ports_per_vm with dynamic allocation #123

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(removes nat_ip_allocate_option)!: add dynamic port mapping #73

feat(removes nat_ip_allocate_option)!: add dynamic port mapping #73

nikhilmakhijani commented Jun 21, 2022 •

edited

Loading

github-actions bot commented Aug 20, 2022

nikhilmakhijani commented Oct 19, 2022

alexander-verharen-qc Nov 22, 2022

bharathkkb left a comment

bharathkkb Nov 29, 2022

nikhilmakhijani Nov 29, 2022

jcalero Nov 30, 2022

bharathkkb Dec 2, 2022

nikhilmakhijani Dec 2, 2022 •

edited

Loading

jeheyer Dec 3, 2022

sclausson May 11, 2023 •

edited

Loading

comment-bot-dev commented Dec 2, 2022

github-actions bot commented Feb 1, 2023

apeabody commented Mar 24, 2023

apeabody commented Mar 24, 2023

apeabody commented Mar 24, 2023

apeabody commented Apr 3, 2023

apeabody commented Apr 4, 2023

sclausson commented May 11, 2023

apeabody commented May 11, 2023

apeabody commented May 11, 2023

nikhilmakhijani commented May 11, 2023

nikhilmakhijani commented May 11, 2023

apeabody commented May 11, 2023

apeabody commented May 11, 2023

apeabody commented May 11, 2023

apeabody left a comment

apeabody commented May 11, 2023

feat(removes nat_ip_allocate_option)!: add dynamic port mapping #73

feat(removes nat_ip_allocate_option)!: add dynamic port mapping #73

Conversation

nikhilmakhijani commented Jun 21, 2022 • edited Loading

github-actions bot commented Aug 20, 2022

nikhilmakhijani commented Oct 19, 2022

alexander-verharen-qc Nov 22, 2022

Choose a reason for hiding this comment

bharathkkb left a comment

Choose a reason for hiding this comment

bharathkkb Nov 29, 2022

Choose a reason for hiding this comment

nikhilmakhijani Nov 29, 2022

Choose a reason for hiding this comment

jcalero Nov 30, 2022

Choose a reason for hiding this comment

bharathkkb Dec 2, 2022

Choose a reason for hiding this comment

nikhilmakhijani Dec 2, 2022 • edited Loading

Choose a reason for hiding this comment

jeheyer Dec 3, 2022

Choose a reason for hiding this comment

sclausson May 11, 2023 • edited Loading

Choose a reason for hiding this comment

comment-bot-dev commented Dec 2, 2022

github-actions bot commented Feb 1, 2023

apeabody commented Mar 24, 2023

apeabody commented Mar 24, 2023

apeabody commented Mar 24, 2023

apeabody commented Apr 3, 2023

apeabody commented Apr 4, 2023

sclausson commented May 11, 2023

apeabody commented May 11, 2023

apeabody commented May 11, 2023

nikhilmakhijani commented May 11, 2023

nikhilmakhijani commented May 11, 2023

apeabody commented May 11, 2023

apeabody commented May 11, 2023

apeabody commented May 11, 2023

apeabody left a comment

Choose a reason for hiding this comment

apeabody commented May 11, 2023

nikhilmakhijani commented Jun 21, 2022 •

edited

Loading

nikhilmakhijani Dec 2, 2022 •

edited

Loading

sclausson May 11, 2023 •

edited

Loading