Merge lp:~abentley/charms/trusty/apache2/apache-website into lp:charms/trusty/apache2

[Aaron Bentley]

Support apache-website interface.

This branch updates the apache2 charm to support a new configuration method: the apache-website interface.

Basically, this is a way to push some configuration responsibility out to subordinate charms. The subordinate charms can then make opinionated decisions, and many vhosts can be supported simultaneously in an understandable way.

juju-qa is using this approach already. We have one charm, apache2-reverse-proxy, which supports proxying arbitrary web sites, and we are discussing having another for proxying s3. The mechanism is also suitable for supplying entire static web sites as subordinate charms. The version of apache2-reverse-proxy which supports the apache-website interface is at https://code.launchpad.net/~abentley/charms/trusty/apache2-reverseproxy/apache-website-interface/

In this approach
1. The subordinate charm supplies the apache config for its particular vhost
2. The subordinate charm indicates whether it is "enabled" or not. (This allows subordinates to delay activating their site until they, themselves are fully configured.)
3. The subordinate charm indicates all the ports it uses. The Apache charm then ensures all ports for all subordinates are open, both on the Apache daemon and via juju open-port. When a port is no longer in use by any configured site, the charm can detect it and close that port.
4. The subordinate indicates which modules its site requires. If Apache is configured to disable those modules, it disables the site. Otherwise, it makes sure that all modules for all sites are enabled.
5. The configurations are named after the relation-id rather than the site name, because the relation-id is permanent, unlike site names.

All tests pass locally.

[Adam Israel (aisrael)]

Hi Aaron,

Thanks for your efforts to land this new functionality in the apache2 charm. I had the opportunity to review the merge proposal today, but ran into a few issues.

There was a merge conflict against trunk, so I used your branch directly.

One of the modules installed into .venv, linecache2, failed with a syntax error:

Compiling /charms/trusty/apache2/.venv/build/linecache2/linecache2/tests/inspect_fodder2.py ...
  File "/charms/trusty/apache2/.venv/build/linecache2/linecache2/tests/inspect_fodder2.py", line 102
    def keyworded(*arg1, arg2=1):

The install continued and the tests ran, however, two of them failed:

tests.test_cert.CertTests.test_is_selfsigned_cert_stale_private_address_changed
tests.test_cert.CertTests.test_is_selfsigned_cert_stale_public_address_changed

I don't believe your changes have anything to do with these two failures; they also fail in trunk. I'm not going to hold those failures against this merge. I've posted the complete log of the test run here:

http://pastebin.ubuntu.com/10622663/

I would like to see the interface documented in the README, so developers unfamiliar with the interface will know how to construct or use subordinates that take advantage of the new functionality, before I'm comfortable pushing this forward. With that, and the clean merge into trunk, this should be good to move forward.

Thanks again for your time on this!

review: Needs Fixing

[Aaron Bentley (abentley)]

Thanks for your review. The linecache failure appears to be python3 syntax that somehow slipped into their python 2 release. arg2 is a "keyword-only" parameter, because it follows a *arg. https://docs.python.org/3/glossary.html#term-parameter

I'll push a revised branch when I get a chance.

[Marco Ceppi (marcoceppi)]

Moving this to Work In Progress, when you're ready for another review move this Needs Review

[Aaron Bentley (abentley)]

I've fixed the conflicts and added docs to the README.

[lp:~abentley/charms/trusty/apache2/apache-website]

80. By Aaron Bentley on 2015-04-15

Add docs.

[Adam Israel (aisrael)]

Hi Aaron,

Thanks for the updated README. Other than the previously noted test failures in trunk, everything here looks good to me. You have my +1

review: Approve

[Tim Van Steenburgh (tvansteenburgh)]

+1, LGTM, nice addition, thanks abentley!

review: Approve

=== modified file 'README.md'
--- README.md	2015-02-27 14:12:58 +0000
+++ README.md	2015-04-15 23:00:26 +0000
@@ -245,6 +245,26 @@
    be available in this setting as a base64 encoded string.
 
 
+### Using the apache-website relation
+
+The apache-website relation provides a very flexible way of configuring an
+Apache2 website, using subordinate charms.  It can support reverse proxies,
+static websites, and probably many other forms.
+
+To support this relation, a charm must set
+
+ * `domain` - The fully-qualified domain name of the site.
+
+ * `enabled` - Must be set to 'true' when the web site is ready to be used.
+
+ * `site_config` - A vhost configuration block.
+
+ * `site_modules` - A list of modules required by the site.  If any of these
+   appear in `disable_modules`, the site will not be enabled.  Otherwise, any
+   required modules will be loaded.
+
+ * `ports` - A space-separated list of ports that the site uses.
+
 ## Certs, keys and chains
 
 `ssl_keylocation`, `ssl_certlocation` and `ssl_chainlocation` are
 
=== added symlink 'hooks/apache-website-relation-changed'
=== target is u'hooks.py'
=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2015-03-13 16:05:48 +0000
+++ hooks/hooks.py	2015-04-15 23:00:26 +0000
@@ -596,23 +596,29 @@
         if is_apache24():
             enable_module('lbmethod_byrequests')
 
-    if config_data['enable_modules']:
-        module_list = config_data['enable_modules'].split()
-        for module in module_list:
-            enable_module(module)
+    disabled_modules = config_data['disable_modules'].split()
+    apache_websites = ApacheWebsites.from_config(
+        relations_of_type("apache-website"), disabled_modules)
+    enabled_modules = config_data.get('enable_modules', '').split()
+    enabled_modules = apache_websites.list_enabled_modules(enabled_modules)
+    for module in enabled_modules:
+        enable_module(module)
 
     if config_data['disable_modules']:
-        module_list = config_data['disable_modules'].split()
-        for module in module_list:
+        for module in disabled_modules:
             disable_module(module)
 
+    apache_websites.disable_sites()
+    apache_websites.write_configs()
+    apache_websites.enable_sites()
+    apache_websites.configure_extra_ports()
+    all_ports = apache_websites.list_enabled_ports()
     enable_mpm(config_data)
     # XXX we only configure the worker mpm?
     create_mpm_workerfile()
     create_security()
 
     ports = {'http': 80, 'https': 443}
-    all_ports = set()
     for protocol, port in ports.iteritems():
         if create_vhost(
                 port,
@@ -710,8 +716,7 @@
     # Disable the default website because we don't want people to see the
     # "It works!" page on production services and remove the
     # conf.d/other-vhosts-access-log conf.
-    if os.path.exists(site_filename("000-default", True)):
-        run(["/usr/sbin/a2dissite", "000-default"])
+    ensure_disabled(["000-default"])
     conf_disable("other-vhosts-access-log")
     if os.path.exists(conf_filename("other-vhosts-access-log")):
         os.unlink(conf_filename("other-vhosts-access-log"))
@@ -734,6 +739,105 @@
     ship_logrotate_conf()
 
 
+def ensure_disabled(sites):
+    to_disable = [s for s in sites if os.path.exists(site_filename(s, True))]
+    if len(to_disable) == 0:
+        return
+    run(["/usr/sbin/a2dissite"] + to_disable)
+
+
+def ensure_removed(filename):
+    try:
+        os.unlink(filename)
+    except OSError as e:
+        if e.errno != errno.ENOENT:
+            raise
+
+
+class ApacheWebsites:
+
+    @classmethod
+    def from_config(cls, relations, disabled_modules):
+        """Return an ApacheWebsites with information about all sites."""
+        if relations is None:
+            relations = []
+        self_relations = {}
+        for relation in relations:
+            self_relation = {'domain': relation.get('domain')}
+            enabled = bool(relation.get('enabled', 'False').lower() == 'true')
+            site_modules = relation.get('site_modules', '').split()
+            for module in site_modules:
+                if module in disabled_modules:
+                    enabled = False
+                    log('site {} requires disabled_module {}'.format(
+                        relation['__relid__'], module))
+                break
+            self_relation['site_modules'] = site_modules
+            self_relation['enabled'] = enabled
+            self_relation['site_config'] = relation.get('site_config')
+            self_relation['ports'] = [
+                int(p) for p in relation.get('ports', '').split()]
+            self_relations[relation['__relid__']] = self_relation
+        return cls(self_relations)
+
+    def __init__(self, relations):
+        self.relations = relations
+
+    def write_configs(self):
+        for key, relation in self.relations.items():
+            config_file = site_filename(key)
+            site_config = relation['site_config']
+            if site_config is None:
+                ensure_removed(config_file)
+            else:
+                with open(config_file, 'w') as output:
+                    output.write(site_config)
+
+    def iter_enabled_sites(self):
+        return ((k, v) for k, v in self.relations.items() if v['enabled'])
+
+    def enable_sites(self):
+        enabled_sites = [k for k, v in self.iter_enabled_sites()]
+        enabled_sites.sort()
+        if len(enabled_sites) == 0:
+            return
+        subprocess.check_call(['/usr/sbin/a2ensite'] + enabled_sites)
+
+    def disable_sites(self):
+        disabled_sites = [k for k, v in self.relations.items()
+                          if not v['enabled']]
+        disabled_sites.sort()
+        if len(disabled_sites) == 0:
+            return
+        ensure_disabled(disabled_sites)
+
+    def list_enabled_modules(self, enabled_modules):
+        enabled_modules = set(enabled_modules)
+        for key, relation in self.iter_enabled_sites():
+            enabled_modules.update(relation['site_modules'])
+        return enabled_modules
+
+    def list_enabled_ports(self):
+        enabled_ports = set()
+        for key, relation in self.iter_enabled_sites():
+            enabled_ports.update(relation['ports'])
+        return enabled_ports
+
+    def configure_extra_ports(self):
+        extra_ports = self.list_enabled_ports()
+        extra_ports.discard(80)
+        extra_ports.discard(443)
+        extra_ports_conf = conf_filename('extra_ports')
+        if len(extra_ports) > 0:
+            with file(extra_ports_conf, 'w') as f:
+                for port in sorted(extra_ports):
+                    f.write('Listen {}\n'.format(port))
+            conf_enable('extra_ports')
+        else:
+            conf_disable('extra_ports')
+            ensure_removed(extra_ports_conf)
+
+
 def update_vhost_config_relation():
     """
     Update the vhost file and include the certificate in the relation
@@ -884,6 +988,8 @@
         config_changed()
     elif hook_name == "website-relation-joined":
         website_interface("joined")
+    elif hook_name == 'apache-website-relation-changed':
+        config_changed()
     elif hook_name in ("nrpe-external-master-relation-changed",
                        "local-monitors-relation-changed"):
         update_nrpe_checks()
 
=== modified file 'hooks/tests/test_config_changed.py'
--- hooks/tests/test_config_changed.py	2015-03-13 15:24:47 +0000
+++ hooks/tests/test_config_changed.py	2015-04-15 23:00:26 +0000
@@ -64,17 +64,21 @@
             "vhost_http_template": "",
             "vhost_https_template": "",
         }
-        hooks.default_apache_base_dir = self.dirname
-        hooks.default_apache22_config_dir = "%s/conf.d" % self.dirname
-        hooks.default_apache24_config_dir = "%s/conf-available" % self.dirname
-        os.mkdir("%s/sites-enabled" % self.dirname)
-        os.mkdir("%s/sites-available" % self.dirname)
-        os.mkdir("%s/conf.d" % self.dirname)
-        hooks.config_changed()
-        self.assertEqual(
-            len(os.listdir("%s/%s" % (self.dirname, "sites-enabled"))), 0)
-        self.assertEqual(
-            len(os.listdir("%s/%s" % (self.dirname, "sites-available"))), 0)
+        base = patch.object(hooks, 'default_apache_base_dir', self.dirname)
+        config22 = patch.object(hooks, 'default_apache22_config_dir',
+                                "%s/conf.d" % self.dirname)
+        config24 = patch.object(hooks, 'default_apache24_config_dir',
+                                "%s/conf-available" % self.dirname)
+        with base, config22, config24:
+            os.mkdir("%s/sites-enabled" % self.dirname)
+            os.mkdir("%s/sites-available" % self.dirname)
+            os.mkdir("%s/conf.d" % self.dirname)
+            hooks.config_changed()
+            self.assertEqual(
+                len(os.listdir("%s/%s" % (self.dirname, "sites-enabled"))), 0)
+            self.assertEqual(
+                len(os.listdir("%s/%s" % (self.dirname, "sites-available"))),
+                0)
 
     @patch('hooks.orig_config_get')
     @patch('hooks.unit_get')
 
=== modified file 'hooks/tests/test_hooks.py'
--- hooks/tests/test_hooks.py	2014-12-29 19:16:25 +0000
+++ hooks/tests/test_hooks.py	2015-04-15 23:00:26 +0000
@@ -54,6 +54,151 @@
             {"ssl_chainlocation": "foo"}), "/etc/ssl/certs/foo")
 
 
+class TestApacheWebsites(TestCase):
+
+    def test_from_config_empty(self):
+        relations = [{
+            '__relid__': 'idrel',
+        }]
+        websites = hooks.ApacheWebsites.from_config(relations, [])
+        self.assertEqual(websites.relations, {'idrel': {
+            'domain': None,
+            'enabled': False,
+            'ports': [],
+            'site_config': None,
+            'site_modules': [],
+            }})
+
+    def test_from_config_filled(self):
+        relations = [{
+            '__relid__': 'idrel',
+            'domain': 'foo.example.com',
+            'ports': '80 8080',
+            'site_config': 'configfoo',
+            'site_modules': 'foo bar',
+        }]
+        websites = hooks.ApacheWebsites.from_config(relations, [])
+        self.assertEqual(websites.relations, {'idrel': {
+            'domain': 'foo.example.com',
+            'enabled': False,
+            'ports': [80, 8080],
+            'site_config': 'configfoo',
+            'site_modules': ['foo', 'bar'],
+            }})
+
+    def test_from_config_disabled_module_disables_site(self):
+        relations = [{
+            '__relid__': 'idrel',
+            'site_modules': 'foo bar',
+            'enabled': 'true',
+        }]
+        with patch('hooks.log') as log_mock:
+            websites = hooks.ApacheWebsites.from_config(relations, ['foo'])
+        self.assertEqual(websites.relations['idrel']['enabled'], False)
+        log_mock.assert_called_once_with(
+            'site idrel requires disabled_module foo')
+
+    def test_write_configs(self):
+        with temp_dir() as configs_temp:
+
+            def site_filename(name):
+                return os.path.join(configs_temp, name)
+
+            websites = hooks.ApacheWebsites({
+                'cfg': {'site_config': 'foobar'},
+                'fcg': {'site_config': None},
+                })
+            open(site_filename('fcg'), 'w').write('')
+            self.assertTrue(os.path.exists(site_filename('fcg')))
+            with patch('hooks.site_filename', site_filename):
+                websites.write_configs()
+            self.assertEqual('foobar', open(site_filename('cfg')).read())
+            self.assertFalse(os.path.exists(site_filename('fcg')))
+
+    def test_iter_enabled_sites(self):
+        websites = hooks.ApacheWebsites({
+            'fcg': {'site_config': None, 'enabled': True},
+            'cgf': {'site_config': None, 'enabled': False},
+            })
+        self.assertItemsEqual(
+            websites.iter_enabled_sites(),
+            [('fcg', {'site_config': None, 'enabled': True})])
+
+    def test_enable_sites(self):
+        websites = hooks.ApacheWebsites({
+            'fcg': {'site_config': None, 'enabled': False},
+            'cgf': {'site_config': None, 'enabled': False},
+            })
+        with patch('subprocess.check_call') as cc_mock:
+            websites.enable_sites()
+        self.assertEqual(len(cc_mock.mock_calls), 0)
+        websites.relations['fcg']['enabled'] = True
+        with patch('subprocess.check_call') as cc_mock:
+            websites.enable_sites()
+        cc_mock.assert_called_once_with(['/usr/sbin/a2ensite', 'fcg'])
+        websites.relations['cgf']['enabled'] = True
+        with patch('subprocess.check_call') as cc_mock:
+            websites.enable_sites()
+        cc_mock.assert_called_once_with(['/usr/sbin/a2ensite', 'cgf', 'fcg'])
+
+    def test_disable_sites(self):
+        websites = hooks.ApacheWebsites({
+            'fcg': {'site_config': None, 'enabled': True},
+            'cgf': {'site_config': None, 'enabled': True},
+            })
+        with temp_dir() as conf_dir:
+
+            def site_filename(f, e=False):
+                return os.path.join(conf_dir, f)
+
+            with patch('hooks.site_filename', site_filename):
+                open(hooks.site_filename('fcg'), 'w').close()
+                open(hooks.site_filename('cgf'), 'w').close()
+                with patch('subprocess.check_output') as co_mock:
+                    websites.disable_sites()
+                self.assertEqual(len(co_mock.mock_calls), 0)
+                websites.relations['fcg']['enabled'] = False
+                with patch('subprocess.check_output') as co_mock:
+                    websites.disable_sites()
+                co_mock.assert_called_once_with(['/usr/sbin/a2dissite', 'fcg'])
+                websites.relations['cgf']['enabled'] = False
+                with patch('subprocess.check_output') as co_mock:
+                    websites.disable_sites()
+        co_mock.assert_called_once_with(['/usr/sbin/a2dissite', 'cgf', 'fcg'])
+
+    def test_list_enabled_modules(self):
+        websites = hooks.ApacheWebsites({
+            'fcg': {'site_modules': ['foo'], 'enabled': True},
+            'cgf': {'site_modules': ['foo', 'bar'], 'enabled': True},
+            'gcf': {'site_modules': ['foo', 'baz'], 'enabled': False},
+            })
+        self.assertEqual(
+            websites.list_enabled_modules(['qux']), set(['foo', 'bar', 'qux']))
+
+    def test_list_enabled_ports(self):
+        websites = hooks.ApacheWebsites({
+            'fcg': {'ports': [80], 'enabled': True},
+            'cgf': {'ports': [80, 81], 'enabled': True},
+            'gcf': {'ports': [81, 82], 'enabled': False},
+            })
+        self.assertEqual(
+            websites.list_enabled_ports(), set([80, 81]))
+
+    def test_configure_extra_ports(self):
+        websites = hooks.ApacheWebsites({
+            'fcg': {'ports': [80], 'enabled': True},
+            'cgf': {'ports': [80, 81], 'enabled': True},
+            'gcf': {'ports': [81, 82], 'enabled': False},
+            })
+        with patch('hooks.conf_enable') as ce_mock:
+            with temp_dir() as conf_dir:
+                fake_conf = os.path.join(conf_dir, 'extra_ports.conf')
+                with patch('hooks.conf_filename', return_value=fake_conf):
+                    with patch('hooks.open_port'):
+                        websites.configure_extra_ports()
+        ce_mock.assert_called_once_with('extra_ports')
+
+
 class TestEnsurePorts(TestCase):
 
     def test_ensure_ports(self):
 
=== modified file 'metadata.yaml'
--- metadata.yaml	2014-10-29 00:10:14 +0000
+++ metadata.yaml	2015-04-15 23:00:26 +0000
@@ -18,6 +18,9 @@
     scope: container
   website:
     interface: http
+  apache-website:
+    interface: apache-website
+    scope: container
 requires:
   reverseproxy:
     interface: http