Add private files support #5836

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
7 participants
@alexlarsson
Contributor

alexlarsson commented May 16, 2014

This implements the private files proposal from:
https://gist.github.com/alexlarsson/c8e3277d2678c1061319

Private files are files stored in /run/private in the container. That is a tmpfs that is only visible in the container and the private files are never stored outside the container. For example you can do something like:

docker run --private-file api.key:/secret/key image

Which will upload the contents of the local /secret/key and create a file called /run/private/api.key in the container with that contents.

You can also place a file called /etc/docker/private.d/api.key on the machine running the docker daemon to achieve the same thing. This is very useful for host-specific private file, like product entitlements.

Private files also work for docker start and docker build, and previous private keys are inherited on docker restart.

alexlarsson added some commits May 15, 2014

libcontainer: Add support for adding files
You can pass a map of filename -> data to nsinit.Exec() and the
files will be created in the container, without being stored
to disk outside the container.

Docker-DCO-1.1-Signed-off-by: Alexander Larsson <alexl@redhat.com> (github: alexlarsson)
Add private file support
This adds support for private files, i.e. files in /run/private/* that
are only visible in the container and are not stored outside them. For
example you can do something like:

    docker run --private-file api.key:/secret/key image

Which will upload the contents of the local /secret/key and create a
file called /run/private/api.key in the container with that contents.

You can also place a file called /etc/docker/private.d/api.key on
the machine running the docker daemon to achieve the same thing.

Private files also work for docker start and docker build, and
previous private keys are inherited on docker restart.

Docker-DCO-1.1-Signed-off-by: Alexander Larsson <alexl@redhat.com> (github: alexlarsson)
@jamtur01

This comment has been minimized.

Show comment
Hide comment
@jamtur01

jamtur01 May 16, 2014

Contributor

Docs?

Contributor

jamtur01 commented May 16, 2014

Docs?

@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson May 16, 2014

Contributor

Yeah, docs are obviously missing.
I'd like to get some feedback on the rest before spending time on docs though. To avoid wasting time.

Contributor

alexlarsson commented May 16, 2014

Yeah, docs are obviously missing.
I'd like to get some feedback on the rest before spending time on docs though. To avoid wasting time.

@proppy

This comment has been minimized.

Show comment
Hide comment
@proppy

proppy May 16, 2014

Contributor

/cc @ktintc

Contributor

proppy commented May 16, 2014

/cc @ktintc

@jamtur01

This comment has been minimized.

Show comment
Hide comment
@jamtur01

jamtur01 May 16, 2014

Contributor

@alexlarsson Okay. Docs are never a waste of time IMHO. :P But understand.

I am personally a +0. I think making docker cp bi-directional would resolve this use case for me but I understand the desire.

Contributor

jamtur01 commented May 16, 2014

@alexlarsson Okay. Docs are never a waste of time IMHO. :P But understand.

I am personally a +0. I think making docker cp bi-directional would resolve this use case for me but I understand the desire.

@ibuildthecloud

This comment has been minimized.

Show comment
Hide comment
@ibuildthecloud

ibuildthecloud May 17, 2014

Contributor

Why does this functionality have to be specific to private files? Instead, if you just created a generic "--add" argument that had the same semantics as the Dockerfile ADD but just allows you to add files on run, that could be used for this also. You'd just do "--add my.key:/run/private/".

Contributor

ibuildthecloud commented May 17, 2014

Why does this functionality have to be specific to private files? Instead, if you just created a generic "--add" argument that had the same semantics as the Dockerfile ADD but just allows you to add files on run, that could be used for this also. You'd just do "--add my.key:/run/private/".

@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson May 19, 2014

Contributor

@jamtur01 docker cp wouldn't really work for most of the usecases here, as it inherently works on the copy-on-write parts of the docker container. I.e. the part that will be commited to an image. It never lets you copy from e.g. a volume or a tmpfs like /run. So, its generally no different from just having the file in the base image, i.e. all private data will be leaked with the image. Additionally, even if you could copy a file into a tmpfs like /run, that would only be for the instance of /run for the cp, and it will be gone the next time you start the container, which means this would not be useful in e.g. a Dockerfile where each line is a new container.

@ibuildthecloud I've intentionally tried to limit the capabilities for this because it doesn't really match with the desire to make docker build fully repeatable and host independent. This is by design of course, the goal is of course to only let you build if you have the right key, etc. However, we want to avoid this being misused for other things. Also, the way the code works (holding data in memory, encoding it, etc) makes it not really work well for e.g. large files.

Contributor

alexlarsson commented May 19, 2014

@jamtur01 docker cp wouldn't really work for most of the usecases here, as it inherently works on the copy-on-write parts of the docker container. I.e. the part that will be commited to an image. It never lets you copy from e.g. a volume or a tmpfs like /run. So, its generally no different from just having the file in the base image, i.e. all private data will be leaked with the image. Additionally, even if you could copy a file into a tmpfs like /run, that would only be for the instance of /run for the cp, and it will be gone the next time you start the container, which means this would not be useful in e.g. a Dockerfile where each line is a new container.

@ibuildthecloud I've intentionally tried to limit the capabilities for this because it doesn't really match with the desire to make docker build fully repeatable and host independent. This is by design of course, the goal is of course to only let you build if you have the right key, etc. However, we want to avoid this being misused for other things. Also, the way the code works (holding data in memory, encoding it, etc) makes it not really work well for e.g. large files.

@djmaze

This comment has been minimized.

Show comment
Hide comment
@djmaze

djmaze May 19, 2014

Contributor

You do not want all third party containers running on your host get access to those private files. So I think it is not a good idea to put the private files from /etc/docker/private.d into every container that is run on a system. There should be an explicit command line option (like --add-private-files) for that.

Contributor

djmaze commented May 19, 2014

You do not want all third party containers running on your host get access to those private files. So I think it is not a good idea to put the private files from /etc/docker/private.d into every container that is run on a system. There should be an explicit command line option (like --add-private-files) for that.

@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson May 19, 2014

Contributor

@djmaze Well, if you want container specific files you can just just --add-private-files, and ignore /etc/docker/private.d. On the other hand, an important usecase for us (redhat) is to have host-specific product entitlements in all containers so you can yum install rhel packages when running on a rhel host.

Contributor

alexlarsson commented May 19, 2014

@djmaze Well, if you want container specific files you can just just --add-private-files, and ignore /etc/docker/private.d. On the other hand, an important usecase for us (redhat) is to have host-specific product entitlements in all containers so you can yum install rhel packages when running on a rhel host.

@ibuildthecloud

This comment has been minimized.

Show comment
Hide comment
@ibuildthecloud

ibuildthecloud May 19, 2014

Contributor

@alexlarsson From what I gather the private files is not persisted in the container *.json config files so you have to do docker start --private-files ... if the container is ever stopped. This is going to be problematic when you restart dockerd. Your containers will be restarted but with no data that was passed in from run --private-files.

How bad would it be to just persist the data in the json config files? Basically only root can see those files, if you're root on the box you can get to the data in the container /run/private anyhow.

Contributor

ibuildthecloud commented May 19, 2014

@alexlarsson From what I gather the private files is not persisted in the container *.json config files so you have to do docker start --private-files ... if the container is ever stopped. This is going to be problematic when you restart dockerd. Your containers will be restarted but with no data that was passed in from run --private-files.

How bad would it be to just persist the data in the json config files? Basically only root can see those files, if you're root on the box you can get to the data in the container /run/private anyhow.

@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson May 20, 2014

Contributor

@ibuildthecloud Well, it wouldn't be hard to persist it, in fact it would be easier code-wise than what the current code is doing. However, how bad would it be from a security/risk perspective? Thats really hard to say, as it depends on all sorts of details. I just erred on the side of caution. If we add it to the config file the main difference is a) its a bit easier to get at when you're root, and b) its possible to get the secrets from non-running containers, and the advantage is that you can start a container that dies without knowing the secret. Is it worth it? I don't really know.

Contributor

alexlarsson commented May 20, 2014

@ibuildthecloud Well, it wouldn't be hard to persist it, in fact it would be easier code-wise than what the current code is doing. However, how bad would it be from a security/risk perspective? Thats really hard to say, as it depends on all sorts of details. I just erred on the side of caution. If we add it to the config file the main difference is a) its a bit easier to get at when you're root, and b) its possible to get the secrets from non-running containers, and the advantage is that you can start a container that dies without knowing the secret. Is it worth it? I don't really know.

@ibuildthecloud

This comment has been minimized.

Show comment
Hide comment
@ibuildthecloud

ibuildthecloud May 20, 2014

Contributor

@alexlarsson If you don't persist it then I think there needs to be something that would prevent restarting a container that was previously launched with run --private-files because the container is most likely not valid without the private files.

The only security issue I can think of if you persist it it is that the contents of the private files is accessible through the API which may be accessible by non-root. Maybe you just don't return the contents in a docker inspect call.

Contributor

ibuildthecloud commented May 20, 2014

@alexlarsson If you don't persist it then I think there needs to be something that would prevent restarting a container that was previously launched with run --private-files because the container is most likely not valid without the private files.

The only security issue I can think of if you persist it it is that the contents of the private files is accessible through the API which may be accessible by non-root. Maybe you just don't return the contents in a docker inspect call.

@timthelion

This comment has been minimized.

Show comment
Hide comment
@timthelion

timthelion May 21, 2014

This should be called PrivateFiles or something more descriptive.

This should be called PrivateFiles or something more descriptive.

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson May 22, 2014

Owner

Well, at this point it can be any kind of file. Its just "put these bytes at this path".

Owner

alexlarsson replied May 22, 2014

Well, at this point it can be any kind of file. Its just "put these bytes at this path".

@timthelion

This comment has been minimized.

Show comment
Hide comment
@timthelion

timthelion May 21, 2014

Contributor

I agree with @ibuildthecloud that runconfig will cause containers to fail to restart cleanly. Also, I'd like to remind @alexlarsson that security doesn't come from "it might be more secure, I'll err on the side of caution". Security comes from actually being secure, and keeping a file path secret isn't security at all. runconfig in my opinion complicates the code base while providing no security benefit at all. In fact, it makes security worse:

The key is held in a file owned by root, presumably a file that is labeled with SELinux to be accessible only to docker. If you store the path to the key in the docker config(also labeled by SELinux as belonging to docker and docker alone) then the information about where the secret file is is confined to the same security context as the secret itself(the docker security context).

However, if the user has to type in the path to the file using --private-files every time they launch the container, then the "secret path" to the "secret file" will now be stored in .bash_history, which is outside the docker context. ;)

Contributor

timthelion commented May 21, 2014

I agree with @ibuildthecloud that runconfig will cause containers to fail to restart cleanly. Also, I'd like to remind @alexlarsson that security doesn't come from "it might be more secure, I'll err on the side of caution". Security comes from actually being secure, and keeping a file path secret isn't security at all. runconfig in my opinion complicates the code base while providing no security benefit at all. In fact, it makes security worse:

The key is held in a file owned by root, presumably a file that is labeled with SELinux to be accessible only to docker. If you store the path to the key in the docker config(also labeled by SELinux as belonging to docker and docker alone) then the information about where the secret file is is confined to the same security context as the secret itself(the docker security context).

However, if the user has to type in the path to the file using --private-files every time they launch the container, then the "secret path" to the "secret file" will now be stored in .bash_history, which is outside the docker context. ;)

@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson May 21, 2014

Contributor

@timthelion Its not the path, its the actual data, which may have been uploaded from another host (and thus not exist anywhere else on the host but inside the tmpfs of the container).

Contributor

alexlarsson commented May 21, 2014

@timthelion Its not the path, its the actual data, which may have been uploaded from another host (and thus not exist anywhere else on the host but inside the tmpfs of the container).

@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson May 21, 2014

Contributor

That said, if you're root on the host you can still nsenter the container and extract the data from the tmpfs, so its not technically safer there. The main real difference is whether the secret data is fully gone when the container exits, or not.

Contributor

alexlarsson commented May 21, 2014

That said, if you're root on the host you can still nsenter the container and extract the data from the tmpfs, so its not technically safer there. The main real difference is whether the secret data is fully gone when the container exits, or not.

@timthelion

This comment has been minimized.

Show comment
Hide comment
@timthelion

timthelion May 21, 2014

Contributor

It seems to me, that it would be more useful and flexible to:

A) make volumes work at build time.

and

B) use a wrapper script to create and destroy the tempfs

Contributor

timthelion commented May 21, 2014

It seems to me, that it would be more useful and flexible to:

A) make volumes work at build time.

and

B) use a wrapper script to create and destroy the tempfs

@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson May 21, 2014

Contributor

@timthelion volumes at buildtime was nak:ed by @shykes due to not wanting host-dependent builds. This pr is the direct result of that, as for these usecases we conceptually need to be somewhat host-dependent, but we try to do it in a minimal way.

Contributor

alexlarsson commented May 21, 2014

@timthelion volumes at buildtime was nak:ed by @shykes due to not wanting host-dependent builds. This pr is the direct result of that, as for these usecases we conceptually need to be somewhat host-dependent, but we try to do it in a minimal way.

@timthelion

This comment has been minimized.

Show comment
Hide comment
@timthelion

timthelion May 21, 2014

Contributor

Hmph, I don't see this re-branding of volumes to be any less host-dependent....

When you think about it from RedHat's point of view, it doesn't make much sense to keep the product key secret while allowing the product itself to be easily copied. So why not keep the key in the image ;)

It just occurred to me that a generally useful command would be WITHFILE.

WITHFILE works a lot like ADD but with a command on the same line. You want WITHFILE if you need to use some large resource at build time, but you don't want to to ADD the file to your final image due to space concerns.

For example, if your build requires linux-headers to complete, but you don't want to add an extra 20 megabytes to your final image you can do:

WITHFILE linux-headers.tar.gz /build/linux-headers.tar.gz cd /build/ ; tar -xzf linux-headers.tar.gz; make ; make clean; rm -rf linux-headers ; rm linux-headers.tar.gz

You've just build your program with the linux headers, but these headers are not stored in the final image, thus bloat is reduced.

WITHFILE may also be useful for installing software that requires a product key to install.

For example to install proprietary redhat software into your image without adding your rhel key to the final image you can do:

WITHFILE rhel.key /etc/yum/rhel.key yum install proprietary-rhel-software ; rm /etc/yum/rhel.key
Contributor

timthelion commented May 21, 2014

Hmph, I don't see this re-branding of volumes to be any less host-dependent....

When you think about it from RedHat's point of view, it doesn't make much sense to keep the product key secret while allowing the product itself to be easily copied. So why not keep the key in the image ;)

It just occurred to me that a generally useful command would be WITHFILE.

WITHFILE works a lot like ADD but with a command on the same line. You want WITHFILE if you need to use some large resource at build time, but you don't want to to ADD the file to your final image due to space concerns.

For example, if your build requires linux-headers to complete, but you don't want to add an extra 20 megabytes to your final image you can do:

WITHFILE linux-headers.tar.gz /build/linux-headers.tar.gz cd /build/ ; tar -xzf linux-headers.tar.gz; make ; make clean; rm -rf linux-headers ; rm linux-headers.tar.gz

You've just build your program with the linux headers, but these headers are not stored in the final image, thus bloat is reduced.

WITHFILE may also be useful for installing software that requires a product key to install.

For example to install proprietary redhat software into your image without adding your rhel key to the final image you can do:

WITHFILE rhel.key /etc/yum/rhel.key yum install proprietary-rhel-software ; rm /etc/yum/rhel.key
@djmaze

This comment has been minimized.

Show comment
Hide comment
@djmaze

djmaze May 21, 2014

Contributor

@timthelion The ; rm /etc/yum/rhel.key in your last line should not be necessary, because that should be automatically handled as part of the WITHFILE directive. (I see this as a temporary ADD just for one line, right?) +1 for WITHFILE.

Contributor

djmaze commented May 21, 2014

@timthelion The ; rm /etc/yum/rhel.key in your last line should not be necessary, because that should be automatically handled as part of the WITHFILE directive. (I see this as a temporary ADD just for one line, right?) +1 for WITHFILE.

@timthelion

This comment has been minimized.

Show comment
Hide comment
@timthelion

timthelion May 21, 2014

Contributor

@djmaze you're right that the ; rm /etc/yum/rhel.key doesn't need to be necessary, and I guess there's no point in not deleting the file automatically.

Contributor

timthelion commented May 21, 2014

@djmaze you're right that the ; rm /etc/yum/rhel.key doesn't need to be necessary, and I guess there's no point in not deleting the file automatically.

@timthelion

This comment has been minimized.

Show comment
Hide comment
@timthelion

timthelion May 21, 2014

Contributor

@djmaze Yes, it is just a temporary ADD...

Contributor

timthelion commented May 21, 2014

@djmaze Yes, it is just a temporary ADD...

@timthelion

This comment has been minimized.

Show comment
Hide comment
@timthelion

timthelion May 21, 2014

Contributor

@djmaze actually there is a good use case in which the file should not be deleted. Compare the disk usage of the following two snippets(given that WITHFILE does NOT delete the file after the command is run:

ADD docker /usr/bin/docker
RUN chmod +x /usr/bin/docker

and

WITHFILE docker /usr/bin/docker chmod +x /usr/bin/docker

;)

Contributor

timthelion commented May 21, 2014

@djmaze actually there is a good use case in which the file should not be deleted. Compare the disk usage of the following two snippets(given that WITHFILE does NOT delete the file after the command is run:

ADD docker /usr/bin/docker
RUN chmod +x /usr/bin/docker

and

WITHFILE docker /usr/bin/docker chmod +x /usr/bin/docker

;)

@timthelion

This comment has been minimized.

Show comment
Hide comment
@timthelion

timthelion May 21, 2014

Contributor

Though admittedly if WITHFILE did always delete the file after the command was run one could also do:

WITHFILE docker /docker mv /docker /usr/bin/docker ; chmod +x /usr/bin/docker
Contributor

timthelion commented May 21, 2014

Though admittedly if WITHFILE did always delete the file after the command was run one could also do:

WITHFILE docker /docker mv /docker /usr/bin/docker ; chmod +x /usr/bin/docker
@djmaze

This comment has been minimized.

Show comment
Hide comment
@djmaze

djmaze May 21, 2014

Contributor

@timthelion Finally I get what you imagined. You want WITHFILE to automatically remove the file after the last line in the Dockerfile has been eval'd, right? I do not think this fits the syntax you are proposing. That should rather be called ADD_TEMPORARY (or similar) and stand on its own line in the Dockerfile, just like ADD.

Contributor

djmaze commented May 21, 2014

@timthelion Finally I get what you imagined. You want WITHFILE to automatically remove the file after the last line in the Dockerfile has been eval'd, right? I do not think this fits the syntax you are proposing. That should rather be called ADD_TEMPORARY (or similar) and stand on its own line in the Dockerfile, just like ADD.

@timthelion

This comment has been minimized.

Show comment
Hide comment
@timthelion

timthelion May 21, 2014

Contributor

No, WITHFILE will should add the file and run the command BEFORE saving a layer. If we removed the file after the last line in the docker file then the file would still be there, just marked as removed in the COW(Copy on write) file system.

Contributor

timthelion commented May 21, 2014

No, WITHFILE will should add the file and run the command BEFORE saving a layer. If we removed the file after the last line in the docker file then the file would still be there, just marked as removed in the COW(Copy on write) file system.

@timthelion

This comment has been minimized.

Show comment
Hide comment
@timthelion

timthelion May 21, 2014

Contributor

@djmaze If the docker binary weighs 20 megabytes and we chmod +x it, then it will get copy-on-writed and it will weigh 40 megabytes. But if we were able to do the chmod +x at the same time as adding it, then it would be written to disk only once...

Contributor

timthelion commented May 21, 2014

@djmaze If the docker binary weighs 20 megabytes and we chmod +x it, then it will get copy-on-writed and it will weigh 40 megabytes. But if we were able to do the chmod +x at the same time as adding it, then it would be written to disk only once...

@timthelion

This comment has been minimized.

Show comment
Hide comment
@timthelion

timthelion May 21, 2014

Contributor

@djmaze in the case of the private key files, we never want them to be written into a layer at all, so that they stay private. Which is why WITHFILE is useful for that case as well.

Contributor

timthelion commented May 21, 2014

@djmaze in the case of the private key files, we never want them to be written into a layer at all, so that they stay private. Which is why WITHFILE is useful for that case as well.

@ibuildthecloud

This comment has been minimized.

Show comment
Hide comment
@ibuildthecloud

ibuildthecloud May 22, 2014

Contributor

WITHFILE would apply only to build, while the current --private-file works for both run and build.

Persisting the private files in the docker config would be nice because it would allow restarts, especially in the context of maintenance because the operator restarting docker and its container will most like not be able to restore the missing private files.

Having said that, I see it would also be nice that in the current approach if you were to use the API to start a container theoretically the private files will never be persisted to disk. For the extra paranoid that could be a nice feature.

What if we could do both, for example --private-file api.key:/secret/key would be purely ephemeral and not persisted to disk while --private-file api.key:!/secret/key (with an exclamation mark) would be persisted to the docker container config. The actual syntax isn't important, just the idea.

Side note, I ran this PR before and I swear the arguments were actually reversed so that its --private-file destination:source not --private-file source:destination.

Contributor

ibuildthecloud commented May 22, 2014

WITHFILE would apply only to build, while the current --private-file works for both run and build.

Persisting the private files in the docker config would be nice because it would allow restarts, especially in the context of maintenance because the operator restarting docker and its container will most like not be able to restore the missing private files.

Having said that, I see it would also be nice that in the current approach if you were to use the API to start a container theoretically the private files will never be persisted to disk. For the extra paranoid that could be a nice feature.

What if we could do both, for example --private-file api.key:/secret/key would be purely ephemeral and not persisted to disk while --private-file api.key:!/secret/key (with an exclamation mark) would be persisted to the docker container config. The actual syntax isn't important, just the idea.

Side note, I ran this PR before and I swear the arguments were actually reversed so that its --private-file destination:source not --private-file source:destination.

@timthelion

This comment has been minimized.

Show comment
Hide comment
@timthelion

timthelion May 22, 2014

Contributor

@ibuildthecloud This is a huge amount of added complexity for a very narrow use case. And completely unnecessary, because the same functionality can be implemented more simply.

Image 6 months from now when everyone will be coming across the RunConfig struct and thinking "hey, I should put the configuration options needed to run the container in the RunConfig struct, that seems logical."

I am not the maintainer, but if I was, I'd never let this through.

Contributor

timthelion commented May 22, 2014

@ibuildthecloud This is a huge amount of added complexity for a very narrow use case. And completely unnecessary, because the same functionality can be implemented more simply.

Image 6 months from now when everyone will be coming across the RunConfig struct and thinking "hey, I should put the configuration options needed to run the container in the RunConfig struct, that seems logical."

I am not the maintainer, but if I was, I'd never let this through.

@unclejack

This comment has been minimized.

Show comment
Hide comment
@unclejack

unclejack May 22, 2014

Contributor

@timthelion I think everyone has understood your point of view.

Contributor

unclejack commented May 22, 2014

@timthelion I think everyone has understood your point of view.

@timthelion

This comment has been minimized.

Show comment
Hide comment
@timthelion

timthelion May 22, 2014

Contributor

@unclejack I don't think that makes my critique less helpful. I'm not just ranting I'm making specific critiques and I'm trying to find solutions as well. Perhaps renaming RunConfig to TempRunConfig would help.

Contributor

timthelion commented May 22, 2014

@unclejack I don't think that makes my critique less helpful. I'm not just ranting I'm making specific critiques and I'm trying to find solutions as well. Perhaps renaming RunConfig to TempRunConfig would help.

@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson May 22, 2014

Contributor

The Run in RunConfig means "per-Run", as the HostConfig means "per-Host" config and "Config" is global Config. Its possible that a better name can be found, in particular since the package is named runconfig this becomes a bit confusing. RuntimeConfig?

Contributor

alexlarsson commented May 22, 2014

The Run in RunConfig means "per-Run", as the HostConfig means "per-Host" config and "Config" is global Config. Its possible that a better name can be found, in particular since the package is named runconfig this becomes a bit confusing. RuntimeConfig?

-func parseRun(cmd *flag.FlagSet, args []string, sysInfo *sysinfo.SysInfo) (*Config, *HostConfig, *flag.FlagSet, error) {
+func AddRunConfigFlags(cmd *flag.FlagSet) {
+ flPrivate := opts.NewListOpts(nil)
+ cmd.Var(&flPrivate, []string{"#private", "-private-file"}, "Add private file (name:path)")

This comment has been minimized.

@timthelion

timthelion May 22, 2014

Contributor

Why the #private ?

@timthelion

timthelion May 22, 2014

Contributor

Why the #private ?

This comment has been minimized.

@alexlarsson

alexlarsson May 22, 2014

Contributor

So that ParseRunConfig() can read it

@alexlarsson

alexlarsson May 22, 2014

Contributor

So that ParseRunConfig() can read it

pipe, err := container.StdoutPipe()
if err != nil {
return nil, err
}
defer pipe.Close()
- if err := container.Start(); err != nil {
+ if err := container.Start(runConfig); err != nil {
return nil, err
}
output, err = ioutil.ReadAll(pipe)

This comment has been minimized.

@timthelion

timthelion May 22, 2014

Contributor

Can we clear runConfig out of memory immediately after Start?

@timthelion

timthelion May 22, 2014

Contributor

Can we clear runConfig out of memory immediately after Start?

This comment has been minimized.

@alexlarsson

alexlarsson May 22, 2014

Contributor

Well, we actually atm keep it in memory so that we can handle a container.Restart() operation and use the same private files. Thats somewhat similar to keeping it in hostconfig, but its also not as we don't keep it after a container exits.

@alexlarsson

alexlarsson May 22, 2014

Contributor

Well, we actually atm keep it in memory so that we can handle a container.Restart() operation and use the same private files. Thats somewhat similar to keeping it in hostconfig, but its also not as we don't keep it after a container exits.

@@ -289,6 +294,9 @@ func (container *Container) Start() (err error) {
if err := setupMountsForContainer(container); err != nil {
return err
}
+ if err := container.setupPrivateFiles(); err != nil {

This comment has been minimized.

@timthelion

timthelion May 22, 2014

Contributor

You should pass the runconfig directly to setupPrivateFiles and keep it out of theContainer` struct entirely.

@timthelion

timthelion May 22, 2014

Contributor

You should pass the runconfig directly to setupPrivateFiles and keep it out of theContainer` struct entirely.

This comment has been minimized.

@alexlarsson

alexlarsson May 22, 2014

Contributor

Its there to support restart

@alexlarsson

alexlarsson May 22, 2014

Contributor

Its there to support restart

@@ -79,6 +79,9 @@ type Container struct {
VolumesRW map[string]bool
hostConfig *runconfig.HostConfig
+ // Only set when running
+ runConfig *runconfig.RunConfig

This comment has been minimized.

@timthelion

timthelion May 22, 2014

Contributor

You should pass the runconfig directly to setupPrivateFiles and keep it out of theContainer` struct entirely.

@timthelion

timthelion May 22, 2014

Contributor

You should pass the runconfig directly to setupPrivateFiles and keep it out of theContainer` struct entirely.

@ibuildthecloud

This comment has been minimized.

Show comment
Hide comment
@ibuildthecloud

ibuildthecloud May 22, 2014

Contributor

So what's the conclusion then? Are we saying that if you use docker run --private-file and then you have to restart docker -d, you're just out of luck, your containers are dead?

Contributor

ibuildthecloud commented May 22, 2014

So what's the conclusion then? Are we saying that if you use docker run --private-file and then you have to restart docker -d, you're just out of luck, your containers are dead?

@ibuildthecloud

This comment has been minimized.

Show comment
Hide comment
@ibuildthecloud

ibuildthecloud May 22, 2014

Contributor

@alexlarsson Are you sure you don't want to flip the syntax to be --private-file /secret/key:api.key. If you look at other args like -p or --link, the format is [host value]:[container value].

Contributor

ibuildthecloud commented May 22, 2014

@alexlarsson Are you sure you don't want to flip the syntax to be --private-file /secret/key:api.key. If you look at other args like -p or --link, the format is [host value]:[container value].

@timthelion

This comment has been minimized.

Show comment
Hide comment
@timthelion

timthelion May 22, 2014

Contributor

@ibuildthecloud Really, this feature is meant for short term tasks like software installation. If you need the file to be in the container permanently and you need it to survive docker daemon restarts than this isn't the feature for you.

Contributor

timthelion commented May 22, 2014

@ibuildthecloud Really, this feature is meant for short term tasks like software installation. If you need the file to be in the container permanently and you need it to survive docker daemon restarts than this isn't the feature for you.

@ibuildthecloud

This comment has been minimized.

Show comment
Hide comment
@ibuildthecloud

ibuildthecloud May 22, 2014

Contributor

According to https://gist.github.com/alexlarsson/c8e3277d2678c1061319 this is not necessarily just short term build operations. For example, I may need an x509 client cert to communicate to some HTTPS service.

If we are limiting the scope of this to short term build operations, then I'd remove --private-file from run and only have it on build. But I don't think this was @alexlarsson original intention.

Contributor

ibuildthecloud commented May 22, 2014

According to https://gist.github.com/alexlarsson/c8e3277d2678c1061319 this is not necessarily just short term build operations. For example, I may need an x509 client cert to communicate to some HTTPS service.

If we are limiting the scope of this to short term build operations, then I'd remove --private-file from run and only have it on build. But I don't think this was @alexlarsson original intention.

@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson May 22, 2014

Contributor

Long discussion at todays meeting lead to a slightly different design. I will re-do this with the new approach.

Contributor

alexlarsson commented May 22, 2014

Long discussion at todays meeting lead to a slightly different design. I will re-do this with the new approach.

@alexlarsson alexlarsson referenced this pull request May 28, 2014

Closed

Add Secret store #6075

@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson May 28, 2014

Contributor

I made a new PR for the new approach to get a fresh start on the discussions:
#6075

Contributor

alexlarsson commented May 28, 2014

I made a new PR for the new approach to get a fresh start on the discussions:
#6075

@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson Jun 2, 2014

Contributor

closing this in favour of #6075

Contributor

alexlarsson commented Jun 2, 2014

closing this in favour of #6075

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment