validate_cron can't deal with crontab whose commands may vary

[stevemil00]

Hi. Let's say you have something like this in a puppet manifest:

  cron { "pointlessly-store-my-hostname":
    command => "echo $::hostname > /var/tmp/myhostname",
    user => 'root',
    minute => '*/5',
  }

there's no way to check that using validate_cron().

The cron entries are loaded using get_crontab(), which generates a hash that looks like:

  # runs `crontab -l <user>` and parses output, returns hash:
  # {
  #   user => {
  #     command => {
  #       :minute => minute,
  #       :hour   => hour,
  #       :dom    => dom, # day of month
  #       :mon    => mon, # month
  #       :dow    => dow, # day of week
  #     }
  #   }
  # }

So when you call validate_cron(), you're passing in a user and a command, but unless you can vary the command in the test to match the hostname for everything you're testing, you're kind of stuck.

Sometimes you can vary that in the test, but if you're trying to read a bunch of canned files and cron entries and services and the like in to test, there's probably not a great way to deal with that.

A more practical (well, less pointless) example might be if you needed to fetch some secret dynamically, then stuff that into the cron entry.

If you could validate cronjobs by the puppet name for the cronjob, and/or validate cronjobs by passing in a pattern, that'd help a lot in these cases. For example, the puppet snippet above would stuff something like this in root's crontab:

# Puppet Name: pointlessly-store-my-hostname
*/5 * * * * echo localhost.localdomain > /var/tmp/myhostname

so if you could call validate_cron('root', 'pointlessly-store-my-hostname', expectation) that'd be useful (and then I guess we could rely on the unit tests to check that the cron entry they store is actually right).

Though of course being able to do something like validate_cron('root', 'echo.*/var/tmp/myhostname', expectation) wouldn't be terrible, either.

[chorankates]

hello again @stevemil00!

from your report, i understand that the validate_cron() return doesn't work for your use case, but i'm a little unclear what changes need to be made.

one of the problems/concerns in play here is that we want to support cron entries whether they were created by Puppet or not, so we can't rely on the # Puppet Name: bit.

i was thinking that the last command that you specified (validate_cron('root', 'echo.*/var/tmp/myhostname', expectation) should already be possible, but it isn't - we check for the exact command. we could expand that by looking for names that match the regular expression, but then we have to figure out how to handle the scenario where we have multiple matches.

taking a step back - what would be useful?

• change the return of get_crontab() such that it returns an Array of Hashes, and put the command itself into the array?

{:command=>"foo", :minute=>"0", :hour=>"*", :dom=>"*", :mon=>"*", :dow=>"*"}
{:command=>"echo foo bar baz", :minute=>"0", :hour=>"*", :dom=>"*", :mon=>"*", :dow=>"*"}
{:command=>"echo localhost.localdomain > /var/tmp/myhostname", :minute=>"*/5", :hour=>"*", :dom=>"*", :mon=>"*", :dow=>"*"}

then you could app.get_crontab('vagrant').select { |c| c[:command].match(/myhostname/) } - though that really only helps if the same command has multiple entries in a crontab.

oh, and either way, i think we should standardize on 'crontab', so will rename validate_cron to validate_crontab

[stevemil00]

I'm a little unclear what changes need to be made, too. :)

Like you said, if you change the code to search the array, I think we might end up with an unpleasant side-effect in which we could get matches that wouldn't match now (so we might break existing tests). That might depend on how validate_crontab interacts with the results of get_crontab. I'm having trouble putting a good, crisp example together but I'm thinking of some sort of unexpected prefix match, which is just a variant of what you said about dealing with multiple matches.

If the only thing that uses get_crontab is validate_crontab, you could change what get_crontab returns, to return some variant on:

{
  user => {
    'by_command' => {
      command => {
        :minute => minute,
        :hour   => hour,
        :dom    => dom, # day of month
        :mon    => mon, # month
        :dow    => dow, # day of week
      }
    },
    'by_puppet_name' => {
      puppet_name => {
        :minute => minute,
        :hour   => hour,
        :dom    => dom, # day of month
        :mon    => mon, # month
        :dow    => dow, # day of week
      } 
    },
  } 
}   

You'd only put things with puppet names in the by_puppet_name hash, so you could still support cron entries that aren't installed by puppet.

And validate_crontab could check the by_command hash first, and if it doesn't find it, it can check by_puppet_name.

You could just stuff the puppet name in alongside minute and mon and the like, and leave the structure the way it is currently, then change validate_crontab to do a search through all the entries to try to match the puppet name if the lookup by command fails.

That's not perfect but it might work.

The only other thing I could think of would be to force the user to indicate somehow that something should be a regex match (maybe by passing the slashes in, like validate_cron('root', '/.*whatever.*/', expectation)) so we'd only get multiple matches in a case where presumably the user is OK with that.

Did that help at all?