diff --git a/changelogs/fragments/role_fixes.yml b/changelogs/fragments/role_fixes.yml new file mode 100644 index 00000000000000..ef68d0f2762e19 --- /dev/null +++ b/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. diff --git a/lib/ansible/config/base.yml b/lib/ansible/config/base.yml index 3b795553eb9fff..69a0d679e235a3 100644 --- a/lib/ansible/config/base.yml +++ b/lib/ansible/config/base.yml @@ -934,9 +934,9 @@ 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. + - 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. + - 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} diff --git a/lib/ansible/playbook/role/__init__.py b/lib/ansible/playbook/role/__init__.py index 31e63097295ec9..34d8ba99175d25 100644 --- a/lib/ansible/playbook/role/__init__.py +++ b/lib/ansible/playbook/role/__init__.py @@ -100,19 +100,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 @@ -169,7 +180,7 @@ def __eq__(self, other): 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: @@ -177,7 +188,7 @@ def load(role_include, play, parent_role=None, from_files=None, from_include=Fal # 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() @@ -428,7 +439,7 @@ def _load_dependencies(self): 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 @@ -528,15 +539,15 @@ def get_all_dependencies(self): 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[:] diff --git a/lib/ansible/playbook/role_include.py b/lib/ansible/playbook/role_include.py index 6061e745d8fcf3..cdf86c0fc6c7e9 100644 --- a/lib/ansible/playbook/role_include.py +++ b/lib/ansible/playbook/role_include.py @@ -88,11 +88,11 @@ def get_block_list(self, play=None, variable_manager=None, loader=None): # 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 @@ -124,10 +124,6 @@ def load(data, block=None, role=None, task_include=None, variable_manager=None, 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()) diff --git a/lib/ansible/vars/manager.py b/lib/ansible/vars/manager.py index 48b34d9abfd697..d210eacd8e231c 100644 --- a/lib/ansible/vars/manager.py +++ b/lib/ansible/vars/manager.py @@ -199,14 +199,10 @@ def _combine_and_track(data, new_data, source): basedirs = [self._loader.get_basedir()] if play: - if not C.DEFAULT_PRIVATE_ROLE_VARS: - 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 - if role.static or role.public and role._completed.get(host.name, False): - all_vars = _combine_and_track(all_vars, role.get_default_vars(), "role '%s' defaults" % role.name) + # 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 @@ -222,8 +218,7 @@ def _combine_and_track(data, new_data, source): # (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 @@ -389,16 +384,10 @@ def plugins_by_groups(): 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 - if not C.DEFAULT_PRIVATE_ROLE_VARS: - for role in play.get_roles(): - if role.static or role.public and role._completed.get(host.name, False): - all_vars = _combine_and_track(all_vars, role.get_vars(include_params=False, only_exports=True), "role '%s' exported vars" % role.name) + # 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 # follow the role dependency chain, and then we merge in the tasks @@ -466,9 +455,8 @@ def _get_magic_variables(self, play, host, task, include_hostvars, _hosts=None, 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 @@ -480,7 +468,7 @@ def _get_magic_variables(self, play, host, task, include_hostvars, _hosts=None, # 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() diff --git a/test/integration/targets/roles/privacy.yml b/test/integration/targets/roles/privacy.yml new file mode 100644 index 00000000000000..2f671c07078d24 --- /dev/null +++ b/test/integration/targets/roles/privacy.yml @@ -0,0 +1,60 @@ +# 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 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 diff --git a/test/integration/targets/roles/roles/a/vars/main.yml b/test/integration/targets/roles/roles/a/vars/main.yml new file mode 100644 index 00000000000000..7812aa7856145c --- /dev/null +++ b/test/integration/targets/roles/roles/a/vars/main.yml @@ -0,0 +1 @@ +privacy: in role a diff --git a/test/integration/targets/roles/runme.sh b/test/integration/targets/roles/runme.sh index 774664294ac526..bf3aaf58239e65 100755 --- a/test/integration/targets/roles/runme.sh +++ b/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 "$@" diff --git a/test/integration/targets/roles/vars/privacy_vars.yml b/test/integration/targets/roles/vars/privacy_vars.yml new file mode 100644 index 00000000000000..9539ed042d9340 --- /dev/null +++ b/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') == ''}}"