From 44ac5077969daec99324a690b180c33293aa8899 Mon Sep 17 00:00:00 2001 From: Steve Baker Date: Fri, 26 Nov 2021 09:57:21 +1300 Subject: [PATCH] Remove isfile check from place_loaders_for_boot isfile will return False for reasons other than the file being missing (such as a permission denied due to /boot/efi being only root readable). Having the isfile check prevents the actual reason from being logged. This change removes the isfile check so that the error can be triggered and logged in the copy2 call. Change-Id: I1d855a7d7c97cabb83cdb3efc284a00bd65d4d5f --- ironic/common/pxe_utils.py | 7 --- ironic/tests/unit/common/test_pxe_utils.py | 51 ++++++++++------------ 2 files changed, 24 insertions(+), 34 deletions(-) diff --git a/ironic/common/pxe_utils.py b/ironic/common/pxe_utils.py index a87de63d5e..5ae5b3ae14 100644 --- a/ironic/common/pxe_utils.py +++ b/ironic/common/pxe_utils.py @@ -1266,13 +1266,6 @@ def place_loaders_for_boot(base_path): raise exception.IncorrectConfiguration(msg) full_dest = os.path.join(base_path, dest) - if not os.path.isfile(src): - msg = ('Was not able to find source path %(src)s ' - 'to copy to %(dest)s.' % - {'src': src, 'dest': full_dest}) - LOG.error(msg) - raise exception.IncorrectConfiguration(error=msg) - LOG.debug('Copying bootloader %(dest)s from %(src)s.', {'src': src, 'dest': full_dest}) try: diff --git a/ironic/tests/unit/common/test_pxe_utils.py b/ironic/tests/unit/common/test_pxe_utils.py index ea0ef4c092..81b065db25 100644 --- a/ironic/tests/unit/common/test_pxe_utils.py +++ b/ironic/tests/unit/common/test_pxe_utils.py @@ -2184,7 +2184,6 @@ class TFTPImageCacheTestCase(db_base.DbTestCase): @mock.patch.object(os, 'makedirs', autospec=True) -@mock.patch.object(os.path, 'isfile', autospec=True) @mock.patch.object(os, 'chmod', autospec=True) @mock.patch.object(shutil, 'copy2', autospec=True) class TestPXEUtilsBootloader(db_base.DbTestCase): @@ -2193,43 +2192,34 @@ class TestPXEUtilsBootloader(db_base.DbTestCase): super(TestPXEUtilsBootloader, self).setUp() def test_place_loaders_for_boot_default_noop(self, mock_copy2, - mock_chmod, mock_isfile, - mock_makedirs): + mock_chmod, mock_makedirs): res = pxe_utils.place_loaders_for_boot('/httpboot') self.assertIsNone(res) self.assertFalse(mock_copy2.called) self.assertFalse(mock_chmod.called) - self.assertFalse(mock_isfile.called) self.assertFalse(mock_makedirs.called) self.assertFalse(mock_makedirs.called) def test_place_loaders_for_boot_no_source(self, mock_copy2, - mock_chmod, mock_isfile, - mock_makedirs): + mock_chmod, mock_makedirs): self.config(loader_file_paths='grubaa64.efi:/path/to/file', group='pxe') - mock_isfile.return_value = False + mock_copy2.side_effect = FileNotFoundError('No such file or directory') self.assertRaises(exception.IncorrectConfiguration, pxe_utils.place_loaders_for_boot, '/tftpboot') - self.assertFalse(mock_copy2.called) + self.assertTrue(mock_copy2.called) self.assertFalse(mock_chmod.called) self.assertFalse(mock_makedirs.called) def test_place_loaders_for_boot_two_files(self, mock_copy2, - mock_chmod, mock_isfile, - mock_makedirs): + mock_chmod, mock_makedirs): self.config(loader_file_paths='bootx64.efi:/path/to/shimx64.efi,' 'grubx64.efi:/path/to/grubx64.efi', group='pxe') self.config(file_permission=420, group='pxe') - mock_isfile.return_value = True res = pxe_utils.place_loaders_for_boot('/tftpboot') self.assertIsNone(res) - mock_isfile.assert_has_calls([ - mock.call('/path/to/shimx64.efi'), - mock.call('/path/to/grubx64.efi'), - ]) mock_copy2.assert_has_calls([ mock.call('/path/to/shimx64.efi', '/tftpboot/bootx64.efi'), mock.call('/path/to/grubx64.efi', '/tftpboot/grubx64.efi') @@ -2241,19 +2231,31 @@ class TestPXEUtilsBootloader(db_base.DbTestCase): self.assertFalse(mock_makedirs.called) def test_place_loaders_for_boot_two_files_exception_on_copy( - self, mock_copy2, mock_chmod, mock_isfile, mock_makedirs): + self, mock_copy2, mock_chmod, mock_makedirs): + self.config(loader_file_paths='bootx64.efi:/path/to/shimx64.efi,' + 'grubx64.efi:/path/to/grubx64.efi', + group='pxe') + self.config(file_permission=420, group='pxe') + mock_copy2.side_effect = PermissionError('Permission denied') + self.assertRaises(exception.IncorrectConfiguration, + pxe_utils.place_loaders_for_boot, + '/tftpboot') + mock_copy2.assert_has_calls([ + mock.call('/path/to/shimx64.efi', '/tftpboot/bootx64.efi'), + ]) + mock_chmod.assert_not_called() + self.assertFalse(mock_makedirs.called) + + def test_place_loaders_for_boot_two_files_exception_on_chmod( + self, mock_copy2, mock_chmod, mock_makedirs): self.config(loader_file_paths='bootx64.efi:/path/to/shimx64.efi,' 'grubx64.efi:/path/to/grubx64.efi', group='pxe') self.config(file_permission=420, group='pxe') - mock_isfile.side_effect = iter([True, False, True, False]) mock_chmod.side_effect = OSError('Chmod not permitted') self.assertRaises(exception.IncorrectConfiguration, pxe_utils.place_loaders_for_boot, '/tftpboot') - mock_isfile.assert_has_calls([ - mock.call('/path/to/shimx64.efi'), - ]) mock_copy2.assert_has_calls([ mock.call('/path/to/shimx64.efi', '/tftpboot/bootx64.efi'), ]) @@ -2263,7 +2265,7 @@ class TestPXEUtilsBootloader(db_base.DbTestCase): self.assertFalse(mock_makedirs.called) def test_place_loaders_for_boot_raises_exception_with_absolute_path( - self, mock_copy2, mock_chmod, mock_isfile, mock_makedirs): + self, mock_copy2, mock_chmod, mock_makedirs): self.config( loader_file_paths='/tftpboot/bootx64.efi:/path/to/shimx64.efi', group='pxe') @@ -2275,19 +2277,14 @@ class TestPXEUtilsBootloader(db_base.DbTestCase): '/tftpboot/bootx64.efi', str(exc)) def test_place_loaders_for_boot_two_files_relative_path( - self, mock_copy2, mock_chmod, mock_isfile, mock_makedirs): + self, mock_copy2, mock_chmod, mock_makedirs): self.config(loader_file_paths='grub/bootx64.efi:/path/to/shimx64.efi,' 'grub/grubx64.efi:/path/to/grubx64.efi', group='pxe') self.config(dir_permission=484, group='pxe') self.config(file_permission=420, group='pxe') - mock_isfile.return_value = True res = pxe_utils.place_loaders_for_boot('/tftpboot') self.assertIsNone(res) - mock_isfile.assert_has_calls([ - mock.call('/path/to/shimx64.efi'), - mock.call('/path/to/grubx64.efi'), - ]) mock_copy2.assert_has_calls([ mock.call('/path/to/shimx64.efi', '/tftpboot/grub/bootx64.efi'), mock.call('/path/to/grubx64.efi', '/tftpboot/grub/grubx64.efi')