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 AppRole login #34

Merged
merged 3 commits into from
Feb 22, 2018
Merged

Add AppRole login #34

merged 3 commits into from
Feb 22, 2018

Conversation

paddycarver
Copy link
Contributor

@paddycarver paddycarver commented Nov 6, 2017

Add a resource that manages AppRole auth backend logins and generating
Vault tokens.

Note: this PR depends on #31.

Add a resource that manages AppRole auth backend logins and generating
Vault tokens.
Copy link
Contributor

@catsby catsby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 question about additional d.SetId

resp = renewed
d.Set("lease_started", time.Now().Format(time.RFC3339))
d.Set("client_token", resp.Auth.ClientToken)
d.SetId(resp.Auth.Accessor)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this resp.Auth.Accessor value change? It's unusual to see d.SetId outside of a create method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be a permanent, stable ID, but I think I was just setting it just to be safe. I'll remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spoke too soon, sorry. Yes, this value changes, because we're essentially replacing the resource. I'm trading in one token (that has expired) for a new token (that has not). I dunno if it's cool to play around with resource IDs like that, but this resource is of minimal utility if I can't swap out the tokens. :/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unusual to swap them, yes, but the circumstances seem to warrant it

catsby
catsby previously approved these changes Feb 9, 2018
Copy link
Contributor

@catsby catsby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@paddycarver paddycarver merged commit c885b88 into master Feb 22, 2018
@tyrannosaurus-becks tyrannosaurus-becks deleted the paddy_approle_auth_backend_login branch February 16, 2019 00:21
dandandy pushed a commit to dandandy/terraform-provider-vault that referenced this pull request Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants