apt-config: allow both old and new format to be present.

This allows both v1/2 and and v3 formats to exist in config.
If both are present, then prefer v3. If values are not the same
then a ValueError is raised.

LP: #1616831

2 files changed, 117 insertions, 31 deletions

diff --git a/cloudinit/config/cc_apt_configure.py b/cloudinit/config/cc_apt_configure.py
index 609dbb51..42c56418 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 d96779c5..ddff4341 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