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

Add support for extending the .dockercfg file #11759

Closed
wants to merge 1 commit into from

Conversation

duglin
Copy link
Contributor

@duglin duglin commented Mar 25, 2015

This PR does a couple of things:

  • makes loading of the .dockercfg file part of the DockerCli creation so
    that we only need to load it once and not at lots of spots throughout
    the code
  • moves the current auth stuff in the config struct down one level so that new
    fields can be added w/o breaking the existing auth config logic
  • add testcases to verify that we can read .dockercfg files in the old
    and new format - but always save in the new format

Next steps if this is accepted:

  • move the config file processing out from under 'registry' since it
    not registry specific any more
  • see what additional fields might be needed - yes I have some in mind :-)

Signed-off-by: Doug Davis dug@us.ibm.com

/cc @crosbymichael @nathanleclaire

@duglin
Copy link
Contributor Author

duglin commented Mar 26, 2015

/cc @dmcgowan

@duglin duglin force-pushed the ExtendConfig branch 2 times, most recently from 5da1953 to 4dec3af Compare March 26, 2015 17:57
This PR does a couple of things:
- makes loading of the .dockercfg file part of the DockerCli creation so
  that we only need have load it once and not at lots of spots throughout
  the code
- moves the current auth stuff in the config down one level so that new
  fields can be added w/o breaking the existing auth config logic
- add testcases to verify that we can read .dockercfg files in the old
  and new format - but always save in the new format

Next steps if this is accepted:
- move the config file processing out from under 'registry' since it
  not registry specific any more
- see what additional fields might be needed - yes I have some in mind :-)

Signed-off-by: Doug Davis <dug@us.ibm.com>
@dmcgowan
Copy link
Member

The code looks good, but will focus on getting the design review approved. I am also +1 for getting it out of the registry directory, but is likely to happen soon.

I am curious what other use cases you see for using the .dockercfg file and whether separate files in a .docker directory would fit it better or worse?

@duglin
Copy link
Contributor Author

duglin commented Mar 27, 2015

@dmcgowan we have an immediate need to add new HTTP headers to the Docker CLI->daemon flows - mainly to carry auth tokens that were setup by 3rd party tooling. So, I have a PR ready to go that will look for a "header" field in the .dockercfg file and have the cli blindly add those headers to all outgoing http messages.

I can also see including things like a "DockerHost" field so that other tooling, like Machine, can modify the user's DOCKER_HOST w/o resorting to tricks/hacks to get the env var modified.

I can move it out from under 'registry' very soon - I just didn't want to do too much within one PR as that tends to slow down the review process. :-)

As for separate files... I'd prefer not to unless we run across a pretty big reason to split it. I think having all of the cli config fields in one spot will make it much easier for people to deal with - especially 3rd party tooling.

@dmcgowan
Copy link
Member

sounds good, +1 to design

also @icecrime @jfrazelle for design review

@icecrime icecrime removed the dco/yes label Mar 30, 2015
@duglin
Copy link
Contributor Author

duglin commented Mar 30, 2015

ping @jfrazelle

@duglin
Copy link
Contributor Author

duglin commented Apr 1, 2015

ping @icecrime

@tianon
Copy link
Member

tianon commented Apr 1, 2015

As I've mentioned previously, I'd love to see ~/.dockercfg move out of ~ entirely (#10151 (comment), #10318 (comment) for two semi-recent examples of this discussion).

@tianon
Copy link
Member

tianon commented Apr 1, 2015

Adding more stuff to a mis-named file to make it less mis-named feels like stepping backwards, IMO. 😉

@duglin
Copy link
Contributor Author

duglin commented Apr 1, 2015

@tianon couple of comments

  • not sure why you think .dockercfg is still misnamed if it includes more than the auth stuff. If it becomes the general purpose docker config file then .dockercfg seems like a fine name to me. But if someone wants to propose a new name I'll rename it - just need a suggestion.
  • if we want to move it (or split it into more files ) into ~/.docker/ I'm ok with that - just need one of the current maintainers ( @dmcgowan ? ) to chime in here.
  • as for a more advanced (git-like) config file manager... see https://github.com/duglin/docker/tree/Config and https://gist.github.com/duglin/16560611876ce5f6adb7 ; I had one all ready to go (because I agree with you that we should offer some variants of an API to help modify our config files no matter how easy we think they are to edit) but there was resistance so I opted for a more focused solution.

@tianon
Copy link
Member

tianon commented Apr 1, 2015

Yes, even if we expand it, I still think the name is a mistake because it's so uncommon. Looking at all the other "dot" files in my ~, it's an odd man out for sure. There are lots of .*rc, *.conf, .[program]/, .config/..., even some .*config, but .*cfg is pretty rare. Also, ~/.docker is established as a thing, so why not do our users a favor and "contain" ourselves in it? 😄 (bad pun just for @jfrazelle)

@duglin
Copy link
Contributor Author

duglin commented Apr 1, 2015

@dmcgowan you ok with the migration code I have moving it from ~/.dockercfg to ~/.docker/config ?

@dmcgowan
Copy link
Member

dmcgowan commented Apr 1, 2015

As I first mentioned, I am in favor of a ~/.docker directory. I had introduced this in a previous PR related to key management, but was never merged 😞 Not sure if anything else is using it in master, I don't see anything else in their locally. I still think its better to split the configs up into multiple files though within that directory.

@duglin
Copy link
Contributor Author

duglin commented Apr 1, 2015

Based on the recent comments, I think what I'm hearing is that there are a set of PRs that might be of interest:

  • a PR to move ~/.dockercfg to ~/.docker/auth_config (or some other name)
  • a PR to create a ~/.docker/config file to store additional config options (for example, this new option I want to add that defines new HTTP headers, or potentially a DockerHost option if we find we want to put that into a config file.)
  • a PR to help 3rd party tooling modify these config files - per @tianon's comment.

Does this sound right?

If so, a few comments:

  • I have concern about splitting the config info across multiple files just from a management perspective. I don't think the amount of data we have justifies the added complexity. Having to justify (or even think about) which option goes in which file (especially when a certain option might span multiple files/purposes) just adds to my complexity concern. But I defer to you guys on this....
  • the last PR, about 3rd party tooling, is something I view as longer term PR (meaning I'm not going to do it soon) due to the recent push-back I got on the idea, but I do really think its going to be important to help our users - especially if we do have options sprinkled across multiple config files.

@dmcgowan @tianon @crosbymichael thoughts?

@tianon
Copy link
Member

tianon commented Apr 1, 2015

IMO ~/.docker/config seems fine for now, and we can use the file migration as a nice "break the syntax" event to move the current contents into a sub-key 👍

More generally, if there are things that are obviously completely separate (like auth credentials), then splitting them into a separate file makes sense, especially since you can then 600 that file and 666 other configuration files (similar to ~/.ssh/id_rsa versus ~/.ssh/config).

@dmcgowan
Copy link
Member

dmcgowan commented Apr 3, 2015

Considering the controversy around what is stored in .dockercfg today, I would advocate for keeping it in a separate file ~/.docker/auth_config and going forward when we have more sensible configuration to store we can use ~/.docker/config and build up tooling around it. Ideally there would be a tool for the different configs that could be stored in ~/.docker. For example we are currently experimenting with a namespace tool which would manipulate its own file and have a package for use inside the registry client. Having a requirement of storing all configs in a single file ~/.docker/config would be a burden for developing these external tools just so that there is only one file with configuration with a consistent format.

@dmcgowan
Copy link
Member

dmcgowan commented Apr 3, 2015

If my previous comment was not clear, that is a 👍 to @duglin's last comment

@icecrime
Copy link
Contributor

Thanks for the comments: pushing this to code review.

@duglin
Copy link
Contributor Author

duglin commented Apr 15, 2015

@icecrime sorry - I should have gone back to update this one. With #12009 we'll be doing the first 2 bullets from #11759 (comment) - meaning migration and support extending the docker file. I'd then like to follow it up with tooling to make it easier to modify that config file - either in this PR or a new one. I can close this one if you'd prefer until then.

@icecrime
Copy link
Contributor

I guess this one could be closed then (sorry, there's really too many information and cross references for me to follow here).

@duglin
Copy link
Contributor Author

duglin commented Apr 15, 2015

Closing this one until #12009 is done and we decide if we want to add tooling to modify the config file.

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.

None yet

5 participants