[libvirt PATCH 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 d= oes nothing really. Erik Skultety (4): ci: Specify the shebang sequence for build.sh ci: Run podman command directly without wrapping it with prepare.sh ci: Expose CI_USER_LOGIN as a Makefile variable for users to use ci: Drop the prepare.sh script ci/Makefile | 43 ++++++++++++++++++++++++------------------- ci/build.sh | 2 ++ ci/prepare.sh | 13 ------------- 3 files changed, 26 insertions(+), 32 deletions(-) delete mode 100644 ci/prepare.sh --=20 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> --- 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
On Wed, 2021-02-10 at 18:00 +0100, Erik Skultety wrote:
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> --- ci/build.sh | 2 ++ 1 file changed, 2 insertions(+)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
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_ARGS 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> --- ci/Makefile | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/ci/Makefile b/ci/Makefile index 7938e14c15..1a376a7f0c 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
On Wed, 2021-02-10 at 18:00 +0100, Erik Skultety wrote:
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_ARGS env variable doesn't need to propagated to the execution
s/MESON_ARGS/MESON_OPTS/
@@ -158,6 +159,11 @@ CI_ENGINE_ARGS = \ --rm \ --interactive \ --tty \ + --user $(CI_UID):$(CI_GID) \ + --workdir $(CI_USER_HOME) \
Please add quotes around the arguments here. Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
More often than not I find myself debugging 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 | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/ci/Makefile b/ci/Makefile index 1a376a7f0c..84f2f77526 100644 --- a/ci/Makefile +++ b/ci/Makefile @@ -47,13 +47,13 @@ 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) +CI_UID = $(shell id -u $(CI_USER_LOGIN)) +CI_GID = $(shell id -g $(CI_USER_LOGIN)) # We also 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)") CI_ENGINE = auto # Container engine we are going to use, can be overridden per make @@ -132,6 +132,13 @@ ifeq ($(CI_ENGINE),podman) --gidmap $(CI_GID):0:1 \ --gidmap $(CI_GID_OTHER):$(CI_GID_OTHER):$(CI_GID_OTHER_RANGE) \ $(NULL) + + # In case we want to debug in the container, having root is actually + # preferable, so reset the CI_PODMAN_ARGS and don't actually perform + # any uid/gid mapping + ifeq ($(CI_UID), 0) + CI_PODMAN_ARGS= + endif endif # Args to use when cloning a git repo. @@ -238,6 +245,7 @@ ci-help: @echo @echo " CI_CLEAN=0 - do not delete '$(CI_SCRATCHDIR)' after completion" @echo " CI_REUSE=1 - re-use existing '$(CI_SCRATCHDIR)' content" + @echo " CI_USER_LOGIN= - which user should run in the container (default is $$USER)" @echo " CI_ENGINE=auto - container engine to use (podman, docker)" @echo " CI_MESON_ARGS= - extra arguments passed to meson" @echo " CI_NINJA_ARGS= - extra arguments passed to ninja" -- 2.29.2
On Wed, 2021-02-10 at 18:00 +0100, Erik Skultety wrote:
# 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) +CI_UID = $(shell id -u $(CI_USER_LOGIN)) +CI_GID = $(shell id -g $(CI_USER_LOGIN))
Please quote uses of $(CI_USER_LOGIN) in the shell. Also, since you're using the variable here, it would be IMHO cleaner if you moved the block that contains its definition before this one.
# We also 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)")
This use of eval makes me a bit uncomfortable. Can we do CI_USER_HOME = $(shell getent passwd "$(CI_USER_LOGIN)" | cut -d: -f6) instead?
+ # In case we want to debug in the container, having root is actually + # preferable, so reset the CI_PODMAN_ARGS and don't actually perform + # any uid/gid mapping + ifeq ($(CI_UID), 0) + CI_PODMAN_ARGS= + endif
Setting $(CI_PODMAN_ARGS) only to reset it moments later seems worse than just doing ifneq ($(CI_UID, 0) CI_PODMAN_ARGS = \ ... $(NULL) endif The comment is also somewhat misleading: whether or not you're going to debug in the container, whatever that means, is not really relevant - the point is simply that performing these mappings is only necessary when the container process is running as non-root.
@echo " CI_CLEAN=0 - do not delete '$(CI_SCRATCHDIR)' after completion" @echo " CI_REUSE=1 - re-use existing '$(CI_SCRATCHDIR)' content" + @echo " CI_USER_LOGIN= - which user should run in the container (default is $$USER)" @echo " CI_ENGINE=auto - container engine to use (podman, docker)" @echo " CI_MESON_ARGS= - extra arguments passed to meson" @echo " CI_NINJA_ARGS= - extra arguments passed to ninja"
We document the default, so this should be CI_USER_LOGIN=$$USER - which user should run in the container And please document this after CI_ENGINE. -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Feb 12, 2021 at 01:27:42PM +0100, Andrea Bolognani wrote:
On Wed, 2021-02-10 at 18:00 +0100, Erik Skultety wrote:
# 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) +CI_UID = $(shell id -u $(CI_USER_LOGIN)) +CI_GID = $(shell id -g $(CI_USER_LOGIN))
Please quote uses of $(CI_USER_LOGIN) in the shell.
Also, since you're using the variable here, it would be IMHO cleaner if you moved the block that contains its definition before this one.
Sure I can do that, I just didn't feel like moving around code to achieve the same thing in a declarative language.
# We also 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)")
This use of eval makes me a bit uncomfortable. Can we do
Can you elaborate what the problem is? The argument is even quoted so I sincerely don't see a problem and find it much clearer than what you suggest. Erik
On Fri, 2021-02-12 at 13:41 +0100, Erik Skultety wrote:
On Fri, Feb 12, 2021 at 01:27:42PM +0100, Andrea Bolognani wrote:
On Wed, 2021-02-10 at 18:00 +0100, Erik Skultety wrote:
# We also 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)")
This use of eval makes me a bit uncomfortable. Can we do
Can you elaborate what the problem is? The argument is even quoted so I sincerely don't see a problem and find it much clearer than what you suggest.
It's just a code smell. In general, I prefer straightforward constructs to "fancy" ones, especially in languages where quoting and the like matter so much. But I agree with you that it's safe in this specific scenario, so if you'd rather keep it this way I won't NACK the patch because of that. -- Andrea Bolognani / Red Hat / Virtualization
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> --- 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
On Wed, 2021-02-10 at 18:00 +0100, Erik Skultety wrote:
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> --- ci/prepare.sh | 13 ------------- 1 file changed, 13 deletions(-) delete mode 100644 ci/prepare.sh
This makes IMHO more sense either as 3/4 or even squashed directly into 2/4. Either way, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Erik Skultety