puppet-redis
puppet-redis copied to clipboard
voxpupuli
Puppet did not detect changes in `/etc/redis.conf` neither reapplied the template
What are you seeing (the issue)
Someone modified /etc/redis.conf and put bogus values (either security issue or other guy mistake, or simply forgotten manual configuration change on machine), and future runs of Puppet never detected the change nor reapplied the correct configuration. I even myself spent a bit of time trying to figure out why Puppet runs did not recreated the file even when i deleted it from the machine
What behaviour did you expect instead
Puppet would
- detect the
/etc/redis.conffile has been modified - optionnally output the diff
- reapply the template to recreate a correct file
Why
Because there: https://github.com/voxpupuli/puppet-redis/blob/d0ee587d0d0327ef85d5701fbd6c9d9f33569fbe/manifests/instance.pp#L312
This is not the real configuration file which is checked by Puppet, but rather a substitute file ! Since the real /etc/redis.conf file is not managed by a Puppet ressource, Puppet does not detect any changes, nor shows diff !
How in the world did this get approved by puppet? Isn't the whole point of puppet is that if someone hand edits a config file, puppet will automatically put it back?
My colleague and I wasted an hour trying to figure out why the module wasn't "fixing" the config file after someone hand edited it - then we came across this. :(
Looks like it's been coded this way so that it's compatible with Sentinel. https://github.com/voxpupuli/puppet-redis/commit/1c004143223e660cbd433422ff8194508aab9763#diff-0ac866172efd29d0ddc2aa75ae6f182d
This!
Looks like it's been coded this way so that it's compatible with Sentinel. 1c00414#diff-0ac866172efd29d0ddc2aa75ae6f182d
Because Sentinel rewrite the /etc/redis.conf at runtime, so puppet should not write in /etc/redis.conf
More generally, if an administrator has altered any configuration at runtime (using CONFIG SET), then either when they (or sentinel) issues CONFIG REWRITE, the redis.conf will be rewritten.
Maybe a new strict_enforce_config option or something would be useful to users not using sentinel?
Anything fancier would probably need someone to write a native type/provider (especially if you want puppet to be able to reconfigure redis without restarting it)
PRs and other suggestions welcome.
I would propose that the default behavior be to enforce file compliance unless you are using a sentinel configuration. Code could check to see if $redis::sentinel::service_enable is defined and true to use the temporary file else use the normal config file.
I agree with all of the above, another suggestion is to use the "extra_config_file" option which you can point to another additional config file that will be included from redis.conf. You can manage that file seperatly and manage it with Puppet. This doesn't solve the fact the source file still isn't managed and can be manually changed.
Maybe a new
strict_enforce_configoption or something would be useful to users not using sentinel?Anything fancier would probably need someone to write a native type/provider (especially if you want puppet to be able to reconfigure redis without restarting it)
PRs and other suggestions welcome.
Absolutely agreeing with that.
In addition to that:
I agree with all of the above, another suggestion is to use the "extra_config_file" option which you can point to another additional config file that will be included from redis.conf.
Absolutely right. But one could simply leave ${redis_file_name} empty to begin with (which would then only contain manual configuration, configuration rewrites by sentinels and state information) and only have an include ${redis_file_name_orig} in there from the start. I see no need for a new parameter. Obviously, the cp -p magic needs be removed in that case.
Same goes for Sentinels, btw. Right now, if you use this module for a Redis / Sentinel combo, every config change after a failover situation will lead to dangerous situations when the state information disappears due to the cp -p for sentinel.conf.
Cheers Thomas
