From 1e2ea795c295f4076c1216bce183468e8bfb40f2 Mon Sep 17 00:00:00 2001 From: Ian Wienand Date: Mon, 13 Feb 2023 15:14:45 +1100 Subject: [PATCH] logrotate: don't use filename to generate config file I noticed on our hosts some logrotate files named '*.1234.conf' -- these are coming from callers of logrotate role specifying '/var/log/program/*.log', where the '*' is turning into a literal filename. I didn't really consider this case. Having a file-name starting with '*' may technically be fine, but is a bad idea for everyone's sanity and it's potential to foot-gun some sort of operation that suddenly wipes out a lot more than you wanted to. Let's just use the hash of the name to be unambiguous and still idempotent. Make it more git-ish by using the same 7 digits as a default short-hash. Change-Id: I13d376f85a25a7b8c3a0bc0dcbabd916e8a9774a --- playbooks/roles/logrotate/README.rst | 9 ++++++--- playbooks/roles/logrotate/tasks/main.yaml | 13 +++++++++++-- testinfra/test_base.py | 4 +++- 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/playbooks/roles/logrotate/README.rst b/playbooks/roles/logrotate/README.rst index c6bfeab9a1..6326d18cf7 100644 --- a/playbooks/roles/logrotate/README.rst +++ b/playbooks/roles/logrotate/README.rst @@ -14,12 +14,15 @@ not an exhaustive list of directives (contributions are welcome). .. zuul:rolevar:: logrotate_file_name - The log file on disk to rotate + The full path to log file on disk to rotate. May be a wild-card; + e.g. ``/var/log/progname/*.log``. .. zuul:rolevar:: logrotate_config_file_name - :default: Unique name based on :zuul:rolevar::`logrotate.logrotate_file_name` + :default: Unique name based on the hash of :zuul:rolevar::`logrotate.logrotate_file_name` - The name of the configuration file in ``/etc/logrotate.d`` + The name of the configuration file in ``/etc/logrotate.d``. If + this is specified, it is up to the caller to ensure it is unique + across all calls of this role. .. zuul:rolevar:: logrotate_compress :default: yes diff --git a/playbooks/roles/logrotate/tasks/main.yaml b/playbooks/roles/logrotate/tasks/main.yaml index fbdfa0800f..5059f838e5 100644 --- a/playbooks/roles/logrotate/tasks/main.yaml +++ b/playbooks/roles/logrotate/tasks/main.yaml @@ -15,10 +15,19 @@ when: logrotate_frequency == 'size' # Hash the full path to avoid any conflicts but remain idempotent. -# "/var/log/ansible/ansible.log" becomes "ansible.log.37237.conf" for example - name: Create a unique config name set_fact: - logrotate_generated_config_file_name: "{{ logrotate_file_name | basename }}.{{ (logrotate_file_name|hash('sha1'))[0:5] }}.conf" + # NOTE(ianw) 2023-02-13 : we missed that this makes files with + # names like "*.1234.conf" when using wild-cards. Below we have + # dropped using the file-name component. After we've removed them + # we can drop this. + _old_logrotate_generated_config_file_name: "{{ logrotate_file_name | basename }}.{{ (logrotate_file_name|hash('sha1'))[0:5] }}.conf" + logrotate_generated_config_file_name: "{{ (logrotate_file_name | hash('sha1'))[0:6] }}.conf" + +- name: Clear out potentially confusing config files + file: + state: absent + path: '{{ _old_logrotate_generated_config_file_name }}' - name: 'Install {{ logrotate_file_name }} rotatation config file' template: diff --git a/testinfra/test_base.py b/testinfra/test_base.py index 7c796acd44..d5a88510e1 100644 --- a/testinfra/test_base.py +++ b/testinfra/test_base.py @@ -130,7 +130,9 @@ def test_logrotate(host): ''' ansible_vars = host.ansible.get_variables() if ansible_vars['inventory_hostname'].startswith('bridge'): - cfg_file = host.file("/etc/logrotate.d/ansible.log.37237.conf") + # Generated for idempotence by logrotate role; hash of + # "/var/log/ansible/ansible.log" + cfg_file = host.file("/etc/logrotate.d/372374.conf") assert cfg_file.exists assert cfg_file.contains('/var/log/ansible/ansible.log')