diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index bf3ff4f41..7064f6465 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -3095,3 +3095,54 @@ func TestContext2Plan_listOrder(t *testing.T) { t.Fatal("aws_instance.a and aws_instance.b diffs should match:\n", plan) } } + +// Make sure ignore-changes doesn't interfere with set/list/map diffs. +// If a resource was being replaced by a RequiresNew attribute that gets +// 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") + p.DiffFn = testDiffFn + s := &State{ + Modules: []*ModuleState{ + &ModuleState{ + Path: rootModulePath, + Resources: map[string]*ResourceState{ + "aws_instance.foo": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "bar", + Attributes: map[string]string{ + "user_data": "x", + "require_new": "", + "set.#": "1", + "set.0.a": "1", + "lst.#": "1", + "lst.0": "j", + }, + }, + }, + }, + }, + }, + } + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + State: s, + }) + + plan, err := ctx.Plan() + if err != nil { + t.Fatalf("err: %s", err) + } + + actual := strings.TrimSpace(plan.Diff.String()) + expected := strings.TrimSpace(testTFPlanDiffIgnoreChangesWithFlatmaps) + if actual != expected { + t.Fatalf("bad:\n%s\n\nexpected\n\n%s", actual, expected) + } +} diff --git a/terraform/context_test.go b/terraform/context_test.go index 91babd75f..3534e9aa3 100644 --- a/terraform/context_test.go +++ b/terraform/context_test.go @@ -262,6 +262,11 @@ func testDiffFn( if _, ok := c.Raw["__"+k+"_requires_new"]; ok { attrDiff.RequiresNew = true } + + if attr, ok := s.Attributes[k]; ok { + attrDiff.Old = attr + } + diff.Attributes[k] = attrDiff } } 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 { diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index f1075b7f1..4a640cf1d 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -1500,7 +1500,7 @@ DIFF: DESTROY/CREATE: aws_instance.foo type: "" => "aws_instance" - vars: "" => "foo" + vars: "foo" => "foo" STATE: @@ -1570,6 +1570,17 @@ aws_instance.foo: ami = ami-abcd1234 ` +const testTFPlanDiffIgnoreChangesWithFlatmaps = ` +UPDATE: aws_instance.foo + lst.#: "1" => "2" + lst.0: "j" => "j" + lst.1: "" => "k" + set.#: "1" => "1" + set.0.a: "1" => "1" + set.0.b: "" => "2" + type: "" => "aws_instance" +` + const testTerraformPlanIgnoreChangesWildcardStr = ` DIFF: diff --git a/terraform/test-fixtures/plan-ignore-changes-with-flatmaps/main.tf b/terraform/test-fixtures/plan-ignore-changes-with-flatmaps/main.tf new file mode 100644 index 000000000..49885194e --- /dev/null +++ b/terraform/test-fixtures/plan-ignore-changes-with-flatmaps/main.tf @@ -0,0 +1,16 @@ +resource "aws_instance" "foo" { + id = "bar" + user_data = "x" + require_new = "yes" + + set = { + a = "1" + b = "2" + } + + lst = ["j", "k"] + + lifecycle { + ignore_changes = ["require_new"] + } +}