Remove "su -c" from upstart scripts

[dlanderson]

Starting salt via su should be avoided. The original reason to exec via su was to read in /etc/environment - if this is needed then it should be done by adding ". /etc/environment" to the appropriate defaults file (/etc/default/salt-{minion,master,syndic}.

[ghost]

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2755/

[mgwilliams]

@joehealy @dlanderson @basepi so... this both fixes and breaks http proxy support. By that, I mean this:

• On Ubuntu Precise, this pull-request will break http proxy support hat already works, until the suggested /etc/default file is added.
• On Ubuntu Trusty, the su trick doesn't work anyway, but the new upstart does work (when /etc/default/salt-minion is configured).

As discussed in IRC with @basepi and @gravyboat, I think we need to add the following /etc/default/salt-minion file to the packages that use upstart:

. /etc/environment
export http_proxy=$http_proxy
export https_proxy=$https_proxy
export no_proxy=$no_proxy

Make sense to everyone?

[joehealy]

Do I do this for 2014.1.4 or wait for helium?

On Fri, May 9, 2014 at 8:17 AM, Matthew Williams
notifications@github.comwrote:

@joehealy https://github.com/joehealy @dlandersonhttps://github.com/dlanderson
@basepi https://github.com/basepi so... this both fixes and breaks http
proxy support. By that, I mean this:

• On Ubuntu Precise, this pull-request will break http proxy support
  hat already works, until the suggested /etc/default file is added.
• On Ubuntu Trusty, the su trick doesn't work anyway, but the new
  upstart does work (when /etc/default/salt-minion is configured).

As discussed in IRC with @basepi https://github.com/basepi and
@gravyboat https://github.com/gravyboat, I think we need to add the
following /etc/default/salt-minion file to the packages that use upstart:

. /etc/environmentexport http_proxy=$http_proxyexport https_proxy=$https_proxyexport no_proxy=$no_proxy

Make sense to everyone?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/11523#issuecomment-42613274
.

[mgwilliams]

In IRC we discussed cherry-picking this PR as well as adding the /etc/default file. I think this is best, since Trusty LTS was released and it's broken by default now.

[joehealy]

I have a couple of concerns with this approach:

1. I am a little reluctant to provide a /etc/default/salt-minion file as
   part of the package as a "configuration file"

This will lead to the package upgrade merge config file situation if people
change it

2. I feel that we could/should put . /etc/environment into the upstart
   script directly. Is there a reason not to? Would this achieve the desired
   effect?

3. Without having tested it, I would prefer the following:

Include . /etc/environment in the upstart file

Check for and if it exists, include /etc/default/salt-minion in the upstart
file

Not provide a /etc/default/salt-minion file in the package

What are your thoughts?

Joe

On Fri, May 9, 2014 at 9:20 AM, Joe Healy joehealy@gmail.com wrote:

Do I do this for 2014.1.4 or wait for helium?

On Fri, May 9, 2014 at 8:17 AM, Matthew Williams <notifications@github.com

wrote:

@joehealy https://github.com/joehealy @dlandersonhttps://github.com/dlanderson
@basepi https://github.com/basepi so... this both fixes and breaks
http proxy support. By that, I mean this:

• On Ubuntu Precise, this pull-request will break http proxy support
  hat already works, until the suggested /etc/default file is added.
• On Ubuntu Trusty, the su trick doesn't work anyway, but the new
  upstart does work (when /etc/default/salt-minion is configured).

As discussed in IRC with @basepi https://github.com/basepi and
@gravyboat https://github.com/gravyboat, I think we need to add the
following /etc/default/salt-minion file to the packages that use upstart:

. /etc/environmentexport http_proxy=$http_proxyexport https_proxy=$https_proxyexport no_proxy=$no_proxy

Make sense to everyone?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/11523#issuecomment-42613274
.

[joehealy]

Part of the reasoning behind not providing a /etc/default/minion file is
that it can be provided by salt quite simply. Yes, it will require a
restart to pick it up, but that should be doable.

Joe

On Fri, May 9, 2014 at 9:26 AM, Joe Healy joehealy@gmail.com wrote:

I have a couple of concerns with this approach:

1. I am a little reluctant to provide a /etc/default/salt-minion file as
   part of the package as a "configuration file"

This will lead to the package upgrade merge config file situation if
people change it

2. I feel that we could/should put . /etc/environment into the upstart
   script directly. Is there a reason not to? Would this achieve the desired
   effect?

3. Without having tested it, I would prefer the following:

Include . /etc/environment in the upstart file

Check for and if it exists, include /etc/default/salt-minion in the
upstart file

Not provide a /etc/default/salt-minion file in the package

What are your thoughts?

Joe

On Fri, May 9, 2014 at 9:20 AM, Joe Healy joehealy@gmail.com wrote:

Do I do this for 2014.1.4 or wait for helium?

On Fri, May 9, 2014 at 8:17 AM, Matthew Williams <
notifications@github.com> wrote:

@joehealy https://github.com/joehealy @dlandersonhttps://github.com/dlanderson
@basepi https://github.com/basepi so... this both fixes and breaks
http proxy support. By that, I mean this:

• On Ubuntu Precise, this pull-request will break http proxy support
  hat already works, until the suggested /etc/default file is added.
• On Ubuntu Trusty, the su trick doesn't work anyway, but the new
  upstart does work (when /etc/default/salt-minion is configured).

As discussed in IRC with @basepi https://github.com/basepi and
@gravyboat https://github.com/gravyboat, I think we need to add the
following /etc/default/salt-minion file to the packages that use
upstart:

. /etc/environmentexport http_proxy=$http_proxyexport https_proxy=$https_proxyexport no_proxy=$no_proxy

Make sense to everyone?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/11523#issuecomment-42613274
.

[mgwilliams]

I agree with the concern about the /etc/default file. If we embed it in the upstart script, I believe the exports are also necessary, unfortunately. I could not get simply . /etc/environment to work without explicitly exporting the http_proxy vars.

[joehealy]

I don't have access to my testing VMs today - I'll compare the various
versions of upstart and su (precise and trusty) tonight.

Joe

On Fri, May 9, 2014 at 9:31 AM, Matthew Williams
notifications@github.comwrote:

I agree with the concern about the /etc/default file. If we embed it in
the upstart script, I believe the exports are also necessary,
unfortunately. I could not get simply . /etc/environment to work without
explicitly exporting the http_proxy vars.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/11523#issuecomment-42618875
.

[basepi]

For the record, this commit is not in any salt release. After talking to @mgwilliams in IRC, I marked it to cherry-pick in the future, but I'll wait pending the decisions from this thread.

[gravyboat]

@joehealy @basepi What ended up happening with this? It seems like it is in 2014.1.4.

[joehealy]

I was sure I had fixed this, though it looks like I have only half done so.

I will update the packages today.

Joe

On Thu, May 29, 2014 at 8:10 AM, Forrest notifications@github.com wrote:

@joehealy https://github.com/joehealy @basepihttps://github.com/basepiWhat ended up happening with this?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/11523#issuecomment-44471353
.

[gravyboat]

@joehealy One of my coworkers actually installed 2014.1.4-2 for precise today and received this on a file diff for /etc/default/salt-minion:

*** salt-minion (Y/I/N/O/D/Z) [default=N] ? D
--- /etc/default/salt-minion    2014-05-01 00:10:58.118667127 +0000
+++ /etc/default/salt-minion.dpkg-new   2014-05-15 00:55:38.000000000 +0000
@@ -1,4 +1,9 @@
+# export the http_proxy environment variables
+# see https://github.com/saltstack/salt/pull/11523 for details
+
 . /etc/environment
 export http_proxy=$http_proxy
 export https_proxy=$https_proxy
 export no_proxy=$no_proxy
+
+
(END)

So it seems to me as though part of it was updated at least.

[joehealy]

Had done it for master and syndic, but not the important one...

On Thu, May 29, 2014 at 9:07 AM, Joe Healy joehealy@gmail.com wrote:

I was sure I had fixed this, though it looks like I have only half done so.

I will update the packages today.

Joe

On Thu, May 29, 2014 at 8:10 AM, Forrest notifications@github.com wrote:

@joehealy https://github.com/joehealy @basepihttps://github.com/basepiWhat ended up happening with this?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/11523#issuecomment-44471353
.

[joehealy]

Yeah - that was the bit I got - I missed the removing su -c from the
upstart file.

Looking now, I think it may have been an issue with me getting the hang of
git-buildpackage - I think I lost it in a merge.

On Thu, May 29, 2014 at 9:11 AM, Forrest notifications@github.com wrote:

@joehealy https://github.com/joehealy One of my coworkers actually
installed 2014.1.4-2 for precise today and received this on a file diff for
/etc/default/salt-minion:

*** salt-minion (Y/I/N/O/D/Z) [default=N] ? D
--- /etc/default/salt-minion    2014-05-01 00:10:58.118667127 +0000
+++ /etc/default/salt-minion.dpkg-new   2014-05-15 00:55:38.000000000 +0000
@@ -1,4 +1,9 @@
+# export the http_proxy environment variables
+# see #11523 for details
+
. /etc/environment
export http_proxy=$http_proxy
export https_proxy=$https_proxy
export no_proxy=$no_proxy
+
+
(END)

So it seems to me as though part of it was updated at least.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/11523#issuecomment-44476437
.

[joehealy]

Fixed in the following package versions:

• 2014.1.4+ds-2trusty3
• 2014.1.4+ds-2saucy4
• 2014.1.4-2precise2
• 2014.1.4-2lucid2

lucid and precise still building right now.

Thanks for your patience.

Joe

[gravyboat]

@joehealy thanks!

[petarmaric]

I got bit pretty bad by this pull request today.

In our lab we must use proxies to access the internet.
Our Ubuntu 12.04 salt master server has been keeping up with salt-* Debian packages on a regular basis for the last few months - which means it picked up the version which packaged /etc/default/salt-minion.

However, our freshly salted workers (using salt-minion 2014.1.5) don't have /etc/default/salt-minion.

Because of this the salt-minion on the master server had no problems using pip.installed states, unlike the workers which were having proxy issues:

$ sudo salt '*' cmd.run "env | grep -i proxy" --show-timeout
master:
    no_proxy=localhost,master,salt,127.0.0.0/8,192.168.18.0/24
    https_proxy=https://192.168.18.1:8080/
    http_proxy=http://192.168.18.1:8080/
s200:

It would be nice to document this issue, not unlike #9491, to prevent other people from being bitten by this bug.

To be quite honest, I don't understand why we just don't ship the /etc/default/salt-minion which parses /etc/environment for *_proxy env vars - or even do the /etc/environment parsing within /etc/init/salt-minion.conf directly. Seems like a sane default to me.

References:

• http://serverfault.com/questions/128605/have-upstart-read-environment-from-etc-environment-for-a-service
• http://ubuntuforums.org/showthread.php?t=2134156
• fixing issues with passing env var to salt-minion,master,syndic #5937

[basepi]

@petarmaric I'll preface this with the statement "I know very little about upstart"

Can you clarify what you think the solution is? Is there a workaround that worked for you? Do we need to add the su -c back in? Is it just a documentation issue?

[gravyboat]

@basepi basically we need to add /etc/default/salt-minion to the package to realistically resolve this issue so the proxy values are properly exported BY DEFAULT, instead of requiring users to add it themselves. @petarmaric feel free to correct me if you believe this to be incorrect.

[basepi]

Can one of you create a new issue for this fix? Don't want to lose it in the shuffle.

[petarmaric]

I think @gravyboat's proposed solution is spot on, creating a new issue now.

[petarmaric]

Created the issue #13786

[joehealy]

Not sure a new issue is needed for that one - happy to use it if needed. I
could have sworn I did that and have the git commit to prove it [1] :( -
problem looks to be that I forgot to add the file :( :(. Sorry about that.
I'll do a new release tonight.

Joe

[1]
http://anonscm.debian.org/gitweb/?p=pkg-salt/salt.git;a=commit;h=de13b30ff90fb5d922fc0c3556703aff9eb8cd36

On Fri, Jun 27, 2014 at 1:50 PM, petarmaric notifications@github.com
wrote:

I think @gravyboat https://github.com/gravyboat's proposed solution is
spot on, creating a new issue now.

—
Reply to this email directly or view it on GitHub
#11523 (comment).

[gravyboat]

@joehealy I think this was turning into a discussion regarding the /etc/default/salt-minion file. Is that what you're referencing? Or the su -c option?

[petarmaric]

@joehealy Please remember #13786 adds ftp_proxy as well.

[joehealy]

@petarmaric, saw that. Thanks.

Re @forrest - yeah - meant to commit the file, but obviously didn't add it.

Joe

On Fri, Jun 27, 2014 at 2:13 PM, petarmaric notifications@github.com
wrote:

@joehealy https://github.com/joehealy Please remember #13786
#13786 adds ftp_proxy as well.

—
Reply to this email directly or view it on GitHub
#11523 (comment).