Repository: ansible/ansible
Pull request: replace - better handling of file exceptions
##### SUMMARY

Existing code was not handling file exceptions. This adds exception handling around the `open` and `read` calls.

Fixes #47429

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Bugfix Pull Request


##### COMPONENT NAME
replace



Problem statement:
replace module - OSError: [Errno 22] Invalid argument
<!--- 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
When trying to use the replace module, to do a search and replace into a file, in local action, I get the deprecated issue. I did not get that issue until upgrade Ansible to 2.7.0 using Brew (which updated Python to 3.7 version at the same time).


##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
Replace module, in local action scope

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes -->
```
ansible 2.7.0
  config file = /Users/antoine/www/ansible-projects/ansible.cfg
  configured module search path = ['/Users/antoine/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/local/Cellar/ansible/2.7.0/libexec/lib/python3.7/site-packages/ansible
  executable location = /usr/local/bin/ansible
  python version = 3.7.0 (default, Oct  2 2018, 09:20:07) [Clang 10.0.0 (clang-1000.11.45.2)]
```

##### CONFIGURATION
<!--- Paste verbatim output from "ansible-config dump --only-changed" between quotes -->
```
DEFAULT_CALLBACK_WHITELIST(/Users/antoine/www/ansible-projects/ansible.cfg) = ['profile_roles']
INVENTORY_IGNORE_EXTS(/Users/antoine/www/ansible-projects/ansible.cfg) = ['~', '.orig', '.bak', '.ini', '.cfg', '.retry', '.pyc', '.pyo', '.sample']
```

##### OS / ENVIRONMENT
<!--- Provide all relevant information below, e.g. target OS versions, network device firmware, etc. -->
MacOS High Sierra 10.13.6

##### STEPS TO REPRODUCE
<!--- Describe exactly how to reproduce the problem, using a minimal test-case -->

<!--- Paste example playbooks or commands between quotes below -->
```yaml
- name: Remove 'xxx' by nothing
    local_action:
      module: replace
      dest: "yyy.sql"
      regexp: 'xxx'
      replace: ''
      backup: no
    tags: [xxx]
```

<!--- HINT: You can paste gist.github.com links for larger files -->

##### EXPECTED RESULTS
<!--- Describe what you expected to happen when running the steps above -->
No error

##### ACTUAL RESULTS
<!--- Describe what actually happened. If possible run with extra verbosity (-vvvv) -->

<!--- Paste verbatim command output between quotes -->
```paste below
fatal: [xxx.local -> localhost]: FAILED! => {"changed": false, "module_stderr": "/Users/antoine/.ansible/tmp/ansible-tmp-1540221672.821883-231750250440895/AnsiballZ_replace.py:17: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses\n  import imp\nTraceback (most recent call last):\n  File \"/Users/antoine/.ansible/tmp/ansible-tmp-1540221672.821883-231750250440895/AnsiballZ_replace.py\", line 113, in <module>\n    _ansiballz_main()\n  File \"/Users/antoine/.ansible/tmp/ansible-tmp-1540221672.821883-231750250440895/AnsiballZ_replace.py\", line 105, in _ansiballz_main\n    invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)\n  File \"/Users/antoine/.ansible/tmp/ansible-tmp-1540221672.821883-231750250440895/AnsiballZ_replace.py\", line 48, in invoke_module\n    imp.load_module('__main__', mod, module, MOD_DESC)\n  File \"/usr/local/Cellar/ansible/2.7.0/libexec/lib/python3.7/imp.py\", line 235, in load_module\n    return load_source(name, filename, file)\n  File \"/usr/local/Cellar/ansible/2.7.0/libexec/lib/python3.7/imp.py\", line 170, in load_source\n    module = _exec(spec, sys.modules[name])\n  File \"<frozen importlib._bootstrap>\", line 630, in _exec\n  File \"<frozen importlib._bootstrap_external>\", line 728, in exec_module\n  File \"<frozen importlib._bootstrap>\", line 219, in _call_with_frames_removed\n  File \"/var/folders/9j/cdpx8gks7r18f1pscsnybcsm0000gn/T/ansible_replace_payload_jq9mtu2o/__main__.py\", line 276, in <module>\n  File \"/var/folders/9j/cdpx8gks7r18f1pscsnybcsm0000gn/T/ansible_replace_payload_jq9mtu2o/__main__.py\", line 222, in main\nOSError: [Errno 22] Invalid argument\n", "module_stdout": "", "msg": "MODULE FAILURE\nSee stdout/stderr for the exact error", "rc": 1}

```

Hints from issue discussion:
Full error:

```
/Users/antoine/.ansible/tmp/ansible-tmp-1540221672.821883-231750250440895/AnsiballZ_replace.py:17: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
  import imp
Traceback (most recent call last):
  File \"/Users/antoine/.ansible/tmp/ansible-tmp-1540221672.821883-231750250440895/AnsiballZ_replace.py\", line 113, in <module>
    _ansiballz_main()
  File \"/Users/antoine/.ansible/tmp/ansible-tmp-1540221672.821883-231750250440895/AnsiballZ_replace.py\", line 105, in _ansiballz_main
    invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)
  File \"/Users/antoine/.ansible/tmp/ansible-tmp-1540221672.821883-231750250440895/AnsiballZ_replace.py\", line 48, in invoke_module
    imp.load_module('__main__', mod, module, MOD_DESC)
  File \"/usr/local/Cellar/ansible/2.7.0/libexec/lib/python3.7/imp.py\", line 235, in load_module
    return load_source(name, filename, file)
  File \"/usr/local/Cellar/ansible/2.7.0/libexec/lib/python3.7/imp.py\", line 170, in load_source
    module = _exec(spec, sys.modules[name])
  File \"<frozen importlib._bootstrap>\", line 630, in _exec
  File \"<frozen importlib._bootstrap_external>\", line 728, in exec_module
  File \"<frozen importlib._bootstrap>\", line 219, in _call_with_frames_removed
  File \"/var/folders/9j/cdpx8gks7r18f1pscsnybcsm0000gn/T/ansible_replace_payload_jq9mtu2o/__main__.py\", line 276, in <module>
  File \"/var/folders/9j/cdpx8gks7r18f1pscsnybcsm0000gn/T/ansible_replace_payload_jq9mtu2o/__main__.py\", line 222, in main
OSError: [Errno 22] Invalid argument
```
Although there is a warning here about `imp`, the real problem seems to be with reading the file specified with `dest`.
Line 222 of replace.py:

https://github.com/ansible/ansible/blob/0a07068054090d5b78b27496aa251be74c484b45/lib/ansible/modules/files/replace.py#L222
Hi @antoinekociuba, thank you for submitting this issue!

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: thank_submitter --->
Hi guys,

If that can help you to investigate, I'd like to precise that that issue is not poping out all the time.
My current feeling is the following:

When the dest file is heavy (~4Go in my case), I get the reported error.
When I try with a tiny file (~30Mo), it works like a charm.

Before (Ansible 2.6/Python 2), everything was working so still an issue there, or at least, an incorrect error handling.

Hope it can help you, and thx in advance for your help.
Any news on that guys? Still facing the same issue with Ansible 2.7.1. Thx
cc @EvanK
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->

Commit message: add cl

Patch to review:
diff --git a/changelogs/fragments/74686-replace-handle-file-exc.yml b/changelogs/fragments/74686-replace-handle-file-exc.yml
new file mode 100644
index 00000000000000..8be2daed04fb30
--- /dev/null
+++ b/changelogs/fragments/74686-replace-handle-file-exc.yml
@@ -0,0 +1,2 @@
+bugfixes:
+    - replace - better handling of file operation exceptions (https://github.com/ansible/ansible/pull/74686).
diff --git a/lib/ansible/modules/replace.py b/lib/ansible/modules/replace.py
index ed9fd1f2873dac..13f690d9fc8d8f 100644
--- a/lib/ansible/modules/replace.py
+++ b/lib/ansible/modules/replace.py
@@ -171,6 +171,7 @@
 import os
 import re
 import tempfile
+from traceback import format_exc
 
 from ansible.module_utils._text import to_text, to_bytes
 from ansible.module_utils.basic import AnsibleModule
@@ -242,9 +243,11 @@ def main():
     if not os.path.exists(path):
         module.fail_json(rc=257, msg='Path %s does not exist !' % path)
     else:
-        f = open(path, 'rb')
-        contents = to_text(f.read(), errors='surrogate_or_strict', encoding=encoding)
-        f.close()
+        try:
+            with open(path, 'rb') as f:
+                contents = to_text(f.read(), errors='surrogate_or_strict', encoding=encoding)
+        except Exception:
+            module.fail_json(msg='Unable to read the contents of %s' % path, exception=format_exc())
 
     pattern = u''
     if params['after'] and params['before']:


Code review comments:
diff_hunk: @@ -242,9 +243,11 @@ def main():
     if not os.path.exists(path):
         module.fail_json(rc=257, msg='Path %s does not exist !' % path)
     else:
-        f = open(path, 'rb')
-        contents = to_text(f.read(), errors='surrogate_or_strict', encoding=encoding)
-        f.close()
+        try:
+            with open(path, 'rb') as f:
+                contents = to_text(f.read(), errors='surrogate_or_strict', encoding=encoding)
+        except Exception:
+            module.fail_json(msg='Unable to read the contents of %s' % path, exception=format_exc())
path: lib/ansible/modules/replace.py
text: @user1:
i would add `: %s` and to_text(e)` to the end of msg

diff_hunk: @@ -242,9 +243,11 @@ def main():
     if not os.path.exists(path):
         module.fail_json(rc=257, msg='Path %s does not exist !' % path)
     else:
-        f = open(path, 'rb')
-        contents = to_text(f.read(), errors='surrogate_or_strict', encoding=encoding)
-        f.close()
+        try:
+            with open(path, 'rb') as f:
+                contents = to_text(f.read(), errors='surrogate_or_strict', encoding=encoding)
+        except Exception:
path: lib/ansible/modules/replace.py
text: @user1:
you could narrow this down to oserror/ioerror