From 074545e5365e773a4b3edc4748e72597080fbd88 Mon Sep 17 00:00:00 2001 From: James Nugent Date: Sun, 5 Jun 2016 09:34:43 +0100 Subject: [PATCH] core: Use .% instead of .# for maps in state The flatmapped representation of state prior to this commit encoded maps and lists (and therefore by extension, sets) with a key corresponding to the number of elements, or the unknown variable indicator under a .# key and then individual items. For example, the list ["a", "b", "c"] would have been encoded as: listname.# = 3 listname.0 = "a" listname.1 = "b" listname.2 = "c" And the map {"key1": "value1", "key2", "value2"} would have been encoded as: mapname.# = 2 mapname.key1 = "value1" mapname.key2 = "value2" Sets use the hash code as the key - for example a set with a (fictional) hashcode calculation may look like: setname.# = 2 setname.12312512 = "value1" setname.56345233 = "value2" Prior to the work done to extend the type system, this was sufficient since the internal representation of these was effectively the same. However, following the separation of maps and lists into distinct first-class types, this encoding presents a problem: given a state file, it is impossible to tell the encoding of an empty list and an empty map apart. This presents problems for the type checker during interpolation, as many interpolation functions will operate on only one of these two structures. This commit therefore changes the representation in state of maps to use a "%" as the key for the number of elements. Consequently the map above will now be encoded as: mapname.% = 2 mapname.key1 = "value1" mapname.key2 = "value2" This has the effect of an empty list (or set) now being encoded as: listname.# = 0 And an empty map now being encoded as: mapname.% = 0 Therefore we can eliminate some nasty guessing logic from the resource variable supplier for interpolation, at the cost of having to migrate state up front (to follow in a subsequent commit). In order to reduce the number of potential situations in which resources would be "forced new", we continue to accept "#" as the count key when reading maps via helper/schema. There is no situation under which we can allow "#" as an actual map key in any case, as it would not be distinguishable from a list or set in state. --- helper/schema/field_reader_diff.go | 2 +- helper/schema/field_reader_diff_test.go | 4 +- helper/schema/field_reader_map.go | 2 +- helper/schema/field_reader_map_test.go | 4 +- helper/schema/field_writer_map.go | 2 +- helper/schema/field_writer_map_test.go | 2 +- helper/schema/resource_data_test.go | 20 +++--- helper/schema/resource_test.go | 2 +- helper/schema/schema.go | 6 +- helper/schema/schema_test.go | 12 ++-- terraform/interpolate.go | 89 ++++++++++++++++--------- terraform/interpolate_test.go | 4 +- 12 files changed, 88 insertions(+), 61 deletions(-) diff --git a/helper/schema/field_reader_diff.go b/helper/schema/field_reader_diff.go index dcb379436..661c5687c 100644 --- a/helper/schema/field_reader_diff.go +++ b/helper/schema/field_reader_diff.go @@ -76,7 +76,7 @@ func (r *DiffFieldReader) readMap( if !strings.HasPrefix(k, prefix) { continue } - if strings.HasPrefix(k, prefix+"#") { + if strings.HasPrefix(k, prefix+"%") { // Ignore the count field continue } diff --git a/helper/schema/field_reader_diff_test.go b/helper/schema/field_reader_diff_test.go index a763e4702..cfd329492 100644 --- a/helper/schema/field_reader_diff_test.go +++ b/helper/schema/field_reader_diff_test.go @@ -22,7 +22,7 @@ func TestDiffFieldReader_MapHandling(t *testing.T) { Schema: schema, Diff: &terraform.InstanceDiff{ Attributes: map[string]*terraform.ResourceAttrDiff{ - "tags.#": &terraform.ResourceAttrDiff{ + "tags.%": &terraform.ResourceAttrDiff{ Old: "1", New: "2", }, @@ -35,7 +35,7 @@ func TestDiffFieldReader_MapHandling(t *testing.T) { Source: &MapFieldReader{ Schema: schema, Map: BasicMapReader(map[string]string{ - "tags.#": "1", + "tags.%": "1", "tags.foo": "bar", }), }, diff --git a/helper/schema/field_reader_map.go b/helper/schema/field_reader_map.go index feb3fcc0a..fc3b5a02f 100644 --- a/helper/schema/field_reader_map.go +++ b/helper/schema/field_reader_map.go @@ -53,7 +53,7 @@ func (r *MapFieldReader) readMap(k string) (FieldReadResult, error) { resultSet = true key := k[len(prefix):] - if key != "#" { + if key != "%" && key != "#" { result[key] = v } } diff --git a/helper/schema/field_reader_map_test.go b/helper/schema/field_reader_map_test.go index 61ffd4484..279b9145b 100644 --- a/helper/schema/field_reader_map_test.go +++ b/helper/schema/field_reader_map_test.go @@ -28,7 +28,7 @@ func TestMapFieldReader(t *testing.T) { "listInt.0": "21", "listInt.1": "42", - "map.#": "2", + "map.%": "2", "map.foo": "bar", "map.bar": "baz", @@ -56,7 +56,7 @@ func TestMapFieldReader_extra(t *testing.T) { Map: BasicMapReader(map[string]string{ "mapDel": "", - "mapEmpty.#": "0", + "mapEmpty.%": "0", }), } diff --git a/helper/schema/field_writer_map.go b/helper/schema/field_writer_map.go index bc8fcd896..4b0efb7d4 100644 --- a/helper/schema/field_writer_map.go +++ b/helper/schema/field_writer_map.go @@ -165,7 +165,7 @@ func (w *MapFieldWriter) setMap( } // Set the count - w.result[k+".#"] = strconv.Itoa(len(vs)) + w.result[k+".%"] = strconv.Itoa(len(vs)) return nil } diff --git a/helper/schema/field_writer_map_test.go b/helper/schema/field_writer_map_test.go index 783c7435f..5fc308aa7 100644 --- a/helper/schema/field_writer_map_test.go +++ b/helper/schema/field_writer_map_test.go @@ -161,7 +161,7 @@ func TestMapFieldWriter(t *testing.T) { map[string]interface{}{"foo": "bar"}, false, map[string]string{ - "map.#": "1", + "map.%": "1", "map.foo": "bar", }, }, diff --git a/helper/schema/resource_data_test.go b/helper/schema/resource_data_test.go index 4e728021f..643a60a1a 100644 --- a/helper/schema/resource_data_test.go +++ b/helper/schema/resource_data_test.go @@ -1987,10 +1987,10 @@ func TestResourceDataState(t *testing.T) { State: &terraform.InstanceState{ Attributes: map[string]string{ "config_vars.#": "2", - "config_vars.0.#": "2", + "config_vars.0.%": "2", "config_vars.0.foo": "bar", "config_vars.0.bar": "bar", - "config_vars.1.#": "1", + "config_vars.1.%": "1", "config_vars.1.bar": "baz", }, }, @@ -2017,9 +2017,9 @@ func TestResourceDataState(t *testing.T) { Result: &terraform.InstanceState{ Attributes: map[string]string{ "config_vars.#": "2", - "config_vars.0.#": "1", + "config_vars.0.%": "1", "config_vars.0.foo": "bar", - "config_vars.1.#": "1", + "config_vars.1.%": "1", "config_vars.1.baz": "bang", }, }, @@ -2444,10 +2444,10 @@ func TestResourceDataState(t *testing.T) { Attributes: map[string]string{ // TODO: broken, shouldn't bar be removed? "config_vars.#": "2", - "config_vars.0.#": "2", + "config_vars.0.%": "2", "config_vars.0.foo": "bar", "config_vars.0.bar": "bar", - "config_vars.1.#": "1", + "config_vars.1.%": "1", "config_vars.1.bar": "baz", }, }, @@ -2551,7 +2551,7 @@ func TestResourceDataState(t *testing.T) { Result: &terraform.InstanceState{ Attributes: map[string]string{ - "tags.#": "1", + "tags.%": "1", "tags.Name": "foo", }, }, @@ -2584,7 +2584,7 @@ func TestResourceDataState(t *testing.T) { Result: &terraform.InstanceState{ Attributes: map[string]string{ - "tags.#": "0", + "tags.%": "0", }, }, }, @@ -2690,7 +2690,7 @@ func TestResourceDataState(t *testing.T) { Attributes: map[string]string{ "ports.#": "1", "ports.10.index": "10", - "ports.10.uuids.#": "1", + "ports.10.uuids.%": "1", "ports.10.uuids.80": "value", }, }, @@ -2831,7 +2831,7 @@ func TestResourceDataState(t *testing.T) { Attributes: map[string]string{ "ports.#": "1", "ports.0.index": "10", - "ports.0.uuids.#": "1", + "ports.0.uuids.%": "1", "ports.0.uuids.80": "value", }, }, diff --git a/helper/schema/resource_test.go b/helper/schema/resource_test.go index 359705069..b6c044036 100644 --- a/helper/schema/resource_test.go +++ b/helper/schema/resource_test.go @@ -162,7 +162,7 @@ func TestResourceApply_destroyCreate(t *testing.T) { Attributes: map[string]string{ "id": "foo", "foo": "42", - "tags.#": "1", + "tags.%": "1", "tags.Name": "foo", }, } diff --git a/helper/schema/schema.go b/helper/schema/schema.go index 703d17956..b2934a763 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -760,8 +760,8 @@ func (m schemaMap) diffMap( stateExists := o != nil // Delete any count values, since we don't use those - delete(configMap, "#") - delete(stateMap, "#") + delete(configMap, "%") + delete(stateMap, "%") // Check if the number of elements has changed. oldLen, newLen := len(stateMap), len(configMap) @@ -795,7 +795,7 @@ func (m schemaMap) diffMap( oldStr = "" } - diff.Attributes[k+".#"] = countSchema.finalizeDiff( + diff.Attributes[k+".%"] = countSchema.finalizeDiff( &terraform.ResourceAttrDiff{ Old: oldStr, New: newStr, diff --git a/helper/schema/schema_test.go b/helper/schema/schema_test.go index 2ec1aa78a..9dac216f8 100644 --- a/helper/schema/schema_test.go +++ b/helper/schema/schema_test.go @@ -1297,7 +1297,7 @@ func TestSchemaMap_Diff(t *testing.T) { Diff: &terraform.InstanceDiff{ Attributes: map[string]*terraform.ResourceAttrDiff{ - "config_vars.#": &terraform.ResourceAttrDiff{ + "config_vars.%": &terraform.ResourceAttrDiff{ Old: "0", New: "1", }, @@ -1473,7 +1473,7 @@ func TestSchemaMap_Diff(t *testing.T) { Old: "1", New: "0", }, - "config_vars.0.#": &terraform.ResourceAttrDiff{ + "config_vars.0.%": &terraform.ResourceAttrDiff{ Old: "2", New: "0", }, @@ -1763,7 +1763,7 @@ func TestSchemaMap_Diff(t *testing.T) { Diff: &terraform.InstanceDiff{ Attributes: map[string]*terraform.ResourceAttrDiff{ - "vars.#": &terraform.ResourceAttrDiff{ + "vars.%": &terraform.ResourceAttrDiff{ Old: "", NewComputed: true, }, @@ -1783,7 +1783,7 @@ func TestSchemaMap_Diff(t *testing.T) { State: &terraform.InstanceState{ Attributes: map[string]string{ - "vars.#": "0", + "vars.%": "0", }, }, @@ -1799,7 +1799,7 @@ func TestSchemaMap_Diff(t *testing.T) { Diff: &terraform.InstanceDiff{ Attributes: map[string]*terraform.ResourceAttrDiff{ - "vars.#": &terraform.ResourceAttrDiff{ + "vars.%": &terraform.ResourceAttrDiff{ Old: "", NewComputed: true, }, @@ -2046,7 +2046,7 @@ func TestSchemaMap_Diff(t *testing.T) { Diff: &terraform.InstanceDiff{ Attributes: map[string]*terraform.ResourceAttrDiff{ - "vars.#": &terraform.ResourceAttrDiff{ + "vars.%": &terraform.ResourceAttrDiff{ Old: "0", New: "1", }, diff --git a/terraform/interpolate.go b/terraform/interpolate.go index 2d3c57c5b..9593080d6 100644 --- a/terraform/interpolate.go +++ b/terraform/interpolate.go @@ -353,6 +353,10 @@ func (i *Interpolater) computeResourceVariable( unknownVariable := unknownVariable() + // These variables must be declared early because of the use of GOTO + var isList bool + var isMap bool + // Get the information about this resource variable, and verify // that it exists and such. module, _, err := i.resourceVariableInfo(scope, v) @@ -388,7 +392,9 @@ func (i *Interpolater) computeResourceVariable( } // computed list or map attribute - if _, ok := r.Primary.Attributes[v.Field+".#"]; ok { + _, isList = r.Primary.Attributes[v.Field+".#"] + _, isMap = r.Primary.Attributes[v.Field+".%"] + if isList || isMap { variable, err := i.interpolateComplexTypeAttribute(v.Field, r.Primary.Attributes) return &variable, err } @@ -566,49 +572,70 @@ func (i *Interpolater) interpolateComplexTypeAttribute( resourceID string, attributes map[string]string) (ast.Variable, error) { - attr := attributes[resourceID+".#"] - log.Printf("[DEBUG] Interpolating computed complex type attribute %s (%s)", - resourceID, attr) + // We can now distinguish between lists and maps in state by the count field: + // - lists (and by extension, sets) use the traditional .# notation + // - maps use the newer .% notation + // Consequently here we can decide how to deal with the keys appropriately + // based on whether the type is a map of list. + if lengthAttr, isList := attributes[resourceID+".#"]; isList { + log.Printf("[DEBUG] Interpolating computed list element attribute %s (%s)", + resourceID, lengthAttr) - // In Terraform's internal dotted representation of list-like attributes, the - // ".#" count field is marked as unknown to indicate "this whole list is - // unknown". We must honor that meaning here so computed references can be - // treated properly during the plan phase. - if attr == config.UnknownVariableValue { - return unknownVariable(), nil - } - - // At this stage we don't know whether the item is a list or a map, so we - // examine the keys to see whether they are all numeric. - var numericKeys []string - var allKeys []string - numberedListKey := regexp.MustCompile("^" + resourceID + "\\.[0-9]+$") - otherListKey := regexp.MustCompile("^" + resourceID + "\\.([^#]+)$") - for id, _ := range attributes { - if numberedListKey.MatchString(id) { - numericKeys = append(numericKeys, id) + // In Terraform's internal dotted representation of list-like attributes, the + // ".#" count field is marked as unknown to indicate "this whole list is + // unknown". We must honor that meaning here so computed references can be + // treated properly during the plan phase. + if lengthAttr == config.UnknownVariableValue { + return unknownVariable(), nil + } + + var keys []string + listElementKey := regexp.MustCompile("^" + resourceID + "\\.[0-9]+$") + for id, _ := range attributes { + if listElementKey.MatchString(id) { + keys = append(keys, id) + } } - if submatches := otherListKey.FindAllStringSubmatch(id, -1); len(submatches) > 0 { - allKeys = append(allKeys, submatches[0][1]) - } - } - if len(numericKeys) == len(allKeys) { - // This is a list var members []string - for _, key := range numericKeys { + for _, key := range keys { members = append(members, attributes[key]) } + // This behaviour still seems very broken to me... it retains BC but is + // probably going to cause problems in future sort.Strings(members) + return hil.InterfaceToVariable(members) - } else { - // This is a map + } + + if lengthAttr, isMap := attributes[resourceID+".%"]; isMap { + log.Printf("[DEBUG] Interpolating computed map element attribute %s (%s)", + resourceID, lengthAttr) + + // In Terraform's internal dotted representation of map attributes, the + // ".%" count field is marked as unknown to indicate "this whole list is + // unknown". We must honor that meaning here so computed references can be + // treated properly during the plan phase. + if lengthAttr == config.UnknownVariableValue { + return unknownVariable(), nil + } + + var keys []string + mapElementKey := regexp.MustCompile("^" + resourceID + "\\.([^%]+)$") + for id, _ := range attributes { + if submatches := mapElementKey.FindAllStringSubmatch(id, -1); len(submatches) > 0 { + keys = append(keys, submatches[0][1]) + } + } + members := make(map[string]interface{}) - for _, key := range allKeys { + for _, key := range keys { members[key] = attributes[resourceID+"."+key] } return hil.InterfaceToVariable(members) } + + return ast.Variable{}, fmt.Errorf("No complex type %s found", resourceID) } func (i *Interpolater) resourceVariableInfo( diff --git a/terraform/interpolate_test.go b/terraform/interpolate_test.go index df87890f4..f6de81060 100644 --- a/terraform/interpolate_test.go +++ b/terraform/interpolate_test.go @@ -294,7 +294,7 @@ func TestInterpolator_resourceMultiAttributes(t *testing.T) { "name_servers.3": "ns-601.awsdns-11.net", "listeners.#": "1", "listeners.0": "red", - "tags.#": "1", + "tags.%": "1", "tags.Name": "reindeer", "nothing.#": "0", }, @@ -529,7 +529,7 @@ func testInterpolate( } if !reflect.DeepEqual(actual, expected) { spew.Config.DisableMethods = true - t.Fatalf("%q: actual: %#v\nexpected: %#v\n\n%s\n\n%s\n\n", n, actual, expected, + t.Fatalf("%q:\n\n actual: %#v\nexpected: %#v\n\n%s\n\n%s\n\n", n, actual, expected, spew.Sdump(actual), spew.Sdump(expected)) } }