openstack/ironic-lib
Code Issues Proposed changes

Remove the rest of the rootwrap machinery

Browse Source 
Change-Id: Ia724d933bbf69fb78a003947a92e952ab1e0e44e
This commit is contained in:
Dmitry Tantsur 2024-09-10 11:43:31 +02:00
parent 779d498663
commit 68bd8ad8a5
No known key found for this signature in database
GPG Key ID: 315B2AF9FD216C60
3 changed files with 25 additions and 95 deletions
etc/ironic/rootwrap.d
ironic-lib.filters
ironic_lib
tests
test_utils.py
utils.py

27
etc/ironic/rootwrap.d/ironic-lib.filters

@ -1,30 +1,5 @@
# An ironic-lib.filters to be used with rootwrap command. # 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. # 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] [Filters]
# ironic_lib/disk_utils.py # Left for backward compatibility only, do not use.
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

58
ironic_lib/tests/test_utils.py

@ -72,34 +72,6 @@ class ExecuteTestCase(base.IronicLibTestCase):
execute_mock.assert_called_once_with('foo', execute_mock.assert_called_once_with('foo',
env_variables={'foo': 'bar'}) 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) @mock.patch.object(utils, 'LOG', autospec=True)
def _test_execute_with_log_stdout(self, log_mock, log_stdout=None): def _test_execute_with_log_stdout(self, log_mock, log_stdout=None):
with mock.patch.object( with mock.patch.object(
@ -147,13 +119,10 @@ class MkfsTestCase(base.IronicLibTestCase):
utils.mkfs('swap', '/my/swap/block/dev') utils.mkfs('swap', '/my/swap/block/dev')
expected = [mock.call('mkfs', '-t', 'ext4', '-F', '/my/block/dev', expected = [mock.call('mkfs', '-t', 'ext4', '-F', '/my/block/dev',
run_as_root=True,
use_standard_locale=True), use_standard_locale=True),
mock.call('mkfs', '-t', 'msdos', '/my/msdos/block/dev', mock.call('mkfs', '-t', 'msdos', '/my/msdos/block/dev',
run_as_root=True,
use_standard_locale=True), use_standard_locale=True),
mock.call('mkswap', '/my/swap/block/dev', mock.call('mkswap', '/my/swap/block/dev',
run_as_root=True,
use_standard_locale=True)] use_standard_locale=True)]
self.assertEqual(expected, execute_mock.call_args_list) 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') utils.mkfs('swap', '/my/swap/block/dev', 'swap-vol')
expected = [mock.call('mkfs', '-t', 'ext4', '-F', '-L', 'ext4-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), use_standard_locale=True),
mock.call('mkfs', '-t', 'msdos', '-n', 'msdos-vol', mock.call('mkfs', '-t', 'msdos', '-n', 'msdos-vol',
'/my/msdos/block/dev', run_as_root=True, '/my/msdos/block/dev',
use_standard_locale=True), use_standard_locale=True),
mock.call('mkswap', '-L', 'swap-vol', mock.call('mkswap', '-L', 'swap-vol',
'/my/swap/block/dev', run_as_root=True, '/my/swap/block/dev',
use_standard_locale=True)] use_standard_locale=True)]
self.assertEqual(expected, execute_mock.call_args_list) self.assertEqual(expected, execute_mock.call_args_list)
@ -548,8 +517,8 @@ class MountedTestCase(base.IronicLibTestCase):
self.assertIs(path, mock_temp.return_value) self.assertIs(path, mock_temp.return_value)
mock_execute.assert_has_calls([ mock_execute.assert_has_calls([
mock.call("mount", '/dev/fake', mock_temp.return_value, mock.call("mount", '/dev/fake', mock_temp.return_value,
run_as_root=True, attempts=1, delay_on_retry=True), attempts=1, delay_on_retry=True),
mock.call("umount", mock_temp.return_value, run_as_root=True, mock.call("umount", mock_temp.return_value,
attempts=3, delay_on_retry=True), attempts=3, delay_on_retry=True),
]) ])
mock_rmtree.assert_called_once_with(mock_temp.return_value) 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: with utils.mounted('/dev/fake', '/mnt/fake') as path:
self.assertEqual('/mnt/fake', path) self.assertEqual('/mnt/fake', path)
mock_execute.assert_has_calls([ 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), 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), attempts=3, delay_on_retry=True),
]) ])
self.assertFalse(mock_temp.called) self.assertFalse(mock_temp.called)
@ -572,8 +541,8 @@ class MountedTestCase(base.IronicLibTestCase):
self.assertEqual('/mnt/fake', path) self.assertEqual('/mnt/fake', path)
mock_execute.assert_has_calls([ mock_execute.assert_has_calls([
mock.call("mount", '/dev/fake', '/mnt/fake', '-o', 'ro,foo=bar', mock.call("mount", '/dev/fake', '/mnt/fake', '-o', 'ro,foo=bar',
run_as_root=True, attempts=1, delay_on_retry=True), 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), attempts=3, delay_on_retry=True),
]) ])
@ -583,8 +552,8 @@ class MountedTestCase(base.IronicLibTestCase):
self.assertEqual('/mnt/fake', path) self.assertEqual('/mnt/fake', path)
mock_execute.assert_has_calls([ mock_execute.assert_has_calls([
mock.call("mount", '/dev/fake', '/mnt/fake', '-t', 'iso9660', mock.call("mount", '/dev/fake', '/mnt/fake', '-t', 'iso9660',
run_as_root=True, attempts=1, delay_on_retry=True), 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), attempts=3, delay_on_retry=True),
]) ])
@ -593,7 +562,6 @@ class MountedTestCase(base.IronicLibTestCase):
self.assertRaises(OSError, utils.mounted('/dev/fake').__enter__) self.assertRaises(OSError, utils.mounted('/dev/fake').__enter__)
mock_execute.assert_called_once_with("mount", '/dev/fake', mock_execute.assert_called_once_with("mount", '/dev/fake',
mock_temp.return_value, mock_temp.return_value,
run_as_root=True,
attempts=1, attempts=1,
delay_on_retry=True) delay_on_retry=True)
mock_rmtree.assert_called_once_with(mock_temp.return_value) 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: with utils.mounted('/dev/fake', '/mnt/fake') as path:
self.assertEqual('/mnt/fake', path) self.assertEqual('/mnt/fake', path)
mock_execute.assert_has_calls([ 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), 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), attempts=3, delay_on_retry=True),
]) ])
self.assertFalse(mock_rmtree.called) self.assertFalse(mock_rmtree.called)

35
ironic_lib/utils.py

@ -29,9 +29,9 @@ import shlex
import shutil import shutil
import tempfile import tempfile
from urllib import parse as urlparse from urllib import parse as urlparse
import warnings
from oslo_concurrency import processutils from oslo_concurrency import processutils
from oslo_config import cfg
from oslo_utils import excutils from oslo_utils import excutils
from oslo_utils import specs_matcher from oslo_utils import specs_matcher
from oslo_utils import strutils from oslo_utils import strutils
@ -40,15 +40,6 @@ from oslo_utils import units
from ironic_lib.common.i18n import _ from ironic_lib.common.i18n import _
from ironic_lib import exception 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__) LOG = logging.getLogger(__name__)
@ -85,13 +76,9 @@ def execute(*cmd, use_standard_locale=False, log_stdout=True, **kwargs):
env['LC_ALL'] = 'C' env['LC_ALL'] = 'C'
kwargs['env_variables'] = env kwargs['env_variables'] = env
# If root_helper config is not specified, no commands are run as root. if kwargs.pop('run_as_root', False):
run_as_root = kwargs.get('run_as_root', False) warnings.warn("run_as_root is deprecated and has no effect",
if run_as_root: DeprecationWarning)
if not CONF.ironic_lib.root_helper:
kwargs['run_as_root'] = False
else:
kwargs['root_helper'] = CONF.ironic_lib.root_helper
def _log(stdout, stderr): def _log(stdout, stderr):
if log_stdout: if log_stdout:
@ -166,7 +153,7 @@ def mkfs(fs, path, label=None):
args.extend([label_opt, label]) args.extend([label_opt, label])
args.append(path) args.append(path)
try: try:
execute(*args, run_as_root=True, use_standard_locale=True) execute(*args, use_standard_locale=True)
except processutils.ProcessExecutionError as e: except processutils.ProcessExecutionError as e:
with excutils.save_and_reraise_exception() as ctx: with excutils.save_and_reraise_exception() as ctx:
if os.strerror(errno.ENOENT) in e.stderr: if os.strerror(errno.ENOENT) in e.stderr:
@ -203,7 +190,7 @@ def dd(src, dst, *args):
""" """
LOG.debug("Starting dd process.") LOG.debug("Starting dd process.")
execute('dd', 'if=%s' % src, 'of=%s' % dst, *args, 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): def is_http_url(url):
@ -213,7 +200,7 @@ def is_http_url(url):
def list_opts(): def list_opts():
"""Entry point for oslo-config-generator.""" """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): 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 mounted = False
try: try:
execute("mount", source, dest, *params, run_as_root=True, execute("mount", source, dest, *params, attempts=mount_attempts,
attempts=mount_attempts, delay_on_retry=True) delay_on_retry=True)
mounted = True mounted = True
yield dest yield dest
finally: finally:
if mounted: if mounted:
try: try:
execute("umount", dest, run_as_root=True, execute("umount", dest, attempts=umount_attempts,
attempts=umount_attempts, delay_on_retry=True) delay_on_retry=True)
except (EnvironmentError, except (EnvironmentError,
processutils.ProcessExecutionError) as exc: processutils.ProcessExecutionError) as exc:
LOG.warning( LOG.warning(