Revert "Remove subprocess with shell=True (#24)" (#33)

This reverts commit 409ecc3.
sonic-net · Jan 12, 2023 · 28afb55 · 28afb55
1 parent 409ecc3
commit 28afb55
Show file tree

Hide file tree

Showing 22 changed files with 467 additions and 554 deletions.
diff --git a/scripts/caclmgrd b/scripts/caclmgrd
diff --git a/scripts/hostcfgd b/scripts/hostcfgd
diff --git a/scripts/procdockerstatsd b/scripts/procdockerstatsd
@@ -13,7 +13,6 @@ from datetime import datetime
 
 from sonic_py_common import daemon_base
 from swsscommon import swsscommon
-from sonic_py_common.general import getstatusoutput_noshell_pipe
 
 VERSION = '1.0'
 
@@ -30,7 +29,7 @@ class ProcDockerStats(daemon_base.DaemonBase):
         self.state_db.connect("STATE_DB")
 
     def run_command(self, cmd):
-        proc = subprocess.Popen(cmd, universal_newlines=True, stdout=subprocess.PIPE)
+        proc = subprocess.Popen(cmd, shell=True, universal_newlines=True, stdout=subprocess.PIPE)
         (stdout, stderr) = proc.communicate()
         if proc.returncode != 0:
             self.log_error("Error running command '{}'".format(cmd))
@@ -119,7 +118,7 @@ class ProcDockerStats(daemon_base.DaemonBase):
         return dockerdict
 
     def update_dockerstats_command(self):
-        cmd = ["docker", "stats", "--no-stream", "-a"]
+        cmd = "docker stats --no-stream -a"
         data = self.run_command(cmd)
         if not data:
             self.log_error("'{}' returned null output".format(cmd))
@@ -136,13 +135,7 @@ class ProcDockerStats(daemon_base.DaemonBase):
         return True
 
     def update_processstats_command(self):
-        cmd0 = ["ps", "-eo", "uid,pid,ppid,%mem,%cpu,stime,tty,time,cmd", "--sort", "-%cpu"]
-        cmd1 = ["head", "-1024"]
-        cmd = ' | '.join([' '.join(cmd0), ' '.join(cmd1)])
-        exitcode, data = getstatusoutput_noshell_pipe(cmd0, cmd1)
-        if any(exitcode):
-            self.log_error("Error running command '{}'".format(cmd))
-            data = None
+        data = self.run_command("ps -eo uid,pid,ppid,%mem,%cpu,stime,tty,time,cmd --sort -%cpu | head -1024")
         processdata = self.format_process_cmd_output(data)
         value = ""
         # wipe out all data before updating with new values

diff --git a/tests/caclmgrd/caclmgrd_bfd_test.py b/tests/caclmgrd/caclmgrd_bfd_test.py
@@ -34,18 +34,17 @@ def test_caclmgrd_bfd(self, test_name, test_data, fs):
 
         MockConfigDb.set_config_db(test_data["config_db"])
 
-        with mock.patch("caclmgrd.ControlPlaneAclManager.run_commands_pipe", return_value='sonic'):
-            with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
-                popen_mock = mock.Mock()
-                popen_attrs = test_data["popen_attributes"]
-                popen_mock.configure_mock(**popen_attrs)
-                mocked_subprocess.Popen.return_value = popen_mock
-                mocked_subprocess.PIPE = -1
-
-                call_rc = test_data["call_rc"]
-                mocked_subprocess.call.return_value = call_rc
-
-                caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
-                caclmgrd_daemon.allow_bfd_protocol('')
-                mocked_subprocess.Popen.assert_has_calls(test_data["expected_subprocess_calls"], any_order=True)
+        with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
+            popen_mock = mock.Mock()
+            popen_attrs = test_data["popen_attributes"]
+            popen_mock.configure_mock(**popen_attrs)
+            mocked_subprocess.Popen.return_value = popen_mock
+            mocked_subprocess.PIPE = -1
+
+            call_rc = test_data["call_rc"]
+            mocked_subprocess.call.return_value = call_rc
+
+            caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
+            caclmgrd_daemon.allow_bfd_protocol('')
+            mocked_subprocess.Popen.assert_has_calls(test_data["expected_subprocess_calls"], any_order=True)
 
diff --git a/tests/caclmgrd/caclmgrd_dhcp_test.py b/tests/caclmgrd/caclmgrd_dhcp_test.py
@@ -33,22 +33,21 @@ def test_caclmgrd_dhcp(self, test_name, test_data, fs):
 
         MockConfigDb.set_config_db(test_data["config_db"])
 
-        with mock.patch("caclmgrd.ControlPlaneAclManager.run_commands_pipe", return_value='sonic'):
-            with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
-                popen_mock = mock.Mock()
-                popen_attrs = test_data["popen_attributes"]
-                popen_mock.configure_mock(**popen_attrs)
-                mocked_subprocess.Popen.return_value = popen_mock
+        with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
+            popen_mock = mock.Mock()
+            popen_attrs = test_data["popen_attributes"]
+            popen_mock.configure_mock(**popen_attrs)
+            mocked_subprocess.Popen.return_value = popen_mock
 
-                call_rc = test_data["call_rc"]
-                mocked_subprocess.call.return_value = call_rc
+            call_rc = test_data["call_rc"]
+            mocked_subprocess.call.return_value = call_rc
 
-                mark = test_data["mark"]
+            mark = test_data["mark"]
 
-                caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
-                mux_update = test_data["mux_update"]
+            caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
+            mux_update = test_data["mux_update"]
 
-                for key,data in mux_update:
-                    caclmgrd_daemon.update_dhcp_acl(key, '', data, mark)
+            for key,data in mux_update:
+                caclmgrd_daemon.update_dhcp_acl(key, '', data, mark)
 
-                mocked_subprocess.call.assert_has_calls(test_data["expected_subprocess_calls"], any_order=False)
+            mocked_subprocess.call.assert_has_calls(test_data["expected_subprocess_calls"], any_order=False)
diff --git a/tests/caclmgrd/caclmgrd_external_client_acl_test.py b/tests/caclmgrd/caclmgrd_external_client_acl_test.py
@@ -41,10 +41,8 @@ def test_caclmgrd_external_client_acl(self, test_name, test_data, fs):
         caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
 
         iptables_rules_ret, _ = caclmgrd_daemon.get_acl_rules_and_translate_to_iptables_commands('')
-        test_data['return'] = [tuple(i) for i in test_data['return']]
-        iptables_rules_ret = [tuple(i) for i in iptables_rules_ret]
         self.assertEqual(set(test_data["return"]).issubset(set(iptables_rules_ret)), True)
-        caclmgrd_daemon.iptables_cmd_ns_prefix['asic0'] = ['ip', 'netns', 'exec', 'asic0']
+        caclmgrd_daemon.iptables_cmd_ns_prefix['asic0'] = 'ip netns exec asic0'
         caclmgrd_daemon.namespace_docker_mgmt_ip['asic0'] = '1.1.1.1'
         caclmgrd_daemon.namespace_mgmt_ip = '2.2.2.2'
         caclmgrd_daemon.namespace_docker_mgmt_ipv6['asic0'] = 'fd::01'

diff --git a/tests/caclmgrd/caclmgrd_feature_test.py b/tests/caclmgrd/caclmgrd_feature_test.py
@@ -34,18 +34,17 @@ def test_feature_present(self, test_name, test_data, fs):
 
         MockConfigDb.set_config_db(test_data["config_db"])
 
-        with mock.patch("caclmgrd.ControlPlaneAclManager.run_commands_pipe", return_value='sonic'):
-            with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
-                popen_mock = mock.Mock()
-                popen_attrs = test_data["popen_attributes"]
-                popen_mock.configure_mock(**popen_attrs)
-                mocked_subprocess.Popen.return_value = popen_mock
-                mocked_subprocess.PIPE = -1
-
-                call_rc = test_data["call_rc"]
-                mocked_subprocess.call.return_value = call_rc
-
-                caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
-                caclmgrd_daemon.update_feature_present()
-                self.assertTrue("bgp" in caclmgrd_daemon.feature_present)
-                self.assertEqual(caclmgrd_daemon.feature_present["bgp"], True)
+        with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
+            popen_mock = mock.Mock()
+            popen_attrs = test_data["popen_attributes"]
+            popen_mock.configure_mock(**popen_attrs)
+            mocked_subprocess.Popen.return_value = popen_mock
+            mocked_subprocess.PIPE = -1
+
+            call_rc = test_data["call_rc"]
+            mocked_subprocess.call.return_value = call_rc
+
+            caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
+            caclmgrd_daemon.update_feature_present()
+            self.assertTrue("bgp" in caclmgrd_daemon.feature_present)
+            self.assertEqual(caclmgrd_daemon.feature_present["bgp"], True)
diff --git a/tests/caclmgrd/caclmgrd_soc_rules_test.py b/tests/caclmgrd/caclmgrd_soc_rules_test.py
@@ -36,20 +36,19 @@ def test_caclmgrd_soc(self, test_name, test_data, fs):
 
         MockConfigDb.set_config_db(test_data["config_db"])
 
-        with mock.patch("caclmgrd.ControlPlaneAclManager.run_commands_pipe", return_value='sonic'):
-            with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
-                popen_mock = mock.Mock()
-                popen_attrs = test_data["popen_attributes"]
-                popen_mock.configure_mock(**popen_attrs)
-                mocked_subprocess.Popen.return_value = popen_mock
-                mocked_subprocess.PIPE = -1
-
-                call_rc = test_data["call_rc"]
-                mocked_subprocess.call.return_value = call_rc
-
-                caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
-                caclmgrd_daemon.update_control_plane_nat_acls('', {})
-                mocked_subprocess.Popen.assert_has_calls(test_data["expected_subprocess_calls"], any_order=True)
+        with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
+            popen_mock = mock.Mock()
+            popen_attrs = test_data["popen_attributes"]
+            popen_mock.configure_mock(**popen_attrs)
+            mocked_subprocess.Popen.return_value = popen_mock
+            mocked_subprocess.PIPE = -1
+
+            call_rc = test_data["call_rc"]
+            mocked_subprocess.call.return_value = call_rc
+
+            caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
+            caclmgrd_daemon.update_control_plane_nat_acls('', {})
+            mocked_subprocess.Popen.assert_has_calls(test_data["expected_subprocess_calls"], any_order=True)
 
     def test_get_ip_from_interface_table(self):
         if not os.path.exists(DBCONFIG_PATH):

diff --git a/tests/caclmgrd/caclmgrd_test.py b/tests/caclmgrd/caclmgrd_test.py
diff --git a/tests/caclmgrd/test_bfd_vectors.py b/tests/caclmgrd/test_bfd_vectors.py
@@ -23,8 +23,8 @@
                 },
             },
             "expected_subprocess_calls": [
-                call(['iptables', '-I', 'INPUT', '2', '-p', 'udp', '-m', 'multiport', '--dports', '3784,4784', '-j', 'ACCEPT'], universal_newlines=True, stdout=subprocess.PIPE),
-                call(['ip6tables', '-I', 'INPUT', '2', '-p', 'udp', '-m', 'multiport', '--dports', '3784,4784', '-j', 'ACCEPT'], universal_newlines=True, stdout=subprocess.PIPE)
+                call("iptables -I INPUT 2 -p udp -m multiport --dports 3784,4784 -j ACCEPT", shell=True, universal_newlines=True, stdout=subprocess.PIPE),
+                call("ip6tables -I INPUT 2 -p udp -m multiport --dports 3784,4784 -j ACCEPT", shell=True, universal_newlines=True, stdout=subprocess.PIPE)
             ],
             "popen_attributes": {
                 'communicate.return_value': ('output', 'error'),