cloud-init-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

Diff comments:

> diff --git a/cloudinit/net/network_state.py b/cloudinit/net/network_state.py
> index db3c357..9def76d 100644
> --- a/cloudinit/net/network_state.py
> +++ b/cloudinit/net/network_state.py
> @@ -289,19 +289,16 @@ class NetworkStateInterpreter(object):
>              iface.update({param: val})
>  
>          # convert subnet ipv6 netmask to cidr as needed
> -        subnets = command.get('subnets')
> -        if subnets:
> +        subnets = _normalize_subnets(command.get('subnets'))
> +        print("normalized subnets: %s" % subnets)

drop the debugging print

> +
> +        # automatically set 'use_ipv6' if any addresses are ipv6
> +        if not self.use_ipv6:
>              for subnet in subnets:
> -                if subnet['type'] == 'static':
> -                    if ':' in subnet['address']:
> -                        self.use_ipv6 = True
> -                    if 'netmask' in subnet and ':' in subnet['address']:
> -                        subnet['netmask'] = mask2cidr(subnet['netmask'])
> -                        for route in subnet.get('routes', []):
> -                            if 'netmask' in route:
> -                                route['netmask'] = mask2cidr(route['netmask'])
> -                elif subnet['type'].endswith('6'):
> +                if (subnet.get('type').endswith('6') or
> +                        is_ipv6_addr(subnet.get('address'))):
>                      self.use_ipv6 = True
> +                    break
>  
>          iface.update({
>              'name': command.get('name'),
> @@ -692,6 +689,122 @@ 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':

do we also need this for static6?

> +        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=()):
> +
> +    """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.
> +    """
> +    network = copy.deepcopy(network)
> +    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)

A comment of what the expected tokens here are would be nice.
My preference would be for named variables.

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

Add the log.warning ?

> +        net['prefix'] = toks[1]
> +        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

This looks worthy of at least a debug message?  Injecting a netmask if one isn't provided?

> +
> +    if ipv6:
> +        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 if ipv4.
> +       """
> +    route = copy.deepcopy(route)
> +    print("normalizing %s" % route)

remove debugging

> +    # Prune None-value keys
> +    normal_route = dict((k, v) for k, v in route.items() if v)
> +    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))
> +    print("normaled to %s" % normal_route)
> +    return normal_route
> +
> +
> +def _normalize_subnets(subnets):
> +    if not subnets:
> +        subnets = []
> +    return [_normalize_subnet(s) for s in subnets]
> +
> +
> +def is_ipv6_addr(address):
> +    if not address:
> +        return False
> +    return ":" in address
> +
> +
>  def subnet_is_ipv6(subnet):
>      """Common helper for checking network_state subnets for ipv6."""
>      # 'static6' or 'dhcp6'
> @@ -733,6 +869,28 @@ def ipv6mask2cidr(mask):
>      return cidr
>  
>  
> +def mask_to_net_prefix(mask):
> +    """Return the network prefix for the netmask provided.
> +
> +    Supports ipv4 or ipv6 netmasks."""
> +    if ':' in str(mask):
> +        return ipv6_mask_to_net_prefix(mask)
> +    elif '.' in str(mask):
> +        return ipv4_mask_to_net_prefix(mask)
> +

should we raise a ValueError here in case mask has neither a ":" or "." in it? ie, it's not a mask?

> +
> +def ipv4mask2cidr(mask):
> +    return ipv4_mask_to_net_prefix(mask, strict=False)
> +
> +
> +def cidr2mask(cidr):
> +    return net_prefix_to_ipv4_mask(cidr)
> +
> +
> +def ipv6mask2cidr(mask):
> +    return ipv6_mask_to_net_prefix(mask, strict=False)
> +
> +
>  def mask2cidr(mask):
>      if ':' in mask:
>          return ipv6mask2cidr(mask)
> @@ -741,4 +899,5 @@ def mask2cidr(mask):
>      else:
>          return mask
>  
> +

whitespace

>  # vi: ts=4 expandtab
> diff --git a/cloudinit/net/sysconfig.py b/cloudinit/net/sysconfig.py
> index 58c5713..3f917ed 100644
> --- a/cloudinit/net/sysconfig.py
> +++ b/cloudinit/net/sysconfig.py
> @@ -26,11 +26,9 @@ def _make_header(sep='#'):
>  
>  
>  def _is_default_route(route):
> -    if route['network'] == '::' and route['netmask'] == 0:
> -        return True
> -    if route['network'] == '0.0.0.0' and route['netmask'] == '0.0.0.0':
> -        return True
> -    return False
> +    print("checking idr route=%s" % route)

remove debugging

> +    default_nets = ('::', '0.0.0.0')
> +    return route['prefix'] == 0 and route['network'] in default_nets
>  
>  
>  def _quote_value(value):
> @@ -323,22 +321,18 @@ class Renderer(renderer.Renderer):
>                              " " + ipv6_cidr)
>                  else:
>                      ipv4_index = ipv4_index + 1
> -                    if ipv4_index == 0:
> -                        iface_cfg['IPADDR'] = subnet['address']
> -                        if 'netmask' in subnet:
> -                            iface_cfg['NETMASK'] = subnet['netmask']
> -                    else:
> -                        iface_cfg['IPADDR' + str(ipv4_index)] = \
> -                            subnet['address']
> -                        if 'netmask' in subnet:
> -                            iface_cfg['NETMASK' + str(ipv4_index)] = \
> -                                subnet['netmask']
> +                    suff = "" if ipv4_index == 0 else str(ipv4_index)
> +                    iface_cfg['IPADDR' + suff] = subnet['address']
> +                    iface_cfg['NETMASK' + suff] = \
> +                        net_prefix_to_ipv4_mask(subnet['prefix'])
>  
>      @classmethod
>      def _render_subnet_routes(cls, iface_cfg, route_cfg, subnets):
>          for i, subnet in enumerate(subnets, start=len(iface_cfg.children)):
> +            print("subnet=%s" % subnet)
>              for route in subnet.get('routes', []):
>                  is_ipv6 = subnet.get('ipv6')
> +                print("route=%s" % route)

remove debugging here and line 331

>  
>                  if _is_default_route(route):
>                      if (
> diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
> index 5169821..a5a43bd 100644
> --- a/tests/unittests/test_net.py
> +++ b/tests/unittests/test_net.py
> @@ -890,6 +888,7 @@ USERCTL=no
>          macs = {'fa:16:3e:ed:9a:59': 'eth0'}
>          render_dir = self.tmp_dir()
>          network_cfg = openstack.convert_net_json(net_json, known_macs=macs)
> +        print("network_cfg=%s" % network_cfg)

remove debugging

>          ns = network_state.parse_net_config_data(network_cfg,
>                                                   skip_broken=False)
>          renderer = sysconfig.Renderer()


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