Bug 55610 - New listener modules may break due to log rotate
New listener modules may break due to log rotate
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: Listener (univention-directory-listener)
UCS 5.0
Other Linux
: P5 normal (vote)
: UCS 5.0-4-errata
Assigned To: Mika Westphal
Christian Castens
https://git.knut.univention.de/univen...
:
: 52419 52503 (view as bug list)
Depends on:
Blocks: 56459
  Show dependency treegraph
 
Reported: 2023-01-30 10:39 CET by Dirk Wiesenthal
Modified: 2023-10-18 08:40 CEST (History)
10 users (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 5: Major Usability: Impairs usability in key scenarios
Who will be affected by this bug?: 2: Will only affect a few installed domains
How will those affected feel about the bug?: 5: Blocking further progress on the daily work
User Pain: 0.286
Enterprise Customer affected?:
School Customer affected?: Yes
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional):
Max CVSS v3 score:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Wiesenthal univentionstaff 2023-01-30 10:39:39 CET
We found log files in /var/log/univention/listener_modules/ that had the ownership

root:adm

This breaks the listener module as we get a PermissionDenied exception during import time of the listener. Thus, the whole module is just skipped.

The problem seems to be the usage of python's built in TimedRotatingFileHandler. I do see code that should set the ownership to listener:adm - but it failed? I don't know. What I do know is that when the ownership is wrong, this can be a very critical bug (the group cache listener module uses the new API).

I suggest letting the system do the log rotation (via UCR integration) and not do it in python directly?

See also Bug#52419
Comment 3 Daniel Tröder univentionstaff 2023-02-14 17:44:47 CET
(In reply to Dirk Wiesenthal from comment #0)

> I suggest letting the system do the log rotation (via UCR integration) and
> not do it in python directly?

FYI: We have code that can handle file permissions in the logging module, which could be adapted to handle this problem in the listener module: https://git.knut.univention.de/univention/ucsschool/-/blob/5.0/ucs-school-lib/modules/ucsschool/lib/models/utils.py#L173

But I would also vote for using the host systems log rotation system and integration.
Comment 5 Julia Bremer univentionstaff 2023-08-21 09:13:07 CEST
Since this change this test fails because the new log files are world readable.
Could you take a look?

https://jenkins2022.knut.univention.de/job/UCS-5.0/job/UCS-5.0-4/job/AutotestJoin/93/SambaVersion=s4,Systemrolle=master/testReport/01_base/27check_logfiles_univention/master091/
Comment 6 Mika Westphal univentionstaff 2023-08-21 12:35:05 CEST
*** Bug 52419 has been marked as a duplicate of this bug. ***
Comment 7 Mika Westphal univentionstaff 2023-08-21 12:37:07 CEST
*** Bug 52503 has been marked as a duplicate of this bug. ***
Comment 8 Florian Best univentionstaff 2023-08-22 03:20:19 CEST
Please discuss in the team if we should use a generic approach for all logfiles underneath of /var/log/univention/listener_modules/ instead of requiring every listener module using the new API to also include a logrotation config.
Comment 9 Mika Westphal univentionstaff 2023-08-22 12:59:25 CEST
(In reply to Florian Best from comment #8)
> Please discuss in the team if we should use a generic approach for all
> logfiles underneath of /var/log/univention/listener_modules/ instead of
> requiring every listener module using the new API to also include a
> logrotation config.

We don't need to discuss this any more, as it was already decided on Friday on Git by the PO. This has already been implemented and is in QA.
Comment 10 Julia Bremer univentionstaff 2023-08-22 19:50:48 CEST
(In reply to Julia Bremer from comment #5)
> Since this change this test fails because the new log files are world
> readable.
> Could you take a look?
> 
> https://jenkins2022.knut.univention.de/job/UCS-5.0/job/UCS-5.0-4/job/
> AutotestJoin/93/SambaVersion=s4,Systemrolle=master/testReport/01_base/
> 27check_logfiles_univention/

I re-reopened the bug.

The test 01_base/27check_logfiles_univention still fails on each machine role every night because the log files are world readable.

Either you (and your team) decide that it is ok that they are world readable and you fix the tests (I doubt that since all other log files are only readable by root)
or you fix the read permissions of that file.  Either way some action is needed before this can be released
Comment 11 Mika Westphal univentionstaff 2023-08-23 12:37:27 CEST
My bad, I forgot the comment with the new MR when I set it to `Resolved Moved`. The fix is already in QA (https://git.knut.univention.de/univention/ucs/-/merge_requests/868) and it also contains a default configuration for logs under `/var/log/univention/listener_modules`
Comment 12 Mika Westphal univentionstaff 2023-08-25 11:14:27 CEST
The log rotation for the listener module logs is now done by logrotate and it can be configured via UCR.

univention-radius.yaml
61d43606f2e2 | Revert "Bug #55610: added radius logrotate configuration and a UCR variable"
16068322b675 | Bug #55610: univention-radius 7.0.5-5
e5111c40f39c | Bug #55610: added radius logrotate configuration and a UCR variable

univention-radius (7.0.5-5)
61d43606f2e2 | Revert "Bug #55610: added radius logrotate configuration and a UCR variable"
e5111c40f39c | Bug #55610: added radius logrotate configuration and a UCR variable

univention-portal.yaml
2272cb482a2c | Revert "Bug #55610: added univention-portal logrotate configuration and a UCR variable"
0169201d5161 | Bug #55610: univention-portal 4.0.14-5
8c336a2fd6aa | Bug #55610: added univention-portal logrotate configuration and a UCR variable

univention-portal (4.0.14-5)
2272cb482a2c | Revert "Bug #55610: added univention-portal logrotate configuration and a UCR variable"
8c336a2fd6aa | Bug #55610: added univention-portal logrotate configuration and a UCR variable

univention-group-membership-cache.yaml
f4cb56b9e6f8 | Revert "Bug #55610: added univention-group-membership-cache logrotate configuration and a UCR variable"
0750d5780c2a | Bug #55610: univention-group-membership-cache 2.0.1-2
d06f3bfdd9a9 | Bug #55610: added univention-group-membership-cache logrotate configuration and a UCR variable

univention-group-membership-cache (2.0.1-2)
f4cb56b9e6f8 | Revert "Bug #55610: added univention-group-membership-cache logrotate configuration and a UCR variable"
d06f3bfdd9a9 | Bug #55610: added univention-group-membership-cache logrotate configuration and a UCR variable

univention-directory-listener.yaml
e8e7b01f2745 | Bug #55610: Fixed a failing permissions test and added a default logrotate configuration
a377b71aa4c1 | Bug #55610: univention-appcenter 9.0.8-4
d90b32aae478 | Bug #55610: replaced TimedRotatingFileHandler with WatchedFileHandler

univention-directory-listener (14.0.8-5)
e8e7b01f2745 | Bug #55610: Fixed a failing permissions test and added a default logrotate configuration

univention-directory-listener (14.0.8-4)
d90b32aae478 | Bug #55610: replaced TimedRotatingFileHandler with WatchedFileHandler

univention-appcenter.yaml
7368c95627d6 | Revert "Bug #55610: added appcenter logrotate configuration and a UCR variable"
a377b71aa4c1 | Bug #55610: univention-appcenter 9.0.8-4
38cb0823c9e9 | Bug #55610: added appcenter logrotate configuration and a UCR variable

univention-appcenter (9.0.8-4)
7368c95627d6 | Revert "Bug #55610: added appcenter logrotate configuration and a UCR variable"
38cb0823c9e9 | Bug #55610: added appcenter logrotate configuration and a UCR variable
Comment 13 Mika Westphal univentionstaff 2023-08-29 09:30:44 CEST
The Logrotate configuration of the listener module log files can now be configured via two new Univention Configuration Registry Variables:
- Global settings: `logrotate/listener-modules/<directive>`
- Specific settings: `logrotate/listener-modules/<logfile name without file extension>/<directive>`

Supported Logrotate directives:
  - rotate
  - rotate/count
  - create
  - missingok
  - compress
  - notifempty

The default settings are
  - rotate: weekly
  - rotate/count: rotate 12
  - create: create 640 listener adm
  - missingok: missingok
  - compress: compress
  - notifempty: notifempty
Comment 14 Daniel Tröder univentionstaff 2023-08-29 10:57:01 CEST
What is the behavior, if _nothing_ is configured for an app?
→ Is this a feature that apps can adopt, or will it be auto-enabled for all apps?
Comment 15 Mika Westphal univentionstaff 2023-08-29 15:38:49 CEST
(In reply to Daniel Tröder from comment #14)
If nothing is set, default settings are used to allow the rotation of the logs anyway. This feature is automatically enabled for all apps that have listener modules.
Comment 16 Mika Westphal univentionstaff 2023-08-29 15:39:29 CEST
(In reply to Mika Westphal from comment #12)
5439fe15c52b | Bug #55610: German documentation
2ca80165b0e9 | Bug #55610: English documentation
Comment 17 Christian Castens univentionstaff 2023-08-29 16:01:01 CEST
QA:
  - logfile permissions can no longer cause the listener module to break and are adjusted correctly if needed: OK

  - a default logrotate config for every logfile in `/var/log/univention/listener_modules/` exists: OK
  - the UCR var `logrotate/listener-modules/<directive>` correctly modifies the logrotate configuration of all listener module log files:  OK
  - the UCR var `logrotate/listener-modules/<logfile name without file extension>/<directive>` correctly modifies the logrotate configuration of individual listener module log files:  OK

  - Code review: OK
  - Advisories:  OK
  - Documentation: OK
  - Tests:  OK