Skip to content
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

Support prefixing Docker build and push commands with 'sudo'. #903

Closed
wants to merge 2 commits into from
Closed

Support prefixing Docker build and push commands with 'sudo'. #903

wants to merge 2 commits into from

Conversation

rbellamy
Copy link

@rbellamy rbellamy commented Nov 3, 2016

When building docker images on CentOS, Fedora or RHEL, the permissions on /var/run/docker.socket have restricted access, preventing Docker build and push commands from executing without severely compromising the system.

See http://www.projectatomic.io/blog/2015/08/why-we-dont-let-non-root-users-run-docker-in-centos-fedora-or-rhel/

Therefore, I've added a new setting which supports prefixing both the Docker build and push commands with 'sudo', which is the recommended method for executing Docker on CentOS, Fedora and RHEL.

When building docker images on CentOS, Fedora or RHEL, the permissions on /var/run/docker.socket have
restricted access, preventing Docker build and push commands from executing without severely compromising
the system.

See http://www.projectatomic.io/blog/2015/08/why-we-dont-let-non-root-users-run-docker-in-centos-fedora-or-rhel/

Therefore, I've added a new setting which supports prefixing both the Docker build and push commands with
'sudo', which is the recommended method for executing Docker on CentOS, Fedora and RHEL.
Copy link
Author

@rbellamy rbellamy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized I should have added some further thoughts.

What I don't want to do is run SBT as root - this will cause the working directory, and the ivy cache to become polluted with artifacts that are owned by root. It's best to limit the use of escalated privileges to just those commands that require them to avoid this pollution.

So, to avoid:

sudo sbt docker:publishLocal
sbt clean <=== will fail because "target" is now owned by root

@@ -1,3 +1,3 @@
# Generate the Docker image locally
> docker:publishLocal
$ exec bash -c 'docker run docker-test:latest | grep -q "Hello world"'
$ exec bash -c 'docker run docker-commands:latest | grep -q "Hello world"'
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just a bug in the scripted test

@rbellamy
Copy link
Author

rbellamy commented Nov 3, 2016

Not sure why the two JDK tests failed in Travis... looks like a problem with the simple-app test (which I didn't touch)?

rbellamy pushed a commit to terradatum/jenkins-pipeline-library that referenced this pull request Nov 3, 2016
@muuki88 muuki88 added the docker label Nov 4, 2016
@muuki88
Copy link
Contributor

muuki88 commented Nov 4, 2016

Not sure why the two JDK tests failed in Travis... looks like a problem with the simple-app test (which I didn't touch)?

This is rather strange. I restarted the jobs. Sometimes travis throws random errors. However tihs looks strange as the same test fails on both JDK8 and 7.

Copy link
Contributor

@muuki88 muuki88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if introducing a new setting is necessary to achieve this. Mainly because you should be able to do this with

dockerBuildCommand := "sudo" +: dockerBuildCommand.value

IMHO an example in the DockerPlugin documentation would be sufficient. WDYT?

@rbellamy
Copy link
Author

rbellamy commented Nov 4, 2016

I'm embarrassed to say ii didn't even think of that. Yeah, that's much better. I'll put together another quick PR.

@rbellamy rbellamy closed this Nov 4, 2016
@rbellamy
Copy link
Author

rbellamy commented Nov 4, 2016

Actually, after further thought, I'm not sure it's so clear cut.

Specifically, ALL docker commands need to prefixed with sudo, which is problematic for the current implementation of the dockerBuildCommands and the fact that it is completely disconnected from the docker push task:

val cmd = Seq("docker", "push", tag)

An alternative approach would alter the build command process by adding a new SettingKey (maybe dockerCommand) which could be used as a prefix for all docker commands.

@muuki88 given the above, and that I've already done this work, I'm reopening this PR - if you have an alternative you'd like from me, just say the word.

@rbellamy rbellamy reopened this Nov 4, 2016
@muuki88
Copy link
Contributor

muuki88 commented Nov 6, 2016

AFAIK you need to run docker as sudo in all distros to make things work.

I have not that much experience building docker images for production. There are a couple of options to workaround this, for example as described in the mentioned blog post

alias docker="sudo /usr/bin/docker"

Introducing new settings always means maintaining them, even when they get obsolete. That's the reason I'm so sceptical if this is really necessary.

CC @fiadliel

@rbellamy
Copy link
Author

rbellamy commented Nov 7, 2016

Hammer, meet screw. I've been so focused on my build scripts and pipeline that I missed that I was solving the problem the wrong way - sorry for the noise.

The solution is to make my pipeline always alias the docker command.

@rbellamy rbellamy closed this Nov 7, 2016
@muuki88
Copy link
Contributor

muuki88 commented Nov 7, 2016

No problem 😉 Happy to help.

@rbellamy
Copy link
Author

rbellamy commented Nov 7, 2016

Here's the major challenge - when working on a CI server, jobs are frequently run using a non-interactive shell. This means setting an alias is VERY problematic. I've just spent several hours (most of today) trying to push Jenkins around to make my build scripts work WITHOUT issuing the sudo command directly, and I'm about where I was when I started...

@rbellamy
Copy link
Author

rbellamy commented Nov 8, 2016

And after further analysis (and a significant rework of my build pipeline to support aliases, including a custom Jenkins pipeline function) even if the shell which calls SBT contains an alias to sudo docker, ProcessBuilder doesn't support it, because aliases are not inherited by child processes, nor are functions.

@rbellamy
Copy link
Author

rbellamy commented Nov 8, 2016

As a matter fact, given what I've found, there are four options here:

  1. run SBT as root (a bad idea as I mentioned in my first comment)
  2. open docker to non-root users (again, a bad idea)
  3. or accept that code calling into docker needs to support prefixing that call with a sudo command
  4. The final alternative would be to provide a dockerExecCommand setting which would be used by both docker build and docker push that would allow any arbitrary prefix.

I'm not going to just re-open this PR, pending feedback from either @muuki88 or @fiadliel - if the dockerExecCommand is preferable, then I'll get that turned around and sent to you guys post-haste.

@muuki88
Copy link
Contributor

muuki88 commented Nov 8, 2016

Oh man :( java's processbuilder doesn't support aliases.

I can understand that 2. is not a good option ( even a lot of people may do it this way ).

The dockerExecCommand settings would be the best solution as it's generic enough to support future quirks of docker, while the dockerUseSudo is only a workaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants