cloud-init-dev team mailing list archive

[Scott Moser <smoser@xxxxxxxxxx>]

looks really good. a couple things to tweak.


Diff comments:

> === modified file 'cloudinit/config/cc_apt_configure.py'
> --- cloudinit/config/cc_apt_configure.py	2016-03-03 22:20:10 +0000
> +++ cloudinit/config/cc_apt_configure.py	2016-05-12 14:50:47 +0000
> @@ -158,12 +169,32 @@
>              log.warn("No template found, not rendering /etc/apt/sources.list")
>              return
>  
> -    params = {'codename': codename}
> -    for k in mirrors:
> -        params[k] = mirrors[k]
>      templater.render_to_file(template_fn, '/etc/apt/sources.list', params)
>  
>  
> +def add_key(ent, errorlist):

the behavior of this function seems odd. seems better to raise exceptions and have the caller append it.

> +    """
> +    add key to the system as defiend in entry (if any)
> +    suppords raw keys or keyid's
> +    The latter will as a first step fetched to get the raw key
> +    """
> +    if ('keyid' in ent and 'key' not in ent):
> +        keyserver = "keyserver.ubuntu.com"
> +        if 'keyserver' in ent:
> +            keyserver = ent['keyserver']
> +        try:
> +            ent['key'] = getkeybyid(ent['keyid'], keyserver)
> +        except:
> +            errorlist.append([ent, "failed to get key from %s" % keyserver])
> +            return
> +
> +    if 'key' in ent:
> +        try:
> +            util.subp(('apt-key', 'add', '-'), ent['key'])
> +        except:
> +            errorlist.append([ent, "failed add key"])
> +
> +
>  def add_sources(srclist, template_params=None, aa_repo_match=None):
>      """
>      add entries in /etc/apt/sources.list.d for each abbreviated
> 
> === modified file 'doc/examples/cloud-config.txt'
> --- doc/examples/cloud-config.txt	2015-03-04 17:42:34 +0000
> +++ doc/examples/cloud-config.txt	2016-05-12 14:50:47 +0000
> @@ -111,12 +141,14 @@
>     keyid: F430BBA5 # GPG key ID published on a key server
>     filename: byobu-ppa.list
>  
> + # this would only import the key without adding a ppa or other source spec
> + - keyid: F430BBA5 # GPG key ID published on a key server

can we test that this works with the long key fingerprint (it should). short key fingerprints could allow easy attack by collision.

> +
>   # Custom apt repository:
>   #  * The apt signing key can also be specified
>   #    by providing a pgp public key block
>   #  * Providing the PBG key here is the most robust method for
>   #    specifying a key, as it removes dependency on a remote key server
> -

i think this is wrong. or it was wrong to be there.

>   - source: deb http://ppa.launchpad.net/alestic/ppa/ubuntu karmic main 
>     key: | # The value needs to start with -----BEGIN PGP PUBLIC KEY BLOCK-----
>        -----BEGIN PGP PUBLIC KEY BLOCK-----
> 
> === added file 'tests/unittests/test_handler/test_handler_apt_configure_sources_list.py'
> --- tests/unittests/test_handler/test_handler_apt_configure_sources_list.py	1970-01-01 00:00:00 +0000
> +++ tests/unittests/test_handler/test_handler_apt_configure_sources_list.py	2016-05-12 14:50:47 +0000
> @@ -0,0 +1,163 @@
> +""" test_handler_apt_configure_sources_list
> +Test templating of sources list
> +"""
> +import os
> +import shutil
> +import tempfile
> +import re
> +
> +import logging
> +
> +try:
> +    from unittest import mock
> +except ImportError:
> +    import mock
> +
> +from cloudinit import cloud
> +from cloudinit import distros
> +from cloudinit import util
> +from cloudinit import helpers
> +from cloudinit import templater
> +
> +from cloudinit.sources import DataSourceNone
> +from cloudinit.config import cc_apt_configure
> +
> +from .. import helpers as t_help
> +
> +LOG = logging.getLogger(__name__)
> +
> +YAML_TEXT_CUSTOM_SL = """
> +apt_mirror: http://archive.ubuntu.com/ubuntu/
> +apt_custom_sources_list: |
> +    ## template:jinja
> +    ## Note, this file is written by cloud-init on first boot of an instance
> +    ## modifications made here will not survive a re-bundle.
> +    ## if you wish to make changes you can:
> +    ## a.) add 'apt_preserve_sources_list: true' to /etc/cloud/cloud.cfg
> +    ##     or do the same in user-data
> +    ## b.) add sources in /etc/apt/sources.list.d
> +    ## c.) make changes to template file /etc/cloud/templates/sources.list.tmpl
> +
> +    # See http://help.ubuntu.com/community/UpgradeNotes for how to upgrade to
> +    # newer versions of the distribution.
> +    deb {{mirror}} {{codename}} main restricted
> +    deb-src {{mirror}} {{codename}} main restricted
> +    # FIND_SOMETHING_SPECIAL
> +"""
> +
> +
> +def load_tfile_or_url(*args, **kwargs):
> +    """ load_tfile_or_url
> +    load file and return content after decoding
> +    """
> +    return util.decode_binary(util.read_file_or_url(*args, **kwargs).contents)
> +
> +
> +class TestAptSourceConfigSourceList(t_help.FilesystemMockingTestCase):
> +    """ TestAptSourceConfigSourceList
> +    Main Class to test sources list rendering
> +    """
> +    def setUp(self):
> +        super(TestAptSourceConfigSourceList, self).setUp()
> +        self.subp = util.subp
> +        self.new_root = tempfile.mkdtemp()
> +        self.addCleanup(shutil.rmtree, self.new_root)
> +
> +    def _get_cloud(self, distro, metadata=None):
> +        self.patchUtils(self.new_root)
> +        paths = helpers.Paths({})
> +        cls = distros.fetch(distro)
> +        mydist = cls(distro, {}, paths)
> +        myds = DataSourceNone.DataSourceNone({}, mydist, paths)
> +        if metadata:
> +            myds.metadata.update(metadata)
> +        return cloud.Cloud(myds, paths, {}, mydist, None)
> +
> +    def apt_source_list(self, distro, mirror, mirrorcheck=None):
> +        """ apt_source_list
> +        Test rendering of a source.list from template for a given distro
> +        """
> +        if mirrorcheck is None:
> +            mirrorcheck = mirror
> +
> +        if isinstance(mirror, list):
> +            cfg = {'apt_mirror_search': mirror}
> +        else:
> +            cfg = {'apt_mirror': mirror}
> +        mycloud = self._get_cloud(distro)
> +
> +        with mock.patch.object(templater, 'render_to_file') as mocktmpl:
> +            with mock.patch.object(os.path, 'isfile',
> +                                   return_value=True) as mockisfile:
> +                cc_apt_configure.handle("notimportant", cfg, mycloud,
> +                                        LOG, None)
> +
> +        mockisfile.assert_any_call(('/etc/cloud/templates/'
> +                                    'sources.list.%s.tmpl' % distro))
> +        mocktmpl.assert_called_once_with(('/etc/cloud/templates/'

indentation coudl be improved i think.

> +                                          'sources.list.%s.tmpl' % distro),
> +                                         '/etc/apt/sources.list',
> +                                         {'codename': '',
> +                                          'primary':
> +                                          mirrorcheck,
> +                                          'mirror':
> +                                          mirrorcheck})
> +
> +    def test_apt_source_list_debian(self):
> +        """ test_apt_source_list_debian
> +        Test rendering of a source.list from template for debian
> +        """
> +        self.apt_source_list('debian', 'http://httpredir.debian.org/debian')
> +
> +    def test_apt_source_list_ubuntu(self):
> +        """ test_apt_source_list_ubuntu
> +        Test rendering of a source.list from template for ubuntu
> +        """
> +        self.apt_source_list('ubuntu', 'http://archive.ubuntu.com/ubuntu/')
> +
> +    def test_apt_srcl_debian_mirrorfail(self):
> +        """ test_apt_source_list_debian_mirrorfail
> +        Test rendering of a source.list from template for debian
> +        """
> +        self.apt_source_list('debian', ['http://does.not.exist',
> +                                        'http://httpredir.debian.org/debian'],
> +                             'http://httpredir.debian.org/debian')
> +
> +    def test_apt_srcl_ubuntu_mirrorfail(self):
> +        """ test_apt_source_list_ubuntu_mirrorfail
> +        Test rendering of a source.list from template for ubuntu
> +        """
> +        self.apt_source_list('ubuntu', ['http://does.not.exist',
> +                                        'http://archive.ubuntu.com/ubuntu/'],
> +                             'http://archive.ubuntu.com/ubuntu/')
> +
> +    def test_apt_srcl_custom(self):
> +        """ test_apt_srcl_custom
> +        Test rendering from a custom source.list template
> +        """
> +        cfg = util.load_yaml(YAML_TEXT_CUSTOM_SL)
> +        mycloud = self._get_cloud('ubuntu')
> +
> +        # the second mock restores the original subp
> +        with mock.patch.object(util, 'write_file') as mockwrite:
> +            with mock.patch.object(util, 'subp', self.subp) as mocksubp:
> +                cc_apt_configure.handle("notimportant", cfg, mycloud,
> +                                        LOG, None)
> +
> +        mockwrite.assert_called_once_with(

this looks a bit odd. probably could do that better.

> +            '/etc/apt/sources.list',
> +            ("## Note, this file is written by cloud-init on first boot of an"
> +             " instance\n## modifications made here will not survive a re-bun"
> +             "dle.\n## if you wish to make changes you can:\n## a.) add 'apt_"
> +             "preserve_sources_list: true' to /etc/cloud/cloud.cfg\n##     or"
> +             " do the same in user-data\n## b.) add sources in /etc/apt/sourc"
> +             "es.list.d\n## c.) make changes to template file /etc/cloud/temp"
> +             "lates/sources.list.tmpl\n\n# See http://help.ubuntu.com/communi";
> +             "ty/UpgradeNotes for how to upgrade to\n# newer versions of the "
> +             "distribution.\ndeb http://archive.ubuntu.com/ubuntu/ xenial mai"
> +             "n restricted\ndeb-src http://archive.ubuntu.com/ubuntu/ xenial "
> +             "main restricted\n# FIND_SOMETHING_SPECIAL\n"),
> +            mode=420)
> +
> +
> +# vi: ts=4 expandtab


-- 
https://code.launchpad.net/~paelzer/cloud-init/test-apt-source/+merge/294521
Your team cloud init development team is requested to review the proposed merge of lp:~paelzer/cloud-init/test-apt-source into lp:cloud-init.