Merge ~jawn-smith/ubuntu/+source/ubuntu-release-upgrader:ubuntu/jammy into ubuntu/+source/ubuntu-release-upgrader:ubuntu/jammy

[William Wilson]

If upgrading to jammy on a Raspberry Pi, check for the presence of the broken "match" section of the yaml config. This was present in the default config for Raspberry Pi images dating back to focal.

e7a2db9... by William Wilson on 2022-05-05

Improve boundary checking to avoid out of bounds error

[Brian Murray (brian-murray)]

See my in-line comment.

review: Needs Fixing

2fee82f... by William Wilson on 2022-05-11

Define failure_action string to be printed

[Dave Jones (waveform)]

I'm afraid there's a few issues:

* I'd much rather parse the YAML with a yaml library given there's so many possible representations here (which cloud-init is free to use when it converts network-config to the netplan config). For example, match: {driver: "bcmgenet smsc95xx lan78xx"} is valid YAML. I know adding dependencies is generally frowned upon, but python3-yaml is seeded on the pi server and desktop images (presumably because cloud-init and ubuntu-advantage-tools both depend on it, among many other things) so adding it as a dep of u-r-u won't *actually* pull in anything new, would simplify this code, and deal with all possible variants of the configuration.

* If you remove the match/driver: clause, the set-name: clause becomes invalid (it requires a match: clause). In this case it's probably sufficient to just remove the set-name: clause along with the match: clause.

* Ideally the config_txt variable in the various tests should be renamed; it's not the config.txt on the boot partition that's being modified (unlike the prior tests concerned with removing u-boot), it's the netplan yaml.

Other than that: nice to see a good set of tests, covering all the netplan config scenarios (match present, not present, present but only in certain files)!

review: Needs Fixing

[William Wilson (jawn-smith)]

Ah I had thought about using python3-yaml but didn't want to add dependencies to the upgrader, which I figured could be problematic. I didn't check if it was seeded though.

[Nick Rosbrook (enr0n)]

If you are going to add python3-yaml as a dependency in jammy, then it should either (1) be SRU'd to focal and impish, or (2) there should be logic to handle a potential import exception.

This recently came up in bug 1965568, i.e. I learned this the hard way.

[Brian Murray (brian-murray)]

> If you are going to add python3-yaml as a dependency in jammy, then it should
> either (1) be SRU'd to focal and impish, or (2) there should be logic to
> handle a potential import exception.
>
> This recently came up in bug 1965568, i.e. I learned this the hard way.

Option 1 would result in there be two SRUs. One from the release being upgraded from and one for the release being upgraded to.

[Dave Jones (waveform)]

If it were a genuinely new dependency I would indeed worry, but I'm *fairly* sure python3-yaml's been seeded since forever (or since focal at least, which is the earliest we need to concern ourselves with as upgrades never span more than one LTS). Just checking ...

$ curl -s http://cdimage.ubuntu.com/ubuntu/releases/focal/release/ubuntu-20.04.4-preinstalled-server-arm64+raspi.manifest | grep python3-yaml
python3-yaml 5.3.1-1ubuntu0.1
$ curl -s http://cdimage.ubuntu.com/ubuntu/releases/focal/release/ubuntu-20.04.4-preinstalled-server-armhf+raspi.manifest | grep python3-yaml
python3-yaml 5.3.1-1ubuntu0.1
$ curl -s http://old-releases.ubuntu.com/releases/20.04.0/ubuntu-20.04-desktop-amd64.manifest | grep python3-yaml
python3-yaml 5.3.1-1
$ curl -s http://old-releases.ubuntu.com/releases/20.04.0/ubuntu-20.04-live-server-amd64.manifest | grep python3-yaml
python3-yaml 5.3.1-1

Yup, python3-yaml was seeded at least as far back as focal on PC server, PC desktop, and pi server images. So, technically yes, an SRU *should* be done but I wouldn't worry greatly in this particular case as python3-yaml should be universally installed.

[Nick Rosbrook (enr0n)]

> Yup, python3-yaml was seeded at least as far back as focal on PC server, PC
> desktop, and pi server images. So, technically yes, an SRU *should* be done
> but I wouldn't worry greatly in this particular case as python3-yaml should be
> universally installed.

Right, then option (2) is probably more appropriate in this case. William's original patch already has a helpful `failure_action` string, so this should be displayed to the user if for some reason `import yaml` raises an exception.

4f2b990... by William Wilson on 2022-05-23

Use python yaml library instead of parsing text line by line

61b3a82... by William Wilson on 2022-05-23

change os.rename to a file write to avoid missing configs

[Dave Jones (waveform)]

Some inline comments; one thing needs changing (orig_yaml being a ref to netplan_yaml) but otherwise looking much better!

c44644f... by William Wilson on 2022-05-24

Ensure backups are correct and have comments preserved

[Dave Jones (waveform)]

Excellent, looking good now (and no, there's nothing wrong with KeyError: continue :)

review: Approve

1e210e1... by William Wilson on 2022-05-24

Remove extra whitespace

Unmerged commits

1e210e1... by William Wilson on 2022-05-24

Remove extra whitespace

c44644f... by William Wilson on 2022-05-24

Ensure backups are correct and have comments preserved

61b3a82... by William Wilson on 2022-05-23

change os.rename to a file write to avoid missing configs

4f2b990... by William Wilson on 2022-05-23

Use python yaml library instead of parsing text line by line

2fee82f... by William Wilson on 2022-05-11

Define failure_action string to be printed

e7a2db9... by William Wilson on 2022-05-05

Improve boundary checking to avoid out of bounds error

4880202... by William Wilson on 2022-05-05

Remove invalid match section from default Raspberry Pi netplan config after upgrades to jammy

diff --git a/DistUpgrade/DistUpgradeQuirks.py b/DistUpgrade/DistUpgradeQuirks.py
index ccbb117..c6be753 100644
--- a/DistUpgrade/DistUpgradeQuirks.py
+++ b/DistUpgrade/DistUpgradeQuirks.py
@@ -151,6 +151,7 @@ class DistUpgradeQuirks(object):
         if 'linux-firmware-raspi2' in cache:
             if cache['linux-firmware-raspi2'].is_installed:
                 self._remove_uboot_on_rpi()
+                self._remove_netplan_match_rpi()
 
     # individual quirks handler when the dpkg run is finished ---------
     def PostCleanup(self):
@@ -1377,3 +1378,62 @@ class DistUpgradeQuirks(object):
         except IOError as exc:
             logging.error("unable to write new boot config to %s: %s; %s",
                           boot_config_filename, exc, failure_action)
+
+    def _remove_netplan_match_rpi(self, netplan_dir='/etc/netplan'):
+        failure_action = (
+            "You may need to manually edit your netplan config "
+            "and remove the line \"driver: bcmgenet smsc95xx lan78xx\". "
+            "See LP: #1970761 for further details")
+
+        try:
+            import yaml
+        except ImportError:
+            logging.error("failed to import yaml library. %s", failure_action)
+            return
+
+        for netplan_config_file in glob.glob("%s/*.yaml" % netplan_dir):
+            try:
+                netplan_config_filename = os.path.join(netplan_dir,
+                                                       netplan_config_file)
+                with open(netplan_config_filename, 'r', encoding='utf-8') as f:
+                    orig_content = f.read()
+                    netplan_yaml = yaml.load(orig_content,
+                                             Loader=yaml.SafeLoader)
+
+            except FileNotFoundError:
+                logging.error("failed to open netplan configuration in %s; %s",
+                              netplan_config_filename, failure_action)
+                return
+
+            file_modified = False
+            try:
+                for eth in netplan_yaml["network"]["ethernets"].values():
+                    if eth["match"]["driver"] == "bcmgenet smsc95xx lan78xx":
+                        file_modified = True
+                        del eth["match"]
+                        if "set-name" in eth:
+                            del eth["set-name"]
+            except KeyError:
+                continue
+
+            if not file_modified:
+                continue
+
+            try:
+                netplan_backup_filename = (netplan_config_filename +
+                                           '.distUpgrade')
+                with open(netplan_backup_filename, 'w', encoding='utf-8') as f:
+                    f.write(orig_content)
+            except IOError as exc:
+                logging.error("unable to write netplan config backup to "
+                              "%s: %s; %s",
+                              netplan_backup_filename, exc, failure_action)
+                return
+            try:
+                with open(netplan_config_filename, 'w', encoding='utf-8') as f:
+                    yaml.dump(netplan_yaml, f, default_flow_style=False)
+            except IOError as exc:
+                logging.error("unable to write new netplan config to "
+                              "%s: %s; %s",
+                              netplan_config_filename, exc, failure_action)
+                return
diff --git a/debian/changelog b/debian/changelog
index ac95dd1..e073f5d 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+ubuntu-release-upgrader (1:22.04.11) jammy; urgency=medium
+
+  * DistUpgrade/DistUpgradeQuirks.py: Remove section of netplan
+    config that is installed by default on Raspberry Pi and
+    causes netplan failures after upgrade to jammy (LP: #1970761)
+
+ -- William 'jawn-smith' Wilson <[e-mail]>  Tue, 03 May 2022 12:54:45 -0500
+
 ubuntu-release-upgrader (1:22.04.10) jammy; urgency=medium
 
   * DistUpgrade: Set NEEDRESTART_SUSPEND during upgrades (LP: #1968919).
diff --git a/tests/test_quirks.py b/tests/test_quirks.py
index 09b0160..eedf8f1 100644
--- a/tests/test_quirks.py
+++ b/tests/test_quirks.py
@@ -10,6 +10,7 @@ import unittest
 import shutil
 import tempfile
 import json
+import yaml
 
 from DistUpgrade.DistUpgradeQuirks import DistUpgradeQuirks
 
@@ -721,6 +722,283 @@ initramfs initrd.img followkernel
             with open(os.path.join(boot_dir, 'config.txt')) as f:
                 self.assertTrue(f.read() == expected_config_txt)
 
+    # This test case tests the scenario where the invalid
+    # match section is present and needs to be removed
+    def test_remove_netplan_match_single_removal(self):
+        with tempfile.TemporaryDirectory() as netplan_dir:
+            config_yaml = """\
+network:
+  ethernets:
+    # this is a test comment to ensure it is preserved in the backup
+    eth0:
+      dhcp4: true
+      # this section will be deleted
+      match:
+        driver: bcmgenet smsc95xx lan78xx
+        name: en*
+      optional: true
+      set-name: eth0
+  wifis:
+    wlan0:
+      dhcp4: true
+      access-points:
+        "TestWifi":
+          password: "TestPassword"
+  version: 2
+"""
+            expected_config_yaml = """\
+network:
+  ethernets:
+    eth0:
+      dhcp4: true
+      optional: true
+  wifis:
+    wlan0:
+      dhcp4: true
+      access-points:
+        "TestWifi":
+          password: "TestPassword"
+  version: 2
+"""
+            config_filename = '99-test-netplan.yaml'
+            with open(os.path.join(netplan_dir, config_filename), 'w') as f:
+                f.write(config_yaml)
+
+            mock_controller = mock.Mock()
+            q = DistUpgradeQuirks(mock_controller, mock.Mock())
+            q._remove_netplan_match_rpi(netplan_dir)
+
+            # make sure the backup exists and is the same as the original
+            backup_config_filename = config_filename + '.distUpgrade'
+            self.assertTrue(os.path.exists(os.path.join(
+                netplan_dir, backup_config_filename)))
+            with open(os.path.join(netplan_dir,
+                      backup_config_filename)) as f:
+                self.assertTrue(f.read() == config_yaml)
+
+            # make sure the new contents are what we expect
+            with open(os.path.join(netplan_dir, config_filename)) as f:
+                new_config = yaml.load(f, yaml.SafeLoader)
+                expected_config = yaml.load(expected_config_yaml,
+                                            yaml.SafeLoader)
+                self.assertTrue(new_config == expected_config)
+
+    # This test case tests the scenario where the match
+    # section is present but is valid syntax
+    def test_remove_netplan_match_valid_syntax(self):
+        with tempfile.TemporaryDirectory() as netplan_dir:
+            config_yaml = """\
+network:
+  ethernets:
+    eth0:
+      dhcp4: true
+      match:
+        # this is a test comment to ensure it is preserved in the backup
+        driver: ["bcmgenet", "smsc95xx", "lan78xx"]
+      optional: true
+      set-name: eth0
+  wifis:
+    wlan0:
+      dhcp4: true
+      access-points:
+        "TestWifi":
+          password: "TestPassword"
+  version: 2
+"""
+
+            mock_controller = mock.Mock()
+            q = DistUpgradeQuirks(mock_controller, mock.Mock())
+            q._remove_netplan_match_rpi(netplan_dir)
+
+            self.assertFalse(os.path.exists(os.path.join(
+                netplan_dir, '50-cloud-init.yaml.distUpgrade')))
+
+    # This test case tests the scenario where the match
+    # section not present at all
+    def test_remove_netplan_match_no_match(self):
+        with tempfile.TemporaryDirectory() as netplan_dir:
+            config_yaml = """\
+network:
+  ethernets:
+    eth0:
+      dhcp4: true
+      optional: true
+  wifis:
+    wlan0:
+      dhcp4: true
+      access-points:
+        "TestWifi":
+          password: "TestPassword"
+    version: 2
+"""
+
+            mock_controller = mock.Mock()
+            q = DistUpgradeQuirks(mock_controller, mock.Mock())
+            q._remove_netplan_match_rpi(netplan_dir)
+
+            self.assertFalse(os.path.exists(os.path.join(
+                netplan_dir, '50-cloud-init.yaml.distUpgrade')))
+
+    # This test case tests the scenario where the invalid
+    # match section is present in one file, but multiple
+    # config files exist
+    def test_remove_netplan_match_one_of_multiple(self):
+        with tempfile.TemporaryDirectory() as netplan_dir:
+            ethernet_config_yaml = """\
+############
+# also
+# test
+# multi
+# line
+# comments
+# are
+# preserved
+############
+network:
+  ethernets:
+    eth0:
+      dhcp4: true
+      match:
+        driver: bcmgenet smsc95xx lan78xx
+      optional: true
+      set-name: eth0
+  version: 2
+"""
+            wifi_config_yaml = """\
+network:
+  wifis:
+    wlan0:
+      dhcp4: true
+      access-points:
+        "TestWifi":
+          password: "TestPassword"
+  version: 2
+"""
+            expected_ethernet_config_yaml = """\
+network:
+  ethernets:
+    eth0:
+      dhcp4: true
+      optional: true
+  version: 2
+"""
+            e_config_filename = '01-test-ethernet.yaml'
+            with open(os.path.join(netplan_dir, e_config_filename), 'w') as f:
+                f.write(ethernet_config_yaml)
+
+            w_config_filename = '02-test-wifi.yaml'
+            with open(os.path.join(netplan_dir, w_config_filename), 'w') as f:
+                f.write(wifi_config_yaml)
+
+            mock_controller = mock.Mock()
+            q = DistUpgradeQuirks(mock_controller, mock.Mock())
+            q._remove_netplan_match_rpi(netplan_dir)
+
+            # make sure the backup exists and is the same as the original
+            backup_config_filename = e_config_filename + '.distUpgrade'
+            self.assertTrue(os.path.exists(os.path.join(
+                netplan_dir, backup_config_filename)))
+            with open(os.path.join(netplan_dir,
+                      backup_config_filename)) as f:
+                self.assertTrue(f.read() == ethernet_config_yaml)
+
+            # make sure the new contents are what we expect
+            with open(os.path.join(netplan_dir, e_config_filename)) as f:
+                new_config = yaml.load(f, yaml.SafeLoader)
+                expected_config = yaml.load(expected_ethernet_config_yaml,
+                                            yaml.SafeLoader)
+                self.assertTrue(new_config == expected_config)
+
+            # make sure a backup wasn't created for the config
+            # that doesn't need modification
+            self.assertFalse(os.path.exists(os.path.join(
+                netplan_dir, w_config_filename + '.distUpgrade')))
+
+    # This test case tests the scenario where the invalid
+    # match section is present in multiple files
+    def test_remove_netplan_match_multiple(self):
+        with tempfile.TemporaryDirectory() as netplan_dir:
+            eth0_config_yaml = """\
+# comments can go at the top
+network:
+  ethernets:
+    eth0:
+      dhcp4: true
+      match:
+        driver: bcmgenet smsc95xx lan78xx
+      optional: true
+      set-name: eth0
+  version: 2
+"""
+            eth1_config_yaml = """\
+network:
+  ethernets:
+    eth1:
+      dhcp4: true
+      match:
+        driver: bcmgenet smsc95xx lan78xx
+  version: 2
+# comments can go at the bottom too
+"""
+            expected_eth0_config_yaml = """\
+network:
+  ethernets:
+    eth0:
+      dhcp4: true
+      optional: true
+  version: 2
+"""
+            expected_eth1_config_yaml = """\
+network:
+  ethernets:
+    eth1:
+      dhcp4: true
+  version: 2
+"""
+            eth0_config_filename = '01-test-eth0.yaml'
+            with open(os.path.join(netplan_dir,
+                      eth0_config_filename), 'w') as f:
+                f.write(eth0_config_yaml)
+
+            eth1_config_filename = '02-test-eth1.yaml'
+            with open(os.path.join(netplan_dir,
+                      eth1_config_filename), 'w') as f:
+                f.write(eth1_config_yaml)
+
+            mock_controller = mock.Mock()
+            q = DistUpgradeQuirks(mock_controller, mock.Mock())
+            q._remove_netplan_match_rpi(netplan_dir)
+
+            # make sure the backup exists and is the same as the original
+            backup_eth0_filename = eth0_config_filename + '.distUpgrade'
+            self.assertTrue(os.path.exists(os.path.join(
+                netplan_dir, backup_eth0_filename)))
+            with open(os.path.join(netplan_dir,
+                      backup_eth0_filename)) as f:
+                self.assertTrue(f.read() == eth0_config_yaml)
+
+            # make sure the modified contents are what we expect
+            with open(os.path.join(netplan_dir, eth0_config_filename)) as f:
+                new_config = yaml.load(f, yaml.SafeLoader)
+                expected_config = yaml.load(expected_eth0_config_yaml,
+                                            yaml.SafeLoader)
+                self.assertTrue(new_config == expected_config)
+
+            # make sure the backup exists and is the same as the original
+            backup_eth1_filename = eth1_config_filename + '.distUpgrade'
+            self.assertTrue(os.path.exists(os.path.join(
+                netplan_dir, backup_eth1_filename)))
+            with open(os.path.join(netplan_dir,
+                      backup_eth1_filename)) as f:
+                self.assertTrue(f.read() == eth1_config_yaml)
+
+            # make sure the modified contents are what we expect
+            with open(os.path.join(netplan_dir, eth1_config_filename)) as f:
+                new_config = yaml.load(f, yaml.SafeLoader)
+                expected_config = yaml.load(expected_eth1_config_yaml,
+                                            yaml.SafeLoader)
+                self.assertTrue(new_config == expected_config)
+
 
 class TestSnapQuirks(unittest.TestCase):