-
Notifications
You must be signed in to change notification settings - Fork 991
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
Fixes #37566 - Add UEFI Secure Boot Firmware to Libvirt #10321
base: develop
Are you sure you want to change the base?
Conversation
Failing tests should be fixed automatically after fog/fog-libvirt#155 is merged. |
As noted in my comment on the VMware-related PR #10324 (comment), the same issue occurs with Libvirt. The |
:port => '-1' } | ||
:port => '-1' }, | ||
:firmware => 'automatic', | ||
:firmware_features => { "secure-boot" => "no" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for having strings as keys in the inner hash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments + it looks like failing tests are related.
Otherwise I was able to provision the Fedora 39 machine with BIOS, UEFI, and UEFI + Secure Boot.
{ | ||
firmware_features: { 'secure-boot' => 'yes', 'enrolled-keys' => 'yes' }, | ||
loader: { 'secure' => 'yes' }, | ||
secure_boot: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For other keys and values, you use 'key' => 'yes'
. Wouldn't it make sense to do the same here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The secure_boot: true
setting here is a special case, required only for displaying the correct firmware in the compute_attributes
form. We don't process this setting in fog-libvirt
the same way we do when creating a new VM. Therefore, if we change its format, there’s little reason to use it, as we could instead rely on the loader
attribute. This approach simplifies the usage of the ComputeResource#firmware_type
method, aligning it with how we handle VMs.
@@ -8,6 +8,13 @@ | |||
<% checked = params[:host] && params[:host][:compute_attributes] && params[:host][:compute_attributes][:start] || '1' %> | |||
<%= checkbox_f f, :start, { :checked => (checked == '1'), :help_inline => _("Power ON this machine"), :label => _('Start'), :label_size => "col-md-2"} if new_vm && controller_name != "compute_attributes" %> | |||
|
|||
<% firmware_type = new_vm && controller_name != 'compute_attributes' ? 'automatic' : compute_resource.firmware_type(f.object.firmware, f.object.secure_boot) %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make it as a helper method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've decided to remove the conditions so that firmware_type
is now consistently determined by the compute_resource.firmware_type
method. The previous logic aimed to set the firmware to Automatic
instead of BIOS
for new VMs, but it didn't account for scenarios where the firmware is inherited from the Compute Profile.
<% firmware_type = new_vm && controller_name != 'compute_attributes' ? 'automatic' : compute_resource.firmware_type(f.object.firmware, f.object.secure_boot) %> | |
<% firmware_type = compute_resource.firmware_type(f.object.firmware, f.object.secure_boot) %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍏 LGTM
The fog-libvirt part has to be merged and released first
Oh I missed the failing tests:
This needs to be fixed. |
As I mentioned here, this should be fixed automatically after fog/fog-libvirt#155 is merged. |
- Added a new firmware type for Secure Boot. - Enable `enrolled-keys` by default when Secure Boot is activated. - Added firmware-related methods to the ComputeResource model for shared use between VMware and Libvirt.
Requires:
This PR includes two commits:
When creating a new host in Foreman, after selecting
Libvirt
as the compute resource, a new option to select the VM's firmware will appear under theVirtual Machine
tab. See the screenshot below for a demonstration:Notes:
enrolled-keys
are enabled by default when Secure Boot is activated.loader secure='yes'
setting.For more details: community post.