| Message ID | 20190904203013.9028-4-alex.bennee@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | current testing/next queue (podman, docker, ci) | expand |
On 9/4/19 4:29 PM, Alex Bennée wrote: > Podman requires a little bit of additional magic to the uid mapping > which was already done for the normal RunCommand. We simplify the > logic by pushing it directly into the Docker::run method to avoid > instantiating an extra Docker() object and ensure the CC command > always runs as the current user. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > tests/docker/docker.py | 30 +++++++++++++++--------------- > tests/tcg/Makefile.include | 2 +- > 2 files changed, 16 insertions(+), 16 deletions(-) > > diff --git a/tests/docker/docker.py b/tests/docker/docker.py > index e23209f71ee..8f391eb278b 100755 > --- a/tests/docker/docker.py > +++ b/tests/docker/docker.py > @@ -318,10 +318,20 @@ class Docker(object): > return False > return checksum == _text_checksum(_dockerfile_preprocess(dockerfile)) > > - def run(self, cmd, keep, quiet): > + def run(self, cmd, keep, quiet, as_user=False): > label = uuid.uuid1().hex > if not keep: > self._instances.append(label) > + > + if as_user: > + uid = os.getuid() > + cmd = [ "-u", str(uid) ] + cmd > + # podman requires a bit more fiddling > + if self._command[0] == "podman": > + cmd = [ "--uidmap", "%d:0:1" % uid, > + "--uidmap", "0:1:%d" % uid, > + "--uidmap", "%d:%d:64536" % (uid + 1, uid + 1)] + cmd > + I was having problems with constructs like these recently. I think we either need to use --userns=keep-id (vastly preferred) or adjust 64536 there to read as "65536 - uid" because not everyone will have a UID of 1000. (My UID is over 20,000 and for whatever reason, this causes podman to crash very badly when using this hackaround.) > ret = self._do_check(["run", "--label", > "com.qemu.instance.uuid=" + label] + cmd, > quiet=quiet) > @@ -364,15 +374,8 @@ class RunCommand(SubCommand): > help="Run container using the current user's uid") > > def run(self, args, argv): > - if args.run_as_current_user: > - uid = os.getuid() > - argv = [ "-u", str(uid) ] + argv > - docker = Docker() > - if docker._command[0] == "podman": > - argv = [ "--uidmap", "%d:0:1" % uid, > - "--uidmap", "0:1:%d" % uid, > - "--uidmap", "%d:%d:64536" % (uid + 1, uid + 1)] + argv > - return Docker().run(argv, args.keep, quiet=args.quiet) > + return Docker().run(argv, args.keep, quiet=args.quiet, > + as_user=args.run_as_current_user) > > > class BuildCommand(SubCommand): > @@ -556,8 +559,6 @@ class CcCommand(SubCommand): > help="The docker image in which to run cc") > parser.add_argument("--cc", default="cc", > help="The compiler executable to call") > - parser.add_argument("--user", > - help="The user-id to run under") > parser.add_argument("--source-path", "-s", nargs="*", dest="paths", > help="""Extra paths to (ro) mount into container for > reading sources""") > @@ -571,11 +572,10 @@ class CcCommand(SubCommand): > if args.paths: > for p in args.paths: > cmd += ["-v", "%s:%s:ro,z" % (p, p)] > - if args.user: > - cmd += ["-u", args.user] > cmd += [args.image, args.cc] > cmd += argv > - return Docker().command("run", cmd, args.quiet) > + return Docker().run(cmd, False, quiet=args.quiet, > + as_user=True) > > > class CheckCommand(SubCommand): > diff --git a/tests/tcg/Makefile.include b/tests/tcg/Makefile.include > index 73b5626fc5f..210f8428237 100644 > --- a/tests/tcg/Makefile.include > +++ b/tests/tcg/Makefile.include > @@ -41,7 +41,7 @@ ifneq ($(DOCKER_IMAGE),) > # We also need the Docker make rules to depend on > include $(SRC_PATH)/tests/docker/Makefile.include > > -DOCKER_COMPILE_CMD="$(DOCKER_SCRIPT) cc --user $(shell id -u) \ > +DOCKER_COMPILE_CMD="$(DOCKER_SCRIPT) cc \ > --cc $(DOCKER_CROSS_COMPILER) \ > -i qemu:$(DOCKER_IMAGE) \ > -s $(SRC_PATH) -- " > -- —js
John Snow <jsnow@redhat.com> writes: > On 9/4/19 4:29 PM, Alex Bennée wrote: >> Podman requires a little bit of additional magic to the uid mapping >> which was already done for the normal RunCommand. We simplify the >> logic by pushing it directly into the Docker::run method to avoid >> instantiating an extra Docker() object and ensure the CC command >> always runs as the current user. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> --- >> tests/docker/docker.py | 30 +++++++++++++++--------------- >> tests/tcg/Makefile.include | 2 +- >> 2 files changed, 16 insertions(+), 16 deletions(-) >> >> diff --git a/tests/docker/docker.py b/tests/docker/docker.py >> index e23209f71ee..8f391eb278b 100755 >> --- a/tests/docker/docker.py >> +++ b/tests/docker/docker.py >> @@ -318,10 +318,20 @@ class Docker(object): >> return False >> return checksum == _text_checksum(_dockerfile_preprocess(dockerfile)) >> >> - def run(self, cmd, keep, quiet): >> + def run(self, cmd, keep, quiet, as_user=False): >> label = uuid.uuid1().hex >> if not keep: >> self._instances.append(label) >> + >> + if as_user: >> + uid = os.getuid() >> + cmd = [ "-u", str(uid) ] + cmd >> + # podman requires a bit more fiddling >> + if self._command[0] == "podman": >> + cmd = [ "--uidmap", "%d:0:1" % uid, >> + "--uidmap", "0:1:%d" % uid, >> + "--uidmap", "%d:%d:64536" % (uid + 1, uid + 1)] + cmd >> + > > I was having problems with constructs like these recently. I think we > either need to use --userns=keep-id (vastly preferred) or adjust 64536 > there to read as "65536 - uid" because not everyone will have a UID of > 1000. From Marc-André's original commit: With a user 1000, the default mapping is: 1000 (host) -> 0 (container). So write access to /var/tmp/ccache ends will end with permission denied error. With "--uidmap 1000:0:1 --uidmap 0:1:1000", the mapping is: 1000 (host) -> 0 (container, 1st namespace) -> 1000 (container, 2nd namespace). (the rest is mumbo jumbo to avoid holes in the range of UIDs) A future podman version may have an option such as --userns-keep-uid. Thanks to Debarshi Ray <rishi@redhat.com> for the help! So I assumed this doesn't exist for all versions of podman yet. Given how new the support is I guess we could just say you need a minimum version for working podman support. > > (My UID is over 20,000 and for whatever reason, this causes podman to > crash very badly when using this hackaround.) > >> ret = self._do_check(["run", "--label", >> "com.qemu.instance.uuid=" + label] + cmd, >> quiet=quiet) >> @@ -364,15 +374,8 @@ class RunCommand(SubCommand): >> help="Run container using the current user's uid") >> >> def run(self, args, argv): >> - if args.run_as_current_user: >> - uid = os.getuid() >> - argv = [ "-u", str(uid) ] + argv >> - docker = Docker() >> - if docker._command[0] == "podman": >> - argv = [ "--uidmap", "%d:0:1" % uid, >> - "--uidmap", "0:1:%d" % uid, >> - "--uidmap", "%d:%d:64536" % (uid + 1, uid + 1)] + argv >> - return Docker().run(argv, args.keep, quiet=args.quiet) >> + return Docker().run(argv, args.keep, quiet=args.quiet, >> + as_user=args.run_as_current_user) >> >> >> class BuildCommand(SubCommand): >> @@ -556,8 +559,6 @@ class CcCommand(SubCommand): >> help="The docker image in which to run cc") >> parser.add_argument("--cc", default="cc", >> help="The compiler executable to call") >> - parser.add_argument("--user", >> - help="The user-id to run under") >> parser.add_argument("--source-path", "-s", nargs="*", dest="paths", >> help="""Extra paths to (ro) mount into container for >> reading sources""") >> @@ -571,11 +572,10 @@ class CcCommand(SubCommand): >> if args.paths: >> for p in args.paths: >> cmd += ["-v", "%s:%s:ro,z" % (p, p)] >> - if args.user: >> - cmd += ["-u", args.user] >> cmd += [args.image, args.cc] >> cmd += argv >> - return Docker().command("run", cmd, args.quiet) >> + return Docker().run(cmd, False, quiet=args.quiet, >> + as_user=True) >> >> >> class CheckCommand(SubCommand): >> diff --git a/tests/tcg/Makefile.include b/tests/tcg/Makefile.include >> index 73b5626fc5f..210f8428237 100644 >> --- a/tests/tcg/Makefile.include >> +++ b/tests/tcg/Makefile.include >> @@ -41,7 +41,7 @@ ifneq ($(DOCKER_IMAGE),) >> # We also need the Docker make rules to depend on >> include $(SRC_PATH)/tests/docker/Makefile.include >> >> -DOCKER_COMPILE_CMD="$(DOCKER_SCRIPT) cc --user $(shell id -u) \ >> +DOCKER_COMPILE_CMD="$(DOCKER_SCRIPT) cc \ >> --cc $(DOCKER_CROSS_COMPILER) \ >> -i qemu:$(DOCKER_IMAGE) \ >> -s $(SRC_PATH) -- " >> -- Alex Bennée
On 9/5/19 5:51 AM, Alex Bennée wrote: > > John Snow <jsnow@redhat.com> writes: > >> On 9/4/19 4:29 PM, Alex Bennée wrote: >>> Podman requires a little bit of additional magic to the uid mapping >>> which was already done for the normal RunCommand. We simplify the >>> logic by pushing it directly into the Docker::run method to avoid >>> instantiating an extra Docker() object and ensure the CC command >>> always runs as the current user. >>> >>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>> --- >>> tests/docker/docker.py | 30 +++++++++++++++--------------- >>> tests/tcg/Makefile.include | 2 +- >>> 2 files changed, 16 insertions(+), 16 deletions(-) >>> >>> diff --git a/tests/docker/docker.py b/tests/docker/docker.py >>> index e23209f71ee..8f391eb278b 100755 >>> --- a/tests/docker/docker.py >>> +++ b/tests/docker/docker.py >>> @@ -318,10 +318,20 @@ class Docker(object): >>> return False >>> return checksum == _text_checksum(_dockerfile_preprocess(dockerfile)) >>> >>> - def run(self, cmd, keep, quiet): >>> + def run(self, cmd, keep, quiet, as_user=False): >>> label = uuid.uuid1().hex >>> if not keep: >>> self._instances.append(label) >>> + >>> + if as_user: >>> + uid = os.getuid() >>> + cmd = [ "-u", str(uid) ] + cmd >>> + # podman requires a bit more fiddling >>> + if self._command[0] == "podman": >>> + cmd = [ "--uidmap", "%d:0:1" % uid, >>> + "--uidmap", "0:1:%d" % uid, >>> + "--uidmap", "%d:%d:64536" % (uid + 1, uid + 1)] + cmd >>> + >> >> I was having problems with constructs like these recently. I think we >> either need to use --userns=keep-id (vastly preferred) or adjust 64536 >> there to read as "65536 - uid" because not everyone will have a UID of >> 1000. > > From Marc-André's original commit: > > With a user 1000, the default mapping is: 1000 (host) -> 0 (container). > > So write access to /var/tmp/ccache ends will end with permission > denied error. > > With "--uidmap 1000:0:1 --uidmap 0:1:1000", the mapping is: > 1000 (host) -> 0 (container, 1st namespace) -> 1000 (container, 2nd namespace). > (the rest is mumbo jumbo to avoid holes in the range of UIDs) > > A future podman version may have an option such as --userns-keep-uid. > Thanks to Debarshi Ray <rishi@redhat.com> for the help! > > So I assumed this doesn't exist for all versions of podman yet. Given > how new the support is I guess we could just say you need a minimum > version for working podman support. > I think that's probably fine to say. Matt Heon says that 1.4.x should be available in RHEL7 and RHEL8 both, and it's available in Fedora 30, so it should be reasonably well represented on modern development machines. It's also entirely optional as you may continue using docker if you wish. Thanks for staging the patch to fix this; I'll try to test it out in conjunction with your patchset here later when time permits. --js
diff --git a/tests/docker/docker.py b/tests/docker/docker.py index e23209f71ee..8f391eb278b 100755 --- a/tests/docker/docker.py +++ b/tests/docker/docker.py @@ -318,10 +318,20 @@ class Docker(object): return False return checksum == _text_checksum(_dockerfile_preprocess(dockerfile)) - def run(self, cmd, keep, quiet): + def run(self, cmd, keep, quiet, as_user=False): label = uuid.uuid1().hex if not keep: self._instances.append(label) + + if as_user: + uid = os.getuid() + cmd = [ "-u", str(uid) ] + cmd + # podman requires a bit more fiddling + if self._command[0] == "podman": + cmd = [ "--uidmap", "%d:0:1" % uid, + "--uidmap", "0:1:%d" % uid, + "--uidmap", "%d:%d:64536" % (uid + 1, uid + 1)] + cmd + ret = self._do_check(["run", "--label", "com.qemu.instance.uuid=" + label] + cmd, quiet=quiet) @@ -364,15 +374,8 @@ class RunCommand(SubCommand): help="Run container using the current user's uid") def run(self, args, argv): - if args.run_as_current_user: - uid = os.getuid() - argv = [ "-u", str(uid) ] + argv - docker = Docker() - if docker._command[0] == "podman": - argv = [ "--uidmap", "%d:0:1" % uid, - "--uidmap", "0:1:%d" % uid, - "--uidmap", "%d:%d:64536" % (uid + 1, uid + 1)] + argv - return Docker().run(argv, args.keep, quiet=args.quiet) + return Docker().run(argv, args.keep, quiet=args.quiet, + as_user=args.run_as_current_user) class BuildCommand(SubCommand): @@ -556,8 +559,6 @@ class CcCommand(SubCommand): help="The docker image in which to run cc") parser.add_argument("--cc", default="cc", help="The compiler executable to call") - parser.add_argument("--user", - help="The user-id to run under") parser.add_argument("--source-path", "-s", nargs="*", dest="paths", help="""Extra paths to (ro) mount into container for reading sources""") @@ -571,11 +572,10 @@ class CcCommand(SubCommand): if args.paths: for p in args.paths: cmd += ["-v", "%s:%s:ro,z" % (p, p)] - if args.user: - cmd += ["-u", args.user] cmd += [args.image, args.cc] cmd += argv - return Docker().command("run", cmd, args.quiet) + return Docker().run(cmd, False, quiet=args.quiet, + as_user=True) class CheckCommand(SubCommand): diff --git a/tests/tcg/Makefile.include b/tests/tcg/Makefile.include index 73b5626fc5f..210f8428237 100644 --- a/tests/tcg/Makefile.include +++ b/tests/tcg/Makefile.include @@ -41,7 +41,7 @@ ifneq ($(DOCKER_IMAGE),) # We also need the Docker make rules to depend on include $(SRC_PATH)/tests/docker/Makefile.include -DOCKER_COMPILE_CMD="$(DOCKER_SCRIPT) cc --user $(shell id -u) \ +DOCKER_COMPILE_CMD="$(DOCKER_SCRIPT) cc \ --cc $(DOCKER_CROSS_COMPILER) \ -i qemu:$(DOCKER_IMAGE) \ -s $(SRC_PATH) -- "
Podman requires a little bit of additional magic to the uid mapping which was already done for the normal RunCommand. We simplify the logic by pushing it directly into the Docker::run method to avoid instantiating an extra Docker() object and ensure the CC command always runs as the current user. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- tests/docker/docker.py | 30 +++++++++++++++--------------- tests/tcg/Makefile.include | 2 +- 2 files changed, 16 insertions(+), 16 deletions(-) -- 2.20.1