Closed idwagner closed 1 year ago
github-actions[bot]
commented
1 year ago 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
idwagner
commented
1 year ago @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
commented
1 year ago @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_deletetovault_kv2_delete_latest_versionto more closely match hvac's methods. Further development could have modules likevault_kv2_delete_versionvault_kv2_undeleteandvault_kv2_destroyetc...
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
commented
1 year ago 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.
codecov[bot]
commented
1 year ago Merging #304 (35eb69f) into main (74835ab) will increase coverage by
0.19%. The diff coverage is100.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%) |
:arrow_up: |
| integration | 81.08% <82.69%> (+0.14%) |
:arrow_up: |
| sanity | 39.01% <42.30%> (+0.23%) |
:arrow_up: |
| 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%) |
:arrow_up: |
| target_filter_vault_login_token | 77.77% <ø> (ø) |
|
| target_import | 39.01% <42.30%> (+0.23%) |
:arrow_up: |
| 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%) |
:arrow_up: |
| 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%) |
:arrow_up: |
| target_module_vault_write | 56.46% <ø> (ø) |
|
| target_modules | 80.51% <97.14%> (+3.08%) |
:arrow_up: |
| units | 96.36% <98.85%> (+0.58%) |
:arrow_up: |
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%) |
:arrow_up: |
| plugins/modules/vault_token_create.py | 100.00% <0.00%> (+4.00%) |
:arrow_up: |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
idwagner
commented
1 year ago @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.
briantist
commented
1 year ago @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_configureassuming that would be run during each integration test, however the failed tests don't match that initial seed data.
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:
It should be removed from the general vault configure target.
idwagner
commented
1 year ago It looks like the change corrected one of the tests, but other timed out. Can that test be re-run without a push?
briantist
commented
1 year ago There is one more thing I forgot (#252 🤦): please add this module to the action group:
briantist
commented
1 year ago @idwagner thanks so much for this contribution! It's now released in version 3.4.0! 🥳
SUMMARY
Implementation of vault_kv2_delete module (hvac delete_latest_version_of_secret)
Fixes #300
ISSUE TYPE
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: