Merge lp:~evarlast/charms/trusty/apache2/add-logs-interface into lp:charms/trusty/apache2

[Jay R. Wren]

Add logs relation.

This exposes log files written by apache via the logs relation. Logger services such as logstash can be related to this service and this will configure the logger to read these files.

[lp:~evarlast/charms/trusty/apache2/add-logs-interface]

70. By Jay R. Wren on 2015-11-20

merge parent

[Review Queue (review-queue)]

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-lxc/1485/

review: Needs Fixing (automated testing)

[Review Queue (review-queue)]

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-aws/1468/

review: Needs Fixing (automated testing)

[lp:~evarlast/charms/trusty/apache2/add-logs-interface]

71. By Jay R. Wren on 2015-11-23

fix broken tests

[Review Queue (review-queue)]

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-lxc/1535/

review: Needs Fixing (automated testing)

[Review Queue (review-queue)]

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-aws/1518/

review: Needs Fixing (automated testing)

[Andrew McLeod (admcleod)]

Hi Jay,

As per my comments in your 'apt source' merge request, this one is failing with the following:

http://pastebin.ubuntu.com/13898257/

Andrew

review: Needs Fixing

[lp:~evarlast/charms/trusty/apache2/add-logs-interface]

72. By Jay R. Wren on 2015-12-10

make utopic+ tests pass on trusty

[Jay R. Wren (evarlast)]

I don't see how these tests EVER passed this criteria. Evidence to the contrary is appreciated.

I've disabled the tests on trusty as the hook code in the charm doesn't work in trusty and is documented as such.

Please consider for merging again.

review: Needs Resubmitting

[Cory Johns (johnsca)]

Jay,

Thanks for addressing those test failures. I think your solution is reasonable, and the tests now pass for me. I think that this mostly looks good, except for two small items.

First, I think there's a corner case in how you write out to the logs relation, that I mentioned in my diff comment below. I think just dropping the call to relation_ids() entirely should resolve it without issue.

Second, I would like to see some documentation in the README about how this is supposed to be used. You mention logstash in the MR, above, but that charm doesn't support this relation directly. I'm guessing that you intend this to be used via the beaver charm (e.g., https://jujucharms.com/u/yellow/beaver/trusty/0) because that has an interface that seems to match up, but it's not at all clear.

With those two small changes, I would give this my +1.

[lp:~evarlast/charms/trusty/apache2/add-logs-interface]

73. By Jay R. Wren on 2016-01-15

use relation_ids always, and add a section about logs relation to the README

[Jay R. Wren (evarlast)]

I have updated with those two things.

[Cory Johns (johnsca)]

Jay,

Thank you for addressing those items. This has now been merged, and should be ingested into the charm store within the hour.

=== modified file 'Makefile'
--- Makefile	2015-03-10 10:56:19 +0000
+++ Makefile	2016-01-15 23:00:04 +0000
@@ -18,7 +18,7 @@
 	@test `cat revision` = 0 && rm revision
 
 .venv:
-	sudo apt-get install -y python-apt python-virtualenv python-jinja2
+	sudo apt-get install -y python-apt python-virtualenv python-jinja2 python-openssl
 	virtualenv .venv --system-site-packages
 	.venv/bin/pip install -I nose testtools mock pyyaml
 
@@ -28,8 +28,8 @@
 
 lint:
 	@echo Checking for Python syntax...
-	@flake8 $(HOOKS_DIR) --ignore=E123 --exclude=$(HOOKS_DIR)/charmhelpers && echo hooks OK
-	@flake8 tests --ignore=E123 && echo tests OK
+	@flake8 $(HOOKS_DIR) --ignore=E123,E402 --exclude=$(HOOKS_DIR)/charmhelpers && echo hooks OK
+	@flake8 tests --ignore=E123,E402 && echo tests OK
 
 sourcedeps: $(PWD)/config-manager.txt
 	@echo Updating source dependencies...

=== modified file 'README.md'
--- README.md	2015-04-15 22:59:59 +0000
+++ README.md	2016-01-15 23:00:04 +0000
@@ -265,6 +265,12 @@
 
  * `ports` - A space-separated list of ports that the site uses.
 
+### Using the logs relation
+
+The logs relation is for use with a logging subordinate charm. The beaver
+subordinate can be deployed and related to apache and logstash. Beaver will
+tail apache logs and send the logs to logstash.
+
 ## Certs, keys and chains
 
 `ssl_keylocation`, `ssl_certlocation` and `ssl_chainlocation` are

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2015-04-15 20:23:01 +0000
+++ hooks/hooks.py	2016-01-15 23:00:04 +0000
@@ -22,6 +22,7 @@
     relations_of_type,
     relation_set,
     relation_ids,
+    relation_id,
     unit_get
 )
 from charmhelpers.contrib.charmsupport import nrpe
@@ -737,6 +738,8 @@
     ensure_ports(all_ports)
     update_nrpe_checks()
     ship_logrotate_conf()
+    if config_get().changed('servername'):
+        logs_relation_joined()
 
 
 def ensure_disabled(sites):
@@ -956,6 +959,53 @@
         yaml.safe_dump(ports, pfile)
 
 
+def get_log_files():
+    """
+    Read all of the apache config files from __ and get ErrorLog and AccessLog
+    values.
+
+    Returns a tuple with first value list of access log files and second value
+    list of error log files.
+    """
+    access_logs = []
+    error_logs = []
+    for protocol in ['http', 'https']:
+        vhost_name = '%s_%s' % (config_get()['servername'], protocol)
+        vhost_file = site_filename(vhost_name)
+        try:
+            # Using read().split('\n') here to work around a mocks open_mock
+            # inadequacy: http://bugs.python.org/issue17467
+            for line in open(vhost_file, 'r').read().split('\n'):
+                if 'CustomLog' in line:
+                    access_logs.append(line.split()[1])
+                elif 'ErrorLog' in line:
+                    error_logs.append(line.split()[1])
+        except:
+            pass
+    return access_logs, error_logs
+
+
+def logs_relation_joined():
+    """
+    Sets relation value with filenames
+    """
+    access_log_files, error_log_files = get_log_files()
+    log_files = access_log_files[:]
+    log_files.extend(error_log_files)
+    types = ['apache_access' for a in access_log_files]
+    types.extend(['apache_error' for a in error_log_files])
+    data = {'files': '\n'.join(log_files),
+            'types': '\n'.join(types),
+            }
+    _relation_ids = relation_ids('logs')
+    for _relation_id in _relation_ids:
+        log("logs-relation-joined setting relation data for {} to {}".format(
+            _relation_id, data))
+        relation_set(
+            relation_id=_relation_id,
+            relation_settings=data)
+
+
 ###############################################################################
 # Main section
 ###############################################################################
@@ -995,6 +1045,8 @@
         update_nrpe_checks()
     elif hook_name == "vhost-config-relation-changed":
         config_changed()
+    elif hook_name == "logs-relation-joined":
+        logs_relation_joined()
     else:
         print "Unknown hook"
         sys.exit(1)

=== added symlink 'hooks/logs-relation-joined'
=== target is u'hooks.py'
=== modified file 'hooks/tests/test_cert.py'
--- hooks/tests/test_cert.py	2014-05-24 06:33:34 +0000
+++ hooks/tests/test_cert.py	2016-01-15 23:00:04 +0000
@@ -73,6 +73,12 @@
     def test_is_selfsigned_cert_stale_public_address_changed(
             self, unit_get, log):
         """With different servername, cert *should* be regenerated."""
+        try:
+            from pyasn1.codec.der import decoder
+            from pyasn1_modules import rfc2459
+        except ImportError:
+            # This test is utopic+ only
+            return
         config = {"servername": "servername"}
         cert_file = os.path.join(self.tempdir, "cert")
         key_file = os.path.join(self.tempdir, "key")
@@ -86,6 +92,12 @@
     def test_is_selfsigned_cert_stale_private_address_changed(
             self, unit_get, log):
         """With different servername, cert *should* be regenerated."""
+        try:
+            from pyasn1.codec.der import decoder
+            from pyasn1_modules import rfc2459
+        except ImportError:
+            # This test is utopic+ only
+            return
         config = {"servername": "servername"}
         cert_file = os.path.join(self.tempdir, "cert")
         key_file = os.path.join(self.tempdir, "key")

=== modified file 'hooks/tests/test_config_changed.py'
--- hooks/tests/test_config_changed.py	2015-04-15 20:23:01 +0000
+++ hooks/tests/test_config_changed.py	2016-01-15 23:00:04 +0000
@@ -4,6 +4,10 @@
 import shutil
 import tempfile
 import os
+import sys
+
+sys.path.insert(0, os.path.join(os.environ['CHARM_DIR'], 'lib'))
+from charmhelpers.core import hookenv
 
 
 class ConfigChangedTest(TestCase):
@@ -47,7 +51,7 @@
             mock_conf_disable, mock_open_port, mock_close_port, mock_call,
             mock_set_open_ports, mock_get_open_ports):
         """config-changed hook: Site directories should be empty."""
-        mock_config_get.return_value = {
+        mock_config_get.return_value = hookenv.Config({
             "ssl_cert": "",
             "ssl_key": "",
             "package_status": "",
@@ -63,7 +67,7 @@
             "servername": "foobar",
             "vhost_http_template": "",
             "vhost_https_template": "",
-        }
+        })
         base = patch.object(hooks, 'default_apache_base_dir', self.dirname)
         config22 = patch.object(hooks, 'default_apache22_config_dir',
                                 "%s/conf.d" % self.dirname)

=== added file 'hooks/tests/test_log_relation.py'
--- hooks/tests/test_log_relation.py	1970-01-01 00:00:00 +0000
+++ hooks/tests/test_log_relation.py	2016-01-15 23:00:04 +0000
@@ -0,0 +1,56 @@
+from testtools import TestCase
+import mock
+import hooks
+hooks.log = mock.MagicMock()
+
+
+class TestLogRelation(TestCase):
+
+    @mock.patch('hooks.config_get')
+    def test_get_log_files_no_raise(self, config_get):
+        'does not raise'
+        hooks.get_log_files()
+
+    @mock.patch('hooks.config_get')
+    def test_get_log_files_parses(self, config_get):
+        'parses files for real'
+        config_get.return_value = {'servername': 'hello'}
+        m = mock.mock_open(read_data='''junk
+            CustomLog /var/log/apache2/access.log combined\n")
+            ErrorLog /var/log/apache2/error.log\n")
+            junk\n"''')
+
+        with mock.patch('hooks.open', m, create=True):
+            access_logs, error_logs = hooks.get_log_files()
+            self.assertEquals('/var/log/apache2/access.log', access_logs[0])
+            self.assertEquals('/var/log/apache2/error.log', error_logs[0])
+
+    @mock.patch('hooks.relation_id')
+    @mock.patch('hooks.relation_set')
+    def test_log_relation_joined(self, relation_set, relation_id):
+        with mock.patch('hooks.get_log_files') as get_log_files:
+            get_log_files.return_value = ['myaccess_log'], ['myerror_log']
+            hooks.logs_relation_joined()
+            self.assertTrue(relation_set.called)
+            self.assertEquals({'files': 'myaccess_log\nmyerror_log',
+                               'types': 'apache_access\napache_error',
+                               },
+                              relation_set.call_args[1]['relation_settings'])
+
+    @mock.patch('hooks.relation_ids')
+    @mock.patch('hooks.relation_id')
+    @mock.patch('hooks.relation_set')
+    def test_log_relation_joined_config_changed(self,
+                                                relation_set,
+                                                relation_id,
+                                                relation_ids):
+        relation_id.return_value = None
+        relation_ids.return_value = ['logs/0']
+        with mock.patch('hooks.get_log_files') as get_log_files:
+            get_log_files.return_value = ['myaccess_log'], ['myerror_log']
+            hooks.logs_relation_joined()
+            self.assertTrue(relation_set.called)
+            self.assertEquals({'files': 'myaccess_log\nmyerror_log',
+                               'types': 'apache_access\napache_error',
+                               },
+                              relation_set.call_args[1]['relation_settings'])

=== modified file 'hooks/tests/test_vhost_config_relation.py'
--- hooks/tests/test_vhost_config_relation.py	2014-12-29 19:37:05 +0000
+++ hooks/tests/test_vhost_config_relation.py	2016-01-15 23:00:04 +0000
@@ -27,23 +27,19 @@
 
     @patch('hooks.log')
     @patch('subprocess.call')
-    @patch('hooks.close_port')
     def test_create_vhost_missing_template(
-            self, mock_close_port, mock_call, mock_log):
+            self, mock_call, mock_log):
         """Create a vhost file, check contents."""
         hooks.create_vhost(80)
         mock_log.assert_called_once_with(
             "Vhost Template not provided, not configuring: %s" % 80)
-        mock_close_port.assert_called_once()
         self.assertEqual(
             len(os.listdir("%s/%s" % (self.dirname, "sites-available"))), 0)
 
     @patch('hooks.log')
     @patch('subprocess.call')
-    @patch('hooks.close_port')
-    @patch('hooks.open_port')
     def test_create_vhost_template_through_config_no_protocol(
-            self, mock_open_port, mock_close_port, mock_call, mock_log):
+            self, mock_call, mock_log):
         """Create a vhost file, check contents."""
         template = b64encode("http://{{ variable }}/")
         config_data = {
@@ -59,17 +55,13 @@
         with open(filename, 'r') as file:
             contents = file.read()
         self.assertEqual(contents, 'http://fantastic/')
-        mock_open_port.assert_called_once()
-        mock_close_port.assert_called_once()
         self.assertEqual(
             len(os.listdir("%s/%s" % (self.dirname, "sites-available"))), 1)
 
     @patch('hooks.log')
     @patch('subprocess.call')
-    @patch('hooks.close_port')
-    @patch('hooks.open_port')
     def test_create_vhost_template_through_config_with_protocol(
-            self, mock_open_port, mock_close_port, mock_call, mock_log):
+            self, mock_call, mock_log):
         """Create a vhost file, check contents."""
         template = b64encode("http://{{ variable }}/")
         config_data = {
@@ -85,18 +77,13 @@
         with open(filename, 'r') as file:
             contents = file.read()
         self.assertEqual(contents, 'http://fantastic/')
-        mock_open_port.assert_called_once()
-        mock_close_port.assert_called_once()
         self.assertEqual(
             len(os.listdir("%s/%s" % (self.dirname, "sites-available"))), 1)
 
     @patch('hooks.log')
     @patch('subprocess.call')
-    @patch('hooks.close_port')
-    @patch('hooks.open_port')
     def test_create_vhost_template_directly(
-            self, mock_open_port, mock_close_port,
-            mock_call, mock_log):
+            self, mock_call, mock_log):
         """Create a vhost file, check contents."""
         template = b64encode("http://{{ variable }}/")
         config_data = {
@@ -111,8 +98,6 @@
         with open(filename, 'r') as file:
             contents = file.read()
         self.assertEqual(contents, 'http://fantastic/')
-        mock_open_port.assert_called_once()
-        mock_close_port.assert_called_once()
         self.assertEqual(
             len(os.listdir("%s/%s" % (self.dirname, "sites-available"))), 1)
 

=== modified file 'metadata.yaml'
--- metadata.yaml	2014-12-18 01:21:23 +0000
+++ metadata.yaml	2016-01-15 23:00:04 +0000
@@ -18,6 +18,8 @@
     scope: container
   website:
     interface: http
+  logs:
+    interface: logs
   apache-website:
     interface: apache-website
     scope: container