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

Can't set keyword path in data #389

Closed
M0NsTeRRR opened this issue Jul 17, 2023 · 5 comments · Fixed by #404
Closed

Can't set keyword path in data #389

M0NsTeRRR opened this issue Jul 17, 2023 · 5 comments · Fixed by #404
Assignees
Labels
bug Something isn't working
Milestone

Comments

@M0NsTeRRR
Copy link

M0NsTeRRR commented Jul 17, 2023

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 hvac/hvac#133
I've opened this issue to track it and add a fix when hvac will fix the issue

ISSUE TYPE
  • Bug Report
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
Copy link
Author

M0NsTeRRR commented Aug 17, 2023

@briantist replying to your message here.

We can base the new code on :

  • hvac.__version__ if supported
    or
  • python 3.8 <
import pkg_resources
pkg_resources.get_distribution('hvac').version

python 3.8 >=

from importlib.metadata import version
version('hvac')

@briantist
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

That seems reasonable, yeah!

@briantist
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants