puppetmodule
puppetmodule copied to clipboard
stephenrjohnson
Convention over configuration in default config settings
While implementing this module we noticed that the /etc/puppet/puppet.conf file has a lot of settings specified that are actually just reiterating the defaults. Specifically:
[main] vardir=/var/lib/puppet templatedir=/var/lib/puppet
[master] hiera_config = /etc/puppet/hiera.yaml digest_algorithm = md5 pluginsync = true parser = current environmentpath = /etc/puppet/environments
[agent] environment = production report = true digest_algorithm = md5 listen = false use_srv_records = false reportserver = $server runinterval = 1800 splay = false pluginsync = true server = puppet masterport = 8140
Following convention over configuration principles, wouldn't it make more sense to not place these already defaulted configuration settings in the conf file unless they are overriden with non-default values?
If you think this would make sense, I would be glad to refactor the module to default these configuration settings to undef, then only create the line in the config file if they are not undef. If there's a reason we are intentionally enforcing these to the default settings please let me know and I'll drop this!
I personally find adding loads of if != undef to the module to be more icky than just adding the defaults to the config file. If anyone has a strong option to either way, I'd like to know. I just took the approach of adding the default config items that Debian added to the config file when you first installed puppet.
No worries on the reply time! Thanks for getting back to me :)
I see your point. It would make the module much more complicated, and maybe unnecessarily so. I don't have strong opinions on this either way, but a coworker of mine @kdimiche did when I filed this. I'll see if I can get him to chime in here if he still does.
As I was typing this up, I got to thinking it might be nice if the ini_setting defined type had some logic to either ensure removal of the setting, or not manage the setting upon getting a value of undef. I checked out the ini_setting module on PuppetLabs and it sounds like they recommend making a defined type that inherits from ini_setting if you're wanting to purge settings. Anyway, I'm not sure I need to go down that rabbit hole yet, but it might be an easy way to handle this without adding a lot of bloat into the pp files if other people have strong feelings on this.
Eventually I want to turn on purging of unknown config items from the puppet.conf but I don't feel the module is mature enough to do this.
Yeah, that's a little intimidating. I was thinking something more along the lines of if it's set to undef the entry just doesn't get put in the file, but without having to write all the if != undef all over the file.
My colleague should be chiming in here shortly.
there is an additional problem with if ! undef.
what if you change from value => foo to value => undef?
does the module remove the setting or ignore it?
I'm personally voting me leave the behaviour as it is as its going to increase complexity and we hit the issues of when we should or shouldn't remove a value.
Sounds good to me. Thanks for taking the time to have a conversation on it.
The puppet configuration file follows the convention that for settings not present in the file, it those values get the application default. That being said, it makes sense to allow users to choose what settings appear in the conf file.
Exposing the opportunity for users to specify those settings should be permitted through the module. This is good module practice. It could be seen as making the module "more complex" but really it exposes greater configuration and will foster a more robust module. I'd gladly contribute to the module and the template to make this happen.
By default, only settings present in a clean install of puppet/puppet master should contain have their value defaulted in the class parameter. Otherwise, everything should be 'undef'. Other acceptable values would be value or 'absent'.
If the value is
- 'absent', the line should be removed
- value the line should be written
- 'undef', then it shouldn't be managed
If a setting changes:
- from undef to value, write setting
- from undef to absent, remove remove setting
- from value to undef, do nothing
- from value to absent, remove remove setting
- from absent to undef, do nothing
- from absent to value, write setting
What do you mean by a clean install of puppet/puppet master if you take the config provided by Ubuntu you get
[main]
logdir=/var/log/puppet
vardir=/var/lib/puppet
ssldir=/var/lib/puppet/ssl
rundir=/var/run/puppet
factpath=$vardir/lib/facter
templatedir=$confdir/templates
prerun_command=/etc/puppet/etckeeper-commit-pre
postrun_command=/etc/puppet/etckeeper-commit-post
[master]
# These are needed when the puppetmaster is run by passenger
# and can safely be removed if webrick is used.
ssl_client_header = SSL_CLIENT_S_DN
ssl_client_verify_header = SSL_CLIENT_VERIFY
If you take the puppetlabs provided package you get
[main]
# The Puppet log directory.
# The default value is '$vardir/log'.
logdir = /var/log/puppet
# Where Puppet PID files are kept.
# The default value is '$vardir/run'.
rundir = /var/run/puppet
# Where SSL certificates are kept.
# The default value is '$confdir/ssl'.
ssldir = $vardir/ssl
[agent]
# The file in which puppetd stores a list of the classes
# associated with the retrieved configuratiion. Can be loaded in
# the separate ``puppet`` executable using the ``--loadclasses``
# option.
# The default value is '$confdir/classes.txt'.
classfile = $vardir/classes.txt
# Where puppetd caches the local configuration. An
# extension indicating the cache format is added automatically.
# The default value is '$confdir/localconfig'.
localconfig = $vardir/localconfig
So just between two debs for the same version of puppet, one from Ubuntu and one from Puppetlabs you have two sets of 'default' config.
So what should be the set of default configs (I originally took them from Debian config file), plus have they always been default values for the setting we remove, this module shouldn't just work with the cutting edge version of puppet.
Using absent to remove a line from the config could have it's issues. What happens if you need a setting to be the value 'absent', could this happen? Also what happens to values / sections in the file that we don't know about do we remove them by default?
We could create our own defined resource to contain this logic and wrap the init_setting in that but I'm still trying to figure out what the major problem is with writing a sane set of defaults per the example I used of the debian config is?
@kdimiche wrote:
If the value is
- 'absent', the line should be removed
- value the line should be written
- 'undef', then it shouldn't be managed
within the narrow scope of puppet config files, i would expect unmanaged to mean default, which to me means the same as line should be removed. otherwise we end up with the potential for cruft to build up in the config file.
If we are going to do this, lets do this with
Anything in puppet.conf we don't manage get's nuked. If it's set to anything, it's set to that in the config. We change the parameter to both agent and master to be a config variable. We then use create resource to do a key value, pair into the master and agent sections respectively.
$configoptinsmaster = {'certname' => {'value' => 'wibble'}, { 'autosign' => { 'value' => 'true'}}
$configotionsagent = {{'report' => { 'value' => 'true'},{'splay', {'value' => 'true'}}
class {'puppet::agent':
puppet_run_sytle => 'service',
config => $configotionsagent
}
class {'puppet::master':
user_id => 'puppet',
config => $configoptinsmaster
}
In the puppet::agent class we do
$efaultsagent = { 'classfile' => {'value' => "$vardir/classes.txt"}}
resource {'Ini_setting':
purge => true,
path => $::puppet::params::puppet_conf,
section => 'agent'
}
# When there is a duplicate key, the key in the rightmost hash will "win."
$configitems = merge($defaultsagent, $config)
create_resouce(puppet::agent::config,$configitems,{path => $::puppet::params::puppet_conf})
In the define resource
define puppet::agent::config($value,$section = 'agent',$path)
{
ini_setting{"${name}${section}":
ensure => 'present'
setting => $name,
section => $section,
path => $path,
}
}
This will also allow use to pump in anything through hiera.
i personally like your suggestion in https://github.com/stephenrjohnson/puppetmodule/issues/63#issuecomment-61233673
I agree, great suggestion. I can take a stab at it in the next couple of weeks if everybody wants.
Luckily I have most of December off but feel free to make a stab and I'll merge the best aspects of both of the approaches.