From 4aed7cb085c87aed68f3dc426644a9d76c075db1 Mon Sep 17 00:00:00 2001 From: Pavel Boldyrev <627562+bpg@users.noreply.github.com> Date: Fri, 8 Mar 2024 19:47:43 -0500 Subject: [PATCH] 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 | 16 ++- .../tests/resource_download_file_test.go | 8 +- .../tests/resource_linux_bridge_test.go | 120 ++++++++---------- fwprovider/tests/resource_user_test.go | 14 +- fwprovider/tests/resource_vm_test.go | 7 +- fwprovider/tests/test_support.go | 22 +++- 6 files changed, 102 insertions(+), 85 deletions(-) diff --git a/fwprovider/resource_linux_bridge.go b/fwprovider/resource_linux_bridge.go index 454799c9..e7ec4b9d 100644 --- a/fwprovider/resource_linux_bridge.go +++ b/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") diff --git a/fwprovider/tests/resource_download_file_test.go b/fwprovider/tests/resource_download_file_test.go index e3d0d183..35651d1e 100644 --- a/fwprovider/tests/resource_download_file_test.go +++ b/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", diff --git a/fwprovider/tests/resource_linux_bridge_test.go b/fwprovider/tests/resource_linux_bridge_test.go index ff82ed15..452bf944 100644 --- a/fwprovider/tests/resource_linux_bridge_test.go +++ b/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), - Check: testAccResourceLinuxBridgeCreatedCheck(iface, ipV4cidr1), + Config: 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 + } + `, 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), - Check: testAccResourceLinuxBridgeUpdatedCheck(iface, ipV4cidr2, ipV6cidr), + Config: fmt.Sprintf(` + 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"), - ), - ) -} diff --git a/fwprovider/tests/resource_user_test.go b/fwprovider/tests/resource_user_test.go index 2bd5d6a0..cbb77391 100644 --- a/fwprovider/tests/resource_user_test.go +++ b/fwprovider/tests/resource_user_test.go @@ -51,14 +51,12 @@ func TestAccResourceUser(t *testing.T) { first_name = "First One" } `, - Check: resource.ComposeTestCheckFunc( - testResourceAttributes("proxmox_virtual_environment_user.user1", map[string]string{ - "enabled": "false", - "expiration_date": "2035-01-01T22:00:00Z", - "first_name": "First One", - "user_id": "user1@pve", - }), - ), + Check: testResourceAttributes("proxmox_virtual_environment_user.user1", map[string]string{ + "enabled": "false", + "expiration_date": "2035-01-01T22:00:00Z", + "first_name": "First One", + "user_id": "user1@pve", + }), }}}, } diff --git a/fwprovider/tests/resource_vm_test.go b/fwprovider/tests/resource_vm_test.go index 38537a49..fbae06ac 100644 --- a/fwprovider/tests/resource_vm_test.go +++ b/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": "", - // "disk.0.discard": "", - // "disk.0.file_id": "", + "disk.0.cache": "none", + "disk.0.discard": "ignore", + "disk.0.file_id": "", "disk.0.datastore_id": "local-lvm", "disk.0.file_format": "raw", "disk.0.interface": "virtio0", diff --git a/fwprovider/tests/test_support.go b/fwprovider/tests/test_support.go index 1db9b81c..435ab9e8 100644 --- a/fwprovider/tests/test_support.go +++ b/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()