cloud-init-dev team mailing list archive

[Robert Schweikert <rjschwei@xxxxxxxx>]

The commit is missing cc_disable_ec2_metadata.py which uses the "route" command from net-tools command

Too bad this had to be started over rather than beating the since last year pending request into shape.

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 = {}
> -    for line in str(ifcfg_out).splitlines():
> +    for line in str(ipaddr_data).splitlines():

in Python 3 the data passed in the call generated with (ipaddr_out, _err) = util.subp(["ip", "--oneline", "addr", "list"]) is, as in Python 2, of type string. I agree the explicit conversion is confusing. If bytes should be handled as a "just in case" implementation then it should be type tested and decode() should be used.

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

I understand the thinking for the name, as in "entry field post fix" but it took me a while, I think protocol_version, or ip_version would be more explicit and more obvious. Of course for IPv4 the version string is "" and that may throw others for a loop.

> +        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("/")
> +                devs[curdev]["mask"] = netdev_cidr_to_mask(cidr)
> +                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]

Loops over every item although we know that more than 1/2 the entries in the list are of no interest. The data, for example:

['2:', 'eth0', 'inet6', 'fe80::c5d:2fff:fe4e:6a54/64', 'scope', 'link', '\\', 'valid_lft', 'forever', 'preferred_lft', 'forever']

so the loop hits "2:", the "eth0" eventually it hits the ip-address etc. We know ahead of time that we don't care about those entries. At the very least the loop should start after the device, i.e. the loop should start at index 2.

> +
> +    for line in str(iplink_data).splitlines():
> +        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():

And another str()

>          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)

Not certain that splitting up the data here is all that advantageous as it creates two arguments to the method and inside the method (netdev_info_iproute()) and inside the method we then end up having to loops to handle the two different arguments. With this route of implementation it would be nicer, IMHO, to use to methods that parse the data, one methods handle the "ipaddr_out" data and the other takes the output from that and the iplink_out data. that would also make testing to be more targeted. Or do it all in one go and use "util.subp(["ip","addr", "list"])

> +    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 neither "ip" nor "ifconfig" are available this generates a traceback as "util.subp(["ifconfig"...." will throw an exception. The new exception may or may not get caught somewhere up the stack.

I think we should exit more gracefully even if the network information cannot be obtained. Yes we can argue that it is the image builders/packagers/sys-admins responsibility to make sure one of those is available and we can also argue that probably other stuff on the system will be broken if neither command is available. I would counter that with the comment the cloud-init should not be the code that throws up due missing tools, error out gracefully.

>  
>      if empty != "":
>          for (_devname, dev) in devs.items():


-- 
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.