theforeman
VMware - Secure Boot & Virtual TPM
Add support for Secure Boot and TPM Inspired by https://github.com/ananace/foreman_vmware_advanced Required FOG PR: https://github.com/fog/fog-vsphere/pull/305
With the firmware we have a field
auto. Wouldn't it make more sense to add a value there for UEFI + SecureBoot? The default isauto, which derives it from the PXE loader. We should enhance that to automatically detect if secureboot was desired.
I didn't know that's how the Automatic option works. I'll have to adjust my recent changes accordingly.
It works by deriving the firmware_type from the pxe_loader:
https://github.com/theforeman/foreman/blob/7ce3336d67e1b0b825b8a4add92a340c7d2d144b/app/models/concerns/pxe_loader_support.rb#L49-L59
This is then used in the firmware_mapping:
https://github.com/theforeman/foreman/blob/7ce3336d67e1b0b825b8a4add92a340c7d2d144b/app/models/compute_resources/foreman/model/vmware.rb#L818-L821
And in parsing the arguments this is used to determine the firmware:
https://github.com/theforeman/foreman/blob/7ce3336d67e1b0b825b8a4add92a340c7d2d144b/app/models/compute_resources/foreman/model/vmware.rb#L495
In my libvirt EFI patch I copied this pattern: https://github.com/theforeman/foreman/pull/9965/files#diff-525a301145c328b1c4d2e6dd7668a9ce4b9f64edb7b2876a4cdebeb5ee2689f9R162-R177
You can see I already added a TODO there to include secure boot.
This is completely lost in https://github.com/theforeman/foreman/pull/10209.
UI changes in my commit:
- Added a new firmware type
UEFI Secure Bootfor Secure Boot. - Removed the Secure Boot checkbox (replaced it with the new firmware type).
- Added a label helper indicating that TPM is only compatible with EFI firmware.
- Hid the TPM option from the UI when it is not relevant.
Test failure looks related. I think you need to delete (and then handle)
virtual_tpminparse_args.
I thought it's because we're missing the https://github.com/fog/fog-vsphere/pull/305 PR.
No, it's because it tries to assign it to the host model now instead of transforming it to a vsphere api parameter
@nofaralfasi did you want to address @ekohl concerns before I test, that way we can test it just once and get this in faster once the code looks good.
concerns before I test, that way we can test it just once and get this in faster once the code looks good.
I assure you, you'll see the exact same failures as the automated tests so there's no point in testing manually.
No, it's because it tries to assign it to the host model now instead of transforming it to a vsphere api parameter
Where do you see it in the tests result? I still think it's because of the missing PR from fog-vsphere.
Oh yes, I think I misread that. Apologies for that. Perhaps you can modify the PR to actually point to the PR so we can see if the tests would pass otherwise.
I have another idea on how to implement this. Moving it to WIP until it's ready.
Changes:
- TPM option is always visible on the host's edit page (disabled for existing hosts).
- Handles TPM conflict with BIOS. This is the error displayed to the user if they enable the TPM option while their firmware is set to BIOS:
Note: tests are in progress.
@nofaralfasi and I had some discussion and I came up with https://github.com/ekohl/foreman/commit/707162487891fdd9d2d744c5f52a83470c9d0d72 as some possible improvements.
Explanation of the Update: Creating a new CR machine differs from creating a bare-metal machine, which means we currently lack a way to validate the VM form fields submitted by the user.
The recent updates have introduced a scenario where setting the firmware type to BIOS and enabling the new Virtual TPM option results in an error, as TPM is not compatible with BIOS. To address this, the best solution would be to catch this error before sending the data to fog-vsphere and raise an error for the user, while preserving the form with its current values.
Future Improvements: This part of the code would benefit from future enhancements, such as providing better descriptions of the fields (e.g., explaining what it means to set the firmware to Automatic) and adding validation of the data before it is sent to fog-vsphere. Unfortunately, we lack this functionality not only for the parameters added in this PR but also for the existing ones. If we want to refactor it, we should do it all together, which would require a larger PR.
This calls the
create_vmwhich callsparse_args. This means you can't really use the errors framework to get specific field level errors and only can raise an exception. Worse, we're actually performing expensive API calls (inparse_network) when we could fail early.
Before that, we are calling the new_vm method also from here compute_object. So if we raise the error from parse_args, the exception will result in breaking the whole page.
Update: I can catch the exception in the compute_object method, which will break the form and display an error message to the user. I was trying to avoid this because I consider it an edge case and didn't want an error that breaks the whole form. However, this approach ensures that the exception is presented in a user-friendly manner, and it's been handled before sending the values to VMware.
My thought process behind it is that you should fail as early as possible...
I agree, but as I mentioned before, I propose adding validations for all the values before we send them to VMware.
Before that, we are calling the new_vm method also from here compute_object. So if we raise the error from parse_args, the exception will result in breaking the whole page.
Ah, this is something I missed altogether. Then I think your approach is good enough now and we need do something more to untangle the mess.
These are the reasons the host creation/edit form is so complex and why so many attempts to improve it end up failing.
Closing this PR in favor of a new one: https://github.com/theforeman/foreman/pull/10324. The new PR addresses the same issue with updated changes. Please continue the discussion and review there.