cloud-init-dev team mailing list archive

[Scott Moser <ssmoser2+ubuntu@xxxxxxxxx>]

there are some merge issues shown below ("<<<<<").

I have some comments inline.

How feasible would it be to add support for azure conversion in the net-convert sub-command?


Diff comments:

> diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py
> index 7007d9e..5d5c64e 100644
> --- a/cloudinit/sources/DataSourceAzure.py
> +++ b/cloudinit/sources/DataSourceAzure.py
> @@ -1025,6 +1079,162 @@ def load_azure_ds_dir(source_dir):
>      return (md, ud, cfg, {'ovf-env.xml': contents})
>  
>  
> +def get_metadata_from_imds(fallback_nic, retries):
> +    """Query Azure's network metadata service, returning a dictionary.
> +
> +    If network is not up, setup ephemeral dhcp on fallback_nic to talk to the
> +    IMDS. For more info on IMDS:
> +        https://docs.microsoft.com/en-us/azure/virtual-machines/windows/instance-metadata-service
> +
> +    @param fallback_nic: String. The name of the nic which requires active
> +        networ in order to query IMDS.
> +    @param retries: The number of retries of the IMDS_URL.
> +
> +    @return: A dict of instance metadata containing compute and network
> +        info.
> +    """
> +    if net.is_up(fallback_nic):
> +        return util.log_time(
> +            logfunc=LOG.debug,
> +            msg='Crawl of Azure Instance Metadata Service (IMDS)',
> +            func=_get_metadata_from_imds, args=(retries,))
> +    else:
> +        with EphemeralDHCPv4(fallback_nic):
> +            return util.log_time(
> +                logfunc=LOG.debug,
> +                msg='Crawl of Azure Instance Metadata Service (IMDS)',
> +                func=_get_metadata_from_imds, args=(retries,))
> +
> +
> +def _get_metadata_from_imds(retries):
> +
> +    def retry_on_url_error(msg, exception):
> +        if isinstance(exception, UrlError) and exception.code == 404:
> +            return True  # Continue retries
> +        return False  # Stop retries on all other exceptions, including 404s
> +
> +    url = IMDS_URL + "instance?api-version=2017-12-01"
> +    headers = {"Metadata": "true"}
> +    try:
> +        response = readurl(
> +            url, timeout=1, headers=headers, retries=retries,
> +            exception_cb=retry_on_url_error)
> +    except Exception as e:
> +        LOG.debug('Ignoring IMDS instance metadata: %s', e)
> +        return {}
> +    try:
> +        return util.load_json(str(response))
> +    except json.decoder.JSONDecodeError:
> +        LOG.warning(
> +            'Ignoring non-json IMDS instance metadata: %s', str(response))
> +    return {}
> +
> +
> +def maybe_remove_ubuntu_network_config_scripts(paths=None):
> +    """Remove Azure-specific ubuntu network config for non-primary nics.
> +
> +    @param paths: List of networking scripts or directories to remove when
> +        present.
> +
> +    In certain supported ubuntu images, static udev rules or netplan yaml
> +    config is delivered in the base ubuntu image to support dhcp on any
> +    additional interfaces which get attached by a customer at some point
> +    after initial boot. Since the Azure datasource can now regenerate
> +    network configuration as metadata reports these new devices, we no longer
> +    want the udev rules or netplan's 90-azure-hotplug.yaml to configure
> +    networking on eth1 or greater as it might collide with cloud-init's
> +    configuration.
> +
> +    Remove the any existing extended network scripts if the datasource is
> +    enabled to write network per-boot.
> +    """
> +    if not paths:
> +        paths = UBUNTU_EXTENDED_NETWORK_SCRIPTS
> +    logged = False
> +    for path in paths:
> +        if os.path.exists(path):
> +            if not logged:
> +                LOG.debug(

this feels INFO to me.
it is moderately important  as it could potentially break things and we will only ever do it once.

> +                    'Removing Ubuntu extended network scripts because'
> +                    ' cloud-init updates Azure network configuration on the'
> +                    ' following event: %s.',
> +                    EventType.BOOT)
> +                logged = True
> +            if os.path.isdir(path):
> +                util.del_dir(path)
> +            else:
> +                util.del_file(path)
> +
> +
> +def _is_platform_viable(seed_dir):
> +    """Check platform environment to report if this datasource may run."""
> +    asset_tag = util.read_dmi_data('chassis-asset-tag')
> +    if asset_tag == AZURE_CHASSIS_ASSET_TAG:
> +        return True
> +    LOG.debug("Non-Azure DMI asset tag '%s' discovered.", asset_tag)
> +    for path in (AGENT_SEED_DIR, seed_dir):
> +        if os.path.exists(os.path.join(path, 'ovf-env.xml')):
> +            return True
> +    if util.which('systemd-detect-virt'):
> +        (virt_type, _err) = util.subp(
> +            ['systemd-detect-virt'], rcs=[0, 1], capture=True)
> +        if virt_type.strip() == 'microsoft':
> +            return True
> +    if util.find_devs_with(criteria='LABEL=rd_rdfe_*'):
> +        return True
> +    return False
> +
> +
> +def _parse_network_config(imds_metadata):
> +    """Convert imds_metadata dictionary to network v2 configuration.
> +
> +    Parses network configuration from imds metadata if present or generate
> +    fallback network config excluding mlx4_core devices.
> +
> +    @param: imds_metadata: Dict of content read from IMDS network service.
> +    @return: Dictionary containing network version 2 standard configuration.
> +    """
> +    if imds_metadata != sources.UNSET and imds_metadata:
> +        netconfig = {'version': 2, 'ethernets': {}}
> +        LOG.debug('Azure: generating network configuration from IMDS')
> +        network_metadata = imds_metadata['network']
> +        for idx, intf in enumerate(network_metadata['interface']):
> +            nicname = 'eth{idx}'.format(idx=idx)
> +            dev_config = {}
> +            for addr4 in intf['ipv4']['ipAddress']:
> +                privateIpv4 = addr4['privateIpAddress']
> +                if privateIpv4:
> +                    if dev_config.get('dhcp4', False):
> +                        # Append static address config for nic > 1
> +                        netPrefix = intf['ipv4']['subnet'][0].get(
> +                            'prefix', '24')
> +                        if not dev_config.get('addresses'):
> +                            dev_config['addresses'] = []
> +                        dev_config['addresses'].append(
> +                            '{ip}/{prefix}'.format(
> +                                ip=privateIpv4, prefix=netPrefix))
> +                    else:
> +                        dev_config['dhcp4'] = True
> +            for addr6 in intf['ipv6']['ipAddress']:
> +                privateIpv6 = addr6['privateIpAddress']
> +                if privateIpv6:
> +                    dev_config['dhcp6'] = True
> +                    break
> +            if dev_config:
> +                mac = ':'.join(re.findall(r'..', intf['macAddress']))
> +                dev_config.update(
> +                    {'match': {'macaddress': mac.lower()},
> +                     'set-name': nicname})
> +                netconfig['ethernets'][nicname] = dev_config
> +    else:
> +        blacklist = ['mlx4_core']
> +        LOG.debug('Azure: generating fallback configuration')
> +        # generate a network config, blacklist picking mlx4_core devs
> +        netconfig = net.generate_fallback_config(
> +            blacklist_drivers=blacklist, config_driver=True)
> +    return netconfig
> +
> +
>  class BrokenAzureDataSource(Exception):
>      pass
>  
> diff --git a/tests/unittests/test_datasource/test_azure.py b/tests/unittests/test_datasource/test_azure.py
> index e82716e..a76cc99 100644
> --- a/tests/unittests/test_datasource/test_azure.py
> +++ b/tests/unittests/test_datasource/test_azure.py
> @@ -1398,4 +1651,127 @@ class TestAzureDataSourcePreprovisioning(CiTestCase):
>          self.assertEqual(m_net.call_count, 1)
>  
>  
> +class TestRemoveUbuntuNetworkConfigScripts(CiTestCase):
> +
> +    with_logs = True
> +
> +    def setUp(self):
> +        super(TestRemoveUbuntuNetworkConfigScripts, self).setUp()
> +        self.tmp = self.tmp_dir()
> +
> +    def test_remove_network_scripts_removes_both_files_and_directories(self):
> +        """Any files or directories in paths are removed when present."""
> +        file1 = self.tmp_path('file1', dir=self.tmp)
> +        subdir = self.tmp_path('sub1', dir=self.tmp)
> +        subfile = self.tmp_path('leaf1', dir=subdir)
> +        write_file(file1, 'file1content')
> +        write_file(subfile, 'leafcontent')
> +        dsaz.maybe_remove_ubuntu_network_config_scripts(paths=[subdir, file1])
> +
> +        for path in (file1, subdir, subfile):
> +            self.assertFalse(os.path.exists(path),
> +                             'Found unremoved: %s' % path)
> +
> +        expected_logs = [
> +            'Removing Ubuntu extended network scripts because cloud-init'
> +            ' updates Azure network configuration on the following event:'
> +            ' System boot.',
> +            'Recursively deleting %s' % subdir,
> +            'Attempting to remove %s' % file1]
> +        for log in expected_logs:
> +            self.assertIn(log, self.logs.getvalue())
> +
> +    def test_remove_network_scripts_only_attemts_removal_if_path_exists(self):
> +        """Any files or directories absent are skipped without error."""
> +        dsaz.maybe_remove_ubuntu_network_config_scripts(paths=[
> +            '/not/a/dir/', '/not/a/file'])

might as well use the tmpdir to guarantee that those files do not exist.
ie, i think if i do:
  mkdir -p /not/a/dir

then i will cause this test to fail.

> +        self.assertNotIn('/not/a', self.logs.getvalue())  # No delete logs
> +
> +    @mock.patch('cloudinit.sources.DataSourceAzure.os.path.exists')
> +    def test_remove_network_scripts_default_removes_stock_scripts(self,
> +                                                                  m_exists):
> +        """Azure's stock ubuntu image scripts and artifacts are removed."""
> +        # Report path absent on all to avoid delete operation
> +        m_exists.return_value = False
> +        dsaz.maybe_remove_ubuntu_network_config_scripts()
> +        calls = m_exists.call_args_list
> +        for path in dsaz.UBUNTU_EXTENDED_NETWORK_SCRIPTS:
> +            self.assertIn(mock.call(path), calls)
> +
> +
> +class TestWBIsPlatformViable(CiTestCase):
> +    """White box tests for _is_platform_viable."""
> +    with_logs = True
> +
> +    @mock.patch('cloudinit.sources.DataSourceAzure.util.read_dmi_data')
> +    def test_true_on_non_azure_chassis(self, m_read_dmi_data):
> +        """Return True if DMI chassis-asset-tag is AZURE_CHASSIS_ASSET_TAG."""
> +        m_read_dmi_data.return_value = dsaz.AZURE_CHASSIS_ASSET_TAG
> +        self.assertTrue(dsaz._is_platform_viable('doesnotmatter'))
> +
> +    @mock.patch('cloudinit.sources.DataSourceAzure.os.path.exists')
> +    @mock.patch('cloudinit.sources.DataSourceAzure.util.read_dmi_data')
> +    def test_true_on_azure_ovf_env_in_seed_dir(self, m_read_dmi_data, m_exist):
> +        """Return True if ovf-env.xml exists in known seed dirs."""
> +        # Non-matching Azure chassis-asset-tag
> +        m_read_dmi_data.return_value = dsaz.AZURE_CHASSIS_ASSET_TAG + 'X'
> +
> +        m_exist.side_effect = [False, True]
> +        self.assertTrue(dsaz._is_platform_viable('/other/seed/dir'))
> +        self.assertEqual(
> +            [mock.call('/var/lib/waagent/ovf-env.xml'),
> +             mock.call('/other/seed/dir/ovf-env.xml')],
> +            m_exist.call_args_list)
> +
> +    @mock.patch('cloudinit.sources.DataSourceAzure.util.which')
> +    @mock.patch('cloudinit.sources.DataSourceAzure.util.subp')
> +    def test_true_on_detect_virt_microsoft(self, m_subp, m_which):
> +        """Return True if a partition label prefix rd_rdfe is present."""
> +        m_which.return_value = '/usr/bin/systemd-detect-virt'
> +        m_subp.return_value = ('microsoft', '')

this probably neesd a '\n' on the end.

> +        self.assertTrue(wrap_and_call(
> +            'cloudinit.sources.DataSourceAzure',
> +            {'os.path.exists': False,
> +             # Non-matching Azure chassis-asset-tag
> +             'util.read_dmi_data': dsaz.AZURE_CHASSIS_ASSET_TAG + 'X'},
> +            dsaz._is_platform_viable, 'doesnotmatter'))
> +        m_which.assert_called_once_with('systemd-detect-virt')
> +        m_subp.assert_called_once_with(
> +            ['systemd-detect-virt'], capture=True, rcs=[0, 1])
> +
> +    @mock.patch('cloudinit.sources.DataSourceAzure.util.find_devs_with')
> +    def test_true_on_azure_when_fs_label_prefix_rd_rdfe(self, m_find_devs):
> +        """Return True if a partition label prefix rd_rdfe is present."""
> +
> +        m_find_devs.return_value = ['/dev/Imatched/azure']
> +        self.assertTrue(wrap_and_call(
> +            'cloudinit.sources.DataSourceAzure',
> +            {'os.path.exists': False,
> +             # Non-matching Azure chassis-asset-tag
> +             'util.read_dmi_data': dsaz.AZURE_CHASSIS_ASSET_TAG + 'X',
> +             'util.which': None},
> +            dsaz._is_platform_viable, 'doesnotmatter'))
> +        m_find_devs.assert_called_once_with(criteria='LABEL=rd_rdfe_*')
> +
> +    def test_false_on_no_matching_azure_criteria(self):
> +        """Report non-azure on unmatched asset tag, ovf-env absent and no dev.
> +
> +        Return False when the asset tag doesn't match Azure's static
> +        AZURE_CHASSIS_ASSET_TAG, no ovf-env.xml files exist in known seed dirs
> +        and no devices have a label starting with prefix 'rd_rdfe_'.
> +        """
> +        self.assertFalse(wrap_and_call(
> +            'cloudinit.sources.DataSourceAzure',
> +            {'os.path.exists': False,
> +             # Non-matching Azure chassis-asset-tag
> +             'util.read_dmi_data': dsaz.AZURE_CHASSIS_ASSET_TAG + 'X',
> +             'util.which': None,
> +             'util.find_devs_with': []},
> +            dsaz._is_platform_viable, 'doesnotmatter'))
> +        self.assertIn(
> +            "DEBUG: Non-Azure DMI asset tag '{0}' discovered.\n".format(
> +                dsaz.AZURE_CHASSIS_ASSET_TAG + 'X'),
> +            self.logs.getvalue())
> +
> +
>  # vi: ts=4 expandtab


-- 
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/348704
Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:feature/azure-network-on-boot into cloud-init:master.