factsource crontab PATH conflicting with other cronjobs

[neoice]

https://github.com/puppet-community/puppet-mcollective/blob/master/manifests/server/config/factsource/yaml.pp#L20

apparently, ENV variables set in crontab are global for all cronjobs. I actually run puppet via cron (https://github.com/theforeman/puppet-puppet/blob/master/manifests/agent/service.pp#L38)

now every Puppet run bloats my PATH variable a little more until it looks like this:

X-Cron-Env: <PATH=/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin:/opt/puppet/bin...

[neoice]

I've always wanted native cron.d support in Puppet, now I have another technical reason for why I think cron.d is superior to crontab -e. I'm personally going to look into using https://forge.puppetlabs.com/torrancew/cron for my in-house modules that use cron declarations. I can't have 1 job declaration polluting another job declaration, it destroys the composability of Puppet modules.

[neoice]

here's an example demonstrating env var handling for multiple cronjobs.

https://gist.github.com/neoice/c482659253b187ad91ba

[ffrank]

This is a serious limitation of cron, yes.

But I would argue that the implementation here is flawed. It assumes that refresh-mcollective-metadata should always use a mutation of the PATH as used by the agent. This is a fairly safe assumption, but doesn't necessarily make sense.

Currently not sure how to handle this more cleanly.

[neoice]

the Dell srvadmin tools use this in /etc/profile.d/srvadmin-path.sh:

if ! echo ${PATH} | /bin/grep -q /opt/dell/srvadmin/bin ; then
    PATH=${PATH}:/opt/dell/srvadmin/bin
fi
if ! echo ${PATH} | /bin/grep -q /opt/dell/srvadmin/sbin ; then
    if [ `/usr/bin/id -u` = 0 ] ; then
        PATH=${PATH}:/opt/dell/srvadmin/sbin
    fi
fi

I use a modified script to set an in-house Ruby path, as I was experiencing obnoxious $PATH bloat in my shell:

RUBY_PATH=/usr/ruby/2.1.2/bin

if [ -d ${RUBY_PATH} ]; then
  if ! echo ${PATH} | /bin/grep -q ${RUBY_PATH}; then
    PATH=${RUBY_PATH}:${PATH}
  fi
fi

you might be able to do some kind of trickery with a helper script or a custom fact?

is there any reason to not set it explicitly? mcollectived sets it's own PATH environment very strictly. here's the results of env when run via mco:

{:stdout=>      ["TERM=dumb",       "PATH=/sbin:/usr/sbin:/bin:/usr/bin",       "PWD=/",       "LANG=en_US.UTF-8",       "SHLVL=2",       "_=/usr/sbin/mcollectived",       "HOME=/"],     :stderr=>[]}

proposed fix would be:

environment => "PATH=/opt/puppet/bin:/sbin:/usr/sbin:/bin:/usr/bin",

[ffrank]

Are you saying we should hardcode it? That would be an approach, and quite pragmatic at that.

Here's my order of preference though:

1. Follow the script example from your above comment - the manifest can apply pretty much the same logic to decide whether the ${::path} value needs a prefix
2. Expose the path as a parameter, so that the user can opt to override the default suffix of ${::path}
3. Actually hardcode a sane value

The original code is by @reidmv apparently. Any thoughts? Pinging @richardc as well.

[richardc]

I assume the intent is so this shebang line which is written in terms of /usr/bin/env finds a ruby from /opt/puppet/bin, which only holds valid for PE.

I think it's a bad idea and should be just hardcoded to the $path fact

[reidmv]

The intent probably was to make sure the shebang found the right ruby, as @richardc suggests. If that's all it does, maybe the fix would be to remove the environment => parameter from the cron resource and templatize the refresh-mcollective-metadata script to drop a puppet-version-appropriate shebang.

  $ruby_shebang = $is_pe ? {
    true    => '/opt/puppet/bin/ruby',
    default => '/usr/bin/env ruby',
  }

  # Template uses:
  #   - $yaml_fact_path_real
  #   - $ruby_shebang
  file { "${mcollective::site_libdir}/refresh-mcollective-metadata":
    owner   => '0',
    group   => '0',
    mode    => '0755',
    content => template('mcollective/refresh-mcollective-metadata.erb'),
    before  => Cron['refresh-mcollective-metadata'],
  }
  cron { 'refresh-mcollective-metadata':
    command => "${mcollective::site_libdir}/refresh-mcollective-metadata >/dev/null 2>&1",
    user    => 'root',
    minute  => [ '0', '15', '30', '45' ],
  }

[richardc]

That doesn't feel right at all, for non-PE PATH may still need to be set as cron generally runs with a reduced environment.

I think the correct thing here is leave the template alone, and just set PATH to $::path, with no attempt to extend it.

  cron { 'refresh-mcollective-metadata':
    command => "${mcollective::site_libdir}/refresh-mcollective-metadata >/dev/null 2>&1",
    user    => 'root',
    path    => $::path,
    minute  => [ '0', '15', '30', '45' ],
  }

[ffrank]

Agree, but do add a parameter for this. It is very likely that mco should use the same path as puppet but I would not like to commit this assumption to actual module logic.

[richardc]

Even if mco and puppet are in the same directory, it doesn't hold that ruby is, and that's what it's important for the either the cron to find or the template to be parameterised in terms of.

Under puppet-agent mco and puppet are in both /etc/puppetlabs/bin and /etc/puppetlabs/puppet/bin but only the latter exposes a ruby binary (precisely so you can safely add /etc/puppetlabs/bin to a shell without getting a surprise ruby)

[ffrank]

Which gives me another idea: There are facts to identify PE, right? Should the module respect those and do the "right" thing to the best of its knowledge?

[nibalizer]

I think the template munging is the correct way forward, since it doesn't pollute the environment of the cron. It is a limitation of standard unix cron for sure.

[reidmv]

Can we set the path for the cron job (which it sounds like is desirable) using something that doesn't leak into other jobs? e.g.

cron { 'refresh-mcollective-metadata':
    command => "PATH=${path} {mcollective::site_libdir}/refresh-mcollective-metadata >/dev/null 2>&1",
    user    => 'root',
    path    => $::path,
    minute  => [ '0', '15', '30', '45' ],
  }

It feels like we have two issues we're trying to solve for.

1. Ensure that a correct path environment variable is set for the script to use while running
2. Ensure the correct ruby executable is used when invoking the script

In the previous implementation the path was munged to try and increase the odds the right ruby version was used, and it was set globally. This was messy and inelegant and the side effects of that are I believe what prompted the issue to be opened.

Although I'm open to alternatives, right now it seems to me like the best path forward (if it's technically valid) would be to add a path to the individual cron command as given above, and separately determine which shebang line to use to ensure the correct version of ruby is used to invoke the script.

[nibalizer]

So I merged this, #217 maybe that was a mistake? Should we revert and get consensus?

[reidmv]

I added another proposed PR. #218 re-implements a PATH variable as part of the refresh-mcollective-metadata invocation, but in a manner that won't leak outside of this cron job.

The PR leaves in place for now the shebang line templating, which causes the shebang to be #!/opt/puppet/bin/ruby when running under Puppet Enterprise and #!/usr/bin/env ruby otherwise.

Open for feedback.

[igalic]

closing with the merge of #218