Merge ~hloeung/charm-rsyslog-forwarder-ha:master into charm-rsyslog-forwarder-ha:master

Proposed by Haw Loeung
Status: Needs review
Proposed branch: ~hloeung/charm-rsyslog-forwarder-ha:master
Merge into: charm-rsyslog-forwarder-ha:master
Diff against target: 97 lines (+34/-12)
2 files modified
hooks/hooks.py (+25/-4)
templates/keep_local.template (+9/-8)
Reviewer Review Type Date Requested Status
prod-jenkaas-bootstack continuous-integration Needs Fixing
Tianqi Xiao Approve
Eric Chen Needs Fixing
JamesLin Needs Information
Review via email: mp+448028@code.launchpad.net

Commit message

Fix rsyslog permission denied; remove duplicate logging

To post a comment you must log in.
Revision history for this message
Canonical IS Merge Bot (canonical-is-mergebot) wrote :

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

Revision history for this message
prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Robert Gildein (rgildein) wrote :

I'm not expert here, but to me it seems that you commented most of
configuration and that do not looks good to me.

Revision history for this message
JamesLin (jneo8) wrote :

We need more information about this config change.

review: Needs Information
Revision history for this message
Haw Loeung (hloeung) wrote :

First of all, sorry about the delay in replying here.

Logging to these log files are not enabled by default in Ubuntu.

I still need to investigate why the `rsyslog` daemon is failing when
these are enabled but the files don't exist. It's likely AppArmor.

I'm going to update this MP to check if these files exist on disk and
only enable logging to these if they do.

Revision history for this message
Haw Loeung (hloeung) wrote :

On Thu, Aug 17, 2023 at 09:30:20PM -0000, Haw Loeung wrote:
> Logging to these log files are not enabled by default in Ubuntu.
>

See here for what's enabled by default in Ubuntu:

| https://git.launchpad.net/ubuntu/+source/rsyslog/tree/debian/50-default.conf

Revision history for this message
Eric Chen (eric-chen) wrote :

Change the status to WIP until any new commit available.

Revision history for this message
Haw Loeung (hloeung) wrote :

Okay, read for re-review please?

This changes it so it detects the existence of additional log files not normally enabled by default[1]. If they exist on disk, then we update the rsyslog conf to include and write out to those.

[1]https://git.launchpad.net/ubuntu/+source/rsyslog/tree/debian/50-default.conf

Revision history for this message
prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
JamesLin (jneo8) (last edit ):
Revision history for this message
JamesLin (jneo8) wrote :

Overall LGTM. Waiting the CI pass.
Rebuild the CI now.

Revision history for this message
prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :
review: Needs Fixing (continuous-integration)
e6a1d2e... by Haw Loeung

Fix lint errors

Revision history for this message
prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Eric Chen (eric-chen) wrote :

The default value of black is 88, we don't need the additional parameter.
https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#line-length

review: Needs Fixing
Revision history for this message
prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Eric Chen (eric-chen) wrote :

Hi Haw, Can you rebase the code to latest one? Tianqi fixed the function test
https://code.launchpad.net/~txiao/charm-rsyslog-forwarder-ha/+git/charm-rsyslog-forwarder-ha/+merge/452565

59425ea... by Haw Loeung

Merge remote-tracking branch 'hloeung/master'

Revision history for this message
Haw Loeung (hloeung) wrote :

Done

Revision history for this message
prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Tianqi Xiao (txiao) wrote :

Code change LGTM. Re-triggering CI

review: Approve
Revision history for this message
prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :
review: Needs Fixing (continuous-integration)

Unmerged commits

59425ea... by Haw Loeung

Merge remote-tracking branch 'hloeung/master'

e6a1d2e... by Haw Loeung

Fix lint errors

34b8974... by Haw Loeung

Rework additional logs

ebe22fe... by Haw Loeung

Remove duplicate logging for kern.* and mail.*

We're logging kern.* and mail.* to separate log files so no need to
also log them to /var/log/syslog.

6b8844f... by Haw Loeung

Comment out output to files that don't exist by default

This fixes issues with newer hosts and permission denied. E.g.:

Jul 28 00:00:01 csb-fw1 rsyslogd: file '/var/log/cron.log': open error: Permission denied [v8.2112.0 try https://www.rsyslog.com/e/2433 ]
Jul 28 00:00:03 csb-fw1 rsyslogd: file '/var/log/user.log': open error: Permission denied [v8.2112.0 try https://www.rsyslog.com/e/2433 ]
Jul 28 00:00:38 csb-fw1 rsyslogd: file '/var/log/daemon.log': open error: Permission denied [v8.2112.0 try https://www.rsyslog.com/e/2433 ]
Jul 28 00:00:38 csb-fw1 rsyslogd: file '/var/log/messages': open error: Permission denied [v8.2112.0 try https://www.rsyslog.com/e/2433 ]

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/hooks/hooks.py b/hooks/hooks.py
2index 19888c7..bb6a0fb 100755
3--- a/hooks/hooks.py
4+++ b/hooks/hooks.py
5@@ -6,7 +6,6 @@ import base64
6 import os
7 import subprocess
8 import sys
9-from shutil import copyfile
10
11 _HERE = os.path.abspath(os.path.dirname(__file__))
12
13@@ -29,7 +28,7 @@ from charmhelpers.core.host import ( # noqa: E402
14 from charmhelpers.fetch import apt_install # noqa:E402
15
16 from model import Server, session # noqa:E402
17-from utils import get_template_dir, get_template # noqa:E402
18+from utils import get_template # noqa:E402
19 from utils import __die as die # noqa:E402
20
21
22@@ -43,7 +42,6 @@ required_packages = [
23
24
25 IMFILE_FILE = "/etc/rsyslog.d/40-rsyslog-imfile.conf"
26-LOGS_TEMPLATE = "keep_local.template"
27 LOGS_SYSTEM_FILE = "/etc/rsyslog.d/50-default.conf"
28 REPLICATION_FILE = "/etc/rsyslog.d/45-rsyslog-replication.conf"
29 CERT_FILE = "/etc/rsyslog.d/42-cert-rsyslog.conf"
30@@ -88,7 +86,30 @@ def update_imfile(imfiles):
31
32 def update_local_logs(keep=True):
33 if keep:
34- copyfile(os.path.join(get_template_dir(), LOGS_TEMPLATE), LOGS_SYSTEM_FILE)
35+ additional_logs = {
36+ "cron.*": "/var/log/cron.log",
37+ "daemon.*": "/var/log/daemon.log",
38+ "lpr.*": "/var/log/lpr.log",
39+ "user.*": "/var/log/user.log",
40+ "mail.info": "/var/log/mail.info",
41+ "mail.warn": "/var/log/mail.warn",
42+ }
43+ new = {}
44+ for al, logf in additional_logs.items():
45+ if os.path.exists(logf):
46+ new[al] = logf
47+ debug_log = False
48+ if os.path.exists("/var/log/debug"):
49+ debug_log = True
50+ messages_log = False
51+ if os.path.exists("/var/log/messages"):
52+ messages_log = True
53+
54+ with open(LOGS_SYSTEM_FILE, "w") as fd:
55+ tmpl = get_template("keep_local").render(
56+ additional=new, debug_log=debug_log, messages_log=messages_log
57+ )
58+ fd.write(tmpl)
59 else:
60 if os.path.exists(LOGS_SYSTEM_FILE):
61 os.remove(LOGS_SYSTEM_FILE)
62diff --git a/templates/keep_local.template b/templates/keep_local.template
63index 5577674..8c6c76b 100644
64--- a/templates/keep_local.template
65+++ b/templates/keep_local.template
66@@ -4,22 +4,23 @@ kern.* -/var/log/kern.log
67
68 # Some defaults
69 auth,authpriv.* /var/log/auth.log
70-*.*;auth,authpriv.none -/var/log/syslog
71-cron.* /var/log/cron.log
72-daemon.* -/var/log/daemon.log
73-lpr.* -/var/log/lpr.log
74+*.*;auth,authpriv.none,kern.none,mail.none -/var/log/syslog
75 mail.* -/var/log/mail.log
76-user.* -/var/log/user.log
77-
78-mail.info -/var/log/mail.info
79-mail.warn -/var/log/mail.warn
80 mail.err /var/log/mail.err
81
82+{%- for key, log in additional.items() %}
83+{{ key }} -{{ log }}
84+{%- endfor %}
85+
86 # Catch-alls
87+{%- if debug_log %}
88 *.=debug;\
89 auth,authpriv.none;\
90 news.none;mail.none -/var/log/debug
91+{%- endif %}
92+{%- if messages_log %}
93 *.=info;*.=notice;*.=warn;\
94 auth,authpriv.none;\
95 cron,daemon.none;\
96 mail,news.none -/var/log/messages
97+{%- endif %}

Subscribers

People subscribed via source and target branches

to all changes: