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

Fixes #24622 - UEFI HTTP boot loader options #5950

Merged
merged 1 commit into from Aug 24, 2018

Conversation

@lzap
Copy link
Member

commented Aug 15, 2018

UEFI clients are capable of booting from HTTP or HTTPS if they are given a valid URL via DHCP filename option.

Let's create such PXE Loader options. The only difference from existing options is that it must contain hostname of the boot server. By default this will be TFTP Smart Proxy if set, Foreman unattended_url otherwise.

For better flexibility, two new provisioning settings will be introduced: port number and root path. This allows users easily run their own web server exposing TFTP directory with grub2 configuration files over HTTP on arbitrary root path.

@theforeman-bot

This comment has been minimized.

Copy link
Member

commented Aug 15, 2018

Issues: #24622

$('#host_pxe_loader').closest('.form-group').find('.help-inline').html('<span class="error-message">' + __("Warning: This combination of loader and OS might not be able to boot.") + '</span>');
$('#host_pxe_loader').closest('.form-group').find('.help-inline').html('<span class="error-message">' +
__("Warning: This combination of loader and OS might not be able to boot.") +
__("Manual configuration is needed.") + '</span>');

This comment has been minimized.

Copy link
@lzap

lzap Aug 15, 2018

Author Member

This warning appears if you select any of the HTTP UEFI boot options, I just want to make clear that it will work if you do some manual changes (e.g. set up HTTP server on TFTP smart proxy).

This comment has been minimized.

Copy link
@stbenjam

stbenjam Aug 21, 2018

Member

Nitpick: needs a space between boot. and Manual

"Grub2 UEFI HTTP" => "http://#{tftp_proxy}:#{port}#{path}/grub2/grub#{precision}.efi",
"Grub2 UEFI HTTPS" => "https://#{tftp_proxy}:#{port}#{path}/grub2/grub#{precision}.efi",
"Grub2 UEFI HTTPS SecureBoot" => "https://#{tftp_proxy}:#{port}#{path}/grub2/shim#{precision}.efi",
"iPXE UEFI HTTP" => "http://#{tftp_proxy}:#{port}#{path}/ipxe-#{precision}.efi"

This comment has been minimized.

Copy link
@lzap

lzap Aug 15, 2018

Author Member

I also add iPXE UEFI which simply loads ipxe-x64.efi which can be grabbed from here: http://boot.ipxe.org/

We don't ship this with Foreman or Satellite tho, users can still try this workflow if they like it.

This comment has been minimized.

Copy link
@stbenjam

stbenjam Aug 15, 2018

Member

Just my initial impression after a quick skim: I have mixed feelings about using the tftp_proxy for building the URL.

Back when the templates proxy was implemented, this approach was taken and it turned out to be problematic. I realize this is a different use case, but I think the end goal would be to have a smart proxy plugin called HTTPBOOT, right? Should it be responsible for knowing the URL for HTTP and HTTPS boot and returning that to Foreman like template_url?

Alos, I realize this is the first "manual" approach. The user is going to have to configure HTTP to serve the files. Could we, at least for the initial deployment, use host parameters instead for the URL? Then once we implement a smart proxy plugin that's a little more intelligent use that.

This comment has been minimized.

Copy link
@lzap

lzap Aug 15, 2018

Author Member

If you prefer having our own module, I can create one, let's do this properly from the day one. The only concern is security bugs, allowing others to read arbitrary files. History shows that many people failed, many times :-)

But yes, this is cleaner from the user and dev perspective, the only drawback is that we will have a feature "httpboot" dependent on other feature "tftp" and I am not sure if smart-proxy plugin API supports that.

:PXELinux => /^(pxelinux.*|PXELinux (BIOS|UEFI))$/,
:PXEGrub => /^(grub\/|Grub UEFI).*/,
:PXEGrub2 => /^(grub2\/|Grub2 UEFI|http.*grub2\/).*/,
:iPXE => /^(iPXE|http.*\/ipxe-).*/

This comment has been minimized.

Copy link
@lzap

lzap Aug 15, 2018

Author Member

I am cleaning those regexps here, covered with many tests.

@@ -57,6 +57,8 @@ def self.default_settings
self.set('libvirt_default_console_address', N_("The IP address that should be used for the console listen address when provisioning new virtual machines via Libvirt"), "0.0.0.0", N_('Libvirt default console address')),
self.set('update_ip_from_built_request', N_("Foreman will update the host IP with the IP that made the built request"), false, N_('Update IP from built request')),
self.set('use_shortname_for_vms', N_("Foreman will use the short hostname instead of the FQDN for creating new virtual machines"), false, N_('Use short name for VMs')),
self.set('http_boot_port', N_("Port number for UEFI HTTP(s) boot clients on TFTP proxy or Foreman server"), 80, N_('UEFI HTTP boot port')),
self.set('http_boot_path', N_("Boot files base path for UEFI HTTP(s) boot clients on TFTP proxy or Foreman server"), '/tftp', N_('UEFI HTTP root path')),

This comment has been minimized.

Copy link
@lzap

lzap Aug 15, 2018

Author Member

New settings for better flexibility, all the rest are tests.

@lzap lzap force-pushed the lzap:uefi-http-boot-24622 branch from 23bb1b3 to 8014ef6 Aug 16, 2018

@lzap

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2018

New version is up, it uses new feature called HTTPBoot, make sure to pick it. If it's not picked, it falls back to Foreman unattended_url setting. Smart proxy with enabled "httpboot" module is required. I haven't tested this end-to-and yet.

@stbenjam
Copy link
Member

left a comment

I successfully end-to-end tested this and the smart-proxy PR. I left some notes on the smart proxy PR but other than minor issues, this part looks good.

$('#host_pxe_loader').closest('.form-group').find('.help-inline').html('<span class="error-message">' + __("Warning: This combination of loader and OS might not be able to boot.") + '</span>');
$('#host_pxe_loader').closest('.form-group').find('.help-inline').html('<span class="error-message">' +
__("Warning: This combination of loader and OS might not be able to boot.") +
__("Manual configuration is needed.") + '</span>');

This comment has been minimized.

Copy link
@stbenjam

stbenjam Aug 21, 2018

Member

Nitpick: needs a space between boot. and Manual

:feature => N_('HTTPBoot'),
:label => N_('HTTPBoot Proxy'),
:api_description => N_('HTTPBoot Proxy ID to use within this subnet'),
:description => N_('HTTPBoot Proxy to use within this subnet')

This comment has been minimized.

Copy link
@stbenjam

stbenjam Aug 21, 2018

Member

There's a def proxies method in Subnet, it should return httpboot too:

https://github.com/theforeman/foreman/blob/develop/app/models/subnet.rb#L213

@lzap

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2018

Done.

@lzap lzap force-pushed the lzap:uefi-http-boot-24622 branch from 8014ef6 to c825a71 Aug 23, 2018

@stbenjam
Copy link
Member

left a comment

Code climate issue is related to PR. But otherwise, this looks good to me.

@stbenjam

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

Oh my checkmark is green. I'm not really a Foreman committer though, I think I only have access to the repo because of my membership in the packaging groups. You'll need someone else to review, too.

@lzap lzap added Ready to merge and removed Needs testing labels Aug 24, 2018

@lzap

This comment has been minimized.

Copy link
Member Author

commented Aug 24, 2018

Thanks for testing this, some can merge this @timogoebel, @ares or @tbrisker ? There is a linked smart-proxy PR as well.

@timogoebel
Copy link
Member

left a comment

Looks good to me.

@timogoebel timogoebel merged commit 861d87b into theforeman:develop Aug 24, 2018

5 of 7 checks passed

codeclimate 1 issue to fix
Details
foreman Build finished. 35394 tests run, 5 skipped, 2 failed.
Details
Hound No violations found. Woof!
continuous-integration/travis-ci/pr The Travis CI build passed
Details
katello Build finished. 3940 tests run, 9 skipped, 0 failed.
Details
prprocessor Commit message style is correct
Details
upgrade Build finished. No test results found.
Details
@timogoebel

This comment has been minimized.

Copy link
Member

commented Aug 24, 2018

Thanks, @lzap.

@lzap lzap deleted the lzap:uefi-http-boot-24622 branch Aug 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.