From f33d54196484e9cd7bb1f21e30d6d7f2c126e983 Mon Sep 17 00:00:00 2001 From: Abhijit Menon-Sen Date: Mon, 28 Sep 2015 20:58:30 +0530 Subject: [PATCH 1/2] Move sshpass checking into a separate method Checking for sshpass is peripheral to the calling code, so it's easier to follow when the details are moved into a method. --- lib/ansible/plugins/connection/ssh.py | 33 +++++++++++++++------------ 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/lib/ansible/plugins/connection/ssh.py b/lib/ansible/plugins/connection/ssh.py index c7bbfc4f44d..f9ee77d0dd5 100644 --- a/lib/ansible/plugins/connection/ssh.py +++ b/lib/ansible/plugins/connection/ssh.py @@ -92,21 +92,7 @@ class Connection(ConnectionBase): # write the password to sshpass. if self._play_context.password: - global SSHPASS_AVAILABLE - - # We test once if sshpass is available, and remember the result. It - # would be nice to use distutils.spawn.find_executable for this, but - # distutils isn't always available; shutils.which() is Python3-only. - - if SSHPASS_AVAILABLE is None: - try: - p = subprocess.Popen(["sshpass"], stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE) - p.communicate() - SSHPASS_AVAILABLE = True - except OSError: - SSHPASS_AVAILABLE = False - - if not SSHPASS_AVAILABLE: + if not self._sshpass_available(): raise AnsibleError("to use the 'ssh' connection type with passwords, you must install the sshpass program") self.sshpass_pipe = os.pipe() @@ -646,6 +632,23 @@ class Connection(ConnectionBase): # Utility functions + def _sshpass_available(self): + global SSHPASS_AVAILABLE + + # We test once if sshpass is available, and remember the result. It + # would be nice to use distutils.spawn.find_executable for this, but + # distutils isn't always available; shutils.which() is Python3-only. + + if SSHPASS_AVAILABLE is None: + try: + p = subprocess.Popen(["sshpass"], stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + p.communicate() + SSHPASS_AVAILABLE = True + except OSError: + SSHPASS_AVAILABLE = False + + return SSHPASS_AVAILABLE + def _terminate_process(self, p): try: p.terminate() From 38c7422da59c2d764a176e5da8d98baceadc7f7a Mon Sep 17 00:00:00 2001 From: Abhijit Menon-Sen Date: Mon, 28 Sep 2015 21:11:56 +0530 Subject: [PATCH 2/2] Move ControlPersist/Path checking into a separate method This is also peripheral to what _build_command needs, can be improved and tested independently, and so makes more sense in a separate method. This commit doesn't change any functionality (and I've verified that it works with the various combinations: control_path set in ansible.cfg, ssh_args adding or not adding ControlMaster/ControlPersist, etc.). --- lib/ansible/plugins/connection/ssh.py | 64 ++++++++++++++++----------- 1 file changed, 37 insertions(+), 27 deletions(-) diff --git a/lib/ansible/plugins/connection/ssh.py b/lib/ansible/plugins/connection/ssh.py index f9ee77d0dd5..8ae35a3517d 100644 --- a/lib/ansible/plugins/connection/ssh.py +++ b/lib/ansible/plugins/connection/ssh.py @@ -190,37 +190,28 @@ class Connection(ConnectionBase): args = self._split_args(self.ssh_extra_args) self.add_args("inventory added ansible_ssh_extra_args", args) - # If ssh_args or ssh_extra_args set ControlPersist but not a - # ControlPath, add one ourselves. + # Check if ControlPersist is enabled (either by default, or using + # ssh_args or ssh_extra_args) and add a ControlPath if one hasn't + # already been set. - cp_in_use = False - cp_path_set = False - for arg in self._command: - if "ControlPersist" in arg: - cp_in_use = True - if "ControlPath" in arg: - cp_path_set = True + controlpersist, controlpath = self._persistence_controls(self._command) - if cp_in_use and not cp_path_set: - self._cp_dir = unfrackpath('$HOME/.ansible/cp') - - args = ("-o", "ControlPath={0}".format( - C.ANSIBLE_SSH_CONTROL_PATH % dict(directory=self._cp_dir)) - ) - self.add_args("found only ControlPersist; added ControlPath", args) - - # The directory must exist and be writable. - makedirs_safe(self._cp_dir, 0o700) - if not os.access(self._cp_dir, os.W_OK): - raise AnsibleError("Cannot write to ControlPath %s" % self._cp_dir) - - # If the configuration dictates that we use a persistent connection, - # then we remember that for later. (We could be more thorough about - # detecting this, though.) - - if cp_in_use: + if controlpersist: self._persistent = True + if not controlpath: + cpdir = unfrackpath('$HOME/.ansible/cp') + + # The directory must exist and be writable. + makedirs_safe(cpdir, 0o700) + if not os.access(cpdir, os.W_OK): + raise AnsibleError("Cannot write to ControlPath %s" % cpdir) + + args = ("-o", "ControlPath={0}".format( + C.ANSIBLE_SSH_CONTROL_PATH % dict(directory=cpdir)) + ) + self.add_args("found only ControlPersist; added ControlPath", args) + ## Finally, we add any caller-supplied extras. if other_args: @@ -649,6 +640,25 @@ class Connection(ConnectionBase): return SSHPASS_AVAILABLE + def _persistence_controls(self, command): + ''' + Takes a command array and scans it for ControlPersist and ControlPath + settings and returns two booleans indicating whether either was found. + This could be smarter, e.g. returning false if ControlPersist is 'no', + but for now we do it simple way. + ''' + + controlpersist = False + controlpath = False + + for arg in command: + if 'controlpersist' in arg.lower(): + controlpersist = True + elif 'controlpath' in arg.lower(): + controlpath = True + + return controlpersist, controlpath + def _terminate_process(self, p): try: p.terminate()