Make fetch default to fail on errors

Fixes #23501

CHANGELOG.md

@@ -45,6 +45,9 @@ Ansible Changes By Release
 
   The new behaviour mirrors how the variables would appear if there was no hash
   mark in the string.
+- As of 2.4.0, the fetch module fails if there are errors reading the remote
+  file.  Use ignore_errors or failed_when in playbooks if you wish to ignore
+  errors.
 
 #### New Inventory scripts:
 - lxd

lib/ansible/modules/files/fetch.py

@@ -27,9 +27,7 @@ short_description: Fetches a file from remote nodes
 description:
      - This module works like M(copy), but in reverse. It is used for fetching
        files from remote machines and storing them locally in a file tree,
-       organized by hostname. Note that this module is written to transfer
+       organized by hostname.
-       log files that might not be present, so a missing remote file won't
-       be an error unless fail_on_missing is set to 'yes'.
 version_added: "0.2"
 options:
   src:
@@ -50,10 +48,13 @@ options:
   fail_on_missing:
     version_added: "1.1"
     description:
-      - When set to 'yes', the task will fail if the source file is missing.
+      - When set to 'yes', the task will fail if the remote file cannot be
+        read for any reason.  Prior to Ansible-2.4, setting this would only fail
+        if the source file was missing.
+      - The default was changed to "yes" in Ansible-2.4.
     required: false
     choices: [ "yes", "no" ]
-    default: "no"
+    default: "yes"
   validate_checksum:
     version_added: "1.4"
     description:
@@ -80,6 +81,11 @@ notes:
       depending on the file size can consume all available memory on the
       remote or local hosts causing a C(MemoryError). Due to this it is
       advisable to run this module without C(become) whenever possible.
+    - Prior to Ansible-2.4 this module would not fail if reading the remote
+      file was impossible unless fail_on_missing was set.  In Ansible-2.4+,
+      playbook authors are encouraged to use fail_when or ignore_errors to
+      get this ability.  They may also explicitly set fail_on_missing to False
+      to get the non-failing behaviour.
 '''
 
 EXAMPLES = '''

lib/ansible/plugins/action/__init__.py

@@ -518,6 +518,7 @@ class ActionBase(with_metaclass(ABCMeta, object)):
         2 = permissions issue
         3 = its a directory, not a file
         4 = stat module failed, likely due to not finding python
+        5 = appropriate json module not found
         '''
         x = "0"  # unknown error has occurred
         try:
@@ -532,6 +533,8 @@ class ActionBase(with_metaclass(ABCMeta, object)):
                 x = "2"  # cannot read file
             elif errormsg.endswith(u'MODULE FAILURE'):
                 x = "4"  # python not found or module uncaught exception
+            elif 'json' in errormsg or 'simplejson' in errormsg:
+                x = "5" # json or simplejson modules needed
         finally:
             return x
 

lib/ansible/plugins/action/fetch.py

@@ -122,26 +122,31 @@ class ActionModule(ActionBase):
 
         dest = dest.replace("//","/")
 
-        if remote_checksum in ('0', '1', '2', '3', '4'):
+        if remote_checksum in ('0', '1', '2', '3', '4', '5'):
-            # these don't fail because you may want to transfer a log file that
-            # possibly MAY exist but keep going to fetch other log files
             result['changed'] = False
             result['file'] = source
             if remote_checksum == '0':
                 result['msg'] = "unable to calculate the checksum of the remote file"
             elif remote_checksum == '1':
-                if fail_on_missing:
+                result['msg'] = "the remote file does not exist"
-                    result['failed'] = True
-                    del result['changed']
-                    result['msg'] = "the remote file does not exist"
-                else:
-                    result['msg'] = "the remote file does not exist, not transferring, ignored"
             elif remote_checksum == '2':
-                result['msg'] = "no read permission on remote file, not transferring, ignored"
+                result['msg'] = "no read permission on remote file"
             elif remote_checksum == '3':
                 result['msg'] = "remote file is a directory, fetch cannot work on directories"
             elif remote_checksum == '4':
                 result['msg'] = "python isn't present on the system.  Unable to compute checksum"
+            elif remote_checksum == '5':
+                result['msg'] = "stdlib json or simplejson was not found on the remote machine. Only the raw module can work without those installed"
+            # Historically, these don't fail because you may want to transfer
+            # a log file that possibly MAY exist but keep going to fetch other
+            # log files. Today, this is better achieved by adding
+            # ignore_errors or failed_when to the task.  Control the behaviour
+            # via fail_when_missing
+            if fail_on_missing:
+                result['failed'] = True
+                del result['changed']
+            else:
+                result['msg'] += ", not transferring, ignored"
             return result
 
         # calculate checksum for the local file
@@ -173,7 +178,9 @@ class ActionModule(ActionBase):
                     msg="checksum mismatch", file=source, dest=dest, remote_md5sum=None,
                     checksum=new_checksum, remote_checksum=remote_checksum))
             else:
-                result.update(dict(changed=True, md5sum=new_md5, dest=dest, remote_md5sum=None, checksum=new_checksum, remote_checksum=remote_checksum))
+                result.update({'changed': True, 'md5sum': new_md5, 'dest': dest,
+                               'remote_md5sum': None, 'checksum': new_checksum,
+                               'remote_checksum': remote_checksum})
         else:
             # For backwards compatibility. We'll return None on FIPS enabled systems
             try:

test/integration/targets/fetch/tasks/main.yml

@@ -56,7 +56,7 @@
       - 'fetched["checksum"] == "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3"'
 
 - name: attempt to fetch a non-existent file - do not fail on missing
-  fetch: src={{ output_dir }}/doesnotexist dest={{ output_dir }}/fetched
+  fetch: src={{ output_dir }}/doesnotexist dest={{ output_dir }}/fetched fail_on_missing=False
   register: fetch_missing_nofail
 
 - name: check fetch missing no fail result
@@ -78,7 +78,7 @@
       - "not fetch_missing|changed"
 
 - name: attempt to fetch a directory - should not fail but return a message
-  fetch: src={{ output_dir }} dest={{ output_dir }}/somedir
+  fetch: src={{ output_dir }} dest={{ output_dir }}/somedir fail_on_missing=False
   register: fetch_dir
 
 - name: check fetch directory result
@@ -87,6 +87,17 @@
       - "not fetch_dir|changed"
       - "fetch_dir.msg"
 
+- name: attempt to fetch a directory - should fail
+  fetch: src={{ output_dir }} dest={{ output_dir }}/somedir fail_on_missing=True
+  register: failed_fetch_dir
+  ignore_errors: true
+
+- name: check fetch directory result
+  assert:
+    that:
+      - "failed_fetch_dir['failed']"
+      - "fetch_dir.msg"
+
 - name: create symlink to a file that we can fetch
   file:
     path: "{{ output_dir }}/link"