Adds support for creating KMS CryptoKeys resources #692

amfarrell · 2017-11-06T17:42:13Z

No description provided.

amfarrell · 2017-11-06T17:47:57Z

Still need to add docs.

Pushing now so that I can see the result of running the acceptance test because I've not yet figured out access to a setup with permissions to run acceptance tests.

danawillow · 2017-11-07T20:50:25Z

Hey @amfarrell, looks like this doesn't build. Happy to run the tests for you once that's all squared away.

tragiclifestories · 2017-11-08T14:23:47Z

@danawillow This should now build.

Interface looks like

resource "google_kms_key_ring" "mykeyring" {
  location = "global"
  name     = "mykeyring"
  project  = "myproject"
}

resource "google_kms_crypto_key" "mykey" {
  name     = "mykey"
  key_ring = "${google_kms_key_ring.mykeyring.id}"
}

resource "google_kms_crypto_key" "myrotatingkey" {
  name            = "myrotatingkey"
  key_ring        = "${google_kms_key_ring.mykeyring.id}"
  # Next rotation time will be set automatically to now + rotation_period
  rotation_period = "100000s"                             
}

tragiclifestories · 2017-11-08T14:24:03Z

cc @mrparkers

mrparkers · 2017-11-09T00:55:05Z

google/resource_kms_crypto_key.go

+
+	log.Printf("[WARNING] KMS CryptoKey resources cannot be deleted from GCP. This CryptoKey %s will be removed from Terraform state, but will still be present on the server.", cryptoKeyId.cryptoKeyId())
+
+	d.SetId("")


We probably shouldn't do this until the very end of the function. clearCryptoKeyVersions could still error.

mrparkers · 2017-11-09T01:13:20Z

google/resource_kms_crypto_key.go

+		return err
+	}
+
+	log.Printf("[WARNING] KMS CryptoKey resources cannot be deleted from GCP. This CryptoKey %s will be removed from Terraform state, but will still be present on the server.", cryptoKeyId.cryptoKeyId())


It might be helpful to add the bit about destroying all of the CryptoKey's versions to this warning message.

mrparkers · 2017-11-09T01:21:24Z

google/resource_kms_crypto_key.go

+
+	if cryptoKeyIdWithoutProjectRegex.MatchString(id) {
+		if config.Project == "" {
+			return nil, fmt.Errorf("The default project for the provider must be set when using the `{location}/{cryptoKeyName}` id format.")


You're missing the keyRingName in the example format

mrparkers · 2017-11-09T01:21:54Z

google/resource_kms_crypto_key.go

+		}, nil
+	}
+
+	return nil, fmt.Errorf("Invalid CryptoKey id format, expecting `{projectId}/{locationId}/{KeyringName}/{cryptoKeyName}` or `{locationId}/{KeyringName}/{cryptoKeyName}.`")


Nitpick: Could you use keyRingName instead of KeyringName here?

mrparkers · 2017-11-09T01:25:51Z

google/resource_kms_crypto_key.go

+
+	if d.Get("purpose") == "" {
+		d.Set("purpose", "ENCRYPT_DECRYPT")
+	}


Is this needed? purpose isn't defined in your resource schema.

It's one of those weird ones you get in GCP (and AWS as well) where there is a parameter that accepts an enum with one possible value (see the REST API docs for CryptoKey). My feeling is that it is not necessary to support this parameter as an argument to the resource, and instead just set it 'on the quiet', until there is another purpose value defined, at which point it will in any case be necessary to add more validation. But I'm new to the Terraform codebases, so perhaps it is better to be explicit but have a default?

It looks like its only being set during import though, not creation. I think we should either set it in both places, or neither. Like you mentioned, there's only one possible value for this enum, so I think we should just get rid of it until more are added.

mrparkers · 2017-11-09T01:42:40Z

google/resource_kms_crypto_key_test.go

+resource "google_kms_crypto_key" "crypto_key" {
+	name     = "%s"
+    key_ring = "${google_kms_key_ring.key_ring.id}"
+    rotation_period = "100000s"


Could you fix the formatting here?

mrparkers · 2017-11-09T01:43:16Z

website/docs/r/google_kms_crypto_key.html.markdown

@@ -0,0 +1,63 @@
+---
+layout: "google"
+page_title: "Google: google_kms_key_ring"


Could you fix this? Should be google_kms_crypto_key

mrparkers · 2017-11-09T01:45:10Z

website/docs/r/google_kms_crypto_key.html.markdown

+  name            = "my-crypto-key"
+  key_ring        = "my_project/us-central1/my-key-ring"
+  rotation_period = "100000s"
+}


It might be nice to include a google_kms_key_ring resource here as well, so you could demonstrate using string interpolation for the key_ring field.

mrparkers · 2017-11-09T01:46:31Z

website/docs/r/google_kms_crypto_key.html.markdown

+```
+$ terraform import google_kms_key_ring.my_key_ring my-gcp-project/us-central1/my-key-ring/my-crypto-key
+
+$ terraform import google_kms_key_ring.my_key_ring us-central1/my-key-ring/my-crypto-key


The resource name should be google_kms_crypto_key.my_crypto_key

mrparkers · 2017-11-09T01:51:38Z

google/resource_kms_crypto_key_test.go

+			ExpectedCryptoKeyId: "projects/test-project/locations/us-central1/keyRings/test-key-ring/cryptoKeys/test-key-id",
+			Config:              &Config{Project: "test-project"},
+		},
+		"id is in location/keyRingName/CyptoKeyID format without project in config": {


Nitpick: For each of these test descriptions, could you use cryptoKeyName instead of CryptoKeyID? This way, the format will be consistent with the error messages in parseKmsCryptoKeyId

mrparkers · 2017-11-09T02:01:08Z

google/resource_kms_crypto_key.go

+			"rotation_period": &schema.Schema{
+				Type:     schema.TypeString,
+				Optional: true,
+				ForceNew: true,


I believe a CryptoKey's rotation period can be changed, or at least there's an API for it.

If this changed, Update would have to be implemented as well. This change probably doesn't need to hold up the PR, but @danawillow can make that call.

I did bump into this, actually, but decided to defer it.

On reflection, it might be worth doing given that you can't destroy the damn things properly...

I'm fine with deferring it to a follow-up PR!

mrparkers · 2017-11-09T02:03:42Z

google/resource_kms_crypto_key.go

+	d.SetId(cryptoKeyId.terraformId())
+
+	return []*schema.ResourceData{d}, nil
+}


It might be a good idea to set rotation_period if it exists.

Done - further up so comment isn't hidden ...

Also add more detail to warning message.

mrparkers · 2017-11-09T15:14:46Z

google/resource_kms_crypto_key_test.go

+
+resource "google_kms_crypto_key" "crypto_key" {
+	name     = "%s"
+    key_ring = "${google_kms_key_ring.key_ring.id}"


Could you fix this indentation?

mrparkers · 2017-11-09T15:14:55Z

google/resource_kms_crypto_key_test.go

+resource "google_kms_crypto_key" "crypto_key" {
+	name     = "%s"
+    key_ring = "${google_kms_key_ring.key_ring.id}"
+    rotation_period = "%s"


Here as well

mrparkers · 2017-11-09T16:00:01Z

@tragiclifestories sorry for the confusion about the indentation. Most of it was fine, there were just a couple of lines where spaces were used instead of tabs. It looks like they were all changed to use spaces, which will probably need to be changed back to tabs.

The only reason I'm asking is because I received similar comments on my PR 😉

mrparkers

Thanks for those changes, this looks good to me

tragiclifestories · 2017-11-09T17:37:10Z

And thanks for a very thorough review - I'm quite new to mucking about with terraform internals so it's all helpful!

danawillow

Thanks @amfarrell, this is super exciting!
And wow @mrparkers you rock, thanks for making my life a lot easier by reviewing this!

I have a bunch of comments, but most of them are pretty small- overall this looks really good.

danawillow · 2017-11-09T18:43:59Z

google/import_kms_crypto_key_test.go

+	"os"
+)
+
+func TestAccGoogleKmsCryptoKey_importBasic(t *testing.T) {


Can you add t.Parallel() (and then a newline) to the beginning of all tests?

danawillow · 2017-11-09T18:48:16Z

google/resource_kms_crypto_key.go

+			"rotation_period": &schema.Schema{
+				Type:     schema.TypeString,
+				Optional: true,
+				ForceNew: true,


I'm fine with deferring it to a follow-up PR!

danawillow · 2017-11-09T18:58:44Z

google/resource_kms_crypto_key.go

+	if err != nil {
+		return fmt.Errorf("Error reading CryptoKey: %s", err)
+	}
+


Generally we like to set as many fields as possible inside of Read, in order to catch out-of-band modifications.

Just curious, are you just looking for rotation_period here? Or would you also set name and key_ring by getting it from cryptoKeyId?

~~I went for rotation_period only, since I believe the other fields are immutable anyway. although the docs are unclear about this.~~ Actually, in relation to the import passthrough comment I realised that it would be better to set them all here anyway.

danawillow · 2017-11-09T19:02:40Z

google/resource_kms_crypto_key.go

+	return nil
+}
+
+func validateKmsCryptoKeyRotationPeriod(period string) error {


This could actually probably become a ValidateFunc, which would allow doing these checks during terraform plan instead of during terraform apply. Here's an example of what that would look like (this example has it inlined but it can certainly be a separate fn): https://github.com/terraform-providers/terraform-provider-google/blob/master/google/resource_dataproc_cluster.go#L39

danawillow · 2017-11-09T19:06:15Z

google/resource_kms_crypto_key.go

+	return nil, fmt.Errorf("Invalid CryptoKey id format, expecting `{projectId}/{locationId}/{KeyringName}/{cryptoKeyName}` or `{locationId}/{keyRingName}/{cryptoKeyName}.`")
+}
+
+func resourceKmsCryptoKeyImportState(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {


I think this is actually similar enough to the Read fn that you don't need a separate Import fn, but instead could use the passthrough one:

Importer: &schema.ResourceImporter{ State: schema.ImportStatePassthrough, },

Then inside the read function, you would do the same d.Set calls that you're doing here, plus an additional one for the rotation_period.

danawillow · 2017-11-09T19:12:08Z

google/resource_kms_crypto_key_test.go

+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckGoogleKmsCryptoKeyWasRemovedFromState("google_kms_crypto_key.crypto_key"),


Since your second TestStep calls this already, this is just duplicating that check- not wrong, but also not necessary.

danawillow · 2017-11-09T19:14:46Z

google/resource_kms_crypto_key_test.go

+
+		_, err = time.Parse(time.RFC3339Nano, getCryptoKeyResponse.NextRotationTime)
+
+		return err


It might be worth it to wrap this error, something like return fmt.Errorf("Error parsing NextRotationTime: %s", err)

danawillow · 2017-11-09T19:17:15Z

google/resource_kms_crypto_key_test.go

+		config := testAccProvider.Meta().(*Config)
+		gcpResourceUri := fmt.Sprintf("projects/%s/locations/%s/keyRings/%s/cryptoKeys/%s", projectId, location, keyRingName, cryptoKeyName)
+
+		response, _ := config.clientKms.Projects.Locations.KeyRings.CryptoKeys.CryptoKeyVersions.List(gcpResourceUri).Do()


I'd feel more comfortable if this error were handled: if we got a mysterious server error, then response.CryptoKeyVersions would (I believe) be empty, which means this test would succeed, even though we don't actually know that the versions were destroyed.

danawillow · 2017-11-09T19:18:48Z

google/resource_kms_crypto_key_test.go

+}
+
+resource "google_kms_key_ring" "key_ring" {
+	project  = "${google_project.acceptance.project_id}"


Let's also use ${google_project_services.acceptance.project} like in the basic test, so that we guarantee the API is enabled before trying to use it.

danawillow · 2017-11-09T19:18:56Z

google/resource_kms_crypto_key_test.go

+}
+
+resource "google_kms_key_ring" "key_ring" {
+	project  = "${google_project.acceptance.project_id}"


danawillow · 2017-11-09T19:23:46Z

Whoops sorry @tragiclifestories, I should be thanking you!

tragiclifestories · 2017-11-10T10:12:56Z

@danawillow I think that's everything ...

danawillow · 2017-11-13T18:27:09Z

google/resource_kms_crypto_key.go

+func parseKmsCryptoKeyId(id string, config *Config) (*kmsCryptoKeyId, error) {
+	parts := strings.Split(id, "/")
+
+	cryptoKeyIdRegex := regexp.MustCompile("^([a-z0-9-]+)/([a-z0-9-])+/([a-zA-Z0-9_-]{1,63})+/([a-zA-Z0-9_-]{1,63})$")


Sorry I didn't catch this earlier- shouldn't the third plus not be there since it already has the {1, 63}? I think that would end up essentially overriding the {1, 63} by allowing it to repeat some number of times

danawillow · 2017-11-13T18:27:44Z

google/resource_kms_crypto_key.go

+	parts := strings.Split(id, "/")
+
+	cryptoKeyIdRegex := regexp.MustCompile("^([a-z0-9-]+)/([a-z0-9-])+/([a-zA-Z0-9_-]{1,63})+/([a-zA-Z0-9_-]{1,63})$")
+	cryptoKeyIdWithoutProjectRegex := regexp.MustCompile("^([a-z0-9-])+/([a-zA-Z0-9_-]{1,63})+/([a-zA-Z0-9_-]{1,63})$")


danawillow · 2017-11-13T18:28:05Z

google/resource_kms_crypto_key.go

+}
+
+/*
+	Because KMS CryptoKey resources cannot be deleted on GCP, we are only going to remove it from state


nit: this comment has mismatched tabs/spaces for indentation, mind choosing one or the other?

danawillow · 2017-11-13T18:28:50Z

google/resource_kms_crypto_key.go

+	Name      string
+}
+
+// TODO: Add the info about rotation frequency and start time.


nit: can you be a bit more specific here as to what you're adding it to? I don't think it belongs in the id, which is the section of code this comment is living in.

danawillow · 2017-11-14T21:21:14Z

$ make testacc TEST=./google TESTARGS='-run=TestCryptoKey'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestCryptoKey -timeout 120m
=== RUN   TestCryptoKeyIdParsing
=== RUN   TestCryptoKeyNextRotationCalculation
=== RUN   TestCryptoKeyNextRotationCalculation_validation
--- PASS: TestCryptoKeyNextRotationCalculation_validation (0.00s)
--- PASS: TestCryptoKeyNextRotationCalculation (0.00s)
--- PASS: TestCryptoKeyIdParsing (0.01s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	0.167s

$ make testacc TEST=./google TESTARGS='-run=TestAccGoogleKmsCryptoKey'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccGoogleKmsCryptoKey -timeout 120m
=== RUN   TestAccGoogleKmsCryptoKey_importBasic
=== RUN   TestAccGoogleKmsCryptoKey_basic
=== RUN   TestAccGoogleKmsCryptoKey_rotation
--- PASS: TestAccGoogleKmsCryptoKey_importBasic (58.22s)
--- PASS: TestAccGoogleKmsCryptoKey_rotation (58.62s)
--- PASS: TestAccGoogleKmsCryptoKey_basic (61.67s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	61.851s

Looks great, thanks so much @tragiclifestories!

tragiclifestories · 2017-11-14T21:57:22Z

You're welcome, and thanks for the thorough review!

Somehow this snuck in to #692 but it doesn't look like it's actually used anywhere.

* Adds support for creating KMS CryptoKeys resources * Destroy extant CryptoKeyVersions on CryptoKey destroy * Inherit project, location etc from KeyRing in CryptoKey * Add function to calculate next rotation * Implement RotationPeriod parameter on CryptoKey * Import CryptoKey state * Uncommit my local acceptance test hacks * Docs for google_kms_crypto_key * Clear id at the end of CryptoKey deletion Also add more detail to warning message. * Fix parseCryptoKeyId error messages * Use correct naming in CryptoKeyIdParsing test * Check RotationPeriod is present in acceptance test * Rename variable in test function for consistency * Fix wrong resource name in cryptokey docs * Add KeyRing to CryptoKey doc example * Run test CryptoKey configs through terraform fmt * Don't set CryptoKey purpose in terraform state on import * Fix indentation in CryptoKey test * Parallelise CryptoKey tests * Set rotation_key on CryptoKey read * Move RotationPeriod validation to planning phase * Use import state passthrough for CryptoKey * Correct casing issues in test case names * Remove redundant CheckDestroy calls in CryptoKey tests * Add explanatory comment about extra test steps * More explicit error handling in CryptoKey tests * Explicit dependency on project services in test keyring configs * Clean up comments in cryptokey resource * Do not repeat in cryptokey id regexes

…ashicorp#689)" (hashicorp#692) This reverts commit e46f436.

ghost · 2019-09-12T13:50:16Z

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!

amfarrell force-pushed the kms-cryptokeys branch from 1c4023a to 23535fa Compare November 6, 2017 17:46

amfarrell and others added 2 commits November 8, 2017 14:07

Adds support for creating KMS CryptoKeys resources

eb497da

Destroy extant CryptoKeyVersions on CryptoKey destroy

b493bac

tragiclifestories force-pushed the kms-cryptokeys branch from 900fe2e to 2cb6a47 Compare November 8, 2017 14:07

James Turley added 3 commits November 8, 2017 14:21

Inherit project, location etc from KeyRing in CryptoKey

9de9eff

Add function to calculate next rotation

7ac65dc

Implement RotationPeriod parameter on CryptoKey

0aa931e

tragiclifestories force-pushed the kms-cryptokeys branch from 2cb6a47 to 0aa931e Compare November 8, 2017 14:21

tragiclifestories mentioned this pull request Nov 8, 2017

Feature Request: Support for KMS KeyRings / CryptoKeys #441

Closed

James Turley added 3 commits November 8, 2017 16:14

Import CryptoKey state

1450129

Uncommit my local acceptance test hacks

58ada43

Docs for google_kms_crypto_key

5da8276

mrparkers suggested changes Nov 9, 2017

View reviewed changes

mrparkers reviewed Nov 9, 2017

View reviewed changes

James Turley added 7 commits November 9, 2017 08:29

Clear id at the end of CryptoKey deletion

4d5f9bf

Also add more detail to warning message.

Fix parseCryptoKeyId error messages

42f6f24

Use correct naming in CryptoKeyIdParsing test

e21d734

Check RotationPeriod is present in acceptance test

1ff44c3

Rename variable in test function for consistency

7faf003

Fix wrong resource name in cryptokey docs

d9ebafa

Add KeyRing to CryptoKey doc example

cde0e24

mrparkers reviewed Nov 9, 2017

View reviewed changes

James Turley added 2 commits November 9, 2017 15:42

Run test CryptoKey configs through terraform fmt

0b49730

Don't set CryptoKey purpose in terraform state on import

41eca4b

mrparkers approved these changes Nov 9, 2017

View reviewed changes

danawillow suggested changes Nov 9, 2017

View reviewed changes

James Turley added 9 commits November 10, 2017 08:40

Parallelise CryptoKey tests

44ec6d6

Set rotation_key on CryptoKey read

d361401

Move RotationPeriod validation to planning phase

adb4d54

Use import state passthrough for CryptoKey

56bb79d

Correct casing issues in test case names

f78800b

Remove redundant CheckDestroy calls in CryptoKey tests

dfe7413

Add explanatory comment about extra test steps

9f1857a

More explicit error handling in CryptoKey tests

18b4791

Explicit dependency on project services in test keyring configs

90a04a4

mrparkers mentioned this pull request Nov 13, 2017

Cloud KMS secret data source #495

Closed

danawillow reviewed Nov 13, 2017

View reviewed changes

James Turley added 2 commits November 14, 2017 09:11

Clean up comments in cryptokey resource

345e299

Do not repeat in cryptokey id regexes

d2e13b2

danawillow approved these changes Nov 14, 2017

View reviewed changes

danawillow merged commit 56d633a into hashicorp:master Nov 14, 2017

danawillow added a commit that referenced this pull request Nov 16, 2017

Delete encryptor-pod.yml

2618f71

Somehow this snuck in to #692 but it doesn't look like it's actually used anywhere.

danawillow mentioned this pull request Nov 16, 2017

Delete encryptor-pod.yml #749

Merged

danawillow added a commit that referenced this pull request Nov 16, 2017

Delete encryptor-pod.yml (#749)

b7177e0

Somehow this snuck in to #692 but it doesn't look like it's actually used anywhere.

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

Revert "Allow project to be specified when importing instance groups (h…

5308278

…ashicorp#689)" (hashicorp#692) This reverts commit e46f436.

amfarrell deleted the kms-cryptokeys branch September 12, 2019 10:46

ghost locked and limited conversation to collaborators Sep 12, 2019


		log.Printf("[WARNING] KMS CryptoKey resources cannot be deleted from GCP. This CryptoKey %s will be removed from Terraform state, but will still be present on the server.", cryptoKeyId.cryptoKeyId())

		d.SetId("")


		_, err = time.Parse(time.RFC3339Nano, getCryptoKeyResponse.NextRotationTime)

		return err

Adds support for creating KMS CryptoKeys resources #692

Adds support for creating KMS CryptoKeys resources #692

Conversation

amfarrell commented Nov 6, 2017

amfarrell commented Nov 6, 2017

danawillow commented Nov 7, 2017

tragiclifestories commented Nov 8, 2017

tragiclifestories commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrparkers Nov 9, 2017 • edited Loading

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 Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrparkers Nov 9, 2017 • edited Loading

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 Nov 9, 2017 • edited Loading

mrparkers left a comment

Choose a reason for hiding this comment

tragiclifestories commented Nov 9, 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

tragiclifestories Nov 10, 2017 • edited Loading

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

danawillow commented Nov 9, 2017

tragiclifestories commented Nov 10, 2017

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

danawillow commented Nov 14, 2017

tragiclifestories commented Nov 14, 2017

ghost commented Sep 12, 2019

mrparkers Nov 9, 2017 •

edited

Loading

mrparkers Nov 9, 2017 •

edited

Loading

mrparkers Nov 9, 2017 •

edited

Loading

mrparkers commented Nov 9, 2017 •

edited

Loading

tragiclifestories Nov 10, 2017 •

edited

Loading