Fix ssh mux socket file error.

ssh ControlPath socket file needs to be unique for each host.
To avoid using ssh ControlPath option placeholders move socket file
to host's temp directory. Since each host has unique temp
directory then, although file name for socket file is fixed, its path
is unique.

cdist/config.py

@@ -27,6 +27,7 @@ import sys
 import time
 import pprint
 import itertools
+import tempfile
 
 import cdist
 
@@ -36,6 +37,36 @@ import cdist.exec.remote
 from cdist import core
 
 
+def inspect_ssh_mux_opts():
+    """Inspect whether or not ssh supports multiplexing options.
+
+       Return string containing multiplexing options if supported.
+       If ControlPath is supported then placeholder for that path is
+       specified and can be used for final string formatting.
+       For example, this function can return string:
+       "-o ControlMaster=auto -o ControlPersist=125 -o ControlPath={}".
+       Then it can be formatted:
+       mux_opts_string.format('/tmp/tmpxxxxxx/ssh-control-path').
+    """
+    import subprocess
+
+    wanted_mux_opts = {
+        "ControlPath": "{}",
+        "ControlMaster": "auto",
+        "ControlPersist": "125",
+    }
+    mux_opts = " ".join([" -o {}={}".format(
+        x, wanted_mux_opts[x]) for x in wanted_mux_opts])
+    try:
+        subprocess.check_output("ssh {}".format(mux_opts),
+                                stderr=subprocess.STDOUT, shell=True)
+    except subprocess.CalledProcessError as e:
+        subproc_output = e.output.decode().lower()
+        if "bad configuration option" in subproc_output:
+            return ""
+    return mux_opts
+
+
 class Config(object):
     """Cdist main class to hold arbitrary data"""
 
@@ -94,7 +125,6 @@ class Config(object):
         initial_manifest_tempfile = None
         if args.manifest == '-':
             # read initial manifest from stdin
-            import tempfile
             try:
                 handle, initial_manifest_temp_path = tempfile.mkstemp(
                         prefix='cdist.stdin.')
@@ -112,18 +142,47 @@ class Config(object):
         failed_hosts = []
         time_start = time.time()
 
+        # default remote cmd patterns
+        args.remote_exec_pattern = None
+        args.remote_copy_pattern = None
+
+        args_dict = vars(args)
+        # if remote-exec and/or remote-copy args are None then user
+        # didn't specify command line options nor env vars:
+        # inspect multiplexing options for default cdist.REMOTE_COPY/EXEC
+        if (args_dict['remote_copy'] is None or
+                args_dict['remote_exec'] is None):
+            mux_opts = inspect_ssh_mux_opts()
+            if args_dict['remote_exec'] is None:
+                args.remote_exec_pattern = cdist.REMOTE_EXEC + mux_opts
+            if args_dict['remote_copy'] is None:
+                args.remote_copy_pattern = cdist.REMOTE_COPY + mux_opts
+
+        if args.out_path:
+            base_root_path = args.out_path
+        else:
+            base_root_path = tempfile.mkdtemp()
+
         hostcnt = 0
         for host in itertools.chain(cls.hosts(args.host),
                                     cls.hosts(args.hostfile)):
+            hostdir = cdist.str_hash(host)
+            host_base_path = os.path.join(base_root_path, hostdir)
+
+            log.debug("Base root path for target host \"{}\" is \"{}\"".format(
+                host, host_base_path))
+
             hostcnt += 1
             if args.parallel:
                 log.debug("Creating child process for %s", host)
                 process[host] = multiprocessing.Process(
-                        target=cls.onehost, args=(host, args, True))
+                        target=cls.onehost,
+                        args=(host, host_base_path, hostdir, args, True))
                 process[host].start()
             else:
                 try:
-                    cls.onehost(host, args, parallel=False)
+                    cls.onehost(host, host_base_path, hostdir,
+                                args, parallel=False)
                 except cdist.Error as e:
                     failed_hosts.append(host)
 
@@ -145,22 +204,42 @@ class Config(object):
                               " ".join(failed_hosts))
 
     @classmethod
-    def onehost(cls, host, args, parallel):
+    def onehost(cls, host, host_base_path, host_dir_name, args, parallel):
         """Configure ONE system"""
 
         log = logging.getLogger(host)
 
         try:
+            control_path = os.path.join(host_base_path, "ssh-control-path")
+            # If we constructed patterns for remote commands then there is
+            # placeholder for ssh ControlPath, format it and we have unique
+            # ControlPath for each host.
+            #
+            # If not then use args.remote_exec/copy that user specified.
+            if args.remote_exec_pattern:
+                remote_exec = args.remote_exec_pattern.format(control_path)
+            else:
+                remote_exec = args.remote_exec
+            if args.remote_copy_pattern:
+                remote_copy = args.remote_copy_pattern.format(control_path)
+            else:
+                remote_copy = args.remote_copy
+            log.debug("remote_exec for host \"{}\": {}".format(
+                host, remote_exec))
+            log.debug("remote_copy for host \"{}\": {}".format(
+                host, remote_copy))
+
             local = cdist.exec.local.Local(
                 target_host=host,
+                base_root_path=host_base_path,
+                host_dir_name=host_dir_name,
                 initial_manifest=args.manifest,
-                base_path=args.out_path,
                 add_conf_dirs=args.conf_dir)
 
             remote = cdist.exec.remote.Remote(
                 target_host=host,
-                remote_exec=args.remote_exec,
+                remote_exec=remote_exec,
-                remote_copy=args.remote_copy)
+                remote_copy=remote_copy)
 
             c = cls(local, remote, dry_run=args.dry_run)
             c.run()

cdist/exec/local.py

@@ -29,7 +29,6 @@ import subprocess
 import shutil
 import logging
 import tempfile
-import hashlib
 
 import cdist
 import cdist.message
@@ -48,40 +47,25 @@ class Local(object):
     """
     def __init__(self,
                  target_host,
+                 base_root_path,
+                 host_dir_name,
                  exec_path=sys.argv[0],
                  initial_manifest=None,
-                 base_path=None,
                  add_conf_dirs=None):
 
         self.target_host = target_host
-        self._init_log()
+        self.hostdir = host_dir_name
-
+        self.base_path = os.path.join(base_root_path, "data")
-        # FIXME: stopped: create base that does not require moving later
-        if base_path:
-            base_path_parent = base_path
-        else:
-            base_path_parent = tempfile.mkdtemp()
-            # TODO: the below atexit hook nukes any debug info we would have
-            #  if cdist exits with error.
-            # import atexit
-            # atexit.register(lambda: shutil.rmtree(base_path_parent))
-        self.hostdir = self._hostdir()
-        self.log.info("Calculated temp dir for target \"{}\" is "
-                      "\"{}\"".format(self.target_host, self.hostdir))
-        self.base_path = os.path.join(base_path_parent, self.hostdir)
-
-        self._init_permissions()
-
-        self.mkdir(self.base_path)
-
-        # FIXME: as well
-        self._init_cache_dir(None)
 
         self.exec_path = exec_path
         self.custom_initial_manifest = initial_manifest
-
         self._add_conf_dirs = add_conf_dirs
 
+        self._init_log()
+        self._init_permissions()
+        self.mkdir(self.base_path)
+        # FIXME: create dir that does not require moving later
+        self._init_cache_dir(None)
         self._init_paths()
         self._init_object_marker()
         self._init_conf_dirs()
@@ -98,12 +82,6 @@ class Local(object):
         else:
             return None
 
-    def _hostdir(self):
-        # Do not assume target_host is anything that can be used as a
-        # directory name.
-        # Instead use a hash, which is known to work as directory name.
-        return hashlib.md5(self.target_host.encode('utf-8')).hexdigest()
-
     def _init_log(self):
         self.log = logging.getLogger(self.target_host)
 

cdist/test/code/__init__.py

@@ -41,10 +41,13 @@ class CodeTestCase(test.CdistTestCase):
 
     def setUp(self):
         self.local_dir = self.mkdtemp()
+        self.hostdir = cdist.str_hash(self.target_host)
+        self.host_base_path = os.path.join(self.local_dir, self.hostdir)
 
         self.local = local.Local(
             target_host=self.target_host,
-            base_path=self.local_dir,
+            base_root_path=self.host_base_path,
+            host_dir_name=self.hostdir,
             exec_path=cdist.test.cdist_exec_path,
             add_conf_dirs=[conf_dir])
         self.local.create_files_dirs()

cdist/test/config/__init__.py

@@ -55,10 +55,13 @@ class ConfigRunTestCase(test.CdistTestCase):
         self.temp_dir = self.mkdtemp()
 
         self.local_dir = os.path.join(self.temp_dir, "local")
-        os.mkdir(self.local_dir)
+        self.hostdir = cdist.str_hash(self.target_host)
+        self.host_base_path = os.path.join(self.local_dir, self.hostdir)
+        os.makedirs(self.host_base_path)
         self.local = cdist.exec.local.Local(
             target_host=self.target_host,
-            base_path=self.local_dir)
+            base_root_path=self.host_base_path,
+            host_dir_name=self.hostdir)
 
         # Setup test objects
         self.object_base_path = op.join(self.temp_dir, 'object')
@@ -161,7 +164,8 @@ class ConfigRunTestCase(test.CdistTestCase):
         """Test if the dryrun option is working like expected"""
         drylocal = cdist.exec.local.Local(
             target_host=self.target_host,
-            base_path=self.local_dir,
+            base_root_path=self.host_base_path,
+            host_dir_name=self.hostdir,
             # exec_path can not derivated from sys.argv in case of unittest
             exec_path=os.path.abspath(os.path.join(
                 my_dir, '../../../scripts/cdist')),
@@ -184,3 +188,8 @@ class ConfigRunTestCase(test.CdistTestCase):
 #        first.requirements = ['__singleton_test/foo']
 #        with self.assertRaises(cdist.core.?????):
 #            self.config.iterate_until_finished()
+
+if __name__ == "__main__":
+    import unittest
+
+    unittest.main()

cdist/test/emulator/__init__.py

@@ -48,10 +48,13 @@ class EmulatorTestCase(test.CdistTestCase):
         handle, self.script = self.mkstemp(dir=self.temp_dir)
         os.close(handle)
         base_path = self.temp_dir
+        hostdir = cdist.str_hash(self.target_host)
+        host_base_path = os.path.join(base_path, hostdir)
 
         self.local = local.Local(
             target_host=self.target_host,
-            base_path=base_path,
+            base_root_path=host_base_path,
+            host_dir_name=hostdir,
             exec_path=test.cdist_exec_path,
             add_conf_dirs=[conf_dir])
         self.local.create_files_dirs()
@@ -148,10 +151,13 @@ class EmulatorConflictingRequirementsTestCase(test.CdistTestCase):
         handle, self.script = self.mkstemp(dir=self.temp_dir)
         os.close(handle)
         base_path = self.temp_dir
+        hostdir = cdist.str_hash(self.target_host)
+        host_base_path = os.path.join(base_path, hostdir)
 
         self.local = local.Local(
             target_host=self.target_host,
-            base_path=base_path,
+            base_root_path=host_base_path,
+            host_dir_name=hostdir,
             exec_path=test.cdist_exec_path,
             add_conf_dirs=[conf_dir])
         self.local.create_files_dirs()
@@ -235,10 +241,13 @@ class AutoRequireEmulatorTestCase(test.CdistTestCase):
     def setUp(self):
         self.temp_dir = self.mkdtemp()
         base_path = os.path.join(self.temp_dir, "out")
+        hostdir = cdist.str_hash(self.target_host)
+        host_base_path = os.path.join(base_path, hostdir)
 
         self.local = local.Local(
             target_host=self.target_host,
-            base_path=base_path,
+            base_root_path=host_base_path,
+            host_dir_name=hostdir,
             exec_path=test.cdist_exec_path,
             add_conf_dirs=[conf_dir])
         self.local.create_files_dirs()
@@ -265,10 +274,13 @@ class OverrideTestCase(test.CdistTestCase):
         handle, self.script = self.mkstemp(dir=self.temp_dir)
         os.close(handle)
         base_path = self.temp_dir
+        hostdir = cdist.str_hash(self.target_host)
+        host_base_path = os.path.join(base_path, hostdir)
 
         self.local = local.Local(
             target_host=self.target_host,
-            base_path=base_path,
+            base_root_path=host_base_path,
+            host_dir_name=hostdir,
             exec_path=test.cdist_exec_path,
             add_conf_dirs=[conf_dir])
         self.local.create_files_dirs()
@@ -303,12 +315,15 @@ class ArgumentsTestCase(test.CdistTestCase):
     def setUp(self):
         self.temp_dir = self.mkdtemp()
         base_path = self.temp_dir
+        hostdir = cdist.str_hash(self.target_host)
+        host_base_path = os.path.join(base_path, hostdir)
         handle, self.script = self.mkstemp(dir=self.temp_dir)
         os.close(handle)
 
         self.local = local.Local(
             target_host=self.target_host,
-            base_path=base_path,
+            base_root_path=host_base_path,
+            host_dir_name=hostdir,
             exec_path=test.cdist_exec_path,
             add_conf_dirs=[conf_dir])
         self.local.create_files_dirs()
@@ -425,10 +440,13 @@ class StdinTestCase(test.CdistTestCase):
 
         self.temp_dir = self.mkdtemp()
         base_path = os.path.join(self.temp_dir, "out")
+        hostdir = cdist.str_hash(self.target_host)
+        host_base_path = os.path.join(base_path, hostdir)
 
         self.local = local.Local(
             target_host=self.target_host,
-            base_path=base_path,
+            base_root_path=host_base_path,
+            host_dir_name=hostdir,
             exec_path=test.cdist_exec_path,
             add_conf_dirs=[conf_dir])
 

cdist/test/exec/local.py

@@ -47,11 +47,14 @@ class LocalTestCase(test.CdistTestCase):
         target_host = 'localhost'
         self.temp_dir = self.mkdtemp()
         self.out_parent_path = self.temp_dir
-        self.out_path = op.join(self.out_parent_path, target_host)
+        self.hostdir = cdist.str_hash(target_host)
+        self.host_base_path = op.join(self.out_parent_path, self.hostdir)
+        self.out_path = op.join(self.host_base_path, "data")
 
         self.local = local.Local(
             target_host=target_host,
-            base_path=self.out_parent_path,
+            base_root_path=self.host_base_path,
+            host_dir_name=self.hostdir,
             exec_path=test.cdist_exec_path
         )
 
@@ -109,7 +112,8 @@ class LocalTestCase(test.CdistTestCase):
 
         link_test_local = local.Local(
             target_host='localhost',
-            base_path=self.out_parent_path,
+            base_root_path=self.host_base_path,
+            host_dir_name=self.hostdir,
             exec_path=test.cdist_exec_path,
         )
 
@@ -127,7 +131,8 @@ class LocalTestCase(test.CdistTestCase):
 
         link_test_local = local.Local(
             target_host='localhost',
-            base_path=self.out_parent_path,
+            base_root_path=self.host_base_path,
+            host_dir_name=self.hostdir,
             exec_path=test.cdist_exec_path,
             add_conf_dirs=[conf_dir]
         )
@@ -148,7 +153,8 @@ class LocalTestCase(test.CdistTestCase):
 
         link_test_local = local.Local(
             target_host='localhost',
-            base_path=self.out_parent_path,
+            base_root_path=self.host_base_path,
+            host_dir_name=self.hostdir,
             exec_path=test.cdist_exec_path,
         )
 

cdist/test/explorer/__init__.py

@@ -42,12 +42,15 @@ class ExplorerClassTestCase(test.CdistTestCase):
     def setUp(self):
         self.temp_dir = self.mkdtemp()
         self.local_path = os.path.join(self.temp_dir, "local")
+        hostdir = cdist.str_hash(self.target_host)
+        base_root_path = os.path.join(self.local_path, hostdir)
         self.remote_base_path = os.path.join(self.temp_dir, "remote")
         os.makedirs(self.remote_base_path)
 
         self.local = local.Local(
             target_host=self.target_host,
-            base_path=self.local_path,
+            base_root_path=base_root_path,
+            host_dir_name=hostdir,
             exec_path=test.cdist_exec_path,
             add_conf_dirs=[conf_dir],
             )

cdist/test/manifest/__init__.py

@@ -49,9 +49,12 @@ class ManifestTestCase(test.CdistTestCase):
         self.temp_dir = self.mkdtemp()
 
         out_path = self.temp_dir
+        hostdir = cdist.str_hash(self.target_host)
+        base_root_path = os.path.join(out_path, hostdir)
         self.local = local.Local(
             target_host=self.target_host,
-            base_path=out_path,
+            base_root_path=base_root_path,
+            host_dir_name=hostdir,
             exec_path=cdist.test.cdist_exec_path,
             add_conf_dirs=[conf_dir])
         self.local.create_files_dirs()

scripts/cdist

@@ -21,31 +21,6 @@
 #
 #
 
-def inspect_ssh_mux_opts(control_path_dir="~/.ssh/"):
-    """Inspect whether or not ssh supports multiplexing options"""
-    import subprocess
-    import os
-
-    # socket is always local to each cdist run, it is created in
-    # temp directory that is created at starting cdist, so 
-    # control_path file name can be static, it will always be
-    # unique due to unique temp directory name
-    control_path = os.path.join(control_path_dir, "ssh-control-path")
-    wanted_mux_opts = {
-        "ControlPath": control_path,
-        "ControlMaster": "auto",
-        "ControlPersist": "125",
-    }
-    mux_opts = " ".join([" -o {}={}".format(x,
-        wanted_mux_opts[x]) for x in wanted_mux_opts])
-    try:
-        subprocess.check_output("ssh {}".format(mux_opts),
-                stderr=subprocess.STDOUT, shell=True)
-    except subprocess.CalledProcessError as e:
-        subproc_output = e.output.decode().lower()
-        if "bad configuration option" in subproc_output:
-            return ""
-    return mux_opts
 
 def commandline():
     """Parse command line"""
@@ -54,7 +29,6 @@ def commandline():
     import cdist.banner
     import cdist.config
     import cdist.shell
-    import tempfile
     import shutil
     import os
 
@@ -62,63 +36,71 @@ def commandline():
     parser = {}
     # Options _all_ parsers have in common
     parser['loglevel'] = argparse.ArgumentParser(add_help=False)
-    parser['loglevel'].add_argument('-d', '--debug',
+    parser['loglevel'].add_argument(
-        help='Set log level to debug', action='store_true',
+            '-d', '--debug', help='Set log level to debug',
-        default=False)
+            action='store_true', default=False)
-    parser['loglevel'].add_argument('-v', '--verbose',
+    parser['loglevel'].add_argument(
-        help='Set log level to info, be more verbose',
+            '-v', '--verbose', help='Set log level to info, be more verbose',
             action='store_true', default=False)
 
     # Main subcommand parser
-    parser['main'] = argparse.ArgumentParser(description='cdist '
+    parser['main'] = argparse.ArgumentParser(
-            + cdist.VERSION,
+            description='cdist ' + cdist.VERSION, parents=[parser['loglevel']])
-        parents=[parser['loglevel']])
+    parser['main'].add_argument(
-    parser['main'].add_argument('-V', '--version',
+            '-V', '--version', help='Show version', action='version',
-        help='Show version', action='version',
             version='%(prog)s ' + cdist.VERSION)
-    parser['sub'] = parser['main'].add_subparsers(title="Commands",
+    parser['sub'] = parser['main'].add_subparsers(
-            dest="command")
+            title="Commands", dest="command")
 
     # Banner
-    parser['banner'] = parser['sub'].add_parser('banner', 
+    parser['banner'] = parser['sub'].add_parser(
-        parents=[parser['loglevel']])
+            'banner', parents=[parser['loglevel']])
     parser['banner'].set_defaults(func=cdist.banner.banner)
 
     # Config
-    parser['config'] = parser['sub'].add_parser('config',
+    parser['config'] = parser['sub'].add_parser(
-        parents=[parser['loglevel']])
+            'config', parents=[parser['loglevel']])
-    parser['config'].add_argument('host', nargs='*',
+    parser['config'].add_argument(
-        help='host(s) to operate on')
+            'host', nargs='*', help='host(s) to operate on')
-    parser['config'].add_argument('-c', '--conf-dir',
+    parser['config'].add_argument(
+            '-c', '--conf-dir',
             help=('Add configuration directory (can be repeated, '
                   'last one wins)'), action='append')
-    parser['config'].add_argument('-f', '--file',
+    parser['config'].add_argument(
+            '-f', '--file',
             help=('Read additional hosts to operate on from specified file '
                   'or from stdin if \'-\' (each host on separate line). '
                   'If no host or host file is specified then, by default, '
                   'read hosts from stdin.'),
             dest='hostfile', required=False)
-    parser['config'].add_argument('-i', '--initial-manifest', 
+    parser['config'].add_argument(
+           '-i', '--initial-manifest',
            help='Path to a cdist manifest or \'-\' to read from stdin.',
            dest='manifest', required=False)
-    parser['config'].add_argument('-n', '--dry-run',
+    parser['config'].add_argument(
+           '-n', '--dry-run',
            help='Do not execute code', action='store_true')
-    parser['config'].add_argument('-o', '--out-dir',
+    parser['config'].add_argument(
+           '-o', '--out-dir',
            help='Directory to save cdist output in', dest="out_path")
-    parser['config'].add_argument('-p', '--parallel',
+    parser['config'].add_argument(
+           '-p', '--parallel',
            help='Operate on multiple hosts in parallel',
            action='store_true', dest='parallel')
-    parser['config'].add_argument('-s', '--sequential',
+    parser['config'].add_argument(
+           '-s', '--sequential',
            help='Operate on multiple hosts sequentially (default)',
            action='store_false', dest='parallel')
     # remote-copy and remote-exec defaults are environment variables
     # if set; if not then None - these will be futher handled after
     # parsing to determine implementation default
-    parser['config'].add_argument('--remote-copy',
+    parser['config'].add_argument(
+           '--remote-copy',
            help='Command to use for remote copy (should behave like scp)',
            action='store', dest='remote_copy',
            default=os.environ.get('CDIST_REMOTE_COPY'))
-    parser['config'].add_argument('--remote-exec',
+    parser['config'].add_argument(
+           '--remote-exec',
            help=('Command to use for remote execution '
                  '(should behave like ssh)'),
            action='store', dest='remote_exec',
@@ -126,15 +108,16 @@ def commandline():
     parser['config'].set_defaults(func=cdist.config.Config.commandline)
 
     # Shell
-    parser['shell'] = parser['sub'].add_parser('shell', 
+    parser['shell'] = parser['sub'].add_parser(
-        parents=[parser['loglevel']])
+            'shell', parents=[parser['loglevel']])
-    parser['shell'].add_argument('-s', '--shell',
+    parser['shell'].add_argument(
+            '-s', '--shell',
             help='Select shell to use, defaults to current shell')
     parser['shell'].set_defaults(func=cdist.shell.Shell.commandline)
 
-
     for p in parser:
-        parser[p].epilog = "Get cdist at http://www.nico.schottelius.org/software/cdist/"
+        parser[p].epilog = (
+                "Get cdist at http://www.nico.schottelius.org/software/cdist/")
 
     args = parser['main'].parse_args(sys.argv[1:])
 
@@ -143,26 +126,6 @@ def commandline():
         logging.root.setLevel(logging.INFO)
     if args.debug:
         logging.root.setLevel(logging.DEBUG)
-    args_dict = vars(args)
-    # if command with remote_copy and remote_exec params
-    if 'remote_copy' in args_dict and 'remote_exec' in args_dict:
-        # if remote-exec and/or remote-copy args are None then user
-        # didn't specify command line options nor env vars:
-        # inspect multiplexing options for default cdist.REMOTE_COPY/EXEC
-        if args_dict['remote_copy'] is None or args_dict['remote_exec'] is None:
-            control_path_dir = tempfile.mkdtemp(prefix="cdist")
-            import atexit
-            atexit.register(lambda: shutil.rmtree(control_path_dir))
-            mux_opts = inspect_ssh_mux_opts(control_path_dir)
-            if args_dict['remote_exec'] is None:
-                args.remote_exec = cdist.REMOTE_EXEC + mux_opts
-            if args_dict['remote_copy'] is None:
-                args.remote_copy = cdist.REMOTE_COPY + mux_opts
-
-    if args.command == 'config':
-        if args.manifest == '-' and args.hostfile == '-':
-            print('cdist config: error: cannot read both, manifest and host file, from stdin')
-            sys.exit(1)
 
     log.debug(args)
     log.info("version %s" % cdist.VERSION)
@@ -194,7 +157,6 @@ if __name__ == "__main__":
               ' is required on the source host.', file=sys.stderr)
         sys.exit(1)
 
-
     exit_code = 0
 
     try: