From d8964d9198a7ef6754972fac965bcc213568833f Mon Sep 17 00:00:00 2001 From: Imre Rad Date: Mon, 27 Feb 2023 11:34:40 +0000 Subject: [PATCH] id of instance templates now relies on the unique id of the resource This is a security improvement to prevent instance templates being replaced before a follow-up operation (e.g. MIG creation) is started. --- ...urce_compute_instance_from_template.go.erb | 3 +- ...urce_compute_instance_group_manager.go.erb | 31 +++++++- .../resource_compute_instance_template.go.erb | 22 ++++-- ...mpute_region_instance_group_manager.go.erb | 2 +- ...compute_instance_group_manager_test.go.erb | 70 +++++++++++++++++++ ...urce_compute_instance_template_test.go.erb | 23 ++++-- .../d/compute_instance_template.html.markdown | 2 +- .../r/compute_instance_template.html.markdown | 2 +- 8 files changed, 138 insertions(+), 17 deletions(-) diff --git a/mmv1/third_party/terraform/resources/resource_compute_instance_from_template.go.erb b/mmv1/third_party/terraform/resources/resource_compute_instance_from_template.go.erb index 94232215ef56..641be9bf1454 100644 --- a/mmv1/third_party/terraform/resources/resource_compute_instance_from_template.go.erb +++ b/mmv1/third_party/terraform/resources/resource_compute_instance_from_template.go.erb @@ -121,7 +121,8 @@ func resourceComputeInstanceFromTemplateCreate(d *schema.ResourceData, meta inte return err } - tpl, err := ParseInstanceTemplateFieldValue(d.Get("source_instance_template").(string), d, config) + sourceTemplate:= ConvertToUniqueIdWhenPresent(d.Get("source_instance_template").(string)) + tpl, err := ParseInstanceTemplateFieldValue(sourceTemplate, d, config) if err != nil { return err } diff --git a/mmv1/third_party/terraform/resources/resource_compute_instance_group_manager.go.erb b/mmv1/third_party/terraform/resources/resource_compute_instance_group_manager.go.erb index 39eb4f2f767c..76fcd3e16aab 100644 --- a/mmv1/third_party/terraform/resources/resource_compute_instance_group_manager.go.erb +++ b/mmv1/third_party/terraform/resources/resource_compute_instance_group_manager.go.erb @@ -55,7 +55,7 @@ func ResourceComputeInstanceGroupManager() *schema.Resource { "instance_template": { Type: schema.TypeString, Required: true, - DiffSuppressFunc: compareSelfLinkRelativePaths, + DiffSuppressFunc: compareSelfLinkRelativePathsIgnoreParams, Description: `The full URL to an instance template from which all new instances of this version will be created.`, }, @@ -495,6 +495,33 @@ func ResourceComputeInstanceGroupManager() *schema.Resource { } } +func parseUniqueId(s string) (string, string) { + splits:= strings.SplitN(s, "?uniqueId=", 2) + if len(splits) == 2 { + return splits[0], splits[1] + } + return s, "" +} + +func compareSelfLinkRelativePathsIgnoreParams(_unused1, old, new string, _unused2 *schema.ResourceData) bool { + oldName, oldUniqueId:= parseUniqueId(old) + newName, newUniqueId:= parseUniqueId(new) + if oldUniqueId != "" && newUniqueId != "" && oldUniqueId != newUniqueId { + return false + } + return compareSelfLinkRelativePaths(_unused1, oldName, newName, _unused2) +} + +func ConvertToUniqueIdWhenPresent(s string) string { + original, uniqueId:= parseUniqueId(s) + if uniqueId != "" { + splits:= strings.Split(original, "/") + splits[len(splits)-1] = uniqueId + return strings.Join(splits, "/") + } + return s +} + func getNamedPorts(nps []interface{}) []*compute.NamedPort { namedPorts := make([]*compute.NamedPort, 0, len(nps)) for _, v := range nps { @@ -1160,7 +1187,7 @@ func expandVersions(configured []interface{}) []*compute.InstanceGroupManagerVer version := compute.InstanceGroupManagerVersion{ Name: data["name"].(string), - InstanceTemplate: data["instance_template"].(string), + InstanceTemplate: ConvertToUniqueIdWhenPresent(data["instance_template"].(string)), TargetSize: expandFixedOrPercent(data["target_size"].([]interface{})), } diff --git a/mmv1/third_party/terraform/resources/resource_compute_instance_template.go.erb b/mmv1/third_party/terraform/resources/resource_compute_instance_template.go.erb index 20808d509f7f..073716038059 100644 --- a/mmv1/third_party/terraform/resources/resource_compute_instance_template.go.erb +++ b/mmv1/third_party/terraform/resources/resource_compute_instance_template.go.erb @@ -1249,7 +1249,7 @@ func resourceComputeInstanceTemplateCreate(d *schema.ResourceData, meta interfac } // Store the ID now - d.SetId(fmt.Sprintf("projects/%s/global/instanceTemplates/%s", project, instanceTemplate.Name)) + d.SetId(fmt.Sprintf("projects/%s/global/instanceTemplates/%v?uniqueId=%v", project, itName, op.TargetId)) err = ComputeOperationWaitTime(config, op, project, "Creating Instance Template", userAgent, d.Timeout(schema.TimeoutCreate)) if err != nil { @@ -1492,12 +1492,19 @@ func resourceComputeInstanceTemplateRead(d *schema.ResourceData, meta interface{ return err } - splits := strings.Split(d.Id(), "/") - instanceTemplate, err := config.NewComputeClient(userAgent).InstanceTemplates.Get(project, splits[len(splits)-1]).Do() + id:= ConvertToUniqueIdWhenPresent(d.Id()) + splits := strings.Split(id, "/") + template:= splits[len(splits)-1] + + instanceTemplate, err := config.NewComputeClient(userAgent).InstanceTemplates.Get(project, template).Do() if err != nil { return handleNotFoundError(err, d, fmt.Sprintf("Instance Template %q", d.Get("name").(string))) } + // fixing the terraform id to refer to this resource through its unique numeric id + uniqueIdSuffix:= fmt.Sprintf("?uniqueId=%v", instanceTemplate.Id) + d.SetId(fmt.Sprintf("projects/%s/global/instanceTemplates/%v%v", project, instanceTemplate.Name, uniqueIdSuffix)) + // Set the metadata fingerprint if there is one. if instanceTemplate.Properties.Metadata != nil { if err = d.Set("metadata_fingerprint", instanceTemplate.Properties.Metadata.Fingerprint); err != nil { @@ -1536,7 +1543,7 @@ func resourceComputeInstanceTemplateRead(d *schema.ResourceData, meta interface{ return fmt.Errorf("Error setting labels: %s", err) } } - if err = d.Set("self_link", instanceTemplate.SelfLink); err != nil { + if err = d.Set("self_link", instanceTemplate.SelfLink + uniqueIdSuffix); err != nil { return fmt.Errorf("Error setting self_link: %s", err) } if err = d.Set("name", instanceTemplate.Name); err != nil { @@ -1667,9 +1674,12 @@ func resourceComputeInstanceTemplateDelete(d *schema.ResourceData, meta interfac return err } - splits := strings.Split(d.Id(), "/") + id:= ConvertToUniqueIdWhenPresent(d.Id()) + splits := strings.Split(id, "/") + template:= splits[len(splits)-1] + op, err := config.NewComputeClient(userAgent).InstanceTemplates.Delete( - project, splits[len(splits)-1]).Do() + project, template).Do() if err != nil { return fmt.Errorf("Error deleting instance template: %s", err) } diff --git a/mmv1/third_party/terraform/resources/resource_compute_region_instance_group_manager.go.erb b/mmv1/third_party/terraform/resources/resource_compute_region_instance_group_manager.go.erb index d5a622749bda..86b98d1f441f 100644 --- a/mmv1/third_party/terraform/resources/resource_compute_region_instance_group_manager.go.erb +++ b/mmv1/third_party/terraform/resources/resource_compute_region_instance_group_manager.go.erb @@ -56,7 +56,7 @@ func ResourceComputeRegionInstanceGroupManager() *schema.Resource { "instance_template": { Type: schema.TypeString, Required: true, - DiffSuppressFunc: compareSelfLinkRelativePaths, + DiffSuppressFunc: compareSelfLinkRelativePathsIgnoreParams, Description: `The full URL to an instance template from which all new instances of this version will be created.`, }, diff --git a/mmv1/third_party/terraform/tests/resource_compute_instance_group_manager_test.go.erb b/mmv1/third_party/terraform/tests/resource_compute_instance_group_manager_test.go.erb index 9f9a03e62712..5d5d22614dc7 100644 --- a/mmv1/third_party/terraform/tests/resource_compute_instance_group_manager_test.go.erb +++ b/mmv1/third_party/terraform/tests/resource_compute_instance_group_manager_test.go.erb @@ -68,6 +68,76 @@ func testSweepComputeInstanceGroupManager(region string) error { return nil } +func TestInstanceGroupManager_parseUniqueId(t *testing.T) { + expectations:= map[string][]string { + "projects/imre-test/global/instanceTemplates/example-template-custom?uniqueId=123": []string{"projects/imre-test/global/instanceTemplates/example-template-custom", "123"}, + "https://www.googleapis.com/compute/v1/projects/imre-test/global/instanceTemplates/example-template-custom?uniqueId=123": []string{"https://www.googleapis.com/compute/v1/projects/imre-test/global/instanceTemplates/example-template-custom", "123"}, + "projects/imre-test/global/instanceTemplates/example-template-custom": []string{"projects/imre-test/global/instanceTemplates/example-template-custom", ""}, + "https://www.googleapis.com/compute/v1/projects/imre-test/global/instanceTemplates/example-template-custom": []string{"https://www.googleapis.com/compute/v1/projects/imre-test/global/instanceTemplates/example-template-custom", ""}, + "example-template-custom?uniqueId=123": []string{"example-template-custom", "123"}, + } + + for k, v:= range expectations { + aName, aUniqueId := parseUniqueId(k) + if v[0] != aName { + t.Errorf("parseUniqueId failed; name of %v should be %v, not %v", k, v[0], aName) + } + if v[1] != aUniqueId { + t.Errorf("parseUniqueId failed; uniqueId of %v should be %v, not %v", k, v[1], aUniqueId) + } + } +} + +func TestInstanceGroupManager_compareInstanceTemplate(t *testing.T) { + shouldAllMatch := []string { + // uniqueId not present + "https://www.googleapis.com/compute/v1/projects/imre-test/global/instanceTemplates/example-template-custom", + "projects/imre-test/global/instanceTemplates/example-template-custom", + // uniqueId present + "https://www.googleapis.com/compute/v1/projects/imre-test/global/instanceTemplates/example-template-custom?uniqueId=123", + "projects/imre-test/global/instanceTemplates/example-template-custom?uniqueId=123", + } + shouldNotMatch := map[string]string { + // mismatching name + "https://www.googleapis.com/compute/v1/projects/imre-test/global/instanceTemplates/example-template-custom": "projects/imre-test/global/instanceTemplates/example-template-custom2", + "projects/imre-test/global/instanceTemplates/example-template-custom": "https://www.googleapis.com/compute/v1/projects/imre-test/global/instanceTemplates/example-template-custom2", + // matching name, but mismatching uniqueId + "https://www.googleapis.com/compute/v1/projects/imre-test/global/instanceTemplates/example-template-custom?uniqueId=123": "projects/imre-test/global/instanceTemplates/example-template-custom?uniqueId=1234", + "projects/imre-test/global/instanceTemplates/example-template-custom?uniqueId=123": "https://www.googleapis.com/compute/v1/projects/imre-test/global/instanceTemplates/example-template-custom?uniqueId=1234", + } + for _, v1 := range shouldAllMatch { + for _, v2:= range shouldAllMatch { + if !compareSelfLinkRelativePathsIgnoreParams("", v1, v2, nil) { + t.Fatalf("compareSelfLinkRelativePathsIgnoreParams did not match (and should have) %v and %v", v1, v2) + } + } + } + + for v1, v2 := range shouldNotMatch { + if compareSelfLinkRelativePathsIgnoreParams("", v1, v2, nil) { + t.Fatalf("compareSelfLinkRelativePathsIgnoreParams did match (and shouldn't) %v and %v", v1, v2) + } + } +} + +func TestInstanceGroupManager_convertUniqueId(t *testing.T) { + matches:= map[string]string { + // uniqueId not present (should return the same) + "https://www.googleapis.com/compute/v1/projects/imre-test/global/instanceTemplates/example-template-custom": "https://www.googleapis.com/compute/v1/projects/imre-test/global/instanceTemplates/example-template-custom", + "projects/imre-test/global/instanceTemplates/example-template-custom": "projects/imre-test/global/instanceTemplates/example-template-custom", + // uniqueId present (should return the last component replaced) + "https://www.googleapis.com/compute/v1/projects/imre-test/global/instanceTemplates/example-template-custom?uniqueId=123": "https://www.googleapis.com/compute/v1/projects/imre-test/global/instanceTemplates/123", + "projects/imre-test/global/instanceTemplates/example-template-custom?uniqueId=123": "projects/imre-test/global/instanceTemplates/123", + "tf-test-igm-8amncgtq22?uniqueId=8361222501423044003": "8361222501423044003", + } + for input, expected := range matches { + actual:= ConvertToUniqueIdWhenPresent(input) + if actual != expected { + t.Fatalf("invalid return value by ConvertToUniqueIdWhenPresent for input %v; expected: %v, actual: %v", input, expected, actual) + } + } +} + func TestAccInstanceGroupManager_basic(t *testing.T) { t.Parallel() diff --git a/mmv1/third_party/terraform/tests/resource_compute_instance_template_test.go.erb b/mmv1/third_party/terraform/tests/resource_compute_instance_template_test.go.erb index 3789cb59a538..b455f11908a6 100644 --- a/mmv1/third_party/terraform/tests/resource_compute_instance_template_test.go.erb +++ b/mmv1/third_party/terraform/tests/resource_compute_instance_template_test.go.erb @@ -6,6 +6,7 @@ import ( "fmt" "reflect" "regexp" + "strconv" "strings" "testing" "time" @@ -1339,7 +1340,10 @@ func testAccCheckComputeInstanceTemplateExistsInProject(t *testing.T, n, p strin config := GoogleProviderConfig(t) - splits := strings.Split(rs.Primary.ID, "/") + // old syntax: projects/imre-test/global/instanceTemplates/something + // new syntax: projects/imre-test/global/instanceTemplates/something?uniqueId=123123 + originalId, uniqueId:= parseUniqueId(rs.Primary.ID) + splits := strings.Split(originalId, "/") templateName := splits[len(splits)-1] found, err := config.NewComputeClient(config.UserAgent).InstanceTemplates.Get( p, templateName).Do() @@ -1348,7 +1352,16 @@ func testAccCheckComputeInstanceTemplateExistsInProject(t *testing.T, n, p strin } if found.Name != templateName { - return fmt.Errorf("Instance template not found") + return fmt.Errorf("Instance template: mismatching name") + } + foundId:= strconv.FormatUint(found.Id, 10) + if foundId != uniqueId { + return fmt.Errorf("Instance template: mismatching unique ID") + } + + selfLink:= rs.Primary.Attributes["self_link"] + if !strings.HasSuffix(selfLink, "?uniqueId="+foundId) { + return fmt.Errorf("unique ID is not present in the self_link") } *instanceTemplate = *found @@ -3250,7 +3263,7 @@ data "google_compute_image" "debian" { } resource "google_compute_disk" "persistent" { - name = "debian-disk" + name = "tf-test-debian-disk-%s" image = data.google_compute_image.debian.self_link size = 10 type = "pd-ssd" @@ -3258,7 +3271,7 @@ resource "google_compute_disk" "persistent" { } resource "google_compute_snapshot" "snapshot" { - name = "my-snapshot" + name = "tf-test-my-snapshot-%s" source_disk = google_compute_disk.persistent.id zone = "us-central1-a" snapshot_encryption_key { @@ -3285,7 +3298,7 @@ resource "google_compute_instance_template" "template" { network = "default" } } -`, kmsRingName, kmsKeyName, suffix, suffix) +`, kmsRingName, kmsKeyName, suffix, suffix, suffix, suffix) } func testAccComputeInstanceTemplate_sourceImageEncryptionKey(kmsRingName, kmsKeyName, suffix string) string { diff --git a/mmv1/third_party/terraform/website/docs/d/compute_instance_template.html.markdown b/mmv1/third_party/terraform/website/docs/d/compute_instance_template.html.markdown index ebb5dbb19681..508c549a9a55 100644 --- a/mmv1/third_party/terraform/website/docs/d/compute_instance_template.html.markdown +++ b/mmv1/third_party/terraform/website/docs/d/compute_instance_template.html.markdown @@ -297,7 +297,7 @@ The `disk_encryption_key` block supports: --- -* `id` - an identifier for the resource with format `projects/{{project}}/global/instanceTemplates/{{name}}` +* `id` - an identifier for the resource with format `projects/{{project}}/global/instanceTemplates/{{name}}?uniqueId={{uniqueId}}` * `metadata_fingerprint` - The unique fingerprint of the metadata. diff --git a/mmv1/third_party/terraform/website/docs/r/compute_instance_template.html.markdown b/mmv1/third_party/terraform/website/docs/r/compute_instance_template.html.markdown index 7ab6ed9a5da8..9346591c93a8 100644 --- a/mmv1/third_party/terraform/website/docs/r/compute_instance_template.html.markdown +++ b/mmv1/third_party/terraform/website/docs/r/compute_instance_template.html.markdown @@ -634,7 +634,7 @@ The `specific_reservation` block supports: In addition to the arguments listed above, the following computed attributes are exported: -* `id` - an identifier for the resource with format `projects/{{project}}/global/instanceTemplates/{{name}}` +* `id` - an identifier for the resource with format `projects/{{project}}/global/instanceTemplates/{{name}}?uniqueId={{uniqueId}}` * `metadata_fingerprint` - The unique fingerprint of the metadata.