Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master
Hi Andrea, this branch was migrated over to GitHub (along with the rest of cloud-init's code hosting :) and was proposed here: https://github.com/canonical/cloud-init/pull/114 It landed in March and was included in the 20.2 cloud-init release. It is available in Ubuntu 20.04 (Focal Fossa), and will be backported to all stable Ubuntu releases (i.e. 16.04 and later) in the next couple of weeks. Thanks for your interest! -- 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
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master
Up! Any news here? Which is the blocker here? -- 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
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master
Inline comment asking for clarification on AWS DHCP lease contents,
requirements for routing traffic to IMDS, DNS, and off-box, as with Azure.
Let's document what's needed; we may need to add secondary ips with a metric.
See
https://github.com/aws/ec2-net-utils/blob/master/ec2net-functions
For AmazonLinux net implementation; I do see some setting of source_ip and
route table/metrics in use.
Diff comments:
> diff --git a/tests/unittests/test_datasource/test_ec2.py
> b/tests/unittests/test_datasource/test_ec2.py
> index 20d59bf..8ed4c18 100644
> --- a/tests/unittests/test_datasource/test_ec2.py
> +++ b/tests/unittests/test_datasource/test_ec2.py
> @@ -302,21 +385,45 @@ class TestEc2(test_helpers.HttprettyTestCase):
> platform_data=self.valid_platform_data,
> sys_cfg={'datasource': {'Ec2': {'strict_id': True}}},
> md={'md': DEFAULT_METADATA})
> -find_fallback_path = (
> -'cloudinit.sources.DataSourceEc2.net.find_fallback_nic')
> +find_fallback_path = M_PATH_NET + 'find_fallback_nic'
> with mock.patch(find_fallback_path) as m_find_fallback:
> m_find_fallback.return_value = 'eth9'
> 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'}]}
> -patch_path = (
> -'cloudinit.sources.DataSourceEc2.net.get_interfaces_by_mac')
> -get_interface_mac_path = (
> -'cloudinit.sources.DataSourceEc2.net.get_interface_mac')
> +expected = {'version': 2, 'ethernets': {'eth9': {
> +'match': {'macaddress': mac1.lower()}, 'set-name': 'eth9',
> +'dhcp4': True}}}
> +patch_path = M_PATH_NET + 'get_interfaces_by_mac'
> +get_interface_mac_path = M_PATH_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.
> +"""
> +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 = M_PATH_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}}}
Do we know if:
1) dhcp response from AWS DHCP server includes classless static routes?
2) if the secondary IPs have to be from the same subnet as the primary
interface (DHCP)?
3) if the IMDS, DNS, or off-box routing requires the source-ip to be the value
from DHCP (Note, I think we know that IMDS requires the source-ip to be from
the DHCP response, IIRC).
> +patch_path = M_PATH_NET + 'get_interfaces_by_mac'
> +get_interface_mac_path = M_PATH_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:
--
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
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master
I think Chad meant after our next SRU[1], which will begin shortly. I'd expect this come in the SRU after the current planned one. 1. https://wiki.ubuntu.com/StableReleaseUpdates -- 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
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master
> Waiting on this change of behavior until after 19.2 upstream release cloud-init 19.2 was released 17 July. Does this unblock this change being applied? -- 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
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master has been updated. Status: Needs review => Work in progress For more details, see: 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
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master
Chad Smith has proposed merging
~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master.
Commit message:
ec2: render secondary IPs on primary nic when present in metadata
Parse local-ipv4s and subnet-ipv4-cidr-block on EC2 metadata version
2018-09-24 to obtain secondary nic private IPs and network mask for
the primary nic.
In adding this feature, convert DataSourceEc2.network_config to
emit network version 2 instead of version 1.
To allow for retaining original network config behavior on earlier
distribution series, surface a datasource config option
configure_secondary_ips which defaults to True on tip.
Older/stable distribution series will set configure_secondary_ips
default to False.
Requested reviews:
Server Team CI bot (server-team-bot): continuous-integration
cloud-init commiters (cloud-init-dev)
For more details, see:
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.
diff --git a/cloudinit/sources/DataSourceEc2.py b/cloudinit/sources/DataSourceEc2.py
index 5c017bf..f39b73f 100644
--- a/cloudinit/sources/DataSourceEc2.py
+++ b/cloudinit/sources/DataSourceEc2.py
@@ -54,7 +54,7 @@ class DataSourceEc2(sources.DataSource):
# Priority ordered list of additional metadata versions which will be tried
# for extended metadata content. IPv6 support comes in 2016-09-02
-extended_metadata_versions = ['2016-09-02']
+extended_metadata_versions = ['2018-09-24', '2016-09-02']
# Setup read_url parameters per get_url_params.
url_max_wait = 120
@@ -332,8 +332,13 @@ class DataSourceEc2(sources.DataSource):
macs_to_nics = {net.get_interface_mac(iface): iface}
net_md = self.metadata.get('network')
if isinstance(net_md, dict):
+# SRU_BLOCKER: xenial, bionic and disco should default
+# configure_secondary_ips to False to retain original behavior on
+# those releases.
result = convert_ec2_metadata_network_config(
-net_md, macs_to_nics=macs_to_nics, fallback_nic=iface)
+net_md, macs_to_nics=macs_to_nics, fallback_nic=iface,
+config_secondary_ips=util.is_true(
+self.ds_cfg.get('configure_secondary_ips', True)))
# RELEASE_BLOCKER: xenial should drop the below if statement,
# because the issue being addressed doesn't exist pre-netplan.
@@ -373,7 +378,7 @@ class DataSourceEc2(sources.DataSource):
if not self.wait_for_metadata_service():
return {}
api_version = self.get_metadata_api_version()
-crawled_metadata = {}
+crawled_metadata = {'_metadata_api_version': api_version}
try:
crawled_metadata['user-data'] = ec2.get_instance_userdata(
api_version, self.metadata_address)
@@ -388,7 +393,6 @@ class DataSourceEc2(sources.DataSource):
LOG, "Failed reading from metadata address %s",
self.metadata_address)
return {}
-crawled_metadata['_metadata_api_version'] = api_version
return crawled_metadata
@@ -523,8 +527,9 @@ def _collect_platform_data():
return data
-def convert_ec2_metadata_network_config(network_md, macs_to_nics=None,
-fallback_nic=None):
+def convert_ec2_metadata_network_config(
+network_md, macs_to_nics=None, fallback_nic=None,
+config_secondary_ips=True):
"""Convert ec2 metadata to network config version 1 data dict.
@param: network_md: 'network' portion of EC2 metadata.
@@ -535,25 +540,52 @@ def convert_ec2_metadata_network_config(network_md, macs_to_nics=None,
not provided, get_interfaces_by_mac is called to get it from the OS.
@param: fallback_nic: Optionally provide the primary nic interface name.
This nic will be guaranteed to minimally have a dhcp4 configuration.
+@param: config_secondary_ips: Boolean set True to configure any
+ secondary IPs described by the metadata service.
-@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
-if (nic_name == fallback_nic or
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master
Waiting on this change of behavior until after 19.2 upstream release -- 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
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master has been updated. Status: Needs review => Work in progress For more details, see: 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
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master
Review: Approve continuous-integration PASSED: Continuous integration, rev:2ae7c7c3b3ee6522f31a4efc6b993e43e82c5bb9 https://jenkins.ubuntu.com/server/job/cloud-init-ci/765/ Executed test runs: SUCCESS: Checkout SUCCESS: Unit & Style Tests SUCCESS: Ubuntu LTS: Build SUCCESS: Ubuntu LTS: Integration IN_PROGRESS: Declarative: Post Actions Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/765/rebuild -- 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
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master
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
> +if isinstance(local_ipv4s, list) and len(local_ipv4s) > 1:
> +if dev_config.get('addresses') is None:
right again. dropped conditional
> +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('/')
Instead checking len(subnet_cidr.split('/')) != 2 in the warning conditional
above.
> +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')
done
> +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.
copy-paste error, dropping that from the docstring.
> +"""
> +ds = self._setup_ds(
> +platform_data=self.valid_platform_data,
> +sys_cfg={'datasource': {'Ec2': {'strict_id': True}}},
> +
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master
Review: Needs Fixing continuous-integration FAILED: Continuous integration, rev:047a654e9b7ca8dd8115aab2467c55f25ba312ca https://jenkins.ubuntu.com/server/job/cloud-init-ci/762/ Executed test runs: SUCCESS: Checkout FAILED: Unit & Style Tests Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/762/rebuild -- 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
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master has been updated. Commit message changed to: ec2: render secondary IPs on primary nic when present in metadata Parse local-ipv4s and subnet-ipv4-cidr-block on EC2 metadata version 2018-09-24 to obtain secondary nic private IPs and network mask for the primary nic. In adding this feature, convert DataSourceEc2.network_config to emit network version 2 instead of version 1. To allow for retaining original network config behavior on earlier distribution series, surface a datasource config option configure_secondary_ips which defaults to True on tip. Older/stable distribution series will set configure_secondary_ips default to False. For more details, see: 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
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master has been updated. Commit message changed to: ec2: render secondary IPs on primary nic when present in metadata Parse local-ipv4s and subnet-ipv4-cidr-block on EC2 metadata version 2018-09-24 to obtain secondary nic private IPs and network mask for the primary nic. In adding this feature, convert DataSourceEc2.network_config to emit network version 2 instead of version 1. To allow for retaining original network config behavior on earlier distribution series, surface a datasource config option configure_secondary_ips which defaults to True on tip. Older distribution series will set configure_secondary_ips default to False. For more details, see: 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
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master
Review: Approve continuous-integration PASSED: Continuous integration, rev:db953566a9e6df91b4eaeebb813e45726136316c https://jenkins.ubuntu.com/server/job/cloud-init-ci/761/ Executed test runs: SUCCESS: Checkout SUCCESS: Unit & Style Tests SUCCESS: Ubuntu LTS: Build SUCCESS: Ubuntu LTS: Integration IN_PROGRESS: Declarative: Post Actions Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/761/rebuild -- 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
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master
Review: Approve continuous-integration PASSED: Continuous integration, rev:f6b0d9eb283917f4c2a5a5f863ad07c048346199 https://jenkins.ubuntu.com/server/job/cloud-init-ci/759/ Executed test runs: SUCCESS: Checkout SUCCESS: Unit & Style Tests SUCCESS: Ubuntu LTS: Build SUCCESS: Ubuntu LTS: Integration IN_PROGRESS: Declarative: Post Actions Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/759/rebuild -- 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
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master
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,
I wonder if we should keep both versions of the convert here and specify the
version in the signature?
> @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
> +if isinstance(local_ipv4s, list) and len(local_ipv4s) > 1:
> +if dev_config.get('addresses') is None:
> +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('/')
> +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
> @@ -277,10 +365,9 @@ class TestEc2(test_helpers.HttprettyTestCase):
> ds.get_data()
>
> mac1 = '06:17:04:d7:26:09' # Defined in DEFAULT_METADATA
> -expected = {'version': 1, 'config': [
> -{'mac_address': '06:17:04:d7:26:09', 'name': 'eth9',
> - 'subnets': [{'type': 'dhcp4'}, {'type': 'dhcp6'}],
> - 'type': 'physical'}]}
> +expected = {'version': 2, 'ethernets': {'eth9': {
> +'match': {'macaddress': '06:17:04:d7:26:09'}, 'set-name': 'eth9',
> +'dhcp4': True, 'dhcp6': True}}}
If we keep both versions of the parser, then we can add a variant to the test
calling with with v1 and v2?
> patch_path = (
> 'cloudinit.sources.DataSourceEc2.net.get_interfaces_by_mac')
> get_interface_mac_path = (
> @@ -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')
I suggest a class attribute or global for the prefix here:
net_path = 'cloudinit.sources.DataSourceEc2.net'
by_mac = mock.patch(net_path + '.get_interfaces_by_mac'
> +get_interface_mac_path = (
> +
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master
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
Re: [Cloud-init-dev] [Merge] ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master
Diff comments:
> diff --git a/cloudinit/sources/DataSourceEc2.py
> b/cloudinit/sources/DataSourceEc2.py
> index 5c017bf..aef5d80 100644
> --- a/cloudinit/sources/DataSourceEc2.py
> +++ b/cloudinit/sources/DataSourceEc2.py
> @@ -536,24 +536,35 @@ 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
> +if isinstance(local_ipv4s, list) and len(local_ipv4s) > 1:
Added the comment # In version < 2018-09-24 local_ipvs is a str
with a single IP
> +subnet_cidr = nic_metadata.get('subnet-ipv4-cidr-block')
> +_ip, prefix = subnet_cidr.split('/')
checking now and wawrning if we see it + default to /32 secondary IP setting
which will at least preserve a strict route and the desired secondary IP.
default routes should still have proper connectivity on preferred primary IP.
> +for secondary_ip in local_ipv4s[1:]:
> +if dev_config.get('addresses') is None:
yes and done.
> +dev_config['addresses'] = []
> +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': nic_metadata.get('mac').lower()},
oops, don't really even need that as we already have mac local variable from
our outer macs_to_nics for loop.
> +'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..c99f58f 100644
> --- a/tests/unittests/test_datasource/test_ec2.py
> +++ b/tests/unittests/test_datasource/test_ec2.py
> @@ -112,6 +112,94 @@ DEFAULT_METADATA = {
> "services": {"domain": "amazonaws.com", "partition": "aws"},
> }
>
> +# collected from api version 2016-09-02/ with
Correct, fixed to 2018-09-24.
> +# python3 -c 'import json
> +# from cloudinit.ec2_utils import get_instance_metadata as gm
> +# print(json.dumps(gm("2018-09-24"), indent=1, sort_keys=True))'
> +DEFAULT_METADATA_2018_09_24 = {
Good point, changed it to SECONDARY_IP_METADATA_2018_09_24
> +"ami-id": "ami-0986c2ac728528ac2",
> +"ami-launch-index": "0",
> +"ami-manifest-path": "(unknown)",
> +"block-device-mapping": {
> +"ami": "/dev/sda1",
> +"root": "/dev/sda1"
> +},
> +"events": {
> +"maintenance": {
> +"history": "[]",
> +"scheduled": "[]"
> +}
> +},
> +"hostname": "ip-172-31-44-13.us-east-2.compute.internal",
> +"identity-credentials": {
> +"ec2": {
> +"info": {
> +"AccountId": "329910648901",
> +"Code": "Success",
> +"LastUpdated": "2019-07-06T14:22:56Z"
> +}
> +}
> +},
> +"instance-action": "none",
> +"instance-id": "i-069e01e8cc43732f8",
> +"instance-type": "t2.micro",
> +"local-hostname": "ip-172-31-44-13.us-east-2.compute.internal",
> +"local-ipv4": "172.31.44.13",
> +"mac": "0a:07:84:3d:6e:38",
> +"metrics": {
> +"vhostmd": ""
> +},
> +"network": {
> +"interfaces": {
> +"macs": {
> +"0a:07:84:3d:6e:38": {
> +"device-number": "0",
> +"interface-id": "eni-0d6335689899ce9cc",
> +"ipv4-associations": {
> +"18.218.219.181": "172.31.44.13"
> +
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master
The proposal to merge ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master has been updated. Commit message changed to: ec2: render secondary IPs on primary nic when present in metadata Parse local-ipv4s and subnet-ipv4-cidr-block on EC2 metadata version 2018-09-24 to obtain secondary nic private IPs and network mask for the primary nic. In adding this feature, convert DataSourceEc2.network_configu to emit network version 2 instead of version 1. For more details, see: 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
[Cloud-init-dev] [Merge] ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master
Chad Smith has proposed merging
~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master.
Commit message:
ec2: render secondary IPs on primary nic when present in metadata
Parse local-ipv4s and subnet-ipv4-cidr-block on EC2 metadata version
2018-09-24 to obtain secondary nic private IPs and network mask for
the primary nic.
Requested reviews:
cloud-init commiters (cloud-init-dev)
For more details, see:
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.
diff --git a/cloudinit/sources/DataSourceEc2.py b/cloudinit/sources/DataSourceEc2.py
index 5c017bf..aef5d80 100644
--- a/cloudinit/sources/DataSourceEc2.py
+++ b/cloudinit/sources/DataSourceEc2.py
@@ -54,7 +54,7 @@ class DataSourceEc2(sources.DataSource):
# Priority ordered list of additional metadata versions which will be tried
# for extended metadata content. IPv6 support comes in 2016-09-02
-extended_metadata_versions = ['2016-09-02']
+extended_metadata_versions = ['2018-09-24', '2016-09-02']
# Setup read_url parameters per get_url_params.
url_max_wait = 120
@@ -536,24 +536,35 @@ 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
+if isinstance(local_ipv4s, list) and len(local_ipv4s) > 1:
+subnet_cidr = nic_metadata.get('subnet-ipv4-cidr-block')
+_ip, prefix = subnet_cidr.split('/')
+for secondary_ip in local_ipv4s[1:]:
+if dev_config.get('addresses') is None:
+dev_config['addresses'] = []
+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': nic_metadata.get('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..c99f58f 100644
--- a/tests/unittests/test_datasource/test_ec2.py
+++ b/tests/unittests/test_datasource/test_ec2.py
@@ -112,6 +112,94 @@ DEFAULT_METADATA = {
"services": {"domain": "amazonaws.com", "partition": "aws"},
}
+# collected from api version 2016-09-02/ with
+# python3 -c 'import json
+# from cloudinit.ec2_utils import get_instance_metadata as gm
+# print(json.dumps(gm("2018-09-24"), indent=1, sort_keys=True))'
+DEFAULT_METADATA_2018_09_24 = {
+"ami-id": "ami-0986c2ac728528ac2",
+"ami-launch-index": "0",
+"ami-manifest-path": "(unknown)",
+"block-device-mapping": {
+"ami": "/dev/sda1",
+"root": "/dev/sda1"
+},
+"events": {
+"maintenance": {
+"history": "[]",
+"scheduled": "[]"
+}
+},
+"hostname": "ip-172-31-44-13.us-east-2.compute.internal",
+"identity-credentials": {
+"ec2": {
+"info": {
+"AccountId": "329910648901",
+"Code": "Success",
+"LastUpdated": "2019-07-06T14:22:56Z"
+}
+}
+},
+"instance-action": "none",
+"instance-id": "i-069e01e8cc43732f8",
+"instance-type": "t2.micro",
+"local-hostname": "ip-172-31-44-13.us-east-2.compute.internal",
+"local-ipv4": "172.31.44.13",
+"mac": "0a:07:84:3d:6e:38",
+"metrics": {
+"vhostmd": ""
+},
+"network": {
+"interfaces": {
+
