From 5cf6a5d13192b5aadf01603615101a9a22a35fb3 Mon Sep 17 00:00:00 2001 From: Paddy Carver Date: Wed, 5 Sep 2018 17:35:44 -0700 Subject: [PATCH 1/4] Fix perma-diff on instance templates. When using instance templates, if you use one of our image shorthands at the moment, you'll get a perma-diff. This is because the config gets resolved to another format before it is set in state, and so we need to set that other format in state. Unfortunately, resolving images requires network access, so we have to do this with CustomizeDiff. CustomizeDiff was having trouble (I think? More on this below) on setting a field as not ForceNew once the field was already set, so I moved the decision for whether a field was ForceNew or not into CustomizeDiff. I also resolved the old and new images, and if they were the same, cleared the diff. Unfortunately, you can't actually clear a field on a sub-block right now. You have to clear top-level fields only. So this will currently throw an error. I opened hashicorp/terraform#18795 to fix that. Once that's merged, and we vendor it here, this patch fixes the problem. If hashicorp/terraform#18795 doesn't get merged, the next best workaround is to keep track of _all_ the fields under `disk` with a diff in our CustomizeDiff, check whether they've all changed or not, and if they've all changed, clear the changes on `disk`, which I _think_ will resolve the issue. That's just a massive pain, unfortunately. --- google/resource_compute_instance_template.go | 50 ++++++++++++++++++-- google/utils.go | 14 ++++++ 2 files changed, 60 insertions(+), 4 deletions(-) diff --git a/google/resource_compute_instance_template.go b/google/resource_compute_instance_template.go index e30c6068..7b973646 100644 --- a/google/resource_compute_instance_template.go +++ b/google/resource_compute_instance_template.go @@ -20,6 +20,7 @@ func resourceComputeInstanceTemplate() *schema.Resource { State: schema.ImportStatePassthrough, }, SchemaVersion: 1, + CustomizeDiff: resourceComputeInstanceTemplateCustomizeDiff, MigrateState: resourceComputeInstanceTemplateMigrateState, // A compute instance template is more or less a subset of a compute @@ -99,10 +100,9 @@ func resourceComputeInstanceTemplate() *schema.Resource { }, "source_image": &schema.Schema{ - Type: schema.TypeString, - Optional: true, - DiffSuppressFunc: compareSelfLinkRelativePaths, - ForceNew: true, + Type: schema.TypeString, + Optional: true, + Computed: true, }, "interface": &schema.Schema{ @@ -420,6 +420,48 @@ func resourceComputeInstanceTemplate() *schema.Resource { } } +func resourceComputeInstanceTemplateCustomizeDiff(diff *schema.ResourceDiff, meta interface{}) error { + config := meta.(*Config) + project, err := getProjectFromDiff(diff, config) + if err != nil { + return err + } + numDisks := diff.Get("disk.#").(int) + for i := 0; i < numDisks; i++ { + key := fmt.Sprintf("disk.%d.source_image", i) + if diff.HasChange(key) { + old, new := diff.GetChange(key) + if old == "" || new == "" { + // no sense in resolving empty strings + err = diff.ForceNew(key) + if err != nil { + return err + } + continue + } + oldResolved, err := resolveImage(config, project, old.(string)) + if err != nil { + return err + } + newResolved, err := resolveImage(config, project, new.(string)) + if err != nil { + return err + } + if oldResolved != newResolved { + err = diff.ForceNew(key) + if err != nil { + return err + } + } + err = diff.Clear(key) + if err != nil { + return err + } + } + } + return nil +} + func buildDisks(d *schema.ResourceData, config *Config) ([]*computeBeta.AttachedDisk, error) { project, err := getProject(d, config) if err != nil { diff --git a/google/utils.go b/google/utils.go index 0590222a..2ced913f 100644 --- a/google/utils.go +++ b/google/utils.go @@ -64,6 +64,20 @@ func getProject(d TerraformResourceData, config *Config) (string, error) { return getProjectFromSchema("project", d, config) } +// getProjectFromDiff reads the "project" field from the given diff and falls +// back to the provider's value if not given. If the provider's value is not +// given, an error is returned. +func getProjectFromDiff(d *schema.ResourceDiff, config *Config) (string, error) { + res, ok := d.GetOk("project") + if ok { + return res.(string), nil + } + if config.Project != "" { + return config.Project, nil + } + return "", fmt.Errorf("%s: required field is not set", "project") +} + func getProjectFromInstanceState(is *terraform.InstanceState, config *Config) (string, error) { res, ok := is.Attributes["project"] From 75c20b4246b9ed776f96f68b1fb90dad451b7145 Mon Sep 17 00:00:00 2001 From: Paddy Carver Date: Wed, 26 Sep 2018 14:22:52 -0700 Subject: [PATCH 2/4] Update helper/schema vendor. Update the version of helper/schema we have vendored to take advantage of fixes to the ResourceDiff.Clear function. --- .../schema/data_source_resource_shim.go | 2 +- .../terraform/helper/schema/resource.go | 8 +++--- .../terraform/helper/schema/resource_data.go | 1 + .../terraform/helper/schema/resource_diff.go | 26 ++++++++++++++----- .../terraform/helper/schema/schema.go | 2 +- .../hashicorp/terraform/helper/schema/set.go | 6 +++++ vendor/vendor.json | 6 ++--- 7 files changed, 34 insertions(+), 17 deletions(-) diff --git a/vendor/github.com/hashicorp/terraform/helper/schema/data_source_resource_shim.go b/vendor/github.com/hashicorp/terraform/helper/schema/data_source_resource_shim.go index 5a03d2d8..8d93750a 100644 --- a/vendor/github.com/hashicorp/terraform/helper/schema/data_source_resource_shim.go +++ b/vendor/github.com/hashicorp/terraform/helper/schema/data_source_resource_shim.go @@ -32,7 +32,7 @@ func DataSourceResourceShim(name string, dataSource *Resource) *Resource { // FIXME: Link to some further docs either on the website or in the // changelog, once such a thing exists. - dataSource.deprecationMessage = fmt.Sprintf( + dataSource.DeprecationMessage = fmt.Sprintf( "using %s as a resource is deprecated; consider using the data source instead", name, ) diff --git a/vendor/github.com/hashicorp/terraform/helper/schema/resource.go b/vendor/github.com/hashicorp/terraform/helper/schema/resource.go index 8f726bfb..d3be2d61 100644 --- a/vendor/github.com/hashicorp/terraform/helper/schema/resource.go +++ b/vendor/github.com/hashicorp/terraform/helper/schema/resource.go @@ -124,9 +124,7 @@ type Resource struct { Importer *ResourceImporter // If non-empty, this string is emitted as a warning during Validate. - // This is a private interface for now, for use by DataSourceResourceShim, - // and not for general use. (But maybe later...) - deprecationMessage string + DeprecationMessage string // Timeouts allow users to specify specific time durations in which an // operation should time out, to allow them to extend an action to suit their @@ -269,8 +267,8 @@ func (r *Resource) Diff( func (r *Resource) Validate(c *terraform.ResourceConfig) ([]string, []error) { warns, errs := schemaMap(r.Schema).Validate(c) - if r.deprecationMessage != "" { - warns = append(warns, r.deprecationMessage) + if r.DeprecationMessage != "" { + warns = append(warns, r.DeprecationMessage) } return warns, errs diff --git a/vendor/github.com/hashicorp/terraform/helper/schema/resource_data.go b/vendor/github.com/hashicorp/terraform/helper/schema/resource_data.go index 22d57a5e..6cc01ee0 100644 --- a/vendor/github.com/hashicorp/terraform/helper/schema/resource_data.go +++ b/vendor/github.com/hashicorp/terraform/helper/schema/resource_data.go @@ -315,6 +315,7 @@ func (d *ResourceData) State() *terraform.InstanceState { mapW := &MapFieldWriter{Schema: d.schema} if err := mapW.WriteField(nil, rawMap); err != nil { + log.Printf("[ERR] Error writing fields: %s", err) return nil } diff --git a/vendor/github.com/hashicorp/terraform/helper/schema/resource_diff.go b/vendor/github.com/hashicorp/terraform/helper/schema/resource_diff.go index 92b891fc..7db3decc 100644 --- a/vendor/github.com/hashicorp/terraform/helper/schema/resource_diff.go +++ b/vendor/github.com/hashicorp/terraform/helper/schema/resource_diff.go @@ -231,7 +231,7 @@ func (d *ResourceDiff) UpdatedKeys() []string { // Note that this does not wipe an override. This function is only allowed on // computed keys. func (d *ResourceDiff) Clear(key string) error { - if err := d.checkKey(key, "Clear"); err != nil { + if err := d.checkKey(key, "Clear", true); err != nil { return err } @@ -287,7 +287,7 @@ func (d *ResourceDiff) diffChange(key string) (interface{}, interface{}, bool, b // // This function is only allowed on computed attributes. func (d *ResourceDiff) SetNew(key string, value interface{}) error { - if err := d.checkKey(key, "SetNew"); err != nil { + if err := d.checkKey(key, "SetNew", false); err != nil { return err } @@ -299,7 +299,7 @@ func (d *ResourceDiff) SetNew(key string, value interface{}) error { // // This function is only allowed on computed attributes. func (d *ResourceDiff) SetNewComputed(key string) error { - if err := d.checkKey(key, "SetNewComputed"); err != nil { + if err := d.checkKey(key, "SetNewComputed", false); err != nil { return err } @@ -535,12 +535,24 @@ func childAddrOf(child, parent string) bool { } // checkKey checks the key to make sure it exists and is computed. -func (d *ResourceDiff) checkKey(key, caller string) error { - s, ok := d.schema[key] - if !ok { +func (d *ResourceDiff) checkKey(key, caller string, nested bool) error { + var schema *Schema + if nested { + keyParts := strings.Split(key, ".") + schemaL := addrToSchema(keyParts, d.schema) + if len(schemaL) > 0 { + schema = schemaL[len(schemaL)-1] + } + } else { + s, ok := d.schema[key] + if ok { + schema = s + } + } + if schema == nil { return fmt.Errorf("%s: invalid key: %s", caller, key) } - if !s.Computed { + if !schema.Computed { return fmt.Errorf("%s only operates on computed keys - %s is not one", caller, key) } return nil diff --git a/vendor/github.com/hashicorp/terraform/helper/schema/schema.go b/vendor/github.com/hashicorp/terraform/helper/schema/schema.go index f44010ea..0ea5aad5 100644 --- a/vendor/github.com/hashicorp/terraform/helper/schema/schema.go +++ b/vendor/github.com/hashicorp/terraform/helper/schema/schema.go @@ -199,7 +199,7 @@ type Schema struct { Sensitive bool } -// SchemaDiffSuppresFunc is a function which can be used to determine +// SchemaDiffSuppressFunc is a function which can be used to determine // whether a detected diff on a schema element is "valid" or not, and // suppress it from the plan if necessary. // diff --git a/vendor/github.com/hashicorp/terraform/helper/schema/set.go b/vendor/github.com/hashicorp/terraform/helper/schema/set.go index bb194ee6..cba28903 100644 --- a/vendor/github.com/hashicorp/terraform/helper/schema/set.go +++ b/vendor/github.com/hashicorp/terraform/helper/schema/set.go @@ -17,6 +17,12 @@ func HashString(v interface{}) int { return hashcode.String(v.(string)) } +// HashInt hashes integers. If you want a Set of integers, this is the +// SchemaSetFunc you want. +func HashInt(v interface{}) int { + return hashcode.String(strconv.Itoa(v.(int))) +} + // HashResource hashes complex structures that are described using // a *Resource. This is the default set implementation used when a set's // element type is a full resource. diff --git a/vendor/vendor.json b/vendor/vendor.json index f9328fdf..307e5a19 100644 --- a/vendor/vendor.json +++ b/vendor/vendor.json @@ -665,10 +665,10 @@ "versionExact": "v0.11.2" }, { - "checksumSHA1": "JHxGzmxcIS8NyLX9pGhK5beIra4=", + "checksumSHA1": "OOwTGBTHcUmQTPBdyscTMkjApbI=", "path": "github.com/hashicorp/terraform/helper/schema", - "revision": "41e50bd32a8825a84535e353c3674af8ce799161", - "revisionTime": "2018-04-10T16:50:42Z", + "revision": "35d82b055591e9d47a254e68754216d8849ba67a", + "revisionTime": "2018-09-26T21:21:28Z", "version": "v0.11.2", "versionExact": "v0.11.2" }, From 2fc8da91e728ca2b86065ef0e1c2c501c928426a Mon Sep 17 00:00:00 2001 From: Paddy Carver Date: Tue, 2 Oct 2018 01:11:03 -0700 Subject: [PATCH 3/4] Fix Dana's comments, expand images to self_links. Fix Dana's comments, one of which exposed a bug: we weren't actually fixing the diff. Because resolveImage can return multiple formats, and only returns a self_link if a self_link is passed in, it was diffing more often than not against a self_link supplied in the config. We just had a bug in our CustomizeDiff function that concealed this. I added the resolvedImageSelfLink helper function to turn the output from resolveImage into a self_link, which fixes the problem. It also has the knock-on effect of fixing #2067 at the same time, which is nice. I decided to add another function instead of just modifying resolveImage to always return a self_link because I don't want to deal with the backwards compatibility problems of changing how we're storing a bunch of things in state this close to 1.19.0. And honestly, we should probably just be storing the self_link always, _anyways_. --- google/image.go | 36 ++++++++++++++++++++ google/resource_compute_instance_template.go | 13 +++++-- vendor/vendor.json | 2 -- 3 files changed, 47 insertions(+), 4 deletions(-) diff --git a/google/image.go b/google/image.go index 3d733e47..dceb69e1 100644 --- a/google/image.go +++ b/google/image.go @@ -196,3 +196,39 @@ func resolveImage(c *Config, project, name string) (string, error) { } return "", fmt.Errorf("Could not find image or family %s", name) } + +// resolvedImageSelfLink takes the output of resolveImage and coerces it into a self_link. +// In the event that a global/images/IMAGE or global/images/family/FAMILY reference is +// returned from resolveImage, providerProject will be used as the project for the self_link. +func resolvedImageSelfLink(providerProject, name string) (string, error) { + switch { + case resolveImageLink.MatchString(name): // https://www.googleapis.com/compute/v1/projects/xyz/global/images/xyz + return name, nil + case resolveImageProjectImage.MatchString(name): // projects/xyz/global/images/xyz + res := resolveImageProjectImage.FindStringSubmatch(name) + if err := sanityTestRegexMatches(2, res, "project image", name); err != nil { + return "", err + } + return fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/%s/global/images/%s", res[1], res[2]), nil + case resolveImageProjectFamily.MatchString(name): // projects/xyz/global/images/family/xyz + res := resolveImageProjectFamily.FindStringSubmatch(name) + if err := sanityTestRegexMatches(2, res, "project family", name); err != nil { + return "", err + } + return fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/%s/global/images/family/%s", res[1], res[2]), nil + case resolveImageGlobalImage.MatchString(name): // global/images/xyz + res := resolveImageGlobalImage.FindStringSubmatch(name) + if err := sanityTestRegexMatches(1, res, "global image", name); err != nil { + return "", err + } + return fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/%s/global/images/%s", providerProject, res[1]), nil + case resolveImageGlobalFamily.MatchString(name): // global/images/family/xyz + res := resolveImageGlobalFamily.FindStringSubmatch(name) + if err := sanityTestRegexMatches(1, res, "global family", name); err != nil { + return "", err + } + return fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/%s/global/images/family/%s", providerProject, res[1]), nil + } + return "", fmt.Errorf("Could not expand image or family %q into a self_link", name) + +} diff --git a/google/resource_compute_instance_template.go b/google/resource_compute_instance_template.go index 7b973646..3030d3f6 100644 --- a/google/resource_compute_instance_template.go +++ b/google/resource_compute_instance_template.go @@ -20,7 +20,7 @@ func resourceComputeInstanceTemplate() *schema.Resource { State: schema.ImportStatePassthrough, }, SchemaVersion: 1, - CustomizeDiff: resourceComputeInstanceTemplateCustomizeDiff, + CustomizeDiff: resourceComputeInstanceTemplateSourceImageCustomizeDiff, MigrateState: resourceComputeInstanceTemplateMigrateState, // A compute instance template is more or less a subset of a compute @@ -420,7 +420,7 @@ func resourceComputeInstanceTemplate() *schema.Resource { } } -func resourceComputeInstanceTemplateCustomizeDiff(diff *schema.ResourceDiff, meta interface{}) error { +func resourceComputeInstanceTemplateSourceImageCustomizeDiff(diff *schema.ResourceDiff, meta interface{}) error { config := meta.(*Config) project, err := getProjectFromDiff(diff, config) if err != nil { @@ -443,15 +443,24 @@ func resourceComputeInstanceTemplateCustomizeDiff(diff *schema.ResourceDiff, met if err != nil { return err } + oldResolved, err = resolvedImageSelfLink(project, oldResolved) + if err != nil { + return err + } newResolved, err := resolveImage(config, project, new.(string)) if err != nil { return err } + newResolved, err = resolvedImageSelfLink(project, newResolved) + if err != nil { + return err + } if oldResolved != newResolved { err = diff.ForceNew(key) if err != nil { return err } + continue } err = diff.Clear(key) if err != nil { diff --git a/vendor/vendor.json b/vendor/vendor.json index 307e5a19..121fb951 100644 --- a/vendor/vendor.json +++ b/vendor/vendor.json @@ -669,8 +669,6 @@ "path": "github.com/hashicorp/terraform/helper/schema", "revision": "35d82b055591e9d47a254e68754216d8849ba67a", "revisionTime": "2018-09-26T21:21:28Z", - "version": "v0.11.2", - "versionExact": "v0.11.2" }, { "checksumSHA1": "Fzbv+N7hFXOtrR6E7ZcHT3jEE9s=", From a8fdf4033ea38d890c2e48152c0ce1f5b73dbabc Mon Sep 17 00:00:00 2001 From: Paddy Date: Tue, 2 Oct 2018 02:44:17 -0700 Subject: [PATCH 4/4] Fix trailing comma JSON is the worst. --- vendor/vendor.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vendor/vendor.json b/vendor/vendor.json index 121fb951..128e07b1 100644 --- a/vendor/vendor.json +++ b/vendor/vendor.json @@ -668,7 +668,7 @@ "checksumSHA1": "OOwTGBTHcUmQTPBdyscTMkjApbI=", "path": "github.com/hashicorp/terraform/helper/schema", "revision": "35d82b055591e9d47a254e68754216d8849ba67a", - "revisionTime": "2018-09-26T21:21:28Z", + "revisionTime": "2018-09-26T21:21:28Z" }, { "checksumSHA1": "Fzbv+N7hFXOtrR6E7ZcHT3jEE9s=",