diff --git a/fwprovider/test/resource_vm_test.go b/fwprovider/test/resource_vm_test.go index 210bda4bc..22c52f28a 100644 --- a/fwprovider/test/resource_vm_test.go +++ b/fwprovider/test/resource_vm_test.go @@ -739,6 +739,188 @@ func TestAccResourceVMNetwork(t *testing.T) { ), }, }}, + {"remove network device", []resource.TestStep{ + { + SkipFunc: func() (bool, error) { + // backward incompatibility with the current implementation of clone + // see https://github.com/bpg/terraform-provider-proxmox/pull/2260 + return true, nil + }, + Config: te.RenderConfig(` + resource "proxmox_virtual_environment_vm" "test_vm" { + node_name = "{{.NodeName}}" + started = false + + network_device { + bridge = "vmbr0" + } + }`), + Check: resource.ComposeTestCheckFunc( + ResourceAttributes("proxmox_virtual_environment_vm.test_vm", map[string]string{ + "network_device.#": "1", + "network_device.0.bridge": "vmbr0", + }), + ), + }, + { + SkipFunc: func() (bool, error) { + // backward incompatibility with the current implementation of clone + // see https://github.com/bpg/terraform-provider-proxmox/pull/2260 + return true, nil + }, + Config: te.RenderConfig(` + resource "proxmox_virtual_environment_vm" "test_vm" { + node_name = "{{.NodeName}}" + started = false + }`), + Check: resource.ComposeTestCheckFunc( + ResourceAttributes("proxmox_virtual_environment_vm.test_vm", map[string]string{ + "network_device.#": "0", + }), + ), + }, + }}, + {"multiple network devices removal", []resource.TestStep{ + { + SkipFunc: func() (bool, error) { + // backward incompatibility with the current implementation of clone + // see https://github.com/bpg/terraform-provider-proxmox/pull/2260 + return true, nil + }, + Config: te.RenderConfig(` + resource "proxmox_virtual_environment_vm" "test_vm" { + node_name = "{{.NodeName}}" + started = false + + network_device { + bridge = "vmbr0" + model = "virtio" + } + + network_device { + bridge = "vmbr1" + model = "virtio" + } + }`), + Check: resource.ComposeTestCheckFunc( + ResourceAttributes("proxmox_virtual_environment_vm.test_vm", map[string]string{ + "network_device.#": "2", + "network_device.0.bridge": "vmbr0", + "network_device.1.bridge": "vmbr1", + }), + ), + }, + { + SkipFunc: func() (bool, error) { + // backward incompatibility with the current implementation of clone + // see https://github.com/bpg/terraform-provider-proxmox/pull/2260 + return true, nil + }, + Config: te.RenderConfig(` + resource "proxmox_virtual_environment_vm" "test_vm" { + node_name = "{{.NodeName}}" + started = false + + # Only keep the first network device + network_device { + bridge = "vmbr0" + model = "virtio" + } + }`), + Check: resource.ComposeTestCheckFunc( + ResourceAttributes("proxmox_virtual_environment_vm.test_vm", map[string]string{ + "network_device.#": "1", + "network_device.0.bridge": "vmbr0", + }), + ), + }, + { + SkipFunc: func() (bool, error) { + // backward incompatibility with the current implementation of clone + // see https://github.com/bpg/terraform-provider-proxmox/pull/2260 + return true, nil + }, + Config: te.RenderConfig(` + resource "proxmox_virtual_environment_vm" "test_vm" { + node_name = "{{.NodeName}}" + started = false + }`), + Check: resource.ComposeTestCheckFunc( + ResourceAttributes("proxmox_virtual_environment_vm.test_vm", map[string]string{ + "network_device.#": "0", + }), + ), + }, + }}, + {"network device state consistency", []resource.TestStep{ + { + SkipFunc: func() (bool, error) { + // backward incompatibility with the current implementation of clone + // see https://github.com/bpg/terraform-provider-proxmox/pull/2260 + return true, nil + }, + Config: te.RenderConfig(` + resource "proxmox_virtual_environment_vm" "test_vm" { + node_name = "{{.NodeName}}" + started = false + + network_device { + bridge = "vmbr0" + model = "virtio" + } + }`), + Check: resource.ComposeTestCheckFunc( + ResourceAttributes("proxmox_virtual_environment_vm.test_vm", map[string]string{ + "network_device.#": "1", + "network_device.0.bridge": "vmbr0", + "network_device.0.model": "virtio", + }), + ), + }, + { + SkipFunc: func() (bool, error) { + // backward incompatibility with the current implementation of clone + // see https://github.com/bpg/terraform-provider-proxmox/pull/2260 + return true, nil + }, + // This step tests that the state is read correctly after network device removal + Config: te.RenderConfig(` + resource "proxmox_virtual_environment_vm" "test_vm" { + node_name = "{{.NodeName}}" + started = false + }`), + Check: resource.ComposeTestCheckFunc( + ResourceAttributes("proxmox_virtual_environment_vm.test_vm", map[string]string{ + "network_device.#": "0", + }), + ), + }, + { + SkipFunc: func() (bool, error) { + // backward incompatibility with the current implementation of clone + // see https://github.com/bpg/terraform-provider-proxmox/pull/2260 + return true, nil + }, + // This step tests that we can add network devices back after removal + Config: te.RenderConfig(` + resource "proxmox_virtual_environment_vm" "test_vm" { + node_name = "{{.NodeName}}" + started = false + + network_device { + bridge = "vmbr0" + model = "virtio" + } + }`), + Check: resource.ComposeTestCheckFunc( + ResourceAttributes("proxmox_virtual_environment_vm.test_vm", map[string]string{ + "network_device.#": "1", + "network_device.0.bridge": "vmbr0", + "network_device.0.model": "virtio", + }), + ), + }, + }}, } for _, tt := range tests { @@ -821,6 +1003,29 @@ func TestAccResourceVMClone(t *testing.T) { }), ), }}}, + {"clone with network devices", []resource.TestStep{{ + Config: te.RenderConfig(` + resource "proxmox_virtual_environment_vm" "template" { + node_name = "{{.NodeName}}" + started = false + network_device { + bridge = "vmbr0" + } + } + resource "proxmox_virtual_environment_vm" "clone" { + node_name = "{{.NodeName}}" + started = false + clone { + vm_id = proxmox_virtual_environment_vm.template.vm_id + } + }`), + Check: resource.ComposeTestCheckFunc( + ResourceAttributes("proxmox_virtual_environment_vm.clone", map[string]string{ + "network_device.#": "1", + "network_device.0.bridge": "vmbr0", + }), + ), + }}}, {"clone initialization datastore does not exist", []resource.TestStep{{ Config: te.RenderConfig(` resource "proxmox_virtual_environment_vm" "template" { diff --git a/proxmoxtf/provider/provider.go b/proxmoxtf/provider/provider.go index 1315d09a7..959e5035a 100644 --- a/proxmoxtf/provider/provider.go +++ b/proxmoxtf/provider/provider.go @@ -89,7 +89,7 @@ func providerConfigure(ctx context.Context, d *schema.ResourceData) (interface{} otp = v.(string) } - ///nolint:staticcheck + //nolint:staticcheck if v, ok := d.GetOkExists(mkProviderUsername); ok { username = v.(string) } diff --git a/proxmoxtf/resource/vm/network/network.go b/proxmoxtf/resource/vm/network/network.go index ba6eb83b9..9264718a3 100644 --- a/proxmoxtf/resource/vm/network/network.go +++ b/proxmoxtf/resource/vm/network/network.go @@ -97,16 +97,21 @@ func GetNetworkDeviceObjects(d *schema.ResourceData) (vms.CustomNetworkDevices, return networkDeviceObjects, nil } -// ReadNetworkDeviceObjects reads the network device objects from the resource data. +func valueOrDefault[T any](v *T, def T) T { + if v == nil { + return def + } + + return *v +} + +// ReadNetworkDeviceObjects reads the network device objects from the response data. func ReadNetworkDeviceObjects(d *schema.ResourceData, vmConfig *vms.GetResponseData) diag.Diagnostics { var diags diag.Diagnostics - // Compare the network devices to those stored in the state. - currentNetworkDeviceList := d.Get(MkNetworkDevice).([]interface{}) + macAddresses := make([]interface{}, 0) + networkDevices := make([]interface{}, 0) - macAddresses := make([]interface{}, MaxNetworkDevices) - networkDeviceLast := -1 - networkDeviceList := make([]interface{}, MaxNetworkDevices) networkDeviceObjects := []*vms.CustomNetworkDevice{ vmConfig.NetworkDevice0, vmConfig.NetworkDevice1, @@ -142,85 +147,42 @@ func ReadNetworkDeviceObjects(d *schema.ResourceData, vmConfig *vms.GetResponseD vmConfig.NetworkDevice31, } - for ni, nd := range networkDeviceObjects { - networkDevice := map[string]interface{}{} - - if nd != nil { - networkDeviceLast = ni - - if nd.Bridge != nil { - networkDevice[mkNetworkDeviceBridge] = *nd.Bridge - } else { - networkDevice[mkNetworkDeviceBridge] = "" - } - - networkDevice[mkNetworkDeviceEnabled] = nd.Enabled - - if nd.LinkDown != nil { - networkDevice[mkNetworkDeviceDisconnected] = *nd.LinkDown - } else { - networkDevice[mkNetworkDeviceDisconnected] = false - } - - if nd.Firewall != nil { - networkDevice[mkNetworkDeviceFirewall] = *nd.Firewall - } else { - networkDevice[mkNetworkDeviceFirewall] = false - } - - if nd.MACAddress != nil { - macAddresses[ni] = *nd.MACAddress - } else { - macAddresses[ni] = "" - } - - networkDevice[mkNetworkDeviceMACAddress] = macAddresses[ni] - networkDevice[mkNetworkDeviceModel] = nd.Model - - if nd.Queues != nil { - networkDevice[mkNetworkDeviceQueues] = *nd.Queues - } else { - networkDevice[mkNetworkDeviceQueues] = 0 - } - - if nd.RateLimit != nil { - networkDevice[mkNetworkDeviceRateLimit] = *nd.RateLimit - } else { - networkDevice[mkNetworkDeviceRateLimit] = 0 - } - - if nd.Tag != nil { - networkDevice[mkNetworkDeviceVLANID] = nd.Tag - } else { - networkDevice[mkNetworkDeviceVLANID] = 0 - } - - if nd.Trunks != nil { - networkDevice[mkNetworkDeviceTrunks] = strings.Trim( - strings.Join(strings.Fields(fmt.Sprint(nd.Trunks)), ";"), "[]") - } else { - networkDevice[mkNetworkDeviceTrunks] = "" - } + for len(networkDeviceObjects) > 0 && networkDeviceObjects[len(networkDeviceObjects)-1] == nil { + // drop + networkDeviceObjects = networkDeviceObjects[:len(networkDeviceObjects)-1] + } - if nd.MTU != nil { - networkDevice[mkNetworkDeviceMTU] = nd.MTU - } else { - networkDevice[mkNetworkDeviceMTU] = 0 - } + for _, netDevice := range networkDeviceObjects { + if netDevice == nil { + networkDevices = append(networkDevices, nil) + macAddresses = append(macAddresses, "") } else { - macAddresses[ni] = "" - networkDevice[mkNetworkDeviceEnabled] = false - } - - networkDeviceList[ni] = networkDevice - } + networkDevices = append(networkDevices, map[string]interface{}{ + mkNetworkDeviceBridge: valueOrDefault(netDevice.Bridge, ""), + mkNetworkDeviceEnabled: netDevice.Enabled, + mkNetworkDeviceDisconnected: valueOrDefault(netDevice.LinkDown, false), + mkNetworkDeviceFirewall: valueOrDefault(netDevice.Firewall, false), + mkNetworkDeviceMACAddress: valueOrDefault(netDevice.MACAddress, ""), + mkNetworkDeviceModel: netDevice.Model, + mkNetworkDeviceQueues: valueOrDefault(netDevice.Queues, 0), + mkNetworkDeviceRateLimit: valueOrDefault(netDevice.RateLimit, 0), + mkNetworkDeviceVLANID: valueOrDefault(netDevice.Tag, 0), + mkNetworkDeviceMTU: valueOrDefault(netDevice.MTU, 0), + mkNetworkDeviceTrunks: func(trunks []int) string { + if trunks == nil { + return "" + } - if len(currentNetworkDeviceList) > 0 || networkDeviceLast > -1 { - err := d.Set(MkNetworkDevice, networkDeviceList[:networkDeviceLast+1]) - diags = append(diags, diag.FromErr(err)...) + return strings.Trim(strings.Join(strings.Fields(fmt.Sprint(trunks)), ";"), "[]") + }(netDevice.Trunks), + }) + macAddresses = append(macAddresses, valueOrDefault(netDevice.MACAddress, "")) + } } - err := d.Set(mkMACAddresses, macAddresses[0:len(currentNetworkDeviceList)]) + err := d.Set(MkNetworkDevice, networkDevices) + diags = append(diags, diag.FromErr(err)...) + err = d.Set(mkMACAddresses, macAddresses) diags = append(diags, diag.FromErr(err)...) return diags