Fix directories permission for tftpboot

Currently method "_ensure_config_dirs_exist" creates tftpboot/<uuid>
dir with wrong permission. This is due to the system umask setting
which overrides the default permission of 0777 to 0755 or 0750. When
the permission is 0750, BM can't get deploy_kernel and ramdisk from
tftpserver. This may happen only when tftp process is launched from
other user than root and as result can't read files created by Ironic.

So this patch tries to fix the issue by explicitly changing the
permissions defined in the config option ``[pxe]/dir_permission``.

Change-Id: I3119ec7ae31bf82f716bf082fa4c3296d6aa3587
Closes-bug: #1655568

etc/ironic/ironic.conf.sample

@@ -3309,6 +3309,17 @@
 # caching. (string value)
 #tftp_master_path = /tftpboot/master_images
 
+# The permission that will be applied to the TFTP folders upon
+# creation. This should be set to the permission such that the
+# tftpserver has access to read the contents of the configured
+# TFTP folder. This setting is only required when the
+# operating system's umask is restrictive such that ironic-
+# conductor is creating files that cannot be read by the TFTP
+# server. Setting to <None> will result in the operating
+# system's umask to be utilized for the creation of new tftp
+# folders. (integer value)
+#dir_permission = <None>
+
 # Bootfile DHCP parameter. (string value)
 #pxe_bootfile_name = pxelinux.0
 

ironic/common/pxe_utils.py

@@ -49,8 +49,17 @@ def _ensure_config_dirs_exist(node_uuid):
 
     """
     root_dir = get_root_dir()
-    fileutils.ensure_tree(os.path.join(root_dir, node_uuid))
+    node_dir = os.path.join(root_dir, node_uuid)
-    fileutils.ensure_tree(os.path.join(root_dir, PXE_CFG_DIR_NAME))
+    pxe_dir = os.path.join(root_dir, PXE_CFG_DIR_NAME)
+    # NOTE: We should only change the permissions if the folder
+    # does not exist. i.e. if defined, an operator could have
+    # already created it and placed specific ACLs upon the folder
+    # 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)
 
 
 def _link_mac_pxe_configs(task):

ironic/conf/pxe.py

@@ -77,6 +77,17 @@ opts = [
                help=_('On ironic-conductor node, directory where master TFTP '
                       'images are stored on disk. '
                       'Setting to <None> disables image caching.')),
+    cfg.IntOpt('dir_permission',
+               help=_("The permission that will be applied to the TFTP "
+                      "folders upon creation. This should be set to the "
+                      "permission such that the tftpserver has access to "
+                      "read the contents of the configured TFTP folder. This "
+                      "setting is only required when the operating system's "
+                      "umask is restrictive such that ironic-conductor is "
+                      "creating files that cannot be read by the TFTP server. "
+                      "Setting to <None> will result in the operating "
+                      "system's umask to be utilized for the creation of new "
+                      "tftp folders.")),
     cfg.StrOpt('pxe_bootfile_name',
                default='pxelinux.0',
                help=_('Bootfile DHCP parameter.')),

ironic/tests/unit/common/test_pxe_utils.py

@@ -277,11 +277,12 @@ class TestPXEUtils(db_base.DbTestCase):
         unlink_mock.assert_called_once_with('/tftpboot/10.10.0.1.conf')
         create_link_mock.assert_has_calls(create_link_calls)
 
+    @mock.patch.object(os, 'chmod', 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):
+                               write_mock, chmod_mock):
         with task_manager.acquire(self.context, self.node.uuid) as task:
             pxe_utils.create_pxe_config(task, self.pxe_options,
                                         CONF.pxe.pxe_config_template)
@@ -291,23 +292,84 @@ class TestPXEUtils(db_base.DbTestCase):
                  'ROOT': '{{ ROOT }}',
                  'DISK_IDENTIFIER': '{{ DISK_IDENTIFIER }}'}
             )
+        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(os.path.join(CONF.pxe.tftp_root, self.node.uuid)),
+            mock.call(node_dir), mock.call(pxe_dir),
-            mock.call(os.path.join(CONF.pxe.tftp_root, 'pxelinux.cfg'))
         ]
         ensure_tree_mock.assert_has_calls(ensure_calls)
+        chmod_mock.assert_not_called()
 
         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('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):
+        self.config(dir_permission=0o755, group='pxe')
+        with task_manager.acquire(self.context, self.node.uuid) as task:
+            pxe_utils.create_pxe_config(task, self.pxe_options,
+                                        CONF.pxe.pxe_config_template)
+            render_mock.assert_called_with(
+                CONF.pxe.pxe_config_template,
+                {'pxe_options': self.pxe_options,
+                 'ROOT': '{{ ROOT }}',
+                 'DISK_IDENTIFIER': '{{ DISK_IDENTIFIER }}'}
+            )
+        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)
+
+        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.path, 'isdir', autospec=True)
+    @mock.patch.object(os, 'chmod', 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(self, ensure_tree_mock,
+                                             render_mock,
+                                             write_mock, chmod_mock,
+                                             isdir_mock):
+        self.config(dir_permission=0o755, group='pxe')
+        with task_manager.acquire(self.context, self.node.uuid) as task:
+            isdir_mock.return_value = True
+            pxe_utils.create_pxe_config(task, self.pxe_options,
+                                        CONF.pxe.pxe_config_template)
+            render_mock.assert_called_with(
+                CONF.pxe.pxe_config_template,
+                {'pxe_options': self.pxe_options,
+                 'ROOT': '{{ ROOT }}',
+                 'DISK_IDENTIFIER': '{{ DISK_IDENTIFIER }}'}
+            )
+        ensure_tree_mock.assert_has_calls([])
+        chmod_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('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_elilo(self, ensure_tree_mock, render_mock,
-                                          write_mock, link_ip_configs_mock):
+                                          write_mock, link_ip_configs_mock,
+                                          chmod_mock):
         self.config(
             uefi_pxe_config_template=('ironic/drivers/modules/'
                                       'elilo_efi_pxe_config.template'),
@@ -320,9 +382,10 @@ class TestPXEUtils(db_base.DbTestCase):
 
             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'))
+                mock.call(os.path.join(CONF.pxe.tftp_root, 'pxelinux.cfg')),
             ]
             ensure_tree_mock.assert_has_calls(ensure_calls)
+            chmod_mock.assert_not_called()
             render_mock.assert_called_with(
                 CONF.pxe.uefi_pxe_config_template,
                 {'pxe_options': self.pxe_options,
@@ -334,13 +397,15 @@ 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('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,
-                                         write_mock, link_ip_configs_mock):
+                                         write_mock, link_ip_configs_mock,
+                                         chmod_mock):
         grub_tmplte = "ironic/drivers/modules/pxe_grub_config.template"
         with task_manager.acquire(self.context, self.node.uuid) as task:
             task.node.properties['capabilities'] = 'boot_mode:uefi'
@@ -349,9 +414,10 @@ class TestPXEUtils(db_base.DbTestCase):
 
             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'))
+                mock.call(os.path.join(CONF.pxe.tftp_root, 'pxelinux.cfg')),
             ]
             ensure_tree_mock.assert_has_calls(ensure_calls)
+            chmod_mock.assert_not_called()
             render_mock.assert_called_with(
                 grub_tmplte,
                 {'pxe_options': self.pxe_options,
@@ -363,13 +429,15 @@ 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('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,
-                                         write_mock, link_mac_pxe_mock):
+                                         write_mock, link_mac_pxe_mock,
+                                         chmod_mock):
         self.config(ipxe_enabled=True, group='pxe')
         ipxe_template = "ironic/drivers/modules/ipxe_config.template"
         with task_manager.acquire(self.context, self.node.uuid) as task:
@@ -379,9 +447,10 @@ class TestPXEUtils(db_base.DbTestCase):
 
             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'))
+                mock.call(os.path.join(CONF.deploy.http_root, 'pxelinux.cfg')),
             ]
             ensure_tree_mock.assert_has_calls(ensure_calls)
+            chmod_mock.assert_not_called()
             render_mock.assert_called_with(
                 ipxe_template,
                 {'pxe_options': self.ipxe_options,

releasenotes/notes/fix-dir-permissions-bc56e83a651bbdb0.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - Adds the capability for an operator to explicitly define the permission
+    for created tftpboot folders. This provides the ability for ironic to be
+    utilized with a restrictive umask, where the tftp server may not be able
+    to read the file. Introduced a new config option ``[pxe]/dir_permission``
+    to specify the permission for the tftpboot directories to be created with.