Merge ~pjdc/ubuntu-mirror-charm/+git/ubuntu-mirror-charm:shared-triggers into ubuntu-mirror-charm:master

[Paul Collins]

merge triggers where key is the same, which is the case when a unit is both a releases and a cdimage mirror

[Paul Collins (pjdc)]

Note pre-req.

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

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

[Paul Collins (pjdc)]

"make test": https://pastebin.canonical.com/p/9d4PCVn9bX/

[Haw Loeung (hloeung)]

LGTM

review: Approve (+1)

[Stuart Bishop (stub)]

This seems good. The CallableDict implementation can be cut down significantly, per inline comments.

review: Approve

[Paul Collins (pjdc)]

Replies for the record.

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

Change successfully merged at revision 9667b405eac9fc0e223190172d63ea22fc97b1e9

diff --git a/.coveragerc b/.coveragerc
 new file mode 100644
index 0000000..f1565c6
--- /dev/null
+++ b/.coveragerc
@@ -0,0 +1,2 @@
+[run]
+omit = hooks/charmhelpers/*
diff --git a/.gitignore b/.gitignore
index 490cc43..526bc62 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,5 +1,7 @@
 *~
 *.charm
+*.pyc
+*.pyo
 .tox
 .coverage
 __pycache__
diff --git a/hooks/hooks.py b/hooks/hooks.py
index e1f4d53..877fa43 100755
--- a/hooks/hooks.py
+++ b/hooks/hooks.py
@@ -561,17 +561,48 @@ def configure_vsftp(conf, hostname):
     log("CHARM: Finished configuring vsftp")
 
 
-#
-# Set up any external ssh sync triggers
-#
-def configure_triggers(conf, hostname):
+SSH_TRIGGER_TEMPLATE = 'no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty,command="{command}",{trigger}'
+
+
+# cdimage-builder.internal triggers releases and cdimage using the
+# same key, so when this unit has both roles, sync both of them.
+def merge_shared_key_triggers(conf, triggers):
+    """Merge (in place) triggers that use the same ssh key."""
+
+    cdimage_key = extract_ssh_public_key(triggers.get('cdimage'))
+    releases_key = extract_ssh_public_key(triggers.get('releases'))
+    if cdimage_key and releases_key and cdimage_key == releases_key:
+        releases = conf.mirror_map('releases')
+        cdimage = conf.mirror_map('cdimage')
+        command = '( {releases_command} releases ; {cdimage_command} cdimage ) &'.format(
+            releases_command=os.path.join(conf.script_dir(), releases['command']),
+            cdimage_command=os.path.join(conf.script_dir(), cdimage['command']),
+        )
+        triggers['releases'] = SSH_TRIGGER_TEMPLATE.format(
+            command=command,
+            trigger=releases['trigger'],
+        )
+        del(triggers['cdimage'])
+
+
+def extract_ssh_public_key(key):
+    """Return the field after the field that begins with "ssh-", which
+    should be the key itself.  This function can therefore cope with
+    fairly arbitrarily-decorated ssh keys, e.g. those with "from", etc."""
+
+    kelts = key.split(' ')
+    while kelts:
+        # This doesn't work for every type of key, but nobody should be using ECDSA anyway.
+        if kelts.pop(0).startswith('ssh-'):
+            return kelts[0]
+    return None
+
+
+def make_triggers(conf, hostname):
+    """Return this unit's triggers in a dict, keyed by role."""
     roles = conf.roles()
-    if hostname not in roles:
-        return
 
-    template = 'no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty,command="${command}",${trigger}'
-    tmpl_data = {}
-    contents = ""
+    triggers = {}
 
     for role in roles[hostname]:
         try:
@@ -583,15 +614,30 @@ def configure_triggers(conf, hostname):
             continue
         else:
             if mirror["trigger"]:
-                tmpl_data["command"] = "%s %s &" % (os.path.join(conf.script_dir(), mirror["command"]), role)
-                tmpl_data["trigger"] = mirror["trigger"]
-                contents += str(Template(template, tmpl_data)) + "\n"
+                triggers[role] = SSH_TRIGGER_TEMPLATE.format(
+                    command="%s %s &" % (os.path.join(conf.script_dir(), mirror["command"]), role),
+                    trigger=mirror["trigger"],
+                )
+
+    return triggers
+
+
+#
+# Set up any external ssh sync triggers
+#
+def configure_triggers(conf, hostname):
+    roles = conf.roles()
+    if hostname not in roles:
+        return
+
+    triggers = make_triggers(conf, hostname)
+    merge_shared_key_triggers(conf, triggers)
 
     keyfile = get_ssh_keyfile(conf.mirror_user())
     ssh_dir = os.path.dirname(keyfile)
     if not os.path.isdir(ssh_dir):
         mkdir(ssh_dir)
-    write_file(keyfile, contents)
+    write_file(keyfile, '\n'.join(triggers.values()) + '\n')
 
 
 def configure_nrpe(conf, hostname):  # noqa: C901
diff --git a/requirements.txt b/requirements.txt
 new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/requirements.txt
diff --git a/tests/unit/requirements.txt b/tests/unit/requirements.txt
 new file mode 100644
index 0000000..449073d
--- /dev/null
+++ b/tests/unit/requirements.txt
@@ -0,0 +1,7 @@
+# test requirements
+mock
+pytest
+pytest-cov
+# charm requirements
+Cheetah
+pyyaml
diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py
 new file mode 100644
index 0000000..41508f6
--- /dev/null
+++ b/tests/unit/test_charm.py
@@ -0,0 +1,97 @@
+import json
+import unittest
+
+from base64 import b64encode
+from copy import deepcopy
+from mock import patch
+
+import Config  # for mockery
+import hooks   # noqa: F401; for mockery
+
+from hooks import (
+    SSH_TRIGGER_TEMPLATE,
+    configure_triggers,
+    extract_ssh_public_key,
+    make_triggers,
+    merge_shared_key_triggers,
+)
+
+from utils import (
+    CallableDict,
+    read_config_yaml,
+)
+
+BARE_RELEASES_SSH_KEY = "AAAAB3NzaC1yc2EAAAABIwAAAQEA7TIXsws/s48o4N1MA+TpaudHS8XVF79JujYQGTumroZZqbsr04lbg1FaJ9HyS9P31e8lP2bmB5rexJaMyfAxDTV+DtrHXmeLi/XcGSOXoliOa/j3eQmdZyYme9z+Tfp8s3S6nxuDP3BRKkOuIzOPgPO5fr8AH7y83v3fyHi+H0sbm6agX2b+xV9oP8+DEgSxPNfZEVTgpdJ7/ULwAl68mWQW0w6VTVx0CMYF+cvcorcZMGmy+THzk8WH2XRalA5HqeiBtS7vJ+yFHJ5WbQOEUeR6gHRcrogR4Yd10eFWUNAFyvt53zF7W91PEgnJWjcCUN/uctybAFSgDK1UB6TPkQ=="  # noqa: E501
+RELEASES_SSH_KEY = 'from="91.189.90.151,91.189.89.127,10.22.96.61,10.22.96.161" ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEA7TIXsws/s48o4N1MA+TpaudHS8XVF79JujYQGTumroZZqbsr04lbg1FaJ9HyS9P31e8lP2bmB5rexJaMyfAxDTV+DtrHXmeLi/XcGSOXoliOa/j3eQmdZyYme9z+Tfp8s3S6nxuDP3BRKkOuIzOPgPO5fr8AH7y83v3fyHi+H0sbm6agX2b+xV9oP8+DEgSxPNfZEVTgpdJ7/ULwAl68mWQW0w6VTVx0CMYF+cvcorcZMGmy+THzk8WH2XRalA5HqeiBtS7vJ+yFHJ5WbQOEUeR6gHRcrogR4Yd10eFWUNAFyvt53zF7W91PEgnJWjcCUN/uctybAFSgDK1UB6TPkQ== cjwatson@little'  # noqa: E501
+
+
+MERGEABLE_TRIGGERS_CONFIG = {
+    'mirror_cdimage_trigger': b64encode(RELEASES_SSH_KEY),
+    'mirror_releases_trigger': b64encode(RELEASES_SSH_KEY),
+    'role_map': json.dumps({
+        'localhost': ['cdimage', 'releases'],
+    })
+}
+
+MERGEABLE_TRIGGERS = {
+    'cdimage': SSH_TRIGGER_TEMPLATE.format(
+        command='/srv/ubuntu-mirror/bin/mirror-1stage.sh cdimage &',
+        trigger=RELEASES_SSH_KEY,
+    ),
+    'releases': SSH_TRIGGER_TEMPLATE.format(
+        command='/srv/ubuntu-mirror/bin/mirror-1stage.sh releases &',
+        trigger=RELEASES_SSH_KEY,
+    ),
+}
+
+MERGED_TRIGGERS = {
+    'releases': SSH_TRIGGER_TEMPLATE.format(
+        command='( /srv/ubuntu-mirror/bin/mirror-1stage.sh releases ; /srv/ubuntu-mirror/bin/mirror-1stage.sh cdimage ) &',  # noqa: E501
+        trigger=RELEASES_SSH_KEY,
+    ),
+}
+
+
+MERGEABLE_TRIGGERS_SSH_AUTHORIZED_KEYS = 'no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty,command="( /srv/ubuntu-mirror/bin/mirror-1stage.sh releases ; /srv/ubuntu-mirror/bin/mirror-1stage.sh cdimage ) &",from="91.189.90.151,91.189.89.127,10.22.96.61,10.22.96.161" ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEA7TIXsws/s48o4N1MA+TpaudHS8XVF79JujYQGTumroZZqbsr04lbg1FaJ9HyS9P31e8lP2bmB5rexJaMyfAxDTV+DtrHXmeLi/XcGSOXoliOa/j3eQmdZyYme9z+Tfp8s3S6nxuDP3BRKkOuIzOPgPO5fr8AH7y83v3fyHi+H0sbm6agX2b+xV9oP8+DEgSxPNfZEVTgpdJ7/ULwAl68mWQW0w6VTVx0CMYF+cvcorcZMGmy+THzk8WH2XRalA5HqeiBtS7vJ+yFHJ5WbQOEUeR6gHRcrogR4Yd10eFWUNAFyvt53zF7W91PEgnJWjcCUN/uctybAFSgDK1UB6TPkQ== cjwatson@little\n'  # noqa: E501
+
+
+class TestUbuntuMirrorCharm(unittest.TestCase):
+    def setUp(self):
+        self.default_config = read_config_yaml()
+        self.maxDiff = None
+
+    def test_extract_ssh_public_key(self):
+        self.assertEqual(extract_ssh_public_key(RELEASES_SSH_KEY), BARE_RELEASES_SSH_KEY)
+
+    @patch('Config.config')
+    def test_make_triggers(self, _config):
+        config = CallableDict(self.default_config)
+        config.update(MERGEABLE_TRIGGERS_CONFIG)
+        _config.side_effect = config  # i.e. we want our CallableDict to supply the charm config
+        conf = Config.Config()
+        triggers = make_triggers(conf, 'localhost')
+        self.assertEqual(triggers, MERGEABLE_TRIGGERS)
+
+    @patch('Config.config')
+    def test_merge_shared_key_triggers(self, _config):
+        config = CallableDict(self.default_config)
+        config.update(MERGEABLE_TRIGGERS_CONFIG)
+        _config.side_effect = config  # i.e. we want our CallableDict to supply the charm config
+        conf = Config.Config()
+        triggers = deepcopy(MERGEABLE_TRIGGERS)
+        merge_shared_key_triggers(conf, triggers)
+        self.assertEqual(triggers, MERGED_TRIGGERS)
+
+    @patch('hooks.os.path.isdir')
+    @patch('hooks.get_ssh_keyfile')
+    @patch('hooks.write_file')
+    @patch('Config.config')
+    def test_configure_triggers(self, _config, _write_file, _get_ssh_keyfile, _isdir):
+        config = CallableDict(self.default_config)
+        config.update(MERGEABLE_TRIGGERS_CONFIG)
+        _config.side_effect = config  # i.e. we want our CallableDict to supply the charm config
+        conf = Config.Config()
+        _get_ssh_keyfile.return_value = '/etc/ssh/user-authorized-keys/{}'.format(config('mirror_user'))
+        _isdir.return_value = True
+        configure_triggers(conf, 'localhost')
+        _write_file.assert_called_with(_get_ssh_keyfile.return_value, MERGEABLE_TRIGGERS_SSH_AUTHORIZED_KEYS)
diff --git a/tests/unit/utils.py b/tests/unit/utils.py
 new file mode 100644
index 0000000..1e7cffd
--- /dev/null
+++ b/tests/unit/utils.py
@@ -0,0 +1,34 @@
+import yaml
+
+
+# The Config class fails if config items aren't present, so it's
+# simplest to just read config.yaml and mock hookenv.config().
+def read_config_yaml():
+    """Read config.yaml and return a dict keyed by setting, with their
+    defaults as values, stringified a la config-get."""
+
+    with open('config.yaml') as config_yaml:
+        config = yaml.safe_load(config_yaml)
+    return {k: str(v['default']) for k, v in config['options'].items() if 'default' in v}
+
+
+# Used to mock hookenv.config().
+class CallableDict(dict):
+    """Wrapper for dict type: d(x) <==> d[x].  Intended as a drop-in
+    replacement (or mock) for functions such as config() while
+    allowing responses to be easily configured.  Example use, which
+    assumes that something() is imported from somelib, and somelib did
+    "from charmhelpers.core.hookenv import config":
+
+        from somelib import something
+
+        class TestSomething(unittest.TestCase):
+            @patch('somelib.config')
+            def test_something(self, _config):
+                _config.side_effect = CallableDict({'key': 'value'})
+                self.assertTrue(something())
+
+    """
+
+    def __call__(self, *args, **kwargs):
+        return self.__getitem__(*args, **kwargs)
diff --git a/tox.ini b/tox.ini
index 7898f03..39a574c 100644
--- a/tox.ini
+++ b/tox.ini
@@ -10,13 +10,13 @@ setenv =
 
 [testenv:unit]
 commands =
-    pytest --ignore mod --ignore {toxinidir}/tests/functional \
-      {posargs:-v  --cov=src --cov-report=term-missing --cov-branch}
+    pytest --ignore {toxinidir}/tests/functional --cov-config={toxinidir}/.coveragerc \
+      {posargs:-v  --cov=hooks --cov-report=term-missing --cov-branch}
 deps = -r{toxinidir}/tests/unit/requirements.txt
        -r{toxinidir}/requirements.txt
 setenv =
-  PYTHONPATH={toxinidir}/src:{toxinidir}/build/lib:{toxinidir}/build/venv
-  TZ=UTC
+  PYTHONPATH = {toxinidir}/hooks
+  TZ = UTC
 
 [testenv:functional]
 passenv =