Repository: ansible/ansible
Pull request: file: return 'useful' error message for recursion error
##### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Fixes #56397
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Bugfix Pull Request

##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below -->
file

Problem statement:
state=directory and recursive=yes with symlink to the path throw an exception
<!--- Verify first that your issue is not already reported on GitHub -->
<!--- Also test if the latest release and devel branch are affected too -->
<!--- Complete *all* sections as described, this form is processed automatically -->

##### SUMMARY
<!--- Explain the problem briefly below -->
Exception thrown on maximum recursion depth exceeded for directory with symlink to that directory.

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below, use your best guess if unsure -->
module files

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes -->
```paste below
ansible 2.7.8
  config file = /home/user/repos/demo/ansible.cfg
  configured module search path = ['/home/user/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/local/lib/python3.5/dist-packages/ansible
  executable location = /usr/local/bin/ansible
  python version = 3.5.2 (default, Nov 12 2018, 13:43:14) [GCC 5.4.0 20160609]
```

##### CONFIGURATION
<!--- Paste verbatim output from "ansible-config dump --only-changed" between quotes -->
```paste below
DEFAULT_HASH_BEHAVIOUR(/home/user/repos/demo/ansible.cfg) = merge
DEFAULT_HOST_LIST(/home/user/repos/demo/ansible.cfg) = ['/home/user/repos/demo/hosts/all.yaml']
```

##### OS / ENVIRONMENT
<!--- Provide all relevant information below, e.g. target OS versions, network device firmware, etc. -->
Ubuntu 16.04

##### STEPS TO REPRODUCE
<!--- Describe exactly how to reproduce the problem, using a minimal test-case -->
```shell
ansible-playbook /tmp/demo-recurse.yml
```

<!--- Paste example playbooks or commands between quotes below -->
```yaml
# /tmp/demo-recurse.yml
- hosts: localhost
  tasks:
    - name: mkdir demo-recurse
      file:
        state: "directory"
        path: "/tmp/demo-recurse"
        owner: "{{ lookup('env', 'USER') }}"
        recurse: yes
    - name: link demo-recurse-link
      file:
        state: "link"
        path: "/tmp/demo-recurse/linktothisdir"
        src: "/tmp/demo-recurse"
        owner: "{{ lookup('env', 'USER') }}"
    - name: repeat demo-recurse
      file:
        state: "directory"
        path: "/tmp/demo-recurse"
        owner: "{{ lookup('env', 'USER') }}"
        recurse: yes
```

<!--- HINT: You can paste gist.github.com links for larger files -->

##### EXPECTED RESULTS
<!--- Describe what you expected to happen when running the steps above -->
Process link once per run.

##### ACTUAL RESULTS
<!--- Describe what actually happened. If possible run with extra verbosity (-vvvv) -->
Python exception thrown.

Hints from issue discussion:
Files identified in the description:
* [lib/ansible/modules/files/file.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/files/file.py)
* [lib/ansible/modules/system/filesystem.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/system/filesystem.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @abulimov @pilou-
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
An option of solution:
```diff
--- /usr/local/lib/python3.5/dist-packages/ansible/modules/files/file.py        2019-05-14 15:26:13.883871177 +0300
+++ /usr/local/lib/python3.5/dist-packages/ansible/modules/files/file.py        2019-05-14 15:25:55.024871438 +0300
@@ -283,8 +283,9 @@


 # This should be moved into the common file utilities
-def recursive_set_attributes(b_path, follow, file_args, mtime, atime):
+def recursive_set_attributes(b_path, follow, file_args, mtime, atime, processed_links=None):
     changed = False
+    processed_links = processed_links or []
     for b_root, b_dirs, b_files in os.walk(b_path):
         for b_fsobj in b_dirs + b_files:
             b_fsname = os.path.join(b_root, b_fsobj)
@@ -294,6 +295,9 @@
                 changed |= module.set_fs_attributes_if_different(tmp_file_args, changed, expand=False)
                 changed |= update_timestamp_for_file(tmp_file_args['path'], mtime, atime)
             else:
+                if b_fsname in processed_links:
+                    continue
+                processed_links.append(b_fsname)
                 # Change perms on the link
                 tmp_file_args = file_args.copy()
                 tmp_file_args['path'] = to_native(b_fsname, errors='surrogate_or_strict')
@@ -306,7 +310,7 @@
                     if os.path.exists(b_fsname):
                         if os.path.isdir(b_fsname):
                             # Link is a directory so change perms on the directory's contents
-                            changed |= recursive_set_attributes(b_fsname, follow, file_args, mtime, atime)
+                            changed |= recursive_set_attributes(b_fsname, follow, file_args, mtime, atime, processed_links)

                         # Change perms on the file pointed to by the link
                         tmp_file_args = file_args.copy()
@@ -882,3 +886,4 @@

 if __name__ == '__main__':
     main()
```
we should fail in this case, but not as a stacktrace, we are keeping the bug open to fix that last part and return a 'useful' error to the user.

we discussed keeping track of links and bypassing the recursion issue, but this does not reflect the expected outcome.

Commit message: Fix line length

Patch to review:
diff --git a/changelogs/fragments/56397-file-return-more-useful-error-for-recursion.yaml b/changelogs/fragments/56397-file-return-more-useful-error-for-recursion.yaml
new file mode 100644
index 00000000000000..70bf4d87bc6833
--- /dev/null
+++ b/changelogs/fragments/56397-file-return-more-useful-error-for-recursion.yaml
@@ -0,0 +1,2 @@
+bugfixes:
+  - file - return more useful error message for recursion error (https://github.com/ansible/ansible/issues/56397)
diff --git a/lib/ansible/modules/files/file.py b/lib/ansible/modules/files/file.py
index 63d6abd4662a03..6825c3ef24fe25 100644
--- a/lib/ansible/modules/files/file.py
+++ b/lib/ansible/modules/files/file.py
@@ -319,35 +319,44 @@ def get_state(path):
 # This should be moved into the common file utilities
 def recursive_set_attributes(b_path, follow, file_args, mtime, atime):
     changed = False
-    for b_root, b_dirs, b_files in os.walk(b_path):
-        for b_fsobj in b_dirs + b_files:
-            b_fsname = os.path.join(b_root, b_fsobj)
-            if not os.path.islink(b_fsname):
-                tmp_file_args = file_args.copy()
-                tmp_file_args['path'] = to_native(b_fsname, errors='surrogate_or_strict')
-                changed |= module.set_fs_attributes_if_different(tmp_file_args, changed, expand=False)
-                changed |= update_timestamp_for_file(tmp_file_args['path'], mtime, atime)
 
-            else:
-                # Change perms on the link
-                tmp_file_args = file_args.copy()
-                tmp_file_args['path'] = to_native(b_fsname, errors='surrogate_or_strict')
-                changed |= module.set_fs_attributes_if_different(tmp_file_args, changed, expand=False)
-                changed |= update_timestamp_for_file(tmp_file_args['path'], mtime, atime)
-
-                if follow:
-                    b_fsname = os.path.join(b_root, os.readlink(b_fsname))
-                    # The link target could be nonexistent
-                    if os.path.exists(b_fsname):
-                        if os.path.isdir(b_fsname):
-                            # Link is a directory so change perms on the directory's contents
-                            changed |= recursive_set_attributes(b_fsname, follow, file_args, mtime, atime)
-
-                        # Change perms on the file pointed to by the link
-                        tmp_file_args = file_args.copy()
-                        tmp_file_args['path'] = to_native(b_fsname, errors='surrogate_or_strict')
-                        changed |= module.set_fs_attributes_if_different(tmp_file_args, changed, expand=False)
-                        changed |= update_timestamp_for_file(tmp_file_args['path'], mtime, atime)
+    try:
+        for b_root, b_dirs, b_files in os.walk(b_path):
+            for b_fsobj in b_dirs + b_files:
+                b_fsname = os.path.join(b_root, b_fsobj)
+                if not os.path.islink(b_fsname):
+                    tmp_file_args = file_args.copy()
+                    tmp_file_args['path'] = to_native(b_fsname, errors='surrogate_or_strict')
+                    changed |= module.set_fs_attributes_if_different(tmp_file_args, changed, expand=False)
+                    changed |= update_timestamp_for_file(tmp_file_args['path'], mtime, atime)
+
+                else:
+                    # Change perms on the link
+                    tmp_file_args = file_args.copy()
+                    tmp_file_args['path'] = to_native(b_fsname, errors='surrogate_or_strict')
+                    changed |= module.set_fs_attributes_if_different(tmp_file_args, changed, expand=False)
+                    changed |= update_timestamp_for_file(tmp_file_args['path'], mtime, atime)
+
+                    if follow:
+                        b_fsname = os.path.join(b_root, os.readlink(b_fsname))
+                        # The link target could be nonexistent
+                        if os.path.exists(b_fsname):
+                            if os.path.isdir(b_fsname):
+                                # Link is a directory so change perms on the directory's contents
+                                changed |= recursive_set_attributes(b_fsname, follow, file_args, mtime, atime)
+
+                            # Change perms on the file pointed to by the link
+                            tmp_file_args = file_args.copy()
+                            tmp_file_args['path'] = to_native(b_fsname, errors='surrogate_or_strict')
+                            changed |= module.set_fs_attributes_if_different(tmp_file_args, changed, expand=False)
+                            changed |= update_timestamp_for_file(tmp_file_args['path'], mtime, atime)
+    except RuntimeError as e:
+        # on Python3 "RecursionError" is raised which is derived from "RuntimeError"
+        # TODO once this function is moved into the common file utilities, this should probably raise more general exception
+        raise AnsibleModuleError(
+            results={'msg': "An error occured when trying recursively setting attributes on %s. Original error was: '%s'" % (to_native(b_path), to_native(e))}
+        )
+
     return changed
 
 


Code review comments:
diff_hunk: @@ -319,35 +319,44 @@ def get_state(path):
 # This should be moved into the common file utilities
 def recursive_set_attributes(b_path, follow, file_args, mtime, atime):
     changed = False
-    for b_root, b_dirs, b_files in os.walk(b_path):
-        for b_fsobj in b_dirs + b_files:
-            b_fsname = os.path.join(b_root, b_fsobj)
-            if not os.path.islink(b_fsname):
-                tmp_file_args = file_args.copy()
-                tmp_file_args['path'] = to_native(b_fsname, errors='surrogate_or_strict')
-                changed |= module.set_fs_attributes_if_different(tmp_file_args, changed, expand=False)
-                changed |= update_timestamp_for_file(tmp_file_args['path'], mtime, atime)
 
-            else:
-                # Change perms on the link
-                tmp_file_args = file_args.copy()
-                tmp_file_args['path'] = to_native(b_fsname, errors='surrogate_or_strict')
-                changed |= module.set_fs_attributes_if_different(tmp_file_args, changed, expand=False)
-                changed |= update_timestamp_for_file(tmp_file_args['path'], mtime, atime)
-
-                if follow:
-                    b_fsname = os.path.join(b_root, os.readlink(b_fsname))
-                    # The link target could be nonexistent
-                    if os.path.exists(b_fsname):
-                        if os.path.isdir(b_fsname):
-                            # Link is a directory so change perms on the directory's contents
-                            changed |= recursive_set_attributes(b_fsname, follow, file_args, mtime, atime)
-
-                        # Change perms on the file pointed to by the link
-                        tmp_file_args = file_args.copy()
-                        tmp_file_args['path'] = to_native(b_fsname, errors='surrogate_or_strict')
-                        changed |= module.set_fs_attributes_if_different(tmp_file_args, changed, expand=False)
-                        changed |= update_timestamp_for_file(tmp_file_args['path'], mtime, atime)
+    try:
+        for b_root, b_dirs, b_files in os.walk(b_path):
+            for b_fsobj in b_dirs + b_files:
+                b_fsname = os.path.join(b_root, b_fsobj)
+                if not os.path.islink(b_fsname):
+                    tmp_file_args = file_args.copy()
+                    tmp_file_args['path'] = to_native(b_fsname, errors='surrogate_or_strict')
+                    changed |= module.set_fs_attributes_if_different(tmp_file_args, changed, expand=False)
+                    changed |= update_timestamp_for_file(tmp_file_args['path'], mtime, atime)
+
+                else:
+                    # Change perms on the link
+                    tmp_file_args = file_args.copy()
+                    tmp_file_args['path'] = to_native(b_fsname, errors='surrogate_or_strict')
+                    changed |= module.set_fs_attributes_if_different(tmp_file_args, changed, expand=False)
+                    changed |= update_timestamp_for_file(tmp_file_args['path'], mtime, atime)
+
+                    if follow:
+                        b_fsname = os.path.join(b_root, os.readlink(b_fsname))
+                        # The link target could be nonexistent
+                        if os.path.exists(b_fsname):
+                            if os.path.isdir(b_fsname):
+                                # Link is a directory so change perms on the directory's contents
+                                changed |= recursive_set_attributes(b_fsname, follow, file_args, mtime, atime)
+
+                            # Change perms on the file pointed to by the link
+                            tmp_file_args = file_args.copy()
+                            tmp_file_args['path'] = to_native(b_fsname, errors='surrogate_or_strict')
+                            changed |= module.set_fs_attributes_if_different(tmp_file_args, changed, expand=False)
+                            changed |= update_timestamp_for_file(tmp_file_args['path'], mtime, atime)
+    except RuntimeError as e:
+        # on Python3 "RecursionError" is raised which is derived from "RuntimeError"
+        # TODO once this function is moved into the common file utilities, this should probably raise more general exception
+        raise AnsibleModuleError(
+            results={'msg': "An error occured when trying recursively setting attributes on %s. Original error was: '%s'" % (to_native(b_path), to_native(e))}
path: lib/ansible/modules/files/file.py
text: @user1:
you should not need 'An error occurred' we already prefix these with ERROR or FAILURE

shouldn't we fail_json at this point?

@author:
> you should not need 'An error occurred' we already prefix these with ERROR or FAILURE

With this PR I get:
```
fatal: [localhost]: FAILED! => {"changed": false, "msg": "An error occured when trying recursively setting attributes on /tmp/demo-recurse. Original error was: 'maximum recursion depth exceeded while calling a Python object'"}
```

How would you re-phrase?

> 
> shouldn't we fail_json at this point?

Raising `AnsibleModuleError` triggers `fail_json`, see https://github.com/ansible/ansible/blob/f0c59d4cc08185ecaa6d1cf71f47c2442597bb7b/lib/ansible/modules/files/file.py#L238

@user1:
`Could not recursively set attri...`