Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug fix: do not update primary_ipv(4|6) values on netbox_virtual_machine #609

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ad8lmondy
Copy link
Contributor

@ad8lmondy ad8lmondy commented Jun 12, 2024

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 can cause Terraform plans that need two applies to work, with the first erroring out.
The fix is to remove the updates to the primaryIP(4|6)Values.

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.

@ad8lmondy ad8lmondy changed the title Bug fix: do not update primary_ip on VM Bug fix: do not update primary_ipv(4|6) values on netbox_virtual_machine Jun 12, 2024
@ad8lmondy
Copy link
Contributor Author

Seems related to this comment too: #538 (comment)

@ad8lmondy
Copy link
Contributor Author

ad8lmondy commented Jun 13, 2024

hmm, this isn't working quite as I hoped - I'm starting to loose primary IP on some of my VMs as I perform other operations.

I've 'fixed' it by adding a replace_triggered_by block to the netbox_primary_ip resource:

resource "netbox_primary_ip" "test" {
  ip_address_id      = resource.netbox_available_ip_address.ip_addr.id
  virtual_machine_id = resource.netbox_virtual_machine.vm.id
  lifecycle {
    replace_triggered_by = [
      netbox_virtual_machine.vm
    ]
  }

Which seems to work - though it's much harder when these things are being set in for_each blocks, for example.

I've had a look at the code in netbox/resource_netbox_primary_ip.go and netbox/resource_netbox_virtual_machine.go, and I can't really see how the netbox_virtual_machine update code would be able to find a netbox_primary_ip assosciated with the VM, without setting up an explicit cycle.

Or another way to put it: having the separate resource of netbox_primary_ip seems not very compatible with Terraform's model of things..? But I could easily be missing something!

A possible solution could be to allow the resource netbox_virtual_machine (or netbox_device, which I just remembered about :( ) to set the primary_ip value to the relevant netbox_ip_address id - though cycles might come back in doing that...

Any ideas or input welcome :)

@fbreckle
Copy link
Collaborator

As for your last question: Setting the primary ip directly in the VM does definitely not work, because it will create a loop. See here

@fbreckle fbreckle marked this pull request as draft June 17, 2024 06:48
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants