Utilize CSV file for EFI loader selection

Adds support to identify and utilize a CSV file to signal which
bootloader to utilize, and set it when the OS is running as opposed
to when EFI is running. This works around EFI loader potentially
crashing some vendors hardware types when entry stored in the
image does not match the EFI loader record which was utilzied to
boot.

Grub2+shim specifically specifically needs the CSV file name
and entry label to match what the system was booted with in order
to prevent the machine from potentially crashing.

See https://storyboard.openstack.org/#!/story/2008962
and https://bugzilla.redhat.com/show_bug.cgi?id=1966129#c37
for more information.

Change-Id: Ibf1ef4fe0764c0a6f1a39cb7eebc23ecc0ee177d
Story: 2008962
Task: 42598
Co-Authored-By: Bob Fournier <bfournie@redhat.com>

ironic_python_agent/extensions/image.py

@@ -37,12 +37,16 @@ CONF = cfg.CONF
 
 BIND_MOUNTS = ('/dev', '/proc', '/run')
 
+# NOTE(TheJulia): Do not add bootia32.csv to this list. That is 32bit
+# EFI booting and never really became popular.
 BOOTLOADERS_EFI = [
+    'bootx64.csv',  # Used by GRUB2 shim loader (Ubuntu, Red Hat)
+    'boot.csv',  # Used by rEFInd, Centos7 Grub2
     'bootia32.efi',
-    'bootx64.efi',
+    'bootx64.efi',  # x86_64 Default
     'bootia64.efi',
     'bootarm.efi',
-    'bootaa64.efi',
+    'bootaa64.efi',  # Arm64 Default
     'bootriscv32.efi',
     'bootriscv64.efi',
     'bootriscv128.efi',
@@ -223,9 +227,10 @@ def _is_bootloader_loaded(dev):
 def _get_efi_bootloaders(location):
     """Get all valid efi bootloaders in a given location
 
-    :param location: the location where it should  start looking for the
+    :param location: the location where it should start looking for the
                      efi files.
-    :return: a list of relative paths to valid efi bootloaders
+    :return: a list of relative paths to valid efi bootloaders or reference
+             files.
     """
     # Let's find all files with .efi or .EFI extension
     LOG.debug('Looking for all efi files on %s', location)
@@ -241,15 +246,30 @@ def _get_efi_bootloaders(location):
                 v_bl = efi_f.split(location)[-1][1:]
                 LOG.debug('%s is a valid bootloader', v_bl)
                 valid_bootloaders.append(v_bl)
+            if 'csv' in efi_f.lower():
+                v_bl = efi_f.split(location)[-1][1:]
+                LOG.debug('%s is a pointer to a bootloader', v_bl)
+                # The CSV files are intended to be authortative as
+                # to the bootloader and the label to be used. Since
+                # we found one, we're going to point directly to it.
+                # centos7 did ship with 2, but with the same contents.
+                # TODO(TheJulia): Perhaps we extend this to make a list
+                # of CSVs instead and only return those?! But then the
+                # question is which is right/first/preferred.
+                return [v_bl]
     return valid_bootloaders
 
 
-def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition):
+def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition,
+                    mount_point):
     """Executes efibootmgr and removes duplicate entries.
 
     :param valid_efi_bootloaders: the list of valid efi bootloaders
     :param device: the device to be used
     :param efi_partition: the efi partition on the device
+    :param mount_point: The mountpoint for the EFI partition so we can
+                        read contents of files if necessary to perform
+                        proper bootloader injection operations.
     """
 
     # Before updating let's get information about the bootorder
@@ -262,13 +282,25 @@ def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition):
                                   r'Boot([0-9a-f-A-F]+)\s.*$')
     label_id = 1
     for v_bl in valid_efi_bootloaders:
-        v_efi_bl_path = '\\' + v_bl.replace('/', '\\')
+        if 'csv' in v_bl.lower():
-        # Update the nvram using efibootmgr
+            # These files are always UTF-16 encoded, sometimes have a header.
-        # https://linux.die.net/man/8/efibootmgr
+            # Positive bonus is python silently drops the FEFF header.
-        label = 'ironic' + str(label_id)
+            with open(mount_point + '/' + v_bl, 'r', encoding='utf-16') as csv:
+                contents = str(csv.read())
+            csv_contents = contents.split(',', maxsplit=3)
+            csv_filename = v_bl.split('/')[-1]
+            v_efi_bl_path = v_bl.replace(csv_filename, str(csv_contents[0]))
+            v_efi_bl_path = '\\' + v_efi_bl_path.replace('/', '\\')
+            label = csv_contents[1]
+        else:
+            v_efi_bl_path = '\\' + v_bl.replace('/', '\\')
+            label = 'ironic' + str(label_id)
+
         LOG.debug("Adding loader %(path)s on partition %(part)s of device "
                   " %(dev)s", {'path': v_efi_bl_path, 'part': efi_partition,
                                'dev': device})
+        # Update the nvram using efibootmgr
+        # https://linux.die.net/man/8/efibootmgr
         cmd = utils.execute('efibootmgr', '-c', '-d', device,
                             '-p', efi_partition, '-w', '-L', label,
                             '-l', v_efi_bl_path)
@@ -343,7 +375,8 @@ def _manage_uefi(device, efi_system_part_uuid=None):
 
         valid_efi_bootloaders = _get_efi_bootloaders(efi_partition_mount_point)
         if valid_efi_bootloaders:
-            _run_efibootmgr(valid_efi_bootloaders, device, efi_partition)
+            _run_efibootmgr(valid_efi_bootloaders, device, efi_partition,
+                            efi_partition_mount_point)
             return True
         else:
             # NOTE(dtantsur): if we have an empty EFI partition, try to use

ironic_python_agent/tests/unit/extensions/test_image.py

@@ -2117,6 +2117,51 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
         mock_execute.assert_has_calls(expected)
         self.assertEqual(7, mock_execute.call_count)
 
+    @mock.patch.object(os.path, 'exists', lambda *_: False)
+    @mock.patch.object(image, '_get_efi_bootloaders', autospec=True)
+    @mock.patch.object(image, '_get_partition', autospec=True)
+    @mock.patch.object(utils, 'get_efi_part_on_device', autospec=True)
+    @mock.patch.object(os, 'makedirs', autospec=True)
+    def test__manage_uefi_found_csv(self, mkdir_mock, mock_utils_efi_part,
+                                    mock_get_part_uuid, mock_efi_bl,
+                                    mock_execute, mock_dispatch):
+        mock_utils_efi_part.return_value = '1'
+        mock_get_part_uuid.return_value = self.fake_dev
+        mock_efi_bl.return_value = ['EFI/vendor/BOOTX64.CSV']
+
+        # Format is <file>,<entry_name>,<options>,humanfriendlytextnotused
+        # https://www.rodsbooks.com/efi-bootloaders/fallback.html
+        # Mild difference, Ubuntu ships a file without a 0xFEFF delimiter
+        # at the start of the file, where as Red Hat *does*
+        csv_file_data = u'shimx64.efi,Vendor String,,Grub2MadeUSDoThis\n'
+
+        mock_execute.side_effect = iter([('', ''), ('', ''),
+                                         ('', ''), ('', ''),
+                                         ('', ''), ('', ''),
+                                         ('', '')])
+
+        expected = [mock.call('partx', '-u', '/dev/fake', attempts=3,
+                              delay_on_retry=True),
+                    mock.call('udevadm', 'settle'),
+                    mock.call('mount', self.fake_efi_system_part,
+                              self.fake_dir + '/boot/efi'),
+                    mock.call('efibootmgr'),
+                    mock.call('efibootmgr', '-c', '-d', self.fake_dev,
+                              '-p', '1', '-w',
+                              '-L', 'Vendor String', '-l',
+                              '\\EFI\\vendor\\shimx64.efi'),
+                    mock.call('umount', self.fake_dir + '/boot/efi',
+                              attempts=3, delay_on_retry=True),
+                    mock.call('sync')]
+        with mock.patch('builtins.open',
+                        mock.mock_open(read_data=csv_file_data)):
+            result = image._manage_uefi(self.fake_dev, self.fake_root_uuid)
+        self.assertTrue(result)
+        mkdir_mock.assert_called_once_with(self.fake_dir + '/boot/efi')
+        mock_efi_bl.assert_called_once_with(self.fake_dir + '/boot/efi')
+        mock_execute.assert_has_calls(expected)
+        self.assertEqual(7, mock_execute.call_count)
+
     @mock.patch.object(os.path, 'exists', lambda *_: False)
     @mock.patch.object(image, '_get_efi_bootloaders', autospec=True)
     @mock.patch.object(image, '_get_partition', autospec=True)
@@ -2205,7 +2250,7 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
             ('/boot/efi', ['EFI'], []),
             ('/boot/efi/EFI', ['centos', 'BOOT'], []),
             ('/boot/efi/EFI/centos', ['fw', 'fonts'],
-             ['shimx64-centos.efi', 'BOOT.CSV', 'BOOTX64.CSV',
+             ['shimx64-centos.efi',
               'MokManager.efi', 'mmx64.efi', 'shim.efi', 'fwupia32.efi',
               'fwupx64.efi', 'shimx64.efi', 'grubenv', 'grubx64.efi',
               'grub.cfg']),
@@ -2226,7 +2271,28 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
             ('/boot/efi', ['EFI'], []),
             ('/boot/efi/EFI', ['centos', 'BOOT'], []),
             ('/boot/efi/EFI/centos', ['fw', 'fonts'],
-             ['shimx64-centos.efi', 'BOOT.CSV', 'BOOTX64.CSV',
+             ['shimx64-centos.efi', 'BOOTX64.CSV',
+              'MokManager.efi', 'mmx64.efi', 'shim.efi', 'fwupia32.efi',
+              'fwupx64.efi', 'shimx64.efi', 'grubenv', 'grubx64.efi',
+              'grub.cfg']),
+            ('/boot/efi/EFI/centos/fw', [], []),
+            ('/boot/efi/EFI/centos/fonts', [], ['unicode.pf2']),
+            ('/boot/efi/EFI/BOOT', [],
+             ['BOOTX64.EFI', 'fallback.efi', 'fbx64.efi'])
+        ]
+        mock_access.return_value = True
+        result = image._get_efi_bootloaders("/boot/efi")
+        self.assertEqual(result[0], 'EFI/centos/BOOTX64.CSV')
+
+    @mock.patch.object(os, 'walk', autospec=True)
+    @mock.patch.object(os, 'access', autospec=True)
+    def test__get_efi_bootloaders_no_csv(
+            self, mock_access, mock_walk, mock_execute, mock_dispatch):
+        mock_walk.return_value = [
+            ('/boot/efi', ['EFI'], []),
+            ('/boot/efi/EFI', ['centos', 'BOOT'], []),
+            ('/boot/efi/EFI/centos', ['fw', 'fonts'],
+             ['shimx64-centos.efi',
               'MokManager.efi', 'mmx64.efi', 'shim.efi', 'fwupia32.efi',
               'fwupx64.efi', 'shimx64.efi', 'grubenv', 'grubx64.efi',
               'grub.cfg']),
@@ -2255,7 +2321,8 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
 
     def test__run_efibootmgr_no_bootloaders(self, mock_execute, mock_dispatch):
         result = image._run_efibootmgr([], self.fake_dev,
-                                       self.fake_efi_system_part)
+                                       self.fake_efi_system_part,
+                                       self.fake_dir)
         expected = []
         self.assertIsNone(result)
         mock_execute.assert_has_calls(expected)
@@ -2263,7 +2330,8 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
     def test__run_efibootmgr(self, mock_execute, mock_dispatch):
         result = image._run_efibootmgr(['EFI/BOOT/BOOTX64.EFI'],
                                        self.fake_dev,
-                                       self.fake_efi_system_part)
+                                       self.fake_efi_system_part,
+                                       self.fake_dir)
         expected = [mock.call('efibootmgr'),
                     mock.call('efibootmgr', '-c', '-d', self.fake_dev,
                               '-p', self.fake_efi_system_part, '-w',

releasenotes/notes/support-bootloader-csv-file-use-c815b520c600cd98.yaml

@@ -0,0 +1,22 @@
+---
+features:
+  - |
+    Adds the capability into the agent to read and act upon bootloader CSV
+    files which serve as authoritative indicators of what bootloader to load
+    instead of leaning towards utilizing the default.
+fixes:
+  - |
+    Fixes nodes failing after deployment completes due to issues in the Grub2
+    EFI loader entry addition where a ``BOOT.CSV`` file provides the
+    authoritative pointer to the bootloader to be used for booting the OS. The
+    base issue with Grub2 is that it would update the UEFI bootloader NVRAM
+    entries with whatever is present in a vendor specific ``BOOT.CSV`` or
+    ``BOOTX64.CSV`` file. In some cases, a baremetal machine *can* crash when
+    this occurs. More information can be found at
+    `story 2008962 <https://storyboard.openstack.org/#!/story/2008962>`_.
+issues:
+  - |
+    If multiple bootloader CSV files are present on the EFI filesystem, the
+    first CSV file discovered will be utilized. The Ironic team considers
+    multiple files to be a defect in the image being deployed. This may be
+    changed in the future.