Re: [Qemu-devel] [PATCH v1 03/42] tests/docker: fix "cc" command to work with podman

[John Snow]

On 9/5/19 5:51 AM, Alex Bennée wrote:
> 
> John Snow <address@hidden> 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 <address@hidden>
>>> ---
>>>  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 <address@hidden> 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