Added policy_based_vpn_mode to NAT resource

[wvanderwaal-iqmessenger]

Hello,

I've added policy_based_vpn_mode to the NAT resource.
This is a feature I'm using, and every time I call terraform, every VPN mode that has the value 'MATCH', will return to its default of 'BYPASS'.
So it would be nice to specify this value in terraform.

[vmwclabot]

@wvanderwaal-iqmessenger, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <john.doe@email.org> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

• Commit 94c9802 must be signed by w.vanderwaal@iqmessenger.com
• Commit 74c26d9 must be signed by w.vanderwaal@iqmessenger.com

[annakhm]

/test-all

[annakhm]

Hi @wvanderwaal-iqmessenger, thank you for this contribution!
Looks like the test is trying to apply wrong config, here is the error:

=== CONT  TestAccResourceNsxtPolicyNATRuleNoSnatWithoutTNet
    resource_nsxt_policy_nat_rule_test.go:321: Step 1/2 error: Error running apply: exit status 1
        
        Error:  Failed to create NAT Rule 295d76d2-46d8-4189-9646-593e803fcad8: Invalid NAT rule action NO_SNAT for policy based vpn mode BYPASS. policy based vpn mode supported only on DNAT/NO_DNAT rule. (code 503683)
        
          with nsxt_policy_nat_rule.test,
          on terraform_plugin_test.tf line 18, in resource "nsxt_policy_nat_rule" "test":
          18: 	resource "nsxt_policy_nat_rule" "test" {

[wvanderwaal-iqmessenger]

Hello @annakhm thanks for the comment with the error log and running the tests.

You're correct, i was applying incorrect config in the test, a DNAT/NO_DNAT only property in a SNAT test will always fail. Perhaps I read the code a little to fast on my part, sorry about that.

I've removed that property from the SNAT test, so it shouldn't no longer fail there, also I've added 'NO_DNAT' to the description of the property.

[annakhm]

/test-all

[annakhm]

Hello, unfortunately we're still hitting this error in the acceptance testing:

=== CONT  TestAccResourceNsxtPolicyNATRule_basicT0
    resource_nsxt_policy_nat_rule_test.go:152: Step 1/2 error: Error running apply: exit status 1
       
        Error:  Failed to create NAT Rule 759769be-12b8-4647-83dc-2edbd23f0db9: Invalid NAT rule action REFLEXIVE for policy based vpn mode BYPASS. policy based vpn mode supported only on DNAT/NO_DNAT rule. (code 503683)
        
          with nsxt_policy_nat_rule.test,
          on terraform_plugin_test.tf line 40, in resource "nsxt_policy_nat_rule" "test":
          40: resource "nsxt_policy_nat_rule" "test" {

looks like action needs to be adjusted for all tests that configure VPN bypass

[ksamoray]

/test-all

[vmwclabot]

@wvanderwaal-iqmessenger, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <john.doe@email.org> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

• Commit 04b698d must be signed by w.vanderwaal@iqmessenger.com

[wvanderwaal-iqmessenger]

Hello @annakhm,

You're right, some more adjustments were required to the tests, hopefully it's fixed now.

[wvanderwaal-iqmessenger]

Hello @ksamoray,

Thanks for starting the tests, I forgot to run the format so the lint jobs failed, but I've committed this.

Small question, will the Global Manager Acceptance Test still run even if the lint job fails?

[ksamoray]

Hello @ksamoray,

Thanks for starting the tests, I forgot to run the format so the lint jobs failed, but I've committed this.

Small question, will the Global Manager Acceptance Test still run even if the lint job fails?

Hi, yeah acceptance test execution is unrelated to the lint execution.

Anyway there's a GM acctest failure as follows:

        Error: Invalid NAT rule action NAT64 for policy based vpn mode BYPASS. policy based vpn mode supported only on DNAT/NO_DNAT rule.
        
          with nsxt_policy_nat_rule.test,
          on terraform_plugin_test.tf line 22, in resource "nsxt_policy_nat_rule" "test":
          22: resource "nsxt_policy_nat_rule" "test" {

Not sure about what's causing it though.

[wvanderwaal-iqmessenger]

Hello,

Thanks for the error log, I've fixed the issue that caused this.

Both the DNAT and the NAT64 tests use the same resource template, I made a condition to only add policy based vpn mode to the DNAT tests.

[ksamoray]

test-all

[wvanderwaal-iqmessenger]

test-all

Small question, I noticed this doesn't begin with '/'.
Does this have effect on starting the test? Since I'm also not seeing the multi tenancy test.

[ksamoray]

test-all

Small question, I noticed this doesn't begin with '/'. Does this have effect on starting the test? Since I'm also not seeing the multi tenancy test.

No, without the slash it'll do nothing... I'll reactivate

[ksamoray]

/test-all

[ksamoray]

@wvanderwaal-iqmessenger there are failures in the following tests for NSX v3.2.3:

TestAccResourceNsxtPolicyNATRule_basicT1Import
TestAccResourceNsxtPolicyNATRule_basicT1

Failure is:

Json de-serialization error: property policy_based_vpn_mode is unrecognized.

Maybe you should set it only when nsxVersionHigherOrEqual() applies - you can see how it's used in other resources.

[wvanderwaal-iqmessenger]

Hello @ksamoray

Thanks for sending the error log and the advice of the nsx version function.

I've used that function to make sure it's only used in version 4.0.0 (I'm assuming that this is 4.0.0.1, the release of policy_based_vpn_mode).

Also I've created a new test function for policy_based_vpn_mode so I can make use of testAccNSXVersion(t, "4.0.0") and not complicate other test functions.

[wvanderwaal-iqmessenger]

Hello,

Is it possible for someone to run the tests for me? Thanks in advanced!

[annakhm]

/test-all

[annakhm]

Hi @wvanderwaal-iqmessenger, the tests are failing now with

=== CONT  TestAccResourceNsxtPolicyNATRule_basicT1Import
    resource_nsxt_policy_nat_rule_test.go:265: Step 1/2 error: After applying this test step, the plan was not empty.
        stdout:
        
        
        Terraform used the selected providers to generate the following execution
        plan. Resource actions are indicated with the following symbols:
          ~ update in-place
        
        Terraform will perform the following actions:
        
          # nsxt_policy_nat_rule.test will be updated in-place
          ~ resource "nsxt_policy_nat_rule" "test" {
                id                    = "be4abdfe-1f6f-49b4-892d-03c61e9c80c9"
              - policy_based_vpn_mode = "BYPASS" -> null
                # (16 unchanged attributes hidden)
        
                # (2 unchanged blocks hidden)
            }
        
        Plan: 0 to add, 1 to change, 0 to destroy.

My guess is that policy_based_vpn_mode is auto-assigned by NSX when not specified in request, and thus we need to define it as Computed: true in the schema, since we can't provide a default value (and that is because the mode only applies to certain types of NAT)
When attribute is defined as Computed, terraform would not detect a diff and will not complain on Update.

[wvanderwaal-iqmessenger]

Hello @annakhm,

Thanks for sending the error log,

You're right, policy_based_vpn_mode is auto-assigned by NSX if it's not specified and it's a certain NAT type. Computed: true is indeed the solution to my problem, thanks for the tip!

[ksamoray]

/test-all

[annakhm]

Could we please specify here that this attribute is only relevant for certain types of NAT?

[wvanderwaal-iqmessenger]

Yes, I've added that it only applies to DNAT and NO_DNAT. Also I've added that the default for these actions is BYPASS and that this attribute is supported from NSX 4.0 and above.

[annakhm]

@wvanderwaal-iqmessenger thanks for addressing all the comments! Looks good to me, just a small request to improve the doc

[wvanderwaal-iqmessenger]

@annakhm No problem! And thanks for the code review! I've improved the documentation as you requested

[ksamoray]

/test-all

[annakhm]

Thank you!