curtin-dev team mailing list archive

[Ryan Harper <rharper@xxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> diff --git a/curtin/commands/install_grub.py b/curtin/commands/install_grub.py
> index 5f8311f..3da1713 100644
> --- a/curtin/commands/install_grub.py
> +++ b/curtin/commands/install_grub.py
> @@ -254,12 +254,21 @@ def gen_uefi_install_commands(grub_name, grub_target, grub_cmd, update_nvram,
>          install_cmds.append(['update-grub'])
>      elif distroinfo.family == distro.DISTROS.redhat:
>          loader = find_efi_loader(target, bootid)
> -        if loader and update_nvram:
> -            grub_cmd = None  # don't install just add entry
> -            efi_disk, efi_part_num = get_efi_disk_part(devices)
> -            install_cmds.append(['efibootmgr', '--create', '--write-signature',
> -                                 '--label', bootid, '--disk', efi_disk,
> -                                 '--part', efi_part_num, '--loader', loader])
> +        if loader:
> +            # CentOS/RHEL install a signed SHIM and GRUB as part of the OS
> +            # installation. If grub2-install runs it will be replaced by a
> +            # generated UEFI which is not signed. This breaks UEFI secure
> +            # boot. Newer versions of CentOS/RHEL also do not include the
> +            # grub2-efi-x64-modules package. Without this package grub2-install
> +            # will fail causing the deployment to fail.

This is a rather long comment; could we simplify this, or at least include the key part, that setting grub_cmd = None disables running grub's install command?

> +            grub_cmd = None
> +            if update_nvram:
> +                efi_disk, efi_part_num = get_efi_disk_part(devices)

And let's add the other part of the short-and-sweet original comment here:

# Add an entry to the efi boot menu

Or something like that.

> +                install_cmds.append(['efibootmgr', '--create',
> +                                     '--write-signature', '--label', bootid,
> +                                     '--disk', efi_disk,
> +                                     '--part', efi_part_num,
> +                                     '--loader', loader])
>              post_cmds.append(['grub2-mkconfig', '-o',
>                                '/boot/efi/EFI/%s/grub.cfg' % bootid])
>          else:
> diff --git a/tests/unittests/test_commands_install_grub.py b/tests/unittests/test_commands_install_grub.py
> index 8808159..87f9edd 100644
> --- a/tests/unittests/test_commands_install_grub.py
> +++ b/tests/unittests/test_commands_install_grub.py
> @@ -774,6 +774,49 @@ class TestGenUefiInstallCommands(CiTestCase):
>                  grub_name, grub_target, grub_cmd, update_nvram, distroinfo,
>                  devices, self.target))
>  
> +    def test_redhat_install_existing_no_nvram(self):

""" verify grub install command is not executed if update_nvram is False on redhat. """

> +        # simulate existing bootloaders already installed in target system
> +        # by touching the files grub would have installed, including shim
> +        def _enable_loaders(bootid):
> +            efi_path = 'boot/efi/EFI'
> +            target_efi_path = os.path.join(self.target, efi_path)
> +            loaders = [
> +                os.path.join(target_efi_path, bootid, 'shimx64.efi'),
> +                os.path.join(target_efi_path, 'BOOT', 'BOOTX64.EFI'),
> +                os.path.join(target_efi_path, bootid, 'grubx64.efi'),
> +            ]
> +            for loader in loaders:
> +                util.ensure_dir(os.path.dirname(loader))
> +                with open(loader, 'w+') as fh:
> +                    fh.write('\n')

This chunk can be replaced with:

for loader in loaders:
    util.write_file(loader, content="")  # equivalent to touch

> +
> +        self.m_os_release.return_value = {'ID': 'redhat'}
> +        distroinfo = install_grub.distro.get_distroinfo()
> +        bootid = distroinfo.variant
> +        _enable_loaders(bootid)
> +        grub_name = 'grub2-efi-x64'
> +        grub_target = 'x86_64-efi'
> +        grub_cmd = 'grub2-install'
> +        update_nvram = False
> +        devices = ['/dev/disk-a-part1']
> +        disk = '/dev/disk-a'
> +        part = '1'
> +        self.m_get_disk_part.return_value = (disk, part)
> +
> +        expected_install = [
> +            ['efibootmgr', '-v'],
> +        ]
> +        expected_post = [
> +            ['grub2-mkconfig', '-o', '/boot/efi/EFI/%s/grub.cfg' % bootid],
> +            ['efibootmgr', '-v']
> +        ]
> +
> +        self.assertEqual(
> +            (expected_install, expected_post),
> +            install_grub.gen_uefi_install_commands(
> +                grub_name, grub_target, grub_cmd, update_nvram, distroinfo,
> +                devices, self.target))
> +
>  
>  class TestGenInstallCommands(CiTestCase):
>  


-- 
https://code.launchpad.net/~ltrager/curtin/+git/curtin/+merge/390517
Your team curtin developers is requested to review the proposed merge of ~ltrager/curtin:lp1895067 into curtin:master.