Merge lp:~abentley/charms/trusty/apache2/apache2-ports into lp:charms/trusty/apache2

[Aaron Bentley]

Unify port handling.

This branch unifies open-port/close-port handling. It is a prerequisite for apache2-website-interface support, which I will introduce in a follow-on branch.

Multiple configuration options may refer to the same port. For example, a vhost_http_template might refer to port 80 and so might a vhost-config relation. But they can have conflicting ideas about whether a port should be open. If the vhost-config has an empty template, port 80 will be closed, even though the vhost_http_template wants it open. And of course, there can be multiple vhost-configs with their own conflicting opinions.

This code unifies the port handling, so that it is done in a single place. If any configuration mechanism wants a paticular port open, it is open. Otherwise it is closed. It also records the current list of ports, so that future hook runs can detect when a port has changed, and close the old port.

[Review Queue (review-queue)]

This items has failed automated testing! Results available here http://reports.vapour.ws/charm-tests/charm-bundle-test-10870-results

review: Needs Fixing (automated testing)

[Aaron Bentley (abentley)]

The automated test failures are a pre-existing issue:
http://reports.vapour.ws/charm-tests/charm-bundle-test-10700-results

They are not caused by my changes.

[Adam Israel (aisrael)]

Hi Aaron,

I had the opportunity to review this MP today, and I agree that the failing tests are unrelated to your changes. LGTM. +1

review: Approve

[David Britton (dpb)]

This looks great, Aaron.

Thanks! +1

review: Approve

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2014-11-20 00:06:41 +0000
+++ hooks/hooks.py	2015-01-07 14:04:20 +0000
@@ -1,5 +1,6 @@
 #!/usr/bin/env python
 
+import errno
 import os
 import re
 import socket
@@ -519,11 +520,10 @@
     """
     if protocol is None:
         protocol = str(port)
-    close_port(port)
     if template_str is None:
         if not config_key or not config_data[config_key]:
             log("Vhost Template not provided, not configuring: %s" % port)
-            return
+            return False
         template_str = config_data[config_key]
     from jinja2 import Template
     template = Template(str(base64.b64decode(template_str)))
@@ -533,8 +533,8 @@
     log("Writing file %s with config and relation data" % vhost_file)
     with open(vhost_file, 'w') as vhost:
         vhost.write(str(template.render(template_data)))
-    open_port(port)
     subprocess.call(['/usr/sbin/a2ensite', vhost_name])
+    return True
 
 
 def config_changed():
@@ -572,13 +572,15 @@
     create_security()
 
     ports = {'http': 80, 'https': 443}
+    all_ports = set()
     for protocol, port in ports.iteritems():
-        create_vhost(
-            port,
-            protocol=protocol,
-            config_key="vhost_%s_template" % protocol,
-            config_data=config_data,
-            relationship_data=relationship_data)
+        if create_vhost(
+                port,
+                protocol=protocol,
+                config_key="vhost_%s_template" % protocol,
+                config_data=config_data,
+                relationship_data=relationship_data):
+            all_ports.add(port)
 
     cert_file = _get_cert_file_location(config_data)
     key_file = _get_key_file_location(config_data)
@@ -686,7 +688,8 @@
             f.write('\n')
             os.chmod(key_file, 0444)
 
-    update_vhost_config_relation()
+    all_ports.update(update_vhost_config_relation())
+    ensure_ports(all_ports)
     update_nrpe_checks()
     ship_logrotate_conf()
 
@@ -696,10 +699,11 @@
     Update the vhost file and include the certificate in the relation
     if it is self-signed.
     """
+    vhost_ports = set()
     relation_data = relations_of_type("vhost-config")
     config_data = config_get()
     if relation_data is None:
-        return
+        return vhost_ports
 
     for unit_data in relation_data:
         if "vhosts" in unit_data:
@@ -711,11 +715,13 @@
             try:
                 vhosts = yaml.safe_load(unit_data["vhosts"])
                 for vhost in vhosts:
-                    create_vhost(
-                        vhost["port"],
-                        template_str=vhost["template"],
-                        config_data=config_data,
-                        relationship_data=all_relation_data)
+                    port = vhost["port"]
+                    if create_vhost(
+                            port,
+                            template_str=vhost["template"],
+                            config_data=config_data,
+                            relationship_data=all_relation_data):
+                        vhost_ports.add(port)
             except Exception as e:
                 log("Error reading configuration data from relation! %s" % e)
                 raise
@@ -736,6 +742,7 @@
             vhost_relation_settings["ssl_cert"] = cert
     for id in relation_ids("vhost-config"):
         relation_set(relation_id=id, relation_settings=vhost_relation_settings)
+    return vhost_ports
 
 
 def start_hook():
@@ -768,6 +775,39 @@
                      'hostname=%s' % my_host])
 
 
+def ensure_ports(ports):
+    """Ensure that only the desired ports are open."""
+    open_ports = set(get_open_ports())
+    ports = set(ports)
+    wanted_closed = ports.difference(open_ports)
+    for port in sorted(wanted_closed):
+        open_port(port)
+    unwanted_open = open_ports.difference(ports)
+    for port in sorted(unwanted_open):
+        close_port(port)
+    set_open_ports(list(sorted(ports)))
+
+
+def get_open_ports():
+    """Get the list of open ports from the standard file."""
+    try:
+        pfile = open(os.path.join(os.environ['CHARM_DIR'], 'ports.yaml'))
+    except IOError as e:
+        if e.errno == errno.ENOENT:
+            return []
+        else:
+            raise
+    with pfile:
+        return yaml.safe_load(pfile)
+
+
+def set_open_ports(ports):
+    """Write the list of open ports to the standard file."""
+    ports_path = os.path.join(os.environ['CHARM_DIR'], 'ports.yaml')
+    with open(ports_path, 'w') as pfile:
+        yaml.safe_dump(ports, pfile)
+
+
 ###############################################################################
 # Main section
 ###############################################################################
 
=== modified file 'hooks/tests/test_balancer_hook.py'
--- hooks/tests/test_balancer_hook.py	2014-10-13 07:55:57 +0000
+++ hooks/tests/test_balancer_hook.py	2015-01-07 14:04:20 +0000
@@ -207,12 +207,12 @@
         check_output.assert_called_with('foo', 1, 2, bar='baz')
 
     @patch('subprocess.check_output')
-    @patch('sys.stdout.write')
-    def test_prints_and_reraises_run_error(self, write, check_output):
+    @patch('sys.stdout')
+    def test_prints_and_reraises_run_error(self, out, check_output):
         check_output.side_effect = RuntimeError('some error')
 
         self.assertRaises(RuntimeError, hooks.run, 'some command')
-        self.assertEqual(write.mock_calls, [
+        self.assertEqual(out.write.mock_calls, [
             call('some error'),
             call('\n'),
         ])
 
=== modified file 'hooks/tests/test_config_changed.py'
--- hooks/tests/test_config_changed.py	2014-05-21 03:24:19 +0000
+++ hooks/tests/test_config_changed.py	2015-01-07 14:04:20 +0000
@@ -16,6 +16,8 @@
         if os.path.exists(self.dirname):
             shutil.rmtree(self.dirname)
 
+    @patch('hooks.get_open_ports')
+    @patch('hooks.set_open_ports')
     @patch('hooks.subprocess.call')
     @patch('hooks.close_port')
     @patch('hooks.open_port')
@@ -41,7 +43,8 @@
             mock_create_mpm_workerfile, mock_create_security, mock_run,
             mock_update_nrpe_checks, mock_ship_logrotate_conf,
             mock_ensure_package_status, mock_ensure_extra_packages,
-            mock_conf_disable, mock_open_port, mock_close_port, mock_call):
+            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 = {
             "ssl_cert": "",
 
=== modified file 'hooks/tests/test_hooks.py'
--- hooks/tests/test_hooks.py	2014-05-20 16:54:57 +0000
+++ hooks/tests/test_hooks.py	2015-01-07 14:04:20 +0000
@@ -1,7 +1,27 @@
+from contextlib import contextmanager
+import os.path
+from tempfile import mkdtemp
+from shutil import rmtree
+
 from testtools import TestCase
+from mock import (
+    call,
+    patch,
+)
+import yaml
+
 import hooks
 
 
+@contextmanager
+def temp_dir():
+    directory = mkdtemp()
+    try:
+        yield directory
+    finally:
+        rmtree(directory)
+
+
 class HooksTest(TestCase):
     def test__get_key_file_location_empty(self):
         """No ssl_keylocation, expect None."""
@@ -32,3 +52,48 @@
         """ssl_keylocation, expect correct path."""
         self.assertEqual(hooks._get_chain_file_location(
             {"ssl_chainlocation": "foo"}), "/etc/ssl/certs/foo")
+
+
+class TestEnsurePorts(TestCase):
+
+    def test_ensure_ports(self):
+        with patch('hooks.open_port') as hop_mock:
+            with patch('hooks.set_open_ports') as hsop_mock:
+                with patch('hooks.get_open_ports', return_value=[]):
+                    hooks.ensure_ports([80, 81])
+        self.assertEqual(hop_mock.mock_calls, [call(80), call(81)])
+        hsop_mock.assert_called_once_with([80, 81])
+
+    def test_idempotent(self):
+        with patch('hooks.get_open_ports', return_value=[81, 82]):
+            with patch('hooks.open_port') as hop_mock:
+                with patch('hooks.set_open_ports') as hsop_mock:
+                    hooks.ensure_ports([81, 83, 82])
+        self.assertEqual(hop_mock.mock_calls, [call(83)])
+        hsop_mock.assert_called_once_with([81, 82, 83])
+
+    def test_closes_unneeded(self):
+        with patch('hooks.get_open_ports', return_value=[81, 82, 83, 84]):
+            with patch('hooks.close_port') as hcp_mock:
+                with patch('hooks.set_open_ports') as hsop_mock:
+                    hooks.ensure_ports([81, 82])
+        self.assertEqual(hcp_mock.mock_calls, [call(83), call(84)])
+        hsop_mock.assert_called_once_with([81, 82])
+
+
+class TestGetSetOpenPorts(TestCase):
+
+    def test_get_open_ports(self):
+        with temp_dir() as charm_dir:
+            with patch('os.environ', {'CHARM_DIR': charm_dir}):
+                self.assertEqual(hooks.get_open_ports(), [])
+                with open(os.path.join(charm_dir, 'ports.yaml'), 'w') as pfile:
+                    yaml.safe_dump([47, 67], pfile)
+                self.assertEqual(hooks.get_open_ports(), [47, 67])
+
+    def test_set_open_ports(self):
+        with temp_dir() as charm_dir:
+            with patch('os.environ', {'CHARM_DIR': charm_dir}):
+                hooks.set_open_ports([44, 23])
+            with open(os.path.join(charm_dir, 'ports.yaml')) as pfile:
+                self.assertEqual(yaml.safe_load(pfile), [44, 23])
 
=== modified file 'hooks/tests/test_vhost_config_relation.py'
--- hooks/tests/test_vhost_config_relation.py	2014-05-21 15:32:02 +0000
+++ hooks/tests/test_vhost_config_relation.py	2015-01-07 14:04:20 +0000
@@ -208,8 +208,12 @@
         mock_create_vhost.assert_has_calls([
             call("80", template_str=b64encode("80"),
                  config_data=config_data, relationship_data={}),
+            call().__nonzero__(),
             call("443", template_str=b64encode("443"),
                  config_data=config_data, relationship_data={}),
+            call().__nonzero__(),
             call("444", template_str=b64encode("444"),
-                 config_data=config_data, relationship_data={})])
+                 config_data=config_data, relationship_data={}),
+            call().__nonzero__(),
+            ])
         self.assertEqual(mock_create_vhost.call_count, 3)