From db9b3785fa9dd7e843b4db7fa4b939b803c755b1 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 8 Apr 2019 18:22:28 +0200 Subject: [PATCH 1/5] Update the paramiko runner code to replace all occurances of \r\n in the stdout and stderr data with \n when pty is used. When pty is used, default behavior is to replace all new line characters \n with \r\n which is almost never desired. --- st2common/st2common/runners/paramiko_ssh.py | 28 ++++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/st2common/st2common/runners/paramiko_ssh.py b/st2common/st2common/runners/paramiko_ssh.py index fdca3dc19a..24826a76d5 100644 --- a/st2common/st2common/runners/paramiko_ssh.py +++ b/st2common/st2common/runners/paramiko_ssh.py @@ -363,7 +363,13 @@ def run(self, cmd, timeout=None, quote=False, call_line_handler_func=False): if cmd.startswith('sudo'): # Note that fabric does this as well. If you set pty, stdout and stderr # streams will be combined into one. + # NOTE: If pty is used, every new line character \n will be converted to \r\n which + # isn't desired. Because of that we sanitize the output and replace \r\n with \n at the + # bottom of this method + uses_pty = True chan.get_pty() + else: + uses_pty = False chan.exec_command(cmd) stdout = StringIO() @@ -404,8 +410,8 @@ def run(self, cmd, timeout=None, quote=False, call_line_handler_func=False): # TODO: Is this the right way to clean up? chan.close() - stdout = strip_shell_chars(stdout.getvalue()) - stderr = strip_shell_chars(stderr.getvalue()) + stdout = self._sanitize_output(stdout.getvalue(), uses_pty=uses_pty) + stderr = self._sanitize_output(stderr.getvalue(), uses_pty=uses_pty) raise SSHCommandTimeoutError(cmd=cmd, timeout=timeout, stdout=stdout, stderr=stderr) @@ -434,8 +440,8 @@ def run(self, cmd, timeout=None, quote=False, call_line_handler_func=False): # Receive the exit status code of the command we ran. status = chan.recv_exit_status() - stdout = strip_shell_chars(stdout.getvalue()) - stderr = strip_shell_chars(stderr.getvalue()) + stdout = self._sanitize_output(stdout.getvalue(), uses_pty=uses_pty) + stderr = self._sanitize_output(stderr.getvalue(), uses_pty=uses_pty) extra = {'_status': status, '_stdout': stdout, '_stderr': stderr} self.logger.debug('Command finished', extra=extra) @@ -736,6 +742,20 @@ def _get_ssh_config_for_host(self, host): return ssh_config_info + def _sanitize_output(self, output, uses_pty=False): + """ + Function which sanitizes the output (stdout / stderr). + + It strips trailing carriage return and new line characters and if pty is used, it also + replaces all occurances of \r\n with \n. + """ + output = strip_shell_chars(output) + + if uses_pty: + output = output.replace('\r\n', '\n') + + return output + @staticmethod def _is_key_file_needs_passphrase(file): for cls in [paramiko.RSAKey, paramiko.DSSKey, paramiko.ECDSAKey]: From c3f10519a6ce7efe4b7a9a0f36aa92ce55b1c2f2 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 8 Apr 2019 19:09:39 +0200 Subject: [PATCH 2/5] Add changelog entry for it. --- CHANGELOG.rst | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index d9ad591c02..66fcfac5cd 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -97,7 +97,10 @@ Fixed is acquired before write operations to avoid write conflicts. (bug fix) Stackstorm/orquesta#125 * Fix a bug with some API endpoints returning 500 internal server error when an exception contained unicode data. (bug fix) #4598 -* Fix the ``st2 workflow inspect`` command so it correctly passes authentication token. (bug fix) #4615 +* Fix the ``st2 workflow inspect`` command so it correctly passes authentication token. (bug fix) + #4615 +* Fix an issue with new line characters (``\n``) being converted to ``\r\n`` in remote shell + command and script actions which use sudo. (bug fix) #4623 2.10.4 - March 15, 2019 ----------------------- From 6594cbcc778782e5644fd35773da2791cf092803 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 8 Apr 2019 19:11:22 +0200 Subject: [PATCH 3/5] Fix typo. --- st2common/st2common/runners/paramiko_ssh.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/st2common/st2common/runners/paramiko_ssh.py b/st2common/st2common/runners/paramiko_ssh.py index 24826a76d5..f3b73b552c 100644 --- a/st2common/st2common/runners/paramiko_ssh.py +++ b/st2common/st2common/runners/paramiko_ssh.py @@ -747,7 +747,7 @@ def _sanitize_output(self, output, uses_pty=False): Function which sanitizes the output (stdout / stderr). It strips trailing carriage return and new line characters and if pty is used, it also - replaces all occurances of \r\n with \n. + replaces all occurrences of \r\n with \n. """ output = strip_shell_chars(output) From 56dfa1c9813ae10195f5dfa2bcd10ef500ad8cb0 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 8 Apr 2019 19:20:47 +0200 Subject: [PATCH 4/5] Make it a function and add a test case for it. --- st2common/st2common/runners/paramiko_ssh.py | 23 +++-------- st2common/st2common/util/misc.py | 29 +++++++++++++- st2common/tests/unit/test_misc_utils.py | 42 +++++++++++++++++++++ 3 files changed, 75 insertions(+), 19 deletions(-) diff --git a/st2common/st2common/runners/paramiko_ssh.py b/st2common/st2common/runners/paramiko_ssh.py index f3b73b552c..ee9c327133 100644 --- a/st2common/st2common/runners/paramiko_ssh.py +++ b/st2common/st2common/runners/paramiko_ssh.py @@ -31,6 +31,7 @@ from st2common.log import logging from st2common.util.misc import strip_shell_chars +from st2common.util.misc import sanitize_output from st2common.util.shell import quote_unix from st2common.constants.runners import DEFAULT_SSH_PORT, REMOTE_RUNNER_PRIVATE_KEY_HEADER import six @@ -410,8 +411,8 @@ def run(self, cmd, timeout=None, quote=False, call_line_handler_func=False): # TODO: Is this the right way to clean up? chan.close() - stdout = self._sanitize_output(stdout.getvalue(), uses_pty=uses_pty) - stderr = self._sanitize_output(stderr.getvalue(), uses_pty=uses_pty) + stdout = sanitize_output(stdout.getvalue(), uses_pty=uses_pty) + stderr = sanitize_output(stderr.getvalue(), uses_pty=uses_pty) raise SSHCommandTimeoutError(cmd=cmd, timeout=timeout, stdout=stdout, stderr=stderr) @@ -440,8 +441,8 @@ def run(self, cmd, timeout=None, quote=False, call_line_handler_func=False): # Receive the exit status code of the command we ran. status = chan.recv_exit_status() - stdout = self._sanitize_output(stdout.getvalue(), uses_pty=uses_pty) - stderr = self._sanitize_output(stderr.getvalue(), uses_pty=uses_pty) + stdout = sanitize_output(stdout.getvalue(), uses_pty=uses_pty) + stderr = sanitize_output(stderr.getvalue(), uses_pty=uses_pty) extra = {'_status': status, '_stdout': stdout, '_stderr': stderr} self.logger.debug('Command finished', extra=extra) @@ -742,20 +743,6 @@ def _get_ssh_config_for_host(self, host): return ssh_config_info - def _sanitize_output(self, output, uses_pty=False): - """ - Function which sanitizes the output (stdout / stderr). - - It strips trailing carriage return and new line characters and if pty is used, it also - replaces all occurrences of \r\n with \n. - """ - output = strip_shell_chars(output) - - if uses_pty: - output = output.replace('\r\n', '\n') - - return output - @staticmethod def _is_key_file_needs_passphrase(file): for cls in [paramiko.RSAKey, paramiko.DSSKey, paramiko.ECDSAKey]: diff --git a/st2common/st2common/util/misc.py b/st2common/st2common/util/misc.py index 02d8f31513..ab9197a207 100644 --- a/st2common/st2common/util/misc.py +++ b/st2common/st2common/util/misc.py @@ -28,9 +28,12 @@ __all__ = [ 'prefix_dict_keys', 'compare_path_file_name', - 'lowercase_value', 'get_field_name_from_mongoengine_error', + 'sanitize_output', + 'strip_shell_chars', + 'rstrip_last_char', + 'lowercase_value' ] @@ -67,6 +70,30 @@ def compare_path_file_name(file_path_a, file_path_b): return (file_name_a > file_name_b) - (file_name_a < file_name_b) +def sanitize_output(input_str, uses_pty=False): + """ + Function which sanitizes paramiko output (stdout / stderr). + + It strips trailing carriage return and new line characters and if pty is used, it also replaces + all occurrences of \r\n with \n. + + By default when pty is used, all \n characters are convered to \r\n and that's not desired + in our remote runner action output. + + :param input_str: Input string to be sanitized. + :type input_str: ``str`` + + :rtype: ``str`` + + """ + output = strip_shell_chars(input_str) + + if uses_pty: + output = output.replace('\r\n', '\n') + + return output + + def strip_shell_chars(input_str): """ Strips the last '\r' or '\n' or '\r\n' string at the end of diff --git a/st2common/tests/unit/test_misc_utils.py b/st2common/tests/unit/test_misc_utils.py index 32f1ac48d7..20568505a4 100644 --- a/st2common/tests/unit/test_misc_utils.py +++ b/st2common/tests/unit/test_misc_utils.py @@ -21,6 +21,7 @@ from st2common.util.misc import rstrip_last_char from st2common.util.misc import strip_shell_chars from st2common.util.misc import lowercase_value +from st2common.util.misc import sanitize_output from st2common.util.ujson import fast_deepcopy __all__ = [ @@ -94,3 +95,44 @@ def test_fast_deepcopy_success(self): result = fast_deepcopy(value) self.assertEqual(result, value) self.assertEqual(result, expected_value) + + def test_sanitize_output(self): + # 1. pty is not used, \r\n shouldn't be replaced with \n + input_strs = [ + 'foo', + 'foo\n', + 'foo\r\n', + 'foo\nbar\nbaz\n', + 'foo\r\nbar\r\nbaz\r\n', + ] + expected = [ + 'foo', + 'foo', + 'foo', + 'foo\nbar\nbaz', + 'foo\r\nbar\r\nbaz', + ] + + for input_str, expected_output in zip(input_strs, expected): + output = sanitize_output(input_str, uses_pty=False) + self.assertEqual(expected_output, output) + + # 2. pty is used, \r\n should be replaced with \n + input_strs = [ + 'foo', + 'foo\n', + 'foo\r\n', + 'foo\nbar\nbaz\n', + 'foo\r\nbar\r\nbaz\r\n', + ] + expected = [ + 'foo', + 'foo', + 'foo', + 'foo\nbar\nbaz', + 'foo\nbar\nbaz', + ] + + for input_str, expected_output in zip(input_strs, expected): + output = sanitize_output(input_str, uses_pty=True) + self.assertEqual(expected_output, output) From 3524e05c253c8e3a9b72d4863ba90f01c4df9acf Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Wed, 10 Apr 2019 10:10:26 +0200 Subject: [PATCH 5/5] Split test case into two methods. --- st2common/tests/unit/test_misc_utils.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/st2common/tests/unit/test_misc_utils.py b/st2common/tests/unit/test_misc_utils.py index 20568505a4..599f41d246 100644 --- a/st2common/tests/unit/test_misc_utils.py +++ b/st2common/tests/unit/test_misc_utils.py @@ -96,8 +96,8 @@ def test_fast_deepcopy_success(self): self.assertEqual(result, value) self.assertEqual(result, expected_value) - def test_sanitize_output(self): - # 1. pty is not used, \r\n shouldn't be replaced with \n + def test_sanitize_output_use_pyt_false(self): + # pty is not used, \r\n shouldn't be replaced with \n input_strs = [ 'foo', 'foo\n', @@ -117,7 +117,8 @@ def test_sanitize_output(self): output = sanitize_output(input_str, uses_pty=False) self.assertEqual(expected_output, output) - # 2. pty is used, \r\n should be replaced with \n + def test_sanitize_output_use_pyt_true(self): + # pty is used, \r\n should be replaced with \n input_strs = [ 'foo', 'foo\n',