Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add logwatch maillog.conf file to support /var/log/mail/ #2112

Merged
merged 3 commits into from
Aug 11, 2021

Conversation

andrewlow
Copy link
Contributor

Description

Create a new target/logwatch file tree.
Add a maillog.conf file that configures logwatch to look in the /var/log/mail/* path
Modify Dockerfile to pull from this target and place in the appropriate /etc/logwatch location

Fixes #2108

Type of change

  • Improvement (non-breaking change that does improve existing functionality)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (README.md or the documentation under docs/)
  • If necessary I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

*ExpandRepeats

# Keep only the lines in the proper date range...
*ApplyStdDate
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the '*' a typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These come directly from the /usr/share/logwatch/default.conf/logfiles/maillog.conf file

To create the maillog.conf file that is in my PR is a modified version of the original.

Copy link
Member

@casperklein casperklein Aug 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand. I would remove the comments on the top, because THIS file isn't maintained Kenneth 😉

I guess /usr/share/logwatch/default.conf/logfiles/maillog.conf is read first and extended/overwritten by the local changes made in /etc/logwatch/conf/logfiles/maillog.conf. That said, are the four lines at the end necessary (Line 26-30)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops.. yeah - I should have removed the comment at the top.

I've now reviewed the doc on logwatch http://www.stellarcore.net/logwatch/tabs/docs/HOWTO-Customize-LogWatch.html - and better understand the way to customize it.

Very good review - thank you.

Copy link
Member

@casperklein casperklein Aug 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested it and it seems to work 👍 The daily logwatch mail went from ~60KB to 170KB.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW - I've been running with this change as well for days now.

It is also possible to test this by shelling into the container

docker exec -it mailserver bash
logwatch --range Yesterday

If you have an existing mail server container that has been running for more than 1 day (because until you have a logrotate happen, the problem doesn't manifest itself) - then you can test things being broken.

Then by adding the /etc/logwatch/conf/logfiles/maillog.conf file - testing again, will result in a complete logwatch showing email that is in the /var/log/mail/mail.log.1 file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have an existing mail server container that has been running for more than 1 day

fyi: The (rotated) logs are kept during container restarts (if you use the default docker-compose.yml)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree - a new installation with no rotations (ever) would not have a /var/log/mail/mail.log.1 file.

I should have been more precise that you need to have data in a /var/log/mail/mail.log.1 file to test in a sane way.

@casperklein
Copy link
Member

Would just an additional symlink also work?

/var/log/mail.log.1 -> /var/log/mail/mail.log.1

@andrewlow
Copy link
Contributor Author

andrewlow commented Aug 5, 2021

An additional symlink might work most of the time. Especially with the default 'Yesterday' logwatch parameter.

There are additional archive files which the maillog.conf file specifies, which would not be found.

# ls /var/log/mail
clamav.log       clamav.log.3.gz  freshclam.log.2.gz  mail.err.1   mail.info.2.gz  mail.log.1     mail.log.4.gz  mail.warn.2.gz
clamav.log.1     freshclam.log    freshclam.log.3.gz  mail.info    mail.info.3.gz  mail.log.2.gz  mail.warn      mail.warn.3.gz
clamav.log.2.gz  freshclam.log.1  mail.err            mail.info.1  mail.log        mail.log.3.gz  mail.warn.1

Adding target/logwatch opens up the possibility to add additional logwatch configurations.

@casperklein
Copy link
Member

If nobody complains, I'll merge this in 1-2 days.

@georglauterbach
Copy link
Member

LGTM

@georglauterbach
Copy link
Member

#2109 should be merged first though @casperklein :)

@casperklein
Copy link
Member

casperklein commented Aug 6, 2021

Haha. JUST thought the same. You were some seconds faster.

@casperklein casperklein added this to the v10.1.1 milestone Aug 6, 2021
@wernerfred wernerfred added the meta/feature freeze On hold due to upcoming release process label Aug 7, 2021
@georglauterbach georglauterbach added area/features kind/improvement Improve an existing feature, configuration file or the documentation priority/low and removed meta/feature freeze On hold due to upcoming release process labels Aug 10, 2021
@georglauterbach
Copy link
Member

This should be merged tomorrow, 11th August, when the pipelines for the v10.1.0 release finish successfully.

@wernerfred
Copy link
Member

This should be merged tomorrow, 11th August, when the pipelines for the v10.1.0 release finish successfully.

assuming the fact that the major outage is fixed soon 🚀 https://www.githubstatus.com/

@georglauterbach
Copy link
Member

This should be merged tomorrow, 11th August, when the pipelines for the v10.1.0 release finish successfully.

assuming the fact that the major outage is fixed soon 🚀 https://www.githubstatus.com/

All systems are operational again. I will merge this when all tests have passed.

@georglauterbach georglauterbach merged commit 0e9c988 into docker-mailserver:master Aug 11, 2021
@wernerfred
Copy link
Member

@all-contributors add @andrewlow for code

@allcontributors
Copy link
Contributor

@wernerfred

I've put up a pull request to add @andrewlow! 🎉

@andrewlow
Copy link
Contributor Author

Thanks for all the work on this project - it's been a pleasure to participate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/features kind/improvement Improve an existing feature, configuration file or the documentation priority/low
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Race condition with logwatch and logrotate
4 participants