Merge ~paelzer/cloud-init:fix-bug-1616831-new-and-old-apt-config into cloud-init:master

[Christian Ehrhardt ]

apt-config: Prefer V3 format if specified together with V1/2 (LP: #1616831)

This set of changes contains the code addressing an issue if V1/2 AND V3 configs are specified at the same time. With these changes the V3 config is preferred now.

There are some sanity checks in place that kick in if the old (now dropped due to the preference) values differ from those that will be used (there would be too much danger to "silently" ignore something of the V1 spec and the user wondering why his changes take no effect.

It also extends and adds unitests in that area to cover even more of the existing and now added format conversion logic.

Passed make check, tox and bddeb+sbuild and thereby I hope ready for review.

diff --git a/cloudinit/config/cc_apt_configure.py b/cloudinit/config/cc_apt_configure.py
index 609dbb5..42c5641 100644
--- a/cloudinit/config/cc_apt_configure.py
+++ b/cloudinit/config/cc_apt_configure.py
@@ -464,13 +464,19 @@ def convert_mirror(oldcfg, aptcfg):
 
 def convert_v2_to_v3_apt_format(oldcfg):
     """convert old to new keys and adapt restructured mirror spec"""
-    oldkeys = ['apt_sources', 'apt_mirror', 'apt_mirror_search',
-               'apt_mirror_search_dns', 'apt_proxy', 'apt_http_proxy',
-               'apt_ftp_proxy', 'apt_https_proxy',
-               'apt_preserve_sources_list', 'apt_custom_sources_list',
-               'add_apt_repo_match']
+    mapoldkeys = {'apt_sources': 'sources',
+                  'apt_mirror': None,
+                  'apt_mirror_search': None,
+                  'apt_mirror_search_dns': None,
+                  'apt_proxy': 'proxy',
+                  'apt_http_proxy': 'http_proxy',
+                  'apt_ftp_proxy': 'https_proxy',
+                  'apt_https_proxy': 'ftp_proxy',
+                  'apt_preserve_sources_list': 'preserve_sources_list',
+                  'apt_custom_sources_list': 'sources_list',
+                  'add_apt_repo_match': 'add_apt_repo_match'}
     needtoconvert = []
-    for oldkey in oldkeys:
+    for oldkey in mapoldkeys:
         if oldcfg.get(oldkey, None) is not None:
             needtoconvert.append(oldkey)
 
@@ -480,32 +486,37 @@ def convert_v2_to_v3_apt_format(oldcfg):
     LOG.debug("apt config: convert V2 to V3 format for keys '%s'",
               ", ".join(needtoconvert))
 
-    if oldcfg.get('apt', None) is not None:
-        msg = ("Error in apt configuration: "
-               "old and new format of apt features are mutually exclusive "
-               "('apt':'%s' vs '%s' key)" % (oldcfg.get('apt', None),
-                                             ", ".join(needtoconvert)))
-        LOG.error(msg)
-        raise ValueError(msg)
+    # if old AND new config are provided, prefer the new one (LP #1616831)
+    newaptcfg = oldcfg.get('apt', None)
+    if newaptcfg is not None:
+        LOG.debug("apt config: V1/2 and V3 format specified, preferring V3")
+        for oldkey in needtoconvert:
+            newkey = mapoldkeys[oldkey]
+            verify = oldcfg[oldkey]  # drop, but keep a ref for verification
+            del oldcfg[oldkey]
+            if newkey is None or newaptcfg.get(newkey, None) is None:
+                # no simple mapping or no collision on this particular key
+                continue
+            if verify != newaptcfg[newkey]:
+                raise ValueError("Old and New apt format defined with unequal "
+                                 "values %s vs %s @ %s" % (verify,
+                                                           newaptcfg[newkey],
+                                                           oldkey))
+        # return conf after clearing conflicting V1/2 keys
+        return oldcfg
 
     # create new format from old keys
     aptcfg = {}
 
-    # renames / moves under the apt key
-    convert_key(oldcfg, aptcfg, 'add_apt_repo_match', 'add_apt_repo_match')
-    convert_key(oldcfg, aptcfg, 'apt_proxy', 'proxy')
-    convert_key(oldcfg, aptcfg, 'apt_http_proxy', 'http_proxy')
-    convert_key(oldcfg, aptcfg, 'apt_https_proxy', 'https_proxy')
-    convert_key(oldcfg, aptcfg, 'apt_ftp_proxy', 'ftp_proxy')
-    convert_key(oldcfg, aptcfg, 'apt_custom_sources_list', 'sources_list')
-    convert_key(oldcfg, aptcfg, 'apt_preserve_sources_list',
-                'preserve_sources_list')
-    # dict format not changed since v2, just renamed and moved
-    convert_key(oldcfg, aptcfg, 'apt_sources', 'sources')
+    # simple renames / moves under the apt key
+    for oldkey in mapoldkeys:
+        if mapoldkeys[oldkey] is not None:
+            convert_key(oldcfg, aptcfg, oldkey, mapoldkeys[oldkey])
 
+    # mirrors changed in a more complex way
     convert_mirror(oldcfg, aptcfg)
 
-    for oldkey in oldkeys:
+    for oldkey in mapoldkeys:
         if oldcfg.get(oldkey, None) is not None:
             raise ValueError("old apt key '%s' left after conversion" % oldkey)
 
diff --git a/tests/unittests/test_handler/test_handler_apt_source_v1.py b/tests/unittests/test_handler/test_handler_apt_source_v1.py
index d96779c..ddff434 100644
--- a/tests/unittests/test_handler/test_handler_apt_source_v1.py
+++ b/tests/unittests/test_handler/test_handler_apt_source_v1.py
@@ -528,6 +528,7 @@ class TestAptSourceConfig(TestCase):
                 'filename': self.aptlistfile2}
         cfg3 = {'source': 'deb $MIRROR $RELEASE universe',
                 'filename': self.aptlistfile3}
+        cfg = {'apt_sources': [cfg1, cfg2, cfg3]}
         checkcfg = {self.aptlistfile: {'filename': self.aptlistfile,
                                        'source': 'deb $MIRROR $RELEASE '
                                                  'multiverse'},
@@ -537,15 +538,89 @@ class TestAptSourceConfig(TestCase):
                                         'source': 'deb $MIRROR $RELEASE '
                                                   'universe'}}
 
-        newcfg = cc_apt_configure.convert_v1_to_v2_apt_format([cfg1, cfg2,
-                                                               cfg3])
-        self.assertEqual(newcfg, checkcfg)
+        newcfg = cc_apt_configure.convert_to_v3_apt_format(cfg)
+        self.assertEqual(newcfg['apt']['sources'], checkcfg)
 
-        newcfg2 = cc_apt_configure.convert_v1_to_v2_apt_format(newcfg)
-        self.assertEqual(newcfg2, checkcfg)
+        # convert again, should stay the same
+        newcfg2 = cc_apt_configure.convert_to_v3_apt_format(newcfg)
+        self.assertEqual(newcfg2['apt']['sources'], checkcfg)
 
+        # should work without raising an exception
+        cc_apt_configure.convert_to_v3_apt_format({})
+
+        with self.assertRaises(ValueError):
+            cc_apt_configure.convert_to_v3_apt_format({'apt_sources': 5})
+
+    def test_convert_to_new_format_collision(self):
+        """Test the conversion of old to new format with collisions
+           That matches e.g. the MAAS case specifying old and new config"""
+        cfg_1_and_3 = {'apt': {'proxy': 'http://192.168.122.1:8000/'},
+                       'apt_proxy': 'http://192.168.122.1:8000/'}
+        cfg_3_only = {'apt': {'proxy': 'http://192.168.122.1:8000/'}}
+        cfgconflict = {'apt': {'proxy': 'http://192.168.122.1:8000/'},
+                       'apt_proxy': 'ftp://192.168.122.1:8000/'}
+
+        # collision (equal)
+        newcfg = cc_apt_configure.convert_to_v3_apt_format(cfg_1_and_3)
+        self.assertEqual(newcfg, cfg_3_only)
+        # collision (equal, so ok to remove)
+        newcfg = cc_apt_configure.convert_to_v3_apt_format(cfg_3_only)
+        self.assertEqual(newcfg, cfg_3_only)
+        # collision (unequal)
+        with self.assertRaises(ValueError):
+            cc_apt_configure.convert_to_v3_apt_format(cfgconflict)
+
+    def test_convert_to_new_format_dict_collision(self):
+        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}
+        fullv3 = {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'}}
+        cfg_3_only = {'apt': {'sources': fullv3}}
+        cfg_1_and_3 = {'apt_sources': [cfg1, cfg2, cfg3]}
+        cfg_1_and_3.update(cfg_3_only)
+
+        # collision (equal, so ok to remove)
+        newcfg = cc_apt_configure.convert_to_v3_apt_format(cfg_1_and_3)
+        self.assertEqual(newcfg, cfg_3_only)
+        # no old spec (same result)
+        newcfg = cc_apt_configure.convert_to_v3_apt_format(cfg_3_only)
+        self.assertEqual(newcfg, cfg_3_only)
+
+        diff = {self.aptlistfile: {'filename': self.aptlistfile,
+                                   'source': 'deb $MIRROR $RELEASE '
+                                             'DIFFERENTVERSE'},
+                self.aptlistfile2: {'filename': self.aptlistfile2,
+                                    'source': 'deb $MIRROR $RELEASE main'},
+                self.aptlistfile3: {'filename': self.aptlistfile3,
+                                    'source': 'deb $MIRROR $RELEASE '
+                                              'universe'}}
+        cfg_3_only = {'apt': {'sources': diff}}
+        cfg_1_and_3_different = {'apt_sources': [cfg1, cfg2, cfg3]}
+        cfg_1_and_3_different.update(cfg_3_only)
+
+        # collision (unequal by dict having a different entry)
+        with self.assertRaises(ValueError):
+            cc_apt_configure.convert_to_v3_apt_format(cfg_1_and_3_different)
+
+        missing = {self.aptlistfile: {'filename': self.aptlistfile,
+                                      'source': 'deb $MIRROR $RELEASE '
+                                                'multiverse'}}
+        cfg_3_only = {'apt': {'sources': missing}}
+        cfg_1_and_3_missing = {'apt_sources': [cfg1, cfg2, cfg3]}
+        cfg_1_and_3_missing.update(cfg_3_only)
+        # collision (unequal by dict missing an entry)
         with self.assertRaises(ValueError):
-            cc_apt_configure.convert_v1_to_v2_apt_format(5)
+            cc_apt_configure.convert_to_v3_apt_format(cfg_1_and_3_missing)
 
 
 # vi: ts=4 expandtab