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
