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.

Sign up for GitHub

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

Jump to bottom

[GH-93] Add kubernetetes_auth_backend_* resources #94

Merged
merged 1 commit into from
Sep 27, 2018
Merged

[GH-93] Add kubernetetes_auth_backend_* resources #94

merged 1 commit into from
Sep 27, 2018

Conversation

syndbg
Copy link
Contributor

@syndbg syndbg commented Apr 10, 2018 •
edited
Loading

Work on #93.

Resources identified and implemented:

  • kubernetes_auth_backend_config (create,update,delete,tested and documented)
  • kubernetes_auth_backend_role (create,update,delete,tested and documented)

Data sources identified and implemented:

  • kubernetes_auth_backend_config (read,tested and documented)
  • kubernetes_auth_backend_role (read,tested and documented)

Also included cherry-picked commits from #95 and #103 .

@syndbg syndbg force-pushed the kubernetes-auth-resources branch 3 times, most recently from 712d195 to ff26ced Compare April 11, 2018 12:50
syndbg
syndbg commented Apr 11, 2018
View reviewed changes
vault/resource_kubernetes_auth_backend_config.go
// NOTE: Since reading the auth/<backend>/config does
// not return the `token_reviewer_jwt`,
// set it from data after successfully storing it in Vault.
d.Set("token_reviewer_jwt", data["token_reviewer_jwt"])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to reviewers that this is probably dangerous.

It seems like the only way to get the token_reviewer_jwt in the terraform state is to set it after successfully writing the data to Vault.

@syndbg syndbg force-pushed the kubernetes-auth-resources branch 10 times, most recently from 72a47b1 to 88e7389 Compare April 15, 2018 11:56
@syndbg
Copy link
Contributor Author

syndbg commented Apr 15, 2018

Ready for review.

@paddycarver

@syndbg syndbg changed the title [WIP][GH-93] Add kubernetetes_auth_backend resources [GH-93] Add kubernetetes_auth_backend resources Apr 15, 2018
@syndbg syndbg changed the title [GH-93] Add kubernetetes_auth_backend resources [GH-93] Add kubernetetes_auth_backend_* resources Apr 15, 2018
@syndbg syndbg force-pushed the kubernetes-auth-resources branch 3 times, most recently from c576f48 to 2fa86b9 Compare April 16, 2018 13:10
@dgonzalezruiz
Copy link

Could this be merged ASAP? Would be really could to have it in.

@syndbg syndbg force-pushed the kubernetes-auth-resources branch 4 times, most recently from 4a3f77c to fc19cb1 Compare May 7, 2018 14:21
@syndbg
Copy link
Contributor Author

syndbg commented May 7, 2018

I'm rebasing latest upstream master.

Noticing that tests fail now after https://github.com/terraform-providers/terraform-provider-vault/pull/110/files was merged.

Good thing is I caught one broken link to the kubernetes_config data source. Fixed and pushed.

However after the website tests pass I get this:

make[1]: *** [website-provider-test] Error 4
make[1]: Leaving directory `/home/travis/gopath/src/github.com/hashicorp/terraform-website'
make: *** [website-test] Error 2
The command "make website-test" exited with 2.

Investigating...

@syndbg syndbg closed this May 7, 2018
@syndbg syndbg reopened this May 7, 2018
@syndbg syndbg closed this May 7, 2018
@syndbg syndbg reopened this May 7, 2018
@syndbg
Copy link
Contributor Author

syndbg commented May 10, 2018

I have no idea why the test suite fails. Was fine before https://github.com/terraform-providers/terraform-provider-vault/pull/110/files .

Could you help out, @radeksimko ?

@paigr paigr mentioned this pull request Jun 19, 2018
Closed
@tyrannosaurus-becks tyrannosaurus-becks self-assigned this Sep 24, 2018
@tyrannosaurus-becks
Copy link
Contributor

@syndbg thank you for contributing this code! Apologies for the long lag on the review, this repo was in transition from the Terraform team to the Vault team.

I'm eager to get going on this PR. I'm curious about the test failure. Would you be willing to merge in master and push it, so we can get a fresher test and take a look?

tyrannosaurus-becks
tyrannosaurus-becks reviewed Sep 24, 2018
View reviewed changes
Copy link
Contributor

@tyrannosaurus-becks tyrannosaurus-becks left a comment

Choose a reason for hiding this comment

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

I pulled the branch and all the tests you wrote are passing and look great. I bet when you merge in master and push again, everything will pass. This code is fantastic! Thank you for contributing it. Just a couple minor things and I'm happy to approve and merge it.

vault/resource_kubernetes_auth_backend_config.go Outdated

_, err := client.Logical().Write(path, data)

d.SetId(path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to move this up above the Write line so it's not between the err creation and the error check?

Copy link
Contributor Author

@syndbg syndbg Sep 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Good note. Actually it's a mistake on my end.

It must be after the Write. Reasoning behind this is to make sure that SetId is called only after the value has been successfully written in Vault.

vault/resource_kubernetes_auth_backend_role.go Outdated

_, err := client.Logical().Write(path, data)

d.SetId(path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be moved up too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto as #94 (comment)

@syndbg
Copy link
Contributor Author

syndbg commented Sep 25, 2018

@tyrannosaurus-becks No problem. I'll get this going in the next few days so that we can finally merge it in master so that everybody can use it.

@ghost ghost added the size/XXL label Sep 26, 2018
@syndbg
[hashicorpGH-93] Add kubernetetes_auth_backend_* resources
2575ef2 
Signed-off-by: Anton Antonov <anton.synd.antonov@gmail.com>
@ghost ghost added the size/XXL label Sep 26, 2018
@syndbg
Copy link
Contributor Author

syndbg commented Sep 26, 2018

@tyrannosaurus-becks Updated and ready for review.

We have a single thing to clear: whether SetId is to be moved before or after Write (creation in general) of resources.

From my understanding of terraform and other providers it's supposed to be set only after a resource is successfully created in terms of terraform apply. This is prevalent in the write function of many resources defined in this provider such as https://github.com/terraform-providers/terraform-provider-vault/blob/master/vault/resource_database_secret_backend_role.go#L119 or https://github.com/terraform-providers/terraform-provider-vault/blob/c34dfd0da9392a7d53a9279d3b4cdaed93ea4288/vault/resource_ldap_auth_backend_group.go#L69 where it's done a little bit back and forth.

For data sources same applies except that if it's not readable from the remote API/service/anything, SetId("") is called so that the data source can be marked for deletion.

Let me know if I'm right or wrong about this. Don't hesitate :P

@tyrannosaurus-becks
Copy link
Contributor

@syndbg ah! Yes, that is correct. My main concern was not doing it between receiving a possible error and checking it. I see that you moved it to after the err is checked, and that is perfect.

tyrannosaurus-becks
tyrannosaurus-becks approved these changes Sep 27, 2018
View reviewed changes
Copy link
Contributor

@tyrannosaurus-becks tyrannosaurus-becks left a comment

Choose a reason for hiding this comment

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

This looks fantastic! Thank you!

@tyrannosaurus-becks tyrannosaurus-becks merged commit 7e5de10 into hashicorp:master Sep 27, 2018
@syndbg syndbg deleted the kubernetes-auth-resources branch September 27, 2018 16:26
@syndbg syndbg mentioned this pull request Sep 27, 2018
Closed
@CharlieC3
Copy link

@tyrannosaurus-becks @syndbg
Do you happen to know how soon we can expect a new release to be cut with this change in it?

dandandy pushed a commit to dandandy/terraform-provider-vault that referenced this pull request Jun 17, 2021
Merge pull request hashicorp#94 from syndbg/kubernetes-auth-resources
9d4236d 
[hashicorpGH-93] Add kubernetetes_auth_backend_* resources
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tyrannosaurus-becks tyrannosaurus-becks tyrannosaurus-becks approved these changes

Assignees

@tyrannosaurus-becks tyrannosaurus-becks

Labels
size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@syndbg @dgonzalezruiz @tyrannosaurus-becks @CharlieC3