From 570e12852780ca80837d41ed384b5a47340f124c Mon Sep 17 00:00:00 2001 From: Paddy Carver Date: Wed, 27 Jan 2021 03:12:03 -0800 Subject: [PATCH] Check output of finalizeDiff before using it. Previously, we'd assign the result of finalizeDiff to the resource diff without checking its return. This caused problems because a "finalized" diff for any given attribute could, in fact, be no diff at all. Which we represent as `nil`. But some consumers of the resource diff expect every attribute in the map to be non-`nil`, and so crash on these attributes that have diff entries but no diffs. See for example hashicorp/terraform-provider-google#7934, which would crash when a config had an explicit empty string as the value for a field that a CustomizeDiff function set to ForceNew. Technically, there was a diff, but finalizeDiff decided it wasn't a "real" diff, because the SDK still interprets empty strings as "unset" for computed fields to align with legacy behavior. But that meant a nil in the resource's map of attribute diffs, which then was dereferenced when populating the response to PlanResourceChange. This caused a crash. This commit fixes that issue by updating all our usages of finalizeDiff to check for a nil diff _before_ writing it to the resource's map of attribute diffs. This was easier than tracking down all the usages of a ResourceAttributeDiff and trying to ensure they were ignoring nil values. --- helper/schema/grpc_provider.go | 1 - helper/schema/resource_diff.go | 3 --- helper/schema/schema.go | 43 ++++++++++++++++++++++++++-------- 3 files changed, 33 insertions(+), 14 deletions(-) diff --git a/helper/schema/grpc_provider.go b/helper/schema/grpc_provider.go index cc4a9ec4452..d9c824d26d8 100644 --- a/helper/schema/grpc_provider.go +++ b/helper/schema/grpc_provider.go @@ -796,7 +796,6 @@ func (s *GRPCProviderServer) PlanResourceChange(ctx context.Context, req *tfprot newExtra := map[string]interface{}{} for k, v := range diff.Attributes { - log.Printf("[TRACE] tpg-7934: copying over attribute %q", k) if v.NewExtra != nil { newExtra[k] = v.NewExtra } diff --git a/helper/schema/resource_diff.go b/helper/schema/resource_diff.go index 521c58768de..984929df7b5 100644 --- a/helper/schema/resource_diff.go +++ b/helper/schema/resource_diff.go @@ -3,7 +3,6 @@ package schema import ( "errors" "fmt" - "log" "reflect" "strings" "sync" @@ -462,11 +461,9 @@ func (d *ResourceDiff) getChange(key string) (getResult, getResult, bool) { var new getResult for p := range d.updatedKeys { if childAddrOf(key, p) { - log.Printf("[TRACE] tpg-7934: key %q is child of parent %q, counts as computed", key, p) new = d.getExact(strings.Split(key, "."), "newDiff") return old, new, true } - log.Printf("[TRACE] tpg-7934: key %q is not child of parent %q, does not count as computed", key, p) } new = d.get(strings.Split(key, "."), "newDiff") return old, new, false diff --git a/helper/schema/schema.go b/helper/schema/schema.go index 2254cac4a69..7146bef766b 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -360,7 +360,6 @@ func (s *Schema) ZeroValue() interface{} { func (s *Schema) finalizeDiff(d *terraform.ResourceAttrDiff, customized bool) *terraform.ResourceAttrDiff { if d == nil { - log.Println("[TRACE] tpg-7934: returning nil from finalizeDiff because nil was passed in") return d } @@ -409,7 +408,7 @@ func (s *Schema) finalizeDiff(d *terraform.ResourceAttrDiff, customized bool) *t if d.Old != "" && d.New == "" { // This is a computed value with an old value set already, // just let it go. - log.Println("[TRACE] tpg-7934: returning nil from finalizeDiff because customized") + log.Println("[DEBUG] A computed value with the empty string as the new value and a non-empty old value was found. Interpreting the empty string as \"unset\" to align with legacy behavior.") return nil } } @@ -1063,13 +1062,18 @@ func (m schemaMap) diffList( oldStr = "" } - diff.Attributes[k+".#"] = countSchema.finalizeDiff( + finalizedAttr := countSchema.finalizeDiff( &terraform.ResourceAttrDiff{ Old: oldStr, New: newStr, }, customized, ) + if finalizedAttr != nil { + diff.Attributes[k+".#"] = finalizedAttr + } else { + delete(diff.Attributes, k+".#") + } } // Figure out the maximum @@ -1169,13 +1173,18 @@ func (m schemaMap) diffMap( oldStr = "" } - diff.Attributes[k+".%"] = countSchema.finalizeDiff( + finalizedAttr := countSchema.finalizeDiff( &terraform.ResourceAttrDiff{ Old: oldStr, New: newStr, }, customized, ) + if finalizedAttr != nil { + diff.Attributes[k+".%"] = finalizedAttr + } else { + delete(diff.Attributes, k+".%") + } } // If the new map is nil and we're computed, then ignore it. @@ -1192,22 +1201,28 @@ func (m schemaMap) diffMap( continue } - diff.Attributes[prefix+k] = schema.finalizeDiff( + finalizedAttr := schema.finalizeDiff( &terraform.ResourceAttrDiff{ Old: old, New: v, }, customized, ) + if finalizedAttr != nil { + diff.Attributes[prefix+k] = finalizedAttr + } } for k, v := range stateMap { - diff.Attributes[prefix+k] = schema.finalizeDiff( + finalizedAttr := schema.finalizeDiff( &terraform.ResourceAttrDiff{ Old: v, NewRemoved: true, }, customized, ) + if finalizedAttr != nil { + diff.Attributes[prefix+k] = finalizedAttr + } } return nil @@ -1279,26 +1294,32 @@ func (m schemaMap) diffSet( countStr = "" } - diff.Attributes[k+".#"] = countSchema.finalizeDiff( + finalizedAttr := countSchema.finalizeDiff( &terraform.ResourceAttrDiff{ Old: countStr, NewComputed: true, }, customized, ) + if finalizedAttr != nil { + diff.Attributes[k+".#"] = finalizedAttr + } return nil } // If the counts are not the same, then record that diff changed := oldLen != newLen if changed || all { - diff.Attributes[k+".#"] = countSchema.finalizeDiff( + finalizedAttr := countSchema.finalizeDiff( &terraform.ResourceAttrDiff{ Old: oldStr, New: newStr, }, customized, ) + if finalizedAttr != nil { + diff.Attributes[k+".#"] = finalizedAttr + } } // Build the list of codes that will make up our set. This is the @@ -1385,8 +1406,7 @@ func (m schemaMap) diffString( return nil } - log.Printf("[TRACE] tpg-7934: setting attribute %q", k) - diff.Attributes[k] = schema.finalizeDiff( + finalizedAttr := schema.finalizeDiff( &terraform.ResourceAttrDiff{ Old: os, New: ns, @@ -1396,6 +1416,9 @@ func (m schemaMap) diffString( }, customized, ) + if finalizedAttr != nil { + diff.Attributes[k] = finalizedAttr + } return nil }