Merge lp:~oddbloke/cloud-init/lp1506187 into lp:~cloud-init-dev/cloud-init/trunk

[Dan Watkins]

Azure: Use DMI data to find instance IDs.

Replace the use of SharedConfig.xml in both the walinuxagent case,
and the case where we communicate with the Azure fabric ourselves.

[Ben Howard (darkmuggle-deactivatedaccount)]

Adding Scott to the reviewers for merge approval and merging.

This change looks sane to me and reduces the complexity of the Datasource. Microsoft has updated the public facing documentation to reflect the use of the system uuid as extracted from dmi data as the authoratative identification of an instance.

We are seeing this as a hot issue. So once this lands in Xenial, I'll run an SRU to get this backported.

+1

review: Approve

[Scott Moser (smoser)]

This looks good. The one question I have is whether or not a instance that used sharedconfig.xml last boot will now think it is a new instance after an upgrade of cloud-init and reboot.

specifically, is the instance id in DMI the same as the instance id in SharedConfig.xml

=== modified file 'cloudinit/sources/DataSourceAzure.py'
--- cloudinit/sources/DataSourceAzure.py	2015-10-15 00:27:07 +0000
+++ cloudinit/sources/DataSourceAzure.py	2015-10-30 16:36:54 +0000
@@ -31,8 +31,7 @@
 from cloudinit.settings import PER_ALWAYS
 from cloudinit import sources
 from cloudinit import util
-from cloudinit.sources.helpers.azure import (
-    get_metadata_from_fabric, iid_from_shared_config_content)
+from cloudinit.sources.helpers.azure import get_metadata_from_fabric
 
 LOG = logging.getLogger(__name__)
 
@@ -41,7 +40,6 @@
 AGENT_START = ['service', 'walinuxagent', 'start']
 BOUNCE_COMMAND = ['sh', '-xc',
     "i=$interface; x=0; ifdown $i || x=$?; ifup $i || x=$?; exit $x"]
-DATA_DIR_CLEAN_LIST = ['SharedConfig.xml']
 
 BUILTIN_DS_CONFIG = {
     'agent_command': AGENT_START,
@@ -144,8 +142,6 @@
                             self.ds_cfg['agent_command'])
 
             ddir = self.ds_cfg['data_dir']
-            shcfgxml = os.path.join(ddir, "SharedConfig.xml")
-            wait_for = [shcfgxml]
 
             fp_files = []
             key_value = None
@@ -160,19 +156,11 @@
 
             missing = util.log_time(logfunc=LOG.debug, msg="waiting for files",
                                     func=wait_for_files,
-                                    args=(wait_for + fp_files,))
+                                    args=(fp_files,))
         if len(missing):
             LOG.warn("Did not find files, but going on: %s", missing)
 
         metadata = {}
-        if shcfgxml in missing:
-            LOG.warn("SharedConfig.xml missing, using static instance-id")
-        else:
-            try:
-                metadata['instance-id'] = iid_from_shared_config(shcfgxml)
-            except ValueError as e:
-                LOG.warn("failed to get instance id in %s: %s", shcfgxml, e)
-
         metadata['public-keys'] = key_value or pubkeys_from_crt_files(fp_files)
         return metadata
 
@@ -229,21 +217,6 @@
         user_ds_cfg = util.get_cfg_by_path(self.cfg, DS_CFG_PATH, {})
         self.ds_cfg = util.mergemanydict([user_ds_cfg, self.ds_cfg])
 
-        if found != ddir:
-            cached_ovfenv = util.load_file(
-                os.path.join(ddir, 'ovf-env.xml'), quiet=True, decode=False)
-            if cached_ovfenv != files['ovf-env.xml']:
-                # source was not walinux-agent's datadir, so we have to clean
-                # up so 'wait_for_files' doesn't return early due to stale data
-                cleaned = []
-                for f in [os.path.join(ddir, f) for f in DATA_DIR_CLEAN_LIST]:
-                    if os.path.exists(f):
-                        util.del_file(f)
-                        cleaned.append(f)
-                if cleaned:
-                    LOG.info("removed stale file(s) in '%s': %s",
-                             ddir, str(cleaned))
-
         # walinux agent writes files world readable, but expects
         # the directory to be protected.
         write_files(ddir, files, dirmode=0o700)
@@ -259,6 +232,7 @@
                      " on Azure.", exc_info=True)
             return False
 
+        self.metadata['instance-id'] = util.read_dmi_data('system-uuid')
         self.metadata.update(fabric_data)
 
         found_ephemeral = find_fabric_formatted_ephemeral_disk()
@@ -649,12 +623,6 @@
     return (md, ud, cfg, {'ovf-env.xml': contents})
 
 
-def iid_from_shared_config(path):
-    with open(path, "rb") as fp:
-        content = fp.read()
-    return iid_from_shared_config_content(content)
-
-
 class BrokenAzureDataSource(Exception):
     pass
 

=== modified file 'cloudinit/sources/helpers/azure.py'
--- cloudinit/sources/helpers/azure.py	2015-05-08 15:52:58 +0000
+++ cloudinit/sources/helpers/azure.py	2015-10-30 16:36:54 +0000
@@ -79,12 +79,6 @@
             './Container/RoleInstanceList/RoleInstance/InstanceId')
 
     @property
-    def shared_config_xml(self):
-        url = self._text_from_xpath('./Container/RoleInstanceList/RoleInstance'
-                                    '/Configuration/SharedConfig')
-        return self.http_client.get(url).contents
-
-    @property
     def certificates_xml(self):
         if self._certificates_xml is None:
             url = self._text_from_xpath(
@@ -172,19 +166,6 @@
         return keys
 
 
-def iid_from_shared_config_content(content):
-    """
-    find INSTANCE_ID in:
-    <?xml version="1.0" encoding="utf-8"?>
-    <SharedConfig version="1.0.0.0" goalStateIncarnation="1">
-    <Deployment name="INSTANCE_ID" guid="{...}" incarnation="0">
-        <Service name="..." guid="{00000000-0000-0000-0000-000000000000}"/>
-    """
-    root = ElementTree.fromstring(content)
-    depnode = root.find('Deployment')
-    return depnode.get('name')
-
-
 class WALinuxAgentShim(object):
 
     REPORT_READY_XML_TEMPLATE = '\n'.join([
@@ -263,8 +244,6 @@
             public_keys = self.openssl_manager.parse_certificates(
                 goal_state.certificates_xml)
         data = {
-            'instance-id': iid_from_shared_config_content(
-                goal_state.shared_config_xml),
             'public-keys': public_keys,
         }
         self._report_ready(goal_state, http_client)

=== modified file 'tests/unittests/test_datasource/test_azure.py'
--- tests/unittests/test_datasource/test_azure.py	2015-10-14 23:32:35 +0000
+++ tests/unittests/test_datasource/test_azure.py	2015-10-30 16:36:54 +0000
@@ -115,10 +115,6 @@
             data['pubkey_files'] = flist
             return ["pubkey_from: %s" % f for f in flist]
 
-        def _iid_from_shared_config(path):
-            data['iid_from_shared_cfg'] = path
-            return 'i-my-azure-id'
-
         if data.get('ovfcontent') is not None:
             populate_dir(os.path.join(self.paths.seed_dir, "azure"),
                          {'ovf-env.xml': data['ovfcontent']})
@@ -127,20 +123,22 @@
         mod.BUILTIN_DS_CONFIG['data_dir'] = self.waagent_d
 
         self.get_metadata_from_fabric = mock.MagicMock(return_value={
-            'instance-id': 'i-my-azure-id',
             'public-keys': [],
         })
 
+        self.instance_id = 'test-instance-id'
+
         self.apply_patches([
             (mod, 'list_possible_azure_ds_devs', dsdevs),
             (mod, 'invoke_agent', _invoke_agent),
             (mod, 'wait_for_files', _wait_for_files),
             (mod, 'pubkeys_from_crt_files', _pubkeys_from_crt_files),
-            (mod, 'iid_from_shared_config', _iid_from_shared_config),
             (mod, 'perform_hostname_bounce', mock.MagicMock()),
             (mod, 'get_hostname', mock.MagicMock()),
             (mod, 'set_hostname', mock.MagicMock()),
             (mod, 'get_metadata_from_fabric', self.get_metadata_from_fabric),
+            (mod.util, 'read_dmi_data', mock.MagicMock(
+                return_value=self.instance_id)),
         ])
 
         dsrc = mod.DataSourceAzureNet(
@@ -193,7 +191,6 @@
         self.assertEqual(dsrc.metadata['local-hostname'], odata['HostName'])
         self.assertTrue(os.path.isfile(
             os.path.join(self.waagent_d, 'ovf-env.xml')))
-        self.assertEqual(dsrc.metadata['instance-id'], 'i-my-azure-id')
 
     def test_waagent_d_has_0700_perms(self):
         # we expect /var/lib/waagent to be created 0700
@@ -345,7 +342,6 @@
         for mypk in mypklist:
             self.assertIn(mypk['value'], dsrc.metadata['public-keys'])
 
-
     def test_default_ephemeral(self):
         # make sure the ephemeral device works
         odata = {}
@@ -434,54 +430,6 @@
         dsrc = self._get_ds({'ovfcontent': xml})
         dsrc.get_data()
 
-    def test_existing_ovf_same(self):
-        # waagent/SharedConfig left alone if found ovf-env.xml same as cached
-        odata = {'UserData': b64e("SOMEUSERDATA")}
-        data = {'ovfcontent': construct_valid_ovf_env(data=odata)}
-
-        populate_dir(self.waagent_d,
-            {'ovf-env.xml': data['ovfcontent'],
-             'otherfile': 'otherfile-content',
-             'SharedConfig.xml': 'mysharedconfig'})
-
-        dsrc = self._get_ds(data)
-        ret = dsrc.get_data()
-        self.assertTrue(ret)
-        self.assertTrue(os.path.exists(
-            os.path.join(self.waagent_d, 'ovf-env.xml')))
-        self.assertTrue(os.path.exists(
-            os.path.join(self.waagent_d, 'otherfile')))
-        self.assertTrue(os.path.exists(
-            os.path.join(self.waagent_d, 'SharedConfig.xml')))
-
-    def test_existing_ovf_diff(self):
-        # waagent/SharedConfig must be removed if ovfenv is found elsewhere
-
-        # 'get_data' should remove SharedConfig.xml in /var/lib/waagent
-        # if ovf-env.xml differs.
-        cached_ovfenv = construct_valid_ovf_env(
-            {'userdata': b64e("FOO_USERDATA")})
-        new_ovfenv = construct_valid_ovf_env(
-            {'userdata': b64e("NEW_USERDATA")})
-
-        populate_dir(self.waagent_d,
-            {'ovf-env.xml': cached_ovfenv,
-             'SharedConfig.xml': "mysharedconfigxml",
-             'otherfile': 'otherfilecontent'})
-
-        dsrc = self._get_ds({'ovfcontent': new_ovfenv})
-        ret = dsrc.get_data()
-        self.assertTrue(ret)
-        self.assertEqual(dsrc.userdata_raw, b"NEW_USERDATA")
-        self.assertTrue(os.path.exists(
-            os.path.join(self.waagent_d, 'otherfile')))
-        self.assertFalse(os.path.exists(
-                        os.path.join(self.waagent_d, 'SharedConfig.xml')))
-        self.assertTrue(os.path.exists(
-                        os.path.join(self.waagent_d, 'ovf-env.xml')))
-        new_xml = load_file(os.path.join(self.waagent_d, 'ovf-env.xml'))
-        self.xml_equals(new_ovfenv, new_xml)
-
     def test_exception_fetching_fabric_data_doesnt_propagate(self):
         ds = self._get_ds({'ovfcontent': construct_valid_ovf_env()})
         ds.ds_cfg['agent_command'] = '__builtin__'
@@ -496,6 +444,17 @@
         self.assertTrue(ret)
         self.assertEqual('value', ds.metadata['test'])
 
+    def test_instance_id_from_dmidecode_used(self):
+        ds = self._get_ds({'ovfcontent': construct_valid_ovf_env()})
+        ds.get_data()
+        self.assertEqual(self.instance_id, ds.metadata['instance-id'])
+
+    def test_instance_id_from_dmidecode_used_for_builtin(self):
+        ds = self._get_ds({'ovfcontent': construct_valid_ovf_env()})
+        ds.ds_cfg['agent_command'] = '__builtin__'
+        ds.get_data()
+        self.assertEqual(self.instance_id, ds.metadata['instance-id'])
+
 
 class TestAzureBounce(TestCase):
 
@@ -505,9 +464,6 @@
         self.patches.enter_context(
             mock.patch.object(DataSourceAzure, 'wait_for_files'))
         self.patches.enter_context(
-            mock.patch.object(DataSourceAzure, 'iid_from_shared_config',
-                              mock.MagicMock(return_value='i-my-azure-id')))
-        self.patches.enter_context(
             mock.patch.object(DataSourceAzure, 'list_possible_azure_ds_devs',
                               mock.MagicMock(return_value=[])))
         self.patches.enter_context(
@@ -521,6 +477,9 @@
         self.patches.enter_context(
             mock.patch.object(DataSourceAzure, 'get_metadata_from_fabric',
                               mock.MagicMock(return_value={})))
+        self.patches.enter_context(
+            mock.patch.object(DataSourceAzure.util, 'read_dmi_data',
+                              mock.MagicMock(return_value='test-instance-id')))
 
     def setUp(self):
         super(TestAzureBounce, self).setUp()

=== modified file 'tests/unittests/test_datasource/test_azure_helper.py'
--- tests/unittests/test_datasource/test_azure_helper.py	2015-05-15 20:28:24 +0000
+++ tests/unittests/test_datasource/test_azure_helper.py	2015-10-30 16:36:54 +0000
@@ -40,7 +40,7 @@
           <HostingEnvironmentConfig>
             http://100.86.192.70:80/...hostingEnvironmentConfig...
           </HostingEnvironmentConfig>
-          <SharedConfig>{shared_config_url}</SharedConfig>
+          <SharedConfig>http://100.86.192.70:80/..SharedConfig..</SharedConfig>
           <ExtensionsConfig>
             http://100.86.192.70:80/...extensionsConfig...
           </ExtensionsConfig>
@@ -55,21 +55,6 @@
 """
 
 
-class TestReadAzureSharedConfig(unittest.TestCase):
-
-    def test_valid_content(self):
-        xml = """<?xml version="1.0" encoding="utf-8"?>
-            <SharedConfig>
-             <Deployment name="MY_INSTANCE_ID">
-              <Service name="myservice"/>
-              <ServiceInstance name="INSTANCE_ID.0" guid="{abcd-uuid}" />
-             </Deployment>
-            <Incarnation number="1"/>
-            </SharedConfig>"""
-        ret = azure_helper.iid_from_shared_config_content(xml)
-        self.assertEqual("MY_INSTANCE_ID", ret)
-
-
 class TestFindEndpoint(TestCase):
 
     def setUp(self):
@@ -140,7 +125,6 @@
         'incarnation': 1,
         'container_id': 'MyContainerId',
         'instance_id': 'MyInstanceId',
-        'shared_config_url': 'MySharedConfigUrl',
         'certificates_url': 'MyCertificatesUrl',
     }
 
@@ -174,20 +158,9 @@
         goal_state = self._get_goal_state(instance_id=instance_id)
         self.assertEqual(instance_id, goal_state.instance_id)
 
-    def test_shared_config_xml_parsed_and_fetched_correctly(self):
-        http_client = mock.MagicMock()
-        shared_config_url = 'TestSharedConfigUrl'
-        goal_state = self._get_goal_state(
-            http_client=http_client, shared_config_url=shared_config_url)
-        shared_config_xml = goal_state.shared_config_xml
-        self.assertEqual(1, http_client.get.call_count)
-        self.assertEqual(shared_config_url, http_client.get.call_args[0][0])
-        self.assertEqual(http_client.get.return_value.contents,
-                         shared_config_xml)
-
     def test_certificates_xml_parsed_and_fetched_correctly(self):
         http_client = mock.MagicMock()
-        certificates_url = 'TestSharedConfigUrl'
+        certificates_url = 'TestCertificatesUrl'
         goal_state = self._get_goal_state(
             http_client=http_client, certificates_url=certificates_url)
         certificates_xml = goal_state.certificates_xml
@@ -324,8 +297,6 @@
                 azure_helper.WALinuxAgentShim, 'find_endpoint'))
         self.GoalState = patches.enter_context(
             mock.patch.object(azure_helper, 'GoalState'))
-        self.iid_from_shared_config_content = patches.enter_context(
-            mock.patch.object(azure_helper, 'iid_from_shared_config_content'))
         self.OpenSSLManager = patches.enter_context(
             mock.patch.object(azure_helper, 'OpenSSLManager'))
         patches.enter_context(
@@ -367,15 +338,6 @@
         data = shim.register_with_azure_and_fetch_data()
         self.assertEqual([], data['public-keys'])
 
-    def test_instance_id_returned_in_data(self):
-        shim = azure_helper.WALinuxAgentShim()
-        data = shim.register_with_azure_and_fetch_data()
-        self.assertEqual(
-            [mock.call(self.GoalState.return_value.shared_config_xml)],
-            self.iid_from_shared_config_content.call_args_list)
-        self.assertEqual(self.iid_from_shared_config_content.return_value,
-                         data['instance-id'])
-
     def test_correct_url_used_for_report_ready(self):
         self.find_endpoint.return_value = 'test_endpoint'
         shim = azure_helper.WALinuxAgentShim()