Test review and cleanup (#888)

.changes/v3.7.0/841-improvements.md

@@ -1,6 +1,6 @@
-* Add VDC Group compatibility for `resource/vcd_nsxt_firewall` and `datasource/vcd_nsxt_firewall` [GH-841]
-* Add VDC Group compatibility for `resource/vcd_nsxt_nat_rule` and `datasource/vcd_nsxt_nat_rule` [GH-841]
-* Add VDC Group compatibility for `resource/vcd_nsxt_ipsec_vpn_tunnel` and `datasource/vcd_nsxt_ipsec_vpn_tunnel` [GH-841]
+* Add VDC Group compatibility for `resource/vcd_nsxt_firewall` and `datasource/vcd_nsxt_firewall` [GH-841, GH-888]
+* Add VDC Group compatibility for `resource/vcd_nsxt_nat_rule` and `datasource/vcd_nsxt_nat_rule` [GH-841, GH-888]
+* Add VDC Group compatibility for `resource/vcd_nsxt_ipsec_vpn_tunnel` and `datasource/vcd_nsxt_ipsec_vpn_tunnel` [GH-841, GH-888]
 * Add VDC Group compatibility for `resource/vcd_nsxt_alb_settings` and `datasource/vcd_nsxt_alb_settings` [GH-841]
 * Add VDC Group compatibility for `resource/vcd_nsxt_alb_edgegateway_service_engine_group` and `datasource/vcd_nsxt_alb_edgegateway_service_engine_group` [GH-841, GH-854]
 * Add VDC Group compatibility for `resource/vcd_nsxt_alb_virtual_service` and `datasource/vcd_nsxt_alb_virtual_service` [GH-841]

.changes/v3.7.0/858-features.md

@@ -1,2 +1,2 @@
-* Add resource `vcd_nsxt_route_advertisement` that allows NSX-T Edge Gateway to advertise subnets to Tier-0 Gateway [GH-858]
-* Add datasource `vcd_nsxt_route_advertisement` that reads the NSX-T Edge Gateway routes that are being advertised [GH-858]
+* Add resource `vcd_nsxt_route_advertisement` that allows NSX-T Edge Gateway to advertise subnets to Tier-0 Gateway [GH-858, GH-888]
+* Add datasource `vcd_nsxt_route_advertisement` that reads the NSX-T Edge Gateway routes that are being advertised [GH-858, GH-888]

.changes/v3.7.0/870-bug-fixes.md

@@ -1 +1 @@
-* Fix `vcd_inserted_media` locking mechanism to avoid race condition with `vcd_vm_internal_disk` [GH-870]
+* Fix `vcd_inserted_media` locking mechanism to avoid race condition with `vcd_vm_internal_disk` [GH-870, GH-888]

.changes/v3.7.0/879-features.md

@@ -1,2 +1,2 @@
-* **New Resource:** `vcd_nsxt_edgegateway_bgp_ip_prefix_list` allows users to configure NSX-T Edge Gateway BGP IP Prefix Lists [GH-879, GH-887]
-* **New Data Source:** `vcd_nsxt_edgegateway_bgp_ip_prefix_list` allows users to read NSX-T Edge Gateway BGP IP Prefix Lists [GH-879, GH-887]
+* **New Resource:** `vcd_nsxt_edgegateway_bgp_ip_prefix_list` allows users to configure NSX-T Edge Gateway BGP IP Prefix Lists [GH-879, GH-887, GH-888]
+* **New Data Source:** `vcd_nsxt_edgegateway_bgp_ip_prefix_list` allows users to read NSX-T Edge Gateway BGP IP Prefix Lists [GH-879, GH-887, GH-888]

.changes/v3.7.0/880-features.md

@@ -1,2 +1,2 @@
-* **New Resource:** `vcd_nsxt_edgegateway_bgp_neighbor` allows users to configure NSX-T Edge Gateway BGP Neighbors [GH-879, GH-887]
-* **New Data Source:** `vcd_nsxt_edgegateway_bgp_neighbor` allows users to read NSX-T Edge Gateway BGP Neighbors [GH-879, GH-887]
+* **New Resource:** `vcd_nsxt_edgegateway_bgp_neighbor` allows users to configure NSX-T Edge Gateway BGP Neighbors [GH-879, GH-887, GH-888]
+* **New Data Source:** `vcd_nsxt_edgegateway_bgp_neighbor` allows users to read NSX-T Edge Gateway BGP Neighbors [GH-879, GH-887, GH-888]

scripts/runtest.sh

@@ -199,7 +199,7 @@ function binary_test {
         exit $?
     fi
     timestamp=$(date +%Y-%m-%d-%H-%M)
-    ./test-binary.sh names '*.tf' 2>&1 | tee test-binary-${timestamp}.txt
+    ./test-binary.sh 2>&1 | tee test-binary-${timestamp}.txt
 }
 
 function exists_in_path {

vcd/config.go

@@ -217,6 +217,7 @@ func (cli *VCDClient) unLockParentVapp(d *schema.ResourceData) {
 
 // lockParentVm locks using vapp_name and vm_name names existing in resource parameters.
 // Parent means the resource belongs to the VM being locked
+//lint:ignore U1000 For future use
 func (cli *VCDClient) lockParentVm(d *schema.ResourceData) {
 	vappName := d.Get("vapp_name").(string)
 	if vappName == "" {
@@ -230,6 +231,7 @@ func (cli *VCDClient) lockParentVm(d *schema.ResourceData) {
 	vcdMutexKV.kvLock(key)
 }
 
+//lint:ignore U1000 For future use
 func (cli *VCDClient) unLockParentVm(d *schema.ResourceData) {
 	vappName := d.Get("vapp_name").(string)
 	if vappName == "" {

vcd/config_test.go

@@ -403,7 +403,7 @@ func templateFill(tmpl string, data StringMap) string {
 		data["PrOrg"] = testConfig.VCD.Org
 		vdcName, found := data["PrVdc"]
 		if !found || vdcName == "" {
-			data["PrVdc"] = testConfig.VCD.Vdc
+			data["PrVdc"] = testConfig.Nsxt.Vdc
 		}
 		data["AllowInsecure"] = testConfig.Provider.AllowInsecure
 		data["MaxRetryTimeout"] = testConfig.Provider.MaxRetryTimeout

vcd/datasource_not_found_test.go

@@ -44,6 +44,8 @@ func testSpecificDataSourceNotFound(t *testing.T, dataSourceName string, vcdClie
 			dataSourceName == "vcd_rights_bundle" || dataSourceName == "vcd_vdc_group") &&
 			!usingSysAdmin():
 			t.Skip(`Works only with system admin privileges`)
+		case (dataSourceName == "vcd_nsxt_edgegateway_bgp_ip_prefix_list" || dataSourceName == "vcd_nsxt_edgegateway_bgp_neighbor") && !usingSysAdmin():
+			t.Skip(`Works only with system admin privileges`)
 		case dataSourceName == "vcd_external_network_v2" && vcdClient.Client.APIVCDMaxVersionIs("< 33"):
 			t.Skip("External network V2 requires at least API version 33 (VCD 10.0+)")
 		case (dataSourceName == "vcd_library_certificate" || dataSourceName == "vcd_vdc_group") && vcdClient.Client.APIVCDMaxVersionIs("< 35"):

vcd/resource_vcd_inserted_media.go

@@ -3,9 +3,10 @@ package vcd
 import (
 	"context"
 	"fmt"
-	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
 	"log"
 
+	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
+
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/vmware/go-vcloud-director/v2/govcd"
 	"github.com/vmware/go-vcloud-director/v2/types/v56"
@@ -72,22 +73,22 @@ func resourceVcdMediaInsert(ctx context.Context, d *schema.ResourceData, meta in
 
 	vcdClient := meta.(*VCDClient)
 
-	vcdClient.lockParentVm(d)
-	defer vcdClient.unLockParentVm(d)
+	vcdClient.lockParentVapp(d)
+	defer vcdClient.unLockParentVapp(d)
 
 	vm, org, err := getVM(d, meta)
 	if err != nil || org == nil {
-		return diag.Errorf("error: %#v", err)
+		return diag.Errorf("error: %s", err)
 	}
 
 	task, err := vm.HandleInsertMedia(org, d.Get("catalog").(string), d.Get("name").(string))
 	if err != nil {
-		return diag.Errorf("error: %#v", err)
+		return diag.Errorf("error: %s", err)
 	}
 
 	err = task.WaitTaskCompletion()
 	if err != nil {
-		return diag.Errorf("error: %#v", err)
+		return diag.Errorf("error: %s", err)
 	}
 
 	d.SetId(d.Get("vapp_name").(string) + "_" + d.Get("vm_name").(string) + "_" + d.Get("name").(string))
@@ -124,22 +125,22 @@ func resourceVcdMediaEject(ctx context.Context, d *schema.ResourceData, meta int
 
 	vcdClient := meta.(*VCDClient)
 
-	vcdClient.lockParentVm(d)
-	defer vcdClient.unLockParentVm(d)
+	vcdClient.lockParentVapp(d)
+	defer vcdClient.unLockParentVapp(d)
 
 	vm, org, err := getVM(d, meta)
 	if err != nil {
-		return diag.Errorf("error: %#v", err)
+		return diag.Errorf("error: %s", err)
 	}
 
 	task, err := vm.HandleEjectMedia(org, d.Get("catalog").(string), d.Get("name").(string))
 	if err != nil {
-		return diag.Errorf("error: %#v", err)
+		return diag.Errorf("error: %s", err)
 	}
 
 	err = task.WaitTaskCompletion(d.Get("eject_force").(bool))
 	if err != nil {
-		return diag.Errorf("error: %#v", err)
+		return diag.Errorf("error: %s", err)
 	}
 
 	return nil

vcd/resource_vcd_nsxt_alb_common_test.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"regexp"
 	"testing"
+	"time"
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
 )
@@ -181,11 +182,15 @@ func TestAccVcdNsxtAlbVdcGroupIntegration(t *testing.T) {
 			{
 				Config: configText1, // Setup prerequisites - configure NSX-T ALB in Provider
 				Check: resource.ComposeAggregateTestCheckFunc(
-					resource.TestMatchResourceAttr("vcd_nsxt_alb_virtual_service.test", "id", regexp.MustCompile(`^urn:vcloud:loadBalancerVirtualService:`)),
+					resource.TestMatchResourceAttr("vcd_nsxt_alb_cloud.first", "id", regexp.MustCompile(`^urn:vcloud:loadBalancerCloud:`)),
 				),
 			},
 			{
-				Config: configText2,
+				// Sleeping extra 5 seconds because sometimes one gets `CLOUD_STATE_IN_PROGRESS`
+				// error when immediatelly configuring user space configuration just after adding
+				// `System` configuration in for ALB (which is done in Step 1 above)
+				PreConfig: func() { time.Sleep(time.Second * 5) },
+				Config:    configText2,
 				Check: resource.ComposeAggregateTestCheckFunc(
 					resource.TestMatchResourceAttr("vcd_nsxt_alb_virtual_service.test", "id", regexp.MustCompile(`^urn:vcloud:loadBalancerVirtualService:`)),
 				),
@@ -259,19 +264,6 @@ resource "vcd_nsxt_edgegateway" "nsxt-edge" {
   }
 }
 
-resource "vcd_nsxt_alb_settings" "test" {
-  org = "{{.Org}}"
-  vdc = vcd_org_vdc.newVdc.0.name
-
-  edge_gateway_id = vcd_nsxt_edgegateway.nsxt-edge.id
-  is_active       = true
-  {{.SupportedFeatureSet}}
-
-  # This dependency is required to make sure that provider part of operations is done
-  depends_on = [vcd_nsxt_alb_service_engine_group.first]
-}
-
-
 locals {
   controller_id = vcd_nsxt_alb_controller.first.id
 }
@@ -305,40 +297,6 @@ resource "vcd_nsxt_alb_service_engine_group" "first" {
   reservation_model                    = "DEDICATED"
   {{.SupportedFeatureSet}}
 }
-
-resource "vcd_nsxt_alb_edgegateway_service_engine_group" "assignment" {
-  org = "{{.Org}}"
-  vdc = vcd_org_vdc.newVdc.0.name
-
-  edge_gateway_id         = vcd_nsxt_alb_settings.test.edge_gateway_id
-  service_engine_group_id = vcd_nsxt_alb_service_engine_group.first.id
-}
-
-resource "vcd_nsxt_alb_pool" "test" {
-  org = "{{.Org}}"
-  vdc = vcd_org_vdc.newVdc.0.name
-
-  name            = "{{.Name}}-pool"
-  edge_gateway_id = vcd_nsxt_alb_settings.test.edge_gateway_id
-}
-
-resource "vcd_nsxt_alb_virtual_service" "test" {
-  org = "{{.Org}}"
-  vdc = vcd_org_vdc.newVdc.0.name
-
-  name            = "{{.Name}}-vs"
-  edge_gateway_id = vcd_nsxt_alb_settings.test.edge_gateway_id
-
-  pool_id                  = vcd_nsxt_alb_pool.test.id
-  service_engine_group_id  = vcd_nsxt_alb_edgegateway_service_engine_group.assignment.service_engine_group_id
-  virtual_ip_address       = tolist(vcd_nsxt_edgegateway.nsxt-edge.subnet)[0].primary_ip
-  application_profile_type = "HTTP"
-  service_port {
-    start_port = 80
-    end_port   = 81
-    type       = "TCP_PROXY"
-  }
-}
 `
 
 const testAccVcdNsxtAlbVdcGroupIntegration2 = testAccVcdVdcGroupNew + `
@@ -417,6 +375,8 @@ resource "vcd_nsxt_alb_edgegateway_service_engine_group" "assignment" {
 
   edge_gateway_id         = vcd_nsxt_alb_settings.test.edge_gateway_id
   service_engine_group_id = vcd_nsxt_alb_service_engine_group.first.id
+
+  depends_on = [vcd_nsxt_alb_settings.test]
 }
 
 resource "vcd_nsxt_alb_pool" "test" {
@@ -441,6 +401,8 @@ resource "vcd_nsxt_alb_virtual_service" "test" {
     end_port   = 81
     type       = "TCP_PROXY"
   }
+
+  depends_on = [vcd_nsxt_alb_settings.test]
 }
 `
 

vcd/resource_vcd_nsxt_firewall.go

@@ -128,8 +128,24 @@ func resourceVcdNsxtFirewall() *schema.Resource {
 // resourceVcdNsxtFirewallCreateUpdate is the same function used for both - Create and Update
 func resourceVcdNsxtFirewallCreateUpdate(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
 	vcdClient := meta.(*VCDClient)
-	vcdClient.lockParentEdgeGtw(d)
-	defer vcdClient.unLockParentEdgeGtw(d)
+
+	// Handling locks is conditional. There are two scenarios:
+	// * When the parent Edge Gateway is in a VDC - a lock on parent Edge Gateway must be acquired
+	// * When the parent Edge Gateway is in a VDC Group - a lock on parent VDC Group must be acquired
+	// To find out parent lock object, Edge Gateway must be looked up and its OwnerRef must be checked
+	// Note. It is not safe to do multiple locks in the same resource as it can result in a deadlock
+	parentEdgeGatewayOwnerId, _, err := getParentEdgeGatewayOwnerId(vcdClient, d)
+	if err != nil {
+		return diag.Errorf("[nsx-t firewall create/update] error finding parent Edge Gateway: %s", err)
+	}
+
+	if govcd.OwnerIsVdcGroup(parentEdgeGatewayOwnerId) {
+		vcdClient.lockById(parentEdgeGatewayOwnerId)
+		defer vcdClient.unlockById(parentEdgeGatewayOwnerId)
+	} else {
+		vcdClient.lockParentEdgeGtw(d)
+		defer vcdClient.unLockParentEdgeGtw(d)
+	}
 
 	orgName := d.Get("org").(string)
 	edgeGatewayId := d.Get("edge_gateway_id").(string)
@@ -186,8 +202,24 @@ func resourceVcdNsxtFirewallRead(ctx context.Context, d *schema.ResourceData, me
 
 func resourceVcdNsxtFirewallDelete(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
 	vcdClient := meta.(*VCDClient)
-	vcdClient.lockParentEdgeGtw(d)
-	defer vcdClient.unLockParentEdgeGtw(d)
+
+	// Handling locks is conditional. There are two scenarios:
+	// * When the parent Edge Gateway is in a VDC - a lock on parent Edge Gateway must be acquired
+	// * When the parent Edge Gateway is in a VDC Group - a lock on parent VDC Group must be acquired
+	// To find out parent lock object, Edge Gateway must be looked up and its OwnerRef must be checked
+	// Note. It is not safe to do multiple locks in the same resource as it can result in a deadlock
+	parentEdgeGatewayOwnerId, _, err := getParentEdgeGatewayOwnerId(vcdClient, d)
+	if err != nil {
+		return diag.Errorf("[nsx-t firewall create/update] error finding parent Edge Gateway: %s", err)
+	}
+
+	if govcd.OwnerIsVdcGroup(parentEdgeGatewayOwnerId) {
+		vcdClient.lockById(parentEdgeGatewayOwnerId)
+		defer vcdClient.unlockById(parentEdgeGatewayOwnerId)
+	} else {
+		vcdClient.lockParentEdgeGtw(d)
+		defer vcdClient.unLockParentEdgeGtw(d)
+	}
 
 	orgName := d.Get("org").(string)
 	edgeGatewayId := d.Get("edge_gateway_id").(string)

vcd/resource_vcd_nsxt_ipsec_vpn_tunnel.go

@@ -235,8 +235,24 @@ func resourceVcdNsxtIpSecVpnTunnel() *schema.Resource {
 
 func resourceVcdNsxtIpSecVpnTunnelCreate(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
 	vcdClient := meta.(*VCDClient)
-	vcdClient.lockParentEdgeGtw(d)
-	defer vcdClient.unLockParentEdgeGtw(d)
+
+	// Handling locks is conditional. There are two scenarios:
+	// * When the parent Edge Gateway is in a VDC - a lock on parent Edge Gateway must be acquired
+	// * When the parent Edge Gateway is in a VDC Group - a lock on parent VDC Group must be acquired
+	// To find out parent lock object, Edge Gateway must be looked up and its OwnerRef must be checked
+	// Note. It is not safe to do multiple locks in the same resource as it can result in a deadlock
+	parentEdgeGatewayOwnerId, _, err := getParentEdgeGatewayOwnerId(vcdClient, d)
+	if err != nil {
+		return diag.Errorf("[nsx-t ipsec vpn tunnel create/update] error finding parent Edge Gateway: %s", err)
+	}
+
+	if govcd.OwnerIsVdcGroup(parentEdgeGatewayOwnerId) {
+		vcdClient.lockById(parentEdgeGatewayOwnerId)
+		defer vcdClient.unlockById(parentEdgeGatewayOwnerId)
+	} else {
+		vcdClient.lockParentEdgeGtw(d)
+		defer vcdClient.unLockParentEdgeGtw(d)
+	}
 
 	orgName := d.Get("org").(string)
 	edgeGatewayId := d.Get("edge_gateway_id").(string)
@@ -277,8 +293,24 @@ func resourceVcdNsxtIpSecVpnTunnelCreate(ctx context.Context, d *schema.Resource
 
 func resourceVcdNsxtIpSecVpnTunnelUpdate(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
 	vcdClient := meta.(*VCDClient)
-	vcdClient.lockParentEdgeGtw(d)
-	defer vcdClient.unLockParentEdgeGtw(d)
+
+	// Handling locks is conditional. There are two scenarios:
+	// * When the parent Edge Gateway is in a VDC - a lock on parent Edge Gateway must be acquired
+	// * When the parent Edge Gateway is in a VDC Group - a lock on parent VDC Group must be acquired
+	// To find out parent lock object, Edge Gateway must be looked up and its OwnerRef must be checked
+	// Note. It is not safe to do multiple locks in the same resource as it can result in a deadlock
+	parentEdgeGatewayOwnerId, _, err := getParentEdgeGatewayOwnerId(vcdClient, d)
+	if err != nil {
+		return diag.Errorf("[nsx-t ipsec vpn tunnel create/update] error finding parent Edge Gateway: %s", err)
+	}
+
+	if govcd.OwnerIsVdcGroup(parentEdgeGatewayOwnerId) {
+		vcdClient.lockById(parentEdgeGatewayOwnerId)
+		defer vcdClient.unlockById(parentEdgeGatewayOwnerId)
+	} else {
+		vcdClient.lockParentEdgeGtw(d)
+		defer vcdClient.unLockParentEdgeGtw(d)
+	}
 
 	orgName := d.Get("org").(string)
 	edgeGatewayId := d.Get("edge_gateway_id").(string)
@@ -387,8 +419,24 @@ func resourceVcdNsxtIpSecVpnTunnelRead(ctx context.Context, d *schema.ResourceDa
 
 func resourceVcdNsxtIpSecVpnTunnelDelete(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
 	vcdClient := meta.(*VCDClient)
-	vcdClient.lockParentEdgeGtw(d)
-	defer vcdClient.unLockParentEdgeGtw(d)
+
+	// Handling locks is conditional. There are two scenarios:
+	// * When the parent Edge Gateway is in a VDC - a lock on parent Edge Gateway must be acquired
+	// * When the parent Edge Gateway is in a VDC Group - a lock on parent VDC Group must be acquired
+	// To find out parent lock object, Edge Gateway must be looked up and its OwnerRef must be checked
+	// Note. It is not safe to do multiple locks in the same resource as it can result in a deadlock
+	parentEdgeGatewayOwnerId, _, err := getParentEdgeGatewayOwnerId(vcdClient, d)
+	if err != nil {
+		return diag.Errorf("[nsx-t ipsec vpn tunnel create/update] error finding parent Edge Gateway: %s", err)
+	}
+
+	if govcd.OwnerIsVdcGroup(parentEdgeGatewayOwnerId) {
+		vcdClient.lockById(parentEdgeGatewayOwnerId)
+		defer vcdClient.unlockById(parentEdgeGatewayOwnerId)
+	} else {
+		vcdClient.lockParentEdgeGtw(d)
+		defer vcdClient.unLockParentEdgeGtw(d)
+	}
 
 	orgName := d.Get("org").(string)
 	edgeGatewayId := d.Get("edge_gateway_id").(string)