[libvirt PATCH v2 0/4] ci: Adjustments to remove our dependency on sudo
Our Debian containers don't have sudo pre-installed and the only reason we actually needed it was to run a custom prepare script which as it turns out does nothing really. Since v1: - applied quoting to the variables as asked during the review process - simplified setting of CI_PODMAN_ARGS - moved 4/4 to 3/4 Erik Skultety (4): ci: Specify the shebang sequence for build.sh ci: Run podman command directly without wrapping it with prepare.sh ci: Drop the prepare.sh script ci: Makefile: Expose the CI_USER_LOGIN variable for users to use ci/Makefile | 62 +++++++++++++++++++++++++-------------------------- ci/build.sh | 2 ++ ci/prepare.sh | 13 ----------- 3 files changed, 33 insertions(+), 44 deletions(-) delete mode 100644 ci/prepare.sh -- 2.29.2
This is necessary for the follow up patch, because the default entrypoint for a Dockerfile is exec. Signed-off-by: Erik Skultety <eskultet@redhat.com> Reviewed-by: Andrea Bolognani <abologna@redhat.com> --- ci/build.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ci/build.sh b/ci/build.sh index 740b46a935..0f23df1fa3 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -1,3 +1,5 @@ +#!/bin/sh + # This script is used to build libvirt inside the container. # # You can customize it to your liking, or alternatively use a -- 2.29.2
The prepare.sh script isn't currently used and forces us to make use of sudo to switch the user inside the container from root to $USER which created a problem on our Debian Slim-based containers which don't have the 'sudo' package installed. This patch removes the sudo invocation and instead runs the CMD directly with podman. Summary of the changes: - move the corresponding env variables which we need to be set in the environment from the sudo invocation to the podman invocation - pass --workdir to podman to retain the original behaviour we had with sudo spawning a login shell. - MESON_OPTS env variable doesn't need to propagated to the execution environment anymore (like we had to do with sudo), because it's defined in the Dockerfile Signed-off-by: Erik Skultety <eskultet@redhat.com> Reviewed-by: Andrea Bolognani <abologna@redhat.com> --- ci/Makefile | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/ci/Makefile b/ci/Makefile index 7938e14c15..9308738d2d 100644 --- a/ci/Makefile +++ b/ci/Makefile @@ -82,7 +82,6 @@ CI_HOME_MOUNTS = \ $(NULL) CI_SCRIPT_MOUNTS = \ - --volume $(CI_SCRATCHDIR)/prepare:$(CI_USER_HOME)/prepare:z \ --volume $(CI_SCRATCHDIR)/build:$(CI_USER_HOME)/build:z \ $(NULL) @@ -150,6 +149,8 @@ CI_GIT_ARGS = \ # --user we execute as the same user & group account # as dev so that file ownership matches host # instead of root:root +# --workdir we change to user's home dir in the container +# before running the workload # --volume to pass in the cloned git repo & config # --ulimit lower files limit for performance reasons # --interactive @@ -158,6 +159,11 @@ CI_ENGINE_ARGS = \ --rm \ --interactive \ --tty \ + --user "$(CI_UID)":"$(CI_GID)" \ + --workdir "$(CI_USER_HOME)" \ + --env CI_CONT_SRCDIR="$(CI_CONT_SRCDIR)" \ + --env CI_MESON_ARGS="$(CI_MESON_ARGS)" \ + --env CI_NINJA_ARGS="$(CI_NINJA_ARGS)" \ $(CI_PODMAN_ARGS) \ $(CI_PWDB_MOUNTS) \ $(CI_HOME_MOUNTS) \ @@ -178,9 +184,8 @@ ci-prepare-tree: ci-check-engine cp /etc/passwd $(CI_SCRATCHDIR); \ cp /etc/group $(CI_SCRATCHDIR); \ mkdir -p $(CI_SCRATCHDIR)/home; \ - cp "$(CI_PREPARE_SCRIPT)" $(CI_SCRATCHDIR)/prepare; \ cp "$(CI_BUILD_SCRIPT)" $(CI_SCRATCHDIR)/build; \ - chmod +x "$(CI_SCRATCHDIR)/prepare" "$(CI_SCRATCHDIR)/build"; \ + chmod +x "$(CI_SCRATCHDIR)/build"; \ echo "Cloning $(CI_GIT_ROOT) to $(CI_HOST_SRCDIR)"; \ git clone $(CI_GIT_ARGS) $(CI_GIT_ROOT) $(CI_HOST_SRCDIR) || exit 1; \ for mod in $$(git submodule | awk '{ print $$2 }' | sed -E 's,^../,,g') ; \ @@ -192,18 +197,10 @@ ci-prepare-tree: ci-check-engine fi ci-run-command@%: ci-prepare-tree - $(CI_ENGINE) run $(CI_ENGINE_ARGS) $(CI_IMAGE_PREFIX)$*$(CI_IMAGE_TAG) \ - /bin/bash -c ' \ - $(CI_USER_HOME)/prepare || exit 1; \ - sudo \ - --login \ - --user="#$(CI_UID)" \ - --group="#$(CI_GID)" \ - MESON_OPTS="$$MESON_OPTS" \ - CI_CONT_SRCDIR="$(CI_CONT_SRCDIR)" \ - CI_MESON_ARGS="$(CI_MESON_ARGS)" \ - CI_NINJA_ARGS="$(CI_NINJA_ARGS)" \ - $(CI_COMMAND) || exit 1' + $(CI_ENGINE) run \ + $(CI_ENGINE_ARGS) \ + $(CI_IMAGE_PREFIX)$*$(CI_IMAGE_TAG) \ + $(CI_COMMAND) @test "$(CI_CLEAN)" = "1" && rm -rf $(CI_SCRATCHDIR) || : ci-shell@%: -- 2.29.2
The purpose of this script was to prepare a customized environment in the container, but was actually never used and it required the usage of sudo to switch the environment from root's context to a regular user's one. The thing is that once someone needs a custom script they would very likely to debug something and would also benefit from root privileges in general, so the usage of 'sudo' in such case was a bit cumbersome. Signed-off-by: Erik Skultety <eskultet@redhat.com> Reviewed-by: Andrea Bolognani <abologna@redhat.com> --- ci/prepare.sh | 13 ------------- 1 file changed, 13 deletions(-) delete mode 100644 ci/prepare.sh diff --git a/ci/prepare.sh b/ci/prepare.sh deleted file mode 100644 index da6fc9a1b5..0000000000 --- a/ci/prepare.sh +++ /dev/null @@ -1,13 +0,0 @@ -# This script is used to prepare the environment that will be used -# to build libvirt inside the container. -# -# You can customize it to your liking, or alternatively use a -# completely different script by passing -# -# CI_PREPARE_SCRIPT=/path/to/your/prepare/script -# -# to make. -# -# Note that this script will have root privileges inside the -# container, so it can be used for things like installing additional -# packages. -- 2.29.2
More often than not I find myself debugging in the containers which means that I need to have root inside, but without manually tweaking the Makefile each time the execution would simply fail thanks to the uid/gid mapping we do. What if we expose the CI_USER_LOGIN variable, so that when needed, the root can be simply passed with this variable and voila - you have a root shell inside the container with CWD=~root. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/Makefile | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/ci/Makefile b/ci/Makefile index 9308738d2d..05a50c021c 100644 --- a/ci/Makefile +++ b/ci/Makefile @@ -45,15 +45,15 @@ CI_CLEAN = 1 # preserved env CI_REUSE = 0 -# We need the container process to run with current host IDs -# so that it can access the passed in build directory -CI_UID = $(shell id -u) -CI_GID = $(shell id -g) - -# We also need the user's login and home directory to prepare the +# We need the user's login and home directory to prepare the # environment the way some programs expect it -CI_USER_LOGIN = $(shell echo "$$USER") -CI_USER_HOME = $(shell echo "$$HOME") +CI_USER_LOGIN = $(shell whoami) +CI_USER_HOME = $(shell eval echo "~$(CI_USER_LOGIN)") + +# We also need the container process to run with current host IDs +# so that it can access the passed in build directory +CI_UID = $(shell id -u "$(CI_USER_LOGIN)") +CI_GID = $(shell id -g "$(CI_USER_LOGIN)") CI_ENGINE = auto # Container engine we are going to use, can be overridden per make @@ -124,14 +124,16 @@ ifeq ($(CI_ENGINE),podman) CI_UID_OTHER_RANGE = $(shell echo $$(($(CI_MAX_UID)-$(CI_UID)))) CI_GID_OTHER_RANGE = $(shell echo $$(($(CI_MAX_GID)-$(CI_GID)))) - CI_PODMAN_ARGS = \ - --uidmap 0:1:$(CI_UID) \ - --uidmap $(CI_UID):0:1 \ - --uidmap $(CI_UID_OTHER):$(CI_UID_OTHER):$(CI_UID_OTHER_RANGE) \ - --gidmap 0:1:$(CI_GID) \ - --gidmap $(CI_GID):0:1 \ - --gidmap $(CI_GID_OTHER):$(CI_GID_OTHER):$(CI_GID_OTHER_RANGE) \ - $(NULL) + ifneq ($(CI_UID), 0) + CI_PODMAN_ARGS = \ + --uidmap 0:1:$(CI_UID) \ + --uidmap $(CI_UID):0:1 \ + --uidmap $(CI_UID_OTHER):$(CI_UID_OTHER):$(CI_UID_OTHER_RANGE) \ + --gidmap 0:1:$(CI_GID) \ + --gidmap $(CI_GID):0:1 \ + --gidmap $(CI_GID_OTHER):$(CI_GID_OTHER):$(CI_GID_OTHER_RANGE) \ + $(NULL) + endif endif # Args to use when cloning a git repo. @@ -239,6 +241,7 @@ ci-help: @echo " CI_CLEAN=0 - do not delete '$(CI_SCRATCHDIR)' after completion" @echo " CI_REUSE=1 - re-use existing '$(CI_SCRATCHDIR)' content" @echo " CI_ENGINE=auto - container engine to use (podman, docker)" + @echo " CI_USER_LOGIN=$$USER - which user should run in the container" @echo " CI_MESON_ARGS= - extra arguments passed to meson" @echo " CI_NINJA_ARGS= - extra arguments passed to ninja" @echo -- 2.29.2
On Fri, 2021-02-12 at 15:19 +0100, Erik Skultety wrote:
@@ -239,6 +241,7 @@ ci-help: @echo " CI_CLEAN=0 - do not delete '$(CI_SCRATCHDIR)' after completion" @echo " CI_REUSE=1 - re-use existing '$(CI_SCRATCHDIR)' content" @echo " CI_ENGINE=auto - container engine to use (podman, docker)" + @echo " CI_USER_LOGIN=$$USER - which user should run in the container"
Alignment is completely broken here. But I just realized that $USER will get expanded here, so we have basically no way of keeping that consistent... Basically, your original version of the help message was better, please go back to that one :) Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Feb 12, 2021 at 04:04:04PM +0100, Andrea Bolognani wrote:
On Fri, 2021-02-12 at 15:19 +0100, Erik Skultety wrote:
@@ -239,6 +241,7 @@ ci-help: @echo " CI_CLEAN=0 - do not delete '$(CI_SCRATCHDIR)' after completion" @echo " CI_REUSE=1 - re-use existing '$(CI_SCRATCHDIR)' content" @echo " CI_ENGINE=auto - container engine to use (podman, docker)" + @echo " CI_USER_LOGIN=$$USER - which user should run in the container"
Alignment is completely broken here. But I just realized that $USER will get expanded here, so we have basically no way of keeping that consistent...
I didn't want to point it out earlier, since to me it's not a deal breaker :).
Basically, your original version of the help message was better, please go back to that one :)
Sure, will do, thanks for the review. Erik
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
-- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Erik Skultety