resilien
/
terraform
2
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

core: fix a crash by suggesting a different approach to solve #11170 (#13541) 

* 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.
This commit is contained in:
Sander van Harmelen 2017-04-14 22:32:30 +02:00 committed by GitHub
parent 80c359c343
commit 3d0073e05c
6 changed files with 30 additions and 483 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
flatmap/expand.go
View File

@ -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)])
}

16
flatmap/expand_test.go
View File

@ -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 {

34
helper/schema/schema.go
View File

@ -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).

357
helper/schema/schema_test.go
View File

@ -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 {

27
terraform/state.go
View File

@ -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
}

60
terraform/state_test.go
View File

@ -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.