cloud-init-dev team mailing list archive

[Scott Moser <smoser@xxxxxxxxxx>]

theres nothing huge in my comments.
thanks.


Diff comments:

> diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py
> index 46cb9c8..d38ea8b 100644
> --- a/cloudinit/net/__init__.py
> +++ b/cloudinit/net/__init__.py
> @@ -175,12 +175,10 @@ def is_disabled_cfg(cfg):
>      return cfg.get('config') == "disabled"
>  
>  
> -def generate_fallback_config(blacklist_drivers=None, config_driver=None):
> -    """Determine which attached net dev is most likely to have a connection and
> -       generate network state to run dhcp on that interface"""
> -
> -    if not config_driver:
> -        config_driver = False
> +def find_fallback_nic(blacklist_drivers=None):
> +    """Return the name of the 'fallback' network device."""
> +    if not blacklist_drivers:
> +        blacklist_drivers = []
>  
>      if not blacklist_drivers:
>          blacklist_drivers = []

if not blacklist_drivers:
...
if not blacklist_drivers:

am i seeing double?

> @@ -233,15 +231,24 @@ def generate_fallback_config(blacklist_drivers=None, config_driver=None):
>      if DEFAULT_PRIMARY_INTERFACE in names:
>          names.remove(DEFAULT_PRIMARY_INTERFACE)
>          names.insert(0, DEFAULT_PRIMARY_INTERFACE)
> -    target_name = None
> -    target_mac = None
> +
> +    # pick the first that has a address

'address' is mac address. unlikely that it changes.
possibly we could make a helper like 'valid_mac()' and use that above in the 'for interface in potential_intefaces' loop.
i say 'valid_mac' because we have knowledge elsewhere to avoid a mac like '00:00:00:00:00:00' (bug 1692028)

>      for name in names:
> -        mac = read_sys_net_safe(name, 'address')
> -        if mac:
> -            target_name = name
> -            target_mac = mac
> -            break
> -    if target_mac and target_name:
> +        if read_sys_net_safe(name, 'address'):
> +            return name
> +    return None
> +
> +
> +def generate_fallback_config(blacklist_drivers=None, config_driver=None):
> +    """Determine which attached net dev is most likely to have a connection and
> +       generate network state to run dhcp on that interface"""
> +
> +    if not config_driver:
> +        config_driver = False
> +
> +    target_name = find_fallback_nic(blacklist_drivers=blacklist_drivers)
> +    if target_name:
> +        target_mac = read_sys_net_safe(target_name, 'address')
>          nconf = {'config': [], 'version': 1}
>          cfg = {'type': 'physical', 'name': target_name,
>                 'mac_address': target_mac, 'subnets': [{'type': 'dhcp'}]}
> 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.smith@xxxxxxxxxxxxx>
> +#
> +# 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.

clean here is "without side affects". see dhcp_clean_discovery . this is just "maybe" do that.

it does say that "If the device is already connected, do nothing."
but i dont see that.

> +
> +    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)
> +        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()
> +    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.
> +
> +    @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>.
> +    # 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.
> +    # 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']

the value of the '-sf' does not get tokenized on space or otherwise interpreted.
so these dont work:
   -sf true    #"Failed to get realpath for true:"
   -sf "/bin/sh -c :"  # execve (/bin/sh -c :, ...): No such file or directory

> +    util.subp(dhclient_cmd, capture=True)
> +    return parse_dhcp_lease_file(lease_file)
> +
> +
> +# vi: ts=4 expandtab
> 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
> @@ -21,7 +23,7 @@ from cloudinit import warnings
>  LOG = logging.getLogger(__name__)
>  
>  # Which version we are requesting of the ec2 metadata apis
> -DEF_MD_VERSION = '2009-04-04'
> +DEF_MD_VERSION = '2016-09-02'

thats right . we do have to support reading the old MD as lookalikes likely just have the 2009 value.

>  
>  STRICT_ID_PATH = ("datasource", "Ec2", "strict_id")
>  STRICT_ID_DEFAULT = "warn"
> @@ -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")
>                  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')}

this mapping, which is essentially dhcp option name to EphemeralIPv4Network argument name seems like it might better be put in the dhcp module. other than 'prefix_or_mask' the names we have here are quite sane.
we could change EphemeralIPv4Network to take either a 'network_prefix' or 'netmask', and then they're *really* sane, and just some function in the dhcp module return you that...
i'm not set on it, it just seemed like lines 401 to 412 here were verbose, and not really related to what we were trying to accomplish.

> +            with net.EphemeralIPv4Network(**net_params):
> +                return self._crawl_metadata()
> +        else:
> +            return self._crawl_metadata()
>  
>      @property
>      def launch_index(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.