Merge ~ogayot/curtin:curthooks-fix-apt-config-translation into curtin:master

[Olivier Gayot]

apt-config: fix curthooks unconditionally triggering apt-config

The following commit:

  1257a38f translate old curtin apt features to new format

introduced the ability to translate old top level keys related to apt
features (e.g., debconf_selections, apt-proxy, apt-mirrors) to the new
format where they are stored as children of 'apt'.

```
                              apt:
debconf_selections: => debconf_selections:
  foobar foobar
```

Sadly, by doing so, it introduced a regression, making curthooks call
apt_config.handle_apt unconditionally.

The curthooks.do_apt_config function only calls apt_config.handle_apt if
the configuration does not contain the 'apt' key or if the 'apt' key has
the value None (i.e., null in YAML). When implementing the translation
from old to new configuration format, we accidentally forced the 'apt'
key to exist and coerced it to dictionary.

This effectively defeats the condition in curthooks.do_apt_config.

Fixed by making sure we only create the apt key if needed when doing the
translation.

The conditional execution of apt_config.handle_apt from curthooks builtins has been broken for a while. A regression was introduced in 2016 when adding a mechanism to translate things like:

```
                                  apt:
    debconf_selections: => debconf_selections:
      foobar foobar
```

The function responsible for calling handle_apt is as follows:

def do_apt_config(cfg, target):
    cfg = apt_config.translate_old_apt_features(cfg)
    apt_cfg = cfg.get("apt")
    if apt_cfg is not None:
        LOG.info("curthooks handling apt to target %s with config %s",
                 target, apt_cfg)
        apt_config.handle_apt(apt_cfg, target)
    else:
        LOG.info("No apt config provided, skipping")

Sadly, the implementation of translate_old_apt_features made is so that the `if apt_cfg is not None:` condition is always true.

This first patch fixes the issue by making sure the translate_old_apt_features function only creates an 'apt' key and assign it a dictionary, if necessary. This also covers the functions with unit tests.

The second patch also fixes an obvious mistake in a debug log.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:f83b46ec951394599c75ab4952166af0ff707599
https://jenkins.canonical.com/server-team/job/curtin-ci/82/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/82/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/82/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/82/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/82/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/82//rebuild

review: Approve (continuous-integration)

[Michael Hudson-Doyle (mwhudson)]

Good job on figuring out what's going on here. As usual I think we can clean the situation up a bit more though! Let me know what you think.

[Olivier Gayot (ogayot)]

Thanks. I'm leaning towards making translate_old_apt_features mutate the config. Shallow copies are not enough in this context.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:cc23249707a2773ecb1f10b17de35fff4620de9a
https://jenkins.canonical.com/server-team/job/curtin-ci/85/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/85/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/85/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/85/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/85/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/85//rebuild

review: Approve (continuous-integration)

[Dan Bungert (dbungert)]

LGTM, quick test suggestion then please merge if Michael is content.

review: Approve

[Michael Hudson-Doyle (mwhudson)]

LGTM, certainly an improvement.

review: Approve

diff --git a/curtin/commands/apt_config.py b/curtin/commands/apt_config.py
index f9cce34..e02327a 100644
--- a/curtin/commands/apt_config.py
+++ b/curtin/commands/apt_config.py
@@ -686,11 +686,9 @@ def apt_command(args):
 
 
 def translate_old_apt_features(cfg):
-    """translate the few old apt related features into the new config format"""
-    predef_apt_cfg = cfg.get("apt")
-    if predef_apt_cfg is None:
-        cfg['apt'] = {}
-        predef_apt_cfg = cfg.get("apt")
+    """translate the few old apt related features into the new config format ;
+    by mutating the cfg object. """
+    predef_apt_cfg = cfg.get("apt", {})
 
     if cfg.get('apt_proxy') is not None:
         if predef_apt_cfg.get('proxy') is not None:
@@ -700,9 +698,10 @@ def translate_old_apt_features(cfg):
             LOG.error(msg)
             raise ValueError(msg)
 
+        cfg.setdefault('apt', {})
         cfg['apt']['proxy'] = cfg.get('apt_proxy')
         LOG.debug("Transferred %s into new format: %s", cfg.get('apt_proxy'),
-                  cfg.get('apte'))
+                  cfg.get('apt'))
         del cfg['apt_proxy']
 
     if cfg.get('apt_mirrors') is not None:
@@ -714,6 +713,7 @@ def translate_old_apt_features(cfg):
             raise ValueError(msg)
 
         old = cfg.get('apt_mirrors')
+        cfg.setdefault('apt', {})
         cfg['apt']['primary'] = [{"arches": ["default"],
                                   "uri": old.get('ubuntu_archive')}]
         cfg['apt']['security'] = [{"arches": ["default"],
@@ -730,6 +730,7 @@ def translate_old_apt_features(cfg):
                        "are mutually exclusive")
                 LOG.error(msg)
                 raise ValueError(msg)
+        cfg.setdefault('apt', {})
         cfg['apt']['preserve_sources_list'] = False
 
     if cfg.get('debconf_selections') is not None:
@@ -741,14 +742,13 @@ def translate_old_apt_features(cfg):
             raise ValueError(msg)
 
         selsets = cfg.get('debconf_selections')
+        cfg.setdefault('apt', {})
         cfg['apt']['debconf_selections'] = selsets
         LOG.info("Transferred %s into new format: %s",
                  cfg.get('debconf_selections'),
                  cfg.get('apt'))
         del cfg['debconf_selections']
 
-    return cfg
-
 
 CMD_ARGUMENTS = (
     ((('-c', '--config'),
diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
index 69833a2..74a67e8 100644
--- a/curtin/commands/curthooks.py
+++ b/curtin/commands/curthooks.py
@@ -89,7 +89,7 @@ UEFI_BOOT_ENTRY_IS_NETWORK = r'.*(Network|PXE|NIC|Ethernet|LAN|IP4|IP6)+.*'
 
 
 def do_apt_config(cfg, target):
-    cfg = apt_config.translate_old_apt_features(cfg)
+    apt_config.translate_old_apt_features(cfg)
     apt_cfg = cfg.get("apt")
     if apt_cfg is not None:
         LOG.info("curthooks handling apt to target %s with config %s",
diff --git a/tests/unittests/test_apt_source.py b/tests/unittests/test_apt_source.py
index 9fb3fc5..9a840c2 100644
--- a/tests/unittests/test_apt_source.py
+++ b/tests/unittests/test_apt_source.py
@@ -678,6 +678,50 @@ Pin-Priority: -1
 
         mockobj.assert_called_with("preferencesfn", expected_content)
 
+    def test_translate_old_apt_features(self):
+        cfg = {}
+        apt_config.translate_old_apt_features(cfg)
+        self.assertEqual(cfg, {})
+
+        cfg = {"debconf_selections": "foo"}
+        apt_config.translate_old_apt_features(cfg)
+        self.assertEqual(cfg, {"apt": {"debconf_selections": "foo"}})
+
+        cfg = {"apt_proxy": {"http_proxy": "http://proxy:3128"}}
+        apt_config.translate_old_apt_features(cfg)
+        self.assertEqual(cfg, {
+            "apt": {
+                "proxy": {"http_proxy": "http://proxy:3128"},
+            }}
+        )
+
+        cfg = {
+            "apt": {"debconf_selections": "foo"},
+            "apt_proxy": {"http_proxy": "http://proxy:3128"},
+        }
+        apt_config.translate_old_apt_features(cfg)
+        self.assertEqual(cfg, {
+            "apt": {
+                "proxy": {"http_proxy": "http://proxy:3128"},
+                "debconf_selections": "foo",
+            }}
+        )
+
+    def test_translate_old_apt_features_conflicts(self):
+        with self.assertRaisesRegex(ValueError, 'mutually exclusive'):
+            apt_config.translate_old_apt_features({
+                "debconf_selections": "foo",
+                "apt": {
+                    "debconf_selections": "bar",
+                }})
+
+        with self.assertRaisesRegex(ValueError, 'mutually exclusive'):
+            apt_config.translate_old_apt_features({
+                "apt_proxy": {"http_proxy": "http://proxy:3128"},
+                "apt": {
+                    "proxy": {"http_proxy": "http://proxy:3128"},
+                }})
+
     def test_mirror(self):
         """test_mirror - Test defining a mirror"""
         pmir = "http://us.archive.ubuntu.com/ubuntu/"
diff --git a/tests/unittests/test_curthooks.py b/tests/unittests/test_curthooks.py
index c13434c..9e4fa87 100644
--- a/tests/unittests/test_curthooks.py
+++ b/tests/unittests/test_curthooks.py
@@ -2268,4 +2268,37 @@ class TestUefiFindGrubDeviceIds(CiTestCase):
                              self._sconfig(cfg)))
 
 
+class TestDoAptConfig(CiTestCase):
+    def setUp(self):
+        super(TestDoAptConfig, self).setUp()
+        self.handle_apt_sym = 'curtin.commands.curthooks.apt_config.handle_apt'
+
+    def test_no_apt_config(self):
+        with patch(self.handle_apt_sym) as m_handle_apt:
+            curthooks.do_apt_config({}, target="/")
+        m_handle_apt.assert_not_called()
+
+    def test_apt_config_none(self):
+        with patch(self.handle_apt_sym) as m_handle_apt:
+            curthooks.do_apt_config({"apt": None}, target="/")
+        m_handle_apt.assert_not_called()
+
+    def test_apt_config_dict(self):
+        with patch(self.handle_apt_sym) as m_handle_apt:
+            curthooks.do_apt_config({"apt": {}}, target="/")
+        m_handle_apt.assert_called()
+
+    def test_with_apt_config(self):
+        with patch(self.handle_apt_sym) as m_handle_apt:
+            curthooks.do_apt_config(
+                    {"apt": {"proxy": {"http_proxy": "http://proxy:3128"}}},
+                    target="/")
+        m_handle_apt.assert_called_once()
+
+    def test_with_debconf_selections(self):
+        # debconf_selections are translated to apt config
+        with patch(self.handle_apt_sym) as m_handle_apt:
+            curthooks.do_apt_config({"debconf_selections": "foo"}, target="/")
+        m_handle_apt.assert_called_once()
+
 # vi: ts=4 expandtab syntax=python