update hvac error handling (Open todo)

[mathijswesterhof]

SUMMARY

This pull request addresses the hashi_vault_common module. In specific, the handling of the HVAC import. In the current situation the import is tried and if failed the exception is handled by setting HAS_HVAC to false. this state is later not used to signal the missing lib to the user.
This pull request fixes this by raising an HVAC specific error which in turn is picked up by the modules that use the common module.
The reason for raising the custom exception is that the module and plugin utils both make use of this class and both have their own error handling service (being; plugins via AnsibleError and modules via the fail handler of the AnsibleModule class)

Note that

• It is not perse a bugfix but more of an open todo still in the code.
• fixing this todo opens up a lot of cleaning in the separate modules where this is being double checked.

I'm not sure if I can confidently remove all the HVAC import try-catch blocks, but based on this change it is a lot easier to write an adapter that can be used for all current modules and future modules significantly reducing duplicated code.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

• module_utils/_hashi_vault_common.py
• module_utils/_hashi_vault_module.py
• plugin_utils/_hashi_vault_plugin.py

ADDITIONAL INFORMATION

Implementing the following change will have a positive effect in replcated code, some examples below for some widely used modules: vault_login

vault_list

before

An exception occurred during task execution. To see the full traceback, use -vvv. The error was: ModuleNotFoundError: No module named 'hvac'
fatal: [192.x.x.x]: FAILED! => {"changed": false, "msg": "Failed to import the required Python library (hvac) on ansible-box's Python /usr/bin/python3. Please read module documentation and install in the appropriate location. If the required library is installed, but Ansible is using the wrong Python interpreter, please consult the documentation on ansible_python_interpreter"}

after

An exception occurred during task execution. To see the full traceback, use -vvv. The error was: No module named 'hvac'
fatal: [192.X.X.X]: FAILED! => { "changed": false, "msg": "Failed to import the required Python library (hvac) on ansible-box's Python /usr/bin/python3. Please read module documentation and install in the appropriate location. If the required library is installed, but Ansible is using the wrong Python interpreter, please consult the documentation on ansible_python_interpreter"}

[mathijswesterhof]

Small note on this pr, I have no clue how to fully and properly run the pytest tests. I'm constantly getting the error that there's a relative import that's missing even though it's clearly there.

     from .compat import mock
E   ImportError: attempted relative import with no known parent package

I'm probably doing something simply stupid but if anyone can get me up to speed with that, that'll be amazing cause I want to properly test everything, especially with the new modules being build.

just for debugging purposes, I'm currently running pytest -c tests/config.yml ./tests in the root folder of the project

[briantist]

Small note on this pr, I have no clue how to fully and properly run the pytest tests. I'm constantly getting the error that there's a relative import that's missing even though it's clearly there.

     from .compat import mock
E   ImportError: attempted relative import with no known parent package

I'm probably doing something simply stupid but if anyone can get me up to speed with that, that'll be amazing cause I want to properly test everything, especially with the new modules being build.

just for debugging purposes, I'm currently running pytest -c tests/config.yml ./tests in the root folder of the project

Hi @mathijswesterhof , welcome and thank you for putting up this PR.

In ansible collection, we don't really run pytest directly, but rather use ansible-test to invoke them. I highly recommend using that tool's container functionality (which is usually used with pre-built containers). The unit and sanity tests are the easiest ones to run locally on your machine.

The first step is to ensure that your checkout directory structure is correct. Your repository should be checked out in this structure: <parent>/ansible_collections/community/hashi_vault

This structure is important!

Then, from the root of the collection:

ansible-test sanity --docker default

ansible-test units --docker default

Units will run their tests across all supported python versions (within the container). For faster local testing you can choose a single python version like so:

ansible-test units --docker default --python 3.10

For more detailed information, please see our Contributor guide.

Let me know if you still need assistance running tests locally, or if the documentation there can be improved.

---

I will have to look over this PR more deeply when I have some more time. I'm traveling now so I will be slower to respond and review for the next several weeks, but I will at least try to approve the CI for new commits while I'm away. I strongly recommend getting set up to run the tests locally so that you can get much faster feedback on your changes.

Thank you!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.21%. Comparing base (cdca188) to head (c624634).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main     #391    +/-   ##
========================================
  Coverage   99.21%   99.21%            
========================================
  Files         109      110     +1     
  Lines        6203     5767   -436     
  Branches     1184     1088    -96     
========================================
- Hits         6154     5722   -432     
+ Misses         39       36     -3     
+ Partials       10        9     -1     

Flag	Coverage Δ
env_docker-default	99.21% <100.00%> (+<0.01%)	⬆️
integration	85.66% <72.64%> (+4.54%)	⬆️
sanity	37.33% <4.27%> (-3.79%)	⬇️
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.82% <89.83%> (+0.40%)	⬆️
target_filter_vault_login_token	77.77% <ø> (ø)
target_import	37.33% <4.27%> (-3.79%)	⬇️
target_lookup_hashi_vault	?
target_lookup_vault_ansible_settings	55.51% <34.61%> (-0.50%)	⬇️
target_lookup_vault_kv1_get	100.00% <100.00%> (+8.69%)	⬆️
target_lookup_vault_kv2_get	100.00% <100.00%> (+8.88%)	⬆️
target_lookup_vault_list	100.00% <100.00%> (?)
target_lookup_vault_login	100.00% <ø> (+11.42%)	⬆️
target_lookup_vault_read	100.00% <100.00%> (+10.00%)	⬆️
target_lookup_vault_token_create	84.44% <ø> (+5.19%)	⬆️
target_lookup_vault_write	?
target_module_utils	96.53% <98.92%> (-0.21%)	⬇️
target_module_vault_database_connection_configure	56.22% <59.09%> (-0.17%)	⬇️
target_module_vault_database_connection_delete	56.04% <61.90%> (-0.18%)	⬇️
target_module_vault_database_connection_read	55.97% <61.90%> (-0.18%)	⬇️
target_module_vault_database_connection_reset	56.04% <61.90%> (-0.18%)	⬇️
target_module_vault_database_connections_list	55.46% <52.38%> (-0.18%)	⬇️
target_module_vault_database_role_create	55.46% <52.38%> (-0.18%)	⬇️
target_module_vault_database_role_delete	56.04% <61.90%> (-0.18%)	⬇️
target_module_vault_database_role_read	55.97% <61.90%> (-0.18%)	⬇️
target_module_vault_database_roles_list	55.46% <52.38%> (-0.18%)	⬇️
target_module_vault_database_rotate_root_creds	55.85% <61.90%> (-0.17%)	⬇️
target_module_vault_database_static_role_create	56.22% <59.09%> (-0.17%)	⬇️
target_module_vault_database_static_role_get_creds	55.97% <61.90%> (-0.18%)	⬇️
target_module_vault_database_static_role_read	55.97% <61.90%> (-0.18%)	⬇️
target_module_vault_database_static_role_rotate_creds	55.85% <61.90%> (-0.17%)	⬇️
target_module_vault_database_static_roles_list	55.46% <52.38%> (-0.18%)	⬇️
target_module_vault_kv1_get	97.43% <100.00%> (+9.93%)	⬆️
target_module_vault_kv2_delete	56.68% <60.00%> (-0.17%)	⬇️
target_module_vault_kv2_get	97.36% <100.00%> (+10.13%)	⬆️
target_module_vault_kv2_write	57.09% <59.25%> (-0.17%)	⬇️
target_module_vault_list	96.96% <100.00%> (+11.25%)	⬆️
target_module_vault_login	93.93% <ø> (+10.21%)	⬆️
target_module_vault_pki_generate_certificate	84.61% <66.66%> (+5.89%)	⬆️
target_module_vault_read	96.96% <100.00%> (+11.25%)	⬆️
target_module_vault_token_create	91.66% <ø> (ø)
target_module_vault_write	55.85% <50.00%> (-0.17%)	⬇️
target_modules	88.94% <82.64%> (+0.39%)	⬆️
units	96.35% <94.09%> (+1.21%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mathijswesterhof]

@briantist I finally had some spare time to put into this again. sorry for the insane long delay. I added some base tests for _hashi_vault_common I tried to trigger code-coverage locally and it shows all tests succeed (but no new CodeCov percentages)

a few questions:

• should i move the tests into a separate file? (I test another class, but it's in the same file so I assumed it should also go in the same test file)
• how do I trigger the codeCov bot? (if I'm able to do that at all)
• should I write an integration test for both the _hashi_vault_plugin and _hashi_vault_module files? (I assume integration since the init is already tested now and the only addition is the try block with the exception handover)

[briantist]

@briantist I finally had some spare time to put into this again. sorry for the insane long delay. I added some base tests for _hashi_vault_common I tried to trigger code-coverage locally and it shows all tests succeed (but no new CodeCov percentages)

• how do I trigger the codeCov bot? (if I'm able to do that at all)

I don't think you can trigger codecov locally because you'd need a token to upload coverage reports to it, but don't worry about that. To be honest, I never run coverage locally, I make sure the tests pass locally and then let CI do it with coverage so we can get the nice reports from codecov. I realize that's not so good when it's your first contribution and you have to wait for me to approve each run, you can do a lot from a single online coverage report since you can browse it and see all the missed lines so easily.

This comment gets edited/updated each time coverage runs, or you can go to the report directly: https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391

Do note it takes a little time for codecov to process the uploaded reports after they are sent.

• should i move the tests into a separate file? (I test another class, but it's in the same file so I assumed it should also go in the same test file)
• should I write an integration test for both the _hashi_vault_plugin and _hashi_vault_module files? (I assume integration since the init is already tested now and the only addition is the try block with the exception handover)

I will have to defer these questions until I can give this a proper thorough review. I am incredibly short on time lately so I will try to get back to this as soon as I can.

[mathijswesterhof]

@briantist any updates?

[briantist]

• should i move the tests into a separate file? (I test another class, but it's in the same file so I assumed it should also go in the same test file)

for now it's probably fine to keep them where they are, we can come back to that if it seems like they'd be better moved

• should I write an integration test for both the _hashi_vault_plugin and _hashi_vault_module files? (I assume integration since the init is already tested now and the only addition is the try block with the exception handover)

I think they mostly get tested in integration implicitly via the integration tests for the plugins and modules that use them. The units help a lot with test cases that are hard to replicate in integration. hvac missing is one of those difficult test cases when it comes to ansible testing because ansible-test installs it for us and there isn't an easy way to tell it to do so selectively.

[briantist]

It might be good to start migrating plugins and modules to use this method. Could you do one plugin and one module and push those up so that I can review? It would be good to (roughly) have each plugin and module be in its own commit, but not strictly necessary. Thanks again for your work on this, sorry for the delays.

[mathijswesterhof]

Not sure why this one failed on the CI - LI, but main is also failing (https://github.com/ansible-collections/community.hashi_vault/actions/runs/9528624007)

Updated the KV modules and Lookups to use the HashiVaultHelper hvac entry instead of an import. (which mainly just removes the import code)

I have also removed the test that runs over the import try/except with the HAS_HVAC switch case. That test is now being done in the HashiVaultHelper test class.

I'm still doubting if it is neater to add:

    def get_hvac(self):
        return self.hvac

to the HashiVaultHelper Class and then use hvac = module.helper.get_hvac() to make it a bit more clear where HVAC is comming from. let me know if that is a good plan.

[briantist]

Not sure why this one failed on the CI - LI, but main is also failing (https://github.com/ansible-collections/community.hashi_vault/actions/runs/9528624007)

yeah the local integration tests have been failing, nothing to do with your changes, I need to get around to fixing that separately.

I'm still doubting if it is neater to add:

    def get_hvac(self):
        return self.hvac

to the HashiVaultHelper Class and then use hvac = module.helper.get_hvac() to make it a bit more clear where HVAC is comming from. let me know if that is a good plan.

I don't have a strong opinion (yet) but one way that method could be advantageous is in unit tests; it makes it easier to mock or wrap in the modules. As you said:

I have also removed the test that runs over the import try/except with the HAS_HVAC switch case. That test is now being done in the HashiVaultHelper test class.

that makes sense, but with a method we could more easily add a test to enforce that modules are actually calling the method, like:

get_hvac = mock.Mock(wraps=helper.get_hvac)
# ...
get_hvac.assert_called_once()

(something like that)

---

disclaimer: I haven't given it deep thought yet, feel free to play around with the different implementations

[briantist]

@mathijswesterhof the devel failures are not related to this, that needs to be fixed separately for the repo.

I want to get started with the new ansible functions next week so if I can get a review before then that would be amazing. otherwise it'll have to wait untill mid september

Unfortunately I don't think it'll be possible. It's an incredibly busy time for me and I have far less time for this than I have in the past, hoping it eases up but looking at my week ahead it feels unlikely I'll be able to take a look before then, we'll see though.

[mathijswesterhof]

@briantist any update?

[briantist]

The CI for the repo has been fixed, so let's get a new CI run after you update your branch

[mathijswesterhof]

@briantist it seems like all checks are successful (apart from the codecov upload timeout) and there are no instances of the import anymore.

[mathijswesterhof]

The previous pipeline check succeeded on all tests. the latest updates on the main branch broke the tests.
more specifically the update on Vault from 1.17 to 1.18 in builtin/credential/cert/path_login.go the exception message has been updated

Should I create a separate issue for this?

[mathijswesterhof]

Should I create a separate issue for this?

#455
It was a fairly easy fix, so I've created a pull request just in case; to speed up the process

[mathijswesterhof]

@briantist checks have re-run and everything is green again :D

[mathijswesterhof]

@briantist can you give me an update on roughly when you can review this? then i can get going with the implementation of the policies and roles routes.

[briantist]

@mathijswesterhof thanks very much for your work on this, sticking with it, and your patience with my dwindling availability. It looks great and I really appreciate your work!

Re: my previous requested change I went ahead and just committed that myself, please review and be sure to pull on your end if you'll be making any other changes. If it all looks good to you then we can merge (as long as my change didn't mess up the tests).

[mathijswesterhof]

@briantist I agree with the changes you've made, I think I messed this up along the way with the various experiments I did on making it look nice and trying to optimize the different components.