mail/mailman: Use .conf suffix for /usr/local/etc/newsyslog.conf.d/

[woodsb02]

Summary

mail/mailman: Use .conf suffix for /usr/local/etc/newsyslog.conf.d/

• Rename the files installed to /usr/local/etc/newsyslog.conf.d/ to end with a '.conf' suffix.
• Add pkg-install script to automatically move any copies of the old newsyslog file to the new location if the new file is unmodified from the default, or print a warning if it the new file has been modified.
• Add a note to UPDATING and pkg-message to warn users of this, in case they are using provisioning/configuration management tools which need to be modified. Note the UPDATING entry was committed in r485721.

Recent changes to /etc/newsyslog.conf (r340318) will only include files
from the /usr/local/etc/newsyslog.conf.d/ directory which end with
'.conf' and do not beginning with a '.' character.

Approved by: mandree (maintainer)

Test Plan

poudriere testport -j 13amd64: ok

[woodsb02]

This change has been reverted pending POLA dicusssions. Refer rS479438

[woodsb02]

D17086 has now been committed to 13-CURRENT.
Re-opening this review.
New proposal to minimise the possibility that people are caught out unaware of this:

1. Modify port to install file to newsyslog/*.conf
2. Add move_newsyslog_conf() to pkg-install to automatically relocate any existing copy that is missing the .conf file
3. Add note to UPDATING and pkg-message to notify the user of this change (important if provisioning tools such as ansible, puppet, salt, etc are in use).

[woodsb02]

• Add pkg-install script to automatically move old newsyslog file to new filename
• Add entries to UPDATING and pkg-message

[mandree]

Given we got slapped over the head last time we went forward with this, I see this new proposed change is trying to be more careful. Please see detailed comments and questions therein.

UPDATING

16 ↗

(On Diff #50260)

UPDATING is supposed to be read before the fact (port upgrade), so present perfect tense "has been moved" is wrong, should be future tense. "will be moved" -- also to be changed in the remainder of this entry.

mail/mailman/files/pkg-install.in

36 ↗

(On Diff #50260)

looks dangerous to me. what if both are present - which one should get precedence? Currently, the older will. The file should only be renamed if that renaming does not clobber the destination file.

54 ↗

(On Diff #50260)

While at it, I am wondering if this is still needed, or if pkg has been fixed in the meanwhile.

mail/mailman/pkg-plist

4 ↗

(On Diff #50260)

I am currently scratching my head over the interaction of the @sample keyword, its possibly changing implementation (ports/Keywords/sample.ucl) and semantics over time, WRT ordering relative to POST-INSTALL.

[woodsb02]

• Align with D17089 which has now been approved by portmgr and committed
• Incorporate logic if both old and new newsyslog file are present
• Note change to UPDATING was committed in rS485721 (wording changes proposed by mandree were incorporated)

[woodsb02]

mail/mailman/pkg-plist

4 ↗

(On Diff #50260)

My understanding is that the @sample keyword takes effect (copies the file into place) during install, so PRE-INSTALL would be before @sample and POST-INSTALL would be after @sample.

[mandree]

There is one change that I'd like made, and that's we should not remove ${oldfile}, instead make the user migrate ${oldfile} to ${newfile}.
Other than that, seems good to go.

mail/mailman/files/pkg-install.in

46 ↗

(On Diff #51035)

2018-11-25: please remove this, and instead request that the user cleans up.

[woodsb02]

@mat preferred to delete the old file if the new file already exists and has been modified from the default. See further description below.

mail/mailman/files/pkg-install.in

46 ↗

(On Diff #51035)

This was discussed for the sysutils/munin-* phabricator review here:
https://reviews.freebsd.org/D17089#383669

Your proposal to ask the user clean it up is essentially option 4 in my proposal. mat@ opted for option 3 which was to delete the old file.

My understanding of the logic behind this is that if the file is the new location varies from the sample file, then the sysadmin must have modified it (knowing about the new location), and therefore the file in the old location is obsolete.

[mandree]

mat@ choice is unacceptable because it can destroy data. In rare circumstances, but it's still possible.

We are not keeping around the reference sample files for each and every version, so when the user moved the file of .sample file we install now with mailman-2.1.29_3 changes in the future into the new location so as to make mailman work on 12.0, the logic will misfire, will assume the .conf file (unmodified from 2.1.29_3) in the new place was altered, and will remove the old possibly modified file. Oops. We cannot assume when or where the user will upgrade the package, which versions he will skip, or how thorough he will read through the half dozen screen walls full of text and act accordingly.

There is no harm in leaving the old file around that gets ignored by FreeBSD 12+'s newsyslog, and I will not scratch configuration files. We can afford 1...3 blocks and 1 inode wasted if the system ignores them.

The only situation where I will tolerate removing the old file is if it the old file's contents are identical to either the new one or to the sample file (so no information lost). Meaning I will require to compare the contents of the file we are about to remove, and not decide it can go because of how two other files unrelated to the "rm" compare against one another.

So let me as the port maintainer rule, the rm has to be made more careful or I will not accept the patch. And to avoid another screenful of logic, I'd propose to just warn the user he needs to choose which file he wants to keep in the new place.

Sorry.

[mandree]

(and yes it's only newsyslog, not Postfix's main.cf or your Samba PDC configuration)

[woodsb02]

Makes sense to me.

[woodsb02]

• Only warn the user if the old configuration file exists and the new file has been modified from the default (do not delete it)

[mat]

The problem is that if you do not remove the file, and have foo and foo.conf both saying rotate xxx, both will be used, and I'm sure the behavior is undocumented and probably won't be what you expect.

[mandree]

In D17088#389354, @mat wrote:

The problem is that if you do not remove the file, and have foo and foo.conf both saying rotate xxx, both will be used, and I'm sure the behavior is undocumented and probably won't be what you expect.

I understand that "both will be used" only applies to FreeBSD <= 11.x. Anyways, we're stuck and need to find another approach, and I am proposing that we look into pre-install scripts to move the configuration file providing that there is no target file - in that situation a "ln && rm" combo might work (not mv which is potentially destructive). Opinions?

[woodsb02]

As per the existing code, if the contents of ${newfile} are the same as ${samplefile} then there is no destruction to move ${oldfile} to ${newfile}. If the user wants to recover the content of the clobbered newfile - it is fully available in samplefile.

Which leaves us only with the question of what to do if the contents of newfile and samplefile differ. As @mat has pointed out, we can’t leave both around as it will cause double rotation of FreeBSD < 12. Given these are typically 1 line files, we are not talking about a large amount of data, and the option to delete oldfile suggested by @mat is a good one. Alternatively, we could move oldfile to /var/backups/newsyslog-mailman.conf.bak?

[mandree]

In D17088#389607, @woodsb02 wrote:

As per the existing code, if the contents of ${newfile} are the same as ${samplefile} then there is no destruction to move ${oldfile} to ${newfile}. If the user wants to recover the content of the clobbered newfile - it is fully available in samplefile.

That is not what I was writing about. My point is that the contents of $oldfile need to be considered before we rm $oldfile, and not a comparison of two other files ($newfile vs. $sample) against one another. That is why I proposed to ask the user to clean up, and what Ben has implemented.

I don't see a good way to go beyond that and automatically decide which of the three files ($oldfile, $newfile, $new.sample) can go, in complex cases. Remember that $new.sample can change on upgrades and upgrades only have paperweight in our descriptions of pkg behaviour and scripting instructions, including the committer's guide, porter's handbook, ports/CHANGES, and other documentation in or on the ports tree. I don't trust port upgrades to do the right thing, which makes assumptions on automatic cleanup all the harder.

We shouldn't program some artificial intelligence or expert system to deal with deciding which newsyslogfile file is the correct one. Let's just handle the obvious cases, and for the complex cases, defer to the user.

Which leaves us only with the question of what to do if the contents of newfile and samplefile differ. As @mat has pointed out, we can’t leave both around as it will cause double rotation of FreeBSD < 12. Given these are typically 1 line files, we are not talking about a large amount of data, and the option to delete oldfile suggested by @mat is a good one. Alternatively, we could move oldfile to /var/backups/newsyslog-mailman.conf.bak?

What is the problem if the file is rotated twice? The default configuration rotates solely based on size (2000 kB, then xz, and keep 10), so the second rotation for mailman should usually be a no-op because a log should not usually grow to 2 MB within the seconds that it takes to rotate log files => No harm done. So the current state with the two echo lines where the rm ${oldfile} used to be is good to go. Approved, please commit.

# logfilename				[owner:group]   mode count	size when	flags	[/pid_file] [sig_num]
/usr/local/mailman/logs/bounce		mailman:mailman	660  10		2000 *		X	/usr/local/mailman/data/master-qrunner.pid
/usr/local/mailman/logs/error		mailman:mailman 660  10		2000 *		X	/usr/local/mailman/data/master-qrunner.pid
/usr/local/mailman/logs/locks		mailman:mailman 660  10		2000 *		X	/usr/local/mailman/data/master-qrunner.pid
/usr/local/mailman/logs/mischief	mailman:mailman 660  10		2000 *		X	/usr/local/mailman/data/master-qrunner.pid
/usr/local/mailman/logs/post		mailman:mailman 660  10		2000 *		X	/usr/local/mailman/data/master-qrunner.pid
/usr/local/mailman/logs/qrunner		mailman:mailman 660  10		2000 *		X	/usr/local/mailman/data/master-qrunner.pid
/usr/local/mailman/logs/security	mailman:mailman 660  10		2000 *		X	/usr/local/mailman/data/master-qrunner.pid
/usr/local/mailman/logs/smtp		mailman:mailman 660  10		2000 *		X	/usr/local/mailman/data/master-qrunner.pid
/usr/local/mailman/logs/smtp-failure	mailman:mailman 660  10		2000 *		X	/usr/local/mailman/data/master-qrunner.pid
/usr/local/mailman/logs/subscribe	mailman:mailman 660  10		2000 *		X	/usr/local/mailman/data/master-qrunner.pid
/usr/local/mailman/logs/vette		mailman:mailman 660  10		2000 *		X	/usr/local/mailman/data/master-qrunner.pid

mail/mailman/files/pkg-install.in

44 ↗

(On Diff #51095)

The logic is right, yet I would also like to see the location/name of $oldfile printed out. But that can happen later. Let's move.

[woodsb02]

Thanks for the review team.
For the record, the old filename is printed out in the warning message if the user needs to do the cleanup - the echo command before the "if cmp" check.

[mandree]

Ben, thanks for handling this and pointing me back to the OLD: output line.
The one incremental improvement I have made afterwards is to expand the ${PREFIX} - %%PREFIX%% does that magic when substituting placeholders by real strings from files/foo.in to foo.