Fix apt configure 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 and moved
   to its own module.
 - packages/bddeb has an option to sign as someone else than smoser
 - make exception handling of apt_source features more specific
   (do not catch broad 'Exception')
 - rename some functions to reflect better what they actually do
 - capture some helper subp calls output to avoid spilling into stdout when
   not intended

5 files changed, 193 insertions, 169 deletions

diff --git a/cloudinit/config/cc_apt_configure.py b/cloudinit/config/cc_apt_configure.py
index 7a9777c0..96c4a43d 100644
--- a/cloudinit/config/cc_apt_configure.py
+++ b/cloudinit/config/cc_apt_configure.py
@@ -22,6 +22,7 @@ import glob
 import os
 import re
 
+from cloudinit import gpg
 from cloudinit import templater
 from cloudinit import util
 
@@ -34,21 +35,6 @@ APT_PROXY_FN = "/etc/apt/apt.conf.d/95cloud-init-proxy"
 # 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 handle(name, cfg, cloud, log, _args):
             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 @@ def handle(name, cfg, cloud, log, _args):
             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 @@ def rename_apt_lists(old_mirrors, new_mirrors, lists_d="/var/lib/apt/lists"):
         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 @@ def generate_sources_list(cfg, codename, mirrors, cloud, log):
     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 @@ def add_key_raw(key):
     try:
         util.subp(('apt-key', 'add', '-'), key)
     except util.ProcessExecutionError:
-        raise Exception('failed add key')
+        raise ValueError('failed to add apt GPG Key to apt keyring')
 
 
-def add_key(ent):
+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 @@ def add_key(ent):
         keyserver = "keyserver.ubuntu.com"
         if 'keyserver' in ent:
             keyserver = ent['keyserver']
-        ent['key'] = getkeybyid(ent['keyid'], keyserver)
+        ent['key'] = gpg.get_key_by_id(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 @@ def convert_to_new_format(srclist):
     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 @@ def add_sources(srclist, template_params=None, aa_repo_match=None):
 
         # 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:
diff --git a/cloudinit/gpg.py b/cloudinit/gpg.py
new file mode 100644
index 00000000..6a76d785
--- /dev/null
+++ b/cloudinit/gpg.py
@@ -0,0 +1,74 @@
+"""gpg.py - Collection of gpg key related functions"""
+# vi: ts=4 expandtab
+#
+#    Copyright (C) 2016 Canonical Ltd.
+#
+#    Author: Scott Moser <scott.moser@canonical.com>
+#    Author: Christian Ehrhardt <christian.ehrhardt@canonical.com>
+#
+#    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 log as logging
+from cloudinit import util
+
+LOG = logging.getLogger(__name__)
+
+
+def 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 receive_key(key, keyserver):
+    """Receive gpg key from the specified keyserver"""
+    LOG.debug('Receive gpg key "%s"', key)
+    try:
+        util.subp(["gpg", "--keyserver", keyserver, "--recv-keys", key],
+                  capture=True)
+    except util.ProcessExecutionError as error:
+        raise ValueError(('Failed to import key "%s" '
+                          'from server "%s" - error %s') %
+                         (key, keyserver, error))
+
+
+def 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 get_key_by_id(keyid, keyserver="keyserver.ubuntu.com"):
+    """get gpg keyid from keyserver"""
+    armour = export_armour(keyid)
+    if not armour:
+        try:
+            receive_key(keyid, keyserver=keyserver)
+            armour = 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
+            delete_key(keyid)
+
+    return armour
diff --git a/packages/bddeb b/packages/bddeb
index 1b0f642c..f8de7b42 100755
--- a/packages/bddeb
+++ b/packages/bddeb
@@ -148,11 +148,17 @@ def main():
     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
diff --git a/tests/unittests/test_handler/test_handler_apt_configure_sources_list.py b/tests/unittests/test_handler/test_handler_apt_configure_sources_list.py
index 03e80c98..acde0863 100644
--- a/tests/unittests/test_handler/test_handler_apt_configure_sources_list.py
+++ b/tests/unittests/test_handler/test_handler_apt_configure_sources_list.py
@@ -20,6 +20,8 @@ from cloudinit import util
 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 @@ class TestAptSourceConfigSourceList(t_help.FilesystemMockingTestCase):
             {'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 @@ class TestAptSourceConfigSourceList(t_help.FilesystemMockingTestCase):
             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',
diff --git a/tests/unittests/test_handler/test_handler_apt_source.py b/tests/unittests/test_handler/test_handler_apt_source.py
index fa28c916..99a4d860 100644
--- a/tests/unittests/test_handler/test_handler_apt_source.py
+++ b/tests/unittests/test_handler/test_handler_apt_source.py
@@ -13,6 +13,7 @@ except ImportError:
 from mock import call
 
 from cloudinit.config import cc_apt_configure
+from cloudinit import gpg
 from cloudinit import util
 
 from ..helpers import TestCase
@@ -62,7 +63,8 @@ class TestAptSourceConfig(TestCase):
         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 @@ class TestAptSourceConfig(TestCase):
         """
         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 @@ class TestAptSourceConfig(TestCase):
                                   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 @@ class TestAptSourceConfig(TestCase):
         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 @@ class TestAptSourceConfig(TestCase):
                                   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 @@ class TestAptSourceConfig(TestCase):
         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 @@ class TestAptSourceConfig(TestCase):
         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 @@ class TestAptSourceConfig(TestCase):
         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 @@ class TestAptSourceConfig(TestCase):
                                   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 @@ class TestAptSourceConfig(TestCase):
                                   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 @@ class TestAptSourceConfig(TestCase):
         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 @@ class TestAptSourceConfig(TestCase):
         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 @@ class TestAptSourceConfig(TestCase):
 
         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 @@ class TestAptSourceConfig(TestCase):
                                   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 @@ class TestAptSourceConfig(TestCase):
         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 @@ class TestAptSourceConfig(TestCase):
                                   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 @@ class TestAptSourceConfig(TestCase):
         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 @@ class TestAptSourceConfig(TestCase):
                                   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 @@ class TestAptSourceConfig(TestCase):
         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 @@ class TestAptSourceConfig(TestCase):
             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 @@ class TestAptSourceConfig(TestCase):
         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 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)
+    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)
         """
-        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)
+        with mock.patch.object(cc_apt_configure, 'add_apt_key_raw') as mockkey:
+            with mock.patch.object(gpg, 'get_key_by_id',
+                                   return_value=expectedkey) as mockgetkey:
+                cc_apt_configure.add_apt_sources([cfg], params)
 
-        mockobj.assert_called_with(EXPECTEDKEY)
+        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 keyid including key add"""
+        keyid = "03683F77"
+        cfg = {'keyid': keyid,
+               'filename': 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)
-        """
+        """test_apt_src_longkeyid_real - Test long keyid including key add"""
         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)
+        self.apt_src_keyid_real(cfg, EXPECTEDKEY)
 
-        mockobj.assert_called_with(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}
 
-        # 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_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 @@ class TestAptSourceConfig(TestCase):
         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 @@ class TestAptSourceConfig(TestCase):
         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 @@ class TestAptSourceConfig(TestCase):
         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 @@ class TestAptSourceConfig(TestCase):
         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',