Merge lp:~pjdc/charm-haproxy/do-not-clobber-logrotate-on-upgrade into lp:charm-haproxy

[Paul Collins]

only configure logrotate if the config item is not empty, and restore a clobbered config file to the default

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

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

[Tom Haddon (mthaddon)]

review: Needs Information

[Tom Haddon (mthaddon)]

LGTM, but see comment about empty logrotate config

review: Approve

[lp:~pjdc/charm-haproxy/do-not-clobber-logrotate-on-upgrade]

137. By Paul Collins on 2019-07-02

configure_logrotate: write default config if file on disk is zero length

[Paul Collins (pjdc)]

I fiddled with the comment a bit before deciding to bite the bullet and restore the default config if the file on disk is zero-length.

review: Needs Resubmitting

[lp:~pjdc/charm-haproxy/do-not-clobber-logrotate-on-upgrade]

138. By Paul Collins on 2019-07-02

configure_logrotate: typo in comment

[Tom Haddon (mthaddon)]

LGTM, but per comment inline there's an odd looking indentation that should be fixed before landing.

review: Approve

[lp:~pjdc/charm-haproxy/do-not-clobber-logrotate-on-upgrade]

139. By Paul Collins on 2019-07-08

hooks.py: indentation fix

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision 136

=== modified file 'config.yaml'
--- config.yaml	2019-06-07 10:09:25 +0000
+++ config.yaml	2019-07-08 01:16:49 +0000
@@ -100,7 +100,9 @@
     description: |
         Override package logrotate configuration.
         .
-        For example.
+        Warning: Setting this value back to the empty string will leave the previous config in place on disk.
+        .
+        Example value:
         .
         /var/log/haproxy.log {
             weekly

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2019-06-07 09:24:02 +0000
+++ hooks/hooks.py	2019-07-08 01:16:49 +0000
@@ -1,15 +1,16 @@
 #!/usr/bin/env python
 
 import base64
+import errno
 import glob
 import os
+import pwd
 import re
+import shutil
 import socket
-import shutil
 import subprocess
 import sys
 import yaml
-import pwd
 
 from itertools import izip, tee
 from operator import itemgetter
@@ -52,7 +53,6 @@
 default_haproxy_lib_dir = "/var/lib/haproxy"
 metrics_cronjob_path = "/etc/cron.d/haproxy_metrics"
 metrics_script_path = "/usr/local/bin/haproxy_to_statsd.sh"
-log_rotate_config_path = "/etc/logrotate.d/haproxy"
 service_affecting_packages = ['haproxy']
 apt_backports_template = (
     "deb http://archive.ubuntu.com/ubuntu %(release)s-backports "
@@ -102,6 +102,29 @@
     ]
 
 
+logrotate_config_path = "/etc/logrotate.d/haproxy"
+
+logrotate_config_header = '''\
+# This file is managed by the haproxy charm (config item logrotate_config).
+# Manual changes may be reverted at any time.  Use Juju to change it.
+'''
+
+# This is used to roll back zero-length configs deployed by a earlier buggy
+# version of the charm.  Extracted from haproxy 1.6.3-1ubuntu0.2.
+default_packaged_haproxy_logrotate_config = '''\
+/var/log/haproxy.log {
+    daily
+    rotate 52
+    missingok
+    notifempty
+    compress
+    delaycompress
+    postrotate
+        invoke-rc.d rsyslog rotate >/dev/null 2>&1 || true
+    endscript
+}
+'''
+
 class InvalidRelationDataError(Exception):
     """Invalid data has been provided in the relation."""
 
@@ -962,9 +985,6 @@
     ensure_package_status(service_affecting_packages, config_data['package_status'])
     enable_haproxy()
 
-    if config_data.get('logrotate_config'):
-        logrotate_configuration()
-
 
 def config_changed():
     config_data = config_get()
@@ -983,7 +1003,7 @@
             raise ValueError('{} is not a valid port/proto value'.format(port_plus_proto))
 
     if config_data.changed('logrotate_config'):
-        logrotate_configuration()
+        configure_logrotate(config_data.get('logrotate_config'))
 
     old_stanzas = get_listen_stanzas()
     haproxy_globals = create_haproxy_globals()
@@ -1395,11 +1415,31 @@
                          user=monitoring_username)
 
 
-def logrotate_configuration():
-    config = config_get()
-    logrotate_config = config['logrotate_config']
-    with open(log_rotate_config_path, 'w') as f:
-        f.write(logrotate_config)
+def configure_logrotate(logrotate_config):
+    # NOTE(pjdc): This function is a trapdoor -- setting logrotate_config and
+    # then changing it to an empty string leaves the last value in place, but
+    # if we ship a default config and always write *something*, we'd clobber
+    # manual configs.  Since this is a relatively new charm feature, we
+    # probably can't lay exclusive claim to the logrotate config file yet.
+    final_logrotate_config = None
+
+    if logrotate_config:
+        final_logrotate_config = logrotate_config_header + logrotate_config
+    else:
+        try:
+            # If the live config is zero-length, it was probably clobbered by a
+            # (fixed) bug in the previous version of the charm (LP:1834980).
+            if os.path.getsize(logrotate_config_path) == 0:
+                final_logrotate_config = default_packaged_haproxy_logrotate_config
+        except OSError as ose:
+            # If the file or directory is missing, just carry on.  Either
+            # someone deliberately deleted it, or logrotate isn't installed.
+            if ose.errno != errno.ENOENT:
+                raise
+
+    if final_logrotate_config:
+        with open(logrotate_config_path, 'w') as f:
+            f.write(final_logrotate_config)
 
 
 # #############################################################################