avoid roles exporting vars: (#69040)

- correct 'vars:' precedence to allow phasing out of include_params
 - actually merge vars and always include role_vars
 - avoided dupe deps from giving wrong vars
 - use 'first' instance of dep as others are from previous instances/invocations
   and can have diff values for vars
 - ensured deps only provide exportable vars themselves
 - added COMMENTS
 - added tests
 - apply export restrictions setting to defaults
 - use 'public' as cutoff

Co-authored-by: Sloane Hertel <19572925+s-hertel@users.noreply.github.com>

ci_complete

changelogs/fragments/role_vars_scope_fix.yml

@@ -0,0 +1,2 @@
+bugfixes:
+    - Fix bug in `vars` applied to roles, they were being incorrectly exported among others while only vars/main.yml was meant to be. Also adjusted the precedence to act the same as inline params.

lib/ansible/playbook/role/__init__.py

@@ -101,12 +101,14 @@ class Role(Base, Conditional, Taggable, CollectionSearch):
     delegate_to = FieldAttribute(isa='string')
     delegate_facts = FieldAttribute(isa='bool')
 
-    def __init__(self, play=None, from_files=None, from_include=False, validate=True):
+    def __init__(self, play=None, from_files=None, from_include=False, validate=True, public=True):
         self._role_name = None
         self._role_path = None
         self._role_collection = None
         self._role_params = dict()
         self._loader = None
+        self.public = public
+        self.static = True
 
         self._metadata = None
         self._play = play
@@ -139,8 +141,7 @@ class Role(Base, Conditional, Taggable, CollectionSearch):
         return self._role_name
 
     @staticmethod
-    def load(role_include, play, parent_role=None, from_files=None, from_include=False, validate=True):
+    def load(role_include, play, parent_role=None, from_files=None, from_include=False, validate=True, public=True):
-
         if from_files is None:
             from_files = {}
         try:
@@ -173,7 +174,7 @@ class Role(Base, Conditional, Taggable, CollectionSearch):
             #  for the in-flight in role cache as a sentinel that we're already trying to load
             #  that role?)
             # see https://github.com/ansible/ansible/issues/61527
-            r = Role(play=play, from_files=from_files, from_include=from_include, validate=validate)
+            r = Role(play=play, from_files=from_files, from_include=from_include, validate=validate, public=public)
             r._load_role_data(role_include, parent_role=parent_role)
 
             if role_include.get_name() not in play.ROLE_CACHE:
@@ -453,13 +454,14 @@ class Role(Base, Conditional, Taggable, CollectionSearch):
         default_vars = combine_vars(default_vars, self._default_vars)
         return default_vars
 
-    def get_inherited_vars(self, dep_chain=None):
+    def get_inherited_vars(self, dep_chain=None, only_exports=False):
         dep_chain = [] if dep_chain is None else dep_chain
 
         inherited_vars = dict()
 
         if dep_chain:
             for parent in dep_chain:
+                if not only_exports:
                     inherited_vars = combine_vars(inherited_vars, parent.vars)
                 inherited_vars = combine_vars(inherited_vars, parent._role_vars)
         return inherited_vars
@@ -474,19 +476,37 @@ class Role(Base, Conditional, Taggable, CollectionSearch):
         params = combine_vars(params, self._role_params)
         return params
 
-    def get_vars(self, dep_chain=None, include_params=True):
+    def get_vars(self, dep_chain=None, include_params=True, only_exports=False):
         dep_chain = [] if dep_chain is None else dep_chain
 
-        all_vars = self.get_inherited_vars(dep_chain)
+        all_vars = {}
 
-        for dep in self.get_all_dependencies():
+        # get role_vars: from parent objects
-            all_vars = combine_vars(all_vars, dep.get_vars(include_params=include_params))
+        # TODO: is this right precedence for inherited role_vars?
+        all_vars = self.get_inherited_vars(dep_chain, only_exports=only_exports)
 
-        all_vars = combine_vars(all_vars, self.vars)
+        # get exported variables from meta/dependencies
+        seen = set()
+        for dep in self.get_all_dependencies():
+            # Avoid reruning dupe deps since they can have vars from previous invocations and they accumulate in deps
+            # TODO: re-examine dep loading to see if we are somehow improperly adding the same dep too many times
+            if dep not in seen:
+                # only take 'exportable' vars from deps
+                all_vars = combine_vars(all_vars, dep.get_vars(include_params=False, only_exports=True))
+                seen.add(dep)
+
+        # role_vars come from vars/ in a role
         all_vars = combine_vars(all_vars, self._role_vars)
+
+        if not only_exports:
+            # include_params are 'inline variables' in role invocation. - {role: x, varname: value}
             if include_params:
+                # TODO: add deprecation notice
                 all_vars = combine_vars(all_vars, self.get_role_params(dep_chain=dep_chain))
 
+            # these come from vars: keyword in role invocation. - {role: x, vars: {varname: value}}
+            all_vars = combine_vars(all_vars, self.vars)
+
         return all_vars
 
     def get_direct_dependencies(self):

lib/ansible/playbook/role_include.py

@@ -89,7 +89,7 @@ class IncludeRole(TaskInclude):
 
         # build role
         actual_role = Role.load(ri, myplay, parent_role=self._parent_role, from_files=from_files,
-                                from_include=True, validate=self.rolespec_validate)
+                                from_include=True, validate=self.rolespec_validate, public=self.public)
         actual_role._metadata.allow_duplicates = self.allow_duplicates
 
         if self.statically_loaded or self.public:
@@ -129,6 +129,10 @@ class IncludeRole(TaskInclude):
 
         ir = IncludeRole(block, role, task_include=task_include).load_data(data, variable_manager=variable_manager, loader=loader)
 
+        # dyanmic role!
+        if ir.action in C._ACTION_INCLUDE_ROLE:
+            ir.static = False
+
         # Validate options
         my_arg_names = frozenset(ir.args.keys())
 
@@ -137,6 +141,7 @@ class IncludeRole(TaskInclude):
         if ir._role_name is None:
             raise AnsibleParserError("'name' is a required field for %s." % ir.action, obj=data)
 
+        # public is only valid argument for includes, imports are always 'public' (after they run)
         if 'public' in ir.args and ir.action not in C._ACTION_INCLUDE_ROLE:
             raise AnsibleParserError('Invalid options for %s: public' % ir.action, obj=data)
 
@@ -145,7 +150,7 @@ class IncludeRole(TaskInclude):
         if bad_opts:
             raise AnsibleParserError('Invalid options for %s: %s' % (ir.action, ','.join(list(bad_opts))), obj=data)
 
-        # build options for role includes
+        # build options for role include/import tasks
         for key in my_arg_names.intersection(IncludeRole.FROM_ARGS):
             from_key = key.removesuffix('_from')
             args_value = ir.args.get(key)
@@ -153,6 +158,7 @@ class IncludeRole(TaskInclude):
                 raise AnsibleParserError('Expected a string for %s but got %s instead' % (key, type(args_value)))
             ir._from_files[from_key] = basename(args_value)
 
+        # apply is only valid for includes, not imports as they inherit directly
         apply_attrs = ir.args.get('apply', {})
         if apply_attrs and ir.action not in C._ACTION_INCLUDE_ROLE:
             raise AnsibleParserError('Invalid options for %s: apply' % ir.action, obj=data)

lib/ansible/vars/manager.py

@@ -197,9 +197,12 @@ class VariableManager:
             basedirs = [self._loader.get_basedir()]
 
         if play:
+            if not C.DEFAULT_PRIVATE_ROLE_VARS:
                 # first we compile any vars specified in defaults/main.yml
                 # for all roles within the specified play
                 for role in play.get_roles():
+                    # role from roles or include_role+public or import_role and completed
+                    if not role.from_include or role.public or (role.static and role._completed.get(to_text(host), False)):
                         all_vars = _combine_and_track(all_vars, role.get_default_vars(), "role '%s' defaults" % role.name)
 
         if task:
@@ -215,6 +218,7 @@ class VariableManager:
             # if we have a task in this context, and that task has a role, make
             # sure it sees its defaults above any other roles, as we previously
             # (v1) made sure each task had a copy of its roles default vars
+            # TODO: investigate why we need play or include_role check?
             if task._role is not None and (play or task.action in C._ACTION_INCLUDE_ROLE):
                 all_vars = _combine_and_track(all_vars, task._role.get_default_vars(dep_chain=task.get_dep_chain()),
                                               "role '%s' defaults" % task._role.name)
@@ -383,19 +387,20 @@ class VariableManager:
                 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,
+            # By default, we now merge in all exported vars from all roles in the play,
             # unless the user has disabled this via a config option
             if not C.DEFAULT_PRIVATE_ROLE_VARS:
                 for role in play.get_roles():
-                    all_vars = _combine_and_track(all_vars, role.get_vars(include_params=False), "role '%s' vars" % role.name)
+                    if not role.from_include or role.public or (role.static and role._completed.get(to_text(host), False)):
+                        all_vars = _combine_and_track(all_vars, role.get_vars(include_params=False, only_exports=True), "role '%s' exported vars" % role.name)
 
         # next, we merge in the vars from the role, which will specifically
         # follow the role dependency chain, and then we merge in the tasks
         # vars (which will look at parent blocks/task includes)
         if task:
             if task._role:
-                all_vars = _combine_and_track(all_vars, task._role.get_vars(task.get_dep_chain(), include_params=False),
+                all_vars = _combine_and_track(all_vars, task._role.get_vars(task.get_dep_chain(), include_params=True, only_exports=False),
-                                              "role '%s' vars" % task._role.name)
+                                              "role '%s' all vars" % task._role.name)
             all_vars = _combine_and_track(all_vars, task.get_vars(), "task vars")
 
         # next, we merge in the vars cache (include vars) and nonpersistent
@@ -408,9 +413,6 @@ class VariableManager:
 
         # next, we merge in role params and task include params
         if task:
-            if task._role:
-                all_vars = _combine_and_track(all_vars, task._role.get_role_params(task.get_dep_chain()), "role '%s' params" % task._role.name)
-
             # special case for include tasks, where the include params
             # may be specified in the vars field for the task, which should
             # have higher precedence than the vars/np facts above

test/integration/targets/include_import/public_exposure/no_bleeding.yml

@@ -5,8 +5,8 @@
     - name: Static imports should expose vars at parse time, not at execution time
       assert:
         that:
-          - static_defaults_var == 'static_defaults'
+          - static_defaults_var is not defined
-          - static_vars_var == 'static_vars'
+          - static_vars_var is not defined
     - import_role:
         name: static
     - assert:

test/integration/targets/include_import/public_exposure/playbook.yml

@@ -10,12 +10,16 @@
     - name: Static imports should expose vars at parse time, not at execution time
       assert:
         that:
-          - static_defaults_var == 'static_defaults'
+          - static_defaults_var is not defined
-          - static_vars_var == 'static_vars'
+          - static_vars_var is not defined
+          - static_task_var is not defined
     - import_role:
         name: static
     - assert:
         that:
+          - static_vars_var is defined
+          - static_tasks_var is defined
+          - static_defaults_var is defined
           - static_tasks_var == 'static_tasks'
           - static_defaults_var == 'static_defaults'
           - static_vars_var == 'static_vars'

test/integration/targets/roles/roles/vars_scope/defaults/main.yml

@@ -0,0 +1,10 @@
+default_only: default
+role_vars_only: default
+play_and_role_vars: default
+play_and_roles_and_role_vars: default
+play_and_import_and_role_vars: default
+play_and_include_and_role_vars: default
+play_and_role_vars_and_role_vars: default
+roles_and_role_vars: default
+import_and_role_vars: default
+include_and_role_vars: default

test/integration/targets/roles/roles/vars_scope/tasks/check_vars.yml

@@ -0,0 +1 @@
+../../../tasks/check_vars.yml

test/integration/targets/roles/roles/vars_scope/tasks/main.yml

@@ -0,0 +1 @@
+- include_tasks: check_vars.yml

test/integration/targets/roles/roles/vars_scope/vars/main.yml

@@ -0,0 +1,9 @@
+role_vars_only: role_vars
+play_and_role_vars: role_vars
+play_and_roles_and_role_vars: role_vars
+play_and_import_and_role_vars: role_vars
+play_and_include_and_role_vars: role_vars
+play_and_role_vars_and_role_vars: role_vars
+roles_and_role_vars: role_vars
+import_and_role_vars: role_vars
+include_and_role_vars: role_vars

test/integration/targets/roles/runme.sh

@@ -26,3 +26,6 @@ else
   echo "Test failed, expected output from playbook failure is missing, output not shown)."
   exit 1
 fi
+
+# ensure vars scope is correct
+ansible-playbook vars_scope.yml -i ../../inventory "$@"

test/integration/targets/roles/tasks/check_vars.yml

@@ -0,0 +1,7 @@
+- debug: var={{item}}
+  loop: '{{possible_vars}}'
+
+- assert:
+    that:
+        - (item in vars and item in defined and vars[item] == defined[item]) or (item not in vars and item not in defined)
+  loop: '{{possible_vars}}'

test/integration/targets/roles/vars/play.yml

@@ -0,0 +1,26 @@
+play_only: play
+play_and_roles: play
+play_and_import: play
+play_and_include: play
+play_and_role_vars: play
+play_and_roles_and_role_vars: play
+play_and_import_and_role_vars: play
+play_and_include_and_role_vars: play
+possible_vars:
+    - default_only
+    - import_and_role_vars
+    - import_only
+    - include_and_role_vars
+    - include_only
+    - play_and_import
+    - play_and_import_and_role_vars
+    - play_and_include
+    - play_and_include_and_role_vars
+    - play_and_roles
+    - play_and_roles_and_role_vars
+    - play_and_role_vars
+    - play_and_role_vars_and_role_vars
+    - play_only
+    - roles_and_role_vars
+    - roles_only
+    - role_vars_only

test/integration/targets/roles/vars_scope.yml

@@ -0,0 +1,335 @@
+- name: play and roles
+  hosts: localhost
+  gather_facts: false
+  vars_files:
+    - vars/play.yml
+  roles:
+    - name: vars_scope
+      vars:
+        roles_only: roles
+        roles_and_role_vars: roles
+        play_and_roles: roles
+        play_and_roles_and_role_vars: roles
+        defined:
+            default_only: default
+            import_and_role_vars: role_vars
+            include_and_role_vars: role_vars
+            play_and_import: play
+            play_and_import_and_role_vars: role_vars
+            play_and_include: play
+            play_and_include_and_role_vars: role_vars
+            play_and_roles: roles
+            play_and_roles_and_role_vars: roles
+            play_and_role_vars: role_vars
+            play_and_role_vars_and_role_vars: role_vars
+            play_only: play
+            roles_and_role_vars: roles
+            roles_only: roles
+            role_vars_only: role_vars
+
+  pre_tasks:
+    - include_tasks: tasks/check_vars.yml
+      vars:
+        defined:
+            default_only: default
+            import_and_role_vars: role_vars
+            include_and_role_vars: role_vars
+            play_and_import: play
+            play_and_import_and_role_vars: role_vars
+            play_and_include: play
+            play_and_include_and_role_vars: role_vars
+            play_and_roles: play
+            play_and_roles_and_role_vars: role_vars
+            play_and_role_vars: role_vars
+            play_and_role_vars_and_role_vars: role_vars
+            play_only: play
+            roles_and_role_vars: role_vars
+            role_vars_only: role_vars
+  tasks:
+    - include_tasks: roles/vars_scope/tasks/check_vars.yml
+      vars:
+        defined:
+          default_only: default
+          import_and_role_vars: role_vars
+          include_and_role_vars: role_vars
+          play_and_import: play
+          play_and_import_and_role_vars: role_vars
+          play_and_include: play
+          play_and_include_and_role_vars: role_vars
+          play_and_roles: play
+          play_and_roles_and_role_vars: role_vars
+          play_and_role_vars: role_vars
+          play_and_role_vars_and_role_vars: role_vars
+          play_only: play
+          roles_and_role_vars: role_vars
+          role_vars_only: role_vars
+
+- name: play and import
+  hosts: localhost
+  gather_facts: false
+  vars_files:
+    - vars/play.yml
+  tasks:
+    - include_tasks: roles/vars_scope/tasks/check_vars.yml
+      vars:
+        defined:
+          play_and_import: play
+          play_and_import_and_role_vars: play
+          play_and_include: play
+          play_and_include_and_role_vars: play
+          play_and_roles: play
+          play_and_roles_and_role_vars: play
+          play_and_role_vars: play
+          play_only: play
+
+    - name: static import
+      import_role:
+        name: vars_scope
+      vars:
+        import_only: import
+        import_and_role_vars: import
+        play_and_import: import
+        play_and_import_and_role_vars: import
+        defined:
+          default_only: default
+          import_and_role_vars: import
+          import_only: import
+          include_and_role_vars: role_vars
+          play_and_import: import
+          play_and_import_and_role_vars: import
+          play_and_include: play
+          play_and_include_and_role_vars: role_vars
+          play_and_roles: play
+          play_and_roles_and_role_vars: role_vars
+          play_and_role_vars: role_vars
+          play_and_role_vars_and_role_vars: role_vars
+          play_only: play
+          roles_and_role_vars: role_vars
+          role_vars_only: role_vars
+
+    - include_tasks: roles/vars_scope/tasks/check_vars.yml
+      vars:
+        defined:
+          default_only: default
+          import_and_role_vars: role_vars
+          include_and_role_vars: role_vars
+          play_and_import: play
+          play_and_import_and_role_vars: role_vars
+          play_and_include: play
+          play_and_include_and_role_vars: role_vars
+          play_and_roles: play
+          play_and_roles_and_role_vars: role_vars
+          play_and_role_vars: role_vars
+          play_and_role_vars_and_role_vars: role_vars
+          play_only: play
+          roles_and_role_vars: role_vars
+          role_vars_only: role_vars
+
+- name: play and include
+  hosts: localhost
+  gather_facts: false
+  vars_files:
+    - vars/play.yml
+  tasks:
+    - include_tasks: roles/vars_scope/tasks/check_vars.yml
+      vars:
+        defined:
+          play_and_import: play
+          play_and_import_and_role_vars: play
+          play_and_include: play
+          play_and_include_and_role_vars: play
+          play_and_roles: play
+          play_and_roles_and_role_vars: play
+          play_and_role_vars: play
+          play_only: play
+
+    - name: dynamic include
+      include_role:
+        name: vars_scope
+      vars:
+        include_only: include
+        include_and_role_vars: include
+        play_and_include: include
+        play_and_include_and_role_vars: include
+        defined:
+          default_only: default
+          import_and_role_vars: role_vars
+          include_and_role_vars: include
+          include_only: include
+          play_and_import: play
+          play_and_import_and_role_vars: role_vars
+          play_and_include: include
+          play_and_include_and_role_vars: include
+          play_and_roles: play
+          play_and_roles_and_role_vars: role_vars
+          play_and_role_vars: role_vars
+          play_and_role_vars_and_role_vars: role_vars
+          play_only: play
+          roles_and_role_vars: role_vars
+          role_vars_only: role_vars
+
+    - include_tasks: roles/vars_scope/tasks/check_vars.yml
+      vars:
+        defined:
+          play_and_import: play
+          play_and_import_and_role_vars: play
+          play_and_include: play
+          play_and_include_and_role_vars: play
+          play_and_roles: play
+          play_and_roles_and_role_vars: play
+          play_and_role_vars: play
+          play_only: play
+
+- name: play and roles and import and include
+  hosts: localhost
+  gather_facts: false
+  vars:
+  vars_files:
+    - vars/play.yml
+  roles:
+    - name: vars_scope
+      vars:
+        roles_only: roles
+        roles_and_role_vars: roles
+        play_and_roles: roles
+        play_and_roles_and_role_vars: roles
+        defined:
+          default_only: default
+          import_and_role_vars: role_vars
+          include_and_role_vars: role_vars
+          play_and_import: play
+          play_and_import_and_role_vars: role_vars
+          play_and_include: play
+          play_and_include_and_role_vars: role_vars
+          play_and_roles: roles
+          play_and_roles_and_role_vars: roles
+          play_and_role_vars: role_vars
+          play_and_role_vars_and_role_vars: role_vars
+          play_only: play
+          roles_and_role_vars: roles
+          roles_only: roles
+          role_vars_only: role_vars
+
+  pre_tasks:
+    - include_tasks: roles/vars_scope/tasks/check_vars.yml
+      vars:
+        defined:
+            default_only: default
+            import_and_role_vars: role_vars
+            include_and_role_vars: role_vars
+            play_and_import: play
+            play_and_import_and_role_vars: role_vars
+            play_and_include: play
+            play_and_include_and_role_vars: role_vars
+            play_and_roles: play
+            play_and_roles_and_role_vars: role_vars
+            play_and_role_vars: role_vars
+            play_and_role_vars_and_role_vars: role_vars
+            play_only: play
+            roles_and_role_vars: role_vars
+            role_vars_only: role_vars
+
+  tasks:
+    - include_tasks: roles/vars_scope/tasks/check_vars.yml
+      vars:
+        defined:
+            default_only: default
+            import_and_role_vars: role_vars
+            include_and_role_vars: role_vars
+            play_and_import: play
+            play_and_import_and_role_vars: role_vars
+            play_and_include: play
+            play_and_include_and_role_vars: role_vars
+            play_and_roles: play
+            play_and_roles_and_role_vars: role_vars
+            play_and_role_vars: role_vars
+            play_and_role_vars_and_role_vars: role_vars
+            play_only: play
+            roles_and_role_vars: role_vars
+            role_vars_only: role_vars
+
+    - name: static import
+      import_role:
+        name: vars_scope
+      vars:
+        import_only: import
+        import_and_role_vars: import
+        play_and_import: import
+        play_and_import_and_role_vars: import
+        defined:
+            default_only: default
+            import_and_role_vars: import
+            import_only: import
+            include_and_role_vars: role_vars
+            play_and_import: import
+            play_and_import_and_role_vars: import
+            play_and_include: play
+            play_and_include_and_role_vars: role_vars
+            play_and_roles: play
+            play_and_roles_and_role_vars: role_vars
+            play_and_role_vars: role_vars
+            play_and_role_vars_and_role_vars: role_vars
+            play_only: play
+            roles_and_role_vars: role_vars
+            role_vars_only: role_vars
+
+    - include_tasks: roles/vars_scope/tasks/check_vars.yml
+      vars:
+        defined:
+            default_only: default
+            import_and_role_vars: role_vars
+            include_and_role_vars: role_vars
+            play_and_import: play
+            play_and_import_and_role_vars: role_vars
+            play_and_include: play
+            play_and_include_and_role_vars: role_vars
+            play_and_roles: play
+            play_and_roles_and_role_vars: role_vars
+            play_and_role_vars: role_vars
+            play_and_role_vars_and_role_vars: role_vars
+            play_only: play
+            roles_and_role_vars: role_vars
+            role_vars_only: role_vars
+
+    - name: dynamic include
+      include_role:
+        name: vars_scope
+      vars:
+        include_only: include
+        include_and_role_vars: include
+        play_and_include: include
+        play_and_include_and_role_vars: include
+        defined:
+            default_only: default
+            import_and_role_vars: role_vars
+            include_and_role_vars: include
+            include_only: include
+            play_and_import: play
+            play_and_import_and_role_vars: role_vars
+            play_and_include: include
+            play_and_include_and_role_vars: include
+            play_and_roles: play
+            play_and_roles_and_role_vars: role_vars
+            play_and_role_vars: role_vars
+            play_and_role_vars_and_role_vars: role_vars
+            play_only: play
+            roles_and_role_vars: role_vars
+            role_vars_only: role_vars
+
+    - include_tasks: roles/vars_scope/tasks/check_vars.yml
+      vars:
+        defined:
+            default_only: default
+            import_and_role_vars: role_vars
+            include_and_role_vars: role_vars
+            play_and_import: play
+            play_and_import_and_role_vars: role_vars
+            play_and_include: play
+            play_and_include_and_role_vars: role_vars
+            play_and_roles: play
+            play_and_roles_and_role_vars: role_vars
+            play_and_role_vars: role_vars
+            play_and_role_vars_and_role_vars: role_vars
+            play_only: play
+            roles_and_role_vars: role_vars
+            role_vars_only: role_vars

test/integration/targets/roles_arg_spec/test.yml

@@ -48,6 +48,7 @@
           name: a
         vars:
           a_int: "{{ INT_VALUE }}"
+          a_str: "import_role"
 
       - name: "Call role entry point that is defined, but has no spec data"
         import_role: