Add rules field to google_compute_router_nat #6643

zli82016 · 2022-10-04T21:31:51Z

Add rules field to google_compute_router_nat

API: https://cloud.google.com/compute/docs/reference/rest/v1/routers
Issue: hashicorp/terraform-provider-google#12179

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: added general field `rules` to `google_compute_router_nat`

modular-magician · 2022-10-04T21:32:07Z

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-10-04T21:32:48Z

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

I've detected that you're a community contributor. @shuyama1, a repository maintainer, has been assigned to assist you and help review your changes.

❓ First time contributing? Click here for more details

Your assigned reviewer will help review your code by:

Ensuring it's backwards compatible, covers common error cases, etc.
Summarizing the change into a user-facing changelog note.
Passes tests, either our "VCR" suite, a set of presubmit tests, or with manual test runs.

You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails.

If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox.

modular-magician · 2022-10-04T21:35:26Z

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-10-04T21:50:49Z

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

Diff report:

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

modular-magician · 2022-10-04T22:28:42Z

Tests analytics

Total tests: 2201
Passed tests 1954
Skipped tests: 240
Failed tests: 7

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccFirebaserulesRelease_BasicRelease|TestAccComputeRouterNat_withNatRules|TestAccComputeRouterNat_withPortAllocationMethods|TestAccComputeRouterNat_withNatIpsAndDrainNatIps|TestAccComputeRouterNat_removeLogConfig|TestAccComputeRouterNat_update|TestAccComputeInstance_soleTenantNodeAffinities

modular-magician · 2022-10-04T22:34:05Z

Tests passed during RECORDING mode:
TestAccFirebaserulesRelease_BasicRelease[Debug log]
TestAccComputeRouterNat_withNatRules[Debug log]
TestAccComputeRouterNat_withPortAllocationMethods[Debug log]
TestAccComputeRouterNat_withNatIpsAndDrainNatIps[Debug log]
TestAccComputeRouterNat_removeLogConfig[Debug log]
TestAccComputeRouterNat_update[Debug log]

Tests failed during RECORDING mode:
TestAccComputeInstance_soleTenantNodeAffinities[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

shuyama1 · 2022-10-05T23:49:35Z

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

+    source_ip_ranges_to_nat = ["ALL_IP_RANGES"]
+  }
+
+  rules {


Can we try testing on setting multiple rules? This feature was originally implemented in #6221 and was reverted in #6343 due to list ordering issue. Just want to make sure we're not running into the same error again.

Thanks for the comment, which is really helpful. I am adding the testing for multiple rules.

Also, use Set instead of List for rules to fix the ordering issue.

modular-magician · 2022-10-07T04:55: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, 699 insertions(+))
Terraform Beta: Diff ( 3 files changed, 663 insertions(+), 1 deletion(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2022-10-07T05:40:11Z

Tests analytics

Total tests: 2208
Passed tests 1956
Skipped tests: 240
Failed tests: 12

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccVPCAccessConnector_cloudrunVPCAccessConnectorExample|TestAccFirebaserulesRelease_BasicRelease|TestAccComputeInstance_soleTenantNodeAffinities|TestAccComputeRouterNat_withNatRules|TestAccComputeRouterNat_withNatIpsAndDrainNatIps|TestAccComputeRouterNat_withPortAllocationMethods|TestAccComputeRouterNat_withManualIpAndSubnetConfiguration|TestAccComputeRouterNat_removeLogConfig|TestAccComputeRouterNat_update|TestAccComputeRouterNat_basic|TestAccCGCSnippet_eventarcWorkflowsExample|TestAccCloudRunService_cloudRunServiceStaticOutboundExample

modular-magician · 2022-10-07T05:46:54Z

Tests passed during RECORDING mode:
TestAccVPCAccessConnector_cloudrunVPCAccessConnectorExample[Debug log]
TestAccFirebaserulesRelease_BasicRelease[Debug log]
TestAccComputeRouterNat_withNatRules[Debug log]
TestAccComputeRouterNat_withNatIpsAndDrainNatIps[Debug log]
TestAccComputeRouterNat_withPortAllocationMethods[Debug log]
TestAccComputeRouterNat_withManualIpAndSubnetConfiguration[Debug log]
TestAccComputeRouterNat_removeLogConfig[Debug log]
TestAccComputeRouterNat_update[Debug log]
TestAccComputeRouterNat_basic[Debug log]
TestAccCloudRunService_cloudRunServiceStaticOutboundExample[Debug log]

Tests failed during RECORDING mode:
TestAccComputeInstance_soleTenantNodeAffinities[Error message] [Debug log]
TestAccCGCSnippet_eventarcWorkflowsExample[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

shuyama1

LGTM in general. Just added some small comments and questions.

mmv1/products/compute/api.yaml

shuyama1 · 2022-10-11T19:33:19Z

mmv1/products/compute/api.yaml

+            - !ruby/object:Api::Type::NestedObject
+              name: 'action'
+              description: 'The action to be enforced for traffic that matches this rule.'
+              send_empty_value: true


please check if it's necessary to add send_empty_value here, and also the subfields.

It is unnecessary to include send_empty_value, so it is removed in the new revision.

mmv1/products/compute/api.yaml

shuyama1

Can we try the following changes and see if it's solving the problem?

shuyama1 · 2022-10-13T20:55:51Z

mmv1/templates/terraform/constants/router_nat.go.erb

+				sourceNatActiveIpSet := sourceNatActiveIps.([]string)
+				for _, sourceNatActiveIp := range sourceNatActiveIpSet {
+					sourceNatActiveIpHash += computeRouterNatIPsHash(sourceNatActiveIp)


Suggested change

sourceNatActiveIpSet := sourceNatActiveIps.([]string)

for _, sourceNatActiveIp := range sourceNatActiveIpSet {

sourceNatActiveIpHash += computeRouterNatIPsHash(sourceNatActiveIp)

sourceNatActiveIpSet := sourceNatActiveIps.(*schema.Set)

for _, sourceNatActiveIp := range sourceNatActiveIpSet.List() {

sourceNatActiveIpHash += computeRouterNatIPsHash(sourceNatActiveIp.(string))

With type assertion []string, the error is
panic: interface conversion: interface {} is *schema.Set, not []string
https://paste.googleplex.com/4690283169054720

With type assertion *schema.Set, the error is
panic: interface conversion: interface {} is []string, not *schema.Set
https://paste.googleplex.com/6158598605373440

shuyama1 · 2022-10-13T20:56:42Z

mmv1/templates/terraform/constants/router_nat.go.erb

+				soureNatDrainIpSet := soureNatDrainIps.([]string)
+				for _, soureNatDrainIp := range soureNatDrainIpSet {
+					sourceNatDrainIpHash += computeRouterNatIPsHash(soureNatDrainIp)


Suggested change

soureNatDrainIpSet := soureNatDrainIps.([]string)

for _, soureNatDrainIp := range soureNatDrainIpSet {

sourceNatDrainIpHash += computeRouterNatIPsHash(soureNatDrainIp)

soureNatDrainIpSet := soureNatDrainIps.(*schema.Set)

for _, soureNatDrainIp := range soureNatDrainIpSet.List() {

sourceNatDrainIpHash += computeRouterNatIPsHash(soureNatDrainIp.(string))

modular-magician · 2022-10-13T21:02:57Z

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

Diff report:

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

modular-magician · 2022-10-13T21:28:12Z

The provider crashed while running the VCR tests in REPLAYING mode
Please fix it to complete your PR
View the build log

modular-magician · 2022-10-14T16:09:45Z

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

Diff report:

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

modular-magician · 2022-10-14T16:48:12Z

Tests analytics

Total tests: 2187
Passed tests 1947
Skipped tests: 239
Failed tests: 1

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccComputeRouterNat_withNatRules

modular-magician · 2022-10-14T16:52:07Z

Tests passed during RECORDING mode:
TestAccComputeRouterNat_withNatRules[Debug log]

All tests passed
View the build log or the debug log for each test

modular-magician · 2022-10-14T18:25:29Z

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

Diff report:

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

modular-magician · 2022-10-14T19:01:56Z

Tests analytics

Total tests: 2187
Passed tests 1946
Skipped tests: 239
Failed tests: 2

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccFirebaserulesRelease_BasicRelease|TestAccComputeRouterNat_withNatRules

modular-magician · 2022-10-14T19:07:09Z

Tests passed during RECORDING mode:
TestAccFirebaserulesRelease_BasicRelease[Debug log]
TestAccComputeRouterNat_withNatRules[Debug log]

All tests passed
View the build log or the debug log for each test

shuyama1

LGTM!

modular-magician requested a review from shuyama1 October 4, 2022 21:32

zli82016 force-pushed the zhenhua-dev branch from f330913 to 5ccbd8f Compare October 4, 2022 21:35

zli82016 force-pushed the zhenhua-dev branch from 5ccbd8f to 39ef1b5 Compare October 4, 2022 21:38

shuyama1 reviewed Oct 5, 2022

View reviewed changes

zli82016 force-pushed the zhenhua-dev branch from 39ef1b5 to 129277e Compare October 7, 2022 04:40

modular-magician requested a review from shuyama1 October 7, 2022 04:41

zli82016 force-pushed the zhenhua-dev branch from 129277e to 00441ea Compare October 7, 2022 04:44

shuyama1 reviewed Oct 11, 2022

View reviewed changes

zli82016 force-pushed the zhenhua-dev branch from 00441ea to d0c45ce Compare October 13, 2022 20:40

modular-magician requested a review from shuyama1 October 13, 2022 20:41

zli82016 force-pushed the zhenhua-dev branch from d0c45ce to 6a3e5bb Compare October 13, 2022 20:49

shuyama1 reviewed Oct 13, 2022

View reviewed changes

zli82016 force-pushed the zhenhua-dev branch from 6a3e5bb to 8034489 Compare October 14, 2022 15:54

modular-magician requested a review from shuyama1 October 14, 2022 15:55

zli82016 force-pushed the zhenhua-dev branch from 8034489 to 2095a1a Compare October 14, 2022 18:09

Add rules field to google_compute_router_nat

6af4d42

zli82016 force-pushed the zhenhua-dev branch from 2095a1a to 6af4d42 Compare October 14, 2022 18:12

shuyama1 approved these changes Oct 17, 2022

View reviewed changes

zli82016 merged commit 8e23a16 into main Oct 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rules field to google_compute_router_nat #6643

Add rules field to google_compute_router_nat #6643

zli82016 commented Oct 4, 2022 •

edited by shuyama1

modular-magician commented Oct 4, 2022

modular-magician commented Oct 4, 2022

modular-magician commented Oct 4, 2022

modular-magician commented Oct 4, 2022

modular-magician commented Oct 4, 2022

modular-magician commented Oct 4, 2022

shuyama1 Oct 5, 2022

zli82016 Oct 6, 2022

zli82016 Oct 7, 2022

modular-magician commented Oct 7, 2022

modular-magician commented Oct 7, 2022

modular-magician commented Oct 7, 2022

shuyama1 left a comment

shuyama1 Oct 11, 2022

zli82016 Oct 14, 2022

shuyama1 left a comment

shuyama1 Oct 13, 2022

zli82016 Oct 13, 2022

shuyama1 Oct 13, 2022

modular-magician commented Oct 13, 2022

modular-magician commented Oct 13, 2022

modular-magician commented Oct 14, 2022

modular-magician commented Oct 14, 2022

modular-magician commented Oct 14, 2022

modular-magician commented Oct 14, 2022

modular-magician commented Oct 14, 2022

modular-magician commented Oct 14, 2022

shuyama1 left a comment

Add rules field to google_compute_router_nat #6643

Add rules field to google_compute_router_nat #6643

Conversation

zli82016 commented Oct 4, 2022 • edited by shuyama1

modular-magician commented Oct 4, 2022

modular-magician commented Oct 4, 2022

modular-magician commented Oct 4, 2022

modular-magician commented Oct 4, 2022

Diff report:

modular-magician commented Oct 4, 2022

Tests analytics

Action taken

modular-magician commented Oct 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Oct 7, 2022

Diff report:

modular-magician commented Oct 7, 2022

Tests analytics

Action taken

modular-magician commented Oct 7, 2022

shuyama1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuyama1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Oct 13, 2022

Diff report:

modular-magician commented Oct 13, 2022

modular-magician commented Oct 14, 2022

Diff report:

modular-magician commented Oct 14, 2022

Tests analytics

Action taken

modular-magician commented Oct 14, 2022

modular-magician commented Oct 14, 2022

Diff report:

modular-magician commented Oct 14, 2022

Tests analytics

Action taken

modular-magician commented Oct 14, 2022

shuyama1 left a comment

Choose a reason for hiding this comment

zli82016 commented Oct 4, 2022 •

edited by shuyama1