From 5cd489af061454565e036dd580d25e41487fbfcd Mon Sep 17 00:00:00 2001 From: Rick Elrod Date: Tue, 22 Sep 2020 16:15:47 -0500 Subject: [PATCH] Let get_file_attributes() work without `lsattr -v` (#71845) * Let get_file_attributes() work without `lsattr -v` Change: - module_utils's get_file_attributes() expects `lsattr -v` to work, but in some cases, it may not. - The function now takes an optional include_version bool parameter, which removes this expectation. - Places where we call get_file_attributes() without using the 'version' it returns, we now call it with include_version=False. Test Plan: - New unit tests Signed-off-by: Rick Elrod --- ...file_attributes-without-lsattr-version.yml | 2 ++ lib/ansible/module_utils/basic.py | 16 +++++++----- test/integration/targets/file/tasks/main.yml | 12 +++------ .../basic/test_get_file_attributes.py | 25 +++++++++++++++++++ 4 files changed, 41 insertions(+), 14 deletions(-) create mode 100644 changelogs/fragments/get_file_attributes-without-lsattr-version.yml diff --git a/changelogs/fragments/get_file_attributes-without-lsattr-version.yml b/changelogs/fragments/get_file_attributes-without-lsattr-version.yml new file mode 100644 index 00000000000..aaf17d18f17 --- /dev/null +++ b/changelogs/fragments/get_file_attributes-without-lsattr-version.yml @@ -0,0 +1,2 @@ +minor_changes: + - module_utils - ``get_file_attributes()`` now takes an optional ``include_version`` boolean parameter. When ``True`` (default), the file's version/generation number is included in the result (but requires ``lsattr -v`` to work on the target platform). diff --git a/lib/ansible/module_utils/basic.py b/lib/ansible/module_utils/basic.py index d46dfc9556d..00a04bf9048 100644 --- a/lib/ansible/module_utils/basic.py +++ b/lib/ansible/module_utils/basic.py @@ -1213,7 +1213,7 @@ class AnsibleModule(object): if self.check_file_absent_if_check_mode(b_path): return True - existing = self.get_file_attributes(b_path) + existing = self.get_file_attributes(b_path, include_version=False) attr_mod = '=' if attributes.startswith(('-', '+')): @@ -1244,17 +1244,21 @@ class AnsibleModule(object): details=to_native(e), exception=traceback.format_exc()) return changed - def get_file_attributes(self, path): + def get_file_attributes(self, path, include_version=True): output = {} attrcmd = self.get_bin_path('lsattr', False) if attrcmd: - attrcmd = [attrcmd, '-vd', path] + flags = '-vd' if include_version else '-d' + attrcmd = [attrcmd, flags, path] try: rc, out, err = self.run_command(attrcmd) if rc == 0: res = out.split() - output['attr_flags'] = res[1].replace('-', '').strip() - output['version'] = res[0].strip() + attr_flags_idx = 0 + if include_version: + attr_flags_idx = 1 + output['version'] = res[0].strip() + output['attr_flags'] = res[attr_flags_idx].replace('-', '').strip() output['attributes'] = format_attributes(output['attr_flags']) except Exception: pass @@ -2322,7 +2326,7 @@ class AnsibleModule(object): raise # Set the attributes - current_attribs = self.get_file_attributes(src) + current_attribs = self.get_file_attributes(src, include_version=False) current_attribs = current_attribs.get('attr_flags', '') self.set_attributes_if_different(dest, current_attribs, True) diff --git a/test/integration/targets/file/tasks/main.yml b/test/integration/targets/file/tasks/main.yml index 722a72f491e..aefbdd63b18 100644 --- a/test/integration/targets/file/tasks/main.yml +++ b/test/integration/targets/file/tasks/main.yml @@ -124,10 +124,7 @@ ignore_errors: yes - name: get attributes from file - # Use of `-v` is important, as that is what the module does (through `set_attributes_if_different` and then `get_file_attributes` in basic.py). - # On some systems, such as in containers, attributes work, but file versions may not. - # It should be possible to update `set_attributes_if_different` in the future to not use `-v` since the file version is unrelated to the attributes. - command: lsattr -vd "{{ attributes_file }}" + command: lsattr -d "{{ attributes_file }}" register: attribute_A_set ignore_errors: yes @@ -136,8 +133,7 @@ ignore_errors: yes - name: get attributes from file - # See the note above on use of the `-v` option. - command: lsattr -vd "{{ attributes_file }}" + command: lsattr -d "{{ attributes_file }}" register: attribute_A_unset ignore_errors: yes @@ -146,9 +142,9 @@ attributes_supported: yes when: - attribute_A_set is success - - "'A' in attribute_A_set.stdout_lines[0].split()[1]" + - "'A' in attribute_A_set.stdout_lines[0].split()[0]" - attribute_A_unset is success - - "'A' not in attribute_A_unset.stdout_lines[0].split()[1]" + - "'A' not in attribute_A_unset.stdout_lines[0].split()[0]" - name: explicitly set file attribute "A" file: path={{output_dir}}/baz.txt attributes=A diff --git a/test/units/module_utils/basic/test_get_file_attributes.py b/test/units/module_utils/basic/test_get_file_attributes.py index 5130a5fbc62..836529cce06 100644 --- a/test/units/module_utils/basic/test_get_file_attributes.py +++ b/test/units/module_utils/basic/test_get_file_attributes.py @@ -39,6 +39,21 @@ DATA = ( ), ) +NO_VERSION_DATA = ( + ( + '--------------e---- /usr/lib32', + {'attr_flags': 'e', 'attributes': ['extents']} + ), + ( + '-----------I--e---- /usr/lib', + {'attr_flags': 'Ie', 'attributes': ['indexed', 'extents']} + ), + ( + '-------A------e---- /tmp/test', + {'attr_flags': 'Ae', 'attributes': ['noatime', 'extents']} + ), +) + @pytest.mark.parametrize('stdin, data', product(({},), DATA), indirect=['stdin']) def test_get_file_attributes(am, stdin, mocker, data): @@ -48,3 +63,13 @@ def test_get_file_attributes(am, stdin, mocker, data): result = am.get_file_attributes('/path/to/file') for key, value in data[1].items(): assert key in result and result[key] == value + + +@pytest.mark.parametrize('stdin, data', product(({},), NO_VERSION_DATA), indirect=['stdin']) +def test_get_file_attributes_no_version(am, stdin, mocker, data): + # Test #18731 + mocker.patch.object(AnsibleModule, 'get_bin_path', return_value=(0, '/usr/bin/lsattr', '')) + mocker.patch.object(AnsibleModule, 'run_command', return_value=(0, data[0], '')) + result = am.get_file_attributes('/path/to/file', include_version=False) + for key, value in data[1].items(): + assert key in result and result[key] == value