[BUG] Various bugs encountered during chat in the Slack #irc channel

[myii]

Your setup

Formula commit hash / release tag

Versions reports (master & minion)

Pillar / config used

---

Bug details

Describe the bug

UPDATE: Full breakdown given in #56 (comment) below.

---

https://freenode.logbot.info/salt/20200925#c5237209-c5237678

-	-	-
20:09	doubletwist	Also having trouble with github.com/saltstack-formulas/logrotate-formula - it works fine for *EL systems and 'works' for deb systems but for Ubuntu it's not setting the 'su root syslog' in logrotate.conf even though it's in the osmap.sls
20:14	doubletwist	for items set in map.jinja, should/will they show up in pillar.items on the minion?
20:17	myii[m]	doubletwist: I can check that out, give me a minute.
20:21	myii[m]	doubletwist: It's definitely getting merged into the map properly so probably some other issue: https://pastebin.com/HJE2aMui.
20:26	doubletwist	You'll have to paste somewhere else. our DNS filter doesn't seem to like pastebin.com
20:29	doubletwist	myii[m]: You may also need to update the map. I think 20.04 changed to using group 'adm' instead of 'syslog'
20:29	doubletwist	though I'm testing against 18.04 so the included map should work
20:33	myii[m]	doubletwist: 20.04 aside, I've tested using Kitchen and it is working fine. Let me paste what's coming through.
20:35	myii[m]	doubletwist: https://paste.debian.net/1164701
20:36	myii[m]	Perhaps you've got some configuration in your pillar which is overwriting the default_config key. I reckon the file template can be improved by getting rid off the first line, which is no longer necessary after the updated map.jinja.
20:36	doubletwist	hrm. maybe that's it
20:36	doubletwist	checking
20:37	doubletwist	So if I set any default_config entries in pillar, it'll blow away any other defaults that I don't include in the pillar?
20:38	myii[m]	Yes, with the current implementation, which should be improved.
20:38	doubletwist	Ok I see
20:38	myii[m]	As I said, this line should be removed and it should work fine, even if you have settings in your pillar: https://github.com/saltstack-formulas/logro…te/templates/logrotate.conf.tmpl#L1.
20:39	doubletwist	Will give that a shot
20:40	myii[m]	Sorry, not removed, adjusted. One second.
20:40	myii[m]	{%- set config = logrotate.default_config %}
20:40	doubletwist	will try
20:41	myii[m]	Yes, I've just tested that and it works fine.
20:42	myii[m]	The map.jinja merges the dicts properly instead of overwriting (or just using the pillar without considering the rest of the mappings).
21:27	doubletwist	myii[m]: So it does add the su line, but seems to combine weirdly on the rotation period
21:28	myii[m]	Your pillar value will trump the value in the YAML files. What do you have set?
21:33	doubletwist	default_config:
21:33	doubletwist	daily: True
21:33	doubletwist	resulted in
21:33	doubletwist	with your new change:
21:33	doubletwist	-daily
21:33	doubletwist	+dailyweekly
21:34	doubletwist	everything else looks fine
21:35	doubletwist	Which makes sense because weekly is set true in defaults.yaml and daily set true in pillar. And the template seems to just cycle through and include any that are true
21:35	doubletwist	if I'm reading it right
21:35	doubletwist	Which is FAR from certain. :)
21:37	myii[m]	Reproduced in Kithcne. It should only use one or the other, right? dailyweekly is nonsense.
21:38	myii[m]	(edited) ... in Kithcne. It ... => ... in Kitchen. It ...
21:39	myii[m]	The map is doing the right thing but the template needs to be fixed here. I reckon whichever it encounters first, it should use and then break out of the loop.
21:40	myii[m]	Hmm, also monthly missing here: https://github.com/saltstack-formulas/logro…e/templates/logrotate.conf.tmpl#L26.
21:43	doubletwist	I don't think I'd depend on order of precedence. I would think it'd be better to have a setting for 'rotation_period' or something that would be set to 'daily' or 'weekly' or 'monthly' or 'yearly' - that could be overridden via pillar.
21:44	myii[m]	That would be a breaking change; I just want to get a quick fix in place and then leave an issue about modifying this to a proper solution in the long run.
21:44	doubletwist	Though I recognize that would be a bit more work
21:45	myii[m]	The important thing right now is to squash these bugs without too much disruption.
22:15	doubletwist	myii[m]: fwiw. I think that changing the macro line to read: {%- elif (value is string or value is number) and (parameter != 'period') -%}
22:16	doubletwist	and then making the pillar/default be "period: weekly" or "period: daily" etc works
22:16	doubletwist	maybe not the best way to do it but seems to be working for me at first blush
22:17	doubletwist	oh and taking out the "for period in" loop
22:18	doubletwist	for a future more 'complete' fix
22:22	myii[m]	doubletwist: Yes, the proper fix will involve something like that but it will result in end users having to update their pillars, which is a pain.
22:24	doubletwist	Yeah makes sense.
22:24	doubletwist	I'm going ahead and keeping the change on my system
22:27	doubletwist	oh and nevermind on that change in Ubuntu 20.04 I think I was mistaken there
22:27	myii[m]	The simplest option for you as-is => simply add all of the values for default_config in your pillar.
22:27	myii[m]	And don't worry about the YAML files.
22:27	myii[m]	Or adjusting the template.
22:27	doubletwist	I've already done it
22:28	myii[m]	As you wish.
22:31	doubletwist	Thanks for your help though! (and the cool formula :) _

So that's:

1. Template isn't respecting the map.jinja merging and only uses pillar items instead; should really be changed to {%- set config = logrotate.default_config %}.
2. However, changing that then reveals the bug that can end up with dailyweekly appearing in the config file.
3. The fix for that would result in a breaking change that would force all users to modify their pillars (setting a single type, such as period: daily).
4. The monthly interval is missing from the template (and the formula).

Steps to reproduce the bug

Expected behaviour

Attempts to fix the bug

Additional context

[daks]

I'm not sure I clearly understand the problem, @myii I'm interested in seeing code which breaks tests.

In any case seems an important bug, which justifies making any breaking change to fix it.

[myii]

I'm not sure I clearly understand the problem, @myii I'm interested in seeing code which breaks tests.

In any case seems an important bug, which justifies making any breaking change to fix it.

@daks I've provided examples of all four problems below; it wouldn't be much effort to write small verification tests in InSpec but that can be done when the right strategy is determined.

---

1. Template isn't respecting the map.jinja merging and only uses pillar items instead; ...

So this is a bug in the formula as of the current time (v0.11.5).

Ubuntu-specific configuration:

logrotate-formula/logrotate/osmap.yaml

Lines 4 to 6 in 8f685e9

	Ubuntu:
	default_config:
	su: root syslog

Section in pillar.example used by Kitchen:

logrotate-formula/pillar.example

Lines 9 to 14 in 8f685e9

	default_config:
	weekly: true
	rotate: 52
	create: true
	compress: true
	dateext: true

The merging in map.jinja is fine:

• https://travis-ci.org/github/myii/logrotate-formula/builds/731089195#L1117-L1123

However, the template ignores the merged map if pillar values exist, so su root syslog isn't rendered in the actual file:

https://travis-ci.org/github/myii/logrotate-formula/builds/731089195#L1891-L1928

Note, it's possible to workaround this issue by supplying all of the required values in the pillar but then that misses the point of having the values set in the YAML files.

---

1. ... should really be changed to {%- set config = logrotate.default_config %}.

So if this fix is applied:

• myii@e0e0a0b

The map is still fine (obviously):

• https://travis-ci.org/github/myii/logrotate-formula/builds/731095086#L916-L922

And now the map is used in the template instead of just the pillar:

• https://travis-ci.org/github/myii/logrotate-formula/builds/731095086#L1710

Everything fixed, right? Not quite...

---

2. However, changing that then reveals the bug that can end up with dailyweekly appearing in the config file.

Say the user decides to have daily log rotation instead (set in the pillar):

• myii@2029704

The map now contains entries for both daily (from the pillar) and weekly (from the defaults):

• https://travis-ci.org/github/myii/logrotate-formula/builds/731175047#L914-L921

And the template ends up using both during the for loop, concatenating them on a single line:

• https://travis-ci.org/github/myii/logrotate-formula/builds/731175047#L1696

One workaround discussed in the #irc channel chat was to modify the for loop to break after the first matching entry but that doesn't really solve the problem, if the entry in the defaults ends up taking precedence over the value in the pillar. I suppose a better workaround would be to add a check for the pillar value first and then drop back to the defaults.

---

3. The fix for that would result in a breaking change that would force all users to modify their pillars (setting a single type, such as period: daily).

In the #irc channel conversation, the idea of having a single value to track the log files rotation period would do the job, such as using one of the following:

• period: daily
• period: weekly
• period: monthly
• ...

Then the map merging would end up with a single value, so we wouldn't need to resort to hacking the for loop to do the right thing. The problem with this solution is that it would be a breaking change, forcing all end users to have to modify their pillars to use this new value instead.

---

4. The monthly interval is missing from the template (and the formula).

This is a separate bug that really should be fixed in all situations:

logrotate-formula/logrotate/templates/logrotate.conf.tmpl

Line 26 in 589cd17

{% for period in ['hourly', 'daily', 'weekly', 'yearly'] -%}