From ae9df4eb86a287fed04a120786543af811e56d71 Mon Sep 17 00:00:00 2001 From: Jake Hunsaker Date: Fri, 3 Nov 2023 16:02:32 -0400 Subject: [PATCH] [collect] Refactor `get_pty` functionality The `get_pty` parameter for remote executed commands was both a bit of a misnomer and applied too broadly. Refactor this to `use_shell` to be more obvious about what the intent behind the option is, and default all transports to `False`, so that by default we do not wrap any commands in a bash shell. This may be overriden on a per-transport basis via the ned `_need_shell` property within transport subclasses. Further, this facility has been expanded to be allowed on a per-command basis from `SoSNode.run_command()` and wherever that is linked. Related: #3399 Related: #3400 Signed-off-by: Jake Hunsaker --- sos/collector/clusters/__init__.py | 11 ++++--- sos/collector/sosnode.py | 36 ++++++++++++----------- sos/collector/transports/__init__.py | 25 ++++++++++------ sos/collector/transports/oc.py | 6 ++-- sos/collector/transports/saltstack.py | 6 ++-- sos/policies/package_managers/__init__.py | 10 +++---- 6 files changed, 53 insertions(+), 41 deletions(-) diff --git a/sos/collector/clusters/__init__.py b/sos/collector/clusters/__init__.py index 1bf1c3abb5..21fa1425ee 100644 --- a/sos/collector/clusters/__init__.py +++ b/sos/collector/clusters/__init__.py @@ -294,7 +294,8 @@ def check_node_is_primary(self, node): """ return node.address == self.primary.address - def exec_primary_cmd(self, cmd, need_root=False, timeout=180): + def exec_primary_cmd(self, cmd, need_root=False, timeout=180, + use_shell='auto'): """Used to retrieve command output from a (primary) node in a cluster :param cmd: The command to run @@ -306,12 +307,14 @@ def exec_primary_cmd(self, cmd, need_root=False, timeout=180): :param timeout: Amount of time to allow cmd to run in seconds :type timeout: ``int`` + :param use_shell: Does the command required execution within a shell? + :type use_shell: ``auto`` or ``bool`` + :returns: The output and status of `cmd` :rtype: ``dict`` """ - pty = self.primary.local is False - res = self.primary.run_command(cmd, get_pty=pty, need_root=need_root, - timeout=timeout) + res = self.primary.run_command(cmd, need_root=need_root, + use_shell=use_shell, timeout=timeout) if res['output']: res['output'] = res['output'].replace('Password:', '') return res diff --git a/sos/collector/sosnode.py b/sos/collector/sosnode.py index 7375da0968..6a9b8775ae 100644 --- a/sos/collector/sosnode.py +++ b/sos/collector/sosnode.py @@ -443,20 +443,25 @@ def is_installed(self, pkg): return False return self.host.package_manager.pkg_by_name(pkg) is not None - def run_command(self, cmd, timeout=180, get_pty=False, need_root=False, + def run_command(self, cmd, timeout=180, use_shell='auto', need_root=False, use_container=False, env=None): """Runs a given cmd, either via the SSH session or locally - Arguments: - cmd - the full command to be run - timeout - time in seconds to wait for the command to complete - get_pty - If a shell is absolutely needed to run a command, set - this to True - need_root - if a command requires root privileges, setting this to - True tells sos-collector to format the command with - sudo or su - as appropriate and to input the password - use_container - Run this command in a container *IF* the host is - containerized + :param cmd: The full command to be run + :type cmd: ``str`` + + :param timeout: Time in seconds to wait for `cmd` to complete + :type timeout: ``int`` + + :param use_shell: If a shell is needed to run `cmd`, set to True + :type use_shell: ``bool`` or ``auto`` for transport-determined + + :param use_container: Run this command in a container *IF* the host + is a containerized host + :type use_container: ``bool`` + + :param env: Pass environment variables to set for this `cmd` + :type env: ``dict`` """ if not self.connected and not self.local: self.log_debug('Node is disconnected, attempting to reconnect') @@ -472,15 +477,11 @@ def run_command(self, cmd, timeout=180, get_pty=False, need_root=False, cmd = self.host.format_container_command(cmd) if need_root: cmd = self._format_cmd(cmd) - - if 'atomic' in cmd: - get_pty = True - if env: _cmd_env = self.env_vars env = _cmd_env.update(env) return self._transport.run_command(cmd, timeout, need_root, env, - get_pty) + use_shell) def sosreport(self): """Run an sos report on the node, then collect it""" @@ -779,7 +780,8 @@ def execute_sos_command(self): checksum = False res = self.run_command(self.sos_cmd, timeout=self.opts.timeout, - get_pty=True, need_root=True, + use_shell=True, + need_root=True, use_container=True, env=self.sos_env_vars) if res['status'] == 0: diff --git a/sos/collector/transports/__init__.py b/sos/collector/transports/__init__.py index 99d5f1e2b3..7330dac476 100644 --- a/sos/collector/transports/__init__.py +++ b/sos/collector/transports/__init__.py @@ -194,8 +194,16 @@ def _disconnect(self): raise NotImplementedError("Transport %s does not define disconnect" % self.name) + @property + def _need_shell(self): + """ + Transports may override this to control when/if commands executed over + the transport needs to utilize a shell on the remote host. + """ + return False + def run_command(self, cmd, timeout=180, need_root=False, env=None, - get_pty=False): + use_shell='auto'): """Run a command on the node, returning its output and exit code. This should return the exit code of the command being executed, not the exit code of whatever mechanism the transport uses to execute that @@ -205,26 +213,25 @@ def run_command(self, cmd, timeout=180, need_root=False, env=None, :type cmd: ``str`` :param timeout: The maximum time in seconds to allow the cmd to run - :type timeout: ``int`` - - :param get_pty: Does ``cmd`` require a pty? - :type get_pty: ``bool`` + :type timeout: ``int``` :param need_root: Does ``cmd`` require root privileges? - :type neeed_root: ``bool`` + :type need_root: ``bool`` :param env: Specify env vars to be passed to the ``cmd`` :type env: ``dict`` - :param get_pty: Does ``cmd`` require execution with a pty? - :type get_pty: ``bool`` + :param use_shell: Does ``cmd`` require execution within a shell? + :type use_shell: ``bool`` or ``auto`` for transport-determined :returns: Output of ``cmd`` and the exit code :rtype: ``dict`` with keys ``output`` and ``status`` """ self.log_debug('Running command %s' % cmd) - if get_pty: + if (use_shell is True or + (self._need_shell if use_shell == 'auto' else False)): cmd = "/bin/bash -c %s" % quote(cmd) + self.log_debug(f"Shell requested, command is now {cmd}") # currently we only use/support the use of pexpect for handling the # execution of these commands, as opposed to directly invoking # subprocess.Popen() in conjunction with tools like sshpass. diff --git a/sos/collector/transports/oc.py b/sos/collector/transports/oc.py index b5f91a82ab..ae228c5c23 100644 --- a/sos/collector/transports/oc.py +++ b/sos/collector/transports/oc.py @@ -208,15 +208,15 @@ def _format_cmd_for_exec(self, cmd): return super(OCTransport, self)._format_cmd_for_exec(cmd) def run_command(self, cmd, timeout=180, need_root=False, env=None, - get_pty=False): + use_shell=False): # debug pod setup is slow, extend all timeouts to account for this if timeout: timeout += 10 - # since we always execute within a bash shell, force disable get_pty + # since we always execute within a bash shell, force disable use_shell # to avoid double-quoting return super(OCTransport, self).run_command(cmd, timeout, need_root, - env, False) + env, use_shell=False) def _disconnect(self): if os.path.exists(self.pod_tmp_conf): diff --git a/sos/collector/transports/saltstack.py b/sos/collector/transports/saltstack.py index 8c127087ba..3691a36392 100644 --- a/sos/collector/transports/saltstack.py +++ b/sos/collector/transports/saltstack.py @@ -33,14 +33,14 @@ class SaltStackMaster(RemoteTransport): def _convert_output_json(self, json_output): return list(json.loads(json_output).values())[0] - def run_command( - self, cmd, timeout=180, need_root=False, env=None, get_pty=False): + def run_command(self, cmd, timeout=180, need_root=False, env=None, + use_shell=False): """ Run a command on the remote host using SaltStack Master. If the output is json, convert it to a string. """ ret = super(SaltStackMaster, self).run_command( - cmd, timeout, need_root, env, get_pty) + cmd, timeout, need_root, env, use_shell) with contextlib.suppress(Exception): ret['output'] = self._convert_output_json(ret['output']) return ret diff --git a/sos/policies/package_managers/__init__.py b/sos/policies/package_managers/__init__.py index 9b95e95d25..92da479f3d 100644 --- a/sos/policies/package_managers/__init__.py +++ b/sos/policies/package_managers/__init__.py @@ -72,7 +72,7 @@ def manager_name(self): return self.__class__.__name__.lower().split('package')[0] def exec_cmd(self, command, timeout=30, need_root=False, env=None, - get_pty=False, chroot=None): + use_shell=False, chroot=None): """ Runs a package manager command, either via sos_get_command_output() if local, or via a SoSTransport's run_command() if this needs to be run @@ -90,9 +90,9 @@ def exec_cmd(self, command, timeout=30, need_root=False, env=None, :param env: Environment variables to set :type env: ``dict`` with keys being env vars to define - :param get_pty: If running remotely, does the command require - obtaining a pty? - :type get_pty: ``bool`` + :param use_shell: If running remotely, does the command require + obtaining a shell? + :type use_shell: ``bool`` :param chroot: If necessary, chroot command execution to here :type chroot: ``None`` or ``str`` @@ -101,7 +101,7 @@ def exec_cmd(self, command, timeout=30, need_root=False, env=None, :rtype: ``str`` """ if self.remote_exec: - ret = self.remote_exec(command, timeout, need_root, env, get_pty) + ret = self.remote_exec(command, timeout, need_root, env, use_shell) else: ret = sos_get_command_output(command, timeout, chroot=chroot, env=env)