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