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 AWS Auth Backend client. #19

Merged
merged 4 commits into from
Oct 12, 2017
Merged

Add AWS Auth Backend client. #19

merged 4 commits into from
Oct 12, 2017

Conversation

paddycarver
Copy link
Contributor

This allows us to configure the client that Vault uses when making
requests on behalf of the AWS Auth backend. See
https://www.vaultproject.io/api/auth/aws/index.html#configure-client
for more info.

This allows us to configure the client that Vault uses when making
requests on behalf of the AWS Auth backend. See
https://www.vaultproject.io/api/auth/aws/index.html#configure-client
for more info.
The destroy check used to check for all mounts that had an auth backend
configured, not just those controlled by the test. That's not what we
want to do, so this updated version just checks for configurations
created by the test.
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.

2 nit/questions, LGTM

}
log.Printf("[DEBUG] Wrote AWS auth backend client config to %q", "auth/"+backend+"/config/client")

d.SetId("auth/" + backend + "/config/client")
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably want "auth/" + backend + "/config/client" to come from a convenience method at this point, it's hard-coded in 4 spots just right here 😄


# vault\_aws\_auth\_backend\_client

Configures the client used by an AWS Auth Backend in Vault.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: is it helpful to say the HTTP client here? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opinion: The Vault docs never reference it as an HTTP client, so I think it's better without. But am open to changing it if you feel strongly one way or another.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not super-familiar with what's going on behind the scenes here, but I suggest generally that we try to use the same terminology used in the Vault documentation so it's easier for users to understand how these things relate. If Vault is considering HTTP to be an implementation detail here, then it seems right for us to do that too.

Add a helper function that builds the path to the client config for us.
@paddycarver
Copy link
Contributor Author

Think I responded to all the feedback. Good to merge @catsby? (I know you gave it a LGTM, but new code and a response to your question makes me want to just doublecheck.)

@paddycarver paddycarver merged commit ba55b89 into master Oct 12, 2017
@tyrannosaurus-becks tyrannosaurus-becks deleted the paddy_aws_client_auth branch February 16, 2019 00:21
dandandy pushed a commit to dandandy/terraform-provider-vault that referenced this pull request Jun 17, 2021
benashz added a commit that referenced this pull request Apr 5, 2022
Resolves Dependabot alert #19
benashz added a commit that referenced this pull request Apr 5, 2022
Resolves Dependabot alert #19
marcboudreau pushed a commit to marcboudreau/terraform-provider-vault that referenced this pull request Nov 6, 2022
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.

4 participants