-
Notifications
You must be signed in to change notification settings - Fork 201
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
Add ability to access Docker host #951
Conversation
Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com>
Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com>
I'm wondering if the build functionality would be better suited for a plugin, rather than a Docker in Docker build. The very first version of Duffle had the concept of components, and The reason I'm leaning towards such a plugin, besides avoiding running privileged containers (which we specifically removed from Ultimately, the difficulty with such component is defining the application model - do you have some examples of applications? ref cnabio/duffle#158 |
What I'm hoping for is not to have to change my porter.yaml definition in any way. It would be nice for Porter to use the same scripts I use locally, some of them which may contain a "docker build" within. In terms of the application - simple app, 3 custom services (1 frontend, 2 backends). All running as containers on Kubernetes. I want for Porter to be given info such as GIT_SSH_KEY, GIT_REMOTE_URL, GIT_COMMIT, and convert source code into a full blown production environment. For example - instantiate a container registry resource on cloud X, and in a later step, build and push images to it. In the case of img, looks like there are some other flags we can set at docker runtime that do not necessarily require the use of --privileged:
|
I'll also point out that I'm currently using a forked release of Porter based on this patch as part of my CI process, and it does exactly what I'm trying to accomplish like I described above. |
Right now people are not accessing Docker from inside a bundle. Early on in duffle, people decided that privileged containers by default was a security problem and it was closed off. They replaced it by making the docker driver configurable. So Porter, Docker App or Duffle could pass in additional configuration options to Docker. That's the background on the enhancement that everyone wants and why we haven't just turned on privileged containers. We have an open issue to consider how we would like to pass configuration to the docker driver to support bundles building images, pushing, using docker-compose, etc. |
@radu-matei The problem isn't limited to building application images, people also want access to using docker for other things, like |
@carolynvs - this seems that it's already supported by cnab-go using If I can make this work with img using
I'm interested in eventual full-blown docker-in-docker support (would like to use Porter with Tilt or example), but using img works for what I'm trying to do despite lack of layer caching etc. without the docker daemon. |
Yup, that is what I was trying to point out. It's already there, we just don't use it yet because we haven't decided how we want to expose configuring it and we don't want it on by default. The approach you have right now, an ambient environment variable only would need to be tweaked to also include in the bundle metadata any non-standard docker configuration required to run. First so that people can understand that when evaluating if they are okay with running a bundle using |
Ok cool. So is it safe to say I should close the PR on cnab-go then (cnabio/cnab-go#199)? That this setup would be a porter-specific configuration option? |
As long as all the settings you need to use for your bundles can be done through |
Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com>
I've extended this to be enabled via "dind" config option:
I do have trouble sourcing this setting from ~/.porter/config.toml .. what am I missing? To build and and run docker containers within porter, the following flags are set on "docker run" command when dind is enabled: 1.) -v /var/run/docker.sock:/var/run/docker.sock A follow up from this might be a docker/docker-compose/etc plugin(s) that are able to configure this for a single step? Let me know thoughts |
Works as a driver for docker-compose! Lots of possibilities here. Example: # This is a porter.yaml that exhibits how to use PORTER_DIND=true
# to build and run Docker containers with Docker Compose.
#
# To build/run docker-compose containers (starts a custom nginx and a redis):
# PORTER_DIND=true porter install --debug
#
# To stop all containers:
# PORTER_DIND=true porter uninstall --debug
#
name: composerator
version: 0.1.0
description: My bundle
tag: some/bundle
# In custom Dockerfile (Dockerfile.tmpl), install Docker CLI + Docker Compose:
#
# ENV DOCKER_VERSION="19.03.8"
# RUN apt-get update && apt-get install -y python3-pip wget && pip3 install --upgrade pip && \
# wget https://download.docker.com/linux/static/stable/x86_64/docker-${DOCKER_VERSION}.tgz && \
# tar -xvf docker-${DOCKER_VERSION}.tgz && \
# mv docker/docker /usr/bin/docker && \
# chmod +x /usr/bin/docker && \
# rm -rf docker/ docker-${DOCKER_VERSION}.tgz && \
# pip3 install docker-compose
#
dockerfile: Dockerfile.tmpl
mixins:
- exec
install:
- exec:
description: Docker compose up
command: bash
flags:
c: |-
echo "FROM nginx" > Dockerfile
cat <<EOF > docker-compose.yml
version: '3'
services:
web:
build: .
ports:
- "8000:80"
redis:
image: "redis:alpine"
EOF
docker-compose up -d
uninstall:
- exec:
description: Docker compose down
command: bash
flags:
c: |-
docker-compose down --remove-orphans and I'm able to access nginx locally at http://localhost:8000/ |
/azp run porter-integration |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
@jdolitsky this is great! As @carolynvs has mentioned, this functionality has been sought for some time.
Besides the DinD vs DooD comment on this review, I thought I'd chime in with a few things.
-
I couldn't get the example
porter.yaml
mentioned in Add ability to access Docker host #951 (comment) working quite right (could have been the multiline bash instructions via theexec
command, so I relegated the bash into scripts and split out thedocker-compose.yaml
andDockerfile
into assets such that both install, uninstall and other potential actions can pull these from the file system). Anyways, it worked swimmingly in the example bundle here: https://github.com/vdice/porter-bundles/tree/master/compose -
@carolynvs and I were wondering if, instead of an env var toggle or even a Porter config toggle, we might want to place the toggle in the form of a required Custom Extension which is a mechanism defined by the CNAB Spec that allows for bundles to declare which custom extensions it requires for runtime compatibility. Porter now allows for setting these in the
custom:
part of the manifest. Therefore, bundles requiring DinD could includeio.cnab.dind: { enabled: true }
or some such in the custom section and the logic added here could key off this info. One big benefit that this would supply down the line is bundle inspection: right off the bat, potential consumers of a bundle could see that this customization is required to run the bundle and decide whether or not they wish to run it. Also, the same custom extension could potentially be used for Duffle or other tool's bundles if/when Duffle or the other tools support these extensions. -
I wonder if we should add an example bundle to
examples
utilizing this functionality (if this PR gains consensus towards merging). Something like the straightforward example provided in comments already...
Despite those comments above, I'd be in support in hashing out the details in follow-up tickets and getting this core functionality into Porter today. I'm always a fan of incrementally adding value with a concrete roadmap of improvements in mind.
pkg/cnab/provider/runtime.go
Outdated
hostCfg.Mounts = []mount.Mount{} | ||
} | ||
hostCfg.Mounts = append(hostCfg.Mounts, dockerSockMount) | ||
hostCfg.Privileged = true |
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.
FWIW, one item I've learned whilst configuring DinD or DooD in/for Brigade is this:
- Privileged is necessary (or alternate security setup that you'd mentioned prior) for running DinD, that is, running the docker daemon inside of the container process. (Therefore, image needs to be based on
docker:stable-dind
and/or can executedockerd-entrypoint.sh
) - Mounting of the docker socket is necessary for running DooD, that is, Docker outside of Docker via the host's docker socket. No intra-container docker daemon process need be started, as it is using the host's. The example mentioned in this PR is actually of this type (DooD), strictly speaking, rather than DinD.
See also https://docs.brigade.sh/topics/scripting/#docker-runtime and brigadecore/brigade#1049 (comment)
We might think about adding toggles for either/or (and both could be toggled on, if for some reason this was desirable). Toggles (mutually exclusive) being:
- DooD: mount
var/run/docker.sock
from host - DinD: set
privileged=true
(container expected to run daemon)
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.
Agreed, thanks for the good explanation Vaughn!
As far as the porter config goes, I think for now we can keep it as a single flag (i.e. do we want to give the bundle access to the host which both privileged and the docker sock provide). Later when we work on bundle level config, we will need to think about what the bundle can request and how Porter can creatively be providing given various environments it's installed on.
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.
Vaughn is right that long term the way we want this feature configured is this:
- A bundle author writes a bundle and adds a notation to the porter.yaml or includes a mixin that indicates that it requires DinD.
- When a bundle consumer runs
porter install
they need to agree to the elevated privileges. Either throughinstall --allow-docker-host-access
, or a config value they can just leave on via env var or the file
porter.yaml
allow-docker-host-access = true
environment variable
PORTER_ALLOW_DOCKER_HOST_ACCESS=true
I figure we can add the configuration first in this PR and leave the rest for a follow up. Adding the config value works like this:
- Go to Config.DataStore and add the field
- Add a function to DataStore that fallsback to the environment variable if the config value was not set explicity, and defaults to false if nothing is set anywhere. There are other functions like that in the file to follow as an example.
- Use that function in Runtime.newDriver and check if we are allowed to create a DinD driver based on that config value before making the driver.
pkg/config/config.go
Outdated
@@ -28,6 +28,9 @@ const ( | |||
// EnvDEBUG is a custom porter parameter that signals that --debug flag has been passed through from the client to the runtime. | |||
EnvDEBUG = "PORTER_DEBUG" | |||
|
|||
// EnvDIND is a custom porter parameter that signals to enable "Docker-in-Docker" support. |
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.
This isn't the right location to set this flag (pretty much all the files from here down need to be reverted).
Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com>
@carolynvs @vdice thanks for taking a look! The DooD vs. DinD thing is interesting.. Updated to address comments and tested it - works. I'm just missing where to check for |
Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com>
@vdice in terms of examples, I agree. I like your compose example, maybe submit it after this is merged? |
Right now we don't have a magic place to check and set that value Suggestions welcome for improving this part of the config system for a later time! 😂 So for now add an accessor function to DataStore and put in it any special logic for that field. Then instead of using the field directly in the driver, use the accessor function. Here is an example that uses an accessor to fallback to a default plugin when none is specified: Our function will want to prefer the configured value on the field, then the environment variable and at last default to false. I hope that helps! |
Yeah adding an example as a follow-up is 👍 in my book. |
Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com>
Thanks for that explanation, done. Should be good to go now. I also created new file at |
Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com>
Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com>
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.
LGTM!
defer os.Unsetenv(EnvAllowDockerHostAccess) | ||
|
||
// Env var - invalid | ||
os.Setenv(EnvAllowDockerHostAccess, "sureenableitplz") |
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.
😂
/azp run porter-integration |
Azure Pipelines successfully started running 1 pipeline(s). |
@carolynvs when convenient, can you sign off and merge this PR if all change requests have been addressed? |
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.
This is great! 🎉 Thank you for plumbing this through so nicely to work with our config system. 🙇♀
🙌 |
Opening this PR mostly to start a discussion and get some advice. I'm looking to use Porter for all the things - including building and pushing container images from source as part of a full system installation.
The problem is that Porter itself runs "porter install" commands within the context of a "docker run". In order to run something like "docker build", this requires certain access to the underlying OS, which is disabled by default.
With Docker specifically, you need to run a docker daemon (i.e. "dind"), and that does not work without the --privileged flag. Even alternative tools like buildah and img appear to require extra privileges/flags in order to work.
This PR in its current state will set the
--privileged
flag ondocker run
, ifPRIVILEGED=1
is set in the environment:The correct place for this code is probably the underlying cnab-go library, so I've also opened a PR there. See cnabio/cnab-go#199
Is there something I'm missing in regards to this? Are other people using Porter to build images? Thanks in advance. Related to #442