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

Add warning if playbook references a var that shadows a builtin method #22273

Closed
wants to merge 11 commits into from
Closed

Add warning if playbook references a var that shadows a builtin method #22273

wants to merge 11 commits into from

Conversation

alikins
Copy link
Contributor

@alikins alikins commented Mar 3, 2017

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/template/init.py

ANSIBLE VERSION
ansible 2.3.0 (warn_on_invalid_var_getattr 3ecac85b92) last updated 2017/03/03 16:27:40 (GMT -400)
  config file = /home/adrian/.ansible.cfg
  configured module search path = Default w/o overrides

SUMMARY

There is a silent gotcha lurking in http://docs.ansible.com/ansible/playbooks_variables.html#what-makes-a-valid-variable-name

If playbook yaml data structures contain fields with any ~80 var names listed there, and the playbook references them via dot/getattr access ( "{{ my_data.my_field }}"), instead of returning the data item set in the data structure, a reference to the python builtin methods of the same names will be returned.

There is a way to avoid that (using {{ my_data['my_field'] }} to prefer the getitem()), but it is very easy to accidentally use a shadowed builtin and the getattr/dot access method and end up with a the method reference which caused very confusing error messages and other weird failures.
ie, if my_data:

my_data:
   get: http://www.ansible.com
   update: http://www.ansible.com/releases
   blippy: 37

If playbook uses '{{ my_data.get }}', the result will be something like '<built-in method get of dict object at 0x7fdef81cc050>' instead of 'http://www.ansible.com'

This pr adds a warning message for that case. It also changes the behavior to prefer the data item ('http://www.ansible.com') for cases where data items shadow built in methods. A reference to
'{{ my_data.popitem }}' will still return the method reference (popitem is not shadowed).

The patch detects if a getattr is used on the object could also reference a data item. If so, it warns and returns the data item.

@ansibot ansibot added affects_2.3 This issue/PR affects Ansible v2.3 bugfix_pull_request needs_triage Needs a first human triage before being processed. labels Mar 3, 2017
@alikins alikins changed the title Warn on invalid var getattr Add warning if playbook references a var that shadows a builtin method Mar 3, 2017
@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Mar 3, 2017
@mattclay
Copy link
Member

mattclay commented Mar 3, 2017

CI failure in integration tests due to:

2017-03-03 21:45:45 TASK [template_getattr : Try to access and call valid_list.confused_for_list.popitem (builtin method) and expect the method to be accessed and called without a warning] ***
2017-03-03 21:45:45 task path: /root/ansible/test/integration/targets/template_getattr/tasks/main.yml:164
2017-03-03 21:45:45 fatal: [testhost]: FAILED! => {
2017-03-03 21:45:45     "assertion": "valid_test.confused_for_a_list.popitem()[0] == \"pop\"",
2017-03-03 21:45:45     "changed": false,
2017-03-03 21:45:45     "evaluated_to": false,
2017-03-03 21:45:45     "failed": true
2017-03-03 21:45:45 }

Also, the test ansible-test sanity --test yamllint failed with the following errors:

test/integration/targets/template_getattr/tasks/main.yml:145:9: too many spaces after hyphen (hyphens)
test/integration/targets/template_getattr/tasks/main.yml:179:1: too many blank lines (1 > 0) (empty-lines)

@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Mar 3, 2017
@ansibot ansibot removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 3, 2017
@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Mar 6, 2017
@ansibot ansibot added affects_2.4 This issue/PR affects Ansible v2.4 needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 23, 2017
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Apr 11, 2017
@ansibot ansibot removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels May 3, 2017
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jun 8, 2017
@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jun 24, 2017
@abadger
Copy link
Contributor

abadger commented Aug 17, 2017

The warning seems like a good idea as the original error is a bit cryptic but changing the behaviour has the potential to break things. here's a bit of a contrived example:

---
- hosts: localhost
  gather_facts: False
  vars:
    test:
      update: data
    test2:
      other: 1
  tasks:
  - debug:
      msg: '{{ test.update(test2) }}{{test}}'

Ah, here's a more useful example:

---
- hosts: localhost
  gather_facts: False
  vars:
    test:
      keys: data
    test2:
      other: 1
  tasks:
  - debug:
      msg: '{{ test.keys() }}'

@alikins
Copy link
Contributor Author

alikins commented Aug 17, 2017

reference a objects method attrs result in getting the repr of the method in devel/.
Even if there is also a data item with the same name (ie, 'update')

[newswoop:F25:ansible (devel % u=)]$ cat test_whatever2.yml 
---
- hosts: localhost
  gather_facts: False
  vars:
    test:
      update: data
    test2:
      other: 1
  tasks:
  - debug:
      msg: "test.update: {{ test.update }}"
  - debug:
      msg: "test2.update: {{test2.update}}"
[newswoop:F25:ansible (devel % u=)]$ ansible-playbook -vvv test_whatever2.yml 
ansible-playbook 2.4.0 (devel 25a9ababcc) last updated 2017/08/17 15:20:13 (GMT -400)
  config file = /home/adrian/src/ansible/ansible.cfg
  configured module search path = [u'/home/adrian/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/adrian/src/ansible/lib/ansible
  executable location = /home/adrian/src/ansible/bin/ansible-playbook
  python version = 2.7.13 (default, May 10 2017, 20:04:28) [GCC 6.3.1 20161221 (Red Hat 6.3.1-1)]
Using /home/adrian/src/ansible/ansible.cfg as config file
 [WARNING]: No inventory was parsed, only implicit localhost is available

 [WARNING]: Could not match supplied host pattern, ignoring: all

 [WARNING]: provided hosts list is empty, only localhost is available


PLAYBOOK: test_whatever2.yml *********************************************************************
1 plays in test_whatever2.yml

PLAY [localhost] *********************************************************************************
META: ran handlers

TASK [debug] *************************************************************************************
task path: /home/adrian/src/ansible/test_whatever2.yml:10
ok: [localhost] => {
    "msg": "test.update: <built-in method update of dict object at 0x7f5df6250050>"
}

TASK [debug] *************************************************************************************
task path: /home/adrian/src/ansible/test_whatever2.yml:12
ok: [localhost] => {
    "msg": "test2.update: <built-in method update of dict object at 0x7f5df6255280>"
}
META: ran handlers
META: ran handlers

PLAY RECAP ***************************************************************************************
localhost                  : ok=2    changed=0    unreachable=0    failed=0   

@abadger
Copy link
Contributor

abadger commented Aug 17, 2017

reference a objects method attrs result in getting the repr of the method in devel/.
Even if there is also a data item with the same name (ie, 'update')

No, it results in getting the method. you see the repr when you attempt to print the method. but most of the time you want to use the method, not print it. We can't tell in the getattr how the method is going to be used. My example of '{{ mydict.keys() }}' is a perfectly valid thing to want to do and with your patch that would be broken.

bcoca also raises the question of performance. We template quite a lot and the templating is already slow. Intercepting getattr would slow that further. We'd want to benchmark the overhead of adding the warning when there's many variables before we push just the warning code in.

Display a warning if a template references a attribute name
that is a object method and also an item in the data.

In this ambiquous scenario, dislay the warning, then return the
object method and not the data item.
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 17, 2017
@mattclay
Copy link
Member

bot_status

@ansibot
Copy link
Contributor

ansibot commented Aug 18, 2017

The test ansible-test sanity --test yamllint failed with the following error:

test/integration/targets/template_getattr/tasks/main.yml:176:1: too many blank lines (1 > 0) (empty-lines)

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Aug 18, 2017

waiting_on: alikins
component: None
supported_by: core
changes_requested_by: null
needs_info: False
needs_revision: True
needs_rebase: False
merge_commits: []
mergeable_state: unstable
shippable_status: failure
maintainer_shipits: False
community_shipits: False
ansible_shipits: False
shipit_actors: None

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Aug 18, 2017
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 26, 2017
@ansibot ansibot added the test This PR relates to tests. label Sep 3, 2017
@ansibot ansibot added the support:community This issue/PR relates to code supported by the Ansible community. label Dec 5, 2017
@ansibot ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 2, 2018
@abadger abadger removed the python3 label Mar 20, 2018
@dagwieers dagwieers added test This PR relates to tests. and removed test This PR relates to tests. test_pull_requests labels Jun 22, 2018
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Mar 30, 2019
@bcoca
Copy link
Member

bcoca commented Jan 10, 2020

closing this as we documented the behaviour and the cost of detection becomes prohibitive at runtime.

@bcoca bcoca closed this Jan 10, 2020
@ansible ansible locked and limited conversation to collaborators Feb 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.3 This issue/PR affects Ansible v2.3 affects_2.4 This issue/PR affects Ansible v2.4 bug This issue/PR relates to a bug. ci_verified Changes made in this PR are causing tests to fail. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants