-
-
Notifications
You must be signed in to change notification settings - Fork 363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Kubernetes | Docker: Add support for rootless images #4151
base: main
Are you sure you want to change the base?
Conversation
well instead of removing it, we should add a check if it was not set and then set it accordingly and HOME should not be WORKDIR in normal cases ... (as tools often create/alter (config-)files in HOME) |
What is "accordingly"? I don't see why we would need to do that - why not let the image decide it?
It isn't anyways? WORKDIR is |
c584fad
to
21ba091
Compare
Deploying preview to https://woodpecker-ci-woodpecker-pr-4151.surge.sh |
@@ -53,6 +53,8 @@ fi | |||
unset CI_NETRC_USERNAME | |||
unset CI_NETRC_PASSWORD | |||
unset CI_SCRIPT | |||
mkdir -p $CI_WORKSPACE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will work for command steps, not sure what plugins will do ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we should extend the git plugin to allow to chown the content ... via setting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my updated OP WRT to the "clone" plugin example.
With the "clone" plugin, the perms are 775 on $CI_WORKSPACE
. Hence, all subsequent operations should work.
Considering the Bitnami's images alone, the statement is too strong (already). |
We won't be able to cover all GIDS, yeah, but I think 1000 is the best possible default? And better than having none. |
⬇️
??? :) I gave you an example of widely used images (IMO, the second ones after official), which will likely be broken by the default settings from this PR. There is a lot other images, that use group other than 1000, I believe. In case of Bitnami, curious person can start from their 9485 issue. Let me share my thoughts. We have the workspace. So, the One of them is
So, prerequisites are:
The third set of permissions is generally referred to as "others". So,
Edit 1: ⬆️ I assumed that In regards volume mount point...
The logic and prerequisites are the same: something should grant I'm sorry for giving non-asked feedback, BTW. |
UIDs are not of interest. The mount point needs to have group
Why would they? AFAIU you can't run non-root bitnami images right now in k8s and execute arbitrary steps due to the apparent limitations. Even when adapting
🤷️ We can only set one default value and we won't be able to cover all non-root variants. A dynamic setting is also not possible AFAICS. Setting a default here is better than none? Do you have a better suggestion?
Why? I don't see that we would need that. And I also would prefer to avoid adding an init container or similar.
In my tests so far, I couldn't find a use case yet for which it doesn't work or for which it would break something else. Still needs more testing of course, but I am optimistic so far, that this is a good solution. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4151 +/- ##
=======================================
Coverage 26.48% 26.48%
=======================================
Files 375 375
Lines 27396 27402 +6
=======================================
+ Hits 7255 7257 +2
- Misses 19477 19480 +3
- Partials 664 665 +1 ☔ View full report in Codecov by Sentry. |
I do not know and lazy to test :) But point was ⬇️
So, you've introduced some options that allow to run any non-root image, right? OK, that is good 👍
Would plugin has access to desired directory?
Seems, yes. |
In some scenarios implicit defaults exists that are honored by a majority. IMO this applies here.
Yes, the PR is breaking. Even though I'd say not many users overwrite
Do you mean by non-root plugins that use a different non-root user? |
Yes, but apparently, it applies to general steps also:
So,
"on volume root" && "workspace" (aka source directory) in case of |
It is (for Scenario:
-> Just tested this and it seems that using any value for So based on all findings so far, defining |
I now verified that NOT setting |
then you are talking about user/group permissions
There are rx permissions for others. I'm lost...
I'm convinced, but have other questions:
|
Not sure what you mean, but we need
WRT to the workspaceBase changes, yes. Everything else is k8s-specific.
The solution aims to be generic and cover The PR does a few things which are all needed together to make non-root images work:
Most changes don't apply to it. The only ones that do, are related to $HOME again. It should I guess, but I can't and won't test it. Not even sure if anyone is using WP on Win? But this is a different discussion. |
Yes
So, others is the last digit => last digit should be 6 or 7.
I was not about HOME in the question 3. Let me cite the docs:
and your code
To recap, there are two solutions:
I retain my first question (Does Docker support fsGroup or something similar?). I'm confused
What mechanism does set write access in Docker backend? Or in Docker non-root steps work without |
Why is Windows in the discussion here? The
I explained this before: this won't work for Using rootless images should ideally just work and not require any additional flags to enable it.
Docker does not have the same volume restrictions k8s has. Running rootless in docker is much easier and should already work after the removal of hard-coded
Yes, docker is different (and simpler but also less secure). But this is a different topic now, I am out of words 🙃️ |
I would extract Home overriding into separate PR: |
@woodpecker-ci/maintainers any comments/review? |
Have to test it yet but my mine concern is permissions also not only for workspace folder but files used in later steps also, how will it work for mixed root/non-root steps |
Root can always write. |
fix #1077
fix #3552
fix #3164
Background
The blocking issue for non-root images in k8s are the volume mount permissions as noted by @6543 in #1077 (comment).
Volumes are by default owned by
root:root
. This can be changed for the group usingsecurityContext.fsGroup
. However, only the mount point of the volume has 775 permissions, i.e. the option for the group to write.WP creates
<mount point>/$CI_WORKSPACE
internally by settingworkingDir
inwoodpecker/pipeline/backend/kubernetes/pod.go
Line 185 in a544836
This directory gets created with
root:<fsgroup>
but with 755, i.e. the group can't write there, even with being set correctly.Solution
Setting
workingDir
toworkspaceBase
, i.e./woodpecker
, and creating<workspaceBase>/src/<repo slug>
via the container user solves this. The subsequently created dir is then owned by the container user and all operations work.To achieve this,
workspaceBase
has been added to theStep
struct and set asworkingDir
during pod creation.Sidework
The hard-coded
$HOME
is an additional issue here. Removing it and letting the container define$HOME
is the most straightforward solution. 99% of all containers have$HOME
defined and I don't see any downside from removing the hard-coded setting, especially as it can't be overwritten withinenvironment:
even.SecurityContext
Most non-root images run with
UID=1000
andGID=1000
. Hence, I argue that we should setfsGroup: 1000
as the default (coupled withfsGroupChangePolicy: "OnRootMismatch"
) and document that users need to change that backend option in case their image user has a different value. This is coupled withfsGroupChangePolicy
, which I added as a new SecurityContext param, but only iffsGroup
is also set.Setting
runAsUser
is not helpful as it would conflict with rootful images. Though, because non-root images are making use of the group write permissions, settingfsGroup
should be enough.Acknowledgements
I learned a lot about
workspaceBase
andworkspacePath
in #2510, thanks @zc-devs and @dominic-p for the discussion!Reprex
yields
Using "clone" plugin