Test with root access in GitHub workflows #1173
Conversation
There was a problem hiding this comment.
EDIT: GH done me again! this was supposed to come after the other two comments...
A more general question is do we need to do all the configuring and compiling as root?
I know this is just a throwaway environment, but from a principle of least privilege, is it really only the actual pytests that need to be run as root?
.github/workflows/ci.yml
Outdated
|
|
||
| - name: Configure unit | ||
| run: | | ||
| ./configure \ | ||
| --prefix=${{ steps.dir.outputs.prefix }} \ | ||
| --sbindir=${{ steps.dir.outputs.bin }} \ | ||
| --logdir=${{ steps.dir.outputs.var }}/log \ | ||
| --log=${{ steps.dir.outputs.var }}/log/unit/unit.log \ | ||
| --runstatedir=${{ steps.dir.outputs.var }}/run \ | ||
| --pid=${{ steps.dir.outputs.var }}/run/unit/unit.pid \ | ||
| --control=unix:${{ steps.dir.outputs.var }}/run/unit/control.sock \ | ||
| --modules=${{ steps.dir.outputs.prefix }}/lib/unit/modules \ | ||
| --statedir=${{ steps.dir.outputs.var }}/state/unit \ | ||
| sudo ./configure \ | ||
| --tests \ | ||
| --openssl \ | ||
| --njs \ |
There was a problem hiding this comment.
Right, Unit doesn't have to be installed to run the pytests...
There was a problem hiding this comment.
I think these have defaults as well in /usr yes?
| # /home/runner will be root only after calling sudo above | ||
| # Ensure all users and processes can execute | ||
| - name: Fix permissions | ||
| run: | | ||
| sudo chmod -R +x /home/runner | ||
| namei -l ${{ github.workspace }} | ||
|
|
There was a problem hiding this comment.
Heh, you passed the test and didn't just chmod 777 the whole thing!
|
Re: @ac000
This is actually how I started, but, there are issues with the Java and Python tests: for whatever reasons there are issues with requiring Specifically:
|
arbourd
force-pushed
the
privileged
branch
13 times, most recently
from
24fb5f0
to
c48974a
Compare
|
Even if we can do most of it without sudo and only use sudo for the bits that currently require it... |
|
@ac000 All done. I will rebase and clean up the commits before merging. 2nd commit is the specific-sudoing. |
|
I would like to see the final result before merging... |
arbourd
force-pushed
the
privileged
branch
2 times, most recently
from
8d4e67d
to
602e9ec
Compare
|
OK, change looks good. Commit message just needs re-wrapping. We want to wrap commit messages after 72 chars. Keep in mind that when viewing commit messages in git, it will put 4 chars of padding at the beginning of each line, so in a standard 80 char terminal you will get your message centred with 4 chars of padding each side. And seeing as you asked, I've added your Signed-off-by and my Reviewed-by to the message. So it looks like and in git it looks like You'll notice I also made a couple of minor tweaks to the message while I was at it...
|
arbourd
changed the title
To enable tests that require privileged root access, this commit tests with `sudo`. The Java and Python jobs have additional permissions issues, so they are also configured and made with `sudo`. A small permissions fix is required before running tests to allow non-root users to execute within the `/home/runner` directory. This change also removes the custom directories that were required without root access. Reviewed-by: Andrew Clayton <a.clayton@nginx.com> Signed-off-by: Dylan Arbour <d.arbour@f5.com>
To enable tests that require privileged root access, this commit tests with
sudo. The Java and Python jobs have additional permissions issues, so they are also configured and made withsudo.A small permissions fix is required before running tests to allow non-root users to execute within the
/home/runnerdirectory.This change also removes the custom directories that were required without root access.