devon-mar commented 2 years ago

SUMMARY

This PR adds the certificate auth method to the hashi_vault lookup plugin.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

hashi_vault lookup

ADDITIONAL INFORMATION

briantist commented 2 years ago

Hi @devon-mar ! Thank you and welcome!

I want to first say say how much I appreciate the thoroughness of this contribution and working from the existing auth methods. Although it's on my list, there currently isn't good contributor documentation for this yet, so I'm very interested in hearing your thoughts and feedback on that.

For the addition itself: I've taken a cursory look and it mostly seems great. There are a couple of changes I will recommend when I have a block of time to look more thoroughly.

Besides that, you'll need a changelog fragment (this can be attributed to community.hashi_vault collection rather than the hashi_vault lookup, as auth methods will be shared with all future content), and in addition, there's a large PR that's going to merge soon that adds support for modules. That PR also demonstrates how to split the integration tests for controller/target testing. See #155

So once that lands, I will ask you to rebase this and modify the integration tests accordingly.

I can also help with any of those additional changes needed, so it's not all on you.

Thank you again!

codecov[bot] commented 2 years ago

Codecov Report

Merging #159 (9702406) into main (1168870) will increase coverage by 0.42%. The diff coverage is 98.57%.

@@            Coverage Diff             @@
##             main     #159      +/-   ##
==========================================
+ Coverage   89.56%   89.99%   +0.42%     
==========================================
  Files          35       37       +2     
  Lines        1399     1469      +70     
  Branches      105      113       +8     
==========================================
+ Hits         1253     1322      +69     
- Misses        135      136       +1     
  Partials       11       11

Flag	Coverage Δ
env_docker-default	`89.99% <98.57%> (+0.42%)`	:arrow_up:
integration	`73.85% <86.95%> (+0.49%)`	:arrow_up:
py2.6	`34.95% <34.28%> (-0.05%)`	:arrow_down:
py2.7	`81.55% <91.42%> (+0.49%)`	:arrow_up:
py3.10	`89.10% <98.57%> (+0.47%)`	:arrow_up:
py3.5	`81.82% <91.42%> (+0.48%)`	:arrow_up:
py3.6	`81.82% <91.42%> (+0.48%)`	:arrow_up:
py3.7	`81.82% <91.42%> (+0.48%)`	:arrow_up:
py3.8	`89.10% <98.57%> (+0.47%)`	:arrow_up:
py3.9	`89.10% <98.57%> (+0.47%)`	:arrow_up:
sanity	`35.43% <43.47%> (+0.30%)`	:arrow_up:
target_ansible-doc	`100.00% <ø> (ø)`
target_auth_approle	`89.47% <ø> (ø)`
target_auth_cert	`56.50% <86.95%> (?)`
target_auth_jwt	`91.30% <ø> (ø)`
target_auth_none	`100.00% <ø> (ø)`
target_auth_token	`71.42% <ø> (ø)`
target_connection_options	`74.76% <ø> (ø)`
target_controller	`67.04% <47.82%> (-0.52%)`	:arrow_down:
target_import	`35.43% <43.47%> (+0.30%)`	:arrow_up:
target_lookup_hashi_vault	`79.72% <ø> (ø)`
target_module_utils	`88.75% <97.14%> (+0.51%)`	:arrow_up:
units	`87.36% <97.14%> (+0.49%)`	:arrow_up:

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

Impacted Files	Coverage Δ
plugins/doc_fragments/auth.py	`100.00% <ø> (ø)`
plugins/lookup/hashi_vault.py	`82.43% <ø> (ø)`
plugins/module_utils/_auth_method_cert.py	`95.45% <95.45%> (ø)`
plugins/module_utils/_authenticator.py	`100.00% <100.00%> (ø)`
...gins/module_utils/authentication/test_auth_cert.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1168870...9702406. Read the comment docs.

briantist commented 2 years ago

@devon-mar #155 has been merged, if you wouldn't mind, please rebase from main and have a look at the way the auth method integration tests now do split controller/target testing.

You may also want to have a look at #156 to see what's coming up, but that's no worry. If that merges before this I can help fix up whatever small changes are needed.

devon-mar commented 2 years ago

Although it's on my list, there currently isn't good contributor documentation for this yet, so I'm very interested in hearing your thoughts and feedback on that.

@briantist The hardest part for me was the integration test setup. I needed to use a Vault server with HTTPS, but had some trouble trying to figure out how to do that. I had to look through the connection_options target (since it used https) and figure out what each variable did and where it was set. I thought that I would have to list setup_vault_server_cert as a dependency, but it turns out that I just needed to use the vault_test_server_https var and the setup_cert_content target (i think). Adding some more documentation on what setup_* integration targets to use maybe documenting some of the variables that are available for use for tests.

I think it would also be useful to have some documentation on how to setup a local development environment (running tests, etc). IMO community.zabbix has, a pretty good CONTRIBUTING.md.

Also, adding some linting would be useful to keep future PRs consistent. I could take a shot at it in a different PR if you want.

devon-mar commented 2 years ago

@devon-mar #155 has been merged, if you wouldn't mind, please rebase from main and have a look at the way the auth method integration tests now do split controller/target testing.

I'll try to take a look at it later this week.

briantist commented 2 years ago

Although it's on my list, there currently isn't good contributor documentation for this yet, so I'm very interested in hearing your thoughts and feedback on that.

@briantist The hardest part for me was the integration test setup. I needed to use a Vault server with HTTPS, but had some trouble trying to figure out how to do that. I had to look through the connection_options target (since it used https) and figure out what each variable did and where it was set. I thought that I would have to list setup_vault_server_cert as a dependency, but it turns out that I just needed to use the vault_test_server_https var and the setup_cert_content target (i think). Adding some more documentation on what setup_* integration targets to use maybe documenting some of the variables that are available for use for tests.

I think it would also be useful to have some documentation on how to setup a local development environment (running tests, etc). IMO community.zabbix has, a pretty good CONTRIBUTING.md.

Thank you, this is great feedback! For running the integration tests locally, I have published this page on the docsite: https://docs.ansible.com/ansible/latest/collections/community/hashi_vault/docsite/contributor_guide.html#running-tests-locally

There are two primary methods of getting the local Vault server setup:

the legacy method only requires you to rename the sample integration_config.yml.sample to remove the .sample extension. But this method is slow and limited. I would like to remove it soon (in fact, it would simplify the integration tests, and many of those setup_* targets wouldn't be needed anymore).
using the docker "localenv": the instructions are on the page, and this is the one I use locally and recommend (it's also used in CI). There's some knobs to turn but I personally don't use them and just run setup.sh.

Both methods take care of certificates and starting a Vault server with TLS, but I highly recommend trying the docker method locally. If you run into issues setting that up I'd like to hear about it so improvements can be made (and PRs to both that setup and the docs are welcome!).

I do apologize about the difficulty of finding the HTTPS vars and such. I plan on writing guides specifically on how to contribute new auth methods, new plugins and modules, new connection options, etc.

You happened to pick quite an interesting auth method! For the moment, only the connection options target needed Vault to run with TLS, but even with the difficulty I'm glad the existing vars worked out.

There is a Contributing section in the README, but I'll look at adding a CONTRIBUTING.md because it's easier to find (I think you are not the first to offer that feedback either), even though it will still probably point to docsite.

Also, adding some linting would be useful to keep future PRs consistent. I could take a shot at it in a different PR if you want.

What kind of linting are you thinking about? Something beyond the Ansible sanity tests?

briantist commented 2 years ago

btw @devon-mar , were you able to get a local test environment running with the steps in the linked documents? If not, I'd like to help you with that, since as a new contributor, GitHub won't run the CI from your commits without approval, and you'll have a much nicer time being able to verify locally. Let me know if I can assist!

devon-mar commented 2 years ago

btw @devon-mar , were you able to get a local test environment running with the steps in the linked documents? If not, I'd like to help you with that, since as a new contributor, GitHub won't run the CI from your commits without approval, and you'll have a much nicer time being able to verify locally. Let me know if I can assist!

Yes, it worked on the first try! I also have the workflows enabled on my fork as well.

devon-mar commented 2 years ago

What kind of linting are you thinking about? Something beyond the Ansible sanity tests?

I was thinking of something along the lines of #132.

briantist commented 2 years ago

What kind of linting are you thinking about? Something beyond the Ansible sanity tests?

I was thinking of something along the lines of #132.

Perfect! Ok, yeah I have that issue open in order to explore that, as time permits. If you have further thoughts or specifics on that your comments in the issue would be appreciated!

btw @devon-mar , were you able to get a local test environment running with the steps in the linked documents? If not, I'd like to help you with that, since as a new contributor, GitHub won't run the CI from your commits without approval, and you'll have a much nicer time being able to verify locally. Let me know if I can assist!

Yes, it worked on the first try! I also have the workflows enabled on my fork as well.

Oh excellent! So glad to hear that!

Are you using the legacy method or the docker localenv?
- I'm especially interested in this because I really want to remove the legacy one 😅
Are you able to run the sanity, units, and integration tests?
Any suggestions for further improvements in that space?

devon-mar commented 2 years ago

Are you using the legacy method or the docker localenv?

Docker. I think most of the other collections and Ansible only support Docker these days so it makes sense to remove the legacy method.

Are you able to run the sanity, units, and integration tests?

Yeah, they all worked fine. Maybe you could add the commands for units and sanity to the docs as well (these are what I'm using):

ansible-test sanity --docker -v --lint
ansible-test units --docker -v

I think this PR should be good to go. I ran sanity, unit, and integration tests locally and they all pass. Let me know if there is anything else that need changing.

briantist commented 2 years ago

I think this PR should be good to go. I ran sanity, unit, and integration tests locally and they all pass. Let me know if there is anything else that need changing.

There are a few unresolved comments/requested changes still:

github-actions[bot] commented 2 years ago

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and the docs are now incorporated into main: https://community-hashi-vault-main.surge.sh

devon-mar commented 2 years ago

There are a few unresolved comments/requested changes still:

Sorry about that, somehow missed them.

Tests look all good on my side.

briantist commented 2 years ago

@devon-mar if you have the time and would like to help me get 1.4.0 out sooner, I have two other PRs I'm looking to get review on:

No obligation of course 😅

ansible-collections / community.hashi_vault

Add cert auth method #159

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

Codecov Report

Docs Build 📝