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

@@ -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 {
+	exact := source&getSourceExact != 0
-		for k, _ := range d.state.Attributes {
+	source &^= getSourceExact
-			if !strings.HasPrefix(k, prefix) {
-				continue
-			}
 
-			single := k[len(prefix):]
+	if !exact || source == getSourceState {
-			result[single] = d.getPrimitive(k, nil, elemSchema, source).Value
+		if d.state != nil && source >= getSourceState {
-			resultSet = true
+			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 {
+	if !exact || source == getSourceDiff {
-		for k, v := range d.diff.Attributes {
+		if d.diff != nil && source >= getSourceDiff {
-			if !strings.HasPrefix(k, prefix) {
+			for k, v := range d.diff.Attributes {
-				continue
+				if !strings.HasPrefix(k, prefix) {
-			}
+					continue
-			resultSet = true
+				}
+				resultSet = true
 
-			single := k[len(prefix):]
+				single := k[len(prefix):]
 
-			if v.NewRemoved {
+				if v.NewRemoved {
-				delete(result, single)
+					delete(result, single)
-			} else {
+				} else {
-				result[single] = d.getPrimitive(k, nil, elemSchema, source).Value
+					result[single] = d.getPrimitive(k, nil, elemSchema, source).Value
+				}
 			}
 		}
 	}
 
-	if d.setMap != nil && source >= getSourceSet {
+	if !exact || source == getSourceSet {
-		cleared := false
+		if d.setMap != nil && source >= getSourceSet {
-		for k, _ := range d.setMap {
+			cleared := false
-			if !strings.HasPrefix(k, prefix) {
+			for k, _ := range d.setMap {
-				continue
+				if !strings.HasPrefix(k, prefix) {
-			}
+					continue
-			resultSet = true
+				}
+				resultSet = true
 
-			if !cleared {
+				if !cleared {
-				// We clear the results if they are in the set map
+					// We clear the results if they are in the set map
-				result = make(map[string]interface{})
+					result = make(map[string]interface{})
-				cleared = true
+					cleared = true
-			}
+				}
 
-			single := k[len(prefix):]
+				single := k[len(prefix):]
-			result[single] = d.getPrimitive(k, nil, elemSchema, source).Value
+				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 {
+	exact := source&getSourceExact != 0
-		result, resultSet = d.state.Attributes[k]
+	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 {
+	if !exact || source == getSourceDiff {
-		attrD, ok := d.diff.Attributes[k]
+		if d.diff != nil && source >= getSourceDiff {
-		if ok {
+			attrD, ok := d.diff.Attributes[k]
-			if !attrD.NewComputed {
+			if ok {
-				result = attrD.New
+				if !attrD.NewComputed {
-				if attrD.NewExtra != nil {
+					result = attrD.New
-					// If NewExtra != nil, then we have processed data as the New,
+					if attrD.NewExtra != nil {
-					// so we store that but decode the unprocessed data into result
+						// If NewExtra != nil, then we have processed data as the New,
-					resultProcessed = result
+						// so we store that but decode the unprocessed data into result
+						resultProcessed = result
 
-					err := mapstructure.WeakDecode(attrD.NewExtra, &result)
+						err := mapstructure.WeakDecode(attrD.NewExtra, &result)
-					if err != nil {
+						if err != nil {
-						panic(err)
+							panic(err)
+						}
 					}
-				}
 
-				resultSet = true
+					resultSet = true
-			} else {
+				} else {
-				result = ""
+					result = ""
-				resultSet = false
+					resultSet = false
+				}
 			}
 		}
 	}
 
-	if d.setMap != nil && source >= getSourceSet {
+	if !exact || source == getSourceSet {
-		if v, ok := d.setMap[k]; ok {
+		if d.setMap != nil && source >= getSourceSet {
-			result = v
+			if v, ok := d.setMap[k]; ok {
-			resultSet = true
+				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()
 	}

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
 		 */

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)

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
 		 */