Merge ~chad.smith/cloud-init:xenial/dont-render-network-from-imds into cloud-init:ubuntu/xenial

[Chad Smith]

xenial-azure: Disable rendering network from IMDS

In Xenial we already have hotplug scripts delivered in stock
cloud-images. Disable rendering network information from IMDS and avoid
deleting Ubuntu cloud-image udev add scripts which create
/etc/network/interface configuration for the hotplugged network
interfaces.

[Ryan Harper (raharper)]

Are any of the cloud-init cli commands affected if we don't have IMDS data? I was thinking in metadata field outputs?

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

PASSED: Continuous integration, rev:8d6b1280ee7987ebf06eb719e3b94fa4c2559f63
https://jenkins.ubuntu.com/server/job/cloud-init-ci/395/
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/395/rebuild

review: Approve (continuous-integration)

7d41f5a... by Chad Smith on 2018-10-16

debian/changelog update

c416f35... by Chad Smith on 2018-10-16

add xenial debian patch to patch series

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

PASSED: Continuous integration, rev:7d41f5a2c28441d7832a5629a15381986c08c6c7
https://jenkins.ubuntu.com/server/job/cloud-init-ci/396/
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/396/rebuild

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

Just built the deb and validated behavior on Xenial. IMDS is still queried and exists in instance-data.json, but that configuration is not used to return network_config note the v1 in
DatasourceAzure.network_config

sudo python3 -c "from cloudinit.stages import _pkl_load; print(_pkl_load('/var/lib/cloud/instance/obj.pkl').network_config)"
sudo: unable to resolve host SRU-worked-azure
{'version': 1, 'config': [{'mac_address': '00:0d:3a:04:3c:ea', 'type': 'physical', 'subnets': [{'type': 'dhcp'}], 'name': 'eth0', 'params': {'device_id': '0x3', 'driver': 'hv_netvsc'}}]}

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

FAILED: Continuous integration, rev:c416f35766358a432b4c2f4d09ac1ae742d49fe8
https://jenkins.ubuntu.com/server/job/cloud-init-ci/397/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    FAILED: Ubuntu LTS: Build

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

review: Needs Fixing (continuous-integration)

4f6bb1c... by Chad Smith on 2018-10-17

fix mock_fallback.assert_called_once_with instead of called_once()

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

PASSED: Continuous integration, rev:4f6bb1c1d62b23e29c62bf399a6a5554eca72542
https://jenkins.ubuntu.com/server/job/cloud-init-ci/398/
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/398/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

I'm not opposed entirely to this change, and if we want to get this into
16.04 as a quick fix, we can do so.

However I do think we can do better. I think that maintaining the patch
in Ubuntu 16.04 delta will likely break with upstream updates to the Azure
datasource.

I think the approach at http://paste.ubuntu.com/p/rNfppD68ZY/ will allow
us to maintain that more easily, and also more easily test both paths in
upstream code.

Thoughts?

review: Needs Information

[Chad Smith (chad.smith)]

> Are any of the cloud-init cli commands affected if we don't have IMDS data? I
> was thinking in metadata field outputs?

There is availability zone info in IMDS but my branch is still WIP and hasn't landed yet for using that field. Also, the imds metadata is not missing from instance-data.json it will still be there, we just won't use it to generate network config on xenial.

[Chad Smith (chad.smith)]

> I'm not opposed entirely to this change, and if we want to get this into
> 16.04 as a quick fix, we can do so.
>
> However I do think we can do better. I think that maintaining the patch
> in Ubuntu 16.04 delta will likely break with upstream updates to the Azure
> datasource.
>
> I think the approach at http://paste.ubuntu.com/p/rNfppD68ZY/ will allow
> us to maintain that more easily, and also more easily test both paths in
> upstream code.

I had started a branch against tip which would surface this type of config setting yesterday, but abandonded when I realized it was just going to be Xenial that didn't use the feature. I'll resurrect the branch (which uses 'apply_network_config' like OpenStack datasource uses). Will push it shortly.

[Chad Smith (chad.smith)]

rejected in favor of a config-based enable/disable toggle per https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/356989

Unmerged commits

4f6bb1c... by Chad Smith on 2018-10-17

fix mock_fallback.assert_called_once_with instead of called_once()

c416f35... by Chad Smith on 2018-10-16

add xenial debian patch to patch series

7d41f5a... by Chad Smith on 2018-10-16

debian/changelog update

8d6b128... by Chad Smith on 2018-10-16

xenial-azure: Disable rendering network from IMDS

In Xenial we already have hotplug scripts delivered in stock
cloud-images. Disable rendering network information from IMDS and avoid
deleting Ubuntu cloud-image udev add scripts which create
/etc/network/interface configuration for the hotplugged network
interfaces.

diff --git a/debian/changelog b/debian/changelog
index 2bdfd36..29fda8e 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+cloud-init (18.4-0ubuntu1~16.04.2) xenial-proposed; urgency=medium
+
+  * debian/patches/azure-dont-render-imds-network.patch
+    add patch to ignore Azure IMDS network_config and avoid removing
+    ubuntu network hotplug scripts
+
+ -- Chad Smith <[e-mail]>  Tue, 16 Oct 2018 16:37:09 -0600
+
 cloud-init (18.4-0ubuntu1~16.04.1) xenial-proposed; urgency=medium
 
   * drop the following cherry-picks now included:
diff --git a/debian/patches/azure-dont-render-imds-network.patch b/debian/patches/azure-dont-render-imds-network.patch
new file mode 100644
index 0000000..cfcb509
--- /dev/null
+++ b/debian/patches/azure-dont-render-imds-network.patch
@@ -0,0 +1,116 @@
+Description: Disable rendering network from Azure IMDS and script cleanup
+ Xenial Azure images already contain working udev script which react to
+ hotplug network interface adds and write /etc/network/interface config
+ for the new interfaces.
+ .
+ This patch disables the cloud-init logic needed on Bionic or later which
+ renders network configuration to netplan from Azure's IMDS. It also
+ avoids removing any of the cloud-image udev hotplug scripts which work as
+ designed in Xenial.
+Forwarded: not-needed
+Author: Chad Smith <[e-mail]>
+
+diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py
+index 783445e..d5726ab 100644
+--- a/cloudinit/sources/DataSourceAzure.py
++++ b/cloudinit/sources/DataSourceAzure.py
+@@ -56,12 +56,7 @@ IMDS_URL = "http://169.254.169.254/metadata/"
+ 
+ # List of static scripts and network config artifacts created by
+ # stock ubuntu suported images.
+-UBUNTU_EXTENDED_NETWORK_SCRIPTS = [
+-    '/etc/netplan/90-azure-hotplug.yaml',
+-    '/usr/local/sbin/ephemeral_eth.sh',
+-    '/etc/udev/rules.d/10-net-device-added.rules',
+-    '/run/network/interfaces.ephemeral.d',
+-]
++UBUNTU_EXTENDED_NETWORK_SCRIPTS = []  # Remove no network scipts on Xenial
+ 
+ 
+ def find_storvscid_from_sysctl_pnpinfo(sysctl_out, deviceid):
+@@ -611,7 +606,9 @@ class DataSourceAzure(sources.DataSource):
+               the blacklisted devices.
+         """
+         if not self._network_config:
+-            self._network_config = parse_network_config(self._metadata_imds)
++            # Xenial-only, use fallback network config not IMDS because
++            # ubuntu images contain the necessary ifup/down hotplug goodness
++            self._network_config = parse_network_config(None)
+         return self._network_config
+ 
+ 
+diff --git a/tests/unittests/test_datasource/test_azure.py b/tests/unittests/test_datasource/test_azure.py
+index 4e428b7..5e9cf08 100644
+--- a/tests/unittests/test_datasource/test_azure.py
++++ b/tests/unittests/test_datasource/test_azure.py
+@@ -501,20 +501,6 @@ fdescfs            /dev/fd          fdescfs rw              0 0
+         self.assertTrue(ret)
+         self.assertEqual(data['agent_invoked'], cfg['agent_command'])
+ 
+-    def test_network_config_set_from_imds(self):
+-        """Datasource.network_config returns IMDS network data."""
+-        odata = {}
+-        data = {'ovfcontent': construct_valid_ovf_env(data=odata)}
+-        expected_network_config = {
+-            'ethernets': {
+-                'eth0': {'set-name': 'eth0',
+-                         'match': {'macaddress': '00:0d:3a:04:75:98'},
+-                         'dhcp4': True}},
+-            'version': 2}
+-        dsrc = self._get_ds(data)
+-        dsrc.get_data()
+-        self.assertEqual(expected_network_config, dsrc.network_config)
+-
+     def test_user_cfg_set_agent_command(self):
+         # set dscfg in via base64 encoded yaml
+         cfg = {'agent_command': "my_command"}
+@@ -780,26 +766,38 @@ fdescfs            /dev/fd          fdescfs rw              0 0
+         self.assertEqual(
+             [mock.call("/dev/cd0")], m_check_fbsd_cdrom.call_args_list)
+ 
++    @mock.patch('cloudinit.net.get_interface_mac')
++    @mock.patch('cloudinit.net.get_devicelist')
++    @mock.patch('cloudinit.net.device_driver')
+     @mock.patch('cloudinit.net.generate_fallback_config')
+-    def test_imds_network_config(self, mock_fallback):
+-        """Network config is generated from IMDS network data when present."""
++    def test_imds_network_config_ignored_on_xenial(self, mock_fallback, mock_dd,
++                                                   mock_devlist, mock_get_mac):
++        """Network config ignores IMDS network data on Xenial. Use fallback."""
+         odata = {'HostName': "myhost", 'UserName': "myuser"}
+         data = {'ovfcontent': construct_valid_ovf_env(data=odata),
+                 'sys_cfg': {}}
+ 
++        fallback_config = {
++            'version': 1,
++            'config': [{
++                'type': 'physical', 'name': 'eth0',
++                'mac_address': '00:11:22:33:44:55',
++                'params': {'driver': 'hv_netsvc'},
++                'subnets': [{'type': 'dhcp'}],
++            }]
++        }
++        mock_fallback.return_value = fallback_config
++        mock_devlist.return_value = ['eth0']
++        mock_dd.return_value = ['hv_netsvc']
++        mock_get_mac.return_value = '00:11:22:33:44:55'
++
+         dsrc = self._get_ds(data)
+         ret = dsrc.get_data()
+         self.assertTrue(ret)
+ 
+-        expected_cfg = {
+-            'ethernets': {
+-                'eth0': {'dhcp4': True,
+-                         'match': {'macaddress': '00:0d:3a:04:75:98'},
+-                         'set-name': 'eth0'}},
+-            'version': 2}
+-
+-        self.assertEqual(expected_cfg, dsrc.network_config)
+-        mock_fallback.assert_not_called()
++        self.assertEqual(fallback_config, dsrc.network_config)
++        mock_fallback.assert_called_once_with(blacklist_drivers=['mlx4_core'],
++                                              config_driver=True)
+ 
+     @mock.patch('cloudinit.net.get_interface_mac')
+     @mock.patch('cloudinit.net.get_devicelist')
diff --git a/debian/patches/series b/debian/patches/series
index d5a24c5..fd9b6e8 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -2,3 +2,4 @@ azure-use-walinux-agent.patch
 ds-identify-behavior-xenial.patch
 stable-release-no-jsonschema-dep.patch
 openstack-no-network-config.patch
+azure-dont-render-imds-network.patch