Address PR comments

Signed-off-by: maipbui <maibui@microsoft.com>

scripts/caclmgrd

@@ -166,46 +166,41 @@ class ControlPlaneAclManager(daemon_base.DaemonBase):
         ip_address_cmd2 = ['cut', '-d', '/', '-f1']
         ip_address_cmd3 = ['head', '-1']
 
-        return self.run_commands_pipe(ip_address_cmd0, ip_address_cmd1, ip_address_cmd2, ip_address_cmd3)
+        return self.run_commands(ip_address_cmd0, ip_address_cmd1, ip_address_cmd2, ip_address_cmd3)
 
     def get_namespace_mgmt_ipv6(self, iptable_ns_cmd_prefix, namespace):
         ipv6_address_cmd0 = iptable_ns_cmd_prefix + ['ip', '-6', '-o', 'addr', 'show', 'scope', 'global', ("eth0" if namespace else "docker0")]
         ipv6_address_cmd1 = ['awk', '{print $4}']
         ipv6_address_cmd2 = ['cut', '-d', '/', '-f1']
         ipv6_address_cmd3 = ['head', '-1']
-        return self.run_commands_pipe(ipv6_address_cmd0, ipv6_address_cmd1, ipv6_address_cmd2, ipv6_address_cmd3)
+        return self.run_commands(ipv6_address_cmd0, ipv6_address_cmd1, ipv6_address_cmd2, ipv6_address_cmd3)
 
     def log_output(self, cmd, exitcodes, stdout):
         if any(exitcodes):
             self.log_error("Error running command '{}'".format(cmd))
         elif stdout:
             return stdout.rstrip('\n')
 
-    def run_commands(self, commands):
+    def run_commands(self, *args):
         """
-        Given a list of shell commands, run them in order
+        Given a list of shell commands (connected by shell pipes), run them in order
         Args:
-            commands: List of List of Strings, each string is a shell command
+            args: List of List of Strings, each string is a shell command
+                  or List of strings, connected by shell pipes
         """
-        for cmd in commands:
-            proc = subprocess.Popen(cmd, universal_newlines=True, stdout=subprocess.PIPE)
+        if isinstance(args[0], list):
+            for cmd in args[0]:
+                proc = subprocess.Popen(cmd, universal_newlines=True, stdout=subprocess.PIPE)
 
-            (stdout, stderr) = proc.communicate()
+                (stdout, stderr) = proc.communicate()
+                self.log_output(cmd, [proc.returncode], stdout)
+        else:
+            exitcodes, stdout = getstatusoutput_noshell_pipe(*args)
+            cmd_list = [' '.join(arg) for arg in args]
+            cmd = '|'.join(cmd_list)
             self.log_output(cmd, [proc.returncode], stdout)
         return ""
 
-    def run_commands_pipe(self, *args):
-        """
-        Run commands connected by shell pipes in a secure way without invoking shell injections.
-        Return empty string and log error if not success. Otherwise, return the command output.
-        Args:
-            args: List of strings
-        """
-        exitcodes, stdout = getstatusoutput_noshell_pipe(*args)
-        cmd_list = [' '.join(arg) for arg in args]
-        cmd = '|'.join(cmd_list)
-        return self.log_output(cmd, exitcodes, stdout)
-
     def parse_int_to_tcp_flags(self, hex_value):
         tcp_flags_str = ""
         if hex_value & 0x01:

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)
 

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)

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)