hashi_vault - add 'proxies' option

[tchernomax]

SUMMARY

Add proxy option so it's possible access vault service that are behind a proxy without using this proxy on the entire playbook.

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

hashi_vault.py

ADDITIONAL INFORMATION

This doesn't work:

---
- hosts: localhost
  gather_facts: false
  tasks:
    - debug:
        msg: "{{ lookup('hashi_vault', 'secret=… url=…') }}"
      environment:
        HTTPS_PROXY: socks5://127.0.0.1:1080

lookups use the environments variables applied to ansible, so I need to use:

HTTPS_PROXY="socks5://127.0.0.1:1080" ansible-playbook toto.yml

But then my whole play use this proxy, which is not what I want.
This MR fix the problem by adding a proxy option:

---
- hosts: localhost
  gather_facts: false
  tasks:
    - debug:
        msg: "{{ lookup('hashi_vault', 'secret=… url=… proxy=socks5://127.0.0.1:1080') }}"

[codecov]

Codecov Report

Merging #50 (08ae63e) into main (8556d4f) will increase coverage by 2.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main      #50      +/-   ##
==========================================
+ Coverage   61.41%   63.43%   +2.01%     
==========================================
  Files           1        1              
  Lines         254      268      +14     
  Branches       45       47       +2     
==========================================
+ Hits          156      170      +14     
  Misses         83       83              
  Partials       15       15              

Impacted Files	Coverage Δ
...ommunity/hashi_vault/plugins/lookup/hashi_vault.py	63.43% <0.00%> (+2.01%)	⬆️

---

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 8556d4f...8e48e00. Read the comment docs.

[briantist]

Hi @tchernomax , thanks for your submission!

I plan to do a more thorough review as soon as I get some time, but for early feedback I'd really like to see tests for this. Do you think you could take a shot at implementing testing for connecting via proxy?

[tchernomax]

Hello @briantist

I'd really like to see tests for this. Do you think you could take a shot at implementing testing for connecting via proxy?

Yes I can… but for that I need a proxy, do you have a idea of how to build that in the CI ?

[briantist]

I'd really like to see tests for this. Do you think you could take a shot at implementing testing for connecting via proxy?

Yes I can… but for that I need a proxy, do you have a idea of how to build that in the CI ?

Not yet! Here's an example I found of testing a proxy in Ansible tests, it looks to be specific to yum so probably not directly usable, but may be a good inspiration.

Small but important distinction: we'd want the proxy tests working not just in the central CI, but via direct ansible-test runs, so like that example above they will be primarily written as ansible. I mention this because it'd probably be much easier to do it in CI only, however we'd like to keep compatibility with ansible-test so people can run tests locally.

There's some internal changes I'm working on that should make writing tests for something like this easier, but it's pretty early in development.

[tchernomax]

@briantist done… (apt install tinyproxy is ugly but every other way I tried failed)

[briantist]

Great work @tchernomax , I appreciate you taking the time! Most of the inline suggestions are around the testing environment and ensuring we don't always assume a container or a specific distro.

Also want to move the selection of tests into tests.yml with the other connections params, but I think it will be simpler (see inline comments).

Some comments around adding handlers for the tinyproxy teardown, you can have a look at handlers/mian.yml for examples of definitions.

Please add an example or two in the EXAMPLES section in the documentation as well.

[tchernomax]

@briantist

Also want to move the selection of tests into tests.yml with the other connections params, but I think it will be simpler (see inline comments).

done

Some comments around adding handlers for the tinyproxy teardown, you can have a look at handlers/main.yml for examples of definitions.

done

Please add an example or two in the EXAMPLES section in the documentation as well.

done (I add to shrink the other parameters else I pass the threshold of 160 column)

[briantist]

Great, I will look deeply through these updates a little later.

Please add an example or two in the EXAMPLES section in the documentation as well.

done (I add to shrink the other parameters else I pass the threshold of 160 column)

Yup no problem, I had to reduce nearly all of the examples in the past, twice. I had a big PR that started before the split to collections, but didn't merge until after, so I had to include reducing all the existing examples to be able to fit the Fully-qualified collection name. Then I had to do it again when we moved the plugin to this collection, since its name is a little bit longer!

[briantist]

Thanks for your patience @tchernomax .

I've done some testing and I think the following is a good way forward:

• a single option named proxies
• uses type: raw
• accepts either a str or dict
• when provided with a dict it is sent directly to hvac unmodified
• when provided with a str, we do your original implementation: use it as a proxy for both http and https

This should give us the best of both worlds: ease of use for the most common cases, and full customization available for those who need it.

The dict can't be set in env: or ini: supplied values but I think that's ok.

What this requires to implement:

• description in the option explaining how to specify it (as I described above, but maybe more user friendly)
• entries in the examples section showing various ways to use it (I can help with that as well)
• validation of the option (ensuring either a string or dict was specified, setting the correct value, etc.). For this part, check out the process_options method; it just calls some other specific purpose methods. I'd like to see a new specific method for this, and then have it called from there.
• it's a bit much to require tests to set up different proxies just to test this; we're not writing the proxy functionality, but since we are already testing both http and https, maybe we can switch it up with varying methods of sending in the proxy, so we at least ensure it works via specifying as string or dict (and maybe an "expected failure" test where we explicitly set the wrong one with dict format?).

What do you think?

[tchernomax]

@briantist

* a single option named `proxies`
* uses `type: raw`
* accepts either a `str` or `dict`
* when provided with a `dict` it is sent directly to `hvac` unmodified
* when provided with a `str`, we do your original implementation: use it as a proxy for both `http` and `https`

This should give us the best of both worlds: ease of use for the most common cases, and full customization available for those who need it.

The dict can't be set in env: or ini: supplied values but I think that's ok.

In fact it can.
I use check_type_dict so if the string is interpretable as a dict, it will be (see the EXAMPLES).

What this requires to implement:

* description in the option explaining how to specify it (as I described above, but maybe more user friendly)

done

* entries in the examples section showing various ways to use it (I can help with that as well)

done

* validation of the option (ensuring either a string or dict was specified, setting the correct value, etc.). For this part, check out the  `process_options` method; it just calls some other specific purpose methods. I'd like to see a new specific method for this, and then have it called from there.

done

* it's a bit much to require tests to set up different proxies just to test this; we're not writing the proxy functionality, but since we are already testing both http and https, maybe we can switch it up with varying methods of sending in the proxy, so we at least ensure it works via specifying as string or dict (and maybe an "expected failure" test where we explicitly set the wrong one with dict format?).

done

What do you think?

good for me

[briantist]

This is great work @tchernomax , I love the ability to accept a serialized dict as a string, which makes for good compatibility with term string, env vars, and ini.

I had thought about that when I proposed the dual idea but discarded it because I didn't want us to implement/support something non-standard, but I didn't know about the validation module_util so that's perfect.

I made a couple tweaks to the param description and examples, but not married to them, lmk if you don't agree.
(and just a newline that went missing)

[briantist]

Looks great @tchernomax , thanks again for taking the time to contribute and working through the changes!