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

[terminalmage]

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.

[barneysowood]

This was a mistake on my part in #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.

[dmurphy18]

@barneysowood As mentioned in Slack https://saltstackcommunity.slack.com/archives/CNZKJMQ1E/p1695073531577479?thread_ts=1694114572.419079&cid=CNZKJMQ1E, not everyone wants user: salt, hence cannot be forcing salt as user and group on log rotation, see #65288

[barneysowood]

@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.

[dmurphy18]

@barneysowood Totally disagree, that is a pita for the user. Multiple steps to take just because they prefer master to run as user: hotdog. The aim should be to make things as simple for the user and not have them jump through hoops to make things work. That is, make the machine do the work as far as possible. Having to pull multiple levers reminds me of JCL, when the machine should be able to be made to handle it (oh the nirvana of a DEC-20 after having to run on an IBM 360 😄 ) .

And just because I say 'make the machine do it', doesn't mean I have figured out how just yet, esp. if they install and have not started the salt-master yet).

However, @dwoz has an issue open for all of the various file and directories which need to take a different owner if user is something other than root or salt, #65264 I agree with initially setting to salt, but that can occur differently. The log rotation configuration doesn't have to force salt, the logs rotate with whatever was previously there, hence if it was salt, then rotate with salt, if it was hotdog, then with hotdog, the create 0640 without the id and group is fine.

So we have a number of issues addressing a similar issue, however this issue here, is about Debian/Ubuntu and paths. Mine is about the log rotation configuration file forcing salt.

The current logrotation configure file is wrong, in that it will force new logs to have salt id and group, that has to change, and yes, upgrades should not over-write.

[barneysowood]

@barneysowood Totally disagree, that is a pita for the user. Multiple steps to take just because they prefer master to run as user: hotdog. The aim should be to make things as simple for the user and not have them jump through hoops to make things work. That is, make the machine do the work as far as possible. Having to pull multiple levers reminds me of JCL, when the machine should be able to be made to handle it (oh the nirvana of a DEC-20 after having to run on an IBM 360 😄 ) .

I mean, the user is going to have to take multiple steps if they want to run the salt master as "user: hotdog", as well as changing the master config, they'll need to set all the correct permissions on the various directories and files, which isn't currently correctly documented.

However, @dwoz has an issue open for all of the various file and directories which need to take a different owner if user is something other than root or salt, #65264 I agree with initially setting to salt, but that can occur differently. The log rotation configuration doesn't have to force salt, the logs rotate with whatever was previously there, hence if it was salt, then rotate with salt, if it was hotdog, then with hotdog, the create 0640 without the id and group is fine.

Having just re-read the logrotate man page.. I've realised that the mode/user/group to "create" are optional and it'll take those values from the existing logfile if you omit them (don't think that used to be the case, but probably 20 years since I last used that in anger). I'm absolutely fine with just having the "create" with the mode - we currently create the logfile with correct ownership at install time - I'll need to do some testing and obviously factor in only touching/chmoding that if it doesn't already exist (as per #65264), but that should mean we can just include the "create" option and get the desired behaviour for the default config and for anyone wants to use a user named after a dubious meat snack.

So we have a number of issues addressing a similar issue, however this issue here, is about Debian/Ubuntu and paths. Mine is about the log rotation configuration file forcing salt.

The current logrotation configure file is wrong, in that it will force new logs to have salt id and group, that has to change, and yes, upgrades should not over-write.

Cool, I'll work on a PR to do all of that.

[dmurphy18]

@barneysowood Working on PR #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 ? 🙄 🤣

[barneysowood]

@barneysowood Working on PR #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 .

Yeah, the package tests are where I spent a lot of time on #64194 - running them locally is tricky although I eventually worked out doing it locally in a container.

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

I mean, things were simpler.. doesn't mean things aren't better now :)

[barneysowood]

Opened a draft PR #65318 for this. Will need to add some tests and get some testing done on the built packages before this is ready.

[barneysowood]

@terminalmage - you indicate that your system has an existing logrotate config at /etc/logrotate.d/salt, but testing the 3005.x packages they install it to /etc/logrotate.d/salt-common - do you know how you've ended up with the file being salt not salt-common?

[terminalmage]

@barneysowood I do! The fact is, we've not used anything besides 3006.2 on Ubuntu 22.04, as we've only recently started using 22.04. And, as I mentioned in the OP, we use a modified version of logrotate-formula to manage our logrotate configurations.

With 3006.2, there is no logrotate config in any of the Ubuntu 22.04 salt packages, and the formula we use for managing logrotate configurations gives us control over what the file is called (since it is pillar-driven). In other words, /etc/logrotate.d/salt is there because we put it there, and not from a package installation.

[dmurphy18]

@barneysowood @terminalmage With classic packages (3005.1) on Redhat boxes, it is /etc/logrotate.d/salt. With the Debian fork, it was /etc/logrotate.d/salt-common on their 3002.6+dfsg1-4+deb11u1, as for Salt's 3005.1 on Debian 11, it is MIA and I am investigating has to what happened. The new onedir packaging, just copied from what was there and if classic packaging was wrong, then they would be wrong too. Issue is getting attention and checking earlier version of classic to see, where the drop got made

[dmurphy18]

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

[barneysowood]

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

[terminalmage]

@barneysowood Installing the salt-common package results in a traceback, unless openssl is already installed:

root@4395c111d830:/# dpkg -i salt-common_3006.3+488.g8083bc7fc1_amd64.deb
Selecting previously unselected package salt-common.
(Reading database ... 4413 files and directories currently installed.)
Preparing to unpack salt-common_3006.3+488.g8083bc7fc1_amd64.deb ...
Adding group salt....done
Adding system user salt....done
Unpacking salt-common (3006.3+488.g8083bc7fc1) ...
Setting up salt-common (3006.3+488.g8083bc7fc1) ...
Error processing line 1 of /opt/saltstack/salt/lib/python3.10/site-packages/relenv.pth:

  Traceback (most recent call last):
    File "/opt/saltstack/salt/lib/python3.10/site.py", line 186, in addpackage
      exec(line)
    File "<string>", line 1, in <module>
    File "/opt/saltstack/salt/lib/python3.10/site-packages/relenv/runtime.py", line 969, in bootstrap
      setup_openssl()
    File "/opt/saltstack/salt/lib/python3.10/site-packages/relenv/runtime.py", line 802, in setup_openssl
      proc = subprocess.run(
    File "/opt/saltstack/salt/lib/python3.10/subprocess.py", line 503, in run
      with Popen(*popenargs, **kwargs) as process:
    File "/opt/saltstack/salt/lib/python3.10/subprocess.py", line 971, in __init__
      self._execute_child(args, executable, preexec_fn, close_fds,
    File "/opt/saltstack/salt/lib/python3.10/subprocess.py", line 1738, in _execute_child
      and os.path.dirname(executable)
    File "/opt/saltstack/salt/lib/python3.10/posixpath.py", line 152, in dirname
      p = os.fspath(p)
  TypeError: expected str, bytes or os.PathLike object, not NoneType

Remainder of file ignored
Processing triggers for libc-bin (2.35-0ubuntu3.4) ...

Other than this the package looks good, and I see that the preinst script has code in there to remove /etc/logrotate.d/salt, when it is a directory, which is excellent.

[s0undt3ch]

Traceback (most recent call last):
    File "/opt/saltstack/salt/lib/python3.10/site.py", line 186, in addpackage
      exec(line)
    File "<string>", line 1, in <module>
    File "/opt/saltstack/salt/lib/python3.10/site-packages/relenv/runtime.py", line 969, in bootstrap
      setup_openssl()
    File "/opt/saltstack/salt/lib/python3.10/site-packages/relenv/runtime.py", line 802, in setup_openssl
      proc = subprocess.run(
    File "/opt/saltstack/salt/lib/python3.10/subprocess.py", line 503, in run
      with Popen(*popenargs, **kwargs) as process:
    File "/opt/saltstack/salt/lib/python3.10/subprocess.py", line 971, in __init__
      self._execute_child(args, executable, preexec_fn, close_fds,
    File "/opt/saltstack/salt/lib/python3.10/subprocess.py", line 1738, in _execute_child
      and os.path.dirname(executable)
    File "/opt/saltstack/salt/lib/python3.10/posixpath.py", line 152, in dirname
      p = os.fspath(p)
  TypeError: expected str, bytes or os.PathLike object, not NoneType

saltstack/relenv#165

[dmurphy18]

Closing this since PR #65318 is merged.