From 683a21ae4ff858196f37ae22e9a599ec64e42b41 Mon Sep 17 00:00:00 2001 From: Lindsay Eliasen <87664908+leliasen-hpe@users.noreply.github.com> Date: Thu, 10 Aug 2023 15:08:24 -0500 Subject: [PATCH] CASMPET-6723 catch UnicodeDecodeError from os/run_command (#54) * CASMPET-6723 raise a UnicodeDecodeError from run_command function * CASMPET-6723 fix test for unicode run_command error * CSAMPET-6723 remove trailing whitespace * CASMPET-6723 remove whitespace in hsm_components_url * Make decoding a parameter Make the `decode` an optional, and just return the string as bytes unless the developer has passed an encoding. It is false to assume an encoding, and auto-resolving the encoding is unreliable. * CASMPET-6723 catch jsonDecodeError if request returns bad json * CASMPET-6723 fixed 'response' spelling * CASMPET-6723 fixing python lint * CASMPET-6723 change JSONDecodeError to ValueError * CASMPET-6723 reformat printed strings * CASMPET-6723 fix line length and indentation --------- Co-authored-by: Russell Bunch --- libcsm/bss/api.py | 35 ++++++++++++++------------- libcsm/hsm/components.py | 12 +++++----- libcsm/os.py | 52 +++++++++++++++++++++++++++++++++------- libcsm/sls/api.py | 36 +++++++++++++++++----------- libcsm/tests/test_os.py | 47 ++++++++++++++++++++++++++++++++++++ 5 files changed, 136 insertions(+), 46 deletions(-) diff --git a/libcsm/bss/api.py b/libcsm/bss/api.py index 6b9ab2d..a40df13 100644 --- a/libcsm/bss/api.py +++ b/libcsm/bss/api.py @@ -57,12 +57,12 @@ def get_bss_bootparams(self, xname: str) -> str: "Content-Type": "application/json"}, data=json.dumps(body)) except requests.exceptions.RequestException as ex: - raise requests.exceptions.RequestException(f'ERROR exception: \ - {type(ex).__name__} when trying to get bootparameters') + raise requests.exceptions.RequestException(f'ERROR exception:' \ + f'{type(ex).__name__} when trying to get bootparameters') if bss_response.status_code != http.HTTPStatus.OK: - raise requests.exceptions.RequestException(f'ERROR Failed to get BSS \ - bootparameters for {xname}. Recieved http response:\ - {bss_response.status_code} from BSS.') + raise requests.exceptions.RequestException(f'ERROR Failed to get BSS' \ + f'bootparameters for {xname}. Recieved http response:' \ + f'{bss_response.status_code} from BSS.') return bss_response.json()[0] @@ -76,12 +76,12 @@ def patch_bss_bootparams(self, xname : str, bss_json) -> None: "Content-Type": "application/json"}, data=json.dumps(bss_json)) except requests.exceptions.RequestException as ex: - raise requests.exceptions.RequestException(f'ERROR exception: \ - {type(ex).__name__} when trying to patch bootparameters') + raise requests.exceptions.RequestException(f'ERROR exception:' \ + f'{type(ex).__name__} when trying to patch bootparameters') if patch_response.status_code != http.HTTPStatus.OK: - raise requests.exceptions.RequestException(f'ERROR Failed to patch BSS \ - bootparameters for {xname}. Recieved {patch_response.status_code} \ - from as BSS response.') + raise requests.exceptions.RequestException(f'ERROR Failed to patch BSS' \ + f'bootparameters for {xname}. Recieved {patch_response.status_code}' \ + f'from as BSS response.') print('BSS entry patched') def set_bss_image(self, xname: str, image_dict: dict) -> None: @@ -93,14 +93,15 @@ def set_bss_image(self, xname: str, image_dict: dict) -> None: """ if 'initrd' not in image_dict or 'kernel' not in image_dict or 'rootfs' not in image_dict: - raise ValueError(f"ERROR set_bss_image has inputs 'xname' and 'image_dictonary' where \ - 'image_dictionary' is a dictionary containing values for 'initrd', 'kernel', and \ - 'rootfs'. The inputs recieved were xname:{xname}, image_dictionary:{image_dict}") + raise ValueError(f"ERROR set_bss_image has inputs 'xname' and 'image_dictonary' where" \ + f"'image_dictionary' is a dictionary containing values for 'initrd', 'kernel', " \ + f"and 'rootfs'. The inputs recieved were xname:{xname}, " \ + f"image_dictionary:{image_dict}") bss_json = self.get_bss_bootparams(xname) if 'initrd' not in bss_json or 'kernel' not in bss_json: - raise KeyError(f"BSS bootparams did not have the expected keys 'initrd' or 'kernel'. \ - Boot parameters recieved: {bss_json}") + raise KeyError(f"BSS bootparams did not have the expected keys 'initrd' or 'kernel'." \ + f"Boot parameters recieved: {bss_json}") # set new images bss_json['initrd'] = image_dict['initrd'] bss_json['kernel'] = image_dict['kernel'] @@ -108,8 +109,8 @@ def set_bss_image(self, xname: str, image_dict: dict) -> None: try: current_rootfs = params.split("metal.server=", 1)[1].split(" ",1)[0] except Exception as exc: - raise KeyError(f"ERROR could not find current metal.server image in {xname} \ - bss params") from exc + raise KeyError(f"ERROR could not find current metal.server image in {xname}" \ + f"bss params") from exc bss_json['params'] = params.replace(current_rootfs, image_dict['rootfs']) diff --git a/libcsm/hsm/components.py b/libcsm/hsm/components.py index c1f7bee..3ada66c 100644 --- a/libcsm/hsm/components.py +++ b/libcsm/hsm/components.py @@ -40,8 +40,8 @@ def get_components(role_subrole: str, api_gateway_address="api-gw-service-nmn.lo auth = api.Auth() auth.refresh_token() session = get_session() - hsm_components_url = f'https://{api_gateway_address}/\ - apis/smd/hsm/v2/State/Components' + hsm_components_url = f'https://{api_gateway_address}/'\ + f'apis/smd/hsm/v2/State/Components' # get components if role_subrole not in ROLE_SUBROLES: raise KeyError(f'ERROR {role_subrole} is not a valid role_subrole') @@ -51,9 +51,9 @@ def get_components(role_subrole: str, api_gateway_address="api-gw-service-nmn.lo f'?role=Management&subrole={subrole}', headers={'Authorization': f'Bearer {auth.token}'}) except requests.exceptions.RequestException as ex: - raise requests.exceptions.RequestException(f'ERROR exception: \ - {type(ex).__name__} when trying to get components') + raise requests.exceptions.RequestException(f'ERROR exception:' \ + f'{type(ex).__name__} when trying to get components') if components_response.status_code != http.HTTPStatus.OK: - raise requests.exceptions.RequestException(f'ERROR Failed \ - to get components with subrole {subrole}') + raise requests.exceptions.RequestException(f'ERROR Failed' \ + f'to get components with subrole {subrole}') return components_response diff --git a/libcsm/os.py b/libcsm/os.py index 5888a9b..af6b18e 100644 --- a/libcsm/os.py +++ b/libcsm/os.py @@ -45,8 +45,8 @@ class _CLI: An object to abstract the return result from ``run_command``. """ - _stdout = '' - _stderr = '' + _stdout = b'' + _stderr = b'' _return_code = None _duration = None @@ -86,9 +86,15 @@ def _run(self) -> None: self._return_code = error.errno LOG.error('Could not find command for given args: %s', self.args) else: - self._stdout = stdout.decode('utf8') - self._stderr = stderr.decode('utf8') - self._return_code = command.returncode + try: + self._stdout = stdout + self._stderr = stderr + self._return_code = command.returncode + except UnicodeDecodeError as error: + self._stderr = error + self._return_code = 1 + LOG.error('Could not decode stdout or stderr recieved from given args: %s. \ +stdout: %s, stderr %s', self.args, stdout, stderr) self._duration = time() - start_time if self._return_code and self._duration: LOG.info( @@ -99,14 +105,14 @@ def _run(self) -> None: ) @property - def stdout(self) -> str: + def stdout(self) -> [str, bytes]: """ ``stdout`` from the command. """ return self._stdout @property - def stderr(self) -> str: + def stderr(self) -> [str, bytes]: """ ``stderr`` from the command. """ @@ -126,6 +132,26 @@ def duration(self) -> float: """ return self._duration + def decode(self, charset: str) -> None: + """ + Decodes ``self._stdout`` and ``self._stderr`` with the given ``charset``. + :param charset: The character set to decode with. + """ + if not isinstance(self._stdout, str): + try: + self._stdout = self._stdout.decode(charset) + except ValueError as error: + LOG.error("Command output was requested to be decoded as" + " %s but failed: %s", charset, error) + raise error + if not isinstance(self._stderr, str): + try: + self._stderr = self._stderr.decode(charset) + except ValueError as error: + LOG.error("Command output was requested to be decoded as" + " %s but failed: %s", charset, error) + raise error + @contextmanager def chdir(directory: str, create: bool = False) -> None: @@ -165,7 +191,9 @@ def chdir(directory: str, create: bool = False) -> None: def run_command( args: [list, str], in_shell: bool = False, - silence: bool = False, ) -> _CLI: + silence: bool = False, + charset: str = None, +) -> _CLI: """ Runs a given command or list of commands by instantiating a ``CLI`` object. @@ -179,6 +207,9 @@ def run_command( :param args: List of arguments to run, can also be a string. If a string, :param in_shell: Whether to use a shell when invoking the command. :param silence: Tells this not to output the command to console. + :param charset: Returns the command ``stdout`` and ``stderr`` as a + string instead of bytes, and decoded with the given + ``charset``. """ args_string = [str(x) for x in args] if not silence: @@ -187,4 +218,7 @@ def run_command( ' '.join(args_string), in_shell ) - return _CLI(args_string, shell=in_shell) + result = _CLI(args_string, shell=in_shell) + if charset: + result.decode(charset) + return result diff --git a/libcsm/sls/api.py b/libcsm/sls/api.py index 3370b46..c4f5ae1 100644 --- a/libcsm/sls/api.py +++ b/libcsm/sls/api.py @@ -54,11 +54,11 @@ def get_management_components_from_sls(self) -> requests.Response: 'search/hardware?extra_properties.Role=Management', headers={'Authorization': f'Bearer {self._auth.token}'}) except requests.exceptions.RequestException as ex: - raise requests.exceptions.RequestException(f'ERROR exception: {type(ex).__name__} \ - when trying to get management components from SLS') from ex + raise requests.exceptions.RequestException(f'ERROR exception: {type(ex).__name__}' \ + f'when trying to get management components from SLS') from ex if components_response.status_code != http.HTTPStatus.OK: - raise requests.exceptions.RequestException(f'ERROR Bad response \ - recieved from SLS. Recived: {components_response}') + raise requests.exceptions.RequestException(f'ERROR Bad response' \ + f'recieved from SLS. Recived: {components_response}') return components_response @@ -66,31 +66,39 @@ def get_xname(self, hostname: str) -> str: """ Function to get the xname of a node from SLS based on a provided hostname. """ - components_response = self.get_management_components_from_sls() + try: + components_response = (self.get_management_components_from_sls()).json() + except ValueError as error: + raise ValueError(f'ERROR did not get valid json for management components' \ + f'from sls. {error}') from error - for node in components_response.json(): + for node in components_response: try: if hostname in node['ExtraProperties']['Aliases']: return node['Xname'] except KeyError as error: - raise KeyError(f'ERROR [ExtraProperties][Aliases] was not in the \ - response from sls. These fields are expected in the json response. \ - The resonponse was {components_response.json()}') from error + raise KeyError(f'ERROR [ExtraProperties][Aliases] was not in the' \ + f'response from sls. These fields are expected in the json response.' \ + f'The response was {components_response}') from error raise ValueError(f'ERROR hostname:{hostname} was not found in management nodes.') def get_hostname(self, xname: str) -> str: """ Function to get the hostname of a management node from SLS based on a provided xname. """ - components_response = self.get_management_components_from_sls() + try: + components_response = (self.get_management_components_from_sls()).json() + except ValueError as error: + raise ValueError(f'ERROR did not get valid json for management components' \ + f'from sls. {error}') from error - for node in components_response.json(): + for node in components_response: try: if xname == node['Xname']: # assumes the hostname is the first entry in ['ExtraProperties']['Aliases'] return node['ExtraProperties']['Aliases'][0] except KeyError as error: - raise KeyError(f'ERROR [ExtraProperties][Aliases] was not in the \ - response from sls. These fields are expected in the json response. \ - The resonponse was {components_response.json()}') from error + raise KeyError(f'ERROR [ExtraProperties][Aliases] was not in the' \ + f'response from sls. These fields are expected in the json response.' \ + f'The response was {components_response}') from error raise ValueError(f'ERROR xname:{xname} was not found in management nodes.') diff --git a/libcsm/tests/test_os.py b/libcsm/tests/test_os.py index 0874448..661ff3f 100644 --- a/libcsm/tests/test_os.py +++ b/libcsm/tests/test_os.py @@ -25,6 +25,10 @@ Tests for the ``os`` module. """ from os import getcwd +from subprocess import Popen + +import pytest +import mock from libcsm.os import run_command from libcsm.os import chdir @@ -53,6 +57,49 @@ def test_run_command(self) -> None: assert isinstance(result.return_code, int) assert result.stdout or result.stderr + def test_run_command_decode(self) -> None: + """ + Assert that a ``_CLI`` object successfully decodes its ``stdout`` and + ``stderr``. + """ + command = ['ls', '-l'] + shell_command = 'ls -l' + bad_command = 'foo!!!' + no_shell_result = run_command(shell_command) + shell_result = run_command( + shell_command, + in_shell=True, + charset='utf-8' + ) + bad_result = run_command(bad_command) + command_result = run_command(command) + for result in [no_shell_result, shell_result, bad_result, + command_result]: + assert result.duration > 0.0 + assert isinstance(result.return_code, int) + assert result.stdout or result.stderr + + def test_run_command_decode_unicode_error(self) -> None: + """ + Assert that ``UnicodeDecodeErrors`` are caught by the ``run_command`` + and a non-zero return code is received. + """ + # Test using a CP1252 (windows ostensibly) character "œ" + byte_string = b'\x9c' + with mock.patch.object( + Popen, + 'communicate', + return_value=(byte_string, byte_string) + ): + result = run_command(['ls', '-l']) + assert result.duration > 0.0 + assert isinstance(result.return_code, int) + assert result.stdout or result.stderr + assert isinstance(result.stdout, bytes) + assert isinstance(result.stderr, bytes) + with pytest.raises(UnicodeDecodeError): + run_command(['ls', '-l'], charset='utf-8') + def test_chdir(self) -> None: """ Assert that our context manager will change directories and