Merge lp:~ahasenack/landscape-charm/service-names-1379133 into lp:~landscape/landscape-charm/trunk

[Andreas Hasenack]

Update the haproxy jinja variables in the apache vhost template files according to whatever service name haproxy was deployed as.

Update the haproxy jinja variables in the apache vhost template files according to whatever service name haproxy was deployed as.

The apache charm expects the jinja variables in the vhost template files to be named after the services it relates to. So if haproxy is deployed as "landscape-haproxy", the variable needs to contain "landscapehaproxy" (sic).

An additional complication is that jinja variables need to be proper python, and the apache charm chose to swallow the "-". So we need to apply the same transformation in our side.

[Benji York (benji)]

This branch looks good. I had one regex simplification and a couple of style nitpicks, but once you consider those you're good to go.

review: Approve

[lp:~ahasenack/landscape-charm/service-names-1379133]

219. By Andreas Hasenack on 2014-10-14

Simplify regexp

220. By Andreas Hasenack on 2014-10-14

Other review comments.

[Benji York (benji)]

Inline comment about my memory of PEP 257 failing.

[lp:~ahasenack/landscape-charm/service-names-1379133]

221. By Andreas Hasenack on 2014-10-15

docstrings

[Chad Smith (chad.smith)]

[1] & [2] inline comments that are nits. I'd like to avoid truncating unacceptable characters in var names if we can replace them with underscores instead. Maybe the hyphens in service names were there for readability.

[3] Also can you run "make lint" target and sort the lint errors in this branch.

[Andreas Hasenack (ahasenack)]

Chad, I'm doing what the apache charm already does. If haproxy is deployed as "landscape-haproxy", the apache charm will be looking for a variable prefixed with "landscapehaproxy". They don't replace "-" with "_" :(

[Chad Smith (chad.smith)]

minor inline docstring comment.

I'll check a full deployment with a hyphenated service name to confirm but +1 otherwise.

review: Approve

[Chad Smith (chad.smith)]

validated both 400 errors on trunk and the fix. +1

review: Approve

[lp:~ahasenack/landscape-charm/service-names-1379133]

222. By Andreas Hasenack on 2014-10-16

lint

223. By Andreas Hasenack on 2014-10-16

docstrings

224. By Andreas Hasenack on 2014-10-16

- haproxy_service_name is now a mandatory argument
- docstrings

225. By Andreas Hasenack on 2014-10-16

test_no_haproxy_service_name_if_not_related_to_haproxy()

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2014-10-01 20:16:43 +0000
+++ hooks/hooks.py	2014-10-16 12:49:51 +0000
@@ -98,22 +98,51 @@
             services=yaml.safe_dump(_get_services_haproxy()))
 
 
-def notify_vhost_config_relation(relation_id=None):
+def _get_haproxy_service_name():
+    """
+    Find out what service name was used to deploy haproxy and sanitize it
+    according to the jinja requirements. The service name is used as a
+    variable name in the apache vhost jinja template.
+
+    For example, if haproxy is deployed as "landscape-haproxy", the apache
+    charm will transform that into landscapehaproxy.
+    """
+    haproxy_relations = juju.relation_ids("website")
+    if not haproxy_relations:
+        return None
+    haproxy_relation_units = juju.relation_list(haproxy_relations[0])
+    haproxy_service = haproxy_relation_units[0].rsplit("/", 1)[0]
+    # jinja2 templates require python-type variables, remove all characters
+    # that do not comply
+    haproxy_service = re.sub("\W", "", haproxy_service)
+    return haproxy_service
+
+
+def _get_vhost_template(template_filename, haproxy_service_name):
+    """Expand the template with the provided haproxy service name."""
+    with open("%s/config/%s" % (ROOT, template_filename), "r") as handle:
+        contents = handle.read()
+        contents = re.sub(r"{{ haproxy_([^}]+) }}", r"{{ %s_\1 }}" %
+                          haproxy_service_name, contents)
+    return contents
+
+
+def notify_vhost_config_relation(haproxy_service_name, relation_id=None):
     """
     Notify the vhost-config relation.
 
     This will mark it "ready to proceed".  If relation_id is specified
     use that as the relation context, otherwise look up and notify all
     vhost-config relations.
+
+    The haproxy_service_name is needed so that the vhost template can be
+    adjusted with the correct jinja variable that apache will look for.
     """
     vhosts = []
-    with open("%s/config/vhostssl.tmpl" % ROOT, 'r') as handle:
-        vhosts.append({
-            "port": "443", "template": b64encode(handle.read())})
-    with open("%s/config/vhost.tmpl" % ROOT, 'r') as handle:
-        vhosts.append({
-            "port": "80", "template": b64encode(handle.read())})
-
+    contents = _get_vhost_template("vhostssl.tmpl", haproxy_service_name)
+    vhosts.append({"port": "443", "template": b64encode(contents)})
+    contents = _get_vhost_template("vhost.tmpl", haproxy_service_name)
+    vhosts.append({"port": "80", "template": b64encode(contents)})
     relation_ids = [relation_id]
     if relation_id is None:
         relation_ids = juju.relation_ids("vhost-config")
@@ -394,7 +423,15 @@
     if not juju.relation_ids("vhost-config"):
         return
 
-    notify_vhost_config_relation(os.environ.get("JUJU_RELATION_ID", None))
+    # If we are not related to haproxy yet, noop, because we need to know the
+    # haproxy service name so we can set the template variable to the correct
+    # name in the apache vhost template.
+    haproxy_service_name = _get_haproxy_service_name()
+    if not haproxy_service_name:
+        return
+
+    notify_vhost_config_relation(haproxy_service_name,
+                                 os.environ.get("JUJU_RELATION_ID", None))
 
     access_details = _get_db_access_details()
     if not access_details:

=== modified file 'hooks/lib/util.py'
--- hooks/lib/util.py	2014-10-01 20:16:43 +0000
+++ hooks/lib/util.py	2014-10-16 12:49:51 +0000
@@ -18,7 +18,7 @@
     Returns true if the given email is safe to use and has no "funny"
     characters. We don't go overboard and look for an RFC compliant email
     here.
-    
+
     @param email: string containing the email to be validated
     """
     valid_email_re = r"^[\w.+-]+@[\w-]+\.[\w.]+$"
@@ -103,7 +103,7 @@
         # let the exception be raised and stderr go through as usual.
         check_output(cmd, cwd="/opt/canonical/landscape", env=env)
         juju.juju_log("Administrator called %s with email %s created" %
-            (admin_name, admin_email))
+                      (admin_name, admin_email))
         return True
     else:
         juju.juju_log("DB not empty, skipping first admin creation")

=== modified file 'hooks/test_hooks.py'
--- hooks/test_hooks.py	2014-10-01 20:16:43 +0000
+++ hooks/test_hooks.py	2014-10-16 12:49:51 +0000
@@ -1,5 +1,4 @@
 from configobj import ConfigObj
-from itertools import product
 import base64
 import hooks
 import mocker
@@ -62,7 +61,10 @@
         Hardcode expected relation_list for tests.  Feel free to expand
         as more tests are added.
         """
-        return list(self._relation_list)
+        if relation_id == "website:1":
+            return ["landscape-haproxy/0"]
+        else:
+            return list(self._relation_list)
 
     def unit_get(self, *args):
         """
@@ -204,9 +206,10 @@
         self.mocker.replay()
         with unittest.TestCase.assertRaises(self, ValueError) as invalid_email:
             hooks.util.create_landscape_admin(db_user, db_password, db_host,
-                admin_name, admin_email, admin_password)
+                                              admin_name, admin_email,
+                                              admin_password)
         self.assertEqual("Invalid administrator email %s" % admin_email,
-            invalid_email.exception.message)
+                         invalid_email.exception.message)
 
     def test_first_admin_not_created_if_account_not_empty(self):
         """
@@ -1575,6 +1578,43 @@
         self._service_conf.seek(0)
         self.assertFalse(hooks._is_db_up())
 
+    def test__get_haproxy_service_name(self):
+        """
+        _get_haproxy_service_name() returns the jinja-ready service name used
+        to deploy haproxy.
+        """
+        haproxy_service_name = hooks._get_haproxy_service_name()
+        self.assertEqual(haproxy_service_name, "landscapehaproxy")
+
+    def test_no_haproxy_service_name_if_not_related_to_haproxy(self):
+        """
+        _get_haproxy_service_name() returns None if we are not related to
+        haproxy.
+        """
+        def no_website_relation(relation_name):
+            return None
+
+        self.addCleanup(setattr, hooks.juju, "relation_ids",
+                        hooks.juju.relation_ids)
+        hooks.juju.relation_ids = no_website_relation
+        haproxy_service_name = hooks._get_haproxy_service_name()
+        self.assertIsNone(haproxy_service_name)
+
+    def test__get_vhost_template(self):
+        """
+        The haproxy prefix in the template variables is replaced by the
+        name of the actual haproxy service that is part of the deployment.
+        """
+        template_file = "vhostssl.tmpl"
+        with open("%s/config/%s" % (hooks.ROOT, template_file), "r") as t:
+            original_template = t.read()
+        new_template = hooks._get_vhost_template(template_file,
+                                                 "landscape-haproxy")
+        self.assertIn("{{ haproxy_msgserver }}", original_template)
+        self.assertNotIn("{{ landscape-haproxy_msgserver }}",
+                         original_template)
+        self.assertIn("{{ landscape-haproxy_msgserver }}", new_template)
+
 
 class TestHooksServiceMock(TestHooks):
 
@@ -1759,7 +1799,7 @@
         """
         notify the vhost-config relation on a separate ID.
         """
-        hooks.notify_vhost_config_relation("foo/0")
+        hooks.notify_vhost_config_relation("haproxy", "foo/0")
         with open("%s/config/vhostssl.tmpl" % hooks.ROOT, 'r') as f:
             vhostssl_template = f.read()
         with open("%s/config/vhost.tmpl" % hooks.ROOT, 'r') as f:
@@ -1772,7 +1812,7 @@
 
     def test_notify_vhost_config_relation(self):
         """notify the vhost-config relation on the "current" ID."""
-        hooks.notify_vhost_config_relation()
+        hooks.notify_vhost_config_relation("haproxy")
         with open("%s/config/vhostssl.tmpl" % hooks.ROOT, 'r') as f:
             vhostssl_template = f.read()
         with open("%s/config/vhost.tmpl" % hooks.ROOT, 'r') as f:
@@ -1802,7 +1842,7 @@
                 "user": "user",
                 "password": "password"}})
         notify_vhost = self.mocker.replace(hooks.notify_vhost_config_relation)
-        notify_vhost(None)
+        notify_vhost(hooks._get_haproxy_service_name(), None)
         self.mocker.replay()
         self.assertRaises(SystemExit, hooks.vhost_config_relation_changed)
         self.assertIn('Waiting for data from apache', hooks.juju._logs[-1])
@@ -1820,7 +1860,7 @@
                 "user": "user",
                 "password": "password"}})
         notify_vhost = self.mocker.replace(hooks.notify_vhost_config_relation)
-        notify_vhost(None)
+        notify_vhost(hooks._get_haproxy_service_name(), None)
         is_db_up = self.mocker.replace(hooks._is_db_up)
         is_db_up()
         self.mocker.result(False)
@@ -1843,7 +1883,7 @@
                 "user": "user",
                 "password": "password"}})
         notify_vhost = self.mocker.replace(hooks.notify_vhost_config_relation)
-        notify_vhost(None)
+        notify_vhost(hooks._get_haproxy_service_name(), None)
         is_db_up = self.mocker.replace(hooks._is_db_up)
         is_db_up()
         self.mocker.result(True)
@@ -1868,7 +1908,7 @@
                 "user": "user",
                 "password": "password"}})
         notify_vhost = self.mocker.replace(hooks.notify_vhost_config_relation)
-        notify_vhost(None)
+        notify_vhost(hooks._get_haproxy_service_name(), None)
         mock_conn = self.mocker.mock()
         mock_conn.close()
         connect_exclusive = self.mocker.replace(hooks.util.connect_exclusive)
@@ -1906,7 +1946,7 @@
                 "user": "user",
                 "password": "password"}})
         notify_vhost = self.mocker.replace(hooks.notify_vhost_config_relation)
-        notify_vhost(None)
+        notify_vhost(hooks._get_haproxy_service_name(), None)
         is_db_up = self.mocker.replace(hooks._is_db_up)
         is_db_up()
         self.mocker.result(True)
@@ -1926,6 +1966,25 @@
         with open(hooks.SSL_CERT_LOCATION, 'r') as f:
             self.assertEqual("foobar", f.read())
 
+    def test_vhost_config_relation_exits_if_haproxy_not_ready(self):
+        """
+        notify_vhost_config_relation() is not called if the haproxy relation
+        is not there.
+        """
+        def should_not_be_here(*args):
+            raise AssertionError("notify_vhost_config_relation() should not "
+                                 "be called")
+
+        self.addCleanup(setattr, hooks, "vhost_config_relation_changed",
+                        hooks.vhost_config_relation_changed)
+        hooks.notify_vhost_config_relation = should_not_be_here
+        get_haproxy_service_name = self.mocker.replace(
+            hooks._get_haproxy_service_name)
+        get_haproxy_service_name()
+        self.mocker.result(None)
+        self.mocker.replay()
+        hooks.vhost_config_relation_changed()
+
 
 class TestHooksUtils(TestHooks):
     def test__setup_apache(self):