Thanks! A couple of follow-ups, and a couple of nits I missed first time around.
Diff comments: > diff --git a/cloudinit/sources/DataSourceEc2.py > b/cloudinit/sources/DataSourceEc2.py > index 5c017bf..9a5ed43 100644 > --- a/cloudinit/sources/DataSourceEc2.py > +++ b/cloudinit/sources/DataSourceEc2.py > @@ -536,24 +536,43 @@ def convert_ec2_metadata_network_config(network_md, > macs_to_nics=None, > @param: fallback_nic: Optionally provide the primary nic interface name. > This nic will be guaranteed to minimally have a dhcp4 configuration. > > - @return A dict of network config version 1 based on the metadata and > macs. > + @return A dict of network config version 2 based on the metadata and > macs. > """ > - netcfg = {'version': 1, 'config': []} > + netcfg = {'version': 2, 'ethernets': {}} > if not macs_to_nics: > macs_to_nics = net.get_interfaces_by_mac() > macs_metadata = network_md['interfaces']['macs'] > for mac, nic_name in macs_to_nics.items(): > + dev_config = {} > nic_metadata = macs_metadata.get(mac) > if not nic_metadata: > continue # Not a physical nic represented in metadata > - nic_cfg = {'type': 'physical', 'name': nic_name, 'subnets': []} > - nic_cfg['mac_address'] = mac > + local_ipv4s = nic_metadata.get('local-ipv4s') > if (nic_name == fallback_nic or nic_metadata.get('public-ipv4s') or > - nic_metadata.get('local-ipv4s')): > - nic_cfg['subnets'].append({'type': 'dhcp4'}) > + local_ipv4s): > + dev_config['dhcp4'] = True > + # In version < 2018-09-24 local_ipvs is a str with a single IP Thanks for this comment! I have the context of the commit message informing me that this about secondary IP addresses, but I wonder if in future people might wonder why we aren't doing anything with the one str IP address; is it worth explicitly mentioning that the next stanza is handling secondary interfaces? > + if isinstance(local_ipv4s, list) and len(local_ipv4s) > 1: > + if dev_config.get('addresses') is None: If we get here, dev_config is, I think, always going to be {'dhcp4': True}. So do we need this to be conditional any longer? > + dev_config['addresses'] = [] > + subnet_cidr = nic_metadata.get('subnet-ipv4-cidr-block') > + if not subnet_cidr or '/' not in subnet_cidr: > + LOG.warning( > + 'Could not parse subnet-ipv4-cidr-block %s.' > + ' Network config for Secondary IPs default to /32', > + subnet_cidr) > + prefix = '32' > + else: > + _ip, prefix = subnet_cidr.split('/') If subnet_cidr has more than one / in it, this will fail with "ValueError: too many values to unpack". (Sorry I didn't catch this the first time around!) > + for secondary_ip in local_ipv4s[1:]: > + dev_config['addresses'].append( > + '{ip}/{prefix}'.format(ip=secondary_ip, > prefix=prefix)) > if nic_metadata.get('ipv6s'): > - nic_cfg['subnets'].append({'type': 'dhcp6'}) > - netcfg['config'].append(nic_cfg) > + dev_config['dhcp6'] = True > + dev_config.update({ > + 'match': {'macaddress': mac.lower()}, > + 'set-name': nic_name}) > + netcfg['ethernets'][nic_name] = dev_config > return netcfg > > > diff --git a/tests/unittests/test_datasource/test_ec2.py > b/tests/unittests/test_datasource/test_ec2.py > index 20d59bf..e031fe9 100644 > --- a/tests/unittests/test_datasource/test_ec2.py > +++ b/tests/unittests/test_datasource/test_ec2.py > @@ -309,10 +396,40 @@ class TestEc2(test_helpers.HttprettyTestCase): > ds.get_data() > > mac1 = '06:17:04:d7:26:0A' # IPv4 only in DEFAULT_METADATA > - expected = {'version': 1, 'config': [ > - {'mac_address': '06:17:04:d7:26:0A', 'name': 'eth9', > - 'subnets': [{'type': 'dhcp4'}], > - 'type': 'physical'}]} > + expected = {'version': 2, 'ethernets': {'eth9': { > + 'match': {'macaddress': '06:17:04:d7:26:0a'}, 'set-name': 'eth9', > + 'dhcp4': True}}} > + patch_path = ( > + 'cloudinit.sources.DataSourceEc2.net.get_interfaces_by_mac') > + get_interface_mac_path = ( > + 'cloudinit.sources.DataSourceEc2.net.get_interface_mac') > + with mock.patch(patch_path) as m_get_interfaces_by_mac: > + with mock.patch(find_fallback_path) as m_find_fallback: > + with mock.patch(get_interface_mac_path) as m_get_mac: > + m_get_interfaces_by_mac.return_value = {mac1: 'eth9'} > + m_find_fallback.return_value = 'eth9' > + m_get_mac.return_value = mac1 > + self.assertEqual(expected, ds.network_config) > + > + def test_network_config_property_secondary_private_ips(self): > + """network_config property configures any secondary ipv4 addresses. > + > + Only one device is configured even when multiple exist in metadata. This test doesn't appear to test the case of having more than one device; am I missing something? > + """ > + ds = self._setup_ds( > + platform_data=self.valid_platform_data, > + sys_cfg={'datasource': {'Ec2': {'strict_id': True}}}, > + md={'md': SECONDARY_IP_METADATA_2018_09_24}) > + find_fallback_path = ( > + 'cloudinit.sources.DataSourceEc2.net.find_fallback_nic') > + with mock.patch(find_fallback_path) as m_find_fallback: > + m_find_fallback.return_value = 'eth9' > + ds.get_data() > + > + mac1 = '0a:07:84:3d:6e:38' # IPv4 with 1 secondary IP > + expected = {'version': 2, 'ethernets': {'eth9': { > + 'match': {'macaddress': mac1}, 'set-name': 'eth9', > + 'addresses': ['172.31.45.70/20'], 'dhcp4': True}}} > patch_path = ( > 'cloudinit.sources.DataSourceEc2.net.get_interfaces_by_mac') > get_interface_mac_path = ( -- https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/369792 Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master. _______________________________________________ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp