From 3d0073e05c89777ffbfb609615131e603e42427c Mon Sep 17 00:00:00 2001 From: Sander van Harmelen Date: Fri, 14 Apr 2017 22:32:30 +0200 Subject: [PATCH] core: fix a crash by suggesting a different approach to solve #11170 (#13541) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Revert #11245, #11321, #11498 and #11757 These PR’s are all related to issue #11170 for which I would like to propose a different solution then the one currently implemented. * A different approach to solve #11170 This approach has (IMHO) a few advantages with regards to the solution currently implemented. I will elaborate on this in the PR. --- flatmap/expand.go | 19 +- flatmap/expand_test.go | 16 ++ helper/schema/schema.go | 34 ---- helper/schema/schema_test.go | 357 ----------------------------------- terraform/state.go | 27 --- terraform/state_test.go | 60 ------ 6 files changed, 30 insertions(+), 483 deletions(-) diff --git a/flatmap/expand.go b/flatmap/expand.go index e325077ef..6f2f6a228 100644 --- a/flatmap/expand.go +++ b/flatmap/expand.go @@ -37,7 +37,7 @@ func Expand(m map[string]string, key string) interface{} { // Check if this is a prefix in the map prefix := key + "." - for k, _ := range m { + for k := range m { if strings.HasPrefix(k, prefix) { return expandMap(m, prefix) } @@ -52,9 +52,17 @@ func expandArray(m map[string]string, prefix string) []interface{} { panic(err) } - // The Schema "Set" type stores its values in an array format, but using - // numeric hash values instead of ordinal keys. Take the set of keys - // regardless of value, and expand them in numeric order. + // If the number of elements in this array is 0, then return an + // empty slice as there is nothing to expand. Trying to expand it + // anyway could lead to crashes as any child maps, arrays or sets + // that no longer exist are still shown as empty with a count of 0. + if num == 0 { + return []interface{}{} + } + + // The Schema "Set" type stores its values in an array format, but + // using numeric hash values instead of ordinal keys. Take the set + // of keys regardless of value, and expand them in numeric order. // See GH-11042 for more details. keySet := map[int]bool{} computed := map[string]bool{} @@ -107,7 +115,7 @@ func expandArray(m map[string]string, prefix string) []interface{} { func expandMap(m map[string]string, prefix string) map[string]interface{} { result := make(map[string]interface{}) - for k, _ := range m { + for k := range m { if !strings.HasPrefix(k, prefix) { continue } @@ -125,6 +133,7 @@ func expandMap(m map[string]string, prefix string) map[string]interface{} { if key == "%" { continue } + result[key] = Expand(m, k[:len(prefix)+len(key)]) } diff --git a/flatmap/expand_test.go b/flatmap/expand_test.go index cf74fadbc..61b151b17 100644 --- a/flatmap/expand_test.go +++ b/flatmap/expand_test.go @@ -147,6 +147,22 @@ func TestExpand(t *testing.T) { }, }, }, + + { + Map: map[string]string{ + "struct.#": "1", + "struct.0.name": "hello", + "struct.0.set.#": "0", + "struct.0.set.0.key": "value", + }, + Key: "struct", + Output: []interface{}{ + map[string]interface{}{ + "name": "hello", + "set": []interface{}{}, + }, + }, + }, } for _, tc := range cases { diff --git a/helper/schema/schema.go b/helper/schema/schema.go index 08c83263e..d04f05b35 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -656,19 +656,6 @@ func (m schemaMap) InternalValidate(topSchemaMap schemaMap) error { return nil } -func (m schemaMap) markAsRemoved(k string, schema *Schema, diff *terraform.InstanceDiff) { - existingDiff, ok := diff.Attributes[k] - if ok { - existingDiff.NewRemoved = true - diff.Attributes[k] = schema.finalizeDiff(existingDiff) - return - } - - diff.Attributes[k] = schema.finalizeDiff(&terraform.ResourceAttrDiff{ - NewRemoved: true, - }) -} - func (m schemaMap) diff( k string, schema *Schema, @@ -792,7 +779,6 @@ func (m schemaMap) diffList( switch t := schema.Elem.(type) { case *Resource: - countDiff, cOk := diff.GetAttribute(k + ".#") // This is a complex resource for i := 0; i < maxLen; i++ { for k2, schema := range t.Schema { @@ -801,15 +787,6 @@ func (m schemaMap) diffList( if err != nil { return err } - - // If parent list is being removed - // remove all subfields which were missed by the diff func - // We process these separately because type-specific diff functions - // lack the context (hierarchy of fields) - subKeyIsCount := strings.HasSuffix(subK, ".#") - if cOk && countDiff.New == "0" && !subKeyIsCount { - m.markAsRemoved(subK, schema, diff) - } } } case *Schema: @@ -1019,7 +996,6 @@ func (m schemaMap) diffSet( for _, code := range list { switch t := schema.Elem.(type) { case *Resource: - countDiff, cOk := diff.GetAttribute(k + ".#") // This is a complex resource for k2, schema := range t.Schema { subK := fmt.Sprintf("%s.%s.%s", k, code, k2) @@ -1027,17 +1003,7 @@ func (m schemaMap) diffSet( if err != nil { return err } - - // If parent set is being removed - // remove all subfields which were missed by the diff func - // We process these separately because type-specific diff functions - // lack the context (hierarchy of fields) - subKeyIsCount := strings.HasSuffix(subK, ".#") - if cOk && countDiff.New == "0" && !subKeyIsCount { - m.markAsRemoved(subK, schema, diff) - } } - case *Schema: // Copy the schema so that we can set Computed/ForceNew from // the parent schema (the TypeSet). diff --git a/helper/schema/schema_test.go b/helper/schema/schema_test.go index 4d93ffd17..d2f667576 100644 --- a/helper/schema/schema_test.go +++ b/helper/schema/schema_test.go @@ -2777,363 +2777,6 @@ func TestSchemaMap_Diff(t *testing.T) { }, }, }, - - { - Name: "Removal of TypeList should cause nested Bool fields w/ Default to be removed too", - Schema: map[string]*Schema{ - "deployment_group_name": &Schema{ - Type: TypeString, - Required: true, - ForceNew: true, - }, - - "alarm_configuration": &Schema{ - Type: TypeList, - Optional: true, - MaxItems: 1, - Elem: &Resource{ - Schema: map[string]*Schema{ - "alarms": &Schema{ - Type: TypeSet, - Optional: true, - Set: HashString, - Elem: &Schema{Type: TypeString}, - }, - - "enabled": &Schema{ - Type: TypeBool, - Optional: true, - }, - - "ignore_poll_alarm_failure": &Schema{ - Type: TypeBool, - Optional: true, - Default: false, - }, - }, - }, - }, - }, - - State: &terraform.InstanceState{ - Attributes: map[string]string{ - "alarm_configuration.#": "1", - "alarm_configuration.0.alarms.#": "1", - "alarm_configuration.0.alarms.2356372769": "foo", - "alarm_configuration.0.enabled": "true", - "alarm_configuration.0.ignore_poll_alarm_failure": "false", - "deployment_group_name": "foo-group-32345345345", - }, - }, - - Config: map[string]interface{}{ - "deployment_group_name": "foo-group-32345345345", - }, - - Diff: &terraform.InstanceDiff{ - Attributes: map[string]*terraform.ResourceAttrDiff{ - "alarm_configuration.#": &terraform.ResourceAttrDiff{ - Old: "1", - New: "0", - NewRemoved: false, - }, - "alarm_configuration.0.alarms": &terraform.ResourceAttrDiff{ - Old: "", - New: "", - NewRemoved: true, - }, - "alarm_configuration.0.alarms.#": &terraform.ResourceAttrDiff{ - Old: "1", - New: "0", - NewRemoved: false, - }, - "alarm_configuration.0.alarms.2356372769": &terraform.ResourceAttrDiff{ - Old: "foo", - New: "", - NewRemoved: true, - }, - "alarm_configuration.0.enabled": &terraform.ResourceAttrDiff{ - Old: "true", - New: "false", - NewRemoved: true, - }, - "alarm_configuration.0.ignore_poll_alarm_failure": &terraform.ResourceAttrDiff{ - Old: "", - New: "", - NewRemoved: true, - }, - }, - }, - }, - - { - Name: "Removal of TypeList should cause all empty nested String fields to be removed too", - Schema: map[string]*Schema{ - "bucket": { - Type: TypeString, - Required: true, - ForceNew: true, - }, - - "acl": { - Type: TypeString, - Default: "private", - Optional: true, - }, - - "website": { - Type: TypeList, - Optional: true, - Elem: &Resource{ - Schema: map[string]*Schema{ - "index_document": { - Type: TypeString, - Optional: true, - }, - - "error_document": { - Type: TypeString, - Optional: true, - }, - - "redirect_all_requests_to": { - Type: TypeString, - Optional: true, - }, - - "routing_rules": { - Type: TypeString, - Optional: true, - }, - }, - }, - }, - }, - - State: &terraform.InstanceState{ - Attributes: map[string]string{ - "acl": "public-read", - "bucket": "tf-test-bucket-5011072831090096749", - "website.#": "1", - "website.0.error_document": "error.html", - "website.0.index_document": "index.html", - "website.0.redirect_all_requests_to": "", - }, - }, - - Config: map[string]interface{}{ - "acl": "public-read", - "bucket": "tf-test-bucket-5011072831090096749", - }, - - Diff: &terraform.InstanceDiff{ - Attributes: map[string]*terraform.ResourceAttrDiff{ - "website.#": &terraform.ResourceAttrDiff{ - Old: "1", - New: "0", - NewRemoved: false, - }, - "website.0.index_document": &terraform.ResourceAttrDiff{ - Old: "index.html", - New: "", - NewRemoved: true, - }, - "website.0.error_document": &terraform.ResourceAttrDiff{ - Old: "error.html", - New: "", - NewRemoved: true, - }, - "website.0.redirect_all_requests_to": &terraform.ResourceAttrDiff{ - Old: "", - New: "", - NewRemoved: true, - }, - "website.0.routing_rules": &terraform.ResourceAttrDiff{ - Old: "", - New: "", - NewRemoved: true, - }, - }, - }, - }, - - { - Name: "Removal of TypeList should cause nested Int fields w/ Default to be removed too", - Schema: map[string]*Schema{ - "availability_zones": &Schema{ - Type: TypeSet, - Elem: &Schema{Type: TypeString}, - Optional: true, - Computed: true, - Set: HashString, - }, - - "access_logs": &Schema{ - Type: TypeList, - Optional: true, - MaxItems: 1, - Elem: &Resource{ - Schema: map[string]*Schema{ - "interval": &Schema{ - Type: TypeInt, - Optional: true, - Default: 60, - }, - "bucket": &Schema{ - Type: TypeString, - Required: true, - }, - "bucket_prefix": &Schema{ - Type: TypeString, - Optional: true, - }, - "enabled": &Schema{ - Type: TypeBool, - Optional: true, - Default: true, - }, - }, - }, - }, - }, - - State: &terraform.InstanceState{ - Attributes: map[string]string{ - "access_logs.#": "1", - "access_logs.0.bucket": "terraform-access-logs-bucket-5906065226840117876", - "access_logs.0.bucket_prefix": "", - "access_logs.0.enabled": "true", - "access_logs.0.interval": "5", - "availability_zones.#": "3", - "availability_zones.2050015877": "us-west-2c", - "availability_zones.221770259": "us-west-2b", - "availability_zones.2487133097": "us-west-2a", - }, - }, - - Config: map[string]interface{}{ - "availability_zones": []interface{}{"us-west-2a", "us-west-2b", "us-west-2c"}, - }, - - Diff: &terraform.InstanceDiff{ - Attributes: map[string]*terraform.ResourceAttrDiff{ - "access_logs.#": &terraform.ResourceAttrDiff{ - Old: "1", - New: "0", - NewRemoved: false, - }, - "access_logs.0.bucket": &terraform.ResourceAttrDiff{ - Old: "terraform-access-logs-bucket-5906065226840117876", - New: "", - NewRemoved: true, - }, - "access_logs.0.bucket_prefix": &terraform.ResourceAttrDiff{ - Old: "", - New: "", - NewRemoved: true, - }, - "access_logs.0.enabled": &terraform.ResourceAttrDiff{ - Old: "", - New: "", - NewRemoved: true, - }, - "access_logs.0.interval": &terraform.ResourceAttrDiff{ - Old: "5", - New: "60", - NewRemoved: true, - }, - }, - }, - }, - - { - Name: "Removal of TypeSet should cause computed fields to be removed", - Schema: map[string]*Schema{ - "type_set": &Schema{ - Type: TypeSet, - Optional: true, - Elem: &Resource{ - Schema: map[string]*Schema{ - "name": &Schema{ - Type: TypeString, - Optional: true, - }, - "required": &Schema{ - Type: TypeString, - Required: true, - }, - "value": &Schema{ - Type: TypeInt, - Optional: true, - }, - "required_value": &Schema{ - Type: TypeInt, - Required: true, - }, - "computed_value": &Schema{ - Type: TypeString, - Optional: true, - Computed: true, - }, - }, - }, - Set: func(i interface{}) int { - if i != nil { - return 12345 - } - return 0 - }, - }, - }, - - State: &terraform.InstanceState{ - Attributes: map[string]string{ - "type_set.#": "1", - "type_set.12345.name": "Name", - "type_set.12345.required": "Required", - "type_set.12345.value": "0", - "type_set.12345.required_value": "5", - "type_set.12345.computed_value": "COMPUTED", - }, - }, - - Config: map[string]interface{}{ - "type_set": []interface{}{}, - }, - - Diff: &terraform.InstanceDiff{ - Attributes: map[string]*terraform.ResourceAttrDiff{ - "type_set.#": &terraform.ResourceAttrDiff{ - Old: "1", - New: "0", - NewRemoved: false, - }, - "type_set.12345.name": &terraform.ResourceAttrDiff{ - Old: "Name", - New: "", - NewRemoved: true, - }, - "type_set.12345.required": &terraform.ResourceAttrDiff{ - Old: "Required", - New: "", - NewRemoved: true, - }, - "type_set.12345.value": &terraform.ResourceAttrDiff{ - Old: "0", - New: "0", - NewRemoved: true, - }, - "type_set.12345.required_value": &terraform.ResourceAttrDiff{ - Old: "5", - New: "0", - NewRemoved: true, - }, - "type_set.12345.computed_value": &terraform.ResourceAttrDiff{ - NewRemoved: true, - }, - }, - }, - }, } for i, tc := range cases { diff --git a/terraform/state.go b/terraform/state.go index 84d4c2669..074b68245 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -10,7 +10,6 @@ import ( "io/ioutil" "log" "reflect" - "regexp" "sort" "strconv" "strings" @@ -1713,32 +1712,6 @@ func (s *InstanceState) MergeDiff(d *InstanceDiff) *InstanceState { } } - // Remove any now empty array, maps or sets because a parent structure - // won't include these entries in the count value. - isCount := regexp.MustCompile(`\.[%#]$`).MatchString - var deleted []string - - for k, v := range result.Attributes { - if isCount(k) && v == "0" { - delete(result.Attributes, k) - deleted = append(deleted, k) - } - } - - for _, k := range deleted { - // Sanity check for invalid structures. - // If we removed the primary count key, there should have been no - // other keys left with this prefix. - - // this must have a "#" or "%" which we need to remove - base := k[:len(k)-1] - for k, _ := range result.Attributes { - if strings.HasPrefix(k, base) { - panic(fmt.Sprintf("empty structure %q has entry %q", base, k)) - } - } - } - return result } diff --git a/terraform/state_test.go b/terraform/state_test.go index 324ab7970..5578f89c9 100644 --- a/terraform/state_test.go +++ b/terraform/state_test.go @@ -1450,66 +1450,6 @@ func TestInstanceState_MergeDiff(t *testing.T) { } } -// Make sure we don't leave empty maps or arrays in the flatmapped Attributes, -// since those may affect the counts of a parent structure. -func TestInstanceState_MergeDiffRemoveCounts(t *testing.T) { - is := InstanceState{ - ID: "foo", - Attributes: map[string]string{ - "all.#": "3", - "all.1111": "x", - "all.1234.#": "1", - "all.1234.0": "a", - "all.5678.%": "1", - "all.5678.key": "val", - - // nested empty lists need to be removed cleanly - "all.nested.#": "0", - "all.nested.0.empty.#": "0", - "all.nested.1.empty.#": "0", - - // the value has a prefix that matches another key - // and ntohing should happen to this. - "all.nested_value": "y", - }, - } - - diff := &InstanceDiff{ - Attributes: map[string]*ResourceAttrDiff{ - "all.#": &ResourceAttrDiff{ - Old: "3", - New: "1", - }, - "all.1234.0": &ResourceAttrDiff{ - NewRemoved: true, - }, - "all.1234.#": &ResourceAttrDiff{ - Old: "1", - New: "0", - }, - "all.5678.key": &ResourceAttrDiff{ - NewRemoved: true, - }, - "all.5678.%": &ResourceAttrDiff{ - Old: "1", - New: "0", - }, - }, - } - - is2 := is.MergeDiff(diff) - - expected := map[string]string{ - "all.#": "1", - "all.1111": "x", - "all.nested_value": "y", - } - - if !reflect.DeepEqual(expected, is2.Attributes) { - t.Fatalf("bad: %#v", is2.Attributes) - } -} - // GH-12183. This tests that a list with a computed set generates the // right partial state. This never failed but is put here for completion // of the test case for GH-12183.