From 4d8208d84004c065df0d302ee68dd4aef4b4d387 Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Sun, 2 Oct 2016 11:02:09 -0700 Subject: [PATCH 1/5] core: Ensure hasComputedSubKeys iterates over Sets and Lists properly This fixes some edge-ish cases where a set in a config has a set or list in it that contains computed values, but non-set or list values in the parent do not. This can cause "diffs didn't match during apply" errors in a scenario such as when a set's hash is calculated off of child items (including any sub-lists or sets, as it should be), and the hash changes between the plan and apply diffs due to the computed values present in the sub-list or set items. These will be marked as computed, but due to the fact that the function was not iterating over the list or set items properly (ie: not adding the item number to the address, so set.0.set.foo was being yielded instead of set.0.set.0.foo), these computed values were not being properly propagated to the parent set to be marked as computed. Fixes hashicorp/terraform#6527. Fixes hashicorp/terraform#8271. This possibly fixes other non-CloudFront related issues too. --- helper/schema/field_reader_config.go | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/helper/schema/field_reader_config.go b/helper/schema/field_reader_config.go index 0d4c2a97c..3e62a70a5 100644 --- a/helper/schema/field_reader_config.go +++ b/helper/schema/field_reader_config.go @@ -265,12 +265,30 @@ func (r *ConfigFieldReader) hasComputedSubKeys(key string, schema *Schema) bool switch t := schema.Elem.(type) { case *Resource: for k, schema := range t.Schema { - if r.Config.IsComputed(prefix + k) { + addr := prefix + k + if r.Config.IsComputed(addr) { return true } - if r.hasComputedSubKeys(prefix+k, schema) { - return true + // We need to loop into sets and lists to ensure we pass the correct + // address to the raw config - otherwise for sets we get something like + // set.0.set.item instead of set.0.set.0.item, which renders an + // inaccurate result. + if schema.Type == TypeSet || schema.Type == TypeList { + raw, err := readListField(&nestedConfigFieldReader{r}, strings.Split(addr, "."), schema) + if err != nil { + panic(fmt.Errorf("readListField failed when field was supposed to be list-like: %v", err)) + } + // Just range into the address space here, we don't need the value. + for i := range raw.Value.([]interface{}) { + if r.hasComputedSubKeys(addr+"."+strconv.Itoa(i), schema) { + return true + } + } + } else { + if r.hasComputedSubKeys(addr, schema) { + return true + } } } } From f25805673121acf6ae5eee3625dc5d6ef18eec46 Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Sun, 2 Oct 2016 13:59:56 -0700 Subject: [PATCH 2/5] core: Tests for hasComputedSubKeys fix This covers: * Complex sets with computed fields in a set * Complex lists with computed fields in a set Adding a test to test basic lists with computed fields seemed to fail, but possibly for an unrelated reason (the list returned as nil). The fix to this inparticular case may be out of the scope of this specific issue. Reference gist and details in hashicorp/terraform#9171. --- helper/schema/schema_test.go | 151 +++++++++++++++++++++++++++++++++++ 1 file changed, 151 insertions(+) diff --git a/helper/schema/schema_test.go b/helper/schema/schema_test.go index 94b6b4dab..872273f99 100644 --- a/helper/schema/schema_test.go +++ b/helper/schema/schema_test.go @@ -3267,6 +3267,157 @@ func TestSchemaMap_DiffSuppress(t *testing.T) { Err: false, }, + + "Complex structure with set of computed string should mark root set as computed": { + Schema: map[string]*Schema{ + "outer": &Schema{ + Type: TypeSet, + Optional: true, + Elem: &Resource{ + Schema: map[string]*Schema{ + "outer_str": &Schema{ + Type: TypeString, + Optional: true, + }, + "inner": &Schema{ + Type: TypeSet, + Optional: true, + Elem: &Resource{ + Schema: map[string]*Schema{ + "inner_str": &Schema{ + Type: TypeString, + Optional: true, + }, + }, + }, + Set: func(v interface{}) int { + return 2 + }, + }, + }, + }, + Set: func(v interface{}) int { + return 1 + }, + }, + }, + + State: nil, + + Config: map[string]interface{}{ + "outer": []map[string]interface{}{ + map[string]interface{}{ + "outer_str": "foo", + "inner": []map[string]interface{}{ + map[string]interface{}{ + "inner_str": "${var.bar}", + }, + }, + }, + }, + }, + + ConfigVariables: map[string]ast.Variable{ + "var.bar": interfaceToVariableSwallowError(config.UnknownVariableValue), + }, + + Diff: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "outer.#": &terraform.ResourceAttrDiff{ + Old: "0", + New: "1", + }, + "outer.~1.outer_str": &terraform.ResourceAttrDiff{ + Old: "", + New: "foo", + }, + "outer.~1.inner.#": &terraform.ResourceAttrDiff{ + Old: "0", + New: "1", + }, + "outer.~1.inner.~2.inner_str": &terraform.ResourceAttrDiff{ + Old: "", + New: "${var.bar}", + }, + }, + }, + + Err: false, + }, + + "Complex structure with complex list of computed string should mark root set as computed": { + Schema: map[string]*Schema{ + "outer": &Schema{ + Type: TypeSet, + Optional: true, + Elem: &Resource{ + Schema: map[string]*Schema{ + "outer_str": &Schema{ + Type: TypeString, + Optional: true, + }, + "inner": &Schema{ + Type: TypeList, + Optional: true, + Elem: &Resource{ + Schema: map[string]*Schema{ + "inner_str": &Schema{ + Type: TypeString, + Optional: true, + }, + }, + }, + }, + }, + }, + Set: func(v interface{}) int { + return 1 + }, + }, + }, + + State: nil, + + Config: map[string]interface{}{ + "outer": []map[string]interface{}{ + map[string]interface{}{ + "outer_str": "foo", + "inner": []map[string]interface{}{ + map[string]interface{}{ + "inner_str": "${var.bar}", + }, + }, + }, + }, + }, + + ConfigVariables: map[string]ast.Variable{ + "var.bar": interfaceToVariableSwallowError(config.UnknownVariableValue), + }, + + Diff: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "outer.#": &terraform.ResourceAttrDiff{ + Old: "0", + New: "1", + }, + "outer.~1.outer_str": &terraform.ResourceAttrDiff{ + Old: "", + New: "foo", + }, + "outer.~1.inner.#": &terraform.ResourceAttrDiff{ + Old: "0", + New: "1", + }, + "outer.~1.inner.0.inner_str": &terraform.ResourceAttrDiff{ + Old: "", + New: "${var.bar}", + }, + }, + }, + + Err: false, + }, } for tn, tc := range cases { From 8d06d68d0f905d345fb7bdfbb185c2063163c12f Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Sat, 19 Nov 2016 09:29:48 -0800 Subject: [PATCH 3/5] core: Backport NewComputed change to nested list/set tests Needed due to work done in 95d37ea, we may need to adjust hasComputedSubKeys to propagate NewComputed in the same way that we have added "~", however will wait for comment from @mitchellh. --- helper/schema/schema_test.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/helper/schema/schema_test.go b/helper/schema/schema_test.go index 872273f99..33ac26f76 100644 --- a/helper/schema/schema_test.go +++ b/helper/schema/schema_test.go @@ -3321,7 +3321,7 @@ func TestSchemaMap_DiffSuppress(t *testing.T) { "var.bar": interfaceToVariableSwallowError(config.UnknownVariableValue), }, - Diff: &terraform.InstanceDiff{ + ExpectedDiff: &terraform.InstanceDiff{ Attributes: map[string]*terraform.ResourceAttrDiff{ "outer.#": &terraform.ResourceAttrDiff{ Old: "0", @@ -3336,8 +3336,9 @@ func TestSchemaMap_DiffSuppress(t *testing.T) { New: "1", }, "outer.~1.inner.~2.inner_str": &terraform.ResourceAttrDiff{ - Old: "", - New: "${var.bar}", + Old: "", + New: "${var.bar}", + NewComputed: true, }, }, }, @@ -3395,7 +3396,7 @@ func TestSchemaMap_DiffSuppress(t *testing.T) { "var.bar": interfaceToVariableSwallowError(config.UnknownVariableValue), }, - Diff: &terraform.InstanceDiff{ + ExpectedDiff: &terraform.InstanceDiff{ Attributes: map[string]*terraform.ResourceAttrDiff{ "outer.#": &terraform.ResourceAttrDiff{ Old: "0", @@ -3410,8 +3411,9 @@ func TestSchemaMap_DiffSuppress(t *testing.T) { New: "1", }, "outer.~1.inner.0.inner_str": &terraform.ResourceAttrDiff{ - Old: "", - New: "${var.bar}", + Old: "", + New: "${var.bar}", + NewComputed: true, }, }, }, From 0634aada6996bc1509f8838cbbddc8e11e35445c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 21 Nov 2016 17:31:19 -0800 Subject: [PATCH 4/5] Revert "core: Ensure hasComputedSubKeys iterates over Sets and Lists properly" This reverts commit 4d8208d84004c065df0d302ee68dd4aef4b4d387. --- helper/schema/field_reader_config.go | 24 +++--------------------- 1 file changed, 3 insertions(+), 21 deletions(-) diff --git a/helper/schema/field_reader_config.go b/helper/schema/field_reader_config.go index 3a08adcb5..042fe9d0e 100644 --- a/helper/schema/field_reader_config.go +++ b/helper/schema/field_reader_config.go @@ -270,30 +270,12 @@ func (r *ConfigFieldReader) hasComputedSubKeys(key string, schema *Schema) bool switch t := schema.Elem.(type) { case *Resource: for k, schema := range t.Schema { - addr := prefix + k - if r.Config.IsComputed(addr) { + if r.Config.IsComputed(prefix + k) { return true } - // We need to loop into sets and lists to ensure we pass the correct - // address to the raw config - otherwise for sets we get something like - // set.0.set.item instead of set.0.set.0.item, which renders an - // inaccurate result. - if schema.Type == TypeSet || schema.Type == TypeList { - raw, err := readListField(&nestedConfigFieldReader{r}, strings.Split(addr, "."), schema) - if err != nil { - panic(fmt.Errorf("readListField failed when field was supposed to be list-like: %v", err)) - } - // Just range into the address space here, we don't need the value. - for i := range raw.Value.([]interface{}) { - if r.hasComputedSubKeys(addr+"."+strconv.Itoa(i), schema) { - return true - } - } - } else { - if r.hasComputedSubKeys(addr, schema) { - return true - } + if r.hasComputedSubKeys(prefix+k, schema) { + return true } } } From 71a1e215d902a074cc7797a06db7daca7ae2124d Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 21 Nov 2016 17:31:47 -0800 Subject: [PATCH 5/5] terraform: add more Same test cases to cover #9171 --- terraform/diff_test.go | 94 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/terraform/diff_test.go b/terraform/diff_test.go index ae5ce6da4..8f29ad08d 100644 --- a/terraform/diff_test.go +++ b/terraform/diff_test.go @@ -991,6 +991,100 @@ func TestInstanceDiffSame(t *testing.T) { true, "", }, + + // Innner computed set should allow outer change in key + { + &InstanceDiff{ + Attributes: map[string]*ResourceAttrDiff{ + "foo.#": &ResourceAttrDiff{ + Old: "0", + New: "1", + }, + "foo.~1.outer_val": &ResourceAttrDiff{ + Old: "", + New: "foo", + }, + "foo.~1.inner.#": &ResourceAttrDiff{ + Old: "0", + New: "1", + }, + "foo.~1.inner.~2.value": &ResourceAttrDiff{ + Old: "", + New: "${var.bar}", + NewComputed: true, + }, + }, + }, + &InstanceDiff{ + Attributes: map[string]*ResourceAttrDiff{ + "foo.#": &ResourceAttrDiff{ + Old: "0", + New: "1", + }, + "foo.12.outer_val": &ResourceAttrDiff{ + Old: "", + New: "foo", + }, + "foo.12.inner.#": &ResourceAttrDiff{ + Old: "0", + New: "1", + }, + "foo.12.inner.42.value": &ResourceAttrDiff{ + Old: "", + New: "baz", + }, + }, + }, + true, + "", + }, + + // Innner computed list should allow outer change in key + { + &InstanceDiff{ + Attributes: map[string]*ResourceAttrDiff{ + "foo.#": &ResourceAttrDiff{ + Old: "0", + New: "1", + }, + "foo.~1.outer_val": &ResourceAttrDiff{ + Old: "", + New: "foo", + }, + "foo.~1.inner.#": &ResourceAttrDiff{ + Old: "0", + New: "1", + }, + "foo.~1.inner.0.value": &ResourceAttrDiff{ + Old: "", + New: "${var.bar}", + NewComputed: true, + }, + }, + }, + &InstanceDiff{ + Attributes: map[string]*ResourceAttrDiff{ + "foo.#": &ResourceAttrDiff{ + Old: "0", + New: "1", + }, + "foo.12.outer_val": &ResourceAttrDiff{ + Old: "", + New: "foo", + }, + "foo.12.inner.#": &ResourceAttrDiff{ + Old: "0", + New: "1", + }, + "foo.12.inner.0.value": &ResourceAttrDiff{ + Old: "", + New: "baz", + }, + }, + }, + true, + "", + }, } for i, tc := range cases {