I only added this to the agent role. Not sure whether master also needs something like this.

The agent profile manifest (the file modified by this PR) is included in all three buildfarm roles: master, repo, and agent, with different configurations. This will be included in new hosts of all three roles.

It is interesting that you hit this and I didn't. On the buildfarm master and repo hosts there is no file /etc/cron.allow and each user can cron just fine.

The agents are periodically rebuilt based on newer Ubuntu base AMIs so I wonder if they're affected by this.

Given that there are some Ubuntu xenial installations (i.e. mine 😀 ) that don't have this file. I'm slightly apprehensive about creating it in all cases. But I also don't off the top of my head know how to only add the file_line if the file exists. There's no unless metaparameter for resources.

I was surprised as well. I'm running the following:

user@master:~$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 16.04.3 LTS
Release:        16.04
Codename:       xenial

Could you check the output of crontab -l -u jenkins-agent when logged in as root on a master?

@nuclearsandwich wrote:

Given that there are some Ubuntu xenial installations (i.e. mine 😀 ) that don't have this file. I'm slightly apprehensive about creating it in all cases.

I agree, but shouldn't puppet skip creation of the file if it already exists?

Reading the man page better, it seems if /etc/cron.allow does not exist, and there is also no /etc/cron.deny, all users should be able to use cron.

That would make this PR unnecessary.

Now to figure out who/what put that /etc/cron.allow on my VMs.

But I also don't off the top of my head know how to only add the file_line if the file exists.

yes, this is a problem. If /etc/cron.allow exists, jenkins-agent must be added to it, or crontab cannot be used and the job will not be added.

But if it isn't there, we shouldn't create it.

If this is something we need to manage there's already modules for this sort of thing: https://github.com/voxpupuli/puppet-cron Though if it's only injected on non-standard installations then I'd advocate for documenting that it's required but not having it in the default config to keep things as simple as possible. And users with a more complicated setup will need to configure cron appropriately or inject the correct module settings.

I've looked at puppet-cron, but it seems to only support either managing cron.allow completely, or not at all. In my case the file 'somehow' existed, but no users were added to it, leading to crontab complaining (and puppet unable to add jobs).

I think documentation would probably be the better approach here, although checking for the file's existence and then adding the user to it doesn't seem too complex and makes deployment that much smoother.

I agree, but shouldn't puppet skip creation of the file if it already exists?

Skipping if it already exists won't help here since when it doesn't exist, that is the desired state. When it does exist, the desired state is that the $agent_username is in it.

While it would be nicer if puppet had the necessary knobs for us to do this by the book, it's a pretty easy thing to write an exec resource for with a guard command. So I think we can still allow handling of both cases.

Another option would be to enforce that /etc/cron.{allow,deny} do not exist. Which would put the machines into the desired state at the cost of letting these modules be included in larger puppet runs that may want to use those mechanisms.

@nuclearsandwich wrote:

[..] it's a pretty easy thing to write an exec resource for with a guard command. So I think we can still allow handling of both cases.

yes, I was thinking of that as well. Something similar to:

That's a PR I'd review and accept. As I mentioned there's some Melodic bootstrapping work that's taken precedence and I'm not sanguine I'll have time to contribute or test any buildfarm deployments until Friday at the earliest.