Adds support for creating KMS KeyRing resources #518

mrparkers · 2017-10-03T14:51:25Z

Create resource with simple acceptance test
Update acceptance test to create a new GCP project to create the resource in
Add docs

Discussion in #441

I'm mostly looking for advice on what I could be doing better as I continue to work on this in my spare time.

…g ID

danawillow

This is off to a really good start! Most of my comments are just nitpicks. I haven't run the tests yet; I'll wait until you feel this is ready for more review.

danawillow · 2017-10-13T01:02:50Z

google/resource_kms_key_ring.go

+
+	parent := createKmsResourceParentString(project, location)
+
+	_, err = config.clientKms.Projects.Locations.KeyRings.


style nit: for consistency with other resources, I'd recommend either not breaking at all or breaking after Create(

Sorry, I'm not sure what you mean by this. Could you give me another resource to look at as an example?

danawillow · 2017-10-13T01:03:42Z

google/resource_kms_key_ring.go

+	}
+}
+
+func createKmsResourceParentString(project, location string) string {


readability nit: I'm not a huge fan of create being used in functions that don't actually create something, especially in a code base like this where it realistically could be making API calls. How about just kmsResourceParentString?

danawillow · 2017-10-13T01:10:49Z

google/resource_kms_key_ring.go

+		return fmt.Errorf("Error reading KeyRing: %s", err)
+	}
+
+	d.SetId(keyRing.Name)


I don't think we usually set the id again on read- any reason why you needed it here?

I thought I saw another resource do this, but I can't remember where I saw it. I'll remove this.

danawillow · 2017-10-13T01:14:26Z

google/resource_kms_key_ring_test.go

+
+func testGoogleKmsKeyRing_basic(name string) string {
+	return fmt.Sprintf(`
+	resource "google_kms_key_ring" "key_ring" {


nit- can you align this all the way to the left?

danawillow · 2017-10-13T01:15:35Z

google/resource_kms_key_ring_test.go

+	})
+}
+
+func testGoogleKmsKeyRing_basic(name string) string {


I don't have a good reason behind this, but we tend to keep these functions at the end of the file

danawillow · 2017-10-13T01:17:14Z

google/resource_kms_key_ring_test.go

+
+// TODO
+// KMS KeyRings cannot be deleted. This will test if the resource can be added back to state after being removed
+func testGoogleKmsKeyRing_recreate(name string) resource.TestCheckFunc {


If the resource already exists server-side, we can add import functionality instead of trying to get it to essentially import on create.

What is the expected behavior of attempting to create a KeyRing that already exists? Should it fail because the resource already exists on the server? Or should it handle it similar to how an import would?

I think most resources would fail here because you'd expect to have to use terraform import for resources not managed by terraform. This seems like a special situation though, because for all other resources, you can do terraform destroy and terraform apply to re-create your infrastructure without having to modify the names of any of your resources. If calling Create for a KMS Keyring fails for KeyRings that already exist on the server, you wouldn't be able to do this.

In either case, my intention was for this test to capture that logic, where I either expect a failure when re-creating, or for it to be handled gracefully.

mrparkers · 2017-10-13T13:06:25Z

Thanks for the comments @danawillow. I was planning on setting aside some time tonight to work on this some more, and I can address your comments then.

danawillow

All right, I consulted @paddycarver and we think the best option is to remove from state on delete, comment in the code why that's happening, log that the resource itself isn't actually being deleted and why, and make sure we have solid documentation around it. Then create should throw an error because we don't want users to assume they can actually delete+recreate a keyring (since if delete were real, then deleting and recreating would probably delete its contents, and we don't want users to be surprised that the contents of the keyring didn't actually get deleted when they delete+recreate). Sound reasonable?

danawillow · 2017-10-17T22:09:28Z

google/resource_kms_key_ring_test.go

+		parent := kmsResourceParentString(project, location)
+		keyRingName := kmsResourceParentKeyRingName(project, location, name)
+
+		listKeyRingsResponse, err := config.clientKms.Projects.Locations.KeyRings.List(parent).Do()


why list rather than get?

I used list because I don't like it when my tests look similar to the production code. I thought this would be a good way to still test the intended functionality without making it too similar to Read.

I am open to changing it if you would like me to.

mrparkers · 2017-10-21T01:05:31Z

That sounds good. I'm guessing that means import will just work as it normally does with other resources then?

danawillow

For import- yup, it would work the same. Right now this one doesn't support import, so if it's not too much to ask you could add it, but I won't block this PR on import support.

$ make testacc TEST=./google TESTARGS='-run=TestAccGoogleKmsKeyRing_basic' GOOGLE_USE_DEFAULT_CREDENTIALS=true
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccGoogleKmsKeyRing_basic -timeout 120m
=== RUN   TestAccGoogleKmsKeyRing_basic
--- PASS: TestAccGoogleKmsKeyRing_basic (57.63s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	57.789s

All that's missing now is docs for the website (and also you need to run make fmt)

danawillow · 2017-10-23T21:19:03Z

google/resource_kms_key_ring.go

+
+	parent := kmsResourceParentString(project, location)
+
+	keyRing, err := config.clientKms.Projects.Locations.KeyRings.


sorry to point this out again, but mind fixing the style here and elsewhere? https://github.com/golang/go/wiki/CodeReviewComments#line-length
I think one line would be fine, two is also fine (we usually break after the left paren)

Ah, I don't think I understood before but I see what you're saying now. I'll change it to a single line.

danawillow · 2017-10-23T21:19:54Z

google/resource_kms_key_ring.go

+func resourceKmsKeyRingDelete(d *schema.ResourceData, meta interface{}) error {
+	keyRingName := d.Id()
+
+	log.Printf("[INFO] KMS KeyRing resources cannot be deleted from GCP. This KeyRing %s will be removed from Terraform state, but will still be present on the server.", keyRingName)


Let's do WARNING juuuuuuust in case

danawillow · 2017-10-23T21:21:42Z

google/resource_kms_key_ring_test.go

+func testGoogleKmsKeyRing_basic(projectId, projectOrg, projectBillingAccount, keyRingName string) string {
+	return fmt.Sprintf(`
+resource "google_project" "acceptance" {
+    name            = "%s"


can you fix the formatting here? I think you just have a mix of tabs and spaces

…gId struct

mrparkers · 2017-10-24T00:54:51Z

@danawillow Thanks for your comments. I went ahead and added import functionality, so if you could review that, I would appreciate it. I'll leave some comments myself where I had questions.

Also, I just exceeded my project creation quota for the GCP free trial, so I won't be able to run my acceptance test locally anymore 😅

I will work on adding the docs tomorrow so this can get wrapped up. Thanks for your patience!

mrparkers · 2017-10-26T01:05:41Z

@danawillow I addressed your comments and added the documentation page. Please let me know if there's anything you'd like me to change. If you could run the acceptance tests for me, I'd appreciate that as well.

…n/name format

danawillow

$ make testacc TEST=./google TESTARGS='-run=TestAccGoogleKmsKeyRing' GOOGLE_USE_DEFAULT_CREDENTIALS=true
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccGoogleKmsKeyRing -timeout 120m
=== RUN   TestAccGoogleKmsKeyRing_importBasic
--- FAIL: TestAccGoogleKmsKeyRing_importBasic (52.52s)
	testing.go:435: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.

		(map[string]string) {
		}


		(map[string]string) (len=1) {
		 (string) (len=7) "project": (string) (len=20) "terraform-68vpmb0ovk"
		}

=== RUN   TestAccGoogleKmsKeyRing_basic
--- PASS: TestAccGoogleKmsKeyRing_basic (54.80s)
FAIL
exit status 1
FAIL	github.com/terraform-providers/terraform-provider-google/google	107.520s
make: *** [testacc] Error 1

danawillow · 2017-10-26T20:30:32Z

google/resource_kms_key_ring_test.go

+func testGoogleKmsKeyRing_basic(projectId, projectOrg, projectBillingAccount, keyRingName string) string {
+	return fmt.Sprintf(`
+resource "google_project" "acceptance" {
+    name            = "%s"


can you fix the formatting here? I think you've got a spaces vs tabs issue (we usually use tabs)

Oops, my editor automatically converts tabs to spaces for me. I can fix this.

danawillow · 2017-10-26T20:30:41Z

google/resource_kms_key_ring_test.go

+func testGoogleKmsKeyRing_removed(projectId, projectOrg, projectBillingAccount string) string {
+	return fmt.Sprintf(`
+resource "google_project" "acceptance" {
+    name            = "%s"


danawillow · 2017-10-26T20:31:09Z

website/docs/r/google_kms_key_ring.html.markdown

+---
+layout: "google"
+page_title: "Google: google_kms_key_ring"
+sidebar_current: "docs-google-kms-key-ring-x"


The file I was using as a reference had it as well. I'll remove it.

mrparkers · 2017-10-26T20:50:55Z

I'm not sure I understand this failure. Is it complaining because it tried to import the project as well?

I ran this test last night without setting up the extra project (I just commented out the google_project and the google_project_services resources) and it passed.

danawillow · 2017-10-26T20:58:39Z

It's checking that the resource that got imported has exactly the same configuration as the one in the test. Since the one in the test had project set explicitly, it wants project to be set in state. (I can explain this with examples if it's confusing.) Right now, nothing sets "project" in state so that's where the mismatch was. There are a few different ways to solve this- the one I just tested out adds this block into the ImportState function:

	if config.Project != keyRingId.Project {
		d.Set("project", keyRingId.Project)
	}

The other ways to solve this would be to add the set in the Read fn or just always do it in Import, but I kind of like the idea that you only need to set "project" on your resource if the project doesn't match the one in the config.

mrparkers · 2017-10-26T21:29:32Z

That makes perfect sense, thank you for explaining it to me.

I must be remembering incorrectly, because there's no way that test would have passed just by commenting out the google_project and the google_project_services resources.

I'll use the block of code you posted and add it to ImportState like you suggested.

danawillow · 2017-10-27T16:39:43Z

$ make testacc TEST=./google TESTARGS='-run=TestAccGoogleKmsKeyRing' GOOGLE_USE_DEFAULT_CREDENTIALS=true
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccGoogleKmsKeyRing -timeout 120m
=== RUN   TestAccGoogleKmsKeyRing_importBasic
--- PASS: TestAccGoogleKmsKeyRing_importBasic (53.64s)
=== RUN   TestAccGoogleKmsKeyRing_basic
--- PASS: TestAccGoogleKmsKeyRing_basic (51.80s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	105.617s

Looks good, thanks @mrparkers!

jasonjho · 2018-02-12T22:12:42Z

@danawillow @mrparkers Sorry if this has already been addressed, but I am confirming behavior on v0.11.1 that due to the immutability of key rings, a destroy and subsequent create will fail with the following error:

Error creating KeyRing: googleapi: Error 409: KeyRing projects/<proj>/locations/global/keyRings/<some_key> already exists., alreadyExists

What's the proper way to handle this kind of scenario? Any help appreciated.

Thanks!

danawillow · 2018-02-12T23:04:37Z

Hey @jasonjho, is your goal to get the key ring back into your terraform state? If so, you can import it: https://www.terraform.io/docs/providers/google/r/google_kms_key_ring.html#import. It sounds like a data source would be a good fit for this type of scenario too- feel free to file a feature request for one.

* Instantiate the cloudkms client * Implement Create and Read for the kms key ring resource * Expose the kms key ring resource * Create acceptance test for creating a KeyRing, fix read to use KeyRing ID * Add cloudkms library to vendor * Address style comments * Use fully-qualified keyring name in read operation * Remove call to SetId during read operation * Set ID as entire resource string * Spin up a new project for acceptance test * Use Getenv for billing and org environment variables * And test and logs around removal from state * Add comments * Fixes formatting * Log warning instead of info * Use a single line for cloudkms client actions * Add resource import test * Add ability to import resource, update helper functions to use keyRingId struct * Use shorter terraform ID for easier import * Update import test to use the same config as the basic test * Update KeyRing name regex to be consistent with API docs * Add documentation page for resource * Add KeyRing documentation to sidebar * Adds unit tests around parsing the KeyRing import id * Allow for project in id to be autopopulated from config * Throw error in import if project provider is not provided for location/name format * Consistent variable names * Use tabs in resource config instead of spaces * Remove "-x" suffix for docs * Set project attribute on import if different from the project config

Signed-off-by: Modular Magician <[email protected]>

ghost · 2020-03-29T14:30:11Z

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 👉 [email protected]. Thanks!

mrparkers added 4 commits October 3, 2017 09:15

Instantiate the cloudkms client

ab18653

Implement Create and Read for the kms key ring resource

d18db25

Expose the kms key ring resource

946eef1

Create acceptance test for creating a KeyRing, fix read to use KeyRin…

7afead9

…g ID

mrparkers mentioned this pull request Oct 3, 2017

Feature Request: Support for KMS KeyRings / CryptoKeys #441

Closed

Add cloudkms library to vendor

b491ad0

rosbo requested a review from danawillow October 5, 2017 22:08

radeksimko added the new-resource label Oct 8, 2017

Merge branch 'master' into kms-keyring

0ce76d7

danawillow reviewed Oct 13, 2017

View reviewed changes

mrparkers added 7 commits October 14, 2017 20:18

Address style comments

34e5b64

Use fully-qualified keyring name in read operation

7536f92

Remove call to SetId during read operation

5b815d7

Set ID as entire resource string

bc9527f

Spin up a new project for acceptance test

1248ab8

Merge branch 'master' into kms-keyring

e3de446

Use Getenv for billing and org environment variables

d6cb880

danawillow reviewed Oct 20, 2017

View reviewed changes

mrparkers added 3 commits October 21, 2017 19:48

Merge branch 'master' into kms-keyring

9d7ff51

And test and logs around removal from state

c524051

Add comments

5a7b690

danawillow suggested changes Oct 23, 2017

View reviewed changes

mrparkers added 5 commits October 23, 2017 17:31

Fixes formatting

33b74c0

Log warning instead of info

5c49d08

Use a single line for cloudkms client actions

775d43f

Add resource import test

23500d0

Add ability to import resource, update helper functions to use keyRin…

b8d0dbe

…gId struct

mrparkers added 2 commits October 25, 2017 20:00

Add documentation page for resource

a2c3363

Add KeyRing documentation to sidebar

82f4785

Adds unit tests around parsing the KeyRing import id

6e2a6c0

mrparkers changed the title ~~[WIP] Adds support for creating KMS KeyRing resources~~ Adds support for creating KMS KeyRing resources Oct 26, 2017

mrparkers added 4 commits October 25, 2017 21:55

Allow for project in id to be autopopulated from config

2120b47

Throw error in import if project provider is not provided for locatio…

65cbadc

…n/name format

Merge branch 'master' into kms-keyring

024b45b

Consistent variable names

2791c74

danawillow reviewed Oct 26, 2017

View reviewed changes

mrparkers added 2 commits October 26, 2017 15:52

Use tabs in resource config instead of spaces

248d518

Remove "-x" suffix for docs

3fbf0bb

mrparkers force-pushed the kms-keyring branch from 1320d98 to 3fbf0bb Compare October 26, 2017 20:53

Set project attribute on import if different from the project config

6dc9035

danawillow approved these changes Oct 27, 2017

View reviewed changes

danawillow merged commit f2fc78d into hashicorp:master Oct 27, 2017

mrparkers mentioned this pull request Nov 9, 2017

Adds support for creating KMS CryptoKeys resources #692

Merged

mrparkers deleted the kms-keyring branch November 10, 2017 00:44

phausman mentioned this pull request Mar 19, 2018

terraform apply throws Error 409: alreadyExists when re-creating google_kms_key_ring #1214

Closed

ghost mentioned this pull request Jan 9, 2019

Question: No data source for KMS key/ring? #2847

Closed

luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request May 21, 2019

Modify docs on managed ssl cert. (hashicorp#518)

05be0da

Signed-off-by: Modular Magician <[email protected]>

ghost locked and limited conversation to collaborators Mar 29, 2020


		parent := createKmsResourceParentString(project, location)

		_, err = config.clientKms.Projects.Locations.KeyRings.


		parent := kmsResourceParentString(project, location)

		keyRing, err := config.clientKms.Projects.Locations.KeyRings.

Adds support for creating KMS KeyRing resources #518

Adds support for creating KMS KeyRing resources #518

Conversation

mrparkers commented Oct 3, 2017 • edited Loading

danawillow 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

Choose a reason for hiding this comment

mrparkers commented Oct 13, 2017

danawillow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrparkers commented Oct 21, 2017

danawillow 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

mrparkers commented Oct 24, 2017

mrparkers commented Oct 26, 2017 • edited Loading

danawillow 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

mrparkers commented Oct 26, 2017

danawillow commented Oct 26, 2017

mrparkers commented Oct 26, 2017

danawillow commented Oct 27, 2017

jasonjho commented Feb 12, 2018

danawillow commented Feb 12, 2018

ghost commented Mar 29, 2020

mrparkers commented Oct 3, 2017 •

edited

Loading

mrparkers commented Oct 26, 2017 •

edited

Loading