fix(network): multiple fixes to network_linux_bridge resource (#1095)

Signed-off-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com>

fwprovider/resource_linux_bridge.go

@@ -118,10 +118,10 @@ func (m *linuxBridgeResourceModel) importFromNetworkInterfaceList(
 		m.MTU = types.Int64Null()
 	}
 
+	// Comments can be set to an empty string in plant, which will translate to a "no value" in PVE
+	// So we don't want to set it to null if it's empty, as this will be indicated as a plan drift
 	if iface.Comments != nil {
 		m.Comment = types.StringValue(strings.TrimSpace(*iface.Comments))
-	} else {
-		m.Comment = types.StringNull()
 	}
 
 	if iface.BridgeVLANAware != nil {
@@ -372,11 +372,21 @@ func (r *linuxBridgeResource) Update(ctx context.Context, req resource.UpdateReq
 
 	var toDelete []string
 
-	if !plan.MTU.Equal(state.MTU) && (plan.MTU.IsUnknown() || plan.MTU.ValueInt64() == 0) {
+	if !plan.MTU.Equal(state.MTU) && plan.MTU.ValueInt64() == 0 {
 		toDelete = append(toDelete, "mtu")
 		body.MTU = nil
 	}
 
+	if !plan.Gateway.Equal(state.Gateway) && plan.Gateway.ValueString() == "" {
+		toDelete = append(toDelete, "gateway")
+		body.Gateway = nil
+	}
+
+	if !plan.Gateway6.Equal(state.Gateway6) && plan.Gateway6.ValueString() == "" {
+		toDelete = append(toDelete, "gateway6")
+		body.Gateway6 = nil
+	}
+
 	// VLANAware is computed, will never be null
 	if !plan.VLANAware.Equal(state.VLANAware) && !plan.VLANAware.ValueBool() {
 		toDelete = append(toDelete, "bridge_vlan_aware")

fwprovider/tests/resource_download_file_test.go

@@ -49,7 +49,7 @@ func TestAccResourceDownloadFile(t *testing.T) {
 					"size":           "3",
 					"verify":         "true",
 				}),
-				testNoResourceAttributes("proxmox_virtual_environment_download_file.iso_image", []string{
+				testNoResourceAttributesSet("proxmox_virtual_environment_download_file.iso_image", []string{
 					"checksum",
 					"checksum_algorithm",
 					"decompression_algorithm",
@@ -83,7 +83,7 @@ func TestAccResourceDownloadFile(t *testing.T) {
 					"checksum":           "688787d8ff144c502c7f5cffaafe2cc588d86079f9de88304c26b0cb99ce91c6",
 					"checksum_algorithm": "sha256",
 				}),
-				testNoResourceAttributes("proxmox_virtual_environment_download_file.qcow2_image", []string{
+				testNoResourceAttributesSet("proxmox_virtual_environment_download_file.qcow2_image", []string{
 					"decompression_algorithm",
 				}),
 			),
@@ -112,7 +112,7 @@ func TestAccResourceDownloadFile(t *testing.T) {
 					"size":           "3",
 					"verify":         "true",
 				}),
-				testNoResourceAttributes("proxmox_virtual_environment_download_file.iso_image", []string{
+				testNoResourceAttributesSet("proxmox_virtual_environment_download_file.iso_image", []string{
 					"checksum",
 					"checksum_algorithm",
 					"decompression_algorithm",
@@ -154,7 +154,7 @@ func TestAccResourceDownloadFile(t *testing.T) {
 					"size":         "3",
 					"verify":       "true",
 				}),
-				testNoResourceAttributes("proxmox_virtual_environment_download_file.iso_image", []string{
+				testNoResourceAttributesSet("proxmox_virtual_environment_download_file.iso_image", []string{
 					"checksum",
 					"checksum_algorithm",
 					"decompression_algorithm",

fwprovider/tests/resource_linux_bridge_test.go

@@ -16,10 +16,6 @@ import (
 	"github.com/hashicorp/terraform-plugin-testing/helper/resource"
 )
 
-const (
-	accTestLinuxBridgeName = "proxmox_virtual_environment_network_linux_bridge.test"
-)
-
 func TestAccResourceLinuxBridge(t *testing.T) {
 	accProviders := testAccMuxProviders(context.Background(), t)
 
@@ -33,76 +29,70 @@ func TestAccResourceLinuxBridge(t *testing.T) {
 		Steps: []resource.TestStep{
 			// Create and Read testing
 			{
-				Config: testAccResourceLinuxBridgeCreatedConfig(iface, ipV4cidr1),
+				Config: fmt.Sprintf(`
-				Check:  testAccResourceLinuxBridgeCreatedCheck(iface, ipV4cidr1),
+				resource "proxmox_virtual_environment_network_linux_bridge" "test" {
+					address = "%s"
+					autostart = true
+					comment = "created by terraform"
+					mtu = 1499
+					name = "%s"
+					node_name = "%s"
+					vlan_aware = true
+				}
+				`, ipV4cidr1, iface, accTestNodeName),
+				Check: resource.ComposeTestCheckFunc(
+					testResourceAttributes("proxmox_virtual_environment_network_linux_bridge.test", map[string]string{
+						"address":    ipV4cidr1,
+						"autostart":  "true",
+						"comment":    "created by terraform",
+						"mtu":        "1499",
+						"name":       iface,
+						"vlan_aware": "true",
+					}),
+					testResourceAttributesSet("proxmox_virtual_environment_network_linux_bridge.test", []string{
+						"id",
+					}),
+				),
 			},
 			// Update testing
 			{
-				Config: testAccResourceLinuxBridgeUpdatedConfig(iface, ipV4cidr2, ipV6cidr),
+				Config: fmt.Sprintf(`
-				Check:  testAccResourceLinuxBridgeUpdatedCheck(iface, ipV4cidr2, ipV6cidr),
+				resource "proxmox_virtual_environment_network_linux_bridge" "test" {
+					address = "%s"
+					address6 = "%s"
+					autostart = false
+					comment = ""
+					mtu = null
+					name = "%s"
+					node_name = "%s"
+					vlan_aware = false
+				}`, ipV4cidr2, ipV6cidr, iface, accTestNodeName),
+				Check: resource.ComposeTestCheckFunc(
+					testResourceAttributes("proxmox_virtual_environment_network_linux_bridge.test", map[string]string{
+						"address":    ipV4cidr2,
+						"address6":   ipV6cidr,
+						"autostart":  "false",
+						"comment":    "",
+						"name":       iface,
+						"vlan_aware": "false",
+					}),
+					testNoResourceAttributesSet("proxmox_virtual_environment_network_linux_bridge.test", []string{
+						"mtu",
+					}),
+					testResourceAttributesSet("proxmox_virtual_environment_network_linux_bridge.test", []string{
+						"id",
+					}),
+				),
 			},
 			// ImportState testing
 			{
-				ResourceName:      accTestLinuxBridgeName,
+				ResourceName:      "proxmox_virtual_environment_network_linux_bridge.test",
 				ImportState:       true,
 				ImportStateVerify: true,
+				ImportStateVerifyIgnore: []string{
+					"comment", // "" comments translates to null in the PVE, but nulls are not imported as empty strings.
+				},
 			},
 		},
 	})
 }
-
-func testAccResourceLinuxBridgeCreatedConfig(name string, ipV4cidr string) string {
-	return fmt.Sprintf(`
-	resource "proxmox_virtual_environment_network_linux_bridge" "test" {
-		address = "%s"
-		autostart = true
-		comment = "created by terraform"
-		mtu = 1499
-		name = "%s"
-		node_name = "%s"
-		vlan_aware = true
-	}
-	`, ipV4cidr, name, accTestNodeName)
-}
-
-func testAccResourceLinuxBridgeCreatedCheck(name string, ipV4cidr string) resource.TestCheckFunc {
-	return resource.ComposeTestCheckFunc(
-		resource.TestCheckResourceAttr(accTestLinuxBridgeName, "address", ipV4cidr),
-		resource.TestCheckResourceAttr(accTestLinuxBridgeName, "autostart", "true"),
-		resource.TestCheckResourceAttr(accTestLinuxBridgeName, "comment", "created by terraform"),
-		resource.TestCheckResourceAttr(accTestLinuxBridgeName, "mtu", "1499"),
-		resource.TestCheckResourceAttr(accTestLinuxBridgeName, "name", name),
-		resource.TestCheckResourceAttr(accTestLinuxBridgeName, "vlan_aware", "true"),
-		resource.TestCheckResourceAttrSet(accTestLinuxBridgeName, "id"),
-	)
-}
-
-func testAccResourceLinuxBridgeUpdatedConfig(name string, ipV4cidr string, ipV6cidr string) string {
-	return fmt.Sprintf(`
-	resource "proxmox_virtual_environment_network_linux_bridge" "test" {
-		address = "%s"
-		address6 = "%s"
-		autostart = false
-		comment = "updated by terraform"
-		mtu = null
-		name = "%s"
-		node_name = "%s"
-		vlan_aware = false
-	}
-	`, ipV4cidr, ipV6cidr, name, accTestNodeName)
-}
-
-func testAccResourceLinuxBridgeUpdatedCheck(name string, ipV4cidr string, ipV6cidr string) resource.TestCheckFunc {
-	return resource.ComposeTestCheckFunc(
-		resource.ComposeAggregateTestCheckFunc(
-			resource.TestCheckResourceAttr(accTestLinuxBridgeName, "address", ipV4cidr),
-			resource.TestCheckResourceAttr(accTestLinuxBridgeName, "address6", ipV6cidr),
-			resource.TestCheckResourceAttr(accTestLinuxBridgeName, "autostart", "false"),
-			resource.TestCheckResourceAttr(accTestLinuxBridgeName, "comment", "updated by terraform"),
-			resource.TestCheckResourceAttr(accTestLinuxBridgeName, "name", name),
-			resource.TestCheckResourceAttr(accTestLinuxBridgeName, "vlan_aware", "false"),
-			resource.TestCheckNoResourceAttr(accTestLinuxBridgeName, "mtu"),
-			resource.TestCheckResourceAttrSet(accTestLinuxBridgeName, "id"),
-		),
-	)
-}

fwprovider/tests/resource_user_test.go

@@ -51,14 +51,12 @@ func TestAccResourceUser(t *testing.T) {
 					  first_name 		= "First One"
 				}
 				`,
-			Check: resource.ComposeTestCheckFunc(
+			Check: testResourceAttributes("proxmox_virtual_environment_user.user1", map[string]string{
-				testResourceAttributes("proxmox_virtual_environment_user.user1", map[string]string{
+				"enabled":         "false",
-					"enabled":         "false",
+				"expiration_date": "2035-01-01T22:00:00Z",
-					"expiration_date": "2035-01-01T22:00:00Z",
+				"first_name":      "First One",
-					"first_name":      "First One",
+				"user_id":         "user1@pve",
-					"user_id":         "user1@pve",
+			}),
-				}),
-			),
 		}}},
 	}
 

fwprovider/tests/resource_vm_test.go

@@ -198,10 +198,9 @@ func TestAccResourceVMDisks(t *testing.T) {
 				}`,
 			Check: resource.ComposeTestCheckFunc(
 				testResourceAttributes("proxmox_virtual_environment_vm.test_disk1", map[string]string{
-					// those are empty by default, but we can't check for that
+					"disk.0.cache":             "none",
-					// "disk.0.cache":          "",
+					"disk.0.discard":           "ignore",
-					// "disk.0.discard":      	"",
+					"disk.0.file_id":           "",
-					// "disk.0.file_id":        "",
 					"disk.0.datastore_id":      "local-lvm",
 					"disk.0.file_format":       "raw",
 					"disk.0.interface":         "virtio0",

fwprovider/tests/test_support.go

@@ -121,6 +121,14 @@ func getNodeStorageClient() *storage.Client {
 func testResourceAttributes(res string, attrs map[string]string) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
 		for k, v := range attrs {
+			if v == "" {
+				if err := resource.TestCheckResourceAttr(res, k, "")(s); err != nil {
+					return fmt.Errorf("expected '%s' to be empty: %w", k, err)
+				}
+
+				continue
+			}
+
 			if err := resource.TestCheckResourceAttrWith(res, k, func(got string) error {
 				match, err := regexp.Match(v, []byte(got)) //nolint:mirror
 				if err != nil {
@@ -139,7 +147,7 @@ func testResourceAttributes(res string, attrs map[string]string) resource.TestCh
 	}
 }
 
-func testNoResourceAttributes(res string, attrs []string) resource.TestCheckFunc {
+func testNoResourceAttributesSet(res string, attrs []string) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
 		for _, k := range attrs {
 			if err := resource.TestCheckNoResourceAttr(res, k)(s); err != nil {
@@ -151,6 +159,18 @@ func testNoResourceAttributes(res string, attrs []string) resource.TestCheckFunc
 	}
 }
 
+func testResourceAttributesSet(res string, attrs []string) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		for _, k := range attrs {
+			if err := resource.TestCheckResourceAttrSet(res, k)(s); err != nil {
+				return err
+			}
+		}
+
+		return nil
+	}
+}
+
 func getProviderConfig(t *testing.T) string {
 	t.Helper()