From 4befefd78c2a19c21e0252860f389d5bb7274eb0 Mon Sep 17 00:00:00 2001 From: Adrian Likins Date: Fri, 9 Jun 2017 13:13:15 -0400 Subject: [PATCH] Try to show original exception info for yaml (and other) errors (#24468) * show original exception for yaml (and other) errors In places where we need to catch a yaml error and raise an AnsibleError, add the orig yaml exc to the AnsibleError via the orig_exc arg. When the AnsibleError is displayed it will now include the AnsibleError (AnsibleParserError for example) and the type and message from the original yaml exception. This provides more detail to the error messages related to yaml errors. This also improves errors from dataloader (for example, previously if a wrong password was used for a vault encrypted yaml file, the error was very vague and suggested yaml errors, but now the message includes the original exception from vault indicating the password was incorrect or missing). Add a text note to playbook helper asserts. For playbook syntax/layout errors that aren't yaml errors, but errors indicating invalid data structures for a playbook/task/role/block, we now include some info about where the assert was and why it was raised. In places we raise an AnsibleParserError in an except clause, pass the original exception to AnsibleParserError via orig_exc arg. Make assorted error messages a little more specific (like the playbook helper load methods) * Revert "Include the original YAML error in syntax error messages" This reverts commit 781bb44b029becef60af9c9ce765129c7c9c7287. --- lib/ansible/errors/__init__.py | 6 +++++- lib/ansible/parsing/dataloader.py | 11 +++-------- lib/ansible/parsing/mod_args.py | 3 ++- lib/ansible/parsing/splitter.py | 2 +- lib/ansible/parsing/yaml/constructor.py | 5 ++++- lib/ansible/parsing/yaml/objects.py | 2 +- lib/ansible/playbook/base.py | 19 ++++++++++--------- lib/ansible/playbook/block.py | 12 ++++++------ lib/ansible/playbook/helpers.py | 12 ++++++------ lib/ansible/playbook/play.py | 22 +++++++++++----------- lib/ansible/playbook/playbook_include.py | 2 +- lib/ansible/playbook/role/__init__.py | 10 ++++++---- lib/ansible/playbook/role/metadata.py | 6 +++--- lib/ansible/playbook/task.py | 4 ++-- lib/ansible/template/__init__.py | 2 +- lib/ansible/vars/manager.py | 8 ++++---- 16 files changed, 66 insertions(+), 60 deletions(-) diff --git a/lib/ansible/errors/__init__.py b/lib/ansible/errors/__init__.py index 2acdefab0b0..97ee3fc413f 100644 --- a/lib/ansible/errors/__init__.py +++ b/lib/ansible/errors/__init__.py @@ -46,7 +46,7 @@ class AnsibleError(Exception): which should be returned by the DataLoader() class. ''' - def __init__(self, message="", obj=None, show_content=True, suppress_extended_error=False): + def __init__(self, message="", obj=None, show_content=True, suppress_extended_error=False, orig_exc=None): # we import this here to prevent an import loop problem, # since the objects code also imports ansible.errors from ansible.parsing.yaml.objects import AnsibleBaseYAMLObject @@ -61,6 +61,10 @@ class AnsibleError(Exception): self.message = '%s' % to_native(message) else: self.message = '%s' % to_native(message) + if orig_exc: + self.orig_exc = orig_exc + self.message += '\nexception type: %s' % to_native(type(orig_exc)) + self.message += '\nexception: %s' % to_native(orig_exc) def __str__(self): return self.message diff --git a/lib/ansible/parsing/dataloader.py b/lib/ansible/parsing/dataloader.py index c436926339f..9d40a3d6b08 100644 --- a/lib/ansible/parsing/dataloader.py +++ b/lib/ansible/parsing/dataloader.py @@ -186,7 +186,7 @@ class DataLoader: return (data, show_content) except (IOError, OSError) as e: - raise AnsibleParserError("an error occurred while trying to read the file '%s': %s" % (file_name, str(e))) + raise AnsibleParserError("an error occurred while trying to read the file '%s': %s" % (file_name, str(e)), orig_exc=e) def _handle_error(self, yaml_exc, file_name, show_content): ''' @@ -202,12 +202,7 @@ class DataLoader: err_obj = AnsibleBaseYAMLObject() err_obj.ansible_pos = (file_name, yaml_exc.problem_mark.line + 1, yaml_exc.problem_mark.column + 1) - if hasattr(yaml_exc, 'problem'): - err_str = "%s\nThe YAML error was:\n > %s" % (YAML_SYNTAX_ERROR.strip(), yaml_exc.problem) - else: - err_str = YAML_SYNTAX_ERROR - - raise AnsibleParserError(err_str, obj=err_obj, show_content=show_content) + raise AnsibleParserError(YAML_SYNTAX_ERROR, obj=err_obj, show_content=show_content, orig_exc=yaml_exc) def get_basedir(self): ''' returns the current basedir ''' @@ -424,7 +419,7 @@ class DataLoader: return real_path except (IOError, OSError) as e: - raise AnsibleParserError("an error occurred while trying to read the file '%s': %s" % (to_native(real_path), to_native(e))) + raise AnsibleParserError("an error occurred while trying to read the file '%s': %s" % (to_native(real_path), to_native(e)), orig_exc=e) def cleanup_tmp_file(self, file_path): """ diff --git a/lib/ansible/parsing/mod_args.py b/lib/ansible/parsing/mod_args.py index d48a3d2468b..ad0bf9c70af 100644 --- a/lib/ansible/parsing/mod_args.py +++ b/lib/ansible/parsing/mod_args.py @@ -95,8 +95,9 @@ class ModuleArgsParser: Args may also be munged for certain shell command parameters. """ + # FIXME: mutable default arg def __init__(self, task_ds=dict()): - assert isinstance(task_ds, dict) + assert isinstance(task_ds, dict), "the type of 'task_ds' should be a dict, but is a %s" % type(task_ds) self._task_ds = task_ds def _split_module_string(self, module_string): diff --git a/lib/ansible/parsing/splitter.py b/lib/ansible/parsing/splitter.py index 5008dd3a6a2..0adb4bf84b6 100644 --- a/lib/ansible/parsing/splitter.py +++ b/lib/ansible/parsing/splitter.py @@ -62,7 +62,7 @@ def parse_kv(args, check_raw=False): vargs = split_args(args) except ValueError as ve: if 'no closing quotation' in str(ve).lower(): - raise AnsibleParserError("error parsing argument string, try quoting the entire line.") + raise AnsibleParserError("error parsing argument string, try quoting the entire line.", orig_exc=ve) else: raise diff --git a/lib/ansible/parsing/yaml/constructor.py b/lib/ansible/parsing/yaml/constructor.py index 0e3a21acb90..67530892832 100644 --- a/lib/ansible/parsing/yaml/constructor.py +++ b/lib/ansible/parsing/yaml/constructor.py @@ -99,7 +99,10 @@ class AnsibleConstructor(SafeConstructor): ciphertext_data = to_bytes(value) if self._b_vault_password is None: - raise ConstructorError(None, None, "found vault but no vault password provided", node.start_mark) + raise ConstructorError(context=None, context_mark=None, + problem="found !vault but no vault password provided", + problem_mark=node.start_mark, + note=None) # could pass in a key id here to choose the vault to associate with vault = self._vaults['default'] diff --git a/lib/ansible/parsing/yaml/objects.py b/lib/ansible/parsing/yaml/objects.py index 41df711a811..99913811327 100644 --- a/lib/ansible/parsing/yaml/objects.py +++ b/lib/ansible/parsing/yaml/objects.py @@ -70,7 +70,7 @@ class AnsibleSequence(AnsibleBaseYAMLObject, list): # Unicode like object that is not evaluated (decrypted) until it needs to be # TODO: is there a reason these objects are subclasses for YAMLObject? -class AnsibleVaultEncryptedUnicode(yaml.YAMLObject, AnsibleUnicode): +class AnsibleVaultEncryptedUnicode(yaml.YAMLObject, AnsibleBaseYAMLObject): __UNSAFE__ = True __ENCRYPTED__ = True yaml_tag = u'!vault' diff --git a/lib/ansible/playbook/base.py b/lib/ansible/playbook/base.py index cdafd5f25db..38091172fda 100644 --- a/lib/ansible/playbook/base.py +++ b/lib/ansible/playbook/base.py @@ -220,7 +220,7 @@ class Base(with_metaclass(BaseMeta, object)): def load_data(self, ds, variable_manager=None, loader=None): ''' walk the input datastructure and assign any values ''' - assert ds is not None + assert ds is not None, 'ds (%s) should not be None but it is.' % ds # cache the datastructure internally setattr(self, '_ds', ds) @@ -440,12 +440,12 @@ class Base(with_metaclass(BaseMeta, object)): setattr(self, name, value) except (TypeError, ValueError) as e: - raise AnsibleParserError("the field '%s' has an invalid value (%s), and could not be converted to an %s. " - "The error was: %s" % (name, value, attribute.isa, e), obj=self.get_ds()) + raise AnsibleParserError("the field '%s' has an invalid value (%s), and could not be converted to an %s." + "The error was: %s" % (name, value, attribute.isa, e), obj=self.get_ds(), orig_exc=e) except (AnsibleUndefinedVariable, UndefinedError) as e: if templar._fail_on_undefined_errors and name != 'name': - raise AnsibleParserError("the field '%s' has an invalid value, which appears to include a variable that is undefined. " - "The error was: %s" % (name, e), obj=self.get_ds()) + raise AnsibleParserError("the field '%s' has an invalid value, which appears to include a variable that is undefined." + "The error was: %s" % (name, e), obj=self.get_ds(), orig_exc=e) self._finalized = True @@ -477,10 +477,11 @@ class Base(with_metaclass(BaseMeta, object)): return {} else: raise ValueError - except ValueError: - raise AnsibleParserError("Vars in a %s must be specified as a dictionary, or a list of dictionaries" % self.__class__.__name__, obj=ds) + except ValueError as e: + raise AnsibleParserError("Vars in a %s must be specified as a dictionary, or a list of dictionaries" % self.__class__.__name__, + obj=ds, orig_exc=e) except TypeError as e: - raise AnsibleParserError("Invalid variable name in vars specified for %s: %s" % (self.__class__.__name__, e), obj=ds) + raise AnsibleParserError("Invalid variable name in vars specified for %s: %s" % (self.__class__.__name__, e), obj=ds, orig_exc=e) def _extend_value(self, value, new_value, prepend=False): ''' @@ -554,7 +555,7 @@ class Base(with_metaclass(BaseMeta, object)): and extended. ''' - assert isinstance(data, dict) + assert isinstance(data, dict), 'data (%s) should be a dict but is a %s' % (data, type(data)) for (name, attribute) in iteritems(self._valid_attrs): if name in data: diff --git a/lib/ansible/playbook/block.py b/lib/ansible/playbook/block.py index 432db55339b..85c5f8b96ea 100644 --- a/lib/ansible/playbook/block.py +++ b/lib/ansible/playbook/block.py @@ -121,8 +121,8 @@ class Block(Base, Become, Conditional, Taggable): loader=self._loader, use_handlers=self._use_handlers, ) - except AssertionError: - raise AnsibleParserError("A malformed block was encountered.", obj=self._ds) + except AssertionError as e: + raise AnsibleParserError("A malformed block was encountered while loading a block", obj=self._ds, orig_exc=e) def _load_rescue(self, attr, ds): try: @@ -136,8 +136,8 @@ class Block(Base, Become, Conditional, Taggable): loader=self._loader, use_handlers=self._use_handlers, ) - except AssertionError: - raise AnsibleParserError("A malformed block was encountered.", obj=self._ds) + except AssertionError as e: + raise AnsibleParserError("A malformed block was encountered while loading rescue.", obj=self._ds, orig_exc=e) def _load_always(self, attr, ds): try: @@ -151,8 +151,8 @@ class Block(Base, Become, Conditional, Taggable): loader=self._loader, use_handlers=self._use_handlers, ) - except AssertionError: - raise AnsibleParserError("A malformed block was encountered.", obj=self._ds) + except AssertionError as e: + raise AnsibleParserError("A malformed block was encountered while loading always", obj=self._ds, orig_exc=e) def get_dep_chain(self): if self._dep_chain is None: diff --git a/lib/ansible/playbook/helpers.py b/lib/ansible/playbook/helpers.py index c347a80713f..ab08b6ecc47 100644 --- a/lib/ansible/playbook/helpers.py +++ b/lib/ansible/playbook/helpers.py @@ -43,7 +43,7 @@ def load_list_of_blocks(ds, play, parent_block=None, role=None, task_include=Non from ansible.playbook.task_include import TaskInclude from ansible.playbook.role_include import IncludeRole - assert isinstance(ds, (list, type(None))) + assert isinstance(ds, (list, type(None))), '%s should be a list or None but is %s' % (ds, type(ds)) block_list = [] if ds: @@ -89,11 +89,11 @@ def load_list_of_tasks(ds, play, block=None, role=None, task_include=None, use_h from ansible.playbook.handler_task_include import HandlerTaskInclude from ansible.template import Templar - assert isinstance(ds, list) + assert isinstance(ds, list), 'The ds (%s) should be a list but was a %s' % (ds, type(ds)) task_list = [] for task_ds in ds: - assert isinstance(task_ds, dict) + assert isinstance(task_ds, dict), 'The ds (%s) should be a dict but was a %s' % (ds, type(ds)) if 'block' in task_ds: t = Block.load( @@ -190,7 +190,7 @@ def load_list_of_tasks(ds, play, block=None, role=None, task_include=None, use_h if not found: try: include_target = templar.template(t.args['_raw_params']) - except AnsibleUndefinedVariable: + except AnsibleUndefinedVariable as e: raise AnsibleParserError( "Error when evaluating variable in include name: %s.\n\n" "When using static includes, ensure that any variables used in their names are defined in vars/vars_files\n" @@ -198,7 +198,7 @@ def load_list_of_tasks(ds, play, block=None, role=None, task_include=None, use_h "sources like group or host vars." % t.args['_raw_params'], obj=task_ds, suppress_extended_error=True, - ) + orig_exc=e) if t._role: include_file = loader.path_dwim_relative(t._role._role_path, subdir, include_target) else: @@ -341,7 +341,7 @@ def load_list_of_roles(ds, play, current_role_path=None, variable_manager=None, # we import here to prevent a circular dependency with imports from ansible.playbook.role.include import RoleInclude - assert isinstance(ds, list) + assert isinstance(ds, list), 'ds (%s) should be a list but was a %s' % (ds, type(ds)) roles = [] for role_def in ds: diff --git a/lib/ansible/playbook/play.py b/lib/ansible/playbook/play.py index f0a87b8a798..b9b5e3d3ac4 100644 --- a/lib/ansible/playbook/play.py +++ b/lib/ansible/playbook/play.py @@ -121,7 +121,7 @@ class Play(Base, Taggable, Become): Adjusts play datastructure to cleanup old/legacy items ''' - assert isinstance(ds, dict) + assert isinstance(ds, dict), 'while preprocessing data (%s), ds should be a dict but was a %s' % (ds, type(ds)) # The use of 'user' in the Play datastructure was deprecated to # line up with the same change for Tasks, due to the fact that @@ -145,8 +145,8 @@ class Play(Base, Taggable, Become): ''' try: return load_list_of_blocks(ds=ds, play=self, variable_manager=self._variable_manager, loader=self._loader) - except AssertionError: - raise AnsibleParserError("A malformed block was encountered.", obj=self._ds) + except AssertionError as e: + raise AnsibleParserError("A malformed block was encountered while loading tasks", obj=self._ds, orig_exc=e) def _load_pre_tasks(self, attr, ds): ''' @@ -155,8 +155,8 @@ class Play(Base, Taggable, Become): ''' try: return load_list_of_blocks(ds=ds, play=self, variable_manager=self._variable_manager, loader=self._loader) - except AssertionError: - raise AnsibleParserError("A malformed block was encountered.", obj=self._ds) + except AssertionError as e: + raise AnsibleParserError("A malformed block was encountered while loading pre_tasks", obj=self._ds, orig_exc=e) def _load_post_tasks(self, attr, ds): ''' @@ -165,8 +165,8 @@ class Play(Base, Taggable, Become): ''' try: return load_list_of_blocks(ds=ds, play=self, variable_manager=self._variable_manager, loader=self._loader) - except AssertionError: - raise AnsibleParserError("A malformed block was encountered.", obj=self._ds) + except AssertionError as e: + raise AnsibleParserError("A malformed block was encountered while loading post_tasks", obj=self._ds, orig_exc=e) def _load_handlers(self, attr, ds): ''' @@ -175,8 +175,8 @@ class Play(Base, Taggable, Become): ''' try: return load_list_of_blocks(ds=ds, play=self, use_handlers=True, variable_manager=self._variable_manager, loader=self._loader) - except AssertionError: - raise AnsibleParserError("A malformed block was encountered.", obj=self._ds) + except AssertionError as e: + raise AnsibleParserError("A malformed block was encountered while loading handlers", obj=self._ds, orig_exc=e) def _load_roles(self, attr, ds): ''' @@ -189,8 +189,8 @@ class Play(Base, Taggable, Become): try: role_includes = load_list_of_roles(ds, play=self, variable_manager=self._variable_manager, loader=self._loader) - except AssertionError: - raise AnsibleParserError("A malformed role declaration was encountered.", obj=self._ds) + except AssertionError as e: + raise AnsibleParserError("A malformed role declaration was encountered.", obj=self._ds, orig_exc=e) roles = [] for ri in role_includes: diff --git a/lib/ansible/playbook/playbook_include.py b/lib/ansible/playbook/playbook_include.py index 04423ff6ec8..fc5a32bf7b1 100644 --- a/lib/ansible/playbook/playbook_include.py +++ b/lib/ansible/playbook/playbook_include.py @@ -105,7 +105,7 @@ class PlaybookInclude(Base, Conditional, Taggable): up with what we expect the proper attributes to be ''' - assert isinstance(ds, dict) + assert isinstance(ds, dict), 'ds (%s) should be a dict but was a %s' % (ds, type(ds)) # the new, cleaned datastructure, which will have legacy # items reduced to a standard structure diff --git a/lib/ansible/playbook/role/__init__.py b/lib/ansible/playbook/role/__init__.py index 2c1d485fb3f..d9805b00fa9 100644 --- a/lib/ansible/playbook/role/__init__.py +++ b/lib/ansible/playbook/role/__init__.py @@ -209,16 +209,18 @@ class Role(Base, Become, Conditional, Taggable): if task_data: try: self._task_blocks = load_list_of_blocks(task_data, play=self._play, role=self, loader=self._loader, variable_manager=self._variable_manager) - except AssertionError: - raise AnsibleParserError("The tasks/main.yml file for role '%s' must contain a list of tasks" % self._role_name, obj=task_data) + except AssertionError as e: + raise AnsibleParserError("The tasks/main.yml file for role '%s' must contain a list of tasks" % self._role_name, + obj=task_data, orig_exc=e) handler_data = self._load_role_yaml('handlers') if handler_data: try: self._handler_blocks = load_list_of_blocks(handler_data, play=self._play, role=self, use_handlers=True, loader=self._loader, variable_manager=self._variable_manager) - except AssertionError: - raise AnsibleParserError("The handlers/main.yml file for role '%s' must contain a list of tasks" % self._role_name, obj=handler_data) + except AssertionError as e: + raise AnsibleParserError("The handlers/main.yml file for role '%s' must contain a list of tasks" % self._role_name, + obj=handler_data, orig_exc=e) # vars and default vars are regular dictionaries self._role_vars = self._load_role_yaml('vars', main=self._from_files.get('vars')) diff --git a/lib/ansible/playbook/role/metadata.py b/lib/ansible/playbook/role/metadata.py index 28e1728ebda..1a9df75634d 100644 --- a/lib/ansible/playbook/role/metadata.py +++ b/lib/ansible/playbook/role/metadata.py @@ -80,7 +80,7 @@ class RoleMetadata(Base): role_def['name'] = def_parsed['name'] roles.append(role_def) except AnsibleError as exc: - raise AnsibleParserError(str(exc), obj=role_def) + raise AnsibleParserError(str(exc), obj=role_def, orig_exc=exc) current_role_path = None if self._owner: @@ -89,8 +89,8 @@ class RoleMetadata(Base): try: return load_list_of_roles(roles, play=self._owner._play, current_role_path=current_role_path, variable_manager=self._variable_manager, loader=self._loader) - except AssertionError: - raise AnsibleParserError("A malformed list of role dependencies was encountered.", obj=self._ds) + except AssertionError as e: + raise AnsibleParserError("A malformed list of role dependencies was encountered.", obj=self._ds, orig_exc=e) def _load_galaxy_info(self, attr, ds): ''' diff --git a/lib/ansible/playbook/task.py b/lib/ansible/playbook/task.py index a40cd79b287..a88a78a1b34 100644 --- a/lib/ansible/playbook/task.py +++ b/lib/ansible/playbook/task.py @@ -161,7 +161,7 @@ class Task(Base, Conditional, Taggable, Become): keep it short. ''' - assert isinstance(ds, dict) + assert isinstance(ds, dict), 'ds (%s) should be a dict but was a %s' % (ds, type(dict)) # the new, cleaned datastructure, which will have legacy # items reduced to a standard structure suitable for the @@ -177,7 +177,7 @@ class Task(Base, Conditional, Taggable, Become): try: (action, args, delegate_to) = args_parser.parse() except AnsibleParserError as e: - raise AnsibleParserError(to_native(e), obj=ds) + raise AnsibleParserError(to_native(e), obj=ds, orig_exc=e) # the command/shell/script modules used to support the `cmd` arg, # which corresponds to what we now call _raw_params, so move that diff --git a/lib/ansible/template/__init__.py b/lib/ansible/template/__init__.py index c9129a31d52..19741a997a7 100644 --- a/lib/ansible/template/__init__.py +++ b/lib/ansible/template/__init__.py @@ -385,7 +385,7 @@ class Templar: are being changed. ''' - assert isinstance(variables, dict) + assert isinstance(variables, dict), "the type of 'variables' should be a dict but was a %s" % (type(variables)) self._available_variables = variables self._cached_result = {} diff --git a/lib/ansible/vars/manager.py b/lib/ansible/vars/manager.py index 47d9fb27b78..6747dff3775 100644 --- a/lib/ansible/vars/manager.py +++ b/lib/ansible/vars/manager.py @@ -142,7 +142,7 @@ class VariableManager: @extra_vars.setter def extra_vars(self, value): ''' ensures a clean copy of the extra_vars are used to set the value ''' - assert isinstance(value, MutableMapping) + assert isinstance(value, MutableMapping), "the type of 'value' for extra_vars should be a MutableMapping, but is a %s" % type(value) self._extra_vars = value.copy() def set_inventory(self, inventory): @@ -156,7 +156,7 @@ class VariableManager: @options_vars.setter def options_vars(self, value): ''' ensures a clean copy of the options_vars are used to set the value ''' - assert isinstance(value, dict) + assert isinstance(value, dict), "the type of 'value' for options_vars should be a dict, but is a %s" % type(value) self._options_vars = value.copy() def _preprocess_vars(self, a): @@ -577,7 +577,7 @@ class VariableManager: Sets or updates the given facts for a host in the fact cache. ''' - assert isinstance(facts, dict) + assert isinstance(facts, dict), "the type of 'facts' to set for host_facts should be a dict but is a %s" % type(facts) if host.name not in self._fact_cache: self._fact_cache[host.name] = facts @@ -592,7 +592,7 @@ class VariableManager: Sets or updates the given facts for a host in the fact cache. ''' - assert isinstance(facts, dict) + assert isinstance(facts, dict), "the type of 'facts' to set for nonpersistent_facts should be a dict but is a %s" % type(facts) if host.name not in self._nonpersistent_fact_cache: self._nonpersistent_fact_cache[host.name] = facts