sources/azure: remove unnecessary hostname bounce (#1143)

Thanks to [1], the hostname is set prior to network bring-up.

The Azure data source has been bouncing the hostname during
setup(), occurring after the hostname has already been
properly configured.

Note that this doesn't prevent leaking the image's hostname
during Azure's _get_data() when it brings up ephemeral DHCP.
However, as are not guaranteed to have the hostname metadata
available from a truly "local" source, this behavior is to
be expected unless we disable `send host-name` from dhclient
config.

[1]: https://github.com/canonical/cloud-init/commit/133ad2cb327ad17b7b81319fac8f9f14577c04df

Signed-off-by: Chris Patterson <cpatterson@microsoft.com>

4 files changed, 0 insertions, 415 deletions

diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py
index 6c1bc085..eee98fa8 100755
--- a/cloudinit/sources/DataSourceAzure.py
+++ b/cloudinit/sources/DataSourceAzure.py
@@ -6,7 +6,6 @@
 
 import base64
 from collections import namedtuple
-import contextlib
 import crypt
 from functools import partial
 import os
@@ -52,20 +51,10 @@ LOG = logging.getLogger(__name__)
 
 DS_NAME = 'Azure'
 DEFAULT_METADATA = {"instance-id": "iid-AZURE-NODE"}
-BOUNCE_COMMAND_IFUP = [
-    'sh', '-xc',
-    "i=$interface; x=0; ifdown $i || x=$?; ifup $i || x=$?; exit $x"
-]
-BOUNCE_COMMAND_FREEBSD = [
-    'sh', '-xc',
-    ("i=$interface; x=0; ifconfig down $i || x=$?; "
-     "ifconfig up $i || x=$?; exit $x")
-]
 
 # azure systems will always have a resource disk, and 66-azure-ephemeral.rules
 # ensures that it gets linked to this path.
 RESOURCE_DISK_PATH = '/dev/disk/cloud/azure_resource'
-DEFAULT_PRIMARY_NIC = 'eth0'
 LEASE_FILE = '/var/lib/dhcp/dhclient.eth0.leases'
 DEFAULT_FS = 'ext4'
 # DMI chassis-asset-tag is set static for all azure instances
@@ -247,7 +236,6 @@ def get_resource_disk_on_freebsd(port_id):
 
 # update the FreeBSD specific information
 if util.is_FreeBSD():
-    DEFAULT_PRIMARY_NIC = 'hn0'
     LEASE_FILE = '/var/db/dhclient.leases.hn0'
     DEFAULT_FS = 'freebsd-ufs'
     res_disk = get_resource_disk_on_freebsd(1)
@@ -261,13 +249,6 @@ if util.is_FreeBSD():
 
 BUILTIN_DS_CONFIG = {
     'data_dir': AGENT_SEED_DIR,
-    'set_hostname': True,
-    'hostname_bounce': {
-        'interface': DEFAULT_PRIMARY_NIC,
-        'policy': True,
-        'command': 'builtin',
-        'hostname_command': 'hostname',
-    },
     'disk_aliases': {'ephemeral0': RESOURCE_DISK_PATH},
     'dhclient_lease_file': LEASE_FILE,
     'apply_network_config': True,  # Use IMDS published network configuration
@@ -293,46 +274,6 @@ DEF_EPHEMERAL_LABEL = 'Temporary Storage'
 DEF_PASSWD_REDACTION = 'REDACTED'
 
 
-def get_hostname(hostname_command='hostname'):
-    if not isinstance(hostname_command, (list, tuple)):
-        hostname_command = (hostname_command,)
-    return subp.subp(hostname_command, capture=True)[0].strip()
-
-
-def set_hostname(hostname, hostname_command='hostname'):
-    subp.subp([hostname_command, hostname])
-
-
-@azure_ds_telemetry_reporter
-@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
-    try:
-        set_hostname(temp_hostname, hostname_command)
-    except Exception as e:
-        report_diagnostic_event(
-            'Failed setting temporary hostname: %s' % e,
-            logger_func=LOG.warning)
-        yield None
-        return
-    try:
-        yield previous_hostname
-    finally:
-        set_hostname(previous_hostname, hostname_command)
-
-
 class DataSourceAzure(sources.DataSource):
 
     dsname = 'Azure'
@@ -369,34 +310,6 @@ class DataSourceAzure(sources.DataSource):
         root = sources.DataSource.__str__(self)
         return "%s [seed=%s]" % (root, self.seed)
 
-    @azure_ds_telemetry_reporter
-    def bounce_network_with_azure_hostname(self):
-        # When using cloud-init to provision, we have to set the hostname from
-        # the metadata and "bounce" the network to force DDNS to update via
-        # dhclient
-        azure_hostname = self.metadata.get('local-hostname')
-        LOG.debug("Hostname in metadata is %s", azure_hostname)
-        hostname_command = self.ds_cfg['hostname_bounce']['hostname_command']
-
-        with temporary_hostname(azure_hostname, self.ds_cfg,
-                                hostname_command=hostname_command) \
-                as previous_hn:
-            if (previous_hn is not None and
-                    util.is_true(self.ds_cfg.get('set_hostname'))):
-                cfg = self.ds_cfg['hostname_bounce']
-
-                # "Bouncing" the network
-                try:
-                    return perform_hostname_bounce(hostname=azure_hostname,
-                                                   cfg=cfg,
-                                                   prev_hostname=previous_hn)
-                except Exception as e:
-                    report_diagnostic_event(
-                        "Failed publishing hostname: %s" % e,
-                        logger_func=LOG.warning)
-                    util.logexc(LOG, "handling set_hostname failed")
-        return False
-
     def _get_subplatform(self):
         """Return the subplatform metadata source details."""
         if self.seed.startswith('/dev'):
@@ -1502,9 +1415,6 @@ class DataSourceAzure(sources.DataSource):
            On success, returns a dictionary including 'public_keys'.
            On failure, returns False.
         """
-
-        self.bounce_network_with_azure_hostname()
-
         pubkey_info = None
         ssh_keys_and_source = self._get_public_ssh_keys_and_source()
 
@@ -1764,42 +1674,6 @@ def address_ephemeral_resize(devpath=RESOURCE_DISK_PATH,
 
 
 @azure_ds_telemetry_reporter
-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
-    # Returns True if the network was bounced, False otherwise.
-    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
-    env['old_hostname'] = prev_hostname
-
-    if command == "builtin":
-        if util.is_FreeBSD():
-            command = BOUNCE_COMMAND_FREEBSD
-        elif subp.which('ifup'):
-            command = BOUNCE_COMMAND_IFUP
-        else:
-            LOG.debug(
-                "Skipping network bounce: ifupdown utils aren't present.")
-            # Don't bounce as networkd handles hostname DDNS updates
-            return False
-    LOG.debug("pubhname: publishing hostname [%s]", msg)
-    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=subp.subp,
-                  kwargs={'args': command, 'shell': shell, 'capture': False,
-                          'env': env})
-    return True
-
-
-@azure_ds_telemetry_reporter
 def write_files(datadir, files, dirmode=None):
 
     def _redact_password(cnt, fname):
diff --git a/doc/examples/cloud-config-datasources.txt b/doc/examples/cloud-config-datasources.txt
index d1a4d79e..7a8c4284 100644
--- a/doc/examples/cloud-config-datasources.txt
+++ b/doc/examples/cloud-config-datasources.txt
@@ -45,12 +45,6 @@ datasource:
       instance-id: i-87018aed
       local-hostname: myhost.internal
 
-  Azure:
-    set_hostname: True
-    hostname_bounce:
-      interface: eth0
-      policy: on # [can be 'on', 'off' or 'force']
-
   SmartOS:
     # For KVM guests:
     # Smart OS datasource works over a serial console interacting with
diff --git a/doc/rtd/topics/datasources/azure.rst b/doc/rtd/topics/datasources/azure.rst
index ad9f2236..bc672486 100644
--- a/doc/rtd/topics/datasources/azure.rst
+++ b/doc/rtd/topics/datasources/azure.rst
@@ -60,20 +60,6 @@ The settings that may be configured are:
    custom DHCP option 245 from Azure fabric.
  * **disk_aliases**: A dictionary defining which device paths should be
    interpreted as ephemeral images. See cc_disk_setup module for more info.
- * **hostname_bounce**: A dictionary Azure hostname bounce behavior to react to
-   metadata changes.  The '``hostname_bounce: command``' entry can be either
-   the literal string 'builtin' or a command to execute.  The command will be
-   invoked after the hostname is set, and will have the 'interface' in its
-   environment.  If ``set_hostname`` is not true, then ``hostname_bounce``
-   will be ignored.  An example might be:
-
-     ``command:  ["sh", "-c", "killall dhclient; dhclient $interface"]``
-
- * **hostname_bounce**: A dictionary Azure hostname bounce behavior to react to
-   metadata changes. Azure will throttle ifup/down in some cases after metadata
-   has been updated to inform dhcp server about updated hostnames.
- * **set_hostname**: Boolean set to True when we want Azure to set the hostname
-   based on metadata.
 
 Configuration for the datasource can also be read from a
 ``dscfg`` entry in the ``LinuxProvisioningConfigurationSet``.  Content in
@@ -91,12 +77,6 @@ An example configuration with the default values is provided below:
       dhclient_lease_file: /var/lib/dhcp/dhclient.eth0.leases
       disk_aliases:
         ephemeral0: /dev/disk/cloud/azure_resource
-      hostname_bounce:
-        interface: eth0
-        command: builtin
-        policy: true
-        hostname_command: hostname
-      set_hostname: true
 
 
 Userdata
diff --git a/tests/unittests/sources/test_azure.py b/tests/unittests/sources/test_azure.py
index 9728a1e7..ad8be04b 100644
--- a/tests/unittests/sources/test_azure.py
+++ b/tests/unittests/sources/test_azure.py
@@ -696,9 +696,6 @@ scbus-1 on xpt0 bus 0
         self.apply_patches([
             (dsaz, 'list_possible_azure_ds',
                 self.m_list_possible_azure_ds),
-            (dsaz, 'perform_hostname_bounce', mock.MagicMock()),
-            (dsaz, 'get_hostname', mock.MagicMock()),
-            (dsaz, 'set_hostname', mock.MagicMock()),
             (dsaz, '_is_platform_viable',
                 self.m_is_platform_viable),
             (dsaz, 'get_metadata_from_fabric',
@@ -1794,21 +1791,6 @@ scbus-1 on xpt0 bus 0
         m_net_get_interfaces.assert_called_with(
             blacklist_drivers=dsaz.BLACKLIST_DRIVERS)
 
-    @mock.patch(MOCKPATH + 'subp.subp', autospec=True)
-    def test_get_hostname_with_no_args(self, m_subp):
-        dsaz.get_hostname()
-        m_subp.assert_called_once_with(("hostname",), capture=True)
-
-    @mock.patch(MOCKPATH + 'subp.subp', autospec=True)
-    def test_get_hostname_with_string_arg(self, m_subp):
-        dsaz.get_hostname(hostname_command="hostname")
-        m_subp.assert_called_once_with(("hostname",), capture=True)
-
-    @mock.patch(MOCKPATH + 'subp.subp', autospec=True)
-    def test_get_hostname_with_iterable_arg(self, m_subp):
-        dsaz.get_hostname(hostname_command=("hostname",))
-        m_subp.assert_called_once_with(("hostname",), capture=True)
-
     @mock.patch(
         'cloudinit.sources.helpers.azure.OpenSSLManager.parse_certificates')
     def test_get_public_ssh_keys_with_imds(self, m_parse_certificates):
@@ -2023,251 +2005,6 @@ scbus-1 on xpt0 bus 0
         self.assertEqual(dsrc.userdata_raw, userdataOVF.encode('utf-8'))
 
 
-class TestAzureBounce(CiTestCase):
-
-    with_logs = True
-
-    def mock_out_azure_moving_parts(self):
-
-        def _load_possible_azure_ds(seed_dir, cache_dir):
-            yield seed_dir
-            yield dsaz.DEFAULT_PROVISIONING_ISO_DEV
-            if cache_dir:
-                yield cache_dir
-
-        self.patches.enter_context(
-            mock.patch.object(dsaz.util, 'wait_for_files'))
-        self.patches.enter_context(
-            mock.patch.object(
-                dsaz, 'list_possible_azure_ds',
-                mock.MagicMock(side_effect=_load_possible_azure_ds)))
-        self.patches.enter_context(
-            mock.patch.object(dsaz, 'get_metadata_from_fabric',
-                              mock.MagicMock(return_value={})))
-        self.patches.enter_context(
-            mock.patch.object(dsaz, 'get_metadata_from_imds',
-                              mock.MagicMock(return_value={})))
-        self.patches.enter_context(
-            mock.patch.object(dsaz.subp, 'which', lambda x: True))
-        self.patches.enter_context(mock.patch.object(
-            dsaz, '_get_random_seed', return_value='wild'))
-
-        def _dmi_mocks(key):
-            if key == 'system-uuid':
-                return 'D0DF4C54-4ECB-4A4B-9954-5BDF3ED5C3B8'
-            elif key == 'chassis-asset-tag':
-                return '7783-7084-3265-9085-8269-3286-77'
-            raise RuntimeError('should not get here')
-
-        self.patches.enter_context(
-            mock.patch.object(dsaz.dmi, 'read_dmi_data',
-                              mock.MagicMock(side_effect=_dmi_mocks)))
-
-    def setUp(self):
-        super(TestAzureBounce, self).setUp()
-        self.tmp = self.tmp_dir()
-        self.waagent_d = os.path.join(self.tmp, 'var', 'lib', 'waagent')
-        self.paths = helpers.Paths(
-            {'cloud_dir': self.tmp, 'run_dir': self.tmp})
-        dsaz.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(dsaz, 'get_hostname'))
-        self.set_hostname = self.patches.enter_context(
-            mock.patch.object(dsaz, 'set_hostname'))
-        self.subp = self.patches.enter_context(
-            mock.patch(MOCKPATH + 'subp.subp'))
-        self.find_fallback_nic = self.patches.enter_context(
-            mock.patch('cloudinit.net.find_fallback_nic', return_value='eth9'))
-
-    def tearDown(self):
-        self.patches.close()
-        super(TestAzureBounce, self).tearDown()
-
-    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})
-        dsrc = dsaz.DataSourceAzure({}, distro=mock.Mock(), paths=self.paths)
-        return dsrc
-
-    def _get_and_setup(self, dsrc):
-        ret = dsrc.get_data()
-        if ret:
-            dsrc.setup(True)
-        return ret
-
-    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'}}
-        ds = self._get_ds(self.get_ovf_env_with_dscfg('test-host', cfg))
-        ds.get_data()
-        self.assertEqual(0, self.set_hostname.call_count)
-
-    @mock.patch(MOCKPATH + 'perform_hostname_bounce')
-    def test_disabled_bounce_does_not_perform_bounce(
-            self, perform_hostname_bounce):
-        cfg = {'hostname_bounce': {'policy': 'off'}}
-        ds = self._get_ds(self.get_ovf_env_with_dscfg('test-host', cfg))
-        ds.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'}}
-        ds = self._get_ds(self.get_ovf_env_with_dscfg(host_name, cfg))
-        ds.get_data()
-        self.assertEqual(0, self.set_hostname.call_count)
-
-    @mock.patch(MOCKPATH + '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'}}
-        ds = self._get_ds(self.get_ovf_env_with_dscfg(host_name, cfg))
-        ds.get_data()
-        self.assertEqual(0, perform_hostname_bounce.call_count)
-
-    @mock.patch(MOCKPATH + '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'}}
-        dsrc = self._get_ds(self.get_ovf_env_with_dscfg(host_name, cfg))
-        ret = self._get_and_setup(dsrc)
-        self.assertTrue(ret)
-        self.assertEqual(1, perform_hostname_bounce.call_count)
-
-    def test_bounce_skipped_on_ifupdown_absent(self):
-        host_name = 'unchanged-host-name'
-        self.get_hostname.return_value = host_name
-        cfg = {'hostname_bounce': {'policy': 'force'}}
-        dsrc = self._get_ds(self.get_ovf_env_with_dscfg(host_name, cfg))
-        patch_path = MOCKPATH + 'subp.which'
-        with mock.patch(patch_path) as m_which:
-            m_which.return_value = None
-            ret = self._get_and_setup(dsrc)
-        self.assertEqual([mock.call('ifup')], m_which.call_args_list)
-        self.assertTrue(ret)
-        self.assertIn(
-            "Skipping network bounce: ifupdown utils aren't present.",
-            self.logs.getvalue())
-
-    def test_different_hostnames_sets_hostname(self):
-        expected_hostname = 'azure-expected-host-name'
-        self.get_hostname.return_value = 'default-host-name'
-        dsrc = self._get_ds(
-            self.get_ovf_env_with_dscfg(expected_hostname, {}))
-        ret = self._get_and_setup(dsrc)
-        self.assertTrue(ret)
-        self.assertEqual(expected_hostname,
-                         self.set_hostname.call_args_list[0][0][0])
-
-    @mock.patch(MOCKPATH + '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'
-        dsrc = self._get_ds(
-            self.get_ovf_env_with_dscfg(expected_hostname, {}))
-        ret = self._get_and_setup(dsrc)
-        self.assertTrue(ret)
-        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
-        dsrc = self._get_ds(
-            self.get_ovf_env_with_dscfg('some-host-name', {}))
-        ret = self._get_and_setup(dsrc)
-        self.assertTrue(ret)
-        self.assertEqual(initial_host_name,
-                         self.set_hostname.call_args_list[-1][0][0])
-
-    @mock.patch(MOCKPATH + '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
-        dsrc = self._get_ds(
-            self.get_ovf_env_with_dscfg('some-host-name', {}))
-        ret = self._get_and_setup(dsrc)
-        self.assertTrue(ret)
-        self.assertEqual(initial_host_name,
-                         self.set_hostname.call_args_list[-1][0][0])
-
-    @mock.patch.object(dsaz, 'get_boot_telemetry')
-    def test_environment_correct_for_bounce_command(
-            self, mock_get_boot_telemetry):
-        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)
-        dsrc = self._get_ds(data)
-        ret = self._get_and_setup(dsrc)
-        self.assertTrue(ret)
-        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'])
-
-    @mock.patch.object(dsaz, 'get_boot_telemetry')
-    def test_default_bounce_command_ifup_used_by_default(
-            self, mock_get_boot_telemetry):
-        cfg = {'hostname_bounce': {'policy': 'force'}}
-        data = self.get_ovf_env_with_dscfg('some-hostname', cfg)
-        dsrc = self._get_ds(data)
-        ret = self._get_and_setup(dsrc)
-        self.assertTrue(ret)
-        self.assertEqual(1, self.subp.call_count)
-        bounce_args = self.subp.call_args[1]['args']
-        self.assertEqual(
-            dsaz.BOUNCE_COMMAND_IFUP, bounce_args)
-
-    @mock.patch(MOCKPATH + '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)
-
-    @mock.patch(MOCKPATH + 'perform_hostname_bounce')
-    def test_set_hostname_failed_disable_bounce(
-            self, perform_hostname_bounce):
-        cfg = {'set_hostname': True, 'hostname_bounce': {'policy': 'force'}}
-        self.get_hostname.return_value = "old-hostname"
-        self.set_hostname.side_effect = Exception
-        data = self.get_ovf_env_with_dscfg('some-hostname', cfg)
-        self._get_ds(data).get_data()
-
-        self.assertEqual(0, perform_hostname_bounce.call_count)
-
-
 class TestLoadAzureDsDir(CiTestCase):
     """Tests for load_azure_ds_dir."""