Correct variable blending from vars_files with hostvars in them

Fixes #8638

lib/ansible/playbook/__init__.py

@@ -396,6 +396,7 @@ class PlayBook(object):
             remote_port=task.play.remote_port,
             module_vars=task.module_vars,
             default_vars=task.default_vars,
+            extra_vars=self.extra_vars,
             private_key_file=self.private_key_file,
             setup_cache=self.SETUP_CACHE,
             vars_cache=self.VARS_CACHE,

lib/ansible/playbook/play.py

@@ -89,15 +89,15 @@ class Play(object):
         self.vars_files = ds.get('vars_files', [])
         if not isinstance(self.vars_files, list):
             raise errors.AnsibleError('vars_files must be a list')
-        self._update_vars_files_for_host(None)
+        processed_vars_files = self._update_vars_files_for_host(None)
 
         # now we load the roles into the datastructure
         self.included_roles = []
         ds = self._load_roles(self.roles, ds)
 
-        # and finally re-process the vars files as they may have
-        # been updated by the included roles
-        self.vars_files = ds.get('vars_files', [])
+        # and finally re-process the vars files as they may have been updated
+        # by the included roles, but exclude any which have been processed
+        self.vars_files = utils.list_difference(ds.get('vars_files', []), processed_vars_files)
         if not isinstance(self.vars_files, list):
             raise errors.AnsibleError('vars_files must be a list')
 
@@ -765,27 +765,37 @@ class Play(object):
 
             """ Render the raw filename into 3 forms """
 
+            # filename2 is the templated version of the filename, which will
+            # be fully rendered if any variables contained within it are 
+            # non-inventory related
             filename2 = template(self.basedir, filename, self.vars)
+
+            # filename3 is the same as filename2, but when the host object is
+            # available, inventory variables will be expanded as well since the
+            # name is templated with the injected variables
             filename3 = filename2
             if host is not None:
                 filename3 = template(self.basedir, filename2, inject)
+
+            # filename4 is the dwim'd path, but may also be mixed-scope, so we use
+            # both play scoped vars and host scoped vars to template the filepath
             if self._has_vars_in(filename3) and host is not None:
-                # allow play scoped vars and host scoped vars to template the filepath
                 inject.update(self.vars)
                 filename4 = template(self.basedir, filename3, inject)
                 filename4 = utils.path_dwim(self.basedir, filename4)
             else:
                 filename4 = utils.path_dwim(self.basedir, filename3)
+
             return filename2, filename3, filename4
 
 
-        def update_vars_cache(host, inject, data, filename):
+        def update_vars_cache(host, data, target_filename=None):
 
             """ update a host's varscache with new var data """
 
-            data = utils.combine_vars(inject, data)
             self.playbook.VARS_CACHE[host] = utils.combine_vars(self.playbook.VARS_CACHE.get(host, {}), data)
-            self.playbook.callbacks.on_import_for_host(host, filename4)
+            if target_filename:
+                self.playbook.callbacks.on_import_for_host(host, target_filename)
 
         def process_files(filename, filename2, filename3, filename4, host=None):
 
@@ -796,21 +806,19 @@ class Play(object):
                 if type(data) != dict:
                     raise errors.AnsibleError("%s must be stored as a dictionary/hash" % filename4)
                 if host is not None:
-                    if self._has_vars_in(filename2) and not self._has_vars_in(filename3):
-                        # running a host specific pass and has host specific variables
-                        # load into setup cache
-                        update_vars_cache(host, inject, data, filename4)
-                    elif self._has_vars_in(filename3) and not self._has_vars_in(filename4):
-                        # handle mixed scope variables in filepath
-                        update_vars_cache(host, inject, data, filename4)
-
-                elif not self._has_vars_in(filename4):
-                    # found a non-host specific variable, load into vars and NOT
-                    # the setup cache
-                    if host is not None:
-                        self.vars.update(data)
-                    else:
-                        self.vars = utils.combine_vars(self.vars, data)
+                    target_filename = None
+                    if self._has_vars_in(filename2):
+                        if not self._has_vars_in(filename3):
+                            target_filename = filename3
+                        else:
+                            target_filename = filename4
+                    update_vars_cache(host, data, target_filename=target_filename)
+                else:
+                    self.vars = utils.combine_vars(self.vars, data)
+                # we did process this file
+                return True
+            # we did not process this file
+            return False
 
         # Enforce that vars_files is always a list
         if type(self.vars_files) != list:
@@ -825,6 +833,7 @@ class Play(object):
         else:
             inject = None
 
+        processed = []
         for filename in self.vars_files:
             if type(filename) == list:
                 # loop over all filenames, loading the first one, and failing if none found
@@ -835,7 +844,8 @@ class Play(object):
                     sequence.append(filename4)
                     if os.path.exists(filename4):
                         found = True
-                        process_files(filename, filename2, filename3, filename4, host=host)
+                        if process_files(filename, filename2, filename3, filename4, host=host):
+                            processed.append(filename)
                     elif host is not None:
                         self.playbook.callbacks.on_not_import_for_host(host, filename4)
                     if found:
@@ -844,14 +854,12 @@ class Play(object):
                     raise errors.AnsibleError(
                         "%s: FATAL, no files matched for vars_files import sequence: %s" % (host, sequence)
                     )
-
             else:
                 # just one filename supplied, load it!
                 filename2, filename3, filename4 = generate_filenames(host, inject, filename)
                 if self._has_vars_in(filename4):
                     continue
-                process_files(filename, filename2, filename3, filename4, host=host)
+                if process_files(filename, filename2, filename3, filename4, host=host):
+                    processed.append(filename)
 
-        # finally, update the VARS_CACHE for the host, if it is set
-        if host is not None:
-            self.playbook.VARS_CACHE.setdefault(host, {}).update(self.playbook.extra_vars)
+        return processed

lib/ansible/runner/__init__.py

@@ -130,6 +130,7 @@ class Runner(object):
         sudo_user=C.DEFAULT_SUDO_USER,      # ex: 'root'
         module_vars=None,                   # a playbooks internals thing
         default_vars=None,                  # ditto
+        extra_vars=None,                    # extra vars specified with he playbook(s)
         is_playbook=False,                  # running from playbook or not?
         inventory=None,                     # reference to Inventory object
         subset=None,                        # subset pattern
@@ -170,6 +171,8 @@ class Runner(object):
 
         self.module_vars      = utils.default(module_vars, lambda: {})
         self.default_vars     = utils.default(default_vars, lambda: {})
+        self.extra_vars       = utils.default(extra_vars, lambda: {})
+
         self.always_run       = None
         self.connector        = connection.Connector(self)
         self.conditional      = conditional
@@ -605,10 +608,13 @@ class Runner(object):
         module_vars = template.template(self.basedir, self.module_vars, module_vars_inject)
 
         inject = {}
+
         inject = utils.combine_vars(inject, self.default_vars)
         inject = utils.combine_vars(inject, host_variables)
+        inject = utils.combine_vars(inject, self.setup_cache.get(host, {}))
         inject = utils.combine_vars(inject, module_vars)
-        inject = utils.combine_vars(inject, combined_cache.get(host, {}))
+        inject = utils.combine_vars(inject, self.vars_cache.get(host, {}))
+        inject = utils.combine_vars(inject, self.extra_vars)
         inject.setdefault('ansible_ssh_user', self.remote_user)
         inject['hostvars']    = hostvars
         inject['group_names'] = host_variables.get('group_names', [])

lib/ansible/utils/__init__.py

@@ -1163,6 +1163,16 @@ def list_intersection(a, b):
             result.append(x)
     return result
 
+def list_difference(a, b):
+    result = []
+    for x in a:
+        if x not in b and x not in result:
+            result.append(x)
+    for x in b:
+        if x not in a and x not in result:
+            result.append(x)
+    return result
+
 def safe_eval(expr, locals={}, include_exceptions=False):
     '''
     This is intended for allowing things like:

test/integration/roles/test_var_precedence/tasks/main.yml

@@ -3,3 +3,4 @@
       - 'extra_var == "extra_var"'
       - 'vars_var == "vars_var"'
       - 'vars_files_var == "vars_files_var"'
+      - 'vars_files_var_role == "vars_files_var_role3"'

test/integration/roles/test_var_precedence_role1/tasks/main.yml

@@ -2,6 +2,7 @@
 - debug: var=param_var
 - debug: var=vars_var
 - debug: var=vars_files_var
+- debug: var=vars_files_var_role
 - debug: var=defaults_file_var_role1
 - assert:
     that:
@@ -9,4 +10,5 @@
       - 'param_var == "param_var_role1"'
       - 'vars_var == "vars_var"'
       - 'vars_files_var == "vars_files_var"'
+      - 'vars_files_var_role == "vars_files_var_role3"'
       - 'defaults_file_var_role1 == "defaults_file_var_role1"'

test/integration/roles/test_var_precedence_role1/vars/main.yml

 ---
 # should override the global vars_files_var since it's local to the role
-vars_files_var: "vars_files_var_role1"
+# but will be set to the value in the last role included which defines it
+vars_files_var_role: "vars_files_var_role1"

test/integration/roles/test_var_precedence_role2/tasks/main.yml

@@ -2,6 +2,7 @@
 - debug: var=param_var
 - debug: var=vars_var
 - debug: var=vars_files_var
+- debug: var=vars_files_var_role
 - debug: var=defaults_file_var_role1
 - assert:
     that:
@@ -9,4 +10,5 @@
       - 'param_var == "param_var_role2"'
       - 'vars_var == "vars_var"'
       - 'vars_files_var == "vars_files_var"'
+      - 'vars_files_var_role == "vars_files_var_role3"'
       - 'defaults_file_var_role2 == "overridden by role vars"'

test/integration/roles/test_var_precedence_role2/vars/main.yml

 ---
 # should override the global vars_files_var since it's local to the role
-vars_files_var: "vars_files_var_role1"
+vars_files_var_role: "vars_files_var_role2"
 # should override the value in defaults/main.yml for role 2
 defaults_file_var_role2: "overridden by role vars"

test/integration/roles/test_var_precedence_role3/tasks/main.yml

@@ -2,6 +2,7 @@
 - debug: var=param_var
 - debug: var=vars_var
 - debug: var=vars_files_var
+- debug: var=vars_files_var_role
 - debug: var=defaults_file_var_role1
 - assert:
     that:
@@ -9,4 +10,5 @@
       - 'param_var == "param_var_role3"'
       - 'vars_var == "vars_var"'
       - 'vars_files_var == "vars_files_var"'
+      - 'vars_files_var_role == "vars_files_var_role3"'
       - 'defaults_file_var_role3 == "overridden from inventory"'

test/integration/roles/test_var_precedence_role3/vars/main.yml

 ---
 # should override the global vars_files_var since it's local to the role
-vars_files_var: "vars_files_var_role1"
+vars_files_var_role: "vars_files_var_role3"

test/integration/test_var_precedence.yml

@@ -12,8 +12,10 @@
   - debug: var=extra_var
   - debug: var=vars_var
   - debug: var=vars_files_var
+  - debug: var=vars_files_var_role
   - assert:
       that:
         - 'extra_var == "extra_var"'
         - 'vars_var == "vars_var"'
         - 'vars_files_var == "vars_files_var"'
+        - 'vars_files_var_role == "vars_files_var_role3"'

test/integration/vars/test_var_precedence.yml

@@ -2,3 +2,4 @@
 extra_var: "BAD!"
 role_var: "BAD!"
 vars_files_var: "vars_files_var"
+vars_files_var_role: "should be overridden by roles"

test/units/TestPlayVarsFiles.py

@@ -266,37 +266,6 @@ class TestMe(unittest.TestCase):
         assert 'foo' in play.playbook.VARS_CACHE['localhost'], "vars_file vars were not loaded into vars_cache"
         assert play.playbook.VARS_CACHE['localhost']['foo'] == 'bar', "foo does not equal bar"
 
-    def test_vars_files_for_host_with_extra_vars(self):
-
-        # host != None
-        # vars in filename2
-        # no vars in filename3
-
-        # make a vars file
-        fd, temp_path = mkstemp()
-        f = open(temp_path, "wb")
-        f.write("foo: bar\n")
-        f.close()
-
-        # build play attributes
-        playbook = FakePlayBook()
-        ds = { "hosts": "localhost",
-               "vars_files": ["{{ temp_path }}"]}
-        basedir = "."
-        playbook.VARS_CACHE['localhost']['temp_path'] = temp_path
-        playbook.extra_vars = {"foo": "extra"}
-
-        # create play and do first run        
-        play = Play(playbook, ds, basedir)
-
-        # the second run is started by calling update_vars_files        
-        play.update_vars_files(['localhost'])
-        os.remove(temp_path)
-
-        assert 'foo' in play.vars, "extra vars were not set in play.vars"
-        assert 'foo' in play.playbook.VARS_CACHE['localhost'], "vars_file vars were not loaded into vars_cache"
-        assert play.playbook.VARS_CACHE['localhost']['foo'] == 'extra', "extra vars did not overwrite vars_files vars"
-
 
     ########################################
     # COMPLEX FILENAME TEMPLATING TESTS