Closed Bug 997721 Opened 10 years ago Closed 10 years ago

Support proxies on Ubuntu and OS X

Categories

(Infrastructure & Operations :: RelOps: Puppet, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dustin, Assigned: whimboo)

References

Details

Attachments

(2 files, 5 obsolete files)

In puppetagain, modules/web_proxy/manifests/init.pp only supports CentOS.

Need to
 1. figure out how system-global environment variables are configured on Ubuntu
 2. write the puppet to set the appropriate variables (see environment.erb)
Blocks: 973535
Same for OS X
Summary: Support HTTP proxies on Ubuntu → Support HTTP proxies on Ubuntu and OS X
The Ubuntu change referenced there will work for apt, but will need some lines added to /etc/environment.  Note that unlike on CentOS, there's some content in that file already, so that will need to be preserved.

The "proxy settings" notes for OS X look complete, though.  There's an example of invoking networksetup in modules/network/manifests/init.pp.
Assignee: relops → hskupin
Blocks: 996629
Dustin, how should this module be implemented? Shall it support parameters, so the clients using it will have to customize it? I don't feel that good in hard-coding everything. Especially if different systems want to use it, e.g. SCL3 vs. PHX1, or others.
Summary: Support HTTP proxies on Ubuntu and OS X → Support proxies on Ubuntu and OS X
Ok, looks like that's how I should do it indeed. We have templates as given by Dustin in comment 0. So here some links for reference and easier access.

(In reply to Dustin J. Mitchell [:dustin] (PTO until ~5/20) from comment #0)
> In puppetagain, modules/web_proxy/manifests/init.pp only supports CentOS.

http://hg.mozilla.org/qa/puppet/file/default/modules/web_proxy/manifests/init.pp

>  2. write the puppet to set the appropriate variables (see environment.erb)

http://hg.mozilla.org/qa/puppet/file/default/modules/web_proxy/templates/environment.erb
Attached patch WIP v1 (obsolete) — Splinter Review
This patch is a WIP, which adds support for proxy support on Ubuntu. Also it improves the handling of proxies for CentOS. Dustin, is that going the right direction? At least for me it works perfectly locally.

Also two more questions:

1. Do we have code for OS X which sets up a /etc/profile.d/ folder by puppet? If not I would have to do that. What would be a good place here?

2. Which package management tool is used on CentOS? I assume the proxy stuff has been implemented there already. Its something I have to know, so that I can add proxy support for apt on Ubuntu. You mentioned setup.pp here, but when is it executed? Before web_proxy or afterward? Maybe it should become part of aptrepo.pp?
Attachment #8429237 - Flags: feedback?(dustin)
(In reply to Henrik Skupin (:whimboo) from comment #6)
> 1. Do we have code for OS X which sets up a /etc/profile.d/ folder by
> puppet? If not I would have to do that. What would be a good place here?

Looks like http://hg.mozilla.org/build/puppet/file/31e7ea8b7751/modules/shellprofile/manifests/base.pp#l34 is doing that. So the same solution for Ubuntu and CentOS should also work for Darwin.
Depends on: 1002624
(In reply to Henrik Skupin (:whimboo) from comment #6)
> 2. Which package management tool is used on CentOS? I assume the proxy stuff
> has been implemented there already. Its something I have to know, so that I
> can add proxy support for apt on Ubuntu. You mentioned setup.pp here, but
> when is it executed? Before web_proxy or afterward? Maybe it should become
> part of aptrepo.pp?

Wait. We do not have to get proxy support implemented, given that we have a full mirror of the repo locally, right?
Comment on attachment 8429237 [details] [diff] [review]
WIP v1

Review of attachment 8429237 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking good, yes!

::: manifests/qa-config.pp
@@ +29,5 @@
>      $web_proxy_url = "http://proxy.dmz.scl3.mozilla.com:3128/"
> +    $web_proxy_exceptions = ['localhost', '127.0.0.1', 'localaddress',
> +                             '.localdomain.com', '10.0.0.0/8',
> +                             '.scl3.mozilla.com', '.phx1.mozilla.com', '.mozqa.com',
> +                             'mm-ci-master', 'mm-ci-staging', 'db1',

It would be better if these hosts were referred to by FQDN -- then they would fall under the rules on the line above.

::: modules/web_proxy/manifests/init.pp
@@ +14,4 @@
>                  file {
> +                    "/etc/profile.puppet.d/proxy.sh":
> +                        ensure => present,
> +                        content => template("${module_name}/environment.erb")

I vaguely recall that profile.d only affects *login* shells.  Have you verified that this sets the env var in *all* processes, even those started at system startup?
Attachment #8429237 - Flags: feedback?(dustin) → feedback+
And yes, you're right - package managers don't need to be configured to use the proxies, because all packages should be local.
Ok, so while the system proxy settings are fine for all platforms, we also have to set proxy settings for the gnome applications on Ubuntu. Dustin, I assume it would be better to not combine all three (CentOS, Ubuntu, and OS X) in web_proxy, but keep them separate?
That's correct - separate them with a case $::operatingsystem { .. }
(In reply to Dustin J. Mitchell [:dustin] from comment #9)
> >      $web_proxy_url = "http://proxy.dmz.scl3.mozilla.com:3128/"
> > +    $web_proxy_exceptions = ['localhost', '127.0.0.1', 'localaddress',
> > +                             '.localdomain.com', '10.0.0.0/8',
> > +                             '.scl3.mozilla.com', '.phx1.mozilla.com', '.mozqa.com',
> > +                             'mm-ci-master', 'mm-ci-staging', 'db1',
> 
> It would be better if these hosts were referred to by FQDN -- then they
> would fall under the rules on the line above.

Hm, or would it be enough to add .qa.scl3.mozilla.com? That way it would match all of them, right?

> ::: modules/web_proxy/manifests/init.pp
> @@ +14,4 @@
> >                  file {
> > +                    "/etc/profile.puppet.d/proxy.sh":
> > +                        ensure => present,
> > +                        content => template("${module_name}/environment.erb")
> 
> I vaguely recall that profile.d only affects *login* shells.  Have you
> verified that this sets the env var in *all* processes, even those started
> at system startup?

As what I can see right now on the db1 box which runs CentOS, I do not get the proxy environment variables when I log in via SSH. Those are in /etc/environment. I will manually try to put the proxy lines in /etc/profile, and see how that works. I will also check for system startup. But at least given http://stackoverflow.com/a/3039 it should work for all and everything.
More digging here. On a CentOS host we have already put the proxy env variables in /etc/environment. But when I check processes started during startup, none of them having those variables:

# ps -ef | grep sshd
root      1460     1  0 03:41 ?        00:00:00 /usr/sbin/sshd
# cat /proc/1460/environ 
TERM=linuxSSH_USE_STRONG_RNG=0PATH=/sbin:/usr/sbin:/bin:/usr/binRUNLEVEL=3runlevel=3PWD=/LANGSH_SOURCED=1LANG=en_US.UTF-8PREVLEVEL=Nprevious=NCONSOLETYPE=vtSHLVL=2UPSTART_INSTANCE=UPSTART_EVENTS=runlevelUPSTART_JOB=rc_=/usr/sbin/sshd

Not sure if I'm doing something wrong here. Please let me know if that is the wrong way to check for environment variables set for a process.
(In reply to Henrik Skupin (:whimboo) from comment #13)
> > > +                             'mm-ci-master', 'mm-ci-staging', 'db1',
..
> > It would be better if these hosts were referred to by FQDN -- then they
> > would fall under the rules on the line above.
> 
> Hm, or would it be enough to add .qa.scl3.mozilla.com? That way it would
> match all of them, right?

No, because '.qa.scl3.mozilla.com' is not a suffix of 'mm-ci-master'.  The proxy rules are applied to the strings extracted from URLs, without any DNS operations to qualify them.  So my suggestion was to use fully qualified hostnames in the URLs, and in general use FQDNs everywhere.  I realize that may be hard to do with your automation, so it's really just a suggestion.

> As what I can see right now on the db1 box which runs CentOS, I do not get
> the proxy environment variables when I log in via SSH. Those are in
> /etc/environment. I will manually try to put the proxy lines in
> /etc/profile, and see how that works. I will also check for system startup.
> But at least given http://stackoverflow.com/a/3039 it should work for all
> and everything.

Yeah, something's odd, since that's working on puppetmaster1.  It sounds like some experimentation is called for :)
(In reply to Dustin J. Mitchell [:dustin] from comment #15)
> operations to qualify them.  So my suggestion was to use fully qualified
> hostnames in the URLs, and in general use FQDNs everywhere.  I realize that
> may be hard to do with your automation, so it's really just a suggestion.

Ok, so lets continue here later once all is setup. Given that this configuration currently works, let use it to ensure all is getting implemented properly.

> > As what I can see right now on the db1 box which runs CentOS, I do not get
> > the proxy environment variables when I log in via SSH. Those are in
> > /etc/environment. I will manually try to put the proxy lines in
> > /etc/profile, and see how that works. I will also check for system startup.
> > But at least given http://stackoverflow.com/a/3039 it should work for all
> > and everything.
> 
> Yeah, something's odd, since that's working on puppetmaster1.  It sounds
> like some experimentation is called for :)

As discussed last week, we have seen that none of the processes started during startup actually has any kind of this environment variable. So when I add them to /etc/profile all is working fine for me on CentOS. So I think I will upload a patch which keeps using what I have right now. I will check how it works through my own environment.
Status: NEW → ASSIGNED
Attached patch Proxy environment variables v1 (obsolete) — Splinter Review
Attachment #8429237 - Attachment is obsolete: true
Attachment #8433390 - Attachment description: Patch v1 → Proxy environment variables v1
Comment on attachment 8433390 [details] [diff] [review]
Proxy environment variables v1

Review of attachment 8433390 [details] [diff] [review]:
-----------------------------------------------------------------

I don't see modules/web_proxy/manifests/gui.pp here.  But this is looking good!

::: modules/shellprofile/manifests/file.pp
@@ +2,5 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +define shellprofile::file(
> +    $filename = $title,

This variable isn't used

::: modules/web_proxy/manifests/environment.pp
@@ +7,5 @@
> +        case $operatingsystem {
> +            Darwin,
> +            CentOS,
> +            Ubuntu: {
> +                shellprofile::file { "proxy":

With this in place, the web_proxy module will depend on the shellprofile module.  Is that OK, if you're planning to upload this to puppetforge?

::: modules/web_proxy/manifests/init.pp
@@ +6,5 @@
> +    $url,
> +    $exceptions
> +){
> +    include environment
> +    include gui

We typically use fully-qualified class names, so web_proxy::environment, web_proxy::gui.  I'd entertain changing that habit, if there's a good reason to do so, but I don't think that would change the usefulness of this module to third parties, would it?
Attachment #8433390 - Flags: feedback+
Attached patch Proxy environment variables v1.1 (obsolete) — Splinter Review
So I applied this patch in my environment and instructed puppet agent to pull from it. Applying the proxy environment variables works fine on the db1 host with CentOS. After a restart command line tools like curl are working.

I'm not sure how to deeply test this given that I don't have any application running on startup yet. Dustin, can you help out here? Also do we need extra code to revert /etc/environment for systems, which already have the old web_proxy settings applied?
Attachment #8433390 - Attachment is obsolete: true
Attachment #8433406 - Flags: review?(dustin)
Oh, and as a note the proxy settings for gnome etc will follow later, after this patch landed, and I can setup custom node manifests for our boxes.
Comment on attachment 8433406 [details] [diff] [review]
Proxy environment variables v1.1

See my feedback comments on the last patch.

Also, the only reason 'include gui' is working is that it's including the 'gui' module, which is going to install an X-Windows system on hosts that support it.  that's not quite what you want!  And also a good argument for always using fully qualified class names.

Yes, I think that the "else" case in web_proxy::environment should ensure /etc/environment is absent, along with a comment explaining that an earlier version of this module created it.  We can remove that later, once the file is gone on all affected systems.
Attachment #8433406 - Flags: review?(dustin) → review-
(In reply to Dustin J. Mitchell [:dustin] from comment #21)
> See my feedback comments on the last patch.

Sorry, missed those. See below.

(In reply to Dustin J. Mitchell [:dustin] from comment #18)
> ::: modules/shellprofile/manifests/file.pp
> @@ +2,5 @@
> >  # License, v. 2.0. If a copy of the MPL was not distributed with this
> >  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +define shellprofile::file(
> > +    $filename = $title,
> 
> This variable isn't used

Sorry, missed to actually update the line for file. did that now.

> ::: modules/web_proxy/manifests/environment.pp
> @@ +7,5 @@
> > +        case $operatingsystem {
> > +            Darwin,
> > +            CentOS,
> > +            Ubuntu: {
> > +                shellprofile::file { "proxy":
> 
> With this in place, the web_proxy module will depend on the shellprofile
> module.  Is that OK, if you're planning to upload this to puppetforge?

I know but right now I would like to get it working based on our system. We will need further tweaks and changes to have independent modules. 

> ::: modules/web_proxy/manifests/init.pp
> @@ +6,5 @@
> > +    $url,
> > +    $exceptions
> > +){
> > +    include environment
> > +    include gui
> 
> We typically use fully-qualified class names, so web_proxy::environment,
> web_proxy::gui.  I'd entertain changing that habit, if there's a good reason
> to do so, but I don't think that would change the usefulness of this module
> to third parties, would it?

Nope. I updated those lines now.


> Also, the only reason 'include gui' is working is that it's including the
> 'gui' module, which is going to install an X-Windows system on hosts that
> support it.  that's not quite what you want!  And also a good argument for
> always using fully qualified class names.

The include gui line was by accident. The version of the patch reviewed didn't contain it anymore.

> Yes, I think that the "else" case in web_proxy::environment should ensure
> /etc/environment is absent, along with a comment explaining that an earlier
> version of this module created it.  We can remove that later, once the file
> is gone on all affected systems.

I don't talk about the else case, but CentOS when a proxy is defined. I will add additional code, which will ensure that it will also clean-up the environment file when a proxy is set.
This hopefully contains all the fixes from your previous review.
Attachment #8433406 - Attachment is obsolete: true
Attachment #8433437 - Flags: review?(dustin)
Attachment #8433437 - Flags: review?(dustin) → review+
Comment on attachment 8433437 [details] [diff] [review]
Proxy environment variables v2

https://hg.mozilla.org/qa/puppet/rev/e0875e6b89d8 (default)
https://hg.mozilla.org/qa/puppet/rev/3c112706a9e5 (production)

Tomorrow I will setup the QA node definitions, and will come back to the UI part once that part is running and I can verify by using VNC.
Attachment #8433437 - Flags: checked-in+
Depends on: 1020659
Depends on: 1021230
I have seen something interesting yesterday. While I was checking the changes as coming with my patch on bug 1021230, Firefox was able to access websites outside of SCL3 without having to modify any proxy settings. This may be because we do not use lightdm but XSession directly? I'm not really sure but at least for this platform, and maybe CentOS too, we wouldn't need proxy settings.
Maybe that could even have been changed because we make use of /etc/profile.d now for the proxy settings. I'm interested to see how this works on OS X.
Blocks: 1002624
No longer depends on: 1002624
Attached patch proxy os x v1 (obsolete) — Splinter Review
Ok, so that's the steps, which have to be done for OS X to setup proxies for the GUI. I tested that it only gets run, if the file gets updated, also in case proxies are getting removed.

As talked with Dustin on IRC, he is ok with leaving the file present.
Attachment #8446742 - Flags: review?(dustin)
Comment on attachment 8446742 [details] [diff] [review]
proxy os x v1

Review of attachment 8446742 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good - just r- for the lack of config::base changes.

::: manifests/qa-config.pp
@@ +26,5 @@
>  
>      $ntp_server = "ns1.private.scl3.mozilla.com"
>  
> +    $web_proxy_url = "http://proxy.dmz.scl3.mozilla.com"
> +    $web_proxy_port = "3128"

This change needs an update to modules/config/manifests/base.pp, with some comment indicating that the port is appended to the URL.

::: modules/web_proxy/manifests/gui.pp
@@ +17,5 @@
> +
> +            exec {
> +                "set-proxy-gui" :
> +                    command => "/usr/local/bin/setproxy.sh",
> +                    refreshonly => true;

I'm a little worried here that this won't fix proxy settings if they are changed by some other mechanism after the file is installed.  Maybe that will never be a problem, though.

The more resilient fix would be to use
  onlyif => "/usr/local/bin/setproxy.sh --needed"
and have that script query networksetup to see if the settings are correct.

No need to do this now, but if problems develop, it's a good next step.

::: modules/web_proxy/templates/environment.erb
@@ +5,5 @@
>    # see https://mana.mozilla.org/wiki/display/SECURITY/Configuring+Your+Servers+to+Use+Proxies -%>
>  <% [ 'http_proxy', 'HTTP_PROXY',
>       'https_proxy', 'HTTPS_PROXY',
>       'ftp_proxy', 'FTP_PROXY' ].each do |var| -%>
> +export <%= var %>="<%= @url %>:<%= @port %>"

Will the lack of a trailing slash here cause a problem?
Attachment #8446742 - Flags: review?(dustin) → review-
(In reply to Dustin J. Mitchell [:dustin] from comment #28)
> >      $ntp_server = "ns1.private.scl3.mozilla.com"
> >  
> > +    $web_proxy_url = "http://proxy.dmz.scl3.mozilla.com"
> > +    $web_proxy_port = "3128"
> 
> This change needs an update to modules/config/manifests/base.pp, with some
> comment indicating that the port is appended to the URL.

Oh, good call. I only checked the top manifest folder, but haven't actually found this one. I will update it.

> ::: modules/web_proxy/manifests/gui.pp
> @@ +17,5 @@
> > +
> > +            exec {
> > +                "set-proxy-gui" :
> > +                    command => "/usr/local/bin/setproxy.sh",
> > +                    refreshonly => true;
> 
> I'm a little worried here that this won't fix proxy settings if they are
> changed by some other mechanism after the file is installed.  Maybe that
> will never be a problem, though.
> 
> The more resilient fix would be to use
>   onlyif => "/usr/local/bin/setproxy.sh --needed"
> and have that script query networksetup to see if the settings are correct.
> 
> No need to do this now, but if problems develop, it's a good next step.

Yeah. But when should this happen? I can only be done by an admin of this host. Given that all is under puppet control, everyone should avoid doing that. I will keep that in mind, and add it when we ever hit this problem.

> ::: modules/web_proxy/templates/environment.erb
> @@ +5,5 @@
> >    # see https://mana.mozilla.org/wiki/display/SECURITY/Configuring+Your+Servers+to+Use+Proxies -%>
> >  <% [ 'http_proxy', 'HTTP_PROXY',
> >       'https_proxy', 'HTTPS_PROXY',
> >       'ftp_proxy', 'FTP_PROXY' ].each do |var| -%>
> > +export <%= var %>="<%= @url %>:<%= @port %>"
> 
> Will the lack of a trailing slash here cause a problem?

It shouldn't. But I will add those back.

Also I have to rename web_proxy_url to web_proxy_host. Under OS X network settings you have to specify the host only. It doesn't work with the protocol given. So we can simply add http:// for the environment case.
Attached patch proxy os x v2 (obsolete) — Splinter Review
Implemented the required fix for base.pp, and split-out the protocol. Tested on both Ubuntu and OS X.
Attachment #8446742 - Attachment is obsolete: true
Attachment #8447912 - Flags: review?(dustin)
Comment on attachment 8447912 [details] [diff] [review]
proxy os x v2

Review of attachment 8447912 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/web_proxy/templates/gui_darwin.erb
@@ +1,5 @@
> +<%# This Source Code Form is subject to the terms of the Mozilla Public
> +  # License, v. 2.0. If a copy of the MPL was not distributed with this
> +  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# see https://mana.mozilla.org/wiki/display/SECURITY/Configuring+Your+Servers+to+Use+Proxies -%>

This isn't a public link, so it should probably be omitted.
Attachment #8447912 - Flags: review?(dustin) → review+
Attached patch proxy os x v2.1Splinter Review
(In reply to Dustin J. Mitchell [:dustin] from comment #31)
> > +# see https://mana.mozilla.org/wiki/display/SECURITY/Configuring+Your+Servers+to+Use+Proxies -%>
> 
> This isn't a public link, so it should probably be omitted.

I took this from the existent erb file for the environment. But I removed it now in both files.
Attachment #8447912 - Attachment is obsolete: true
Attachment #8448017 - Flags: review+
http://hg.mozilla.org/qa/puppet/rev/0663c72c7617 (default)
http://hg.mozilla.org/qa/puppet/rev/2c539ad60344 (via merge to production)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 1059141
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: