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

Ansible 2.x errors on the task with the with_items loop with when: some_var is defined condition when it is used in a role, but not in a play #14383

Closed
romanrev opened this issue Feb 9, 2016 · 14 comments
Labels
bug This issue/PR relates to a bug.
Milestone

Comments

@romanrev
Copy link

romanrev commented Feb 9, 2016

Issue Type: Bug Report
Ansible Version:

$ ansible-playbook --version
ansible-playbook 2.1.0 (devel a584ab3420) last updated 2016/02/02 18:36:05 (GMT +1100)
  lib/ansible/modules/core: (detached HEAD a19fa6ba48) last updated 2015/04/25 10:47:42 (GMT +1100)
  lib/ansible/modules/extras: (detached HEAD df7fcc90d9) last updated 2015/04/25 10:47:42 (GMT +1100)
  config file =
  configured module search path = Default w/o overrides

Ansible Configuration: nothing changed
Environment: OS X 10.11.2
Summary:

Provided the some_var variable is not defined, Ansible 2.x produces an error on the task with the with_items loop with when: some_var is defined condition when it is used in a role, however it does not produce an error when that task is used in a play.

Given the input and command to run:

cat > undefined_var_test_role_inclusion.yml <<EOF

---
- hosts: all
  connection: local
  gather_facts: false

  roles:

  - { role: some_role,
      some_var: "{{ some_value }}",
      when: some_value is defined,
      tags: [ tag1, tag2 ]
    }
EOF
mkdir -p roles/some_role/tasks
cat > roles/some_role/tasks/main.yml <<EOF

---
- name: reference the some_var variable
  local_action: >
    shell
    echo {{ some_var }}

- name: reference the some_var variable in a loop
  local_action: >
    shell
    echo {{ item }}
  with_items: some_var

- debug: var=some_result
EOF

With ansible-playbook from the develop branch I get this:

No config file found; using defaults
1 plays in ./undefined_var_test_role_inclusion.yml

PLAY ***************************************************************************

TASK [some_role : reference the some_var variable] *****************************
task path: /Users/roman/Dev/ansible-bugs/roles/some_role/tasks/main.yml:2
skipping: [localhost] => {"changed": false, "skip_reason": "Conditional check failed", "skipped": true}
[DEPRECATION WARNING]: Using bare variables is deprecated. Update your playbooks
 so that the environment value uses the full variable syntax ('{{some_var}}').
This feature will be removed in a future release. Deprecation warnings can be
disabled by setting deprecation_warnings=False in ansible.cfg.
ERROR! 'some_value' is undefined

Expected output and behaviour (with Ansible 1.9.4):

$ ansible-playbook -i localhost, ./undefined_var_test_role_inclusion.yml -vvv

PLAY [all] ********************************************************************

TASK: [some_role | reference the some_var variable] ***************************
skipping: [localhost]

TASK: [some_role | reference the some_var variable in a loop] *****************
skipping: [localhost] => (item=some_var)

TASK: [some_role | debug var=some_result] *************************************
skipping: [localhost]

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

If, however the task

- name: reference the some_var variable in a loop
  local_action: >
    shell
    echo {{ item }}
  with_items: some_var
  when: some_var is defined

is used in the tasks list in play instead of inside roles, Ansible 2.x correctly ignores it.

Just to reiterate and make it as possibly clear as I can:

My main point is that Ansible 2.x does not error when the above mentioned task is being used in a play, but does error when the task is used in a role. It should either:

  • emit an error ​_all the time_​ regardless of where the task is, and then one can use bold statements such as “when using with_items loop please make sure to define a variable that is used in the loop, regardless of the when: condition or it will error!”, or
  • not emit an error in case the when: some_var is defined is supplied in a task with a with_items: some_var loop - regardless of whether the task is in a role or a play

Please note: this issue has been initially reported in https://github.com/ansible/ansible/issues/14201but since the latter one has been closed for no reason, I am opening a new issue

@jimi-c jimi-c added this to the stable-2.0 milestone Feb 9, 2016
evrardjp added a commit to evrardjp/ansible-haproxy that referenced this issue Feb 11, 2016
Issues were listed here:
ansible/ansible#14383
and here:
ansible/ansible#13791

But at the end, it's normal ansible behaviour and ansible 1.9
was too kind with that.

This should avoid all the problems.
@Wtower
Copy link

Wtower commented Feb 12, 2016

I believe I have reproduced this with Ansible 2.1 in Ubuntu Trusty.

@sivel
Copy link
Member

sivel commented Feb 12, 2016

This should no longer be the case as of 347b282

An undefined variable should now cause the task to skip instead of causing an error.

@dkasak
Copy link
Contributor

dkasak commented Feb 28, 2016

So just to confirm, there is no undeprecated way of conditionally depending on a role? I realize that with_items does something akin to expansion of a task into multiple instances for each of the items specified so it fails if one of them is not defined, but this is an implementation detail from my point of view and hurts composability.

The use case we're aiming for is having a set of N roles, each of which is providing the same feature semantically but through a different mechanism. We're switching between those N roles based on a variable in the top-level playbook so that only one of those roles is run. However, this fails because some of those N roles have unique variables which are left undefined if we don't want to depend on that particular role. The only way I see this could be done with the current implementation is if we defined all the variables in each of those roles even if we're going to skip the entire role for that particular run and that seems awful.

Is there a reason why the when clause is not checked before the when_items expansion?

EDIT: I guess the reason is that this way {{ item }} can appear inside the when. It would be nice to have a stronger when which operated on the task in its unexpanded form.

@jimi-c
Copy link
Member

jimi-c commented Apr 3, 2016

Closing, as this should be resolved by the above commit.

If you continue seeing any problems related to this issue, or if you have any further questions, please let us know by stopping by one of the two mailing lists, as appropriate:

Because this project is very active, we're unlikely to see comments made on closed tickets, but the mailing list is a great way to ask questions, or post if you don't think this particular issue is resolved.

Thank you!

@jimi-c jimi-c closed this as completed Apr 3, 2016
@yfried
Copy link

yfried commented Apr 3, 2016

Please reopen. I've recreated it

[DEPRECATION WARNING]: Skipping task due to undefined Error, in the future this will be a fatal error.: 'dict object' has no attribute 'flavors'.
This feature will be removed in a future release. Deprecation warnings can be disabled by 
setting deprecation_warnings=False in ansible.cfg.
fatal: [localhost]: FAILED! => {"failed": true, "msg": "with_dict expects a dict"}



ansible 2.1.0 (devel 2dd687acdd) last updated 2016/04/03 17:07:47 (GMT +300)
  lib/ansible/modules/core: (detached HEAD a38bdfc720) last updated 2016/04/03 17:07:51 (GMT +300)
  lib/ansible/modules/extras: (detached HEAD 7c3999a92a) last updated 2016/04/03 17:07:51 (GMT +300)
  config file = /home/yfried/workspace/git/InfraRed/ansible.cfg
  configured module search path = ['library', '/home/yfried/.virtualenvs/InfraRed/lib/python2.7/site-packages/ansible/modules']

@romanrev
Copy link
Author

romanrev commented Apr 3, 2016

@yfried-redhat Could you open the topic on the https://groups.google.com/forum/#!forum/ansible-project - my experience with the Ansible github issues is that once they close it, the issue becomes dormant. Thank you!

@aaomoware
Copy link

Undefined variable(s) causes ansible to throw "Fatal". Perhaps the fix for this bug is yet to be released officially as the issue still persists.

I am running ansible 2.0.1.0, in Ubuntu Trusty.

TASK [sensu : custom configs] **************************************************
[DEPRECATION WARNING]: Skipping task due to undefined Error, in the future this will be a fatal error.. This feature will be removed in a future release. Deprecation warnings can be disabled by setting
deprecation_warnings=False in ansible.cfg.
[DEPRECATION WARNING]: Skipping task due to undefined Error, in the future this will be a fatal error.. This feature will be removed in a future release. Deprecation warnings can be disabled by setting
deprecation_warnings=False in ansible.cfg.
fatal: [monitor-1]: FAILED! => {"failed": true, "msg": "with_dict expects a dict"}
fatal: [monitor-2]: FAILED! => {"failed": true, "msg": "with_dict expects a dict"}

@sboulkour
Copy link

I too am facing this issue, while moving project from 1.9.x to 2.0.x.
Everything works fine with Ansible 1.9 but I can't make it work with ansible 2.

Tasks in role :

- name: Enable administrators SSH Keys for user ansible
  authorized_key: user=ansible key="{{ lookup('file', item )}}"
  with_fileglob:
    - "{{ ssh_keys_root }}/admins/enabled/*"
  become: yes

- name: Enable team SSH keys for user ansible
  authorized_key: user=ansible key="{{ lookup('file', item) }}"
  when: team_name is defined
  with_fileglob:
    - "{{ ssh_keys_root }}/{{ team_name }}/enabled/*"

The output (with some hidden path & filenames)

TASK [ssh-keys : Enable administrators SSH Keys for user ansible] **************
ok: [xxx.xxx] => (item=/Volumes/SecuredDisk/aaa/aaa/aaa.pub)
ok: [xxx.xxx] => (item=/Volumes/SecuredDisk/bbb/bbb/bbb.pub)
ok: [yyy.yyy] => (item=/Volumes/SecuredDisk/aaa/aaa/aaa.pub)
ok: [yyy.yyy] => (item=/Volumes/SecuredDisk/bbb/bbb/bbb.pub)

TASK [ssh-keys : Enable team SSH keys for user ansible] ************************
[DEPRECATION WARNING]: Skipping task due to undefined Error, in the future this will be a fatal error.. This feature will be removed in a future release. Deprecation warnings can be disabled by setting deprecation_warnings=False in ansible.cfg.
[DEPRECATION WARNING]: Skipping task due to undefined Error, in the future this will be a fatal error.. This feature will be removed in a future release. Deprecation warnings can be disabled by setting deprecation_warnings=False in ansible.cfg.

@aaomoware
Copy link

@sboulkour
Check undefine/unsagined variables. Make sure any variables you are referencing are a properly defined/assigned. That was the issue in my case.

The way I understand the new improvement in 2.0 is: undefined/unassigned variable should be ignored. But apparently that is not happening.

@sboulkour
Copy link

@aaomoware
Yeah thks, issue were with team_name that wasn't defined for this team :)

But still, the error message doesn't make sense to me.

@aaomoware
Copy link

@sboulkour
I agree with you. The exception don't make sense as it misleads one to think it is a deprecated method error; it is not. It is the with_* just moaning because of an undefined variable. Hopefully it gets fixed soon

@jimi-c
Copy link
Member

jimi-c commented May 12, 2016

We're keeping it as a warning for now. In the future, this will be a play-ending error.

@waffledonkey
Copy link

It's probably not good form, but this seems to avoid the exception; YMMV

with_items: "{{undefined_var| default({}) }}"

it results in what looks like a skipped step... the -name, if present, will outputs but no ok, changed, skipping, etc.

If you want the result of the step, you can do this:

with_items: "{{undefined_var| default(omit) }}"

which results in:

skipping: [control] => (item=__omit_place_holder__8a599a567482c5400c4ff70ff4849759780c47e1)

@djowett
Copy link

djowett commented Aug 25, 2016

This was awkward for me, my eventual workaround used a local_action: shell cat instead of the with_fileglob, like this:

- name: Read SSH Keys
  local_action:
    shell cat {{ odoo_user_sshkeys }}
  register: all_ssh_keys

- name: Set SSH public keys for the Odoo user
  authorized_key: user={{ odoo_user }}
                  key="{{ all_ssh_keys.stdout }}"
  when: all_ssh_keys.stdout

maybe this helps @sboulkour and others?

c-mart pushed a commit to CyVerse-Ansible/ansible-tls-cert that referenced this issue Jan 13, 2017
…-tls-cert into cdosborn-tls-cert-refactor

09:40]
connor Tacked on a few more changes to that pr, when you get the chance could you try them? I'm running both versions of the deploy/create self-signed w/o deprecation warnings

[09:41]
cmart sure i'm between things, i'll look now

[09:42]
connor doh

[09:42]
i just* pushed them

[09:46]
connor woops, i thought i had more changes, but i don't atm. With those changes i'm not getting any deprecation warnings (edited)

[09:51]
cmart really. which version of Ansible are you running?

[09:51]
connor 2.1.0.0

[09:52]
cmart i'm on 2.1.1.0

[09:52]
connor let me try that

[09:53]
cmart and i'm running a very bare playbook; the second example playbook in the readme

[09:53]
connor Okay i just got a dep warn

[10:10]
connor So it looks like this isn't something they are going to "fix"

[10:11]
 ```- debug:
    msg: "awasdf {{ asdfasdf }}"
  when: asdfasdf is defined
```

[10:11]
^ valid

[10:12]
so it really only impacts the "with_items". If I understand correctly its because the when clause can use {{ item }}, so the items must be evaluated prior to evaluating the when clause

[10:13]
cmart apparently, at least under the constraints of how they have designed the project

[10:14]
connor this decision makes using with_items much less useful

[10:15]
cmart it basically forces Ansible users to use inelegant workarounds

[10:15]
connor every var in the with items needs a default, since it could be skipped anywhere

[10:16]
i think for the convenience it adds, its not worth it, if you have variables where the types change, it becomes harder to write correct code

[10:17]
for example if someone includes half of those default variables, you'll be dealing with half which are paths and half which are booleans

[10:17]
cmart why is that an issue, if you're testing whether the variable is equal to false?

[10:18]
connor anywhere you see the variable, you have to think about whether its potentially a bool or a path

[10:19]
potentially you write paths with false in the file name

[10:20]
cmart or you could just be disciplined about only using variables whose type may mutate, inside conditionally included code which first ensures that the variables are of the expected type?

[10:20]
i agree that in theory, it becomes harder to write correct code. but i don't see how it's much harder, in this case

[10:21]
connor i think if we decide at large to work around this ansible issue, by using false, it will introduce more errors is all

[10:21]
cmart maybe you're right

[10:22]
so the alternative i see is to use the `| default('')` filter on every variable inside a `with_items` loop

[10:24]
but perhaps that also makes it harder to write correct code in the future, because we may have a situation where the variable *should* be defined, but isn't, and the default filter suppresses the error

[10:24]
connor yeah its as bad

[10:24]
cmart and it also makes the code more cluttered and less readable

[10:25]
so i picked what I saw as the least bad option. if you feel strongly otherwise, then i'll accept a PR that does it the other way :)

[10:49]
connor One solution would be to ensure that all variables have defaults (in defaults.yml), we cannot do that here because there are no sensible defaults for the src cert/ src key when creating a self-signed. The larger reason that there are no sensible defaults is because the role is doing too much, its really doing one of two jobs. When it does one job the other vars don't make sense and vice versa.

I don't want to belabor this (just trying to find a good soln). I think the options are keep it the way it is, throw in useless jinja defaults into the with_items, or abandon with_items and just use three copy tasks. I like the last one.

The perfectionist in me would break up the role into create self signed and install certs (i'm assuming that would work).

[10:50]
not sure tho, like if you skip a role with a when does it go and evaluate everything in that role 😆 (edited)

[10:50]
cmart i actually just read about that

[10:52]
ansible/ansible#14383 (comment)

[10:52]
> The use case we're aiming for is having a set of N roles, each of which is providing the same feature semantically but through a different mechanism. We're switching between those N roles based on a variable in the top-level playbook so that only one of those roles is run. However, this fails because some of those N roles have unique variables which are left undefined if we don't want to depend on that particular role. The only way I see this could be done with the current implementation is if we defined all the variables in each of those roles even if we're going to skip the entire role for that particular run and that seems awful.

[10:53]
connor lol

[10:56]
cmart we could abandon with_items and use three copy tasks. but that is again a very local solution, not a global one -- that doesn't make sense when we'd need to expand one loop into 10 tasks, or N tasks where N is not known until execution time (edited)

[10:59]
connor wonder if this is true ansible/ansible#14383 (comment)

[11:01]
okay so that commit linked in the comment was part of a feature for the current behavior, where a deprecation warning is thrown

[11:01]
cmart another solution -- we could just ignore the deprecation warnings

[11:02]
but that produces ugly Ansible output

[11:02]
connor I like that, I think they'll have to produce a fix

[11:13]
connor i pushed an addl commit to address when 'TLS_CACHAIN_SRC_FILE' is not defined
c-mart pushed a commit to CyVerse-Ansible/ansible-tls-cert that referenced this issue Jan 13, 2017
We decided that even with deprecation warnings, conditionally defining the variables was our least bad option. See instant message conversation below.

[10:10]
connor So it looks like this isn't something they are going to "fix"

[10:11]
 ```- debug:
    msg: "awasdf {{ asdfasdf }}"
  when: asdfasdf is defined
```

[10:11]
^ valid

[10:12]
so it really only impacts the "with_items". If I understand correctly its because the when clause can use {{ item }}, so the items must be evaluated prior to evaluating the when clause

[10:13]
cmart apparently, at least under the constraints of how they have designed the project

[10:14]
connor this decision makes using with_items much less useful

[10:15]
cmart it basically forces Ansible users to use inelegant workarounds

[10:15]
connor every var in the with items needs a default, since it could be skipped anywhere

[10:16]
i think for the convenience it adds, its not worth it, if you have variables where the types change, it becomes harder to write correct code

[10:17]
for example if someone includes half of those default variables, you'll be dealing with half which are paths and half which are booleans

[10:17]
cmart why is that an issue, if you're testing whether the variable is equal to false?

[10:18]
connor anywhere you see the variable, you have to think about whether its potentially a bool or a path

[10:19]
potentially you write paths with false in the file name

[10:20]
cmart or you could just be disciplined about only using variables whose type may mutate, inside conditionally included code which first ensures that the variables are of the expected type?

[10:20]
i agree that in theory, it becomes harder to write correct code. but i don't see how it's much harder, in this case

[10:21]
connor i think if we decide at large to work around this ansible issue, by using false, it will introduce more errors is all

[10:21]
cmart maybe you're right

[10:22]
so the alternative i see is to use the `| default('')` filter on every variable inside a `with_items` loop

[10:24]
but perhaps that also makes it harder to write correct code in the future, because we may have a situation where the variable *should* be defined, but isn't, and the default filter suppresses the error

[10:24]
connor yeah its as bad

[10:24]
cmart and it also makes the code more cluttered and less readable

[10:25]
so i picked what I saw as the least bad option. if you feel strongly otherwise, then i'll accept a PR that does it the other way :)

[10:49]
connor One solution would be to ensure that all variables have defaults (in defaults.yml), we cannot do that here because there are no sensible defaults for the src cert/ src key when creating a self-signed. The larger reason that there are no sensible defaults is because the role is doing too much, its really doing one of two jobs. When it does one job the other vars don't make sense and vice versa.

I don't want to belabor this (just trying to find a good soln). I think the options are keep it the way it is, throw in useless jinja defaults into the with_items, or abandon with_items and just use three copy tasks. I like the last one.

The perfectionist in me would break up the role into create self signed and install certs (i'm assuming that would work).

[10:50]
not sure tho, like if you skip a role with a when does it go and evaluate everything in that role 😆 (edited)

[10:50]
cmart i actually just read about that

[10:52]
ansible/ansible#14383 (comment)

[10:52]
> The use case we're aiming for is having a set of N roles, each of which is providing the same feature semantically but through a different mechanism. We're switching between those N roles based on a variable in the top-level playbook so that only one of those roles is run. However, this fails because some of those N roles have unique variables which are left undefined if we don't want to depend on that particular role. The only way I see this could be done with the current implementation is if we defined all the variables in each of those roles even if we're going to skip the entire role for that particular run and that seems awful.

[10:53]
connor lol

[10:56]
cmart we could abandon with_items and use three copy tasks. but that is again a very local solution, not a global one -- that doesn't make sense when we'd need to expand one loop into 10 tasks, or N tasks where N is not known until execution time (edited)

[10:59]
connor wonder if this is true ansible/ansible#14383 (comment)

[11:01]
okay so that commit linked in the comment was part of a feature for the current behavior, where a deprecation warning is thrown

[11:01]
cmart another solution -- we could just ignore the deprecation warnings

[11:02]
but that produces ugly Ansible output

[11:02]
connor I like that, I think they'll have to produce a fix

[11:06]
being unable to conditionally include a role/task (without first evaluating the jinja) seems so broken

[11:07]
on the other hand the core devs seem pretty committed (from an implementation pov)

[11:08]
cmart i don't know. maybe more people just need to bitch about it. i'm sure that would happen if they actually do what the deprecation warning threatens.

it definitely discourages polymorphism -- having one role that can do the same job several different ways, depending on the deployer's preference.

[11:09]
which is what we're trying to accomplish here. I don't think tls-cert is doing two different jobs, just offering two paths to the same result (a TLS certificate on the target)

[11:13]
connor i pushed an addl commit to address when 'TLS_CACHAIN_SRC_FILE' is not defined
@ansibot ansibot added bug This issue/PR relates to a bug. and removed bug_report labels Mar 7, 2018
@ansible ansible locked and limited conversation to collaborators Apr 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue/PR relates to a bug.
Projects
None yet
Development

No branches or pull requests