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

provider/vault: vault_auth_backend resource #10988

Merged
merged 1 commit into from May 3, 2017

Conversation

Projects
None yet
7 participants
@Mongey
Contributor

Mongey commented Jan 2, 2017

mitchellh/mapstructure bump is needed to handle a conversion fromjson.Number to int in vault/api

@apparentlymart

This comment has been minimized.

Contributor

apparentlymart commented Jan 2, 2017

Hi @Mongey! Thanks for implementing this.

Am I understanding correctly that this is for enabling Vault auth backends? If so, I'd like to suggest to call it vault_auth_backend rather than just vault_auth. It feels clearer to me to use a count noun here, so we can describe what this does as "enable an auth backend" rather than just "enable an auth".

One other bit of design feedback is that although by default Vault will mount a given auth backend at a path whose name matches the backend name, this is not actually required and it's possible to mount the same auth backend multiple times e.g. to support multiple different AWS accounts, or Github organizations. To support that, perhaps we could have an additional optional+computed path argument that gives the path name, with it defaulting to the same value as given in type if not specified.

I didn't have time yet to dig into the code in detail or test it but I will take a look at this more deeply soon.

@apparentlymart apparentlymart self-requested a review Jan 2, 2017

@Mongey

This comment has been minimized.

Contributor

Mongey commented Jan 2, 2017

Thanks for the feedback @apparentlymart

If so, I'd like to suggest to call it vault_auth_backend rather than just vault_auth.

💯 I'll update

... it's possible to mount the same auth backend multiple times e.g. to support multiple different AWS accounts, or Github organizations.

Ah cool, I didn't really understand what the path arg was when I was implementing this.

To support that, perhaps we could have an additional optional+computed path argument that gives the path name, with it defaulting to the same value as given in type if not specified.

Makes sense, ^ mimics the behaviour of the vault cli auth-enable command.

@Mongey Mongey force-pushed the Mongey:cm-vault-auth branch from b895a07 to c133f86 Jan 2, 2017

@Mongey Mongey changed the title from [WIP] provider/vault: vault_auth resource to [WIP] provider/vault: vault_auth_backend resource Jan 2, 2017

@Mongey Mongey force-pushed the Mongey:cm-vault-auth branch 6 times, most recently from 158c4a6 to 85fee47 Jan 3, 2017

@Mongey Mongey changed the title from [WIP] provider/vault: vault_auth_backend resource to provider/vault: vault_auth_backend resource Jan 6, 2017

@jen20

This comment has been minimized.

Contributor

jen20 commented Jan 23, 2017

Hi @Mongey! There are a few changes that need to be made to this before it can be merged. I'm going to go ahead and pull it down locally to fix it up, and then will open a new PR with the changes.

@jen20 jen20 self-assigned this Jan 23, 2017

@jason-riddle

This comment has been minimized.

Contributor

jason-riddle commented Feb 14, 2017

Any update on this?

@Mongey

This comment has been minimized.

Contributor

Mongey commented Mar 8, 2017

@stack72 / @apparentlymart 👀 This might need to be re-assigned 😓

@Mongey Mongey unassigned jen20 Mar 8, 2017

@stack72

This comment has been minimized.

Contributor

stack72 commented Mar 8, 2017

Hi @Mongey

Apologies for this - yes @jen20 no longer works with us :(

Can you rebase it and I will start a review?

Paul

@Mongey Mongey force-pushed the Mongey:cm-vault-auth branch from 85fee47 to 3b3ddd2 Mar 8, 2017

@Mongey

This comment has been minimized.

Contributor

Mongey commented Mar 15, 2017

@stack72 🙇 rebased the conflict away

}
}
// If we fell out here then we didn't find our Auth in the list.

This comment has been minimized.

@stack72

stack72 Mar 16, 2017

Contributor

I think if we fall out, we should log a message to let the user know that their state has been refreshed - because it wasn't found

@stack72

This comment has been minimized.

Contributor

stack72 commented Mar 16, 2017

Hi @Mongey

So I just tested this and it works as expected :) When i ran your tests, I could see my Vault log as follows:

2017/03/16 10:41:04.499466 [INFO ] core: enabled credential backend: path=github/ type=github
2017/03/16 10:41:05.030997 [INFO ] core: disabled credential backend: path=github/
2017/03/16 10:41:05.033431 [INFO ] core: enabled credential backend: path=ldap/ type=ldap
2017/03/16 10:41:05.385489 [INFO ] core: disabled credential backend: path=ldap/

The tests are also green:

% make testacc TEST=./builtin/providers/vault                                                      ✭
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/03/16 12:40:55 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/vault -v  -timeout 120m
=== RUN   TestDataSourceGenericSecret
--- PASS: TestDataSourceGenericSecret (0.67s)
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestResourceAuth
--- PASS: TestResourceAuth (1.04s)
=== RUN   TestResourceGenericSecret
--- PASS: TestResourceGenericSecret (0.87s)
=== RUN   TestResourcePolicy
--- PASS: TestResourcePolicy (0.85s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/vault	3.450s

I left 1 minor nit - but that's not worth blocking the PR over :)

Paul

@stack72

This comment has been minimized.

Contributor

stack72 commented Mar 16, 2017

Also, just a FYI, the failure in the build here is unrelated to this PR. The failure is as follows:

go test -timeout=30s -parallel=4 github.com/hashicorp/terraform/state/remote github.com/hashicorp/terraform/terraform 
ok  	github.com/hashicorp/terraform/state/remote	0.772s
map[string]dag.Vertex{}
"module.middle.module.inner.null"
map[string]dag.Vertex{}
"module.middle.null"
--- FAIL: TestContext2Plan_computedValueInMap (0.01s)
	context_plan_test.go:2994: err: 2 error(s) occurred:
		
		* module.test_mod.var.services: variable services in module test_mod should be type list, got map
		* shadow graph: module.test_mod.var.services: variable services in module test_mod should be type list, got map
map[string]dag.Vertex{}
"aws"
FAIL
FAIL	github.com/hashicorp/terraform/terraform	4.011s
make: *** [test] Error 123

Merging this for now and will speak to the team about the failure

@stack72

This comment has been minimized.

Contributor

stack72 commented Mar 16, 2017

Actually, will speak to the team first - I believe the test failure is because of the updated vendoring

@apparentlymart apparentlymart removed their request for review Apr 17, 2017

@coen-hyde

This comment has been minimized.

coen-hyde commented May 1, 2017

Any progress on this? I'm pretty excited about having more of Vault configured via Terraform. Avoids the custom provisioning scripts.

@Mongey Mongey force-pushed the Mongey:cm-vault-auth branch from 3b3ddd2 to cd23a20 May 1, 2017

@Mongey

This comment has been minimized.

Contributor

Mongey commented May 1, 2017

@stack72 looks like mapstructure got updated in another PR. Rebased

@stack72

This comment has been minimized.

Contributor

stack72 commented May 3, 2017

This LGTM now @Mongey :) Nice work on this!

Paul

@stack72 stack72 merged commit eb4aa9e into hashicorp:master May 3, 2017

1 check passed

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

@Mongey Mongey deleted the Mongey:cm-vault-auth branch May 3, 2017

@allandrick

This comment has been minimized.

allandrick commented May 16, 2017

Sounds interesting! Any docs for this?

@Mongey

This comment has been minimized.

Contributor

Mongey commented May 16, 2017

@allandrick I can follow up with a PR. (This might help in the time being)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment