From 0ae0076e3a089770a357f73e7d16a7ad466abd3e Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 20 Mar 2017 11:23:31 -0400 Subject: [PATCH] Correctly filter flatmapped values in diff When transforming a diff from DestroyCreate to a simple Update, ignore_changes can cause keys from flatmapped objects to be filtered form the diff. We need to filter each flatmapped container as a whole to ensure that unchanged keys aren't lost in the update. --- terraform/context_plan_test.go | 3 +- terraform/diff.go | 4 +- terraform/eval_diff.go | 134 ++++++++++++++++++++++----------- 3 files changed, 97 insertions(+), 44 deletions(-) diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index e27b90c40..7064f6465 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -3098,7 +3098,8 @@ func TestContext2Plan_listOrder(t *testing.T) { // Make sure ignore-changes doesn't interfere with set/list/map diffs. // If a resource was being replaced by a RequiresNew attribute that gets -// ignores, we need to filter out the diff properly. +// ignored, we need to filter the diff properly to properly update rather than +// replace. func TestContext2Plan_ignoreChangesWithFlatmaps(t *testing.T) { m := testModule(t, "plan-ignore-changes-with-flatmaps") p := testProvider("aws") diff --git a/terraform/diff.go b/terraform/diff.go index 5cf1b78ce..a9fae6c2c 100644 --- a/terraform/diff.go +++ b/terraform/diff.go @@ -25,6 +25,9 @@ const ( DiffDestroyCreate ) +// multiVal matches the index key to a flatmapped set, list or map +var multiVal = regexp.MustCompile(`\.(#|%)$`) + // Diff trackes the changes that are necessary to apply a configuration // to an existing infrastructure. type Diff struct { @@ -808,7 +811,6 @@ func (d *InstanceDiff) Same(d2 *InstanceDiff) (bool, string) { } // search for the suffix of the base of a [computed] map, list or set. - multiVal := regexp.MustCompile(`\.(#|~#|%)$`) match := multiVal.FindStringSubmatch(k) if diffOld.NewComputed && len(match) == 2 { diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index 717d95105..6f09526a4 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -152,6 +152,7 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { }) } + // filter out ignored resources if err := n.processIgnoreChanges(diff); err != nil { return nil, err } @@ -190,72 +191,81 @@ func (n *EvalDiff) processIgnoreChanges(diff *InstanceDiff) error { return nil } - changeType := diff.ChangeType() - // If we're just creating the resource, we shouldn't alter the // Diff at all - if changeType == DiffCreate { + if diff.ChangeType() == DiffCreate { return nil } // If the resource has been tainted then we don't process ignore changes // since we MUST recreate the entire resource. - if diff.DestroyTainted { + if diff.GetDestroyTainted() { return nil } + attrs := diff.CopyAttributes() + + // get the complete set of keys we want to ignore ignorableAttrKeys := make(map[string]bool) for _, ignoredKey := range ignoreChanges { - for k := range diff.CopyAttributes() { + for k := range attrs { if ignoredKey == "*" || strings.HasPrefix(k, ignoredKey) { ignorableAttrKeys[k] = true } } } - // If we are replacing the resource, then we expect there to be a bunch of - // extraneous attribute diffs we need to filter out for the other - // non-requires-new attributes going from "" -> "configval" or "" -> - // "". Filtering these out allows us to see if we might be able to - // skip this diff altogether. - if changeType == DiffDestroyCreate { - for k, v := range diff.CopyAttributes() { + // If the resource was being destroyed, check to see if we can ignore the + // reason for it being destroyed. + if diff.GetDestroy() { + for k, v := range attrs { + if k == "id" { + // id will always be changed if we intended to replace this instance + continue + } if v.Empty() || v.NewComputed { + continue + } + + // If any RequiresNew attribute isn't ignored, we need to keep the diff + // as-is to be able to replace the resource. + if v.RequiresNew && !ignorableAttrKeys[k] { + return nil + } + } + + // Now that we know that we aren't replacing the instance, we can filter + // out all the empty and computed attributes. There may be a bunch of + // extraneous attribute diffs for the other non-requires-new attributes + // going from "" -> "configval" or "" -> "". + // We must make sure any flatmapped containers are filterred (or not) as a + // whole. + containers := groupContainers(diff) + keep := map[string]bool{} + for _, v := range containers { + if v.keepDiff() { + // At least one key has changes, so list all the sibling keys + // to keep in the diff. + for k := range v { + keep[k] = true + } + } + } + + for k, v := range attrs { + if (v.Empty() || v.NewComputed) && !keep[k] { ignorableAttrKeys[k] = true } } - - // Here we emulate the implementation of diff.RequiresNew() with one small - // tweak, we ignore the "id" attribute diff that gets added by EvalDiff, - // since that was added in reaction to RequiresNew being true. - requiresNewAfterIgnores := false - for k, v := range diff.CopyAttributes() { - if k == "id" { - continue - } - if _, ok := ignorableAttrKeys[k]; ok { - continue - } - if v.RequiresNew == true { - requiresNewAfterIgnores = true - } - } - - // If we still require resource replacement after ignores, we - // can't touch the diff, as all of the attributes will be - // required to process the replacement. - if requiresNewAfterIgnores { - return nil - } - - // Here we undo the two reactions to RequireNew in EvalDiff - the "id" - // attribute diff and the Destroy boolean field - log.Printf("[DEBUG] Removing 'id' diff and setting Destroy to false " + - "because after ignore_changes, this diff no longer requires replacement") - diff.DelAttribute("id") - diff.SetDestroy(false) } + // Here we undo the two reactions to RequireNew in EvalDiff - the "id" + // attribute diff and the Destroy boolean field + log.Printf("[DEBUG] Removing 'id' diff and setting Destroy to false " + + "because after ignore_changes, this diff no longer requires replacement") + diff.DelAttribute("id") + diff.SetDestroy(false) + // If we didn't hit any of our early exit conditions, we can filter the diff. for k := range ignorableAttrKeys { log.Printf("[DEBUG] [EvalIgnoreChanges] %s - Ignoring diff attribute: %s", @@ -266,6 +276,46 @@ func (n *EvalDiff) processIgnoreChanges(diff *InstanceDiff) error { return nil } +// a group of key-*ResourceAttrDiff pairs from the same flatmapped container +type flatAttrDiff map[string]*ResourceAttrDiff + +// we need to keep all keys if any of them have a diff +func (f flatAttrDiff) keepDiff() bool { + for _, v := range f { + if !v.Empty() && !v.NewComputed { + return true + } + } + return false +} + +// sets, lists and maps need to be compared for diff inclusion as a whole, so +// group the flatmapped keys together for easier comparison. +func groupContainers(d *InstanceDiff) map[string]flatAttrDiff { + isIndex := multiVal.MatchString + containers := map[string]flatAttrDiff{} + attrs := d.CopyAttributes() + // we need to loop once to find the index key + for k := range attrs { + if isIndex(k) { + // add the key, always including the final dot to fully qualify it + containers[k[:len(k)-1]] = flatAttrDiff{} + } + } + + // loop again to find all the sub keys + for prefix, values := range containers { + for k, attrDiff := range attrs { + // we include the index value as well, since it could be part of the diff + if strings.HasPrefix(k, prefix) { + values[k] = attrDiff + } + } + } + + return containers +} + // EvalDiffDestroy is an EvalNode implementation that returns a plain // destroy diff. type EvalDiffDestroy struct {