From 27b4d7ed31b6688253fc4089b7a6b97f2d548167 Mon Sep 17 00:00:00 2001
From: Matt Martz <matt@sivel.net>
Date: Sun, 15 Jul 2018 09:59:30 -0500
Subject: [PATCH] Add feature to expose vars/defaults with include/import_role
 (#41330)

* First pass at making 'private' work on include_role, imports are always public

* Prevent dupe task execution and overwriting handlers

* New functionality will use public instead of deprecated private

* Add tests for public exposure

* Validate vars before import/include to ensure they don't expose too early

* Add porting guide docs about public argument and change to import_role

* Add additional docs about public and vars exposure to module docs

* Insert role handlers at parse time, exposing them globally
---
 .../rst/porting_guides/porting_guide_2.7.rst  |  9 +++
 .../modules/utilities/logic/import_role.py    |  3 +
 .../modules/utilities/logic/include_role.py   |  7 +++
 lib/ansible/playbook/play.py                  |  4 ++
 lib/ansible/playbook/role/__init__.py         | 20 +++++--
 lib/ansible/playbook/role_include.py          | 16 ++++--
 .../public_exposure/playbook.yml              | 56 +++++++++++++++++++
 .../roles/dynamic/defaults/main.yml           |  1 +
 .../roles/dynamic/tasks/main.yml              |  5 ++
 .../roles/dynamic/vars/main.yml               |  1 +
 .../roles/dynamic_private/defaults/main.yml   |  1 +
 .../roles/dynamic_private/tasks/main.yml      |  5 ++
 .../roles/dynamic_private/vars/main.yml       |  1 +
 .../roles/from/defaults/from.yml              |  1 +
 .../public_exposure/roles/from/tasks/from.yml |  5 ++
 .../public_exposure/roles/from/vars/from.yml  |  1 +
 .../roles/regular/defaults/main.yml           |  1 +
 .../roles/regular/tasks/main.yml              |  5 ++
 .../roles/regular/vars/main.yml               |  1 +
 .../roles/static/defaults/main.yml            |  1 +
 .../roles/static/tasks/main.yml               |  5 ++
 .../roles/static/vars/main.yml                |  1 +
 .../targets/include_import/runme.sh           |  4 +-
 23 files changed, 145 insertions(+), 9 deletions(-)
 create mode 100644 test/integration/targets/include_import/public_exposure/playbook.yml
 create mode 100644 test/integration/targets/include_import/public_exposure/roles/dynamic/defaults/main.yml
 create mode 100644 test/integration/targets/include_import/public_exposure/roles/dynamic/tasks/main.yml
 create mode 100644 test/integration/targets/include_import/public_exposure/roles/dynamic/vars/main.yml
 create mode 100644 test/integration/targets/include_import/public_exposure/roles/dynamic_private/defaults/main.yml
 create mode 100644 test/integration/targets/include_import/public_exposure/roles/dynamic_private/tasks/main.yml
 create mode 100644 test/integration/targets/include_import/public_exposure/roles/dynamic_private/vars/main.yml
 create mode 100644 test/integration/targets/include_import/public_exposure/roles/from/defaults/from.yml
 create mode 100644 test/integration/targets/include_import/public_exposure/roles/from/tasks/from.yml
 create mode 100644 test/integration/targets/include_import/public_exposure/roles/from/vars/from.yml
 create mode 100644 test/integration/targets/include_import/public_exposure/roles/regular/defaults/main.yml
 create mode 100644 test/integration/targets/include_import/public_exposure/roles/regular/tasks/main.yml
 create mode 100644 test/integration/targets/include_import/public_exposure/roles/regular/vars/main.yml
 create mode 100644 test/integration/targets/include_import/public_exposure/roles/static/defaults/main.yml
 create mode 100644 test/integration/targets/include_import/public_exposure/roles/static/tasks/main.yml
 create mode 100644 test/integration/targets/include_import/public_exposure/roles/static/vars/main.yml

diff --git a/docs/docsite/rst/porting_guides/porting_guide_2.7.rst b/docs/docsite/rst/porting_guides/porting_guide_2.7.rst
index cb7db78bb44..5f370bc2c9e 100644
--- a/docs/docsite/rst/porting_guides/porting_guide_2.7.rst
+++ b/docs/docsite/rst/porting_guides/porting_guide_2.7.rst
@@ -26,6 +26,15 @@ Before Ansible 2.7, when loading a role, the variables defined in the role's ``v
 
 In Ansible 2.7, role ``vars`` and ``defaults`` are now parsed before ``tasks/main.yml``. This can cause a change in behavior if the same variable is defined at the play level and the role level with different values, and leveraged in ``import_tasks`` or ``import_role`` to define the role or file to import.
 
+include_role and import_role variable exposure
+----------------------------------------------
+
+In Ansible 2.7 a new module argument named ``public`` was added to the ``include_role`` module that dictates whether or not the role's ``defaults`` and ``vars`` will be exposed outside of the role, allowing those variables to be used by later tasks.  This value defaults to ``public: False``, matching current behavior.
+
+``import_role`` does not support the ``public`` argument, and will unconditionally expose the role's ``defaults`` and ``vars`` to the rest of the playbook. This functinality brings ``import_role`` into closer alignment with roles listed within the ``roles`` header in a play.
+
+There is an important difference in the way that ``include_role`` (dynamic) will expose the role's variables, as opposed to ``import_role`` (static). ``import_role`` is a pre-processor, and the ``defaults`` and ``vars`` are evaluated at playbook parsing, making the variables available to tasks and roles listed at any point in the play. ``include_role`` is a conditional task, and the ``defaults`` and ``vars`` are evaluated at execution time, making the variables available to tasks and roles listed *after* the ``include_role`` task.
+
 Deprecated
 ==========
 
diff --git a/lib/ansible/modules/utilities/logic/import_role.py b/lib/ansible/modules/utilities/logic/import_role.py
index 79dc0af361a..e60577e1289 100644
--- a/lib/ansible/modules/utilities/logic/import_role.py
+++ b/lib/ansible/modules/utilities/logic/import_role.py
@@ -56,6 +56,9 @@ options:
     default: 'no'
 notes:
   - Handlers are made available to the whole play.
+  - Variables defined in C(vars) and C(default) for the role are exposed at playbook parsing time. Due to this,
+    these variables will be accessible to roles and tasks executed before the the location of the C(import_role) task.
+  - Unlike C(include_role) variable exposure is not configurable, and will always be exposed.
 '''
 
 EXAMPLES = """
diff --git a/lib/ansible/modules/utilities/logic/include_role.py b/lib/ansible/modules/utilities/logic/include_role.py
index 010f597323b..155ce10f00c 100644
--- a/lib/ansible/modules/utilities/logic/include_role.py
+++ b/lib/ansible/modules/utilities/logic/include_role.py
@@ -55,8 +55,15 @@ options:
     description:
       - This option is a no op, and the functionality described in previous versions was not implemented. This
         option will be removed in Ansible v2.8.
+  public:
+    description:
+      - This option dictates whether the role's C(vars) and C(defaults) are exposed to the playbook. If set to C(yes)
+        the variables will be available to tasks following the C(include_role) task. This functionality differs from
+        standard variable exposure for roles listed under the C(roles) header or C(import_role) as they are exposed at
+        playbook parsing time, and available to earlier roles and tasks as well.
     type: bool
     default: 'no'
+    version_added: '2.7'
 notes:
   - Handlers are made available to the whole play.
   - Before Ansible 2.4, as with C(include), this task could be static or dynamic, If static, it implied that it won't
diff --git a/lib/ansible/playbook/play.py b/lib/ansible/playbook/play.py
index 7554b92b8e0..8933a21fd71 100644
--- a/lib/ansible/playbook/play.py
+++ b/lib/ansible/playbook/play.py
@@ -233,6 +233,10 @@ class Play(Base, Taggable, Become):
 
         if len(self.roles) > 0:
             for r in self.roles:
+                # Don't insert tasks from ``import/include_role``, preventing
+                # duplicate execution at the wrong time
+                if r.from_include:
+                    continue
                 block_list.extend(r.compile(play=self))
 
         return block_list
diff --git a/lib/ansible/playbook/role/__init__.py b/lib/ansible/playbook/role/__init__.py
index 18776a0953e..51ee5a0b589 100644
--- a/lib/ansible/playbook/role/__init__.py
+++ b/lib/ansible/playbook/role/__init__.py
@@ -96,7 +96,7 @@ class Role(Base, Become, Conditional, Taggable):
     _delegate_to = FieldAttribute(isa='string')
     _delegate_facts = FieldAttribute(isa='bool', default=False)
 
-    def __init__(self, play=None, from_files=None):
+    def __init__(self, play=None, from_files=None, from_include=False):
         self._role_name = None
         self._role_path = None
         self._role_params = dict()
@@ -108,6 +108,7 @@ class Role(Base, Become, Conditional, Taggable):
         self._dependencies = []
         self._task_blocks = []
         self._handler_blocks = []
+        self._compiled_handler_blocks = None
         self._default_vars = dict()
         self._role_vars = dict()
         self._had_task_run = dict()
@@ -117,6 +118,9 @@ class Role(Base, Become, Conditional, Taggable):
             from_files = {}
         self._from_files = from_files
 
+        # Indicates whether this role was included via include/import_role
+        self.from_include = from_include
+
         super(Role, self).__init__()
 
     def __repr__(self):
@@ -126,7 +130,7 @@ class Role(Base, Become, Conditional, Taggable):
         return self._role_name
 
     @staticmethod
-    def load(role_include, play, parent_role=None, from_files=None):
+    def load(role_include, play, parent_role=None, from_files=None, from_include=False):
 
         if from_files is None:
             from_files = {}
@@ -153,7 +157,7 @@ class Role(Base, Become, Conditional, Taggable):
                             role_obj.add_parent(parent_role)
                         return role_obj
 
-            r = Role(play=play, from_files=from_files)
+            r = Role(play=play, from_files=from_files, from_include=from_include)
             r._load_role_data(role_include, parent_role=parent_role)
 
             if role_include.role not in play.ROLE_CACHE:
@@ -359,7 +363,15 @@ class Role(Base, Become, Conditional, Taggable):
         return self._task_blocks[:]
 
     def get_handler_blocks(self, play, dep_chain=None):
-        block_list = []
+        # Do not recreate this list each time ``get_handler_blocks`` is called.
+        # Cache the results so that we don't potentially overwrite with copied duplicates
+        #
+        # ``get_handler_blocks`` may be called when handling ``import_role`` during parsing
+        # as well as with ``Play.compile_roles_handlers`` from ``TaskExecutor``
+        if self._compiled_handler_blocks:
+            return self._compiled_handler_blocks
+
+        self._compiled_handler_blocks = block_list = []
 
         # update the dependency chain here
         if dep_chain is None:
diff --git a/lib/ansible/playbook/role_include.py b/lib/ansible/playbook/role_include.py
index e924422ee9d..37fc0ad6836 100644
--- a/lib/ansible/playbook/role_include.py
+++ b/lib/ansible/playbook/role_include.py
@@ -46,7 +46,7 @@ class IncludeRole(TaskInclude):
 
     BASE = ('name', 'role')  # directly assigned
     FROM_ARGS = ('tasks_from', 'vars_from', 'defaults_from')  # used to populate from dict in role
-    OTHER_ARGS = ('apply', 'private', 'allow_duplicates')  # assigned to matching property
+    OTHER_ARGS = ('apply', 'private', 'public', 'allow_duplicates')  # assigned to matching property
     VALID_ARGS = tuple(frozenset(BASE + FROM_ARGS + OTHER_ARGS))  # all valid args
 
     # =================================================================================
@@ -55,6 +55,7 @@ class IncludeRole(TaskInclude):
     # private as this is a 'module options' vs a task property
     _allow_duplicates = FieldAttribute(isa='bool', default=True, private=True)
     _private = FieldAttribute(isa='bool', default=None, private=True)
+    _public = FieldAttribute(isa='bool', default=False, private=True)
 
     def __init__(self, block=None, role=None, task_include=None):
 
@@ -81,9 +82,13 @@ class IncludeRole(TaskInclude):
         ri.vars.update(self.vars)
 
         # build role
-        actual_role = Role.load(ri, myplay, parent_role=self._parent_role, from_files=self._from_files)
+        actual_role = Role.load(ri, myplay, parent_role=self._parent_role, from_files=self._from_files,
+                                from_include=True)
         actual_role._metadata.allow_duplicates = self.allow_duplicates
 
+        if self.statically_loaded or self.public:
+            myplay.roles.append(actual_role)
+
         # save this for later use
         self._role_path = actual_role._role_path
 
@@ -119,9 +124,12 @@ class IncludeRole(TaskInclude):
         if ir._role_name is None:
             raise AnsibleParserError("'name' is a required field for %s." % ir.action, obj=data)
 
-        if ir.private is not None:
+        if 'public' in ir.args and ir.action != 'include_role':
+            raise AnsibleParserError('Invalid options for %s: private' % ir.action, obj=data)
+
+        if 'private' in ir.args:
             display.deprecated(
-                msg='Supplying "private" for "include_role" is a no op, and is deprecated',
+                msg='Supplying "private" for "%s" is a no op, and is deprecated' % ir.action,
                 version='2.8'
             )
 
diff --git a/test/integration/targets/include_import/public_exposure/playbook.yml b/test/integration/targets/include_import/public_exposure/playbook.yml
new file mode 100644
index 00000000000..11735e7731b
--- /dev/null
+++ b/test/integration/targets/include_import/public_exposure/playbook.yml
@@ -0,0 +1,56 @@
+---
+- hosts: testhost
+  gather_facts: false
+  roles:
+    - regular
+  tasks:
+    - debug:
+        msg: start tasks
+
+    - name: Static imports should expose vars at parse time, not at execution time
+      assert:
+        that:
+          - static_defaults_var == 'static_defaults'
+          - static_vars_var == 'static_vars'
+    - import_role:
+        name: static
+    - assert:
+        that:
+          - static_tasks_var == 'static_tasks'
+          - static_defaults_var == 'static_defaults'
+          - static_vars_var == 'static_vars'
+
+    - include_role:
+        name: dynamic_private
+    - assert:
+        that:
+          - private_tasks_var == 'private_tasks'
+          - private_defaults_var is undefined
+          - private_vars_var is undefined
+
+    - name: Dynamic include should not expose vars until execution time
+      assert:
+        that:
+          - dynamic_tasks_var is undefined
+          - dynamic_defaults_var is undefined
+          - dynamic_vars_var is undefined
+    - include_role:
+        name: dynamic
+        public: true
+    - assert:
+        that:
+          - dynamic_tasks_var == 'dynamic_tasks'
+          - dynamic_defaults_var == 'dynamic_defaults'
+          - dynamic_vars_var == 'dynamic_vars'
+
+    - include_role:
+        name: from
+        public: true
+        tasks_from: from.yml
+        vars_from: from.yml
+        defaults_from: from.yml
+    - assert:
+        that:
+          - from_tasks_var == 'from_tasks'
+          - from_defaults_var == 'from_defaults'
+          - from_vars_var == 'from_vars'
diff --git a/test/integration/targets/include_import/public_exposure/roles/dynamic/defaults/main.yml b/test/integration/targets/include_import/public_exposure/roles/dynamic/defaults/main.yml
new file mode 100644
index 00000000000..099ac29b712
--- /dev/null
+++ b/test/integration/targets/include_import/public_exposure/roles/dynamic/defaults/main.yml
@@ -0,0 +1 @@
+dynamic_defaults_var: dynamic_defaults
diff --git a/test/integration/targets/include_import/public_exposure/roles/dynamic/tasks/main.yml b/test/integration/targets/include_import/public_exposure/roles/dynamic/tasks/main.yml
new file mode 100644
index 00000000000..e9b9ad3df7c
--- /dev/null
+++ b/test/integration/targets/include_import/public_exposure/roles/dynamic/tasks/main.yml
@@ -0,0 +1,5 @@
+- debug:
+    msg: dynamic
+
+- set_fact:
+    dynamic_tasks_var: dynamic_tasks
diff --git a/test/integration/targets/include_import/public_exposure/roles/dynamic/vars/main.yml b/test/integration/targets/include_import/public_exposure/roles/dynamic/vars/main.yml
new file mode 100644
index 00000000000..b33c12df33b
--- /dev/null
+++ b/test/integration/targets/include_import/public_exposure/roles/dynamic/vars/main.yml
@@ -0,0 +1 @@
+dynamic_vars_var: dynamic_vars
diff --git a/test/integration/targets/include_import/public_exposure/roles/dynamic_private/defaults/main.yml b/test/integration/targets/include_import/public_exposure/roles/dynamic_private/defaults/main.yml
new file mode 100644
index 00000000000..b19ef72c2d7
--- /dev/null
+++ b/test/integration/targets/include_import/public_exposure/roles/dynamic_private/defaults/main.yml
@@ -0,0 +1 @@
+private_defaults_var: private_defaults
diff --git a/test/integration/targets/include_import/public_exposure/roles/dynamic_private/tasks/main.yml b/test/integration/targets/include_import/public_exposure/roles/dynamic_private/tasks/main.yml
new file mode 100644
index 00000000000..1c7f653d41d
--- /dev/null
+++ b/test/integration/targets/include_import/public_exposure/roles/dynamic_private/tasks/main.yml
@@ -0,0 +1,5 @@
+- debug:
+    msg: private
+
+- set_fact:
+    private_tasks_var: private_tasks
diff --git a/test/integration/targets/include_import/public_exposure/roles/dynamic_private/vars/main.yml b/test/integration/targets/include_import/public_exposure/roles/dynamic_private/vars/main.yml
new file mode 100644
index 00000000000..60f7ca8102f
--- /dev/null
+++ b/test/integration/targets/include_import/public_exposure/roles/dynamic_private/vars/main.yml
@@ -0,0 +1 @@
+private_vars_var: private_vars
diff --git a/test/integration/targets/include_import/public_exposure/roles/from/defaults/from.yml b/test/integration/targets/include_import/public_exposure/roles/from/defaults/from.yml
new file mode 100644
index 00000000000..6729c4b4803
--- /dev/null
+++ b/test/integration/targets/include_import/public_exposure/roles/from/defaults/from.yml
@@ -0,0 +1 @@
+from_defaults_var: from_defaults
diff --git a/test/integration/targets/include_import/public_exposure/roles/from/tasks/from.yml b/test/integration/targets/include_import/public_exposure/roles/from/tasks/from.yml
new file mode 100644
index 00000000000..932efc9f861
--- /dev/null
+++ b/test/integration/targets/include_import/public_exposure/roles/from/tasks/from.yml
@@ -0,0 +1,5 @@
+- debug:
+    msg: from
+
+- set_fact:
+    from_tasks_var: from_tasks
diff --git a/test/integration/targets/include_import/public_exposure/roles/from/vars/from.yml b/test/integration/targets/include_import/public_exposure/roles/from/vars/from.yml
new file mode 100644
index 00000000000..98b2ad47455
--- /dev/null
+++ b/test/integration/targets/include_import/public_exposure/roles/from/vars/from.yml
@@ -0,0 +1 @@
+from_vars_var: from_vars
diff --git a/test/integration/targets/include_import/public_exposure/roles/regular/defaults/main.yml b/test/integration/targets/include_import/public_exposure/roles/regular/defaults/main.yml
new file mode 100644
index 00000000000..21a6967c31c
--- /dev/null
+++ b/test/integration/targets/include_import/public_exposure/roles/regular/defaults/main.yml
@@ -0,0 +1 @@
+regular_defaults_var: regular_defaults
diff --git a/test/integration/targets/include_import/public_exposure/roles/regular/tasks/main.yml b/test/integration/targets/include_import/public_exposure/roles/regular/tasks/main.yml
new file mode 100644
index 00000000000..eafa141a65e
--- /dev/null
+++ b/test/integration/targets/include_import/public_exposure/roles/regular/tasks/main.yml
@@ -0,0 +1,5 @@
+- debug:
+    msg: regular
+
+- set_fact:
+    regular_tasks_var: regular_tasks
diff --git a/test/integration/targets/include_import/public_exposure/roles/regular/vars/main.yml b/test/integration/targets/include_import/public_exposure/roles/regular/vars/main.yml
new file mode 100644
index 00000000000..3d06546f7e9
--- /dev/null
+++ b/test/integration/targets/include_import/public_exposure/roles/regular/vars/main.yml
@@ -0,0 +1 @@
+regular_vars_var: regular_vars
diff --git a/test/integration/targets/include_import/public_exposure/roles/static/defaults/main.yml b/test/integration/targets/include_import/public_exposure/roles/static/defaults/main.yml
new file mode 100644
index 00000000000..d88f5559afd
--- /dev/null
+++ b/test/integration/targets/include_import/public_exposure/roles/static/defaults/main.yml
@@ -0,0 +1 @@
+static_defaults_var: static_defaults
diff --git a/test/integration/targets/include_import/public_exposure/roles/static/tasks/main.yml b/test/integration/targets/include_import/public_exposure/roles/static/tasks/main.yml
new file mode 100644
index 00000000000..5a6488c180b
--- /dev/null
+++ b/test/integration/targets/include_import/public_exposure/roles/static/tasks/main.yml
@@ -0,0 +1,5 @@
+- debug:
+    msg: static
+
+- set_fact:
+    static_tasks_var: static_tasks
diff --git a/test/integration/targets/include_import/public_exposure/roles/static/vars/main.yml b/test/integration/targets/include_import/public_exposure/roles/static/vars/main.yml
new file mode 100644
index 00000000000..982e34d0531
--- /dev/null
+++ b/test/integration/targets/include_import/public_exposure/roles/static/vars/main.yml
@@ -0,0 +1 @@
+static_vars_var: static_vars
diff --git a/test/integration/targets/include_import/runme.sh b/test/integration/targets/include_import/runme.sh
index c2b4abf4a7f..cc3c1010b63 100755
--- a/test/integration/targets/include_import/runme.sh
+++ b/test/integration/targets/include_import/runme.sh
@@ -78,4 +78,6 @@ fi
 ANSIBLE_STRATEGY='linear' ansible-playbook tasks/test_include_dupe_loop.yml -i ../../inventory "$@" | tee test_include_dupe_loop.out
 test "$(grep -c '"item=foo"' test_include_dupe_loop.out)" = 3
 ANSIBLE_STRATEGY='free' ansible-playbook tasks/test_include_dupe_loop.yml -i ../../inventory "$@" | tee test_include_dupe_loop.out
-test "$(grep -c '"item=foo"' test_include_dupe_loop.out)" = 3
\ No newline at end of file
+test "$(grep -c '"item=foo"' test_include_dupe_loop.out)" = 3
+
+ansible-playbook public_exposure/playbook.yml -i ../../inventory "$@"