From 2758433450873ed096151da7c75e2a29d59a0f10 Mon Sep 17 00:00:00 2001 From: Luke Mondy Date: Wed, 12 Jun 2024 22:11:10 +1000 Subject: [PATCH] Bug fix: do not update primary_ip on VM TL;DR: the `primary_ipv4` and `primary_ipv6` properties on the netbox_virtual_machine resource create an implicit loop, since these values are detemined by making a netbox_primary_ip resource (which itself must explicitly depend on the netbox_virtual_machine). This is can cause Terraform plans that need two applies to work. Longer version: Creating a netbox_primary_ip resource implicitly modifies the related netbox_virtual_machine, since the latter keeps a record of the primaryIP4Value and primaryIP6Value - but this only happens when resourceNetboxVirtualMachineRead is called, so this info lags behind. However, it normally works fine, but when the netbox_virtual_machine resource is modified, and the netbox_primary_ip is deleted, it leads to an issue where when NetBox tries to update the VM, it can no longer find the IP address ID. This is because netbox_virtual_machine doesn't know it has an implicit dependency on the netbox_primary_ip, while at the same time, netbox_primary_ip has an explicit dependency on the netbox_virtual_machine resource (note that this is a loop). Thus, Terraform reads the state of netbox_virtual_machine, including the primary IPs, then deletes the netbox_primary_ip resource, and then (due to the explicit dependency) tries to modify the netbox_virtual_machine resource with now out-of-date info about the primaryIP(4|6)Values. The solution is to remove updating the primaryIP(4|6)Values when updating a VM. If a netbox_primary_ip resource is defined, the relationship is still preserved - but as before this fix, the info lags behind, since it's only updated on a resourceNetboxVirtualMachineRead. I don't think we can do much about that for now, but this at least removes an actual error that can be hit. Two tests were added in regards to this issue: - TestAccNetboxVirtualMachine_removePrimaryIPAddress - TestAccNetboxVirtualMachine_changePrimaryIPAddress They are a little odd, in that they have repeated steps, which is there to force a call to resourceNetboxVirtualMachineRead. When the code in netbox/resource_netbox_virtual_machine.go still has the calls to update primaryIP(4|6)Values in resourceNetboxVirtualMachineUpdate, the remove_primaryIPAddress test fails. The changePrimaryIPAddress was put in just to ensure the values still could be retrieved after the code was removed. --- netbox/resource_netbox_virtual_machine.go | 12 - .../resource_netbox_virtual_machine_test.go | 223 ++++++++++++++++++ 2 files changed, 223 insertions(+), 12 deletions(-) diff --git a/netbox/resource_netbox_virtual_machine.go b/netbox/resource_netbox_virtual_machine.go index 2d1ae4c7..cc53755a 100644 --- a/netbox/resource_netbox_virtual_machine.go +++ b/netbox/resource_netbox_virtual_machine.go @@ -387,18 +387,6 @@ func resourceNetboxVirtualMachineUpdate(ctx context.Context, d *schema.ResourceD data.Disk = &diskSize } - primaryIP4Value, ok := d.GetOk("primary_ipv4") - if ok { - primaryIP4 := int64(primaryIP4Value.(int)) - data.PrimaryIp4 = &primaryIP4 - } - - primaryIP6Value, ok := d.GetOk("primary_ipv6") - if ok { - primaryIP6 := int64(primaryIP6Value.(int)) - data.PrimaryIp6 = &primaryIP6 - } - localContextValue, ok := d.GetOk("local_context_data") if ok { var jsonObj any diff --git a/netbox/resource_netbox_virtual_machine_test.go b/netbox/resource_netbox_virtual_machine_test.go index 4d6746bc..5c1f1e63 100644 --- a/netbox/resource_netbox_virtual_machine_test.go +++ b/netbox/resource_netbox_virtual_machine_test.go @@ -275,6 +275,229 @@ resource "netbox_virtual_machine" "test" { }) } +func TestAccNetboxVirtualMachine_removePrimaryIPAddress(t *testing.T) { + testSlug := "vm_removePrimaryIPAddress" + testName := testAccGetTestName(testSlug) + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckVirtualMachineDestroy, + Steps: []resource.TestStep{ + { + Config: testAccNetboxVirtualMachineFullDependencies(testName) + fmt.Sprintf(` +resource "netbox_virtual_machine" "test" { + name = "%s" + cluster_id = netbox_cluster.test.id + site_id = netbox_site.test.id +} +resource "netbox_interface" "test" { + name = "test" + virtual_machine_id = netbox_virtual_machine.test.id +} +resource "netbox_ip_address" "test" { + ip_address = "10.0.0.60/24" + status = "active" + virtual_machine_interface_id = netbox_interface.test.id +} +resource "netbox_primary_ip" "test" { + ip_address_id = netbox_ip_address.test.id + virtual_machine_id = netbox_virtual_machine.test.id +}`, testName), + }, + // A repeated second step is required, so that the resource "netbox_virtual_machine" "test" goes through a resourceNetboxVirtualMachineRead cycle + // This is needed because adding a netbox_primary_ip implicitly updates the netbox_virtual_machine + { + Config: testAccNetboxVirtualMachineFullDependencies(testName) + fmt.Sprintf(` +resource "netbox_virtual_machine" "test" { + name = "%s" + cluster_id = netbox_cluster.test.id + site_id = netbox_site.test.id +} +resource "netbox_interface" "test" { + name = "test" + virtual_machine_id = netbox_virtual_machine.test.id +} +resource "netbox_ip_address" "test" { + ip_address = "10.0.0.60/24" + status = "active" + virtual_machine_interface_id = netbox_interface.test.id +} +resource "netbox_primary_ip" "test" { + ip_address_id = netbox_ip_address.test.id + virtual_machine_id = netbox_virtual_machine.test.id +}`, testName), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttrPair("netbox_virtual_machine.test", "primary_ipv4", "netbox_ip_address.test", "id"), + ), + }, + // Now the netbox_primary_ip is removed AND the netbox_virtual_machine is modified + { + Config: testAccNetboxVirtualMachineFullDependencies(testName) + fmt.Sprintf(` +resource "netbox_virtual_machine" "test" { + name = "%s_modified" + cluster_id = netbox_cluster.test.id + site_id = netbox_site.test.id +}`, testName), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("netbox_virtual_machine.test", "name", testName+"_modified"), + resource.TestCheckResourceAttr("netbox_virtual_machine.test", "primary_ipv4", "0"), + ), + }, + { + ResourceName: "netbox_virtual_machine.test", + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +func TestAccNetboxVirtualMachine_changePrimaryIPAddress(t *testing.T) { + testSlug := "vm_changePrimaryIPAddress" + testName := testAccGetTestName(testSlug) + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckVirtualMachineDestroy, + Steps: []resource.TestStep{ + { + Config: testAccNetboxVirtualMachineFullDependencies(testName) + fmt.Sprintf(` +resource "netbox_virtual_machine" "test" { + name = "%s" + cluster_id = netbox_cluster.test.id + site_id = netbox_site.test.id +} +resource "netbox_interface" "test1" { + name = "test1" + virtual_machine_id = netbox_virtual_machine.test.id +} +resource "netbox_ip_address" "test1" { + ip_address = "10.0.0.60/24" + status = "active" + virtual_machine_interface_id = netbox_interface.test1.id +} +resource "netbox_interface" "test2" { + name = "test2" + virtual_machine_id = netbox_virtual_machine.test.id +} +resource "netbox_ip_address" "test2" { + ip_address = "10.0.0.61/24" + status = "active" + virtual_machine_interface_id = netbox_interface.test2.id +} +resource "netbox_primary_ip" "test" { + ip_address_id = netbox_ip_address.test1.id + virtual_machine_id = netbox_virtual_machine.test.id +}`, testName), + }, + // A repeated second step is required, so that the resource "netbox_virtual_machine" "test" goes through a resourceNetboxVirtualMachineRead cycle + // This is needed because adding a netbox_primary_ip implicitly updates the netbox_virtual_machine + { + Config: testAccNetboxVirtualMachineFullDependencies(testName) + fmt.Sprintf(` +resource "netbox_virtual_machine" "test" { + name = "%s" + cluster_id = netbox_cluster.test.id + site_id = netbox_site.test.id +} +resource "netbox_interface" "test1" { + name = "test1" + virtual_machine_id = netbox_virtual_machine.test.id +} +resource "netbox_ip_address" "test1" { + ip_address = "10.0.0.60/24" + status = "active" + virtual_machine_interface_id = netbox_interface.test1.id +} +resource "netbox_interface" "test2" { + name = "test2" + virtual_machine_id = netbox_virtual_machine.test.id +} +resource "netbox_ip_address" "test2" { + ip_address = "10.0.0.61/24" + status = "active" + virtual_machine_interface_id = netbox_interface.test2.id +} +resource "netbox_primary_ip" "test" { + ip_address_id = netbox_ip_address.test1.id + virtual_machine_id = netbox_virtual_machine.test.id +}`, testName), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttrPair("netbox_virtual_machine.test", "primary_ipv4", "netbox_ip_address.test1", "id"), + ), + }, + { + Config: testAccNetboxVirtualMachineFullDependencies(testName) + fmt.Sprintf(` +resource "netbox_virtual_machine" "test" { + name = "%s" + cluster_id = netbox_cluster.test.id + site_id = netbox_site.test.id +} +resource "netbox_interface" "test1" { + name = "test1" + virtual_machine_id = netbox_virtual_machine.test.id +} +resource "netbox_ip_address" "test1" { + ip_address = "10.0.0.60/24" + status = "active" + virtual_machine_interface_id = netbox_interface.test1.id +} +resource "netbox_interface" "test2" { + name = "test2" + virtual_machine_id = netbox_virtual_machine.test.id +} +resource "netbox_ip_address" "test2" { + ip_address = "10.0.0.61/24" + status = "active" + virtual_machine_interface_id = netbox_interface.test2.id +} +resource "netbox_primary_ip" "test" { + ip_address_id = netbox_ip_address.test2.id + virtual_machine_id = netbox_virtual_machine.test.id +}`, testName), + }, + // Again, a repeated step is required, so that the resource "netbox_virtual_machine" "test" goes through a resourceNetboxVirtualMachineRead cycle + { + Config: testAccNetboxVirtualMachineFullDependencies(testName) + fmt.Sprintf(` +resource "netbox_virtual_machine" "test" { + name = "%s" + cluster_id = netbox_cluster.test.id + site_id = netbox_site.test.id +} +resource "netbox_interface" "test1" { + name = "test1" + virtual_machine_id = netbox_virtual_machine.test.id +} +resource "netbox_ip_address" "test1" { + ip_address = "10.0.0.60/24" + status = "active" + virtual_machine_interface_id = netbox_interface.test1.id +} +resource "netbox_interface" "test2" { + name = "test2" + virtual_machine_id = netbox_virtual_machine.test.id +} +resource "netbox_ip_address" "test2" { + ip_address = "10.0.0.61/24" + status = "active" + virtual_machine_interface_id = netbox_interface.test2.id +} +resource "netbox_primary_ip" "test" { + ip_address_id = netbox_ip_address.test2.id + virtual_machine_id = netbox_virtual_machine.test.id +}`, testName), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttrPair("netbox_virtual_machine.test", "primary_ipv4", "netbox_ip_address.test2", "id"), + ), + }, + { + ResourceName: "netbox_virtual_machine.test", + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + func TestAccNetboxVirtualMachine_fractionalVcpu(t *testing.T) { testSlug := "vm_fracVcpu" testName := testAccGetTestName(testSlug)