Merge ~hloeung/content-cache-charm:master into content-cache-charm:master

[Haw Loeung]

Enable dateext and allow overriding log retention - LP: #1848407

The copy_file() and write_file() bits were copied from https://code.launchpad.net/~fo0bar/content-cache-charm/+git/content-cache-charm/+merge/373778

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

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

[Joel Sing (jsing)]

review: Needs Fixing

[Haw Loeung (hloeung)]

review: Needs Resubmitting (ptal)

[Joel Sing (jsing)]

LGTM

review: Approve (+1)

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

Change successfully merged at revision 340cde9bd9636762cdae12b6770b874c8231641c

diff --git a/config.yaml b/config.yaml
index 4166111..fddb691 100644
--- a/config.yaml
+++ b/config.yaml
@@ -19,6 +19,11 @@ options:
     default: "/var/lib/nginx/proxy"
     description: >
       Path or directory to store cached objects.
+  log_retention:
+    type: int
+    default: 30
+    description: >
+      Number of log files to retain during rotation.
   nagios_context:
     type: string
     default: "juju"
diff --git a/lib/utils.py b/lib/utils.py
index 66e94da..b3f0dfb 100644
--- a/lib/utils.py
+++ b/lib/utils.py
@@ -1,5 +1,6 @@
 import hashlib
 import hmac
+import os
 import re
 import shutil
 import subprocess
@@ -136,3 +137,26 @@ def tls_cipher_suites(tls_cipher_suites):
     except Exception:
         raise InvalidTLSCiphersError('Unable to parse provided OpenSSL cipher string "{}"'.format(tls_cipher_suites))
     return tls_cipher_suites
+
+
+def logrotate(path, retention=30, dateext=True):
+    if not os.path.exists(path):
+        return None
+
+    with open(path, 'r', encoding='utf-8') as f:
+        config = f.read().split('\n')
+
+    new = []
+    regex = re.compile('^(\\s+)(rotate|dateext)')
+    for line in config:
+        m = regex.match(line)
+        if m:
+            if m.group(2) == 'dateext':
+                continue
+            if dateext:
+                new.append('{}dateext'.format(m.group(1)))
+            new.append('{}rotate {}'.format(m.group(1), retention))
+        else:
+            new.append(line)
+
+    return '\n'.join(new)
diff --git a/reactive/content_cache.py b/reactive/content_cache.py
index d001775..592588e 100644
--- a/reactive/content_cache.py
+++ b/reactive/content_cache.py
@@ -149,6 +149,7 @@ def configure_nginx(conf_path=None):
     if changed:
         service_start_or_restart('nginx')
 
+    update_logrotate('nginx', retention=config.get('log_retention'))
     reactive.set_flag('content_cache.nginx.configured')
 
 
@@ -273,6 +274,7 @@ def configure_haproxy():
     if haproxy.write(rendered_config):
         service_start_or_restart('haproxy')
 
+    update_logrotate('haproxy', retention=config.get('log_retention'))
     reactive.set_flag('content_cache.haproxy.configured')
 
 
@@ -471,16 +473,32 @@ def _interpolate_secrets_origin_headers(headers, secrets):
     return headers
 
 
-def copy_file(source_path, dest_path, perms=0o644, owner=None, group=None):
+def update_logrotate(service, retention, dateext=True):
+    conf_path = os.path.join('/etc/logrotate.d', service)
+    write_file(utils.logrotate(conf_path, retention=retention, dateext=dateext), conf_path)
+
+
+def copy_file(source_path, dest_path, **kwargs):
     """Copy a file from the charm directory onto the local filesystem.
 
-    Returns True if the file was copied, False if the file already exists and
-    is identical.
+    Reads the contents of source_path and passes through to write_file().
+    Please see the help for write_file() for argument usage.
     """
 
-    # Compare and only write out file on change.
     with open(source_path, 'r') as f:
         source = f.read()
+    return write_file(source, dest_path, **kwargs)
+
+
+def write_file(source, dest_path, perms=0o644, owner=None, group=None):
+    """Write a source string to a file.
+
+    Returns True if the file was modified (new file, file changed, file
+    deleted), False if the file is not modified or is intentionally not
+    created.
+    """
+
+    # Compare and only write out file on change.
     dest = ''
     if os.path.exists(dest_path):
         with open(dest_path, 'r') as f:
diff --git a/tests/unit/files/haproxy-dateext-logrotate.conf b/tests/unit/files/haproxy-dateext-logrotate.conf
new file mode 100644
index 0000000..9f4a9e3
--- /dev/null
+++ b/tests/unit/files/haproxy-dateext-logrotate.conf
@@ -0,0 +1,12 @@
+/var/log/haproxy.log {
+    daily
+    dateext
+    rotate 52
+    missingok
+    notifempty
+    compress
+    delaycompress
+    postrotate
+        invoke-rc.d rsyslog rotate >/dev/null 2>&1 || true
+    endscript
+}
diff --git a/tests/unit/files/haproxy-logrotate.conf b/tests/unit/files/haproxy-logrotate.conf
new file mode 100644
index 0000000..442dc4e
--- /dev/null
+++ b/tests/unit/files/haproxy-logrotate.conf
@@ -0,0 +1,11 @@
+/var/log/haproxy.log {
+    daily
+    rotate 52
+    missingok
+    notifempty
+    compress
+    delaycompress
+    postrotate
+        invoke-rc.d rsyslog rotate >/dev/null 2>&1 || true
+    endscript
+}
diff --git a/tests/unit/files/nginx-dateext-logrotate.conf b/tests/unit/files/nginx-dateext-logrotate.conf
new file mode 100644
index 0000000..a098eb2
--- /dev/null
+++ b/tests/unit/files/nginx-dateext-logrotate.conf
@@ -0,0 +1,19 @@
+/var/log/nginx/*.log {
+	daily
+	missingok
+	dateext
+	rotate 14
+	compress
+	delaycompress
+	notifempty
+	create 0640 www-data adm
+	sharedscripts
+	prerotate
+		if [ -d /etc/logrotate.d/httpd-prerotate ]; then \
+			run-parts /etc/logrotate.d/httpd-prerotate; \
+		fi \
+	endscript
+	postrotate
+		invoke-rc.d nginx rotate >/dev/null 2>&1
+	endscript
+}
diff --git a/tests/unit/files/nginx-logrotate.conf b/tests/unit/files/nginx-logrotate.conf
new file mode 100644
index 0000000..423c6ad
--- /dev/null
+++ b/tests/unit/files/nginx-logrotate.conf
@@ -0,0 +1,18 @@
+/var/log/nginx/*.log {
+	daily
+	missingok
+	rotate 14
+	compress
+	delaycompress
+	notifempty
+	create 0640 www-data adm
+	sharedscripts
+	prerotate
+		if [ -d /etc/logrotate.d/httpd-prerotate ]; then \
+			run-parts /etc/logrotate.d/httpd-prerotate; \
+		fi \
+	endscript
+	postrotate
+		invoke-rc.d nginx rotate >/dev/null 2>&1
+	endscript
+}
diff --git a/tests/unit/test_content_cache.py b/tests/unit/test_content_cache.py
index 537a081..0107b6a 100644
--- a/tests/unit/test_content_cache.py
+++ b/tests/unit/test_content_cache.py
@@ -58,6 +58,10 @@ class TestCharm(unittest.TestCase):
         self.addCleanup(patcher.stop)
         self.mock_cpu_count.return_value = 4
 
+        patcher = mock.patch('reactive.content_cache.update_logrotate')
+        self.mock_update_logrotate = patcher.start()
+        self.addCleanup(patcher.stop)
+
         status.active.reset_mock()
         status.blocked.reset_mock()
         status.maintenance.reset_mock()
@@ -664,6 +668,19 @@ site1.local:
         want = {'site1.local': {'locations': {'/': {'signed-url-hmac-key': 'Maiqu7ohmeiSh6ooroa0'}}}}
         self.assertEqual(content_cache.interpolate_secrets(config, secrets), want)
 
+    def test_write_file(self):
+        source = '# User-provided config added here'
+        dest = os.path.join(self.tmpdir, '90-content-cache.conf')
+
+        self.assertTrue(content_cache.write_file(source, dest))
+        # Write again, should return False and not True per above.
+        self.assertFalse(content_cache.write_file(source, dest))
+
+        # Check contents
+        with open(dest, 'r') as f:
+            got = f.read()
+        self.assertEqual(got, source)
+
     def test_copy_file(self):
         source = os.path.join(self.charm_dir, 'files/nginx-logging-format.conf')
         dest = os.path.join(self.tmpdir, os.path.basename(source))
diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py
index ac7c417..0bb3d5a 100644
--- a/tests/unit/test_utils.py
+++ b/tests/unit/test_utils.py
@@ -133,3 +133,19 @@ class TestLibUtils(unittest.TestCase):
         tls_cipher_suites = '--help'
         with self.assertRaises(utils.InvalidTLSCiphersError):
             utils.tls_cipher_suites(tls_cipher_suites)
+
+    def test_logrotate(self):
+        with open('tests/unit/files/haproxy-dateext-logrotate.conf') as f:
+            self.assertEqual(utils.logrotate(retention=52, path='tests/unit/files/haproxy-logrotate.conf'), f.read())
+        with open('tests/unit/files/nginx-dateext-logrotate.conf') as f:
+            self.assertEqual(utils.logrotate(retention=14, path='tests/unit/files/nginx-logrotate.conf'), f.read())
+
+        # Test dateext removal.
+        with open('tests/unit/files/nginx-logrotate.conf') as f:
+            self.assertEqual(
+                utils.logrotate(retention=14, dateext=False, path='tests/unit/files/nginx-dateext-logrotate.conf'),
+                f.read(),
+            )
+
+        # Test when config file doesn't exist.
+        self.assertEqual(utils.logrotate(retention=14, path='tests/unit/files/some-file-that-doesnt-exist'), None)