On Fri, Aug 26, 2016 at 9:23 AM, Scott Moser <email address hidden> wrote:

> NITPICK: i reformatted your commit message to keep line lengths < 74 and
> some spelling.
>
>
> i like the general idea, and the fact that you added some good vmtest
> tests.
>
> Diff comments:
>
> > === modified file 'curtin/commands/apply_net.py'
> > --- curtin/commands/apply_net.py      2016-07-13 07:50:49 +0000
> > +++ curtin/commands/apply_net.py      2016-08-25 23:32:25 +0000
> > @@ -26,6 +26,54 @@
> >
> >  LOG = log.LOG
> >
> > +IFUPDOWN_IPV6_MTU_PRE_HOOK = """#!/bin/sh -x
>
> as much as i *hate* set -e, i agree here. either we need to run with -e,
> or otherwise correctly exit failure on failure rather than barreling on.
>

ACK


>
> > +# injected by curtin installer
> > +
> > +[ "$IFACE" != "lo" ] || exit 0
> > +
> > +# Trigger only if MTU configured
> > +[ -n "$IF_MTU" ] || exit 0
> > +
> > +CUR_DEV_MTU=$(/bin/cat /sys/class/net/$IFACE/mtu ||:)
>
> [ ! -e /sys/class/net/$IFACE/mtu ] || read CUR_DEV_MTU
> /sys/class/net/$IFACE/mtu
>

Nice, will change.


>
> > +CUR_IPV6_MTU=$(sysctl -n net.ipv6.conf.$IFACE.mtu ||:)
> > +[ -n "$CUR_DEV_MTU" ] && echo $CUR_DEV_MTU > /run/network/$IFACE_dev.mtu
> > +[ -n "$CUR_IPV6_MTU" ] && echo $CUR_IPV6_MTU >
> /run/network/$IFACE_ipv6.mtu
> > +exit 0
> > +"""
> > +
> > +IFUPDOWN_IPV6_MTU_POST_HOOK = """#!/bin/sh -x
> > +# injected by curtin installer
> > +
> > +[ "$IFACE" != "lo" ] || exit 0
> > +
> > +# Trigger only if MTU configured
> > +[ -n "$IF_MTU" ] || exit 0
> > +
> > +PRE_DEV_MTU=$(cat /run/network/$IFACE_dev.mtu)
>
> +one on christian's {} i know its obnoxious, but there are times when
> doing something like above ends up referencing a variable like '$IFACE_dev'
> rather than ${IFACE}_dev.  i have never been able to find out a pattern on
> it, but its why i alays surround variables names.  Also makes search and
> replace for var names easier.
>

I entirely agree with you; I was keeping with existing style of ifupdown
hooks.  But I'm happy to do things safer.


>
> > +CUR_DEV_MTU=$(/bin/cat /sys/class/net/$IFACE/mtu)
>
> read CUR_DEV_MTU < /sys/class/net/$IFACE/mtu
> is lots faster and equivalent.
>
> i realize this is picking on the order of hundreths of a second, but this
> blocks network coming up which blocks boot.
>

Understood, worth a change.


>
> > +PRE_IPV6_MTU=$(cat /run/network/$IFACE_ipv6.mtu)
> > +CUR_IPV6_MTU=$(sysctl -n net.ipv6.conf.$IFACE.mtu)
>
> echo > /sys/class/net .... ? rather than sysctl ? is there a reason for
> one over the other ?
> i thought you recently told me sysctl was just doing that.
>

I can replace the sysctl -n with a read;  it's somewhat non-obvious
that sysctl net. implies /proc/sys/net .  But it saves the exec overhead


>
> > +
> > +if [ "$ADDRFAM" = "inet6" ]; then
> > +  # We need to check the underlying interface MTU and
> > +  # raise it if the IPV6 mtu is larger
> > +  if [ $CUR_DEV_MTU -lt $IF_MTU ]; then
> > +      /bin/ip link set $IFACE mtu $IF_MTU
> > +  fi
> > +  /sbin/sysctl -q -e -w net.ipv6.conf.$IFACE.mtu=$IF_MTU
>
> even better, just do not hard code paths.
> if /sbin/ is not in your path you are probably foobarred in other ways
> you've not considered.
> if /bin is not in your path, same deal.
> if the PATH variable here is maliciously affected, you're already rooted.
>
> let the system set the correct path for you, or do it once if you want to
> be pedantic by setting PATH. in my opinioni hard coding paths just makes
> your code longer and more brittle (when you happen on a system where
> someone installed 'ip' into /usr/bin or /usr/local/bin).
>

While I agree with you, all of the hooks and ifupdown packages do hard code
the path.
If you insist, I'll remove it; but it won't fix the case where an OS using
ifupdown has ip somewhere
not in /sbin.


>
> > +
> > +elif [ "$ADDRFAM" = "inet" ]; then
> > +  # handle the clobber case where inet mtu changes v6 mtu
> > +  # ifupdown will already have set dev mtu, so lower mtu
> > +  # if needed.  If v6 mtu was larger, it get's clamped down
> > +  # to the dev MTU value.
> > +  if [ $PRE_IPV6_MTU -lt $CUR_IPV6_MTU ]; then
> > +    /sbin/sysctl -q -e -w net.ipv6.conf.$IFACE.mtu=$PRE_IPV6_MTU
> > +  fi
> > +fi
> > +exit 0
> > +"""
> > +
> >
> >  def apply_net(target, network_state=None, network_config=None):
> >      if network_state is None and network_config is None:
> > @@ -45,6 +93,111 @@
> >
> >      net.render_network_state(target=target, network_state=ns)
> >
> > +    _maybe_remove_legacy_eth0(target)
> > +    LOG.info('Attempting to remove ipv6 privacy extensions')
> > +    _disable_ipv6_privacy_extensions(target)
> > +    _patch_ifupdown_ipv6_mtu_hook(target)
> > +
> > +
> > +def _patch_ifupdown_ipv6_mtu_hook(target,
> > +                                  prehookfn="etc/network/if-pre-
> up.d/mtuipv6",
> > +                                  posthookfn="etc/network/if-up.
> d/mtuipv6"):
> > +
> > +    contents = {
> > +        'prehook': IFUPDOWN_IPV6_MTU_PRE_HOOK,
> > +        'posthook': IFUPDOWN_IPV6_MTU_POST_HOOK,
> > +    }
> > +
> > +    hookfn = {
> > +        'prehook': prehookfn,
> > +        'posthook': posthookfn,
> > +    }
> > +
> > +    for hook in ['prehook', 'posthook']:
> > +        fn = hookfn[hook]
> > +        cfg = os.path.sep.join([target, fn])
>
> this is easier done and safer as
>   util.target_path(target, fn)
>
> then above, you dont have to worry that you (could) accidently add a / in
> front of prehookfn .
>

OK


>
> > +        LOG.info('Injecting fix for ipv6 mtu settings: %s', cfg)
> > +        util.write_file(cfg, contents[hook], mode=0o755)
> > +
> > +
> > +def _disable_ipv6_privacy_extensions(target,
> > +                                     path="etc/sysctl.d/10-ipv6-
> privacy.conf"):
> > +
> > +    """Ubuntu server image sets a preference to use IPv6 privacy
> extensions
> > +       by default; this races with the cloud-image desire to disable
> them.
> > +       Resolve this by allowing the cloud-image setting to win. """
> > +
> > +    if path.startswith("/"):
> > +        raise ValueError("path parameter must not start with '/': %s",
> path)
>
> use target_path, then it can.
>

OK


>
> > +
> > +    cfg = os.path.normpath(os.path.sep.join([target, path]))
> > +    if not os.path.exists(cfg):
> > +        LOG.warn('Failed to find legacy conf file %s', cfg)
>
> i think you should raise exception or not care.
> logging warn just puts WARN statements in the log, that seem to me to
> probably be harmless, outside of the case where that filename got moved.
> if it *did* get moved in the ubuntu package, and thus we are no longer able
> to disable this, then you should probably log exception.
>
> i dont know what seems best here.
>

Honestly, WARN seems right.  It's a warning, something *may* not be right;
but we can't say for sure.
In this case, depending on the default settings of your kernel (trusty
blows up, everything else doesn't, until they do)

Again, it may not affect you since your config only had ipv4, not ipv6.

This seems like a reasonable warning.



>
> > +        return
> > +
> > +    bmsg = "Disabling IPv6 privacy extensions config may not apply."
> > +    try:
> > +        contents = util.load_file(cfg)
> > +        known_contents = ["net.ipv6.conf.all.use_tempaddr = 2",
> > +                          "net.ipv6.conf.default.use_tempaddr = 2"]
> > +        lines = [f.strip() for f in contents.splitlines()
> > +                 if not f.startswith("#")]
> > +        if lines == known_contents:
> > +            LOG.info('deleting file: %s', cfg)
> > +            util.del_file(cfg)
> > +            msg = "removed %s with known contents" % cfg
> > +            curtin_contents = '\n'.join(
> > +                ["# IPv6 Privacy Extensions (RFC 4941)",
> > +                 "# Disabled by curtin",
> > +                 "# net.ipv6.conf.all.use_tempaddr = 2",
> > +                 "# net.ipv6.conf.default.use_tempaddr = 2"])
> > +            util.write_file(cfg, curtin_contents)
> > +        else:
> > +            LOG.info('skipping, content didnt match')
> > +            LOG.info(lines)
>
> logging an entire file at INFO level seems loud to me.
>

Yeah, I should mark debug and add context, or drop.


>
> > +            LOG.info(known_contents)
> > +            msg = (bmsg + " '%s' exists with user configured content."
> % cfg)
> > +    except:
> > +        msg = bmsg + " %s exists, but could not be read." % cfg
> > +        LOG.exception(msg)
>
> raising exception here seems completely reasonable. no?
>

Same format as your _maybe remove, so unless you want to change this is a
common idiom we've used.



>
> > +        return
> > +
> > +
> > +def _maybe_remove_legacy_eth0(target,
> > +                              path="etc/network/interfaces.
> d/eth0.cfg"):
> > +    """Ubuntu cloud images previously included a 'eth0.cfg' that had
> > +       hard coded content.  That file would interfere with the rendered
> > +       configuration if it was present.
> > +
> > +       if the file does not exist do nothing.
> > +       If the file exists:
> > +         - with known content, remove it and warn
> > +         - with unknown content, leave it and warn
> > +    """
> > +
> > +    cfg = os.path.normpath(os.path.sep.join([target, path]))
> > +    if not os.path.exists(cfg):
> > +        LOG.warn('Failed to find legacy conf file %s', cfg)
> > +        return
> > +
> > +    bmsg = "Dynamic networking config may not apply."
> > +    try:
> > +        contents = util.load_file(cfg)
> > +        known_contents = ["auto eth0", "iface eth0 inet dhcp"]
> > +        lines = [f.strip() for f in contents.splitlines()
> > +                 if not f.startswith("#")]
> > +        if lines == known_contents:
> > +            util.del_file(cfg)
> > +            msg = "removed %s with known contents" % cfg
> > +        else:
> > +            msg = (bmsg + " '%s' exists with user configured content."
> % cfg)
> > +    except:
> > +        msg = bmsg + " %s exists, but could not be read." % cfg
> > +        LOG.exception(msg)
> > +        return
> > +
> > +    LOG.warn(msg)
> > +
> >
> >  def apply_net_main(args):
> >      #  curtin apply_net [--net-state=/config/netstate.yml] [--target=/]
> >
> > === modified file 'tests/vmtests/helpers.py'
> > --- tests/vmtests/helpers.py  2016-06-16 18:45:05 +0000
> > +++ tests/vmtests/helpers.py  2016-08-25 23:32:25 +0000
> > @@ -286,3 +286,123 @@
> >          return _parse_ifconfig_yakkety(ifconfig_a)
> >      else:
> >          return _parse_ifconfig_xenial(ifconfig_a)
> > +
> > +
> > +def _parse_ip_a(ip_a):
> > +    """
> > +    2: interface0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1480 qdisc
> pfifo_fast\
> > +        state UP group default qlen 1000
> > +        link/ether 52:54:00:12:34:00 brd ff:ff:ff:ff:ff:ff
> > +        inet 192.168.1.2/24 brd 192.168.1.255 scope global interface0
> > +            valid_lft forever preferred_lft forever
> > +        inet6 2001:4800:78ff:1b:be76:4eff:fe06:1000/64 scope global
> > +            valid_lft forever preferred_lft forever
> > +        inet6 fe80::5054:ff:fe12:3400/64 scope link
> > +        valid_lft forever preferred_lft forever
> > +    """
> > +    ifaces = {}
> > +    combined_fields = {
> > +        'brd': 'broadcast',
> > +        'link/ether': 'mac_address',
> > +    }
> > +    interface_fields = [
> > +        'group',
> > +        'master',
> > +        'mtu',
> > +        'qdisc',
> > +        'qlen',
> > +        'state',
> > +    ]
> > +    inet_fields = [
> > +        'valid_lft',
> > +        'preferred_left'
> > +    ]
> > +    boolmap = {
> > +        'BROADCAST': 'broadcast',
> > +        'LOOPBACK': 'loopback',
> > +        'LOWER_UP': 'lower_up',
> > +        'MULTICAST': 'multicast',
> > +        'RUNNING': 'running',
> > +        'UP': 'up',
> > +    }
> > +
> > +    for line in ip_a.splitlines():
> > +        if not line:
> > +            continue
> > +
> > +        toks = line.split()
> > +        if not line.startswith("    "):
> > +            cur_iface = line.split()[1].rstrip(":")
> > +            cur_data = {
> > +                'inet4': [],
> > +                'inet6': [],
> > +                'interface': cur_iface
> > +            }
> > +            # vlan's get a fancy name <iface name>@<vlan_link>
> > +            if '@' in cur_iface:
> > +                cur_iface, vlan_link = cur_iface.split("@")
> > +                cur_data.update({'interface': cur_iface,
> > +                                 'vlan_link': vlan_link})
> > +            for t in boolmap.values():
> > +                # <BROADCAST,MULTICAST,UP,LOWER_UP>
> > +                cur_data[t] = t.upper() in line[2]
> > +            ifaces[cur_iface] = cur_data
> > +
> > +        for i in range(0, len(toks)):
> > +            cur_tok = toks[i]
> > +            try:
> > +                next_tok = toks[i+1]
> > +            except IndexError:
> > +                next_tok = None
> > +
> > +            # parse link/ether, brd
> > +            if cur_tok in combined_fields.keys():
> > +                cur_data[combined_fields[cur_tok]] = next_tok
> > +            # mtu an other interface line key/value pairs
> > +            elif cur_tok in interface_fields:
> > +                cur_data[cur_tok] = next_tok
> > +            elif cur_tok.startswith("inet"):
> > +                cidr = toks[1]
> > +                address, prefixlen = cidr.split("/")
> > +                cur_ip = {
> > +                    'address': address,
> > +                    'prefixlen': prefixlen,
> > +                }
> > +                if ":" in address:
> > +                    cur_ipv6 = cur_ip.copy()
> > +                    cur_ipv6.update({'scope': toks[3]})
> > +                    cur_data['inet6'].append(cur_ipv6)
> > +                else:
> > +                    cur_ipv4 = cur_ip.copy()
> > +                    if len(toks) > 5:
> > +                        cur_ipv4.update({'scope': toks[5]})
> > +                    else:
> > +                        cur_ipv4.update({'scope': toks[3]})
> > +                    cur_data['inet4'].append(cur_ipv4)
> > +
> > +                continue
> > +            elif cur_tok in inet_fields:
> > +                if ":" in address:
> > +                    cur_ipv6[cur_tok] = next_tok
> > +                else:
> > +                    cur_ipv4[cur_tok] = next_tok
> > +                continue
> > +
> > +    return ifaces
> > +
> > +
> > +def ip_a_to_dict(ip_a):
> > +    # return a dictionary of network information like:
> > +    #  {'ens2': {'address': '10.5.0.78', 'broadcast': '10.5.255.255',
> > +    #         'broadcast_flag': True,
> > +    #         'inet6': [{'address': 'fe80::f816:3eff:fe05:9673',
> > +    #                    'prefixlen': '64', 'scopeid': '0x20<link>'},
> > +    #                   {'address': 'fe80::f816:3eff:fe05:9673',
> > +    #                    'prefixlen': '64', 'scopeid': '0x20<link>'}],
> > +    #         'interface': 'ens2', 'link_encap': 'Ethernet',
> > +    #         'mac_address': 'fa:16:3e:05:96:73', 'mtu': 1500,
> > +    #         'multicast': True, 'netmask': '255.255.0.0',
> > +    #         'running': True, 'up': True}}
> > +    # from iproute2 ip a command
>
> should we remove my ifconfig_to_dict  things ?
>

Yes!  I forgot that.


>
> > +
> > +    return _parse_ip_a(ip_a)
>
>
> --
> https://code.launchpad.net/~raharper/curtin/trunk.more-ipv6/+merge/304027
> You are the owner of lp:~raharper/curtin/trunk.more-ipv6.
>