Add support for Google Cloud IoT Core registry #970

Ebolon · 2018-01-17T18:56:16Z

This pull request adds support for the Google Cloud IoT Core registry. The config structure reflects the structure of the registry API. The downside is a lot of nesting in the config. But hopefully it allows a easy extension if the API evolves.

nat-henderson · 2018-01-17T20:58:08Z

google/provider.go

@@ -88,6 +88,7 @@ func Provider() terraform.ResourceProvider {
 			"google_bigtable_instance":                     resourceBigtableInstance(),
 			"google_bigtable_table":                        resourceBigtableTable(),
 			"google_cloudfunctions_function":               resourceCloudFunctionsFunction(),
+			"google_cloudiot_registry":                     resourceCloudiotRegistry(),


I prefer CloudIoT for capitalization consistency - I think most monospace fonts render the I sufficiently differently from an l that it probably won't be an issue, and this is what https://github.com/golang/go/wiki/CodeReviewComments#initialisms requests.

Looks much better 👍 Done.

nat-henderson · 2018-01-19T18:32:07Z

google/resource_cloudiot_registry_test.go

+    {
+      "public_key_certificate" = {
+        format      = "X509_CERTIFICATE_PEM"
+        certificate = "-----BEGIN CERTIFICATE-----\nMIICnjCCAYYCCQC/5gx7LgJFqTANBgkqhkiG9w0BAQsFADARMQ8wDQYDVQQDDAZ1\nbnVzZWQwHhcNMTgwMTEyMjAxMzQzWhcNMjMwMTExMjAxMzQzWjARMQ8wDQYDVQQD\nDAZ1bnVzZWQwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDStfQvJzmN\nCYLSWpwvTmyCKn8t19cfWZ69wFaB3OSglxXgYe3w9An0QHybDpKITt61PpfsKov3\nEcnzH5IA+Ox+4jUppBL1mSkO/BWig+sd1dG7pQMbGi4nMxW704A0PRUaNIOarOlR\nrNUJZQrsghkMjLayCTJ2HISBBiPnKKB3f3KCc9sDhj2Z7zy7HfeW0apZ1m6xAQCC\neSZNW0IyGIYKTd9F7HEJFzOWg9JHvabbciBEcFWKGVzM8nQr1q8KU8Xi3iN2mpNK\nJkbRLNnqKhvjPyIZ4s4cDSEZN1/OaGQ4XP2mvU03+4UAoMPoJ8IczBKTB0mFxfX8\nlDZZa5IWU9sNAgMBAAEwDQYJKoZIhvcNAQELBQADggEBAHnkTIghRj/cerR9ctji\nkancnjlsdNEuPiVpMj+SOtOH8cvlgl0oWG6segYTVzk4VEHlq3POB67Yjoz829XM\nCEgUxSqGvDrQ7IaPLPryYy8o5azMLnEZDr+Yd6CUKr/pUZzJoZxHj7z3iqeQZnMW\nS6kb6HYvG5PKlJ7+JUIKLou0RQmaM9BQ0Nln/YDRRIerD0MY9k7No2ZEDbywZqQK\nGRIqT+BlN84oHOR44h2RqWhn9O50tkbcmAIKgmeg/mxwmeAm/6hQ8VrOhDHqsFdT\nzh2l6IeCl8EF8MjNrFRcQx21TTqeU6vGIPgM3E0k8PQUc+s+lir8UFsIzKaOFsIh\nuKU=\n-----END CERTIFICATE-----\n"


Consider committing this as a file to google/test-fixtures instead.

nat-henderson · 2018-01-19T18:33:34Z

google/resource_cloudiot_registry_test.go

+	}
+}
+
+func testAccCloudiotRegistryUpdate(n string) resource.TestCheckFunc {


This seems elaborate - I think it only tests that the state matches the config, which is implicitly part of a test that has config. If the state doesn't match the config post-update, the test will fail even without this method, and so removing this method reduces the difficulty of maintenance.

👍 Have removed this function. I was not aware of this internal test.

Oh, I apologize but I was wrong about this - this is true only when there's an importstate step on the test. If you can add one of those in, these tests will be easier to maintain, but if that's a hassle for unforeseen reasons you can definitely go back to this method. My bad!

nat-henderson · 2018-01-19T18:34:17Z

website/docs/r/cloudiot_registry.html.markdown

+    {
+      public_key_certificate = {
+        format      = "X509_CERTIFICATE_PEM"
+        certificate = "<CERTIFICATE>"


Wouldn't hurt to do, maybe, "${file("foo.pem")}" or something like that so the example is more valid?

Yep! Sounds much better!

nat-henderson · 2018-01-19T18:36:00Z

google/resource_cloudiot_registry.go

+				Type:     schema.TypeList,
+				Optional: true,
+				ForceNew: false,
+				MaxItems: 10,


Where does the max of 10 come from?

I found the max 10 in the documentation of DeviceRegistry for the credentials field: "No more than 10 credentials can be bound to a single registry at a time.". Hope I got it right.

nat-henderson · 2018-01-19T18:40:29Z

google/resource_cloudiot_registry.go

+
+	parent := fmt.Sprintf("projects/%s/locations/%s", project, region)
+
+	deviceRegistry := expandDeviceRegistry(d)


Here, for instance, createDeviceRegistry would be more accurate.

nat-henderson · 2018-01-19T18:42:13Z

google/resource_cloudiot_registry.go

+		}
+		d.Set("event_notification_configs", eventConfigs)
+	}
+	// to keep the data model lean only changes are pushed if not the default is


Could you clean up the wording in this comment a bit?

nat-henderson · 2018-01-19T18:43:05Z

google/resource_cloudiot_registry.go

+
+	d.Set("name", res.Id)
+
+	if res.EventNotificationConfigs != nil {


If they are nil, is it right to clear them from the ResourceData object? To stay up to date with, for instance, a deleted config from the console?

Yes I missed that. Now I reset first everything to nil.

nat-henderson · 2018-01-19T18:43:57Z

google/resource_cloudiot_registry.go

+	return nil
+}
+
+func resourceCloudiotRegistryStateImporter(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {


Need to see an import test.

nat-henderson · 2018-01-19T18:44:41Z

google/resource_cloudiot_registry.go

+		return nil, err
+	}
+
+	id := fmt.Sprintf("projects/%s/locations/%s/registries/%s", project, region, d.Id())


Problem - in this mode, the user must configure the provider such that the default region and project are the right ones - d.Id() is the only thing you'll have going into the import.

Not thought about that. I now ask for full path projects/{project}/locations/{region}/registries/{name} as id. (see also registry get API).

- Removed default parameters from schema. - Added import test. - Refactored naming of methods (expand->build) - Before read set all parameters of model to nil. - Moved cert data to external file.

…fied regex in import.

Ebolon · 2018-01-21T17:04:09Z

Thank you very much for your feedback @ndmckinley! Hope I got now everything right. How does it look now?

PS: I added a validation function for the registry id. I hope the function name is ok. This can be reuses later for a device id (see: Creating or editing a device).

nat-henderson · 2018-01-22T19:21:24Z

google/resource_cloudiot_registry.go

+		return handleNotFoundError(err, d, fmt.Sprintf("Registry %q", name))
+	}
+
+	d.Set("name", res.Id)


A comment here which explains that you're speculatively setting all attributes to nil and updating the ones you find later might help.

After some testing I saw that all the nil checks are not necessary. Hope my changes now makes it more readable.
Just to make sure we have the same thing in mind when we read the code:
This is

resource "google_cloudiot_registry" "hello" { name = "hello" }

comparable with

{ "id": "hello", "name": "projects/<...>/locations/<...>/registries/hello", "mqttConfig": { "mqttEnabledState": "MQTT_ENABLED" }, "stateNotificationConfig": { }, "httpConfig": { "httpEnabledState": "HTTP_ENABLED" } }

because the code ignores default state, mqtt, http config:

nat-henderson · 2018-01-22T19:25:46Z

google/validation.go

@@ -107,3 +109,16 @@ func validateRFC1035Name(min, max int) schema.SchemaValidateFunc {

 	return validateRegexp(fmt.Sprintf("^"+RFC1035NameTemplate+"$", min-2, max-2))
 }
+
+func validateCloudIoTId(v interface{}, k string) (warnings []string, errors []error) {


nitpick: "ID" is capitalized when it's short for "identifier"
https://github.com/golang/go/wiki/CodeReviewComments#initialisms

nat-henderson · 2018-01-22T19:25:47Z

google/validation.go

@@ -18,6 +19,7 @@ const (
 	SubnetworkLinkRegex = "projects/(" + ProjectRegex + ")/regions/(" + RegionRegex + ")/subnetworks/(" + SubnetworkRegex + ")$"

 	RFC1035NameTemplate = "[a-z](?:[-a-z0-9]{%d,%d}[a-z0-9])"
+	CloudIoTIdRegex     = "[a-zA-Z][-a-zA-Z0-9._+~%]{2,254}"


Is this well-tested? The formatter is trying to complain about the % as if it's a format-string specifier and I'm pretty sure it's a formatter bug and not a bug in your code. :)

Unit test is now included. Found bug and yep this seems to be a formatter bug :-)

nat-henderson · 2018-01-22T19:31:05Z

google/resource_cloudiot_registry.go

+	if err != nil {
+		return err
+	}
+	d.SetId(res.Name)


It can be helpful to do d.SetId(name) before the create call, then do d.SetId(nil) in the event of an error - that way, if the process is killed during the request, the state file will include the resource, and we'll be able to do a Read() on refresh to determine if the call ever actually succeeded.

nat-henderson · 2018-01-22T19:31:31Z

It's looking pretty good to me - I made a mistake in the earlier review, sorry for the back-and-forth, but other than that, and these small changes, I'm happy with it.

- Removed unnecessary nil checks in read function. - Added unit test for cloudiot id validation func. - Added start/end delimiter to cloud iot id regex. - Fixed bug were certficates were missing in read. - Set id to empty if creating a registry fails.

Ebolon · 2018-01-22T22:52:57Z

Have implemented the requested changes. Found a bug where the certificates were not properly read into the state, because I forgot the public_key_certificate key. Ready for review.

nat-henderson · 2018-01-22T23:23:17Z

Great - I'm running the tests now and if they pass, I'll merge it.

nat-henderson · 2018-01-22T23:48:59Z

Hey there, looks like we're getting errors from the tests, which seem to assume a particular setup. Specifically, for TestAccCloudIoTRegistryUpdate we're getting

* google_cloudiot_registry.foobar: googleapi: Error 403: Topic 'projects/<OUR PROJECT NAME>/topics/psregistry-test-devicestatus-pqzo18advm' was specified, but user '[email protected]' does not have 'pubsub.topics.publish' permission to publish events to the topic, or the topic doesn't exist. Make sure that the topic exists, and edit the topic permissions to add the 'Pub/Sub Publisher' role https://cloud.google.com/pubsub/docs/access_control#tbl_roles to the '[email protected]' account. You can also run the following command to add publish permission to all topics in your project:\n\ngcloud projects add-iam-policy-binding $PROJECT --member=serviceAccount:[email protected] --role=roles/pubsub.publisher, forbidden

Ebolon · 2018-01-23T12:16:53Z

The test creates two pubsub topics and sets no polices. I were not aware of this restricted test env. My setup IAM Role has those simple settings:

I am trying to reproduce this on my side.

--

I found that the terraform schema validation did not work if certificate in public_key_certificate is missing, although it is marked as required. Should I catch those errors or wait for terraform to catch those errors?

PS: Sorry for last minute changes. I played a bit more with the API. UNSPECIFIED_PUBLIC_KEY_CERTIFICATE_FORMAT only results in an error and MQTT_STATE_UNSPECIFIED or MQTT_STATE_UNSPECIFIED are never returned. (Only _ENABLEDis returned) Just thought I remove those before it generates some confusion.

nat-henderson · 2018-01-23T19:09:44Z

The important thing is that the test passes without any special configuration on our side ahead of time - ideally we wouldn't need to manually create a service account and grant it permissions. The best case would be for you to put that service account creation and permission grants into the test terraform config! But if that doesn't work out, let me know how to set it up and I'll see what I can do. :)

…ervice to publish to pubsub.

nat-henderson · 2018-01-23T21:10:24Z

Thanks! I'll run the tests again.

Ebolon · 2018-01-23T21:11:15Z

Ouch! This was a RTFM moment. Got it fixed now :-)

nat-henderson · 2018-01-23T21:19:55Z

Hey, wonderful, it passes! :) Last thing - the import test is obsolete since your other tests do an import now, and you can remove it without decreasing coverage. After that, I'll merge it. :)

Ebolon · 2018-01-23T21:26:50Z

Do you mean TestAccCloudIoTRegistry_import?

nat-henderson · 2018-01-23T23:42:20Z

Yes, you can remove the entire import_*_test.go file.

Ebolon · 2018-01-24T08:47:45Z

Done.

nat-henderson · 2018-01-24T21:03:15Z

Thank you!

ghost · 2020-03-29T14:36: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!

Add support for Google Cloud IoT Core registry.

2d9f9d8

nat-henderson self-requested a review January 17, 2018 20:46

nat-henderson suggested changes Jan 19, 2018

View reviewed changes

Ebolon added 2 commits January 20, 2018 17:36

Fixes following issues:

0b055e9

- Removed default parameters from schema. - Added import test. - Refactored naming of methods (expand->build) - Before read set all parameters of model to nil. - Moved cert data to external file.

Add validation for registry id, removed diff supress in name and modi…

a45e5a5

…fied regex in import.

nat-henderson reviewed Jan 22, 2018

View reviewed changes

Ebolon and others added 3 commits January 22, 2018 23:27

This change includes:

1f60d2f

- Removed unnecessary nil checks in read function. - Added unit test for cloudiot id validation func. - Added start/end delimiter to cloud iot id regex. - Fixed bug were certficates were missing in read. - Set id to empty if creating a registry fails.

Merge branch 'master' into feature-cloudiot-registry

dd7128a

Reformat code.

eb2fac9

Do not save state_notification_config topic if it is empty.

27ea382

Ebolon added 2 commits January 23, 2018 13:01

Removed "unspecified" enum values.

41c1133

Removed 'unspecified' values from doc.

7df130b

Added google_project_iam_binding in test case to allow the iotcloud s…

b8a0cd8

…ervice to publish to pubsub.

nat-henderson approved these changes Jan 23, 2018

View reviewed changes

Removed obsolete import test. Import already covered by other unit test.

b1bcae8

nat-henderson merged commit fec2f0b into hashicorp:master Jan 24, 2018

chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018

Add support for Google Cloud IoT Core registry (hashicorp#970)

411a268

ghost locked and limited conversation to collaborators Mar 29, 2020


		parent := fmt.Sprintf("projects/%s/locations/%s", project, region)

		deviceRegistry := expandDeviceRegistry(d)


		d.Set("name", res.Id)

		if res.EventNotificationConfigs != nil {

Add support for Google Cloud IoT Core registry #970

Add support for Google Cloud IoT Core registry #970

Conversation

Ebolon commented Jan 17, 2018

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

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

Ebolon commented Jan 21, 2018

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

nat-henderson commented Jan 22, 2018

Ebolon commented Jan 22, 2018

nat-henderson commented Jan 22, 2018

nat-henderson commented Jan 22, 2018 • edited Loading

Ebolon commented Jan 23, 2018 • edited Loading

nat-henderson commented Jan 23, 2018

nat-henderson commented Jan 23, 2018

Ebolon commented Jan 23, 2018 • edited Loading

nat-henderson commented Jan 23, 2018

Ebolon commented Jan 23, 2018 • edited Loading

nat-henderson commented Jan 23, 2018

Ebolon commented Jan 24, 2018

nat-henderson commented Jan 24, 2018

ghost commented Mar 29, 2020

nat-henderson commented Jan 22, 2018 •

edited

Loading

Ebolon commented Jan 23, 2018 •

edited

Loading

Ebolon commented Jan 23, 2018 •

edited

Loading

Ebolon commented Jan 23, 2018 •

edited

Loading