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

more flexible ipconfig setup #962

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

Conversation

spettinichi
Copy link
Contributor

No description provided.

@spettinichi
Copy link
Contributor Author

@hestiahacker hey this is my change for adding NICs more flexibly similar to what we discussed for disks. This one should be fully backwards compatible with the current method but allow more programmatic approach as well.

@hestiahacker
Copy link
Contributor

I don't have time to test this change out this week, but I took a quick look at the code and have some feedback:

  1. Overall, I like the idea of removing the numbers
  2. I like that it is backward compatible
  3. Could you update the examples to make sure there's at least one that includes multiple ipconfig blocks?
  4. What happens when a machine is deployed and then multiple changes are made the the network interfaces?
  5. What happens if someone were to mix and match with ipconfig0 and ipconfig (especially in combination with point 3)?
  6. If someone has more than 16 of these ipconfig blocks, is it going to cause a problem? I'm not sure if the numbers stopped there because of a limit on the Proxmox side, or if it was just an arbitrary stopping point. If it's a limitation from Proxmox, we should be sure to add that to the documentation.

I'm imaging some machine that was set up with eth0 being the uplink and eth1 being to some internal network. The services running on the machine specify which network interface they should be listening on, but then a third interface is added and we want to be sure that the NICs are not reordered. In this example, we would want the new NIC to be eth2.

Maybe this isn't a reasonable concern because we're just defining the IP address/netmask/gateway for a single interface and so it doesn't matter what order we list them in, the result will always be the sum of all the entries. In that case, I have questions about how to define which network interface each of these ipconfig blocks apply to.

As you may have guessed, I don't currently apply multiple NICs nor multiple IP addresses to any of my machines. But I'm willing to give it a try on my test environment to help test out this feature.

@hestiahacker
Copy link
Contributor

I tried running this branch and got a crash. Here's the output from terraform apply -auto-approve:

Plan: 1 to add, 0 to change, 0 to destroy.
proxmox_vm_qemu.server: Creating...
╷
│ Error: Plugin did not respond
│ 
│   with proxmox_vm_qemu.server,
│   on issue876.tf line 1, in resource "proxmox_vm_qemu" "server":
│    1: resource "proxmox_vm_qemu" "server" {
│ 
│ The plugin encountered an error, and failed to respond to the plugin.(*GRPCProvider).ApplyResourceChange call. The plugin logs may contain more details.
╵

Stack trace from the terraform-provider-proxmox plugin:

panic: interface conversion: interface {} is map[string]interface {}, not []interface {}

goroutine 38 [running]:
github.com/Telmate/terraform-provider-proxmox/v2/proxmox.resourceVmQemuCreate({0xe3bee0, 0xc000564310}, 0xc0008f4200, {0xbaf7e0?, 0xc0002518b0?})
	github.com/Telmate/terraform-provider-proxmox/v2/proxmox/resource_vm_qemu.go:829 +0x3b25
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*Resource).create(0xc0004db7a0, {0xe3be38, 0xc0005ac9c0}, 0xd?, {0xbaf7e0, 0xc0002518b0})
	github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/resource.go:778 +0x11b
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*Resource).Apply(0xc0004db7a0, {0xe3be38, 0xc0005ac9c0}, 0xc000660410, 0xc0003b3180, {0xbaf7e0, 0xc0002518b0})
	github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/resource.go:909 +0xa7e
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*GRPCProviderServer).ApplyResourceChange(0xc000010018, {0xe3be38?, 0xc0005ac8d0?}, 0xc000966690)
	github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/grpc_provider.go:1077 +0xdbc
github.com/hashicorp/terraform-plugin-go/tfprotov5/tf5server.(*server).ApplyResourceChange(0xc0002b6000, {0xe3be38?, 0xc000595aa0?}, 0xc00022a230)
	github.com/hashicorp/[email protected]/tfprotov5/tf5server/server.go:845 +0x3d0
github.com/hashicorp/terraform-plugin-go/tfprotov5/internal/tfplugin5._Provider_ApplyResourceChange_Handler({0xcdedc0?, 0xc0002b6000}, {0xe3be38, 0xc000595aa0}, 0xc0003b2000, 0x0)
	github.com/hashicorp/[email protected]/tfprotov5/internal/tfplugin5/tfplugin5_grpc.pb.go:518 +0x169
google.golang.org/grpc.(*Server).processUnaryRPC(0xc00019d000, {0xe3be38, 0xc000595740}, {0xe3fe00, 0xc0003b84e0}, 0xc000a5a000, 0xc0004736b0, 0x13816b8, 0x0)
	google.golang.org/[email protected]/server.go:1385 +0xe03
google.golang.org/grpc.(*Server).handleStream(0xc00019d000, {0xe3fe00, 0xc0003b84e0}, 0xc000a5a000)
	google.golang.org/[email protected]/server.go:1796 +0xfec
google.golang.org/grpc.(*Server).serveStreams.func2.1()
	google.golang.org/[email protected]/server.go:1029 +0x8b
created by google.golang.org/grpc.(*Server).serveStreams.func2 in goroutine 8
	google.golang.org/[email protected]/server.go:1040 +0x135

Error: The terraform-provider-proxmox plugin crashed!

This is always indicative of a bug within the plugin. It would be immensely
helpful if you could report the crash with the plugin's maintainers so that it
can be fixed. The output above should help diagnose the issue.

Here are my terraform files that I'm using to test:

provider.tf:

terraform {
  required_providers {
    proxmox = {
      source = "registry.example.com/telmate/proxmox"
      version = ">=1.0.0"
    }
  }
  required_version = ">= 0.14"
}

and issue876.tf (one I had lying around for testing):

resource "proxmox_vm_qemu" "server" {
  name              = var.name
  target_node       = var.target_node
  clone             = "debian-12"
  agent             = 0
  os_type           = "cloud-init"
  scsihw            = "virtio-scsi-pci"
  disks {
    virtio {
      virtio0 {
        disk {
          size            = 20
          cache           = "writeback"
          storage         = var.storage_backend
        }
      }
      virtio1 {
        disk {
          size            = 40
          cache           = "writeback"
          storage         = var.storage_backend
        }
      }
    }
  }
  network {
    model           = "virtio"
    bridge          = "vmbr0"
  }

  # Cloud Init Settings
  # Reference: https://pve.proxmox.com/wiki/Cloud-Init_Support
  cloudinit_cdrom_storage = var.storage_backend
  boot = "order=virtio0;ide3"
  ipconfig0 = "ip=${var.ip_address}/${var.cidr},gw=${var.gateway}"
  nameserver = var.nameservers
  sshkeys = file("${path.root}/test.pub")
}

To reproduce my work, you'll need to define some variables that are specific to your environment. The template can be found here

This test was intended to verify backwards compatibility. I expected this fork/branch to work the same as 3.0.1-rc1 with this configuration. The following test was going to be to try out the new configuration format.

To try to determine if the crash was because of something I was doing wrong, I took a look at the line at the top of the stacktrace (resource_vm_qemu.go:829) and saw that it was changed by this MR. https://github.com/spettinichi/terraform-provider-proxmox/blame/ifconfig-flexibility/proxmox/resource_vm_qemu.go#L829 So I don't think this is a testing error on my part, but if so I'd be happy to run a different terraform file and give it another try.

@spettinichi
Copy link
Contributor Author

thanks for testing! given the breaking changes with disks in v3 we have abandoned using our own module to simplify configuration. that means that this isn’t particularly useful anymore, unfortunately.
that being said, while working this i did find some issues in the code unrelated to this which i was addressing, so i am going to modify this to remove my ipconfig changes but keep those bug fixes

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