API Refactoring for VPN resources #866

2ez4szliu · 2023-04-05T21:56:50Z

NSX API that uses locale service path of VPN resources will be deprecated this PR adds support for the new API that uses gateway path.

Also change data_source_nsxt_policy_ipsec_vpn_local_endpoint to use regex-based search API.

Signed-off-by: Shizhao Liu lshizhao@vmware.com

vmwclabot · 2023-04-05T21:56:52Z

@2ez4szliu, 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 bd4b211 must be signed by shizhaoliu@brandeis.edu

vmwclabot · 2023-04-06T00:17:07Z

@2ez4szliu, 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 e76f703 must be signed by shizhaoliu@brandeis.edu

annakhm · 2023-04-10T21:53:56Z

nsxt/resource_nsxt_policy_ipsec_vpn_service.go

 	localeServicePath := d.Get("locale_service_path").(string)
+	if gatewayPath == "" && localeServicePath == "" {
+		return fmt.Errorf("At least one of gateway path and locale service path should be provided for VPN resources")
+	}
 	isT0, gwID, localeServiceID, err := parseLocaleServicePolicyPath(localeServicePath)


Since this block is repeated below, can we move it to a function?

annakhm · 2023-04-10T22:40:08Z

nsxt/resource_nsxt_policy_ipsec_vpn_service.go

+		d.Set("locale_service_path", s[0])
+	} else {
+		d.Set("gateway_path", s[0])
+	}
 	return []*schema.ResourceData{d}, nil
 }



looks like new support is missing in deleteNsxtPolicyIPSecVpnService below

yes, seems it's also missing in getNsxtPolicyIPSecVpnService, now fixed

annakhm · 2023-04-14T19:07:16Z

/test-all

2ez4szliu · 2023-04-17T18:16:31Z

/test-all

annakhm · 2023-04-18T19:25:52Z

nsxt/resource_nsxt_policy_l2_vpn_session_test.go

@@ -142,7 +142,7 @@ func TestAccResourceNsxtPolicyL2VpnSession_tier1(t *testing.T) {
 		},


any reason this test is only running for versions below 3.2.0?

It should be "version higher than 3.2.0", now fixed

annakhm · 2023-04-18T19:27:39Z

nsxt/resource_nsxt_policy_ipsec_vpn_service_test.go

+	testResourceName := "nsxt_policy_ipsec_vpn_service.test"
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:  func() { testAccPreCheck(t); testAccOnlyLocalManager(t) },


I think version check is needed here, since GW api was not exposed for early versions (3.2.0?)

done, same change is made in l2_vpn_service_test

annakhm · 2023-04-18T19:28:57Z

website/docs/r/policy_ipsec_vpn_service.html.markdown

@@ -37,7 +37,8 @@ The following arguments are supported:
 * `description` - (Optional) Description of the resource.
 * `tag` - (Optional) A list of scope + tag pairs to associate with this resource.
 * `nsx_id` - (Optional) The NSX ID of this resource. If set, this ID will be used to create the resource.
-* `locale_service_path` - (Required) Path of the gateway locale service associated with the IPSec VPN Service.
+* `locale_service_path` - (Optional) Path of the gateway locale service associated with the IPSec VPN Service.


We should mark this as Deprecated (same for other resources)

annakhm · 2023-04-18T19:29:28Z

nsxt/resource_nsxt_policy_ipsec_vpn_service.go

@@ -41,7 +43,8 @@ func resourceNsxtPolicyIPSecVpnService() *schema.Resource {
 			"description":         getDescriptionSchema(),
 			"revision":            getRevisionSchema(),
 			"tag":                 getTagsSchema(),
-			"locale_service_path": getPolicyPathSchema(true, false, "Polciy path for the locale service."),
+			"locale_service_path": getPolicyPathSchema(false, false, "Polciy path for the locale service."),


We should mark this as Deprecated (same for other resources)

2ez4szliu · 2023-04-19T00:50:01Z

/test-all

2ez4szliu · 2023-04-19T16:58:39Z

/test-all

annakhm · 2023-04-20T22:50:26Z

nsxt/resource_nsxt_policy_ipsec_vpn_service.go

+				Type:         schema.TypeString,
+				Description:  "Polciy path for the locale service.",
+				Optional:     true,
+				ForceNew:     false,


I think ForceNew needs to be true here, since different API path would be used for updated locale service

annakhm · 2023-04-20T22:51:51Z

nsxt/resource_nsxt_policy_l2_vpn_service.go

+				Type:         schema.TypeString,
+				Description:  "Polciy path for the locale service.",
+				Optional:     true,
+				ForceNew:     false,


annakhm · 2023-04-20T22:54:48Z

LGTM, only small comment about ForceNew.
Is it possible to add a test that would move from locale_service_path to gateway_path?

NSX API that uses locale service path of VPN resources will be deprecated, this PR adds support for the new API that uses gateway path. The change in this PR includes: (1) Support for configuring VPN Resource with gateway path. (2) change data_source_nsxt_policy_ipsec_vpn_local_endpoint to use regex-based search API. (3) For VPN Sessions, if only specify the direction of Tcp Mss Clampling, the provider will use the auto-assigned value for the Maximum Segment Size. Signed-off-by: Shizhao Liu <lshizhao@vmware.com>

2ez4szliu · 2023-04-23T04:54:40Z

LGTM, only small comment about ForceNew. Is it possible to add a test that would move from locale_service_path to gateway_path?

Added test case TestAccResourceNsxtPolicyIPSecVpnService_updateFromlocaleServicePathToGatewayPath and TestAccResourceNsxtPolicyL2VpnService_updateFromlocaleServicePathToGatewayPath

2ez4szliu · 2023-04-23T04:54:48Z

/test-all

2ez4szliu · 2023-04-23T17:11:33Z

/test-all

annakhm · 2023-04-24T17:47:00Z

LGTM, only small comment about ForceNew. Is it possible to add a test that would move from locale_service_path to gateway_path?

Added test case TestAccResourceNsxtPolicyIPSecVpnService_updateFromlocaleServicePathToGatewayPath and TestAccResourceNsxtPolicyL2VpnService_updateFromlocaleServicePathToGatewayPath

Great, thanks!

vmwclabot added the dco-required DCO Required label Apr 5, 2023

2ez4szliu force-pushed the vpn-gateway-path branch from bd4b211 to e76f703 Compare April 6, 2023 00:17

2ez4szliu force-pushed the vpn-gateway-path branch from e76f703 to 5f8c3ef Compare April 6, 2023 00:29

vmwclabot removed the dco-required DCO Required label Apr 6, 2023

2ez4szliu force-pushed the vpn-gateway-path branch 3 times, most recently from aca6719 to 2dc6a62 Compare April 8, 2023 21:34

annakhm reviewed Apr 10, 2023

View reviewed changes

2ez4szliu force-pushed the vpn-gateway-path branch 5 times, most recently from 977150a to 776658a Compare April 14, 2023 18:12

annakhm reviewed Apr 18, 2023

View reviewed changes

2ez4szliu force-pushed the vpn-gateway-path branch from 776658a to 69b91c2 Compare April 18, 2023 21:10

2ez4szliu force-pushed the vpn-gateway-path branch 2 times, most recently from 0c507d7 to 8ead01c Compare April 19, 2023 05:12

annakhm reviewed Apr 20, 2023

View reviewed changes

2ez4szliu force-pushed the vpn-gateway-path branch from 8ead01c to 515cf3e Compare April 21, 2023 00:05

annakhm approved these changes Apr 24, 2023

View reviewed changes

2ez4szliu merged commit 8954935 into vmware:master Apr 24, 2023

2ez4szliu mentioned this pull request Apr 24, 2023

Configuration of TCP Clamping for IPSec VPN sessions via TF #853

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Refactoring for VPN resources #866

API Refactoring for VPN resources #866

2ez4szliu commented Apr 5, 2023

vmwclabot commented Apr 5, 2023

vmwclabot commented Apr 6, 2023

annakhm Apr 10, 2023

2ez4szliu Apr 12, 2023

annakhm Apr 10, 2023

2ez4szliu Apr 12, 2023

annakhm commented Apr 14, 2023

2ez4szliu commented Apr 17, 2023

annakhm Apr 18, 2023

2ez4szliu Apr 19, 2023

annakhm Apr 18, 2023

2ez4szliu Apr 19, 2023

annakhm Apr 18, 2023

2ez4szliu Apr 19, 2023

annakhm Apr 18, 2023

2ez4szliu Apr 19, 2023

2ez4szliu commented Apr 19, 2023

2ez4szliu commented Apr 19, 2023

annakhm Apr 20, 2023

annakhm Apr 20, 2023

annakhm commented Apr 20, 2023

2ez4szliu commented Apr 23, 2023

2ez4szliu commented Apr 23, 2023

2ez4szliu commented Apr 23, 2023

annakhm commented Apr 24, 2023

		@@ -142,7 +142,7 @@ func TestAccResourceNsxtPolicyL2VpnSession_tier1(t *testing.T) {
		},

API Refactoring for VPN resources #866

API Refactoring for VPN resources #866

Conversation

2ez4szliu commented Apr 5, 2023

vmwclabot commented Apr 5, 2023

vmwclabot commented Apr 6, 2023

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

annakhm commented Apr 14, 2023

2ez4szliu commented Apr 17, 2023

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

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

2ez4szliu commented Apr 19, 2023

2ez4szliu commented Apr 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annakhm commented Apr 20, 2023

2ez4szliu commented Apr 23, 2023

2ez4szliu commented Apr 23, 2023

2ez4szliu commented Apr 23, 2023

annakhm commented Apr 24, 2023