chef_client_cron resource does not include node['chef_client']['cron']['environment_variables'] in cron.d file

[scalp42]

Hi folks,

Just noticed upgrading to 12.0.1 that the cron.d file won't include node['chef_client']['cron']['environment_variables'] anymore.

We use InSpec so we could catch it easily:

  File /etc/cron.d/chef-client
     ✔  is expected to be file
     ✔  is expected to be owned by "root"
     ✔  is expected to be writable
     ✔  content is expected to match /sleep (\d+);/
     ×  content is expected to include "/var/lock/chef_locked"
     expected "# Generated by Chef. Changes will be overwritten.\n\n0,30 3,7,11,15,19,23 * * * root /bin/sleep 296;...l info --force-formatter -c /etc/chef/client.rb > /var/log/chef/var/log/chef/chef-client.log 2>&1\n" to include "/var/lock/chef_locked"
     Diff:
     @@ -1,2 +1,4 @@
     -/var/lock/chef_locked
     +# Generated by Chef. Changes will be overwritten.
     +
     +0,30 3,7,11,15,19,23 * * * root /bin/sleep 296; /opt/chef/bin/chef-client -l info --force-formatter -c /etc/chef/client.rb > /var/log/chef/var/log/chef/chef-client.log 2>&1

     ×  content is expected to include "/bin/nice -n 19"
     expected "# Generated by Chef. Changes will be overwritten.\n\n0,30 3,7,11,15,19,23 * * * root /bin/sleep 296;...l info --force-formatter -c /etc/chef/client.rb > /var/log/chef/var/log/chef/chef-client.log 2>&1\n" to include "/bin/nice -n 19"
     Diff:
     @@ -1,2 +1,4 @@
     -/bin/nice -n 19
     +# Generated by Chef. Changes will be overwritten.
     +
     +0,30 3,7,11,15,19,23 * * * root /bin/sleep 296; /opt/chef/bin/chef-client -l info --force-formatter -c /etc/chef/client.rb > /var/log/chef/var/log/chef/chef-client.log 2>&1

     ✔  content is expected to include "/bin/chef-client"
     ✔  content is expected to include "/var/log/chef/chef-client.log"

Attributes (we didn't change them):

default['chef_client']['cron']['environment_variables'] = [
  'rm -f /var/lock/chef_completed;',
  "[ ! -f '/var/lock/chef_locked' ] &&",
  'PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:$PATH',
].join(' ').strip

Recipe (nothing was changed either):

include_recipe 'chef-client::config'
include_recipe 'chef-client::cron'

I tracked it down to chef_client_cron (https://github.com/chef-cookbooks/chef-client/blob/master/resources/cron.rb#L84-L94) not having any notion of environment variables (which should come after the sleep). For non Linux platforms, it'll work here: https://github.com/chef-cookbooks/chef-client/blob/master/recipes/cron.rb#L114

Workaround is to just drop our own cron_d resource using the cookbook attributes but it'd be neat if upstream could just include it:

In https://github.com/chef-cookbooks/chef-client/blob/master/recipes/cron.rb#L80-L89:

  chef_client_cron 'chef-client cron.d job' do
    minute            node['chef_client']['cron']['minute']
    hour              node['chef_client']['cron']['hour']
    weekday           node['chef_client']['cron']['weekday']
    chef_binary_path  node['chef_client']['cron']['path'] if node['chef_client']['cron']['path']
    mailto            node['chef_client']['cron']['mailto'] if node['chef_client']['cron']['mailto']
    splay             node['chef_client']['splay']
    log_file_name     node['chef_client']['cron']['log_file']
    append_log_file   node['chef_client']['cron']['append_log']
    daemon_options    node['chef_client']['daemon_options']
    # NOTE: inject environment variables from https://github.com/chef-cookbooks/chef-client/blob/master/libraries/helpers.rb#L84-L86
    environment_variables env_vars
  end

In https://github.com/chef-cookbooks/chef-client/blob/master/resources/cron.rb#L84-L94

def cron_command
    # NOTE: convert to array to flatten as new_resource.environment_variables will be an array
    cmd = []
    cmd << new_resource.environment_variables
    cmd << "/bin/sleep #{splay_sleep_time(new_resource.splay)};"
    cmd << "#{new_resource.chef_binary_path}"
    cmd << "#{new_resource.daemon_options.join(' ')}" unless new_resource.daemon_options.empty?
    cmd << "-c #{::File.join(new_resource.config_directory, 'client.rb')}"
    cmd << '--chef-license accept' if new_resource.accept_chef_license && Gem::Requirement.new('>= 14.12.9').satisfied_by?(Gem::Version.new(Chef::VERSION))
    cmd << log_command
    cmd << " || echo \"#{Chef::Dist::PRODUCT} execution failed\"" if new_resource.mailto
    cmd.flatten.join(' ').strip
  end

Cheers!

cc @tas50 for visibility

[scalp42]

node['chef_client']['cron']['priority'] doesn't make it anymore to the cron.d file as well.

[scalp42]

If someone comes across this, here's our workaround in a wrapper cookbook:

  delete_resource(:chef_client_cron, 'chef-client cron.d job')

  chef_client_cron_command = []
  chef_client_cron_command << "/bin/sleep #{Random.rand(node['chef_client']['splay'].to_i)};"
  chef_client_cron_command << node['chef_client']['cron']['environment_variables']
  chef_client_cron_command << "#{node['chef_client']['cron']['nice_path']} -n #{node['chef_client']['cron']['priority']}" if node['chef_client']['cron']['priority']
  chef_client_cron_command << node['chef_client']['bin']
  chef_client_cron_command << node['chef_client']['daemon_options'] unless node['chef_client']['daemon_options'].empty?
  chef_client_cron_command << "-c #{::File.join(node['chef_client']['conf_dir'], 'client.rb')}"
  chef_client_cron_command << (node['chef_client']['cron']['append_log'] ? '>>' : '>')
  chef_client_cron_command << "#{::File.join(node['chef_client']['log_dir'], node['chef_client']['cron']['log_file'])} 2>&1"

  cron_d 'chef-client' do
    minute            node['chef_client']['cron']['minute']
    hour              node['chef_client']['cron']['hour']
    weekday           node['chef_client']['cron']['weekday']
    month             '*'
    mailto            node['chef_client']['cron']['mailto'] if node['chef_client']['cron']['mailto']
    command           chef_client_cron_command.flatten.join(' ').strip
  end

[scalp42]

I don't think this is fixed @ramereth or @tas50

As I mentioned we're not passing down the environment variables to the cron_command. We're passing additional arbitrary environment variables under which the cron job will be run but the behavior is different as node['chef_client']['cron']['environment_variables'] could be used as a hook to do whatever before the actual run.

This is about the environment variables that get passed before the cron command, see our work around:

  chef_client_cron_command = []
  chef_client_cron_command << "/bin/sleep #{splay};"
  chef_client_cron_command << node['chef_client']['cron']['environment_variables']
  chef_client_cron_command << "#{node['chef_client']['cron']['nice_path']} -n #{node['chef_client']['cron']['priority']}" if node['chef_client']['cron']['priority']
  chef_client_cron_command << node['chef_client']['bin']
  chef_client_cron_command << node['chef_client']['daemon_options'] unless node['chef_client']['daemon_options'].empty?
  chef_client_cron_command << "-c #{::File.join(node['chef_client']['conf_dir'], 'client.rb')}"
  chef_client_cron_command << (node['chef_client']['cron']['append_log'] ? '>>' : '>')
  chef_client_cron_command << "#{::File.join(node['chef_client']['log_dir'], node['chef_client']['cron']['log_file'])} 2>&1"

  cron_d 'chef-client' do
    minute            node['chef_client']['cron']['minute']
    hour              node['chef_client']['cron']['hour']
    weekday           node['chef_client']['cron']['weekday']
    month             '*'
    mailto            node['chef_client']['cron']['mailto'] if node['chef_client']['cron']['mailto']
    command           chef_client_cron_command.flatten.join(' ').strip
  end

[ramereth]

@scalp42 I believe you can work around that by putting that in the node['chef_client']['cron']['path'] attribute. We do something like the following in ours: /usr/bin/flock -n /tmp/chef-client.lock /opt/chef/bin/chef-client

[scalp42]

Sure but my point is that the behavior is changed here 🤷‍♂️