Skip to content
This repository has been archived by the owner on Sep 26, 2021. It is now read-only.

added {http,https,no} proxy support to docker engines #1497

Merged
merged 1 commit into from
Jul 22, 2015

Conversation

udryan10
Copy link
Contributor

@udryan10 udryan10 commented Jul 9, 2015

Adds HTTP_PROXY, HTTPS_PROXY and NO_PROXY support to docker engines per issue #1319

@nathanleclaire
Copy link
Contributor

Hm, cool, thanks for the contribution.

I'm wondering if it might be better to have just one flag, which will then apply to any environment variable, instead of three (and possibly more in the future) different flags?

That way, you would :

$ docker-machine create -d provider --engine-env HTTP_PROXY=http://foo.com --engine-env HTTPS_PROXY=bar.com proxybox

What do you think?

@udryan10
Copy link
Contributor Author

@nathanleclaire I like it! I'll update the PR.

@udryan10 udryan10 force-pushed the engine-set-proxy-config branch from 42f531a to 09e087b Compare July 12, 2015 16:59
@udryan10
Copy link
Contributor Author

@nathanleclaire the PR now has code that implements the more general --engine-env flag

@nathanleclaire
Copy link
Contributor

Very cool. Any ideas how to test this in the integration suite?

@udryan10 udryan10 force-pushed the engine-set-proxy-config branch from 09e087b to 83a9c3c Compare July 14, 2015 00:56
@udryan10
Copy link
Contributor Author

@nathanleclaire I came up with a test that should work well -- spins up a machine and checks /proc/<docker daemon pid>/environs for the set Environment Variable. Let me know what you think.

@udryan10 udryan10 force-pushed the engine-set-proxy-config branch 2 times, most recently from 473cb42 to fc8da9d Compare July 14, 2015 14:37
@ehazlett
Copy link
Contributor

👍 for this -- i know a lot will benefit :)

@nathanleclaire
Copy link
Contributor

Looking pretty good --- we are about to merge in some structural changes to our docs, I will need you to rebase after that and then we will be ready to rock. I'll ping you when it's ready to rebase.

@nathanleclaire
Copy link
Contributor

OK I've merged that docs PR -- please rebase when it is convenient for you. Thanks!

@udryan10 udryan10 force-pushed the engine-set-proxy-config branch 2 times, most recently from 43434ed to 071db28 Compare July 15, 2015 02:31
@udryan10
Copy link
Contributor Author

@nathanleclaire I have rebased my doc changes into the new doc structure

@@ -1,4 +1,4 @@
<!--[metadata]>
## Specifying Docker Swarm options for the created machine<!--[metadata]>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is supposed to be here?

@nathanleclaire
Copy link
Contributor

cc @ehazlett for review of this PR

@udryan10 udryan10 force-pushed the engine-set-proxy-config branch from 071db28 to ca694e2 Compare July 15, 2015 20:16
@udryan10
Copy link
Contributor Author

@nathanleclaire fixed up the mistake in the docs/reference/create.md you referenced - apologies for that oversight.

@nathanleclaire
Copy link
Contributor

Thanks @udryan10 and no problem.

Hey @tianon, could you please take a quick look at these changes to the various OS configurations and note if anything strikes you as incorrect? For instance, I'm not sure if we should do export VAR=blah in /var/lib/boot2docker/profile, the rest of environment variables set there are not exported.

```
$ docker-machine create -d virtualbox \
--engine-env HTTP_PROXY=http://example.com:8080 \
--engine-env HTTPS_PROXY=http://example.com:8080 \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh... this is HTTPS_PROXY but the proto specified is http ;D

@udryan10 udryan10 force-pushed the engine-set-proxy-config branch from ca694e2 to c3b991c Compare July 15, 2015 21:06
@tianon
Copy link
Contributor

tianon commented Jul 15, 2015

It all looks reasonably sane, but what happens when I need quotes in my environment variable values? ie: --engine-env SOME_VAR='some "value" ' (which will come through into Go as "SOME_VAR=some \"value\" ")

@tianon
Copy link
Contributor

tianon commented Jul 15, 2015

(ie, we probably ought to pass these through %q from fmt at the very least)

Signed-off-by: Ryan Grothouse <rgrothouse@gmail.com>
@udryan10 udryan10 force-pushed the engine-set-proxy-config branch from c3b991c to d553a2c Compare July 16, 2015 20:12
@udryan10
Copy link
Contributor Author

I've updated the code to run the env's through %q

@ehazlett
Copy link
Contributor

LGTM

nathanleclaire added a commit that referenced this pull request Jul 22, 2015
added {http,https,no} proxy support to docker engines
@nathanleclaire nathanleclaire merged commit ad6d8d4 into docker:master Jul 22, 2015
@nathanleclaire
Copy link
Contributor

LGTM

@chadswen
Copy link

chadswen commented Sep 2, 2015

It seems that some aspect of proxy variable parsing can be picky about the proxy URL schema. I was getting "Proxy Authentication" errors with a proxy that doesn't require credentials. Adding a trailing slash to my proxy variables fixed the issue, as in:

docker-machine create -d virtualbox \
    --engine-env HTTP_PROXY=http://example.com:8080/ \
    --engine-env HTTPS_PROXY=https://example.com:8080/ \
    default

This format is used in the Docker Engine example, but it's not used in the docker-machine create example.

@nathanleclaire
Copy link
Contributor

@chadswen Could I get you to file a new issue about the documentation change required and cc @udryan10 and myself please? Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants