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

Allow Proxy Configuration in config.json #93

Merged
merged 1 commit into from Jun 20, 2017

Conversation

Projects
None yet
7 participants
@dave-tucker
Member

dave-tucker commented May 16, 2017

This is moby/moby#32966 re-openend against docker/cli
The above PR was a redux of moby/moby#30588 rebased and after the repo move!

- What I did

Added a mechanism to allow HTTP/HTTPS/FTP/NO proxy variables to be configured in config.json. These are then automatically populated in a docker run command as environment variables or as build-args in docker build!

This makes it easier for users suffering behind an HTTP Proxy as they only need to configure this once as opposed to creating custom aliases or forgetting the args and being denied buildage/runnage.

Updates #30323

- How I did it

Extended the configfile with some new proxy related variables.

These are scoped per Docker Host with a default catchall.

The config file is then read by the build or run command and the necessary variables are exported.

A -e flag or --build-arg provided on the CLI will override these default settings.

- How to verify it

  1. Create a config.json with { "proxies" : { "httpProxy" : "http://127.0.0.1:3001" }
  2. Create a Dockerfile
FROM busybox
RUN echo $HTTP_PROXY
CMD echo $HTTP_PROXY
  1. docker --config /path/to/config build -t configtest .
  2. Verify that the HTTP_PROXY variable is as configured
  3. docker run --rm configtest env
  4. Verify that the HTTP_PROXY variable is as configured

- Description for the changelog

Added HTTP/HTTPS/FTP proxy configuration to config.json

- A picture of a cute animal (not mandatory but encouraged)

Show outdated Hide outdated cli/command/container/run.go
Show outdated Hide outdated cli/command/container/run.go
Show outdated Hide outdated cli/command/container/run.go
Show outdated Hide outdated cli/command/image/build.go
@dnephin

Design LGTM

The client user config seems like a reasonable place to provide proxy information considering the current method is from client environment variables.

@dave-tucker

This comment has been minimized.

Show comment
Hide comment
@dave-tucker

dave-tucker May 16, 2017

Member

Thanks @dnephin I'll address your review comments and add some unit tests tomorrow

Member

dave-tucker commented May 16, 2017

Thanks @dnephin I'll address your review comments and add some unit tests tomorrow

@dave-tucker

This comment has been minimized.

Show comment
Hide comment
@dave-tucker

dave-tucker May 17, 2017

Member

@dnephin comments addressed. I realised I'd touched vendored code directly when git did it's apply magic. I've opened moby/moby#33253 to cover the moby/moby side of the changes. This will fail CI until such time as I can vendor and updated moby/moby/client.

Member

dave-tucker commented May 17, 2017

@dnephin comments addressed. I realised I'd touched vendored code directly when git did it's apply magic. I've opened moby/moby#33253 to cover the moby/moby side of the changes. This will fail CI until such time as I can vendor and updated moby/moby/client.

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin May 17, 2017

Collaborator

We actually pass in host from cli/command/cli.go, so we could refactor that to store the Host on ServerInfo struct, instead of pulling it from the client.

Collaborator

dnephin commented May 17, 2017

We actually pass in host from cli/command/cli.go, so we could refactor that to store the Host on ServerInfo struct, instead of pulling it from the client.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io May 24, 2017

Codecov Report

Merging #93 into master will increase coverage by 0.04%.
The diff coverage is 59.37%.

@@            Coverage Diff             @@
##           master      #93      +/-   ##
==========================================
+ Coverage   45.67%   45.72%   +0.04%     
==========================================
  Files         171      171              
  Lines       11513    11542      +29     
==========================================
+ Hits         5259     5278      +19     
- Misses       5947     5956       +9     
- Partials      307      308       +1

codecov-io commented May 24, 2017

Codecov Report

Merging #93 into master will increase coverage by 0.04%.
The diff coverage is 59.37%.

@@            Coverage Diff             @@
##           master      #93      +/-   ##
==========================================
+ Coverage   45.67%   45.72%   +0.04%     
==========================================
  Files         171      171              
  Lines       11513    11542      +29     
==========================================
+ Hits         5259     5278      +19     
- Misses       5947     5956       +9     
- Partials      307      308       +1
@dave-tucker

This comment has been minimized.

Show comment
Hide comment
@dave-tucker

dave-tucker May 24, 2017

Member

@dnephin requested changes made. I've also vendored a newer moby/moby to pick up moby/moby#33253. CI is now green

Member

dave-tucker commented May 24, 2017

@dnephin requested changes made. I've also vendored a newer moby/moby to pick up moby/moby#33253. CI is now green

@dave-tucker

This comment has been minimized.

Show comment
Hide comment
@dave-tucker

dave-tucker Jun 6, 2017

Member

ping @dnephin - does this need more attention or is it good to merge?

Member

dave-tucker commented Jun 6, 2017

ping @dnephin - does this need more attention or is it good to merge?

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Jun 6, 2017

Member

LGTM

Member

tonistiigi commented Jun 6, 2017

LGTM

Allow Proxy Configuration in config.json
This commit modifies config.json to allow for any proxies allowed in
build-args to be configured. These values will then be used
by default as build-args in docker build.

Signed-off-by: Dave Tucker <dt@docker.com>
@dave-tucker

This comment has been minimized.

Show comment
Hide comment
@dave-tucker

dave-tucker Jun 20, 2017

Member

Rebased. It'd be nice for this to merge soon, assuming it passes CI 🙏

Member

dave-tucker commented Jun 20, 2017

Rebased. It'd be nice for this to merge soon, assuming it passes CI 🙏

@cpuguy83

LGTM

@cpuguy83 cpuguy83 merged commit 6f6ccbd into docker:master Jun 20, 2017

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 59.37% of diff hit (target 50%)
Details
codecov/project 45.72% (+0.04%) compared to 5dd3073
Details
dco-signed All commits are signed

@GordonTheTurtle GordonTheTurtle added this to the 17.07.0 milestone Jun 20, 2017

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 21, 2017

Member

ping @dave-tucker @mstanleyjones looks like this change didn't make it into the documentation; there's a section in the reference docs describing the options that are available in config.json https://github.com/docker/cli/blob/master/docs/reference/commandline/cli.md

Member

thaJeztah commented Jul 21, 2017

ping @dave-tucker @mstanleyjones looks like this change didn't make it into the documentation; there's a section in the reference docs describing the options that are available in config.json https://github.com/docker/cli/blob/master/docs/reference/commandline/cli.md

somcsel added a commit to somcsel/docker.github.io that referenced this pull request Sep 21, 2017

Update documentation of proxy server configuration
As per the implementation in docker/cli#93 the proxy server addresses
are specified per docker daemon or as defaults.  Updated the example to
show the default option.

Fixes docker#4686

mistyhacks added a commit to docker/docker.github.io that referenced this pull request Oct 13, 2017

Update documentation of proxy server configuration (#4687)
* Update documentation of proxy server configuration

As per the implementation in docker/cli#93 the proxy server addresses
are specified per docker daemon or as defaults.  Updated the example to
show the default option.

Fixes #4686

* Fix table layout of proxy environment variables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment