bugfixes to JSON junk filter, added unit/integration tests to exercise (#17834)

7 files changed, 272 insertions, 84 deletions

diff --git a/lib/ansible/module_utils/json_utils.py b/lib/ansible/module_utils/json_utils.py
new file mode 100644
index 0000000000..d46626124b
--- /dev/null
+++ b/lib/ansible/module_utils/json_utils.py
@@ -0,0 +1,75 @@
+# This code is part of Ansible, but is an independent component.
+# This particular file snippet, and this file snippet only, is BSD licensed.
+# Modules you write using this snippet, which is embedded dynamically by Ansible
+# still belong to the author of the module, and may assign their own license
+# to the complete work.
+#
+# Redistribution and use in source and binary forms, with or without modification,
+# are permitted provided that the following conditions are met:
+#
+#    * Redistributions of source code must retain the above copyright
+#      notice, this list of conditions and the following disclaimer.
+#    * Redistributions in binary form must reproduce the above copyright notice,
+#      this list of conditions and the following disclaimer in the documentation
+#      and/or other materials provided with the distribution.
+#
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+# WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
+# IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
+# INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+# PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+# INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+# LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
+# USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+#
+
+try:
+    import json
+except ImportError:
+    import simplejson as json
+
+# NB: a copy of this function exists in ../../modules/core/async_wrapper.py. Ensure any
+# changes are propagated there.
+def _filter_non_json_lines(data):
+    '''
+    Used to filter unrelated output around module JSON output, like messages from
+    tcagetattr, or where dropbear spews MOTD on every single command (which is nuts).
+
+    Filters leading lines before first line-starting occurrence of '{' or '[', and filter all
+    trailing lines after matching close character (working from the bottom of output).
+    '''
+    warnings = []
+
+    # Filter initial junk
+    lines = data.splitlines()
+
+    for start, line in enumerate(lines):
+        line = line.strip()
+        if line.startswith(u'{'):
+            endchar = u'}'
+            break
+        elif line.startswith(u'['):
+            endchar = u']'
+            break
+    else:
+        raise ValueError('No start of json char found')
+
+    # Filter trailing junk
+    lines = lines[start:]
+
+    for reverse_end_offset, line in enumerate(reversed(lines)):
+        if line.strip().endswith(endchar):
+            break
+    else:
+        raise ValueError('No end of json char found')
+
+    if reverse_end_offset > 0:
+        # Trailing junk is uncommon and can point to things the user might
+        # want to change.  So print a warning if we find any
+        trailing_junk = lines[len(lines) - reverse_end_offset:]
+        warnings.append('Module invocation had junk after the JSON data: %s' % '\n'.join(trailing_junk))
+
+    lines = lines[:(len(lines) - reverse_end_offset)]
+
+    return ('\n'.join(lines), warnings)
diff --git a/lib/ansible/plugins/action/__init__.py b/lib/ansible/plugins/action/__init__.py
index 1a9805fbc0..75aa5c0fd3 100644
--- a/lib/ansible/plugins/action/__init__.py
+++ b/lib/ansible/plugins/action/__init__.py
@@ -36,6 +36,7 @@ from ansible import constants as C
 from ansible.errors import AnsibleError, AnsibleConnectionFailure
 from ansible.executor.module_common import modify_module
 from ansible.module_utils._text import to_bytes, to_native, to_text
+from ansible.module_utils.json_utils import _filter_non_json_lines
 from ansible.parsing.utils.jsonify import jsonify
 from ansible.release import __version__
 
@@ -503,50 +504,6 @@ class ActionBase(with_metaclass(ABCMeta, object)):
         else:
             return initial_fragment
 
-    @staticmethod
-    def _filter_non_json_lines(data):
-        '''
-        Used to avoid random output from SSH at the top of JSON output, like messages from
-        tcagetattr, or where dropbear spews MOTD on every single command (which is nuts).
-
-        need to filter anything which does not start with '{', '[', or is an empty line.
-        Have to be careful how we filter trailing junk as multiline JSON is valid.
-        '''
-        # Filter initial junk
-        lines = data.splitlines()
-        for start, line in enumerate(lines):
-            line = line.strip()
-            if line.startswith(u'{'):
-                endchar = u'}'
-                break
-            elif line.startswith(u'['):
-                endchar = u']'
-                break
-        else:
-            display.debug('No start of json char found')
-            raise ValueError('No start of json char found')
-
-        # Filter trailing junk
-        lines = lines[start:]
-        lines.reverse()
-        for end, line in enumerate(lines):
-            if line.strip().endswith(endchar):
-                break
-        else:
-            display.debug('No end of json char found')
-            raise ValueError('No end of json char found')
-
-        if end < len(lines) - 1:
-            # Trailing junk is uncommon and can point to things the user might
-            # want to change.  So print a warning if we find any
-            trailing_junk = lines[:end]
-            trailing_junk.reverse()
-            display.warning('Module invocation had junk after the JSON data: %s' % '\n'.join(trailing_junk))
-
-        lines = lines[end:]
-        lines.reverse()
-        return '\n'.join(lines)
-
     def _strip_success_message(self, data):
         '''
         Removes the BECOME-SUCCESS message from the data.
@@ -708,7 +665,10 @@ class ActionBase(with_metaclass(ABCMeta, object)):
 
     def _parse_returned_data(self, res):
         try:
-            data = json.loads(self._filter_non_json_lines(res.get('stdout', u'')))
+            filtered_output, warnings = _filter_non_json_lines(res.get('stdout', u''))
+            for w in warnings:
+                display.warning(w)
+            data = json.loads(filtered_output)
             data['_ansible_parsed'] = True
         except ValueError:
             # not valid json, lets try to capture error
diff --git a/test/integration/roles/test_async/library/async_test.py b/test/integration/roles/test_async/library/async_test.py
new file mode 100644
index 0000000000..5c77a27c8d
--- /dev/null
+++ b/test/integration/roles/test_async/library/async_test.py
@@ -0,0 +1,39 @@
+import sys
+import json
+from ansible.module_utils.basic import AnsibleModule
+
+def main():
+    if "--interactive" in sys.argv:
+        import ansible.module_utils.basic
+        ansible.module_utils.basic._ANSIBLE_ARGS = json.dumps(dict(
+            ANSIBLE_MODULE_ARGS=dict(
+                fail_mode="graceful"
+            )
+        ))
+
+    module = AnsibleModule(argument_spec = dict(
+        fail_mode = dict(type='list', default=['success'])
+        )
+    )
+
+    result = dict(changed=True)
+
+    fail_mode = module.params['fail_mode']
+
+    try:
+        if 'leading_junk' in fail_mode:
+            print("leading junk before module output")
+
+        if 'graceful' in fail_mode:
+            module.fail_json(msg="failed gracefully")
+
+        if 'exception' in fail_mode:
+            raise Exception('failing via exception')
+
+        module.exit_json(**result)
+
+    finally:
+        if 'trailing_junk' in fail_mode:
+            print("trailing junk after module output")
+
+main()
\ No newline at end of file
diff --git a/test/integration/roles/test_async/tasks/main.yml b/test/integration/roles/test_async/tasks/main.yml
index 8aa8f60ece..c6739dc256 100644
--- a/test/integration/roles/test_async/tasks/main.yml
+++ b/test/integration/roles/test_async/tasks/main.yml
@@ -87,3 +87,68 @@
   assert:
     that:
     - fnf_result.finished
+
+- name: test graceful module failure
+  async_test:
+    fail_mode: graceful
+  async: 30
+  poll: 1
+  register: async_result
+  ignore_errors: true
+
+- name: assert task failed correctly
+  assert:
+    that:
+    - async_result.ansible_job_id is match('\d+\.\d+')
+    - async_result.finished == 1
+    - async_result | changed == false
+    - async_result | failed
+    - async_result.msg == 'failed gracefully'
+
+- name: test exception module failure
+  async_test:
+    fail_mode: exception
+  async: 5
+  poll: 1
+  register: async_result
+  ignore_errors: true
+
+- name: validate response
+  assert:
+    that:
+    - async_result.ansible_job_id is match('\d+\.\d+')
+    - async_result.finished == 1
+    - async_result.changed == false
+    - async_result | failed == true
+    - async_result.stderr is search('failing via exception', multiline=True)
+
+- name: test leading junk before JSON
+  async_test:
+    fail_mode: leading_junk
+  async: 5
+  poll: 1
+  register: async_result
+
+- name: validate response
+  assert:
+    that:
+    - async_result.ansible_job_id is match('\d+\.\d+')
+    - async_result.finished == 1
+    - async_result.changed == true
+    - async_result | success
+
+- name: test trailing junk after JSON
+  async_test:
+    fail_mode: trailing_junk
+  async: 5
+  poll: 1
+  register: async_result
+
+- name: validate response
+  assert:
+    that:
+    - async_result.ansible_job_id is match('\d+\.\d+')
+    - async_result.finished == 1
+    - async_result.changed == true
+    - async_result | success
+    - async_result.warnings[0] is search('trailing junk after module output')
diff --git a/test/units/module_utils/json_utils/__init__.py b/test/units/module_utils/json_utils/__init__.py
new file mode 100644
index 0000000000..e69de29bb2
--- /dev/null
+++ b/test/units/module_utils/json_utils/__init__.py
diff --git a/test/units/module_utils/json_utils/test_filter_non_json_lines.py b/test/units/module_utils/json_utils/test_filter_non_json_lines.py
new file mode 100644
index 0000000000..b111cda040
--- /dev/null
+++ b/test/units/module_utils/json_utils/test_filter_non_json_lines.py
@@ -0,0 +1,88 @@
+# -*- coding: utf-8 -*-
+# (c) 2016, Matt Davis <mdavis@ansible.com>
+#
+# This file is part of Ansible
+#
+# Ansible is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# Ansible is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
+
+# Make coding more python3-ish
+from __future__ import (absolute_import, division)
+__metaclass__ = type
+
+import json
+
+from ansible.compat.tests import unittest
+from nose.tools import eq_, raises
+
+from ansible.module_utils.json_utils import _filter_non_json_lines
+
+class TestAnsibleModuleExitJson(unittest.TestCase):
+    single_line_json_dict = u"""{"key": "value", "olá": "mundo"}"""
+    single_line_json_array = u"""["a","b","c"]"""
+    multi_line_json_dict = u"""{
+"key":"value"
+}"""
+    multi_line_json_array = u"""[
+"a",
+"b",
+"c"]"""
+
+    all_inputs = [single_line_json_dict,
+              single_line_json_array,
+              multi_line_json_dict,
+              multi_line_json_array]
+
+    junk = [u"single line of junk", u"line 1/2 of junk\nline 2/2 of junk"]
+
+    unparsable_cases = (
+            u'No json here',
+            u'"olá": "mundo"',
+            u'{"No json": "ending"',
+            u'{"wrong": "ending"]',
+            u'["wrong": "ending"}',
+            )
+
+    def test_just_json(self):
+        for i in self.all_inputs:
+            filtered, warnings = _filter_non_json_lines(i)
+            self.assertEquals(filtered, i)
+            self.assertEquals(warnings, [])
+
+    def test_leading_junk(self):
+        for i in self.all_inputs:
+            for j in self.junk:
+                filtered, warnings = _filter_non_json_lines(j + "\n" + i)
+                self.assertEquals(filtered, i)
+                self.assertEquals(warnings, [])
+
+    def test_trailing_junk(self):
+        for i in self.all_inputs:
+            for j in self.junk:
+                filtered, warnings = _filter_non_json_lines(i + "\n" + j)
+                self.assertEquals(filtered, i)
+                self.assertEquals(warnings, [u"Module invocation had junk after the JSON data: %s" % j.strip()])
+
+    def test_leading_and_trailing_junk(self):
+        for i in self.all_inputs:
+            for j in self.junk:
+                filtered, warnings = _filter_non_json_lines("\n".join([j, i, j]))
+                self.assertEquals(filtered, i)
+                self.assertEquals(warnings, [u"Module invocation had junk after the JSON data: %s" % j.strip()])
+
+    def test_unparsable_filter_non_json_lines(self):
+        for i in self.unparsable_cases:
+            self.assertRaises(ValueError,
+                lambda data: _filter_non_json_lines(data),
+                data=i
+            )
diff --git a/test/units/plugins/action/test_action.py b/test/units/plugins/action/test_action.py
index d9b2d33864..091b50fb88 100644
--- a/test/units/plugins/action/test_action.py
+++ b/test/units/plugins/action/test_action.py
@@ -556,42 +556,3 @@ class TestActionBase(unittest.TestCase):
             play_context.make_become_cmd.assert_called_once_with("ECHO SAME", executable=None)
         finally:
             C.BECOME_ALLOW_SAME_USER = become_allow_same_user
-
-
-# Note: Using nose's generator test cases here so we can't inherit from
-# unittest.TestCase
-class TestFilterNonJsonLines(object):
-    parsable_cases = (
-            (u'{"hello": "world"}', u'{"hello": "world"}'),
-            (u'{"hello": "world"}\n', u'{"hello": "world"}'),
-            (u'{"hello": "world"} ', u'{"hello": "world"} '),
-            (u'{"hello": "world"} \n', u'{"hello": "world"} '),
-            (u'Message of the Day\n{"hello": "world"}', u'{"hello": "world"}'),
-            (u'{"hello": "world"}\nEpilogue', u'{"hello": "world"}'),
-            (u'Several\nStrings\nbefore\n{"hello": "world"}\nAnd\nAfter\n', u'{"hello": "world"}'),
-            (u'{"hello": "world",\n"olá": "mundo"}', u'{"hello": "world",\n"olá": "mundo"}'),
-            (u'\nPrecedent\n{"hello": "world",\n"olá": "mundo"}\nAntecedent', u'{"hello": "world",\n"olá": "mundo"}'),
-            )
-
-    unparsable_cases = (
-            u'No json here',
-            u'"olá": "mundo"',
-            u'{"No json": "ending"',
-            u'{"wrong": "ending"]',
-            u'["wrong": "ending"}',
-            )
-
-    def check_filter_non_json_lines(self, stdout_line, parsed):
-        eq_(parsed, ActionBase._filter_non_json_lines(stdout_line))
-
-    def test_filter_non_json_lines(self):
-        for stdout_line, parsed in self.parsable_cases:
-            yield self.check_filter_non_json_lines, stdout_line, parsed
-
-    @raises(ValueError)
-    def check_unparsable_filter_non_json_lines(self, stdout_line):
-        ActionBase._filter_non_json_lines(stdout_line)
-
-    def test_unparsable_filter_non_json_lines(self):
-        for stdout_line in self.unparsable_cases:
-            yield self.check_unparsable_filter_non_json_lines, stdout_line