[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 <jacob.r.hunsaker@gmail.com>

6 files changed, 53 insertions, 41 deletions

diff --git a/sos/collector/clusters/__init__.py b/sos/collector/clusters/__init__.py
index 1bf1c3ab..21fa1425 100644
--- a/sos/collector/clusters/__init__.py
+++ b/sos/collector/clusters/__init__.py
@@ -294,7 +294,8 @@ class Cluster():
         """
         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 @@ class Cluster():
         :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 7375da09..6a9b8775 100644
--- a/sos/collector/sosnode.py
+++ b/sos/collector/sosnode.py
@@ -443,20 +443,25 @@ class SosNode():
             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 @@ class SosNode():
             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 @@ class SosNode():
             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 99d5f1e2..7330dac4 100644
--- a/sos/collector/transports/__init__.py
+++ b/sos/collector/transports/__init__.py
@@ -194,8 +194,16 @@ class RemoteTransport():
         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 @@ class RemoteTransport():
         :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 b5f91a82..ae228c5c 100644
--- a/sos/collector/transports/oc.py
+++ b/sos/collector/transports/oc.py
@@ -208,15 +208,15 @@ class OCTransport(RemoteTransport):
         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 8c127087..3691a363 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 9b95e95d..92da479f 100644
--- a/sos/policies/package_managers/__init__.py
+++ b/sos/policies/package_managers/__init__.py
@@ -72,7 +72,7 @@ class PackageManager():
         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 @@ class PackageManager():
         :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 @@ class PackageManager():
         :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)