From ad0b81de81f5624e1693e27694f4a7f41387c939 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 29 Sep 2020 15:36:49 -0400 Subject: [PATCH 1/2] allow ignore_changes to reference any map key There are situations when a user may want to keep or exclude a map key using `ignore_changes` which may not be listed directly in the configuration. This didn't work previously because the transformation always started off with the configuration, and would never encounter a key if it was only present in the prior value. --- terraform/eval_diff.go | 141 ++++++++++++++++++++++++++------ terraform/eval_diff_test.go | 158 ++++++++++++++++++++++++++++++++++++ 2 files changed, 273 insertions(+), 26 deletions(-) diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index aec3fba97..190510009 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -607,38 +607,127 @@ func processIgnoreChangesIndividual(prior, config cty.Value, ignoreChanges []hcl ignoreChangesPath[i] = path } - var diags tfdiags.Diagnostics - ret, _ := cty.Transform(config, func(path cty.Path, v cty.Value) (cty.Value, error) { - // First we must see if this is a path that's being ignored at all. - // We're looking for an exact match here because this walk will visit - // leaf values first and then their containers, and we want to do - // the "ignore" transform once we reach the point indicated, throwing - // away any deeper values we already produced at that point. - var ignoreTraversal hcl.Traversal - for i, candidate := range ignoreChangesPath { - if path.Equals(candidate) { - ignoreTraversal = ignoreChanges[i] + type ignoreChange struct { + // Path is the full path, minus any trailing map index + path cty.Path + // Value is the value we are to retain at the above path. If there is a + // key value, this must be a map and the desired value will be at the + // key index. + value cty.Value + // Key is the index key if the ignored path ends in a map index. + key cty.Value + } + var ignoredValues []ignoreChange + + // Find the actual changes first and store them in the ignoreChange struct. + // If the change was to a map value, and the key doesn't exist in the + // config, it would never be visited in the transform walk. + for _, icPath := range ignoreChangesPath { + key := cty.NullVal(cty.String) + // check for a map index, since maps are the only structure where we + // could have invalid path steps. + last, ok := icPath[len(icPath)-1].(cty.IndexStep) + if ok { + if last.Key.Type() == cty.String { + icPath = icPath[:len(icPath)-1] + key = last.Key } } - if ignoreTraversal == nil { - return v, nil + + // The structure should have been validated already, and we already + // trimmed the trailing map index. Any other intermediate index error + // means we wouldn't be able to apply the value below, so no need to + // record this. + p, err := icPath.Apply(prior) + if err != nil { + continue + } + c, err := icPath.Apply(config) + if err != nil { + continue } - // If we're able to follow the same path through the prior value, - // we'll take the value there instead, effectively undoing the - // change that was planned. - priorV, diags := hcl.ApplyPath(prior, path, nil) - if diags.HasErrors() { - // We just ignore the errors and move on here, since we assume it's - // just because the prior value was a slightly-different shape. - // It could potentially also be that the traversal doesn't match - // the schema, but we should've caught that during the validate - // walk if so. - return v, nil + // If this is a map, it is checking the entire map value for equality + // rather than the individual key. This means that the change is stored + // here even if our ignored key doesn't change. That is OK since it + // won't cause any changes in the transformation, but allows us to skip + // breaking up the maps and checking for key existence here too. + eq := p.Equals(c) + if eq.IsKnown() && eq.False() { + // there a change to ignore at this path, store the prior value + ignoredValues = append(ignoredValues, ignoreChange{icPath, p, key}) } - return priorV, nil + } + + if len(ignoredValues) == 0 { + return config, nil + } + + ret, _ := cty.Transform(config, func(path cty.Path, v cty.Value) (cty.Value, error) { + for _, ignored := range ignoredValues { + if !path.Equals(ignored.path) { + return v, nil + } + + // no index, so we can return the entire value + if ignored.key.IsNull() { + return ignored.value, nil + } + + // we have an index key, so make sure we have a map + if !v.Type().IsMapType() { + // we'll let other validation catch any type mismatch + return v, nil + } + + // Now we know we are ignoring a specific index of this map, so get + // the config map and modify, add, or remove the desired key. + var configMap map[string]cty.Value + var priorMap map[string]cty.Value + + if !v.IsNull() { + if !v.IsKnown() { + // if the entire map is not known, we can't ignore any + // specific keys yet. + continue + } + configMap = v.AsValueMap() + } + if configMap == nil { + configMap = map[string]cty.Value{} + } + + // We also need to create a prior map, so we can check for + // existence while getting the value. Value.Index will always + // return null. + if !ignored.value.IsNull() { + priorMap = ignored.value.AsValueMap() + } + if priorMap == nil { + priorMap = map[string]cty.Value{} + } + + key := ignored.key.AsString() + priorElem, keep := priorMap[key] + + switch { + case !keep: + // this didn't exist in the old map value, so we're keeping the + // "absence" of the key by removing it from the config + delete(configMap, key) + default: + configMap[key] = priorElem + } + + if len(configMap) == 0 { + return cty.MapValEmpty(v.Type().ElementType()), nil + } + + return cty.MapVal(configMap), nil + } + return v, nil }) - return ret, diags + return ret, nil } // a group of key-*ResourceAttrDiff pairs from the same flatmapped container diff --git a/terraform/eval_diff_test.go b/terraform/eval_diff_test.go index afc60a9d3..8f2a100d1 100644 --- a/terraform/eval_diff_test.go +++ b/terraform/eval_diff_test.go @@ -68,6 +68,164 @@ func TestProcessIgnoreChangesIndividual(t *testing.T) { "b": cty.StringVal("new b value"), }), }, + "list_index": { + cty.ObjectVal(map[string]cty.Value{ + "a": cty.ListVal([]cty.Value{ + cty.StringVal("a0 value"), + cty.StringVal("a1 value"), + }), + "b": cty.StringVal("b value"), + }), + cty.ObjectVal(map[string]cty.Value{ + "a": cty.ListVal([]cty.Value{ + cty.StringVal("new a0 value"), + cty.StringVal("new a1 value"), + }), + "b": cty.StringVal("new b value"), + }), + []string{"a[1]"}, + cty.ObjectVal(map[string]cty.Value{ + "a": cty.ListVal([]cty.Value{ + cty.StringVal("new a0 value"), + cty.StringVal("a1 value"), + }), + "b": cty.StringVal("new b value"), + }), + }, + "map_index": { + cty.ObjectVal(map[string]cty.Value{ + "a": cty.MapVal(map[string]cty.Value{ + "a0": cty.StringVal("a0 value"), + "a1": cty.StringVal("a1 value"), + }), + "b": cty.StringVal("b value"), + }), + cty.ObjectVal(map[string]cty.Value{ + "a": cty.MapVal(map[string]cty.Value{ + "a0": cty.StringVal("new a0 value"), + "a1": cty.StringVal("new a1 value"), + }), + "b": cty.StringVal("b value"), + }), + []string{`a["a1"]`}, + cty.ObjectVal(map[string]cty.Value{ + "a": cty.MapVal(map[string]cty.Value{ + "a0": cty.StringVal("new a0 value"), + "a1": cty.StringVal("a1 value"), + }), + "b": cty.StringVal("b value"), + }), + }, + "map_index_no_config": { + cty.ObjectVal(map[string]cty.Value{ + "a": cty.MapVal(map[string]cty.Value{ + "a0": cty.StringVal("a0 value"), + "a1": cty.StringVal("a1 value"), + }), + "b": cty.StringVal("b value"), + }), + cty.ObjectVal(map[string]cty.Value{ + "a": cty.NullVal(cty.Map(cty.String)), + "b": cty.StringVal("b value"), + }), + []string{`a["a1"]`}, + cty.ObjectVal(map[string]cty.Value{ + "a": cty.MapVal(map[string]cty.Value{ + "a1": cty.StringVal("a1 value"), + }), + "b": cty.StringVal("b value"), + }), + }, + "missing_map_index": { + cty.ObjectVal(map[string]cty.Value{ + "a": cty.MapVal(map[string]cty.Value{ + "a0": cty.StringVal("a0 value"), + "a1": cty.StringVal("a1 value"), + }), + "b": cty.StringVal("b value"), + }), + cty.ObjectVal(map[string]cty.Value{ + "a": cty.MapValEmpty(cty.String), + "b": cty.StringVal("b value"), + }), + []string{`a["a1"]`}, + cty.ObjectVal(map[string]cty.Value{ + "a": cty.MapVal(map[string]cty.Value{ + "a1": cty.StringVal("a1 value"), + }), + "b": cty.StringVal("b value"), + }), + }, + "missing_map_index_empty": { + cty.ObjectVal(map[string]cty.Value{ + "a": cty.MapValEmpty(cty.String), + }), + cty.ObjectVal(map[string]cty.Value{ + "a": cty.MapVal(map[string]cty.Value{ + "a": cty.StringVal("a0 value"), + }), + }), + []string{`a["a"]`}, + cty.ObjectVal(map[string]cty.Value{ + "a": cty.MapValEmpty(cty.String), + }), + }, + "missing_map_index_to_object": { + cty.ObjectVal(map[string]cty.Value{ + "a": cty.MapVal(map[string]cty.Value{ + "a": cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("aa0"), + "b": cty.StringVal("ab0"), + }), + "b": cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("ba0"), + "b": cty.StringVal("bb0"), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "a": cty.MapValEmpty( + cty.Object(map[string]cty.Type{ + "a": cty.String, + "b": cty.String, + }), + ), + }), + // we expect the config to be used here, as the ignore changes was + // `a["a"].b`, but the change was larger than that removing + // `a["a"]` entirely. + []string{`a["a"].b`}, + cty.ObjectVal(map[string]cty.Value{ + "a": cty.MapValEmpty( + cty.Object(map[string]cty.Type{ + "a": cty.String, + "b": cty.String, + }), + ), + }), + }, + "missing_prior_map_index": { + cty.ObjectVal(map[string]cty.Value{ + "a": cty.MapVal(map[string]cty.Value{ + "a0": cty.StringVal("a0 value"), + }), + "b": cty.StringVal("b value"), + }), + cty.ObjectVal(map[string]cty.Value{ + "a": cty.MapVal(map[string]cty.Value{ + "a0": cty.StringVal("a0 value"), + "a1": cty.StringVal("new a1 value"), + }), + "b": cty.StringVal("b value"), + }), + []string{`a["a1"]`}, + cty.ObjectVal(map[string]cty.Value{ + "a": cty.MapVal(map[string]cty.Value{ + "a0": cty.StringVal("a0 value"), + }), + "b": cty.StringVal("b value"), + }), + }, "object attribute": { cty.ObjectVal(map[string]cty.Value{ "a": cty.ObjectVal(map[string]cty.Value{ From 82793f8158e812010980147e3f22f2e2ed001fbf Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 30 Sep 2020 09:46:10 -0400 Subject: [PATCH 2/2] update ignore_changes doc We can remove the caveat about changing map elements. Add a little more text about the intended use case for ignore_changes, as the common case of fixing erroneous provider behavior should not be the primary motivation for the maintenance of this feature. --- website/docs/configuration/resources.html.md | 49 ++++++-------------- 1 file changed, 13 insertions(+), 36 deletions(-) diff --git a/website/docs/configuration/resources.html.md b/website/docs/configuration/resources.html.md index d5bc07618..04570885c 100644 --- a/website/docs/configuration/resources.html.md +++ b/website/docs/configuration/resources.html.md @@ -638,16 +638,22 @@ meta-arguments are supported: any difference in the current settings of a real infrastructure object and plans to update the remote object to match configuration. - In some rare cases, settings of a remote object are modified by processes - outside of Terraform, which Terraform would then attempt to "fix" on the - next run. In order to make Terraform share management responsibilities - of a single object with a separate process, the `ignore_changes` - meta-argument specifies resource attributes that Terraform should ignore - when planning updates to the associated remote object. + The `ignore_chages` feature is intended to be used when a resource is + created with references to data that may change in the future, but should + not effect said resource after its creation. In some rare cases, settings + of a remote object are modified by processes outside of Terraform, which + Terraform would then attempt to "fix" on the next run. In order to make + Terraform share management responsibilities of a single object with a + separate process, the `ignore_changes` meta-argument specifies resource + attributes that Terraform should ignore when planning updates to the + associated remote object. The arguments corresponding to the given attribute names are considered when planning a _create_ operation, but are ignored when planning an - _update_. + _update_. The arugments are the relative address of the attributes in the + resource. Map and list elements can be referenced using index notation, + like `tags["Name"]` and `list[0]` respectively. + ```hcl resource "aws_instance" "example" { @@ -663,35 +669,6 @@ meta-arguments are supported: } ``` - You can also ignore specific map elements by writing references like - `tags["Name"]` in the `ignore_changes` list, though with an important - caveat: the ignoring applies only to in-place updates to an existing - key. Adding or removing a key is treated by Terraform as a change to the - containing map itself rather than to the individual key, and so if you - wish to ignore changes to a particular tag made by an external system - you must ensure that the Terraform configuration creates a placeholder - element for that tag name so that the external system changes will be - understood as an in-place edit of that key: - - ```hcl - resource "aws_instance" "example" { - # ... - - tags = { - # Initial value for Name is overridden by our automatic scheduled - # re-tagging process; changes to this are ignored by ignore_changes - # below. - Name = "placeholder" - } - - lifecycle { - ignore_changes = [ - tags["Name"], - ] - } - } - ``` - Instead of a list, the special keyword `all` may be used to instruct Terraform to ignore _all_ attributes, which means that Terraform can create and destroy the remote object but will never propose updates to it.