Debian: module change ownership of directory /run to unbound
ap-16 opened this issue · 13 comments
Hello,
regarding "name": "zleslie-unbound", "version": "2.4.1", "summary": "Puppet Unbound management module",
I would suggest this patch:
puppet-unbound.patch.txt
Best wishes
ap-16
Hello,
I do not have push access!
puppet-unbound.patch.txt
And yes, IMHO this would solve #180
Best wishes
ap-16
Regarding puppet-unbound Debian rollout, I aimed for 3 targets:
./ Introduce an intermediate directory to store unbound.pid file below /run directory. IMHO should this be state of the art.
./ Leave owner-ship of /run directory to root (this was solved automatically from aim 1).
./ Handle this tmpfile with systemd means (ie. after reboot, when tmpfiles are removed no puppet run should be needed to start unbound). OK, maybe this is better done by an unbound Debian-package maintainer but I've sadly missed it.
@ap-16 yes thanks for the patch i understand what you are doing and think its a completely fine aproch however there are a few minor things i don't like
- changing the Debian default for storing the pid file
- using systemd for this seems overkill
the solution i have in #202 has its own issues which i don't like either. That said im not the maintainer so will wait for @xaque208 to comment
Hi,
You are completely right when you want to obey application defaults, but I have to draw your attention to these defaults with Debian/systemd:
Debian9-host# grep pid /etc/unbound/unbound.conf
pidfile: "/run/unbound/unbound.pid"
Thus my patch fixes this issue. Your value originates from the old systemV init.d setup which isn't in charge any more. Maybe you should remove the entry from modules/unbound/data/os/Debian.yaml as a fix.
Regarding systemd, like it or not: systemd is removing all tmpfiles and tmpdirs when OS is shutting down (where /run directory is part of), thus with systemd means pid-file directory must be setup during boot or service start. Some do it with systemd-tmpfiles facility, I prefer to use the unit.service method.
Whatever you choose as solution, I would be heavily glad to use it.
Best wishes
ap-16
Debian9-host# grep pid /etc/unbound/unbound.conf
pidfile: "/run/unbound/unbound.pid"
I dont know where this is from but i tested stretch and buster and neither of them have this in the config file after a fresh install of unbound. further this bug shows debian changed this a long time ago. finally i also checked the source package for buster and stretch and they both use /run/unbound.pid. Also check https://wiki.debian.org/ReleaseGoals/RunDirectory
Regarding systemd I'm not against it out right however just because systemd can do everything dosn't mean it should do everything.
You'll have to forgive my ignorance, as I've been away from Debian for some years now. Thank you for chasing down the Debian patch @b4ldr. It looks like the default has changed, so I'd suspect that the only change necessary to this module would be to update the default Debian.yaml here https://github.com/xaque208/puppet-unbound/blob/master/data/os/Debian.yaml#L2 . If this was not true in Debian 8, then perhaps an 8.yaml is also in order to avoid breaking users who are on Debian 8 and using this module. The other systemd changes in #209 don't seem to be necessary to me if we only want to relocate the pid dir set by this module. Perhaps a test condition also needs updating.
As for contribution @ap-16, feel free to fork this module, and then you can send a PR from your fork, which we will be able to review.
@xaque208 not sure if you miss read my last coment or if i miss read yours however for clarity
/run/unbound.pidis the correct location for Debian >= 9/var/run/unbound.pidis the correct location in debian <= 8)
so in relation to the current defaults everything is good except that you probably do want to copy Debian/7.yaml -> Debian/8.yaml.
That said there is still the initial bug reported in #180. The patch in #209 would fix this issue by changing the pid file to a debian none standard location *which as you point our could be done in different ways avoiding systemd). the patch in #202 fixes the issues with some hackery (i need to refresh my own brain on what i did there). neither solution is ideal
I've added #210, a simpler variant of #202, with tests.
My first attempt was a $manage_piddir parameter set per OS, but it would have been less friendly to users in case they tried changing the PID file location.