Skip to content
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

Add modules to configure LDAP and create, update, and delete groups #140

Merged
merged 5 commits into from Jul 24, 2019

Conversation

@jasonneurohr
Copy link
Contributor

commented Jul 3, 2019

Pull request includes

  • NEW- ansible/modules/hashivault/hashivault_auth_ldap.py - to configure LDAP settings
  • NEW - ansible/modules/hashivault/hashivault_identity_group.py - to create, update, and delete groups
  • MODIFY - ansible/modules/hashivault/hashivault_identity_entity.py - fixed a typo

jasonneurohr added some commits Jul 2, 2019

Fixed spelling in ansible/modules/hashivault/hashivault_identity_grou…
…p.py

Renamed ansible/modules/hashivault/hashivault_auth_ldap.py
Added ansible/modules/hashivault/hashivault_identity_group.py to create/update/delete groups
@jasonneurohr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

Added commit:

  • MODIFY - ansible/modules/hashivault/hashivault_auth_list.py - updated the return to be {'changed': False.. since there is no change
@@ -81,7 +81,7 @@ def hashivault_auth_list(params):
result = client.sys.list_auth_methods()
if isinstance(result, dict):
result = result.get('data', result)
return {'changed': True, 'backends': result}
return {'changed': False, 'backends': result}

This comment has been minimized.

Copy link
@drewmullen

drewmullen Jul 15, 2019

Contributor

this is a good change

@drewmullen
Copy link
Contributor

left a comment

you need to add idempotency checks before configuring the ldap method

i typically do this using a desired_state map and a current_state map

  1. if mount already exists, read current mount config and set to current_state
  2. set desired_state params (the params youre passing to ansible module)
  3. run a comparison of the 2

in the end this means that you wont be overwriting your config and also wont see changed when nothing should have changed

bonus points if you include check mode

heres example code for both:

desired_state['tenant_id'] = params.get('tenant_id')

@jasonneurohr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2019

Thanks for the feedback. I've updated it to perform the comparison and also provide check mode.

@drewmullen

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

great work man! looks very good. glad you switched to the default param instead too

I'm adding a few more comments inline, on top of those heres a couple more suggestions (aren't code reviews fun!?):

  • can idempotence checks be added to the identity modules?
  • the vault identity plays are candidates to leverage our automated testing and should have coverage

below is some further direction. im happy to discuss if you need any help / clarity too

example of functional testing for a module: https://github.com/TerryHowe/ansible-modules-hashivault/blob/master/functional/test_mounts.yml

edit this doc to include testing: https://github.com/TerryHowe/ansible-modules-hashivault/blob/master/functional/run.sh

running tests locally so you dont have to rely on travis: https://dev.to/drewmullen/how-to-run-simple-tests-against-custom-ansible-modules-4875

@drewmullen

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

on a side note (unrelated to code): why are you using both LDAP auth method and Vault identities? why not just create / manage groups in LDAP?

Updated wording in hashivault_auth_ldap & hashivault_identity_group
Updated hashivault_identity_group
    default params used
    fixed issues with current vs desired config
Updated functional/run.sh to include tests for hashivault_identity_group
Created functional/test_identity_group.yml
@drewmullen
Copy link
Contributor

left a comment

great work @jasonneurohr! thanks for putting in the effort to make these great modules with good testing

LGTM @TerryHowe

@jasonneurohr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2019

Thanks, @drewmullen - appreciate the time you spent reviewing and providing feedback. To respond to your other message, our current dev environment is using vault identities.

@TerryHowe TerryHowe merged commit 3334c6a into TerryHowe:master Jul 24, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@TerryHowe

This comment has been minimized.

Copy link
Owner

commented Jul 24, 2019

Thanks @jasonneurohr for the contribution and @drewmullen for the review! Released 3.18.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.