-
Notifications
You must be signed in to change notification settings - Fork 23.8k
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
Added ipa vault lookup plugin #42050
Conversation
The test
The test
|
The test
The test
|
@mvazquezc this PR contains the following merge commits: Please rebase your branch to remove these commits. |
c10b86f
to
2c62c57
Compare
The only test that failed seems that was due to a connectivity error. Could tests be re-run? |
@samdoran Is there anything I could do in order to re-run the failed test? |
I just re-ran the failed test. |
I'm surprised you implemented this as a lookup plugin instead of a module. Any reason not to go the module route? I ask because there's another very similar PR open right now (#41902). |
Well, I thought that since the main functionality is gathering data from a vault, a lookup would be "a better choice". For me, it works like the "file" lookup which gets the content of a file. Does that make sense? @fxfitz |
@fxfitz regarding your question, a module purpose will perform an action (create vault, archive vault data, etc...) into the destination system and this just will catch the vault information/data. Time ago many PR's was rejected because of that fact. Ansible people always tries to separate the lookup purposes from the modules ones. |
@jparrill @mvazquezc Thanks for the clarification! To be honest I hadn't dived into a full-blown review yet to catch the difference (woops; my bad. I spoke too soon! 😄), I just noticed the similarity in the PR list. I'll dive in on this ASAP. |
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.
What do you think about using IPAClient to achieve the same results? That way you're not parsing stdout, you'll have json results, you won't have the ipa client utility, etc etc etc
from __future__ import (absolute_import, division, print_function) | ||
__metaclass__ = type | ||
|
||
ANSIBLE_METADATA = {'metadata_version': '0.1', |
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.
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.
Hmm. On second thought, I'm not even sure if ANSIBLE_METADATA is used in lookup plugins? I think that's only for modules specifically.
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.
Fixed.
author: | ||
- Mario Vazquez (mavazque@redhat.com) | ||
lookup: ipa_vault | ||
version_added: "2.5" |
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.
I think this would be 2.7.
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.
Fixed.
def __init__(self, ipa_cli='/usr/bin/ipa'): | ||
self.ipa_cli = ipa_cli | ||
if not os.path.isfile(self.ipa_cli): | ||
raise AnsibleError("IPA Client not found in the controll machine %s, install IPA Client and try againg." % self.ipa_cli) |
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.
Typo: s/controll/control
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.
Typo: s/againg/again
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.
Fixed
version_added: "2.5" | ||
short_description: Gets info from IPA Vault | ||
requirements: | ||
- ipa client (command line utility) |
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.
just an idea: While I know this isn't module, you could get around this requirement by using IPAClient and pull the same information from the IPA Web API.
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 was my first approach, but sadly IPAClient should change the way it works in order to be able to take advantage of it within a lookup plugin.
Basically their post_json method needs a module object to work, in this lookup we don't use this kind of objects. I tried initializing an empty module object and it didn't work because post_json method needs some information present in the object.
On top of that, they should implement cryptographic functions in order to generate the session key needed to interact with the Vault API.
As a first step we opened an Issue[1] so maybe they can fix the first problem (post_json needing a module object), once this issue is solved we can try to implement this using IPAClient rather than using ipa cli.
[1] #42616
""" | ||
|
||
EXAMPLES = """ | ||
- name: Query ipa for all vaults availabe for the krb token |
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.
Typo: s/availabe/available
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.
Solved.
8caee4a
to
f25dbb5
Compare
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.
Changes pushed.
""" | ||
|
||
EXAMPLES = """ | ||
- name: Query ipa for all vaults availabe for the krb token |
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.
Solved.
def __init__(self, ipa_cli='/usr/bin/ipa'): | ||
self.ipa_cli = ipa_cli | ||
if not os.path.isfile(self.ipa_cli): | ||
raise AnsibleError("IPA Client not found in the controll machine %s, install IPA Client and try againg." % self.ipa_cli) |
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.
Fixed
author: | ||
- Mario Vazquez (mavazque@redhat.com) | ||
lookup: ipa_vault | ||
version_added: "2.5" |
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.
Fixed.
from __future__ import (absolute_import, division, print_function) | ||
__metaclass__ = type | ||
|
||
ANSIBLE_METADATA = {'metadata_version': '0.1', |
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.
Fixed.
version_added: "2.5" | ||
short_description: Gets info from IPA Vault | ||
requirements: | ||
- ipa client (command line utility) |
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 was my first approach, but sadly IPAClient should change the way it works in order to be able to take advantage of it within a lookup plugin.
Basically their post_json method needs a module object to work, in this lookup we don't use this kind of objects. I tried initializing an empty module object and it didn't work because post_json method needs some information present in the object.
On top of that, they should implement cryptographic functions in order to generate the session key needed to interact with the Vault API.
As a first step we opened an Issue[1] so maybe they can fix the first problem (post_json needing a module object), once this issue is solved we can try to implement this using IPAClient rather than using ipa cli.
[1] #42616
@mvazquezc this PR contains the following merge commits: Please rebase your branch to remove these commits. |
f25dbb5
to
817d488
Compare
Hey @fxfitz, Have you seen the pushed changes? Thanks, |
I added some ipa_vault related modules recently (cf. #44631). Together with the modules i added IPAVaultClient (ipa_vault.py in module_utils) as extention to IPAClient which adds all necessray transport, symmetric and asymmetric en/decryption functionality necessary to handle vault data. Maybe this is interessting for you to get rid of all cli calls in your lookup. But it has to pass review first. |
Hi @mvazquezc, Thank you very much for your interest in Ansible. This plugin/module is no longer maintained in this repository and has been migrated to https://github.com/ansible-collections/community.general |
SUMMARY
Added IPA Vault lookup plugin. With this lookup you can:
ISSUE TYPE
COMPONENT NAME
ipa_vault
ANSIBLE VERSION
ADDITIONAL INFORMATION