Add support for Shielded VMs to compute_instance and compute_instance_template (#3209) #3531

mlauter · 2019-05-01T20:47:46Z

Adds support for Shielded VMs to compute instances and instance templates.

Test output:

make testacc TEST=./google TESTARGS='-run=TestAccComputeInstance_shieldedVmConfig'
==> Checking source code against gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google -v -run=TestAccComputeInstance_shieldedVmConfig -timeout 240m -ldflags="-X=github.com/terraform-providers/terraform-provider-google/version.ProviderVersion=acc"
=== RUN   TestAccComputeInstance_shieldedVmConfig
=== PAUSE TestAccComputeInstance_shieldedVmConfig
=== RUN   TestAccComputeInstance_shieldedVmConfigSkip
=== PAUSE TestAccComputeInstance_shieldedVmConfigSkip
=== CONT  TestAccComputeInstance_shieldedVmConfig
=== CONT  TestAccComputeInstance_shieldedVmConfigSkip
--- PASS: TestAccComputeInstance_shieldedVmConfigSkip (237.78s)
--- PASS: TestAccComputeInstance_shieldedVmConfig (248.79s)
PASS
ok      github.com/terraform-providers/terraform-provider-google/google 248.853s

make testacc TEST=./google TESTARGS='-run=TestAccComputeInstance_basic'
==> Checking source code against gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google -v -run=TestAccComputeInstance_basic -timeout 240m -ldflags="-X=github.com/terraform-providers/terraform-provider-google/version.ProviderVersion=acc"
=== RUN   TestAccComputeInstance_basic1
=== PAUSE TestAccComputeInstance_basic1
=== RUN   TestAccComputeInstance_basic2
=== PAUSE TestAccComputeInstance_basic2
=== RUN   TestAccComputeInstance_basic3
=== PAUSE TestAccComputeInstance_basic3
=== RUN   TestAccComputeInstance_basic4
=== PAUSE TestAccComputeInstance_basic4
=== RUN   TestAccComputeInstance_basic5
=== PAUSE TestAccComputeInstance_basic5
=== CONT  TestAccComputeInstance_basic1
=== CONT  TestAccComputeInstance_basic4
=== CONT  TestAccComputeInstance_basic3
=== CONT  TestAccComputeInstance_basic5
=== CONT  TestAccComputeInstance_basic2
--- PASS: TestAccComputeInstance_basic1 (156.77s)
--- PASS: TestAccComputeInstance_basic4 (239.65s)
--- PASS: TestAccComputeInstance_basic3 (271.10s)
--- PASS: TestAccComputeInstance_basic5 (292.26s)
--- PASS: TestAccComputeInstance_basic2 (187.23s)
PASS
ok      github.com/terraform-providers/terraform-provider-google/google 344.041s

make testacc TEST=./google TESTARGS='-run=TestAccComputeInstanceTemplate_shieldedVmConfig'
==> Checking source code against gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google -v -run=TestAccComputeInstanceTemplate_shieldedVmConfig -timeout 240m -ldflags="-X=github.com/terraform-p
roviders/terraform-provider-google/version.ProviderVersion=acc"
=== RUN   TestAccComputeInstanceTemplate_shieldedVmConfig
=== PAUSE TestAccComputeInstanceTemplate_shieldedVmConfig
=== RUN   TestAccComputeInstanceTemplate_shieldedVmConfigSkip
=== PAUSE TestAccComputeInstanceTemplate_shieldedVmConfigSkip
=== CONT  TestAccComputeInstanceTemplate_shieldedVmConfig
=== CONT  TestAccComputeInstanceTemplate_shieldedVmConfigSkip
--- PASS: TestAccComputeInstanceTemplate_shieldedVmConfigSkip (13.38s)
--- PASS: TestAccComputeInstanceTemplate_shieldedVmConfig (15.62s)
PASS
ok      github.com/terraform-providers/terraform-provider-google/google 15.722s

make testacc TEST=./google TESTARGS='-run=TestAccComputeInstanceTemplate_basic'
==> Checking source code against gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google -v -run=TestAccComputeInstanceTemplate_basic -timeout 240m -ldflags="-X=github.com/terraform-providers/terraform-provider-google/version.ProviderVersion=acc"
=== RUN   TestAccComputeInstanceTemplate_basic
=== PAUSE TestAccComputeInstanceTemplate_basic
=== CONT  TestAccComputeInstanceTemplate_basic
--- PASS: TestAccComputeInstanceTemplate_basic (11.32s)
PASS
ok      github.com/terraform-providers/terraform-provider-google/google 11.372s

make testacc TEST=./google TESTARGS='-run=TestAccComputeInstanceFromTemplate_basic'
==> Checking source code against gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google -v -run=TestAccComputeInstanceFromTemplate_basic -timeout 240m -ldflags="-X=github.com/terraform-providers/terraform-provider-google/version.ProviderVersion=acc"
=== RUN   TestAccComputeInstanceFromTemplate_basic
=== PAUSE TestAccComputeInstanceFromTemplate_basic
=== CONT  TestAccComputeInstanceFromTemplate_basic
--- PASS: TestAccComputeInstanceFromTemplate_basic (272.02s)
PASS
ok      github.com/terraform-providers/terraform-provider-google/google 272.069s

google/resource_compute_instance.go

rileykarson

Made a first pass with some comments- do you mind rebasing / merging to resolve the merge conflicts GitHub is seeing as well?

google/resource_compute_instance.go

rileykarson · 2019-05-10T16:12:57Z

google/resource_compute_instance_template_test.go

+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckComputeInstanceTemplateDestroy,
+		Steps: []resource.TestStep{


Do you mind adding an import step to this test like the one above?

rileykarson · 2019-05-10T16:13:26Z

google/resource_compute_instance_test.go

+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckComputeInstanceDestroy,
+		Steps: []resource.TestStep{


Do you mind adding an import step here like in the test above?

google/resource_compute_instance_test.go

google/compute_instance_helpers.go

rileykarson

Looking great! I have a few comments about defaults, I think we want to keep in line with API defaults. This is mostly because Terraform won't send schema-defined defaults when a field is undefined so for consistency we need to define exactly what the API does.

In addition, I think we can eliminate a few test cases. Since tests on the provider are acceptance-level and take a while to run (At 8x parallelism a run takes 4 hours today!) we tend towards testing minimum novel behaviour instead of testing as thoroughly as you have, unfortunately.

google/resource_compute_instance.go

rileykarson · 2019-05-13T18:34:40Z

google/resource_compute_instance_template_test.go

+	})
+}
+
+func TestAccComputeInstanceTemplate_shieldedVmConfigSkip(t *testing.T) {


I'm inclined to omit this test since it's nearly identical to https://github.com/terraform-providers/terraform-provider-google/blob/b1e181e09ffc6492d17ccb220f0c03ccd6d39ef3/google/resource_compute_instance_template_test.go#L18. You can add testAccCheckComputeInstanceTemplateHasShieldedVmConfig to that test if you'd like, though.

I initially wanted to add the lacksShieldedVmConfig check to the basic test. That would just make sure that omitting shieldedVmConfig from the instance config would mean the block was absent rather than shieldedVmConfig {} which would result in an api error.

The issue is that TestAcccomputeInstance(Template)_basic uses compute.Instance and not compute.BetaInstance. This actually was a bit of a point of confusion for me. ShieldedVmConfig settings are available in the v1 REST api (under the name ShieldedInstanceConfig, but neither ShieldedInstanceConfig nor ShieldedVmConfig are available in the generated go structs for the v1 api. I wasn't sure where that left the provider in terms of including the block in the regular vs beta provider.

I'd be happy with it being added to any test that's already using the beta instance, or changing that test to use the beta instance. They're pretty indistinguishable.

I hadn't noticed that the field was renamed from Beta -> GA, how do you feel about using the GA naming in the Terraform schema name for the block? That way we'll be aligned with the finalised naming of the feature.

rileykarson · 2019-05-13T18:36:16Z

google/resource_compute_instance_template_test.go

+	})
+}
+
+func TestAccComputeInstanceTemplate_shieldedVmConfig3(t *testing.T) {


I think cases 3 and 4 are redundant with 1 and 2, they're testing how Terraform's defaults work more than they're testing novel resource behaviour. Let me know if you think I'm missing something, though.

rileykarson · 2019-05-13T18:37:12Z

google/resource_compute_instance_template.go

@@ -389,6 +389,37 @@ func resourceComputeInstanceTemplate() *schema.Resource {
 				},
 			},

+			"shielded_vm_config": {


The suggestions for compute instance apply here as well, unless this resource behaves differently in your experience.

rileykarson · 2019-05-13T18:38:09Z

website/docs/r/compute_instance.html.markdown

@@ -134,6 +134,9 @@ The following arguments are supported:

 * `tags` - (Optional) A list of tags to attach to the instance.

+* `shielded_vm_config` - (Optional) Enable [Shielded VM](https://cloud.google.com/security/shielded-cloud/shielded-vm) on this instance. Shielded VM provides verifiable integrity to prevent against malware and rootkits. Defaults to disabled. Structure is documented below.


Nice! I was gonna ask if we could list the images, but you already did.

rileykarson · 2019-05-13T18:38:57Z

website/docs/r/compute_instance.html.markdown

@@ -134,6 +134,9 @@ The following arguments are supported:

 * `tags` - (Optional) A list of tags to attach to the instance.

+* `shielded_vm_config` - (Optional) Enable [Shielded VM](https://cloud.google.com/security/shielded-cloud/shielded-vm) on this instance. Shielded VM provides verifiable integrity to prevent against malware and rootkits. Defaults to disabled. Structure is documented below.


(also, remember to update the defaults in these docs too)

google/resource_compute_instance_test.go

…_template (hashicorp#3209)

google/resource_compute_instance_test.go

mlauter · 2019-05-14T16:56:06Z

Latest update:

removes redundant tests
moves lacksShieldedVmConfig check to a pre-existing test in instance_test and instance_template_test
renames shielded_vm_config to shielded_instance_config to match the v1 REST api (but only for the config key, i left the variables names as is to match the google struct name, but happy to revisit)
Updates defaults to match api defaults (and docs)
Adds computed property

Thanks so much for being so patient and helpful!

rileykarson

Thanks @mlauter, great work!

I'll upstream these changes to our code generator (https://github.com/GoogleCloudPlatform/magic-modules) so it targets google-beta as well and then merge this PR.

ghost · 2019-06-16T13:51:19Z

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 added size/xl documentation labels May 1, 2019

mlauter commented May 1, 2019

View reviewed changes

google/resource_compute_instance.go Show resolved Hide resolved

mlauter commented May 1, 2019

View reviewed changes

google/resource_compute_instance.go Show resolved Hide resolved

rileykarson self-requested a review May 10, 2019 15:51

rileykarson reviewed May 10, 2019

View reviewed changes

mlauter force-pushed the master branch from 42b662e to b1e181e Compare May 13, 2019 16:26

mlauter commented May 13, 2019

View reviewed changes

google/compute_instance_helpers.go Show resolved Hide resolved

rileykarson reviewed May 13, 2019

View reviewed changes

Add support for Shielded VMs to compute_instance and compute_instance…

cae20ba

…_template (hashicorp#3209)

mlauter force-pushed the master branch from b1e181e to cae20ba Compare May 14, 2019 16:49

mlauter commented May 14, 2019

View reviewed changes

google/resource_compute_instance_test.go Show resolved Hide resolved

rileykarson approved these changes May 15, 2019

View reviewed changes

rileykarson mentioned this pull request May 15, 2019

Upstream compute instance shielded VM changes GoogleCloudPlatform/magic-modules#1769

Merged

rileykarson merged commit bc927db into hashicorp:master May 16, 2019

rileykarson mentioned this pull request May 16, 2019

Add support for Shielded VMs #3209

Closed

ghost locked and limited conversation to collaborators Jun 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Shielded VMs to compute_instance and compute_instance_template (#3209) #3531

Add support for Shielded VMs to compute_instance and compute_instance_template (#3209) #3531

mlauter commented May 1, 2019 •

edited

Loading

rileykarson left a comment

rileykarson May 10, 2019

rileykarson May 10, 2019

rileykarson left a comment

rileykarson May 13, 2019

mlauter May 13, 2019

rileykarson May 13, 2019

rileykarson May 13, 2019

rileykarson May 13, 2019

rileykarson May 13, 2019

rileykarson May 13, 2019

mlauter commented May 14, 2019

rileykarson left a comment

ghost commented Jun 16, 2019

		@@ -134,6 +134,9 @@ The following arguments are supported:

		* `tags` - (Optional) A list of tags to attach to the instance.

		* `shielded_vm_config` - (Optional) Enable [Shielded VM](https://cloud.google.com/security/shielded-cloud/shielded-vm) on this instance. Shielded VM provides verifiable integrity to prevent against malware and rootkits. Defaults to disabled. Structure is documented below.

Add support for Shielded VMs to compute_instance and compute_instance_template (#3209) #3531

Add support for Shielded VMs to compute_instance and compute_instance_template (#3209) #3531

Conversation

mlauter commented May 1, 2019 • edited Loading

rileykarson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rileykarson 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

mlauter commented May 14, 2019

rileykarson left a comment

Choose a reason for hiding this comment

ghost commented Jun 16, 2019

mlauter commented May 1, 2019 •

edited

Loading