Add TPM Support #260
Add TPM Support #260
Conversation
|
Any update on this? |
There was a problem hiding this comment.
Hi @mpywell,
Thanks for the PR, and sorry I haven't looked at it before today.
I left a couple questions and one suggestion regarding the function that builds the tpm config object from the config, but aside from those LGTM!
Let me know when you've had time to address those, and I'll do another round of review ASAP.
Thanks!
There was a problem hiding this comment.
Hi @mpywell,
Thanks for the reroll and explanations! Just left a final question/suggestion regarding the reflects for building the disk slice as I'm not sure I understand why we need them, but besides that, this looks good to me.
Pre-approving so it's not a blocker later.
|
Hi @lbajolet-hashicorp, hope the reflect answer helps! If it helps with planning the 1.9 release I plan to follow this PR up with another for #257 shortly after merge. I also have some draft changes started to tackle #263 and #187 (duplicates of the same issue), and #80. |
The was we set disks for a VM startup since the structure changed uses reflection as a way to not have to do some ugly switches with named fields. While this works, without a good understanding of both Go's reflection and the proxmox APIs, this is a non-trivial piece of code, that is hardly statically validable, and error prone. To mitigate this, we write a lengthy documentation on the first of the reflection usages in the generateProxmoxDisks function so it explains the whys and hows of the approach, in order to make it hopefully clearer to future developers.
Accessing the fields of a structure through reflection is possible through several means, namely by index, or by name. Index-based access can be relevant, but is rather susceptible to reordering changes later in the life of the product, and given that it occurs at runtime only, we lose the possibility to statically detect those changes. Name-based access is slightly more robust, as we are avoiding issues like new fields changing the access, and the named fields generally change only in case of a refactor, which is rarer. Besides named accesses are somewhat self-documentational, and refect better on what we're trying to achieve in this code. Therefore, we change how the reflection accesses happen for generateProxmoxDisks, defaulting on named-based accesses instead of index-based ones for filling in the disks.
Add TPM support for Windows 11 builds.
TPM was merged into Telmate/proxmox-api-go@32c480f, v1.1.7 uses a version from ~12 months ago. Between versions changes were made upstream relating to the proxmox.ConfigQemu struct that required uplift to storage in packer-plugin-proxmox, in addition to the deprecation of the UpdateConfig and CreateVm functions.
While the tests have been updated and pass, I've also extensively tested proxmox-iso and proxmox-clone on a PVE environment and have not observed any breaking changes for packer templates that worked with v1.1.7.
Closes #73