[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: sosreport#3399
Related: sosreport#3400

Signed-off-by: Jake Hunsaker <[email protected]>

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

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:

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.

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):

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

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)