PR#1939: Add pungi_buildinstall plugin wrapping runroot Lorax call. - koji

koji

#1939 Add pungi_buildinstall plugin wrapping runroot Lorax call.

Closed 4 years ago by tkopecek. Opened 4 years ago by jkaluza.

jkaluza/koji pungi-buildinstall into master

Add pungi_buildinstall plugin wrapping runroot Lorax call.

Jan Kaluza • 4 years ago

04af95e

plugins/builder/pungi.py

file added

+127

		`@@ -0,0 +1,127 @@`
		`+ # kojid plugin`
		`+`
		`+ from __future__ import absolute_import`
		`+ import six`
		`+ from six.moves import shlex_quote`
		`+ import os`
		`+`
		`+ import koji`
		`+ import koji.tasks`
		`+`
		`+`
		`+ __all__ = ('PungiBuildinstallTask',)`
		`+`
		`+`
		`+ class PungiBuildinstallTask(koji.tasks.BaseTaskHandler):`
		`+`
		`+ Methods = ['pungi_buildinstall']`
		`+`
		`+ _taskWeight = 0.5`
		`+`
		`+ def __init__(self, args, *kwargs):`
		`+ self.allowed_lorax_args = set([`
		`+ "product",`
		`+ "version",`
		`+ "release",`
		`+ "sources",`
		`+ "variant",`
		`+ "bugurl",`
		`+ "nomacboot",`
		`+ "noupgrade",`
		`+ "isfinal",`
		`+ "buildarch",`
		`+ "volid",`
		`+ "installpkgs",`
		`+ "add-template",`
		`+ "add-arch-template",`
		`+ "add-template-var",`
		`+ "add-arch-template-var",`
		`+ "rootfs-size",`
		`+ "dracut-args"`
		`+ ])`
		`+ return super(PungiBuildinstallTask, self).__init__(args, *kwargs)`
		`+`
		`+ def handler(self, tag, arch, packages=[], mounts=[], weight=None, lorax_args=None,`
		`+ chown_uid=None):`
		`+ if weight is not None:`
		`+ weight = max(weight, 0.5)`
		`+ self.session.host.setTaskWeight(self.id, weight)`
		`+`
		`+ if lorax_args is None:`
		`+ lorax_args = {}`
		`+`
		`+ if "outputdir" in lorax_args:`
		`+ output_dir = lorax_args["outputdir"]`
		`+ del lorax_args["outputdir"]`
		`+ else:`
		`+ output_dir = self.workdir`
		`+`
		`+ if "lorax" not in packages:`
		`+ packages.append("lorax")`
		`+`
		`+ # Raise an exception if not allowed argument is used.`
		`+ not_allowed_args = set(lorax_args.keys()) - self.allowed_lorax_args`
		`+ if not_allowed_args:`
		`+ args = ', '.join(str(x) for x in not_allowed_args)`
		`+ raise koji.GenericError("Not allowed lorax arguments found: %s." % args)`
		`+`
		`+ # Generate the lorax command with lorax_args.`
		`+ lorax_cmd = "lorax"`
		`+ for opt, arg in lorax_args.items():`
		`+ if opt == "sources":`
		`+ for source in arg:`
		`+ if "://" not in source:`
		`+ source = "file://%s" % source`
		`+ quoted_source = shlex_quote(source)`
		`+ lorax_cmd +=" --source=%s" % quoted_source`
		`+ elif opt == "dracut-args":`
		`+ for dracut_arg in arg:`
		`+ quoted_arg = shlex_quote(dracut_arg)`
		`+ lorax_cmd += " --dracut-arg=%s" % quoted_arg`
		`+ elif isinstance(arg, list):`
		`+ for lorax_arg in arg:`
		`+ quoted_arg = shlex_quote(lorax_arg)`
		`+ lorax_cmd += " --%s=%s" % (opt, quoted_arg)`
		`+ elif isinstance(arg, six.string_types):`
		`+ quoted_arg = shlex_quote(arg)`
		`+ lorax_cmd += " --%s=%s" % (opt, quoted_arg)`
		`+ elif arg:`
		`+ lorax_cmd += " --%s" % opt`
		`+`
		`+ if os.path.exists(output_dir):`
		`+ raise koji.GenericError('The "outputdir" "%s" already exists.' % output_dir)`
		`+`
		`+ # Create log directory and add --logfile.`
		`+ logdir = os.path.join(output_dir, "logs")`
		`+ os.makedirs(logdir)`
		`+ logfile = os.path.join(logdir, "lorax.log")`
		`+ lorax_cmd += " --logfile=%s" % logfile`
		`+`
		`+ # Set the output directory and add it to lorax_cmd. This directory`
		`+ # must not exist, otherwise the Lorax command fails, so we won't`
		`+ # create it.`
		`+ result_dir = os.path.join(output_dir, "results")`
		`+ lorax_cmd += " %s" % result_dir`
		`+`
		`+ if chown_uid:`
		`+ # Store the exit code of "lorax" command.`
		`+ lorax_cmd += "; ret=$?;"`
		`+ # Run chmod/chown to make the lorax output readible for requester.`
		`+ lorax_cmd += " chmod -R a+r %s" % shlex_quote(output_dir)`
		`+ lorax_cmd += " && chown -R %s %s" % (`
		`+ shlex_quote(str(chown_uid)), shlex_quote(output_dir))`
		`+ # Exit with the original lorax exit code.`
		`+ lorax_cmd += "; exit $ret"`
		`+`
		`+ # Execute runroot subtask.`
		`+ kwargs = {`
		`+ "mounts": mounts,`
		`+ "packages": packages,`
		`+ }`
		`+ task_id = self.session.host.subtask(`
		`+ method='runroot', arglist=[tag, arch, lorax_cmd], parent=self.id, kwargs=kwargs)`
		`+`
		`+ # In case the runroot task fails, this raises an exception.`
		`+ self.wait(task_id)`
		`+`
		`+ return "Completed successfully"`

plugins/cli/pungi.py

file added

+96

		`@@ -0,0 +1,96 @@`
		`+ from __future__ import absolute_import`
		`+ import sys`
		`+ import time`
		`+ from optparse import OptionParser`
		`+`
		`+ import koji`
		`+ from koji.plugin import export_cli`
		`+ from koji_cli.lib import _, activate_session, watch_tasks`
		`+`
		`+ @export_cli`
		`+ def handle_pungi_buildinstall(options, session, args):`
		`+ "[admin] Run a command in a buildroot"`
		`+ usage = _("usage: %prog pungi_buildinstall [options] <tag> <arch> [lorax_arguments, ...]")`
		`+ usage += _("\n(Specify the --help global option for a list of other help options)")`
		`+ parser = OptionParser(usage=usage)`
		`+ parser.disable_interspersed_args()`
		`+ parser.add_option("-p", "--package", action="append", default=[], help=_("make sure this package is in the chroot"))`
		`+ parser.add_option("-m", "--mount", action="append", default=[], help=_("mount this directory read-write in the chroot"))`
		`+ parser.add_option("-w", "--weight", type='int', help=_("set task weight"))`
		`+ parser.add_option("--chown-uid", type='int', help=_("set UID owning the output files."))`
		`+ parser.add_option("--channel-override", help=_("use a non-standard channel"))`
		`+ parser.add_option("--task-id", action="store_true", default=False,`
		`+ help=_("Print the ID of the pungi_buildinstall task"))`
		`+ parser.add_option("--nowait", action="store_false", dest="wait", default=True, help=_("Do not wait on task"))`
		`+ parser.add_option("--watch", action="store_true", help=_("Watch task instead of printing pungi_buildinstall.log"))`
		`+ parser.add_option("--quiet", action="store_true", default=options.quiet,`
		`+ help=_("Do not print the task information"))`
		`+`
		`+ (opts, args) = parser.parse_args(args)`
		`+`
		`+ if len(args) < 2:`
		`+ parser.error(_("Incorrect number of arguments"))`
		`+ assert False # pragma: no cover`
		`+`
		`+ activate_session(session, options)`
		`+`
		`+ if not session.hasPerm('admin') or session.hasPerm('pungi_buildinstall'):`
		`+ parser.error(_("This action requires pungi_buildinstall or admin privileges"))`
		`+`
		`+ tag = args[0]`
		`+ arch = args[1]`
		`+ lorax_args = {}`
		`+ for arg in args[2:]:`
		`+ if "=" in arg:`
		`+ k, v = arg.split("=")`
		`+ if k in lorax_args:`
		`+ if not isinstance(lorax_args[k], list):`
		`+ lorax_args[k] = [lorax_args[k]]`
		`+ lorax_args[k].append(v)`
		`+ else:`
		`+ lorax_args[k] = v`
		`+ else:`
		`+ lorax_args[arg] = True`
		`+ try:`
		`+ kwargs = { 'channel': opts.channel_override,`
		`+ 'packages': opts.package,`
		`+ 'mounts': opts.mount,`
		`+ 'weight': opts.weight,`
		`+ 'chown_uid': opts.chown_uid,`
		`+ 'lorax_args': lorax_args}`
		`+`
		`+ task_id = session.pungi_buildinstall(tag, arch, **kwargs)`
		`+ except koji.GenericError as e:`
		`+ if 'Invalid method' in str(e):`
		`+ print("* The pungi_buildinstall plugin appears to not be installed on the"`
julian8628 commented 4 years ago might be in stderr?
		`+ " koji hub. Please contact the administrator.")`
		`+ raise`
		`+ if opts.task_id:`
		`+ print(task_id)`
julian8628 commented 4 years ago is this a debug print?
jkaluza commented 4 years ago No, this is actually similar to "koji runroot --task-id" - it prints the ID of the executed Koji task.
		`+`
		`+ if not opts.wait:`
		`+ return`
		`+`
		`+ if opts.watch:`
		`+ session.logout()`
		`+ return watch_tasks(session, [task_id], quiet=opts.quiet,`
		`+ poll_interval=options.poll_interval)`
		`+`
		`+ try:`
		`+ while True:`
		`+ # wait for the task to finish`
		`+ if session.taskFinished(task_id):`
		`+ break`
		`+ time.sleep(options.poll_interval)`
		`+ except KeyboardInterrupt:`
		`+ # this is probably the right thing to do here`
		`+ print("User interrupt: canceling pungi_buildinstall task")`
		`+ session.cancelTask(task_id)`
		`+ raise`
		`+ sys.stdout.flush()`
		`+ info = session.getTaskInfo(task_id)`
		`+ if info is None:`
		`+ sys.exit(1)`
		`+ state = koji.TASK_STATES[info['state']]`
		`+ if state in ('FAILED', 'CANCELED'):`
		`+ sys.exit(1)`

plugins/hub/pungi_hub.py

file added

+32

		`@@ -0,0 +1,32 @@`
		`+ #koji hub plugin`
		`+ # There is a kojid plugin that goes with this hub plugin. The kojid builder`
		`+ # plugin has a config file. This hub plugin has no config file.`
		`+`
		`+`
		`+ from __future__ import absolute_import`
		`+ from koji.context import context`
		`+ from koji.plugin import export`
		`+ import koji`
		`+ import sys`
		`+`
		`+ #XXX - have to import kojihub for make_task`
		`+ sys.path.insert(0, '/usr/share/koji-hub/')`
		`+ import kojihub`
		`+`
		`+ __all__ = ('pungi_buildinstall',)`
		`+`
		`+`
		`+ @export`
		`+ def pungi_buildinstall(tag, arch, channel=None, **opts):`
		`+ """ Create a pungi_buildinstall task """`
		`+ context.session.assertPerm('pungi_buildinstall')`
		`+ taskopts = {`
		`+ 'priority': 15,`
		`+ 'arch': arch,`
		`+ }`
		`+`
		`+ taskopts['channel'] = channel or 'runroot'`
		`+`
		`+ args = koji.encode_args(tag, arch, **opts)`
		`+ return kojihub.make_task('pungi_buildinstall', args, **taskopts)`
		`+`

tests/test_plugins/test_pungi_builder.py

file added

+123

		`@@ -0,0 +1,123 @@`
		`+ from __future__ import absolute_import`
		`+ import copy`
		`+ import mock`
		`+ from nose.tools import raises`
		`+ try:`
		`+ import unittest2 as unittest`
		`+ except ImportError:`
		`+ import unittest`
		`+`
		`+ import six.moves.configparser`
		`+`
		`+ # inject builder data`
		`+ from tests.test_builder.loadkojid import kojid`
		`+ import __main__`
		`+ __main__.BuildRoot = kojid.BuildRoot`
		`+`
		`+ import koji`
		`+ import pungi`
		`+`
		`+`
		`+ class TestHandler(unittest.TestCase):`
		`+ def setUp(self):`
		`+ self.session = mock.MagicMock()`
		`+ self.session.host.taskWait.return_value = [{}, {}]`
		`+ self.session.host.subtask.return_value = 124`
		`+`
		`+ self.br = mock.MagicMock()`
		`+ self.br.mock.return_value = 0`
		`+ self.br.id = 678`
		`+ self.br.rootdir.return_value = '/rootdir'`
		`+ pungi.BuildRoot = mock.MagicMock()`
		`+ pungi.BuildRoot.return_value = self.br`
		`+`
		`+ options = mock.MagicMock()`
		`+ options.workdir = '/tmp/nonexistentdirectory'`
		`+ options.topurls = None`
		`+ self.t = pungi.PungiBuildinstallTask(123, 'pungi_buildinstall', {}, self.session, options)`
		`+ self.t.wait = mock.MagicMock()`
		`+ self.t.wait.return_value = {124: {'brootid': 2342345}}`
		`+ self.t.uploadTree = mock.MagicMock()`
		`+`
		`+ def tearDown(self):`
		`+ self.t.removeWorkdir()`
		`+ pungi.BuildRoot = kojid.BuildRoot`
		`+`
		`+ def test_handler_simple(self):`
		`+ self.t.handler('tag_name', 'noarch', packages=[], mounts=["/mnt/koji"], weight=10.0,`
		`+ lorax_args={"sources": ['https://foo/', 'http://bar/'], "variant": "foo"})`
		`+ lorax_cmd = (`
		`+ 'lorax --source=https://foo/ --source=http://bar/ --variant=foo '`
		`+ '--logfile=/tmp/nonexistentdirectory/tasks/123/123/logs/lorax.log '`
		`+ '/tmp/nonexistentdirectory/tasks/123/123/results')`
		`+ self.session.host.subtask.assert_called_once_with(`
		`+ arglist=['tag_name', 'noarch', lorax_cmd],`
		`+ kwargs={'mounts': ['/mnt/koji'], 'packages': ['lorax']}, method='runroot', parent=123)`
		`+`
		`+ @raises(koji.GenericError)`
		`+ def test_handler_not_allowed_arg(self):`
		`+ self.t.handler('tag_name', 'noarch', lorax_args={"force": True})`
		`+`
		`+ def test_handler_quoting_str_arg(self):`
		`+ self.t.handler(`
		`+ 'tag_name', 'noarch', lorax_args={"variant": "foo; echo 1;\" echo 2;\' echo 3;"})`
		`+ lorax_cmd = (`
		`+ 'lorax --variant=\'foo; echo 1;" echo 2;\'"\'"\' echo 3;\' '`
		`+ '--logfile=/tmp/nonexistentdirectory/tasks/123/123/logs/lorax.log '`
		`+ '/tmp/nonexistentdirectory/tasks/123/123/results')`
		`+ self.session.host.subtask.assert_called_once_with(`
		`+ arglist=['tag_name', 'noarch', lorax_cmd],`
		`+ kwargs={'mounts': [], 'packages': ['lorax']}, method='runroot', parent=123)`
		`+`
		`+ def test_handler_quoting_extra_list_args(self):`
		`+ for opt in ["source", "dracut-arg"]:`
		`+ # + "s" to get the plural form - "sources" and "dracut_args"`
		`+ lorax_args = {opt + "s": ["foo; echo 1;\" echo 2;\' echo 3;"]}`
		`+ self.t.handler('tag_name', 'noarch', lorax_args=lorax_args)`
		`+ lorax_cmd = (`
		`+ 'lorax --%s=\'foo; echo 1;" echo 2;\'"\'"\' echo 3;\' '`
		`+ '--logfile=/tmp/nonexistentdirectory/tasks/123/123/logs/lorax.log '`
		`+ '/tmp/nonexistentdirectory/tasks/123/123/results' % opt)`
		`+ self.session.host.subtask.assert_called_once_with(`
		`+ arglist=['tag_name', 'noarch', lorax_cmd],`
		`+ kwargs={'mounts': [], 'packages': ['lorax']}, method='runroot', parent=123)`
		`+`
		`+ def test_handler_quoting_list_args(self):`
		`+ self.t.handler(`
		`+ 'tag_name', 'noarch', lorax_args={"installpkgs": ["foo; echo 1;\" echo 2;\' echo 3;"]})`
		`+ lorax_cmd = (`
		`+ 'lorax --installpkgs=\'foo; echo 1;" echo 2;\'"\'"\' echo 3;\' '`
		`+ '--logfile=/tmp/nonexistentdirectory/tasks/123/123/logs/lorax.log '`
		`+ '/tmp/nonexistentdirectory/tasks/123/123/results')`
		`+ self.session.host.subtask.assert_called_once_with(`
		`+ arglist=['tag_name', 'noarch', lorax_cmd],`
		`+ kwargs={'mounts': [], 'packages': ['lorax']}, method='runroot', parent=123)`
		`+`
		`+ def test_handler_unset_bool_arg(self):`
		`+ self.t.handler('tag_name', 'noarch', lorax_args={"isfinal": False})`
		`+ lorax_cmd = (`
		`+ 'lorax --logfile=/tmp/nonexistentdirectory/tasks/123/123/logs/lorax.log '`
		`+ '/tmp/nonexistentdirectory/tasks/123/123/results')`
		`+ self.session.host.subtask.assert_called_once_with(`
		`+ arglist=['tag_name', 'noarch', lorax_cmd],`
		`+ kwargs={'mounts': [], 'packages': ['lorax']}, method='runroot', parent=123)`
		`+`
		`+ def test_handler_chown_uid(self):`
		`+ self.t.handler('tag_name', 'noarch', packages=[], mounts=["/mnt/koji"], weight=10.0,`
		`+ chown_uid=999, lorax_args={})`
		`+ lorax_cmd = (`
		`+ 'lorax '`
		`+ '--logfile=/tmp/nonexistentdirectory/tasks/123/123/logs/lorax.log '`
		`+ '/tmp/nonexistentdirectory/tasks/123/123/results; '`
		`+ 'ret=$?; '`
		`+ 'chmod -R a+r /tmp/nonexistentdirectory/tasks/123/123 '`
		`+ '&& chown -R 999 /tmp/nonexistentdirectory/tasks/123/123; '`
		`+ 'exit $ret')`
		`+ self.session.host.subtask.assert_called_once_with(`
		`+ arglist=['tag_name', 'noarch', lorax_cmd],`
		`+ kwargs={'mounts': ['/mnt/koji'], 'packages': ['lorax']}, method='runroot', parent=123)`
		`+`
		`+ @raises(koji.GenericError)`
		`+ def test_handler_outputdir_exists(self):`
		`+ self.t.handler('tag_name', 'noarch', packages=[], mounts=["/mnt/koji"], weight=10.0,`
		`+ lorax_args={"outputdir": "/tmp"})`

jkaluza commented 4 years ago

Current API of runroot plugin allows anyone with "runroot" permission
to run any random command on the builder (although it is run in the
chroot environment). This is often considered as unsafe and possible
security issue and prevents granting "runroot" permissions to other
Koji users which might need it to run Pungi.

In fact, the only runroot task which is really needed by Pungi
is execution of "lorax" command to generate buildinstall deliverables.
Other runroot use-cases can be successfully executed on local machine
without "root" user these days.

In this commit, new pungi_buildinstall plugin is introduced which
wraps "runroot" task in a way that only lorax command can be executed
with only predefined set of arguments (So far example "unsafe" --force
Lorax flag is not supported).

To use this new plugin, only new pungi_buildinstall permission is
required. This new permission can be granted to users who should be able
to execute Pungi compose including the buildinstall phase, but who are
not trusted to be granted full "runroot" permission.

Fixes: #1940

Signed-off-by: Jan Kaluza jkaluza@redhat.com

jkaluza commented 4 years ago

This has been discussed with @tkopecek who agreed to own this review.

jkaluza commented 4 years ago

@lsedlar, also CCing you here. Once this gets reviewed, I will add support for this new Koji task to Pungi.

tkopecek commented on line 21 of plugins/builder/pungi.py 4 years ago

It should be less, it is effectively not blocking anything and immediately spawns runroot. You can set it to 0.1

tkopecek commented on line 33 of plugins/cli/pungi.py 4 years ago

Is there any lorax argument needed by default? If not, lower it to 2. If yes, mark it properly in "usage"

tkopecek commented on line 37 of plugins/cli/pungi.py 4 years ago

permission should be checked here

    if not session.hasPerm('admin') or session.hasPerm('pungi_buildinstall'):
        parser.error(_("This action requires pungi_buildinstall or admin privileges"))

tkopecek commented on line 11 of plugins/hub/pungi_hub.py 4 years ago

unused

tkopecek commented on line 11 of plugins/builder/pungi.py 4 years ago

unused

tkopecek commented on line 10 of plugins/cli/pungi.py 4 years ago

unused

tkopecek commented on line 24 of plugins/builder/pungi.py 4 years ago

This syntax is now working on py26 (use set() instead)

jkaluza commented 4 years ago

Thanks for the review. I will be AFK for the rest of this week. I'm going to address your comments on Monday.

tkopecek commented 4 years ago

Also, please reference placeholder issue in commit message via line: "Fixes: https://pagure.io/koji/issue/1940"

rebased onto 0e920ef0679a1dddc3ff646736d3359156e51448

4 years ago

rebased onto b398544906e9c0889a6c83eb5d8e226fa6d70e64

4 years ago

rebased onto 8495d96eaf34d34972a504e50f01ddb9b59aac82

4 years ago

rebased onto a04588fbe53e5e1e2b289972cfecb64d55b11c42

4 years ago

rebased onto 3108f568019a564d13e8af8b1b89f606440ea0fc

4 years ago

jkaluza commented 4 years ago

@tkopecek, I've added more tests and also fixed the names of some Lorax args.

rebased onto 6115d3d769bc9f01b94b1dceb30afe14d07d792d

4 years ago

rebased onto 60811b4b277e1789991dffab420984a1b3c6b2b1

4 years ago

rebased onto 81289d423d17c9c70b008075dd61f9e8dd309dd4

4 years ago

jkaluza commented 4 years ago

@tkopecek, I've tested the current code with my Pungi changes and it successfully generated all the deliverables. I think the plugin is working correctly for me now and I treat it as complete for now.

tkopecek commented 4 years ago

@julian8628 @mikem

julian8628 commented on line 66 of plugins/cli/pungi.py 4 years ago

might be in stderr?

julian8628 commented on line 70 of plugins/cli/pungi.py 4 years ago

is this a debug print?

jkaluza commented on line 70 of plugins/cli/pungi.py 4 years ago

No, this is actually similar to "koji runroot --task-id" - it prints the ID of the executed Koji task.

julian8628 commented 4 years ago

No, this is actually similar to "koji runroot --task-id" - it prints the ID of the executed Koji task.

ok, could you make it more readable like task ID: xxxx?

julian8628 commented 4 years ago

:thumbsup: for the code

jkaluza commented 4 years ago

No, this is actually similar to "koji runroot --task-id" - it prints the ID of the executed Koji task.

ok, could you make it more readable like task ID: xxxx?

I could, but I wanted to keep this output compatible with the koji runroot output if possible. It saves some code in the Pungi. But of course I can change it if you still think so.

julian8628 commented 4 years ago

No, this is actually similar to "koji runroot --task-id" - it prints the ID of the executed Koji task.
ok, could you make it more readable like task ID: xxxx?

I could, but I wanted to keep this output compatible with the koji runroot output if possible. It saves some code in the Pungi. But of course I can change it if you still think so.

OK, that makes sense

lsedlar commented on line 110 of plugins/builder/pungi.py 4 years ago

How difficult would it be to make sure the permissions and owner is always updated, no matter whether lorax succeeded or failed?
Currently if lorax fails but manages to create some files already, the user who spawned the task will quite likely not be able to delete the files. (This is an issue with the existing implementation in Pungi. We didn't fix it since it's rather tricky to escape such a command line to pass to runroot.)

jkaluza commented 4 years ago

@lsedlar, that's good point. I will see what I can do to improve that.

rebased onto 04af95e

4 years ago

tkopecek commented 4 years ago

We're going to bundle it only internally. Fedora seems to not use it in near future. If it will, we will bring it back to koji.