diff --git a/.gitreview b/.gitreview index 9df0f11ec..0ee69f4b9 100644 --- a/.gitreview +++ b/.gitreview @@ -2,3 +2,4 @@ host=review.opendev.org port=29418 project=openstack/ironic-python-agent.git +defaultbranch=unmaintained/zed diff --git a/README.rst b/README.rst index 4494ed5a9..42e65dcd8 100644 --- a/README.rst +++ b/README.rst @@ -11,6 +11,11 @@ Team and repository tags Overview ======== +*WARNING:* The Ironic-Python-Agent version in this branch is vulnerable to +CVE-2024-44082. Do not run this in production unless using a patched +conductor with ``[conductor]/conductor_always_validate_images`` set to +``True``. + An agent for controlling and deploying Ironic controlled baremetal nodes. The ironic-python-agent works with the agent driver in Ironic to provision diff --git a/ironic_python_agent/config.py b/ironic_python_agent/config.py index 9251a3e37..d81e67c43 100644 --- a/ironic_python_agent/config.py +++ b/ironic_python_agent/config.py @@ -326,6 +326,11 @@ 'cleaning from inadvertently destroying a running ' 'cluster which may be visible over a storage fabric ' 'such as FibreChannel.'), + cfg.BoolOpt('config_drive_rebuild', + default=False, + help='If the agent should rebuild the configuration drive ' + 'using a local filesystem, instead of letting Ironic ' + 'determine if this action is necessary.'), ] CONF.register_cli_opts(cli_opts) diff --git a/ironic_python_agent/efi_utils.py b/ironic_python_agent/efi_utils.py index 48e643d3f..6fc8dff19 100644 --- a/ironic_python_agent/efi_utils.py +++ b/ironic_python_agent/efi_utils.py @@ -275,8 +275,15 @@ def get_boot_records(): :return: an iterator yielding pairs (boot number, boot record). """ - efi_output = utils.execute('efibootmgr', '-v') - for line in efi_output[0].split('\n'): + # Invokes binary=True so we get a bytestream back. + efi_output = utils.execute('efibootmgr', '-v', binary=True) + # Bytes must be decoded before regex can be run and + # matching to work as intended. + # Also ignore errors on decoding, as we can basically get + # garbage out of the nvram record, this way we don't fail + # hard on unrelated records. + cmd_output = efi_output[0].decode('utf-16', errors='ignore') + for line in cmd_output.split('\n'): match = _ENTRY_LABEL.match(line) if match is not None: yield (match[1], match[2]) @@ -293,7 +300,7 @@ def add_boot_record(device, efi_partition, loader, label): # https://linux.die.net/man/8/efibootmgr utils.execute('efibootmgr', '-v', '-c', '-d', device, '-p', str(efi_partition), '-w', '-L', label, - '-l', loader) + '-l', loader, binary=True) def remove_boot_record(boot_num): @@ -301,7 +308,7 @@ def remove_boot_record(boot_num): :param boot_num: the number of the boot record """ - utils.execute('efibootmgr', '-b', boot_num, '-B') + utils.execute('efibootmgr', '-b', boot_num, '-B', binary=True) def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition, diff --git a/ironic_python_agent/ironic_api_client.py b/ironic_python_agent/ironic_api_client.py index a2e7e1d00..8ea7029e7 100644 --- a/ironic_python_agent/ironic_api_client.py +++ b/ironic_python_agent/ironic_api_client.py @@ -13,6 +13,7 @@ # limitations under the License. import json +import time from oslo_config import cfg from oslo_log import log @@ -45,6 +46,7 @@ class APIClient(object): heartbeat_api = '/%s/heartbeat/{uuid}' % api_version _ironic_api_version = None agent_token = None + lookup_lock_pause = 0 def __init__(self, api_url): self.api_url = api_url.rstrip('/') @@ -176,7 +178,7 @@ def heartbeat(self, uuid, advertise_address, advertise_protocol='http', raise errors.HeartbeatError(error) def lookup_node(self, hardware_info, timeout, starting_interval, - node_uuid=None, max_interval=30): + node_uuid=None, max_interval=60): retry = tenacity.retry( retry=tenacity.retry_if_result(lambda r: r is False), stop=tenacity.stop_after_delay(timeout), @@ -244,10 +246,31 @@ def _do_lookup(self, hardware_info, node_uuid): LOG.error(exc_msg) raise errors.LookupNodeError(msg) + if response.status_code == requests.codes.CONFLICT: + if self.lookup_lock_pause == 0: + self.lookup_lock_pause = 5 + elif self.lookup_lock_pause == 5: + self.lookup_lock_pause = 10 + elif self.lookup_lock_pause == 10: + # If we're reaching this point, we've got a long held + # persistent lock, which means things can go very sideways + # or the ironic deployment is downright grumpy. Either way, + # we need to slow things down. + self.lookup_lock_pause = 30 + LOG.warning( + 'Ironic has responded with a conflict, signaling the ' + 'node is locked. We will wait %(time)s seconds before trying ' + 'again. %(err)s', + {'time': self.lookup_lock_pause, + 'error': self._error_from_response(response)} + ) + time.sleep(self.lookup_lock_pause) + return False + if response.status_code != requests.codes.OK: LOG.warning( 'Failed looking up node with addresses %r at %s. ' - '%s. Check if inspection has completed.', + 'Check if inspection has completed? %s', params['addresses'], self.api_url, self._error_from_response(response) ) diff --git a/ironic_python_agent/partition_utils.py b/ironic_python_agent/partition_utils.py index cb8225fbc..ab933e03f 100644 --- a/ironic_python_agent/partition_utils.py +++ b/ironic_python_agent/partition_utils.py @@ -465,12 +465,27 @@ def create_config_drive_partition(node_uuid, device, configdrive): {'part': config_drive_part, 'node': node_uuid}) utils.execute('test', '-e', config_drive_part, attempts=15, delay_on_retry=True) - - disk_utils.dd(confdrive_file, config_drive_part) + if not CONF.config_drive_rebuild: + disk_utils.dd(confdrive_file, config_drive_part) + if not _does_config_drive_work(config_drive_part): + # If we have reached this point, we might have an + # invalid configuration drive, OR the block device + # layer doesn't support 2K block Logical IO (iso9660) + _try_build_fat32_config_drive(config_drive_part, + confdrive_file) + else: + LOG.info('Extracting configuration drive to write copy to disk.') + _try_build_fat32_config_drive(config_drive_part, confdrive_file) LOG.info("Configdrive for node %(node)s successfully " "copied onto partition %(part)s", {'node': node_uuid, 'part': config_drive_part}) + except exception.InstanceDeployFailure: + # Since we no longer have a final action on the decorator, we need + # to catch the failure, and still perform the cleanup. + if confdrive_file: + utils.unlink_without_raise(confdrive_file) + raise except (processutils.UnknownArgumentError, processutils.ProcessExecutionError, OSError) as e: msg = ('Failed to create config drive on disk %(disk)s ' @@ -478,13 +493,90 @@ def create_config_drive_partition(node_uuid, device, configdrive): {'disk': device, 'node': node_uuid, 'error': e}) LOG.error(msg) raise exception.InstanceDeployFailure(msg) - finally: # If the configdrive was requested make sure we delete the file # after copying the content to the partition + + finally: if confdrive_file: utils.unlink_without_raise(confdrive_file) +def _does_config_drive_work(config_drive_part): + """Attempts to mount the config drive to validate it works. + + :param config_drive_part: The partition to which the configuration drive + was written. + :returns: True if we were able to mount the configuration drive partition. + """ + temp_folder = tempfile.mkdtemp() + try: + # Why: If the filesystem is ISO9660 or vfat, and the logical sector + # size which is supported is *not* something which supports 512 bytes, + # i.e. a 4k Block size, then ISO9660 just will not work. Vfat also + # will not work because the logical size needs to match the logical + # size which is usable. If the underlying driver cannot use that size, + # then the filesystem will not work and cannot be updated because + # structurally it is incompaible with the block device driver. + utils.execute('mount', '-o', 'ro', '-t', 'auto', config_drive_part, + temp_folder) + utils.execute('umount', temp_folder) + except (processutils.ProcessExecutionError, OSError) as e: + LOG.error('Encountered issue attempting to validate the ' + 'supplied configuration drive. Error: %s', e) + return False + finally: + utils.unlink_without_raise(temp_folder) + + return True + + +def _try_build_fat32_config_drive(partition, confdrive_file): + conf_drive_temp = tempfile.mkdtemp() + try: + utils.execute('mount', '-o', 'loop,ro', '-t', 'auto', + confdrive_file, conf_drive_temp) + except (processutils.ProcessExecutionError, OSError) as e: + # Config drive is invalid, at least to our point of view. + # Bailing. + LOG.warning('We were unable to examine the configuration drive, ' + 'bypassing. Error: %s', e) + return + + new_drive_temp = tempfile.mkdtemp() + try: + # While creating a config drive file from scratch or on + # a loopback will likely result in a 512 byte sector size, + # the underlying fat filesystem utilities *automatically* + # check the device block sector sizing. This *will* break + # above 4k blocks, or at least might. Officially, 4k is the + # *maximum* in the FAT standard. See: + # https://github.com/dosfstools/dosfstools/blame/c483196dd46eab22abba756cef511d36f5f42070/src/mkfs.fat.c#L1987 + utils.mkfs(fs='vfat', path=partition, label='CONFIG-2') + utils.execute('mount', '-t', 'auto', partition, new_drive_temp) + # copytree, using copy2, copies everything in the source folder + # into the destination folder, so we should be good, and metadata + # is attempted to be preserved. + shutil.copytree(conf_drive_temp, new_drive_temp, dirs_exist_ok=True) + except (processutils.ProcessExecutionError, OSError) as e: + # We failed to make the filesystem :( + # This is a fairly hard error as we could not use the + # config drive, nor could we recover the state. + LOG.error('We were unable to make a new filesystem for the ' + 'configuration drive. Error: %s', e) + msg = ('A failure occured while attempting to format, copy, and ' + 're-create the configuration drive in a structure which ' + 'is compatible with the underlying hardware and Operating ' + 'System. Due to the nature of configuration drive, it could ' + 'have been incorrectly formatted. Operator investigation is ' + 'required. Error: {}'.format(str(e))) + raise exception.InstanceDeployFailure(msg) + finally: + utils.execute('umount', conf_drive_temp) + utils.execute('umount', new_drive_temp) + utils.unlink_without_raise(new_drive_temp) + utils.unlink_without_raise(conf_drive_temp) + + def _is_disk_larger_than_max_size(device, node_uuid): """Check if total disk size exceeds 2TB msdos limit diff --git a/ironic_python_agent/raid_utils.py b/ironic_python_agent/raid_utils.py index 84c6941fd..27b0bb5b2 100644 --- a/ironic_python_agent/raid_utils.py +++ b/ironic_python_agent/raid_utils.py @@ -12,6 +12,7 @@ import copy import re +import shlex from ironic_lib import disk_utils from ironic_lib import utils as il_utils @@ -342,50 +343,58 @@ def prepare_boot_partitions_for_softraid(device, holders, efi_part, if efi_part: efi_part = '{}p{}'.format(device, efi_part['number']) - LOG.info("Creating EFI partitions on software RAID holder disks") - # We know that we kept this space when configuring raid,see - # hardware.GenericHardwareManager.create_configuration. - # We could also directly get the EFI partition size. - partsize_mib = ESP_SIZE_MIB - partlabel_prefix = 'uefi-holder-' - efi_partitions = [] - for number, holder in enumerate(holders): - # NOTE: see utils.get_partition_table_type_from_specs - # for uefi we know that we have setup a gpt partition table, - # sgdisk can be used to edit table, more user friendly - # for alignment and relative offsets - partlabel = '{}{}'.format(partlabel_prefix, number) - out, _u = utils.execute('sgdisk', '-F', holder) - start_sector = '{}s'.format(out.splitlines()[-1].strip()) - out, _u = utils.execute( - 'sgdisk', '-n', '0:{}:+{}MiB'.format(start_sector, - partsize_mib), - '-t', '0:ef00', '-c', '0:{}'.format(partlabel), holder) - - # Refresh part table - utils.execute("partprobe") - utils.execute("blkid") - - target_part, _u = utils.execute( - "blkid", "-l", "-t", "PARTLABEL={}".format(partlabel), holder) - - target_part = target_part.splitlines()[-1].split(':', 1)[0] - efi_partitions.append(target_part) - - LOG.debug("EFI partition %s created on holder disk %s", - target_part, holder) - - # RAID the ESPs, metadata=1.0 is mandatory to be able to boot - md_device = get_next_free_raid_device() - LOG.debug("Creating md device %(md_device)s for the ESPs " - "on %(efi_partitions)s", - {'md_device': md_device, 'efi_partitions': efi_partitions}) - utils.execute('mdadm', '--create', md_device, '--force', - '--run', '--metadata=1.0', '--level', '1', - '--name', 'esp', '--raid-devices', len(efi_partitions), - *efi_partitions) - - disk_utils.trigger_device_rescan(md_device) + # check if we have a RAIDed ESP already + md_device = find_esp_raid() + if md_device: + LOG.info("Found RAIDed ESP %s, skip creation", md_device) + else: + LOG.info("Creating EFI partitions on software RAID holder disks") + # We know that we kept this space when configuring raid,see + # hardware.GenericHardwareManager.create_configuration. + # We could also directly get the EFI partition size. + partsize_mib = ESP_SIZE_MIB + partlabel_prefix = 'uefi-holder-' + efi_partitions = [] + for number, holder in enumerate(holders): + # NOTE: see utils.get_partition_table_type_from_specs + # for uefi we know that we have setup a gpt partition table, + # sgdisk can be used to edit table, more user friendly + # for alignment and relative offsets + partlabel = '{}{}'.format(partlabel_prefix, number) + out, _u = utils.execute('sgdisk', '-F', holder) + start_sector = '{}s'.format(out.splitlines()[-1].strip()) + out, _u = utils.execute( + 'sgdisk', '-n', '0:{}:+{}MiB'.format(start_sector, + partsize_mib), + '-t', '0:ef00', '-c', '0:{}'.format(partlabel), holder) + + # Refresh part table + utils.execute("partprobe") + utils.execute("blkid") + + target_part, _u = utils.execute( + "blkid", "-l", "-t", "PARTLABEL={}".format(partlabel), + holder) + + target_part = target_part.splitlines()[-1].split(':', 1)[0] + efi_partitions.append(target_part) + + LOG.debug("EFI partition %s created on holder disk %s", + target_part, holder) + + # RAID the ESPs, metadata=1.0 is mandatory to be able to boot + md_device = get_next_free_raid_device() + LOG.debug("Creating md device %(md_device)s for the ESPs " + "on %(efi_partitions)s", + {'md_device': md_device, + 'efi_partitions': efi_partitions}) + utils.execute('mdadm', '--create', md_device, '--force', + '--run', '--metadata=1.0', '--level', '1', + '--name', 'esp', '--raid-devices', + len(efi_partitions), + *efi_partitions) + + disk_utils.trigger_device_rescan(md_device) if efi_part: # Blockdev copy the source ESP and erase it @@ -420,3 +429,18 @@ def prepare_boot_partitions_for_softraid(device, holders, efi_part, # disk, as in virtual disk, where to load the data from. # Since there is a structural difference, this means it will # fail. + + +def find_esp_raid(): + """Find the ESP md device in case of a rebuild.""" + + # find devices of type 'RAID1' and fstype 'VFAT' + lsblk = utils.execute('lsblk', '-PbioNAME,TYPE,FSTYPE') + report = lsblk[0] + for line in report.split('\n'): + dev = {} + vals = shlex.split(line) + for key, val in (v.split('=', 1) for v in vals): + dev[key] = val.strip() + if dev.get('TYPE') == 'raid1' and dev.get('FSTYPE') == 'vfat': + return '/dev/' + dev.get('NAME') diff --git a/ironic_python_agent/tests/unit/extensions/test_image.py b/ironic_python_agent/tests/unit/extensions/test_image.py index e488b74f7..32f4addb5 100644 --- a/ironic_python_agent/tests/unit/extensions/test_image.py +++ b/ironic_python_agent/tests/unit/extensions/test_image.py @@ -32,6 +32,9 @@ from ironic_python_agent.tests.unit.samples import hardware_samples as hws +EFI_RESULT = ''.encode('utf-16') + + @mock.patch.object(hardware, 'dispatch_to_managers', autospec=True) @mock.patch.object(ilib_utils, 'execute', autospec=True) @mock.patch.object(tempfile, 'mkdtemp', lambda *_: '/tmp/fake-dir') @@ -231,7 +234,7 @@ def test__uefi_bootloader_given_partition( mock_execute.side_effect = iter([('', ''), ('', ''), ('', ''), ('', ''), - ('', ''), ('', ''), + (EFI_RESULT, ''), (EFI_RESULT, ''), ('', ''), ('', '')]) expected = [mock.call('efibootmgr', '--version'), @@ -240,11 +243,11 @@ def test__uefi_bootloader_given_partition( mock.call('udevadm', 'settle'), mock.call('mount', self.fake_efi_system_part, self.fake_dir + '/boot/efi'), - mock.call('efibootmgr', '-v'), + mock.call('efibootmgr', '-v', binary=True), mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev, '-p', '1', '-w', '-L', 'ironic1', '-l', - '\\EFI\\BOOT\\BOOTX64.EFI'), + '\\EFI\\BOOT\\BOOTX64.EFI', binary=True), mock.call('umount', self.fake_dir + '/boot/efi', attempts=3, delay_on_retry=True), mock.call('sync')] @@ -279,7 +282,7 @@ def test__uefi_bootloader_find_partition( mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI'] mock_execute.side_effect = iter([('', ''), ('', ''), ('', ''), ('', ''), - ('', ''), ('', ''), + (EFI_RESULT, ''), (EFI_RESULT, ''), ('', ''), ('', '')]) expected = [mock.call('efibootmgr', '--version'), @@ -288,11 +291,11 @@ def test__uefi_bootloader_find_partition( mock.call('udevadm', 'settle'), mock.call('mount', self.fake_efi_system_part, self.fake_dir + '/boot/efi'), - mock.call('efibootmgr', '-v'), + mock.call('efibootmgr', '-v', binary=True), mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev, '-p', '1', '-w', '-L', 'ironic1', '-l', - '\\EFI\\BOOT\\BOOTX64.EFI'), + '\\EFI\\BOOT\\BOOTX64.EFI', binary=True), mock.call('umount', self.fake_dir + '/boot/efi', attempts=3, delay_on_retry=True), mock.call('sync')] @@ -333,10 +336,11 @@ def test__uefi_bootloader_with_entry_removal( Boot0001 ironic2 HD(1,GPT,4f3c6294-bf9b-4208-9808-111111111112)File(\EFI\Boot\BOOTX64.EFI) Boot0002 VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51) """ # noqa This is a giant literal string for testing. + stdout_msg = stdout_msg.encode('utf-16') mock_execute.side_effect = iter([('', ''), ('', ''), ('', ''), ('', ''), - (stdout_msg, ''), ('', ''), - ('', ''), ('', ''), + (stdout_msg, ''), (EFI_RESULT, ''), + (EFI_RESULT, ''), (EFI_RESULT, ''), ('', ''), ('', '')]) expected = [mock.call('efibootmgr', '--version'), @@ -345,12 +349,12 @@ def test__uefi_bootloader_with_entry_removal( mock.call('udevadm', 'settle'), mock.call('mount', self.fake_efi_system_part, self.fake_dir + '/boot/efi'), - mock.call('efibootmgr', '-v'), - mock.call('efibootmgr', '-b', '0000', '-B'), + mock.call('efibootmgr', '-v', binary=True), + mock.call('efibootmgr', '-b', '0000', '-B', binary=True), mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev, '-p', '1', '-w', '-L', 'ironic1', '-l', - '\\EFI\\BOOT\\BOOTX64.EFI'), + '\\EFI\\BOOT\\BOOTX64.EFI', binary=True), mock.call('umount', self.fake_dir + '/boot/efi', attempts=3, delay_on_retry=True), mock.call('sync')] @@ -396,6 +400,7 @@ def test__uefi_bootloader_with_entry_removal_lenovo( Boot0003* Network VenHw(1fad3248-0000-7950-2166-a1e506fdb83a,05000000)..GO..NO............U.E.F.I.:. . . .S.L.O.T.2. .(.2.F./.0./.0.). .P.X.E. .I.P.4. . .Q.L.o.g.i.c. .Q.L.4.1.2.6.2. .P.C.I.e. .2.5.G.b. .2.-.P.o.r.t. .S.F.P.2.8. .E.t.h.e.r.n.e.t. .A.d.a.p.t.e.r. .-. .P.X.E........A....................%.4..Z...............................................................Gd-.;.A..MQ..L.P.X.E. .I.P.4. .Q.L.o.g.i.c. .Q.L.4.1.2.6.2. .P.C.I.e. .2.5.G.b. .2.-.P.o.r.t. .S.F.P.2.8. .E.t.h.e.r.n.e.t. .A.d.a.p.t.e.r. .-. .P.X.E.......BO..NO............U.E.F.I.:. . . .S.L.O.T.1. .(.3.0./.0./.0.). .P.X.E. .I.P.4. . .Q.L.o.g.i.c. .Q.L.4.1.2.6.2. .P.C.I.e. .2.5.G.b. .2.-.P.o.r.t. .S.F.P.2.8. .E.t.h.e.r.n.e.t. .A.d.a.p.t.e.r. .-. Boot0004* ironic1 HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x64000)/File(\EFI\boot\shimx64.efi) """ # noqa This is a giant literal string for testing. + stdout_msg = stdout_msg.encode('utf-16') mock_execute.side_effect = iter([('', ''), ('', ''), ('', ''), ('', ''), (stdout_msg, ''), ('', ''), @@ -407,13 +412,13 @@ def test__uefi_bootloader_with_entry_removal_lenovo( mock.call('udevadm', 'settle'), mock.call('mount', self.fake_efi_system_part, self.fake_dir + '/boot/efi'), - mock.call('efibootmgr', '-v'), - mock.call('efibootmgr', '-b', '0000', '-B'), - mock.call('efibootmgr', '-b', '0004', '-B'), + mock.call('efibootmgr', '-v', binary=True), + mock.call('efibootmgr', '-b', '0000', '-B', binary=True), + mock.call('efibootmgr', '-b', '0004', '-B', binary=True), mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev, '-p', '1', '-w', '-L', 'ironic1', '-l', - '\\EFI\\BOOT\\BOOTX64.EFI'), + '\\EFI\\BOOT\\BOOTX64.EFI', binary=True), mock.call('umount', self.fake_dir + '/boot/efi', attempts=3, delay_on_retry=True), mock.call('sync')] @@ -450,8 +455,8 @@ def test__add_multi_bootloaders( mock_execute.side_effect = iter([('', ''), ('', ''), ('', ''), ('', ''), - ('', ''), ('', ''), - ('', ''), ('', ''), + (EFI_RESULT, ''), (EFI_RESULT, ''), + (EFI_RESULT, ''), ('', ''), ('', '')]) expected = [mock.call('efibootmgr', '--version'), @@ -460,15 +465,15 @@ def test__add_multi_bootloaders( mock.call('udevadm', 'settle'), mock.call('mount', self.fake_efi_system_part, self.fake_dir + '/boot/efi'), - mock.call('efibootmgr', '-v'), + mock.call('efibootmgr', '-v', binary=True), mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev, '-p', '1', '-w', '-L', 'ironic1', '-l', - '\\EFI\\BOOT\\BOOTX64.EFI'), + '\\EFI\\BOOT\\BOOTX64.EFI', binary=True), mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev, '-p', '1', '-w', '-L', 'ironic2', '-l', - '\\WINDOWS\\system32\\winload.efi'), + '\\WINDOWS\\system32\\winload.efi', binary=True), mock.call('umount', self.fake_dir + '/boot/efi', attempts=3, delay_on_retry=True), mock.call('sync')] diff --git a/ironic_python_agent/tests/unit/samples/hardware_samples.py b/ironic_python_agent/tests/unit/samples/hardware_samples.py index c9e597a94..b8b80585d 100644 --- a/ironic_python_agent/tests/unit/samples/hardware_samples.py +++ b/ironic_python_agent/tests/unit/samples/hardware_samples.py @@ -1667,3 +1667,33 @@ ' `-+- policy=\'service-time 0\' prio=1 status=active\n' ' `- 0:0:0:0 device s 8:0 active ready running\n' ) + +LSBLK_OUPUT = (""" +NAME="sda" TYPE="disk" FSTYPE="" +NAME="sdb" TYPE="disk" FSTYPE="" +""") + +LSBLK_OUPUT_ESP_RAID = (""" +NAME="sda" TYPE="disk" FSTYPE="" +NAME="sda1" TYPE="part" FSTYPE="linux_raid_member" +NAME="md127" TYPE="raid1" FSTYPE="" +NAME="md127p1" TYPE="md" FSTYPE="xfs" +NAME="md127p2" TYPE="md" FSTYPE="iso9660" +NAME="md127p14" TYPE="md" FSTYPE="" +NAME="md127p15" TYPE="md" FSTYPE="" +NAME="sda2" TYPE="part" FSTYPE="linux_raid_member" +NAME="md126" TYPE="raid0" FSTYPE="" +NAME="sda3" TYPE="part" FSTYPE="linux_raid_member" +NAME="md125" TYPE="raid1" FSTYPE="vfat" +NAME="sdb" TYPE="disk" FSTYPE="" +NAME="sdb1" TYPE="part" FSTYPE="linux_raid_member" +NAME="md127" TYPE="raid1" FSTYPE="" +NAME="md127p1" TYPE="md" FSTYPE="xfs" +NAME="md127p2" TYPE="md" FSTYPE="iso9660" +NAME="md127p14" TYPE="md" FSTYPE="" +NAME="md127p15" TYPE="md" FSTYPE="" +NAME="sdb2" TYPE="part" FSTYPE="linux_raid_member" +NAME="md126" TYPE="raid0" FSTYPE="" +NAME="sdb3" TYPE="part" FSTYPE="linux_raid_member" +NAME="md125" TYPE="raid1" FSTYPE="vfat" +""") diff --git a/ironic_python_agent/tests/unit/test_efi_utils.py b/ironic_python_agent/tests/unit/test_efi_utils.py index 64de61bce..09e897966 100644 --- a/ironic_python_agent/tests/unit/test_efi_utils.py +++ b/ironic_python_agent/tests/unit/test_efi_utils.py @@ -28,6 +28,9 @@ from ironic_python_agent import utils +EFI_RESULT = ''.encode('utf-16') + + @mock.patch.object(os, 'walk', autospec=True) @mock.patch.object(os, 'access', autospec=False) class TestGetEfiBootloaders(base.IronicAgentTest): @@ -115,16 +118,16 @@ def test__run_efibootmgr_no_bootloaders(self, mock_execute): mock_execute.assert_has_calls(expected) def test__run_efibootmgr(self, mock_execute): - mock_execute.return_value = ('', '') + mock_execute.return_value = (''.encode('utf-16'), '') result = efi_utils._run_efibootmgr(['EFI/BOOT/BOOTX64.EFI'], self.fake_dev, self.fake_efi_system_part, self.fake_dir) - expected = [mock.call('efibootmgr', '-v'), + expected = [mock.call('efibootmgr', '-v', binary=True), mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev, '-p', self.fake_efi_system_part, '-w', '-L', 'ironic1', '-l', - '\\EFI\\BOOT\\BOOTX64.EFI')] + '\\EFI\\BOOT\\BOOTX64.EFI', binary=True)] self.assertIsNone(result) mock_execute.assert_has_calls(expected) @@ -176,19 +179,18 @@ def test_ok(self, mkdir_mock, mock_efi_bl, mock_is_md_device, mock_is_md_device.return_value = False mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI'] - - mock_execute.side_effect = iter([('', ''), ('', ''), - ('', ''), ('', ''), + mock_execute.side_effect = iter([('', ''), (EFI_RESULT, ''), + (EFI_RESULT, ''), ('', ''), ('', ''), ('', ''), ('', '')]) expected = [mock.call('mount', self.fake_efi_system_part, self.fake_dir + '/boot/efi'), - mock.call('efibootmgr', '-v'), + mock.call('efibootmgr', '-v', binary=True), mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev, '-p', '1', '-w', '-L', 'ironic1', '-l', - '\\EFI\\BOOT\\BOOTX64.EFI'), + '\\EFI\\BOOT\\BOOTX64.EFI', binary=True), mock.call('umount', self.fake_dir + '/boot/efi', attempts=3, delay_on_retry=True), mock.call('sync')] @@ -219,30 +221,34 @@ def test_found_csv(self, mkdir_mock, mock_efi_bl, mock_is_md_device, # at the start of the file, where as Red Hat *does* csv_file_data = u'shimx64.efi,Vendor String,,Grub2MadeUSDoThis\n' # This test also handles deleting a pre-existing matching vendor - # string in advance. + # string in advance. This string also includes a UTF16 character + # *on* purpose, to force proper decoding to be tested and garbage + # characters which can be found in OVMF test VM NVRAM records. dupe_entry = """ BootCurrent: 0001 Timeout: 0 seconds BootOrder: 0000,00001 -Boot0000* Vendor String HD(1,GPT,4f3c6294-bf9b-4208-9808-be45dfc34b5c)File(\EFI\Boot\BOOTX64.EFI) -Boot0001 Vendor String HD(2,GPT,4f3c6294-bf9b-4208-9808-be45dfc34b5c)File(\EFI\Boot\BOOTX64.EFI) -Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51) +Boot0000 UTF16ΓΏ HD(1,GPT,4f3c6294-bf9b-4208-9808-be45dfc34b5c)File(\EFI\Boot\BOOTX64.EFI) +Boot0001* Vendor String HD(1,GPT,4f3c6294-bf9b-4208-9808-be45dfc34b5c)File(\EFI\Boot\BOOTX64.EFI) +Boot0002 Vendor String HD(2,GPT,4f3c6294-bf9b-4208-9808-be45dfc34b5c)File(\EFI\Boot\BOOTX64.EFI) +Boot0003: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51)N.....YM....R,Y. """ # noqa This is a giant literal string for testing. - - mock_execute.side_effect = iter([('', ''), (dupe_entry, ''), + dupe_entry = dupe_entry.encode('utf-16') + mock_execute.side_effect = iter([('', ''), + (dupe_entry, ''), ('', ''), ('', ''), ('', ''), ('', ''), ('', '')]) expected = [mock.call('mount', self.fake_efi_system_part, self.fake_dir + '/boot/efi'), - mock.call('efibootmgr', '-v'), - mock.call('efibootmgr', '-b', '0000', '-B'), - mock.call('efibootmgr', '-b', '0001', '-B'), + mock.call('efibootmgr', '-v', binary=True), + mock.call('efibootmgr', '-b', '0001', '-B', binary=True), + mock.call('efibootmgr', '-b', '0002', '-B', binary=True), mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev, '-p', '1', '-w', '-L', 'Vendor String', '-l', - '\\EFI\\vendor\\shimx64.efi'), + '\\EFI\\vendor\\shimx64.efi', binary=True), mock.call('umount', self.fake_dir + '/boot/efi', attempts=3, delay_on_retry=True), mock.call('sync')] @@ -266,19 +272,18 @@ def test_nvme_device(self, mkdir_mock, mock_efi_bl, mock_is_md_device, mock_is_md_device.return_value = False mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI'] - - mock_execute.side_effect = iter([('', ''), ('', ''), - ('', ''), ('', ''), + mock_execute.side_effect = iter([('', ''), (EFI_RESULT, ''), + (EFI_RESULT, ''), ('', ''), ('', ''), ('', ''), ('', '')]) expected = [mock.call('mount', '/dev/fakenvme0p1', self.fake_dir + '/boot/efi'), - mock.call('efibootmgr', '-v'), + mock.call('efibootmgr', '-v', binary=True), mock.call('efibootmgr', '-v', '-c', '-d', '/dev/fakenvme0', '-p', '1', '-w', '-L', 'ironic1', '-l', - '\\EFI\\BOOT\\BOOTX64.EFI'), + '\\EFI\\BOOT\\BOOTX64.EFI', binary=True), mock.call('umount', self.fake_dir + '/boot/efi', attempts=3, delay_on_retry=True), mock.call('sync')] @@ -301,19 +306,18 @@ def test_wholedisk(self, mkdir_mock, mock_efi_bl, mock_is_md_device, mock_is_md_device.return_value = False mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI'] - - mock_execute.side_effect = iter([('', ''), ('', ''), - ('', ''), ('', ''), + mock_execute.side_effect = iter([('', ''), (EFI_RESULT, ''), + (EFI_RESULT, ''), ('', ''), ('', ''), ('', ''), ('', '')]) expected = [mock.call('mount', self.fake_efi_system_part, self.fake_dir + '/boot/efi'), - mock.call('efibootmgr', '-v'), + mock.call('efibootmgr', '-v', binary=True), mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev, '-p', '1', '-w', '-L', 'ironic1', '-l', - '\\EFI\\BOOT\\BOOTX64.EFI'), + '\\EFI\\BOOT\\BOOTX64.EFI', binary=True), mock.call('umount', self.fake_dir + '/boot/efi', attempts=3, delay_on_retry=True), mock.call('sync')] @@ -347,11 +351,14 @@ def test_software_raid(self, mkdir_mock, mock_efi_bl, mock_is_md_device, mock_get_component_devices.return_value = ['/dev/sda3', '/dev/sdb3'] mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI'] - - mock_execute.side_effect = iter([('', ''), ('', ''), - ('', ''), ('', ''), - ('', ''), ('', ''), - ('', ''), ('', ''), + mock_execute.side_effect = iter([('', ''), + ('', ''), + ('', ''), + (EFI_RESULT, ''), + (EFI_RESULT, ''), + (EFI_RESULT, ''), + (EFI_RESULT, ''), + ('', ''), ('', '')]) expected = [mock.call('mount', self.fake_efi_system_part, @@ -360,14 +367,14 @@ def test_software_raid(self, mkdir_mock, mock_efi_bl, mock_is_md_device, attempts=3, delay_on_retry=True), mock.call('mount', self.fake_efi_system_part, self.fake_dir + '/boot/efi'), - mock.call('efibootmgr', '-v'), + mock.call('efibootmgr', '-v', binary=True), mock.call('efibootmgr', '-v', '-c', '-d', '/dev/sda3', '-p', '3', '-w', '-L', 'ironic1 (RAID, part0)', - '-l', '\\EFI\\BOOT\\BOOTX64.EFI'), - mock.call('efibootmgr', '-v'), + '-l', '\\EFI\\BOOT\\BOOTX64.EFI', binary=True), + mock.call('efibootmgr', '-v', binary=True), mock.call('efibootmgr', '-v', '-c', '-d', '/dev/sdb3', '-p', '3', '-w', '-L', 'ironic1 (RAID, part1)', - '-l', '\\EFI\\BOOT\\BOOTX64.EFI'), + '-l', '\\EFI\\BOOT\\BOOTX64.EFI', binary=True), mock.call('umount', self.fake_dir + '/boot/efi', attempts=3, delay_on_retry=True), mock.call('sync')] @@ -425,7 +432,7 @@ def test_failure_after_mount(self, mkdir_mock, mock_efi_bl, expected = [mock.call('mount', self.fake_efi_system_part, self.fake_dir + '/boot/efi'), - mock.call('efibootmgr', '-v'), + mock.call('efibootmgr', '-v', binary=True), mock.call('umount', self.fake_dir + '/boot/efi', attempts=3, delay_on_retry=True), mock.call('sync')] @@ -462,7 +469,7 @@ def test_failure_after_failure(self, mkdir_mock, mock_efi_bl, expected = [mock.call('mount', self.fake_efi_system_part, self.fake_dir + '/boot/efi'), - mock.call('efibootmgr', '-v'), + mock.call('efibootmgr', '-v', binary=True), mock.call('umount', self.fake_dir + '/boot/efi', attempts=3, delay_on_retry=True)] diff --git a/ironic_python_agent/tests/unit/test_ironic_api_client.py b/ironic_python_agent/tests/unit/test_ironic_api_client.py index 8c05652ce..6c101acc7 100644 --- a/ironic_python_agent/tests/unit/test_ironic_api_client.py +++ b/ironic_python_agent/tests/unit/test_ironic_api_client.py @@ -13,6 +13,7 @@ # limitations under the License. import json +import time from unittest import mock from oslo_config import cfg @@ -461,6 +462,32 @@ def test_do_lookup_transient_exceptions(self, mock_log): mock_log.error.assert_has_calls([]) self.assertEqual(1, mock_log.warning.call_count) + @mock.patch.object(time, 'sleep', autospec=True) + @mock.patch.object(ironic_api_client, 'LOG', autospec=True) + def test_do_lookup_node_locked(self, mock_log, mock_sleep): + response = FakeResponse(status_code=409, content={}) + self.api_client.session.request = mock.Mock() + self.api_client.session.request.return_value = response + self.assertEqual(0, self.api_client.lookup_lock_pause) + error = self.api_client._do_lookup(self.hardware_info, + node_uuid=None) + self.assertFalse(error) + mock_log.error.assert_has_calls([]) + self.assertEqual(1, mock_log.warning.call_count) + self.assertEqual(1, mock_sleep.call_count) + self.assertEqual(5, self.api_client.lookup_lock_pause) + error = self.api_client._do_lookup(self.hardware_info, + node_uuid=None) + self.assertEqual(10, self.api_client.lookup_lock_pause) + error = self.api_client._do_lookup(self.hardware_info, + node_uuid=None) + self.assertFalse(error) + self.assertEqual(30, self.api_client.lookup_lock_pause) + error = self.api_client._do_lookup(self.hardware_info, + node_uuid=None) + self.assertFalse(error) + self.assertEqual(30, self.api_client.lookup_lock_pause) + @mock.patch.object(ironic_api_client, 'LOG', autospec=True) def test_do_lookup_unknown_exception(self, mock_log): self.api_client.session.request = mock.Mock() diff --git a/ironic_python_agent/tests/unit/test_partition_utils.py b/ironic_python_agent/tests/unit/test_partition_utils.py index 4923642b7..b9893f1bd 100644 --- a/ironic_python_agent/tests/unit/test_partition_utils.py +++ b/ironic_python_agent/tests/unit/test_partition_utils.py @@ -20,6 +20,7 @@ from ironic_lib import exception from ironic_lib import utils from oslo_concurrency import processutils +from oslo_config import cfg import requests from ironic_python_agent import errors @@ -28,6 +29,9 @@ from ironic_python_agent.tests.unit import base +CONF = cfg.CONF + + @mock.patch.object(shutil, 'copyfileobj', autospec=True) @mock.patch.object(requests, 'get', autospec=True) class GetConfigdriveTestCase(base.IronicAgentTest): @@ -649,7 +653,10 @@ def test_create_partition_exists(self, mock_get_configdrive, self.config_part_label, self.node_uuid) self.assertFalse(mock_list_partitions.called) - self.assertFalse(mock_execute.called) + mock_execute.assert_has_calls([ + mock.call('mount', '-o', 'ro', '-t', 'auto', + '/dev/fake-part1', mock.ANY), + mock.call('umount', mock.ANY)]) self.assertFalse(mock_table_type.called) mock_dd.assert_called_with(configdrive_file, configdrive_part) mock_unlink.assert_called_with(configdrive_file) @@ -706,6 +713,135 @@ def test_create_partition_gpt(self, mock_get_configdrive, mock_dd.assert_called_with(configdrive_file, expected_part) mock_unlink.assert_called_with(configdrive_file) + @mock.patch('oslo_utils.uuidutils.generate_uuid', lambda: 'fake-uuid') + @mock.patch.object(partition_utils, '_try_build_fat32_config_drive', + autospec=True) + @mock.patch.object(partition_utils, '_does_config_drive_work', + autospec=True) + @mock.patch.object(utils, 'execute', autospec=True) + @mock.patch.object(utils, 'unlink_without_raise', + autospec=True) + @mock.patch.object(disk_utils, 'dd', + autospec=True) + @mock.patch.object(disk_utils, 'fix_gpt_partition', + autospec=True) + @mock.patch.object(disk_utils, 'get_partition_table_type', + autospec=True) + @mock.patch.object(partition_utils, 'get_partition', + autospec=True) + @mock.patch.object(partition_utils, 'get_labelled_partition', + autospec=True) + @mock.patch.object(partition_utils, 'get_configdrive', + autospec=True) + def test_create_partition_gpt_with_fallback( + self, mock_get_configdrive, + mock_get_labelled_partition, + mock_get_partition_by_uuid, + mock_table_type, + mock_fix_gpt_partition, + mock_dd, mock_unlink, mock_execute, + mock_config_drive_work, + mock_rebuild_config_drive): + config_url = 'http://1.2.3.4/cd' + configdrive_file = '/tmp/xyz' + configdrive_mb = 10 + + mock_get_configdrive.return_value = (configdrive_mb, configdrive_file) + mock_get_labelled_partition.return_value = None + + mock_table_type.return_value = 'gpt' + expected_part = '/dev/fake4' + mock_get_partition_by_uuid.return_value = expected_part + mock_config_drive_work.return_value = False + + partition_utils.create_config_drive_partition(self.node_uuid, self.dev, + config_url) + mock_execute.assert_has_calls([ + mock.call('sgdisk', '-n', '0:-64MB:0', '-u', '0:fake-uuid', + self.dev, run_as_root=True), + mock.call('sync'), + mock.call('udevadm', 'settle'), + mock.call('partprobe', self.dev, attempts=10, run_as_root=True), + mock.call('sgdisk', '-v', self.dev, run_as_root=True), + + mock.call('udevadm', 'settle'), + mock.call('test', '-e', expected_part, attempts=15, + delay_on_retry=True) + ]) + + mock_table_type.assert_called_with(self.dev) + mock_fix_gpt_partition.assert_called_with(self.dev, self.node_uuid) + mock_dd.assert_called_with(configdrive_file, expected_part) + mock_unlink.assert_called_with(configdrive_file) + mock_config_drive_work.assert_called_once_with(expected_part) + mock_rebuild_config_drive.assert_called_once_with(expected_part, + configdrive_file) + + @mock.patch('oslo_utils.uuidutils.generate_uuid', lambda: 'fake-uuid') + @mock.patch.object(partition_utils, '_try_build_fat32_config_drive', + autospec=True) + @mock.patch.object(partition_utils, '_does_config_drive_work', + autospec=True) + @mock.patch.object(utils, 'execute', autospec=True) + @mock.patch.object(utils, 'unlink_without_raise', + autospec=True) + @mock.patch.object(disk_utils, 'dd', + autospec=True) + @mock.patch.object(disk_utils, 'fix_gpt_partition', + autospec=True) + @mock.patch.object(disk_utils, 'get_partition_table_type', + autospec=True) + @mock.patch.object(partition_utils, 'get_partition', + autospec=True) + @mock.patch.object(partition_utils, 'get_labelled_partition', + autospec=True) + @mock.patch.object(partition_utils, 'get_configdrive', + autospec=True) + def test_create_partition_gpt_use_vfat( + self, mock_get_configdrive, + mock_get_labelled_partition, + mock_get_partition_by_uuid, + mock_table_type, + mock_fix_gpt_partition, + mock_dd, mock_unlink, mock_execute, + mock_config_drive_work, + mock_rebuild_config_drive): + config_url = 'http://1.2.3.4/cd' + configdrive_file = '/tmp/xyz' + configdrive_mb = 10 + + CONF.set_override('config_drive_rebuild', True) + mock_get_configdrive.return_value = (configdrive_mb, configdrive_file) + mock_get_labelled_partition.return_value = None + + mock_table_type.return_value = 'gpt' + expected_part = '/dev/fake4' + mock_get_partition_by_uuid.return_value = expected_part + mock_config_drive_work.return_value = True + + partition_utils.create_config_drive_partition(self.node_uuid, self.dev, + config_url) + mock_execute.assert_has_calls([ + mock.call('sgdisk', '-n', '0:-64MB:0', '-u', '0:fake-uuid', + self.dev, run_as_root=True), + mock.call('sync'), + mock.call('udevadm', 'settle'), + mock.call('partprobe', self.dev, attempts=10, run_as_root=True), + mock.call('sgdisk', '-v', self.dev, run_as_root=True), + + mock.call('udevadm', 'settle'), + mock.call('test', '-e', expected_part, attempts=15, + delay_on_retry=True) + ]) + + mock_table_type.assert_called_with(self.dev) + mock_fix_gpt_partition.assert_called_with(self.dev, self.node_uuid) + mock_dd.assert_not_called() + mock_unlink.assert_called_with(configdrive_file) + mock_config_drive_work.assert_not_called() + mock_rebuild_config_drive.assert_called_once_with(expected_part, + configdrive_file) + @mock.patch.object(disk_utils, 'count_mbr_partitions', autospec=True) @mock.patch.object(utils, 'execute', autospec=True) @mock.patch.object(partition_utils.LOG, 'warning', autospec=True) @@ -1288,3 +1424,94 @@ def test_label(self, mock_is_md_device, mock_execute): mock.call('lsblk', '-PbioKNAME,UUID,PARTUUID,TYPE,LABEL', self.fake_dev)] mock_execute.assert_has_calls(expected) + + +@mock.patch.object(utils, 'execute', autospec=True) +class TestConfigDriveTestRecovery(base.IronicAgentTest): + + fake_dev = '/dev/fake' + configdrive_file = '/tmp/config-drive' + + def test__does_config_drive_work(self, mock_execute): + self.assertTrue(partition_utils._does_config_drive_work(self.fake_dev)) + mock_execute.assert_has_calls([ + mock.call('mount', '-o', 'ro', '-t', 'auto', self.fake_dev, + mock.ANY), + mock.call('umount', mock.ANY)]) + + def test__does_config_drive_failed(self, mock_execute): + mock_execute.side_effect = processutils.ProcessExecutionError('boom') + self.assertFalse( + partition_utils._does_config_drive_work(self.fake_dev) + ) + mock_execute.assert_has_calls([ + mock.call('mount', '-o', 'ro', '-t', 'auto', self.fake_dev, + mock.ANY)]) + + @mock.patch.object(shutil, 'copytree', autospec=True) + @mock.patch.object(utils, 'mkfs', autospec=True) + def test__try_build_fat32_config_drive(self, + mock_mkfs, + mock_copy, + mock_execute): + partition_utils._try_build_fat32_config_drive(self.fake_dev, + self.configdrive_file) + mock_execute.assert_has_calls([ + mock.call('mount', '-o', 'loop,ro', '-t', 'auto', + self.configdrive_file, mock.ANY), + mock.call('mount', '-t', 'auto', self.fake_dev, mock.ANY), + mock.call('umount', mock.ANY), + mock.call('umount', mock.ANY), + ]) + mock_mkfs.assert_called_once_with(fs='vfat', path=self.fake_dev, + label='CONFIG-2') + # Validate we called copy as we expect, both source and destination + # are temporary folders. + mock_copy.assert_called_once_with(mock.ANY, mock.ANY, + dirs_exist_ok=True) + + @mock.patch.object(shutil, 'copytree', autospec=True) + @mock.patch.object(utils, 'mkfs', autospec=True) + def test__try_build_fat32_config_drive_graceful_fail( + self, + mock_mkfs, + mock_copy, + mock_execute): + mock_execute.side_effect = processutils.ProcessExecutionError('boom') + self.assertIsNone( + partition_utils._try_build_fat32_config_drive( + self.fake_dev, + self.configdrive_file) + ) + mock_execute.assert_called_once_with( + 'mount', '-o', 'loop,ro', '-t', 'auto', + self.configdrive_file, mock.ANY) + mock_mkfs.assert_not_called() + # Validate we called copy as we expect, both source and destination + # are temporary folders. + mock_copy.assert_not_called() + + @mock.patch.object(shutil, 'copytree', autospec=True) + @mock.patch.object(utils, 'mkfs', autospec=True) + def test__try_build_fat32_config_drive_fails_once_invalid( + self, + mock_mkfs, + mock_copy, + mock_execute): + mock_mkfs.side_effect = processutils.ProcessExecutionError('boom') + self.assertRaisesRegex( + exception.InstanceDeployFailure, + 'A failure occured while attempting to format.*', + partition_utils._try_build_fat32_config_drive, + self.fake_dev, + self.configdrive_file) + mock_execute.assert_has_calls([ + mock.call('mount', '-o', 'loop,ro', '-t', 'auto', + self.configdrive_file, mock.ANY), + mock.call('umount', mock.ANY), + mock.call('umount', mock.ANY), + ]) + + mock_mkfs.assert_called_once_with(fs='vfat', path=self.fake_dev, + label='CONFIG-2') + mock_copy.assert_not_called() diff --git a/ironic_python_agent/tests/unit/test_raid_utils.py b/ironic_python_agent/tests/unit/test_raid_utils.py index a82027c8a..f68796056 100644 --- a/ironic_python_agent/tests/unit/test_raid_utils.py +++ b/ironic_python_agent/tests/unit/test_raid_utils.py @@ -153,6 +153,7 @@ def test_get_volume_name_of_raid_device_invalid(self, mock_execute): volume_name = raid_utils.get_volume_name_of_raid_device('/dev/md0') self.assertIsNone(volume_name) + @mock.patch.object(raid_utils, 'find_esp_raid', autospec=True) @mock.patch.object(disk_utils, 'trigger_device_rescan', autospec=True) @mock.patch.object(raid_utils, 'get_next_free_raid_device', autospec=True, return_value='/dev/md42') @@ -161,7 +162,7 @@ def test_get_volume_name_of_raid_device_invalid(self, mock_execute): @mock.patch.object(disk_utils, 'find_efi_partition', autospec=True) def test_prepare_boot_partitions_for_softraid_uefi_gpt( self, mock_efi_part, mock_execute, mock_dispatch, - mock_free_raid_device, mock_rescan): + mock_free_raid_device, mock_rescan, mock_find_esp): mock_efi_part.return_value = {'number': '12'} mock_execute.side_effect = [ ('451', None), # sgdisk -F @@ -178,6 +179,7 @@ def test_prepare_boot_partitions_for_softraid_uefi_gpt( (None, None), # cp (None, None), # wipefs ] + mock_find_esp.return_value = None efi_part = raid_utils.prepare_boot_partitions_for_softraid( '/dev/md0', ['/dev/sda', '/dev/sdb'], None, @@ -209,6 +211,7 @@ def test_prepare_boot_partitions_for_softraid_uefi_gpt( self.assertEqual(efi_part, '/dev/md42') mock_rescan.assert_called_once_with('/dev/md42') + @mock.patch.object(raid_utils, 'find_esp_raid', autospec=True) @mock.patch.object(disk_utils, 'trigger_device_rescan', autospec=True) @mock.patch.object(raid_utils, 'get_next_free_raid_device', autospec=True, return_value='/dev/md42') @@ -218,7 +221,7 @@ def test_prepare_boot_partitions_for_softraid_uefi_gpt( @mock.patch.object(ilib_utils, 'mkfs', autospec=True) def test_prepare_boot_partitions_for_softraid_uefi_gpt_esp_not_found( self, mock_mkfs, mock_efi_part, mock_execute, mock_dispatch, - mock_free_raid_device, mock_rescan): + mock_free_raid_device, mock_rescan, mock_find_esp): mock_efi_part.return_value = None mock_execute.side_effect = [ ('451', None), # sgdisk -F @@ -233,6 +236,7 @@ def test_prepare_boot_partitions_for_softraid_uefi_gpt_esp_not_found( ('/dev/sdb14: whatever', None), # blkid (None, None), # mdadm ] + mock_find_esp.return_value = None efi_part = raid_utils.prepare_boot_partitions_for_softraid( '/dev/md0', ['/dev/sda', '/dev/sdb'], None, @@ -262,6 +266,7 @@ def test_prepare_boot_partitions_for_softraid_uefi_gpt_esp_not_found( self.assertEqual(efi_part, '/dev/md42') mock_rescan.assert_called_once_with('/dev/md42') + @mock.patch.object(raid_utils, 'find_esp_raid', autospec=True) @mock.patch.object(disk_utils, 'trigger_device_rescan', autospec=True) @mock.patch.object(raid_utils, 'get_next_free_raid_device', autospec=True, return_value='/dev/md42') @@ -269,7 +274,7 @@ def test_prepare_boot_partitions_for_softraid_uefi_gpt_esp_not_found( @mock.patch.object(ilib_utils, 'execute', autospec=True) def test_prepare_boot_partitions_for_softraid_uefi_gpt_efi_provided( self, mock_execute, mock_dispatch, mock_free_raid_device, - mock_rescan): + mock_rescan, mock_find_esp): mock_execute.side_effect = [ ('451', None), # sgdisk -F (None, None), # sgdisk create part @@ -285,6 +290,7 @@ def test_prepare_boot_partitions_for_softraid_uefi_gpt_efi_provided( (None, None), # cp (None, None), # wipefs ] + mock_find_esp.return_value = None efi_part = raid_utils.prepare_boot_partitions_for_softraid( '/dev/md0', ['/dev/sda', '/dev/sdb'], '/dev/md0p15', @@ -389,3 +395,17 @@ def test_no_device(self, mock_dispatch): ] self.assertRaises(errors.SoftwareRAIDError, raid_utils.get_next_free_raid_device) + + +@mock.patch.object(utils, 'execute', autospec=True) +class TestFindESPRAID(base.IronicAgentTest): + + def test_no_esp_raid(self, mock_execute): + mock_execute.side_effect = [(hws.LSBLK_OUPUT, '')] + result = raid_utils.find_esp_raid() + self.assertIsNone(result) + + def test_esp_raid(self, mock_execute): + mock_execute.side_effect = [(hws.LSBLK_OUPUT_ESP_RAID, '')] + result = raid_utils.find_esp_raid() + self.assertEqual('/dev/md125', result) diff --git a/releasenotes/config.yaml b/releasenotes/config.yaml new file mode 100644 index 000000000..3230b6810 --- /dev/null +++ b/releasenotes/config.yaml @@ -0,0 +1,2 @@ +--- +closed_branch_tag_re: 'r"(?!^bugfix.*-eol$).+-eol"' diff --git a/releasenotes/notes/4k-block-size-config-drives-4470828dd06d2600.yaml b/releasenotes/notes/4k-block-size-config-drives-4470828dd06d2600.yaml new file mode 100644 index 000000000..7e65348c6 --- /dev/null +++ b/releasenotes/notes/4k-block-size-config-drives-4470828dd06d2600.yaml @@ -0,0 +1,12 @@ +--- +fixes: + - | + Fixes a failure case where a deployed instance may be unable to access + the configuration drive post-deployment. This can occur when block + devices only support 4KB IO interactions. When 4KB block IO sizes + are in use, the ISO9660 filesystem driver in Linux cannot be used + as it is modeled around a 2KB block. We now attempt to verify, and + rebuild the configuration drive on a FAT filesystem when we cannot + mount the supplied configuration drive. Operators can force the agent + to write configuration drives using the FAT filesystem using the + ``[DEFAULT]config_drive_rebuild`` option. diff --git a/releasenotes/notes/cve-2024-44082-image-security-warning-37ac1ac7647a806a.yaml b/releasenotes/notes/cve-2024-44082-image-security-warning-37ac1ac7647a806a.yaml new file mode 100644 index 000000000..92509277a --- /dev/null +++ b/releasenotes/notes/cve-2024-44082-image-security-warning-37ac1ac7647a806a.yaml @@ -0,0 +1,11 @@ +--- +security: + - | + Ironic-Python-Agent versions prior to the 2023.1 release are vulnerable to + CVE-2024-44082, tracked in + `bug 2071740 _`. Deployers of + Ironic versions Zed or older must apply CVE-2024-44082 fixes to their + Ironic environment and leave (default for all releases Zed and older) + ``[conductor]/conductor_always_validates_images`` set to ``True``. This + ensures the conductor will security check the image because + Ironic-Python-Agent will not. diff --git a/releasenotes/notes/fixes-efibootmgr-character-encoding-19e531ba694824c1.yaml b/releasenotes/notes/fixes-efibootmgr-character-encoding-19e531ba694824c1.yaml new file mode 100644 index 000000000..b4c3270cf --- /dev/null +++ b/releasenotes/notes/fixes-efibootmgr-character-encoding-19e531ba694824c1.yaml @@ -0,0 +1,9 @@ +--- +fixes: + - | + Fixes UEFI NVRAM record handling with efibootmgr so we can accept and + handle UTF-16 encoded data which is to be expected in UEFI NVRAM as + the records are UTF-16 encoded. + - | + Fixes handling of UEFI NVRAM records to allow for unexpected characters + in the response, so it is non-fatal to Ironic. diff --git a/releasenotes/notes/rebuild_on_esp_raid-33f359bdf5ccaa09.yaml b/releasenotes/notes/rebuild_on_esp_raid-33f359bdf5ccaa09.yaml new file mode 100644 index 000000000..09e8bf4d1 --- /dev/null +++ b/releasenotes/notes/rebuild_on_esp_raid-33f359bdf5ccaa09.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Fixes an issue with rebuilding instances on Software RAID with + RAIDed ESP partitions. diff --git a/releasenotes/notes/understand-node-conflict-is-locked-2ea21dd45abfe4f7.yaml b/releasenotes/notes/understand-node-conflict-is-locked-2ea21dd45abfe4f7.yaml new file mode 100644 index 000000000..c0e77964e --- /dev/null +++ b/releasenotes/notes/understand-node-conflict-is-locked-2ea21dd45abfe4f7.yaml @@ -0,0 +1,13 @@ +--- +fixes: + - | + Fixes, or at least lessens the case where a running Ironic agent can stack + up numerous lookup requests against an Ironic deployment when a node is + locked. In particular, this is beause the lookup also drives generation of + the agent token, which requires the conductor to allocate a worker, and + generate the token, and return the result to the API client. + Ironic's retry logic will now wait up to ``60`` seconds, and if an HTTP + Conflict (409) message is received, the agent will automatically pause + lookup operations for thirty seconds as opposed continue to attempt + lookups which could create more work for the Ironic deployment + needlessly. diff --git a/tox.ini b/tox.ini index 8605785dc..b76573f9a 100644 --- a/tox.ini +++ b/tox.ini @@ -1,11 +1,9 @@ [tox] minversion = 3.18.0 -skipsdist = True envlist = py3,functional,pep8 ignore_basepython_conflict=true [testenv] -usedevelop = True basepython = python3 setenv = VIRTUAL_ENV={envdir} @@ -15,11 +13,18 @@ setenv = LANGUAGE=en_US LC_ALL=en_US.utf-8 deps = - -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/master} + -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/zed} -r{toxinidir}/requirements.txt -r{toxinidir}/test-requirements.txt + setuptools>=64.0.0,<82.0.0 # MIT commands = stestr run {posargs} -passenv = http_proxy HTTP_PROXY https_proxy HTTPS_PROXY no_proxy NO_PROXY +passenv = + http_proxy + HTTP_PROXY + https_proxy + HTTPS_PROXY + no_proxy + NO_PROXY [testenv:functional] # Define virtualenv directory, port to use for functional testing, and number @@ -33,8 +38,7 @@ setenv = commands = stestr run {posargs} [testenv:pep8] -usedevelop = False -deps= +deps = hacking>=4.1.0,<5.0.0 # Apache-2.0 bashate>=0.5.1 # Apache-2.0 flake8-import-order>=0.17.1 # LGPLv3 @@ -71,9 +75,10 @@ setenv = PYTHONHASHSEED=0 sitepackages = False # NOTE(dtantsur): documentation building process requires importing IPA deps = - -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/master} + -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/zed} -r{toxinidir}/requirements.txt -r{toxinidir}/doc/requirements.txt + setuptools>=64.0.0,<82.0.0 # MIT commands = sphinx-build -b html doc/source doc/build/html @@ -87,10 +92,10 @@ commands = make -C doc/build/pdf [testenv:releasenotes] -usedevelop = False deps = - -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/master} + -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/zed} -r{toxinidir}/doc/requirements.txt + setuptools>=64.0.0,<82.0.0 # MIT commands = sphinx-build -a -E -W -d releasenotes/build/doctrees -b html releasenotes/source releasenotes/build/html [flake8] @@ -126,10 +131,10 @@ commands = oslo-config-generator --config-file=tools/config/ipa-config-generator.conf [testenv:bandit] -usedevelop = False deps = - -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/master} + -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/zed} -r{toxinidir}/test-requirements.txt + setuptools>=64.0.0,<82.0.0 # MIT commands = bandit -r ironic_python_agent -x tests -n5 -ll -c tools/bandit.yml [testenv:examples] diff --git a/zuul.d/ironic-python-agent-jobs.yaml b/zuul.d/ironic-python-agent-jobs.yaml index 3e06c8c06..3f5c6068a 100644 --- a/zuul.d/ironic-python-agent-jobs.yaml +++ b/zuul.d/ironic-python-agent-jobs.yaml @@ -15,6 +15,8 @@ - ^tox.ini$ required-projects: - openstack/ironic-lib + - name: openstack/ironic-python-agent-builder + override-checkout: zed-eol vars: # The default is 1GB, we need a little more to prevent OOMs killing the jobs configure_swap_size: 8192 @@ -47,7 +49,8 @@ name: ipa-tempest-uefi-redfish-vmedia-src parent: ironic-ipa-base required-projects: - - opendev.org/openstack/sushy-tools + - name: opendev.org/openstack/sushy-tools + override-checkout: 0.21.0 vars: devstack_services: s-account: True @@ -105,7 +108,6 @@ - ^ironic_python_agent/tests/.*$ - ^releasenotes/.*$ - ^setup.cfg$ - - ^tools/(?!bandit.yml).*$ - ^tox.ini$ - job: @@ -121,7 +123,6 @@ - ^ironic_python_agent/tests/.*$ - ^releasenotes/.*$ - ^setup.cfg$ - - ^tools/(?!bandit.yml).*$ - ^tox.ini$ # used by ironic-python-agent-builder @@ -129,6 +130,9 @@ name: ironic-standalone-ipa-src parent: ironic-standalone description: Test ironic standalone with IPA from source + required-projects: + - name: openstack/ironic-python-agent-builder + override-checkout: zed-eol vars: devstack_localrc: IRONIC_BUILD_DEPLOY_RAMDISK: True @@ -141,7 +145,8 @@ parent: metalsmith-integration-glance-centos8-uefi required-projects: - openstack/ironic-python-agent - - openstack/ironic-python-agent-builder + - name: openstack/ironic-python-agent-builder + override-checkout: zed-eol - openstack/ironic-lib vars: devstack_localrc: @@ -154,7 +159,8 @@ parent: metalsmith-integration-glance-centos8-legacy required-projects: - openstack/ironic-python-agent - - openstack/ironic-python-agent-builder + - name: openstack/ironic-python-agent-builder + override-checkout: zed-eol - openstack/ironic-lib vars: devstack_localrc: diff --git a/zuul.d/project.yaml b/zuul.d/project.yaml index a4f9ee05f..f17f28ddb 100644 --- a/zuul.d/project.yaml +++ b/zuul.d/project.yaml @@ -12,10 +12,13 @@ - ipa-tox-examples # NOTE(iurygregory) Only run this two jobs since we are testing # wholedisk + partition on tempest - - ipa-tempest-bios-ipmi-direct-src - - ipa-tempest-uefi-redfish-vmedia-src - - metalsmith-integration-ipa-src-uefi - - metalsmith-integration-ipa-src-legacy + # NOTE(elod.illes) The below two jobs are intermittently failing, + # which makes the gate unstable and the error seems to be environment + # related. Setting them as non-voting to do not waste CI resource. + - ipa-tempest-bios-ipmi-direct-src: + voting: false + - ipa-tempest-uefi-redfish-vmedia-src: + voting: false - ironic-standalone-ipa-src # NOTE(dtantsur): non-voting because IPA source code is very unlikely # to break them. They rather serve as a canary for broken POST jobs. @@ -32,10 +35,6 @@ jobs: - openstack-tox-functional - ipa-tox-examples - - ipa-tempest-bios-ipmi-direct-src - - ipa-tempest-uefi-redfish-vmedia-src - - metalsmith-integration-ipa-src-uefi - - metalsmith-integration-ipa-src-legacy - ironic-standalone-ipa-src post: jobs: