From 68bd8ad8a5ac95969b8d8ef1afcfa56e37d68877 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Tue, 10 Sep 2024 11:43:31 +0200 Subject: [PATCH] Remove the rest of the rootwrap machinery Change-Id: Ia724d933bbf69fb78a003947a92e952ab1e0e44e --- etc/ironic/rootwrap.d/ironic-lib.filters | 27 +---------- ironic_lib/tests/test_utils.py | 58 ++++++------------------ ironic_lib/utils.py | 35 +++++--------- 3 files changed, 25 insertions(+), 95 deletions(-) diff --git a/etc/ironic/rootwrap.d/ironic-lib.filters b/etc/ironic/rootwrap.d/ironic-lib.filters index b03a9f8b..899c7fea 100644 --- a/etc/ironic/rootwrap.d/ironic-lib.filters +++ b/etc/ironic/rootwrap.d/ironic-lib.filters @@ -1,30 +1,5 @@ # An ironic-lib.filters to be used with rootwrap command. -# The following commands should be used in filters for disk manipulation. # This file should be owned by (and only-writeable by) the root user. -# NOTE: -# if you update this file, you will also need to adjust the -# ironic-lib.filters from the ironic module. - [Filters] -# ironic_lib/disk_utils.py -blkid: CommandFilter, blkid, root -blockdev: CommandFilter, blockdev, root -hexdump: CommandFilter, hexdump, root -lsblk: CommandFilter, lsblk, root -wipefs: CommandFilter, wipefs, root -sgdisk: CommandFilter, sgdisk, root -partprobe: CommandFilter, partprobe, root - -# ironic_lib/utils.py -mkswap: CommandFilter, mkswap, root -mkfs: CommandFilter, mkfs, root -dd: CommandFilter, dd, root -mount: CommandFilter, mount, root - -# ironic_lib/disk_partitioner.py -fuser: CommandFilter, fuser, root -parted: CommandFilter, parted, root - -# ironic_lib/qemu_img.py -qemu-img: CommandFilter, qemu-img, root +# Left for backward compatibility only, do not use. diff --git a/ironic_lib/tests/test_utils.py b/ironic_lib/tests/test_utils.py index d520d1bc..d03f383b 100644 --- a/ironic_lib/tests/test_utils.py +++ b/ironic_lib/tests/test_utils.py @@ -72,34 +72,6 @@ class ExecuteTestCase(base.IronicLibTestCase): execute_mock.assert_called_once_with('foo', env_variables={'foo': 'bar'}) - def test_execute_without_root_helper(self): - CONF.set_override('root_helper', None, group='ironic_lib') - with mock.patch.object( - processutils, 'execute', autospec=True) as execute_mock: - utils.execute('foo', run_as_root=False) - execute_mock.assert_called_once_with('foo', run_as_root=False) - - def test_execute_without_root_helper_run_as_root(self): - CONF.set_override('root_helper', None, group='ironic_lib') - with mock.patch.object( - processutils, 'execute', autospec=True) as execute_mock: - utils.execute('foo', run_as_root=True) - execute_mock.assert_called_once_with('foo', run_as_root=False) - - def test_execute_with_root_helper(self): - with mock.patch.object( - processutils, 'execute', autospec=True) as execute_mock: - utils.execute('foo', run_as_root=False) - execute_mock.assert_called_once_with('foo', run_as_root=False) - - def test_execute_with_root_helper_run_as_root(self): - with mock.patch.object( - processutils, 'execute', autospec=True) as execute_mock: - utils.execute('foo', run_as_root=True) - execute_mock.assert_called_once_with( - 'foo', run_as_root=True, - root_helper=CONF.ironic_lib.root_helper) - @mock.patch.object(utils, 'LOG', autospec=True) def _test_execute_with_log_stdout(self, log_mock, log_stdout=None): with mock.patch.object( @@ -147,13 +119,10 @@ class MkfsTestCase(base.IronicLibTestCase): utils.mkfs('swap', '/my/swap/block/dev') expected = [mock.call('mkfs', '-t', 'ext4', '-F', '/my/block/dev', - run_as_root=True, use_standard_locale=True), mock.call('mkfs', '-t', 'msdos', '/my/msdos/block/dev', - run_as_root=True, use_standard_locale=True), mock.call('mkswap', '/my/swap/block/dev', - run_as_root=True, use_standard_locale=True)] self.assertEqual(expected, execute_mock.call_args_list) @@ -164,13 +133,13 @@ class MkfsTestCase(base.IronicLibTestCase): utils.mkfs('swap', '/my/swap/block/dev', 'swap-vol') expected = [mock.call('mkfs', '-t', 'ext4', '-F', '-L', 'ext4-vol', - '/my/block/dev', run_as_root=True, + '/my/block/dev', use_standard_locale=True), mock.call('mkfs', '-t', 'msdos', '-n', 'msdos-vol', - '/my/msdos/block/dev', run_as_root=True, + '/my/msdos/block/dev', use_standard_locale=True), mock.call('mkswap', '-L', 'swap-vol', - '/my/swap/block/dev', run_as_root=True, + '/my/swap/block/dev', use_standard_locale=True)] self.assertEqual(expected, execute_mock.call_args_list) @@ -548,8 +517,8 @@ class MountedTestCase(base.IronicLibTestCase): self.assertIs(path, mock_temp.return_value) mock_execute.assert_has_calls([ mock.call("mount", '/dev/fake', mock_temp.return_value, - run_as_root=True, attempts=1, delay_on_retry=True), - mock.call("umount", mock_temp.return_value, run_as_root=True, + attempts=1, delay_on_retry=True), + mock.call("umount", mock_temp.return_value, attempts=3, delay_on_retry=True), ]) mock_rmtree.assert_called_once_with(mock_temp.return_value) @@ -558,9 +527,9 @@ class MountedTestCase(base.IronicLibTestCase): with utils.mounted('/dev/fake', '/mnt/fake') as path: self.assertEqual('/mnt/fake', path) mock_execute.assert_has_calls([ - mock.call("mount", '/dev/fake', '/mnt/fake', run_as_root=True, + mock.call("mount", '/dev/fake', '/mnt/fake', attempts=1, delay_on_retry=True), - mock.call("umount", '/mnt/fake', run_as_root=True, + mock.call("umount", '/mnt/fake', attempts=3, delay_on_retry=True), ]) self.assertFalse(mock_temp.called) @@ -572,8 +541,8 @@ class MountedTestCase(base.IronicLibTestCase): self.assertEqual('/mnt/fake', path) mock_execute.assert_has_calls([ mock.call("mount", '/dev/fake', '/mnt/fake', '-o', 'ro,foo=bar', - run_as_root=True, attempts=1, delay_on_retry=True), - mock.call("umount", '/mnt/fake', run_as_root=True, + attempts=1, delay_on_retry=True), + mock.call("umount", '/mnt/fake', attempts=3, delay_on_retry=True), ]) @@ -583,8 +552,8 @@ class MountedTestCase(base.IronicLibTestCase): self.assertEqual('/mnt/fake', path) mock_execute.assert_has_calls([ mock.call("mount", '/dev/fake', '/mnt/fake', '-t', 'iso9660', - run_as_root=True, attempts=1, delay_on_retry=True), - mock.call("umount", '/mnt/fake', run_as_root=True, + attempts=1, delay_on_retry=True), + mock.call("umount", '/mnt/fake', attempts=3, delay_on_retry=True), ]) @@ -593,7 +562,6 @@ class MountedTestCase(base.IronicLibTestCase): self.assertRaises(OSError, utils.mounted('/dev/fake').__enter__) mock_execute.assert_called_once_with("mount", '/dev/fake', mock_temp.return_value, - run_as_root=True, attempts=1, delay_on_retry=True) mock_rmtree.assert_called_once_with(mock_temp.return_value) @@ -604,9 +572,9 @@ class MountedTestCase(base.IronicLibTestCase): with utils.mounted('/dev/fake', '/mnt/fake') as path: self.assertEqual('/mnt/fake', path) mock_execute.assert_has_calls([ - mock.call("mount", '/dev/fake', '/mnt/fake', run_as_root=True, + mock.call("mount", '/dev/fake', '/mnt/fake', attempts=1, delay_on_retry=True), - mock.call("umount", '/mnt/fake', run_as_root=True, + mock.call("umount", '/mnt/fake', attempts=3, delay_on_retry=True), ]) self.assertFalse(mock_rmtree.called) diff --git a/ironic_lib/utils.py b/ironic_lib/utils.py index 9437811a..e9175d86 100644 --- a/ironic_lib/utils.py +++ b/ironic_lib/utils.py @@ -29,9 +29,9 @@ import shlex import shutil import tempfile from urllib import parse as urlparse +import warnings from oslo_concurrency import processutils -from oslo_config import cfg from oslo_utils import excutils from oslo_utils import specs_matcher from oslo_utils import strutils @@ -40,15 +40,6 @@ from oslo_utils import units from ironic_lib.common.i18n import _ from ironic_lib import exception -utils_opts = [ - cfg.StrOpt('root_helper', - default='sudo ironic-rootwrap /etc/ironic/rootwrap.conf', - help='Command that is prefixed to commands that are run as ' - 'root. If not specified, no commands are run as root.'), -] - -CONF = cfg.CONF -CONF.register_opts(utils_opts, group='ironic_lib') LOG = logging.getLogger(__name__) @@ -85,13 +76,9 @@ def execute(*cmd, use_standard_locale=False, log_stdout=True, **kwargs): env['LC_ALL'] = 'C' kwargs['env_variables'] = env - # If root_helper config is not specified, no commands are run as root. - run_as_root = kwargs.get('run_as_root', False) - if run_as_root: - if not CONF.ironic_lib.root_helper: - kwargs['run_as_root'] = False - else: - kwargs['root_helper'] = CONF.ironic_lib.root_helper + if kwargs.pop('run_as_root', False): + warnings.warn("run_as_root is deprecated and has no effect", + DeprecationWarning) def _log(stdout, stderr): if log_stdout: @@ -166,7 +153,7 @@ def mkfs(fs, path, label=None): args.extend([label_opt, label]) args.append(path) try: - execute(*args, run_as_root=True, use_standard_locale=True) + execute(*args, use_standard_locale=True) except processutils.ProcessExecutionError as e: with excutils.save_and_reraise_exception() as ctx: if os.strerror(errno.ENOENT) in e.stderr: @@ -203,7 +190,7 @@ def dd(src, dst, *args): """ LOG.debug("Starting dd process.") execute('dd', 'if=%s' % src, 'of=%s' % dst, *args, - use_standard_locale=True, run_as_root=True) + use_standard_locale=True) def is_http_url(url): @@ -213,7 +200,7 @@ def is_http_url(url): def list_opts(): """Entry point for oslo-config-generator.""" - return [('ironic_lib', utils_opts)] + return [('ironic_lib', [])] # placeholder def _extract_hint_operator_and_values(hint_expression, hint_name): @@ -551,15 +538,15 @@ def mounted(source, dest=None, opts=None, fs_type=None, mounted = False try: - execute("mount", source, dest, *params, run_as_root=True, - attempts=mount_attempts, delay_on_retry=True) + execute("mount", source, dest, *params, attempts=mount_attempts, + delay_on_retry=True) mounted = True yield dest finally: if mounted: try: - execute("umount", dest, run_as_root=True, - attempts=umount_attempts, delay_on_retry=True) + execute("umount", dest, attempts=umount_attempts, + delay_on_retry=True) except (EnvironmentError, processutils.ProcessExecutionError) as exc: LOG.warning(