Merge ~kotodama/ubuntu-mirror-charm:role-map-hostname-all into ubuntu-mirror-charm:master

[Loïc Gomez]

Add <<all>> hostname placeholder in role-maps

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

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

[Loïc Gomez (kotodama)]

$ make test
[...]
_________________ summary _________________
  lint: commands succeeded
  congratulations :)
[...]
tests/unit/test_charm.py::TestUbuntuMirrorCharm::test_get_roles PASSED [ 47%]
tests/unit/test_charm.py::TestUbuntuMirrorCharm::test_has_roles PASSED [ 52%]
[...]
======= 21 passed in 3.48 seconds =========
_________________ summary _________________
  unit: commands succeeded
  congratulations :)

[Tom Haddon (mthaddon)]

Some comments inline

[Loïc Gomez (kotodama)]

Thank you, updated the code with suggested changes.
Also realized we do not need hostname parameter in get_role_config anymore, removed it.

[Tom Haddon (mthaddon)]

LGTM

review: Approve

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

Change successfully merged at revision 5c839e9c5f767e26a35a3913c519047d8dee9517

diff --git a/hooks/hooks.py b/hooks/hooks.py
index 24a6cc8..1b91c90 100755
--- a/hooks/hooks.py
+++ b/hooks/hooks.py
@@ -319,6 +319,29 @@ def get_ssh_keyfile(username):
     return newkeyfile
 
 
+def has_roles(conf, hostname):
+    """Returns true if there are specific roles for `hostname` hostname
+    or global roles defined for <<all>> hostnames"""
+    roles = conf.roles()
+    if hostname in roles or '<<all>>' in roles:
+        return True
+    return False
+
+
+def get_roles(conf, hostname):
+    """Return the list of specific roles for `hostname` hostname.
+    If there are none, but global roles are defined for <<all>> hostname,
+    return these instead of an empty list."""
+    if not has_roles(conf, hostname):
+        return []
+
+    roles = conf.roles()
+    if hostname in roles:
+        return roles[hostname]
+    else:
+        return roles['<<all>>']
+
+
 #
 # Set up cronned rsyncing to pull archive contents from masters
 # Creation of any necessary destination directories is the responsibility
@@ -338,16 +361,14 @@ def configure_rsync_client(conf, hostname):  # noqa: C901
         os.chmod(mirror_logdir, 0o755)
         os.chown(mirror_logdir, mirror_userinfo.pw_uid, mirror_userinfo.pw_gid)
 
-    roles = conf.roles()
-    if hostname not in roles:
+    if not has_roles(conf, hostname):
         log("CHARM: hostname {} not found in role_map".format(hostname))
-        roles[hostname] = []
 
     enabled_configs = [f for f in os.listdir(mirror_confdir) if os.path.isfile(os.path.join(mirror_confdir, f))]
     wanted_configs = []
 
     cron = juju_header()
-    for role in roles[hostname]:
+    for role in get_roles(conf, hostname):
         try:
             mirror = conf.mirror_map(role)
             if mirror["unsafe"]:
@@ -401,13 +422,12 @@ def configure_rsync_client(conf, hostname):  # noqa: C901
 def configure_rsync_server(conf, hostname):
     log("CHARM: Configuring rsync server")
 
-    roles = conf.roles()
-    if hostname not in roles:
+    if not has_roles(conf, hostname):
         log("CHARM: hostname {} not found in role_map - not configuring rsync".format(hostname))
         return
 
     targets = {}
-    for role in roles[hostname]:
+    for role in get_roles(conf, hostname):
         try:
             mirror = conf.mirror_map(role)
             if mirror["unsafe"]:
@@ -474,9 +494,11 @@ def configure_rsync_server(conf, hostname):
     log("CHARM: Finished configuring rsync server")
 
 
-def get_role_config(roles, hostname, role):
-    if isinstance(roles[hostname], types.DictType) and roles[hostname][role]:
-        return roles[hostname][role]
+def get_role_config(host_roles, role):
+    """Get the role configuration for `role` as defined in host_roles
+    or return an empty dict if not defined"""
+    if isinstance(host_roles, types.DictType) and host_roles[role]:
+        return host_roles[role]
     return {}
 
 
@@ -496,23 +518,23 @@ def configure_apache(conf, hostname):  # noqa: C901
     mkdir(apache_logdir)
     os.chmod(apache_logdir, 0o755)
 
-    roles = conf.roles()
-    if hostname not in roles:
+    if not has_roles(conf, hostname):
         log("CHARM: hostname {} not found in role_map - not configuring apache".format(hostname))
         return
 
     wanted_vhosts = []
-
     all_addresses = set()
     all_ports = set()
+    host_roles = get_roles(conf, hostname)
+
     # Create vhost configs for all active roles
-    for role in roles[hostname]:
+    for role in host_roles:
         try:
             mirror = conf.mirror_map(role)
         except ValueError:
             log("CHARM: Missing details - skipping {}".format(role))
         else:
-            role_config = get_role_config(roles, hostname, role)
+            role_config = get_role_config(host_roles, role)
             if mirror['unsafe'] and 'addresses' not in role_config:
                 log("CHARM: {} has no addresses defined - not configuring".format(role))
                 continue
@@ -776,11 +798,12 @@ def extract_ssh_public_key(key):
 
 def make_triggers(conf, hostname):
     """Return this unit's triggers in a dict, keyed by role."""
-    roles = conf.roles()
 
-    triggers = {}
+    if not has_roles(conf, hostname):
+        return {}
 
-    for role in roles[hostname]:
+    triggers = {}
+    for role in get_roles(conf, hostname):
         try:
             mirror = conf.mirror_map(role)
             if mirror["unsafe"]:
@@ -802,8 +825,7 @@ def make_triggers(conf, hostname):
 # Set up any external ssh sync triggers
 #
 def configure_triggers(conf, hostname):
-    roles = conf.roles()
-    if hostname not in roles:
+    if not has_roles(conf, hostname):
         return
 
     triggers = make_triggers(conf, hostname)
@@ -867,10 +889,10 @@ def generate_disk_check(conf, path):
     log("CHARM: Adding disk check for {}".format(path))
 
 
-def generate_global_rsync_check(conf, hostname, roles):
+def generate_global_rsync_check(conf, hostname, host_roles):
     # One extra for the rsync daemon itself, another two
     # per role for pulling from syncproxy, one more for luck.
-    rsync_crit = conf.rsync_max_connections() + 1 + len(roles[hostname]) * 2 + 1
+    rsync_crit = conf.rsync_max_connections() + 1 + len(host_roles) * 2 + 1
     rsync_warn = conf.rsync_max_connections() + 1  # all client slots used
 
     file_from_template(
@@ -917,15 +939,16 @@ def configure_nrpe(conf, hostname):  # noqa: C901
         log("CHARM: Essential nagios dirs missing - skipping Nagios checks")
         return
 
-    wanted_checks = []
     # Add Nagios role checks
-    roles = conf.roles()
+    wanted_checks = []
     paths = []
-    if hostname in roles:
+
+    if has_roles(conf, hostname):
+        host_roles = get_roles(conf, hostname)
         # We generate a single check here, and delete the legacy per-role checks later.
-        generate_global_rsync_check(conf, hostname, roles)
+        generate_global_rsync_check(conf, hostname, host_roles)
         tmpl_data = {}
-        for role in roles[hostname]:
+        for role in host_roles:
             try:
                 mirror = conf.mirror_map(role)
             except ValueError:
@@ -934,7 +957,7 @@ def configure_nrpe(conf, hostname):  # noqa: C901
 
             paths.append(mirror['path'])
             tmpl_data["name"] = mirror["name"]
-            role_config = get_role_config(roles, hostname, role)
+            role_config = get_role_config(host_roles, role)
             addresses = role_config.get('addresses', [])
             if not addresses:
                 ipaddr = get_default_address(socket.AF_INET)
diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py
index 20582cb..ba0e688 100644
--- a/tests/unit/test_charm.py
+++ b/tests/unit/test_charm.py
@@ -26,6 +26,8 @@ from hooks import (
     merge_shared_key_triggers,
     mountpoint_of,
     remove_bind_mount,
+    get_roles,
+    has_roles
 )
 
 from utils import (
@@ -80,6 +82,19 @@ MERGED_TRIGGERS = {
 
 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
 
+MERGEABLE_ROLES_CONFIG = {
+    'role_map': json.dumps({
+        'localhost': ['cdimage', 'releases'],
+    })
+}
+
+MERGEABLE_ROLES_ALL_CONFIG = {
+    'role_map': json.dumps({
+        'localhost': ['cdimage', 'releases'],
+        '<<all>>': ['cdimage', 'releases'],
+    })
+}
+
 XINETD_CONFIG = {
     'rsync_use_xinetd': True,
     'role_map': json.dumps({
@@ -150,6 +165,34 @@ class TestUbuntuMirrorCharm(unittest.TestCase):
         configure_triggers(conf, 'localhost')
         _write_file.assert_called_with(_get_ssh_keyfile.return_value, MERGEABLE_TRIGGERS_SSH_AUTHORIZED_KEYS)
 
+    @patch('Config.config')
+    def test_has_roles(self, _config):
+        config = CallableDict(self.default_config)
+        config.update(MERGEABLE_ROLES_CONFIG)
+        _config.side_effect = config  # i.e. we want our CallableDict to supply the charm config
+        conf = Config.Config()
+        self.assertTrue(has_roles(conf, 'localhost'))
+        self.assertFalse(has_roles(conf, 'invalidhostname'))
+        config = CallableDict(self.default_config)
+        config.update(MERGEABLE_ROLES_ALL_CONFIG)
+        _config.side_effect = config  # i.e. we want our CallableDict to supply the charm config
+        conf = Config.Config()
+        self.assertTrue(has_roles(conf, 'invalidhostname'))
+
+    @patch('Config.config')
+    def test_get_roles(self, _config):
+        config = CallableDict(self.default_config)
+        config.update(MERGEABLE_ROLES_CONFIG)
+        _config.side_effect = config  # i.e. we want our CallableDict to supply the charm config
+        conf = Config.Config()
+        self.assertEqual(get_roles(conf, 'localhost'), ['cdimage', 'releases'])
+        self.assertEqual(get_roles(conf, 'invalidhostname'), [])
+        config = CallableDict(self.default_config)
+        config.update(MERGEABLE_ROLES_ALL_CONFIG)
+        _config.side_effect = config  # i.e. we want our CallableDict to supply the charm config
+        conf = Config.Config()
+        self.assertEqual(get_roles(conf, 'invalidhostname'), ['cdimage', 'releases'])
+
     def test_mountpoint_of_root(self):
         """If the mountpoint of / isn't /, we're running in a weird environment."""
         self.assertEqual(mountpoint_of('/'), '/')