Merge ~chad.smith/cloud-init:ec2-dhcp-local-ipv4 into cloud-init:master

[Chad Smith]

EC2: Limit network config to fallback nic, fix local-ipv4 only instances.

VPC instances have the option to specific local only IPv4 addresses. Allow
Ec2Datasource to enable dhcp4 on instances even if local-ipv4s is
configured on an instance.

Also limit network_configuration to only the primary (fallback) nic.

LP: #1728152

EC2: Use the fallback nic for network configuration. Limit config to 1 nic

VPC instances have the option to specific local only IPv4 addresses. Allow
Ec2Datasource to enable dhcp4 on instances even if local-ipv4s is
configured on an instance. Also limit network_configuration to only the
primary (fallback) nic.

LP: #1728152

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:dacb86f7cc6976c1154c5a2171500e226cc04a8e
https://jenkins.ubuntu.com/server/job/cloud-init-ci/442/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/442/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

Thanks Chad.

review: Approve

[Ryan Harper (raharper)]

I'm a bit confused here w.r.t testing for the presense of public or private ips and resulting in configuring DHCPv4.

I'm looking in the Amazon EC2 docs to see (and maybe it's worth looking into an Amazon Linux instance here) but; if we're being provided with a specific IP (or list of ips) then instead of configuring DHCPv4 shouldn't we just configuring statically?

Also, it seems that we should at a minimum provide a fallback on "dhcp on 'eth0'" equivalent that we had prior to the IPv6 check.

b6e0d54... by Chad Smith on 2017-10-30

WIP: limit EC2.network_config to just the fallback nic

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:b6e0d54c171ad3a801ef0aab2e8f30bd5a8f0daf
https://jenkins.ubuntu.com/server/job/cloud-init-ci/446/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/446/rebuild

review: Needs Fixing (continuous-integration)

[Scott Moser (smoser)]

one minor comment.

f5b4601... by Chad Smith on 2017-10-30

add multiple nic definitions to ec2 datasource tests. More unit tests for fallback_nic parameter to conver_ec2_metadata_network_config

b2064bf... by Chad Smith on 2017-10-30

flakes

[Ryan Harper (raharper)]

I would like to see a bit of re-wording on the commit.

The primary change is to use the fallback-nic and always configure DHCP4 on it;
that was the change in behavior that really needs to be resolved.

Secondarily, under VPC which only use private-ips, we need to check if there are
interfaces there.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:b2064bf6eeb2a96d50051ed4e0203e3c7dcb5a44
https://jenkins.ubuntu.com/server/job/cloud-init-ci/447/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/447/rebuild

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

Just tested on cloud-init with the following:

git clone <email address hidden>:cloud-init/qa-scripts.git

git clone https://github.com/smoser/talk-simplestreams.git
export PATH=$PATH:qa-scripts/scripts:talk-simplestreams/bin

./scripts/launch-ec2 --keep-alive --deb-file ~/src/server/cloud-init/cloud-init/cloud-init_17.1-30-gb2064bf6-1~bddeb_all.deb

ssh -i ~/Downloads/ubuntu.pem ubuntu@<ec2_instance>

ubuntu@ip-172-31-28-42:~$ dpkg-query --show cloud-init
cloud-init 17.1-30-gb2064bf6-1~bddeb
ubuntu@ip-172-31-28-42:~$ cat /run/cloud-init/result.json
{
 "v1": {
  "datasource": "DataSourceEc2Local",
  "errors": []
 }
}

[Chad Smith (chad.smith)]

ubuntu@ip-172-31-28-42:~$ cat /etc/network/interfaces.d/50-cloud-init.cfg
# This file is generated from information provided by
# the datasource. Changes to it will not persist across an instance.
# To disable cloud-init's network configuration capabilities, write a file
# /etc/cloud/cloud.cfg.d/99-disable-network-config.cfg with the following:
# network: {config: disabled}
auto lo
iface lo inet loopback

auto eth0
iface eth0 inet dhcp

ac118ac... by Chad Smith on 2017-10-31

address review comments. Call maybe_perform_dhcp_discovery with the fallback_nic, drop accidentaly whitespace

[Scott Moser (smoser)]

I adjusted the commit message a bit, but pending a c-i bot approving, then go ahead and merge.

thanks.

review: Approve

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:ac118ac287b0113b16b9243403348f15437c9b1c
https://jenkins.ubuntu.com/server/job/cloud-init-ci/449/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/449/rebuild

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

Validated latest on EC2
ubuntu@ip-172-31-21-76:~$ dpkg-query --show cloud-init
cloud-init 17.1-27-geb292c18-1~bddeb
ubuntu@ip-172-31-21-76:~$ cat /run/cloud-init/result.json
{
 "v1": {
  "datasource": "DataSourceEc2Local",
  "errors": []
 }
}
ubuntu@ip-172-31-21-76:~$ grep Trace /var/log/cloud-init.log

diff --git a/cloudinit/sources/DataSourceEc2.py b/cloudinit/sources/DataSourceEc2.py
index 41367a8..0ef2217 100644
--- a/cloudinit/sources/DataSourceEc2.py
+++ b/cloudinit/sources/DataSourceEc2.py
@@ -64,6 +64,9 @@ class DataSourceEc2(sources.DataSource):
     # Whether we want to get network configuration from the metadata service.
     get_network_metadata = False
 
+    # Track the discovered fallback nic for use in configuration generation.
+    fallback_nic = None
+
     def __init__(self, sys_cfg, distro, paths):
         sources.DataSource.__init__(self, sys_cfg, distro, paths)
         self.metadata_address = None
@@ -89,16 +92,18 @@ class DataSourceEc2(sources.DataSource):
         elif self.cloud_platform == Platforms.NO_EC2_METADATA:
             return False
 
+        self.fallback_nic = net.find_fallback_nic()
         if self.get_network_metadata:  # Setup networking in init-local stage.
             if util.is_FreeBSD():
                 LOG.debug("FreeBSD doesn't support running dhclient with -sf")
                 return False
-            dhcp_leases = dhcp.maybe_perform_dhcp_discovery()
+            dhcp_leases = dhcp.maybe_perform_dhcp_discovery(self.fallback_nic)
             if not dhcp_leases:
                 # DataSourceEc2Local failed in init-local stage. DataSourceEc2
                 # will still run in init-network stage.
                 return False
             dhcp_opts = dhcp_leases[-1]
+            self.fallback_nic = dhcp_opts.get('interface')
             net_params = {'interface': dhcp_opts.get('interface'),
                           'ip': dhcp_opts.get('fixed-address'),
                           'prefix_or_mask': dhcp_opts.get('subnet-mask'),
@@ -297,8 +302,13 @@ class DataSourceEc2(sources.DataSource):
 
         result = None
         net_md = self.metadata.get('network')
+        # Limit network configuration to only the primary/fallback nic
+        macs_to_nics = {
+            net.get_interface_mac(self.fallback_nic): self.fallback_nic}
         if isinstance(net_md, dict):
-            result = convert_ec2_metadata_network_config(net_md)
+            result = convert_ec2_metadata_network_config(
+                net_md, macs_to_nics=macs_to_nics,
+                fallback_nic=self.fallback_nic)
         else:
             LOG.warning("unexpected metadata 'network' key not valid: %s",
                         net_md)
@@ -458,15 +468,18 @@ def _collect_platform_data():
     return data
 
 
-def convert_ec2_metadata_network_config(network_md, macs_to_nics=None):
+def convert_ec2_metadata_network_config(network_md, macs_to_nics=None,
+                                        fallback_nic=None):
     """Convert ec2 metadata to network config version 1 data dict.
 
     @param: network_md: 'network' portion of EC2 metadata.
        generally formed as {"interfaces": {"macs": {}} where
        'macs' is a dictionary with mac address as key and contents like:
        {"device-number": "0", "interface-id": "...", "local-ipv4s": ...}
-    @param: macs_to_name: Optional dict mac addresses and the nic name. If
+    @param: macs_to_nics: Optional dict of mac addresses and nic names. If
        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.
 
     @return A dict of network config version 1 based on the metadata and macs.
     """
@@ -480,7 +493,8 @@ def convert_ec2_metadata_network_config(network_md, macs_to_nics=None):
             continue  # Not a physical nic represented in metadata
         nic_cfg = {'type': 'physical', 'name': nic_name, 'subnets': []}
         nic_cfg['mac_address'] = mac
-        if nic_metadata.get('public-ipv4s'):
+        if (nic_name == fallback_nic or nic_metadata.get('public-ipv4s') or
+                nic_metadata.get('local-ipv4s')):
             nic_cfg['subnets'].append({'type': 'dhcp4'})
         if nic_metadata.get('ipv6s'):
             nic_cfg['subnets'].append({'type': 'dhcp6'})
diff --git a/tests/unittests/test_datasource/test_ec2.py b/tests/unittests/test_datasource/test_ec2.py
index a7301db..6af699a 100644
--- a/tests/unittests/test_datasource/test_ec2.py
+++ b/tests/unittests/test_datasource/test_ec2.py
@@ -51,6 +51,29 @@ DEFAULT_METADATA = {
                     "vpc-ipv4-cidr-block": "172.31.0.0/16",
                     "vpc-ipv4-cidr-blocks": "172.31.0.0/16",
                     "vpc-ipv6-cidr-blocks": "2600:1f16:aeb:b200::/56"
+                },
+                "06:17:04:d7:26:0A": {
+                    "device-number": "1",   # Only IPv4 local config
+                    "interface-id": "eni-e44ef49f",
+                    "ipv4-associations": {"": "172.3.3.16"},
+                    "ipv6s": "",  # No IPv6 config
+                    "local-hostname": ("ip-172-3-3-16.us-east-2."
+                                       "compute.internal"),
+                    "local-ipv4s": "172.3.3.16",
+                    "mac": "06:17:04:d7:26:0A",
+                    "owner-id": "950047163771",
+                    "public-hostname": ("ec2-172-3-3-16.us-east-2."
+                                        "compute.amazonaws.com"),
+                    "public-ipv4s": "",  # No public ipv4 config
+                    "security-group-ids": "sg-5a61d333",
+                    "security-groups": "wide-open",
+                    "subnet-id": "subnet-20b8565b",
+                    "subnet-ipv4-cidr-block": "172.31.16.0/20",
+                    "subnet-ipv6-cidr-blocks": "",
+                    "vpc-id": "vpc-87e72bee",
+                    "vpc-ipv4-cidr-block": "172.31.0.0/16",
+                    "vpc-ipv4-cidr-blocks": "172.31.0.0/16",
+                    "vpc-ipv6-cidr-blocks": ""
                 }
             }
         }
@@ -209,12 +232,20 @@ class TestEc2(test_helpers.HttprettyTestCase):
 
     @httpretty.activate
     def test_network_config_property_returns_version_1_network_data(self):
-        """network_config property returns network version 1 for metadata."""
+        """network_config property returns network version 1 for metadata.
+
+        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=DEFAULT_METADATA)
-        ds.get_data()
+        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 = '06:17:04:d7:26:09'  # Defined in DEFAULT_METADATA
         expected = {'version': 1, 'config': [
             {'mac_address': '06:17:04:d7:26:09', 'name': 'eth9',
@@ -222,9 +253,48 @@ class TestEc2(test_helpers.HttprettyTestCase):
              'type': 'physical'}]}
         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)
+
+    @httpretty.activate
+    def test_network_config_property_set_dhcp4_on_private_ipv4(self):
+        """network_config property configures dhcp4 on private ipv4 nics.
+
+        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=DEFAULT_METADATA)
+        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 = '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')
         with mock.patch(patch_path) as m_get_interfaces_by_mac:
-            m_get_interfaces_by_mac.return_value = {mac1: 'eth9'}
-            self.assertEqual(expected, ds.network_config)
+            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_is_cached_in_datasource(self):
         """network_config property is cached in DataSourceEc2."""
@@ -321,9 +391,11 @@ class TestEc2(test_helpers.HttprettyTestCase):
 
     @httpretty.activate
     @mock.patch('cloudinit.net.EphemeralIPv4Network')
+    @mock.patch('cloudinit.net.find_fallback_nic')
     @mock.patch('cloudinit.net.dhcp.maybe_perform_dhcp_discovery')
     @mock.patch('cloudinit.sources.DataSourceEc2.util.is_FreeBSD')
-    def test_ec2_local_performs_dhcp_on_non_bsd(self, m_is_bsd, m_dhcp, m_net):
+    def test_ec2_local_performs_dhcp_on_non_bsd(self, m_is_bsd, m_dhcp,
+                                                m_fallback_nic, m_net):
         """Ec2Local returns True for valid platform data on non-BSD with dhcp.
 
         DataSourceEc2Local will setup initial IPv4 network via dhcp discovery.
@@ -331,6 +403,7 @@ class TestEc2(test_helpers.HttprettyTestCase):
         When the platform data is valid, return True.
         """
 
+        m_fallback_nic.return_value = 'eth9'
         m_is_bsd.return_value = False
         m_dhcp.return_value = [{
             'interface': 'eth9', 'fixed-address': '192.168.2.9',
@@ -344,7 +417,7 @@ class TestEc2(test_helpers.HttprettyTestCase):
 
         ret = ds.get_data()
         self.assertTrue(ret)
-        m_dhcp.assert_called_once_with()
+        m_dhcp.assert_called_once_with('eth9')
         m_net.assert_called_once_with(
             broadcast='192.168.2.255', interface='eth9', ip='192.168.2.9',
             prefix_or_mask='255.255.255.0', router='192.168.2.1')
@@ -389,6 +462,57 @@ class TestConvertEc2MetadataNetworkConfig(test_helpers.CiTestCase):
             ec2.convert_ec2_metadata_network_config(
                 network_metadata_ipv6, macs_to_nics))
 
+    def test_convert_ec2_metadata_network_config_handles_local_dhcp4(self):
+        """Config dhcp4 when there are no public addresses in public-ipv4s."""
+        macs_to_nics = {self.mac1: 'eth9'}
+        network_metadata_ipv6 = copy.deepcopy(self.network_metadata)
+        nic1_metadata = (
+            network_metadata_ipv6['interfaces']['macs'][self.mac1])
+        nic1_metadata['local-ipv4s'] = '172.3.3.15'
+        nic1_metadata.pop('public-ipv4s')
+        expected = {'version': 1, 'config': [
+            {'mac_address': self.mac1, 'type': 'physical',
+             'name': 'eth9', 'subnets': [{'type': 'dhcp4'}]}]}
+        self.assertEqual(
+            expected,
+            ec2.convert_ec2_metadata_network_config(
+                network_metadata_ipv6, macs_to_nics))
+
+    def test_convert_ec2_metadata_network_config_handles_absent_dhcp4(self):
+        """Config dhcp4 on fallback_nic when there are no ipv4 addresses."""
+        macs_to_nics = {self.mac1: 'eth9'}
+        network_metadata_ipv6 = copy.deepcopy(self.network_metadata)
+        nic1_metadata = (
+            network_metadata_ipv6['interfaces']['macs'][self.mac1])
+        nic1_metadata['public-ipv4s'] = ''
+
+        # When no ipv4 or ipv6 content but fallback_nic set, set dhcp4 config.
+        expected = {'version': 1, 'config': [
+            {'mac_address': self.mac1, 'type': 'physical',
+             'name': 'eth9', 'subnets': [{'type': 'dhcp4'}]}]}
+        self.assertEqual(
+            expected,
+            ec2.convert_ec2_metadata_network_config(
+                network_metadata_ipv6, macs_to_nics, fallback_nic='eth9'))
+
+    def test_convert_ec2_metadata_network_config_handles_local_v4_and_v6(self):
+        """When dhcp6 is public and dhcp4 is set to local enable both."""
+        macs_to_nics = {self.mac1: 'eth9'}
+        network_metadata_both = copy.deepcopy(self.network_metadata)
+        nic1_metadata = (
+            network_metadata_both['interfaces']['macs'][self.mac1])
+        nic1_metadata['ipv6s'] = '2620:0:1009:fd00:e442:c88d:c04d:dc85/64'
+        nic1_metadata.pop('public-ipv4s')
+        nic1_metadata['local-ipv4s'] = '10.0.0.42'  # Local ipv4 only on vpc
+        expected = {'version': 1, 'config': [
+            {'mac_address': self.mac1, 'type': 'physical',
+             'name': 'eth9',
+             'subnets': [{'type': 'dhcp4'}, {'type': 'dhcp6'}]}]}
+        self.assertEqual(
+            expected,
+            ec2.convert_ec2_metadata_network_config(
+                network_metadata_both, macs_to_nics))
+
     def test_convert_ec2_metadata_network_config_handles_dhcp4_and_dhcp6(self):
         """Config both dhcp4 and dhcp6 when both vpc-ipv6 and ipv4 exists."""
         macs_to_nics = {self.mac1: 'eth9'}