Cleanups to synchronize including:

* Better comments
* Reorganize code so related settings are close to each other
* Add ::1 to the "localhost" patterns we look for
* Make the dest_port parameter override the ansible_ssh_port setting
* Fix dest_port (wasn't being set)
* more complete detection of delegate_to
* Fix set_remote_user (wasn't being looked for in parameters)
* Instead of removing mode here, have the ansible module accept it
  (better documents the parameters doing it htat way)

CHANGELOG.md

@@ -18,7 +18,7 @@ Major Changes:
  * big_ip modules now support turning off ssl certificate validation (use only for self signed)
  * template code now retains types for bools and numbers instead of turning them into strings.
    If you need the old behaviour, quote the value and it will get passed around as a string
- * Consiidated code from modules using urllib2 to normalize features, TLS and SNI support
+ * Consolidated code from modules using urllib2 to normalize features, TLS and SNI support
 
 Deprecated Modules (new ones in parens):
   * ec2_ami_search (ec2_ami_find)
@@ -150,6 +150,8 @@ New Inventory scripts:
 
 Other Notable Changes:
 
+* synchronize module's dest_port parameter now takes precedence over the ansible_ssh_port inventory setting
+
 ## 1.9.2 "Dancing In the Street" - Jun 26, 2015
 
 * Security fixes to check that hostnames match certificates with https urls (CVE-2015-3908)

lib/ansible/plugins/action/synchronize.py

@@ -18,7 +18,6 @@
 from __future__ import (absolute_import, division, print_function)
 __metaclass__ = type
 
-import sys
 import os.path
 
 from ansible.plugins.action import ActionBase
@@ -77,7 +76,7 @@ class ActionModule(ActionBase):
             if key.startswith("ansible_") and key.endswith("_interpreter"):
                 del task_vars[key]
 
-        # Add the definition from localhost
+        # Add the definitions from localhost
         localhost = task_vars['hostvars']['localhost']
         if 'ansible_syslog_facility' in localhost:
             task_vars['ansible_syslog_facility'] = localhost['ansible_syslog_facility']
@@ -89,7 +88,10 @@ class ActionModule(ActionBase):
         ''' generates params and passes them on to the rsync module '''
 
         original_transport = task_vars.get('ansible_connection') or self._play_context.connection
-        transport_overridden = False
+        remote_transport = False
+        if original_transport != 'local':
+            remote_transport = True
+
         try:
             delegate_to = self._play_context.delegate_to
         except (AttributeError, KeyError):
@@ -100,74 +102,65 @@ class ActionModule(ActionBase):
         # Parameter name needed by the ansible module
         self._task.args['_local_rsync_path'] = task_vars.get('ansible_rsync_path') or 'rsync'
 
-        # from the perspective of the rsync call the delegate is the localhost
+        # rsync thinks that one end of the connection is localhost and the
+        # other is the host we're running the task for  (Note: We use
+        # ansible's delegate_to mechanism to determine which host rsync is
+        # running on so localhost could be a non-controller machine if
+        # delegate_to is used)
         src_host = '127.0.0.1'
         dest_host = task_vars.get('ansible_ssh_host') or task_vars.get('inventory_hostname')
 
-        ### FIXME: do we still need to explicitly template ansible_ssh_host here in v2?
+        dest_is_local = dest_host in ('127.0.0.1', 'localhost', '::1')
-
-        dest_is_local = dest_host in ['127.0.0.1', 'localhost']
-
 
         # CHECK FOR NON-DEFAULT SSH PORT
-        dest_port = task_vars.get('ansible_ssh_port') or self._task.args.get('dest_port') or 22
+        if self._task.args.get('dest_port', None) is None:
+            inv_port = task_vars.get('ansible_ssh_port', None) or constants.DEFAULT_REMOTE_PORT
+            if inv_port is not None:
+                self._task.args['dest_port'] = inv_port
 
-        # CHECK DELEGATE HOST INFO
+        # Set use_delegate if we are going to run rsync on a delegated host
+        # instead of localhost
         use_delegate = False
-
         if dest_host == delegate_to:
             # edge case: explicit delegate and dest_host are the same
             # so we run rsync on the remote machine targetting its localhost
             # (itself)
             dest_host = '127.0.0.1'
             use_delegate = True
-        else:
+        elif delegate_to is not None and remote_transport:
-            if 'hostvars' in task_vars:
+            # If we're delegating to a remote host then we need to use the
-                if delegate_to in task_vars['hostvars'] and original_transport != 'local':
+            # delegate_to settings
-                    # use a delegate host instead of localhost
             use_delegate = True
 
-        # COMPARE DELEGATE, HOST AND TRANSPORT
-        process_args = False
-        if dest_host != src_host and original_transport != 'local':
-            # interpret and task_vars remote host info into src or dest
-            process_args = True
-
-        # SWITCH SRC AND DEST PER MODE
-        if self._task.args.get('mode', 'push') == 'pull':
-            (dest_host, src_host) = (src_host, dest_host)
-
         # Delegate to localhost as the source of the rsync unless we've been
         # told (via delegate_to) that a different host is the source of the
         # rsync
         transport_overridden = False
-        if not use_delegate and original_transport != 'local':
+        if not use_delegate and remote_transport:
             # Create a connection to localhost to run rsync on
             new_stdin = self._connection._new_stdin
             new_connection = connection_loader.get('local', self._play_context, new_stdin)
             self._connection = new_connection
             transport_overridden = True
             self._override_module_replaced_vars(task_vars)
-            ### FIXME: We think that this was here for v1 because the local
-            # connection didn't support sudo.  In v2 it does so we think it's
-            # safe to remove this now.
 
-            # Also disable sudo
+        # COMPARE DELEGATE, HOST AND TRANSPORT
-            #self._play_context.become = False
+        process_args = False
+        if dest_host != src_host and remote_transport:
+            # We're not copying two filesystem trees on the same host so we
+            # need to correctly format the paths for rsync (like
+            # user@host:path/to/tree
+            process_args = True
+
+        # SWITCH SRC AND DEST HOST PER MODE
+        if self._task.args.get('mode', 'push') == 'pull':
+            (dest_host, src_host) = (src_host, dest_host)
 
         # MUNGE SRC AND DEST PER REMOTE_HOST INFO
         src = self._task.args.get('src', None)
         dest = self._task.args.get('dest', None)
         if process_args or use_delegate:
-
+            # Private key handling
-            user = None
-            if boolean(task_vars.get('set_remote_user', 'yes')):
-                if use_delegate:
-                    user = task_vars['hostvars'][delegate_to].get('ansible_ssh_user')
-
-                if not use_delegate or not user:
-                    user = task_vars.get('ansible_ssh_user') or self._play_context.remote_user
-
             if use_delegate:
                 private_key = task_vars.get('ansible_ssh_private_key_file') or self._play_context.private_key_file
             else:
@@ -177,6 +170,17 @@ class ActionModule(ActionBase):
                 private_key = os.path.expanduser(private_key)
                 self._task.args['private_key'] = private_key
 
+            # Src and dest rsync "path" handling
+            # Determine if we need a user@
+            user = None
+            if boolean(self._task.args.get('set_remote_user', 'yes')):
+                if use_delegate:
+                    if 'hostvars' in task_vars and delegate_to in task_vars['hostvars']:
+                        user = task_vars['hostvars'][delegate_to].get('ansible_ssh_user', None)
+
+                if not use_delegate or not user:
+                    user = task_vars.get('ansible_ssh_user') or self._play_context.remote_user
+
             # use the mode to define src and dest's url
             if self._task.args.get('mode', 'push') == 'pull':
                 # src is a remote path: <user>@<host>, dest is a local path
@@ -190,14 +194,10 @@ class ActionModule(ActionBase):
         self._task.args['src'] = src
         self._task.args['dest'] = dest
 
-        # Remove mode as it is handled purely in this action module
+        # Allow custom rsync path argument
-        if 'mode' in self._task.args:
-            del self._task.args['mode']
-
-        # Allow custom rsync path argument.
         rsync_path = self._task.args.get('rsync_path', None)
 
-        # If no rsync_path is set, sudo was originally set, and dest is remote then add 'sudo rsync' argument.
+        # If no rsync_path is set, sudo was originally set, and dest is remote then add 'sudo rsync' argument
         if not rsync_path and transport_overridden and self._play_context.become and self._play_context.become_method == 'sudo' and not dest_is_local:
             rsync_path = 'sudo rsync'