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

fix: workaround ansible inconsistent var handling #431

Merged
merged 5 commits into from
Feb 21, 2024
Merged

fix: workaround ansible inconsistent var handling #431

merged 5 commits into from
Feb 21, 2024

Conversation

mhitza
Copy link
Contributor

@mhitza mhitza commented Feb 15, 2024

Context: if a variable is defined within a set_fact module call, and the variable value is used within the new varibles defined via set_fact. If the variable defined already has a value, within the set_fact variables the existing value will be used instead of the newly assigned value. Encounted with Ansible version 2.16.3

Reproduction example:

- ansible.builtin.set_fact:
    status: "no a json"

- ansible.builtin.shell: >
     echo "{ \"Self\": { \"Online\": true }, \"Version\": 1.0 }"
  changed_when: false
  register: tailscale_status

- vars:
    status: "{{ tailscale_status.stdout | from_json }}" # <- does not take place
  ansible.builtin.set_fact:
    tailscale_is_online: "{{ status.Self.Online }}"
    tailscale_version: "{{ status.Version }}"

How I encountered the issue. In a project where the devsec.hardening collection -> os_hardeding role is used alongside this role, and that roles is included before this role. It defines it's own status variable which triggers a failure case in this role on the changed lines.

In some sense it looks like an unintentional behaviour that it works to use vars alongside set_fact as it's not listed as part of the attributes it supports https://docs.ansible.com/ansible/latest/collections/ansible/builtin/set_fact_module.html

The same behaviour is relied upon within the handlers, which I've also changed as it was triggering the error.

Upstream issue reported in Ansible ansible/ansible#82698

Context: if a variable is defined within a set_fact module call, and the
variable value is used within the new varibles defined via set_fact.
If the variable defined already has a value, within the set_fact
variables the existing value will be used instead of the newly assigned
value. Encounted with Ansible version 2.16.3

Reproduction example:

```yaml
- ansible.builtin.set_fact:
    status: "no a json"

- ansible.builtin.shell: >
     echo "{ \"Self\": { \"Online\": true }, \"Version\": 1.0 }"
  changed_when: false
  register: tailscale_status

- vars:
    status: "{{ tailscale_status.stdout | from_json }}" # <- does not take place
  ansible.builtin.set_fact:
    tailscale_is_online: "{{ status.Self.Online }}"
    tailscale_version: "{{ status.Version }}"
```
@flowerysong
Copy link
Contributor

A better choice would be to use a less generic name for this temporary variable, rather than exacerbating the situation by making this role's status variable clobber any one that's set elsewhere.

Copy link
Owner

@artis3n artis3n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR!

@artis3n artis3n merged commit c116505 into artis3n:main Feb 21, 2024
29 checks passed
@mhitza
Copy link
Contributor Author

mhitza commented Mar 6, 2024

@artis3n any planned new release for the role that includes this fix?

@artis3n
Copy link
Owner

artis3n commented Mar 6, 2024

Yes, sorry I am meaning to release it under other structural changes to this role and I've needed to set aside time to get those other things done.

@artis3n
Copy link
Owner

artis3n commented Mar 7, 2024

Published a patch release tonight to get this and some other changes out while the larger changes are still in progress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants