client: Allow Proxy Configuration in config.json by dave-tucker · Pull Request #30588 · moby/moby

dave-tucker · 2017-01-31T12:22:39Z

- 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

There are integration tests! But manually, you may:

Create a config.json with { "proxies" : { "httpProxy" : "http://127.0.0.1:3001" }
Create a Dockerfile

FROM busybox
RUN echo $HTTP_PROXY
CMD echo $HTTP_PROXY

docker --config /path/to/config build -t configtest .
Verify that the HTTP_PROXY variable is as configured
docker run --rm configtest env
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)

dave-tucker · 2017-01-31T12:24:14Z

Note: This will fail CI as the Unset test case is failing.
Strangely, this also fails with the same error when run without my patch e.g docker build -t foo --build-arg HTTP_PROXY .

alexellis · 2017-02-01T17:23:05Z

I'd like to test this on our network connection config @ ADP. It's just what I was after. @dave-tucker

alexellis · 2017-02-02T14:54:10Z

@dave-tucker

I can't build this behind a proxy without hacking all the Docker build scripts :-(

i.e.

-no-install-recommends 	&& pip install awscli==1.10.15
 ---> Running in 336264ab14d0
Ign http://deb.debian.org jessie InRelease
Ign http://security.debian.org jessie/updates InRelease
Ign http://ppa.launchpad.net trusty InRelease
Ign http://deb.debian.org jessie-updates InRelease
Err http://security.debian.org jessie/updates Release.gpg
  Connection failed [IP: 128.31.0.63 80]
Err http://ppa.launchpad.net trusty Release.gpg
  Connection failed
Err http://deb.debian.org jessie Release.gpg
  Connection failed [IP: 130.89.148.14 80]
Ign http://security.debian.org jessie/updates Release
Ign http://ppa.launchpad.net trusty Release
Err http://deb.debian.org jessie-updates Release.gpg
  Connection failed [IP: 5.153.231.4 80]

dave-tucker · 2017-02-02T15:17:51Z

@alexellis let me see if I can get this passing CI so you can download a bundle from CI to test with

alexellis · 2017-02-02T15:48:27Z

@dave-tucker I hacked the Makefile so that it passed --build args into the docker build command and make cross / install worked. I now have the unsupported build with your commit.

I edited a local config.json file, but it's not working for me (might be a user error)

Here's what I have:

$ ls
Dockerfile  config.json
$ docker --config config.json build -t curl .
WARNING: Error loading config file:config.json/config.json - stat config.json/config.json: not a directory

$ docker version
Client:
 Version:      1.14.0-dev
 API version:  1.26
 Go version:   go1.7.5
 Git commit:   43f3b63-unsupported
 Built:        Thu Feb  2 15:25:47 2017
 OS/Arch:      darwin/amd64

Server:
 Version:      1.14.0-dev
 API version:  1.26 (minimum version 1.12)
 Go version:   go1.7.5
 Git commit:   43f3b63-unsupported
 Built:        Thu Feb  2 15:25:47 2017
 OS/Arch:      linux/amd64
 Experimental: true

$ cat ./config.json
{ "httpProxy": "10.x.x.x:3128",
"httpsProxy": "10.x.x.x:3128"
}

alexellis · 2017-02-02T15:56:02Z

Also tried editing ~/.docker/config.json which is what I think I needed to edit anyway. Do the new values belong at the root element of JSON? Still doesn't appear to work / pick-up values.

RUN env is showing up like this (without the http:// scheme, that I think is needed)

 ---> Running in fe5c3f911788
HTTPS_PROXY=x.x.x.x:3128
HOSTNAME=11fbdc1f630f
SHLVL=1
HOME=/root
https_proxy=10.x.x.x:3128
http_proxy=10.x.x.x:3128

dave-tucker · 2017-02-02T16:23:36Z

I see bugs 🐛 Working on a fix @alexellis will ping you when it's ready for testing.

dave-tucker · 2017-02-03T15:01:44Z

cli/command/image/build.go

Perhaps some Golang expert can help here as I'm out of my depth.

If instead of o.Set I initially tried m[k] = &v, however the value of m[k] outside of this function was always "".

This makes no sense to me as when I try to reproduce in the Go Playground I can't.
There is probably some subtle interaction happening here that I don't quite understand.

I'd like to fix this before merge so I can avoid calling ConvertKVStringsToMapWithNil twice.

yeah, taking address from range variable doesn't work.

dave-tucker · 2017-02-03T15:07:31Z

Ok @alexellis it's ready for testing. A couple more pointers:

Yes this uses your ~/.docker/config.json by default. You can change this by passing the --config flag which takes a path to a directory where your config.json can be found (not the full path to config.json)
Yes, httpProxy is part of the root in the JSON. You can also set httpsProxy, ftpProxy and noProxy depending on your environment.
Yes, you should include the schema (e.g http://) in the value that's written to the httpProxy variable. This is directly mapped to the HTTP_PROXY and http_proxy build args. No additional validation is done so it's important to enter the right values!

danielpanteleit · 2017-02-08T11:02:37Z

@dave-tucker This would be really helpful in my case since I need to specify those build-args all the time.

I have two suggestions though:

Usually (at least for me) the proxy settings are known, e.g. in /etc/environment, on the host. So I can use the following build-args without specifying the values:

--build-args={http{,s},ftp,no}_proxy,{HTTP{,S},FTP,NO}_PROXY

Wouldn't this be a typical use case? Maybe you can also add something like "systemProxy": bool to the config file?

I have the same problem with docker run. It would be awesome if the same configuration would be used there too.

dave-tucker · 2017-02-10T15:45:12Z

@danielpanteleit Thanks for the feedback

Yes you are right, it would be nice to have an equivalent to the existing --build-args=HTTP_PROXY style syntax that will read the value configured in the environment. I'll look at adding it soon.
Yes! If the approach in this PR is accepted by the maintainers then I also plan on extending this functionality to docker run too (and also scoping these configuration options per docker host).

alexellis · 2017-02-11T15:56:42Z

Hey Dave, is this ready for testing yet? Do you need any help with the coding?

dave-tucker · 2017-02-11T17:02:10Z

@alexellis ready for testing and passing CI.

alexellis · 2017-02-15T19:24:23Z

Planning to try it out again this week. I'll test with this on DfM? Should also work on Linux client?

make cross
./contrib/mac-install-bundle.sh install

dave-tucker · 2017-02-15T21:00:42Z

@alexellis yes it should work on any client - either on OSX or Linux

alexellis · 2017-02-16T10:55:28Z

Showing client and server installed on DfM @dave-tucker :

/Users/aellis/go/src/github.com/docker/docker/bundles/latest/cross/darwin/amd64/docker version
Client:
 Version:      1.14.0-dev
 API version:  1.26
 Go version:   go1.7.5
 Git commit:   e37c03c-unsupported
 Built:        Wed Feb 15 02:11:56 2017
 OS/Arch:      darwin/amd64

Server:
 Version:      1.14.0-dev
 API version:  1.26 (minimum version 1.12)
 Go version:   go1.7.5
 Git commit:   e37c03c-unsupported
 Built:        Wed Feb 15 02:11:56 2017
 OS/Arch:      linux/amd64
 Experimental: true

With minimal Dockerfile this is working:

FROM alpine:latest

RUN apk add --update curl

Internet reachable:

build-args with known proxy
config.json with known proxy

Unreachable:

without build-args and no config.json
without build-args and invalid port for proxy

Looks good on DfM 👍

btw.. --config flag doesn't appear to accept a filename, just a folder.

alexellis · 2017-02-22T14:33:48Z

@dave-tucker I could really do with this soon.. we're running some workshops with Docker and the instructions are getting way more complicated with having to detail --build-arg and -e for every docker build / docker run command.

What is the process for moving the change forward - does it need buy-in from more maintainers?

dave-tucker · 2017-02-22T14:48:51Z

re: next steps, where are we at @thaJeztah?
do we need to discuss on the next maintainers meeting?

alexellis · 2017-03-02T20:02:49Z

Yes, specifically for puling images from remote registries such as the Hub. I originally thought that was what the daemon.json option was going to be doing. Thanks for clarifying.

alexellis · 2017-03-02T20:03:59Z

For the client; do we need a "default" option (i.e., a default for any daemon that is not explicitly defined in the client configuration)? We thought this could be added in a later stage if needed.

From a user experience point of view - at ADP all daemons needing internet access will have to go through a proxy, so a default option (now or in a later PR) would be a plus.

alexellis · 2017-03-02T20:05:20Z

To prevent the proxy-settings from leaking into the image's metadata, any build-arg that is part of the "default" set of build args should not be included in the image history.

Definitely agree with this point.

dave-tucker · 2017-03-06T13:48:25Z

Thanks @thaJeztah. In that case what I'll do is as follows:

I'll work on 3 PR's:

Client-side proxy configuration in ~/.docker/config.json (This PR)
Server-side proxy configuration in daemon.json (daemon: Allow Proxy Configuration in daemon.json #26318)
Exclude “default” build-args from image history (Exclude “default” build-args from image history #31584)

@alexellis your other use case is a tricky one 🤡

Implementation-wise... I see two options:

Imagine that we create a new function ProxyFromConfigOrEnvironment that reads daemon.json to get proxy settings (with a fallback to ProxyFromEnvironment from net/http). We'd need to create a new transport using this and then make sure it was used anywhere that attempts HTTP connections. When I last looked, I wasn't sure how many places this was and/or whether all of this code was limited to the docker/docker repo - i.e what about communication with a registry which I think is handled in docker/distribution package. You might need to touch a wide range of API's to expose a means of providing your own transport 😢
The other way would be to update the environment of dockerd PID. This would mean less changes but at a cost. I'm pretty sure many Linux aficionados will 😦 at changing the environment of a running process. There is also the issue of what happens to children of that PID - do we need to find all children and update their environments too. Assuming it works and is stable you'd have proxy hot-reload capability... but IMO it's quite a hacky approach.

TL;DR is that It's possible, but definitely a lot of work (read: I'd prefer to focus on my other PR's but if someone else wants to take a stab at it, great!)

My question is, is it worth the effort? We'd be deviating from a fairly standard practice (configuration of proxy via environment variables) which many people are already managing with Chef, Puppet, Salt, Ansible etc..

duglin · 2017-03-07T13:24:11Z

cli/config/configfile/file.go

This could be misleading. Someone could look at these and think they're for all client connections and not just for use in the build as build-args. Could we prefix these with BuildArg, or something, to make this clear?

I've changed this (patch set coming soon) to:

{ "proxies": { "default": { "httpProxy" : "http://proxy.example.com" } } }

These will now be used for both docker build and docker run.

The top-level key can be easily changed to reflect this.
@alexellis suggested containerProxies but I'm undecided as yet

duglin · 2017-03-07T13:27:46Z

cli/command/image/build.go

Why are we doing this?

To set both upper and lowercase derivatives (e.g HTTP_PROXY and http_proxy) as some applications will only check for one.

interesting, didn't notice before that we actually list both variants in the code.

Yes, there's no official spec for those; some software uses lowercase other use uppercase 😞

duglin · 2017-03-07T13:30:08Z

cli/config/configfile/file.go

Should the json keys be all uppercase to match what people would have to type on the --build-arg option? Since case does matter (at least in some envs) I think using the right variant would reduce potential confusion and typos.

I'm not sure what the convention is here. Personally I'd prefer http-proxy, https-proxy etc.. but from what I saw in the code it seems that camelCase is the done thing. Happy to change to whatever.

yea, I tend to view this less of a go or json convention and more one of consistency with how they would specify things elsewhere. For example, --build-arg http_PROXY=... should probably not be the same a --build-arg HTTP_PROXY=... since those really are two different env vars. So we should treat them as different in the config file too (assuming go allows us to - it might try to be "helpful" here and treat them the same :-( ). But we should document/code it the right way.

it would be really nice if the build args were nested under a "BuildArgs" property - then we wouldn't need to prefix them. But I'm not sure if our config file supports nested things.

dave-tucker · 2017-03-07T17:17:49Z

ok! updated based on #30588 (comment)

This PR not only adds the config to config.json (scoped on a per daemon level with a default setting too), but it now works for both docker build and docker run!

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 <[email protected]>

dave-tucker · 2017-03-27T16:31:09Z

Rebased but not able to get green tests locally. Seeing if I get the same result in CI...

Locally I get lots of errors like this:

START: docker_cli_build_test.go:4895: DockerSuite.TestBuildBuildTimeArgFromSpecificConfig
docker_cli_build_test.go:4916:
    result.Assert(c, icmd.Success)
/go/src/github.com/docker/docker/pkg/testutil/cmd/command.go:64:
    t.Fatalf("at %s:%d - %s", filepath.Base(file), line, err.Error())
... Error: at docker_cli_build_test.go:4916 -
Command:  /go/src/github.com/docker/docker/bundles/17.05.0-dev/dynbinary-client/docker -D --config /tmp/integration-cli-922980763 build -t bldargtest -
ExitCode: 1
Error:    exit status 1
Stdout:   Sending build context to Docker daemon  2.048kB
Step 1/3 : FROM busybox
 ---> d9551b4026f0
Step 2/3 : RUN echo $HTTP_PROXY
 ---> Running in b9df6f422f82
http://proxy.example.com

Stderr:   unexpected EOF


Failures:
ExitCode was 1 expected 0
Expected no error

It's the --config file here that causes the issue. No idea why it didn't appear in earlier testing.
The error message is pretty generic 😿 although Duck Duck Go tells me that previous errors of this sort were related to permissions. Possibly related to changes in Docker for Mac but will reserve judgement until one CI has passed.

thaJeztah · 2017-04-06T14:26:12Z

@dave-tucker what's the status on this one; are you still working on this? Should we close in the meantime?

dave-tucker · 2017-04-06T14:44:19Z

I don't think I'm going to get time to fix this before Dockercon so feel free to close for now...
Unless someone wants to carry this.
This was working fine... but there have been changes to the CLI and integration tests that seems to have broken it. It could also be my clumsy rebasing 😉

thaJeztah · 2017-04-06T14:51:15Z

I think everyone is quite busy, so let me close this for now. But we definitely want this so, please remind us to reopen once you have some bandwidth again

alexellis · 2017-04-06T22:45:51Z

Hmm this is the most useful PR for proxies and it's just been closed. Is the design review and implementation complete? What needs to be done to "finish it"?

dave-tucker · 2017-04-07T10:37:23Z

@alexellis AFAIK the design was "ok" as it follows the guidelines set out in #30588 (comment)

All that needs doing here is to troubleshoot and fix the integration test failures.
FWIW, I built a binary and tested it manually and it still works as expected

dave-tucker · 2017-04-21T12:48:58Z

@thaJeztah can you (or someone else with permission) re-open this PR?
The failing tests should be fixed in #32763
Once merged, I'll happily rebase to pick this up..
I've cherry-picked my other PR and run integration tests and they all pass 🎉

alexellis · 2017-04-21T15:08:20Z

Yay! thanks Dave

…

On 21 Apr 2017 07:49, "Dave Tucker" ***@***.***> wrote: @thaJeztah <https://github.com/thaJeztah> can you (or someone else with permission) re-open this PR? The failing tests should be fixed in #32763 <#32763> Once merged, I'll happily rebase to pick this up.. I've cherry-picked my other PR and run integration tests and they all pass 🎉 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#30588 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGEGz5AtRxLuYYFPXdBsDy5ltI0NHCrJks5ryKXvgaJpZM4Lyj-M> .

thaJeztah · 2017-05-02T00:51:26Z

Ugh, sorry didn't see your ping. GitHub doesn't allow me to reopen, possibly due to the repo-move (or did you push to the branch after it was closed?)

GordonTheTurtle added the status/0-triage label Jan 31, 2017

justincormack added status/1-design-review and removed status/0-triage labels Jan 31, 2017

dave-tucker force-pushed the proxyConfig branch from a693c7a to 43f3b63 Compare January 31, 2017 14:44

alexellis mentioned this pull request Feb 2, 2017

docker/docker Makefile won't acknowledge HTTP_PROXY #30677

Closed

dave-tucker force-pushed the proxyConfig branch 4 times, most recently from bb34d69 to f75c2e0 Compare February 3, 2017 03:38

dave-tucker commented Feb 3, 2017

View reviewed changes

dave-tucker force-pushed the proxyConfig branch 2 times, most recently from 96d19f1 to e37c03c Compare February 3, 2017 19:13

GordonTheTurtle assigned thaJeztah Feb 15, 2017

dave-tucker changed the title ~~Allow Proxy Configuration in config.json~~ client: Allow Proxy Configuration in config.json Mar 6, 2017

dave-tucker mentioned this pull request Mar 6, 2017

Exclude “default” build-args from image history #31584

Merged

duglin reviewed Mar 7, 2017

View reviewed changes

dave-tucker force-pushed the proxyConfig branch from e37c03c to 0fc3276 Compare March 7, 2017 17:08

dave-tucker force-pushed the proxyConfig branch from 0fc3276 to 3dc49c2 Compare March 7, 2017 17:22

dave-tucker mentioned this pull request Mar 11, 2017

Proxy information (HTTP_PROXY, HTTPS_PROXY) are not available at build time docker/for-win#96

Closed

Allow Proxy Configuration in config.json

647ab3e

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 <[email protected]>

dave-tucker force-pushed the proxyConfig branch from 3dc49c2 to 647ab3e Compare March 27, 2017 16:27

thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Apr 6, 2017

thaJeztah closed this Apr 6, 2017

thaJeztah mentioned this pull request Apr 7, 2017

Cannot initiate the connection to archive.ubuntu.com:80 #20394

Closed

dave-tucker mentioned this pull request May 2, 2017

client: Allow Proxy Configuration in config.json #32966

Closed

dave-tucker mentioned this pull request May 16, 2017

Allow Proxy Configuration in config.json docker/cli#93

Merged

thaJeztah mentioned this pull request Mar 14, 2019

Add the ability to specify global build args #27949

Closed

Conversation

dave-tucker commented Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dave-tucker commented Jan 31, 2017

Uh oh!

alexellis commented Feb 1, 2017

Uh oh!

alexellis commented Feb 2, 2017

Uh oh!

dave-tucker commented Feb 2, 2017

Uh oh!

alexellis commented Feb 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexellis commented Feb 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dave-tucker commented Feb 2, 2017

Uh oh!

dave-tucker Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dave-tucker commented Feb 3, 2017

Uh oh!

danielpanteleit commented Feb 8, 2017

Uh oh!

dave-tucker commented Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexellis commented Feb 11, 2017

Uh oh!

dave-tucker commented Feb 11, 2017

Uh oh!

alexellis commented Feb 15, 2017

Uh oh!

dave-tucker commented Feb 15, 2017

Uh oh!

alexellis commented Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexellis commented Feb 22, 2017

Uh oh!

dave-tucker commented Feb 22, 2017

Uh oh!

alexellis commented Mar 2, 2017

Uh oh!

alexellis commented Mar 2, 2017

Uh oh!

alexellis commented Mar 2, 2017

Uh oh!

dave-tucker commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dave-tucker commented Mar 7, 2017

dave-tucker commented Jan 31, 2017 •

edited

Loading

alexellis commented Feb 2, 2017 •

edited

Loading

alexellis commented Feb 2, 2017 •

edited

Loading

dave-tucker Feb 3, 2017 •

edited

Loading

dave-tucker commented Feb 10, 2017 •

edited

Loading

alexellis commented Feb 16, 2017 •

edited

Loading

dave-tucker commented Mar 6, 2017 •

edited

Loading

duglin Mar 7, 2017 •

edited

Loading