pid dir permissions could cause problems
b4ldr opened this issue · 6 comments
the unbound manifest tries to set the permissions of the pid file base directory[1]. For debian this is set to /run/unbound.pid[2]. As such puppet tries to set the dir permissions for /run to unbound which is not desirable. for now im going to just going to set the value to an empty dir as unbuound on debian is compiled with the same default.
Im not sure if this is a bug or desirable, especially if we consider chroot environments. for now i have just set unbound::pidfile: ~ however i did need to patch the module[3] to get that to work. also note that dirname doesn't act as i expected when pidfile was blank[4]
[1]https://github.com/xaque208/puppet-unbound/blob/master/manifests/init.pp#L178-L180
[2]https://github.com/xaque208/puppet-unbound/blob/master/data/os/Debian.yaml#L2
[3]#179
[4]puppetlabs/puppetlabs-stdlib#913
Maybe the permissions for the pid dir could only get set if the directory name ends with unbound? Just a thought.
fixes for this got merged
also worth pointing out that the fixes to stdlib basedir have also been merged. can you cut a new release.
Cheers
Yep, can do.
I'm not sure #179 is the right fix for this. Regardless of the changes in it, the module by default (on Debian) is still changing the owner of /run to unbound which is definitely undesirable.
I'm not sure the suggestion in the comments earlier is great. It makes a lot of assumptions.
Is the only distro that actually requires something like this (at least out of the box -- where the run directory isn't an OS provided one), FreeBSD? That's all I can spot from the data files.
I think freebsd creates the directory as part of the port. I can check later. OpenBSD ships with unbound by default, as does FreeBSD, so I'd assume the directories needed for the base unbound would not be an issue. We could do something like not manage the permissions, but then we're not able to guarantee that the daemon can read the files necessary.
@chrisboulton Is there another action you're suggesting we take here?
@chrisboulton first please send code if you have it.
Now I am unsure as to what assumptions you think i made. however let me try to address your comments
Regardless of the changes in it, the module by default (on Debian) is still changing the owner of /run to unbound
That's right i did not change the default behavior. As i stated in the issues i was unsure if the default behavior was desired or not. I simply provided a method to allow people to override this parameter locally and set it to an empty value so the error did not manifest
Is the only distro that actually requires something like
the list of supported os's is in the metadata and you can easily see where each os stores its pidfile
$ grep -r pidfile puppet-unbound/data
puppet-unbound/data/common.yaml:unbound::pidfile: '/var/run/unbound/unbound.pid'
puppet-unbound/data/os/Debian/7.yaml:unbound::pidfile: '/var/run/unbound.pid'
puppet-unbound/data/os/Solaris/SmartOS.yaml:unbound::pidfile: '/usr/local/etc/unbound/unbound.pid'
puppet-unbound/data/os/OpenBSD.yaml:unbound::pidfile: '/var/run/unbound.pid'
puppet-unbound/data/os/FreeBSD.yaml:unbound::pidfile: '/usr/local/etc/unbound/unbound.pid'
puppet-unbound/data/os/Debian.yaml:unbound::pidfile: '/run/unbound.pid'
as you can see from here the only pid file in a none unbound specific folder is Debian != version 7 and OpenBSD. so by default the following supported systems would be affected
- Debian 6
- Ubuntu 14.04
- Ubuntu 16.04
- OpenBSD 5
- OpenBSD 6
As to the way forward TBH im not sure why i ever questioned if this was desirable, it obviously is not but i think any way forward is a hack. As @xaque208 mentioned we do need to manage this folder in many situations as such i have created a PR with a possible way forward #190
EDIT: the other option is to just remove trying to work out and set permissions on basedire($pidfile) if its different from rundir with the assumption that in that case something elses will take care of it???