Fix sonic-kdump-config for running commands with pipe (#4045)

* Fix sonic-kdump-config for running commands with pipe

The exising code does not work because the commands with pipe
are only able to run in shell

* Avoid using use_shell=True in sonic-kdump-config

* Fix unit test for the usage of getstatusoutput_noshell_pipe

Author: byu343

scripts/sonic-kdump-config

@@ -28,6 +28,7 @@ import subprocess
 from swsscommon.swsscommon import ConfigDBConnector
 from sonic_installer.bootloader import get_bootloader
 from sonic_installer.common import IMAGE_PREFIX
+from sonic_py_common.general import getstatusoutput_noshell_pipe
 
 aboot_cfg_template ="/host/image-%s/kernel-cmdline"
 grub_cfg = "/host/grub/grub.cfg"
@@ -371,8 +372,12 @@ def get_kdump_ssh_path():
 #
 #  @return The integer value X from USE_KDUMP=X in /etc/default/kdump-tools
 def read_use_kdump():
-    (rc, lines, err_str) = run_command("grep 'USE_KDUMP=.*' %s | cut -d = -f 2" % kdump_cfg, use_shell=False);
-    if rc == 0 and type(lines) == list and len(lines) >= 1:
+    cmd0 = ['grep', 'USE_KDUMP=.*', kdump_cfg]
+    cmd1 = ['cut', '-d', '=', '-f', '2' ]
+
+    exitcodes, output = getstatusoutput_noshell_pipe(cmd0, cmd1)
+    lines = output.splitlines()
+    if all(code == 0 for code in exitcodes) and type(lines) == list and len(lines) >= 1:
         try:
             return int(lines[0])
         except Exception as e:
@@ -405,8 +410,12 @@ def write_use_kdump(use_kdump):
 #
 #  @return The integer value X from KDUMP_NUM_DUMPS=X in /etc/default/kdump-tools
 def read_num_dumps():
-    (rc, lines, err_str) = run_command("grep '#*KDUMP_NUM_DUMPS=.*' %s | cut -d = -f 2" % kdump_cfg, use_shell=False);
-    if rc == 0 and type(lines) == list and len(lines) >= 1:
+    cmd0 = ['grep', '#*KDUMP_NUM_DUMPS=.*', kdump_cfg]
+    cmd1 = ['cut', '-d', '=', '-f', '2' ]
+
+    exitcodes, output = getstatusoutput_noshell_pipe(cmd0, cmd1)
+    lines = output.splitlines()
+    if all(code == 0 for code in exitcodes) and type(lines) == list and len(lines) >= 1:
         try:
             return int(lines[0])
         except Exception as e:
@@ -448,8 +457,13 @@ def write_kdump_remote():
         print("SSH and SSH_KEY commented out for local configuration.")
 
 def read_ssh_string():
-    (rc, lines, err_str) = run_command("grep '#*SSH=.*' %s | cut -d '=' -f 2 | tr -d '\"'" % kdump_cfg, use_shell=False)
-    if rc == 0 and type(lines) == list and len(lines) >= 1:
+    cmd0 = ['grep', '#*SSH=.*', kdump_cfg]
+    cmd1 = ['cut', '-d', '=', '-f', '2' ]
+    cmd2 = ['tr', '-d', '\"']
+
+    exitcodes, output = getstatusoutput_noshell_pipe(cmd0, cmd1, cmd2)
+    lines = output.splitlines()
+    if all(code == 0 for code in exitcodes) and type(lines) == list and len(lines) >= 1:
         try:
             return lines[0].strip()  # Return the SSH string (e.g., user@ip_address)
         except Exception as e:
@@ -479,8 +493,13 @@ def write_ssh_string(ssh_string):
         sys.exit(1)
 
 def read_ssh_path():
-    (rc, lines, err_str) = run_command("grep '#*SSH_KEY=.*' %s | cut -d '=' -f 2 | tr -d '\"'" % kdump_cfg, use_shell=False)
-    if rc == 0 and type(lines) == list and len(lines) >= 1:
+    cmd0 = ['grep', '#*SSH_KEY=.*', kdump_cfg]
+    cmd1 = ['cut', '-d', '=', '-f', '2' ]
+    cmd2 = ['tr', '-d', '\"']
+
+    exitcodes, output = getstatusoutput_noshell_pipe(cmd0, cmd1, cmd2)
+    lines = output.splitlines()
+    if all(code == 0 for code in exitcodes) and type(lines) == list and len(lines) >= 1:
         try:
             ssh_path = lines[0].strip()
             if not ssh_path.startswith('/'):  # Validate that the path starts with a slash

tests/sonic_kdump_config_test.py

@@ -54,90 +54,65 @@ class TestSonicKdumpConfig(unittest.TestCase):
     def setup_class(cls):
         print("SETUP")
 
-    @patch("sonic_kdump_config.run_command")
+    @patch('sonic_kdump_config.getstatusoutput_noshell_pipe')
     def test_read_num_kdumps(self, mock_run_cmd):
         """Tests the function `read_num_kdumps(...)` in script `sonic-kdump-config`.
         """
-        mock_run_cmd.return_value = (0, ["0"], None)
+        mock_run_cmd.return_value = ([0, 0], "0")
         num_dumps = sonic_kdump_config.read_num_dumps()
         assert num_dumps == 0
         logger.info("Value of 'num_dumps' is: '{}'.".format(num_dumps))
         logger.info("Expected value of 'num_dumps' is: '0'.")
 
-        mock_run_cmd.return_value = (0, ["NotInteger"], None)
-        with self.assertRaises(SystemExit) as sys_exit:
-            num_dumps = sonic_kdump_config.read_num_dumps()
-        self.assertEqual(sys_exit.exception.code, 1)
-
-        mock_run_cmd.return_value = (0, (), None)
-        with self.assertRaises(SystemExit) as sys_exit:
-            num_dumps = sonic_kdump_config.read_num_dumps()
-        self.assertEqual(sys_exit.exception.code, 1)
-
-        mock_run_cmd.return_value = (0, [], None)
+        mock_run_cmd.return_value = ([0, 0], "NotInteger")
         with self.assertRaises(SystemExit) as sys_exit:
             num_dumps = sonic_kdump_config.read_num_dumps()
         self.assertEqual(sys_exit.exception.code, 1)
 
-        mock_run_cmd.return_value = (1, [], None)
+        mock_run_cmd.return_value = ([0, 0], "")
         with self.assertRaises(SystemExit) as sys_exit:
             num_dumps = sonic_kdump_config.read_num_dumps()
         self.assertEqual(sys_exit.exception.code, 1)
 
-        mock_run_cmd.return_value = (1, ["3"], None)
+        mock_run_cmd.return_value = ([1, 0], "3")
         with self.assertRaises(SystemExit) as sys_exit:
             num_dumps = sonic_kdump_config.read_num_dumps()
         self.assertEqual(sys_exit.exception.code, 1)
 
-        mock_run_cmd.return_value = (1, (), None)
+        mock_run_cmd.return_value = ([0, 1], "3")
         with self.assertRaises(SystemExit) as sys_exit:
             num_dumps = sonic_kdump_config.read_num_dumps()
         self.assertEqual(sys_exit.exception.code, 1)
 
-        mock_run_cmd.return_value = (1, ["NotInteger"], None)
+        mock_run_cmd.return_value = ([0, 1], "NotInteger")
         with self.assertRaises(SystemExit) as sys_exit:
             num_dumps = sonic_kdump_config.read_num_dumps()
         self.assertEqual(sys_exit.exception.code, 1)
 
-    @patch("sonic_kdump_config.run_command")
+    @patch('sonic_kdump_config.getstatusoutput_noshell_pipe')
     def test_read_use_kdump(self, mock_run_cmd):
         """Tests the function `read_use_kdump(...)` in script `sonic-kdump-config`.
         """
-        mock_run_cmd.return_value = (0, ["0"], None)
+        mock_run_cmd.return_value = ([0, 0], "0")
         is_kdump_enabled = sonic_kdump_config.read_use_kdump()
         assert is_kdump_enabled == 0
 
-        mock_run_cmd.return_value = (0, (), None)
+        mock_run_cmd.return_value = ([0, 0], "NotInteger")
         with self.assertRaises(SystemExit) as sys_exit:
             is_kdump_enabled = sonic_kdump_config.read_use_kdump()
         self.assertEqual(sys_exit.exception.code, 1)
 
-        mock_run_cmd.return_value = (0, [], None)
+        mock_run_cmd.return_value = ([0, 0], "")
         with self.assertRaises(SystemExit) as sys_exit:
             is_kdump_enabled = sonic_kdump_config.read_use_kdump()
         self.assertEqual(sys_exit.exception.code, 1)
 
-        mock_run_cmd.return_value = (0, ["NotInteger"], None)
+        mock_run_cmd.return_value = ([1, 0], "3")
         with self.assertRaises(SystemExit) as sys_exit:
             is_kdump_enabled = sonic_kdump_config.read_use_kdump()
         self.assertEqual(sys_exit.exception.code, 1)
 
-        mock_run_cmd.return_value = (1, ["0"], None)
-        with self.assertRaises(SystemExit) as sys_exit:
-            is_kdump_enabled = sonic_kdump_config.read_use_kdump()
-        self.assertEqual(sys_exit.exception.code, 1)
-
-        mock_run_cmd.return_value = (1, ["NotInteger"], None)
-        with self.assertRaises(SystemExit) as sys_exit:
-            is_kdump_enabled = sonic_kdump_config.read_use_kdump()
-        self.assertEqual(sys_exit.exception.code, 1)
-
-        mock_run_cmd.return_value = (1, (), None)
-        with self.assertRaises(SystemExit) as sys_exit:
-            is_kdump_enabled = sonic_kdump_config.read_use_kdump()
-        self.assertEqual(sys_exit.exception.code, 1)
-
-        mock_run_cmd.return_value = (1, [], None)
+        mock_run_cmd.return_value = ([0, 1], "3")
         with self.assertRaises(SystemExit) as sys_exit:
             is_kdump_enabled = sonic_kdump_config.read_use_kdump()
         self.assertEqual(sys_exit.exception.code, 1)
@@ -467,28 +442,28 @@ def test_cmd_kdump_remote(self, mock_run_command, mock_read_remote):
             sonic_kdump_config.cmd_kdump_remote(verbose=True)
             mock_print.assert_called_with("SSH and SSH_KEY commented out for local configuration.")
 
-    @patch("sonic_kdump_config.run_command")
+    @patch('sonic_kdump_config.getstatusoutput_noshell_pipe')
     def test_read_ssh_string(self, mock_run_cmd):
         """Tests the function `read_ssh_string(...)` in script `sonic-kdump-config`."""
 
         # Test case for successful read
-        mock_run_cmd.return_value = (0, ['user@ip_address'], None)  # Simulate successful command execution
+        mock_run_cmd.return_value = ([0, 0, 0], 'user@ip_address')  # Simulate successful command execution
         ssh_string = sonic_kdump_config.read_ssh_string()
         self.assertEqual(ssh_string, 'user@ip_address')
 
         # Test case for non-integer output
-        mock_run_cmd.return_value = (0, ['NotAString'], None)  # Simulate command execution returning a non-string
+        mock_run_cmd.return_value = ([0, 0, 0], 'NotAString')  # Simulate command execution returning a non-string
         ssh_string = sonic_kdump_config.read_ssh_string()
         self.assertEqual(ssh_string, 'NotAString')
 
         # Test case for empty output
-        mock_run_cmd.return_value = (0, [], None)  # Simulate command execution with empty output
+        mock_run_cmd.return_value = ([0, 0, 0], '')  # Simulate command execution with empty output
         with self.assertRaises(SystemExit) as sys_exit:
             sonic_kdump_config.read_ssh_string()
         self.assertEqual(sys_exit.exception.code, 1)
 
         # Test case for command failure
-        mock_run_cmd.return_value = (1, [], None)  # Simulate command failure
+        mock_run_cmd.return_value = ([0, 0, 1], '')  # Simulate command failure
         with self.assertRaises(SystemExit) as sys_exit:
             sonic_kdump_config.read_ssh_string()
         self.assertEqual(sys_exit.exception.code, 1)
@@ -530,23 +505,23 @@ def test_write_ssh_string(self, mock_read_ssh_string, mock_run_cmd):
             sonic_kdump_config.write_ssh_string('user@ip_address')
         self.assertEqual(sys_exit.exception.code, 1)
 
-    @patch("sonic_kdump_config.run_command")
+    @patch('sonic_kdump_config.getstatusoutput_noshell_pipe')
     def test_read_ssh_path(self, mock_run_cmd):
         """Tests the function `read_ssh_path(...)` in script `sonic-kdump-config`."""
 
         # Test successful case with valid SSH path
-        mock_run_cmd.return_value = (0, ['/path/to/keys'], None)
+        mock_run_cmd.return_value = ([0, 0, 0], '/path/to/keys')
         ssh_path = sonic_kdump_config.read_ssh_path()
         self.assertEqual(ssh_path, '/path/to/keys')
 
         # Test case where SSH path is invalid
-        mock_run_cmd.return_value = (0, ['NotAPath'], None)
+        mock_run_cmd.return_value = ([0, 0, 0], 'NotAPath')
         with self.assertRaises(SystemExit) as sys_exit:
             ssh_path = sonic_kdump_config.read_ssh_path()
         self.assertEqual(sys_exit.exception.code, 1)
 
         # Test case where grep fails (no SSH path found)
-        mock_run_cmd.return_value = (1, [], None)
+        mock_run_cmd.return_value = ([0, 0, 1], '')
         with self.assertRaises(SystemExit) as sys_exit:
             ssh_path = sonic_kdump_config.read_ssh_path()
         self.assertEqual(sys_exit.exception.code, 1)