Add resource for Okta auth backend #8

wjam · 2017-07-23T20:46:33Z

This adds a resource specific to the Okta auth backend to start to allow a more 'terraform-native' approach to maintaining the configuration of a Vault cluster, rather than having to deal with using vault_generic_secret with random paths. This fixes #7.

wjam · 2017-08-15T14:14:24Z

@radeksimko @apparentlymart Any thoughts on this PR or the approach of having resources more specific to Vault rather than just having the vault_generic_secret resource?

wjam · 2017-09-02T10:32:36Z

@grubernaut @apparentlymart @paddycarver @catsby Is there any appetite for merging this?

wjam · 2017-09-29T22:01:51Z

Any update on this being reviewed?

paddycarver

Hey @wjam! Thanks for submitting this resource, and my apologies for the extremely delayed review.

This is, overall, very well done and your hard work on it is super appreciated. I did notice a few areas that I'd like to see some changes to before I can merge this, and then a few nitpicks. Let me know if you have any questions.

If, due to the lengthy delay before this got reviewed, you've moved on, or don't have the time/interest to shepherd this through the review process anymore, that's totally understandable. Either just comment here, or ignore this, and if I haven't heard back by the first week in November, I'll make the necessary changes myself.

paddycarver · 2017-10-23T12:41:28Z

vault/resource_okta_auth_backend.go

+
+	var err error
+
+	if path == "" {


It's easier to just set path to Default: true.

paddycarver · 2017-10-23T12:43:53Z

vault/resource_okta_auth_backend.go

+			"organization": {
+				Type:        schema.TypeString,
+				Required:    true,
+				ForceNew:    true,


Why is organization ForceNew if it's being set in Update?

paddycarver · 2017-10-23T12:58:27Z

vault/resource_okta_auth_backend.go

+	return nil
+}
+
+func oktaAuthBackendRead(d *schema.ResourceData, meta interface{}) error {


This function should update all the values on the resource with the values that Vault returns. In this case, that'd mean setting all the fields of the vault_okta_auth_backend resource--including users and groups.

This is important to detect when the state of the world has drifted from the config, so it can be corrected. It's also important if import functionality is to be added.

paddycarver · 2017-10-23T12:58:59Z

vault/resource_okta_auth_backend_group.go

+	return nil
+}
+
+func oktaAuthBackendGroupRead(d *schema.ResourceData, meta interface{}) error {


Same here, we need to actually set these values in state, not just assert that the group exists.

paddycarver · 2017-10-23T13:04:00Z

vault/resource_okta_auth_backend_test.go

+	return nil
+}
+
+func assertEquals(expected, actual interface{}) error {


I'm not super crazy about the assert* helpers; first because the Terraform style tends to just repeat the error checking and formatting, second because the way it's implemented right now makes it a bit tricky to understand which field didn't match if there's not a match, and finally because it obscures the implementation detail (that it's doing a != instead of (e.g., for time.Time) using the built-in equality method.

paddycarver · 2017-10-23T13:05:29Z

vault/resource_okta_auth_backend_user.go

+	return nil
+}
+
+func oktaAuthBackendUserRead(d *schema.ResourceData, meta interface{}) error {


Again, we should set the data from the API response here, not just check for existence.

paddycarver · 2017-10-23T13:08:20Z

vault/resource_okta_auth_backend_test.go

+    token = "this must be kept secret"
+    ttl = "1h"
+    group {
+        group_name = "dummy"


We like making any test data that requires uniqueness randomized, so multiple tests can run at once and so if a test doesn't clean up properly after itself, it doesn't break integration tests until someone goes in and cleans it up. You can find some helpers for randomising these names in the acctest package: https://godoc.org/github.com/hashicorp/terraform/helper/acctest

Generally how this is done is by making the config a function, passing in randomised or environment-determined variables, injecting them with fmt.Sprintf, and initializing all these variables in the TestAcc* functions. You can see a few examples in the tests for the AWS auth backend.

paddycarver · 2017-10-23T13:09:38Z

website/docs/r/okta_auth_backend.html.md

@@ -0,0 +1,75 @@
+---


These website docs should be added to the sidebar, in website/vault.erb.

Additional changes: * Change things that should have been a set to be sets * Read the resource after updating it

wjam · 2017-10-24T20:33:43Z

Fixed issues and somehow managed to rebase the PR, although this did fix the merge conflict

wjam · 2017-12-01T23:31:01Z

@paddycarver @grubernaut ???

grubernaut · 2017-12-04T14:29:00Z

Hey @wjam,

Sorry for the delay in someone looking at this, unfortunately I no longer work for HashiCorp. However, I do see that there is now a merge conflict, but the majority of the issues mentioned by @paddycarver were addressed. I believe fixing the merge conflict in website/vault.erb should be all that's needed for this contribution. Thank you again for your patience here, and the code contribution. 💯

paddycarver · 2017-12-04T19:44:30Z

Hey @wjam, apologies for the delay on this. I have notes here on the last few pieces of feedback, which somehow I had convinced myself I had submitted to you, and was waiting on your response. :( Sincere apologies for losing track like that.

Because you've been so patient and worked hard on this, and the pieces of feedback I have are just minor bugs and nitpicks at this point (adding docs to the sidebar, a bug with IDs not being unique enough, and a couple things I'd like to see persisted in state that aren't at the moment) I'm going to just fix them and merge this so it's in the next release of the provider, leaving your commits intact so you get credit for doing ~all the work on this.

Thank you so much for your patience and this awesome contribution to the Vault provider. Really excited about it getting merged.

wjam · 2018-01-21T15:46:05Z

This PR will be 6 months old on Tuesday

paddycarver · 2018-02-13T11:12:24Z

Really sorry for the delay on this. I've opened #72 with some really minor fixes and the merge conflicts sorted, and I'll get a coworker to look over my changes commit in the morning so we can get this merged and closed out.

paddycarver · 2018-02-13T20:25:50Z

This has now been merged and will be released with the next release of the provider. Thanks for contributing it to Terraform!

wjam mentioned this pull request Jul 23, 2017

Stop terraform wanting to reset mount TTLs back to zero #6

Merged

grubernaut added enhancement new-resource labels Jul 24, 2017

grubernaut requested a review from paddycarver September 5, 2017 13:14

paddycarver self-assigned this Sep 6, 2017

paddycarver suggested changes Oct 23, 2017

View reviewed changes

wjam added 2 commits October 24, 2017 21:30

Add resource for Okta auth backend

ad3993c

Implement changes from code review

017e469

Additional changes: * Change things that should have been a set to be sets * Read the resource after updating it

wjam force-pushed the okta_auth_backend branch from 4f471f9 to 017e469 Compare October 24, 2017 20:32

paddycarver mentioned this pull request Feb 13, 2018

Updated Okta Auth backend #72

Merged

paddycarver merged commit 017e469 into hashicorp:master Feb 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add resource for Okta auth backend #8

Add resource for Okta auth backend #8

wjam commented Jul 23, 2017

wjam commented Aug 15, 2017

wjam commented Sep 2, 2017

wjam commented Sep 29, 2017

paddycarver left a comment

paddycarver Oct 23, 2017

paddycarver Oct 23, 2017

paddycarver Oct 23, 2017

paddycarver Oct 23, 2017

paddycarver Oct 23, 2017

paddycarver Oct 23, 2017

paddycarver Oct 23, 2017

paddycarver Oct 23, 2017

wjam commented Oct 24, 2017

wjam commented Dec 1, 2017

grubernaut commented Dec 4, 2017

paddycarver commented Dec 4, 2017

wjam commented Jan 21, 2018

paddycarver commented Feb 13, 2018

paddycarver commented Feb 13, 2018

Add resource for Okta auth backend #8

Add resource for Okta auth backend #8

Conversation

wjam commented Jul 23, 2017

wjam commented Aug 15, 2017

wjam commented Sep 2, 2017

wjam commented Sep 29, 2017

paddycarver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjam commented Oct 24, 2017

wjam commented Dec 1, 2017

grubernaut commented Dec 4, 2017

paddycarver commented Dec 4, 2017

wjam commented Jan 21, 2018

paddycarver commented Feb 13, 2018

paddycarver commented Feb 13, 2018