Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 #36833 - Add SecureBoot support for arbitrary operating systems to "Grub2 UEFI" PXE loaders #877
base: develop
Are you sure you want to change the base?
Fixes #36833 - Add SecureBoot support for arbitrary operating systems to "Grub2 UEFI" PXE loaders #877
Changes from all commits
68357a9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why do we delete these binaries here if we are already using
force: true
inFileUtils.ln_s
, which overwrites existing symlinks?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 added this to guarantee that we have a clean directory state on each rebuild of a host in case the files required in the host_config directory change in future.
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.
This only applies if the OS changes, as the bootloader names remain the same if the OS is unchanged, so there's no issue in that case. My goal is to minimize the risk of accidentally deleting necessary files (e.g., the binaries in
/var/lib/tftpboot/grub2
). However, since we validate the mac address beforehand (see here), I believe we’re safe in this regard.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 took another and again deeper look at what happens regarding MAC address validation when the
setup_bootloader
method is called and yes: The waysetup_bootloader
is called guarantees that theFileUtils.rm_f
command can and will only delete*.efi
files in the:tftproot:/host-config/<MAC_address>/
directory.VARIANTS
others than Syslinux require a MAC address.instantiate
method in the line mentioned by you in your comment.pxeconfig_dir_mac
does not point to the:tftproot:/grub2/
directory. This would be the case if the MAC address isnil
when calling the pxeconfig_dir method.Although I have to admit that the cases I am thinking about here might be really rare corner cases [1], I would prefer to stick to deleting the present
*.efi
files on updating the symlinks.[1] An example would be when an OS vendor decides to follow another naming scheme for the shim and GRUB2 binaries.