From 6227c2ac75acefbe30d3a22252adfc25ea52a3ae Mon Sep 17 00:00:00 2001 From: Toshio Kuratomi Date: Wed, 16 May 2018 10:41:11 -0700 Subject: [PATCH] More file refactoring (#40114) * Set src in the state functions rather than the toplevel A good API should only require passing one version of a piece of data around so do that for src * Move the rewriting of path into additional_parameter_handling When the path is a directory we can rewrite the path to be a file inside of the directory * Emit a warning when src is used with a state where it should be ignored --- changelogs/fragments/file-disallow-src.yaml | 8 +++ .../rst/porting_guides/porting_guide_2.6.rst | 12 +++++ lib/ansible/modules/files/file.py | 50 ++++++++++++------- test/integration/targets/file/tasks/main.yml | 4 +- 4 files changed, 54 insertions(+), 20 deletions(-) create mode 100644 changelogs/fragments/file-disallow-src.yaml diff --git a/changelogs/fragments/file-disallow-src.yaml b/changelogs/fragments/file-disallow-src.yaml new file mode 100644 index 00000000000..e018392d7a6 --- /dev/null +++ b/changelogs/fragments/file-disallow-src.yaml @@ -0,0 +1,8 @@ +--- +bugfixes: +- file module - The file module allowed the user to specify src as a parameter + when state was not link or hard. This is documented as only applying to + state=link or state=hard but in previous Ansible, this could have an effect + in rare cornercases. For instance, "ansible -m file -a 'state=directory + path=/tmp src=/var/lib'" would create /tmp/lib. This has been disabled and + a warning emitted (will change to an error in Ansible-2.10). diff --git a/docs/docsite/rst/porting_guides/porting_guide_2.6.rst b/docs/docsite/rst/porting_guides/porting_guide_2.6.rst index ca9996ea8c4..456717ee83a 100644 --- a/docs/docsite/rst/porting_guides/porting_guide_2.6.rst +++ b/docs/docsite/rst/porting_guides/porting_guide_2.6.rst @@ -51,6 +51,18 @@ Noteworthy module changes * The ``win_iis_webapppool`` module no longer accepts a string for the ``atributes`` module option; use the free form dictionary value instead * The ``name`` module option for ``win_package`` has been removed; this is not used anywhere and should just be removed from your playbooks * The ``win_regedit`` module no longer automatically corrects the hive path ``HCCC`` to ``HKCC``; use ``HKCC`` because this is the correct hive path +* The :ref:`file_module` now emits a deprecation warning when ``src`` is specified with a state + other than ``hard`` or ``link`` as it is only supposed to be useful with those. This could have + an effect on people who were depending on a buggy interaction between src and other state's to + place files into a subdirectory. For instance:: + + $ ansible localhost -m file -a 'path=/var/lib src=/tmp/ state=directory' + + Would create a directory named ``/tmp/lib``. Instead of the above, simply spell out the entire + destination path like this:: + + $ ansible localhost -m file -a 'path=/tmp/lib state=directory' + Plugins ======= diff --git a/lib/ansible/modules/files/file.py b/lib/ansible/modules/files/file.py index cfda5279a0a..ecf6e2a0aa8 100644 --- a/lib/ansible/modules/files/file.py +++ b/lib/ansible/modules/files/file.py @@ -159,9 +159,6 @@ def _ansible_excepthook(exc_type, exc_value, tb): def additional_parameter_handling(params): """Additional parameter validation and reformatting""" - - params['b_src'] = to_bytes(params['src'], errors='surrogate_or_strict', nonstring='passthru') - # state should default to file, but since that creates many conflicts, # default state to 'current' when it exists. prev_state = get_state(to_bytes(params['path'], errors='surrogate_or_strict')) @@ -179,6 +176,30 @@ def additional_parameter_handling(params): raise ParameterError(results={"msg": "recurse option requires state to be 'directory'", "path": params["path"]}) + # Make sure that src makes sense with the state + if params['src'] and params['state'] not in ('link', 'hard'): + params['src'] = None + module.warn("The src option requires state to be 'link' or 'hard'. This will become an" + " error in Ansible 2.10") + + # In 2.10, switch to this + # raise ParameterError(results={"msg": "src option requires state to be 'link' or 'hard'", + # "path": params["path"]}) + + # When path is a directory, rewrite the pathname to be the file inside of the directory + # TODO: Why do we exclude link? Why don't we exclude directory? Should we exclude touch? + if (params['state'] not in ("link", "absent") and os.path.isdir(to_bytes(params['path'], errors='surrogate_or_strict'))): + basename = None + + # original_basename is used by other modules that depend on file + if params['original_basename']: + basename = params['original_basename'] + elif params['src'] is not None: + basename = os.path.basename(params['src']) + + if basename: + params['path'] = params['path'] = os.path.join(params['path'], basename) + def get_state(path): ''' Find out current state ''' @@ -426,8 +447,9 @@ def ensure_directory(path, follow, recurse): return {'path': path, 'changed': changed, 'diff': diff} -def ensure_symlink(path, src, b_src, follow, force): +def ensure_symlink(path, src, follow, force): b_path = to_bytes(path, errors='surrogate_or_strict') + b_src = to_bytes(src, errors='surrogate_or_strict') prev_state = get_state(b_path) file_args = module.load_file_common_arguments(module.params) @@ -437,7 +459,7 @@ def ensure_symlink(path, src, b_src, follow, force): if follow: # use the current target of the link as the source src = to_native(os.path.realpath(b_path), errors='strict') - b_src = to_bytes(os.path.realpath(b_path), errors='strict') + b_src = to_bytes(src, errors='surrogate_or_strict') if not os.path.islink(b_path) and os.path.isdir(b_path): relpath = path @@ -537,8 +559,9 @@ def ensure_symlink(path, src, b_src, follow, force): return {'dest': path, 'src': src, 'changed': changed, 'diff': diff} -def ensure_hardlink(path, src, b_src, follow, force): +def ensure_hardlink(path, src, follow, force): b_path = to_bytes(path, errors='surrogate_or_strict') + b_src = to_bytes(src, errors='surrogate_or_strict') prev_state = get_state(b_path) file_args = module.load_file_common_arguments(module.params) @@ -665,31 +688,20 @@ def main(): follow = params['follow'] path = params['path'] src = params['src'] - b_src = params['b_src'] # short-circuit for diff_peek if params['_diff_peek'] is not None: appears_binary = execute_diff_peek(to_bytes(path, errors='surrogate_or_strict')) module.exit_json(path=path, changed=False, appears_binary=appears_binary) - # original_basename is used by other modules that depend on file. - if state not in ("link", "absent") and os.path.isdir(to_bytes(path, errors='surrogate_or_strict')): - basename = None - if params['original_basename']: - basename = params['original_basename'] - elif b_src is not None: - basename = os.path.basename(b_src) - if basename: - params['path'] = path = os.path.join(path, basename) - if state == 'file': result = ensure_file_attributes(path, follow) elif state == 'directory': result = ensure_directory(path, follow, recurse) elif state == 'link': - result = ensure_symlink(path, src, b_src, follow, force) + result = ensure_symlink(path, src, follow, force) elif state == 'hard': - result = ensure_hardlink(path, src, b_src, follow, force) + result = ensure_hardlink(path, src, follow, force) elif state == 'touch': result = execute_touch(path, follow) elif state == 'absent': diff --git a/test/integration/targets/file/tasks/main.yml b/test/integration/targets/file/tasks/main.yml index 9108d7649b7..51e3590dad4 100644 --- a/test/integration/targets/file/tasks/main.yml +++ b/test/integration/targets/file/tasks/main.yml @@ -134,7 +134,9 @@ - "file6_result.changed == true" - name: touch a hard link - file: src={{output_file}} dest={{output_dir}}/hard.txt state=touch + file: + dest: '{{ output_dir }}/hard.txt' + state: 'touch' register: file6_touch_result - name: verify that the hard link was touched