Data Sources for KMS Key Ring and Key #1235

ghost · 2019-01-11T00:43:12Z

[all]

Adding datasources to KMS key ring and crypto key for the sake of not having to import them once they've been created and making it easier to manage them in case the state needs to be rolled back after initial create.

This is a bit tricky because tests require creating non-destructible resources.
There's no yaml-based template for these yet so I just added plain go code.

The tests expect specific environment variables to run, so I was only able to verify by hardcoding my own account/etc.

I verified this by building my own copy of terraform-provider and running against my own project:

An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
 <= read (data resources)

Terraform will perform the following actions:

 <= data.google_kms_key_ring.kms-key-ring
      id:       <computed>
      location: "us-west1"
      name:     "tf-state-key-ring"


Plan: 0 to add, 0 to change, 0 to destroy.

Outputs of data:

Apply complete! Resources: 0 added, 0 changed, 0 destroyed.

Outputs:

2019-01-11T18:15:17.040-0700 [DEBUG] plugin.terraform-provider-google:                                                  
test-key = projects/my-project/locations/us-west1/keyRings/tf-state-key-ring/cryptoKeys/tf-state-key
test-ring = projects/my-project/locations/us-west1/keyRings/tf-state-key-ring

Related issue: hashicorp/terraform-provider-google#2847

[terraform]

[terraform-beta]

googlebot · 2019-01-11T00:43:16Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ghost · 2019-01-11T00:45:11Z

I signed it!

googlebot · 2019-01-11T00:45:14Z

CLAs look good, thanks!

ghost · 2019-01-11T00:46:14Z

third_party/terraform/data_sources/data_source_google_kms_key_ring.go

+	"github.com/hashicorp/terraform/helper/schema"
+)
+
+func dataSourceGoogleKmsKeyRing() *schema.Resource {


the resource-related tests/func for this do not have google in the name: don't know if you have a standard, but I just went with how other data sources are named

I think consistency with the other data sources is just fine.

ghost · 2019-01-15T17:22:30Z

third_party/terraform/data_sources/data_source_google_kms_crypto_key.go

+				Type:     schema.TypeList,
+				MaxItems: 1,
+				Computed: true,
+				Elem: &schema.Resource{


not sure how this is parsed in a data source

nat-henderson · 2019-01-16T02:50:31Z

Thanks for your contribution! Can you elaborate a little on your use case, or link a related issue?

chrisst · 2019-01-16T05:37:46Z

Looks like this duplicates work in hashicorp/terraform-provider-google#2804 as well as adding the key_ring data source

ghost · 2019-01-16T13:23:40Z

Dangit, missed that other PR :( I was told to look here if I wanted to make changes to the provider

I asked about this feature in this issue: hashicorp/terraform-provider-google#2847 ( I think the link is buried at the bottom of the original PR template)

My current use case is outline there and here's how I am getting about without these data sources currently: https://github.com/kierachell/k8s-in-gcp/blob/master/tf/setup.sh#L48

Basically, this would allow me to roll all of my terraform config forward and back in its entirety. (with resource blocks I run into errors since the 2nd+ create requires me importing resource that were made on the first run)

chrisst · 2019-01-16T20:18:10Z

No worries, I was mostly commenting to make sure the 2 get linked and followed up with appropriately.

chrisst

Thanks so much for all the work!

There are a couple of breaking issues related to the current schema. However I think they should be fixed if you inherit the schema instead of duplicating it. I've tried to review the rest of the changes not related to the schema (docs, etc) but I'll run the tests and finish the review when you've address the schema issues.

chrisst · 2019-01-16T19:58:51Z

third_party/terraform/data_sources/data_source_google_kms_crypto_key.go

+	return &schema.Resource{
+		Read: dataSourceGoogleKmsCryptoKeyRead,
+		Schema: map[string]*schema.Schema{
+			"name": {


This looks like you've duplicated the schema from the resource. This was how we used to do things but we've added a convenience method in our newer data sources that just inherits the schema from the resource so that we don't have to maintain it in two different places.

Check out the cloud functions data source for an example

chrisst · 2019-01-16T20:07:36Z

third_party/terraform/data_sources/data_source_google_kms_crypto_key.go

+	if err != nil {
+		return fmt.Errorf("Error reading CryptoKey: %s", err)
+	}
+	d.Set("key_ring", cryptoKeyId.KeyRingId.terraformId())


Similar to the comment above, you should reuse the Read method from the resource so you don't have to set all the properties manually. Again see cloud functions for an example.

chrisst · 2019-01-16T20:13:25Z

third_party/terraform/tests/data_source_google_kms_crypto_key_test.go

+}
+
+/*
+	This test should run in its own project, because KMS key rings and crypto keys are not deletable


Thanks for adding full tests! I recently added a test helper that will rely on a shared project for KMS tests that don't explicitly need to test key creation/deletion. See https://github.com/terraform-providers/terraform-provider-google/blob/master/google/resource_compute_disk_test.go#L319 for an example using this pattern. This helps us cut down on superfluous project creation which slows down our test suite and creates a bog of projects that are partially deleted.

chrisst · 2019-01-16T20:16:54Z

third_party/terraform/data_sources/data_source_google_kms_crypto_key.go

+						},
+						"protection_level": {
+							Type:         schema.TypeString,
+							Commpute:     true,


Inheriting from the resource will also help catch typos like this :)

🤦‍♂️ somehow that still built and ran... Thank you for all the suggestions, I'll get it updated tonight

nvm, found that my symlinks got all wonky with gomodules

chrisst · 2019-01-16T20:18:20Z

third_party/terraform/data_sources/data_source_google_kms_key_ring.go

+	"github.com/hashicorp/terraform/helper/schema"
+)
+
+func dataSourceGoogleKmsKeyRing() *schema.Resource {


I think consistency with the other data sources is just fine.

chrisst · 2019-01-16T20:24:19Z

third_party/terraform/website/docs/d/google_kms_crypto_key.html.markdown

+* `name` - (Required) The CryptoKey's name.
+    A CryptoKey’s name belonging to the specified Google Cloud Platform KeyRing and match the regular expression `[a-zA-Z0-9_-]{1,63}`
+
+* `key_ring` - (Required) The id of the Google Cloud Platform KeyRing to which the key shall belong.


Can you change this to "The self_link of..." This helps user's differentiate between using a name or self_link here when referencing a key_ring specified elsewhere in a terraform config. We also encourage people not to rely on the terraform id as it's not a consistent experience between resources as to what it actually contains.

chrisst · 2019-01-16T20:25:36Z

third_party/terraform/website/docs/d/google_kms_crypto_key.html.markdown

+# google\_kms\_crypto\_key
+
+Provides access to a Google Cloud Platform KMS CryptoKey. For more information see
+[the official documentation](https://cloud.google.com/kms/docs/object-hierarchy#cryptokey)


should this anchor tag be #key ?

Yes, it should and should be updated in the resource doc 😲

chrisst · 2019-01-16T21:06:53Z

third_party/terraform/website/docs/d/google_kms_crypto_key.html.markdown

+---
+layout: "google"
+page_title: "Google: google_kms_crypto_key"
+sidebar_current: "docs-google-kms-crypto-key-x"


Can you change the side_bar to docs-google-datasource-kms-crypto-key. Also feel free to add the 2 new docs files to website/google.erb . This is so that the links show up in the side bar, and when clicked they will highlight the current page in purple and the parent section based on a prefix match of the sidebar_current field (hence adding the -datasource- in there).
example here

chrisst · 2019-01-16T21:07:58Z

third_party/terraform/website/docs/d/google_kms_key_ring.html.markdown

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


rename to docs-google-datasource-kms-key-ring

ghost · 2019-01-17T16:09:00Z

@chrisst Thank you for detailed comments . I think I covered all of them; and double checked all the links and rebuilt the provider and it still works on my own project 😰 .

Let me know if there is anything else to update - I think I'm getting a hang of the build/test/run process 😄

PROOF:


Apply complete! Resources: 0 added, 0 changed, 0 destroyed.
2019/01/17 09:18:46 [DEBUG] plugin: waiting for all plugin processes to complete...

Outputs:

key = tf-state-key
ring = projects/proj/locations/us-central1/keyRings/tf-state-key-ring

chrisst

I have a couple more comments, but mostly around paring down the amount of work your tests need to do. Since you've already done a ton of work here and I think people want this I'm going to just push up my changes to the tests and get it running through our build system. Thanks again for adding this!

chrisst · 2019-01-17T17:35:44Z

third_party/terraform/data_sources/data_source_google_kms_crypto_key.go

+		return err
+	}
+
+	cryptoKeyId := &kmsCryptoKeyId{


This doesn't need to be a reference, you can just use a plain struct.

Suggested change

cryptoKeyId := &kmsCryptoKeyId{

cryptoKeyId := kmsCryptoKeyId{

Yeah, I saw it being a reference in other files and it didn't quite make sense but I followed the "common denominator"

chrisst · 2019-01-17T17:36:01Z

third_party/terraform/data_sources/data_source_google_kms_key_ring.go

+		return err
+	}
+
+	keyRingId := &kmsKeyRingId{


same

Suggested change

keyRingId := &kmsKeyRingId{

keyRingId := kmsKeyRingId{

chrisst · 2019-01-17T17:39:52Z

third_party/terraform/data_sources/data_source_google_kms_key_ring.go

+	}
+	d.SetId(keyRingId.terraformId())
+
+	err = resourceKmsKeyRingRead(d, meta)


Instead of capturing and returning the error or nil separately, you can just shortcut and return the call to read. This way if the call returns an error, this method will return that error and vice versa. So it's equivalent to what you have here.
eg:

return resourceKmsKeyRingRead(d, meta)

Gah, I left that from when I was trying to make the method fail and debug what the error would be, sorry.

chrisst · 2019-01-17T19:14:58Z

third_party/terraform/tests/data_source_google_kms_crypto_key_test.go

+		Providers: testAccProviders,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccDataSourceGoogleKmsCryptoKey_basic(projectId, projectOrg, projectBillingAccount, kms.KeyRing.Name, kms.CryptoKey.Name),


This is closer to what I imagined using bootstrapKMS would be, but you can still exclude a ton of the resource building so that this test only focuses on the datasource functionality (which is test that an existing resource can be imported).

I did wonder about that but it make more sense now that the read function is shared.

chrisst · 2019-01-17T19:32:03Z

third_party/terraform/tests/data_source_google_kms_crypto_key_test.go

+		Steps: []resource.TestStep{
+			{
+				Config: testAccDataSourceGoogleKmsCryptoKey_basic(projectId, projectOrg, projectBillingAccount, kms.KeyRing.Name, kms.CryptoKey.Name),
+				Check:  resource.TestMatchResourceAttr("data.google_kms_crypto_key", "name", regexp.MustCompile(kms.CryptoKey.Name)),


Suggested change

Check: resource.TestMatchResourceAttr("data.google_kms_crypto_key", "name", regexp.MustCompile(kms.CryptoKey.Name)),

Check: resource.TestMatchResourceAttr("data.google_kms_crypto_key.kms_crypto_key", "name", regexp.MustCompile(kms.CryptoKey.Name)),

chrisst · 2019-01-17T19:35:46Z

If you have any questions on the changes I've made I'm happy to explain them.

modular-magician · 2019-01-17T19:51:36Z

I am a robot that works on MagicModules PRs!

I built this PR into one or more PRs on other repositories, and when those are closed, this PR will also be merged and closed.
depends: hashicorp/terraform-provider-google-beta#359
depends: hashicorp/terraform-provider-google#2891
depends: modular-magician/inspec-gcp#78

Use kms bootstrap to avoid creating projects Simplify returns in the resource

Tracked submodules are build/terraform-beta build/terraform build/ansible build/inspec.

googlebot added the cla: no label Jan 11, 2019

googlebot added cla: yes and removed cla: no labels Jan 11, 2019

ghost commented Jan 11, 2019

View reviewed changes

ghost changed the title ~~[WIP] Data Sources for KMS Key Ring and Key~~ Data Sources for KMS Key Ring and Key Jan 12, 2019

ghost commented Jan 15, 2019

View reviewed changes

nat-henderson self-requested a review January 16, 2019 02:50

nat-henderson requested review from chrisst and removed request for nat-henderson January 16, 2019 18:26

chrisst suggested changes Jan 16, 2019

View reviewed changes

chrisst self-assigned this Jan 16, 2019

chrisst approved these changes Jan 17, 2019

View reviewed changes

modular-magician added downstream-generated inspec terraform-provider-google-beta terraform-provider-google automerged labels Jan 17, 2019

artemvovk added 3 commits January 17, 2019 21:38

key ring; and tests

c4bc0bf

real self link

5a56a9e

add crypto key

5eea16d

artemvovk and others added 7 commits January 17, 2019 21:38

docs and strange fields

da9ba72

working refactor; tests pass

be3598a

bootstrapped tests

2841a9b

update docs

31e1085

broken ref

a92393c

Update KMS data source tests

4640a2d

Use kms bootstrap to avoid creating projects Simplify returns in the resource

Update tracked submodules -> HEAD on Thu Jan 17 21:38:26 UTC 2019

4ec03d6

Tracked submodules are build/terraform-beta build/terraform build/ansible build/inspec.

modular-magician merged commit a10725b into GoogleCloudPlatform:master Jan 17, 2019

	cryptoKeyId := &kmsCryptoKeyId{
	cryptoKeyId := kmsCryptoKeyId{

	Check: resource.TestMatchResourceAttr("data.google_kms_crypto_key", "name", regexp.MustCompile(kms.CryptoKey.Name)),
	Check: resource.TestMatchResourceAttr("data.google_kms_crypto_key.kms_crypto_key", "name", regexp.MustCompile(kms.CryptoKey.Name)),

Data Sources for KMS Key Ring and Key #1235

Data Sources for KMS Key Ring and Key #1235

Conversation

ghost commented Jan 11, 2019 • edited by ghost Loading

[all]

[terraform]

[terraform-beta]

googlebot commented Jan 11, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

ghost commented Jan 11, 2019

googlebot commented Jan 11, 2019

ghost Jan 11, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nat-henderson commented Jan 16, 2019

chrisst commented Jan 16, 2019

ghost commented Jan 16, 2019 • edited by ghost Loading

chrisst commented Jan 16, 2019

chrisst 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jan 17, 2019 • edited by ghost Loading

chrisst 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

chrisst commented Jan 17, 2019

modular-magician commented Jan 17, 2019

ghost commented Jan 11, 2019 •

edited by ghost

Loading

ghost Jan 11, 2019 •

edited by ghost

Loading

ghost commented Jan 16, 2019 •

edited by ghost

Loading

ghost commented Jan 17, 2019 •

edited by ghost

Loading