From b6e3d58bfe7aad06189a680071453dbef6fd5793 Mon Sep 17 00:00:00 2001 From: magodo Date: Mon, 16 Mar 2020 23:10:26 +0800 Subject: [PATCH] Type Block need handle attribute marked as removed contionally. In some provider, e.g. AzureRM, TypeSet is use quite prevalent to hold a set of sub-resources (e.g. security_rule in azurerm_network_security_group). The reason for using TypeSet instead of TypeList might because service return a collection of info with arbitrary order, or other reasons. In this case: Given a slice of nested blocks, if one of the nested block (B1) attributes is optional and has no default value, and user didn't specify a value for it. Then if another nested block (B2) changed, which triggers some diff, then B1 will also be replaced. That is because the optional attribute contribute a diff of "zero value" -> `null`, which changed the hash of the block. This fix is to carefully handle this case. We keep attribute marked as `NewRemoved` after a `diffString` only when all the attributes are marked as such. Otherwise, as long as one attribute is not marked to be removed, that means this block will be kept. Then we will manipulate the attributes in this block, which being marked as removed just because user didn't specify a value and the original value is the zero value. This keeps consistent as other Resource types (e.g. List Resource). (Though those type just remove the attribute from diff set, while for Set we need to return the complete state as we will not depend on the old state during diff apply). --- helper/schema/schema.go | 54 +++++++++++++ helper/schema/schema_test.go | 150 +++++++++++++++++++++++++++++++++++ 2 files changed, 204 insertions(+) diff --git a/helper/schema/schema.go b/helper/schema/schema.go index df0172fa108..ee007f4a6cf 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -1305,13 +1305,67 @@ func (m schemaMap) diffSet( switch t := schema.Elem.(type) { case *Resource: // This is a complex resource + + // As long as one of the attribute is not marked to be removed, we should unmark the other attributes + // that are marked to be removed just because their new value is not specified in config and their + // old value is the zero value. + isAllSubKNewRemoved := true + for k2, schema := range t.Schema { subK := fmt.Sprintf("%s.%s.%s", k, code, k2) err := m.diff(subK, schema, diff, d, true) if err != nil { return err } + + if subV, ok := diff.Attributes[subK]; ok && !subV.NewRemoved { + isAllSubKNewRemoved = false + } } + if !isAllSubKNewRemoved { + for k2 := range t.Schema { + subK := fmt.Sprintf("%s.%s.%s", k, code, k2) + if subV, ok := diff.Attributes[subK]; ok && subV.NewRemoved { + schemaList := addrToSchema(strings.Split(subK, "."), map[string]*Schema{k: schema}) + if len(schemaList) == 0 { + continue + } + subSchema := schemaList[len(schemaList)-1] + + var oldIsZero bool + switch subSchema.Type { + case TypeBool: + ov := subV.Old == "true" + oldIsZero = ov == subSchema.ZeroValue() + case TypeInt: + ov, err := strconv.Atoi(subV.Old) + if err != nil { + return err + } + oldIsZero = ov == subSchema.ZeroValue() + case TypeFloat: + ov, err := strconv.ParseFloat(subV.Old, 64) + if err != nil { + return err + } + oldIsZero = ov == subSchema.ZeroValue() + case TypeString: + oldIsZero = subV.Old == subSchema.ZeroValue() + case TypeList, TypeSet: + // TODO + case TypeMap: + // TODO + default: + return fmt.Errorf("%s: unknown type %#v", k, schema.Type) + } + if oldIsZero { + subV.NewRemoved = false + subV.New = subV.Old + } + } + } + } + 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 3b5ea8a052e..0434412c6a4 100644 --- a/helper/schema/schema_test.go +++ b/helper/schema/schema_test.go @@ -3161,6 +3161,156 @@ func TestSchemaMap_Diff(t *testing.T) { }, }, }, + { + Name: "Set element with unset Optional attributes should not be affected when another element get updated", + Schema: map[string]*Schema{ + "foo": { + Type: TypeSet, + Required: true, + Elem: &Resource{ + Schema: map[string]*Schema{ + "a": { + Type: TypeInt, + Required: true, + }, + "b": { + Type: TypeInt, + Optional: true, + }, + }, + }, + Set: func(v interface{}) int { + m := v.(map[string]interface{}) + return m["a"].(int) + m["b"].(int) + }, + }, + }, + + State: &terraform.InstanceState{ + Attributes: map[string]string{ + "foo.#": "2", + "foo.1.a": "1", + "foo.1.b": "0", + "foo.2.a": "2", + "foo.2.b": "0", + }, + }, + + Config: map[string]interface{}{ + "foo": []interface{}{ + map[string]interface{}{ + "a": 1, + }, + map[string]interface{}{ + "a": 3, + }, + }, + }, + + Diff: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "foo.1.a": { + Old: "1", + New: "1", + }, + "foo.1.b": { + Old: "0", + New: "0", + }, + "foo.2.a": { + Old: "2", + New: "0", + NewRemoved: true, + }, + "foo.2.b": { + Old: "0", + New: "0", + NewRemoved: true, + }, + "foo.3.a": { + Old: "", + New: "3", + }, + "foo.3.b": { + Old: "", + New: "", + }, + }, + }, + + Err: false, + }, + { + Name: "Set element with unset Optional attributes should not be affected when new element get added", + Schema: map[string]*Schema{ + "foo": { + Type: TypeSet, + Required: true, + Elem: &Resource{ + Schema: map[string]*Schema{ + "a": { + Type: TypeInt, + Required: true, + }, + "b": { + Type: TypeInt, + Optional: true, + }, + }, + }, + Set: func(v interface{}) int { + m := v.(map[string]interface{}) + return m["a"].(int) + m["b"].(int) + }, + }, + }, + + State: &terraform.InstanceState{ + Attributes: map[string]string{ + "foo.#": "1", + "foo.1.a": "1", + "foo.1.b": "0", + }, + }, + + Config: map[string]interface{}{ + "foo": []interface{}{ + map[string]interface{}{ + "a": 1, + }, + map[string]interface{}{ + "a": 2, + }, + }, + }, + + Diff: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "foo.#": { + Old: "1", + New: "2", + }, + "foo.1.a": { + Old: "1", + New: "1", + }, + "foo.1.b": { + Old: "0", + New: "0", + }, + "foo.2.a": { + Old: "", + New: "2", + }, + "foo.2.b": { + Old: "", + New: "", + }, + }, + }, + + Err: false, + }, } for i, tc := range cases {