Remove user from docker run command #57
Conversation
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
|
Compared to #25 I can't see user set when |
|
Good catch, I'll get that added. |
|
@ndeloof i added the --user change. i had some troubles verifying that it is actually doing what I expected (I couldn't directly see the docker exec command) but it seems to pass in the demo container and against some of my existing jenkins jobs that use the plugin. I performed an 'id' in the sh step and it was indeed the jenkins user (in my test instance, in the demo container the jenkins user is root). I tried to figure out how to make the 'docker exec' command it was running print out to the console log like the docker run command but was unsuccessful at figuring out how to do that. This may be a long-term thing (and probably a mailing list discussion), but I would like to see/determine if the core logic of this plugin and the Docker Custom Build Environment plugin can be merged so the results are consistent between the two. I currently use both and the behavior between these two are similar but different enough to make it confusing. |
|
LGTM @antoniobeyah I would have expected the docker custom build environment plugin to provide |
|
@ndeloof thanks for the review, are there any additional required reviewers or can you merge it? I don't have merge rights or else I would based on your approval. I just want to get it off my plate as one less thing to check-in/monitor status of before someone else introduces some conflicting changes to master. ;) |
|
@antoniobeyah need to wait for plugin maintainer (@jglick) |
| DockerFingerprints.addRunFacet(dockerClient.getContainerRecord(env, container), run); | ||
| ImageAction.add(step.image, run); | ||
| getContext().newBodyInvoker(). | ||
| withContext(BodyInvoker.mergeLauncherDecorators(getContext().get(LauncherDecorator.class), new Decorator(container, envHost, ws))). | ||
| withContext(BodyInvoker.mergeLauncherDecorators(getContext().get(LauncherDecorator.class), new Decorator(container, envHost, ws, dockerClient.whoAmI()))). |
There was a problem hiding this comment.
For safety, run whoAmI before starting the container, since if it fails we cannot clean up.
|
So is this purporting to address JENKINS-34289 or not? And is there a self-contained demo case that can be used to prove that this change fixes some concrete bug? What I am missing from all this is a clear description of what problem is being solved. |
| List<String> prefix = new ArrayList<String>(Arrays.asList("docker", "exec", container, "env")); | ||
| List<String> prefix; | ||
| if (userId == null) { | ||
| prefix = new ArrayList<String>(Arrays.asList("docker", "exec", container, "env")); |
There was a problem hiding this comment.
Good, this is necessary for compatibility with builds running before the upgrade.
This sounds like a separate issue that would better be fixed in its own pull request. (If basic testing of this PR requires that fix, just |
This is an example of something you have apparently tested interactively but which is not reflected in an automated test, or even written manual test instructions. |
Given how fundamentally different Pipeline asynchronous (durable) process execution is from freestyle, I do not think this is practical. |
|
@jglick both rely on |
|
@jglick JENKINS-34289 was marked as resolved so I ignored it for the purposes of this PR. Point taken on the clear explanation on why this is needed, I will try to get a test case written up that shows the failure. I have a limited amount of time I can spend on this- we are running on a forked version at Target that has the changes in it (not ideal). Are you able to provide a bulleted list of items that you require for a merge? So far I have gathered:
Are there any additional changes you would require? |
Well, desirable practice generally, so that the obvious bug fix can get to users while we are discussing the main part of this PR. Can be skipped if you do not have time.
Yes, manual test steps if it is impractical to write an automated test (for example because you need to be using a specific user ID on the host).
It is fine,
Right, safer.
No, the test—or just some demonstration that this is fixing a particular bug I can see for myself—is the main thing. |
|
I do not understand why removing the user constraints put on a container for @carlossg pointed me here from carlossg/docker-maven#16, and yes this would fix the problem but at the cost of a good deal more downstream breakage. What this PR purports to fix, that being #25, seems of dubious value to me. I think it would make more sense to leave the default behavior of specifying the user as-is unless the image specified for |
|
@dweomer The constraint isn't being removed, it is being moved to the exec command. This will allow the container to start up as expected, without unintended side effects. I've been slow on getting this pr updated with the requests, but this is our use case: We use docker images for CI testing of RPM builds. The RPMs happen to use systemd. That means entrypoint is 'init' which must be run as root, otherwise the container blows up. All subsequent commands are executed as the jenkins user, and that is supported by 'docker exec --user '. The container behaves very different when it is started with 'docker run --user' vs 'docker exec --user'. You are unable to do certain things when --user is used on the 'docker run', such as using sudo to install any required packages using yum, or to get the container to start at all. This behavior is already supported by other plugins, such as Docker Custom Build Environment plugin. We are running a forked version of the docker-pipeline plugin without the docker run --user constraint and it is working great. This PR only allows for the possibility of using additional containers without the need of custom tailoring them to be used within Jenkins. I would also be for using a different mechanism for determining the user to run .inside as, but as it stands it works 'well enough' with the changes in this PR. @dweomer Will this PR break your environment somehow? If so I may be able to add in some checks to make that less likely, just need some specific details on your concerns. @jglick As far as the automated test- I don't think I will be able to write one due to the reasons you provided (the user in the host machine is root, my test requires it to be a non-root user). Can you provide more details on how specifically I can provide manual verification steps? |
Well, something I can run locally (perhaps inside VirtualBox if host permissions matter) that would let me understand the problem for myself and evaluate how the proposed fix works. Minimally, a demo Pipeline script relying only on images on Docker Hub (can use |
If there is any concern about this, I would rather make the new behavior opt-in. |
|
@antoniobeyah wrote:
In my ire related to carlossg/docker-maven#16, I had forgotten about the run/exec distinction. This should not break environments but I feel that you should ensure that by implementing a test case such as:
|
|
@antoniobeyah @jglick: In short, defaulting to the image's |
|
@jglick it might be better to add |
|
Is this PR going anywhere? |
|
doing a sweep of older PRs and it looks like this one is no longer live, and there are sufficient work arounds - can we close this? |
|
@michaelneale I still think we need this one, especially if we restore |
|
@ndeloof @antoniobeyah want to bring this one up to date then? |
|
All the discussion here has been with the Scripted Pipeline. Declarative pipeline is also affected. For example, to make the Python image work, you need to add Example: agent {
docker {
image 'python:2'
args '--user root:root'
}
}I find this rather unexpected and confusing behavior when commands will work directly if using the container on CLI but don't under Jenkins. It has taken me quite a while (~a month) to figure out what is going on, find this issue, and know how to make all builds consistently work. My gut reaction is to agree that Jenkins should use the user from the container unless specified otherwise. This would make the naive declarative Jenkinsfile just work with images pulled from Dockerhub and those wanting different behavior could still specify so. As such I would like to see this go forward, but based on the discussion above (and the |
|
The problem of the Jenkins uid/gid outside the container not matching anything in |
|
By my opinion, default behaviour of running docker images from jenkins pipeline must be the same, what you expect and what you get, when running same docker images on your local workspace. |
|
Never mind. I see that this has been marked as wontfix. |
|
Hey guys. Why is this one 'wontfix'? Is it a desired behaviour? What's the purpose if yes? Best, |
|
@michaelneale what are the "sufficient workarounds"? |
|
Should be closed, risk of regression is too high. The workaround is to simply not use |
|
This is my current code, not sure how it can be replaced with Edit: frankly speaking, it was a long time since I looked at this issue and code and I'm quite hesitant to rewrite it while it works. But maybe this example will provide more input before closing the issue as wont-fix or something. |
|
Using Unless you are suggesting we check out the source code inside the docker container, because doing a bind mount (which is what |
|
There is no universal advice, but if you need access to files from |
This is a horrible workaround: given in other cases workspace is shared automatically - doing something manually anywhere is a recipe for a disaster. |
|
Copying large source trees into a Yet, in most cases I'll need the source code checked out to be able to use the included |
|
This bug still affects declarative pipelines and essentially makes using a docker agent not possible. What would be the workaround if you are not using scripted pipelines? |
|
There is https://issues.jenkins.io/browse/JENKINS-63233. In a rootless podman container you are root insider the container, but jenkins outside. So the workspace file permission issue doesn't exist. I guess the same applies to https://docs.docker.com/engine/security/rootless/. But you may have less control over the user the Docker daemon is running as while with podman, being daemon-less, it just runs as the user calling it (i.e. jenkins). |
|
I'm using simple declarative pipelines like this pipeline {
agent {
dockerfile true
}
stages {
...
}
}or like this: pipeline {
agent {
docker {
image "some-predefined-docker-image"
}
}
stages {
...
}
}as described in https://www.jenkins.io/doc/book/pipeline/docker/ However, due to this issue I need to add the following to all my RUN groupadd --gid 1000 jenkins && \
useradd --create-home --uid 1000 --gid 1000 --groups sudo jenkins && \
echo "jenkins ALL=(ALL) NOPASSWD: ALL" >> /etc/sudoersWhich breaks as soon as I run Jenkins on a machine where the user |
|
Hi! Is there any progress with this MR? I've just been hit by issue with unknow uid in container when run via jenkins. I would also appreciate if jenkins documentation would mention that jenkins is overriding uid in my containers. I'll try out workarounf from previous post for now |
|
My PR #160 try to resolve that issue. The plugins do the following: The docker behaviour is that, the commands run with exec, without any override will be executed with the same user specifed at docker run: https://stackoverflow.com/questions/35734474/connect-to-docker-container-as-user-other-than-root In order to use an other user, in the pipeline, he can add another "-u user:user" parameter in order to override the default plugin behaviour. The issue is in that case all the next exec commands will be also executed with that user, which for most the case is not wanted. |
|
Before I was shown this issue, I had a workaround documented here - Wondering if there's a better solution to my hacky approach or if there's a better solution coming from this PR? Any updates since last year? |
|
#57 (comment) though BuildX now supports |
|
For the love of xyz, document this behavior...if you're not going to merge this. Do the example code in this documentation even work as-is (without setting the |
Yes. |
Fixes #25 , removes the user from the docker run command. The changes from #25 were merged in and tested against the demo container. I also updated the Dockerfile in the demo in preparation of the release to support the new required minimum version (Docker client 1.7.0). During testing, I found a bug in master with docker client 1.7.0, the bug was introduced in this commit. The bug is that the redirection is causing the exit status to be 1, when it should be 0 in some instances.
To workaround that, I removed the output redirection which restored previous behavior. See the full commit message for the result of each command. Here is the result of testing the return value of the command (inside the demo container):
This is the error that appeared in the Jenkins job: