cmueller/ansible
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

deprecate _remote_checksum() and remove use in fetch (#74848)

Browse source 
* Remove use of _remote_checksum from fetch module
* Add deprecation message displayed during runtime
* Increase test coverage for fetch
* Add tests covering the use of stat from the fetch module

    This required creating an unpriveleged user account and connecting as that user remotely since
    it is not possible to create a file that the root user cannot stat.

* Use fact caching to persist remote tmp dir across playbook runs
* Add variables to setup_remote_tmp test role to allow caching of the remote temp dir fact
  and preventing removal of the remote_tmp_dir
This commit is contained in:
Sam Doran 2021-06-16 13:51:07 -04:00 committed by GitHub
parent 5a5a1882d4
commit be0cdc0ea2
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
20 changed files with 369 additions and 181 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

7
lib/ansible/plugins/action/__init__.py
View file

@ -794,7 +794,8 @@ class ActionBase(with_metaclass(ABCMeta, object)):
return mystat['stat']
def _remote_checksum(self, path, all_vars, follow=False):
'''
"""Deprecated. Use _execute_remote_stat() instead.
Produces a remote checksum given a path,
Returns a number 0-4 for specific errors instead of checksum, also ensures it is different
0 = unknown error
@ -803,7 +804,9 @@ class ActionBase(with_metaclass(ABCMeta, object)):
3 = its a directory, not a file
4 = stat module failed, likely due to not finding python
5 = appropriate json module not found
'''
"""
self._display.deprecated("The '_remote_checksum()' method is deprecated. "
"The plugin author should update the code to use '_execute_remote_stat()' instead", "2.16")
x = "0" # unknown error has occurred
try:
remote_stat = self._execute_remote_stat(path, all_vars, follow=follow)

80
lib/ansible/plugins/action/fetch.py
View file

@ -19,9 +19,8 @@ __metaclass__ = type
import os
import base64
from ansible.errors import AnsibleActionFail, AnsibleActionSkip
from ansible.module_utils._text import to_bytes
from ansible.errors import AnsibleError, AnsibleActionFail, AnsibleActionSkip
from ansible.module_utils.common.text.converters import to_bytes, to_text
from ansible.module_utils.six import string_types
from ansible.module_utils.parsing.convert_bool import boolean
from ansible.plugins.action import ActionBase
@ -69,23 +68,59 @@ class ActionModule(ActionBase):
source = self._connection._shell.join_path(source)
source = self._remote_expand_user(source)
remote_stat = {}
remote_checksum = None
if not self._connection.become:
# calculate checksum for the remote file, don't bother if using become as slurp will be used
# Force remote_checksum to follow symlinks because fetch always follows symlinks
remote_checksum = self._remote_checksum(source, all_vars=task_vars, follow=True)
# Get checksum for the remote file. Don't bother if using become as slurp will be used.
# Follow symlinks because fetch always follows symlinks
try:
remote_stat = self._execute_remote_stat(source, all_vars=task_vars, follow=True)
except AnsibleError as ae:
result['changed'] = False
result['file'] = source
if fail_on_missing:
result['failed'] = True
result['msg'] = to_text(ae)
else:
result['msg'] = "%s, ignored" % to_text(ae, errors='surrogate_or_replace')
return result
remote_checksum = remote_stat.get('checksum')
if remote_stat.get('exists'):
if remote_stat.get('isdir'):
result['failed'] = True
result['changed'] = False
result['msg'] = "remote file is a directory, fetch cannot work on directories"
# Historically, these don't fail because you may want to transfer
# a log file that possibly MAY exist but keep going to fetch other
# log files. Today, this is better achieved by adding
# ignore_errors or failed_when to the task. Control the behaviour
# via fail_when_missing
if not fail_on_missing:
result['msg'] += ", not transferring, ignored"
del result['changed']
del result['failed']
return result
# use slurp if permissions are lacking or privilege escalation is needed
remote_data = None
if remote_checksum in ('1', '2', None):
if remote_checksum in (None, '1', ''):
slurpres = self._execute_module(module_name='ansible.legacy.slurp', module_args=dict(src=source), task_vars=task_vars)
if slurpres.get('failed'):
if not fail_on_missing and (slurpres.get('msg').startswith('file not found') or remote_checksum == '1'):
result['msg'] = "the remote file does not exist, not transferring, ignored"
if not fail_on_missing:
result['file'] = source
result['changed'] = False
else:
result.update(slurpres)
if 'not found' in slurpres.get('msg', ''):
result['msg'] = "the remote file does not exist, not transferring, ignored"
elif slurpres.get('msg', '').startswith('source is a directory'):
result['msg'] = "remote file is a directory, fetch cannot work on directories"
return result
else:
if slurpres['encoding'] == 'base64':
@ -124,33 +159,6 @@ class ActionModule(ActionBase):
target_name = self._play_context.remote_addr
dest = "%s/%s/%s" % (self._loader.path_dwim(dest), target_name, source_local)
if remote_checksum in ('0', '1', '2', '3', '4', '5'):
result['changed'] = False
result['file'] = source
if remote_checksum == '0':
result['msg'] = "unable to calculate the checksum of the remote file"
elif remote_checksum == '1':
result['msg'] = "the remote file does not exist"
elif remote_checksum == '2':
result['msg'] = "no read permission on remote file"
elif remote_checksum == '3':
result['msg'] = "remote file is a directory, fetch cannot work on directories"
elif remote_checksum == '4':
result['msg'] = "python isn't present on the system. Unable to compute checksum"
elif remote_checksum == '5':
result['msg'] = "stdlib json was not found on the remote machine. Only the raw module can work without those installed"
# Historically, these don't fail because you may want to transfer
# a log file that possibly MAY exist but keep going to fetch other
# log files. Today, this is better achieved by adding
# ignore_errors or failed_when to the task. Control the behaviour
# via fail_when_missing
if fail_on_missing:
result['failed'] = True
del result['changed']
else:
result['msg'] += ", not transferring, ignored"
return result
dest = os.path.normpath(dest)
# calculate checksum for the local file

16
test/integration/targets/fetch/cleanup.yml Normal file
View file

@ -0,0 +1,16 @@
- name: Cleanup user account
hosts: testhost
tasks:
- name: remove test user
user:
name: fetcher
state: absent
remove: yes
force: yes
- name: delete temporary directory
file:
path: "{{ remote_tmp_dir }}"
state: absent
no_log: yes

8
test/integration/targets/fetch/hosts.yml Normal file
View file

@ -0,0 +1,8 @@
all:
hosts:
testhost:
ansible_host: localhost
ansible_connection: ssh
ansible_python_interpreter: "{{ ansible_playbook_python }}"
ansible_host_key_checking: no
ansible_ssh_common_args: -o UserKnownHostsFile={{ output_dir }}/known_hosts -o StrictHostKeyChecking=no

1
test/integration/targets/fetch/roles/fetch_tests/defaults/main.yml Normal file
View file

@ -0,0 +1 @@
skip_cleanup: no

8
test/integration/targets/fetch/roles/fetch_tests/handlers/main.yml Normal file
View file

@ -0,0 +1,8 @@
- name: remove test user
user:
name: fetcher
state: absent
remove: yes
force: yes
become: yes
when: not skip_cleanup | bool

53
test/integration/targets/fetch/roles/fetch_tests/tasks/fail_on_missing.yml Normal file
View file

@ -0,0 +1,53 @@
- name: Attempt to fetch a non-existent file - do not fail on missing
fetch:
src: "{{ remote_tmp_dir }}/doesnotexist"
dest: "{{ output_dir }}/fetched"
fail_on_missing: no
register: fetch_missing_nofail
- name: Attempt to fetch a non-existent file - fail on missing
fetch:
src: "{{ remote_tmp_dir }}/doesnotexist"
dest: "{{ output_dir }}/fetched"
fail_on_missing: yes
register: fetch_missing
ignore_errors: yes
- name: Attempt to fetch a non-existent file - fail on missing implicit
fetch:
src: "{{ remote_tmp_dir }}/doesnotexist"
dest: "{{ output_dir }}/fetched"
register: fetch_missing_implicit
ignore_errors: yes
- name: Attempt to fetch a directory - should not fail but return a message
fetch:
src: "{{ remote_tmp_dir }}"
dest: "{{ output_dir }}/somedir"
fail_on_missing: no
register: fetch_dir
- name: Attempt to fetch a directory - should fail
fetch:
src: "{{ remote_tmp_dir }}"
dest: "{{ output_dir }}/somedir"
fail_on_missing: yes
register: failed_fetch_dir
ignore_errors: yes
- name: Check fetch missing with failure with implicit fail
assert:
that:
- fetch_missing_nofail.msg is search('ignored')
- fetch_missing_nofail is not changed
- fetch_missing is failed
- fetch_missing is not changed
- fetch_missing.msg is search ('remote file does not exist')
- fetch_missing_implicit is failed
- fetch_missing_implicit is not changed
- fetch_missing_implicit.msg is search ('remote file does not exist')
- fetch_dir is not changed
- fetch_dir.msg is search('is a directory')
- failed_fetch_dir is failed
- failed_fetch_dir is not changed
- failed_fetch_dir.msg is search('is a directory')

41
test/integration/targets/fetch/roles/fetch_tests/tasks/failures.yml Normal file
View file

@ -0,0 +1,41 @@
- name: Fetch with no parameters
fetch:
register: fetch_no_params
ignore_errors: yes
- name: Fetch with incorrect source type
fetch:
src: [1, 2]
dest: "{{ output_dir }}/fetched"
register: fetch_incorrect_src
ignore_errors: yes
- name: Try to fetch a file inside an inaccessible directory
fetch:
src: "{{ remote_tmp_dir }}/noaccess/file1"
dest: "{{ output_dir }}"
register: failed_fetch_no_access
become: yes
become_user: fetcher
become_method: su
ignore_errors: yes
- name: Dest is an existing directory name without trailing slash and flat=yes, should fail
fetch:
src: "{{ remote_tmp_dir }}/orig"
dest: "{{ output_dir }}"
flat: yes
register: failed_fetch_dest_dir
ignore_errors: true
- name: Ensure fetch failed
assert:
that:
- fetch_no_params is failed
- fetch_no_params.msg is search('src and dest are required')
- fetch_incorrect_src is failed
- fetch_incorrect_src.msg is search('Invalid type supplied for source')
- failed_fetch_no_access is failed
- failed_fetch_no_access.msg is search('file is not readable')
- failed_fetch_dest_dir is failed
- failed_fetch_dest_dir.msg is search('dest is an existing directory')

146
test/integration/targets/fetch/roles/fetch_tests/tasks/main.yml
View file

@ -1,141 +1,5 @@
# test code for the pip module
# (c) 2014, Michael DeHaan <michael.dehaan@gmail.com>
# This file is part of Ansible
#
# Ansible is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
#
# Ansible is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with Ansible. If not, see <http://www.gnu.org/licenses/>.
- name: create a file that we can use to fetch
copy: content="test" dest={{ remote_tmp_dir }}/orig
- name: fetch the test file
fetch: src={{ remote_tmp_dir }}/orig dest={{ output_dir }}/fetched
register: fetched
- debug: var=fetched
- name: Assert that we fetched correctly
assert:
that:
- 'fetched["changed"] == True'
- 'fetched["checksum"] == "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3"'
- 'fetched["remote_checksum"] == "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3"'
- 'lookup("file", output_dir + "/fetched/" + inventory_hostname + remote_tmp_dir + "/orig") == "test"'
# TODO: check the become and non-become forms of fetch because in one form we'll do
# the get method of the connection plugin and in the become case we'll use the
# fetch module.
- name: fetch a second time to show idempotence
fetch: src={{ remote_tmp_dir }}/orig dest={{ output_dir }}/fetched
register: fetched
- name: Assert that the file was not fetched the second time
assert:
that:
- 'fetched["changed"] == False'
- 'fetched["checksum"] == "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3"'
- name: attempt to fetch a non-existent file - do not fail on missing
fetch: src={{ remote_tmp_dir }}/doesnotexist dest={{ output_dir }}/fetched fail_on_missing=False
register: fetch_missing_nofail
- name: check fetch missing no fail result
assert:
that:
- "fetch_missing_nofail.msg"
- "fetch_missing_nofail is not changed"
- name: attempt to fetch a non-existent file - fail on missing
fetch: src={{ remote_tmp_dir }}/doesnotexist dest={{ output_dir }}/fetched fail_on_missing=yes
register: fetch_missing
ignore_errors: true
- name: check fetch missing with failure
assert:
that:
- "fetch_missing is failed"
- "fetch_missing.msg"
- "fetch_missing is not changed"
- name: attempt to fetch a non-existent file - fail on missing implicit
fetch: src={{ remote_tmp_dir }}/doesnotexist dest={{ output_dir }}/fetched
register: fetch_missing_implicit
ignore_errors: true
- name: check fetch missing with failure with implicit fail
assert:
that:
- "fetch_missing_implicit is failed"
- "fetch_missing_implicit.msg"
- "fetch_missing_implicit is not changed"
- name: attempt to fetch a directory - should not fail but return a message
fetch: src={{ remote_tmp_dir }} dest={{ output_dir }}/somedir fail_on_missing=False
register: fetch_dir
- name: check fetch directory result
assert:
that:
- "fetch_dir is not changed"
- "fetch_dir.msg"
- name: attempt to fetch a directory - should fail
fetch: src={{ remote_tmp_dir }} dest={{ output_dir }}/somedir fail_on_missing=True
register: failed_fetch_dir
ignore_errors: true
- name: check fetch directory result
assert:
that:
- "failed_fetch_dir is failed"
- "fetch_dir.msg"
- name: create symlink to a file that we can fetch
file:
path: "{{ remote_tmp_dir }}/link"
src: "{{ remote_tmp_dir }}/orig"
state: "link"
- name: fetch the file via a symlink
fetch: src={{ remote_tmp_dir }}/link dest={{ output_dir }}/fetched-link
register: fetched
- debug: var=fetched
- name: Assert that we fetched correctly
assert:
that:
- 'fetched["changed"] == True'
- 'fetched["checksum"] == "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3"'
- 'fetched["remote_checksum"] == "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3"'
- 'lookup("file", output_dir + "/fetched-link/" + inventory_hostname + remote_tmp_dir + "/link") == "test"'
# TODO: check the become and non-become forms of fetch because in one form we'll do
# the get method of the connection plugin and in the become case we'll use the
# fetch module.
- name: dest is an existing directory name without trailing slash and flat=yes, should fail
fetch:
src: "{{ remote_tmp_dir }}/orig"
dest: "{{ output_dir }}"
flat: yes
register: failed_fetch_dest_dir
ignore_errors: true
- name: check that it indeed failed
assert:
that:
- "failed_fetch_dest_dir is failed"
- "failed_fetch_dest_dir.msg"
- import_tasks: setup.yml
- import_tasks: normal.yml
- import_tasks: symlink.yml
- import_tasks: fail_on_missing.yml
- import_tasks: failures.yml

38
test/integration/targets/fetch/roles/fetch_tests/tasks/normal.yml Normal file
View file

@ -0,0 +1,38 @@
- name: Fetch the test file
fetch: src={{ remote_tmp_dir }}/orig dest={{ output_dir }}/fetched
register: fetched
- name: Fetch a second time to show no changes
fetch: src={{ remote_tmp_dir }}/orig dest={{ output_dir }}/fetched
register: fetched_again
- name: Fetch the test file in check mode
fetch:
src: "{{ remote_tmp_dir }}/orig"
dest: "{{ output_dir }}/fetched"
check_mode: yes
register: fetch_check_mode
- name: Fetch with dest ending in path sep
fetch:
src: "{{ remote_tmp_dir }}/orig"
dest: "{{ output_dir }}/"
flat: yes
- name: Fetch with dest with relative path
fetch:
src: "{{ remote_tmp_dir }}/orig"
dest: "{{ output_dir[1:] }}"
flat: yes
- name: Assert that we fetched correctly
assert:
that:
- fetched is changed
- fetched.checksum == "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3"
- fetched_again is not changed
- fetched_again.checksum == "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3"
- fetched.remote_checksum == "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3"
- lookup("file", output_dir + "/fetched/" + inventory_hostname + remote_tmp_dir + "/orig") == "test"
- fetch_check_mode is skipped
- fetch_check_mode.msg is search('not \(yet\) supported')

38
test/integration/targets/fetch/roles/fetch_tests/tasks/setup.yml Normal file
View file

@ -0,0 +1,38 @@
- name: Include system specific variables
include_vars: "{{ lookup('first_found', params) }}"
vars:
params:
files:
- "{{ ansible_facts.system }}.yml"
- default.yml
paths:
- "{{ role_path }}/vars"
- name: Create test user
user:
name: fetcher
create_home: yes
groups: "{{ _fetch_additional_groups | default(omit) }}"
append: "{{ True if _fetch_additional_groups else False }}"
become: yes
notify:
- remove test user
- name: Create a file that we can use to fetch
copy:
content: "test"
dest: "{{ remote_tmp_dir }}/orig"
- name: Create symlink to a file that we can fetch
file:
path: "{{ remote_tmp_dir }}/link"
src: "{{ remote_tmp_dir }}/orig"
state: "link"
- name: Create an inaccessible directory
file:
path: "{{ remote_tmp_dir }}/noaccess"
state: directory
mode: '0600'
owner: root
become: yes

13
test/integration/targets/fetch/roles/fetch_tests/tasks/symlink.yml Normal file
View file

@ -0,0 +1,13 @@
- name: Fetch the file via a symlink
fetch:
src: "{{ remote_tmp_dir }}/link"
dest: "{{ output_dir }}/fetched-link"
register: fetched_symlink
- name: Assert that we fetched correctly
assert:
that:
- fetched_symlink is changed
- fetched_symlink.checksum == "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3"
- fetched_symlink.remote_checksum == "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3"
- 'lookup("file", output_dir + "/fetched-link/" + inventory_hostname + remote_tmp_dir + "/link") == "test"'

3
test/integration/targets/fetch/roles/fetch_tests/vars/Darwin.yml Normal file
View file

@ -0,0 +1,3 @@
# macOS requires users to be in an additional group for ssh access
_fetch_additional_groups: com.apple.access_ssh

1
test/integration/targets/fetch/roles/fetch_tests/vars/default.yml Normal file
View file

@ -0,0 +1 @@
_fetch_additional_groups: []

26
test/integration/targets/fetch/runme.sh
View file

@ -2,11 +2,33 @@
set -eux
function cleanup {
ansible-playbook -i hosts.yml cleanup.yml -e "output_dir=${OUTPUT_DIR}" -b "$@"
unset ANSIBLE_CACHE_PLUGIN
unset ANSIBLE_CACHE_PLUGIN_CONNECTION
}
trap 'cleanup "$@"' EXIT
# setup required roles
ln -s ../../setup_remote_tmp_dir roles/setup_remote_tmp_dir
# run old type role tests
ansible-playbook -i ../../inventory run_fetch_tests.yml -e "output_dir=${OUTPUT_DIR}" -v "$@"
ansible-playbook -i ../../inventory run_fetch_tests.yml -e "output_dir=${OUTPUT_DIR}" "$@"
# run same test with become
ansible-playbook -i ../../inventory run_fetch_tests.yml -e "output_dir=${OUTPUT_DIR}" -b "$@"
# run tests to avoid path injection from slurp when fetch uses become
ansible-playbook -i ../../inventory injection/avoid_slurp_return.yml -e "output_dir=${OUTPUT_DIR}" -v "$@"
ansible-playbook -i ../../inventory injection/avoid_slurp_return.yml -e "output_dir=${OUTPUT_DIR}" "$@"
## Test unreadable file with stat. Requires running without become and as a user other than root.
#
# Change the known_hosts file to avoid changing the test environment
export ANSIBLE_CACHE_PLUGIN=jsonfile
export ANSIBLE_CACHE_PLUGIN_CONNECTION="${OUTPUT_DIR}/cache"
# Create a non-root user account and configure SSH acccess for that account
ansible-playbook -i hosts.yml setup_unreadable_test.yml -e "output_dir=${OUTPUT_DIR}" "$@"
# Run the tests as the unprivileged user without become to test the use of the stat module from the fetch module
ansible-playbook --user fetcher -i hosts.yml test_unreadable_with_stat.yml -e "output_dir=${OUTPUT_DIR}" "$@"

39
test/integration/targets/fetch/setup_unreadable_test.yml Normal file
View file

@ -0,0 +1,39 @@
- name: Create a user account and configure ssh access
hosts: testhost
gather_facts: no
tasks:
- import_role:
name: fetch_tests
tasks_from: setup.yml
vars:
# Keep the remote temp dir and cache the remote_tmp_dir fact. The directory itself
# and the fact that contains the path are needed in a separate ansible-playbook run.
setup_remote_tmp_dir_skip_cleanup: yes
setup_remote_tmp_dir_cache_path: yes
# This prevents ssh access. It is fixed in some container images but not all.
# https://github.com/ansible/distro-test-containers/pull/70
- name: Remove /run/nologin
file:
path: /run/nologin
state: absent
# Setup ssh access for the unprivileged user.
- name: Get home directory for temporary user
command: echo ~fetcher
register: fetcher_home
- name: Create .ssh dir
file:
path: "{{ fetcher_home.stdout }}/.ssh"
state: directory
owner: fetcher
mode: '0700'
- name: Configure authorized_keys
copy:
src: "~root/.ssh/authorized_keys"
dest: "{{ fetcher_home.stdout }}/.ssh/authorized_keys"
owner: fetcher
mode: '0600'

27
test/integration/targets/fetch/test_unreadable_with_stat.yml Normal file
View file

@ -0,0 +1,27 @@
# This playbook needs to be run as a non-root user without become. Under
# those circumstances, the fetch module uses stat and not slurp.
- name: Test unreadable file using stat
hosts: testhost
gather_facts: no
tasks:
- name: Try to fetch a file inside an inaccessible directory
fetch:
src: "{{ remote_tmp_dir }}/noaccess/file1"
dest: "{{ output_dir }}"
register: failed_fetch_no_access
ignore_errors: yes
- name: Try to fetch a file inside an inaccessible directory without fail_on_missing
fetch:
src: "{{ remote_tmp_dir }}/noaccess/file1"
dest: "{{ output_dir }}"
fail_on_missing: no
register: failed_fetch_no_access_fail_on_missing
- assert:
that:
- failed_fetch_no_access is failed
- failed_fetch_no_access.msg is search('Permission denied')
- failed_fetch_no_access_fail_on_missing.msg is search(', ignored')

2
test/integration/targets/setup_remote_tmp_dir/defaults/main.yml Normal file
View file

@ -0,0 +1,2 @@
setup_remote_tmp_dir_skip_cleanup: no
setup_remote_tmp_dir_cache_path: no

2
test/integration/targets/setup_remote_tmp_dir/handlers/main.yml
View file

@ -1,5 +1,7 @@
- name: delete temporary directory
include_tasks: default-cleanup.yml
when: not setup_remote_tmp_dir_skip_cleanup | bool
- name: delete temporary directory (windows)
include_tasks: windows-cleanup.yml
when: not setup_remote_tmp_dir_skip_cleanup | bool

1
test/integration/targets/setup_remote_tmp_dir/tasks/default.yml
View file

@ -9,3 +9,4 @@
- name: record temporary directory
set_fact:
remote_tmp_dir: "{{ remote_tmp_dir.path }}"
cacheable: "{{ setup_remote_tmp_dir_cache_path | bool }}"