-
Notifications
You must be signed in to change notification settings - Fork 59
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
hashi_vault - add 'proxies' option #50
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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? |
Hello @briantist
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 Small but important distinction: we'd want the proxy tests working not just in the central CI, but via direct 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. |
@briantist done… ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
tests/integration/targets/lookup_hashi_vault/lookup_hashi_vault/tasks/tests.yml
Outdated
Show resolved
Hide resolved
tests/integration/targets/lookup_hashi_vault/lookup_hashi_vault/tasks/main.yml
Outdated
Show resolved
Hide resolved
tests/integration/targets/lookup_hashi_vault/lookup_hashi_vault/tasks/tinyproxy_server.yml
Outdated
Show resolved
Hide resolved
tests/integration/targets/lookup_hashi_vault/lookup_hashi_vault/tasks/tinyproxy_server.yml
Outdated
Show resolved
Hide resolved
tests/integration/targets/lookup_hashi_vault/lookup_hashi_vault/tasks/tinyproxy_server.yml
Show resolved
Hide resolved
tests/integration/targets/lookup_hashi_vault/lookup_hashi_vault/tasks/tinyproxy_server.yml
Outdated
Show resolved
Hide resolved
tests/integration/targets/lookup_hashi_vault/lookup_hashi_vault/tasks/token_test.yml
Outdated
Show resolved
Hide resolved
done
done
done (I add to shrink the other parameters else I pass the threshold of 160 column) |
Great, I will look deeply through these updates a little later.
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
ordict
- when provided with a
dict
it is sent directly tohvac
unmodified - when provided with a
str
, we do your original implementation: use it as a proxy for bothhttp
andhttps
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?
In fact it can.
done
done
done
done
good for me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
tests/integration/targets/lookup_hashi_vault/lookup_hashi_vault/tasks/tests.yml
Show resolved
Hide resolved
Co-authored-by: Brian Scholer <1260690+briantist@users.noreply.github.com>
Co-authored-by: Brian Scholer <1260690+briantist@users.noreply.github.com>
Co-authored-by: Brian Scholer <1260690+briantist@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @tchernomax , thanks again for taking the time to contribute and working through the changes!
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
COMPONENT NAME
hashi_vault.py
ADDITIONAL INFORMATION
This doesn't work:
lookups use the environments variables applied to ansible, so I need to use:
But then my whole play use this proxy, which is not what I want.
This MR fix the problem by adding a proxy option: