Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Make proxy settings truly global #7162
Conversation
|
!!test!! |
|
!!test!! |
jameinel
reviewed
Mar 28, 2017
I think this is better than what we have, but ideally I'd like to not have another round of doing this when we find something "better", so I'd like to make sure we're concretely solving the use case. We probably also need to get a CI test involved so that we know that we continue to support getting proxy settings available to applications that have been deployed.
| @@ -111,17 +111,19 @@ func (s *ProxyUpdaterSuite) TestProxyConfig(c *gc.C) { | ||
| ProxySettings: params.ProxyConfig{ |
jameinel
Mar 28, 2017
Owner
does proxyupdater take care to update all the files on disk correctly? The patch looks like we are only setting /etc/juju-proxy.conf from cloud-init, rather than keeping it up-to-date if a user changes anything.
| @@ -1164,17 +1164,16 @@ func (s *cloudinitSuite) TestProxyWritten(c *gc.C) { | ||
| c.Assert(err, jc.ErrorIsNil) | ||
| cmds := cloudcfg.RunCmds() | ||
| - first := `([ ! -e /home/ubuntu/.profile ] || grep -q '.juju-proxy' /home/ubuntu/.profile) || printf '\n# Added by juju\n[ -f "$HOME/.juju-proxy" ] && . "$HOME/.juju-proxy"\n' >> /home/ubuntu/.profile` | ||
| + first := `[ -e /etc/profile.d/juju-proxy.sh ] || printf '\n# Added by juju\n[ -f "/etc/juju-proxy.conf" ] && . "/etc/juju-proxy.conf"\n' >> /etc/profile.d/juju-proxy.sh` |
jameinel
Mar 28, 2017
Owner
I definitely think this is better than what we did before. I don't quite understand why we would create '/etc/juju-proxy.conf' and then source it in /etc/profile.d/juju-proxy.sh instead of just putting the values in /etc/profile.d/* directly.
I'm also not sure whether /etc/environment is better than /etc/profile.d
I tried to do some research, but there didn't seem to be anything definitive. (And actually, the recommendation I found was to hack /etc/pam.conf because more things inherited from there.)
The big asks are that things running inside 'systemd' scripts end up with proxy values set. Have we been able to test various possibilities and make sure our solution actually solves those?
(eg, juju deploy app; will install an application and set up a systemd script to make sure that application is running, we want to make sure that app also sees the http proxy settings.)
|
It looks like we may have a place to put a systemd.d.conf file so we should probably follow up with that, but we could land this first. |
|
It looks like we dropped this one for a bit, do we just need to do systemd, should we land this without changing for systemd yet? |
wupeka
added some commits
Apr 12, 2017
wupeka
changed the title from
Make proxy settings global and disable fallback for apt-*-proxy to *-proxy
to
Make proxy settings truly global
Apr 14, 2017
|
!!retest!! |
jameinel
approved these changes
Apr 20, 2017
The only part I'm worried we may have to revert is forcing the systemd proxy information. I realize people were explicitly asking for that, but I'm concerned that they are being shortsighted. (this charm that I'm trying to use is breaking if I don't set proxy in this circumstance) without knowing about (all these charms break if I do set proxy settings).
But we can try it, and see if it is better than before. I think /etc/juju-proxy.conf is a great idea
| @@ -38,6 +38,7 @@ import ( | ||
| "github.com/juju/juju/service/common" | ||
| "github.com/juju/juju/state/multiwatcher" | ||
| coretools "github.com/juju/juju/tools" | ||
| + "strings" |
| + } | ||
| + if cfg.APIInfo != nil { | ||
| + for _, addr := range cfg.APIInfo.Addrs { | ||
| + hosts = append(hosts, strings.Split(addr, ":")[0]) |
rogpeppe
Apr 21, 2017
Owner
strings.Split isn't sufficient for getting the host part of an address when hosts can be IPv6.
I think you probably want net.SplitHostPort instead.
| + for _, file := range w.config.SystemdFiles { | ||
| + err := ioutil.WriteFile(file, []byte(w.proxy.AsSystemdDefaultEnv()), 0644) | ||
| + if err != nil { | ||
| + logger.Errorf("Error updating systemd file %s - %v", file, err) |
rogpeppe
Apr 21, 2017
Owner
The error from WriteFile will contain the file name, so probably no need to duplicate it:
wupeka
added some commits
Apr 25, 2017
|
Is this blocked on anything? |
wupeka
added some commits
May 1, 2017
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$randomfailure$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$onceagain$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$again$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$andagain$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$onemoretime$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$againandagain$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$doitagain$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$again $$ |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
wupeka commentedMar 27, 2017
•
Edited 1 time
-
wupeka
Apr 14, 2017
Description of change
Make proxy settings global - previously the settings were only used on 'ubuntu' user login shell, with this change the settings are global for all users using /etc/profile.d and, on systems using systemd, also set for services launched using systemd via /etc/systemd/{system,user}.conf.d/juju-proxy.conf.
QA steps
Documentation changes
This change has to be noted in release notes and docs.
Bug reference
https://bugs.launchpad.net/juju/+bug/1666353