Fix group variable precedence in #6734

[brian-brazil]

Group variables defined in inventory should override
entries in the group_vars directories.

Add regression test.

[abadger]

Example behaviour from @brian-brazil 's comment on the other ticket:

I've discovered a regression in this change via git bisect, group_vars/all now overrides inventory file variables.

With an inventory of:

localhost

[all:vars]
a=bar

A site.yml of

---
- name: play
  hosts: all
  gather_facts: false
  tasks:
    - name: task
      debug: var=a

and a group_vars/all of

---
a: foo

make && ansible-playbook -i inv site.yml now has foo instead of bar.

[abadger]

Example behaviour from @brian-brazil 's comment on the other ticket:

I've discovered a regression in this change via git bisect, group_vars/all now overrides inventory file variables.

With an inventory of:

localhost

[all:vars]
a=bar

A site.yml of

---
- name: play
  hosts: all
  gather_facts: false
  tasks:
    - name: task
      debug: var=a

and a group_vars/all of

---
a: foo

make && ansible-playbook -i inv site.yml now has foo instead of bar.

[maxim]

I suggest to bump priority to P1 since this is not only breaking things, but also a security issue, where secret variables (not begin overridden) could end up on servers where they don't belong.

[maxim]

I suggest to bump priority to P1 since this is not only breaking things, but also a security issue, where secret variables (not begin overridden) could end up on servers where they don't belong.

[bcoca]

actually we want group/host_vars to override inventory (and play group/host vars overriding inventory group/host vars).

[bcoca]

actually we want group/host_vars to override inventory (and play group/host vars overriding inventory group/host vars).

[brian-brazil]

Can you explain why that is?

I have settings that are specific to a particular inventory file rather than any single group and I don't see a way around this regression for groups.

[brian-brazil]

Can you explain why that is?

I have settings that are specific to a particular inventory file rather than any single group and I don't see a way around this regression for groups.

[bcoca]

I'm saying that this seems to be the intended way for this to work, the change looks to have been done to 'correct' the precedence to what it is now.

[bcoca]

I'm saying that this seems to be the intended way for this to work, the change looks to have been done to 'correct' the precedence to what it is now.

[brian-brazil]

Hmm, my reading of the description of #6734 was that it was a refactor that shouldn't have affected behaviour - just performance.

@srvg Can you comment?

[brian-brazil]

Hmm, my reading of the description of #6734 was that it was a refactor that shouldn't have affected behaviour - just performance.

@srvg Can you comment?

[srgvg]

Sorry, seems I missed my notification from this ticket.
AFAICR, I did not work explicitly on this precedence behaviour, in #6734

I'm not even sure what the right answer should be, as I normally never use variables in the ini file, and the ansible docs are not clear on precedence within inventory. I don't even think that was ever a well defined thing.

Either way, I can confirm this: if my refactoring changed this, it was not intended.

[srgvg]

Sorry, seems I missed my notification from this ticket.
AFAICR, I did not work explicitly on this precedence behaviour, in #6734

I'm not even sure what the right answer should be, as I normally never use variables in the ini file, and the ansible docs are not clear on precedence within inventory. I don't even think that was ever a well defined thing.

Either way, I can confirm this: if my refactoring changed this, it was not intended.

[brian-brazil]

@srvg Thanks for the clarification.

@bcoca Given this behaviour was unintended, can this change be merged to fix the regression?

[brian-brazil]

@srvg Thanks for the clarification.

@bcoca Given this behaviour was unintended, can this change be merged to fix the regression?

[srgvg]

If I'm not mistaken, I suspect this patch will also change the precedence between vars from a dynamic inventory script, and (group/host_vars and vars_plugins).

Perhaps we first should have a broader discussion on how precedence within the inventory should happen.

[srgvg]

If I'm not mistaken, I suspect this patch will also change the precedence between vars from a dynamic inventory script, and (group/host_vars and vars_plugins).

Perhaps we first should have a broader discussion on how precedence within the inventory should happen.

[amenonsen]

In reviewing this change, I thought playbooks_variables.rst now clearly documented whether group_vars overrides group variables defined in the inventory, but somewhat to my surprise it does not. I can live with either behaviour (with a slight preference for status quo, i.e. things in the inventory are overriden by group_vars), but I really think it should be documented first, and then this PR accepted or rejected once and for all. It's a legitimate source of confusion otherwise.

[amenonsen]

In reviewing this change, I thought playbooks_variables.rst now clearly documented whether group_vars overrides group variables defined in the inventory, but somewhat to my surprise it does not. I can live with either behaviour (with a slight preference for status quo, i.e. things in the inventory are overriden by group_vars), but I really think it should be documented first, and then this PR accepted or rejected once and for all. It's a legitimate source of confusion otherwise.

[jimi-c]

Looking back at this issue, it appears we have corrected it in devel (though it is still broken in stable-2.1, so it appears to be a recent fix).

# ansible-playbook -vv -i inv test.yml 
Using /etc/ansible/ansible.cfg as config file

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

PLAY [play] ********************************************************************

TASK [task] ********************************************************************
task path: /root/testing/9620/test.yml:6
ok: [localhost] => {
    "a": "bar"
}

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

As such, we're going to go ahead and close this now (and I'll ensure the fix ends up in stable-2.1 before the next release as well).

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:

• https://groups.google.com/forum/#!forum/ansible-project - for user questions, tips, and tricks
• https://groups.google.com/forum/#!forum/ansible-devel - for strategy, future planning, and questions about writing code

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]

Looking back at this issue, it appears we have corrected it in devel (though it is still broken in stable-2.1, so it appears to be a recent fix).

# ansible-playbook -vv -i inv test.yml 
Using /etc/ansible/ansible.cfg as config file

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

PLAY [play] ********************************************************************

TASK [task] ********************************************************************
task path: /root/testing/9620/test.yml:6
ok: [localhost] => {
    "a": "bar"
}

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

As such, we're going to go ahead and close this now (and I'll ensure the fix ends up in stable-2.1 before the next release as well).

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:

• https://groups.google.com/forum/#!forum/ansible-project - for user questions, tips, and tricks
• https://groups.google.com/forum/#!forum/ansible-devel - for strategy, future planning, and questions about writing code

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]

Cherry picked fixes to stable-2.1 as well:

# ansible --version
ansible 2.1.0.0 (stable-2.1 9936d7355c) last updated 2016/06/06 14:57:59 (GMT -500)
  lib/ansible/modules/core: (detached HEAD bc0bc7a027) last updated 2016/06/06 14:47:41 (GMT -500)
  lib/ansible/modules/extras: (detached HEAD 891928b8d2) last updated 2016/06/06 14:47:41 (GMT -500)
  config file = /etc/ansible/ansible.cfg
  configured module search path = ['/usr/share/ansible']

# ansible-playbook -vv -i inv test.yml 
Using /etc/ansible/ansible.cfg as config file

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

PLAY [play] ********************************************************************

TASK [task] ********************************************************************
task path: /root/testing/9620/test.yml:6
ok: [localhost] => {
    "a": "bar"
}

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

[jimi-c]

Cherry picked fixes to stable-2.1 as well:

# ansible --version
ansible 2.1.0.0 (stable-2.1 9936d7355c) last updated 2016/06/06 14:57:59 (GMT -500)
  lib/ansible/modules/core: (detached HEAD bc0bc7a027) last updated 2016/06/06 14:47:41 (GMT -500)
  lib/ansible/modules/extras: (detached HEAD 891928b8d2) last updated 2016/06/06 14:47:41 (GMT -500)
  config file = /etc/ansible/ansible.cfg
  configured module search path = ['/usr/share/ansible']

# ansible-playbook -vv -i inv test.yml 
Using /etc/ansible/ansible.cfg as config file

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

PLAY [play] ********************************************************************

TASK [task] ********************************************************************
task path: /root/testing/9620/test.yml:6
ok: [localhost] => {
    "a": "bar"
}

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