Skip to content

Remove Docker-in-Docker from builds#111

Merged
openshift-bot merged 1 commit intoopenshift:masterfrom
ironcladlou:host_docker_builds
Sep 19, 2014
Merged

Remove Docker-in-Docker from builds#111
openshift-bot merged 1 commit intoopenshift:masterfrom
ironcladlou:host_docker_builds

Conversation

@ironcladlou
Copy link
Copy Markdown
Contributor

Remove all traces of Docker-in-Docker from builds. The rationale is explained
generally in the docs/builds.md file. In short, Docker-in-Docker isn't secure
or stable for production, and there's no indication it will be in the forseeable
future. The codebase can be simplified by eliminating it, and our focus and
efforts can shift to solving the remaining issues with host Docker access.

@ironcladlou
Copy link
Copy Markdown
Contributor Author

@ironcladlou
Copy link
Copy Markdown
Contributor Author

Should builds.md be re-framed as a current OpenShift design document rather than a k8s proposal?

@bparees
Copy link
Copy Markdown
Contributor

bparees commented Sep 18, 2014

@ironcladlou thanks, I like the rational, I am wondering though, why are privileged containers needed for DIND? I would think there are more risks w/ exposing the host docker daemon than having a contained docker daemon.

@bparees
Copy link
Copy Markdown
Contributor

bparees commented Sep 18, 2014

@ironcladlou and yeah, reframing as OS design makes sense, i believe.

@smarterclayton
Copy link
Copy Markdown
Contributor

Unfortunately to run DIND you need to be able to mount things... to mount you need CAP_SYS_ADMIN which is ~ root (because if you can mount you can mount things that the kernel calls directly).

So basically, we can make host docker more secure than we can make DIND.

On Sep 18, 2014, at 5:37 PM, Ben Parees notifications@github.com wrote:

@ironcladlou thanks, I like the rational, I am wondering though, why are privileged containers needed for DIND? I would think there are more risks w/ exposing the host docker daemon than having a contained docker daemon.


Reply to this email directly or view it on GitHub.

@smarterclayton
Copy link
Copy Markdown
Contributor

Agree with Ben

On Sep 18, 2014, at 5:50 PM, Ben Parees notifications@github.com wrote:

@ironcladlou and yeah, reframing as OS design makes sense, i believe.


Reply to this email directly or view it on GitHub.

@pmorie
Copy link
Copy Markdown
Contributor

pmorie commented Sep 18, 2014

+1 to reframe

On Thursday, September 18, 2014, Clayton Coleman notifications@github.com
wrote:

Agree with Ben

On Sep 18, 2014, at 5:50 PM, Ben Parees <notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

@ironcladlou and yeah, reframing as OS design makes sense, i believe.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
#111 (comment).

@ironcladlou
Copy link
Copy Markdown
Contributor Author

Will revamp the docs later on tonight. It's really unfortunate about DinD since it's so conceptually clean, and I'd love to be proven wrong.

@ironcladlou ironcladlou force-pushed the host_docker_builds branch 3 times, most recently from b2795dc to 753e70d Compare September 19, 2014 01:26
@ironcladlou
Copy link
Copy Markdown
Contributor Author

I made major changes to build.md and moved the code-level implementation details to package docs. Please review.

Comment thread docs/builds.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe it'd make sense to provide also a link to https://docs.docker.com/reference/commandline/cli/#build.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks.

@soltysh
Copy link
Copy Markdown
Contributor

soltysh commented Sep 19, 2014

LGTM

@ironcladlou ironcladlou force-pushed the host_docker_builds branch 2 times, most recently from 0c20c7d to f885cac Compare September 19, 2014 11:38
Remove all traces of Docker-in-Docker from builds. The rationale is explained
generally in the `docs/builds.md` file. In short, Docker-in-Docker isn't secure
or stable for production, and there's no indication it will be in the forseeable
future. The codebase can be simplified by eliminating it, and our focus and
efforts can shift to solving the remaining issues with host Docker access.
@smarterclayton
Copy link
Copy Markdown
Contributor

@csrwng review please

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be configurable?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems to me to fall into the category of things which should be
parameters to the build controller. @smarterclayton does this pass the
threshold for adding a new cli flag?

On Friday, September 19, 2014, Michal Fojtik notifications@github.com
wrote:

In pkg/build/strategy/util.go:

  •   podSpec.DesiredState.Manifest.Volumes = append(podSpec.DesiredState.Manifest.Volumes,
    
  •       dockerSocketVolume)
    
  •   podSpec.DesiredState.Manifest.Containers[0].VolumeMounts =
    
  •       append(podSpec.DesiredState.Manifest.Containers[0].VolumeMounts,
    
  •           dockerSocketVolumeMount)
    
  • } else {
  •   podSpec.DesiredState.Manifest.Containers[0].Privileged = true
    
  • dockerSocketVolumeMount := api.VolumeMount{
  •   Name:      "docker-socket",
    
  •   MountPath: "/var/run/docker.sock",
    

Shouldn't this be configurable?


Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/111/files#r17789238.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It assumes that the minion is running the docker daemon locally which I guess is the point of the minion. But if we need to support minions with different configs I think we should handle it in a separate item.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have yet to see a docker system running with that on a different port

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In the future we may also want to somehow expose it over TCP rather than a socket... I'd rather deal with those cases as they arise for now.

@csrwng
Copy link
Copy Markdown
Contributor

csrwng commented Sep 19, 2014

LGTM

@smarterclayton
Copy link
Copy Markdown
Contributor

LGTM [merge]

@openshift-bot
Copy link
Copy Markdown
Contributor

Origin Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/111/) (Image: devenv-fedora_178)

@openshift-bot
Copy link
Copy Markdown
Contributor

Evaluated for origin up to 60b9d7b

openshift-bot pushed a commit that referenced this pull request Sep 19, 2014
@openshift-bot openshift-bot merged commit e22dfcb into openshift:master Sep 19, 2014
@bparees
Copy link
Copy Markdown
Contributor

bparees commented Sep 19, 2014

@ironcladlou did you kick off rebuilds of the docker builder images? we do not have automatic builds setup right now.

@smarterclayton
Copy link
Copy Markdown
Contributor

I turned them back on to automatic

On Sep 19, 2014, at 4:23 PM, Ben Parees notifications@github.com wrote:

@ironcladlou did you kick off rebuilds of the docker builder images? we do not have automatic builds setup right now.


Reply to this email directly or view it on GitHub.

@ironcladlou ironcladlou deleted the host_docker_builds branch September 26, 2014 13:29
pmorie pushed a commit to pmorie/origin that referenced this pull request Nov 5, 2014
…imeout

Remove unneccessary content from Vagrantfile to correct test builds
jpeeler pushed a commit to jpeeler/origin that referenced this pull request Feb 1, 2018
Change-Id: Ia271141c7332b9ae0085a8729c93cad7303f793c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants