cloud-init-dev team mailing list archive

[Scott Moser <smoser@xxxxxxxxxx>]

bunch of fixes in tree now. i thikn i addressed all feedback.


Diff comments:

> diff --git a/cloudinit/net/netplan.py b/cloudinit/net/netplan.py
> index d7ddf0c..6754330 100644
> --- a/cloudinit/net/netplan.py
> +++ b/cloudinit/net/netplan.py
> @@ -119,8 +119,8 @@ def _extract_addresses(config, entry):
>              entry.update({sn_type: True})
>          elif sn_type in ['static']:
>              addr = "%s" % subnet.get('address')
> -            if 'netmask' in subnet:
> -                addr += "/%s" % subnet.get('netmask')

actually we should just drop the 'if' for prefix.
and say that 'prefix' will always be there, and
   addr = "%s/%s" % (subnet['address'], subnet['prefix'])

Unless there is any reason that there would ever be an address
that does not have a network prefix.  Is there?

Either way, the code above is *wrong* because "<address>/<netmask>"
is not valid input for netplan (or really anything)
192.168.1.1/255.255.255.0
versus the correct (or at least more command and short):
 192.168.1.1/24

> +            if 'prefix' in subnet:
> +                addr += "/%d" % subnet.get('prefix')
>              if 'gateway' in subnet and subnet.get('gateway'):
>                  gateway = subnet.get('gateway')
>                  if ":" in gateway:
> @@ -138,7 +138,7 @@ def _extract_addresses(config, entry):
>                  entry.update({mtukey: subnet.get('mtu')})
>              for route in subnet.get('routes', []):
>                  to_net = "%s/%s" % (route.get('network'),
> -                                    route.get('netmask'))
> +                                    route.get('prefix'))

Ah. and additionally, currently netmask is only present if the address is ipv4 while prefix is present in both.
i'm not opposed to fixing that inconsistency.

>                  route = {
>                      'via': route.get('gateway'),
>                      'to': to_net,
> diff --git a/cloudinit/net/network_state.py b/cloudinit/net/network_state.py
> index db3c357..ef000ae 100644
> --- a/cloudinit/net/network_state.py
> +++ b/cloudinit/net/network_state.py
> @@ -692,6 +670,125 @@ class NetworkStateInterpreter(object):
>          return subnets
>  
>  
> +def _normalize_subnet(subnet):
> +    # Prune all keys with None values.
> +    subnet = copy.deepcopy(subnet)
> +    normal_subnet = dict((k, v) for k, v in subnet.items() if v)
> +
> +    if subnet.get('type') == 'static':

ack. fixed.

> +        normal_subnet.update(
> +            _normalize_net_keys(normal_subnet, address_keys=('address',)))
> +    normal_subnet['routes'] = [_normalize_route(r)
> +                               for r in subnet.get('routes', [])]
> +    return normal_subnet
> +
> +
> +def _normalize_net_keys(network, address_keys=()):
> +

fixed.

> +    """Normalize dictionary network keys returning prefix and address keys.
> +
> +    @param network: A dict of network-related definition containing prefix,
> +        netmask and address_keys.
> +    @param address_keys: A tuple of keys to search for representing the address
> +        or cidr. The first address_key discovered will be used for
> +        normalization.
> +
> +    @returns: A dict containing normalized prefix and matching addr_key.
> +    """
> +    net = dict((k, v) for k, v in network.items() if v)
> +    addr_key = None
> +    for key in address_keys:
> +        if net.get(key):
> +            addr_key = key
> +            break
> +    if not addr_key:
> +        message = (
> +            'No config network address keys [%s] found in %s' %
> +            (','.join(address_keys), network))
> +        LOG.error(message)
> +        raise ValueError(message)
> +
> +    addr = net.get(addr_key)
> +    ipv6 = is_ipv6_addr(addr)
> +    netmask = net.get('netmask')
> +    if "/" in addr:
> +        toks = addr.split("/", 2)

ack. fixed.

> +        net[addr_key] = toks[0]
> +        # If prefix is defined by the user but addr_key is a CIDR, we
> +        # silently overwrite the user's original prefix here. We should
> +        # log a warning.
> +        net['prefix'] = toks[1]

done.

> +        try:
> +            net['prefix'] = int(toks[1])
> +        except ValueError:
> +            # this supports input of <address>/255.255.255.0
> +            net['prefix'] = mask_to_net_prefix(toks[1])
> +
> +    elif netmask:
> +        net['prefix'] = mask_to_net_prefix(netmask)
> +    else:
> +        net['prefix'] = 64 if ipv6 else 24
> +
> +    if ipv6:
> +        # TODO: we could/maybe should add this back with the very uncommon
> +        # 'netmask' for ipv6.  We need a 'net_prefix_to_ipv6_mask' for that.
> +        if 'netmask' in net:
> +            del net['netmask']
> +    else:
> +        net['netmask'] = net_prefix_to_ipv4_mask(net['prefix'])
> +
> +    prefix = net['prefix']
> +    if prefix:
> +        try:
> +            net['prefix'] = int(prefix)
> +        except ValueError:
> +            raise TypeError(
> +                'Network config prefix {} is not an integer'.format(prefix))
> +    return net
> +
> +
> +def _normalize_route(route):
> +    """normalize a route.
> +    return a dictionary with only:
> +       'type': 'route' (only present if it was present in input)
> +       'network': the network portion of the route as a string.
> +       'prefix': the network prefix for address as an integer.
> +       'metric': integer metric (only if present in input).
> +       'netmask': netmask (string) equivalent to prefix iff network is ipv4.
> +       """
> +    # Prune None-value keys.  Specifically allow 0 (a valid metric).
> +    normal_route = dict((k, v) for k, v in route.items()
> +                        if v not in ("", None))
> +    if 'destination' in normal_route:
> +        normal_route['network'] = normal_route['destination']
> +        del normal_route['destination']
> +
> +    normal_route.update(
> +        _normalize_net_keys(
> +            normal_route, address_keys=('network', 'destination')))
> +
> +    metric = normal_route.get('metric')
> +    if metric:
> +        try:
> +            normal_route['metric'] = int(metric)
> +        except ValueError:
> +            raise TypeError(
> +                'Route config metric {} is not an integer'.format(metric))
> +    return normal_route
> +
> +
> +def _normalize_subnets(subnets):
> +    if not subnets:
> +        subnets = []
> +    return [_normalize_subnet(s) for s in subnets]
> +
> +
> +def is_ipv6_addr(address):

fixed.

> +    if not address:
> +        return False
> +    return ":" in str(address)
> +
> +
>  def subnet_is_ipv6(subnet):
>      """Common helper for checking network_state subnets for ipv6."""
>      # 'static6' or 'dhcp6'


-- 
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324677
Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:cleanup/mask2cidr into cloud-init:master.