Idempotency issue for hashivault_approle_role

[Roxyrob]

Hi,
also 'hashivault_approle_role' isn't idempotent.

Base playbook code that works:

save in test.yml code below and set ENV vars (token/addr)

- name: hashivault_approle_role test
  hosts: localhost
  gather_facts: false

  tasks:
    - hashivault_auth_method:
        token: "{{ lookup('env', 'VAULT_TOKEN') }}"
        url: "{{ lookup('env', 'VAULT_ADDR') }}"
        method_type: "approle"
        mount_point: "approletest"

    - hashivault_approle_role:
        token: "{{ lookup('env', 'VAULT_TOKEN') }}"
        url: "{{ lookup('env', 'VAULT_ADDR') }}"
        name: "role1"
        mount_point: "approletest"

running playbook multiple times will always return ok (idempotent)
ansible-playbook test.yml

TASK [hashivault_approle_role] ********************************************************************************************
ok: [localhost]

You can run above playbook with correct idempotency but, as in below code, any argument passed different by module/api defaults value (also simple state: absent) make the module not idempotent anymore:

   ...
    - hashivault_approle_role:
        token: "{{ lookup('env', 'VAULT_TOKEN') }}"
        url: "{{ lookup('env', 'VAULT_ADDR') }}"
        name: 'role1'
        mount_point: 'approletest'
        state: 'absent'

running playbook multiple times will always return changed (idempotency issue)
ansible-playbook test.yml

TASK [hashivault_approle_role] ********************************************************************************************
changed: [localhost]

same issue with any other arguments:

   ...
    - hashivault_approle_role:
        token: "{{ lookup('env', 'VAULT_TOKEN') }}"
        url: "{{ lookup('env', 'VAULT_ADDR') }}"
        name: 'role1'
        mount_point: 'approletest'
        token_ttl: 30

running playbook multiple times will always return changed (idempotency issue)
ansible-playbook test.yml

TASK [hashivault_approle_role] ********************************************************************************************
changed: [localhost]

[TerryHowe]

Should be going through https://github.com/TerryHowe/ansible-modules-hashivault/blob/main/ansible/modules/hashivault/hashivault_approle_role.py#L182

I would suspect https://github.com/TerryHowe/ansible-modules-hashivault/blob/main/ansible/modules/hashivault/hashivault_approle_role.py#L169 I see that token_policies defaults to [] and maybe that is transformed to None. Need a higher logging level maybe to see more.

[Roxyrob]

I would suspect https://github.com/TerryHowe/ansible-modules-hashivault/blob/main/ansible/modules/hashivault/hashivault_approle_role.py#L169 I see that token_policies defaults to [] and maybe that is transformed to None. Need a higher logging level maybe to see more.

Not sure why a value should be None in that code snippet;

if role_file:
            try:
                desired_state = json.loads(open(params.get('role_file'), 'r').read())
            except Exception as e:
                return {'changed': False, 'failed': True,
                        'msg': 'Error opening role file <%s>: %s' % (params.get('role_file'), str(e))}
        else:
            for arg in args:
                value = params.get(arg)
                if value is not None: 
                    desired_state[arg] = value

if arg are present (for arg in args) on ansible I think coders cannot pass a null (python None) value and in the module you initialize every possible argument to deafult value (so never None). 'm missing something ?

[Roxyrob]

Hi @TerryHowe,
do you know devel/debug python tools usable "without GUI" on Linux ?

What tools you are using for hashivault devel/debug ?

This can speed up my python/ansible devel exeperience so if I'll can catch issues I'll can help the community

Is pdb an option ?

[ahamilto-nodal]

I have identified a few different issues that appear to be able to cause this and wanted to get some feedback on methods of fixing them:

1. This module allows specifying TTLs as a string with unit suffixes (similar to the Vault CLI), however, the Vault API always returns the TTL in seconds so this is a mismatch if the TTL is not specified in seconds (eg. 24h != 86400). The options here are either make the TTL values type int (this appears to be what some other modules do) or write a converter function. I'm leaning towards option 1 but this is a breaking change.
2. In a similar vein, token_bound_cidrs removes /32 netmasks from IPv4 entries. (interestingly, secret_id_bound_cidrs keeps them). This could probably just be fixed with a note in the documentation; I might also open an upstream issue for the inconsistent behavior.

If the proposed fixes above sound good I can test them and open a PR to merge.

cc. @TerryHowe @Roxyrob

[ahamilto-nodal]

For 2 above, hashicorp/vault#11961 appears to be the hashicorp-vault upstream issue. They don't appear to have any timeline for leads for a fix so a documentation note is likely our best path here.

[TerryHowe]

Hi @TerryHowe, do you know devel/debug python tools usable "without GUI" on Linux ?
What tools you are using for hashivault devel/debug ?

This can speed up my python/ansible devel exeperience so if I'll can catch issues I'll can help the community

Is pdb an option ?

I use a combination of PyCharm and vi depending on what I'm doing and rarely use a debugger, especially for this project. Normally a stack trace, a couple debug prints or returning data in the results is more than enough. Sorry about the super slow response, totally missed this question.

[TerryHowe]

I have identified a few different issues that appear to be able to cause this and wanted to get some feedback on methods of fixing them: ...

I think there are some other modules that compare times and have unit problems like that. There may be a precedent set somewhere else or maybe just always convert to seconds before comparison.

[ahamilto-nodal]

I did some digging on the Vault side and it appears to use time.ParseDuration (https://cs.opensource.google/go/go/+/refs/tags/go1.20.5:src/time/format.go;l=1589) to handle parsing the friendly values. Based on the complexity of that function and the precedent I've found in other parts of the code for TTL to always be integer seconds, I think that is the approach I will write up in my PR.