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

Fix empty cloud init ipconfig #238

Merged
merged 1 commit into from
Mar 2, 2023

Conversation

tnyeanderson
Copy link
Contributor

@tnyeanderson tnyeanderson commented Feb 12, 2023

For the life of me I cannot figure out what is happening... but empty (non-existent) config.Ipconfig entries are being unmarshaled as empty strings instead of nil which causes the insurmountable error below:

proxmox_vm_qemu.docker-test: Creating...
╷
│ Error: cloud-init parameters only supported on clones or updates
│
│   with proxmox_vm_qemu.docker-test,
│   on docker-test.tf line 1, in resource "proxmox_vm_qemu" "docker-test":
│    1: resource "proxmox_vm_qemu" "docker-test" {
│
╵

Below is the docker-test terraform config:

resource "proxmox_vm_qemu" "docker-test" {
  name        = "docker-test"
  target_node = "node1"
  boot        = "order=ide2;scsi0;net0"
  pxe         = true
  cores       = 4
  memory      = 8192
  network {
    bridge   = "vmbr0"
    firewall = true
    model    = "virtio"
    macaddr  = "<redacted>"
  }
  disk {
    type    = "scsi"
    storage = "local-lvm"
    size    = "8G"
  }
}

I have tried bisecting the issue and cannot pin down where it was introduced or why it is happening. But the included fix makes it work for me.

P.S. I was able to determine that, in my case, none of the Ipconfig entries were unmarshaled as anything except an empty string (no nil, and no actual definitions).

@tnyeanderson tnyeanderson force-pushed the fix-cloud-init-ipconfig branch from 14988ee to 9776008 Compare February 12, 2023 18:18
@sebastian-de
Copy link
Contributor

Hi @tnyeanderson, I changed the Ipconfig handling in this PR: #216

I can't reproduce your issue here. Running proxmox-api-go -debug createQemu 125 pve < test_vm.json with the following config creates a VM without problems.

{
  "name": "golang1.test.com",
  "desc": "Test proxmox-api-go",
  "memory": 2048,
  "os": "l26",
  "cores": 2,
  "sockets": 1,
  "network": {
    "0": {
      "model": "virtio",
      "bridge": "vmbr0"
    }
  }
}

Please test if your issue occurs when you use proxmox-api-go directly. If not I suspect this is an issue with the Terraform provider.

@tnyeanderson
Copy link
Contributor Author

tnyeanderson commented Feb 16, 2023

Thank you for taking a look, Sebastian :)

I have a feeling it was introduced in Telmate/terraform-provider-proxmox#653

I still think this PR might be valid based on the diff from #216 (the checks for empty strings were replaced with a nil check) so I'm not closing it yet. But I will put it in draft while I figure out what happened on the provider side.

Running into the "things aren't strongly typed" hurdle again here, so I'll need to take some time to understand what is going on in the code. Speaking of which, is there a reason IpconfigMap is a map[int]interface{} instead of a map[int]string, given that the datatype of the values that were converted to a map were originally strings themselves?

@tnyeanderson tnyeanderson marked this pull request as draft February 16, 2023 04:22
@sebastian-de
Copy link
Contributor

I can remember that I tried using map[int]string, but I can't remember why I ended up using map[int]interface{} instead, sorry.
Looking at it again now, it really should work with map[int]string...

@tnyeanderson
Copy link
Contributor Author

Found the provider fix in Telmate/terraform-provider-proxmox#696, but I do think this PR is still relevant. Currently, the nil check is being performed on what is, really, a string (or nil). Before #216, the check was "is the string empty".

If an empty string ipconfig value should not be applied (as it wasn't being applied before), then we should check for that condition and account for it, which this PR does

@tnyeanderson tnyeanderson marked this pull request as ready for review February 19, 2023 06:23
@sebastian-de
Copy link
Contributor

Hey @tnyeanderson thanks for taking a look at the Terraform provider. Since your change seems harmless - ack from my side.
But You'll have to wait for the maintainer to merge your changes.

@GannonTdW
Copy link

Hello,

I reproduce this issue.

proxmox_vm_qemu.pxe-minimal-example: Creating...

│ Error: cloud-init parameters only supported on clones or updates

│ with proxmox_vm_qemu.pxe-minimal-example,
│ on main.tf line 22, in resource "proxmox_vm_qemu" "pxe-minimal-example":
│ 22: resource "proxmox_vm_qemu" "pxe-minimal-example" {

The main.tf part:

resource "proxmox_vm_qemu" "pxe-minimal-example" {
    name                      = "pxe-minimal-example"
    agent                     = 0
    boot                      = "order=net0;scsi0"
    pxe                       = true
    target_node = var.target_node
    network {
        bridge    = "vmbr0"
        firewall  = false
        link_down = false
        model     = "e1000"
    }
}

Version:

  • Proxmox: 7.3-4
  • terraform: v1.3.7 (linux)
  • terraform-provider-proxmox: 2.9.13
  • VM OS: Fedora CoreOS 37.20221225.3.0

@mleone87 mleone87 merged commit 64b4d07 into Telmate:master Mar 2, 2023
frostyfab pushed a commit to frostyfab/proxmox-api-go that referenced this pull request Nov 17, 2023
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.

4 participants