Merge ~ack/maas:1929552-3.0 into maas:3.0

Proposed by Alberto Donato
Status: Merged
Approved by: Alberto Donato
Approved revision: 75f0e0ebe0a485b8f79806986f484722cc9cee48
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~ack/maas:1929552-3.0
Merge into: maas:3.0
Diff against target: 266 lines (+114/-26)
6 files modified
debian/extras/99-maas-sudoers (+10/-0)
src/provisioningserver/refresh/__init__.py (+9/-7)
src/provisioningserver/refresh/maas-lshw (+2/-7)
src/provisioningserver/refresh/tests/test_refresh.py (+75/-4)
src/provisioningserver/utils/ipaddr.py (+4/-1)
src/provisioningserver/utils/tests/test_ipaddr.py (+14/-7)
Reviewer Review Type Date Requested Status
Alberto Donato Approve
Review via email: mp+403309@code.launchpad.net

Commit message

LP: #1929552 - use sudo for running scripts and resources binary on controllers

To post a comment you must log in.
Revision history for this message
Alberto Donato (ack) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/extras/99-maas-sudoers b/debian/extras/99-maas-sudoers
2index 56c4e94..9a53225 100644
3--- a/debian/extras/99-maas-sudoers
4+++ b/debian/extras/99-maas-sudoers
5@@ -7,3 +7,13 @@ maas ALL= NOPASSWD: /bin/systemctl start maas-dhcpd6
6 maas ALL= NOPASSWD: /bin/systemctl restart maas-dhcpd6
7 maas ALL= NOPASSWD: /bin/systemctl stop maas-dhcpd6
8 maas ALL= NOPASSWD: /bin/systemctl restart maas-rackd
9+maas ALL= NOPASSWD:SETENV: /usr/share/maas/machine-resources/amd64
10+maas ALL= NOPASSWD:SETENV: /usr/share/maas/machine-resources/arm64
11+maas ALL= NOPASSWD:SETENV: /usr/share/maas/machine-resources/armhf
12+maas ALL= NOPASSWD:SETENV: /usr/share/maas/machine-resources/i386
13+maas ALL= NOPASSWD:SETENV: /usr/share/maas/machine-resources/ppc64el
14+maas ALL= NOPASSWD:SETENV: /usr/share/maas/machine-resources/s390x
15+maas ALL= NOPASSWD:SETENV: /usr/lib/python3/dist-packages/provisioningserver/refresh/maas-list-modaliases
16+maas ALL= NOPASSWD:SETENV: /usr/lib/python3/dist-packages/provisioningserver/refresh/maas-lshw
17+maas ALL= NOPASSWD:SETENV: /usr/lib/python3/dist-packages/provisioningserver/refresh/maas-serial-ports
18+maas ALL= NOPASSWD:SETENV: /usr/lib/python3/dist-packages/provisioningserver/refresh/maas-support-info
19diff --git a/src/provisioningserver/refresh/__init__.py b/src/provisioningserver/refresh/__init__.py
20index ea984b8..7e7a240 100644
21--- a/src/provisioningserver/refresh/__init__.py
22+++ b/src/provisioningserver/refresh/__init__.py
23@@ -21,7 +21,7 @@ from provisioningserver.refresh.node_info_scripts import (
24 LXD_OUTPUT_NAME,
25 NODE_INFO_SCRIPTS,
26 )
27-from provisioningserver.utils.snap import SnapPaths
28+from provisioningserver.utils.snap import running_in_snap, SnapPaths
29 from provisioningserver.utils.twisted import synchronous
30
31 maaslog = get_maas_logger("refresh")
32@@ -97,15 +97,15 @@ def refresh(
33 post_process_hook=post_process_hook,
34 )
35
36- if len(failed_scripts) == 0:
37- signal_wrapper(url, creds, "OK", "Finished refreshing %s" % system_id)
38+ if failed_scripts:
39+ signal_wrapper(url, creds, "FAILED", f"Failed refreshing {system_id}")
40 else:
41- signal_wrapper(
42- url, creds, "FAILED", "Failed refreshing %s" % system_id
43- )
44+ signal_wrapper(url, creds, "OK", f"Finished refreshing {system_id}")
45
46
47 def runscripts(scripts, url, creds, tmpdir, post_process_hook=None):
48+ in_snap = running_in_snap()
49+
50 total_scripts = len(scripts)
51 current_script = 1
52 failed_scripts = []
53@@ -143,9 +143,11 @@ def runscripts(scripts, url, creds, tmpdir, post_process_hook=None):
54
55 timeout = 60
56
57+ command = [script_path] if in_snap else ["sudo", "-E", script_path]
58+
59 try:
60 proc = Popen(
61- script_path, stdin=DEVNULL, stdout=PIPE, stderr=PIPE, env=env
62+ command, stdin=DEVNULL, stdout=PIPE, stderr=PIPE, env=env
63 )
64 capture_script_output(
65 proc, combined_path, stdout_path, stderr_path, timeout
66diff --git a/src/provisioningserver/refresh/maas-lshw b/src/provisioningserver/refresh/maas-lshw
67index ce84388..3779061 100755
68--- a/src/provisioningserver/refresh/maas-lshw
69+++ b/src/provisioningserver/refresh/maas-lshw
70@@ -26,10 +26,5 @@
71 # timeout: 00:05:00
72 # --- End MAAS 1.0 script metadata ---
73
74-# The script is run both on controllers (which can be run inside a snap) and
75-# commissioned machines.
76-if [ -z "$SNAP" ]; then
77- sudo -n /usr/bin/lshw -xml
78-else
79- "$SNAP/usr/bin/lshw" -xml
80-fi
81+# the SNAP var is empty if not running in a snap
82+"$SNAP/usr/bin/lshw" -xml
83diff --git a/src/provisioningserver/refresh/tests/test_refresh.py b/src/provisioningserver/refresh/tests/test_refresh.py
84index 8dcec18..7f841f0 100644
85--- a/src/provisioningserver/refresh/tests/test_refresh.py
86+++ b/src/provisioningserver/refresh/tests/test_refresh.py
87@@ -1,18 +1,16 @@
88 # Copyright 2016-2020 Canonical Ltd. This software is licensed under the
89 # GNU Affero General Public License version 3 (see the file LICENSE).
90
91-"""Test refresh functions."""
92-
93-
94 from collections import OrderedDict
95 import os
96 from pathlib import Path
97 import random
98 import stat
99+import subprocess
100 import sys
101 import tempfile
102 from textwrap import dedent
103-from unittest.mock import patch, sentinel
104+from unittest.mock import ANY, patch, sentinel
105
106 from testtools.matchers import Contains, DirExists, Not
107
108@@ -55,6 +53,9 @@ class TestRefresh(MAASTestCase):
109 # When running scripts in a tty MAAS outputs the results to help with
110 # debug. Quiet the output when running in tests.
111 self.patch(sys, "stdout")
112+ # by default, fake running in snap so sudo is not used
113+ self.mock_running_in_snap = self.patch(refresh, "running_in_snap")
114+ self.mock_running_in_snap.return_value = True
115
116 def _cleanup(self, path):
117 if os.path.exists(path):
118@@ -581,3 +582,73 @@ class TestRefresh(MAASTestCase):
119 self.assertThat(
120 maaslog, MockAnyCall("Error during controller refresh: %s" % error)
121 )
122+
123+ def test_refresh_runs_script_no_sudo_snap(self):
124+ mock_popen = self.patch(refresh, "Popen")
125+ mock_popen.side_effect = OSError()
126+ self.patch(refresh, "signal")
127+ script_name = factory.make_name("script_name")
128+ script_content = dedent(
129+ """\
130+ #!/bin/bash
131+ echo 'test script'
132+ """
133+ )
134+ info_scripts = self.create_scripts_success(
135+ script_name, script_content=script_content
136+ )
137+
138+ system_id = factory.make_name("system_id")
139+ consumer_key = factory.make_name("consumer_key")
140+ token_key = factory.make_name("token_key")
141+ token_secret = factory.make_name("token_secret")
142+ url = factory.make_url()
143+
144+ with patch.dict(refresh.NODE_INFO_SCRIPTS, info_scripts, clear=True):
145+ refresh.refresh(
146+ system_id, consumer_key, token_key, token_secret, url
147+ )
148+ script_path = (Path(refresh.__file__) / ".." / script_name).resolve()
149+ mock_popen.assert_called_once_with(
150+ [str(script_path)],
151+ stdin=subprocess.DEVNULL,
152+ stdout=subprocess.PIPE,
153+ stderr=subprocess.PIPE,
154+ env=ANY,
155+ )
156+
157+ def test_refresh_runs_script_sudo_if_no_snap(self):
158+ self.mock_running_in_snap.return_value = False
159+ mock_popen = self.patch(refresh, "Popen")
160+ mock_popen.side_effect = OSError()
161+ self.patch(refresh, "signal")
162+ script_name = factory.make_name("script_name")
163+ script_content = dedent(
164+ """\
165+ #!/bin/bash
166+ echo 'test script'
167+ echo '{status: skipped}' > $RESULT_PATH
168+ """
169+ )
170+ info_scripts = self.create_scripts_success(
171+ script_name, script_content=script_content
172+ )
173+
174+ system_id = factory.make_name("system_id")
175+ consumer_key = factory.make_name("consumer_key")
176+ token_key = factory.make_name("token_key")
177+ token_secret = factory.make_name("token_secret")
178+ url = factory.make_url()
179+
180+ with patch.dict(refresh.NODE_INFO_SCRIPTS, info_scripts, clear=True):
181+ refresh.refresh(
182+ system_id, consumer_key, token_key, token_secret, url
183+ )
184+ script_path = (Path(refresh.__file__) / ".." / script_name).resolve()
185+ mock_popen.assert_called_once_with(
186+ ["sudo", "-E", str(script_path)],
187+ stdin=subprocess.DEVNULL,
188+ stdout=subprocess.PIPE,
189+ stderr=subprocess.PIPE,
190+ env=ANY,
191+ )
192diff --git a/src/provisioningserver/utils/ipaddr.py b/src/provisioningserver/utils/ipaddr.py
193index 9587029..a7d7c02 100644
194--- a/src/provisioningserver/utils/ipaddr.py
195+++ b/src/provisioningserver/utils/ipaddr.py
196@@ -13,6 +13,7 @@ import netifaces
197 from provisioningserver.refresh import get_resources_bin_path
198 from provisioningserver.utils.lxd import parse_lxd_networks
199 from provisioningserver.utils.shell import call_and_check
200+from provisioningserver.utils.snap import running_in_snap
201
202
203 def get_vid_from_ifname(ifname):
204@@ -40,7 +41,9 @@ def get_ip_addr():
205 :raises:ExternalProcessError: if IP address information could not be
206 gathered.
207 """
208- output = call_and_check([get_resources_bin_path()])
209+ cmd_path = get_resources_bin_path()
210+ command = [cmd_path] if running_in_snap() else ["sudo", cmd_path]
211+ output = call_and_check(command)
212 ifaces = parse_lxd_networks(json.loads(output)["networks"])
213 _update_interface_type(ifaces)
214 _annotate_with_proc_net_bonding_original_macs(ifaces)
215diff --git a/src/provisioningserver/utils/tests/test_ipaddr.py b/src/provisioningserver/utils/tests/test_ipaddr.py
216index 5907c4a..02663c9 100644
217--- a/src/provisioningserver/utils/tests/test_ipaddr.py
218+++ b/src/provisioningserver/utils/tests/test_ipaddr.py
219@@ -1,9 +1,6 @@
220 # Copyright 2015-2016 Canonical Ltd. This software is licensed under the
221 # GNU Affero General Public License version 3 (see the file LICENSE).
222
223-"""Test parser for 'ip addr show'."""
224-
225-
226 import json
227 import os
228 from pathlib import Path
229@@ -14,7 +11,6 @@ from textwrap import dedent
230 import netifaces
231
232 from maastesting.factory import factory
233-from maastesting.matchers import MockCalledOnceWith
234 from maastesting.testcase import MAASTestCase
235 from provisioningserver.refresh import get_resources_bin_path
236 from provisioningserver.utils import ipaddr as ipaddr_module
237@@ -133,15 +129,26 @@ class TestGetIPAddr(MAASTestCase):
238 """Tests for `get_ip_addr`, `get_mac_addresses`."""
239
240 def test_get_ip_addr_calls_methods(self):
241+ self.patch(ipaddr_module, "running_in_snap").return_value = False
242 patch_call_and_check = self.patch(ipaddr_module, "call_and_check")
243 patch_call_and_check.return_value = json.dumps(
244 {"networks": SAMPLE_LXD_NETWORKS}
245 )
246 # all interfaces from binary output are included in result
247 self.assertCountEqual(SAMPLE_LXD_NETWORKS, get_ip_addr())
248- self.assertThat(
249- patch_call_and_check,
250- MockCalledOnceWith([get_resources_bin_path()]),
251+ patch_call_and_check.assert_called_once_with(
252+ ["sudo", get_resources_bin_path()]
253+ )
254+
255+ def test_no_use_sudo_in_snap(self):
256+ patch_call_and_check = self.patch(ipaddr_module, "call_and_check")
257+ patch_call_and_check.return_value = json.dumps(
258+ {"networks": SAMPLE_LXD_NETWORKS}
259+ )
260+ self.patch(ipaddr_module, "running_in_snap").return_value = True
261+ get_ip_addr()
262+ patch_call_and_check.assert_called_once_with(
263+ [get_resources_bin_path()]
264 )
265
266 def test_get_mac_addresses_returns_all_mac_addresses(self):

Subscribers

People subscribed via source and target branches