From 59349cca1184c7903dac06bfd54f4dbc9e131262 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 11 Oct 2014 10:40:54 -0700 Subject: [PATCH] helper/schema: sets must be treated atomically within ResourceData This fixes a seemingly minor issue (GH-255) around plans showing changes when in fact there are none. But in reality this turned out to uncover a really terrible bug. The effect of what was happening was that multiple items in a set were being merged. Now, they were being merged in the right order, so if you didn't have rich types (lists in a set) then you never saw the effect since the later value would overwrite the earlier. But with lists (such as in security groups), you would end up with the lists merging. So, if you had one ingress rule with CIDR blocks and one with SGs, then after the merge both ingress rules would have BOTH CIDR and SGs, resulting in an incorrect plan (GH-255). This fixes the issue by introducing a `getSourceExact` bitflag to the ResourceData source. When this is set, ALL data must come from this level, instead of merging lower levels. In the case of sets and diffs, this is exactly what you want: "Get me the set 'foo' from the config and the config ONLY (not the state or diff or w/e)". Andddddd its fixed. GH-255 --- helper/schema/resource_data.go | 181 +++++++++++++++++++--------- helper/schema/resource_data_test.go | 98 +++++++++++++++ helper/schema/schema.go | 8 ++ helper/schema/schema_test.go | 52 ++++++++ 4 files changed, 280 insertions(+), 59 deletions(-) diff --git a/helper/schema/resource_data.go b/helper/schema/resource_data.go index 4d0637fa8..deb331a05 100644 --- a/helper/schema/resource_data.go +++ b/helper/schema/resource_data.go @@ -41,10 +41,12 @@ type ResourceData struct { type getSource byte const ( - getSourceState getSource = iota + getSourceState getSource = 1 << iota getSourceConfig getSourceDiff getSourceSet + getSourceExact + getSourceMax = getSourceSet ) // getResult is the internal structure that is generated when a Get @@ -225,7 +227,7 @@ func (d *ResourceData) init() { func (d *ResourceData) diffChange( k string) (interface{}, interface{}, bool, bool) { // Get the change between the state and the config. - o, n := d.getChange(k, getSourceState, getSourceConfig) + o, n := d.getChange(k, getSourceState, getSourceConfig|getSourceExact) if !o.Exists { o.Value = nil } @@ -282,7 +284,21 @@ func (d *ResourceData) getSet( source getSource) getResult { s := &Set{F: schema.Set} result := getResult{Schema: schema, Value: s} - raw := d.getList(k, nil, schema, source) + + // Get the list. For sets, the entire source must be exact: the + // entire set must come from set, diff, state, etc. So we go backwards + // and once we get a result, we take it. Or, we never get a result. + var raw getResult + for listSource := source; listSource > 0; listSource >>= 1 { + if source&getSourceExact != 0 && listSource != source { + break + } + + raw = d.getList(k, nil, schema, listSource|getSourceExact) + if raw.Exists { + break + } + } if !raw.Exists { if len(parts) > 0 { return d.getList(k, parts, schema, source) @@ -368,15 +384,20 @@ func (d *ResourceData) getMap( resultSet := false prefix := k + "." - if d.state != nil && source >= getSourceState { - for k, _ := range d.state.Attributes { - if !strings.HasPrefix(k, prefix) { - continue - } + exact := source&getSourceExact != 0 + source &^= getSourceExact - single := k[len(prefix):] - result[single] = d.getPrimitive(k, nil, elemSchema, source).Value - resultSet = true + if !exact || source == getSourceState { + if d.state != nil && source >= getSourceState { + for k, _ := range d.state.Attributes { + if !strings.HasPrefix(k, prefix) { + continue + } + + single := k[len(prefix):] + result[single] = d.getPrimitive(k, nil, elemSchema, source).Value + resultSet = true + } } } @@ -412,39 +433,43 @@ func (d *ResourceData) getMap( } } - if d.diff != nil && source >= getSourceDiff { - for k, v := range d.diff.Attributes { - if !strings.HasPrefix(k, prefix) { - continue - } - resultSet = true + if !exact || source == getSourceDiff { + if d.diff != nil && source >= getSourceDiff { + for k, v := range d.diff.Attributes { + if !strings.HasPrefix(k, prefix) { + continue + } + resultSet = true - single := k[len(prefix):] + single := k[len(prefix):] - if v.NewRemoved { - delete(result, single) - } else { - result[single] = d.getPrimitive(k, nil, elemSchema, source).Value + if v.NewRemoved { + delete(result, single) + } else { + result[single] = d.getPrimitive(k, nil, elemSchema, source).Value + } } } } - if d.setMap != nil && source >= getSourceSet { - cleared := false - for k, _ := range d.setMap { - if !strings.HasPrefix(k, prefix) { - continue - } - resultSet = true + if !exact || source == getSourceSet { + if d.setMap != nil && source >= getSourceSet { + cleared := false + for k, _ := range d.setMap { + if !strings.HasPrefix(k, prefix) { + continue + } + resultSet = true - if !cleared { - // We clear the results if they are in the set map - result = make(map[string]interface{}) - cleared = true - } + if !cleared { + // We clear the results if they are in the set map + result = make(map[string]interface{}) + cleared = true + } - single := k[len(prefix):] - result[single] = d.getPrimitive(k, nil, elemSchema, source).Value + single := k[len(prefix):] + result[single] = d.getPrimitive(k, nil, elemSchema, source).Value + } } } @@ -552,10 +577,16 @@ func (d *ResourceData) getPrimitive( var result string var resultProcessed interface{} var resultComputed, resultSet bool - if d.state != nil && source >= getSourceState { - result, resultSet = d.state.Attributes[k] + exact := source&getSourceExact != 0 + source &^= getSourceExact + + if !exact || source == getSourceState { + if d.state != nil && source >= getSourceState { + result, resultSet = d.state.Attributes[k] + } } + // No exact check is needed here because config is always exact if d.config != nil && source == getSourceConfig { // For config, we always return the exact value if v, ok := d.config.Get(k); ok { @@ -573,34 +604,38 @@ func (d *ResourceData) getPrimitive( resultComputed = d.config.IsComputed(k) } - if d.diff != nil && source >= getSourceDiff { - attrD, ok := d.diff.Attributes[k] - if ok { - if !attrD.NewComputed { - result = attrD.New - if attrD.NewExtra != nil { - // If NewExtra != nil, then we have processed data as the New, - // so we store that but decode the unprocessed data into result - resultProcessed = result + if !exact || source == getSourceDiff { + if d.diff != nil && source >= getSourceDiff { + attrD, ok := d.diff.Attributes[k] + if ok { + if !attrD.NewComputed { + result = attrD.New + if attrD.NewExtra != nil { + // If NewExtra != nil, then we have processed data as the New, + // so we store that but decode the unprocessed data into result + resultProcessed = result - err := mapstructure.WeakDecode(attrD.NewExtra, &result) - if err != nil { - panic(err) + err := mapstructure.WeakDecode(attrD.NewExtra, &result) + if err != nil { + panic(err) + } } - } - resultSet = true - } else { - result = "" - resultSet = false + resultSet = true + } else { + result = "" + resultSet = false + } } } } - if d.setMap != nil && source >= getSourceSet { - if v, ok := d.setMap[k]; ok { - result = v - resultSet = true + if !exact || source == getSourceSet { + if d.setMap != nil && source >= getSourceSet { + if v, ok := d.setMap[k]; ok { + result = v + resultSet = true + } } } @@ -862,6 +897,34 @@ func (d *ResourceData) setSet( return fmt.Errorf("%s: can only set the full set, not elements", k) } + // If it is a slice, then we have to turn it into a *Set so that + // we get the proper order back based on the hash code. + if v := reflect.ValueOf(value); v.Kind() == reflect.Slice { + // Set the entire list, this lets us get sane values out of it + if err := d.setList(k, nil, schema, value); err != nil { + return err + } + + // Build the set by going over the list items in order and + // hashing them into the set. The reason we go over the list and + // not the `value` directly is because this forces all types + // to become []interface{} (generic) instead of []string, which + // most hash functions are expecting. + s := &Set{F: schema.Set} + source := getSourceSet | getSourceExact + for i := 0; i < v.Len(); i++ { + is := strconv.FormatInt(int64(i), 10) + result := d.getList(k, []string{is}, schema, source) + if !result.Exists { + panic("just set item doesn't exist") + } + + s.Add(result.Value) + } + + value = s + } + if s, ok := value.(*Set); ok { value = s.List() } diff --git a/helper/schema/resource_data_test.go b/helper/schema/resource_data_test.go index 081f92f70..01f3616b3 100644 --- a/helper/schema/resource_data_test.go +++ b/helper/schema/resource_data_test.go @@ -1762,6 +1762,104 @@ func TestResourceDataState(t *testing.T) { }, }, + { + Schema: map[string]*Schema{ + "ports": &Schema{ + Type: TypeSet, + Optional: true, + Computed: true, + Elem: &Schema{Type: TypeInt}, + Set: func(a interface{}) int { + return a.(int) + }, + }, + }, + + State: nil, + + Diff: nil, + + Set: map[string]interface{}{ + "ports": []interface{}{100, 80}, + }, + + Result: &terraform.InstanceState{ + Attributes: map[string]string{ + "ports.#": "2", + "ports.0": "80", + "ports.1": "100", + }, + }, + }, + + { + Schema: map[string]*Schema{ + "ports": &Schema{ + Type: TypeSet, + Optional: true, + Computed: true, + Elem: &Resource{ + Schema: map[string]*Schema{ + "order": &Schema{ + Type: TypeInt, + }, + + "a": &Schema{ + Type: TypeList, + Elem: &Schema{Type: TypeInt}, + }, + + "b": &Schema{ + Type: TypeList, + Elem: &Schema{Type: TypeInt}, + }, + }, + }, + Set: func(a interface{}) int { + m := a.(map[string]interface{}) + return m["order"].(int) + }, + }, + }, + + State: &terraform.InstanceState{ + Attributes: map[string]string{ + "ports.#": "2", + "ports.0.order": "10", + "ports.0.a.#": "1", + "ports.0.a.0": "80", + "ports.1.order": "20", + "ports.1.b.#": "1", + "ports.1.b.0": "100", + }, + }, + + Set: map[string]interface{}{ + "ports": []interface{}{ + map[string]interface{}{ + "order": 20, + "b": []interface{}{100}, + }, + map[string]interface{}{ + "order": 10, + "a": []interface{}{80}, + }, + }, + }, + + Result: &terraform.InstanceState{ + Attributes: map[string]string{ + "ports.#": "2", + "ports.0.order": "10", + "ports.0.a.#": "1", + "ports.0.a.0": "80", + "ports.1.order": "20", + "ports.1.b.#": "1", + "ports.1.b.0": "100", + }, + }, + }, + /* * PARTIAL STATES */ diff --git a/helper/schema/schema.go b/helper/schema/schema.go index cb3e11617..7b756689d 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -435,6 +435,7 @@ func (m schemaMap) diffList( diff *terraform.InstanceDiff, d *ResourceData) error { o, n, _, computedList := d.diffChange(k) + nSet := n != nil // If we have an old value, but no new value set but we're computed, // then nothing has changed. @@ -457,6 +458,13 @@ func (m schemaMap) diffList( os := o.([]interface{}) vs := n.([]interface{}) + // If the new value was set, and the two are equal, then we're done. + // We have to do this check here because sets might be NOT + // reflect.DeepEqual so we need to wait until we get the []interface{} + if nSet && reflect.DeepEqual(os, vs) { + return nil + } + // Get the counts oldLen := len(os) newLen := len(vs) diff --git a/helper/schema/schema_test.go b/helper/schema/schema_test.go index 34f57c891..d9bcd1cf5 100644 --- a/helper/schema/schema_test.go +++ b/helper/schema/schema_test.go @@ -886,6 +886,58 @@ func TestSchemaMap_Diff(t *testing.T) { Err: false, }, + { + Schema: map[string]*Schema{ + "ingress": &Schema{ + Type: TypeSet, + Required: true, + Elem: &Resource{ + Schema: map[string]*Schema{ + "ports": &Schema{ + Type: TypeList, + Optional: true, + Elem: &Schema{Type: TypeInt}, + }, + }, + }, + Set: func(v interface{}) int { + m := v.(map[string]interface{}) + ps := m["ports"].([]interface{}) + result := 0 + for _, p := range ps { + result += p.(int) + } + return result + }, + }, + }, + + State: &terraform.InstanceState{ + Attributes: map[string]string{ + "ingress.#": "2", + "ingress.0.ports.#": "1", + "ingress.0.ports.0": "80", + "ingress.1.ports.#": "1", + "ingress.1.ports.0": "443", + }, + }, + + Config: map[string]interface{}{ + "ingress": []interface{}{ + map[string]interface{}{ + "ports": []interface{}{443}, + }, + map[string]interface{}{ + "ports": []interface{}{80}, + }, + }, + }, + + Diff: nil, + + Err: false, + }, + /* * List of structure decode */