From f97e9340ec9b1a69b35d045481351ed18aa5438e Mon Sep 17 00:00:00 2001 From: Steve Baker Date: Thu, 16 Sep 2021 10:21:19 +1200 Subject: [PATCH] Add missing mode setting on pxe created directories Two calls to fileutils.ensure_tree were missing the follow-up chmod for CONF.pxe.dir_permission. This is fixed with a local ensure_tree function which creates the directory with the appropriate mode. The configuration help for the permissions attributes clarify that the system default umask is masked out on the supplied value. Change-Id: I124d2ba09b0fc78b284c1ae871ca5a16fc44b8c9 --- ironic/common/pxe_utils.py | 18 ++-- ironic/conf/pxe.py | 7 +- ironic/tests/unit/common/test_pxe_utils.py | 104 ++++++++++----------- 3 files changed, 61 insertions(+), 68 deletions(-) diff --git a/ironic/common/pxe_utils.py b/ironic/common/pxe_utils.py index 00de4c723f..02c9e95528 100644 --- a/ironic/common/pxe_utils.py +++ b/ironic/common/pxe_utils.py @@ -24,7 +24,6 @@ import jinja2 from oslo_concurrency import processutils from oslo_log import log as logging from oslo_utils import excutils -from oslo_utils import fileutils from ironic.common import dhcp_factory from ironic.common import exception @@ -72,6 +71,11 @@ def _get_root_dir(ipxe_enabled): return CONF.pxe.tftp_root +def ensure_tree(path): + mode = CONF.pxe.dir_permission or 0o755 + os.makedirs(path, mode=mode, exist_ok=True) + + def _ensure_config_dirs_exist(task, ipxe_enabled=False): """Ensure that the node's and PXE configuration directories exist. @@ -88,9 +92,7 @@ def _ensure_config_dirs_exist(task, ipxe_enabled=False): # which may not recurse downward. for directory in (node_dir, pxe_dir): if not os.path.isdir(directory): - fileutils.ensure_tree(directory) - if CONF.pxe.dir_permission: - os.chmod(directory, CONF.pxe.dir_permission) + ensure_tree(directory) def _link_mac_pxe_configs(task, ipxe_enabled=False): @@ -1190,12 +1192,12 @@ def cache_ramdisk_kernel(task, pxe_info, ipxe_enabled=False): path = os.path.join(CONF.deploy.http_root, node.uuid) else: path = os.path.join(CONF.pxe.tftp_root, node.uuid) - fileutils.ensure_tree(path) + ensure_tree(path) # anconda deploy will have 'stage2' as one of the labels in pxe_info dict if 'stage2' in pxe_info.keys(): # stage2 will be stored in ipxe http directory. So make sure they # exist. - fileutils.ensure_tree( + ensure_tree( get_file_path_from_label( node.uuid, CONF.deploy.http_root, @@ -1258,9 +1260,7 @@ def place_loaders_for_boot(base_path): raise exception.IncorrectConfiguration(msg) else: try: - os.makedirs( - os.path.join(base_path, head), - mode=CONF.pxe.dir_permission or 0o755, exist_ok=True) + ensure_tree(os.path.join(base_path, head)) except OSError as e: msg = ('Failed to create supplied directories in ' 'asset copy paths. Error: %s') % e diff --git a/ironic/conf/pxe.py b/ironic/conf/pxe.py index 08c554f870..626c5ef6bd 100644 --- a/ironic/conf/pxe.py +++ b/ironic/conf/pxe.py @@ -101,14 +101,15 @@ opts = [ "creating files that cannot be read by the TFTP server. " "Setting to will result in the operating " "system's umask to be utilized for the creation of new " - "tftp folders. It is required that an octal " + "tftp folders. The system default umask is masked out " + "on the specified value. It is required that an octal " "representation is specified. For example: 0o755")), cfg.IntOpt('file_permission', default=0o644, help=_('The permission which is used on files created as part ' 'of configuration and setup of file assets for PXE ' - 'based operations. Defaults to a value of 0o644.' - 'This value must be specified as an octal ' + 'based operations. Defaults to a value of ' + '0o644. This value must be specified as an octal ' 'representation. For example: 0o644')), cfg.StrOpt('pxe_bootfile_name', default='pxelinux.0', diff --git a/ironic/tests/unit/common/test_pxe_utils.py b/ironic/tests/unit/common/test_pxe_utils.py index f22f536ab9..161b85932b 100644 --- a/ironic/tests/unit/common/test_pxe_utils.py +++ b/ironic/tests/unit/common/test_pxe_utils.py @@ -451,12 +451,12 @@ class TestPXEUtils(db_base.DbTestCase): @mock.patch.object(pxe_utils, '_link_ip_address_pxe_configs', autospec=True) - @mock.patch.object(os, 'chmod', autospec=True) + @mock.patch.object(os, 'makedirs', autospec=True) @mock.patch('ironic.common.utils.write_to_file', autospec=True) @mock.patch('ironic.common.utils.render_template', autospec=True) @mock.patch('oslo_utils.fileutils.ensure_tree', autospec=True) def test_create_pxe_config(self, ensure_tree_mock, render_mock, - write_mock, chmod_mock, mock_link_ip_addr): + write_mock, makedirs_mock, mock_link_ip_addr): self.config(tftp_root=tempfile.mkdtemp(), group='pxe') with task_manager.acquire(self.context, self.node.uuid) as task: pxe_utils.create_pxe_config(task, self.pxe_options, @@ -469,11 +469,10 @@ class TestPXEUtils(db_base.DbTestCase): ) node_dir = os.path.join(CONF.pxe.tftp_root, self.node.uuid) pxe_dir = os.path.join(CONF.pxe.tftp_root, 'pxelinux.cfg') - ensure_calls = [ - mock.call(node_dir), mock.call(pxe_dir), - ] - ensure_tree_mock.assert_has_calls(ensure_calls) - chmod_mock.assert_not_called() + makedirs_mock.assert_has_calls([ + mock.call(node_dir, 0o755, exist_ok=True), + mock.call(pxe_dir, 0o755, exist_ok=True), + ]) pxe_cfg_file_path = pxe_utils.get_pxe_config_file_path(self.node.uuid) write_mock.assert_called_with(pxe_cfg_file_path, @@ -482,13 +481,13 @@ class TestPXEUtils(db_base.DbTestCase): @mock.patch.object(pxe_utils, '_link_ip_address_pxe_configs', autospec=True) - @mock.patch.object(os, 'chmod', autospec=True) + @mock.patch.object(os, 'makedirs', autospec=True) @mock.patch('ironic.common.utils.write_to_file', autospec=True) @mock.patch('ironic.common.utils.render_template', autospec=True) @mock.patch('oslo_utils.fileutils.ensure_tree', autospec=True) def test_create_pxe_config_set_dir_permission(self, ensure_tree_mock, render_mock, - write_mock, chmod_mock, + write_mock, makedirs_mock, mock_link_ip_addr): self.config(tftp_root=tempfile.mkdtemp(), group='pxe') self.config(dir_permission=0o755, group='pxe') @@ -503,12 +502,10 @@ class TestPXEUtils(db_base.DbTestCase): ) node_dir = os.path.join(CONF.pxe.tftp_root, self.node.uuid) pxe_dir = os.path.join(CONF.pxe.tftp_root, 'pxelinux.cfg') - ensure_calls = [ - mock.call(node_dir), mock.call(pxe_dir), - ] - ensure_tree_mock.assert_has_calls(ensure_calls) - chmod_calls = [mock.call(node_dir, 0o755), mock.call(pxe_dir, 0o755)] - chmod_mock.assert_has_calls(chmod_calls) + makedirs_mock.assert_has_calls([ + mock.call(node_dir, 0o755, exist_ok=True), + mock.call(pxe_dir, 0o755, exist_ok=True), + ]) pxe_cfg_file_path = pxe_utils.get_pxe_config_file_path(self.node.uuid) write_mock.assert_called_with(pxe_cfg_file_path, @@ -518,14 +515,14 @@ class TestPXEUtils(db_base.DbTestCase): @mock.patch.object(pxe_utils, '_link_ip_address_pxe_configs', autospec=True) @mock.patch.object(os.path, 'isdir', autospec=True) - @mock.patch.object(os, 'chmod', autospec=True) + @mock.patch.object(os, 'makedirs', autospec=True) @mock.patch('ironic.common.utils.write_to_file', autospec=True) @mock.patch('ironic.common.utils.render_template', autospec=True) @mock.patch('oslo_utils.fileutils.ensure_tree', autospec=True) def test_create_pxe_config_existing_dirs_uefi( self, ensure_tree_mock, render_mock, - write_mock, chmod_mock, + write_mock, makedirs_mock, isdir_mock, mock_link_ip_address): self.config(dir_permission=0o755, group='pxe') with task_manager.acquire(self.context, self.node.uuid) as task: @@ -539,7 +536,7 @@ class TestPXEUtils(db_base.DbTestCase): 'DISK_IDENTIFIER': '(( DISK_IDENTIFIER ))'} ) ensure_tree_mock.assert_has_calls([]) - chmod_mock.assert_not_called() + makedirs_mock.assert_not_called() isdir_mock.assert_has_calls([]) pxe_cfg_file_path = pxe_utils.get_pxe_config_file_path(self.node.uuid) write_mock.assert_called_with(pxe_cfg_file_path, @@ -547,14 +544,14 @@ class TestPXEUtils(db_base.DbTestCase): self.assertTrue(mock_link_ip_address.called) @mock.patch.object(os.path, 'isdir', autospec=True) - @mock.patch.object(os, 'chmod', autospec=True) + @mock.patch.object(os, 'makedirs', autospec=True) @mock.patch('ironic.common.utils.write_to_file', autospec=True) @mock.patch('ironic.common.utils.render_template', autospec=True) @mock.patch('oslo_utils.fileutils.ensure_tree', autospec=True) def test_create_pxe_config_existing_dirs_bios( self, ensure_tree_mock, render_mock, - write_mock, chmod_mock, + write_mock, makedirs_mock, isdir_mock): self.config(dir_permission=0o755, group='pxe') self.config(default_boot_mode='bios', group='deploy') @@ -568,22 +565,20 @@ class TestPXEUtils(db_base.DbTestCase): 'ROOT': '{{ ROOT }}', 'DISK_IDENTIFIER': '{{ DISK_IDENTIFIER }}'} ) - ensure_tree_mock.assert_has_calls([]) - chmod_mock.assert_not_called() + makedirs_mock.assert_not_called() isdir_mock.assert_has_calls([]) pxe_cfg_file_path = pxe_utils.get_pxe_config_file_path(self.node.uuid) write_mock.assert_called_with(pxe_cfg_file_path, render_mock.return_value) - @mock.patch.object(os, 'chmod', autospec=True) + @mock.patch.object(os, 'makedirs', autospec=True) @mock.patch('ironic.common.pxe_utils._link_ip_address_pxe_configs', autospec=True) @mock.patch('ironic.common.utils.write_to_file', autospec=True) @mock.patch('ironic.common.utils.render_template', autospec=True) - @mock.patch('oslo_utils.fileutils.ensure_tree', autospec=True) - def test_create_pxe_config_uefi_grub(self, ensure_tree_mock, render_mock, + def test_create_pxe_config_uefi_grub(self, render_mock, write_mock, link_ip_configs_mock, - chmod_mock): + makedirs_mock): self.config(tftp_root=tempfile.mkdtemp(), group='pxe') grub_tmplte = "ironic/drivers/modules/pxe_grub_config.template" with task_manager.acquire(self.context, self.node.uuid) as task: @@ -591,12 +586,12 @@ class TestPXEUtils(db_base.DbTestCase): pxe_utils.create_pxe_config(task, self.pxe_options, grub_tmplte) - ensure_calls = [ - mock.call(os.path.join(CONF.pxe.tftp_root, self.node.uuid)), - mock.call(os.path.join(CONF.pxe.tftp_root, 'pxelinux.cfg')), - ] - ensure_tree_mock.assert_has_calls(ensure_calls) - chmod_mock.assert_not_called() + makedirs_mock.assert_has_calls([ + mock.call(os.path.join(CONF.pxe.tftp_root, self.node.uuid), + 0o755, exist_ok=True), + mock.call(os.path.join(CONF.pxe.tftp_root, 'pxelinux.cfg'), + 0o755, exist_ok=True), + ]) render_mock.assert_called_with( grub_tmplte, {'pxe_options': self.pxe_options, @@ -608,18 +603,16 @@ class TestPXEUtils(db_base.DbTestCase): write_mock.assert_called_with(pxe_cfg_file_path, render_mock.return_value) - @mock.patch.object(os, 'chmod', autospec=True) + @mock.patch.object(os, 'makedirs', autospec=True) @mock.patch('ironic.common.pxe_utils._link_mac_pxe_configs', autospec=True) @mock.patch('ironic.common.pxe_utils._link_ip_address_pxe_configs', autospec=True) @mock.patch('ironic.common.utils.write_to_file', autospec=True) @mock.patch('ironic.common.utils.render_template', autospec=True) - @mock.patch('oslo_utils.fileutils.ensure_tree', autospec=True) def test_create_pxe_config_uefi_mac_address( - self, ensure_tree_mock, render_mock, - write_mock, link_ip_configs_mock, - link_mac_pxe_configs_mock, chmod_mock): + self, render_mock, write_mock, link_ip_configs_mock, + link_mac_pxe_configs_mock, makedirs_mock): # TODO(TheJulia): We should... like... fix the template to # enable mac address usage..... grub_tmplte = "ironic/drivers/modules/pxe_grub_config.template" @@ -632,12 +625,12 @@ class TestPXEUtils(db_base.DbTestCase): pxe_utils.create_pxe_config(task, self.pxe_options, grub_tmplte) - ensure_calls = [ - mock.call(os.path.join(CONF.pxe.tftp_root, self.node.uuid)), - mock.call(os.path.join(CONF.pxe.tftp_root, 'pxelinux.cfg')), - ] - ensure_tree_mock.assert_has_calls(ensure_calls) - chmod_mock.assert_not_called() + makedirs_mock.assert_has_calls([ + mock.call(os.path.join(CONF.pxe.tftp_root, self.node.uuid), + 0o755, exist_ok=True), + mock.call(os.path.join(CONF.pxe.tftp_root, 'pxelinux.cfg'), + 0o755, exist_ok=True), + ]) render_mock.assert_called_with( grub_tmplte, {'pxe_options': self.pxe_options, @@ -651,14 +644,13 @@ class TestPXEUtils(db_base.DbTestCase): write_mock.assert_called_with(pxe_cfg_file_path, render_mock.return_value) - @mock.patch.object(os, 'chmod', autospec=True) + @mock.patch.object(os, 'makedirs', autospec=True) @mock.patch('ironic.common.pxe_utils._link_mac_pxe_configs', autospec=True) @mock.patch('ironic.common.utils.write_to_file', autospec=True) @mock.patch('ironic.common.utils.render_template', autospec=True) - @mock.patch('oslo_utils.fileutils.ensure_tree', autospec=True) - def test_create_pxe_config_uefi_ipxe(self, ensure_tree_mock, render_mock, + def test_create_pxe_config_uefi_ipxe(self, render_mock, write_mock, link_mac_pxe_mock, - chmod_mock): + makedirs_mock): self.config(http_root=tempfile.mkdtemp(), group='deploy') ipxe_template = "ironic/drivers/modules/ipxe_config.template" @@ -667,12 +659,12 @@ class TestPXEUtils(db_base.DbTestCase): pxe_utils.create_pxe_config(task, self.ipxe_options, ipxe_template, ipxe_enabled=True) - ensure_calls = [ - mock.call(os.path.join(CONF.deploy.http_root, self.node.uuid)), - mock.call(os.path.join(CONF.deploy.http_root, 'pxelinux.cfg')), - ] - ensure_tree_mock.assert_has_calls(ensure_calls) - chmod_mock.assert_not_called() + makedirs_mock.assert_has_calls([ + mock.call(os.path.join(CONF.deploy.http_root, self.node.uuid), + 0o755, exist_ok=True), + mock.call(os.path.join(CONF.deploy.http_root, 'pxelinux.cfg'), + 0o755, exist_ok=True), + ]) render_mock.assert_called_with( ipxe_template, {'pxe_options': self.ipxe_options, @@ -1335,7 +1327,7 @@ class PXEInterfacesTestCase(db_base.DbTestCase): image_path = os.path.join(temp_dir, self.node.uuid, 'deploy_kernel') image_info = {'deploy_kernel': ('deploy_kernel', image_path)} - fileutils.ensure_tree(CONF.pxe.tftp_master_path) + pxe_utils.ensure_tree(CONF.pxe.tftp_master_path) with task_manager.acquire(self.context, self.node.uuid, shared=True) as task: pxe_utils.cache_ramdisk_kernel(task, image_info) @@ -1347,7 +1339,7 @@ class PXEInterfacesTestCase(db_base.DbTestCase): True) @mock.patch.object(pxe_utils, 'TFTPImageCache', lambda: None) - @mock.patch.object(fileutils, 'ensure_tree', autospec=True) + @mock.patch.object(pxe_utils, 'ensure_tree', autospec=True) @mock.patch.object(deploy_utils, 'fetch_images', autospec=True) def test_cache_ramdisk_kernel(self, mock_fetch_image, mock_ensure_tree): fake_pxe_info = {'foo': 'bar'} @@ -1360,7 +1352,7 @@ class PXEInterfacesTestCase(db_base.DbTestCase): self.context, mock.ANY, list(fake_pxe_info.values()), True) @mock.patch.object(pxe_utils, 'TFTPImageCache', lambda: None) - @mock.patch.object(fileutils, 'ensure_tree', autospec=True) + @mock.patch.object(pxe_utils, 'ensure_tree', autospec=True) @mock.patch.object(deploy_utils, 'fetch_images', autospec=True) def test_cache_ramdisk_kernel_ipxe(self, mock_fetch_image, mock_ensure_tree):