feat(builder+docker+user-data): proxy settings #2825

lorieri · 2014-12-30T20:10:23Z

Proxy settings for coreos, builder, slugrunner, slugbuilder and docker

originally using /etc/environment, changed to /etc/environment_proxy to avoid current automations failures

lorieri · 2014-12-30T22:45:37Z

any chance to have a new user-data in the jenkins job to create the /etc/environment_proxy ?

mboersma · 2014-12-30T23:00:14Z

@lorieri the functional tests are currently failing because starting the deis-builder container runs into this:
/app/bin/entry: line 4: /etc/environment_proxy: No such file or directory
The entrypoint script isn't finding that file inside the container.

If test-integration.sh isn't passing for you locally, it's not going to work for Jenkins either. I'm not sure exactly what this PR intends to fix, but maybe user-data isn't the right place for this. Are users supposed to edit user-data to add proxy values before provisioning?

bacongobbler · 2014-12-30T23:01:55Z

Jenkins should be using the same user-data file here. It's relying on https://github.com/deis/deis/blob/master/tests/bin/test-integration.sh so the issue has to do with the functional tests: https://github.com/deis/deis/blob/master/builder/tests/builder_test.go#L48-L56

You'll probably have to write out a temporary file and mount that in to reflect the integration tests.

lorieri · 2014-12-30T23:12:55Z

yes, if they want proxy settings
but they can leave it with empty values

all units load /etc/environment, I was going to use that file but I was afraid to break other things, so I created that new one

@bacongobbler I tried to fix the test

bacongobbler · 2014-12-30T23:16:49Z

That test is run on the host instead of in a docker container (such as your laptop), so you'll need to populate a temp file via http://golang.org/pkg/io/ioutil/#TempFile (it can be empty since we're just source'ing it), then you can reference that file and mount it into the container.

bacongobbler · 2014-12-30T23:28:11Z

looks like a few stray commits made it in: d3b6347 and eff3b7a. Would you mind removing those and squashing your other commits together once you fix up the functional test, please? :)

bacongobbler · 2015-01-02T19:25:31Z

code LGTM, though let's squash these commits again and change the commit message to

feat(builder): add proxy support

As well as an explanation/justification of the problem and solution as noted in http://docs.deis.io/en/latest/contributing/standards/#commit-style so we can better understand the changes involved here.

carmstrong · 2015-01-12T23:40:30Z

@lorieri Can you add documentation around using this? It's a helpful change, but a user wouldn't know it's there without documentation.

It also looks like you need to run gofmt on the source.

ianblenke · 2015-01-13T02:54:27Z

contrib/coreos/user-data.example

+      http_proxy=
+      https_proxy=
+      all_proxy=
+      no_proxy=


Why both uppercase and lowercase? I'm familiar with the lowercase form for many unix commands, but not the uppercase form as a shell environment variable (outside of CGI which is a different use context and scope).

@ianblenke builder runs custom buildpacks and we don't know what kind of program it will run, some programs use uppercase and others lowercase

https://wiki.archlinux.org/index.php/proxy_settings#Environment_variables

carmstrong · 2015-01-13T06:23:59Z

@lorieri I took your base docs and fleshed them out a bit. Feel free to make changes as necessary.

carmstrong · 2015-01-13T06:24:37Z

ping @bacongobbler @mboersma for docs review - otherwise, I think this is ready to ship.

lorieri · 2015-01-13T12:23:49Z

@carmstrong thanks, I have nothing to add

mboersma · 2015-01-13T14:13:31Z

builder/tests/builder_test.go

@@ -31,6 +35,14 @@ func TestBuilder(t *testing.T) {
 		"/deis/domains",
 		"/deis/services",
 	}
+	setproxy := []byte("HTTP_PROXY=\nhttp_proxy=\n")
+
+        tmpfile,errfile := ioutil.TempFile("/tmp", "deis-test-")


Indentation here isn't consistent: we need to gofmt this.

I run gofmt but I was waiting @carmstrong to help me with the docs, let me do this now

lorieri · 2015-01-13T16:12:37Z

~~@carmstrong I've lost your docs commit, could commit it again ?~~

nervermind

bacongobbler · 2015-01-13T16:41:40Z

builder/tests/builder_test.go

@@ -65,4 +79,6 @@ func TestBuilder(t *testing.T) {
 	dockercli.DeisServiceTest(t, name, port, "tcp")
 	etcdutils.VerifyEtcdValue(t, "/deis/builder/host", host, etcdPort)
 	etcdutils.VerifyEtcdValue(t, "/deis/builder/port", port, etcdPort)
+
+	defer os.Remove(tmpfile.Name())


This should be moved up to line 45. That way, the next person reading this won't be wondering where the call to defer is hiding.

sure, at that time I was not sure I could do that :)

carmstrong · 2015-02-12T19:18:03Z

@lorieri Could you rebase this off master? We'll review again and try to get it in v1.4.0. Thanks!

lorieri · 2015-02-12T21:18:56Z

done

carmstrong · 2015-02-12T21:52:53Z

@lorieri You'll have to rebase one last time - we just fixed a bug in the tests in master. Sorry!

lorieri · 2015-02-13T00:41:23Z

done 👍

mboersma · 2015-02-13T18:34:39Z

builder/tests/builder_test.go

+
+	tmpfile, errfile := ioutil.TempFile("/tmp", "deis-test-")
+	if errfile != nil {
+		panic(errfile)


We should call t.Fatal here instead of panic.

Proxy support for builder and coreos' docker daemon Non-cloud installations may require http proxy, this commit creates empty values for those settings

lorieri · 2015-02-13T20:29:38Z

@mboersma done

mboersma · 2015-02-13T22:20:07Z

These proxy settings changes and related docs LGTM.

feat(builder+docker+user-data): proxy settings

lorieri changed the title ~~Proxy Settings~~ feat(builder+docker+user-data): proxy settings Dec 30, 2014

lorieri force-pushed the proxy branch 2 times, most recently from d6af240 to 528c3a6 Compare December 31, 2014 00:21

lorieri force-pushed the proxy branch 2 times, most recently from 9e06ba3 to 9e38475 Compare January 3, 2015 22:51

lorieri force-pushed the proxy branch from 9e38475 to 771e0e8 Compare January 12, 2015 01:42

ianblenke reviewed Jan 13, 2015
View reviewed changes

mboersma reviewed Jan 13, 2015
View reviewed changes

lorieri force-pushed the proxy branch from c6d7b5e to ed89657 Compare January 13, 2015 16:09

lorieri force-pushed the proxy branch from ed89657 to 1b480f5 Compare January 13, 2015 16:32

bacongobbler reviewed Jan 13, 2015
View reviewed changes

lorieri force-pushed the proxy branch from 1b480f5 to 2684d33 Compare January 14, 2015 22:16

mboersma self-assigned this Jan 22, 2015

carmstrong added this to the v1.4 milestone Feb 12, 2015

carmstrong assigned carmstrong and unassigned mboersma Feb 12, 2015

lorieri force-pushed the proxy branch from 2684d33 to 31b79bf Compare February 12, 2015 21:17

lorieri force-pushed the proxy branch from 31b79bf to 6bcaa84 Compare February 13, 2015 00:37

docs(managing_deis): add proxy docs

5f6ce0b

lorieri force-pushed the proxy branch from 6bcaa84 to dc112a5 Compare February 13, 2015 00:40

mboersma reviewed Feb 13, 2015
View reviewed changes

lorieri force-pushed the proxy branch from dc112a5 to 1a3d8eb Compare February 13, 2015 20:27

feat(builder): add proxy support

82574fc

Proxy support for builder and coreos' docker daemon Non-cloud installations may require http proxy, this commit creates empty values for those settings

lorieri force-pushed the proxy branch from 1a3d8eb to 82574fc Compare February 13, 2015 20:29

mboersma added a commit that referenced this pull request Feb 13, 2015

Merge pull request #2825 from lorieri/proxy

3d59fef

feat(builder+docker+user-data): proxy settings

mboersma merged commit 3d59fef into deis:master Feb 13, 2015

mboersma mentioned this pull request Feb 17, 2015

fix(tests): source /etc/environment only if it exists #3103

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(builder+docker+user-data): proxy settings #2825

feat(builder+docker+user-data): proxy settings #2825

lorieri commented Dec 30, 2014

lorieri commented Dec 30, 2014

mboersma commented Dec 30, 2014

bacongobbler commented Dec 30, 2014

lorieri commented Dec 30, 2014

bacongobbler commented Dec 30, 2014

bacongobbler commented Dec 30, 2014

bacongobbler commented Jan 2, 2015

carmstrong commented Jan 12, 2015

ianblenke Jan 13, 2015

lorieri Jan 13, 2015

lorieri Jan 13, 2015

carmstrong commented Jan 13, 2015

carmstrong commented Jan 13, 2015

lorieri commented Jan 13, 2015

mboersma Jan 13, 2015

lorieri Jan 13, 2015

lorieri commented Jan 13, 2015

bacongobbler Jan 13, 2015

lorieri Jan 14, 2015

carmstrong commented Feb 12, 2015

lorieri commented Feb 12, 2015

carmstrong commented Feb 12, 2015

lorieri commented Feb 13, 2015

mboersma Feb 13, 2015

bacongobbler Feb 13, 2015

lorieri commented Feb 13, 2015

mboersma commented Feb 13, 2015

feat(builder+docker+user-data): proxy settings #2825

feat(builder+docker+user-data): proxy settings #2825

Conversation

lorieri commented Dec 30, 2014

lorieri commented Dec 30, 2014

mboersma commented Dec 30, 2014

bacongobbler commented Dec 30, 2014

lorieri commented Dec 30, 2014

bacongobbler commented Dec 30, 2014

bacongobbler commented Dec 30, 2014

bacongobbler commented Jan 2, 2015

carmstrong commented Jan 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carmstrong commented Jan 13, 2015

carmstrong commented Jan 13, 2015

lorieri commented Jan 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorieri commented Jan 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carmstrong commented Feb 12, 2015

lorieri commented Feb 12, 2015

carmstrong commented Feb 12, 2015

lorieri commented Feb 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorieri commented Feb 13, 2015

mboersma commented Feb 13, 2015