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

feat(vm): Add support for setting the VM TPM State device #743

Merged
merged 7 commits into from
Dec 5, 2023

Conversation

rgl
Copy link
Contributor

@rgl rgl commented Nov 27, 2023

Add support for setting the VM TPM State hardware.

Please feel free to squash all the commits.

The TPM State hardware can be declared as:

resource "proxmox_virtual_environment_vm" "example" {
  tpm_state {
    datastore_id = "local-lvm"
    version      = "v2.0"
  }
  ...
}

Contributor's Note

  • I have added / updated documentation in /docs for any user-facing features or additions.
  • I have added / updated templates in /example for any new or updated resources / data sources.
  • I have ran make example to verify that the change works as expected.

I've partially executed the example (see error after the screenshot). Here's the screenshot of the example created VM that shows it with the TPM State device:

image

Here's the error (which seems not related to this PR):

╷
│ Error: error updating container: received an HTTP 400 response - Reason: Parameter verification failed. (mp0: unable to hotplug mp0: directory '/mnt/bindmounts/shared' does not exist)
│ 
│   with proxmox_virtual_environment_container.example,
│   on resource_virtual_environment_container.tf line 52, in resource "proxmox_virtual_environment_container" "example":
│   52: resource "proxmox_virtual_environment_container" "example" {
│ 
╵

Proof of Work

Given the following terraform program:

resource "proxmox_virtual_environment_vm" "template" {
  name      = "gh-743-template"
  node_name = "pve"
  vm_id     = 7430
  started   = false
  template  = true
}

resource "proxmox_virtual_environment_vm" "a" {
  depends_on = [proxmox_virtual_environment_vm.template]
  name       = "gh-743-a"
  node_name  = "pve"
  vm_id      = 7431
  clone {
    vm_id = proxmox_virtual_environment_vm.template.vm_id
  }
  tpm_state {
    datastore_id = "local-lvm"
    version      = "v2.0"
  }
  started = false
}

resource "proxmox_virtual_environment_vm" "b" {
  depends_on = [proxmox_virtual_environment_vm.a]
  name       = "gh-743-b"
  node_name  = "pve"
  vm_id      = 7432
  clone {
    vm_id = proxmox_virtual_environment_vm.a.vm_id
  }
  tpm_state {
    datastore_id = "local-lvm"
    version      = "v2.0"
  }
  started = false
}

terraform apply looks like:

$ terraform apply

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # proxmox_virtual_environment_vm.a will be created
  + resource "proxmox_virtual_environment_vm" "a" {
      + acpi                    = true
      + bios                    = "seabios"
      + id                      = (known after apply)
      + ipv4_addresses          = (known after apply)
      + ipv6_addresses          = (known after apply)
      + keyboard_layout         = "en-us"
      + mac_addresses           = (known after apply)
      + migrate                 = false
      + name                    = "gh-743-a"
      + network_interface_names = (known after apply)
      + node_name               = "pve"
      + on_boot                 = true
      + reboot                  = false
      + scsi_hardware           = "virtio-scsi-pci"
      + started                 = false
      + tablet_device           = true
      + template                = false
      + timeout_clone           = 1800
      + timeout_create          = 1800
      + timeout_migrate         = 1800
      + timeout_move_disk       = 1800
      + timeout_reboot          = 1800
      + timeout_shutdown_vm     = 1800
      + timeout_start_vm        = 1800
      + timeout_stop_vm         = 300
      + vm_id                   = 7431

      + clone {
          + full    = true
          + retries = 1
          + vm_id   = 7430
        }

      + tpm_state {
          + datastore_id = "local-lvm"
          + version      = "v2.0"
        }
    }

  # proxmox_virtual_environment_vm.b will be created
  + resource "proxmox_virtual_environment_vm" "b" {
      + acpi                    = true
      + bios                    = "seabios"
      + id                      = (known after apply)
      + ipv4_addresses          = (known after apply)
      + ipv6_addresses          = (known after apply)
      + keyboard_layout         = "en-us"
      + mac_addresses           = (known after apply)
      + migrate                 = false
      + name                    = "gh-743-b"
      + network_interface_names = (known after apply)
      + node_name               = "pve"
      + on_boot                 = true
      + reboot                  = false
      + scsi_hardware           = "virtio-scsi-pci"
      + started                 = false
      + tablet_device           = true
      + template                = false
      + timeout_clone           = 1800
      + timeout_create          = 1800
      + timeout_migrate         = 1800
      + timeout_move_disk       = 1800
      + timeout_reboot          = 1800
      + timeout_shutdown_vm     = 1800
      + timeout_start_vm        = 1800
      + timeout_stop_vm         = 300
      + vm_id                   = 7432

      + clone {
          + full    = true
          + retries = 1
          + vm_id   = 7431
        }

      + tpm_state {
          + datastore_id = "local-lvm"
          + version      = "v2.0"
        }
    }

  # proxmox_virtual_environment_vm.template will be created
  + resource "proxmox_virtual_environment_vm" "template" {
      + acpi                    = true
      + bios                    = "seabios"
      + id                      = (known after apply)
      + ipv4_addresses          = (known after apply)
      + ipv6_addresses          = (known after apply)
      + keyboard_layout         = "en-us"
      + mac_addresses           = (known after apply)
      + migrate                 = false
      + name                    = "gh-743-template"
      + network_interface_names = (known after apply)
      + node_name               = "pve"
      + on_boot                 = true
      + reboot                  = false
      + scsi_hardware           = "virtio-scsi-pci"
      + tablet_device           = true
      + template                = true
      + timeout_clone           = 1800
      + timeout_create          = 1800
      + timeout_migrate         = 1800
      + timeout_move_disk       = 1800
      + timeout_reboot          = 1800
      + timeout_shutdown_vm     = 1800
      + timeout_start_vm        = 1800
      + timeout_stop_vm         = 300
      + vm_id                   = 7430
    }

Plan: 3 to add, 0 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

proxmox_virtual_environment_vm.template: Creating...
proxmox_virtual_environment_vm.template: Creation complete after 1s [id=7430]
proxmox_virtual_environment_vm.a: Creating...
proxmox_virtual_environment_vm.a: Creation complete after 1s [id=7431]
proxmox_virtual_environment_vm.b: Creating...
proxmox_virtual_environment_vm.b: Creation complete after 5s [id=7432]

Apply complete! Resources: 3 added, 0 changed, 0 destroyed.

And the end result in Proxmox looks like:

image

image

image

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #453

Copy link
Owner

@bpg bpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rgl! Thank you so much for contributing this feature!

I've added a few missing pieces to make CustomTPMState encoding / decoding work. After that I was able to create a VM without issues:
Screenshot 2023-11-28 at 10 39 58 AM

I've also removed Size field from CustomTPMState, as we can't readlly do anything with the size of TPM.

Overall it looks really good! The next steps would be to:

  • add proof of work
  • add documentation
  • perhaps check how does it work when cloning a VM with tpm

Feel free to reach out with any questions, will be happy to help!

fmt.Sprintf("file=%s", r.FileVolume),
}

if r.Version != nil {
Copy link
Contributor Author

@rgl rgl Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit puzzled about this. When, in my terraform program, I just use tpm_state {}, it seems this is being nil because what end's up being created in proxmox is a v1.2 TPM device, but shouldn't this be v2.0 instead (because that's the default value set in the terraform schema definition)?

PS I'm now even more puzzled, since, after the first terraform apply, I've modified the terraform program to use tpm_state { version = "v2.0" }, but that change was not picked up by the next terraform apply.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's odd. What is in the TF output? When I'm using the same tpm_state {} the TF prints out this:

<skipped>
 
      + tpm_state {
          + datastore_id = "local-lvm"
          + version      = "v2.0"
        }
    }

Plan: 1 to add, 0 to change, 0 to destroy.
proxmox_virtual_environment_vm.ubuntu_vm: Creating...
proxmox_virtual_environment_vm.ubuntu_vm: Creation complete after 7s [id=453]

and VM gets creating with expected TPM.

Probably your local TF state got messed up after renaming? The provider's default version v2.0 was not set to the correct attribute, and when provider was reading the state it got no version. Then when this was pushed to TF the PVE's default (i assume it is 1.2) got applied.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When in doubt, I always destroy everything and delete the tfstate, but it still does not work as I expected.

I do see the same diff as you, with v2.0, but all the requests/responses to/from proxmox do not mention version=v2.0 at all, in fact, there is no version argument floating around at all, so the default proxmox tpm version of v1.2 is used. And I still don't known why this is happening.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... I'll try to debug it today.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to mention:

Copy link
Owner

@bpg bpg Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it's about cloning then! :) Fixed a small bug in vmGetTPMState, should be working now.

BTW, this is a bare minimum template I used for testing TPM, takes a second to apply:

resource "proxmox_virtual_environment_vm" "ubuntu_vm_template" {
  name      = "453-test-template"
  node_name = "pve"
  vm_id     = 453

  started  = false
  template = true
}


resource "proxmox_virtual_environment_vm" "ubuntu_vm_cloned" {
  depends_on = [proxmox_virtual_environment_vm.ubuntu_vm_template]
  name       = "453-test-tpm"
  node_name  = "pve"
  vm_id      = 454

  clone {
    vm_id = 453
  }

  tpm_state {
    datastore_id = "local-lvm"
    version      = "v2.0"
  }

  started = false
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you! its now working nicely!

I've rebased the branch over v0.39.0. the final PR should be squashed if you agree.

I will do the remaining tasks that you've asked initially.

@rgl rgl changed the title WIP add support for setting the vm tpm state Add support for setting the VM TPM State device Nov 30, 2023
@rgl rgl marked this pull request as ready for review November 30, 2023 20:45
@rgl rgl changed the title Add support for setting the VM TPM State device feat(vm): Add support for setting the VM TPM State device Nov 30, 2023
Copy link
Owner

@bpg bpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work, thank a lot @rgl for contributing this!
I like your very detailed Proof of Work 🤩

LGTM! 🚀

@bpg bpg merged commit 66bba2a into bpg:main Dec 5, 2023
8 checks passed
@ghost ghost mentioned this pull request Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

allow creating tpm
2 participants