From 9250e459497d1dc5212a073cafa172d4eed07d35 Mon Sep 17 00:00:00 2001 From: s-hertel <19572925+s-hertel@users.noreply.github.com> Date: Thu, 14 Nov 2024 21:38:59 -0500 Subject: [PATCH] WIP - add diff mode support to the copy module --- lib/ansible/modules/copy.py | 108 ++++++- test/integration/targets/copy/tasks/diff.yml | 285 +++++++++++++++++++ test/integration/targets/copy/tasks/main.yml | 15 +- 3 files changed, 383 insertions(+), 25 deletions(-) create mode 100644 test/integration/targets/copy/tasks/diff.yml diff --git a/lib/ansible/modules/copy.py b/lib/ansible/modules/copy.py index da458732a2c..b9725d2937a 100644 --- a/lib/ansible/modules/copy.py +++ b/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) - return module.set_group_if_different(path, group, changed) + changed = False + 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) - module.set_group_if_different(b_dest_item_path, group, False) + chown_path(module, dest_item_path, owner, group) 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) diff --git a/test/integration/targets/copy/tasks/diff.yml b/test/integration/targets/copy/tasks/diff.yml new file mode 100644 index 00000000000..068fdfc6b57 --- /dev/null +++ b/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" diff --git a/test/integration/targets/copy/tasks/main.yml b/test/integration/targets/copy/tasks/main.yml index d46b783d746..efc0c6a077f 100644 --- a/test/integration/targets/copy/tasks/main.yml +++ b/test/integration/targets/copy/tasks/main.yml @@ -91,20 +91,7 @@ - import_tasks: check_mode.yml - # 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: 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' + - import_tasks: diff.yml - name: tests with remote_src and non files import_tasks: src_remote_file_is_not_file.yml