openstack/ironic
Code Issues Proposed changes

Remove isfile check from place_loaders_for_boot

Browse Source 
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
This commit is contained in:
Steve Baker 2021-11-26 09:57:21 +13:00
parent 5d5d3e8c16
commit 44ac507796
2 changed files with 24 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
ironic/common/pxe_utils.py
View File

@ -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:

51
ironic/tests/unit/common/test_pxe_utils.py
View File

@ -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')