salt: [BUG] [3006.3] Ubuntu packages destroy existing logrotate config and break log rotation

Description

Due to an apparent oversight, the Ubuntu packages for 3006.3 added logrotate configuration files, but messed up the path. Instead of simply adding the source file as /etc/logrotate.d/salt, the salt-common package contains a directory at the path /etc/logrotate.d/salt, and the source file appears to have been copied into the directory.

Installing Salt will destroy a logrotate config file, if present, at /etc/logrotate.d/salt, and replace it with the new path. This also breaks Salt log rotation, as logrotate does not recurse into subdirs of paths included using the include logrotate config option (so the path /etc/logrotate.d/salt/salt-common.logrotate is never processed by logrotate).

Setup

Just install salt-common on any Ubuntu 22.04 machine. I reproduced this using an ubuntu:22.04 Docker container.

Steps to Reproduce the behavior

root@fa0c54418051:~# cd /etc/logrotate.d/
root@fa0c54418051:/etc/logrotate.d# cat salt
# this is a dummy file
root@fa0c54418051:/etc/logrotate.d# apt -y install salt-common
...
...
...
root@fa0c54418051:/etc/logrotate.d# ls -l
total 16
-rw-r--r-- 1 root root  120 Sep 11  2021 alternatives
-rw-r--r-- 1 root root  173 Apr  8  2022 apt
-rw-r--r-- 1 root root  112 Sep 11  2021 dpkg
drwxr-xr-x 2 root root 4096 Sep 19 12:43 salt
root@fa0c54418051:/etc/logrotate.d# ls -l salt/
total 4
-rw-r--r-- 1 root root 523 Sep  6 16:51 salt-common.logrotate

Additional Notes

To fix this, it will probably be necessary to add code to the preinst script for the salt-common package, which ensures that there is no directory present at /etc/logrotate.d/salt. Otherwise, anyone using logrotate-formula will be unable to manage logrotate confiugrations, as the formula will fail if /etc/logrotate.d/salt is a directory.

Additionally, this is something that should not need to wait for 3006.4, a revision should be able to be added to the Version metadata for the packages (e.g. 3006.3-1).

Finally, other platforms should be checked to see if the same bug affects them. The EL7 RPMs are not affected, but it’s possible that the Debian packages are.

About this issue

Original URL
State: closed
Created 9 months ago
Comments: 16 (16 by maintainers)

Most upvoted comments

@terminalmage - there are packages with my fix here. If you’re able to test those out, that would be much appreciated.

barneysowood on Oct 25, 2023

Well appears to have been an oversight, checked 3002.8 which earliest on the repo for Ubuntu 20.04 and no salt anything in /etc/logrotate.d, will have to defer to the Debian fork for Salt which is /etc/logrotate.d/salt-common

dmurphy18 on Oct 4, 2023

@barneysowood Working on PR https://github.com/saltstack/salt/pull/65290 to do that, the code is easy, writing the packaging test case is taking much much longer 😃 Leaving the correct directories ownership on install etc. to @dwoz 's PR .

And yeah, remember how commands operated in the 70’s (UNIX) then 90’s (gnu) and they do what now ? 🙄 🤣

dmurphy18 on Sep 28, 2023

@dmurphy18 - I hear what you’re saying and understand not everyone wants to run the salt-master as the salt user. However, if we don’t set the ownership of the logs to salt, for those users who do run the salt-master as the salt user (the default) will have breakage next time logrotate runs.

I think the correct thing to do is:

On fresh installs, install the logrotate config as is, with config to set logs to being owned by salt
Document the that the salt-master runs as the salt user by default and logrotate is configured appropriately
Document the process to run the salt-master as another user - edit the master config and logrotate config
On package upgrades, do not overwrite the logrotate config if it has been modified

I think that’s a reasonable way to ensure sensible default behaviour, but respect peoples choices to not use the defaults - let me know what you think.

barneysowood on Sep 27, 2023

This was a mistake on my part in https://github.com/saltstack/salt/pull/64194, I noticed that the logrotate config was being included in the RPM builds but not Debian package builds when updating the logrotate config to set perms on various salt logs to allow the salt user to read/write them. I’d intended that to copy the file, not create a directory under /etc/logrotate.d. I’ll create a fix with the correct handling of the file and an addition to the pre-inst script to fix broken systems. Agree with @terminalmage we should release this as a package update, not wait for a point release.

barneysowood on Sep 19, 2023