ci: run docker wrapper with a non-root user #25900
Conversation
|
Concept ACK. Fix linter failures? |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
ConflictsNo conflicts as of last run. |
|
Concept ACK. I hope to circle back and do a code review. I tested this PR, and things behave as expected. In addition to doing a spot check on the directories, I made an attempt to tally up all the files that had ownership changes after running the CI ( Number of files that had some kind of change to their read/write/attribute/execute permissions:
For both scenarios I made a fresh directory and clone of the code base. I used I'm completely new to For the 41 file permission changes that were present with this PR, I see that a lot of them are the result of git commands, and |
a852749 to
190b4f3
Compare
|
I reworked this to incorporate feedback from @MarcoFalke: removed the @satsie thanks for providing the testing script! I ran it and confirmed local file permissions were unchanged after running the ci script. |
190b4f3 to
05ce35e
Compare
|
removed to extraneous |
There was a problem hiding this comment.
ACK 05ce35e, I have reviewed the code and it looks OK. Also tested on Ubuntu 22.04: no root-owned files were detected after execution of:
$ make distclean
$ env MAKEJOBS="-j16" FILE_ENV="./ci/test/00_setup_env_win64.sh" ./ci/test_run_all.sh
|
After more tests done, suggesting: --- a/ci/test/04_install.sh
+++ b/ci/test/04_install.sh
@@ -144,7 +144,7 @@ if [[ "${RUN_TIDY}" == "true" ]]; then
CI_EXEC "mkdir -p ${DIR_IWYU}/build/"
CI_EXEC "git clone --depth=1 https://github.com/include-what-you-use/include-what-you-use -b clang_14 ${DIR_IWYU}/include-what-you-use"
CI_EXEC "cd ${DIR_IWYU}/build && cmake -G 'Unix Makefiles' -DCMAKE_PREFIX_PATH=/usr/lib/llvm-14 ../include-what-you-use"
- CI_EXEC "cd ${DIR_IWYU}/build && make install $MAKEJOBS"
+ CI_EXEC_ROOT "cd ${DIR_IWYU}/build && make install $MAKEJOBS"
fi
fi
|
|
ACK 05ce35e - I tested the latest changes by performing the following steps on (1.) the master branch, and (2.) a fresh clone of the repo with this PR. I'm using Ubuntu 22.04, with the new 6.0 Linux kernel:
Per hebasto's comment, realized there is a much, much easier way to check that this PR is working 😅 Instead of the Master branch: This PR: Since you mentioned failing guix builds as a motivator for this PR, and I need the practice, I went ahead and ran a guix build: guix SHA256 sumsAs far as code review goes, I appreciate the effort you put in to get all the commands into the |
|
This is still open: #25900 (comment) |
|
It seems the only changes, which are required to get this nice PR merged, are as follows: diff --git a/ci/test/04_install.sh b/ci/test/04_install.sh
index e83d78d657..99da8e3b00 100755
--- a/ci/test/04_install.sh
+++ b/ci/test/04_install.sh
@@ -144,7 +144,7 @@ if [[ "${RUN_TIDY}" == "true" ]]; then
CI_EXEC "mkdir -p ${DIR_IWYU}/build/"
CI_EXEC "git clone --depth=1 https://github.com/include-what-you-use/include-what-you-use -b clang_14 ${DIR_IWYU}/include-what-you-use"
CI_EXEC "cd ${DIR_IWYU}/build && cmake -G 'Unix Makefiles' -DCMAKE_PREFIX_PATH=/usr/lib/llvm-14 ../include-what-you-use"
- CI_EXEC "cd ${DIR_IWYU}/build && make install $MAKEJOBS"
+ CI_EXEC_ROOT "cd ${DIR_IWYU}/build && make install $MAKEJOBS"
fi
fi
diff --git a/ci/test/05_before_script.sh b/ci/test/05_before_script.sh
index ef3dff86ca..dd2b43d38b 100755
--- a/ci/test/05_before_script.sh
+++ b/ci/test/05_before_script.sh
@@ -11,6 +11,7 @@ if [ "$CI_OS_NAME" == "macos" ]; then
echo > "${HOME}/Library/Application Support/Bitcoin"
else
CI_EXEC echo \> \$HOME/.bitcoin
+ CI_EXEC_ROOT echo \> \$HOME/.bitcoin
fi
CI_EXEC mkdir -p "${DEPENDS_DIR}/SDKs" "${DEPENDS_DIR}/sdk-sources"which in turn address both #25900 (comment) and #25900 (comment). |
Running all commands as the root user in the docker image will change local file permissions in the ci and depends directory. Add a non-root user to the container and use this user whenever possible when running docker exec commands.
05ce35e to
849f20a
Compare
|
hey @hebasto , thanks for the testing and suggested changes! I have added both. @MarcoFalke I verified that there is a changes: |
There was a problem hiding this comment.
ACK 849f20a, tested on Ubuntu 22.04 by running commands as follows:
git clean -xdff
MAKEJOBS="-j8" FILE_ENV="./ci/test/00_setup_env_arm.sh" ./ci/test_run_all.sh
MAKEJOBS="-j8" FILE_ENV="./ci/test/00_setup_env_native_tidy.sh" ./ci/test_run_all.sh
git clean -xdff
| LOCAL_GID=$(id -g) | ||
|
|
||
| # the name isn't important, so long as we use the same UID | ||
| LOCAL_USER=nonroot |
There was a problem hiding this comment.
I don't think this is true. If a different name is used, it will require root permission to create non-default folders.
DIR_QA_ASSETS=/home/new_user/qa_assets_clean CCACHE_DIR=/home/new_user/ccache_dir_clean CCACHE_SIZE=500M MAKEJOBS="-j4" FILE_ENV="./ci/test/00_setup_env_arm.sh" ./ci/test_run_all.sh
fails with:
mkdir: cannot create directory ‘/home/new_user/qa_assets_clean’: Permission denied
Presumably because new_user does not exist in the docker (only outside) and is thus root-owned.
There was a problem hiding this comment.
If the username from the host is used, this will print a warning:
useradd: warning: the home directory /home/new_user already exists.
But I guess this is fine?
There was a problem hiding this comment.
Sorry, wrong alarm. I think your code is all fine. It is not possible to set DIR_QA_ASSETS, because it is not mounted.
Any folder that is mounted (like CCACHE_DIR), will be created before mounting, see the comment # Create folders that are mounted into the docker. Also, the BASE_ROOT_DIR, which may live outside of /home/nonroot in the docker, will be chown'd to nonroot.
So the code should be good as-is. (I'll continue testing a bit)
There was a problem hiding this comment.
thanks for the thorough testing! much appreciated.
|
Did some more testing and (not sure if related to this pull) |
good catch! this is related to this pull. I reverted the commit, ran ci without errors, and then reproduced the error running with the commit. fixed in #26574 |
54dd8f5 ci: use ci_exec_root for clang install (josibake) Pull request description: fixes a bug introduced in #25900 ; see #25900 (comment) the general idea of #25900 was to use a non-root user as much as possible to avoid modifying the user's local filesystem. however, it appears the root user is needed to correctly install clang. ACKs for top commit: hebasto: ACK 54dd8f5, tested on Ubuntu 22.04. Tree-SHA512: beb01d4b6127fbba3c8d18e85cf7ec7d1b2ec93ea05c475ab51bcaa04ef1b0591d886f1a7e0732c5ae86806013f022c0b44027380d2b0cfb1bfdc843e40f99b4
54dd8f5 ci: use ci_exec_root for clang install (josibake) Pull request description: fixes a bug introduced in bitcoin#25900 ; see bitcoin#25900 (comment) the general idea of bitcoin#25900 was to use a non-root user as much as possible to avoid modifying the user's local filesystem. however, it appears the root user is needed to correctly install clang. ACKs for top commit: hebasto: ACK 54dd8f5, tested on Ubuntu 22.04. Tree-SHA512: beb01d4b6127fbba3c8d18e85cf7ec7d1b2ec93ea05c475ab51bcaa04ef1b0591d886f1a7e0732c5ae86806013f022c0b44027380d2b0cfb1bfdc843e40f99b4
Previously, everything in the ci docker image ran as the root user. This would lead to certain directories (
ci/scratch,depends) being owned byrootafter running the ci locally which would lead to annoying behavior such as subsequent guix builds failing due todepends/being owned by root.This PR adds a non-root user in the container and chowns the mounted working directory. All the
docker execcommands now run as the non-root user, except for the few that still need to run as root (mainly, installing packages).To test this I checked out a fresh copy of the repo, applied my changes, ran the CI, and verified all the local file permissions were unchanged after the CI was finished running.