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

IAM resources for KMS KeyRIng and CryptoKey #781

Merged
merged 7 commits into from
Nov 23, 2017
Merged

IAM resources for KMS KeyRIng and CryptoKey #781

merged 7 commits into from
Nov 23, 2017

Conversation

tragiclifestories
Copy link
Contributor

@tragiclifestories tragiclifestories commented Nov 22, 2017 •
edited
Loading

Resolves #762. Many thanks to @rosbo for making it much easier to add this sort of thing.

@rosbo rosbo self-assigned this Nov 22, 2017
@rosbo rosbo self-requested a review November 22, 2017 17:46
rosbo
rosbo suggested changes Nov 22, 2017
View reviewed changes
Copy link
Contributor

@rosbo rosbo left a comment

Choose a reason for hiding this comment

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

Thanks. A few comments and are good to merge

google/iam_kms_crypto_key.go Outdated

cloudResourcePolicy := &cloudresourcemanager.Policy{}

err = Convert(p, cloudResourcePolicy)
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider extracting the conversion logic in a function and reuse it for the keys and the rings. See:
https://github.com/terraform-providers/terraform-provider-google/blob/master/google/iam_folder.go#L85

google/resource_google_kms_crypto_key_iam_test.go Outdated
"testing"
)

// Bindings and members are tested serially to avoid concurrent updates of the org's IAM policy.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove this comment, it only applies to organizations. We cannot create organization on-the-fly the same way we create a different crypto key for each test.

You can split the iam_member test from the iam_binding test. Since a different crypto key is created for each, there won't be concurrent update of the IAM policy between the tests.

I envision two tests: TestAccGoogleKmsCryptoKeyIamMember and TestAccGoogleKmsCryptoKeyIamBinding. You can also put them in two separate test files.

google/resource_google_kms_key_ring_iam_test.go Outdated
"testing"
)

// Bindings and members are tested serially to avoid concurrent updates of the org's IAM policy.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment than for crypto keys

website/docs/r/google_kms_crypto_key_iam_binding.html.markdown
@@ -0,0 +1,47 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

You have to update also the website/google.erb file to add an entry in the left menu panel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added. There are now enough KMS resources in there that it might be a good idea to pull out another submenu - what do you think?

Also, how exactly do I build these docs? I'm basically slamming changes in there and hoping it works at this point ...

Copy link
Contributor

Choose a reason for hiding this comment

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

I am planning on reorganizing the docs around IAM next week. I want to discuss it with @paddycarver and @danawillow first. I would say for now it is fine the way you did it.

About building the docs, @paddycarver do we have documentation somewhere about how to easily build the docs?

Copy link
Contributor

Choose a reason for hiding this comment

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

@tragiclifestories The documentation on how to build the website is there:
https://github.com/hashicorp/terraform-website

website/docs/r/google_kms_key_ring_iam_binding.html.markdown
@@ -0,0 +1,46 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

No doc entry for the member resource?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They''re in now.

@tragiclifestories
Copy link
Contributor Author

Thanks. Extra Todo so I remember tomorrow - the tests need to run in their own project so the created resources can be cleaned up after (keyrings etc are immutable).

@tragiclifestories
Copy link
Contributor Author

I haven't actually been able to run the latest version of the acceptance tests as GCP will not allow me to create any more projects. If someone could fire off a test run when they have a moment, that would be just ducky - I do not anticipate that they will fail, but it's possible that I haven't got all the needed services enabled in the configs.

@rosbo
Copy link
Contributor

rosbo commented Nov 23, 2017

All tests are passing.

--- PASS: TestAccGoogleKmsCryptoKeyIamBinding (86.97s)
--- PASS: TestAccGoogleKmsCryptoKeyIamMember (82.93s)
--- PASS: TestAccGoogleKmsKeyRingIamMember (79.73s)
--- PASS: TestAccGoogleKmsKeyRingIamBinding (88.74s)

@rosbo rosbo merged commit 6f4a792 into hashicorp:master Nov 23, 2017
@tragiclifestories
Copy link
Contributor Author

tragiclifestories commented Nov 28, 2017 via email

chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018
@tragiclifestories @rosbo
IAM resources for KMS KeyRIng and CryptoKey (hashicorp#781)
2f48211 
* Add IAM bindings and member resources for KMS KeyRings

* Add IAM bindings and member resources for KMS CryptoKeys

* Docs for key ring and crypto key IAM resources

* Exctract KMS policy conversions to helper functions

* Split iam_binding and iam_member tests for KMS

* Docs for kms IAM member resources

* Run KMS IAM tests in own project
@maroux maroux mentioned this pull request Aug 5, 2019
Closed
modular-magician added a commit to modular-magician/terraform-provider-google that referenced this pull request Sep 27, 2019
@modular-magician @emilymye
Add fine-grained network endpoint to compute NEGs (hashicorp#781)
74f067e 
Signed-off-by: Modular Magician <magic-modules@google.com>
@ghost
Copy link

ghost commented Mar 30, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rosbo rosbo rosbo approved these changes

Assignees

@rosbo rosbo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support IAM policy bindings for KMS Keyring and CryptoKey resources
2 participants
@tragiclifestories @rosbo