From 958ad7726a5c495ae3699b8065414c34f9b4235a Mon Sep 17 00:00:00 2001 From: Ryan Brown Date: Mon, 9 Oct 2017 11:27:50 -0400 Subject: [PATCH] Properly handle user selection of `None` as vars_files (#31313) * Properly handle user selection of `None` as vars_files In a playbook, if a user has a playbook like: ``` - hosts: localhost connection: local vars_files: tasks: - .... ``` Then `vars_files` will be none, and cause a `TypeError` in vars-manager when it tries to iterate over them. To avoid this, I changed the getter to either send back the vars files from the user, or an empty list when the user passed `None`. * Only replace None with an empty list, not all falsey values * Catch error when vars_files isn't iterable * Move whole `for` loop into try/except and catch TypeError * Line length --- lib/ansible/playbook/play.py | 2 + lib/ansible/vars/manager.py | 95 +++++++++++++++++++----------------- 2 files changed, 52 insertions(+), 45 deletions(-) diff --git a/lib/ansible/playbook/play.py b/lib/ansible/playbook/play.py index 2fe4a9020de..ccd3b323a1d 100644 --- a/lib/ansible/playbook/play.py +++ b/lib/ansible/playbook/play.py @@ -277,6 +277,8 @@ class Play(Base, Taggable, Become): return self.vars.copy() def get_vars_files(self): + if self.vars_files is None: + return [] return self.vars_files def get_handlers(self): diff --git a/lib/ansible/vars/manager.py b/lib/ansible/vars/manager.py index 0a50261a888..e8e5f2a0d9d 100644 --- a/lib/ansible/vars/manager.py +++ b/lib/ansible/vars/manager.py @@ -346,53 +346,58 @@ class VariableManager: if play: all_vars = combine_vars(all_vars, play.get_vars()) - for vars_file_item in play.get_vars_files(): - # create a set of temporary vars here, which incorporate the extra - # and magic vars so we can properly template the vars_files entries - temp_vars = combine_vars(all_vars, self._extra_vars) - temp_vars = combine_vars(temp_vars, magic_variables) - templar = Templar(loader=self._loader, variables=temp_vars) - - # we assume each item in the list is itself a list, as we - # support "conditional includes" for vars_files, which mimics - # the with_first_found mechanism. - vars_file_list = vars_file_item - if not isinstance(vars_file_list, list): - vars_file_list = [vars_file_list] - - # now we iterate through the (potential) files, and break out - # as soon as we read one from the list. If none are found, we - # raise an error, which is silently ignored at this point. - try: - for vars_file in vars_file_list: - vars_file = templar.template(vars_file) - try: - data = preprocess_vars(self._loader.load_from_file(vars_file, unsafe=True)) - if data is not None: - for item in data: - all_vars = combine_vars(all_vars, item) - break - except AnsibleFileNotFound: - # we continue on loader failures + vars_files = play.get_vars_files() + try: + for vars_file_item in vars_files: + # create a set of temporary vars here, which incorporate the extra + # and magic vars so we can properly template the vars_files entries + temp_vars = combine_vars(all_vars, self._extra_vars) + temp_vars = combine_vars(temp_vars, magic_variables) + templar = Templar(loader=self._loader, variables=temp_vars) + + # we assume each item in the list is itself a list, as we + # support "conditional includes" for vars_files, which mimics + # the with_first_found mechanism. + vars_file_list = vars_file_item + if not isinstance(vars_file_list, list): + vars_file_list = [vars_file_list] + + # now we iterate through the (potential) files, and break out + # as soon as we read one from the list. If none are found, we + # raise an error, which is silently ignored at this point. + try: + for vars_file in vars_file_list: + vars_file = templar.template(vars_file) + try: + data = preprocess_vars(self._loader.load_from_file(vars_file, unsafe=True)) + if data is not None: + for item in data: + all_vars = combine_vars(all_vars, item) + break + except AnsibleFileNotFound: + # we continue on loader failures + continue + except AnsibleParserError: + raise + else: + # if include_delegate_to is set to False, we ignore the missing + # vars file here because we're working on a delegated host + if include_delegate_to: + raise AnsibleFileNotFound("vars file %s was not found" % vars_file_item) + except (UndefinedError, AnsibleUndefinedVariable): + if host is not None and self._fact_cache.get(host.name, dict()).get('module_setup') and task is not None: + raise AnsibleUndefinedVariable("an undefined variable was found when attempting to template the vars_files item '%s'" + % vars_file_item, obj=vars_file_item) + else: + # we do not have a full context here, and the missing variable could be because of that + # so just show a warning and continue + display.vvv("skipping vars_file '%s' due to an undefined variable" % vars_file_item) continue - except AnsibleParserError: - raise - else: - # if include_delegate_to is set to False, we ignore the missing - # vars file here because we're working on a delegated host - if include_delegate_to: - raise AnsibleFileNotFound("vars file %s was not found" % vars_file_item) - except (UndefinedError, AnsibleUndefinedVariable): - if host is not None and self._fact_cache.get(host.name, dict()).get('module_setup') and task is not None: - raise AnsibleUndefinedVariable("an undefined variable was found when attempting to template the vars_files item '%s'" % vars_file_item, - obj=vars_file_item) - else: - # we do not have a full context here, and the missing variable could be because of that - # so just show a warning and continue - display.vvv("skipping vars_file '%s' due to an undefined variable" % vars_file_item) - continue - display.vvv("Read vars_file '%s'" % vars_file_item) + display.vvv("Read vars_file '%s'" % vars_file_item) + except TypeError: + raise AnsibleParserError("Error while reading vars files - please supply a list of file names. " + "Got '%s' of type %s" % (vars_files, type(vars_files))) # By default, we now merge in all vars from all roles in the play, # unless the user has disabled this via a config option