Add vault_kv2_delete module #304

idwagner · 2022-09-17T20:23:53Z

SUMMARY

Implementation of vault_kv2_delete module (hvac delete_latest_version_of_secret)

Fixes #300

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

vault_kv2_delete

ADDITIONAL INFORMATION

This implements the delete latest version functionality of a KV v2 secret in Vault. This is a WIP and is missing:

Module docs
Integration tests

Adds vault_kv2_delete module Adds vault_kv2_delete unit tests

github-actions · 2022-09-17T20:25:39Z

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

briantist

Thank you @idwagner ! This is a great start :)

plugins/modules/vault_kv2_delete.py

briantist · 2022-09-17T23:00:09Z

plugins/modules/vault_kv2_delete.py

+
+    try:
+        raw = client.secrets.kv.v2.delete_latest_version_of_secret(path=path, mount_point=engine_mount_point)
+    except hvac.exceptions.Forbidden as e:


You may want to check for some more exceptions, depending on what hvac might raise/handle (best to check the source there to get an idea what to handle here).

Looks like hvac does not do any error handling for this. I also peeked at the Vault source, and it doesn't do anything special in the delete itself, just passing through any errors it gets such as any errors updating the metadata. If theres anything else you can think of, I'd be happy to add it.

tests/unit/plugins/modules/test_vault_kv2_delete.py

- Set default engine path to secret - Add initial documentation

idwagner · 2022-09-19T00:25:46Z

@briantist few questions,

Is there any convention for adding policies to the integration tests by path or key? Should I create a policy on a new kv path for these tests rather than modifying an existing path permission?

Also, I'm thinking maybe I should rename vault_kv2_delete to vault_kv2_delete_latest_version to more closely match hvac's methods. Further development could have modules like vault_kv2_delete_version vault_kv2_undelete and vault_kv2_destroy etc...

briantist · 2022-09-19T00:45:56Z

@briantist few questions,

Is there any convention for adding policies to the integration tests by path or key?

Yes, have a look at this file for the tasks, and this file for the policy definitions.

Should I create a policy on a new kv path for these tests rather than modifying an existing path permission?

Feel it out and see what makes sense. A new path is probably the way to go, where the "setup" parts of the integration tests would create the secret(s) to be deleted in the tests.

Also have a look at the test plugins, in case a new one needs to be added there for any reason (for example for creating/deleting the secrets).

Also, I'm thinking maybe I should rename vault_kv2_delete to vault_kv2_delete_latest_version to more closely match hvac's methods. Further development could have modules like vault_kv2_delete_version vault_kv2_undelete and vault_kv2_destroy etc...

I think I'm still leaning toward a single vaukt_kv2_delete that will have the ability to optionally delete a specific version or versions depending on how the options are set. There can still be other modules like vault_kv2_undelete and vault_kv2_destroy. Any reason in particular you're thinking of going back the other way?

idwagner · 2022-09-22T18:32:11Z

I don't think we really ever settled on going either way, thats probably my fault for venturing ahead :) I am fine implementing a single kv2_delete and adding an optional version parameter.

- Adds versions as list[int]. Hvac call will vary based on wether versions is defined or not - Updated unit tests to check correct function call. - Fixes some docs

codecov · 2022-10-02T01:30:28Z

Codecov Report

Merging #304 (35eb69f) into main (74835ab) will increase coverage by 0.19%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #304      +/-   ##
==========================================
+ Coverage   98.52%   98.72%   +0.19%     
==========================================
  Files          73       75       +2     
  Lines        3602     3854     +252     
  Branches      321      252      -69     
==========================================
+ Hits         3549     3805     +256     
+ Misses         44       40       -4     
  Partials        9        9

Flag	Coverage Δ
env_docker-default	`98.72% <100.00%> (+0.19%)`	⬆️
integration	`81.08% <82.69%> (+0.14%)`	⬆️
sanity	`39.01% <42.30%> (+0.23%)`	⬆️
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	`73.07% <ø> (ø)`
target_auth_userpass	`85.71% <ø> (ø)`
target_connection_options	`74.76% <ø> (ø)`
target_controller	`83.25% <ø> (+0.73%)`	⬆️
target_filter_vault_login_token	`77.77% <ø> (ø)`
target_import	`39.01% <42.30%> (+0.23%)`	⬆️
target_lookup_hashi_vault	`81.33% <ø> (ø)`
target_lookup_vault_ansible_settings	`55.87% <ø> (ø)`
target_lookup_vault_kv1_get	`91.30% <ø> (ø)`
target_lookup_vault_kv2_get	`91.66% <ø> (ø)`
target_lookup_vault_login	`88.57% <ø> (ø)`
target_lookup_vault_read	`90.00% <ø> (ø)`
target_lookup_vault_token_create	`79.24% <ø> (+1.06%)`	⬆️
target_lookup_vault_write	`57.57% <ø> (ø)`
target_module_utils	`97.02% <ø> (ø)`
target_module_vault_kv1_get	`87.50% <ø> (ø)`
target_module_vault_kv2_delete	`57.14% <82.69%> (?)`
target_module_vault_kv2_get	`87.23% <ø> (ø)`
target_module_vault_login	`83.72% <ø> (ø)`
target_module_vault_pki_generate_certificate	`78.72% <ø> (ø)`
target_module_vault_read	`85.71% <ø> (ø)`
target_module_vault_token_create	`91.66% <ø> (+1.66%)`	⬆️
target_module_vault_write	`56.46% <ø> (ø)`
target_modules	`80.51% <97.14%> (+3.08%)`	⬆️
units	`96.36% <98.85%> (+0.58%)`	⬆️

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

Impacted Files	Coverage Δ
plugins/modules/vault_kv2_delete.py	`100.00% <100.00%> (ø)`
...ests/unit/plugins/modules/test_vault_kv2_delete.py	`100.00% <100.00%> (ø)`
...sts/unit/plugins/lookup/test_vault_token_create.py	`100.00% <0.00%> (ø)`
...ts/unit/plugins/modules/test_vault_token_create.py	`100.00% <0.00%> (ø)`
plugins/lookup/vault_token_create.py	`100.00% <0.00%> (+3.63%)`	⬆️
plugins/modules/vault_token_create.py	`100.00% <0.00%> (+4.00%)`	⬆️

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

idwagner · 2022-10-03T01:13:37Z

@briantist I believe this is about ready with the integration tests in place. Two of the tests failed with errors that lead me to believe they are re-using a vault session from a previous test? I setup some seed data in setup_vault_configure assuming that would be run during each integration test, however the failed tests don't match that initial seed data.

https://github.com/idwagner/community.hashi_vault/blob/a0f8e7b4597408e8ad330a612e4a783d9b3db6a1/tests/integration/targets/setup_vault_configure/tasks/configure.yml#L100-L107

briantist · 2022-10-05T20:20:52Z

@briantist I believe this is about ready with the integration tests in place. Two of the tests failed with errors that lead me to believe they are re-using a vault session from a previous test? I setup some seed data in setup_vault_configure assuming that would be run during each integration test, however the failed tests don't match that initial seed data.

https://github.com/idwagner/community.hashi_vault/blob/a0f8e7b4597408e8ad330a612e4a783d9b3db6a1/tests/integration/targets/setup_vault_configure/tasks/configure.yml#L100-L107

Ah! I'm sorry you hit that error, but I am happy to see that those tests are failing in exactly the right situation they were designed to!

Two of the tests failed with errors that lead me to believe they are re-using a vault session from a previous test?

They are doing that, intentionally. The key is that I want the tests to be able to run against a local instance of Vault that doesn't need setup on every single run, because it speeds up local iteration.

So in your case, the problem, is that after you create those secret versions and delete some of them, those versions do not exist again. And if you tried to insert new versions, they would not "fill in" those missing ones either they would continue adding new versions to the end (versions 6, 7, 8, etc.).

For a test like this, instead of setting up that secret in the "test-wide" re-usable secrets, you should set it up in tests/integration/targets/module_vault_kv2_delete/tasks/module_vault_kv2_delete_setup.yml in the configuration tasks. In here, you should:

Delete the existing secret completely if it exists (this must succeed whether it exists or not)
Create the secret and all of its versions.

It should be removed from the general vault configure target.

galaxy.yml

- Adds integration tests for module_vault_kv2_delete module. - Adds vault_ci_kv2_metadata_read for reading kv2 metadata in ci tests.

Moves the setup for vault_kv2_delete into a test specific setup instead of the general setup.

idwagner · 2022-10-07T00:24:21Z

It looks like the change corrected one of the tests, but other timed out. Can that test be re-run without a push?

briantist

See the comment about check mode.

The rest are mostly small changes.

In case you haven't seen it before, you can commit multiple suggestions from review at once, if you view the comments in the Files tab. Then, there will be an option to add to batch, and then you can commit the batch at once.

plugins/modules/vault_kv2_delete.py

briantist · 2022-10-08T05:22:42Z

plugins/modules/vault_kv2_delete.py

+
+    module = HashiVaultModule(
+        argument_spec=argspec,
+        supports_check_mode=True


At the moment, the module does not account for check mode, and so with this set to True, check mode would delete secrets 😱

One issue currently (same reason status is always changed) is that without checking the current status of the secret, check mode has nothing to do but return changed.

I'm mulling over the idea of using metadata reads to get current versions status/secret existence to be able to solve both of those, but their use should be optional (a client may have permission to delete but not to hit those other endpoints).

I'll look at this again tomorrow and think about whether that makes sense at all, and if so whether it should be in this PR or a follow-up.

The first thing we should do is add an integration test and unit test (example) for check mode before making any additional changes, and those tests should fail right now.

Thank you, I think for now the empty result with no Vault connection is fine.

The unit tests are great! Please also add an integration test for check mode; it should be easy, just checking that the result is changed and that the deletion did not happen.

Even though the unit test covers this a little more thoroughly, it's always good to have it covered by integration too just to have the code actually invoked by ansible.

tests/integration/targets/module_vault_kv2_delete/tasks/module_vault_kv2_delete_setup.yml

Co-authored-by: Brian Scholer <[email protected]>

Updates vault_kv2_delete module and tests for check mode. Vault_kv2_delete will skip the hvac call on check mode and return an empty dictionary.

plugins/modules/vault_kv2_delete.py

briantist · 2022-10-14T13:07:41Z

plugins/modules/vault_kv2_delete.py

+
+    module = HashiVaultModule(
+        argument_spec=argspec,
+        supports_check_mode=True


Thank you, I think for now the empty result with no Vault connection is fine.

The unit tests are great! Please also add an integration test for check mode; it should be easy, just checking that the result is changed and that the deletion did not happen.

Even though the unit test covers this a little more thoroughly, it's always good to have it covered by integration too just to have the code actually invoked by ansible.

tests/integration/targets/module_vault_kv2_delete/tasks/module_vault_kv2_delete_setup.yml

tests/integration/targets/module_vault_kv2_delete/tasks/module_vault_kv2_delete_test.yml

Co-authored-by: Brian Scholer <[email protected]>

tests/integration/targets/module_vault_kv2_delete/tasks/module_vault_kv2_delete_test.yml

briantist · 2022-10-19T22:27:32Z

There is one more thing I forgot (#252 🤦): please add this module to the action group:

https://github.com/ansible-collections/community.hashi_vault/blob/main/meta/runtime.yml#L4

…_vault_kv2_delete_test.yml Co-authored-by: Brian Scholer <[email protected]>

Updated integration test to fix invalid yaml anchors

briantist · 2022-11-04T02:51:44Z

@idwagner thanks so much for this contribution! It's now released in version 3.4.0! 🥳

WIP on hashi_vault.vault_kv2_delete module

91dd47c

Adds vault_kv2_delete module Adds vault_kv2_delete unit tests

idwagner mentioned this pull request Sep 17, 2022

Add KV v2 delete functionality. #300

Closed

briantist self-assigned this Sep 17, 2022

briantist added the enhancement New feature or request label Sep 17, 2022

briantist reviewed Sep 17, 2022

View reviewed changes

idwagner added 3 commits September 18, 2022 10:39

Tweaks to vault_kv2_delete module

23ebf50

- Set default engine path to secret - Add initial documentation

Use vault_write response handling for 200/204s

dec95f6

Additional vault_kv2_delete docs

73bd8f3

Update Author

f2e5231

idwagner added 3 commits October 1, 2022 20:21

Update unit tests for change in response

dece66c

Add optional versions parameter for kv2_delete

e330ef2

- Adds versions as list[int]. Hvac call will vary based on wether versions is defined or not - Updated unit tests to check correct function call. - Fixes some docs

Fix lint errors

330d380

Add latest/specific version matrix to tests

936b466

briantist reviewed Oct 6, 2022

View reviewed changes

galaxy.yml Outdated Show resolved Hide resolved

idwagner added 2 commits October 6, 2022 19:18

Add integration tests for module_vault_kv2_delete

6db80c8

- Adds integration tests for module_vault_kv2_delete module. - Adds vault_ci_kv2_metadata_read for reading kv2 metadata in ci tests.

Move setup to test specific setup

6c0c2d6

Moves the setup for vault_kv2_delete into a test specific setup instead of the general setup.

idwagner force-pushed the develop branch from a0f8e7b to 6c0c2d6 Compare October 6, 2022 23:45

idwagner changed the title ~~WIP: Add vault_kv2_delete module~~ Add vault_kv2_delete module Oct 7, 2022

briantist reviewed Oct 8, 2022

View reviewed changes

briantist added the hacktoberfest-accepted A PR accepted for Hacktoberfest purposes (even if it's not yet approved or merged). label Oct 10, 2022

idwagner and others added 2 commits October 10, 2022 07:31

Apply suggestions from code review

3e523bf

Co-authored-by: Brian Scholer <[email protected]>

Update to add check mode

cc76a15

Updates vault_kv2_delete module and tests for check mode. Vault_kv2_delete will skip the hvac call on check mode and return an empty dictionary.

idwagner added 2 commits October 13, 2022 19:53

Fix docs and linting

9cafc72

Remove self-referential link

1b3aeb1

idwagner force-pushed the develop branch from 8be9861 to 1b3aeb1 Compare October 14, 2022 00:41

briantist reviewed Oct 14, 2022

View reviewed changes

idwagner and others added 4 commits October 15, 2022 15:32

Apply suggestions from code review

ccba8d3

Co-authored-by: Brian Scholer <[email protected]>

Add integration test for check mode

8c5d801

Style fixes

c29ce68

Fully qualify the module name

df98b67

briantist reviewed Oct 16, 2022

View reviewed changes

tests/integration/targets/module_vault_kv2_delete/tasks/module_vault_kv2_delete_test.yml Show resolved Hide resolved

tests/integration/targets/module_vault_kv2_delete/tasks/module_vault_kv2_delete_test.yml Outdated Show resolved Hide resolved

idwagner and others added 2 commits October 19, 2022 19:59

Update tests/integration/targets/module_vault_kv2_delete/tasks/module…

0e908ed

…_vault_kv2_delete_test.yml Co-authored-by: Brian Scholer <[email protected]>

Fix tests and add meta runtime

35eb69f

Updated integration test to fix invalid yaml anchors

briantist approved these changes Oct 21, 2022

View reviewed changes

briantist added this to the v3.4.0 milestone Oct 23, 2022

briantist merged commit e6c9a18 into ansible-collections:main Oct 30, 2022

briantist mentioned this pull request Oct 30, 2022

Revoke tokens created by the modules and lookups #287

Open

briantist mentioned this pull request Nov 19, 2022

✨ Feature Request: Dedicated Module for KV secrets #331

Closed

briantist mentioned this pull request Aug 2, 2024

add a vault_kv2_destroy module #446

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add vault_kv2_delete module #304

Add vault_kv2_delete module #304

idwagner commented Sep 17, 2022 •

edited

Loading

github-actions bot commented Sep 17, 2022 •

edited

Loading

briantist left a comment

briantist Sep 17, 2022

idwagner Sep 19, 2022

idwagner commented Sep 19, 2022

briantist commented Sep 19, 2022

idwagner commented Sep 22, 2022

codecov bot commented Oct 2, 2022 •

edited

Loading

idwagner commented Oct 3, 2022

briantist commented Oct 5, 2022

idwagner commented Oct 7, 2022

briantist left a comment

briantist Oct 8, 2022

briantist Oct 14, 2022

briantist Oct 14, 2022

briantist commented Oct 19, 2022

briantist commented Nov 4, 2022

Add vault_kv2_delete module #304

Add vault_kv2_delete module #304

Conversation

idwagner commented Sep 17, 2022 • edited Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

github-actions bot commented Sep 17, 2022 • edited Loading

Docs Build 📝

briantist left a comment

Choose a reason for hiding this comment

briantist Sep 17, 2022

Choose a reason for hiding this comment

idwagner Sep 19, 2022

Choose a reason for hiding this comment

idwagner commented Sep 19, 2022

briantist commented Sep 19, 2022

idwagner commented Sep 22, 2022

codecov bot commented Oct 2, 2022 • edited Loading

Codecov Report

idwagner commented Oct 3, 2022

briantist commented Oct 5, 2022

idwagner commented Oct 7, 2022

briantist left a comment

Choose a reason for hiding this comment

briantist Oct 8, 2022

Choose a reason for hiding this comment

briantist Oct 14, 2022

Choose a reason for hiding this comment

briantist Oct 14, 2022

Choose a reason for hiding this comment

briantist commented Oct 19, 2022

briantist commented Nov 4, 2022

idwagner commented Sep 17, 2022 •

edited

Loading

github-actions bot commented Sep 17, 2022 •

edited

Loading

codecov bot commented Oct 2, 2022 •

edited

Loading