Merge ~ack/maas:1929552-controller-scripts-sudo into maas:master

[Alberto Donato]

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

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1929552-controller-scripts-sudo lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/10132/console
COMMIT: ac15da820348f9c86c245bafa132eb4da4661ad7

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1929552-controller-scripts-sudo lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 5f09031252cd62d5835c8ef7742869bb694ff453

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1929552-controller-scripts-sudo lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: d58d778f802b7956d1dd4710009a405aab64054c

review: Approve

[Björn Tillenius (bjornt)]

+1

review: Approve

diff --git a/debian/extras/99-maas-sudoers b/debian/extras/99-maas-sudoers
index 56c4e94..9a53225 100644
--- a/debian/extras/99-maas-sudoers
+++ b/debian/extras/99-maas-sudoers
@@ -7,3 +7,13 @@ maas ALL= NOPASSWD: /bin/systemctl start maas-dhcpd6
 maas ALL= NOPASSWD: /bin/systemctl restart maas-dhcpd6
 maas ALL= NOPASSWD: /bin/systemctl stop maas-dhcpd6
 maas ALL= NOPASSWD: /bin/systemctl restart maas-rackd
+maas ALL= NOPASSWD:SETENV: /usr/share/maas/machine-resources/amd64
+maas ALL= NOPASSWD:SETENV: /usr/share/maas/machine-resources/arm64
+maas ALL= NOPASSWD:SETENV: /usr/share/maas/machine-resources/armhf
+maas ALL= NOPASSWD:SETENV: /usr/share/maas/machine-resources/i386
+maas ALL= NOPASSWD:SETENV: /usr/share/maas/machine-resources/ppc64el
+maas ALL= NOPASSWD:SETENV: /usr/share/maas/machine-resources/s390x
+maas ALL= NOPASSWD:SETENV: /usr/lib/python3/dist-packages/provisioningserver/refresh/maas-list-modaliases
+maas ALL= NOPASSWD:SETENV: /usr/lib/python3/dist-packages/provisioningserver/refresh/maas-lshw
+maas ALL= NOPASSWD:SETENV: /usr/lib/python3/dist-packages/provisioningserver/refresh/maas-serial-ports
+maas ALL= NOPASSWD:SETENV: /usr/lib/python3/dist-packages/provisioningserver/refresh/maas-support-info
diff --git a/src/provisioningserver/refresh/__init__.py b/src/provisioningserver/refresh/__init__.py
index ea984b8..7e7a240 100644
--- a/src/provisioningserver/refresh/__init__.py
+++ b/src/provisioningserver/refresh/__init__.py
@@ -21,7 +21,7 @@ from provisioningserver.refresh.node_info_scripts import (
     LXD_OUTPUT_NAME,
     NODE_INFO_SCRIPTS,
 )
-from provisioningserver.utils.snap import SnapPaths
+from provisioningserver.utils.snap import running_in_snap, SnapPaths
 from provisioningserver.utils.twisted import synchronous
 
 maaslog = get_maas_logger("refresh")
@@ -97,15 +97,15 @@ def refresh(
             post_process_hook=post_process_hook,
         )
 
-    if len(failed_scripts) == 0:
-        signal_wrapper(url, creds, "OK", "Finished refreshing %s" % system_id)
+    if failed_scripts:
+        signal_wrapper(url, creds, "FAILED", f"Failed refreshing {system_id}")
     else:
-        signal_wrapper(
-            url, creds, "FAILED", "Failed refreshing %s" % system_id
-        )
+        signal_wrapper(url, creds, "OK", f"Finished refreshing {system_id}")
 
 
 def runscripts(scripts, url, creds, tmpdir, post_process_hook=None):
+    in_snap = running_in_snap()
+
     total_scripts = len(scripts)
     current_script = 1
     failed_scripts = []
@@ -143,9 +143,11 @@ def runscripts(scripts, url, creds, tmpdir, post_process_hook=None):
 
         timeout = 60
 
+        command = [script_path] if in_snap else ["sudo", "-E", script_path]
+
         try:
             proc = Popen(
-                script_path, stdin=DEVNULL, stdout=PIPE, stderr=PIPE, env=env
+                command, stdin=DEVNULL, stdout=PIPE, stderr=PIPE, env=env
             )
             capture_script_output(
                 proc, combined_path, stdout_path, stderr_path, timeout
diff --git a/src/provisioningserver/refresh/maas-lshw b/src/provisioningserver/refresh/maas-lshw
index ce84388..3779061 100755
--- a/src/provisioningserver/refresh/maas-lshw
+++ b/src/provisioningserver/refresh/maas-lshw
@@ -26,10 +26,5 @@
 # timeout: 00:05:00
 # --- End MAAS 1.0 script metadata ---
 
-# The script is run both on controllers (which can be run inside a snap) and
-# commissioned machines.
-if [ -z "$SNAP" ]; then
-    sudo -n /usr/bin/lshw -xml
-else
-    "$SNAP/usr/bin/lshw" -xml
-fi
+# the SNAP var is empty if not running in a snap
+"$SNAP/usr/bin/lshw" -xml
diff --git a/src/provisioningserver/refresh/tests/test_refresh.py b/src/provisioningserver/refresh/tests/test_refresh.py
index 8dcec18..7f841f0 100644
--- a/src/provisioningserver/refresh/tests/test_refresh.py
+++ b/src/provisioningserver/refresh/tests/test_refresh.py
@@ -1,18 +1,16 @@
 # Copyright 2016-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Test refresh functions."""
-
-
 from collections import OrderedDict
 import os
 from pathlib import Path
 import random
 import stat
+import subprocess
 import sys
 import tempfile
 from textwrap import dedent
-from unittest.mock import patch, sentinel
+from unittest.mock import ANY, patch, sentinel
 
 from testtools.matchers import Contains, DirExists, Not
 
@@ -55,6 +53,9 @@ class TestRefresh(MAASTestCase):
         # When running scripts in a tty MAAS outputs the results to help with
         # debug. Quiet the output when running in tests.
         self.patch(sys, "stdout")
+        # by default, fake running in snap so sudo is not used
+        self.mock_running_in_snap = self.patch(refresh, "running_in_snap")
+        self.mock_running_in_snap.return_value = True
 
     def _cleanup(self, path):
         if os.path.exists(path):
@@ -581,3 +582,73 @@ class TestRefresh(MAASTestCase):
         self.assertThat(
             maaslog, MockAnyCall("Error during controller refresh: %s" % error)
         )
+
+    def test_refresh_runs_script_no_sudo_snap(self):
+        mock_popen = self.patch(refresh, "Popen")
+        mock_popen.side_effect = OSError()
+        self.patch(refresh, "signal")
+        script_name = factory.make_name("script_name")
+        script_content = dedent(
+            """\
+        #!/bin/bash
+        echo 'test script'
+        """
+        )
+        info_scripts = self.create_scripts_success(
+            script_name, script_content=script_content
+        )
+
+        system_id = factory.make_name("system_id")
+        consumer_key = factory.make_name("consumer_key")
+        token_key = factory.make_name("token_key")
+        token_secret = factory.make_name("token_secret")
+        url = factory.make_url()
+
+        with patch.dict(refresh.NODE_INFO_SCRIPTS, info_scripts, clear=True):
+            refresh.refresh(
+                system_id, consumer_key, token_key, token_secret, url
+            )
+        script_path = (Path(refresh.__file__) / ".." / script_name).resolve()
+        mock_popen.assert_called_once_with(
+            [str(script_path)],
+            stdin=subprocess.DEVNULL,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.PIPE,
+            env=ANY,
+        )
+
+    def test_refresh_runs_script_sudo_if_no_snap(self):
+        self.mock_running_in_snap.return_value = False
+        mock_popen = self.patch(refresh, "Popen")
+        mock_popen.side_effect = OSError()
+        self.patch(refresh, "signal")
+        script_name = factory.make_name("script_name")
+        script_content = dedent(
+            """\
+        #!/bin/bash
+        echo 'test script'
+        echo '{status: skipped}' > $RESULT_PATH
+        """
+        )
+        info_scripts = self.create_scripts_success(
+            script_name, script_content=script_content
+        )
+
+        system_id = factory.make_name("system_id")
+        consumer_key = factory.make_name("consumer_key")
+        token_key = factory.make_name("token_key")
+        token_secret = factory.make_name("token_secret")
+        url = factory.make_url()
+
+        with patch.dict(refresh.NODE_INFO_SCRIPTS, info_scripts, clear=True):
+            refresh.refresh(
+                system_id, consumer_key, token_key, token_secret, url
+            )
+        script_path = (Path(refresh.__file__) / ".." / script_name).resolve()
+        mock_popen.assert_called_once_with(
+            ["sudo", "-E", str(script_path)],
+            stdin=subprocess.DEVNULL,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.PIPE,
+            env=ANY,
+        )
diff --git a/src/provisioningserver/utils/ipaddr.py b/src/provisioningserver/utils/ipaddr.py
index f6f4689..a24d644 100644
--- a/src/provisioningserver/utils/ipaddr.py
+++ b/src/provisioningserver/utils/ipaddr.py
@@ -12,6 +12,7 @@ import netifaces
 from provisioningserver.refresh import get_resources_bin_path
 from provisioningserver.utils.lxd import parse_lxd_networks
 from provisioningserver.utils.shell import call_and_check
+from provisioningserver.utils.snap import running_in_snap
 
 
 def get_ip_addr():
@@ -20,7 +21,9 @@ def get_ip_addr():
     :raises:ExternalProcessError: if IP address information could not be
         gathered.
     """
-    output = call_and_check([get_resources_bin_path()])
+    cmd_path = get_resources_bin_path()
+    command = [cmd_path] if running_in_snap() else ["sudo", cmd_path]
+    output = call_and_check(command)
     ifaces = parse_lxd_networks(json.loads(output)["networks"])
     _update_interface_type(ifaces)
     _annotate_with_proc_net_bonding_original_macs(ifaces)
diff --git a/src/provisioningserver/utils/tests/test_ipaddr.py b/src/provisioningserver/utils/tests/test_ipaddr.py
index 5907c4a..02663c9 100644
--- a/src/provisioningserver/utils/tests/test_ipaddr.py
+++ b/src/provisioningserver/utils/tests/test_ipaddr.py
@@ -1,9 +1,6 @@
 # Copyright 2015-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Test parser for 'ip addr show'."""
-
-
 import json
 import os
 from pathlib import Path
@@ -14,7 +11,6 @@ from textwrap import dedent
 import netifaces
 
 from maastesting.factory import factory
-from maastesting.matchers import MockCalledOnceWith
 from maastesting.testcase import MAASTestCase
 from provisioningserver.refresh import get_resources_bin_path
 from provisioningserver.utils import ipaddr as ipaddr_module
@@ -133,15 +129,26 @@ class TestGetIPAddr(MAASTestCase):
     """Tests for `get_ip_addr`, `get_mac_addresses`."""
 
     def test_get_ip_addr_calls_methods(self):
+        self.patch(ipaddr_module, "running_in_snap").return_value = False
         patch_call_and_check = self.patch(ipaddr_module, "call_and_check")
         patch_call_and_check.return_value = json.dumps(
             {"networks": SAMPLE_LXD_NETWORKS}
         )
         # all interfaces from binary output are included in result
         self.assertCountEqual(SAMPLE_LXD_NETWORKS, get_ip_addr())
-        self.assertThat(
-            patch_call_and_check,
-            MockCalledOnceWith([get_resources_bin_path()]),
+        patch_call_and_check.assert_called_once_with(
+            ["sudo", get_resources_bin_path()]
+        )
+
+    def test_no_use_sudo_in_snap(self):
+        patch_call_and_check = self.patch(ipaddr_module, "call_and_check")
+        patch_call_and_check.return_value = json.dumps(
+            {"networks": SAMPLE_LXD_NETWORKS}
+        )
+        self.patch(ipaddr_module, "running_in_snap").return_value = True
+        get_ip_addr()
+        patch_call_and_check.assert_called_once_with(
+            [get_resources_bin_path()]
         )
 
     def test_get_mac_addresses_returns_all_mac_addresses(self):