Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

better cleanup on task results display #27175

Merged
merged 2 commits into from
Oct 16, 2017
Merged

better cleanup on task results display #27175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c6db2e5
better cleanup on task results display
bcoca Jul 21, 2017
c3a27df
moved import as per feedback
bcoca Oct 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 13 additions & 1 deletion lib/ansible/executor/task_queue_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
from ansible.errors import AnsibleError
from ansible.executor.play_iterator import PlayIterator
from ansible.executor.stats import AggregateStats
from ansible.executor.task_result import TaskResult
from ansible.module_utils.six import string_types
from ansible.module_utils._text import to_text
from ansible.playbook.block import Block
Expand Down Expand Up @@ -371,9 +372,20 @@ def send_callback(self, method_name, *args, **kwargs):
if gotit is not None:
methods.append(gotit)

# send clean copies
new_args = []
for arg in args:
# FIXME: add play/task cleaners
if isinstance(arg, TaskResult):
new_args.append(arg.clean_copy())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this might break more esoteric uses of callbacks (if something is relying on the original reference...).
But I think that is ok since that isnt really something we can (or should) guarantee.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

callbacks should not be manipulating the 'live objects', it can create issues and alter play flow in 'magical' ways the user would not understand/see/be able to audit.

In any case we are moving this here to avoid doing it in the callback itself should not worry about security nor disclosures.

# elif isinstance(arg, Play):
# elif isinstance(arg, Task):
else:
new_args.append(arg)

for method in methods:
try:
method(*args, **kwargs)
method(*new_args, **kwargs)
except Exception as e:
# TODO: add config toggle to make this fatal or not?
display.warning(u"Failure using method (%s) in callback plugin (%s): %s" % (to_text(method_name), to_text(callback_plugin), to_text(e)))
Expand Down
34 changes: 34 additions & 0 deletions lib/ansible/executor/task_result.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,12 @@
from __future__ import (absolute_import, division, print_function)
__metaclass__ = type

from copy import deepcopy

from ansible.parsing.dataloader import DataLoader
from ansible.vars.manager import strip_internal_keys

_IGNORE = ('changed', 'failed', 'skipped')


class TaskResult:
Expand Down Expand Up @@ -69,3 +74,32 @@ def _check_key(self, key):
if isinstance(res, dict):
flag |= res.get(key, False)
return flag

def clean_copy(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[note for potential future changes that doesnt need to be addressed now for this PR]

We have a couple of places where we do some variant of 'copy a dict, but then remove certain keys'. Might be worthwhile to generalize and add to utils and use.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should end up doing more than that, also creating mock task/play to avoid direct refs .. but that should not preclude such function


''' returns 'clean' taskresult object '''

# FIXME: clean task_fields, _task and _host copies
result = TaskResult(self._host, self._task, {}, self._task_fields)

# statuses are already reflected on the event type
if result._task and result._task.action in ['debug']:
# debug is verbose by default to display vars, no need to add invocation
ignore = _IGNORE + ('invocation',)
else:
ignore = _IGNORE

if self._result.get('_ansible_no_log', False):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is all for dealing with a TaskResult._result, move the _result processing code to it's own method and call from here?

def _clean_result(self, result, ignore):
<...>

It could even be staticmethod or module scope method

def clean_result_dict(result_dict, ignore):
   <..>
   return result_dict

so, clean_result() could:

    if self._result:
       result._result = clean_result(result._result, ignore)
    return result

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self._result is the start, planning on also cleaning up task and play

result._result = {"censored": "the output has been hidden due to the fact that 'no_log: true' was specified for this result"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could return here?

Then the next stanza doesn't need the extra indent level

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but i thought the preferred style was the 'single return' which we follow in most places, both do exact same thing.

elif self._result:
result._result = deepcopy(self._result)

# actualy remove
for remove_key in ignore:
if remove_key in result._result:
del result._result[remove_key]

# remove almost ALL internal keys, keep ones relevant to callback
strip_internal_keys(result._result, exceptions=('_ansible_verbose_always', '_ansible_item_label', '_ansible_no_log'))

return result
8 changes: 2 additions & 6 deletions lib/ansible/plugins/callback/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,6 @@ def set_options(self, options):
self._plugin_options = options

def _dump_results(self, result, indent=None, sort_keys=True, keep_invocation=False):
if result.get('_ansible_no_log', False):
return json.dumps(dict(censored="the output has been hidden due to the fact that 'no_log: true' was specified for this result"))

if not indent and (result.get('_ansible_verbose_always') or self._display.verbosity > 2):
indent = 4
Expand Down Expand Up @@ -219,10 +217,8 @@ def _process_items(self, result):
del result._result['results']

def _clean_results(self, result, task_name):
if task_name in ['debug']:
for remove_key in ('changed', 'invocation', 'failed', 'skipped'):
if remove_key in result:
del result[remove_key]
''' removes data from results for display '''
pass

def set_play_context(self, play_context):
pass
Expand Down
8 changes: 6 additions & 2 deletions lib/ansible/vars/manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,15 +72,19 @@ def preprocess_vars(a):
return data


def strip_internal_keys(dirty):
def strip_internal_keys(dirty, exceptions=None):
'''
All keys stating with _ansible_ are internal, so create a copy of the 'dirty' dict
and remove them from the clean one before returning it
'''

if exceptions is None:
exceptions = ()
clean = dirty.copy()
for k in dirty.keys():
if isinstance(k, string_types) and k.startswith('_ansible_'):
del clean[k]
if k not in exceptions:
del clean[k]
elif isinstance(dirty[k], dict):
clean[k] = strip_internal_keys(dirty[k])
return clean
Expand Down
9 changes: 9 additions & 0 deletions test/units/executor/test_task_result.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,3 +138,12 @@ def test_task_result_is_failed(self):
# test with failed_when in result
tr = TaskResult(mock_host, mock_task, dict(failed_when_result=True))
self.assertTrue(tr.is_failed())

def test_task_result_no_log(self):
mock_host = MagicMock()
mock_task = MagicMock()

# no_log should remove secrets
tr = TaskResult(mock_host, mock_task, dict(_ansible_no_log=True, secret='DONTSHOWME'))
clean = tr.clean_copy()
self.assertTrue('secret' not in clean._result)
10 changes: 0 additions & 10 deletions test/units/plugins/callback/test_callback.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,16 +93,6 @@ def test_internal_keys(self):
self.assertFalse('SENTINEL' in json_out)
self.assertTrue('LEFTIN' in json_out)

def test_no_log(self):
cb = CallbackBase()
result = {'item': 'some_item',
'_ansible_no_log': True,
'some_secrets': 'SENTINEL'}
json_out = cb._dump_results(result)
self.assertFalse('SENTINEL' in json_out)
self.assertTrue('no_log' in json_out)
self.assertTrue('output has been hidden' in json_out)

def test_exception(self):
cb = CallbackBase()
result = {'item': 'some_item LEFTIN',
Expand Down