cloud-init-dev team mailing list archive

[Chad Smith <chad.smith@xxxxxxxxxxxxx>]

All comments addressed. Please glance over the changes if you get a chance.

Diff comments:

> diff --git a/cloudinit/netinfo.py b/cloudinit/netinfo.py
> index 993b26c..081f7b4 100644
> --- a/cloudinit/netinfo.py
> +++ b/cloudinit/netinfo.py
> @@ -18,18 +21,81 @@ 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 = {
> +    "addr": "",
> +    "bcast": "",
> +    "hwaddr": "",
> +    "mask": "",
> +    "scope": "",
> +    "up": False
> +}
> +
> +
> +def netdev_cidr_to_mask(cidr):
> +    mask = socket.inet_ntoa(
> +        struct.pack(">I", (0xffffffff << (32 - int(cidr)) & 0xffffffff)))
> +    return mask
> +
> +
> +def netdev_info_iproute(ipaddr_data, iplink_data):
> +    """
> +    Get network device dicts from ip route and ip link info.
> +
> +    @param ipaddr_data: Output string from ip address command.
> +    @param iplink_data: Output string from ip link command.
> +
> +    @returns: A dict of device info keyed by network device name containing
> +              device configuration values.
> +    """
>      devs = {}

We won't blowup on empty data, we return devs = {} in that case, which feels appropriate if no routes are listed in output.

> -    for line in str(ifcfg_out).splitlines():
> +    for line in str(ipaddr_data).splitlines():

Yeah, I should've looked more at some of this code when I took over the branch. The util.subp function should take care of this data decoding for us if necessary, I'd expect it to be handling in a common layer instead of making all call-sites type-check and decode. This str might have been inherited from when subp didn't return the right string type.

> +        details = line.lower().strip().split()
> +        curdev = details[1]
> +        fieldpost = ""

Good though, changed to ip_version_suffix and added a comment about its use.

> +        if curdev not in devs:
> +            devs[curdev] = copy(DEFAULT_NETDEV_INFO)
> +        for i in range(len(details)):
> +            if details[i] == 'inet':
> +                (addr, cidr) = details[i + 1].split("/")

I used partition('/') and set the cidr to '24' if not present. We can look at consolidating in a separate branch.

> +                devs[curdev]["mask"] = netdev_cidr_to_mask(cidr)

Moved it. good thought. I missed the duplication.

> +                devs[curdev]["addr"] = addr
> +                fieldpost = ""
> +            elif details[i] == 'inet6':
> +                addr = details[i + 1]
> +                devs[curdev]["addr6"] = addr
> +                fieldpost = "6"
> +            elif details[i] == "scope":
> +                devs[curdev]["scope" + fieldpost] = details[i + 1]
> +            elif details[i] == "brd":
> +                devs[curdev]["bcast" + fieldpost] = details[i + 1]

Took both of your comments. I simplified netdev_info_iproute to parse all of 'ip addr show' in one go. I also tweaked the logic to use regex matches instead of walking through individual tokens. It's only about the same performance as far timeit is concerned, bit the regex gives us a better view of the expected format of the output.

> +
> +    for line in str(iplink_data).splitlines():

dropped the str throughout.

> +        details = line.lower().strip().split()
> +        # Strip trailing ':' and truncate any interface alias '@if34'
> +        curdev = details[1].rstrip(':').split('@')[0]
> +        for i in range(len(details)):
> +            if details[i] == curdev + ":":
> +                flags = details[i + 1].strip("<>").split(",")
> +                if "lower_up" in flags and "up" in flags:
> +                    devs[curdev]["up"] = True
> +            elif details[i] == "link/ether":
> +                devs[curdev]["hwaddr"] = details[i + 1]
> +    return devs
> +
> +
> +def netdev_info_ifconfig(ifconfig_data):
> +    # fields that need to be returned in devs for each dev
> +    devs = {}
> +    for line in str(ifconfig_data).splitlines():

dropped the str throughout.

>          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] = copy(DEFAULT_NETDEV_INFO)
>          toks = line.lower().strip().split()
>          if toks[0] == "up":
>              devs[curdev]['up'] = True
> @@ -39,41 +105,50 @@ 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":
> +                if toks[i + 1].startswith("addr:"):
> +                    devs[curdev]['addr'] = toks[i + 1].lstrip("addr:")
> +                else:
> +                    devs[curdev]['addr'] = toks[i + 1]
> +            elif toks[i].startswith("bcast:"):
> +                devs[curdev]['bcast'] = toks[i].lstrip("bcast:")
> +            elif toks[i] == "broadcast":
> +                devs[curdev]['bcast'] = toks[i + 1]
> +            elif toks[i].startswith("mask:"):
> +                devs[curdev]['mask'] = toks[i].lstrip("mask:")
> +            elif toks[i] == "netmask":
> +                devs[curdev]['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]['addr6'] = toks[i + 2]
> +                else:
> +                    devs[curdev]['addr6'] = toks[i + 1]
> +            elif toks[i] == "prefixlen":
> +                addr6 = devs[curdev]['addr6'] + "/" + toks[i + 1]
> +                devs[curdev]['addr6'] = addr6
> +            elif toks[i].startswith("scope:"):
> +                devs[curdev]['scope6'] = toks[i].lstrip("scope:")
> +            elif toks[i] == "scopeid":
> +                res = re.match(".*<(\S+)>", toks[i + 1])
> +                if res:
> +                    devs[curdev]['scope6'] = res.group(1)
> +    return devs
> +
> +
> +def netdev_info(empty=""):
> +    devs = {}
> +    try:
> +        # Try iproute first of all
> +        (ipaddr_out, _err) = util.subp(["ip", "--oneline", "addr", "list"])
> +        (iplink_out, _err) = util.subp(["ip", "--oneline", "link", "list"])
> +        devs = netdev_info_iproute(ipaddr_out, iplink_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)

+1 great point. fixed.

>  
>      if empty != "":
>          for (_devname, dev) in devs.items():
> @@ -125,31 +279,53 @@ def route_info():
>          routes['ipv4'].append(entry)
>  
>      try:
> -        (route_out6, _err6) = util.subp(["netstat", "-A", "inet6", "-n"],
> -                                        rcs=[0, 1])
> +        (route_data6, _err6) = util.subp(
> +            ["netstat", "-A", "inet6", "--route", "--numeric"], rcs=[0, 1])
>      except util.ProcessExecutionError:
>          pass
>      else:
> -        entries6 = route_out6.splitlines()[1:]
> +        entries6 = route_data6.splitlines()
>          for line in entries6:
>              if not line:
>                  continue
>              toks = line.split()
> -            if (len(toks) < 6 or toks[0] == "Kernel" or
> +            if (len(toks) < 7 or toks[0] == "Kernel" or
> +                    toks[0] == "Destination" or toks[0] == "Internet" or
>                      toks[0] == "Proto" or toks[0] == "Active"):
>                  continue
>              entry = {
> -                'proto': toks[0],
> -                'recv-q': toks[1],
> -                'send-q': toks[2],
> -                'local address': toks[3],
> -                'foreign address': toks[4],
> -                'state': toks[5],
> +                'destination': toks[0],
> +                'gateway': toks[1],
> +                'flags': toks[2],
> +                'metric': toks[3],
> +                'ref': toks[4],
> +                'use': toks[5],
> +                'iface': toks[6],
>              }
> +            # skip lo interface on ipv6
> +            if entry['iface'] == "lo":
> +                continue
> +            # strip /128 from address if it's included
> +            if entry['destination'].endswith('/128'):
> +                entry['destination'] = entry['destination'][:-4]

Good suggestion, though I went with re.sub(r'\/128$', '', ...) here just to be explicit.

>              routes['ipv6'].append(entry)
>      return routes
>  
>  
> +def route_info():
> +    routes = {}
> +    try:
> +        # Try iproute first of all
> +        (iproute_out, _err) = util.subp(["ip", "-o", "route", "list"])
> +        routes = netdev_route_info_iproute(iproute_out)
> +    except util.ProcessExecutionError:
> +        # Fall back to net-tools if iproute2 is not present
> +        (route_out, _err) = util.subp(
> +            ["netstat", "--route", "--numeric", "--extend"], rcs=[0, 1])
> +        routes = netdev_route_info_netstat(route_out)
> +    return routes
> +
> +
>  def getgateway():
>      try:
>          routes = route_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.