Diff comments:
> diff --git a/cloudinit/net/dhcp.py b/cloudinit/net/dhcp.py > new file mode 100644 > index 0000000..4d59bd0 > --- /dev/null > +++ b/cloudinit/net/dhcp.py > @@ -0,0 +1,118 @@ > +# Copyright (C) 2017 Canonical Ltd. > +# > +# Author: Chad Smith <chad.sm...@canonical.com> > +# > +# This file is part of cloud-init. See LICENSE file for license information. > + > +import logging > +import os > +import re > + > +from cloudinit.net import find_fallback_nic, get_devicelist > +from cloudinit import util > + > +LOG = logging.getLogger(__name__) > + > + > +class InvalidDHCPLeaseFileError(Exception): > + """Raised when parsing an empty or invalid dhcp.leases file. > + > + Current uses are DataSourceAzure and DataSourceEc2 during ephemeral > + boot to scrape metadata. > + """ > + pass > + > + > +def maybe_dhcp_clean_discovery(nic=None): > + """Create a temporary working directory and find the nic if needed. Good point, fixed this and dhcp_clean_discovery > + > + If the device is already connected, do nothing. > + > + @param nic: Name of the network interface we want to run dhclient on. > + @return: A dict of dhcp options from the dhclient discovery, otherwise an > + empty dict. > + """ > + if nic is None: > + nic = find_fallback_nic() > + if nic is None: > + LOG.debug( > + 'Skip dhcp_clean_discovery: Unable to find network > interface.') > + return {} > + elif nic not in get_devicelist(): > + LOG.debug( > + 'Skip dhcp_clean_discovery: Invalid interface name %s.', nic) done. > + return {} > + with util.tempdir(prefix='cloud-init-dhcp-') as tmpdir: > + return dhcp_clean_discovery(nic, tmpdir) > + > + > +def parse_dhcp_lease_file(lease_file): > + """Parse the given dhcp lease file for the most recent lease. > + > + Return a dict of dhcp options as key value pairs for the most recent > lease > + block. > + > + @raises: InvalidDHCPLeaseFileError on empty of unparseable leasefile > + content. > + """ > + lease_regex = re.compile(r"lease {(?P<lease>[^}]*)}\n") > + dhcp_leases = [] > + with open(lease_file) as stream: > + lease_content = stream.read() done > + if len(lease_content) == 0: > + raise InvalidDHCPLeaseFileError( > + 'Cannot parse empty dhcp lease file {0}'.format(lease_file)) > + for lease in lease_regex.findall(lease_content): > + lease_options = [] > + for line in lease.split(';'): > + # Strip newlines, double-quotes and option prefix > + line = line.strip().replace('"', '').replace('option ', '') > + if not line: > + continue > + lease_options.append(line.split(' ', 1)) > + dhcp_leases.append(dict(lease_options)) > + if not dhcp_leases: > + raise InvalidDHCPLeaseFileError( > + 'Cannot parse dhcp lease file {0}. No leases found'.format( > + lease_file)) > + return dhcp_leases > + > + > +def dhcp_clean_discovery(interface, cleandir): > + """Start up dhclient on the provided inteface without side-effects. done. > + > + @param interface: Name of the network inteface on which to dhclient. > + @param cleandir: The directory from which to run dhclient as well as > store > + dhcp leases. > + > + @return: A dict of dhcp options parsed from the dhcp.leases file or empty > + dict. > + """ > + dhclient_script = util.which('dhclient') > + if not dhclient_script: > + LOG.debug('Skip dhclient configuration: No dhclient found.') > + return {} > + LOG.debug('Performing a clean dhcp discovery on %s', interface) > + > + # XXX We copy dhclient out of /sbin/dhclient to avoid dealing with strict > + # app armor profiles which disallow running dhclient -sf > <our-script-file>. enabled SELinux doesn't have rules currrently which prevent dhclient from running using a separate dhclient-script, so SELinux passes this case. > + # We want to avoid running /sbin/dhclient-script because of side-effects > in > + # /etc/resolv.conf any any other vendor specific scripts in > + # /etc/dhcp/dhclient*hooks.d. > + dhclient_cmd = os.path.join(cleandir, 'dhclient') > + util.copy(dhclient_script, dhclient_cmd) > + pid_file = os.path.join(cleandir, 'dhclient.pid') > + lease_file = os.path.join(cleandir, 'dhcp.leases') > + > + # dhclient needs the interface up to send initial discovery packets. done > + # Generally dhclient relies on dhclient-script PREINIT action to bring > the > + # link up before attempting discovery. Since we are using -sf /bin/true, > + # we need to do that "link up" ourselves first. > + util.subp(['ip', 'link', 'set', 'dev', interface, 'up'], capture=True) > + dhclient_cmd = [dhclient_cmd, '-1', '-v', '-lf', lease_file, '-pf', > + pid_file, interface, '-sf', '/bin/true'] > + util.subp(dhclient_cmd, capture=True) > + return parse_dhcp_lease_file(lease_file) > + > + > +# vi: ts=4 expandtab > diff --git a/cloudinit/net/tests/test_dhcp.py > b/cloudinit/net/tests/test_dhcp.py > new file mode 100644 > index 0000000..eee2dd9 > --- /dev/null > +++ b/cloudinit/net/tests/test_dhcp.py > @@ -0,0 +1,142 @@ > +# This file is part of cloud-init. See LICENSE file for license information. > + > +import mock > +import os > +from textwrap import dedent > + > +from cloudinit.net.dhcp import ( > + InvalidDHCPLeaseFileError, maybe_dhcp_clean_discovery, > + parse_dhcp_lease_file, dhcp_clean_discovery) > +from cloudinit.util import ensure_file, write_file > +from tests.unittests.helpers import CiTestCase > + > + > +class TestParseDHCPLeasesFile(CiTestCase): > + > + def test_parse_empty_lease_file_errors(self): > + """parse_dhcp_lease_file erros when file content is empty.""" done. > + empty_file = self.tmp_path('leases') > + ensure_file(empty_file) > + with self.assertRaises(InvalidDHCPLeaseFileError) as context_manager: > + parse_dhcp_lease_file(empty_file) > + error = context_manager.exception > + self.assertIn('Cannot parse empty dhcp lease file', str(error)) > + > + def test_parse_malformed_lease_file_content_errors(self): > + """parse_dhcp_lease_file erros when file content isn't dhcp > leases.""" > + non_lease_file = self.tmp_path('leases') > + write_file(non_lease_file, 'hi mom.') > + with self.assertRaises(InvalidDHCPLeaseFileError) as context_manager: > + parse_dhcp_lease_file(non_lease_file) > + error = context_manager.exception > + self.assertIn('Cannot parse dhcp lease file', str(error)) > + > + def test_parse_multiple_leases(self): > + """parse_dhcp_lease_file returns a list of all leases within.""" > + lease_file = self.tmp_path('leases') > + content = dedent(""" > + lease { > + interface "wlp3s0"; > + fixed-address 192.168.2.74; > + option subnet-mask 255.255.255.0; > + option routers 192.168.2.1; > + renew 4 2017/07/27 18:02:30; > + expire 5 2017/07/28 07:08:15; > + } > + lease { > + interface "wlp3s0"; > + fixed-address 192.168.2.74; > + option subnet-mask 255.255.255.0; > + option routers 192.168.2.1; > + } > + """) > + expected = [ > + {'interface': 'wlp3s0', 'fixed-address': '192.168.2.74', > + 'subnet-mask': '255.255.255.0', 'routers': '192.168.2.1', > + 'renew': '4 2017/07/27 18:02:30', > + 'expire': '5 2017/07/28 07:08:15'}, > + {'interface': 'wlp3s0', 'fixed-address': '192.168.2.74', > + 'subnet-mask': '255.255.255.0', 'routers': '192.168.2.1'}] > + write_file(lease_file, content) > + self.assertItemsEqual(expected, parse_dhcp_lease_file(lease_file)) > + > + > +class TestDHCPDiscoveryClean(CiTestCase): > + with_logs = True > + > + @mock.patch('cloudinit.net.dhcp.find_fallback_nic') > + def test_no_fallback_nic_found(self, m_fallback_nic): > + """Log and do nothing when nic is absent and no fallback is found.""" > + m_fallback_nic.return_value = None # No fallback nic found > + self.assertEqual({}, maybe_dhcp_clean_discovery()) > + self.assertIn( > + 'Skip dhcp_clean_discovery: Unable to find network interface', > + self.logs.getvalue()) > + > + def test_provided_nic_does_not_exist(self): > + """When the provided nic doesn't exist, log a message and no-op.""" > + self.assertEqual({}, maybe_dhcp_clean_discovery('idontexist')) > + self.assertIn( > + 'Skip dhcp_clean_discovery: Invalid interface name idontexist', > + self.logs.getvalue()) > + > + @mock.patch('cloudinit.net.dhcp.util.which') > + def test_absent_dhclient_command(self, m_which): > + """When dhclient doesn't exist in the OS, log the issue and no-op.""" > + m_which.return_value = None # dhclient isn't found > + tmpdir = self.tmp_dir() > + self.assertEqual({}, dhcp_clean_discovery('idontexist', tmpdir)) > + self.assertIn( > + 'Skip dhclient configuration: No dhclient found.', > + self.logs.getvalue()) > + > + @mock.patch('cloudinit.net.dhcp.dhcp_clean_discovery') > + @mock.patch('cloudinit.net.dhcp.find_fallback_nic') > + def test_dhclient_run_with_tmpdir(self, m_fallback, m_dhcp_discovery): > + """maybe_dhcp_clean_discovery passes tmpdir to > dhcp_clean_discovery.""" > + m_fallback.return_value = 'eth9' > + m_dhcp_discovery.return_value = {'address': '192.168.2.2'} > + self.assertEqual( > + {'address': '192.168.2.2'}, maybe_dhcp_clean_discovery()) > + m_dhcp_discovery.assert_called_once() > + call = m_dhcp_discovery.call_args_list[0] > + self.assertEqual('eth9', call[0][0]) > + self.assertIn('/tmp/cloud-init-dhcp-', call[0][1]) > + > + @mock.patch('cloudinit.net.dhcp.util.subp') > + @mock.patch('cloudinit.net.dhcp.util.which') > + def test_dhcp_clean_discovery_run_in_sandbox(self, m_which, m_subp): > + """dhcp_clean_discovery brings up the interface and runs dhclient. > + > + It also returns the parsed dhcp.leases file generated in the sandbox. > + """ > + tmpdir = self.tmp_dir() > + dhclient_script = os.path.join(tmpdir, 'dhclient.orig') > + script_content = '#!/bin/bash\necho fake-dhclient' > + write_file(dhclient_script, script_content, mode=0o755) > + lease_content = dedent(""" > + lease { > + interface "eth9"; > + fixed-address 192.168.2.74; > + option subnet-mask 255.255.255.0; > + option routers 192.168.2.1; > + } > + """) > + lease_file = os.path.join(tmpdir, 'dhcp.leases') > + write_file(lease_file, lease_content) > + m_which.return_value = dhclient_script # dhclient command is present > + self.assertItemsEqual( > + [{'interface': 'eth9', 'fixed-address': '192.168.2.74', > + 'subnet-mask': '255.255.255.0', 'routers': '192.168.2.1'}], > + dhcp_clean_discovery('eth9', tmpdir)) > + # dhclient script got copied > + with open(os.path.join(tmpdir, 'dhclient')) as stream: > + self.assertEqual(script_content, stream.read()) > + # Interface was brought up before dhclient called from sandbox > + m_subp.assert_has_calls([ > + mock.call( > + ['ip', 'link', 'set', 'dev', 'eth9', 'up'], capture=True), > + mock.call( > + [os.path.join(tmpdir, 'dhclient'), '-1', '-v', '-lf', > + lease_file, '-pf', os.path.join(tmpdir, 'dhclient.pid'), > + 'eth9', '-sf', '/bin/true'], capture=True)]) > diff --git a/cloudinit/sources/DataSourceEc2.py > b/cloudinit/sources/DataSourceEc2.py > index 4ec9592..ae0fe26 100644 > --- a/cloudinit/sources/DataSourceEc2.py > +++ b/cloudinit/sources/DataSourceEc2.py > @@ -73,21 +77,25 @@ class DataSourceEc2(sources.DataSource): > elif self.cloud_platform == Platforms.NO_EC2_METADATA: > return False > > - try: > - if not self.wait_for_metadata_service(): > + if self.get_network_metadata: # Setup networking in init-local > stage. > + if util.is_FreeBSD(): > + LOG.debug("FreeBSD doesn't support running dhclient with > -sf") No this is just that freebsd amis' dhclient doesn't have a -sf parameter as we have on debian/ubuntu and centos. If we wanted to do this on freebsd, it looks like we could create our own dhclient.conf file which specified "script sh -c true"; > return False > - start_time = time.time() > - self.userdata_raw = \ > - ec2.get_instance_userdata(self.api_ver, > self.metadata_address) > - self.metadata = ec2.get_instance_metadata(self.api_ver, > - self.metadata_address) > - LOG.debug("Crawl of metadata service took %.3f seconds", > - time.time() - start_time) > - return True > - except Exception: > - util.logexc(LOG, "Failed reading from metadata address %s", > - self.metadata_address) > - return False > + dhcp_leases = dhcp.maybe_dhcp_clean_discovery() > + if not dhcp_leases: > + # DataSourceEc2Local failed in init-local stage. > DataSourceEc2 > + # will still run in init-network stage. > + return False > + dhcp_opts = dhcp_leases[-1] > + net_params = {'interface': dhcp_opts.get('interface'), > + 'ip': dhcp_opts.get('fixed-address'), > + 'prefix_or_mask': dhcp_opts.get('subnet-mask'), > + 'broadcast': dhcp_opts.get('broadcast-address'), > + 'router': dhcp_opts.get('routers')} > + with net.EphemeralIPv4Network(**net_params): > + return self._crawl_metadata() > + else: > + return self._crawl_metadata() > > @property > def launch_index(self): > diff --git a/tests/unittests/test_datasource/test_ec2.py > b/tests/unittests/test_datasource/test_ec2.py > index 12230ae..4bb04d2 100644 > --- a/tests/unittests/test_datasource/test_ec2.py > +++ b/tests/unittests/test_datasource/test_ec2.py > @@ -123,8 +157,9 @@ class TestEc2(test_helpers.HttprettyTestCase): > > def setUp(self): > super(TestEc2, self).setUp() > - self.metadata_addr = ec2.DataSourceEc2.metadata_urls[0] > - self.api_ver = '2009-04-04' > + self.datasource = ec2.DataSourceEc2 > + self.metadata_addr = self.datasource.metadata_urls[0] > + self.api_ver = '2016-09-02' Yes we could/should use that instead of defining it locally. Fixed. > > @property > def metadata_url(self): -- https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/328241 Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:aws-local-dhcp into cloud-init:master. _______________________________________________ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp