Exception in safe_eval() on expr with jinja2 >= 2.9 #20098

Open
major opened this Issue Jan 10, 2017 · 24 comments

Projects

None yet

10 participants

@major
Contributor
major commented Jan 10, 2017
ISSUE TYPE
  • Bug Report
COMPONENT NAME

ansible core

ANSIBLE VERSION
ansible 2.2.0.0
  config file = 
  configured module search path = Default w/o overrides
CONFIGURATION

Defaults.

OS / ENVIRONMENT

N/A (tested on CentOS 7, Ubuntu 16.04, Fedora 25)

SUMMARY

Ansible 2.2.0.0 with jinja2 < 2.9 will handle a variable with curly braces properly in self_eval(). However, jinja >= 2.9 fails with an exception.

STEPS TO REPRODUCE

Here's an example playbook:

---

- hosts: all
  vars:
    fruits:
      - name: apple
        enjoy: yes
      - name: orange
        enjoy: no
      - name: strawberry
        enjoy: yes
  tasks:

    - name: Loop through my fruits
      debug:
        msg: |
          {% set fruit_list = [] %}
          {% for fruit_dict in item[1] %}
          {{ fruit_dict.name }}
          {% endfor %}
      with_items:
        - "{{ fruits | groupby('enjoy') }}"

Start with jinja2 2.8.1. The playbook should execute successfully. Upgrade to jinja2 2.9 (or higher). The playbook will have an error (see below).

EXPECTED RESULTS

The playbook should run without errors.

ACTUAL RESULTS
TASK [Loop through my fruits] **************************************************
 [WARNING]: Exception in safe_eval() on expr: [_GroupTuple(grouper=False, list=[{u'enjoy': False, u'name': u'orange'}]), _GroupTuple(grouper=True,
list=[{u'enjoy': True, u'name': u'apple'}, {u'enjoy': True, u'name': u'strawberry'}])] (invalid expression ([_GroupTuple(grouper=False, list=[{u'enjoy':
False, u'name': u'orange'}]), _GroupTuple(grouper=True, list=[{u'enjoy': True, u'name': u'apple'}, {u'enjoy': True, u'name': u'strawberry'}])]))

fatal: [localhost]: FAILED! => {"failed": true, "msg": "the field 'args' has an invalid value, which appears to include a variable that is undefined. The error was: 'unicode object' has no attribute 'name'\n\nThe error appears to have been in '/tmp/ansible/playbook.yml': line 14, column 7, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n\n    - name: Loop through my fruits\n      ^ here\n"}

I still get the same problem if I install ansible from the latest commit in git.

@sivel
Member
sivel commented Jan 10, 2017

The full exception here is:

Traceback (most recent call last):
  File "/Users/matt/python_venvs/ansibledev/ansible/lib/ansible/executor/task_executor.py", line 88, in run
    items = self._get_loop_items()
  File "/Users/matt/python_venvs/ansibledev/ansible/lib/ansible/executor/task_executor.py", line 186, in _get_loop_items
    loop_terms = listify_lookup_plugin_terms(terms=self._task.loop_args, templar=templar, loader=self._loader, fail_on_undefined=True, convert_bare=False)
  File "/Users/matt/python_venvs/ansibledev/ansible/lib/ansible/utils/listify.py", line 36, in listify_lookup_plugin_terms
    terms = templar.template(terms, fail_on_undefined=fail_on_undefined)
  File "/Users/matt/python_venvs/ansibledev/ansible/lib/ansible/template/__init__.py", line 352, in template
    return [self.template(v, preserve_trailing_newlines=preserve_trailing_newlines, fail_on_undefined=fail_on_undefined, overrides=overrides) for v in variable]
  File "/Users/matt/python_venvs/ansibledev/ansible/lib/ansible/template/__init__.py", line 336, in template
    eval_results = safe_eval(result, locals=self._available_variables, include_exceptions=True)
  File "/Users/matt/python_venvs/ansibledev/ansible/lib/ansible/template/safe_eval.py", line 131, in safe_eval
    cnv.visit(parsed_tree)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/ast.py", line 241, in visit
    return visitor(node)
  File "/Users/matt/python_venvs/ansibledev/ansible/lib/ansible/template/safe_eval.py", line 120, in generic_visit
    self.generic_visit(child_node, inside_call)
  File "/Users/matt/python_venvs/ansibledev/ansible/lib/ansible/template/safe_eval.py", line 120, in generic_visit
    self.generic_visit(child_node, inside_call)
  File "/Users/matt/python_venvs/ansibledev/ansible/lib/ansible/template/safe_eval.py", line 120, in generic_visit
    self.generic_visit(child_node, inside_call)
  File "/Users/matt/python_venvs/ansibledev/ansible/lib/ansible/template/safe_eval.py", line 112, in generic_visit
    raise Exception("invalid expression (%s)" % expr)
Exception: invalid expression ([_GroupTuple(grouper=False, list=[{u'enjoy': False, u'name': u'orange'}]), _GroupTuple(grouper=True, list=[{u'enjoy': True, u'name': u'apple'}, {u'enjoy': True, u'name': u'strawberry'}])])

And to be more concise, it is failing on the grouper keyword in the _GroupTuple object. Based on this, it appears that we don't support ast.keyword as a SAFE_NODE

ast.keyword has 2 properties, arg and value, and in this case value is ast.Name which is allowed. I guess we just need deeper inspection here.

The root cause of this seems to be the change from _GroupTuple in jinja2 changing from a tuple to a namedtuple in 2.9.

@major
Contributor
major commented Jan 10, 2017

Thanks, @sivel! You saved me some extra digging. Should this be a bug in jinja2 instead?

@sivel
Member
sivel commented Jan 10, 2017

I'm not sure, I don't think so. Our safe_eval in ansible.templates.safe_eval tries to ensure that the data is "safe", and our implementation is flagging the new _GroupTuple as unsafe.

It looks like we get a repr of _GroupTuple in the expression, and when we parse it with ast we run down a rabbit hole, and fail.

What we probably need, is to cast _GroupTuple into an object that reprs as something else that is parsable by ast into a standard python data type, but I'm honestly not sure where to start.

@major
Contributor
major commented Jan 10, 2017

For what it's worth, here's the commit in jinja2 that caused the problem: pallets/jinja@6d356f1

@major
Contributor
major commented Jan 10, 2017 edited

Opened a bug with jinja for reference: pallets/jinja#654

@major
Contributor
major commented Jan 10, 2017

@sivel After some digging in jinja, it appears to be a collections.namedtuple, but AST has nothing for that. I wonder if we could convert the namedtuple to something else.

@sivel
Member
sivel commented Jan 10, 2017

I have a proof of concept fix, that overrides the groupby filter, replacing it with out own, that calls the jinja2 groupby and maps to tuple:

diff --git a/lib/ansible/plugins/filter/core.py b/lib/ansible/plugins/filter/core.py
index 70dfaac..d1c242f 100644
--- a/lib/ansible/plugins/filter/core.py
+++ b/lib/ansible/plugins/filter/core.py
@@ -36,7 +36,7 @@ from datetime import datetime
 import uuid
 
 import yaml
-from jinja2.filters import environmentfilter
+from jinja2.filters import environmentfilter, do_groupby as _do_groupby
 
 try:
     import passlib.hash
@@ -438,11 +438,17 @@ def skipped(*a, **kw):
     skipped = item.get('skipped', False)
     return skipped
 
+@environmentfilter
+def do_groupby(environment, value, attribute):
+    return map(tuple, _do_groupby(environment, value, attribute))
+
 class FilterModule(object):
     ''' Ansible core jinja2 filters '''
 
     def filters(self):
         return {
+            'groupby': do_groupby,
+
             # base 64
             'b64decode': partial(unicode_wrap, base64.b64decode),
             'b64encode': partial(unicode_wrap, base64.b64encode),

But there may be other ways to do this as well

@sigmavirus24

@sivel coercing to a tuple will absolutely be the fastest fix. It's potentially a lossy operation though.

@major
Contributor
major commented Jan 11, 2017 edited

@sigmavirus24 Thanks for the help! When you said "coercing", I thought of this for some reason:

Good good

@openstack-gerrit openstack-gerrit pushed a commit to openstack/openstack-ansible-security that referenced this issue Jan 11, 2017
@major major Remove groupby filter to avoid bug
This patch adjusts the package installation/removal tasks to get around
a bug. Jinja2 now returns namedtuples with the groupby filter and
Ansible is unable to convert that to a usable variable data type with
AST.

Jinja2 bug: pallets/jinja#654
Ansible bug: ansible/ansible#20098
Related-Bug: 1655397
Change-Id: I3ce764bfb643bda58c4b6ad282e71c312f41465e
ea8a0f0
@bcoca
Member
bcoca commented Jan 11, 2017

shot in the dark, but wouldn't something like this work?

diff --git a/lib/ansible/template/safe_eval.py b/lib/ansible/template/safe_eval.py
index 76df7ed..8384bbf 100644
--- a/lib/ansible/template/safe_eval.py
+++ b/lib/ansible/template/safe_eval.py
@@ -19,6 +19,7 @@ __metaclass__ = type

 import ast
 import sys
+import collections

 from ansible.compat.six import string_types
 from ansible.compat.six.moves import builtins
@@ -72,6 +73,7 @@ def safe_eval(expr, locals={}, include_exceptions=False):
             ast.USub,
             ast.Tuple,
             ast.UnaryOp,
+            collections.namedtuple
         )
     )
@sivel
Member
sivel commented Jan 11, 2017

@bcoca unfortunately not. At that point we are looping over an AST tree, so all of the items that we are inspecting are of ast types. We're effectively taking a python source, and ensuring that it is safe before evaling it.

ast doesn't seem to have support for interpreting namedtuples. And I also don't think we would be able to eval _GroupTuple anyway, since we don't have access to it's definition. Even if we could get it to pass through the inspection, we would also have to import _GroupTuple into save_eval.py and pass that in as part of the globals that we supply to eval.

@sigmavirus24

ast doesn't seem to have support for interpreting namedtuples

It doesn't need support. Creating a named tuple is essentially instantiating a class from its perspective. The name of the tuple, is bound like the name of a class so it sees a ast.Call and there's no way to do that except for finding _GroupTuple's definition, importing and providing it to eval() (as you already said).

In short, y'all might want to work with Jinja2 to ask them to define strong types (not ducked-types) for their return values. Judging by the name, I doubt _GroupTuple was ever meant to be used by anyout outside of Jinja2. I think y'all have a good case for asking them to convert that to a tuple for you, or provide a function that does.

@mitsuhiko

As Jinja2 maintainer here. The 2.9 update did break quite a few things for ansible but it was in fact the popularity of ansible that made me finally do this update. There were so many weird behaviors in Jinja2 that I wanted to clean it up for once so that the rules can be clearly defined and not be driven by unintended bugs.

So while this breaks stuff, from now on it should be much smoother sailing, in particular for Ansible. At least that's the intention on my part.

Which is why I'm curious to hear what sort of policy one would expect from the stability of Jinja2 for filters here.

In short, y'all might want to work with Jinja2 to ask them to define strong types (not ducked-types) for their return values.

What do you mean by ducked-typed ones? This type exists for a long time now in general, it just happend to be slightly different implementation now.

@bcoca
Member
bcoca commented Jan 12, 2017

@mitsuhiko first of all, thanks for Jinja, I've been a user for a long time (even before Ansible).

The issue here is our 'custom typing'. Jinja is designed to return strings, which is what a templating engine should do, but sometimes we want to preserve/have actual base types used internally in Ansible, which is why we created safe_eval.

We hit a snag after the update as safe_eval relies on Ast, which only provides base python types to compare against but not something like namedtuples. We are currently reviewing the function as to find a way to adapt to the change on our side.

@davidism
davidism commented Jan 12, 2017 edited

_GroupTuple isn't meant to be seen, since the expected use case is that groupby is used in a for loop. The issue seems to be that namedtuple has a different repr than the previous simple tuple subclass. Can you confirm that if the render looked like a regular tuple, this wouldn't be an issue? pallets/jinja#654 (comment)

@sigmavirus24

What do you mean by ducked-typed ones? This type exists for a long time now in general, it just happend [sic] to be slightly different implementation now.

So it seems whatever Ansible was calling was returning a tuple. It's now returning a namedtuple, which I prefer in general but which appears to be intended as a private class. So while a namedtuple is a tuple, it broke something for Ansible. I suspect @davidism is right that this is related to the repr.

@mitsuhiko

@sigmavirus24 it never returned a tuple for what it's worth.

@bcoca
Member
bcoca commented Jan 12, 2017

Jinja always retrurned text, this is a problem with Ansible doing some interjection and evaluating things internally and not meant for 'normal' consumption.

@openstack-gerrit openstack-gerrit added a commit to openstack/openstack that referenced this issue Jan 12, 2017
@major @openstack-gerrit major + openstack-gerrit Updated openstack/openstack
Project: openstack/requirements  f0fb896f4416fe100b18826444b6583a42fab46e

Avoid Jinja2 versions 2.9.[0-4]

There is a bug within ansible and/or jinja2 that causes an exception
when a string contains curly braces. A bug has already been opened
with the Ansible team to review the bug.

This patch pins the jinja2 release at a version less than 2.9. The
issue first appeared in the 2.9 release and is still present in
2.9.4 (latest available).

Jinja2 bug: pallets/jinja#654
Ansible bug: ansible/ansible#20098
Related-Bug: 1655397
Change-Id: I82561d3d738147683499697883d9d03ceb897f3d
b4488f2
@openstack-gerrit openstack-gerrit pushed a commit to openstack/requirements that referenced this issue Jan 12, 2017
@major major Avoid Jinja2 versions 2.9.[0-4]
There is a bug within ansible and/or jinja2 that causes an exception
when a string contains curly braces. A bug has already been opened
with the Ansible team to review the bug.

This patch pins the jinja2 release at a version less than 2.9. The
issue first appeared in the 2.9 release and is still present in
2.9.4 (latest available).

Jinja2 bug: pallets/jinja#654
Ansible bug: ansible/ansible#20098
Related-Bug: 1655397
Change-Id: I82561d3d738147683499697883d9d03ceb897f3d
f0fb896
@openstack-gerrit openstack-gerrit added a commit to openstack/openstack that referenced this issue Jan 12, 2017
@major @openstack-gerrit major + openstack-gerrit Updated openstack/openstack
Project: openstack/requirements  f0fb896f4416fe100b18826444b6583a42fab46e

Avoid Jinja2 versions 2.9.[0-4]

There is a bug within ansible and/or jinja2 that causes an exception
when a string contains curly braces. A bug has already been opened
with the Ansible team to review the bug.

This patch pins the jinja2 release at a version less than 2.9. The
issue first appeared in the 2.9 release and is still present in
2.9.4 (latest available).

Jinja2 bug: pallets/jinja#654
Ansible bug: ansible/ansible#20098
Related-Bug: 1655397
Change-Id: I82561d3d738147683499697883d9d03ceb897f3d
90264aa
@major
Contributor
major commented Jan 12, 2017

It appears that the new jinja release (2.9.5) restores the original repr.

I'm not sure if this bug needs to be closed or if it should remain open to resolve some of those AST questions from earlier.

@prometheanfire
Contributor

well, I think it should stay open since the latests rc from 2.1 and 2.2 capped the version of jinja to <2.9

That probably needs to be reverted

@abadger
Member
abadger commented Jan 16, 2017

The version limitations have been removed in the stable-2.1 and stable-2.2 branches. Hopefully we'll be able to work out the remaining problems before the next version of one of those branches is made.

@abadger abadger closed this Jan 16, 2017
@jimi-c
Member
jimi-c commented Jan 16, 2017

I'd still like to look into fixing this, as I'm reproducing it with the version of Jinja2 currently installed via pip. At least to remove problems for users in the short term.

@abadger abadger reopened this Jan 16, 2017
@abadger
Member
abadger commented Jan 16, 2017

Oops, I misread the jinja2-2.9.5 comments as meaning it was already released. I see now that 2.9.5 is what's in development upstream. I'm +1 for sivel's changes as they're not very intrusive. Especially as this is just needed to workaround the issue until the next jinja2 release.

@sivel
Member
sivel commented Jan 17, 2017

At the request of @jimi-c I have created a PR at #20362 to assist in aiding those with an impacted version of jinja2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment