Add vault_kv2_write module

[devon-mar]

SUMMARY

Fixes #331

ISSUE TYPE

• New Module Pull Request

COMPONENT NAME

vault_kv2_write

ADDITIONAL INFORMATION

[github-actions]

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and the docs are now incorporated into main:
https://ansible-collections.github.io/community.hashi_vault/branch/main

[codecov]

Codecov Report

Merging #348 (3ef307e) into main (fa44d9c) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head 3ef307e differs from pull request most recent head 2ef00d4. Consider uploading reports for the commit 2ef00d4 to get more accurate results

@@            Coverage Diff             @@
##             main     #348      +/-   ##
==========================================
+ Coverage   98.82%   98.86%   +0.04%     
==========================================
  Files          80       82       +2     
  Lines        4086     4244     +158     
  Branches      258      269      +11     
==========================================
+ Hits         4038     4196     +158     
  Misses         39       39              
  Partials        9        9              

Flag	Coverage Δ
env_docker-default	98.86% <100.00%> (+0.04%)	⬆️
integration	81.07% <79.01%> (-0.24%)	⬇️
sanity	39.47% <39.50%> (-0.40%)	⬇️
target_ansible-doc	100.00% <ø> (ø)
target_auth_approle	89.47% <ø> (ø)
target_auth_aws_iam	50.00% <ø> (ø)
target_auth_azure	53.84% <ø> (ø)
target_auth_cert	86.36% <ø> (ø)
target_auth_jwt	91.30% <ø> (ø)
target_auth_ldap	89.47% <ø> (ø)
target_auth_none	100.00% <ø> (ø)
target_auth_token	71.42% <ø> (ø)
target_auth_userpass	85.71% <ø> (ø)
target_connection_options	74.76% <ø> (ø)
target_controller	83.78% <100.00%> (+0.11%)	⬆️
target_filter_vault_login_token	77.77% <ø> (ø)
target_import	39.47% <39.50%> (-0.40%)	⬇️
target_lookup_hashi_vault	81.33% <ø> (ø)
target_lookup_vault_ansible_settings	56.00% <63.63%> (-0.28%)	⬇️
target_lookup_vault_kv1_get	91.30% <ø> (ø)
target_lookup_vault_kv2_get	91.11% <ø> (ø)
target_lookup_vault_list	90.00% <ø> (ø)
target_lookup_vault_login	88.57% <100.00%> (ø)
target_lookup_vault_read	90.00% <ø> (ø)
target_lookup_vault_token_create	79.24% <100.00%> (ø)
target_lookup_vault_write	57.06% <20.00%> (-0.34%)	⬇️
target_module_utils	97.35% <100.00%> (-0.01%)	⬇️
target_module_vault_kv1_get	87.50% <ø> (ø)
target_module_vault_kv2_delete	56.93% <ø> (ø)
target_module_vault_kv2_get	87.23% <ø> (ø)
target_module_vault_kv2_write	57.32% <80.59%> (?)
target_module_vault_list	85.71% <ø> (ø)
target_module_vault_login	83.72% <100.00%> (ø)
target_module_vault_pki_generate_certificate	78.72% <ø> (ø)
target_module_vault_read	85.71% <ø> (ø)
target_module_vault_token_create	91.66% <ø> (ø)
target_module_vault_write	56.25% <ø> (ø)
target_modules	82.09% <86.18%> (+0.30%)	⬆️
units	96.24% <90.16%> (-0.30%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
plugins/lookup/vault_login.py	100.00% <100.00%> (ø)
plugins/lookup/vault_token_create.py	100.00% <100.00%> (ø)
plugins/modules/vault_kv2_write.py	100.00% <100.00%> (ø)
plugins/modules/vault_login.py	100.00% <100.00%> (ø)
plugins/plugin_utils/_hashi_vault_lookup_base.py	100.00% <100.00%> (ø)
...gins/module_utils/authentication/test_auth_none.py	100.00% <100.00%> (ø)
...ins/module_utils/authentication/test_auth_token.py	100.00% <100.00%> (ø)
...s/authentication/test_hashi_vault_authenticator.py	100.00% <100.00%> (ø)
...it/plugins/module_utils/test_hashi_vault_helper.py	100.00% <100.00%> (ø)
tests/unit/plugins/modules/test_vault_kv2_write.py	100.00% <100.00%> (ø)
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[briantist]

@devon-mar welcome back and thanks for submitting this! I gave it a quick skim, I'll have to go over it in detail but a few small things I noticed so far:

• examples reference kv1_write
• short description should not end in .
• we may want to avoid the hvac patch method because it will do its own read to compare data, and since plugin already did that, it would add a redundant read
  • we might even want to offer an option for whether the module should attempt to read the current state or not:
    • this means we can't given accurate changed status (always report changed)
    • but this prevents unnecessary reads when you don't care about the current state
    • but this will make the conditionals a little more complex, since we do still need to read once on a patch
    • when hvac gets the ability to make the server-side-patch calls, this could change again

anyway most of that is just ideas, I need to think about it a little when I'm more awake.

Thanks again!

[devon-mar]

• we may want to avoid the hvac patch method because it will do its own read to compare data, and since plugin already did that, it would add a redundant read

Yeah I took a look at hvac and it also uses the read to get the version for cas, hence why it's not supported with patch in this module. I'm fine with dropping the functionality altogether or adding a warning to the documentation.

• we might even want to offer an option for whether the module should attempt to read the current state or not:

Yeah, I don't even need idempotency in my own use case haha.
I think I could just do current_data={} if the user doesn't want a read.

Let me know what you think.

[briantist]

As I look a little more closely, I think we'll definitely want an option that does straight writes without reading first (would not be compatible with Patch until hvac has server-side patch support).

In addition to merely allowing for fewer roundtrips, it would support a "write-only" scenario where a token has write access to a path but not read access.

I'm also looking at the patch parameter and wondering about changing it from a boolean to a string with choices. Although we can't support server-side patch just yet, I do think we will eventually, and because the permissions needed for the read-then-write method are different, we'll want to make that configurable probably, similar to how it is in the CLI: https://developer.hashicorp.com/vault/docs/secrets/kv/kv-v2

The vault kv patch command also supports a -method flag which can be used to specify HTTP PATCH or read-then-write. The supported values are patch and rw for HTTP PATCH and read-then-write, respectively.

The choices could look like none, auto, patch, rw maybe (we would exclude patch method now).

It might be premature optimization, but I'm trying to avoid a scenario where we later want to do this and have to choose between 1) a breaking change on the patch option's type (will take time to release due to deprecation), or 2) adding a second patch_method option (cluttered options), or 3) converting patch to a raw option that takes either a method or a boolean (more complicated to maintain and test).

I suppose with option 2), we could deprecate the patch option and then remove it.

I'm also open to splitting direct writes and patches to two different modules which could help in handling the various option interactions and reduce branching, though I'm having trouble at the moment deciding if splitting in two is even worse.

Anyway, thinking out loud, please let me know your thoughts!

[briantist]

btw if you want to accept multiple suggestions in one commit, you can do so by looking at the suggestions from the Files changed tab, and then there will be an option to "Add suggestion to batch" so you can group similar changes together that way and save a lot of time waiting for CI :)

[devon-mar]

In addition to merely allowing for fewer roundtrips, it would support a "write-only" scenario where a token has write access to a path but not read access.

I've added the read option which defaults to false.

The choices could look like none, auto, patch, rw maybe (we would exclude patch method now).

What would the auto option do? Choose the method based on hvac version?

It might be premature optimization, but I'm trying to avoid a scenario where we later want to do this and have to choose between 1) a breaking change on the patch option's type (will take time to release due to deprecation), or 2) adding a second patch_method option (cluttered options), or 3) converting patch to a raw option that takes either a method or a boolean (more complicated to maintain and test).
I suppose with option 2), we could deprecate the patch option and then remove it.

I wouldn't even mind another option, leaving patch functionality out together and waiting for a feature request to add it back in. That way we can get an actual use case for it (unless you have one).

I'm also open to splitting direct writes and patches to two different modules which could help in handling the various option interactions and reduce branching, though I'm having trouble at the moment deciding if splitting in two is even worse.

Yeah, I'm on the fence about this one. On one hand, I can't think of a scenario of the top of my head where I'd like to control whether I'd like to write/patch dynamically (i.e. jinja2 template). On the other, write/patch seem closely related and many other Ansible modules group different functionality together (see systemd, yum, etc).

[briantist]

In addition to merely allowing for fewer roundtrips, it would support a "write-only" scenario where a token has write access to a path but not read access.

I've added the read option which defaults to false.

The implementation looks good thank you, I'm thinking about whether the option should have a slightly more specific name, like read_existing or read_before_write, both for clarity within the module, and reducing ambiguity between options in other modules and plugins (in case any introduce a similar option).

Other than that, I might want to make some changes to this depending on whether we keep patch mode or not, and depending on the other questions about patch implementation.

The choices could look like none, auto, patch, rw maybe (we would exclude patch method now).

What would the auto option do? Choose the method based on hvac version?

It would first try the patch method (server-side), and if it failed, would try rw. This would mimic the way the CLI does it.
There's a chance we'll implement this same "auto" functionality into hvac in some way too (see hvac/hvac#925), and then in this collection we'll have to consider also whether we also check hvac version and fallback or not.

It might be premature optimization, but I'm trying to avoid a scenario where we later want to do this and have to choose between 1) a breaking change on the patch option's type (will take time to release due to deprecation), or 2) adding a second patch_method option (cluttered options), or 3) converting patch to a raw option that takes either a method or a boolean (more complicated to maintain and test).
I suppose with option 2), we could deprecate the patch option and then remove it.

I wouldn't even mind another option, leaving patch functionality out together and waiting for a feature request to add it back in. That way we can get an actual use case for it (unless you have one).

Sure, it's totally fine to leave out patch for now, and that puts aside a bunch of the open questions for the time being, and we can at least get the direct write functionality out the door.

I'm not too worried about waiting for a feature request, I think it will be used if it's available, but I want to be respectful of your time and effort and if it's not functionality you need right now, it might not be worth the additional complication I'm adding to do it all at once. This will still be a very useful and welcome addition!

So let me know what you think, if you'd like to get patch in at the same time, I'll put time into trying to resolve these questions to unblock you (and can add some commits myself as time allows to hep it along).

If you'd rather take it out for now and get this landed first, please go ahead with the necessary updates, and then I'll continue reviewing from there.

I'm also open to splitting direct writes and patches to two different modules which could help in handling the various option interactions and reduce branching, though I'm having trouble at the moment deciding if splitting in two is even worse.

Yeah, I'm on the fence about this one. On one hand, I can't think of a scenario of the top of my head where I'd like to control whether I'd like to write/patch dynamically (i.e. jinja2 template). On the other, write/patch seem closely related and many other Ansible modules group different functionality together (see systemd, yum, etc).

Yeah, if we opt not to include patch in this iteration, we can open a discussion about separating it or adding it in (though it can be difficult to get input).

[devon-mar]

The implementation looks good thank you, I'm thinking about whether the option should have a slightly more specific name, like read_existing or read_before_write, both for clarity within the module, and reducing ambiguity between options in other modules and plugins (in case any introduce a similar option).

I think read_before_write is the better of the two since we may be creating a secret (so there isn't an "existing" one to read).

Yeah, if we opt not to include patch in this iteration, we can open a discussion about separating it or adding it in (though it can be difficult to get input).

Since patch needs some more planning I'll take it out of this PR. We can always add it back in another PR.

[devon-mar]

@briantist

Let me know what you want to rename read to + any other changes.

[briantist]

I think read_before_write is the better of the two since we may be creating a secret (so there isn't an "existing" one to read).

ok let's go with read_before_write then, or I'm open to other suggestions, let me know if you have other ideas too

Thanks for sticking with this it's looking really great!

[devon-mar]

@briantist I've applied the requested changes.

[briantist]

Thank you I'm AFK right now so I'll look later but before I forget, please add this module to the action group in runtime.yml

[briantist]

Hi sorry for the delay here. This looks great! I have two small suggested changes and then I think that's it; I've looked at it thoroughly and don't expect any more changes after that. Looking forward to getting this merged

[devon-mar]

@briantist changes applied!

[briantist]

@devon-mar thanks again for contributing this module! It has been released in v4.2.0!