From 68683b4c73c08fa6b70c740c3c4deb863eddef5b Mon Sep 17 00:00:00 2001 From: Shuang Wang Date: Thu, 2 Aug 2018 04:02:27 +0900 Subject: [PATCH] fix issue [ get_url does not change mode when checksum matches ] (#43342) * fix #29614 * add change log for #43342 * Cleanup tests and add tests for this scenario Co-authored-by: ptux --- changelogs/fragments/get_url_bug_fix.yaml | 2 + .../modules/net_tools/basics/get_url.py | 22 +++--- .../targets/get_url/tasks/main.yml | 71 +++++++++++++------ 3 files changed, 61 insertions(+), 34 deletions(-) create mode 100644 changelogs/fragments/get_url_bug_fix.yaml diff --git a/changelogs/fragments/get_url_bug_fix.yaml b/changelogs/fragments/get_url_bug_fix.yaml new file mode 100644 index 0000000000..a0a3edacd1 --- /dev/null +++ b/changelogs/fragments/get_url_bug_fix.yaml @@ -0,0 +1,2 @@ +bugfixes: +- get_url - fix the bug that get_url does not change mode when checksum matches (https://github.com/ansible/ansible/issues/29614) diff --git a/lib/ansible/modules/net_tools/basics/get_url.py b/lib/ansible/modules/net_tools/basics/get_url.py index 38c48e2bc2..cbbf125f08 100644 --- a/lib/ansible/modules/net_tools/basics/get_url.py +++ b/lib/ansible/modules/net_tools/basics/get_url.py @@ -451,22 +451,18 @@ def main(): destination_checksum = module.digest_from_file(dest, algorithm) if checksum == destination_checksum: - module.exit_json(msg="file already exists", dest=dest, url=url, changed=False) + # Not forcing redownload, unless checksum does not match + # allow file attribute changes + module.params['path'] = dest + file_args = module.load_file_common_arguments(module.params) + file_args['path'] = dest + changed = module.set_fs_attributes_if_different(file_args, False) + if changed: + module.exit_json(msg="file already exists but file attributes changed", dest=dest, url=url, changed=changed) + module.exit_json(msg="file already exists", dest=dest, url=url, changed=changed) checksum_mismatch = True - # Not forcing redownload, unless checksum does not match - if not force and not checksum_mismatch: - # allow file attribute changes - module.params['path'] = dest - file_args = module.load_file_common_arguments(module.params) - file_args['path'] = dest - changed = module.set_fs_attributes_if_different(file_args, False) - - if changed: - module.exit_json(msg="file already exists but file attributes changed", dest=dest, url=url, changed=changed) - module.exit_json(msg="file already exists", dest=dest, url=url, changed=changed) - # If the file already exists, prepare the last modified time for the # request. mtime = os.path.getmtime(dest) diff --git a/test/integration/targets/get_url/tasks/main.yml b/test/integration/targets/get_url/tasks/main.yml index bc962eb640..f3af012319 100644 --- a/test/integration/targets/get_url/tasks/main.yml +++ b/test/integration/targets/get_url/tasks/main.yml @@ -1,4 +1,4 @@ -# Test code for the file module. +# Test code for the get_url module # (c) 2014, Richard Isaacson # This file is part of Ansible @@ -51,8 +51,8 @@ - name: assert success and change assert: that: - - result.changed - - '"OK" in result.msg' + - result is changed + - '"OK" in result.msg' - name: test nonexisting file fetch get_url: @@ -64,21 +64,27 @@ - name: assert success and change assert: that: - - result.failed + - result is failed - name: test HTTP HEAD request for file in check mode - get_url: url="https://{{ httpbin_host }}/get" dest={{ output_dir }}/get_url_check.txt force=yes + get_url: + url: "https://{{ httpbin_host }}/get" + dest: "{{ output_dir }}/get_url_check.txt" + force: yes check_mode: True register: result - name: assert that the HEAD request was successful in check mode assert: that: - - result.changed + - result is changed - '"OK" in result.msg' - name: test HTTP HEAD for nonexistent URL in check mode - get_url: url="https://{{ httpbin_host }}/DOESNOTEXIST" dest={{ output_dir }}/shouldnotexist.html force=yes + get_url: + url: "https://{{ httpbin_host }}/DOESNOTEXIST" + dest: "{{ output_dir }}/shouldnotexist.html" + force: yes check_mode: True register: result ignore_errors: True @@ -86,7 +92,7 @@ - name: assert that HEAD request for nonexistent URL failed assert: that: - - result.failed + - result is failed - name: test https fetch get_url: url="https://{{ httpbin_host }}/get" dest={{output_dir}}/get_url.txt force=yes @@ -95,8 +101,8 @@ - name: assert the get_url call was successful assert: that: - - result.changed - - '"OK" in result.msg' + - result is changed + - '"OK" in result.msg' - name: test https fetch to a site with mismatched hostname and certificate get_url: @@ -130,7 +136,7 @@ - name: Assert that the file was downloaded assert: that: - - "result.changed == true" + - result is changed - "stat_result.stat.exists == true" # SNI Tests @@ -144,21 +150,23 @@ - command: "grep '{{ sni_host }}' {{ output_dir}}/sni.html" register: data_result - when: "{{ python_has_ssl_context }}" + when: python_has_ssl_context + +- debug: + var: get_url_result -- debug: var=get_url_result - name: Assert that SNI works with this python version assert: that: - 'data_result.rc == 0' - when: "{{ python_has_ssl_context }}" + when: python_has_ssl_context # If the client doesn't support SNI then get_url should have failed with a certificate mismatch - name: Assert that hostname verification failed because SNI is not supported on this version of python assert: that: - 'get_url_result is failed' - when: "{{ not python_has_ssl_context }}" + when: not python_has_ssl_context # These tests are just side effects of how the site is hosted. It's not # specifically a test site. So the tests may break due to the hosting changing @@ -171,22 +179,24 @@ - command: "grep '{{ sni_host }}' {{ output_dir}}/sni.html" register: data_result - when: "{{ python_has_ssl_context }}" + when: python_has_ssl_context + +- debug: + var: get_url_result -- debug: var=get_url_result - name: Assert that SNI works with this python version assert: that: - 'data_result.rc == 0' - 'get_url_result is not failed' - when: "{{ python_has_ssl_context }}" + when: python_has_ssl_context # If the client doesn't support SNI then get_url should have failed with a certificate mismatch - name: Assert that hostname verification failed because SNI is not supported on this version of python assert: that: - 'get_url_result is failed' - when: "{{ not python_has_ssl_context }}" + when: not python_has_ssl_context # End hacky SNI test section - name: Test get_url with redirect @@ -208,7 +218,7 @@ - name: Assert that the file has the right permissions assert: that: - - "result.changed == true" + - result is changed - "stat_result.stat.mode == '0707'" - name: Test that setting file modes on an already downlaoded file work @@ -225,9 +235,28 @@ - name: Assert that the file has the right permissions assert: that: - - "result.changed == true" + - result is changed - "stat_result.stat.mode == '0070'" +# https://github.com/ansible/ansible/issues/29614 +- name: Change mode on an already downloaded file and specify checksum + get_url: + url: 'https://{{ httpbin_host }}/' + dest: '{{ output_dir }}/test' + checksum: 'sha256:7036ede810fad2b5d2e7547ec703cae8da61edbba43c23f9d7203a0239b765c4.' + mode: '0775' + register: result + +- stat: + path: "{{ output_dir }}/test" + register: stat_result + +- name: Assert that file permissions on already downloaded file were changed + assert: + that: + - result is changed + - "stat_result.stat.mode == '0775'" + #https://github.com/ansible/ansible/issues/16191 - name: Test url split with no filename get_url: