Role fixes (#82339)

* Various fixes to roles

  - static property is now properly set
  - role_names and other magic vars now have full list
  - role public/private var loading is now done when adding to play.roles instead of on each var query
  - added tests

Co-authored-by: Felix Fontein <felix@fontein.de>

changelogs/fragments/role_fixes.yml

@@ -0,0 +1,3 @@
+bugfixes:
+  - roles, the ``static`` property is now correctly set, this will fix issues with ``public`` and ``DEFAULT_PRIVATE_ROLE_VARS`` controls on exporting vars.
+  - roles, code cleanup and performance optimization of dependencies, now cached,  and ``public`` setting is now determined once, at role instantiation.

lib/ansible/config/base.yml

@@ -944,10 +944,12 @@ DEFAULT_PRIVATE_ROLE_VARS:
   name: Private role variables
   default: False
   description:
-    - Makes role variables inaccessible from other roles.
-    - This was introduced as a way to reset role variables to default values if
-      a role is used more than once in a playbook.
-    - Starting in version '2.17' M(ansible.builtin.include_roles) and M(ansible.builtin.import_roles) can override this via the C(public) parameter.
+    - By default, imported roles publish their variables to the play and other roles, this setting can avoid that.
+    - This was introduced as a way to reset role variables to default values if a role is used more than once
+      in a playbook.
+    - Starting in version '2.17' M(ansible.builtin.include_roles) and M(ansible.builtin.import_roles) can
+      indivudually override this via the C(public) parameter.
+    - Included roles only make their variables public at execution, unlike imported roles which happen at playbook compile time.
   env: [{name: ANSIBLE_PRIVATE_ROLE_VARS}]
   ini:
   - {key: private_role_vars, section: defaults}

lib/ansible/playbook/role/__init__.py

@@ -98,19 +98,30 @@ def hash_params(params):
 
 class Role(Base, Conditional, Taggable, CollectionSearch, Delegatable):
 
-    def __init__(self, play=None, from_files=None, from_include=False, validate=True, public=True):
+    def __init__(self, play=None, from_files=None, from_include=False, validate=True, public=None, static=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.static = static
+
+        # includes (static=false) default to private, while imports (static=true) default to public
+        # but both can be overriden by global config if set
+        if public is None:
+            global_private, origin = C.config.get_config_value_and_origin('DEFAULT_PRIVATE_ROLE_VARS')
+            if origin == 'default':
+                self.public = static
+            else:
+                self.public = not global_private
+        else:
+            self.public = public
 
         self._metadata = RoleMetadata()
         self._play = play
         self._parents = []
         self._dependencies = []
+        self._all_dependencies = None
         self._task_blocks = []
         self._handler_blocks = []
         self._compiled_handler_blocks = None
@@ -167,7 +178,7 @@ class Role(Base, Conditional, Taggable, CollectionSearch, Delegatable):
         return self._get_hash_dict() == other._get_hash_dict()
 
     @staticmethod
-    def load(role_include, play, parent_role=None, from_files=None, from_include=False, validate=True, public=True):
+    def load(role_include, play, parent_role=None, from_files=None, from_include=False, validate=True, public=None, static=True):
         if from_files is None:
             from_files = {}
         try:
@@ -175,7 +186,7 @@ class Role(Base, Conditional, Taggable, CollectionSearch, Delegatable):
             #  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, public=public)
+            r = Role(play=play, from_files=from_files, from_include=from_include, validate=validate, public=public, static=static)
             r._load_role_data(role_include, parent_role=parent_role)
 
             role_path = r.get_role_path()
@@ -426,7 +437,7 @@ class Role(Base, Conditional, Taggable, CollectionSearch, Delegatable):
 
         deps = []
         for role_include in self._metadata.dependencies:
-            r = Role.load(role_include, play=self._play, parent_role=self)
+            r = Role.load(role_include, play=self._play, parent_role=self, static=self.static)
             deps.append(r)
 
         return deps
@@ -526,15 +537,15 @@ class Role(Base, Conditional, Taggable, CollectionSearch, Delegatable):
         Returns a list of all deps, built recursively from all child dependencies,
         in the proper order in which they should be executed or evaluated.
         '''
+        if self._all_dependencies is None:
 
-        child_deps = []
-
-        for dep in self.get_direct_dependencies():
-            for child_dep in dep.get_all_dependencies():
-                child_deps.append(child_dep)
-            child_deps.append(dep)
+            self._all_dependencies = []
+            for dep in self.get_direct_dependencies():
+                for child_dep in dep.get_all_dependencies():
+                    self._all_dependencies.append(child_dep)
+                self._all_dependencies.append(dep)
 
-        return child_deps
+        return self._all_dependencies
 
     def get_task_blocks(self):
         return self._task_blocks[:]

lib/ansible/playbook/role_include.py

@@ -86,11 +86,11 @@ 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, public=self.public)
+                                from_include=True, validate=self.rolespec_validate, public=self.public, static=self.statically_loaded)
         actual_role._metadata.allow_duplicates = self.allow_duplicates
 
-        if self.statically_loaded or self.public:
-            myplay.roles.append(actual_role)
+        # add role to play
+        myplay.roles.append(actual_role)
 
         # save this for later use
         self._role_path = actual_role._role_path
@@ -122,10 +122,6 @@ 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())
 

lib/ansible/vars/manager.py

@@ -197,15 +197,11 @@ class VariableManager:
             basedirs = [self._loader.get_basedir()]
 
         if play:
-            for role in play.get_roles():
-                # role is public and
-                #    either static or dynamic and completed
-                # role is not set
-                #    use config option as default
-                role_is_static_or_completed = role.static or role._completed.get(host.name, False)
-                if role.public and role_is_static_or_completed or \
-                   role.public is None and not C.DEFAULT_PRIVATE_ROLE_VARS and role_is_static_or_completed:
+            # get role defaults (lowest precedence)
+            for role in play.roles:
+                if role.public:
                     all_vars = _combine_and_track(all_vars, role.get_default_vars(), "role '%s' defaults" % role.name)
+
         if task:
             # set basedirs
             if C.PLAYBOOK_VARS_ROOT == 'all':  # should be default
@@ -221,8 +217,7 @@ class VariableManager:
             # (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)
+                all_vars = _combine_and_track(all_vars, task._role.get_default_vars(dep_chain=task.get_dep_chain()), "role '%s' defaults" % task._role.name)
 
         if host:
             # THE 'all' group and the rest of groups for a host, used below
@@ -388,17 +383,9 @@ 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)))
 
-            # We now merge in all exported vars from all roles in the play,
-            # unless the user has disabled this
-            # role is public and
-            #    either static or dynamic and completed
-            # role is not set
-            #    use config option as default
-            for role in play.get_roles():
-                role_is_static_or_completed = role.static or role._completed.get(host.name, False)
-                if role.public and role_is_static_or_completed or \
-                   role.public is None and not C.DEFAULT_PRIVATE_ROLE_VARS and role_is_static_or_completed:
-
+            # We now merge in all exported vars from all roles in the play (very high precedence)
+            for role in play.roles:
+                if role.public:
                     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
@@ -467,9 +454,8 @@ class VariableManager:
         variables['ansible_config_file'] = C.CONFIG_FILE
 
         if play:
-            # This is a list of all role names of all dependencies for all roles for this play
+            # using role_cache as play.roles only has 'public' roles for vars exporting
             dependency_role_names = list({d.get_name() for r in play.roles for d in r.get_all_dependencies()})
-            # This is a list of all role names of all roles for this play
             play_role_names = [r.get_name() for r in play.roles]
 
             # ansible_role_names includes all role names, dependent or directly referenced by the play
@@ -481,7 +467,7 @@ class VariableManager:
             # dependencies that are also explicitly named as roles are included in this list
             variables['ansible_dependent_role_names'] = dependency_role_names
 
-            # DEPRECATED: role_names should be deprecated in favor of ansible_role_names or ansible_play_role_names
+            # TODO: data tagging!!! DEPRECATED: role_names should be deprecated in favor of ansible_ prefixed ones
             variables['role_names'] = variables['ansible_play_role_names']
 
             variables['ansible_play_name'] = play.get_name()

test/integration/targets/roles/privacy.yml

@@ -0,0 +1,82 @@
+# use this to debug issues
+#- debug: msg={{ is_private ~ ', ' ~ is_default ~ ', ' ~ privacy|default('nope')}}
+
+- hosts: localhost
+  name: test global privacy setting
+  gather_facts: false
+  roles:
+    - a
+  pre_tasks:
+
+    - name: 'test roles: privacy'
+      assert:
+        that:
+          - is_private and privacy is undefined or not is_private and privacy is defined
+          - not is_default or is_default and privacy is defined
+
+- hosts: localhost
+  name: test import_role privacy
+  gather_facts: false
+  tasks:
+    - import_role: name=a
+
+    - name: role is private, var should be undefined
+      assert:
+        that:
+          - is_private and privacy is undefined or not is_private and privacy is defined
+          - not is_default or is_default and privacy is defined
+
+- hosts: localhost
+  name: test public no always overrides global on import_role
+  gather_facts: false
+  tasks:
+    - import_role: name=a public=no
+
+    - name: role is private, var should be undefined
+      assert:
+        that:
+          - privacy is undefined
+
+- hosts: localhost
+  name: test public yes always overrides global on import_role
+  gather_facts: false
+  tasks:
+    - import_role: name=a public=yes
+
+    - name: role is private, var should be undefined
+      assert:
+        that:
+          - privacy is defined
+
+- hosts: localhost
+  name: test global privacy setting on includes
+  gather_facts: false
+  tasks:
+    - include_role: name=a
+
+    - name: test include_role privacy
+      assert:
+        that:
+          - not is_default and (is_private and privacy is undefined or not is_private and privacy is defined) or is_default and privacy is undefined
+
+- hosts: localhost
+  name: test public yes always overrides global privacy setting on includes
+  gather_facts: false
+  tasks:
+    - include_role: name=a public=yes
+
+    - name: test include_role privacy
+      assert:
+        that:
+          - privacy is defined
+
+- hosts: localhost
+  name: test public no always overrides global privacy setting on includes
+  gather_facts: false
+  tasks:
+    - include_role: name=a public=no
+
+    - name: test include_role privacy
+      assert:
+        that:
+          - privacy is undefined

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

@@ -0,0 +1 @@
+privacy: in role a

test/integration/targets/roles/runme.sh

@@ -42,3 +42,8 @@ ansible-playbook vars_scope.yml -i ../../inventory "$@"
 
 # ensure import_role called from include_role has the include_role in the dep chain
 ansible-playbook role_dep_chain.yml -i ../../inventory "$@"
+
+# global role privacy setting test, set to private, set to not private, default
+ANSIBLE_PRIVATE_ROLE_VARS=1 ansible-playbook privacy.yml -e @vars/privacy_vars.yml "$@"
+ANSIBLE_PRIVATE_ROLE_VARS=0 ansible-playbook privacy.yml -e @vars/privacy_vars.yml "$@"
+ansible-playbook privacy.yml -e @vars/privacy_vars.yml "$@"

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

@@ -0,0 +1,2 @@
+is_private: "{{lookup('config', 'DEFAULT_PRIVATE_ROLE_VARS')}}"
+is_default: "{{lookup('env', 'ANSIBLE_PRIVATE_ROLE_VARS') == ''}}"