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

Add plugins to list objects in Vault #343

Merged
merged 23 commits into from Jan 18, 2023

Conversation

tomkivlin
Copy link
Contributor

SUMMARY

Two plugins: one lookup, one module to list objects from a given path.
e.g.

  • list all secrets in a given path
  • list all access policies
  • list all userpass users

Fixes #295

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

vault_list (lookup and module)

@github-actions
Copy link

github-actions bot commented Jan 13, 2023

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

codecov bot commented Jan 14, 2023

Codecov Report

Merging #343 (2b43f4c) into main (6c93da9) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #343      +/-   ##
==========================================
+ Coverage   98.75%   98.82%   +0.06%     
==========================================
  Files          76       80       +4     
  Lines        3862     4086     +224     
  Branches      250      258       +8     
==========================================
+ Hits         3814     4038     +224     
  Misses         39       39              
  Partials        9        9              
Flag Coverage Δ
env_docker-default 98.82% <100.00%> (+0.06%) ⬆️
integration 81.30% <87.80%> (+0.37%) ⬆️
sanity 39.86% <47.56%> (+0.43%) ⬆️
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.67% <93.40%> (+0.47%) ⬆️
target_filter_vault_login_token 77.77% <ø> (ø)
target_import 39.86% <47.56%> (+0.43%) ⬆️
target_lookup_hashi_vault 81.33% <ø> (ø)
target_lookup_vault_ansible_settings 56.28% <ø> (ø)
target_lookup_vault_kv1_get 91.30% <ø> (ø)
target_lookup_vault_kv2_get 91.11% <ø> (ø)
target_lookup_vault_list 90.00% <90.00%> (?)
target_lookup_vault_login 88.57% <ø> (ø)
target_lookup_vault_read 90.00% <ø> (ø)
target_lookup_vault_token_create 79.24% <ø> (ø)
target_lookup_vault_write 57.39% <ø> (ø)
target_module_utils 97.36% <ø> (ø)
target_module_vault_kv1_get 87.50% <ø> (ø)
target_module_vault_kv2_delete 56.93% <ø> (ø)
target_module_vault_kv2_get 87.23% <ø> (ø)
target_module_vault_list 85.71% <85.71%> (?)
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% <ø> (ø)
target_module_vault_write 56.25% <ø> (ø)
target_modules 81.78% <96.24%> (+1.12%) ⬆️
units 96.54% <96.42%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
plugins/lookup/vault_list.py 100.00% <100.00%> (ø)
plugins/modules/vault_list.py 100.00% <100.00%> (ø)
tests/unit/plugins/lookup/test_vault_list.py 100.00% <100.00%> (ø)
tests/unit/plugins/modules/test_vault_list.py 100.00% <100.00%> (ø)

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

Copy link
Collaborator

@briantist briantist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomkivlin this a fantastic first contribution, thank you!

There's some inline comments that are mostly small things.

The vault action group in meta/runtime.yml also needs the module added there:
https://github.com/ansible-collections/community.hashi_vault/blob/main/meta/runtime.yml#L4

The only other thing that I think would be nice is a couple of extra examples for using the lookup in a way where you loop over the actual thing being listed. Since this is the "generic" list lookup, you are (correctly!) returning the raw result, which is always a dictionary, but somewhere inside the dictionary (possibly nested) is a list.

So for example, an example showing how to loop over a list of policies might show a loop like this:

loop: "{{ query('community.hashi_vault.vault_list', 'sys/policies/acl').keys }}"

and an example for kv2 might look like this:

loop: "{{ query('community.hashi_vault.vault_list', 'secret/metadata/path').data.keys }}"

(EDIT I just realized fixing the above path that the examples should also have their "secret" listing paths updates to include metadata and some path)

These types of examples that show how to do specific things tend to be highly used, and in fact sometimes people try to do these operations, get failures, and open issues about it, even if they only needed to tweak their path, or massage the output data.


One last thing just in case you haven't seen it before. Several of my comments have suggested edits, which you can commit from the comment. If you click that on each comment separately, they will all be separate tiny commits that make the CI take forever.

If you view the comments on the "Files" tab instead, you'll see an option to "Add suggestion to batch" and then you click wait until you've added all the suggestions you want to keep, and commit them at once.

You can also make any changes including changes to things I suggested, locally on your branch instead.

Thanks again! Really looking forward to adding these to the collection.

plugins/lookup/vault_list.py Outdated Show resolved Hide resolved
plugins/lookup/vault_list.py Outdated Show resolved Hide resolved
plugins/lookup/vault_list.py Outdated Show resolved Hide resolved
plugins/modules/vault_list.py Outdated Show resolved Hide resolved
plugins/modules/vault_list.py Outdated Show resolved Hide resolved
plugins/modules/vault_list.py Outdated Show resolved Hide resolved
tests/unit/plugins/modules/test_vault_list.py Outdated Show resolved Hide resolved
tests/unit/plugins/lookup/test_vault_list.py Outdated Show resolved Hide resolved
@briantist briantist self-assigned this Jan 15, 2023
@briantist briantist added the enhancement New feature or request label Jan 15, 2023
@briantist briantist added this to the v4.1.0 milestone Jan 15, 2023
@tomkivlin
Copy link
Contributor Author

So for example, an example showing how to loop over a list of policies might show a loop like this:

loop: "{{ query('community.hashi_vault.vault_list', 'sys/policies/acl').keys }}"

and an example for kv2 might look like this:

loop: "{{ query('community.hashi_vault.vault_list', 'secret/metadata/path').data.keys }}"

(EDIT I just realized fixing the above path that the examples should also have their "secret" listing paths updates to include metadata and some path)

These types of examples that show how to do specific things tend to be highly used, and in fact sometimes people try to do these operations, get failures, and open issues about it, even if they only needed to tweak their path, or massage the output data.

Thanks - great idea! However, I have come across some unexpected (to me) behaviour, possibly because 'keys' is a built-in method - hope my alternatives make sense. (In tomkivlin@cd583f9).

@tomkivlin
Copy link
Contributor Author

@briantist thanks for your comprehensive feedback on this, really helpful. I've added some updates, hopefully that covers it all?

@briantist
Copy link
Collaborator

Thanks - great idea! However, I have come across some unexpected (to me) behaviour, possibly because 'keys' is a built-in method - hope my alternatives make sense. (In tomkivlin@cd583f9).

ah! yes that makes sense, I might tweak it further with jinja in a way that will work even with multiple terms, I'll give it a closer look

@briantist thanks for your comprehensive feedback on this, really helpful. I've added some updates, hopefully that covers it all?

Pretty much yeah, I see a little bit I'd like to update in the fixture tests, and we're missing a tiny bit of coverage that I want to fill. I aim for 100% coverage on everything new, and we're missing just one line I think (a 404/missing path response).

I have some time today so I'm going to pull this down and see if I button up these last little things, and then give it another over. I'll push something up soon.

@briantist
Copy link
Collaborator

briantist commented Jan 16, 2023

hey @tomkivlin , it seems I cannot push my changes to your fork. There should be a checkbox on this PR in the right column that allows you to grant maintainers the ability to push changes (see the last screenshot here).

Would you mind ensuring that is checked?

If you prefer not to enable that, that's ok too, I can send a .patch or .diff that you can apply locally to push. Let me know!

EDIT: please disregard, I didn't realize you opened the PR from main on your fork and I was trying to push to the wrong branch.

@briantist
Copy link
Collaborator

briantist commented Jan 16, 2023

@tomkivlin would you take a look at the commits I've pushed up and see if everything looks ok to you? Let me know if there are any questions or things I missed.

There's some weirdness zuul right now (ansible third party check), but that's because of anything in this PR as far as I can tell.

Copy link
Collaborator

@briantist briantist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! a few very small things, otherwise it's really just the merge resolution commit that I'm confused about

I really appreciate the extra tests and attention to detail on the scenarios!

Co-authored-by: Brian Scholer <1260690+briantist@users.noreply.github.com>
@briantist briantist merged commit 1f68053 into ansible-collections:main Jan 18, 2023
@briantist
Copy link
Collaborator

briantist commented Jan 18, 2023

Thanks very much @tomkivlin ! Great work, I hope you'll stick and around consider contributing more in the future :)

I'll look to get version 4.1.0 released soon.

@tomkivlin
Copy link
Contributor Author

Thanks very much @tomkivlin ! Great work, I hope you'll stick and around consider contributing more in the future :)

I'll look to get version 4.1.0 released soon.

Me too, thanks for your support 👍

fh-carlosp pushed a commit to flatironhealth/community.hashi_vault that referenced this pull request May 15, 2023
* read -> list

* lookup and module for vault_list - initial tests

* unit tests for list lookup/module

* copyright - not sure if done correctly

* add new plugins to codecov.yml

* update documentation block for both plugins

* Apply suggestions from @briantist code review

Co-authored-by: Brian Scholer <1260690+briantist@users.noreply.github.com>

* add vault_list to meta/runtime.yml

* add extra examples as per suggestion

* new fixtures for unit tests

* dedup unit test for fixtures

* update module units with new fixtures

* more list lookup examples and formatting

* update secret path in list module examples

* fix policies to test inexistant path response

* fix inexistant path integration tests

* missed variable substitution

* update paths and add comments explaining

* correct the path for lookup plugin

* Update tests/integration/targets/setup_vault_configure/vars/main.yml

* add further tests and comments

* Apply suggestions from code review

Co-authored-by: Brian Scholer <1260690+briantist@users.noreply.github.com>

Co-authored-by: Brian Scholer <1260690+briantist@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Add possibility to get list of secrets from path
2 participants