WIP - add diff mode support to the copy module

lib/ansible/modules/copy.py

@@ -293,9 +293,54 @@ import stat
 import tempfile
 import traceback
 
-from ansible.module_utils.common.text.converters import to_bytes, to_native
+from ansible.module_utils.common.text.converters import to_bytes, to_native, to_text
 from ansible.module_utils.basic import AnsibleModule
 
+DIFF_BY_PATH = {}
+
+
+def generate_dirnames(path):
+    """Yield the parent directories from an absolute path until the dirname is /."""
+    while path not in (os.path.sep):
+        path = os.path.dirname(path)
+        yield path
+
+
+def update_diff(path, diff):
+    """Update the global diff cache for the provided path."""
+    if not diff:
+        return
+
+    # TODO: module is passing around bytes in some places, text in others.
+    path = to_text(os.path.abspath(path))
+
+    # The 'before' needs to be settable, but not overwritable, in order to preserve the real before state for cumulative updates.
+    # TODO: Simplify once callers are unified.
+
+    # initialize DIFF_BY_PATH[path] and add/update before
+    if path not in DIFF_BY_PATH:
+        DIFF_BY_PATH[path] = {"before": {}, "after": {"dest": path}}
+        # new or updated?
+        if any(
+            bool(parent_path in DIFF_BY_PATH and DIFF_BY_PATH[parent_path]["before"]["dest"] is None)
+            for parent_path in generate_dirnames(path)
+        ):
+            # new by extension
+            DIFF_BY_PATH[path]["before"]["dest"] = None
+        else:
+            # maybe existed, but let the caller override
+            DIFF_BY_PATH[path]["before"]["dest"] = path
+            DIFF_BY_PATH[path]["before"].update(diff["before"])
+    else:
+        for key in diff["before"]:
+            # preserve the earliest
+            if key in DIFF_BY_PATH[path]["before"]:
+                continue
+            DIFF_BY_PATH[path]["before"][key] = diff["before"][key]
+
+    # update after to preserve latest
+    DIFF_BY_PATH[path]["after"].update(diff["after"])
+
 
 class AnsibleModuleError(Exception):
     def __init__(self, results):
@@ -328,15 +373,21 @@ def adjust_recursive_directory_permissions(pre_existing_dir, new_directory_list,
     if new_directory_list:
         working_dir = os.path.join(pre_existing_dir, new_directory_list.pop(0))
         directory_args['path'] = working_dir
-        changed = module.set_fs_attributes_if_different(directory_args, changed)
+        _diff = {}
+        changed = module.set_fs_attributes_if_different(directory_args, changed, _diff)
+        update_diff(working_dir, _diff)
         changed = adjust_recursive_directory_permissions(working_dir, new_directory_list, module, directory_args, changed)
     return changed
 
 
 def chown_path(module, path, owner, group):
     """Update the owner/group if specified and different from the current owner/group."""
-    changed = module.set_owner_if_different(path, owner, False)
+    changed = False
-    return module.set_group_if_different(path, group, changed)
+    for func, apply_owner in [(module.set_owner_if_different, owner), (module.set_group_if_different, group)]:
+        _diff = {}
+        changed = func(path, apply_owner, changed, _diff)
+        update_diff(path, _diff)
+    return changed
 
 
 def chown_recursive(path, module):
@@ -365,6 +416,13 @@ def copy_diff_files(src, dest, module):
     diff_files = filecmp.dircmp(src, dest).diff_files
     if len(diff_files):
         changed = True
+    if changed and module._diff:
+        for file in diff_files:
+            _diff = {
+                'before': {'lines': open(os.path.join(dest, file), 'r').read().splitlines()},
+                'after': {'lines': open(os.path.join(src, file), 'r').read().splitlines()}
+            }
+            update_diff(os.path.join(dest, file), _diff)
     if not module.check_mode:
         for item in diff_files:
             src_item_path = os.path.join(src, item)
@@ -378,8 +436,7 @@ def copy_diff_files(src, dest, module):
                 shutil.copyfile(b_src_item_path, b_dest_item_path)
                 shutil.copymode(b_src_item_path, b_dest_item_path)
 
-            module.set_owner_if_different(b_dest_item_path, owner, False)
+            chown_path(module, dest_item_path, owner, group)
-            module.set_group_if_different(b_dest_item_path, group, False)
             changed = True
     return changed
 
@@ -394,6 +451,10 @@ def copy_left_only(src, dest, module):
     left_only = filecmp.dircmp(src, dest).left_only
     if len(left_only):
         changed = True
+    if changed and module._diff:
+        for new in left_only:
+            dest_path = os.path.join(dest, new)
+            update_diff(dest_path, {'before': {'dest': None}, 'after': {'dest': dest_path}})
     if not module.check_mode:
         for item in left_only:
             src_item_path = os.path.join(src, item)
@@ -543,8 +604,14 @@ def main():
                 e.result['msg'] += ' Could not copy to {0}'.format(dest)
                 module.fail_json(**e.results)
 
+            create_path = pre_existing_dir
+            for new_dir in new_directory_list:
+                create_path = os.path.join(create_path, new_dir)
+                update_diff(create_path, {'before': {'dest': None}, 'after': {'dest': create_path}})
+
             if module.check_mode:
-                module.exit_json(msg='dest directory %s would be created' % dirname, changed=True, src=src)
+                exit_kwargs = {} if not (module._diff and DIFF_BY_PATH) else {'diff': [DIFF_BY_PATH[modified_content] for modified_content in DIFF_BY_PATH]}
+                module.exit_json(msg='dest directory %s would be created' % dirname, changed=True, src=src, **exit_kwargs)
             os.makedirs(b_dirname)
             changed = True
             directory_args = module.load_file_common_arguments(module.params)
@@ -588,6 +655,19 @@ def main():
 
     backup_file = None
     if checksum_src != checksum_dest or os.path.islink(b_dest):
+        if remote_src:
+            # local source diff is handled by the action plugin
+            if checksum_dest is None and not os.path.islink(b_dest):
+                dest_before = None
+            elif os.path.islink(b_dest):
+                dest_before = to_text(os.path.realpath(b_dest))
+            else:
+                dest_before = to_text(b_dest)
+            _diff = {"before": {"dest": dest_before}, "after": {}}
+            update_diff(b_dest, _diff)
+        else:
+            # TODO: move/simplify action plugin diff handling so it just returns the diffs from the modules it executes
+            pass
 
         if not module.check_mode:
             try:
@@ -649,6 +729,7 @@ def main():
                 b_basename = to_bytes(os.path.basename(src), errors='surrogate_or_strict')
                 b_dest = to_bytes(os.path.join(b_dest, b_basename), errors='surrogate_or_strict')
                 b_src = to_bytes(os.path.join(module.params['src'], ""), errors='surrogate_or_strict')
+                update_diff(b_dest, {"before": {"dest": None}, "after": {}})
                 if not module.check_mode:
                     shutil.copytree(b_src, b_dest, symlinks=not local_follow)
                 chown_recursive(dest, module)
@@ -675,16 +756,16 @@ def main():
             if not src.endswith(os.path.sep) and not os.path.exists(module.params['dest']):
                 b_basename = to_bytes(os.path.basename(module.params['src']), errors='surrogate_or_strict')
                 b_dest = to_bytes(os.path.join(b_dest, b_basename), errors='surrogate_or_strict')
+                if not os.path.exists(b_dest):
+                    update_diff(b_dest, {"before": {"dest": None}, "after": {}})
+                    changed = True
                 if not module.check_mode and not os.path.exists(b_dest):
                     os.makedirs(b_dest)
-                    changed = True
                     b_src = to_bytes(os.path.join(module.params['src'], ""), errors='surrogate_or_strict')
                     diff_files_changed = copy_diff_files(b_src, b_dest, module)
                     left_only_changed = copy_left_only(b_src, b_dest, module)
                     common_dirs_changed = copy_common_dirs(b_src, b_dest, module)
                     owner_group_changed = chown_recursive(b_dest, module)
-                if module.check_mode and not os.path.exists(b_dest):
-                    changed = True
 
     res_args = dict(
         dest=dest, src=src, md5sum=md5sum_src, checksum=checksum_src, changed=changed
@@ -693,7 +774,12 @@ def main():
         res_args['backup_file'] = backup_file
 
     file_args = module.load_file_common_arguments(module.params, path=dest)
-    res_args['changed'] = module.set_fs_attributes_if_different(file_args, res_args['changed'])
+    _diff = {}
+    res_args['changed'] = module.set_fs_attributes_if_different(file_args, res_args['changed'], _diff)
+    update_diff(file_args['path'], _diff)
+
+    if module._diff and DIFF_BY_PATH:
+        res_args['diff'] = [DIFF_BY_PATH[modified_content] for modified_content in DIFF_BY_PATH]
 
     module.exit_json(**res_args)
 

test/integration/targets/copy/tasks/diff.yml

@@ -0,0 +1,285 @@
+- name: Test copy action plugin (remote_src=false) and diff mode
+  block:
+    # https://github.com/ansible/ansible/issues/57618
+    # https://github.com/ansible/ansible/issues/79749
+    - name: Test diff contents
+      copy:
+        content: "Ansible managed\n"
+        dest: "{{ local_temp_dir }}/file.txt"
+      diff: true
+      register: diff_output
+
+    - assert:
+        that:
+          - diff_output.diff[0].before == ""
+          - '"Ansible managed" in diff_output.diff[0].after'
+          - '"file.txt" in diff_output.diff[0].after_header'
+
+- name: Test copy module (remote_src=true) and diff mode
+  vars:
+    test_remote_src: "{{ remote_tmp_dir }}/test_remote_src_diff"
+  block:
+    - name: Setup - create remote directory structure
+      file:
+        path: "{{ test_remote_src }}/{{ item }}"
+        state: directory
+      loop:
+        - ""
+        - ../copied_with_diff
+        - dir1
+
+    - name: Setup - create remote files
+      file:
+        path: "{{ test_remote_src }}/{{ item }}"
+        state: touch
+        mode: "0660"
+      loop:
+        - file1
+        - dir1/file2
+
+    - name: Test - copy remote directory recursively with diff=true (check mode)
+      copy:
+        src: "{{ test_remote_src }}/"
+        dest: "{{ remote_tmp_dir }}/copied_with_diff"
+        mode: preserve
+        remote_src: true
+      diff: true
+      check_mode: true
+      register: diff_copy_check
+
+    - name: Test - copy remote directory recursively with diff=true
+      copy:
+        src: "{{ test_remote_src }}/"
+        dest: "{{ remote_tmp_dir }}/copied_with_diff"
+        mode: preserve
+        remote_src: true
+      diff: true
+      register: diff_copy
+
+    - assert:
+        that:
+          - (diff_copy_check.diff | length) == 2
+          - (diff_copy.diff | length) == 2
+          - (diff_copy_check.diff | map(attribute="before.dest") | unique) == [absent]
+          - (diff_copy_check.diff | map(attribute="after.dest")) == dest_after
+          - (diff_copy.diff | map(attribute="before.dest")| unique) == [absent]
+          - (diff_copy.diff | map(attribute="after.dest")) == dest_after
+      vars:
+        absent: null
+        dest_after:
+          - "{{ remote_tmp_dir }}/copied_with_diff/dir1"
+          - "{{ remote_tmp_dir }}/copied_with_diff/file1"
+
+    - name: Test - validate the nested file was also copied
+      stat:
+        path: "{{ test_remote_src }}/dir1/file2"
+      register: expected_to_exist
+      failed_when: not expected_to_exist.stat.exists
+
+    - name: Setup - modify nested src file content
+      copy:
+        dest: "{{ test_remote_src }}/dir1/file2"
+        content: |
+          line1
+          line2
+
+    - name: Test - copy recursively with modified content and diff=true (check mode)
+      copy:
+        src: "{{ test_remote_src }}/"
+        dest: "{{ remote_tmp_dir }}/copied_with_diff"
+        remote_src: true
+      diff: true
+      check_mode: true
+      register: diff_update_content_check
+
+    - name: Test - copy recursively with modified content and diff=true
+      copy:
+        src: "{{ test_remote_src }}/"
+        dest: "{{ remote_tmp_dir }}/copied_with_diff"
+        remote_src: true
+      diff: true
+      register: diff_update_content
+
+    - assert:
+        that:
+          - (diff_update_content_check.diff | length) == 1
+          - (diff_update_content.diff | length) == 1
+          - diff_update_content_check.diff[0].before.lines == []
+          - diff_update_content_check.diff[0].after.lines == ["line1", "line2"]
+          - diff_update_content.diff[0].before.lines == []
+          - diff_update_content.diff[0].after.lines == ["line1", "line2"]
+
+    # TODO: this test should be updated when https://github.com/ansible/ansible/issues/81126 is fixed
+    # Currently this modified the ownership of everything, not just new content
+    - name: Test - copy recursively with modified ownership and diff=true (check mode)
+      copy:
+        src: "{{ test_remote_src }}/"
+        dest: "{{ remote_tmp_dir }}/copied_with_diff"
+        owner: "{{ remote_unprivileged_user }}"
+        group: "{{ remote_unprivileged_user_group }}"
+        remote_src: true
+      diff: true
+      check_mode: true
+      register: diff_update_ownership_check
+
+    - name: Test - copy recursively with modified ownership and diff=true
+      copy:
+        src: "{{ test_remote_src }}/"
+        dest: "{{ remote_tmp_dir }}/copied_with_diff"
+        owner: "{{ remote_unprivileged_user }}"
+        group: "{{ remote_unprivileged_user_group }}"
+        remote_src: true
+      diff: true
+      check_mode: true
+      register: diff_update_ownership
+
+    - assert:
+        that:
+          - (diff_update_ownership_check.diff | length) == 4
+          - (diff_update_ownership.diff | length) == 4
+          - (diff_update_ownership_check.diff | map(attribute="before.dest")) == expected_dest
+          - (diff_update_ownership_check.diff | map(attribute="after.dest")) == expected_dest
+          - (diff_update_ownership_check.diff | map(attribute="before.owner") | unique) == [0]
+          - (diff_update_ownership_check.diff | map(attribute="after.owner") | unique | length) == 1
+          - (diff_update_ownership_check.diff | map(attribute="after.owner") | unique) != [0]
+          - (diff_update_ownership.diff | map(attribute="before.dest")) == expected_dest
+          - (diff_update_ownership.diff | map(attribute="after.dest")) == expected_dest
+          - (diff_update_ownership.diff | map(attribute="before.owner") | unique) == [0]
+          - (diff_update_ownership.diff | map(attribute="after.owner") | unique | length) == 1
+          - (diff_update_ownership.diff | map(attribute="after.owner") | unique) != [0]
+      vars:
+        expected_dest:
+          - "{{ remote_tmp_dir }}/copied_with_diff"
+          - "{{ remote_tmp_dir }}/copied_with_diff/dir1"
+          - "{{ remote_tmp_dir }}/copied_with_diff/file1"
+          - "{{ remote_tmp_dir }}/copied_with_diff/dir1/file2"
+
+    # TODO: the module does not handle modifying mode recursively (https://github.com/ansible/ansible/issues/81126)
+    # TODO: the module does not handle directory_mode correctly (https://github.com/ansible/ansible/issues/81292)
+    - name: Test - copy with modified mode and diff=true (check mode)
+      copy:
+        src: "{{ test_remote_src }}/file1"
+        dest: "{{ remote_tmp_dir }}/copied_with_diff/file1"
+        mode: "0664"
+        remote_src: true
+      diff: true
+      check_mode: true
+      register: diff_update_mode_check
+
+    - name: Test - copy with modified mode and diff=true
+      copy:
+        src: "{{ test_remote_src }}/file1"
+        dest: "{{ remote_tmp_dir }}/copied_with_diff/file1"
+        mode: "0664"
+        remote_src: true
+      diff: true
+      register: diff_update_mode
+
+    - assert:
+        that:
+          - (diff_update_mode_check.diff | length) == 1
+          - (diff_update_mode.diff | length) == 1
+          - diff_update_mode_check.diff[0].before.mode == "0660"
+          - diff_update_mode_check.diff[0].after.mode == "0664"
+          - diff_update_mode.diff[0].before.mode == "0660"
+          - diff_update_mode.diff[0].after.mode == "0664"
+
+    - name: Setup - remove dest to test intermediate dir creation
+      file:
+        path: "{{ remote_tmp_dir }}/copied_with_diff/"
+        state: absent
+
+    - name: Test - copy source with intermediate directory creation and diff=true (check mode)
+      copy:
+        src: "{{ test_remote_src }}/"
+        dest: "{{ remote_tmp_dir }}/inter1/inter2/"
+        remote_src: true
+      diff: true
+      check_mode: true
+      register: diff_intermediate_dirs_check
+
+    - name: Test - copy source with intermediate directory creation and diff=true
+      copy:
+        src: "{{ test_remote_src }}/"
+        dest: "{{ remote_tmp_dir }}/inter1/inter2/"
+        remote_src: true
+        owner: "{{ remote_unprivileged_user }}"
+        group: "{{ remote_unprivileged_user_group }}"
+      diff: true
+      register: diff_intermediate_dirs
+
+    - assert:
+        that:
+          - (diff_intermediate_dirs_check.diff | length) == 2
+          - (diff_intermediate_dirs.diff | length) == 5
+          - (diff_intermediate_dirs_check.diff | map(attribute="before.dest") | unique) == [absent]
+          - (diff_intermediate_dirs_check.diff | map(attribute="after.dest")) == dest_after_check
+          - (diff_intermediate_dirs.diff | map(attribute="before.dest") | unique) == [absent]
+          - (diff_intermediate_dirs.diff | map(attribute="after.dest")) == dest_after
+      vars:
+        absent: null
+        dest_after_check:
+          - "{{ remote_tmp_dir }}/inter1"
+          - "{{ remote_tmp_dir }}/inter1/inter2"
+        dest_after:
+          - "{{ remote_tmp_dir }}/inter1"
+          - "{{ remote_tmp_dir }}/inter1/inter2"
+          - "{{ remote_tmp_dir }}/inter1/inter2/dir1"
+          - "{{ remote_tmp_dir }}/inter1/inter2/file1"
+          - "{{ remote_tmp_dir }}/inter1/inter2/dir1/file2"
+
+    - name: Setup - add new deeply nested file in the remote src
+      file:
+        path: "{{ test_remote_src }}/{{ item.name }}"
+        state: "{{ item.state }}"
+        mode: "{{ (item.state == 'touch') | ternary('0660', omit) }}"
+      loop:
+        - name: dir2
+          state: directory
+        - name: dir2/subdir1
+          state: directory
+        - name: dir2/subdir1/file3
+          state: touch
+
+    - name: Test - copy recursively with new nested paths and diff=true (check mode)
+      copy:
+        src: "{{ test_remote_src }}/"
+        dest: "{{ remote_tmp_dir }}/inter1/inter2/"
+        remote_src: true
+      diff: true
+      check_mode: true
+      register: diff_new_check
+
+    - name: Test - copy recursively with new nested paths and diff=true
+      copy:
+        src: "{{ test_remote_src }}/"
+        dest: "{{ remote_tmp_dir }}/inter1/inter2/"
+        remote_src: true
+      diff: true
+      register: diff_new
+
+    - assert:
+        that:
+          - (diff_new_check.diff | length ) == 1
+          - (diff_new.diff | length ) == 1
+          - diff_new_check.diff[0]["before"]["dest"] == absent
+          - diff_new_check.diff[0]["after"]["dest"] == expected
+          - diff_new.diff[0]["before"]["dest"] == absent
+          - diff_new.diff[0]["after"]["dest"] == expected
+      vars:
+        absent: null
+        expected: "{{ remote_tmp_dir }}/inter1/inter2/dir2"
+
+    - name: Test - validate the nested file was copied
+      stat:
+        path: "{{ remote_tmp_dir }}/inter1/inter2/dir2/subdir1/file3"
+      register: expected_to_exist
+      failed_when: not expected_to_exist.stat.exists
+  always:
+    - name: Cleanup after setup and test
+      file:
+        path: "{{ remote_tmp_dir }}/{{ item }}"
+        state: absent
+      loop:
+        - "{{ test_remote_src }}"
+        - "{{ remote_tmp_dir }}/copied_with_diff"

test/integration/targets/copy/tasks/main.yml

@@ -91,20 +91,7 @@
 
     - import_tasks: check_mode.yml
 
-    # https://github.com/ansible/ansible/issues/57618
+    - import_tasks: diff.yml
-    # https://github.com/ansible/ansible/issues/79749
-    - name: Test diff contents
-      copy:
-        content: 'Ansible managed\n'
-        dest: "{{ local_temp_dir }}/file.txt"
-      diff: yes
-      register: diff_output
-
-    - assert:
-        that:
-          - 'diff_output.diff[0].before == ""'
-          - '"Ansible managed" in diff_output.diff[0].after'
-          - '"file.txt" in diff_output.diff[0].after_header'
 
     - name: tests with remote_src and non files
       import_tasks: src_remote_file_is_not_file.yml