Repository: ansible/ansible
Pull request: add jinja2 global to reserved names
  also allow expansion by additional context provided from caller

  fixes #41955

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Bugfix Pull Request
##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below -->
vars

Problem statement:
jinja keyword collision for variable names
##### SUMMARY
<!--- Explain the problem briefly -->
k8s_raw fails using jinja default filter to default undefined variables

##### ISSUE TYPE
 - Bug Report

##### COMPONENT NAME
k8s_raw

##### ANSIBLE VERSION

```
ansible 2.6.0rc4
(python 3)
```

##### STEPS TO REPRODUCE
```
---
- name: K8s
  hosts: localhost
  tasks:
  - name: Create namespace
    k8s:
      name: "{{ namespace | default('horizontesting123', true) }}"
      api_version: v1
      kind: Namespace
      state: present
```
This will fail, because the `name:` does not get the default value.

```
TASK [Create namespace] *****************************************************************************************************************************************************************************************
task path: /Users/proto/SORGENTI/SWITCH-openstack-horizon-k8s-deployment/ansible/test.yaml:7
<127.0.0.1> ESTABLISH LOCAL CONNECTION FOR USER: proto
<127.0.0.1> EXEC /bin/sh -c 'echo ~proto && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /Users/proto/.ansible/tmp/ansible-tmp-1530002441.681397-275101092917815 `" && echo ansible-tmp-1530002441.681397-275101092917815="` echo /Users/proto/.ansible/tmp/ansible-tmp-1530002441.681397-275101092917815 `" ) && sleep 0'
Using module file /usr/local/lib/python3.6/site-packages/ansible/modules/clustering/k8s/k8s.py
<127.0.0.1> PUT /Users/proto/.ansible/tmp/ansible-local-308812tchkxm/tmpco_jq671 TO /Users/proto/.ansible/tmp/ansible-tmp-1530002441.681397-275101092917815/k8s.py
<127.0.0.1> EXEC /bin/sh -c 'chmod u+x /Users/proto/.ansible/tmp/ansible-tmp-1530002441.681397-275101092917815/ /Users/proto/.ansible/tmp/ansible-tmp-1530002441.681397-275101092917815/k8s.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '/usr/local/opt/python/bin/python3.6 /Users/proto/.ansible/tmp/ansible-tmp-1530002441.681397-275101092917815/k8s.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c 'rm -f -r /Users/proto/.ansible/tmp/ansible-tmp-1530002441.681397-275101092917815/ > /dev/null 2>&1 && sleep 0'
The full traceback is:
  File "/var/folders/10/99r84g5d0_n5ct1248_dmtgh00012p/T/ansible__6rt22sd/ansible_modlib.zip/ansible/module_utils/k8s/raw.py", line 111, in perform_action
    existing = resource.get(name=name, namespace=namespace)
  File "/usr/local/Cellar/python/3.6.5/Frameworks/Python.framework/Versions/3.6/lib/python3.6/site-packages/openshift/dynamic/client.py", line 33, in inner
    raise api_exception(e)

fatal: [localhost]: FAILED! => {
    "changed": false,
    "error": 400,
    "invocation": {
        "module_args": {
            "api_key": null,
            "cert_file": null,
            "context": null,
            "host": null,
            "key_file": null,
            "kubeconfig": null,
            "password": null,
            "ssl_ca_cert": null,
            "username": null,
            "verify_ssl": null
        }
    },
    "msg": "Failed to retrieve requested object: b'400 Bad Request'",
    "reason": "Bad Request",
    "status": 400
}
	to retry, use: --limit @/Users/proto/SORGENTI/SWITCH-openstack-horizon-k8s-deployment/ansible/test.retry
```

I have found that doing:
```
---
- name: K8s
  hosts: localhost
  tasks:
  - name: Create namespace
    k8s:
      name: "horizontesting123"
      api_version: v1
      kind: Namespace
      state: present
```
or 
```
---
- name: K8s
  hosts: localhost
  vars:
     namespace: mytest234
  tasks:
  - name: Create namespace
    k8s:
      name: "{{ namespace }}"
      api_version: v1
      kind: Namespace
      state: present
```
then everything works as expected.

##### EXPECTED RESULTS

It looks like the filter `"{{ namespace | default('horizontesting123', true) }}"` does not work in the `name`. Not sure if this is an ansible bug or a k8s_raw module bug. Is there anything wrong in using the default filter there of an undefined variable ?

I would expect the everything to work when using the jinja filter to default undefined variables.

Hints from issue discussion:
Files identified in the description:
* [lib/ansible/modules/clustering/k8s/_k8s_raw.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/clustering/k8s/_k8s_raw.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @chouseknecht @fabianvf @flaper87 @maxamillion
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
Does this occur if you use the full definition like so:

```yaml
---
- name: K8s
  hosts: localhost
  tasks:
  - name: Create namespace
    k8s:
      definition:
        metadata:
          name: "{{ namespace | default('horizontesting123', true) }}"
        apiVersion: v1
        kind: Namespace
      state: present
```
Yes the problem occurs also when using the extended `definition`.

Here how to reproduce the problem.

This version works correctly:
```
---
- name: K8s
  hosts: localhost
  tasks:
  - name: Create namespace
    k8s:
      kubeconfig: /Users/proto/.kube/config
      definition:
        metadata:
          name: "horizontesting123"
        kind: Namespace

```

But this version fails:
```
---
- name: K8s
  hosts: localhost
  tasks:
  - name: Create namespace
    k8s:
      kubeconfig: /Users/proto/.kube/config
      definition:
        metadata:
          name: "{{ namespace | default('debugdebug123', true) }}"
        kind: Namespace
```

With the following output:
```
TASK [Create namespace] **********************************************************************************************************************************************************************************************************************************
task path: /Users/proto/SORGENTI/SWITCH-openstack-horizon-k8s-deployment/ansible/test2.yaml:5
<127.0.0.1> ESTABLISH LOCAL CONNECTION FOR USER: proto
<127.0.0.1> EXEC /bin/sh -c 'echo ~proto && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /Users/proto/.ansible/tmp/ansible-tmp-1530019222.39-43013695408546 `" && echo ansible-tmp-1530019222.39-43013695408546="` echo /Users/proto/.ansible/tmp/ansible-tmp-1530019222.39-43013695408546 `" ) && sleep 0'
Using module file /usr/local/lib/python2.7/site-packages/ansible/modules/clustering/k8s/k8s.py
<127.0.0.1> PUT /Users/proto/.ansible/tmp/ansible-local-15468mi9F9J/tmp1MncYR TO /Users/proto/.ansible/tmp/ansible-tmp-1530019222.39-43013695408546/k8s.py
<127.0.0.1> EXEC /bin/sh -c 'chmod u+x /Users/proto/.ansible/tmp/ansible-tmp-1530019222.39-43013695408546/ /Users/proto/.ansible/tmp/ansible-tmp-1530019222.39-43013695408546/k8s.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '/usr/local/opt/python@2/bin/python2.7 /Users/proto/.ansible/tmp/ansible-tmp-1530019222.39-43013695408546/k8s.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c 'rm -f -r /Users/proto/.ansible/tmp/ansible-tmp-1530019222.39-43013695408546/ > /dev/null 2>&1 && sleep 0'
The full traceback is:
  File "/var/folders/10/99r84g5d0_n5ct1248_dmtgh00012p/T/ansible_mivhMq/ansible_modlib.zip/ansible/module_utils/k8s/raw.py", line 111, in perform_action
    existing = resource.get(name=name, namespace=namespace)
  File "/usr/local/lib/python2.7/site-packages/openshift/dynamic/client.py", line 33, in inner
    raise api_exception(e)

fatal: [localhost]: FAILED! => {
    "changed": false,
    "error": 400,
    "invocation": {
        "module_args": {
            "api_key": null,
            "cert_file": null,
            "context": null,
            "host": null,
            "key_file": null,
            "kubeconfig": "/Users/proto/.kube/config",
            "password": null,
            "ssl_ca_cert": null,
            "username": null,
            "verify_ssl": null
        }
    },
    "msg": "Failed to retrieve requested object: 400 Bad Request",
    "reason": "Bad Request",
    "status": 400
}
	to retry, use: --limit @/Users/proto/SORGENTI/SWITCH-openstack-horizon-k8s-deployment/ansible/test2.retry
```

Interestingly it seems that namespace is a jinja keyword, if I add the task

```yaml
- debug: var=namespace
```
it outputs
```
TASK [debug] *****************************************************************************************************************************************************************************************************************************************************************************************************************
ok: [localhost] => {
    "namespace": "<class 'jinja2.utils.Namespace'>"
}
```

If you replace the variable `namespace` with anything else, like `my_namespace`, it seems to work fine. @maxamillion is there anything we can do about this? `namespace` is going to be a very popular choice for the namespace attribute of kubernetes objects.
@zioproto would you mind changing the issue name to note that this is an issue of name collision? 
cc @willthames
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
ping @jimi-c do you have any thoughts on this?
Files identified in the description:
None

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
This isn't limited to just the k8s_raw module, it's a global issue with Ansible's filtering.

e.g
```
 - name: "Setup Kubernetes Ingress"
    shell: >
      echo "{{ lookup( 'template', 'ingress.yaml.j2') }}" | kubectl apply -f -
```

Commit message: actually spell conflicting var corerezstly

no need for -E either

Patch to review:
diff --git a/changelogs/fragments/moar_reserved_vars.yml b/changelogs/fragments/moar_reserved_vars.yml
new file mode 100644
index 00000000000000..2b8e0e2bd74ce4
--- /dev/null
+++ b/changelogs/fragments/moar_reserved_vars.yml
@@ -0,0 +1,2 @@
+bugfixes:
+  - Handle more varnames that can create conflicts, expand function in general, handle jinja2 globasl in particular.
diff --git a/lib/ansible/executor/task_queue_manager.py b/lib/ansible/executor/task_queue_manager.py
index e7277c9183c3d0..07ca5c42e8145f 100644
--- a/lib/ansible/executor/task_queue_manager.py
+++ b/lib/ansible/executor/task_queue_manager.py
@@ -31,13 +31,10 @@
 from ansible.executor.task_result import TaskResult
 from ansible.module_utils.six import string_types
 from ansible.module_utils._text import to_text, to_native
-from ansible.playbook.block import Block
 from ansible.playbook.play_context import PlayContext
 from ansible.plugins.loader import callback_loader, strategy_loader, module_loader
 from ansible.plugins.callback import CallbackBase
 from ansible.template import Templar
-from ansible.utils.collection_loader import AnsibleCollectionRef
-from ansible.utils.helpers import pct_to_int
 from ansible.vars.hostvars import HostVars
 from ansible.vars.reserved import warn_if_reserved
 from ansible.utils.display import Display
@@ -197,8 +194,8 @@ def run(self, play):
             self.load_callbacks()
 
         all_vars = self._variable_manager.get_vars(play=play)
-        warn_if_reserved(all_vars)
         templar = Templar(loader=self._loader, variables=all_vars)
+        warn_if_reserved(all_vars, templar.environment.globals.keys())
 
         new_play = play.copy()
         new_play.post_validate(templar)
diff --git a/lib/ansible/vars/reserved.py b/lib/ansible/vars/reserved.py
index d7daea070579f3..2a2ec8de1b5df6 100644
--- a/lib/ansible/vars/reserved.py
+++ b/lib/ansible/vars/reserved.py
@@ -65,12 +65,17 @@ def get_reserved_names(include_private=True):
     return result
 
 
-def warn_if_reserved(myvars):
+def warn_if_reserved(myvars, additional=None):
     ''' this function warns if any variable passed conflicts with internally reserved names '''
 
+    if additional is None:
+        reserved = _RESERVED_NAMES
+    else:
+        reserved = _RESERVED_NAMES.union(additional)
+
     varnames = set(myvars)
     varnames.discard('vars')  # we add this one internally, so safe to ignore
-    for varname in varnames.intersection(_RESERVED_NAMES):
+    for varname in varnames.intersection(reserved):
         display.warning('Found variable using reserved name: %s' % varname)
 
 
diff --git a/test/integration/targets/var_reserved/reserved_varname_warning.yml b/test/integration/targets/var_reserved/reserved_varname_warning.yml
new file mode 100644
index 00000000000000..52444a2aa8710a
--- /dev/null
+++ b/test/integration/targets/var_reserved/reserved_varname_warning.yml
@@ -0,0 +1,6 @@
+- hosts: localhost
+  gather_facts: false
+  vars:
+    namespace: jinja2 uses me internally
+  tasks:
+    - debug: msg={{namespace}}
diff --git a/test/integration/targets/var_reserved/runme.sh b/test/integration/targets/var_reserved/runme.sh
new file mode 100755
index 00000000000000..28298446a484f4
--- /dev/null
+++ b/test/integration/targets/var_reserved/runme.sh
@@ -0,0 +1,5 @@
+#!/usr/bin/env bash
+
+set -eux
+
+ansible-playbook reserved_varname_warning.yml "$@" 2>&1| grep 'Found variable using reserved name: namespace'


Code review comments:
diff_hunk: @@ -0,0 +1,2 @@
+bugfixes:
+  - Handle more varnames that can create conflicts, expand function in general, handle jinja2 globasl in particular.
path: changelogs/fragments/moar_reserved_vars.yml
text: @user1:
```suggestion
  - Handle more varnames that can create conflicts, expand a function in general, handle jinja2 globals in particular (https://github.com/ansible/ansible/issues/41955).
```

diff_hunk: @@ -0,0 +1,2 @@
+bugfixes:
+  - Handle more varnames that can create conflicts, expand function in general, handle jinja2 globasl in particular.
path: changelogs/fragments/moar_reserved_vars.yml
text: @user1:
```suggestion
  - Handle more varnames that can create conflicts, expand function in general, handle jinja2 globals in particular.
```