cloud-init-dev team mailing list archive

[Chad Smith <chad.smith@xxxxxxxxxxxxx>]

Diff comments:

> diff --git a/cloudinit/netinfo.py b/cloudinit/netinfo.py
> index 993b26c..a8c7f7a 100644
> --- a/cloudinit/netinfo.py
> +++ b/cloudinit/netinfo.py
> @@ -18,18 +20,89 @@ from cloudinit.simpletable import SimpleTable
>  LOG = logging.getLogger()
>  
>  
> -def netdev_info(empty=""):
> -    fields = ("hwaddr", "addr", "bcast", "mask")
> -    (ifcfg_out, _err) = util.subp(["ifconfig", "-a"], rcs=[0, 1])
> +DEFAULT_NETDEV_INFO = {
> +    "ipv4": [],
> +    "ipv6": [],
> +    "hwaddr": "",
> +    "up": False
> +}
> +
> +
> +def netdev_info_iproute(ipaddr_out):
> +    """
> +    Get network device dicts from ip route and ip link info.
> +
> +    @param ipaddr_out: Output string from 'ip addr show' command.
> +
> +    @returns: A dict of device info keyed by network device name containing
> +              device configuration values.
> +    """
>      devs = {}
> -    for line in str(ifcfg_out).splitlines():
> +    dev_name = None
> +    for num, line in enumerate(ipaddr_out.splitlines()):

True, but this is a programming error if we pass in the wrong type and the all call paths of this method should already be exercised by our unit tests on netdev_pformat(). This is not a failure path that can be introduced easily by users because we avoid calling this method at all if subp raises a ProcessExecutionError. Our failure mode if that did happen will be printing  !!!!!Net device info failed!!!!!!!! to cloud-init-output.log, no functional impact otherwise. We already catch potential errors here, but I think our error print could contain the specific error so we can triage more easily.

> +        m = re.match(r'^\d+:\s(?P<dev>[^:]+):\s+<(?P<flags>\S+)>\s+.*', line)
> +        if m:
> +            dev_name = m.group('dev').lower().split('@')[0]
> +            flags = m.group('flags').split(',')
> +            devs[dev_name] = {
> +                'ipv4': [], 'ipv6': [], 'hwaddr': '',
> +                'up': bool('UP' in flags and 'LOWER_UP' in flags),
> +            }
> +        elif 'inet6' in line:
> +            m = re.match(
> +                r'\s+inet6\s(?P<ip>\S+)\sscope\s(?P<scope6>\S+).*', line)
> +            if not m:
> +                LOG.warning(
> +                    'Could not parse ip addr show: (line:%d) %s', num, line)
> +                continue
> +            devs[dev_name]['ipv6'].append(m.groupdict())
> +        elif 'inet' in line:
> +            m = re.match(
> +                r'\s+inet\s(?P<cidr4>\S+)(\sbrd\s(?P<bcast>\S+))?\sscope\s'
> +                r'(?P<scope>\S+).*', line)
> +            if not m:
> +                LOG.warning(
> +                    'Could not parse ip addr show: (line:%d) %s', num, line)
> +                continue
> +            match = m.groupdict()
> +            cidr4 = match.pop('cidr4')
> +            addr, _, prefix = cidr4.partition('/')
> +            if not prefix:
> +                prefix = '32'
> +            devs[dev_name]['ipv4'].append({
> +                'ip': addr,
> +                'bcast': match['bcast'] if match['bcast'] else '',
> +                'mask': net_prefix_to_ipv4_mask(prefix),
> +                'scope': match['scope']})
> +        elif 'link' in line:
> +            m = re.match(
> +                r'\s+link/(?P<link_type>\S+)\s(?P<hwaddr>\S+).*', line)
> +            if not m:
> +                LOG.warning(
> +                    'Could not parse ip addr show: (line:%d) %s', num, line)
> +                continue
> +            if m.group('link_type') == 'ether':
> +                devs[dev_name]['hwaddr'] = m.group('hwaddr')
> +            else:
> +                devs[dev_name]['hwaddr'] = ''
> +        else:
> +            continue
> +    return devs
> +
> +
> +def netdev_info_ifconfig(ifconfig_data):
> +    # fields that need to be returned in devs for each dev
> +    devs = {}
> +    for line in ifconfig_data.splitlines():

Same case here if we do 'blow up' the only caller of this function will print Net device info failed. I think I'll tweak the printed message to contain the str(e) so we can see the actual error in cloud-init-output.log to catch this case and the netdev_info_iproute()

>          if len(line) == 0:
>              continue
>          if line[0] not in ("\t", " "):
>              curdev = line.split()[0]
> -            devs[curdev] = {"up": False}
> -            for field in fields:
> -                devs[curdev][field] = ""
> +            # current ifconfig pops a ':' on the end of the device
> +            if curdev.endswith(':'):
> +                curdev = curdev[:-1]
> +            if curdev not in devs:
> +                devs[curdev] = deepcopy(DEFAULT_NETDEV_INFO)
>          toks = line.lower().strip().split()
>          if toks[0] == "up":
>              devs[curdev]['up'] = True
> @@ -39,41 +112,47 @@ def netdev_info(empty=""):
>              if re.search(r"flags=\d+<up,", toks[1]):
>                  devs[curdev]['up'] = True
>  
> -        fieldpost = ""
> -        if toks[0] == "inet6":
> -            fieldpost = "6"
> -
>          for i in range(len(toks)):
> -            # older net-tools (ubuntu) show 'inet addr:xx.yy',
> -            # newer (freebsd and fedora) show 'inet xx.yy'
> -            # just skip this 'inet' entry. (LP: #1285185)
> -            try:
> -                if ((toks[i] in ("inet", "inet6") and
> -                     toks[i + 1].startswith("addr:"))):
> -                    continue
> -            except IndexError:
> -                pass
> -
> -            # Couple the different items we're interested in with the correct
> -            # field since FreeBSD/CentOS/Fedora differ in the output.
> -            ifconfigfields = {
> -                "addr:": "addr", "inet": "addr",
> -                "bcast:": "bcast", "broadcast": "bcast",
> -                "mask:": "mask", "netmask": "mask",
> -                "hwaddr": "hwaddr", "ether": "hwaddr",
> -                "scope": "scope",
> -            }
> -            for origfield, field in ifconfigfields.items():
> -                target = "%s%s" % (field, fieldpost)
> -                if devs[curdev].get(target, ""):
> -                    continue
> -                if toks[i] == "%s" % origfield:
> -                    try:
> -                        devs[curdev][target] = toks[i + 1]
> -                    except IndexError:
> -                        pass
> -                elif toks[i].startswith("%s" % origfield):
> -                    devs[curdev][target] = toks[i][len(field) + 1:]
> +            if toks[i] == "inet":  # Create new ipv4 addr entry
> +                devs[curdev]['ipv4'].append(
> +                    {'ip': toks[i + 1].lstrip("addr:")})
> +            elif toks[i].startswith("bcast:"):
> +                devs[curdev]['ipv4'][-1]['bcast'] = toks[i].lstrip("bcast:")
> +            elif toks[i] == "broadcast":
> +                devs[curdev]['ipv4'][-1]['bcast'] = toks[i + 1]
> +            elif toks[i].startswith("mask:"):
> +                devs[curdev]['ipv4'][-1]['mask'] = toks[i].lstrip("mask:")
> +            elif toks[i] == "netmask":
> +                devs[curdev]['ipv4'][-1]['mask'] = toks[i + 1]
> +            elif toks[i] == "hwaddr" or toks[i] == "ether":
> +                devs[curdev]['hwaddr'] = toks[i + 1]
> +            elif toks[i] == "inet6":
> +                if toks[i + 1] == "addr:":
> +                    devs[curdev]['ipv6'].append({'ip': toks[i + 2]})
> +                else:
> +                    devs[curdev]['ipv6'].append({'ip': toks[i + 1]})
> +            elif toks[i] == "prefixlen":  # Add prefix to current ipv6 value
> +                addr6 = devs[curdev]['ipv6'][-1]['ip'] + "/" + toks[i + 1]
> +                devs[curdev]['ipv6'][-1]['ip'] = addr6
> +            elif toks[i].startswith("scope:"):
> +                devs[curdev]['ipv6'][-1]['scope6'] = toks[i].lstrip("scope:")
> +            elif toks[i] == "scopeid":
> +                res = re.match(".*<(\S+)>", toks[i + 1])
> +                if res:
> +                    devs[curdev]['ipv6'][-1]['scope6'] = res.group(1)
> +    return devs
> +
> +
> +def netdev_info(empty=""):
> +    devs = {}
> +    try:
> +        # Try iproute first of all

strange I thought I committed this. must've lost a changeset in the shuffle. fixing now.

> +        (ipaddr_out, _err) = util.subp(["ip", "addr", "show"])
> +        devs = netdev_info_iproute(ipaddr_out)
> +    except util.ProcessExecutionError:
> +        # Fall back to net-tools if iproute2 is not present
> +        (ifcfg_out, _err) = util.subp(["ifconfig", "-a"], rcs=[0, 1])
> +        devs = netdev_info_ifconfig(ifcfg_out)
>  
>      if empty != "":
>          for (_devname, dev) in devs.items():
> @@ -84,14 +163,94 @@ def netdev_info(empty=""):

Good thought, added and tested

>      return devs
>  
>  
> -def route_info():
> -    (route_out, _err) = util.subp(["netstat", "-rn"], rcs=[0, 1])
> +def netdev_route_info_iproute(iproute_data):
> +    """
> +    Get network route dicts from ip route info.
> +
> +    @param iproute_data: Output string from ip route command.
> +
> +    @returns: A dict containing ipv4 and ipv6 route entries as lists. Each
> +              item in the list is a route dictionary representing destination,
> +              gateway, flags, genmask and interface information.
> +    """
>  
>      routes = {}
>      routes['ipv4'] = []
>      routes['ipv6'] = []
> +    entries = iproute_data.splitlines()

We handle exceptions raised and ultimately 'pass' on non-string so this error is covered.

> +    default_route_entry = {
> +        'destination': '', 'flags': '', 'gateway': '', 'genmask': '',
> +        'iface': '', 'metric': ''}
> +    for line in entries:
> +        entry = copy(default_route_entry)
> +        if not line:
> +            continue
> +        toks = line.split()
> +        flags = ['U']
> +        if toks[0] == "default":
> +            entry['destination'] = "0.0.0.0"
> +            entry['genmask'] = "0.0.0.0"
> +        else:
> +            if '/' in toks[0]:
> +                (addr, cidr) = toks[0].split("/")
> +            else:
> +                addr = toks[0]
> +                cidr = '32'
> +                flags.append("H")
> +                entry['genmask'] = net_prefix_to_ipv4_mask(cidr)
> +            entry['destination'] = addr
> +            entry['genmask'] = net_prefix_to_ipv4_mask(cidr)
> +            entry['gateway'] = "0.0.0.0"
> +        for i in range(len(toks)):
> +            if toks[i] == "via":
> +                entry['gateway'] = toks[i + 1]
> +                flags.insert(1, "G")
> +            if toks[i] == "dev":
> +                entry["iface"] = toks[i + 1]
> +            if toks[i] == "metric":
> +                entry['metric'] = toks[i + 1]
> +        entry['flags'] = ''.join(flags)
> +        routes['ipv4'].append(entry)
> +    try:
> +        (iproute_data6, _err6) = util.subp(
> +            ["ip", "--oneline", "-6", "route", "list", "table", "all"],
> +            rcs=[0, 1])
> +    except util.ProcessExecutionError:
> +        pass
> +    else:
> +        entries6 = iproute_data6.splitlines()
> +        for line in entries6:
> +            entry = {}
> +            if not line:
> +                continue
> +            toks = line.split()
> +            if toks[0] == "default":
> +                entry['destination'] = "::/0"
> +                entry['flags'] = "UG"
> +            else:
> +                entry['destination'] = toks[0]
> +                entry['gateway'] = "::"
> +                entry['flags'] = "U"
> +            for i in range(len(toks)):
> +                if toks[i] == "via":
> +                    entry['gateway'] = toks[i + 1]
> +                    entry['flags'] = "UG"
> +                if toks[i] == "dev":
> +                    entry["iface"] = toks[i + 1]
> +                if toks[i] == "metric":
> +                    entry['metric'] = toks[i + 1]
> +                if toks[i] == "expires":
> +                    entry['flags'] = entry['flags'] + 'e'
> +            routes['ipv6'].append(entry)
> +    return routes
>  
> -    entries = route_out.splitlines()[1:]
> +
> +def netdev_route_info_netstat(route_data):
> +    routes = {}
> +    routes['ipv4'] = []
> +    routes['ipv6'] = []
> +
> +    entries = route_data.splitlines()
>      for line in entries:
>          if not line:
>              continue
> @@ -166,21 +348,25 @@ def netdev_pformat():
>      lines = []
>      try:
>          netdev = netdev_info(empty=".")
> -    except Exception:
> +    except Exception as e:
>          lines.append(util.center("Net device info failed", '!', 80))

Added the exception string to what the "Net device info failed" message so we use it now. Didn't tweak lint ignore rules.

>      else:
>          fields = ['Device', 'Up', 'Address', 'Mask', 'Scope', 'Hw-Address']
>          tbl = SimpleTable(fields)
> -        for (dev, d) in sorted(netdev.items()):
> -            tbl.add_row([dev, d["up"], d["addr"], d["mask"], ".", d["hwaddr"]])
> -            if d.get('addr6'):
> -                tbl.add_row([dev, d["up"],
> -                             d["addr6"], ".", d.get("scope6"), d["hwaddr"]])
> +        for (dev, data) in sorted(netdev.items()):
> +            for addr in data.get('ipv4'):
> +                tbl.add_row(
> +                    [dev, data["up"], addr["ip"], addr["mask"], ".",
> +                     data["hwaddr"]])
> +            for addr in data.get('ipv6'):
> +                tbl.add_row(
> +                    [dev, data["up"], addr["ip"], ".", addr["scope6"],
> +                     data["hwaddr"]])
>          netdev_s = tbl.get_string()
>          max_len = len(max(netdev_s.splitlines(), key=len))
>          header = util.center("Net device info", "+", max_len)
>          lines.extend([header, netdev_s])
> -    return "\n".join(lines)
> +    return "\n".join(lines) + "\n"
>  
>  
>  def route_pformat():
> @@ -193,32 +379,31 @@ def route_pformat():
>      else:
>          if routes.get('ipv4'):
>              fields_v4 = ['Route', 'Destination', 'Gateway',
> -                         'Genmask', 'Interface', 'Flags']
> +                         'Genmask', 'Interface', 'Flags', 'Metric']

Yeah we've added a column.

>              tbl_v4 = SimpleTable(fields_v4)
>              for (n, r) in enumerate(routes.get('ipv4')):
>                  route_id = str(n)
>                  tbl_v4.add_row([route_id, r['destination'],
>                                  r['gateway'], r['genmask'],
> -                                r['iface'], r['flags']])
> +                                r['iface'], r['flags'], r['metric']])
>              route_s = tbl_v4.get_string()
>              max_len = len(max(route_s.splitlines(), key=len))
>              header = util.center("Route IPv4 info", "+", max_len)
>              lines.extend([header, route_s])
>          if routes.get('ipv6'):
> -            fields_v6 = ['Route', 'Proto', 'Recv-Q', 'Send-Q',
> -                         'Local Address', 'Foreign Address', 'State']
> +            fields_v6 = ['Route', 'Destination', 'Gateway', 'Interface',
> +                         'Flags', 'Metric']
>              tbl_v6 = SimpleTable(fields_v6)
>              for (n, r) in enumerate(routes.get('ipv6')):
>                  route_id = str(n)
> -                tbl_v6.add_row([route_id, r['proto'],
> -                                r['recv-q'], r['send-q'],
> -                                r['local address'], r['foreign address'],
> -                                r['state']])
> +                tbl_v6.add_row([route_id, r['destination'],
> +                                r['gateway'], r['iface'],
> +                                r['flags'], r['metric']])
>              route_s = tbl_v6.get_string()
>              max_len = len(max(route_s.splitlines(), key=len))
>              header = util.center("Route IPv6 info", "+", max_len)
>              lines.extend([header, route_s])
> -    return "\n".join(lines)
> +    return "\n".join(lines) + "\n"
>  
>  
>  def debug_info(prefix='ci-info: '):


-- 
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/342428
Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:net-tools-deprecation-plus-review-comments into cloud-init:master.