ansible-collections / community.hashi_vault

Ansible collection for managing and working with HashiCorp Vault.
https://docs.ansible.com/ansible/devel/collections/community/hashi_vault/index.html
GNU General Public License v3.0
80 stars 58 forks source link

Can't set keyword `path` in data #389

Closed M0NsTeRRR closed 8 months ago

M0NsTeRRR commented 11 months ago
SUMMARY

We can't set path key in data, for example to configure ACME path in PKI plugin. The issue is in hvac library see https://github.com/hvac/hvac/issues/133 I've opened this issue to track it and add a fix when hvac will fix the issue

ISSUE TYPE
COMPONENT NAME
ANSIBLE VERSION
2.15.0
COLLECTION VERSION
5.0.0
STEPS TO REPRODUCE
---
- name: Create vault secret
  community.hashi_vault.vault_write:
    url: https://127.0.0.1:8200
    token: token
    path: pki/config/cluster
    data:
      path: https://vault-active.vault.svc:8200/v1/pki
EXPECTED RESULTS

Update path value of pki/config/cluster

ACTUAL RESULTS
hvac.v1.Client.write() got multiple values for keyword argument 'path'
M0NsTeRRR commented 10 months ago

@briantist replying to your message here.

We can base the new code on :

briantist commented 10 months ago

I think I prefer to use the method we use in other areas of the code where we try to use the desired method, and then fall back, so it'd be something like:

try:
    client.write_data(...)
except (NotImplementedError, AttributeError):
    self.warn("blah blah")
    client.write(...)

See for example:

In this case it'll be slightly different because the write method is not really deprecated, so I'm not sure if we'll actually warn or not, and if we do, I'm not yet sure what it will say.

It's also a different situation because eventually, the code will change back to preferring write (since that's our endgame ultimately), but the code doesn't need to account for that yet.


Since our CI can't currently use multiple hvac versions easily, we test this condition with unit tests instead, see for example:

M0NsTeRRR commented 10 months ago

Maybe something like this is better, I think we don't need to warn people that don't use 'path' or 'wrap_ttl' ?

try:
    client.write_data(data)
except (NotImplementedError, AttributeError):
    if "path" in data or "wrap_ttl" in data:
        raise_from(
            AnsibleError("To support 'path' and 'wrap_ttl' key in 'data' parameter you must have hvac >= 1.2"),
            HashiVaultValueError
        )
    else:
        client.write(data)
briantist commented 10 months ago

That seems reasonable, yeah!

briantist commented 8 months ago

Fix released!