cloud-init-dev team mailing list archive

[Scott Moser <ssmoser2+ubuntu@xxxxxxxxx>]

Scott Moser has proposed merging ~smoser/cloud-init:fix/fix-dhclient-hook-down into cloud-init:master.

Commit message:
dhclient-hook: cleanups, tests and fix a bug on 'down' event.

I noticed a bug in dhclient_hook on the 'down' event, using 'is'
operator rather than '==' (if self.net_action is 'down').

This refactors/simplifies the code a bit for easier testing and adds
tests.  The reason for the rename of 'action' to 'event' is to just
be internally consistent.  The word and Namespace 'action' is used
by cloud-init main, so it was not really usable here.

Requested reviews:
  cloud-init commiters (cloud-init-dev)

For more details, see:
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/356428

see commit message
-- 
Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:fix/fix-dhclient-hook-down into cloud-init:master.

diff --git a/cloudinit/cmd/main.py b/cloudinit/cmd/main.py
index 5a43702..933c019 100644
--- a/cloudinit/cmd/main.py
+++ b/cloudinit/cmd/main.py
@@ -41,7 +41,7 @@ from cloudinit.settings import (PER_INSTANCE, PER_ALWAYS, PER_ONCE,
 from cloudinit import atomic_helper
 
 from cloudinit.config import cc_set_hostname
-from cloudinit.dhclient_hook import LogDhclient
+from cloudinit import dhclient_hook
 
 
 # Welcome message template
@@ -586,12 +586,6 @@ def main_single(name, args):
         return 0
 
 
-def dhclient_hook(name, args):
-    record = LogDhclient(args)
-    record.check_hooks_dir()
-    record.record()
-
-
 def status_wrapper(name, args, data_d=None, link_d=None):
     if data_d is None:
         data_d = os.path.normpath("/var/lib/cloud/data")
@@ -795,15 +789,9 @@ def main(sysv_args=None):
         'query',
         help='Query standardized instance metadata from the command line.')
 
-    parser_dhclient = subparsers.add_parser('dhclient-hook',
-                                            help=('run the dhclient hook'
-                                                  'to record network info'))
-    parser_dhclient.add_argument("net_action",
-                                 help=('action taken on the interface'))
-    parser_dhclient.add_argument("net_interface",
-                                 help=('the network interface being acted'
-                                       ' upon'))
-    parser_dhclient.set_defaults(action=('dhclient_hook', dhclient_hook))
+    parser_dhclient = subparsers.add_parser(
+        dhclient_hook.NAME, help=dhclient_hook.__doc__)
+    dhclient_hook.get_parser(parser_dhclient)
 
     parser_features = subparsers.add_parser('features',
                                             help=('list defined features'))
diff --git a/cloudinit/dhclient_hook.py b/cloudinit/dhclient_hook.py
index 7f02d7f..d258931 100644
--- a/cloudinit/dhclient_hook.py
+++ b/cloudinit/dhclient_hook.py
@@ -1,5 +1,8 @@
 # This file is part of cloud-init. See LICENSE file for license information.
 
+"""Run the dhclient hook to record network info."""
+
+import argparse
 import os
 
 from cloudinit import atomic_helper
@@ -8,44 +11,75 @@ from cloudinit import stages
 
 LOG = logging.getLogger(__name__)
 
+NAME = "dhclient-hook"
+UP = "up"
+DOWN = "down"
+ACTIONS = (UP, DOWN)
+
+
+def _get_hooks_dir():
+    i = stages.Init()
+    return os.path.join(i.paths.get_runpath(), 'dhclient.hooks')
+
+
+def _filter_env_vals(info):
+    """Given info (os.environ), return a dictionary with
+    lower case keys for each entry starting with DHCP4_ or new_."""
+    new_info = {}
+    for k, v in info.items():
+        if k.startswith("DHCP4_") or k.startswith("new_"):
+            key = (k.replace('DHCP4_', '').replace('new_', '')).lower()
+            new_info[key] = v
+    return new_info
+
+
+def run_hook(interface, event, data_d=None, env=None):
+    if event not in ACTIONS:
+        raise ValueError("Unexpected event '%s'. Expected one of: %s" %
+                         (event, ACTIONS))
+    if data_d is None:
+        data_d = _get_hooks_dir()
+    if env is None:
+        env = os.environ
+    hook_file = os.path.join(data_d, interface + ".json")
+
+    if event == UP:
+        if not os.path.exists(data_d):
+            os.makedirs(data_d)
+        atomic_helper.write_json(hook_file, _filter_env_vals(env))
+        LOG.debug("Wrote dhclient options in %s", hook_file)
+    elif event == DOWN:
+        if os.path.exists(hook_file):
+            os.remove(hook_file)
+            LOG.debug("Removed dhclient options file %s", hook_file)
+
+
+def get_parser(parser=None):
+    if parser is None:
+        parser = argparse.ArgumentParser(prog=NAME, description=__doc__)
+    parser.add_argument(
+        "event", help='event taken on the interface', choices=ACTIONS)
+    parser.add_argument(
+        "interface", help='the network interface being acted upon')
+    # cloud-init main uses 'action'
+    parser.set_defaults(action=(NAME, handle_args))
+    return parser
+
+
+def handle_args(name, args, data_d=None):
+    """Handle the Namespace args.
+    Takes 'name' as passed by cloud-init main. not used here."""
+    return run_hook(interface=args.interface, event=args.event, data_d=data_d)
+
+
+if __name__ == '__main__':
+    import sys
+    parser = get_parser()
+    args = parser.parse_args(args=sys.argv[1:])
+    return_value = handle_args(
+        NAME, args, data_d=os.environ.get('_CI_DHCP_HOOK_DATA_D'))
+    if return_value:
+        sys.exit(return_value)
 
-class LogDhclient(object):
-
-    def __init__(self, cli_args):
-        self.hooks_dir = self._get_hooks_dir()
-        self.net_interface = cli_args.net_interface
-        self.net_action = cli_args.net_action
-        self.hook_file = os.path.join(self.hooks_dir,
-                                      self.net_interface + ".json")
-
-    @staticmethod
-    def _get_hooks_dir():
-        i = stages.Init()
-        return os.path.join(i.paths.get_runpath(), 'dhclient.hooks')
-
-    def check_hooks_dir(self):
-        if not os.path.exists(self.hooks_dir):
-            os.makedirs(self.hooks_dir)
-        else:
-            # If the action is down and the json file exists, we need to
-            # delete the file
-            if self.net_action is 'down' and os.path.exists(self.hook_file):
-                os.remove(self.hook_file)
-
-    @staticmethod
-    def get_vals(info):
-        new_info = {}
-        for k, v in info.items():
-            if k.startswith("DHCP4_") or k.startswith("new_"):
-                key = (k.replace('DHCP4_', '').replace('new_', '')).lower()
-                new_info[key] = v
-        return new_info
-
-    def record(self):
-        envs = os.environ
-        if self.hook_file is None:
-            return
-        atomic_helper.write_json(self.hook_file, self.get_vals(envs))
-        LOG.debug("Wrote dhclient options in %s", self.hook_file)
 
 # vi: ts=4 expandtab
diff --git a/cloudinit/tests/test_dhclient_hook.py b/cloudinit/tests/test_dhclient_hook.py
new file mode 100644
index 0000000..7aab8dd
--- /dev/null
+++ b/cloudinit/tests/test_dhclient_hook.py
@@ -0,0 +1,105 @@
+# This file is part of cloud-init. See LICENSE file for license information.
+
+"""Tests for cloudinit.dhclient_hook."""
+
+from cloudinit import dhclient_hook as dhc
+from cloudinit.tests.helpers import CiTestCase, dir2dict, populate_dir
+
+import argparse
+import json
+import mock
+import os
+
+
+class TestDhclientHook(CiTestCase):
+
+    ex_env = {
+        'interface': 'eth0',
+        'new_dhcp_lease_time': '3600',
+        'new_host_name': 'x1',
+        'new_ip_address': '10.145.210.163',
+        'new_subnet_mask': '255.255.255.0',
+        'old_host_name': 'x1',
+        'PATH': '/usr/sbin:/usr/bin:/sbin:/bin',
+        'pid': '614',
+        'reason': 'BOUND',
+    }
+
+    # some older versions of dhclient put the same content,
+    # but in upper case with DHCP4_ instead of new_
+    ex_env_dhcp4 = {
+        'REASON': 'BOUND',
+        'DHCP4_dhcp_lease_time': '3600',
+        'DHCP4_host_name': 'x1',
+        'DHCP4_ip_address': '10.145.210.163',
+        'DHCP4_subnet_mask': '255.255.255.0',
+        'INTERFACE': 'eth0',
+        'PATH': '/usr/sbin:/usr/bin:/sbin:/bin',
+        'pid': '614',
+    }
+
+    expected = {
+        'dhcp_lease_time': '3600',
+        'host_name': 'x1',
+        'ip_address': '10.145.210.163',
+        'subnet_mask': '255.255.255.0'}
+
+    def setUp(self):
+        super(TestDhclientHook, self).setUp()
+        self.tmp = self.tmp_dir()
+
+    def test_handle_args(self):
+        """quick test of call to handle_args."""
+        nic = 'eth0'
+        args = argparse.Namespace(event=dhc.UP, interface=nic)
+        with mock.patch.dict("os.environ", clear=True, values=self.ex_env):
+            dhc.handle_args(dhc.NAME, args, data_d=self.tmp)
+        found = dir2dict(self.tmp + os.path.sep)
+        self.assertEqual([nic + ".json"], list(found.keys()))
+        self.assertEqual(self.expected, json.loads(found[nic + ".json"]))
+
+    def test_run_hook_up_creates_dir(self):
+        """If dir does not exist, run_hook should create it."""
+        subd = self.tmp_path("subdir", self.tmp)
+        nic = 'eth1'
+        dhc.run_hook(nic, 'up', data_d=subd, env=self.ex_env)
+        self.assertEqual(
+            set([nic + ".json"]), set(dir2dict(subd + os.path.sep)))
+
+    def test_run_hook_up(self):
+        """Test expected use of run_hook_up."""
+        nic = 'eth0'
+        dhc.run_hook(nic, 'up', data_d=self.tmp, env=self.ex_env)
+        found = dir2dict(self.tmp + os.path.sep)
+        self.assertEqual([nic + ".json"], list(found.keys()))
+        self.assertEqual(self.expected, json.loads(found[nic + ".json"]))
+
+    def test_run_hook_up_dhcp4_prefix(self):
+        """Test run_hook filters correctly with older DHCP4_ data."""
+        nic = 'eth0'
+        dhc.run_hook(nic, 'up', data_d=self.tmp, env=self.ex_env_dhcp4)
+        found = dir2dict(self.tmp + os.path.sep)
+        self.assertEqual([nic + ".json"], list(found.keys()))
+        self.assertEqual(self.expected, json.loads(found[nic + ".json"]))
+
+    def test_run_hook_down_deletes(self):
+        """down should delete the created json file."""
+        nic = 'eth1'
+        populate_dir(
+            self.tmp, {nic + ".json": "{'abcd'}", 'myfile.txt': 'text'})
+        dhc.run_hook(nic, 'down', data_d=self.tmp, env={'old_host_name': 'x1'})
+        self.assertEqual(
+            set(['myfile.txt']),
+            set(dir2dict(self.tmp + os.path.sep)))
+
+    def test_get_parser(self):
+        """Smoke test creation of get_parser."""
+        # cloud-init main uses 'action'.
+        event, interface = (dhc.UP, 'mynic0')
+        self.assertEqual(
+            argparse.Namespace(event=event, interface=interface,
+                               action=(dhc.NAME, dhc.handle_args)),
+            dhc.get_parser().parse_args([event, interface]))
+
+
+# vi: ts=4 expandtab
diff --git a/tests/unittests/test_cli.py b/tests/unittests/test_cli.py
index 199d69b..d283f13 100644
--- a/tests/unittests/test_cli.py
+++ b/tests/unittests/test_cli.py
@@ -246,18 +246,18 @@ class TestCLI(test_helpers.FilesystemMockingTestCase):
         self.assertEqual('cc_ntp', parseargs.name)
         self.assertFalse(parseargs.report)
 
-    @mock.patch('cloudinit.cmd.main.dhclient_hook')
-    def test_dhclient_hook_subcommand(self, m_dhclient_hook):
+    @mock.patch('cloudinit.cmd.main.dhclient_hook.handle_args')
+    def test_dhclient_hook_subcommand(self, m_handle_args):
         """The subcommand 'dhclient-hook' calls dhclient_hook with args."""
-        self._call_main(['cloud-init', 'dhclient-hook', 'net_action', 'eth0'])
-        (name, parseargs) = m_dhclient_hook.call_args_list[0][0]
-        self.assertEqual('dhclient_hook', name)
+        self._call_main(['cloud-init', 'dhclient-hook', 'up', 'eth0'])
+        (name, parseargs) = m_handle_args.call_args_list[0][0]
+        self.assertEqual('dhclient-hook', name)
         self.assertEqual('dhclient-hook', parseargs.subcommand)
-        self.assertEqual('dhclient_hook', parseargs.action[0])
+        self.assertEqual('dhclient-hook', parseargs.action[0])
         self.assertFalse(parseargs.debug)
         self.assertFalse(parseargs.force)
-        self.assertEqual('net_action', parseargs.net_action)
-        self.assertEqual('eth0', parseargs.net_interface)
+        self.assertEqual('up', parseargs.event)
+        self.assertEqual('eth0', parseargs.interface)
 
     @mock.patch('cloudinit.cmd.main.main_features')
     def test_features_hook_subcommand(self, m_features):