Unset a subtree crashes

Bug #1942930 reported by Michael Vogt
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
netplan
Fix Released
Undecided
Simon Chopin
netplan.io (Ubuntu)
Fix Released
Undecided
Unassigned
Focal
Fix Released
Undecided
Unassigned
Hirsute
Fix Released
Undecided
Unassigned
Impish
Fix Released
Undecided
Unassigned

Bug Description

[Impact]

The netplan CLI would crash when trying to set an entire device type
subtree to null, e.g. `netplan set devices.ethernets=null`, and the
configuration would remain unaffected. This is obviously undesirable, as
the only workaround is to explictly loop through all the definitions.

The fix comes in two parts:

* add some internal APIs to libnetplan to be able to list all defintions
  of a given type, with some care taken in the consuming Python bindings
  if the symbols aren't present.

* use this new API to, when encountering this special case, expand the
  devtype subtree into all its definitions, letting the normal subtree
  nullification feature taking it up from there.

[Test Plan]

Boot a VM image with at least one ethernet device.
Write the following config to /etc/netplan/00-ethernets.yaml

network:
  version: 2
  ethernets:
    all-en:
      dhcp4: true
      match:
        name: en*
    all-eth:
      dhcp4: true
      match:
        name: eth*

Check that it has been taken into account using this command

netplan get

Unset the definitions we added:

netplan set network.ethernets=null

This command should silently succeed.
Finally, check that there are no ethernet device configured by running

netplan get

[Where problems could occur]

This is backported from a branch with considerable changes in the API
and ABI handling of libnetplan, so some ABI breakage might slip through
unnoticed, even though steps have been taken to prevent it.

The most risk of regression is in the `netplan set` command, as it is
where the existing code actually changes behaviour. This could be
particularly bad as this could overwrite existing configuration files
with erroneous configuration, leading to non-functional networking.

Also, the DBus endpoints for netplan directly uses the `netplan set`
command, so its users could be impacted as well.

[Original report]

While working on the snapd netplan integration I ran into the following peculiar issue.
I want to unset a subtree of configuration, e.g. I have:
# netplan get
network:
  version: 2
  ethernets:
    all-en:
      dhcp4: true
      match:
        name: en*
    all-eth:
      dhcp4: true
      match:
        name: eth*

and want to remove all ethernet configuration. AIUI this can be done via setting the subtree to "null" (or did I misunderstood this)? If I try that I get:

# netplan set network.ethernets=null
Traceback (most recent call last):
  File "/usr/sbin/netplan", line 23, in <module>
    netplan.main()
  File "/usr/share/netplan/netplan/cli/core.py", line 50, in main
    self.run_command()
  File "/usr/share/netplan/netplan/cli/utils.py", line 264, in run_command
    self.func()
  File "/usr/share/netplan/netplan/cli/commands/set.py", line 52, in run
    self.run_command()
  File "/usr/share/netplan/netplan/cli/utils.py", line 264, in run_command
    self.func()
  File "/usr/share/netplan/netplan/cli/commands/set.py", line 95, in command_set
    hints = self.split_tree_by_hint(set_tree)
  File "/usr/share/netplan/netplan/cli/commands/set.py", line 61, in split_tree_by_hint
    for netdef in network.get(devtype, []):
TypeError: 'NoneType' object is not iterable

Do you have any hints how I can delete an entire subtree via the commandline (well, via dbus but it's a direct mapping so cmdline is fine).

This is Ubuntu Core 20 with netplan 0.102-0ubuntu1~20.04.2 but I can also reproduce this on my 21.04 system with:
$ sudo netplan get
network:
  bridges:
    br54:
      dhcp4: true
      dhcp6: true
  renderer: NetworkManager
  version: 2
$ sudo netplan set network.bridges=null
Traceback (most recent call last):
  File "/usr/sbin/netplan", line 23, in <module>
    netplan.main()
  File "/usr/share/netplan/netplan/cli/core.py", line 50, in main
    self.run_command()
  File "/usr/share/netplan/netplan/cli/utils.py", line 264, in run_command
    self.func()
  File "/usr/share/netplan/netplan/cli/commands/set.py", line 52, in run
    self.run_command()
  File "/usr/share/netplan/netplan/cli/utils.py", line 264, in run_command
    self.func()
  File "/usr/share/netplan/netplan/cli/commands/set.py", line 95, in command_set
    hints = self.split_tree_by_hint(set_tree)
  File "/usr/share/netplan/netplan/cli/commands/set.py", line 61, in split_tree_by_hint
    for netdef in network.get(devtype, []):
TypeError: 'NoneType' object is not iterable

Related branches

Lukas Märdian (slyon)
Changed in netplan:
status: New → Triaged
Revision history for this message
Lukas Märdian (slyon) wrote :

Yes. Looks like you discovered a bug here.
It works if you unset an "interface subtree", e.g.:
$ sudo netplan set network.bridges.br54=null

But apparently not if you delete a whole set of devices, that might be spread across a set of YAML files (like network.bridges=null, network=null).

I can reproduce the issue with a simple unit-test in `test_clie_get_set.py` and we should fix it.

    def test_set_delete_subtree(self):
        with open(self.path, 'w') as f:
            f.write('''network:\n version: 2\n renderer: NetworkManager
  ethernets:
    eth0: {addresses: [1.2.3.4/24]}''')
        ret = self._set(['network.ethernets=null'])
        print(ret, flush=True) # debugging
        self.assertTrue(os.path.isfile(self.path))
        with open(self.path, 'r') as f:
            out = f.read()
            print(out, flush=True) # debugging
            self.assertIn('network:\n', out)
            self.assertIn(' version: 2\n', out)
            self.assertIn(' renderer: NetworkManager\n', out)
            self.assertNotIn('ethernets:', out)

tags: added: fr-1685
Simon Chopin (schopin)
Changed in netplan:
assignee: nobody → Simon Chopin (schopin)
Revision history for this message
Simon Chopin (schopin) wrote :
Changed in netplan:
status: Triaged → In Progress
Simon Chopin (schopin)
description: updated
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package netplan.io - 0.103-0ubuntu7

---------------
netplan.io (0.103-0ubuntu7) impish; urgency=medium

  * Add d/p/0006-netplan-set-make-it-possible-to-unset-a-whole-devtyp.patch:
    Fix unset of a devtype subtree, e.g. "netplan set network.ethernets=null".
    (LP: #1942930)

 -- Lukas Märdian <email address hidden> Tue, 05 Oct 2021 09:41:31 +0200

Changed in netplan.io (Ubuntu Impish):
status: New → Fix Released
Revision history for this message
Brian Murray (brian-murray) wrote : Please test proposed package

Hello Michael, or anyone else affected,

Accepted netplan.io into hirsute-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/netplan.io/0.103-0ubuntu5~21.04.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-hirsute to verification-done-hirsute. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-hirsute. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Changed in netplan.io (Ubuntu Hirsute):
status: New → Fix Committed
tags: added: verification-needed verification-needed-hirsute
Changed in netplan.io (Ubuntu Focal):
status: New → Fix Committed
tags: added: verification-needed-focal
Revision history for this message
Brian Murray (brian-murray) wrote :

Hello Michael, or anyone else affected,

Accepted netplan.io into focal-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/netplan.io/0.103-0ubuntu5~20.04.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-focal to verification-done-focal. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-focal. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Revision history for this message
Ubuntu SRU Bot (ubuntu-sru-bot) wrote : Autopkgtest regression report (netplan.io/0.103-0ubuntu5~21.04.2)

All autopkgtests for the newly accepted netplan.io (0.103-0ubuntu5~21.04.2) for hirsute have finished running.
The following regressions have been reported in tests triggered by the package:

netplan.io/0.103-0ubuntu5~21.04.2 (arm64)

Please visit the excuses page listed below and investigate the failures, proceeding afterwards as per the StableReleaseUpdates policy regarding autopkgtest regressions [1].

https://people.canonical.com/~ubuntu-archive/proposed-migration/hirsute/update_excuses.html#netplan.io

[1] https://wiki.ubuntu.com/StableReleaseUpdates#Autopkgtest_Regressions

Thank you!

Revision history for this message
Michael Vogt (mvo) wrote :

Hm, maybe I'm holding it wrong (again) but I'm not sure this is fully fixed:
$ apt list netplan.io
netplan.io/now 0.103-0ubuntu5~21.04.2 amd64 [installed,local]
$ $ zcat /usr/share/doc/netplan.io/changelog.Debian.gz |head -n 6
netplan.io (0.103-0ubuntu5~21.04.2) hirsute; urgency=medium

  * Backport patches from impish:
    + Add d/p/0006-netplan-set-make-it-possible-to-unset-a-whole-devtyp.patch:
      Fix unset of a devtype subtree, e.g. "netplan set network.ethernets=null"
      (LP: #1942930)
$ netplan get
network:
  renderer: NetworkManager
  version: 2
 netplan set network=null
Traceback (most recent call last):
  File "/usr/sbin/netplan", line 23, in <module>
    netplan.main()
  File "/usr/share/netplan/netplan/cli/core.py", line 50, in main
    self.run_command()
  File "/usr/share/netplan/netplan/cli/utils.py", line 310, in run_command
    self.func()
  File "/usr/share/netplan/netplan/cli/commands/set.py", line 53, in run
    self.run_command()
  File "/usr/share/netplan/netplan/cli/utils.py", line 310, in run_command
    self.func()
  File "/usr/share/netplan/netplan/cli/commands/set.py", line 103, in command_set
    hints = self.split_tree_by_hint(set_tree)
  File "/usr/share/netplan/netplan/cli/commands/set.py", line 59, in split_tree_by_hint
    for devtype in network:
TypeError: 'NoneType' object is not iterable

But maybe that is a different bug that needs to be reported differently. Anyway, I can probably work around this one.

Revision history for this message
Simon Chopin (schopin) wrote : Re: [Bug 1942930] Re: Unset a subtree crashes

I don't know if you're holding it wrong per se, but at the very least
you've moved the goalpost a little bit. My patchset addresses the case

netplan set network.ethernets=null

but NOT the case

netplan set network=null

I suppose we could special-case the latter one by simply nuking all
config files... ¯\_(ツ)_/¯

Revision history for this message
Lukas Märdian (slyon) wrote :

Indeed, a `netplan set network=null` would basically be the same as `rm /etc/netplan/*.yaml`.

Revision history for this message
Lukas Märdian (slyon) wrote :

I Tested netplan.io 0.103-0ubuntu5~21.04.2 inside a fresh Hirsute LXD container:
$ lxc launch ubuntu-daily:hirsute hh
$ lxc exec hh bash

=> Enabled hirsute-proposed & installed/upgraded netplan.io

root@hh:~# dpkg -l | grep netplan
ii libnetplan0:amd64 0.103-0ubuntu5~21.04.2
ii netplan.io 0.103-0ubuntu5~21.04.2
root@hh:~# cat /etc/netplan/00-ethernets.yaml
network:
  version: 2
  ethernets:
    all-en:
      dhcp4: true
      match:
        name: en*
    all-eth:
      dhcp4: true
      match:
        name: eth*
root@hh:~# netplan get
network:
  ethernets:
    all-en:
      dhcp4: true
      match:
        name: en*
    all-eth:
      dhcp4: true
      match:
        name: eth*
  version: 2
root@hh:~# netplan set network.ethernets=null
root@hh:~# netplan get
{}

In accordance with https://wiki.ubuntu.com/NetplanUpdates I'm also attaching the full autopkgtest logs for those builds:
https://git.launchpad.net/~slyon/+git/files/tree/LP1943120/hirsute_amd64.log
https://git.launchpad.net/~slyon/+git/files/tree/LP1943120/hirsute_arm64.log
https://git.launchpad.net/~slyon/+git/files/tree/LP1943120/hirsute_armhf.log
https://git.launchpad.net/~slyon/+git/files/tree/LP1943120/hirsute_ppc64el.log
https://git.launchpad.net/~slyon/+git/files/tree/LP1943120/hirsute_s390x.log

The mentioned autopkgtest regression is gone after a retry.

tags: added: verification-done-hirsute
removed: verification-needed-hirsute
Revision history for this message
Lukas Märdian (slyon) wrote (last edit ):

I Tested netplan.io 0.103-0ubuntu5~20.04.2 inside a fresh Focal LXD container:
$ lxc launch ubuntu-daily:focal ff
$ lxc exec ff bash

=> Enabled focal-proposed & installed/upgraded netplan.io

root@ff:~# dpkg -l | grep netplan
ii libnetplan0:amd64 0.103-0ubuntu5~20.04.2
ii netplan.io 0.103-0ubuntu5~20.04.2
root@ff:~# rm /etc/netplan/*
root@ff:~# cat /etc/netplan/00-ethernets.yaml
network:
  version: 2
  ethernets:
    all-en:
      dhcp4: true
      match:
        name: en*
    all-eth:
      dhcp4: true
      match:
        name: eth*
root@ff:~# netplan get
network:
  ethernets:
    all-en:
      dhcp4: true
      match:
        name: en*
    all-eth:
      dhcp4: true
      match:
        name: eth*
  version: 2
root@ff:~# netplan set network.ethernets=null
root@ff:~# netplan get
{}

In accordance with https://wiki.ubuntu.com/NetplanUpdates I'm also attaching the full autopkgtest logs for those builds:
https://git.launchpad.net/~slyon/+git/files/tree/LP1943120/focal_amd64.log
https://git.launchpad.net/~slyon/+git/files/tree/LP1943120/focal_arm64.log
https://git.launchpad.net/~slyon/+git/files/tree/LP1943120/focal_armhf.log
https://git.launchpad.net/~slyon/+git/files/tree/LP1943120/focal_ppc64el.log
https://git.launchpad.net/~slyon/+git/files/tree/LP1943120/focal_s390x.log

tags: added: verification-done-focal
removed: verification-needed-focal
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package netplan.io - 0.103-0ubuntu5~21.04.2

---------------
netplan.io (0.103-0ubuntu5~21.04.2) hirsute; urgency=medium

  * Backport patches from impish:
    + Add d/p/0006-netplan-set-make-it-possible-to-unset-a-whole-devtyp.patch:
      Fix unset of a devtype subtree, e.g. "netplan set network.ethernets=null"
      (LP: #1942930)
    + Add d/p/0005-Implement-YAML-state-tracking-and-use-it-in-the-DBus.patch:
      Allow to pass a state to netplan apply/try so it can cleanup unused
      virtual network interfaces after itself. Make use of this functionality
      inside the DBus Config.Try()/Apply() API and the 'netplan try' CLI.
      (LP: #1943120)

 -- Simon Chopin <email address hidden> Wed, 06 Oct 2021 12:57:35 +0200

Changed in netplan.io (Ubuntu Hirsute):
status: Fix Committed → Fix Released
Revision history for this message
Brian Murray (brian-murray) wrote : Update Released

The verification of the Stable Release Update for netplan.io has completed successfully and the package is now being released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package netplan.io - 0.103-0ubuntu5~20.04.2

---------------
netplan.io (0.103-0ubuntu5~20.04.2) focal; urgency=medium

  * Backport patches from impish:
    + Add d/p/0006-netplan-set-make-it-possible-to-unset-a-whole-devtyp.patch:
      Fix unset of a devtype subtree, e.g. "netplan set network.ethernets=null"
      (LP: #1942930)
    + Add d/p/0005-Implement-YAML-state-tracking-and-use-it-in-the-DBus.patch:
      Allow to pass a state to netplan apply/try so it can cleanup unused
      virtual network interfaces after itself. Make use of this functionality
      inside the DBus Config.Try()/Apply() API and the 'netplan try' CLI.
      (LP: #1943120)

 -- Simon Chopin <email address hidden> Wed, 06 Oct 2021 12:57:35 +0200

Changed in netplan.io (Ubuntu Focal):
status: Fix Committed → Fix Released
Lukas Märdian (slyon)
Changed in netplan:
status: In Progress → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers