From 2bebfcd7aab6b680709de993f90b9a7279690cc0 Mon Sep 17 00:00:00 2001 From: Pavel Raiskup Date: Fri, 27 Sep 2024 13:40:38 +0200 Subject: [PATCH 1/4] chroot_scan: correctly create the result directory This explains the permissions problems before aa9a5b0d776eabd051, because while we created the plugin's result-directory (with root permissions), the mock's (the main) resultdir was created as well as the parent directory. Both these directories were created with root:root ownership, but only the sub-directory `chroot_scan` was later `chowned` to the non-privileged user. Complements: aa9a5b0d776eabd051045f64189c10f1f237db00 --- mock/py/mockbuild/plugins/chroot_scan.py | 4 +++- releng/release-notes-next/chroot_scan_resultdir.bugfix | 3 +++ 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 releng/release-notes-next/chroot_scan_resultdir.bugfix diff --git a/mock/py/mockbuild/plugins/chroot_scan.py b/mock/py/mockbuild/plugins/chroot_scan.py index 56f729367..8f139ecf7 100644 --- a/mock/py/mockbuild/plugins/chroot_scan.py +++ b/mock/py/mockbuild/plugins/chroot_scan.py @@ -13,7 +13,7 @@ # our imports from mockbuild.trace_decorator import getLog, traceLog -from mockbuild import util +from mockbuild import util, file_util requires_api_version = "1.1" @@ -57,6 +57,8 @@ def __scanChroot(self): regex = re.compile(regexstr) chroot = self.buildroot.make_chroot_path() self.buildroot.create_resultdir() + # self.resultdir != self.buildroot.resultdir + file_util.mkdirIfAbsent(self.resultdir) count = 0 logger = getLog() logger.debug("chroot_scan: Starting scan of %s", chroot) diff --git a/releng/release-notes-next/chroot_scan_resultdir.bugfix b/releng/release-notes-next/chroot_scan_resultdir.bugfix new file mode 100644 index 000000000..6c172b65f --- /dev/null +++ b/releng/release-notes-next/chroot_scan_resultdir.bugfix @@ -0,0 +1,3 @@ +Mock v5.7 introduced a regression in the `chroot_scan` plugin that prevented the +result directory from being created properly. This issue has been +[fixed][PR#1472]. From 1741718004b7042e60dfaeac70d65bf65aa07153 Mon Sep 17 00:00:00 2001 From: Pavel Raiskup Date: Fri, 27 Sep 2024 13:43:58 +0200 Subject: [PATCH 2/4] chroot_scan: use util.do to display command in --verbose Relates: #1472 Relates: #1455 --- mock/py/mockbuild/plugins/chroot_scan.py | 8 +++++--- releng/release-notes-next/chroot-scan-util-do.bugfix | 3 +++ 2 files changed, 8 insertions(+), 3 deletions(-) create mode 100644 releng/release-notes-next/chroot-scan-util-do.bugfix diff --git a/mock/py/mockbuild/plugins/chroot_scan.py b/mock/py/mockbuild/plugins/chroot_scan.py index 8f139ecf7..fec278dd4 100644 --- a/mock/py/mockbuild/plugins/chroot_scan.py +++ b/mock/py/mockbuild/plugins/chroot_scan.py @@ -9,7 +9,6 @@ import os.path import re import shutil -import subprocess # our imports from mockbuild.trace_decorator import getLog, traceLog @@ -68,7 +67,10 @@ def __scanChroot(self): m = regex.search(f) if m: srcpath = os.path.join(root, f) - subprocess.call("cp --preserve=mode --parents %s %s" % (srcpath, self.resultdir), shell=True) + # we intentionally ignore errors here: + # https://github.com/rpm-software-management/mock/issues/1455 + util.do(["cp", "--preserve=mode", "--parents", srcpath, + self.resultdir], raiseExc=False) count += 1 copied.append(srcpath) logger.debug("chroot_scan: finished with %d files found", count) @@ -78,7 +80,7 @@ def __scanChroot(self): self.buildroot.uid_manager.changeOwner(self.resultdir, recursive=True) # some packages installs 555 perms on dirs, # so user can't delete/move chroot_scan's results - subprocess.call(['chmod', '-R', 'u+w', self.resultdir]) + util.do(['chmod', '-R', 'u+w', self.resultdir]) if self._tarball(): tarfile = self.resultdir + ".tar.gz" logger.info("chroot_scan: creating tarball %s", tarfile) diff --git a/releng/release-notes-next/chroot-scan-util-do.bugfix b/releng/release-notes-next/chroot-scan-util-do.bugfix new file mode 100644 index 000000000..31704a452 --- /dev/null +++ b/releng/release-notes-next/chroot-scan-util-do.bugfix @@ -0,0 +1,3 @@ +The `chroot_scan` plugin now consistently uses the `util.do` method instead of +custom `subprocess.call` calls. This ensures that the `mock --verbose` output +properly displays the commands (like `cp`, or `tar`) being executed. From 33861bc1c316e321daab87aaad13d6b47fc96203 Mon Sep 17 00:00:00 2001 From: Pavel Raiskup Date: Fri, 27 Sep 2024 13:44:26 +0200 Subject: [PATCH 3/4] chroot_scan: make sure the tarball is owned by non-priv user Relates: #1472 --- mock/py/mockbuild/plugins/chroot_scan.py | 1 + releng/release-notes-next/chroot-scan-permissions.bugfix | 2 ++ 2 files changed, 3 insertions(+) create mode 100644 releng/release-notes-next/chroot-scan-permissions.bugfix diff --git a/mock/py/mockbuild/plugins/chroot_scan.py b/mock/py/mockbuild/plugins/chroot_scan.py index fec278dd4..49247013e 100644 --- a/mock/py/mockbuild/plugins/chroot_scan.py +++ b/mock/py/mockbuild/plugins/chroot_scan.py @@ -90,3 +90,4 @@ def __scanChroot(self): shell=False, printOutput=True ) shutil.rmtree(self.resultdir) + self.buildroot.uid_manager.changeOwner(tarfile) diff --git a/releng/release-notes-next/chroot-scan-permissions.bugfix b/releng/release-notes-next/chroot-scan-permissions.bugfix new file mode 100644 index 000000000..ef97387b3 --- /dev/null +++ b/releng/release-notes-next/chroot-scan-permissions.bugfix @@ -0,0 +1,2 @@ +The ownership of the `chroot_scan` plugin "output tarball" (when `write_tar = +True`) has been corrected, ensuring the file is no longer root-owned. From 3ca4d2427195b9c86ab381efa50743bf073f24f4 Mon Sep 17 00:00:00 2001 From: Pavel Raiskup Date: Fri, 27 Sep 2024 15:31:50 +0200 Subject: [PATCH 4/4] chroot_scan: add a behave test-case Fixes: #1472 --- behave/features/chroot-scan.feature | 19 ++++++++++ behave/features/environment.py | 4 ++ behave/features/steps/other.py | 57 +++++++++++++++++++++++++++++ behave/testlib.py | 12 +++++- 4 files changed, 91 insertions(+), 1 deletion(-) create mode 100644 behave/features/chroot-scan.feature diff --git a/behave/features/chroot-scan.feature b/behave/features/chroot-scan.feature new file mode 100644 index 000000000..d78990c80 --- /dev/null +++ b/behave/features/chroot-scan.feature @@ -0,0 +1,19 @@ +Feature: The chroot_scan plugin + +@chroot_scan +Scenario: Check that chroot_scan works and file permissions are correct + Given chroot_scan is enabled for dnf5.log + And an unique mock namespace + When an online source RPM is rebuilt + Then the build succeeds + And dnf5.log file is in chroot_scan result dir + And ownership of all chroot_scan files is correct + +@chroot_scan +Scenario: Check that chroot_scan tarball is created correctly + Given an unique mock namespace + And chroot_scan is enabled for dnf5.log + And chroot_scan is configured to produce tarball + When an online source RPM is rebuilt + Then the build succeeds + And chroot_scan tarball has correct perms and provides dnf5.log diff --git a/behave/features/environment.py b/behave/features/environment.py index 9567c817c..d702b0ba8 100644 --- a/behave/features/environment.py +++ b/behave/features/environment.py @@ -3,6 +3,7 @@ """ import os +import pwd import random import shutil import string @@ -57,11 +58,14 @@ def before_all(context): def _cleanup_workdir(context): shutil.rmtree(context.workdir) context.workdir = None + context.custom_config = "" def before_scenario(context, _scenario): """ execute before - once for each - scenario """ context.workdir = tempfile.mkdtemp(prefix="mock-behave-tests-") + context.custom_config = "" context.add_cleanup(_cleanup_workdir, context) context.mock = Mock(context) context.add_repos = [] + context.current_user = pwd.getpwuid(os.getuid())[0] diff --git a/behave/features/steps/other.py b/behave/features/steps/other.py index 849635522..fa8d5b629 100644 --- a/behave/features/steps/other.py +++ b/behave/features/steps/other.py @@ -5,7 +5,9 @@ import json import os import shutil +import tarfile import tempfile +from pathlib import Path from hamcrest import ( assert_that, @@ -244,3 +246,58 @@ def step_impl(context, option): def step_impl(_, directory): assert_that(os.path.exists(directory), equal_to(True)) assert_that(not os.listdir(directory), equal_to(True)) + + +@given('chroot_scan is enabled for {regex}') +def step_impl(context, regex): + context.custom_config += f"""\ +config_opts['plugin_conf']['chroot_scan_enable'] = True +config_opts['plugin_conf']['chroot_scan_opts']['regexes'] = ["{regex}"] +config_opts['plugin_conf']['chroot_scan_opts']['only_failed'] = False +""" + + +@then('{file} file is in chroot_scan result dir') +def step_impl(context, file): + resultdir = os.path.join(context.mock.resultdir, 'chroot_scan') + + # Find the expected file + found = False + print("resultdir: ", resultdir) + for _, _, files in os.walk(resultdir): + for f in files: + print(f) + if f == file: + found = True + break + if found: + break + assert_that(found, equal_to(True)) + + +@given('chroot_scan is configured to produce tarball') +def step_impl(context): + context.custom_config += """\ +config_opts['plugin_conf']['chroot_scan_opts']['write_tar'] = True +""" + + +@then('ownership of all chroot_scan files is correct') +def step_impl(context): + resultdir = os.path.join(context.mock.resultdir, 'chroot_scan') + for root, dirs, files in os.walk(resultdir): + for f in files + dirs: + path = Path(root) / f + assert_that(path.group(), equal_to("mock")) + assert_that(path.owner(), equal_to(context.current_user)) + + +@then('chroot_scan tarball has correct perms and provides dnf5.log') +def step_impl(context): + tarball = Path(context.mock.resultdir, 'chroot_scan.tar.gz') + with tarfile.open(tarball, 'r:gz') as tarf: + for file in tarf.getnames(): + if file.endswith("dnf5.log"): + break + assert_that(tarball.group(), equal_to("mock")) + assert_that(tarball.owner(), equal_to(context.current_user)) diff --git a/behave/testlib.py b/behave/testlib.py index d2cd61354..8f463211b 100644 --- a/behave/testlib.py +++ b/behave/testlib.py @@ -2,6 +2,7 @@ from contextlib import contextmanager import io +from pathlib import Path import shlex import os import subprocess @@ -98,7 +99,16 @@ def init(self): def rebuild(self, srpms): """ Rebuild source RPM(s) """ - out, err = run_check(self.basecmd + ["--rebuild"] + srpms) + + chrootspec = [] + if self.context.custom_config: + config_file = Path(self.context.workdir) / "custom.cfg" + with config_file.open("w") as fd: + fd.write(f"include('{self.context.chroot}.cfg')\n") + fd.write(self.context.custom_config) + chrootspec = ["-r", str(config_file)] + + out, err = run_check(self.basecmd + chrootspec + ["--rebuild"] + srpms) self.context.mock_runs['rebuild'] += [{ "status": 0, "out": out,