Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Ensure bare & full Jinja var give the same result #45719

Open
wants to merge 3 commits into
base: devel
Choose a base branch
from

Conversation

pilou-
Copy link
Contributor

@pilou- pilou- commented Sep 17, 2018

SUMMARY

Ensure bare & full Jinja var give the same result

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/playbook/conditional.py

ANSIBLE VERSION
ansible 2.8.0.dev0 (devel 66f03827d6) last updated 2018/09/16 17:09:22 (GMT +200)

@ansibot ansibot added WIP affects_2.8 bug needs_triage support:core small_patch labels Sep 17, 2018
@bcoca bcoca removed the needs_triage label Sep 17, 2018
@bcoca
Copy link
Member

bcoca commented Sep 17, 2018

i'm not sure this is a good fix since it can force double interpolation when it is not wanted.

Also, you should not hardcode {{ as this can be changed via the Jinja2 environment

@sivel
Copy link
Member

sivel commented Sep 17, 2018

I'm in agreement with bcoca here too. We warn when people use {{ }} in when conditions, because it causes double interpolation and unexpected results.

I don't think we should double eval the conditional.

This could cause us to end up with even more of a problem with the string "false" than we already have.

@pilou-
Copy link
Contributor Author

pilou- commented Sep 17, 2018

The fix might be not the right but I don't find any reason to have the first task executed while the other two are skipped:

- debug:
  when: '["0", "1"]|min'

- debug:
  when: '{{ ["0", "1"]|min }}'

- debug:
  vars:
    expr: '{{ ["0", "1"]|min }}'
  when: 'expr'

How can when: ["0", "1"]|min different than {{ when: ["0", "1"]|min }} ? Even if the expression should be ["0", "1"]|min|int, this should not happen.

@sivel
Copy link
Member

sivel commented Sep 17, 2018

This happens very specifically because the results are templated at different times.

In the first example, we template ["0", "1"]|min at the time that the conditional is evaluated, so the conditional uses the string "0".

In the second, we template it at task parsing, and then use the evaluated expression in the conditional.

So the first is evaluated as:

{% if ["0", "1"]|min %} True {% else %} False {% endif %}

The second, because of how we construct the conditional, just becomes:

{% if 0 %} True {% else %} False {% endif %}

Notice that the 0 is no longer quoted, because presented is defined as:

presented = "{%% if %s %%} True {%% else %%} False {%% endif %%}" % conditional

So because you templated it early, you got an unexpected result. Which is typically why when statements shouldn't use explicit templating, and why we issue a warning.

@sivel
Copy link
Member

sivel commented Sep 17, 2018

With the above outlined, I would say that your change is actually causing the wrong thing to happen even more than it already does.

@pilou-
Copy link
Contributor Author

pilou- commented Sep 17, 2018

This could cause us to end up with even more of a problem with the string "false" than we already have.

This patch doesn't change how string "false" is handled: when: ["false"]|first is the same as when: {{ ["false"]|first }} (with or without this patch).

How can when: ["0", "1"]|min different than {{ when: ["0", "1"]|min }} ?

I meant: AFAIK this behavior isn't documented and is unexpected.
Do you think the current behavior is acceptable ?

@sivel
Copy link
Member

sivel commented Sep 17, 2018

Do you think the current behavior is acceptable ?

I would consider what happens when you use explicit jinja templating in the when statement to be what is unexpected. So the current behavior is acceptable for me.

We warn users to not use explicit templating, for these reasons, but you are correct that it isn't really documented.

I believe we should probably expand the warning and potentially add documentation to this affect.

@pilou-
Copy link
Contributor Author

pilou- commented Sep 17, 2018

Note that this one:

- debug:
  vars:
    expr: '{{ ["0", "1"]|min }}'
  when: 'expr'

doesn't generate a warning and have the same behavior than

- debug:
  vars:
    expr: '["0", "1"]|min'
  when: 'expr'

Moreover, it is very confusing that the recommended format is:

- debug:
  vars:
    expr: '["0", "1"]|min'
  when: 'expr'

while the following task doesn't work as expected:

- hosts: localhost
  vars:
    varplay: '["0", "1"]|min'
  tasks:
    - debug:
      vars:
        expr: 'varplay'
      when: 'expr'

What is the recommended way to write the task above ?

@sivel
Copy link
Member

sivel commented Sep 18, 2018

What is the recommended way to write the task above ?

That is complicated, and the amount of indirection doesn't really make it feasible.

I'm not saying we don't have a problem, but it needs to be addressed in the opposite direction.

Instead of making bare expressions work like expressions with explicit jinja, we need to make explicit jinja expressions work like bare expressions.

This involves ensuring that we don't always insert "bare" strings into the jinja template.

I've played around with something this morning, although I am positive it has bugs:

diff --git a/lib/ansible/playbook/conditional.py b/lib/ansible/playbook/conditional.py
index 90c213bde3..e41b82d9ab 100644
--- a/lib/ansible/playbook/conditional.py
+++ b/lib/ansible/playbook/conditional.py
@@ -29,6 +29,7 @@ from ansible.errors import AnsibleError, AnsibleUndefinedVariable
 from ansible.module_utils.six import text_type
 from ansible.module_utils._text import to_native
 from ansible.playbook.attribute import FieldAttribute
+from ansible.utils.vars import isidentifier
 
 try:
     from __main__ import display
@@ -39,7 +40,6 @@ except ImportError:
 
 DEFINED_REGEX = re.compile(r'(hostvars\[.+\]|[\w_]+)\s+(not\s+is|is|is\s+not)\s+(defined|undefined)')
 LOOKUP_REGEX = re.compile(r'lookup\s*\(')
-VALID_VAR_REGEX = re.compile("^[_A-Za-z][_a-zA-Z0-9]*$")
 
 
 class Conditional:
@@ -129,7 +129,7 @@ class Conditional:
         #     - item
         #   with_items:
         #   - 1 == 1
-        if conditional in all_vars and VALID_VAR_REGEX.match(conditional):
+        if conditional in all_vars and isidentifier(conditional):
             conditional = all_vars[conditional]
 
         # make sure the templar is using the variables specified with this method
@@ -188,8 +188,11 @@ class Conditional:
             except Exception as e:
                 raise AnsibleError("Invalid conditional detected: %s" % to_native(e))
 
+            if conditional != original or (not isidentifier(conditional) and len(conditional.split()) == 1):
+                conditional = repr(to_native(conditional))
+
             # and finally we generate and template the presented string and look at the resulting string
-            presented = "{%% if %s %%} True {%% else %%} False {%% endif %%}" % conditional
+            presented = u"{%% if %s %%} True {%% else %%} False {%% endif %%}" % conditional
             val = templar.template(presented, disable_lookups=disable_lookups).strip()
             if val == "True":
                 return True

In your initial example, this creates templates like the following for presented:

{% if ["0", "1"]|min %} True {% else %} False {% endif %}
{% if '0' %} True {% else %} False {% endif %}

The notable difference is the 2nd, that shows a string of '0' instead of the integer 0.

@ansibot ansibot added the stale_ci label Sep 26, 2018
pilou- added 2 commits Oct 19, 2018
Behavior should not differ when bare variable is used with 'when' keyword.
Before this change:

    - debug:
      when: '["0", "1"]|min'  # not skipped
    - debug:
      when: '{{ ["0", "1"]|min }}'  # skipped
    - debug:
      vars:
        expr: '{{ ["0", "1"]|min }}'  # skipped
      when: 'expr'
Completely written by sivel, thanks to him :)
@ansibot ansibot removed small_patch stale_ci labels Oct 19, 2018
@ansibot ansibot added the stale_ci label Oct 27, 2018
@ansibot ansibot added the needs_rebase label Nov 26, 2018
@ansibot ansibot added pre_azp and removed stale_ci labels Dec 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.8 bug needs_rebase pre_azp support:core WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants