ADD: unix socket support #458
Conversation
|
Hi @bcachet thanks for opening this! This error is not from your PR, it's failing in the whole collection and I'm not sure why yet so it will be addressed in a different PR at some point. The other failures need to be looked into. I'd also like to see some tests added for this new functionality, ideally both units and integration but let's see how implementation goes for those tests. |
bcachet
force-pushed
the
vault-agent_unix-socket
branch
2 times, most recently
from
8451c53
to
8ae1636
Compare
I added some unit tests around |
|
The error I mentioned before should be fixed in |
This PR is adding support to `VAULT_ADDR`/`url` for unix socket
This allow to connect with vault-agent listening onto Unix socket
```yaml
community.hashi_vault.vault_pki_generate_certificate:
common_name: "{{ ansible_fqdn }}"
role_name: my-role
auth_method: none
engine_mount_point: pki/host/
```
* Validate that client.session is requests_unixsocket.Session when url is unix socket * Validate that HashiVaultHelper.get_vault_client throws when url is unix socket and requests_unixsocket is not available
bcachet
force-pushed
the
vault-agent_unix-socket
branch
from
831aafb
to
c3a6977
Compare
|
I would be interested in discussing about the integration tests strategy. |
This PR is the best place to discuss it I think |
|
Do you have any idea/proposal for an integration test around this change ? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #458 +/- ##
=======================================
Coverage 99.21% 99.22%
=======================================
Files 110 110
Lines 5767 5797 +30
Branches 1088 1094 +6
=======================================
+ Hits 5722 5752 +30
Misses 36 36
Partials 9 9 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I think the things I mentioned in the review should be failing tests, if we were testing the right things, so before trying to address them, directly, I'd like to see the tests modified such that they are catching these conditions (failing) and then we could change the code more.
It's possible that unit tests will be enough, if we are testing the right conditions, so look for tests that test env var precedence, custom session creation, connection retries, etc.
But we may be able to implement minimal integration without adding a Vault agent to the test environment, by catching error messages related to the unix socket, so that we know it's failing for the "right" reason (the socket doesn't exist). For now, I think there's a lot that can be done with units.
| @@ -57,6 +61,20 @@ def get_vault_client( | |||
| :type hashi_vault_revoke_on_logout: bool | |||
| ''' | |||
|
|
|||
| url = kwargs.get('url') or os.environ.get('VAULT_ADDR') | |||
There was a problem hiding this comment.
I think we need to remove the env var retrieval here. This collection specially processes some env vars and VAULT_ADDR is one of them. I think this method will happen after env late-binding so hopefully we can just remove the or condition here.
See also:
| .replace('/', '%2F')) | ||
| socket_url = f"http+unix://{socket_path}" | ||
| kwargs['url'] = socket_url | ||
| kwargs['session'] = requests_unixsocket.Session() |
There was a problem hiding this comment.
I have some concerns that creating a blank session here is going to end up erasing certain other connection options if they are specified. We have helper methods for this purpose, see:
SUMMARY
This PR is adding support to
VAULT_ADDR/urlfor unix socket.This allow, for example, to connect with vault-agent listening onto Unix socket.
Implementation is following guidance proposed by hvac: https://hvac.readthedocs.io/en/latest/advanced_usage.html#vault-agent-unix-socket-listener
ISSUE TYPE
COMPONENT NAME
HashiVaultHelper::get_vault_clientADDITIONAL INFORMATION
Here is the task configuration
Before the change
After the change