From f332151f59f8297fabdc2abc25182c8dad9763bf Mon Sep 17 00:00:00 2001 From: Toshio Kuratomi Date: Mon, 6 Nov 2017 09:26:41 -0800 Subject: [PATCH] Fix copy to only follow symlinks for files in the non-recursive case Revert "**Temporary**" This reverts commit 28b86b11485444d2dd46ed60dc8ae27fd2b81303. We don't need this now that copy has been fixed --- .../fragments/copy-files-default-follow.yaml | 4 ++ lib/ansible/modules/files/file.py | 2 +- lib/ansible/plugins/action/copy.py | 17 +++++- test/integration/targets/copy/tasks/main.yml | 4 +- test/integration/targets/copy/tasks/tests.yml | 57 +++++++++++++++---- 5 files changed, 66 insertions(+), 18 deletions(-) create mode 100644 changelogs/fragments/copy-files-default-follow.yaml diff --git a/changelogs/fragments/copy-files-default-follow.yaml b/changelogs/fragments/copy-files-default-follow.yaml new file mode 100644 index 0000000000..0bce044ae7 --- /dev/null +++ b/changelogs/fragments/copy-files-default-follow.yaml @@ -0,0 +1,4 @@ +--- +bugfixes: +- copy - fixed copy to only follow symlinks for files in the non-recursive case +- file - fixed the default follow behaviour of file to be true diff --git a/lib/ansible/modules/files/file.py b/lib/ansible/modules/files/file.py index ce663dd507..997d8401e9 100644 --- a/lib/ansible/modules/files/file.py +++ b/lib/ansible/modules/files/file.py @@ -182,7 +182,7 @@ def main(): original_basename=dict(required=False), # Internal use only, for recursive ops recurse=dict(default=False, type='bool'), force=dict(required=False, default=False, type='bool'), - follow=dict(required=False, default=False, type='bool'), + follow=dict(required=False, default=True, type='bool'), diff_peek=dict(default=None), # Internal use only, for internal checks in the action plugins validate=dict(required=False, default=None), # Internal use only, for template and copy src=dict(required=False, default=None, type='path'), diff --git a/lib/ansible/plugins/action/copy.py b/lib/ansible/plugins/action/copy.py index 961546fa39..5853efc61c 100644 --- a/lib/ansible/plugins/action/copy.py +++ b/lib/ansible/plugins/action/copy.py @@ -192,9 +192,9 @@ class ActionModule(ActionBase): # remove action plugin only keys return dict((k, v) for k, v in module_args.items() if k not in ('content', 'decrypt')) - def _copy_file(self, source_full, source_rel, content, content_tempfile, dest, task_vars): + def _copy_file(self, source_full, source_rel, content, content_tempfile, + dest, task_vars, follow): decrypt = boolean(self._task.args.get('decrypt', True), strict=False) - follow = boolean(self._task.args.get('follow', False), strict=False) force = boolean(self._task.args.get('force', 'yes'), strict=False) raw = boolean(self._task.args.get('raw', 'no'), strict=False) @@ -289,6 +289,7 @@ class ActionModule(ActionBase): src=tmp_src, dest=dest, original_basename=source_rel, + follow=follow ) ) if not self._task.args.get('checksum'): @@ -491,7 +492,14 @@ class ActionModule(ActionBase): for source_full, source_rel in source_files['files']: # copy files over. This happens first as directories that have # a file do not need to be created later - module_return = self._copy_file(source_full, source_rel, content, content_tempfile, dest, task_vars) + + # We only follow symlinks for files in the non-recursive case + if source_files['directories']: + follow = False + else: + follow = boolean(self._task.args.get('follow', False), strict=False) + + module_return = self._copy_file(source_full, source_rel, content, content_tempfile, dest, task_vars, follow) if module_return is None: continue @@ -528,6 +536,9 @@ class ActionModule(ActionBase): new_module_args['src'] = target_path new_module_args['state'] = 'link' new_module_args['force'] = True + # Only follow remote symlinks in the non-recursive case + if source_files['directories']: + new_module_args['follow'] = False module_return = self._execute_module(module_name='file', module_args=new_module_args, task_vars=task_vars) module_executed = True diff --git a/test/integration/targets/copy/tasks/main.yml b/test/integration/targets/copy/tasks/main.yml index 29520447ce..c5f6c03bb4 100644 --- a/test/integration/targets/copy/tasks/main.yml +++ b/test/integration/targets/copy/tasks/main.yml @@ -14,7 +14,7 @@ name: ensure temp dir exists # file cannot do this properly, use command instead - - name: Create ciruclar symbolic link + - name: Create circular symbolic link command: ln -s ../ circles args: chdir: '{{role_path}}/files/subdir/subdir1' @@ -57,7 +57,7 @@ - name: Remove circular symbolic link file: - path: subdir/subdir1/circles + path: '{{ role_path }}/files/subdir/subdir1/circles' state: absent connection: local diff --git a/test/integration/targets/copy/tasks/tests.yml b/test/integration/targets/copy/tasks/tests.yml index 02180451b9..699561c809 100644 --- a/test/integration/targets/copy/tasks/tests.yml +++ b/test/integration/targets/copy/tasks/tests.yml @@ -16,6 +16,10 @@ mode: 0444 register: copy_result +- name: Record the sha of the test file for later tests + set_fact: + remote_file_hash: "{{ copy_result['checksum'] }}" + - name: Check the mode of the output file file: name: "{{ remote_file }}" @@ -173,7 +177,7 @@ - name: Create a hardlink to the file file: src: '{{ remote_file }}' - dest: '{{ output_dir }}/hard.lnk' + dest: '{{ remote_dir }}/hard.lnk' state: hard - name: copy the same contents into place @@ -191,7 +195,7 @@ - name: Check the stat results of the hard link stat: - path: "{{ output_dir }}/hard.lnk" + path: "{{ remote_dir }}/hard.lnk" register: hlink_results - name: Check that the file did not change @@ -216,7 +220,7 @@ - name: Check the stat results of the hard link stat: - path: "{{ output_dir }}/hard.lnk" + path: "{{ remote_dir }}/hard.lnk" register: hlink_results - name: Check that the file changed permissions but is still the same @@ -242,7 +246,7 @@ - name: Check the stat results of the hard link stat: - path: "{{ output_dir }}/hard.lnk" + path: "{{ remote_dir }}/hard.lnk" register: hlink_results - name: Check that the file changed and hardlink was broken @@ -1105,6 +1109,10 @@ remote_user: root +# +# Follow=True tests +# + # test overwriting a link using "follow=yes" so that the link # is preserved and the link target is updated @@ -1122,7 +1130,7 @@ - name: Update the test file using follow=True to preserve the link copy: dest: "{{ remote_dir }}/follow_link" - content: "this is the new content\n" + src: foo.txt follow: yes register: replace_follow_result @@ -1131,19 +1139,37 @@ path: "{{ remote_dir }}/follow_link" register: stat_link_result -- name: Assert that the link is still a link +- name: Assert that the link is still a link and contents were changed assert: that: - - stat_link_result.stat.islnk + - stat_link_result['stat']['islnk'] + - stat_link_result['stat']['lnk_target'] == './follow_test' + - replace_follow_result['changed'] + - "replace_follow_result['checksum'] == remote_file_hash" -- name: Get the checksum of the link target - shell: "{{ ansible_python_interpreter }} -c 'import hashlib; print(hashlib.sha1(open(\"{{remote_dir | expanduser}}/follow_test\", \"rb\").read()).hexdigest())'" - register: target_file_result +# Symlink handling when the dest is already there +# https://github.com/ansible/ansible-modules-core/issues/1568 -- name: Assert that the link target was updated +- name: test idempotency by trying to copy to the symlink with the same contents + copy: + dest: "{{ remote_dir }}/follow_link" + src: foo.txt + follow: yes + register: replace_follow_result + +- name: Stat the link path + stat: + path: "{{ remote_dir }}/follow_link" + register: stat_link_result + +- name: Assert that the link is still a link and contents were changed assert: that: - - replace_follow_result.checksum == target_file_result.stdout + - stat_link_result['stat']['islnk'] + - stat_link_result['stat']['lnk_target'] == './follow_test' + - not replace_follow_result['changed'] + - replace_follow_result['checksum'] == remote_file_hash + - name: Update the test file using follow=False to overwrite the link copy: @@ -1169,6 +1195,13 @@ - "stat_results.stat.checksum == ('modified'|hash('sha1'))" - "not stat_results.stat.islnk" +# test overwriting a link using "follow=yes" so that the link +# is preserved and the link target is updated when the thing being copied is a link + +# +# File mode tests +# + - name: setup directory for test file: state=directory dest={{remote_dir }}/directory mode=0755