NetworkManager

Merge ~danilogondolfo/network-manager/+git/network-manager:lp2040153 into network-manager:ubuntu-mantic

  1. Git
  2. lp:~danilogondolfo/network-manager/+git/network-manager
  3. lp2040153
  4. Merge into ubuntu-mantic
Proposed by Danilo Egea Gondolfo
Status: Merged
Merged at revision: ad4c5c166b19d084ddd5738db3bb30eb8e4e0a19
Proposed branch: ~danilogondolfo/network-manager/+git/network-manager:lp2040153
Merge into: network-manager:ubuntu-mantic
Diff against target: 154 lines (+69/-13)
5 files modified
debian/changelog (+14/-0)
debian/patches/netplan/0003-Allow-the-NetworkManager-daemon-to-write-to-lib-netp.patch (+33/-0)
debian/patches/series (+1/-0)
debian/tests/control (+1/-1)
debian/tests/nm_netplan.py (+20/-12)
Reviewer Review Type Date Requested Status
Sebastien Bacher Approve
Lukas Märdian Approve
Network-manager Pending
Review via email: mp+454296@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Danilo Egea Gondolfo (danilogondolfo) wrote (last edit ):

Changes:

nm_netplan.py: start NetworkManager with systemd instead of calling /usr/sbin/NetworkManager directly. This ensures that NetworkManager will behave like it would in a non-testing environment. Also, add a few more connection deletes to some tests.

new patch d/p/netplan/0003-Allow-the-NetworkManager-daemon-to-write-to-lib-netp.patch: Because of ProtectSystem=true, systemd is mounting /usr as read-only for Network Manager. When the file /usr/lib/netplan/00-network-manager-all.yaml is present, libnetplan will try to open it for writing. This is leading to a failure when libnetplan tries to delete a connection. This change adds ReadWritePaths=/usr/lib/netplan so this path will not be read-only.

debian/tests/control: add all the dependencies required to run the test script plus ubuntu-settings to install /usr/lib/netplan/00-network-manager-all.yaml. This test script was being skipped for some time now due to missing dependencies.

How to test and verify it works:

Launch a new Mantic VM:

lxc launch ubuntu:mantic --vm

Add the PPA with the fix:

sudo add-apt-repository ppa:danilogondolfo/network-manager
sudo apt update

Install network-manager and ubuntu-settings:

sudo apt install network-manager ubuntu-settings

Run Netplan

netplan apply

Create a dummy connection:

nmcli con add type dummy connection.interface-name dummy0

Check a new YAML will be created in /etc/netplan

Delete the connection

nmcli con del dummy-dummy0

Check the YAML was removed from /etc/netplan

Run the same test in a new VM WITHOUT the PPA and check the YAML will not be delete when you delete the connection.
You will see the error below in the NetworkManager's journal

netplan_delete_connection: Cannot write output state: Read-only file system

Autopkgtests

amd64 - https://autopkgtest.ubuntu.com/results/autopkgtest-mantic-danilogondolfo-network-manager/mantic/amd64/n/network-manager/20231023_175203_b2798@/log.gz

ppc64 - https://autopkgtest.ubuntu.com/results/autopkgtest-mantic-danilogondolfo-network-manager/mantic/ppc64el/n/network-manager/20231023_182332_f0497@/log.gz

s390x - https://autopkgtest.ubuntu.com/results/autopkgtest-mantic-danilogondolfo-network-manager/mantic/s390x/n/network-manager/20231023_190810_ced8d@/log.gz

arm64 - https://autopkgtest.ubuntu.com/results/autopkgtest-mantic-danilogondolfo-network-manager/mantic/arm64/n/network-manager/20231024_084542_ac017@/log.gz

armhf - https://autopkgtest.ubuntu.com/results/autopkgtest-mantic-danilogondolfo-network-manager/mantic/armhf/n/network-manager/20231024_083545_ac017@/log.gz

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

LGTM, thanks!

It's great that you provided some real autopkgtest results, showing success!

Two small comments/nitpicks:
- please fill the SRU template on bug #2040153, you can re-use the test-case/reproducer from your comment to this MP.

- (non-blocking) Consider adding a "Forwarded: not-needed" DEP-3 header to the new patch, as this is Netplan/Ubuntu specific.

If Seb is fine with this, I can cherry-pick the same changes into the "ubuntu/master" branch and upload it into "noble", once the archive opens (and also sponsor your SRU).

review: Approve
Revision history for this message
Danilo Egea Gondolfo (danilogondolfo) wrote :

Thank you, Lukas. I just added the "Forwarded: not-needed".

Revision history for this message
Sebastien Bacher (seb128) wrote :

thanks, makes sense to me!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index d2a98ab..4b7d37f 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,17 @@
6+network-manager (1.44.2-1ubuntu1.2) mantic; urgency=medium
7+
8+ * debian/tests/nm_netplan.py:
9+ Start Network Manager via systemd. The .service unit file sets
10+ ProtectSystem to true and we want to run the Netplan tests with this
11+ restriction enabled.
12+ * d/p/netplan/0003-Allow-the-NetworkManager-daemon-to-write-to-lib-netp.patch:
13+ Allow-list /usr/lib/netplan so libnetplan can open files from that
14+ directory with writing permission. See LP: #2040153
15+ * debian/tests/control.
16+ Add all the dependencies required by the nm_netplan.py tests.
17+
18+ -- Danilo Egea Gondolfo <danilo.egea.gondolfo@canonical.com> Mon, 23 Oct 2023 16:29:46 +0100
19+
20 network-manager (1.44.2-1ubuntu1.1) mantic; urgency=medium
21
22 * network-manager.postinst: Skip unknown connection profiles (LP: #2039503)
23diff --git a/debian/patches/netplan/0003-Allow-the-NetworkManager-daemon-to-write-to-lib-netp.patch b/debian/patches/netplan/0003-Allow-the-NetworkManager-daemon-to-write-to-lib-netp.patch
24new file mode 100644
25index 0000000..f2600e8
26--- /dev/null
27+++ b/debian/patches/netplan/0003-Allow-the-NetworkManager-daemon-to-write-to-lib-netp.patch
28@@ -0,0 +1,33 @@
29+From: Danilo Egea Gondolfo <danilo.egea.gondolfo@canonical.com>
30+Date: Mon, 23 Oct 2023 15:05:02 +0100
31+Subject: Allow the NetworkManager daemon to write to /lib/netplan
32+
33+The systemd service file is setting ProtectSystem to true,
34+which is giving Network Manager read only access to /usr. libnetplan
35+currently requires rights to open files from /usr/lib/netplan with permission
36+to write to them.
37+
38+Allow-list /usr/lib/netplan so libnetplan will not fail when called from the
39+Network Manager's daemon.
40+
41+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/netplan.io/+bug/2040153
42+Forwarded: not-needed
43+---
44+ data/NetworkManager.service.in | 1 +
45+ 1 file changed, 1 insertion(+)
46+
47+diff --git a/data/NetworkManager.service.in b/data/NetworkManager.service.in
48+index f09ae86ce..bc9022a6e 100644
49+--- a/data/NetworkManager.service.in
50++++ b/data/NetworkManager.service.in
51+@@ -24,6 +24,7 @@ CapabilityBoundingSet=CAP_NET_ADMIN CAP_DAC_OVERRIDE CAP_NET_RAW CAP_NET_BIND_SE
52+
53+ ProtectSystem=true
54+ ProtectHome=read-only
55++ReadWritePaths=/usr/lib/netplan
56+
57+ # We require file descriptors for DHCP etc. When activating many interfaces,
58+ # the default limit of 1024 is easily reached.
59+--
60+2.40.1
61+
62diff --git a/debian/patches/series b/debian/patches/series
63index 2e570aa..3682f47 100644
64--- a/debian/patches/series
65+++ b/debian/patches/series
66@@ -5,3 +5,4 @@ Provide-access-to-some-of-NM-s-interfaces-to-whoopsie.patch
67 Update-dnsmasq-parameters.patch
68 netplan/0001-netplan-Adopt-buildsystems-for-Netplan-integration.patch
69 netplan/0002-netplan-make-use-of-libnetplan-for-YAML-backend.patch
70+netplan/0003-Allow-the-NetworkManager-daemon-to-write-to-lib-netp.patch
71diff --git a/debian/tests/control b/debian/tests/control
72index e22c775..75b987e 100644
73--- a/debian/tests/control
74+++ b/debian/tests/control
75@@ -15,5 +15,5 @@ Depends: network-manager, build-essential, linux-headers-generic [!i386], rfkill
76 Restrictions: needs-root allow-stderr isolation-machine skippable
77
78 Tests: nm_netplan.py
79-Depends: python3, gir1.2-nm-1.0, network-manager, netplan.io, openvpn, easy-rsa, network-manager-openvpn
80+Depends: python3, gir1.2-nm-1.0, network-manager, netplan.io, openvpn, easy-rsa, network-manager-openvpn, dnsmasq-base, hostapd, wpasupplicant, isc-dhcp-client, ubuntu-settings
81 Restrictions: needs-root allow-stderr isolation-container
82diff --git a/debian/tests/nm_netplan.py b/debian/tests/nm_netplan.py
83index 84a19ec..19448df 100644
84--- a/debian/tests/nm_netplan.py
85+++ b/debian/tests/nm_netplan.py
86@@ -72,20 +72,15 @@ class TestNetplan(unittest.TestCase):
87 % (extra_main, denylist)
88 )
89
90- log = "/tmp/NetworkManager.log"
91- f_log = os.open(log, os.O_CREAT | os.O_WRONLY | os.O_SYNC)
92-
93- # build NM command line
94- argv = ["NetworkManager", "--log-level=debug", "--debug", "--config=" + conf]
95- # allow specifying extra arguments
96- argv += os.environ.get("NM_TEST_DAEMON_ARGS", "").strip().split()
97-
98- p = subprocess.Popen(argv, stdout=f_log, stderr=subprocess.STDOUT)
99+ # We start NM via systemd because its .service file sets some restrictions
100+ # at run time, such as ProtectSystem and ProtectHome, and we want to test it
101+ # with these restrictions in place.
102+ # Sleep for 5 seconds to prevent hitting "start-limit-hit" in systemd.
103+ sleep(5)
104+ self._start_network_manager()
105 network_test_base.wait_nm_online()
106 # automatically terminate process at end of test case
107- self.addCleanup(p.wait)
108- self.addCleanup(p.terminate)
109- self.addCleanup(os.close, f_log)
110+ self.addCleanup(self._stop_network_manager)
111 self.addCleanup(self._clear_connections)
112
113 self._process_glib_events()
114@@ -97,6 +92,10 @@ class TestNetplan(unittest.TestCase):
115 while context.iteration(False):
116 pass
117
118+ def _start_network_manager(self):
119+ cmd = ['systemctl', 'start', 'NetworkManager']
120+ subprocess.call(cmd, stdout=subprocess.DEVNULL)
121+
122 def _restart_network_manager(self):
123 cmd = ['systemctl', 'restart', 'NetworkManager']
124 subprocess.call(cmd, stdout=subprocess.DEVNULL)
125@@ -765,6 +764,9 @@ EASYRSA_BATCH=1 /usr/share/easy-rsa/easyrsa build-client-full client nopass
126 capture_output=True, text=True)
127
128 self.assertEqual(out.returncode, 0, f'"netplan get" failed due to issues in the resulting YAML files.')
129+ self.assertEqual(self._get_number_of_yaml_files(), 1)
130+ self._nmcli(['con', 'del', 'netplan-wireguard-wg0'])
131+ self.assertEqual(self._get_number_of_yaml_files(), 0)
132
133 def test_create_wireguard_tunnel_in_multiple_steps_nmcli(self):
134 """
135@@ -836,6 +838,9 @@ PersistentKeepalive = 15'''
136 '802-1x.ca-cert', '/etc/netplan/a.crt'])
137
138 self.assertEqual(ret, 0, 'nmcli failed to add connection')
139+ self.assertEqual(self._get_number_of_yaml_files(), 1)
140+ self._nmcli(['con', 'del', 'eduroam'])
141+ self.assertEqual(self._get_number_of_yaml_files(), 0)
142
143 def test_create_gre_connection(self):
144 """
145@@ -848,6 +853,9 @@ PersistentKeepalive = 15'''
146 '10.20.20.2', 'local', '10.20.20.1'])
147
148 self.assertEqual(ret, 0, 'nmcli failed to add connection')
149+ self.assertEqual(self._get_number_of_yaml_files(), 1)
150+ self._nmcli(['con', 'del', '"IP tunnel connection 1"'])
151+ self.assertEqual(self._get_number_of_yaml_files(), 0)
152
153 if __name__ == '__main__':
154 runner = unittest.TextTestRunner(stream=sys.stdout, verbosity=2)

Subscribers

People subscribed via source and target branches