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 <rick@elrod.me>

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).

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()
+                    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)
 

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).
+  command: lsattr -d "{{ attributes_file }}"
-  # 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 }}"
   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 -d "{{ attributes_file }}"
-  command: lsattr -vd "{{ 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

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