Merge lp:~paelzer/cloud-init/test-apt-source into lp:~cloud-init-dev/cloud-init/trunk

[Christian Ehrhardt ]

Apt sources configuration improvements

- keyid-only (no source statement)
- key only (no source statement)
- custom source.list template
- support long gpg key fingerprints with spaces
- fix issue with key's that were already in the local gpg keyring
- allowing a new format to specify apt_sources in a dictionary instead of a list to allow merging of configurations

Along the way to ensure things are working this series also added several tests. Related to the new format and the realization of corner cases related to "no" or "multiple" specified filenames further unittests were added.

Testing various combinations of the execution as it was so far:
- test_apt_source_basic Test Fix deb source string, has to overwrite mirror conf in params
- test_apt_source_replace Test Autoreplacement of MIRROR and RELEASE in source specs
- test_apt_source_keyid Test specification of a source + keyid
- test_apt_source_key Test specification of a source + key
- test_apt_source_keyonly Test specification key without source
- test_apt_source_keyidonly Test specification of a keyid without source
- test_apt_source_ppa Test specification of a ppa
- test_apt_source_list_debian Test rendering of a source.list from template for debian
- test_apt_source_list_ubuntu Test rendering of a source.list from template for ubuntu
- test_apt_source_list_debian_mirrorfail Test rendering of a source.list from template for debian
- test_apt_source_list_ubuntu_mirrorfail Test rendering of a source.list from template for ubuntu
- test_apt_srcl_custom Test rendering from a custom source.list template
- test_apt_source_keyid_real Test specification of a keyid without source incl real addition (no mock) of the add
- test_apt_source_longkeyid_real Test specification of a long key fingerprint without source incl real addition (no mock) of the add

Testing variations of above cases but without filename being specified
- test_apt_src_basic_nofn
- test_apt_src_replace_nofn
- test_apt_src_keyid_nofn
- test_apt_src_key_nofn

Testing variations of above cases with multiple specifications in one call:
test_apt_src_basic_tri
test_apt_src_replace_tri
test_apt_src_keyid_tri
test_apt_src_ppa_tri

Testing the new format:
test_apt_src_basic_dict
test_apt_src_basic_dict_tri
test_apt_src_replace_dict_tri
test_convert_to_new_format

As discussed, the cloud-init portion of key-only (no source) and alternative source.list template.

I refused to do all the major changes you indicated as "might" in your mail.
Especially since IIRC the thought is to SRU it back.
The impact on backward compatibility could be too big.

Instead I tried to take a safe approach.
For none of the already existing features that touched existed unit tests.
So I wrote these, then added functionality and verified at least nothing on the old tests broke.
Finally I added tests for the new functions and also documentations in the example files.

[Scott Moser (smoser)]

looks really good. a couple things to tweak.

[Scott Moser (smoser)]

I *do* want the dictionary format supported though.
dictionaries just merge so much more easily.

so this MP doesn't have to have it, but we want dictionary 'apt_sources' to support being a dictionary to fix that bug, and want that in curtin.

[lp:~paelzer/cloud-init/test-apt-source]

1246. By Christian Ehrhardt  on 2016-05-12

remove no more applicable "not supported" statements

1247. By Christian Ehrhardt  on 2016-05-12

move errorlist.append out of add_key

1248. By Christian Ehrhardt  on 2016-05-12

remove Unnecessary parens in add_key

1249. By Christian Ehrhardt  on 2016-05-12

fix EXPORT_GPG_KEYID for long key fingerprints

1250. By Christian Ehrhardt  on 2016-05-12

split add_key and add_key_raw fior better testability

1251. By Christian Ehrhardt  on 2016-05-12

Adding test_apt_source_keyid_real and test_apt_source_longkeyid_real

This now ensures that the stack of fetching IDs from keyservers and adding them
really works by comparing against known good keys that are expected.

1252. By Christian Ehrhardt  on 2016-05-12

remove superfluous import

1253. By Christian Ehrhardt  on 2016-05-12

alphabetical import order

1254. By Christian Ehrhardt  on 2016-05-12

alphabetical order on imports

1255. By Christian Ehrhardt  on 2016-05-12

fix old typo in example

1256. By Christian Ehrhardt  on 2016-05-12

improve spacing in apt_source_list test

1257. By Christian Ehrhardt  on 2016-05-12

streamline code and sanitize expected result string definition

1258. By Christian Ehrhardt  on 2016-05-12

make pep8 happy with a few spaces

1259. By Christian Ehrhardt  on 2016-05-23

generalize test_apt_source_basic to be reusable across more testcases

1260. By Christian Ehrhardt  on 2016-05-23

test_apt_source_basic_nofn check for non-specified filename

Cloud-inint uses a default fallback, we want to ensure no code change modfies
this behaviour.

1261. By Christian Ehrhardt  on 2016-05-23

drop unused mockappsubp

1262. By Christian Ehrhardt  on 2016-05-23

extend test_apt_source_replace by a no-filename case

1263. By Christian Ehrhardt  on 2016-05-23

extend test_apt_source_keyid by no filename case

1264. By Christian Ehrhardt  on 2016-05-23

put fallbackfn to init

This was now used by multiple methods, no need to duplicate code.

1265. By Christian Ehrhardt  on 2016-05-23

extend test_apt_source_key by nofn case

1266. By Christian Ehrhardt  on 2016-05-23

support apt_sources to be a dictionary

key is the filename, and "old" input shall be handled as it was all the time.
For compatibility this will (continue to) overwrite the file of multiple
options that did not specify an output file (they all get the same default).
Yet it will process them all - as it always did - e.g. to add the keys of all
of them.

Any users of the new format won't have these issues, as they will always have
a key.

1267. By Christian Ehrhardt  on 2016-05-23

warn about multiple colliding apt_source without filenames

1268. By Christian Ehrhardt  on 2016-05-23

fix function names in inline doc

1269. By Christian Ehrhardt  on 2016-05-23

testcases with multiple source list entries

1270. By Christian Ehrhardt  on 2016-05-23

add triple case for test_apt_source_keyid_triple incl triple key check

1271. By Christian Ehrhardt  on 2016-05-23

make checkers happy about unused loop index

1272. By Christian Ehrhardt  on 2016-05-23

add triple test for ppa adding

1273. By Christian Ehrhardt  on 2016-05-23

fix issue with dictionary style apt_sources handling filenames

1274. By Christian Ehrhardt  on 2016-05-23

add test_apt_source_basic_dict

This is the basic testcase but in the new dictionary format

1275. By Christian Ehrhardt  on 2016-05-23

unify basic triple check and add test_apt_src_basic_dict_triple based on it

1276. By Christian Ehrhardt  on 2016-05-23

make sure we only handle list or dict apt_sources and bail out for others

1277. By Christian Ehrhardt  on 2016-05-23

shorten method names to follow python rules

1278. By Christian Ehrhardt  on 2016-05-23

add test_apt_src_replace_dict_tri

This includes a test for the weird but valid case in the new dictionary syntax
that one sets a key (which is the filename) but overwrites the filename value
inside of it.

1279. By Christian Ehrhardt  on 2016-05-23

modify cloud-config examples to match the new apt_source format

1280. By Christian Ehrhardt  on 2016-05-23

final pep8 check fixups

[Christian Ehrhardt  (paelzer)]

Following the discussion with smoser I added the requested new dictionary based format to apt_sources. This goes along with a collection of extra testcases to ensure the behavior of new (and old) configuration works.

[Scott Moser (smoser)]

so, some comments inline . that might seem like a lot, and that i'm nit picking. Sorry if it seems like that.

review: Needs Fixing

[lp:~paelzer/cloud-init/test-apt-source]

1281. By Christian Ehrhardt  on 2016-05-24

fix typo in examples doc

1282. By Christian Ehrhardt  on 2016-05-24

improve examples of ap_source

[Christian Ehrhardt  (paelzer)]

> so, some comments inline . that might seem like a lot, and that i'm nit
> picking. Sorry if it seems like that.

You are not, I'm happy about a good review and that it is!
If the file would be free of warnings in general it would be easier to see the new ones :-)

I usually found a lot of missing docstrings and invalid constant/variable name issues of pylint littering my view of checker warnings probably too much.

Anyway - most of the (not nit) picks are stuff I copied or moved and now you just look at it again - so I don't feel offended but assisted to make the code better.

I found even more minor things on my own in the meantime that I can merge in given that changes of a respin.

[Christian Ehrhardt  (paelzer)]

On Tue, May 24, 2016 at 4:01 PM, Scott Moser <email address hidden> wrote:

> so, some comments inline . that might seem like a lot, and that i'm nit
> picking. Sorry if it seems like that.
>

Having worked on the list of feedback I really found that they are mostly
picks on older code I moved - so I feel good :-)

And for my English sometimes I think I just shouldn't try to race to a new
MP at the end of a day :-)

> > + """
> > + 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:
> > + raise Exception('failed to get key from %s' % keyserver)
>
> KeyError ? or LookupError maybe? might seem reasonable.
> catch the subp.ProcessExecutionError specifically or even just let it go
> through would be better than swallowing it with a more generic error.
>
>
Yeah, given a second thought I like not catching it at that level the most.

> >
> > errorlist = []
> > - for ent in srclist:
> > + # convert old list format to new dict based format
>
> converting the array to a dictionary seems like a method itself that would
> be stand alone be easily unit tested.
>

 I first thought to refuse that since so many older tests are testing that
implicitly but ten decided to add it (refusing to go home to the family -
remote this week for better internet access).
Uploading updated MP now ...

[lp:~paelzer/cloud-init/test-apt-source]

1283. By Christian Ehrhardt  on 2016-05-24

rebased with upstream and reolved merge conflicts

1284. By Christian Ehrhardt  on 2016-05-24

integrate further smaller review feedback

1285. By Christian Ehrhardt  on 2016-05-24

pacify pep8 regarding the new changes

1286. By Christian Ehrhardt  on 2016-05-24

add test for the now isolated convert_to_new_format function

[Christian Ehrhardt  (paelzer)]

Review feedback integrated and new test test_convert_to_new_format added as requested.
Ready for another review or merging.

[lp:~paelzer/cloud-init/test-apt-source]

1287. By Christian Ehrhardt  on 2016-05-25

make test_apt_srcl_custom independent to where it is executed

[Christian Ehrhardt  (paelzer)]

by ongoing testing I found that one of the unittests depended on the system being xenial, this last commit fixes that dependency

[lp:~paelzer/cloud-init/test-apt-source]

1288. By Christian Ehrhardt  on 2016-05-25

fix inline doc of test_apt_src_longkeyid_real

[Scott Moser (smoser)]

one more, but i think we're there.

[lp:~paelzer/cloud-init/test-apt-source]

1289. By Christian Ehrhardt  on 2016-05-30

drop errorlist from convert_to_new_format

1290. By Christian Ehrhardt  on 2016-05-30

add test for wrong apt_source format

1291. By Christian Ehrhardt  on 2016-05-30

improve wording in the examples

1292. By Christian Ehrhardt  on 2016-05-30

fix EXPORT_GPG_KEYID for existing keys

This was broken for keys already existing in the local keyring.
There instead of the keycontent it reported the header like:
 pub 1024R/03683F77 2009-10-27
 uid Launchpad PPA for Scott Moser

1293. By Christian Ehrhardt  on 2016-05-30

merge with last upstream to avoid merging conflicts on MP

[Christian Ehrhardt  (paelzer)]

Thanks for the repeated great review, due to that I changed:
- warning/errors in convert_to_new_format
- improved example wording

Further on I:
- extended test_convert_to_new_format to test wrong format
- fixed an issue identified the key* tests depending if it was already imported in gpg (that turned out to be a real issue with the implementation)
- rebased again to avoid merge conflicts

Will update commit message now ...

=== modified file 'cloudinit/config/cc_apt_configure.py'
--- cloudinit/config/cc_apt_configure.py	2016-05-12 17:56:26 +0000
+++ cloudinit/config/cc_apt_configure.py	2016-05-30 11:10:57 +0000
@@ -40,9 +40,9 @@
     k=${1} ks=${2};
     exec 2>/dev/null
     [ -n "$k" ] || exit 1;
-    armour=$(gpg --list-keys --armour "${k}")
+    armour=$(gpg --export --armour "${k}")
     if [ -z "${armour}" ]; then
-       gpg --keyserver ${ks} --recv $k >/dev/null &&
+       gpg --keyserver ${ks} --recv "${k}" >/dev/null &&
           armour=$(gpg --export --armour "${k}") &&
           gpg --batch --yes --delete-keys "${k}"
     fi
@@ -70,7 +70,7 @@
 
     if not util.get_cfg_option_bool(cfg,
                                     'apt_preserve_sources_list', False):
-        generate_sources_list(release, mirrors, cloud, log)
+        generate_sources_list(cfg, release, mirrors, cloud, log)
         old_mirrors = cfg.get('apt_old_mirrors',
                               {"primary": "archive.ubuntu.com/ubuntu",
                                "security": "security.ubuntu.com/ubuntu"})
@@ -149,7 +149,17 @@
     return stdout.strip()
 
 
-def generate_sources_list(codename, mirrors, cloud, log):
+def generate_sources_list(cfg, codename, mirrors, cloud, log):
+    params = {'codename': codename}
+    for k in mirrors:
+        params[k] = mirrors[k]
+
+    custtmpl = cfg.get('apt_custom_sources_list', None)
+    if custtmpl is not None:
+        templater.render_string_to_file(custtmpl,
+                                        '/etc/apt/sources.list', params)
+        return
+
     template_fn = cloud.get_template_filename('sources.list.%s' %
                                               (cloud.distro.name))
     if not template_fn:
@@ -158,12 +168,60 @@
             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_raw(key):
+    """
+    actual adding of a key as defined in key argument
+    to the system
+    """
+    try:
+        util.subp(('apt-key', 'add', '-'), key)
+    except util.ProcessExecutionError:
+        raise Exception('failed add key')
+
+
+def add_key(ent):
+    """
+    add key to the system as defined in ent (if any)
+    supports raw keys or keyid's
+    The latter will as a first step fetch the raw key from a keyserver
+    """
+    if 'keyid' in ent and 'key' not in ent:
+        keyserver = "keyserver.ubuntu.com"
+        if 'keyserver' in ent:
+            keyserver = ent['keyserver']
+        ent['key'] = getkeybyid(ent['keyid'], keyserver)
+
+    if 'key' in ent:
+        add_key_raw(ent['key'])
+
+
+def convert_to_new_format(srclist):
+    """ convert_to_new_format
+        convert the old list based format to the new dict based one
+    """
+    srcdict = {}
+    if isinstance(srclist, list):
+        for srcent in srclist:
+            if 'filename' not in srcent:
+                # file collides for multiple !filename cases for compatibility
+                # yet we need them all processed, so not same dictionary key
+                srcent['filename'] = "cloud_config_sources.list"
+                key = util.rand_dict_key(srcdict, "cloud_config_sources.list")
+            else:
+                # all with filename use that as key (matching new format)
+                key = srcent['filename']
+            srcdict[key] = srcent
+    elif isinstance(srclist, dict):
+        srcdict = srclist
+    else:
+        raise ValueError("unknown apt_sources format")
+
+    return srcdict
+
+
 def add_sources(srclist, template_params=None, aa_repo_match=None):
     """
     add entries in /etc/apt/sources.list.d for each abbreviated
@@ -178,14 +236,29 @@
             return False
 
     errorlist = []
-    for ent in srclist:
+    srcdict = convert_to_new_format(srclist)
+
+    for filename in srcdict:
+        ent = srcdict[filename]
+        if 'filename' not in ent:
+            ent['filename'] = filename
+
+        # keys can be added without specifying a source
+        try:
+            add_key(ent)
+        except Exception as detail:
+            errorlist.append([ent, detail])
+
         if 'source' not in ent:
             errorlist.append(["", "missing source"])
             continue
-
         source = ent['source']
         source = templater.render_string(source, template_params)
 
+        if not ent['filename'].startswith(os.path.sep):
+            ent['filename'] = os.path.join("/etc/apt/sources.list.d/",
+                                           ent['filename'])
+
         if aa_repo_match(source):
             try:
                 util.subp(["add-apt-repository", source])
@@ -194,29 +267,6 @@
                                   ("add-apt-repository failed. " + str(e))])
             continue
 
-        if 'filename' not in ent:
-            ent['filename'] = 'cloud_config_sources.list'
-
-        if not ent['filename'].startswith("/"):
-            ent['filename'] = os.path.join("/etc/apt/sources.list.d/",
-                                           ent['filename'])
-
-        if ('keyid' in ent and 'key' not in ent):
-            ks = "keyserver.ubuntu.com"
-            if 'keyserver' in ent:
-                ks = ent['keyserver']
-            try:
-                ent['key'] = getkeybyid(ent['keyid'], ks)
-            except Exception:
-                errorlist.append([source, "failed to get key from %s" % ks])
-                continue
-
-        if 'key' in ent:
-            try:
-                util.subp(('apt-key', 'add', '-'), ent['key'])
-            except Exception:
-                errorlist.append([source, "failed add key"])
-
         try:
             contents = "%s\n" % (source)
             util.write_file(ent['filename'], contents, omode="ab")

=== modified file 'cloudinit/templater.py'
--- cloudinit/templater.py	2015-01-23 02:21:04 +0000
+++ cloudinit/templater.py	2016-05-30 11:10:57 +0000
@@ -142,6 +142,11 @@
     util.write_file(outfn, contents, mode=mode)
 
 
+def render_string_to_file(content, outfn, params, mode=0o644):
+    contents = render_string(content, params)
+    util.write_file(outfn, contents, mode=mode)
+
+
 def render_string(content, params):
     if not params:
         params = {}

=== modified file 'cloudinit/util.py'
--- cloudinit/util.py	2016-05-12 17:56:26 +0000
+++ cloudinit/util.py	2016-05-30 11:10:57 +0000
@@ -336,6 +336,16 @@
     return "".join([random.choice(select_from) for _x in range(0, strlen)])
 
 
+def rand_dict_key(dictionary, postfix=None):
+    if not postfix:
+        postfix = ""
+    while True:
+        newkey = rand_str(strlen=8) + "_" + postfix
+        if newkey not in dictionary:
+            break
+    return newkey
+
+
 def read_conf(fname):
     try:
         return load_yaml(load_file(fname), default={})

=== 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-30 11:10:57 +0000
@@ -72,14 +72,87 @@
 # then apt_mirror above will have no effect
 apt_preserve_sources_list: true
 
+# Provide a custom template for rendering sources.list
+# Default: a default template for Ubuntu/Debain will be used as packaged in
+#  Ubuntu: /etc/cloud/templates/sources.list.ubuntu.tmpl
+#  Debian: /etc/cloud/templates/sources.list.debian.tmpl
+#  Others: n/a
+# This will follow the normal mirror/codename replacement rules before
+# being written to disk.
+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
+    deb {{mirror}} {{codename}} main restricted
+    deb-src {{mirror}} {{codename}} main restricted
+
+    # could drop some of the usually used entries
+
+    # could refer to other mirrors
+    deb http://ddebs.ubuntu.com {{codename}} main restricted universe multiverse
+    deb http://ddebs.ubuntu.com {{codename}}-updates main restricted universe multiverse
+    deb http://ddebs.ubuntu.com {{codename}}-proposed main restricted universe multiverse
+
+    # or even more uncommon examples like local or NFS mounted repos,
+    # eventually whatever is compatible with sources.list syntax
+    deb file:/home/apt/debian unstable main contrib non-free
+
 # 'source' entries in apt-sources that match this python regex
 # expression will be passed to add-apt-repository
 add_apt_repo_match: '^[\w-]+:\w'
 
+# 'apt_sources' is a dictionary
+# The key is the filename and will be prepended by /etc/apt/sources.list.d/ if
+# it doesn't start with a '/'.
+# There are certain cases - where no content is written into a source.list file
+# where the filename will be ignored - yet it can still be used as index for
+# merging.
+# The value it maps to is a dictionary with the following optional entries:
+#   source: a sources.list entry (some variable replacements apply)
+#   keyid: providing a key to import via shortid or fingerprint
+#   key: providing a raw PGP key
+#   keyserver: keyserver to fetch keys from, default is keyserver.ubuntu.com
+#   filename: for compatibility with the older format (now the key to this
+#               dictionary is the filename). If specified this overwrites the
+#               filename given as key.
+
+# the new "filename: {specification-dictionary}, filename2: ..." format allows
+# better merging between multiple input files than a list like:
+# cloud-config1
+# sources:
+    s1: {'key': 'key1', 'source': 'source1'}
+# cloud-config2
+# sources:
+    s2: {'key': 'key2'}
+    s1: {filename: 'foo'}
+# this would be merged to
+#sources:
+#    s1:
+#        filename: foo
+#        key: key1
+#        source: source1
+#    s2:
+#        key: key2
+# Be aware that this style of merging is not the default (for backward
+# compatibility reasons). You should specify the following merge_how to get
+# this more complete and modern merging behaviour:
+#   merge_how: "list()+dict()+str()"
+# This would then also be equivalent to the config merging used in curtin
+# (https://launchpad.net/curtin).
+
+# for more details see below in the various examples
+
 apt_sources:
- - source: "deb http://ppa.launchpad.net/byobu/ppa/ubuntu karmic main"
+ byobu-ppa.list:
+   source: "deb http://ppa.launchpad.net/byobu/ppa/ubuntu karmic main"
    keyid: F430BBA5 # GPG key ID published on a key server
-   filename: byobu-ppa.list
+ # adding a source.list line, importing a gpg key for a given key id and
+ # storing it in the file /etc/apt/sources.list.d/byobu-ppa.list
 
  # PPA shortcut:
  #  * Setup correct apt sources.list line
@@ -87,7 +160,9 @@
  #
  #  See https://help.launchpad.net/Packaging/PPA for more information
  #  this requires 'add-apt-repository'
- - source: "ppa:smoser/ppa"    # Quote the string
+ #  due to that the filename key is ignored in this case
+ ignored1:
+   source: "ppa:smoser/ppa"    # Quote the string
 
  # Custom apt repository:
  #  * all that is required is 'source'
@@ -95,42 +170,74 @@
  #  * [optional] Import the apt signing key from the keyserver 
  #  * Defaults:
  #    + keyserver: keyserver.ubuntu.com
- #    + filename: cloud_config_sources.list
  #
  #    See sources.list man page for more information about the format
- - source: deb http://archive.ubuntu.com/ubuntu karmic-backports main universe multiverse restricted
+ my-repo.list:
+   source: deb http://archive.ubuntu.com/ubuntu karmic-backports main universe multiverse restricted
 
  # sources can use $MIRROR and $RELEASE and they will be replaced
  # with the local mirror for this cloud, and the running release
  # the entry below would be possibly turned into:
- # - source: deb http://us-east-1.ec2.archive.ubuntu.com/ubuntu natty multiverse
- - source: deb $MIRROR $RELEASE multiverse
+ # source: deb http://us-east-1.ec2.archive.ubuntu.com/ubuntu natty multiverse
+ my-repo.list:
+   source: deb $MIRROR $RELEASE multiverse
 
  # this would have the same end effect as 'ppa:byobu/ppa'
- - source: "deb http://ppa.launchpad.net/byobu/ppa/ubuntu karmic main"
+ my-repo.list:
+   source: "deb http://ppa.launchpad.net/byobu/ppa/ubuntu karmic main"
    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
+ # since this doesn't generate a source.list file the filename key is ignored
+ ignored2:
+   keyid: F430BBA5 # GPG key ID published on a key server
+
+ # In general keyid's can also be specified via their long fingerprints
+ # since this doesn't generate a source.list file the filename key is ignored
+ ignored3:
+   keyid: B59D 5F15 97A5 04B7 E230  6DCA 0620 BBCF 0368 3F77
+
  # 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
+ #  * Providing the PGP key here is the most robust method for
  #    specifying a key, as it removes dependency on a remote key server
-
- - 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-----
-      Version: SKS 1.0.10
-
-      mI0ESpA3UQEEALdZKVIMq0j6qWAXAyxSlF63SvPVIgxHPb9Nk0DZUixn+akqytxG4zKCONz6
-      qLjoBBfHnynyVLfT4ihg9an1PqxRnTO+JKQxl8NgKGz6Pon569GtAOdWNKw15XKinJTDLjnj
-      9y96ljJqRcpV9t/WsIcdJPcKFR5voHTEoABE2aEXABEBAAG0GUxhdW5jaHBhZCBQUEEgZm9y
-      IEFsZXN0aWOItgQTAQIAIAUCSpA3UQIbAwYLCQgHAwIEFQIIAwQWAgMBAh4BAheAAAoJEA7H
-      5Qi+CcVxWZ8D/1MyYvfj3FJPZUm2Yo1zZsQ657vHI9+pPouqflWOayRR9jbiyUFIn0VdQBrP
-      t0FwvnOFArUovUWoKAEdqR8hPy3M3APUZjl5K4cMZR/xaMQeQRZ5CHpS4DBKURKAHC0ltS5o
-      uBJKQOZm5iltJp15cgyIkBkGe8Mx18VFyVglAZey
-      =Y2oI
-      -----END PGP PUBLIC KEY BLOCK-----
+ my-repo.list:
+   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-----
+      Version: SKS 1.0.10
+
+      mI0ESpA3UQEEALdZKVIMq0j6qWAXAyxSlF63SvPVIgxHPb9Nk0DZUixn+akqytxG4zKCONz6
+      qLjoBBfHnynyVLfT4ihg9an1PqxRnTO+JKQxl8NgKGz6Pon569GtAOdWNKw15XKinJTDLjnj
+      9y96ljJqRcpV9t/WsIcdJPcKFR5voHTEoABE2aEXABEBAAG0GUxhdW5jaHBhZCBQUEEgZm9y
+      IEFsZXN0aWOItgQTAQIAIAUCSpA3UQIbAwYLCQgHAwIEFQIIAwQWAgMBAh4BAheAAAoJEA7H
+      5Qi+CcVxWZ8D/1MyYvfj3FJPZUm2Yo1zZsQ657vHI9+pPouqflWOayRR9jbiyUFIn0VdQBrP
+      t0FwvnOFArUovUWoKAEdqR8hPy3M3APUZjl5K4cMZR/xaMQeQRZ5CHpS4DBKURKAHC0ltS5o
+      uBJKQOZm5iltJp15cgyIkBkGe8Mx18VFyVglAZey
+      =Y2oI
+      -----END PGP PUBLIC KEY BLOCK-----
+
+ # Custom gpg key:
+ #  * As with keyid, a key may also be specified without a related source.
+ #  * all other facts mentioned above still apply
+ # since this doesn't generate a source.list file the filename key is ignored
+ ignored4:
+   key: | # The value needs to start with -----BEGIN PGP PUBLIC KEY BLOCK-----
+      -----BEGIN PGP PUBLIC KEY BLOCK-----
+      Version: SKS 1.0.10
+
+      mI0ESpA3UQEEALdZKVIMq0j6qWAXAyxSlF63SvPVIgxHPb9Nk0DZUixn+akqytxG4zKCONz6
+      qLjoBBfHnynyVLfT4ihg9an1PqxRnTO+JKQxl8NgKGz6Pon569GtAOdWNKw15XKinJTDLjnj
+      9y96ljJqRcpV9t/WsIcdJPcKFR5voHTEoABE2aEXABEBAAG0GUxhdW5jaHBhZCBQUEEgZm9y
+      IEFsZXN0aWOItgQTAQIAIAUCSpA3UQIbAwYLCQgHAwIEFQIIAwQWAgMBAh4BAheAAAoJEA7H
+      5Qi+CcVxWZ8D/1MyYvfj3FJPZUm2Yo1zZsQ657vHI9+pPouqflWOayRR9jbiyUFIn0VdQBrP
+      t0FwvnOFArUovUWoKAEdqR8hPy3M3APUZjl5K4cMZR/xaMQeQRZ5CHpS4DBKURKAHC0ltS5o
+      uBJKQOZm5iltJp15cgyIkBkGe8Mx18VFyVglAZey
+      =Y2oI
+      -----END PGP PUBLIC KEY BLOCK-----
+
 
 ## apt config via system_info:
 # under the 'system_info', you can further customize cloud-init's interaction

=== 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-30 11:10:57 +0000
@@ -0,0 +1,166 @@
+""" test_handler_apt_configure_sources_list
+Test templating of sources list
+"""
+import logging
+import os
+import re
+import shutil
+import tempfile
+
+try:
+    from unittest import mock
+except ImportError:
+    import mock
+
+from cloudinit import cloud
+from cloudinit import distros
+from cloudinit import helpers
+from cloudinit import templater
+from cloudinit import util
+
+from cloudinit.config import cc_apt_configure
+from cloudinit.sources import DataSourceNone
+
+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
+"""
+
+EXPECTED_CONVERTED_CONTENT = (
+    """## 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 http://archive.ubuntu.com/ubuntu/ fakerelease main restricted
+deb-src http://archive.ubuntu.com/ubuntu/ fakerelease 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/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):
+                with mock.patch.object(cc_apt_configure, 'get_release',
+                                       return_value='fakerelease'):
+                    cc_apt_configure.handle("notimportant", cfg, mycloud,
+                                            LOG, None)
+
+        mockwrite.assert_called_once_with(
+            '/etc/apt/sources.list',
+            EXPECTED_CONVERTED_CONTENT,
+            mode=420)
+
+
+# vi: ts=4 expandtab

=== added file 'tests/unittests/test_handler/test_handler_apt_source.py'
--- tests/unittests/test_handler/test_handler_apt_source.py	1970-01-01 00:00:00 +0000
+++ tests/unittests/test_handler/test_handler_apt_source.py	2016-05-30 11:10:57 +0000
@@ -0,0 +1,551 @@
+""" test_handler_apt_source
+Testing various config variations of the apt_source config
+"""
+import os
+import re
+import shutil
+import tempfile
+
+try:
+    from unittest import mock
+except ImportError:
+    import mock
+from mock import call
+
+from cloudinit.config import cc_apt_configure
+from cloudinit import util
+
+from ..helpers import TestCase
+
+EXPECTEDKEY = """-----BEGIN PGP PUBLIC KEY BLOCK-----
+Version: GnuPG v1
+
+mI0ESuZLUgEEAKkqq3idtFP7g9hzOu1a8+v8ImawQN4TrvlygfScMU1TIS1eC7UQ
+NUA8Qqgr9iUaGnejb0VciqftLrU9D6WYHSKz+EITefgdyJ6SoQxjoJdsCpJ7o9Jy
+8PQnpRttiFm4qHu6BVnKnBNxw/z3ST9YMqW5kbMQpfxbGe+obRox59NpABEBAAG0
+HUxhdW5jaHBhZCBQUEEgZm9yIFNjb3R0IE1vc2VyiLYEEwECACAFAkrmS1ICGwMG
+CwkIBwMCBBUCCAMEFgIDAQIeAQIXgAAKCRAGILvPA2g/d3aEA/9tVjc10HOZwV29
+OatVuTeERjjrIbxflO586GLA8cp0C9RQCwgod/R+cKYdQcHjbqVcP0HqxveLg0RZ
+FJpWLmWKamwkABErwQLGlM/Hwhjfade8VvEQutH5/0JgKHmzRsoqfR+LMO6OS+Sm
+S0ORP6HXET3+jC8BMG4tBWCTK/XEZw==
+=ACB2
+-----END PGP PUBLIC KEY BLOCK-----"""
+
+
+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 TestAptSourceConfig(TestCase):
+    """ TestAptSourceConfig
+    Main Class to test apt_source configs
+    """
+    def setUp(self):
+        super(TestAptSourceConfig, self).setUp()
+        self.tmp = tempfile.mkdtemp()
+        self.addCleanup(shutil.rmtree, self.tmp)
+        self.aptlistfile = os.path.join(self.tmp, "single-deb.list")
+        self.aptlistfile2 = os.path.join(self.tmp, "single-deb2.list")
+        self.aptlistfile3 = os.path.join(self.tmp, "single-deb3.list")
+        self.join = os.path.join
+        # mock fallback filename into writable tmp dir
+        self.fallbackfn = os.path.join(self.tmp, "etc/apt/sources.list.d/",
+                                       "cloud_config_sources.list")
+
+    @staticmethod
+    def _get_default_params():
+        """ get_default_params
+        Get the most basic default mrror and release info to be used in tests
+        """
+        params = {}
+        params['RELEASE'] = cc_apt_configure.get_release()
+        params['MIRROR'] = "http://archive.ubuntu.com/ubuntu"
+        return params
+
+    def myjoin(self, *args, **kwargs):
+        """ myjoin - redir into writable tmpdir"""
+        if (args[0] == "/etc/apt/sources.list.d/" and
+                args[1] == "cloud_config_sources.list" and
+                len(args) == 2):
+            return self.join(self.tmp, args[0].lstrip("/"), args[1])
+        else:
+            return self.join(*args, **kwargs)
+
+    def apt_src_basic(self, filename, cfg):
+        """ apt_src_basic
+        Test Fix deb source string, has to overwrite mirror conf in params
+        """
+        params = self._get_default_params()
+
+        cc_apt_configure.add_sources(cfg, params)
+
+        self.assertTrue(os.path.isfile(filename))
+
+        contents = load_tfile_or_url(filename)
+        self.assertTrue(re.search(r"%s %s %s %s\n" %
+                                  ("deb", "http://archive.ubuntu.com/ubuntu",
+                                   "karmic-backports",
+                                   "main universe multiverse restricted"),
+                                  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.
+        """
+        cfg = {'source': ('deb http://archive.ubuntu.com/ubuntu'
+                          ' karmic-backports'
+                          ' main universe multiverse restricted'),
+               'filename': self.aptlistfile}
+        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)
+        """
+        cfg = {self.aptlistfile: {'source':
+                                  ('deb http://archive.ubuntu.com/ubuntu'
+                                   ' karmic-backports'
+                                   ' main universe multiverse restricted')}}
+        self.apt_src_basic(self.aptlistfile, cfg)
+
+    def apt_src_basic_tri(self, cfg):
+        """ apt_src_basic_tri
+        Test Fix three deb source string, has to overwrite mirror conf in
+        params. Test with filenames provided in config.
+        generic part to check three files with different content
+        """
+        self.apt_src_basic(self.aptlistfile, cfg)
+
+        # extra verify on two extra files of this test
+        contents = load_tfile_or_url(self.aptlistfile2)
+        self.assertTrue(re.search(r"%s %s %s %s\n" %
+                                  ("deb", "http://archive.ubuntu.com/ubuntu",
+                                   "precise-backports",
+                                   "main universe multiverse restricted"),
+                                  contents, flags=re.IGNORECASE))
+        contents = load_tfile_or_url(self.aptlistfile3)
+        self.assertTrue(re.search(r"%s %s %s %s\n" %
+                                  ("deb", "http://archive.ubuntu.com/ubuntu",
+                                   "lucid-backports",
+                                   "main universe multiverse restricted"),
+                                  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.
+        """
+        cfg1 = {'source': ('deb http://archive.ubuntu.com/ubuntu'
+                           ' karmic-backports'
+                           ' main universe multiverse restricted'),
+                'filename': self.aptlistfile}
+        cfg2 = {'source': ('deb http://archive.ubuntu.com/ubuntu'
+                           ' precise-backports'
+                           ' main universe multiverse restricted'),
+                'filename': self.aptlistfile2}
+        cfg3 = {'source': ('deb http://archive.ubuntu.com/ubuntu'
+                           ' lucid-backports'
+                           ' main universe multiverse restricted'),
+                'filename': self.aptlistfile3}
+        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)
+        """
+        cfg = {self.aptlistfile: {'source':
+                                  ('deb http://archive.ubuntu.com/ubuntu'
+                                   ' karmic-backports'
+                                   ' main universe multiverse restricted')},
+               self.aptlistfile2: {'source':
+                                   ('deb http://archive.ubuntu.com/ubuntu'
+                                    ' precise-backports'
+                                    ' main universe multiverse restricted')},
+               self.aptlistfile3: {'source':
+                                   ('deb http://archive.ubuntu.com/ubuntu'
+                                    ' lucid-backports'
+                                    ' main universe multiverse restricted')}}
+        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.
+        """
+        cfg = {'source': ('deb http://archive.ubuntu.com/ubuntu'
+                          ' karmic-backports'
+                          ' main universe multiverse restricted')}
+        with mock.patch.object(os.path, 'join', side_effect=self.myjoin):
+            self.apt_src_basic(self.fallbackfn, [cfg])
+
+    def apt_src_replacement(self, filename, cfg):
+        """ apt_src_replace
+        Test Autoreplacement of MIRROR and RELEASE in source specs
+        """
+        params = self._get_default_params()
+        cc_apt_configure.add_sources(cfg, params)
+
+        self.assertTrue(os.path.isfile(filename))
+
+        contents = load_tfile_or_url(filename)
+        self.assertTrue(re.search(r"%s %s %s %s\n" %
+                                  ("deb", params['MIRROR'], params['RELEASE'],
+                                   "multiverse"),
+                                  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
+        """
+        cfg = {'source': 'deb $MIRROR $RELEASE multiverse',
+               'filename': self.aptlistfile}
+        self.apt_src_replacement(self.aptlistfile, [cfg])
+
+    def apt_src_replace_tri(self, cfg):
+        """ apt_src_replace_tri
+        Test three autoreplacements of MIRROR and RELEASE in source specs with
+        generic part
+        """
+        self.apt_src_replacement(self.aptlistfile, cfg)
+
+        # extra verify on two extra files of this test
+        params = self._get_default_params()
+        contents = load_tfile_or_url(self.aptlistfile2)
+        self.assertTrue(re.search(r"%s %s %s %s\n" %
+                                  ("deb", params['MIRROR'], params['RELEASE'],
+                                   "main"),
+                                  contents, flags=re.IGNORECASE))
+        contents = load_tfile_or_url(self.aptlistfile3)
+        self.assertTrue(re.search(r"%s %s %s %s\n" %
+                                  ("deb", params['MIRROR'], params['RELEASE'],
+                                   "universe"),
+                                  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
+        """
+        cfg1 = {'source': 'deb $MIRROR $RELEASE multiverse',
+                'filename': self.aptlistfile}
+        cfg2 = {'source': 'deb $MIRROR $RELEASE main',
+                'filename': self.aptlistfile2}
+        cfg3 = {'source': 'deb $MIRROR $RELEASE universe',
+                'filename': self.aptlistfile3}
+        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.
+        """
+        cfg = {self.aptlistfile: {'source': 'deb $MIRROR $RELEASE multiverse'},
+               'notused':        {'source': 'deb $MIRROR $RELEASE main',
+                                  'filename': self.aptlistfile2},
+               self.aptlistfile3: {'source': 'deb $MIRROR $RELEASE universe'}}
+        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
+        """
+        cfg = {'source': 'deb $MIRROR $RELEASE multiverse'}
+        with mock.patch.object(os.path, 'join', side_effect=self.myjoin):
+            self.apt_src_replacement(self.fallbackfn, [cfg])
+
+    def apt_src_keyid(self, filename, cfg, keynum):
+        """ apt_src_keyid
+        Test specification of a source + keyid
+        """
+        params = self._get_default_params()
+
+        with mock.patch.object(util, 'subp',
+                               return_value=('fakekey 1234', '')) as mockobj:
+            cc_apt_configure.add_sources(cfg, params)
+
+        # check if it added the right ammount of keys
+        calls = []
+        for _ in range(keynum):
+            calls.append(call(('apt-key', 'add', '-'), 'fakekey 1234'))
+        mockobj.assert_has_calls(calls, any_order=True)
+
+        self.assertTrue(os.path.isfile(filename))
+
+        contents = load_tfile_or_url(filename)
+        self.assertTrue(re.search(r"%s %s %s %s\n" %
+                                  ("deb",
+                                   ('http://ppa.launchpad.net/smoser/'
+                                    'cloud-init-test/ubuntu'),
+                                   "xenial", "main"),
+                                  contents, flags=re.IGNORECASE))
+
+    def test_apt_src_keyid(self):
+        """ test_apt_src_keyid
+        Test specification of a source + keyid with filename being set
+        """
+        cfg = {'source': ('deb '
+                          'http://ppa.launchpad.net/'
+                          'smoser/cloud-init-test/ubuntu'
+                          ' xenial main'),
+               'keyid': "03683F77",
+               'filename': self.aptlistfile}
+        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
+        """
+        cfg1 = {'source': ('deb '
+                           'http://ppa.launchpad.net/'
+                           'smoser/cloud-init-test/ubuntu'
+                           ' xenial main'),
+                'keyid': "03683F77",
+                'filename': self.aptlistfile}
+        cfg2 = {'source': ('deb '
+                           'http://ppa.launchpad.net/'
+                           'smoser/cloud-init-test/ubuntu'
+                           ' xenial universe'),
+                'keyid': "03683F77",
+                'filename': self.aptlistfile2}
+        cfg3 = {'source': ('deb '
+                           'http://ppa.launchpad.net/'
+                           'smoser/cloud-init-test/ubuntu'
+                           ' xenial multiverse'),
+                'keyid': "03683F77",
+                'filename': self.aptlistfile3}
+
+        self.apt_src_keyid(self.aptlistfile, [cfg1, cfg2, cfg3], 3)
+        contents = load_tfile_or_url(self.aptlistfile2)
+        self.assertTrue(re.search(r"%s %s %s %s\n" %
+                                  ("deb",
+                                   ('http://ppa.launchpad.net/smoser/'
+                                    'cloud-init-test/ubuntu'),
+                                   "xenial", "universe"),
+                                  contents, flags=re.IGNORECASE))
+        contents = load_tfile_or_url(self.aptlistfile3)
+        self.assertTrue(re.search(r"%s %s %s %s\n" %
+                                  ("deb",
+                                   ('http://ppa.launchpad.net/smoser/'
+                                    'cloud-init-test/ubuntu'),
+                                   "xenial", "multiverse"),
+                                  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
+        """
+        cfg = {'source': ('deb '
+                          'http://ppa.launchpad.net/'
+                          'smoser/cloud-init-test/ubuntu'
+                          ' xenial main'),
+               'keyid': "03683F77"}
+        with mock.patch.object(os.path, 'join', side_effect=self.myjoin):
+            self.apt_src_keyid(self.fallbackfn, [cfg], 1)
+
+    def apt_src_key(self, filename, cfg):
+        """ apt_src_key
+        Test specification of a source + key
+        """
+        params = self._get_default_params()
+
+        with mock.patch.object(util, 'subp') as mockobj:
+            cc_apt_configure.add_sources([cfg], params)
+
+        mockobj.assert_called_with(('apt-key', 'add', '-'), 'fakekey 4321')
+
+        self.assertTrue(os.path.isfile(filename))
+
+        contents = load_tfile_or_url(filename)
+        self.assertTrue(re.search(r"%s %s %s %s\n" %
+                                  ("deb",
+                                   ('http://ppa.launchpad.net/smoser/'
+                                    'cloud-init-test/ubuntu'),
+                                   "xenial", "main"),
+                                  contents, flags=re.IGNORECASE))
+
+    def test_apt_src_key(self):
+        """ test_apt_src_key
+        Test specification of a source + key with filename being set
+        """
+        cfg = {'source': ('deb '
+                          'http://ppa.launchpad.net/'
+                          'smoser/cloud-init-test/ubuntu'
+                          ' xenial main'),
+               'key': "fakekey 4321",
+               'filename': self.aptlistfile}
+        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
+        """
+        cfg = {'source': ('deb '
+                          'http://ppa.launchpad.net/'
+                          'smoser/cloud-init-test/ubuntu'
+                          ' xenial main'),
+               'key': "fakekey 4321"}
+        with mock.patch.object(os.path, 'join', side_effect=self.myjoin):
+            self.apt_src_key(self.fallbackfn, cfg)
+
+    def test_apt_src_keyonly(self):
+        """ test_apt_src_keyonly
+        Test specification 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)
+
+        mockobj.assert_called_once_with(('apt-key', 'add', '-'),
+                                        'fakekey 4242')
+
+        # filename should be ignored on key only
+        self.assertFalse(os.path.isfile(self.aptlistfile))
+
+    def test_apt_src_keyidonly(self):
+        """ test_apt_src_keyidonly
+        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)
+
+        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 (nothing but add_key_raw mocked)
+        """
+        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:
+            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))
+
+    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:
+            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))
+
+    def test_apt_src_ppa(self):
+        """ test_apt_src_ppa
+        Test specification of a ppa
+        """
+        params = self._get_default_params()
+        cfg = {'source': 'ppa:smoser/cloud-init-test',
+               'filename': self.aptlistfile}
+
+        # default matcher needed for ppa
+        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)
+        mockobj.assert_called_once_with(['add-apt-repository',
+                                         'ppa:smoser/cloud-init-test'])
+
+        # adding ppa should ignore filename (uses add-apt-repository)
+        self.assertFalse(os.path.isfile(self.aptlistfile))
+
+    def test_apt_src_ppa_tri(self):
+        """ test_apt_src_ppa_tri
+        Test specification of a ppa
+        """
+        params = self._get_default_params()
+        cfg1 = {'source': 'ppa:smoser/cloud-init-test',
+                'filename': self.aptlistfile}
+        cfg2 = {'source': 'ppa:smoser/cloud-init-test2',
+                'filename': self.aptlistfile2}
+        cfg3 = {'source': 'ppa:smoser/cloud-init-test3',
+                'filename': self.aptlistfile3}
+
+        # default matcher needed for ppa
+        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)
+        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'])]
+        mockobj.assert_has_calls(calls, any_order=True)
+
+        # adding ppa should ignore all filenames (uses add-apt-repository)
+        self.assertFalse(os.path.isfile(self.aptlistfile))
+        self.assertFalse(os.path.isfile(self.aptlistfile2))
+        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
+        """
+        cfg1 = {'source': 'deb $MIRROR $RELEASE multiverse',
+                'filename': self.aptlistfile}
+        cfg2 = {'source': 'deb $MIRROR $RELEASE main',
+                'filename': self.aptlistfile2}
+        cfg3 = {'source': 'deb $MIRROR $RELEASE universe',
+                'filename': self.aptlistfile3}
+        checkcfg = {self.aptlistfile: {'filename': self.aptlistfile,
+                                       'source': 'deb $MIRROR $RELEASE '
+                                                 'multiverse'},
+                    self.aptlistfile2: {'filename': self.aptlistfile2,
+                                        'source': 'deb $MIRROR $RELEASE main'},
+                    self.aptlistfile3: {'filename': self.aptlistfile3,
+                                        'source': 'deb $MIRROR $RELEASE '
+                                                  'universe'}}
+
+        newcfg = cc_apt_configure.convert_to_new_format([cfg1, cfg2, cfg3])
+        self.assertEqual(newcfg, checkcfg)
+
+        newcfg2 = cc_apt_configure.convert_to_new_format(newcfg)
+        self.assertEqual(newcfg2, checkcfg)
+
+        with self.assertRaises(ValueError):
+            cc_apt_configure.convert_to_new_format(5)
+
+
+# vi: ts=4 expandtab