cannot run west update if manifest does not belong to user
nvincent-vossloh opened this issue · 10 comments
Hello,
I'm using a container image with the tools to build a zephyr application from github action (https://github.com/SiemaApplications/zephyr-action/)
I use this action alongside actions/cache.
I (recently) noticed that when running west update (or other commands parsing the manifest) the command fails with the following message:
root@30c604986fc5:/src# west list
Traceback (most recent call last):
File "/usr/local/bin/west", line 8, in <module>
sys.exit(main())
File "/usr/local/lib/python3.8/dist-packages/west/app/main.py", line 786, in main
app.run(argv or sys.argv[1:])
File "/usr/local/lib/python3.8/dist-packages/west/app/main.py", line 109, in run
self.run_command(argv)
File "/usr/local/lib/python3.8/dist-packages/west/app/main.py", line 337, in run_command
self.handle_builtin_manifest_load_err(args)
File "/usr/local/lib/python3.8/dist-packages/west/app/main.py", line 195, in handle_builtin_manifest_load_err
p, f = self.mle.project, self.mle.filename
AttributeError: 'ManifestImportFailed' object has no attribute 'filename'
I tracked the issue, and here is what I found:
- does not happen with
West version: v0.12.0 - happens with
West version: v0.13.1 - happens if user running
west listcommand does not own the manifest.
In my case, when running github action, the cache files (mainly west modules, including zephyr) restored belongs to docker user (id 1001), however in my container image I am running with the user root (id 0).
If i launch my container manually with -u $(id -u):$(id -g) then everything belongs to the user running the west list command and that works fine.
I don't really know if this behavior is intended or not.
As far as I am concerned, I don't really like the idea of having part of the files belonging to a user (docker) and another part belonging to another one (root) so I'll run a pre-entrypoint so the user running west from the container will own everything.
Best regards,
Nicolas.
In my case, when running github action, the cache files (mainly west modules, including zephyr) restored belongs to docker user (id 1001), however in my container image I am running with the user root (id 0).
File ownership is an extremely common issue with Docker, see https://stackoverflow.com/questions/35291520/docker-and-userns-remap-how-to-manage-volume-permissions-to-share-data-betwee + many, many others.
As a coincidence I ran into a similar issue recently: thesofproject/sof#5730. I had to drop the chown / chgrp approach.
Note even a simple git diff requires write permission (to refresh the index): https://public-inbox.org/git/20160331140515.GA31116@sigill.intra.peff.net/
In other words, git diff appears to be a read-only operation but it's not; git diff can be updating some files. west is just a wrapper on top of git so nothing it can do here.
Git now requires ownership of the repository for security reasons too: https://github.blog/2022-04-12-git-security-vulnerability-announced/
As far as I am concerned, I don't really like the idea of having part of the files belonging to a user (docker) and another part belonging to another one (root)
Agreed 200%, this is an absolute disaster. I'm afraid the recommended Docker practice is to simply NOT share filesystems between host and container.
so I'll run a pre-entrypoint so the user running west from the container will own everything.
Can you provide more details about this pre-entrypoint?
EDIT: Github just added a link to https://github.com/SiemaApplications/zephyr-action/commit/de5a459510b793494e874f3b8b85bdd877de175f
File ownership is an extremely common issue with Docker
Totally agree, when working locally with docker I always try to pass -u on the docker run command line to be sure that file created from the docker environment belongs to user running docker. However this approach does not seem to be possible when running github actions step with docker container where it is not possible to set the -u option for a single step (as far as i know).
Can you provide more details about this pre-entrypoint?
In the end I had other isues due to the pre-entrypoint doing a chown because a step outside of SiemaApplications/zephyr-action would try to write into a folder now owned by root and my action would fail...
Totally agree, when working locally with docker I always try to pass -u on the docker run command line to be sure that file created from the docker environment belongs to user running docker.
Please correct me but this works only when there is actually a user in the container that exists with a UID that matches the UID outside. I believe this lack of matching UID inside the container is the most common and mostly unsolved issue with Linux containers.
Yes, when I build the image, I pass the UID/GID of the user to create and pass those same UID/GID in the docker run. However this solution is not portable to another user and/or marchine.
Regarding this issue, I finally updated my workflow to use a custom container rather than github's default with west and zephyr toolchain installed. Now every steps are run with the same UID/GID and I'm not bothered by the initial error mentionned.
I don't know if you wish to close this issue or keep it open in order to improve the error message.
rather than github's default with west and zephyr toolchain installed.
You mean instead of https://github.com/zephyrproject-rtos/docker-image ? Just curious.
I don't know if you wish to close this issue or keep it open in order to improve the error message.
I'll give @mbolivar-nordic a chance to think of something but I'm afraid mixed file ownership means git and/or west could fail at many random places.
You mean instead of https://github.com/zephyrproject-rtos/docker-image ? Just curious.
I meant that, rather than using github's default env in which I would run a step invoking a container where I have the various tools installed, now I specify my own container for the entire workflow, in that container I have the various tools I need (west, zephyr SDK, ...)
I did not go with https://github.com/zephyrproject-rtos/docker-image because the image is too big for what I need, I don't really care for the compilers other than arm and I don't want my actions to waste minutes on downloading an image for which I will only use a fraction of it.
The repository running the workflow action is private, developers forked the main private repo, so that means it is their user account which is billed for actions triggered on push events on their fork. So in the case an employee choose to use his personnal github account for work that means he may run out of minutes for his own private repos (that never happened though).
No matter what, west list should definitely not be bombing out with an exception like that. We'll need to fix that at least. I will need to think about the larger issue about ownership, though.
Yes, when I build the image, I pass the UID/GID of the user to create and pass those same UID/GID in the docker run. However this solution is not portable to another user and/or marchine.
We're not alone: https://lwn.net/Articles/896255/ ID-mapped mounts
I think #591 is a fix.
The basic idea is that if we try to load the manifest, but we cannot due to a permissions error, then we will proceed as long as the current west command does not require the manifest in order to do its work. So, for example, west config would still run. However, if the manifest is required for the command to do anything meaningful, we now print FATAL ERROR: permission denied: <foo.yml> for the file which failed to load due to permissions errors.
@nvincent-vossloh this at least addresses the stack bombing out (this was actually also partially addressed by a9ce810), and hopefully is a good compromise for other commands, but please let me know if I'm missing something.
This is looking good on my side, thanks.