Azure: if user has set hostname, do not overwrite it

Due to the way the azure boot works, where we identify the hostname to the
fabric, we were inadvertently overwriting hostname that the user had set.

LP: #1375252

4 files changed, 246 insertions, 105 deletions

diff --git a/ChangeLog b/ChangeLog
index 6f843933..ae9926a9 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -37,6 +37,7 @@
  - GCE datasource does not handle instance ssh keys (LP: 1403617)
  - sysvinit: make cloud-init-local run before network (LP: #1275098)
    [Surojit Pathak]
+ - Azure: do not re-set hostname if user has changed it (LP: #1375252)
 0.7.6:
  - open 0.7.6
  - Enable vendordata on CloudSigma datasource (LP: #1303986)
diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py
index 6e030217..a19d9ca2 100644
--- a/cloudinit/sources/DataSourceAzure.py
+++ b/cloudinit/sources/DataSourceAzure.py
@@ -17,6 +17,7 @@
 #    along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import base64
+import contextlib
 import crypt
 import fnmatch
 import os
@@ -66,6 +67,36 @@ DS_CFG_PATH = ['datasource', DS_NAME]
 DEF_EPHEMERAL_LABEL = 'Temporary Storage'
 
 
+def get_hostname(hostname_command='hostname'):
+    return util.subp(hostname_command, capture=True)[0].strip()
+
+
+def set_hostname(hostname, hostname_command='hostname'):
+    util.subp([hostname_command, hostname])
+
+
+@contextlib.contextmanager
+def temporary_hostname(temp_hostname, cfg, hostname_command='hostname'):
+    """
+    Set a temporary hostname, restoring the previous hostname on exit.
+
+    Will have the value of the previous hostname when used as a context
+    manager, or None if the hostname was not changed.
+    """
+    policy = cfg['hostname_bounce']['policy']
+    previous_hostname = get_hostname(hostname_command)
+    if (not util.is_true(cfg.get('set_hostname'))
+            or util.is_false(policy)
+            or (previous_hostname == temp_hostname and policy != 'force')):
+        yield None
+        return
+    set_hostname(temp_hostname, hostname_command)
+    try:
+        yield previous_hostname
+    finally:
+        set_hostname(previous_hostname, hostname_command)
+
+
 class DataSourceAzureNet(sources.DataSource):
     def __init__(self, sys_cfg, distro, paths):
         sources.DataSource.__init__(self, sys_cfg, distro, paths)
@@ -154,33 +185,40 @@ class DataSourceAzureNet(sources.DataSource):
         # the directory to be protected.
         write_files(ddir, files, dirmode=0o700)
 
-        # handle the hostname 'publishing'
-        try:
-            handle_set_hostname(mycfg.get('set_hostname'),
-                                self.metadata.get('local-hostname'),
-                                mycfg['hostname_bounce'])
-        except Exception as e:
-            LOG.warn("Failed publishing hostname: %s", e)
-            util.logexc(LOG, "handling set_hostname failed")
-
-        try:
-            invoke_agent(mycfg['agent_command'])
-        except util.ProcessExecutionError:
-            # claim the datasource even if the command failed
-            util.logexc(LOG, "agent command '%s' failed.",
-                        mycfg['agent_command'])
-
-        shcfgxml = os.path.join(ddir, "SharedConfig.xml")
-        wait_for = [shcfgxml]
-
-        fp_files = []
-        for pk in self.cfg.get('_pubkeys', []):
-            bname = str(pk['fingerprint'] + ".crt")
-            fp_files += [os.path.join(ddir, bname)]
+        temp_hostname = self.metadata.get('local-hostname')
+        hostname_command = mycfg['hostname_bounce']['hostname_command']
+        with temporary_hostname(temp_hostname, mycfg,
+                                hostname_command=hostname_command) \
+                as previous_hostname:
+            if (previous_hostname is not None
+                    and util.is_true(mycfg.get('set_hostname'))):
+                cfg = mycfg['hostname_bounce']
+                try:
+                    perform_hostname_bounce(hostname=temp_hostname,
+                                            cfg=cfg,
+                                            prev_hostname=previous_hostname)
+                except Exception as e:
+                    LOG.warn("Failed publishing hostname: %s", e)
+                    util.logexc(LOG, "handling set_hostname failed")
 
-        missing = util.log_time(logfunc=LOG.debug, msg="waiting for files",
-                                func=wait_for_files,
-                                args=(wait_for + fp_files,))
+            try:
+                invoke_agent(mycfg['agent_command'])
+            except util.ProcessExecutionError:
+                # claim the datasource even if the command failed
+                util.logexc(LOG, "agent command '%s' failed.",
+                            mycfg['agent_command'])
+
+            shcfgxml = os.path.join(ddir, "SharedConfig.xml")
+            wait_for = [shcfgxml]
+
+            fp_files = []
+            for pk in self.cfg.get('_pubkeys', []):
+                bname = str(pk['fingerprint'] + ".crt")
+                fp_files += [os.path.join(ddir, bname)]
+
+            missing = util.log_time(logfunc=LOG.debug, msg="waiting for files",
+                                    func=wait_for_files,
+                                    args=(wait_for + fp_files,))
         if len(missing):
             LOG.warn("Did not find files, but going on: %s", missing)
 
@@ -299,39 +337,15 @@ def support_new_ephemeral(cfg):
     return mod_list
 
 
-def handle_set_hostname(enabled, hostname, cfg):
-    if not util.is_true(enabled):
-        return
-
-    if not hostname:
-        LOG.warn("set_hostname was true but no local-hostname")
-        return
-
-    apply_hostname_bounce(hostname=hostname, policy=cfg['policy'],
-                          interface=cfg['interface'],
-                          command=cfg['command'],
-                          hostname_command=cfg['hostname_command'])
-
-
-def apply_hostname_bounce(hostname, policy, interface, command,
-                          hostname_command="hostname"):
+def perform_hostname_bounce(hostname, cfg, prev_hostname):
     # set the hostname to 'hostname' if it is not already set to that.
     # then, if policy is not off, bounce the interface using command
-    prev_hostname = util.subp(hostname_command, capture=True)[0].strip()
-
-    util.subp([hostname_command, hostname])
-
-    msg = ("phostname=%s hostname=%s policy=%s interface=%s" %
-           (prev_hostname, hostname, policy, interface))
-
-    if util.is_false(policy):
-        LOG.debug("pubhname: policy false, skipping [%s]", msg)
-        return
-
-    if prev_hostname == hostname and policy != "force":
-        LOG.debug("pubhname: no change, policy != force. skipping. [%s]", msg)
-        return
+    command = cfg['command']
+    interface = cfg['interface']
+    policy = cfg['policy']
 
+    msg = ("hostname=%s policy=%s interface=%s" %
+           (hostname, policy, interface))
     env = os.environ.copy()
     env['interface'] = interface
     env['hostname'] = hostname
@@ -344,9 +358,9 @@ def apply_hostname_bounce(hostname, policy, interface, command,
     shell = not isinstance(command, (list, tuple))
     # capture=False, see comments in bug 1202758 and bug 1206164.
     util.log_time(logfunc=LOG.debug, msg="publishing hostname",
-        get_uptime=True, func=util.subp,
-        kwargs={'args': command, 'shell': shell, 'capture': False,
-                'env': env})
+                  get_uptime=True, func=util.subp,
+                  kwargs={'args': command, 'shell': shell, 'capture': False,
+                          'env': env})
 
 
 def crtfile_to_pubkey(fname):
diff --git a/tests/unittests/test_data.py b/tests/unittests/test_data.py
index 1b15dafa..c603bfdb 100644
--- a/tests/unittests/test_data.py
+++ b/tests/unittests/test_data.py
@@ -511,7 +511,6 @@ c: 4
         mockobj.assert_called_once_with(
             ci.paths.get_ipath("cloud_config"), "", 0o600)
 
-
     def test_cloud_config_archive(self):
         non_decodable = b'\x11\xc9\xb4gTH\xee\x12'
         data = [{'content': '#cloud-config\npassword: gocubs\n'},
diff --git a/tests/unittests/test_datasource/test_azure.py b/tests/unittests/test_datasource/test_azure.py
index 8112c69b..7e789853 100644
--- a/tests/unittests/test_datasource/test_azure.py
+++ b/tests/unittests/test_datasource/test_azure.py
@@ -116,9 +116,6 @@ class TestAzureDataSource(TestCase):
             data['iid_from_shared_cfg'] = path
             return 'i-my-azure-id'
 
-        def _apply_hostname_bounce(**kwargs):
-            data['apply_hostname_bounce'] = kwargs
-
         if data.get('ovfcontent') is not None:
             populate_dir(os.path.join(self.paths.seed_dir, "azure"),
                          {'ovf-env.xml': data['ovfcontent']})
@@ -132,7 +129,9 @@ class TestAzureDataSource(TestCase):
             (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, 'apply_hostname_bounce', _apply_hostname_bounce),
+            (mod, 'perform_hostname_bounce', mock.MagicMock()),
+            (mod, 'get_hostname', mock.MagicMock()),
+            (mod, 'set_hostname', mock.MagicMock()),
             ])
 
         dsrc = mod.DataSourceAzureNet(
@@ -272,47 +271,6 @@ class TestAzureDataSource(TestCase):
         for mypk in mypklist:
             self.assertIn(mypk, dsrc.cfg['_pubkeys'])
 
-    def test_disabled_bounce(self):
-        pass
-
-    def test_apply_bounce_call_1(self):
-        # hostname needs to get through to apply_hostname_bounce
-        odata = {'HostName': 'my-random-hostname'}
-        data = {'ovfcontent': construct_valid_ovf_env(data=odata)}
-
-        self._get_ds(data).get_data()
-        self.assertIn('hostname', data['apply_hostname_bounce'])
-        self.assertEqual(data['apply_hostname_bounce']['hostname'],
-                         odata['HostName'])
-
-    def test_apply_bounce_call_configurable(self):
-        # hostname_bounce should be configurable in datasource cfg
-        cfg = {'hostname_bounce': {'interface': 'eth1', 'policy': 'off',
-                                   'command': 'my-bounce-command',
-                                   'hostname_command': 'my-hostname-command'}}
-        odata = {'HostName': "xhost",
-                'dscfg': {'text': b64e(yaml.dump(cfg)),
-                          'encoding': 'base64'}}
-        data = {'ovfcontent': construct_valid_ovf_env(data=odata)}
-        self._get_ds(data).get_data()
-
-        for k in cfg['hostname_bounce']:
-            self.assertIn(k, data['apply_hostname_bounce'])
-
-        for k, v in cfg['hostname_bounce'].items():
-            self.assertEqual(data['apply_hostname_bounce'][k], v)
-
-    def test_set_hostname_disabled(self):
-        # config specifying set_hostname off should not bounce
-        cfg = {'set_hostname': False}
-        odata = {'HostName': "xhost",
-                'dscfg': {'text': b64e(yaml.dump(cfg)),
-                          'encoding': 'base64'}}
-        data = {'ovfcontent': construct_valid_ovf_env(data=odata)}
-        self._get_ds(data).get_data()
-
-        self.assertEqual(data.get('apply_hostname_bounce', "N/A"), "N/A")
-
     def test_default_ephemeral(self):
         # make sure the ephemeral device works
         odata = {}
@@ -425,6 +383,175 @@ class TestAzureDataSource(TestCase):
             load_file(os.path.join(self.waagent_d, 'ovf-env.xml')))
 
 
+class TestAzureBounce(TestCase):
+
+    def mock_out_azure_moving_parts(self):
+        self.patches.enter_context(
+            mock.patch.object(DataSourceAzure, 'invoke_agent'))
+        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(
+            mock.patch.object(DataSourceAzure, 'find_ephemeral_disk',
+                              mock.MagicMock(return_value=None)))
+        self.patches.enter_context(
+            mock.patch.object(DataSourceAzure, 'find_ephemeral_part',
+                              mock.MagicMock(return_value=None)))
+
+    def setUp(self):
+        super(TestAzureBounce, self).setUp()
+        self.tmp = tempfile.mkdtemp()
+        self.waagent_d = os.path.join(self.tmp, 'var', 'lib', 'waagent')
+        self.paths = helpers.Paths({'cloud_dir': self.tmp})
+        self.addCleanup(shutil.rmtree, self.tmp)
+        DataSourceAzure.BUILTIN_DS_CONFIG['data_dir'] = self.waagent_d
+        self.patches = ExitStack()
+        self.mock_out_azure_moving_parts()
+        self.get_hostname = self.patches.enter_context(
+            mock.patch.object(DataSourceAzure, 'get_hostname'))
+        self.set_hostname = self.patches.enter_context(
+            mock.patch.object(DataSourceAzure, 'set_hostname'))
+        self.subp = self.patches.enter_context(
+            mock.patch('cloudinit.sources.DataSourceAzure.util.subp'))
+
+    def tearDown(self):
+        self.patches.close()
+
+    def _get_ds(self, ovfcontent=None):
+        if ovfcontent is not None:
+            populate_dir(os.path.join(self.paths.seed_dir, "azure"),
+                         {'ovf-env.xml': ovfcontent})
+        return DataSourceAzure.DataSourceAzureNet(
+            {}, distro=None, paths=self.paths)
+
+    def get_ovf_env_with_dscfg(self, hostname, cfg):
+        odata = {
+            'HostName': hostname,
+            'dscfg': {
+                'text': b64e(yaml.dump(cfg)),
+                'encoding': 'base64'
+            }
+        }
+        return construct_valid_ovf_env(data=odata)
+
+    def test_disabled_bounce_does_not_change_hostname(self):
+        cfg = {'hostname_bounce': {'policy': 'off'}}
+        self._get_ds(self.get_ovf_env_with_dscfg('test-host', cfg)).get_data()
+        self.assertEqual(0, self.set_hostname.call_count)
+
+    @mock.patch('cloudinit.sources.DataSourceAzure.perform_hostname_bounce')
+    def test_disabled_bounce_does_not_perform_bounce(
+            self, perform_hostname_bounce):
+        cfg = {'hostname_bounce': {'policy': 'off'}}
+        self._get_ds(self.get_ovf_env_with_dscfg('test-host', cfg)).get_data()
+        self.assertEqual(0, perform_hostname_bounce.call_count)
+
+    def test_same_hostname_does_not_change_hostname(self):
+        host_name = 'unchanged-host-name'
+        self.get_hostname.return_value = host_name
+        cfg = {'hostname_bounce': {'policy': 'yes'}}
+        self._get_ds(self.get_ovf_env_with_dscfg(host_name, cfg)).get_data()
+        self.assertEqual(0, self.set_hostname.call_count)
+
+    @mock.patch('cloudinit.sources.DataSourceAzure.perform_hostname_bounce')
+    def test_unchanged_hostname_does_not_perform_bounce(
+            self, perform_hostname_bounce):
+        host_name = 'unchanged-host-name'
+        self.get_hostname.return_value = host_name
+        cfg = {'hostname_bounce': {'policy': 'yes'}}
+        self._get_ds(self.get_ovf_env_with_dscfg(host_name, cfg)).get_data()
+        self.assertEqual(0, perform_hostname_bounce.call_count)
+
+    @mock.patch('cloudinit.sources.DataSourceAzure.perform_hostname_bounce')
+    def test_force_performs_bounce_regardless(self, perform_hostname_bounce):
+        host_name = 'unchanged-host-name'
+        self.get_hostname.return_value = host_name
+        cfg = {'hostname_bounce': {'policy': 'force'}}
+        self._get_ds(self.get_ovf_env_with_dscfg(host_name, cfg)).get_data()
+        self.assertEqual(1, perform_hostname_bounce.call_count)
+
+    def test_different_hostnames_sets_hostname(self):
+        expected_hostname = 'azure-expected-host-name'
+        self.get_hostname.return_value = 'default-host-name'
+        self._get_ds(
+            self.get_ovf_env_with_dscfg(expected_hostname, {})).get_data()
+        self.assertEqual(expected_hostname,
+                         self.set_hostname.call_args_list[0][0][0])
+
+    @mock.patch('cloudinit.sources.DataSourceAzure.perform_hostname_bounce')
+    def test_different_hostnames_performs_bounce(
+            self, perform_hostname_bounce):
+        expected_hostname = 'azure-expected-host-name'
+        self.get_hostname.return_value = 'default-host-name'
+        self._get_ds(
+            self.get_ovf_env_with_dscfg(expected_hostname, {})).get_data()
+        self.assertEqual(1, perform_hostname_bounce.call_count)
+
+    def test_different_hostnames_sets_hostname_back(self):
+        initial_host_name = 'default-host-name'
+        self.get_hostname.return_value = initial_host_name
+        self._get_ds(
+            self.get_ovf_env_with_dscfg('some-host-name', {})).get_data()
+        self.assertEqual(initial_host_name,
+                         self.set_hostname.call_args_list[-1][0][0])
+
+    @mock.patch('cloudinit.sources.DataSourceAzure.perform_hostname_bounce')
+    def test_failure_in_bounce_still_resets_host_name(
+            self, perform_hostname_bounce):
+        perform_hostname_bounce.side_effect = Exception
+        initial_host_name = 'default-host-name'
+        self.get_hostname.return_value = initial_host_name
+        self._get_ds(
+            self.get_ovf_env_with_dscfg('some-host-name', {})).get_data()
+        self.assertEqual(initial_host_name,
+                         self.set_hostname.call_args_list[-1][0][0])
+
+    def test_environment_correct_for_bounce_command(self):
+        interface = 'int0'
+        hostname = 'my-new-host'
+        old_hostname = 'my-old-host'
+        self.get_hostname.return_value = old_hostname
+        cfg = {'hostname_bounce': {'interface': interface, 'policy': 'force'}}
+        data = self.get_ovf_env_with_dscfg(hostname, cfg)
+        self._get_ds(data).get_data()
+        self.assertEqual(1, self.subp.call_count)
+        bounce_env = self.subp.call_args[1]['env']
+        self.assertEqual(interface, bounce_env['interface'])
+        self.assertEqual(hostname, bounce_env['hostname'])
+        self.assertEqual(old_hostname, bounce_env['old_hostname'])
+
+    def test_default_bounce_command_used_by_default(self):
+        cmd = 'default-bounce-command'
+        DataSourceAzure.BUILTIN_DS_CONFIG['hostname_bounce']['command'] = cmd
+        cfg = {'hostname_bounce': {'policy': 'force'}}
+        data = self.get_ovf_env_with_dscfg('some-hostname', cfg)
+        self._get_ds(data).get_data()
+        self.assertEqual(1, self.subp.call_count)
+        bounce_args = self.subp.call_args[1]['args']
+        self.assertEqual(cmd, bounce_args)
+
+    @mock.patch('cloudinit.sources.DataSourceAzure.perform_hostname_bounce')
+    def test_set_hostname_option_can_disable_bounce(
+            self, perform_hostname_bounce):
+        cfg = {'set_hostname': False, 'hostname_bounce': {'policy': 'force'}}
+        data = self.get_ovf_env_with_dscfg('some-hostname', cfg)
+        self._get_ds(data).get_data()
+
+        self.assertEqual(0, perform_hostname_bounce.call_count)
+
+    def test_set_hostname_option_can_disable_hostname_set(self):
+        cfg = {'set_hostname': False, 'hostname_bounce': {'policy': 'force'}}
+        data = self.get_ovf_env_with_dscfg('some-hostname', cfg)
+        self._get_ds(data).get_data()
+
+        self.assertEqual(0, self.set_hostname.call_count)
+
+
 class TestReadAzureOvf(TestCase):
     def test_invalid_xml_raises_non_azure_ds(self):
         invalid_xml = "<foo>" + construct_valid_ovf_env(data={})