Merge lp:~paelzer/cloud-init/bug-1589174-fix-tests-in-adt-env into lp:~cloud-init-dev/cloud-init/trunk

[ChristianEhrhardt]

Fixes to the unittests to run in more environments;
As well as some improvements that were found along testing them and due to the fact that we review some of that code again in the scope of curtin currently.

Tests:
- add a test for an alternate keyserver
- harden mirrorfail tests to detect and skip if no network is available
- improve apt_source related tests to work on CentOS7

Changes:
- gpg key handling is now in python instead of a shell blob
- packages/bddeb has an option to sign as someone else than smoser
- make exception handling of apt_source features more specific (i.e. no catch Exception)
- rename some functions to relfect better what they actually do
- capture some helper subp calls output to avoid spilling into stdout when not intended

Fixes to the unittests to run in more environments;
As well as some improvements that were found along testing them and due to the fact that we review some of that code again in the scope of curtin currently.

Tests:
- add a test for an alternate keyserver
- harden mirrorfail tests to detect and skip if no network is available
- improve apt_source related tests to work on CentOS7

Changes:
- gpg key handling is now in python instead of a shell blob
- packages/bddeb has an option to sign as someone else than smoser
- make exception handling of apt_source features more specific (i.e. no catch Exception)
- rename some functions to relfect better what they actually do
- capture some helper subp calls output to avoid spilling into stdout when not intended

[lp:~paelzer/cloud-init/bug-1589174-fix-tests-in-adt-env]

1246. By ChristianEhrhardt on 2016-06-07

merge with upstream to avoid merge conflicts on the merge proposal

[ChristianEhrhardt (paelzer)]

I hope it merges without conflict now

[Scott Moser (smoser)]

mostly looks good. a couple comments. thank you.

[lp:~paelzer/cloud-init/bug-1589174-fix-tests-in-adt-env]

1247. By ChristianEhrhardt on 2016-06-09

move gpg functions into gpg.py

This helps for cleaner code structuring.
ALong that makeing sure all these functions have a gpg_prefix.

1248. By ChristianEhrhardt on 2016-06-09

improve error handling and reporting in gpg functions

1249. By ChristianEhrhardt on 2016-06-09

move SkipTest definition to tests/unittests/helpers.py to be reusable

1250. By ChristianEhrhardt on 2016-06-09

remove unused BIN_APT constant

1251. By ChristianEhrhardt on 2016-06-09

fix docstring for check connectivity

1252. By ChristianEhrhardt on 2016-06-09

mock is_resolvable in mirrorfail tests to remove dependency to external net

[ChristianEhrhardt (paelzer)]

Thanks, addressed all you mentioned - will throw it through more testing now.

[ChristianEhrhardt (paelzer)]

Ok, fine in sbuild, a partially shutdown network env and buildd (https://launchpad.net/~paelzer/+archive/ubuntu/derived-repository-testbuilds/+build/9891133), no merge conflict atm - please reconsider for merging

=== modified file 'cloudinit/config/cc_apt_configure.py'
--- cloudinit/config/cc_apt_configure.py	2016-06-03 19:27:32 +0000
+++ cloudinit/config/cc_apt_configure.py	2016-06-09 08:37:01 +0000
@@ -24,6 +24,7 @@
 
 from cloudinit import templater
 from cloudinit import util
+from cloudinit import gpg
 
 distros = ['ubuntu', 'debian']
 
@@ -34,21 +35,6 @@
 # this will match 'XXX:YYY' (ie, 'cloud-archive:foo' or 'ppa:bar')
 ADD_APT_REPO_MATCH = r"^[\w-]+:\w"
 
-# A temporary shell program to get a given gpg key
-# from a given keyserver
-EXPORT_GPG_KEYID = """
-    k=${1} ks=${2};
-    exec 2>/dev/null
-    [ -n "$k" ] || exit 1;
-    armour=$(gpg --export --armour "${k}")
-    if [ -z "${armour}" ]; then
-       gpg --keyserver ${ks} --recv "${k}" >/dev/null &&
-          armour=$(gpg --export --armour "${k}") &&
-          gpg --batch --yes --delete-keys "${k}"
-    fi
-    [ -n "${armour}" ] && echo "${armour}"
-"""
-
 
 def handle(name, cfg, cloud, log, _args):
     if util.is_false(cfg.get('apt_configure_enabled', True)):
@@ -94,8 +80,8 @@
             def matcher(x):
                 return False
 
-        errors = add_sources(cfg['apt_sources'], params,
-                             aa_repo_match=matcher)
+        errors = add_apt_sources(cfg['apt_sources'], params,
+                                 aa_repo_match=matcher)
         for e in errors:
             log.warn("Add source error: %s", ':'.join(e))
 
@@ -108,17 +94,7 @@
             util.logexc(log, "Failed to run debconf-set-selections")
 
 
-# get gpg keyid from keyserver
-def getkeybyid(keyid, keyserver):
-    with util.ExtendedTemporaryFile(suffix='.sh', mode="w+", ) as fh:
-        fh.write(EXPORT_GPG_KEYID)
-        fh.flush()
-        cmd = ['/bin/sh', fh.name, keyid, keyserver]
-        (stdout, _stderr) = util.subp(cmd)
-        return stdout.strip()
-
-
-def mirror2lists_fileprefix(mirror):
+def mirrorurl_to_apt_fileprefix(mirror):
     string = mirror
     # take off http:// or ftp://
     if string.endswith("/"):
@@ -135,8 +111,8 @@
         nmirror = new_mirrors.get(name)
         if not nmirror:
             continue
-        oprefix = os.path.join(lists_d, mirror2lists_fileprefix(omirror))
-        nprefix = os.path.join(lists_d, mirror2lists_fileprefix(nmirror))
+        oprefix = os.path.join(lists_d, mirrorurl_to_apt_fileprefix(omirror))
+        nprefix = os.path.join(lists_d, mirrorurl_to_apt_fileprefix(nmirror))
         if oprefix == nprefix:
             continue
         olen = len(oprefix)
@@ -171,7 +147,7 @@
     templater.render_to_file(template_fn, '/etc/apt/sources.list', params)
 
 
-def add_key_raw(key):
+def add_apt_key_raw(key):
     """
     actual adding of a key as defined in key argument
     to the system
@@ -179,10 +155,10 @@
     try:
         util.subp(('apt-key', 'add', '-'), key)
     except util.ProcessExecutionError:
-        raise Exception('failed add key')
-
-
-def add_key(ent):
+        raise ValueError('failed to add apt GPG Key to apt keyring')
+
+
+def add_apt_key(ent):
     """
     add key to the system as defined in ent (if any)
     supports raw keys or keyid's
@@ -192,10 +168,10 @@
         keyserver = "keyserver.ubuntu.com"
         if 'keyserver' in ent:
             keyserver = ent['keyserver']
-        ent['key'] = getkeybyid(ent['keyid'], keyserver)
+        ent['key'] = gpg.gpg_getkeybyid(ent['keyid'], keyserver)
 
     if 'key' in ent:
-        add_key_raw(ent['key'])
+        add_apt_key_raw(ent['key'])
 
 
 def convert_to_new_format(srclist):
@@ -222,7 +198,7 @@
     return srcdict
 
 
-def add_sources(srclist, template_params=None, aa_repo_match=None):
+def add_apt_sources(srclist, template_params=None, aa_repo_match=None):
     """
     add entries in /etc/apt/sources.list.d for each abbreviated
     sources.list entry in 'srclist'.  When rendering template, also
@@ -245,8 +221,8 @@
 
         # keys can be added without specifying a source
         try:
-            add_key(ent)
-        except Exception as detail:
+            add_apt_key(ent)
+        except ValueError as detail:
             errorlist.append([ent, detail])
 
         if 'source' not in ent:

=== added file 'cloudinit/gpg.py'
--- cloudinit/gpg.py	1970-01-01 00:00:00 +0000
+++ cloudinit/gpg.py	2016-06-09 08:37:01 +0000
@@ -0,0 +1,76 @@
+"""gpg.py - Collection of gpg key related functions"""
+# vi: ts=4 expandtab
+#
+#    Copyright (C) 2016 Canonical Ltd.
+#
+#    Author: Scott Moser <[e-mail]>
+#    Author: Juerg Haefliger <[e-mail]>
+#    Author: Joshua Harlow <[e-mail]>
+#    Author: Christian Ehrhardt <[e-mail]>
+#
+#    This program is free software: you can redistribute it and/or modify
+#    it under the terms of the GNU General Public License version 3, as
+#    published by the Free Software Foundation.
+#
+#    This program is distributed in the hope that it will be useful,
+#    but WITHOUT ANY WARRANTY; without even the implied warranty of
+#    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+#    GNU General Public License for more details.
+#
+#    You should have received a copy of the GNU General Public License
+#    along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+from cloudinit import util
+from cloudinit import log as logging
+
+LOG = logging.getLogger(__name__)
+
+
+def gpg_export_armour(key):
+    """Export gpg key, armoured key gets returned"""
+    try:
+        (armour, _) = util.subp(["gpg", "--export", "--armour", key],
+                                capture=True)
+    except util.ProcessExecutionError as error:
+        # debug, since it happens for any key not on the system initially
+        LOG.debug('Failed to export armoured key "%s": %s', key, error)
+        armour = None
+    return armour
+
+
+def gpg_recv_key(key, keyserver):
+    """Receive gpg key from the specified keyserver"""
+    LOG.debug('Receive gpg key "%s"', key)
+    try:
+        util.subp(["gpg", "--keyserver", keyserver, "--recv", key],
+                  capture=True)
+    except util.ProcessExecutionError as error:
+        raise ValueError(('Failed to import key "%s" '
+                          'from server "%s" - error %s') %
+                         (key, keyserver, error))
+
+
+def gpg_delete_key(key):
+    """Delete the specified key from the local gpg ring"""
+    try:
+        util.subp(["gpg", "--batch", "--yes", "--delete-keys", key],
+                  capture=True)
+    except util.ProcessExecutionError as error:
+        LOG.warn('Failed delete key "%s": %s', key, error)
+
+
+def gpg_getkeybyid(keyid, keyserver):
+    """get gpg keyid from keyserver"""
+    armour = gpg_export_armour(keyid)
+    if not armour:
+        try:
+            gpg_recv_key(keyid, keyserver=keyserver)
+            armour = gpg_export_armour(keyid)
+        except ValueError:
+            LOG.exception('Failed to obtain gpg key %s', keyid)
+            raise
+        finally:
+            # delete just imported key to leave environment as it was before
+            gpg_delete_key(keyid)
+
+    return armour.rstrip('\n')

=== modified file 'packages/bddeb'
--- packages/bddeb	2016-05-26 15:51:38 +0000
+++ packages/bddeb	2016-06-09 08:37:01 +0000
@@ -148,11 +148,17 @@
     parser.add_argument("--sign", default=False, action='store_true',
                         help="sign result. do not pass -us -uc to debuild")
 
+    parser.add_argument("--signuser", default=False, action='store',
+                        help="user to sign, see man dpkg-genchanges")
+
     args = parser.parse_args()
 
     if not args.sign:
         args.debuild_args.extend(['-us', '-uc'])
 
+    if args.signuser:
+        args.debuild_args.extend(['-e%s' % args.signuser])
+
     os.environ['INIT_SYSTEM'] = args.init_system
 
     capture = True

=== modified file 'tests/unittests/test_handler/test_handler_apt_configure_sources_list.py'
--- tests/unittests/test_handler/test_handler_apt_configure_sources_list.py	2016-06-05 00:50:37 +0000
+++ tests/unittests/test_handler/test_handler_apt_configure_sources_list.py	2016-06-09 08:37:01 +0000
@@ -20,6 +20,8 @@
 from cloudinit.config import cc_apt_configure
 from cloudinit.sources import DataSourceNone
 
+from cloudinit.distros.debian import Distro
+
 from .. import helpers as t_help
 
 LOG = logging.getLogger(__name__)
@@ -115,38 +117,47 @@
             {'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
-        """
+        """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
-        """
+        """Test rendering of a source.list from template for ubuntu"""
         self.apt_source_list('ubuntu', 'http://archive.ubuntu.com/ubuntu/')
 
-    @t_help.skipIf(True, "LP: #1589174")
+    @staticmethod
+    def myresolve(name):
+        """Fake util.is_resolvable for mirrorfail tests"""
+        if name == "does.not.exist":
+            print("Faking FAIL for '%s'" % name)
+            return False
+        else:
+            print("Faking SUCCESS for '%s'" % name)
+            return True
+
     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')
+        """Test rendering of a source.list from template for debian"""
+        with mock.patch.object(util, 'is_resolvable',
+                               side_effect=self.myresolve) as mockresolve:
+            self.apt_source_list('debian',
+                                 ['http://does.not.exist',
+                                  'http://httpredir.debian.org/debian'],
+                                 'http://httpredir.debian.org/debian')
+        mockresolve.assert_any_call("does.not.exist")
+        mockresolve.assert_any_call("httpredir.debian.org")
 
     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/')
+        """Test rendering of a source.list from template for ubuntu"""
+        with mock.patch.object(util, 'is_resolvable',
+                               side_effect=self.myresolve) as mockresolve:
+            self.apt_source_list('ubuntu',
+                                 ['http://does.not.exist',
+                                  'http://archive.ubuntu.com/ubuntu/'],
+                                 'http://archive.ubuntu.com/ubuntu/')
+        mockresolve.assert_any_call("does.not.exist")
+        mockresolve.assert_any_call("archive.ubuntu.com")
 
     def test_apt_srcl_custom(self):
-        """test_apt_srcl_custom
-        Test rendering from a custom source.list template
-        """
+        """Test rendering from a custom source.list template"""
         cfg = util.load_yaml(YAML_TEXT_CUSTOM_SL)
         mycloud = self._get_cloud('ubuntu')
 
@@ -155,8 +166,10 @@
             with mock.patch.object(util, 'subp', self.subp):
                 with mock.patch.object(cc_apt_configure, 'get_release',
                                        return_value='fakerelease'):
-                    cc_apt_configure.handle("notimportant", cfg, mycloud,
-                                            LOG, None)
+                    with mock.patch.object(Distro, 'get_primary_arch',
+                                           return_value='amd64'):
+                        cc_apt_configure.handle("notimportant", cfg, mycloud,
+                                                LOG, None)
 
         mockwrite.assert_called_once_with(
             '/etc/apt/sources.list',

=== modified file 'tests/unittests/test_handler/test_handler_apt_source.py'
--- tests/unittests/test_handler/test_handler_apt_source.py	2016-06-04 02:56:45 +0000
+++ tests/unittests/test_handler/test_handler_apt_source.py	2016-06-09 08:37:01 +0000
@@ -14,6 +14,7 @@
 
 from cloudinit.config import cc_apt_configure
 from cloudinit import util
+from cloudinit import gpg
 
 from ..helpers import TestCase
 
@@ -62,7 +63,8 @@
         get_rel.return_value = self.release
         self.addCleanup(patcher.stop)
 
-    def _get_default_params(self):
+    @staticmethod
+    def _get_default_params():
         """get_default_params
         Get the most basic default mrror and release info to be used in tests
         """
@@ -86,7 +88,7 @@
         """
         params = self._get_default_params()
 
-        cc_apt_configure.add_sources(cfg, params)
+        cc_apt_configure.add_apt_sources(cfg, params)
 
         self.assertTrue(os.path.isfile(filename))
 
@@ -98,10 +100,7 @@
                                   contents, flags=re.IGNORECASE))
 
     def test_apt_src_basic(self):
-        """test_apt_src_basic
-        Test Fix deb source string, has to overwrite mirror conf in params.
-        Test with a filename provided in config.
-        """
+        """Test deb source string, overwrite mirror and filename"""
         cfg = {'source': ('deb http://archive.ubuntu.com/ubuntu'
                           ' karmic-backports'
                           ' main universe multiverse restricted'),
@@ -109,11 +108,7 @@
         self.apt_src_basic(self.aptlistfile, [cfg])
 
     def test_apt_src_basic_dict(self):
-        """test_apt_src_basic_dict
-        Test Fix deb source string, has to overwrite mirror conf in params.
-        Test with a filename provided in config.
-        Provided in a dictionary with filename being the key (new format)
-        """
+        """Test deb source string, overwrite mirror and filename (dict)"""
         cfg = {self.aptlistfile: {'source':
                                   ('deb http://archive.ubuntu.com/ubuntu'
                                    ' karmic-backports'
@@ -143,10 +138,7 @@
                                   contents, flags=re.IGNORECASE))
 
     def test_apt_src_basic_tri(self):
-        """test_apt_src_basic_tri
-        Test Fix three deb source string, has to overwrite mirror conf in
-        params. Test with filenames provided in config.
-        """
+        """Test Fix three deb source string with filenames"""
         cfg1 = {'source': ('deb http://archive.ubuntu.com/ubuntu'
                            ' karmic-backports'
                            ' main universe multiverse restricted'),
@@ -162,11 +154,7 @@
         self.apt_src_basic_tri([cfg1, cfg2, cfg3])
 
     def test_apt_src_basic_dict_tri(self):
-        """test_apt_src_basic_dict_tri
-        Test Fix three deb source string, has to overwrite mirror conf in
-        params. Test with filenames provided in config.
-        Provided in a dictionary with filename being the key (new format)
-        """
+        """Test Fix three deb source string with filenames (dict)"""
         cfg = {self.aptlistfile: {'source':
                                   ('deb http://archive.ubuntu.com/ubuntu'
                                    ' karmic-backports'
@@ -182,10 +170,7 @@
         self.apt_src_basic_tri(cfg)
 
     def test_apt_src_basic_nofn(self):
-        """test_apt_src_basic_nofn
-        Test Fix deb source string, has to overwrite mirror conf in params.
-        Test without a filename provided in config and test for known fallback.
-        """
+        """Test Fix three deb source string without filenames (dict)"""
         cfg = {'source': ('deb http://archive.ubuntu.com/ubuntu'
                           ' karmic-backports'
                           ' main universe multiverse restricted')}
@@ -197,7 +182,7 @@
         Test Autoreplacement of MIRROR and RELEASE in source specs
         """
         params = self._get_default_params()
-        cc_apt_configure.add_sources(cfg, params)
+        cc_apt_configure.add_apt_sources(cfg, params)
 
         self.assertTrue(os.path.isfile(filename))
 
@@ -208,10 +193,7 @@
                                   contents, flags=re.IGNORECASE))
 
     def test_apt_src_replace(self):
-        """test_apt_src_replace
-        Test Autoreplacement of MIRROR and RELEASE in source specs with
-        Filename being set
-        """
+        """Test Autoreplacement of MIRROR and RELEASE in source specs"""
         cfg = {'source': 'deb $MIRROR $RELEASE multiverse',
                'filename': self.aptlistfile}
         self.apt_src_replacement(self.aptlistfile, [cfg])
@@ -237,10 +219,7 @@
                                   contents, flags=re.IGNORECASE))
 
     def test_apt_src_replace_tri(self):
-        """test_apt_src_replace_tri
-        Test three autoreplacements of MIRROR and RELEASE in source specs with
-        Filename being set
-        """
+        """Test triple Autoreplacement of MIRROR and RELEASE in source specs"""
         cfg1 = {'source': 'deb $MIRROR $RELEASE multiverse',
                 'filename': self.aptlistfile}
         cfg2 = {'source': 'deb $MIRROR $RELEASE main',
@@ -250,13 +229,7 @@
         self.apt_src_replace_tri([cfg1, cfg2, cfg3])
 
     def test_apt_src_replace_dict_tri(self):
-        """test_apt_src_replace_dict_tri
-        Test three autoreplacements of MIRROR and RELEASE in source specs with
-        Filename being set
-        Provided in a dictionary with filename being the key (new format)
-        We also test a new special conditions of the new format that allows
-        filenames to be overwritten inside the directory entry.
-        """
+        """Test triple Autoreplacement in source specs (dict)"""
         cfg = {self.aptlistfile: {'source': 'deb $MIRROR $RELEASE multiverse'},
                'notused': {'source': 'deb $MIRROR $RELEASE main',
                            'filename': self.aptlistfile2},
@@ -264,10 +237,7 @@
         self.apt_src_replace_tri(cfg)
 
     def test_apt_src_replace_nofn(self):
-        """test_apt_src_replace_nofn
-        Test Autoreplacement of MIRROR and RELEASE in source specs with
-        No filename being set
-        """
+        """Test Autoreplacement of MIRROR and RELEASE in source specs nofile"""
         cfg = {'source': 'deb $MIRROR $RELEASE multiverse'}
         with mock.patch.object(os.path, 'join', side_effect=self.myjoin):
             self.apt_src_replacement(self.fallbackfn, [cfg])
@@ -280,7 +250,7 @@
 
         with mock.patch.object(util, 'subp',
                                return_value=('fakekey 1234', '')) as mockobj:
-            cc_apt_configure.add_sources(cfg, params)
+            cc_apt_configure.add_apt_sources(cfg, params)
 
         # check if it added the right ammount of keys
         calls = []
@@ -299,9 +269,7 @@
                                   contents, flags=re.IGNORECASE))
 
     def test_apt_src_keyid(self):
-        """test_apt_src_keyid
-        Test specification of a source + keyid with filename being set
-        """
+        """Test specification of a source + keyid with filename being set"""
         cfg = {'source': ('deb '
                           'http://ppa.launchpad.net/'
                           'smoser/cloud-init-test/ubuntu'
@@ -311,10 +279,7 @@
         self.apt_src_keyid(self.aptlistfile, [cfg], 1)
 
     def test_apt_src_keyid_tri(self):
-        """test_apt_src_keyid_tri
-        Test specification of a source + keyid with filename being set
-        Setting three of such, check for content and keys
-        """
+        """Test 3x specification of a source + keyid with filename being set"""
         cfg1 = {'source': ('deb '
                            'http://ppa.launchpad.net/'
                            'smoser/cloud-init-test/ubuntu'
@@ -351,9 +316,7 @@
                                   contents, flags=re.IGNORECASE))
 
     def test_apt_src_keyid_nofn(self):
-        """test_apt_src_keyid_nofn
-        Test specification of a source + keyid without filename being set
-        """
+        """Test specification of a source + keyid without filename being set"""
         cfg = {'source': ('deb '
                           'http://ppa.launchpad.net/'
                           'smoser/cloud-init-test/ubuntu'
@@ -369,7 +332,7 @@
         params = self._get_default_params()
 
         with mock.patch.object(util, 'subp') as mockobj:
-            cc_apt_configure.add_sources([cfg], params)
+            cc_apt_configure.add_apt_sources([cfg], params)
 
         mockobj.assert_called_with(('apt-key', 'add', '-'), 'fakekey 4321')
 
@@ -384,9 +347,7 @@
                                   contents, flags=re.IGNORECASE))
 
     def test_apt_src_key(self):
-        """test_apt_src_key
-        Test specification of a source + key with filename being set
-        """
+        """Test specification of a source + key with filename being set"""
         cfg = {'source': ('deb '
                           'http://ppa.launchpad.net/'
                           'smoser/cloud-init-test/ubuntu'
@@ -396,9 +357,7 @@
         self.apt_src_key(self.aptlistfile, cfg)
 
     def test_apt_src_key_nofn(self):
-        """test_apt_src_key_nofn
-        Test specification of a source + key without filename being set
-        """
+        """Test specification of a source + key without filename being set"""
         cfg = {'source': ('deb '
                           'http://ppa.launchpad.net/'
                           'smoser/cloud-init-test/ubuntu'
@@ -408,15 +367,13 @@
             self.apt_src_key(self.fallbackfn, cfg)
 
     def test_apt_src_keyonly(self):
-        """test_apt_src_keyonly
-        Test specification key without source
-        """
+        """Test specifying key without source"""
         params = self._get_default_params()
         cfg = {'key': "fakekey 4242",
                'filename': self.aptlistfile}
 
         with mock.patch.object(util, 'subp') as mockobj:
-            cc_apt_configure.add_sources([cfg], params)
+            cc_apt_configure.add_apt_sources([cfg], params)
 
         mockobj.assert_called_once_with(('apt-key', 'add', '-'),
                                         'fakekey 4242')
@@ -425,66 +382,68 @@
         self.assertFalse(os.path.isfile(self.aptlistfile))
 
     def test_apt_src_keyidonly(self):
-        """test_apt_src_keyidonly
-        Test specification of a keyid without source
-        """
+        """Test specification of a keyid without source"""
         params = self._get_default_params()
         cfg = {'keyid': "03683F77",
                'filename': self.aptlistfile}
 
         with mock.patch.object(util, 'subp',
                                return_value=('fakekey 1212', '')) as mockobj:
-            cc_apt_configure.add_sources([cfg], params)
+            cc_apt_configure.add_apt_sources([cfg], params)
 
         mockobj.assert_called_with(('apt-key', 'add', '-'), 'fakekey 1212')
 
         # filename should be ignored on key only
         self.assertFalse(os.path.isfile(self.aptlistfile))
 
+    def apt_src_keyid_real(self, cfg, expectedkey):
+        """apt_src_keyid_real
+        Test specification of a keyid without source including
+        up to addition of the key (add_apt_key_raw mocked to keep the
+        environment as is)
+        """
+        params = self._get_default_params()
+
+        with mock.patch.object(cc_apt_configure, 'add_apt_key_raw') as mockkey:
+            with mock.patch.object(gpg, 'gpg_getkeybyid',
+                                   return_value=expectedkey) as mockgetkey:
+                cc_apt_configure.add_apt_sources([cfg], params)
+
+        mockgetkey.assert_called_with(cfg['keyid'],
+                                      cfg.get('keyserver',
+                                              'keyserver.ubuntu.com'))
+        mockkey.assert_called_with(expectedkey)
+
+        # filename should be ignored on key only
+        self.assertFalse(os.path.isfile(self.aptlistfile))
+
     def test_apt_src_keyid_real(self):
-        """test_apt_src_keyid_real
-        Test specification of a keyid without source incl
-        up to addition of the key (add_key_raw, getkeybyid mocked)
-        """
+        """test_apt_src_keyid_real - Test keyid including key add"""
         keyid = "03683F77"
-        params = self._get_default_params()
         cfg = {'keyid': keyid,
                'filename': self.aptlistfile}
 
-        with mock.patch.object(cc_apt_configure, 'add_key_raw') as mockobj:
-            with mock.patch.object(cc_apt_configure, 'getkeybyid') as gkbi:
-                gkbi.return_value = EXPECTEDKEY
-                cc_apt_configure.add_sources([cfg], params)
-
-        mockobj.assert_called_with(EXPECTEDKEY)
-
-        # filename should be ignored on key only
-        self.assertFalse(os.path.isfile(self.aptlistfile))
+        self.apt_src_keyid_real(cfg, EXPECTEDKEY)
 
     def test_apt_src_longkeyid_real(self):
-        """test_apt_src_longkeyid_real
-        Test specification of a long key fingerprint without source incl
-        up to addition of the key (nothing but add_key_raw mocked)
-        """
-        keyid = "B59D 5F15 97A5 04B7 E230  6DCA 0620 BBCF 0368 3F77"
-        params = self._get_default_params()
-        cfg = {'keyid': keyid,
-               'filename': self.aptlistfile}
-
-        with mock.patch.object(cc_apt_configure, 'add_key_raw') as mockobj:
-            with mock.patch.object(cc_apt_configure, 'getkeybyid') as gkbi:
-                gkbi.return_value = EXPECTEDKEY
-                cc_apt_configure.add_sources([cfg], params)
-
-        mockobj.assert_called_with(EXPECTEDKEY)
-
-        # filename should be ignored on key only
-        self.assertFalse(os.path.isfile(self.aptlistfile))
+        """test_apt_src_longkeyid_real - Test long keyid including key add"""
+        keyid = "B59D 5F15 97A5 04B7 E230  6DCA 0620 BBCF 0368 3F77"
+        cfg = {'keyid': keyid,
+               'filename': self.aptlistfile}
+
+        self.apt_src_keyid_real(cfg, EXPECTEDKEY)
+
+    def test_apt_src_longkeyid_ks_real(self):
+        """test_apt_src_longkeyid_ks_real - Test long keyid from other ks"""
+        keyid = "B59D 5F15 97A5 04B7 E230  6DCA 0620 BBCF 0368 3F77"
+        cfg = {'keyid': keyid,
+               'keyserver': 'keys.gnupg.net',
+               'filename': self.aptlistfile}
+
+        self.apt_src_keyid_real(cfg, EXPECTEDKEY)
 
     def test_apt_src_ppa(self):
-        """test_apt_src_ppa
-        Test specification of a ppa
-        """
+        """Test adding a ppa"""
         params = self._get_default_params()
         cfg = {'source': 'ppa:smoser/cloud-init-test',
                'filename': self.aptlistfile}
@@ -493,7 +452,8 @@
         matcher = re.compile(r'^[\w-]+:\w').search
 
         with mock.patch.object(util, 'subp') as mockobj:
-            cc_apt_configure.add_sources([cfg], params, aa_repo_match=matcher)
+            cc_apt_configure.add_apt_sources([cfg], params,
+                                             aa_repo_match=matcher)
         mockobj.assert_called_once_with(['add-apt-repository',
                                          'ppa:smoser/cloud-init-test'])
 
@@ -501,9 +461,7 @@
         self.assertFalse(os.path.isfile(self.aptlistfile))
 
     def test_apt_src_ppa_tri(self):
-        """test_apt_src_ppa_tri
-        Test specification of a ppa
-        """
+        """Test adding three ppa's"""
         params = self._get_default_params()
         cfg1 = {'source': 'ppa:smoser/cloud-init-test',
                 'filename': self.aptlistfile}
@@ -516,8 +474,8 @@
         matcher = re.compile(r'^[\w-]+:\w').search
 
         with mock.patch.object(util, 'subp') as mockobj:
-            cc_apt_configure.add_sources([cfg1, cfg2, cfg3], params,
-                                         aa_repo_match=matcher)
+            cc_apt_configure.add_apt_sources([cfg1, cfg2, cfg3], params,
+                                             aa_repo_match=matcher)
         calls = [call(['add-apt-repository', 'ppa:smoser/cloud-init-test']),
                  call(['add-apt-repository', 'ppa:smoser/cloud-init-test2']),
                  call(['add-apt-repository', 'ppa:smoser/cloud-init-test3'])]
@@ -529,10 +487,7 @@
         self.assertFalse(os.path.isfile(self.aptlistfile3))
 
     def test_convert_to_new_format(self):
-        """test_convert_to_new_format
-        Test the conversion of old to new format
-        And the noop conversion of new to new format as well
-        """
+        """Test the conversion of old to new format"""
         cfg1 = {'source': 'deb $MIRROR $RELEASE multiverse',
                 'filename': self.aptlistfile}
         cfg2 = {'source': 'deb $MIRROR $RELEASE main',