Switch to run command (Azure#2022)

* Temporarily removing the depreciated functions to let the linter find everything that needs changing.

* Migrated run_get_output to run_command

* Fixed typo

* Fixed arg formatting issues.

* Fixed file mode.

* fixed some unit tests

* Potentially fixed unit tests?

* Fixed file name

* Addressed comments

* Fixed comments

* fixed tests.

azurelinuxagent/common/osutil/default.py

@@ -554,7 +554,12 @@ def set_selinux_context(self, path, con): # pylint: disable=R1710
             if not os.path.exists(path):
                 logger.error("Path does not exist: {0}".format(path))
                 return 1
-            return shellutil.run('chcon ' + con + ' ' + path)
+
+            try:
+                shellutil.run_command(['chcon', con, path], log_error=True)
+            except shellutil.CommandError as cmd_err:
+                return cmd_err.returncode
+            return 0
 
     def conf_sshd(self, disable_password):
         option = "no" if disable_password else "yes"
@@ -630,9 +635,20 @@ def umount_dvd(self, chk_err=True, mount_point=None):
 
     def eject_dvd(self, chk_err=True):
         dvd = self.get_dvd_device()
-        retcode = shellutil.run("eject {0}".format(dvd))
-        if chk_err and retcode != 0:
-            raise OSUtilError("Failed to eject dvd: ret={0}".format(retcode))
+        try:
+            shellutil.run_command(["eject", dvd])
+        except shellutil.CommandError as cmd_err:
+            if chk_err:
+
+                msg = """Failed to eject dvd: ret={0}
+                [stdout]
+                {1}
+
+                [stderr]
+                {2}
+                """.format(cmd_err.returncode, cmd_err.stdout, cmd_err.stderr)
+
+                raise OSUtilError(msg)
 
     def try_load_atapiix_mod(self):
         try:
@@ -669,15 +685,22 @@ def is_atapiix_mod_loaded(self, max_retry=1):
         return False
 
     def mount(self, device, mount_point, option="", chk_err=True):
-        cmd = "mount {0} {1} {2}".format(option, device, mount_point)
-        retcode, err = shellutil.run_get_output(cmd, chk_err)
-        if retcode != 0:
-            detail = "[{0}] returned {1}: {2}".format(cmd, retcode, err)
-            err = detail
-        return retcode, err
+        cmd = ["mount", option, device, mount_point]
+        try:
+            output = shellutil.run_command(cmd, log_error=chk_err)
+        except shellutil.CommandError as cmd_err:
+            detail = "[{0}] returned {1}:\n stdout: {2}\n\nstderr: {3}".format(cmd, cmd_err.returncode,
+                cmd_err.stdout, cmd_err.stderr)
+            return cmd_err.returncode, detail
+
+        return 0, output
 
     def umount(self, mount_point, chk_err=True):
-        return shellutil.run("umount {0}".format(mount_point), chk_err=chk_err)
+        try:
+            shellutil.run_command(["umount", mount_point], log_error=chk_err)
+        except shellutil.CommandError as cmd_err:
+            return cmd_err.returncode
+        return 0
 
     def allow_dhcp_broadcast(self):
         # Open DHCP port if iptables is enabled.
@@ -1112,15 +1135,26 @@ def set_dhcp_hostname(self, hostname):
     def restart_if(self, ifname, retries=3, wait=5):
         retry_limit=retries+1
         for attempt in range(1, retry_limit):
-            return_code=shellutil.run("ifdown {0} && ifup {0}".format(ifname), expected_errors=[1] if attempt < retries else [])
-            if return_code == 0:
+            try:
+                shellutil.run_command(["ifdown", ifname])
+                shellutil.run_command(["ifup", ifname])
                 return
-            logger.warn("failed to restart {0}: return code {1}".format(ifname, return_code))
-            if attempt < retry_limit:
-                logger.info("retrying in {0} seconds".format(wait))
-                time.sleep(wait)
-            else:
-                logger.warn("exceeded restart retries")
+            except shellutil.CommandError as cmd_err:
+
+                msg = """failed to restart {0}: returncode={1}
+                [stdout]
+                {2}
+                
+                [stderr]
+                {3}
+                """.format(ifname, cmd_err.returncode, cmd_err.stdout, cmd_err.stderr)
+                logger.warn(msg)
+
+                if attempt < retry_limit:
+                    logger.info("retrying in {0} seconds".format(wait))
+                    time.sleep(wait)
+                else:
+                    logger.warn("exceeded restart retries")
 
     def publish_hostname(self, hostname):
         self.set_dhcp_hostname(hostname)

azurelinuxagent/common/osutil/ubuntu.py

@@ -39,10 +39,18 @@ def start_network(self):
         return shellutil.run("service networking start", chk_err=False)
 
     def stop_agent_service(self):
-        return shellutil.run("service {0} stop".format(self.service_name), chk_err=False)
+        try:
+            shellutil.run_command(["service", self.service_name, "stop"])
+        except shellutil.CommandError as cmd_err:
+            return cmd_err.returncode
+        return 0
 
     def start_agent_service(self):
-        return shellutil.run("service {0} start".format(self.service_name), chk_err=False)
+        try:
+            shellutil.run_command(["service", self.service_name, "start"])
+        except shellutil.CommandError as cmd_err:
+            return cmd_err.returncode
+        return 0
 
     def remove_rules_files(self, rules_files=""):
         pass
@@ -119,15 +127,17 @@ def restart_if(self, ifname, retries=3, wait=5):
         """
         retry_limit=retries+1
         for attempt in range(1, retry_limit):
-            return_code=shellutil.run("ip link set {0} down && ip link set {0} up".format(ifname))
-            if return_code == 0:
-                return
-            logger.warn("failed to restart {0}: return code {1}".format(ifname, return_code))
-            if attempt < retry_limit:
-                logger.info("retrying in {0} seconds".format(wait))
-                time.sleep(wait)
-            else:
-                logger.warn("exceeded restart retries")
+            try:
+                shellutil.run_command(["ip", "link", "set", ifname, "down"])
+                shellutil.run_command(["ip", "link", "set", ifname, "up"])
+
+            except shellutil.CommandError as cmd_err:
+                logger.warn("failed to restart {0}: return code {1}".format(ifname, cmd_err.returncode))
+                if attempt < retry_limit:
+                    logger.info("retrying in {0} seconds".format(wait))
+                    time.sleep(wait)
+                else:
+                    logger.warn("exceeded restart retries")
 
 
 class UbuntuSnappyOSUtil(Ubuntu14OSUtil):

azurelinuxagent/common/utils/cryptutil.py

@@ -41,12 +41,19 @@ def gen_transport_cert(self, prv_file, crt_file):
         """
         Create ssl certificate for https communication with endpoint server.
         """
-        cmd = ("{0} req -x509 -nodes -subj /CN=LinuxTransport -days 730 "
-               "-newkey rsa:2048 -keyout {1} "
-               "-out {2}").format(self.openssl_cmd, prv_file, crt_file)
-        rc = shellutil.run(cmd) # pylint: disable=C0103
-        if rc != 0:
-            logger.error("Failed to create {0} and {1} certificates".format(prv_file, crt_file))
+        cmd = [self.openssl_cmd, "req", "-x509", "-nodes", "-subj", "/CN=LinuxTransport", 
+            "-days", "730", "-newkey", "rsa:2048", "-keyout", prv_file, "-out", crt_file]
+        try:
+            shellutil.run_command(cmd)
+        except shellutil.CommandError as cmd_err:
+            msg = """Failed to create {0} and {1} certificates.
+            [stdout]
+            {2}
+
+            [stderr]
+            {3}
+            """.format(prv_file, crt_file, cmd_err.stdout, cmd_err.stderr)
+            logger.error(msg)
 
     def get_pubkey_from_prv(self, file_name):
         if not os.path.exists(file_name): # pylint: disable=R1720
@@ -79,18 +86,39 @@ def decrypt_p7m(self, p7m_file, trans_prv_file, trans_cert_file, pem_file):
         elif not os.path.exists(trans_prv_file):
             raise IOError(errno.ENOENT, "File not found", trans_prv_file)
         else:
-            cmd = ("{0} cms -decrypt -in {1} -inkey {2} -recip {3} "
-                   "| {4} pkcs12 -nodes -password pass: -out {5}"
-                   "").format(self.openssl_cmd, p7m_file, trans_prv_file,
-                              trans_cert_file, self.openssl_cmd, pem_file)
-            shellutil.run(cmd)
-            rc = shellutil.run(cmd) # pylint: disable=C0103
-            if rc != 0:
-                logger.error("Failed to decrypt {0}".format(p7m_file))
+            first_cmd = [self.openssl_cmd, "cms", "-decrypt", "-in", p7m_file, "-inkey",
+                trans_prv_file, "-recip", trans_cert_file]
+            second_cmd = [self.openssl_cmd, "pkcs12", "-nodes", "-password", "pass:", 
+                "-out", pem_file]
+
+            first_proc = subprocess.Popen(first_cmd, stdout=subprocess.PIPE)
+            second_proc = subprocess.Popen(second_cmd, stdin=first_proc.stdout, stdout=subprocess.PIPE)
+            stdout, stderr = second_proc.communicate()
+
+            if first_proc.returncode != 0 or second_proc.returncode != 0:
+                stdout = ustr(stdout, encoding='utf-8', errors="backslashreplace") if stdout else ""
+                stderr =  ustr(stderr, encoding='utf-8', errors="backslashreplace") if stderr else ""
+
+                msg = """Failed to decrypt {0}
+                [stdout]
+                {1}
+
+                [stderr]
+                {2}
+                """.format(p7m_file, stdout, stderr)
+                logger.error(msg)
+
 
     def crt_to_ssh(self, input_file, output_file):
-        shellutil.run("ssh-keygen -i -m PKCS8 -f {0} >> {1}".format(input_file,
-                                                                    output_file))
+        with open(output_file, "a") as file_out:
+            keygen_proc = subprocess.Popen(["ssh-keygen", "-i", "-m", "PKCS8", "-f", input_file])
+            stdout, _ = keygen_proc.communicate()
+
+            if keygen_proc.returncode != 0:
+                return
+
+            file_out.write(stdout)
+
 
     def asn1_to_ssh(self, pubkey):
         lines = pubkey.split("\n")

tests/common/osutil/test_default.py

@@ -56,15 +56,22 @@ def test_restart(self):
         # setup
         retries = 3
         ifname = 'dummy'
-        with patch.object(shellutil, "run") as run_patch:
-            run_patch.return_value = 1
+        with patch.object(shellutil, "run_command") as run_patch:
+            run_patch.side_effect = shellutil.CommandError("ifupdown dummy", 1, "", "")
 
             # execute
             osutil.DefaultOSUtil.restart_if(osutil.DefaultOSUtil(), ifname=ifname, retries=retries, wait=0)
 
             # assert
             self.assertEqual(run_patch.call_count, retries)
-            self.assertEqual(run_patch.call_args_list[0][0][0], 'ifdown {0} && ifup {0}'.format(ifname))
+
+            cmd_queue = list(args[0] for (args, _) in run_patch.call_args_list)            
+            while cmd_queue:
+                self.assertEqual(cmd_queue.pop(0), ["ifdown", ifname])
+                # We don't expect the following command to be called because 'dummy' does
+                # not exist.
+                self.assertNotEqual(cmd_queue[0] if cmd_queue else None, ["ifup", ifname])
+
 
     def test_get_dvd_device_success(self):
         with patch.object(os, 'listdir', return_value=['cpu', 'cdrom0']):
@@ -85,8 +92,8 @@ def test_mount_dvd_success(self, _):
                           'get_dvd_device',
                           return_value='/dev/cdrom'):
             with patch.object(shellutil,
-                              'run_get_output',
-                              return_value=(0, msg)) as patch_run: # pylint: disable=unused-variable
+                              'run_command',
+                              return_value=msg) as patch_run: # pylint: disable=unused-variable
                 with patch.object(os, 'makedirs'):
                     try:
                         osutil.DefaultOSUtil().mount_dvd()
@@ -99,16 +106,15 @@ def test_mount_dvd_failure(self, _):
         with patch.object(osutil.DefaultOSUtil,
                           'get_dvd_device',
                           return_value='/dev/cdrom'):
-            with patch.object(shellutil,
-                              'run_get_output',
-                              return_value=(1, msg)) as patch_run:
+            with patch.object(shellutil, 'run_command',
+                              side_effect=shellutil.CommandError("mount dvd", 1, "", msg)) as patch_run:
                 with patch.object(os, 'makedirs'):
                     try:
                         osutil.DefaultOSUtil().mount_dvd()
                         self.fail('OSUtilError was not raised')
                     except OSUtilError as ose:
                         self.assertTrue(msg in ustr(ose))
-                        self.assertTrue(patch_run.call_count == 6)
+                        self.assertEqual(patch_run.call_count, 5)
 
     def test_empty_proc_net_route(self):
         routing_table = ""