helper/schema: diffs for sets should include the full set [GH-457]

Prior to this, the diff only contained changed set elements. The issue
with this is that `getSet`, the internal function that reads a set from
the ResourceData, expects that each level (state, config, diff, etc.)
has the _full set_ information. This change was done to fix merging
issues.

Because of this, we need to make sure the full set is visible in the
diff.

CHANGELOG.md

@@ -42,6 +42,8 @@ BUG FIXES:
   * providers/aws: Disassociate EIP before destroying.
   * providers/aws: ELB treats subnets as a set.
   * providers/aws: Fix case where in a destroy/create tags weren't reapplied. [GH-464]
+  * providers/aws: Fix incorrect/erroneous apply cases around security group
+      rules. [GH-457]
   * providers/consul: Fix regression where `key` param changed to `keys. [GH-475]
 
 ## 0.3.0 (October 14, 2014)

builtin/providers/aws/resource_aws_security_group.go

@@ -66,14 +66,18 @@ func resourceAwsSecurityGroup() *schema.Resource {
 						},
 
 						"security_groups": &schema.Schema{
-							Type:     schema.TypeList,
+							Type:     schema.TypeSet,
 							Optional: true,
 							Elem:     &schema.Schema{Type: schema.TypeString},
+							Set: func(v interface{}) int {
+								return hashcode.String(v.(string))
+							},
 						},
 
 						"self": &schema.Schema{
 							Type:     schema.TypeBool,
 							Optional: true,
+							Default:  false,
 						},
 					},
 				},
@@ -112,7 +116,7 @@ func resourceAwsSecurityGroupIngressHash(v interface{}) int {
 		}
 	}
 	if v, ok := m["security_groups"]; ok {
-		vs := v.([]interface{})
+		vs := v.(*schema.Set).List()
 		s := make([]string, len(vs))
 		for i, raw := range vs {
 			s[i] = raw.(string)
@@ -283,15 +287,28 @@ func resourceAwsSecurityGroupRead(d *schema.ResourceData, meta interface{}) erro
 	sg := sgRaw.(*ec2.SecurityGroupInfo)
 
 	// Gather our ingress rules
-	ingressRules := make([]map[string]interface{}, len(sg.IPPerms))
-	for i, perm := range sg.IPPerms {
-		n := make(map[string]interface{})
-		n["from_port"] = perm.FromPort
-		n["protocol"] = perm.Protocol
-		n["to_port"] = perm.ToPort
+	ingressMap := make(map[string]map[string]interface{})
+	for _, perm := range sg.IPPerms {
+		k := fmt.Sprintf("%s-%d-%d", perm.Protocol, perm.FromPort, perm.ToPort)
+		m, ok := ingressMap[k]
+		if !ok {
+			m = make(map[string]interface{})
+			ingressMap[k] = m
+		}
+
+		m["from_port"] = perm.FromPort
+		m["to_port"] = perm.ToPort
+		m["protocol"] = perm.Protocol
 
 		if len(perm.SourceIPs) > 0 {
-			n["cidr_blocks"] = perm.SourceIPs
+			raw, ok := m["cidr_blocks"]
+			if !ok {
+				raw = make([]string, 0, len(perm.SourceIPs))
+			}
+			list := raw.([]string)
+
+			list = append(list, perm.SourceIPs...)
+			m["cidr_blocks"] = list
 		}
 
 		var groups []string
@@ -301,14 +318,24 @@ func resourceAwsSecurityGroupRead(d *schema.ResourceData, meta interface{}) erro
 		for i, id := range groups {
 			if id == d.Id() {
 				groups[i], groups = groups[len(groups)-1], groups[:len(groups)-1]
-				n["self"] = true
+				m["self"] = true
 			}
 		}
+
 		if len(groups) > 0 {
-			n["security_groups"] = groups
-		}
+			raw, ok := m["security_groups"]
+			if !ok {
+				raw = make([]string, 0, len(groups))
+			}
+			list := raw.([]string)
 
-		ingressRules[i] = n
+			list = append(list, groups...)
+			m["security_groups"] = list
+		}
+	}
+	ingressRules := make([]map[string]interface{}, 0, len(ingressMap))
+	for _, m := range ingressMap {
+		ingressRules = append(ingressRules, m)
 	}
 
 	d.Set("description", sg.Description)

builtin/providers/aws/structure.go

@@ -3,6 +3,7 @@ package aws
 import (
 	"strings"
 
+	"github.com/hashicorp/terraform/helper/schema"
 	"github.com/mitchellh/goamz/ec2"
 	"github.com/mitchellh/goamz/elb"
 )
@@ -48,7 +49,7 @@ func expandIPPerms(id string, configured []interface{}) []ec2.IPPerm {
 
 		var groups []string
 		if raw, ok := m["security_groups"]; ok {
-			list := raw.([]interface{})
+			list := raw.(*schema.Set).List()
 			for _, v := range list {
 				groups = append(groups, v.(string))
 			}

helper/schema/resource_data.go

@@ -2,6 +2,7 @@ package schema
 
 import (
 	"fmt"
+	"log"
 	"reflect"
 	"strconv"
 	"strings"
@@ -647,6 +648,17 @@ func (d *ResourceData) getPrimitive(
 		}
 	}
 
+	if strings.HasSuffix(k, "protocol") && result == "" {
+		log.Printf("------------------------------------------------")
+		log.Printf("KEY: %s", k)
+		log.Printf("LEVEL: %#v", source)
+		log.Printf("DIFF: %#v", diff)
+		log.Printf("EXACT: %#v", exact)
+		log.Printf("Config: %#v\n\n", d.config)
+		log.Printf("DIFF: %#v\n\n", d.diff)
+		log.Printf("State: %#v\n\n", d.state)
+	}
+
 	if !exact || source == getSourceSet {
 		if d.setMap != nil && source >= getSourceSet {
 			if v, ok := d.setMap[k]; ok {

helper/schema/schema.go

@@ -205,7 +205,7 @@ func (m schemaMap) Diff(
 	}
 
 	for k, schema := range m {
-		err := m.diff(k, schema, result, d)
+		err := m.diff(k, schema, result, d, false)
 		if err != nil {
 			return nil, err
 		}
@@ -225,7 +225,7 @@ func (m schemaMap) Diff(
 
 		// Perform the diff again
 		for k, schema := range m {
-			err := m.diff(k, schema, result2, d)
+			err := m.diff(k, schema, result2, d, false)
 			if err != nil {
 				return nil, err
 			}
@@ -427,21 +427,22 @@ func (m schemaMap) diff(
 	k string,
 	schema *Schema,
 	diff *terraform.InstanceDiff,
-	d *ResourceData) error {
+	d *ResourceData,
+	all bool) error {
 	var err error
 	switch schema.Type {
 	case TypeBool:
 		fallthrough
 	case TypeInt:
 		fallthrough
 	case TypeString:
-		err = m.diffString(k, schema, diff, d)
+		err = m.diffString(k, schema, diff, d, all)
 	case TypeList:
-		err = m.diffList(k, schema, diff, d)
+		err = m.diffList(k, schema, diff, d, all)
 	case TypeMap:
-		err = m.diffMap(k, schema, diff, d)
+		err = m.diffMap(k, schema, diff, d, all)
 	case TypeSet:
-		err = m.diffSet(k, schema, diff, d)
+		err = m.diffSet(k, schema, diff, d, all)
 	default:
 		err = fmt.Errorf("%s: unknown type %#v", k, schema.Type)
 	}
@@ -453,7 +454,8 @@ func (m schemaMap) diffList(
 	k string,
 	schema *Schema,
 	diff *terraform.InstanceDiff,
-	d *ResourceData) error {
+	d *ResourceData,
+	all bool) error {
 	o, n, _, computedList := d.diffChange(k)
 	nSet := n != nil
 
@@ -481,7 +483,7 @@ func (m schemaMap) diffList(
 	// 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) {
+	if !all && nSet && reflect.DeepEqual(os, vs) {
 		return nil
 	}
 
@@ -502,7 +504,7 @@ func (m schemaMap) diffList(
 	// If the counts are not the same, then record that diff
 	changed := oldLen != newLen
 	computed := oldLen == 0 && newLen == 0 && schema.Computed
-	if changed || computed {
+	if changed || computed || all {
 		countSchema := &Schema{
 			Type:     TypeInt,
 			Computed: schema.Computed,
@@ -539,7 +541,7 @@ func (m schemaMap) diffList(
 		// just diff each.
 		for i := 0; i < maxLen; i++ {
 			subK := fmt.Sprintf("%s.%d", k, i)
-			err := m.diff(subK, &t2, diff, d)
+			err := m.diff(subK, &t2, diff, d, all)
 			if err != nil {
 				return err
 			}
@@ -549,7 +551,7 @@ func (m schemaMap) diffList(
 		for i := 0; i < maxLen; i++ {
 			for k2, schema := range t.Schema {
 				subK := fmt.Sprintf("%s.%d.%s", k, i, k2)
-				err := m.diff(subK, schema, diff, d)
+				err := m.diff(subK, schema, diff, d, all)
 				if err != nil {
 					return err
 				}
@@ -566,8 +568,8 @@ func (m schemaMap) diffMap(
 	k string,
 	schema *Schema,
 	diff *terraform.InstanceDiff,
-	d *ResourceData) error {
-	//elemSchema := &Schema{Type: TypeString}
+	d *ResourceData,
+	all bool) error {
 	prefix := k + "."
 
 	// First get all the values from the state
@@ -590,7 +592,7 @@ func (m schemaMap) diffMap(
 		old := stateMap[k]
 		delete(stateMap, k)
 
-		if old == v {
+		if old == v && !all {
 			continue
 		}
 
@@ -613,15 +615,35 @@ func (m schemaMap) diffSet(
 	k string,
 	schema *Schema,
 	diff *terraform.InstanceDiff,
-	d *ResourceData) error {
-	return m.diffList(k, schema, diff, d)
+	d *ResourceData,
+	all bool) error {
+	if !all {
+		// This is a bit strange, but we expect the entire set to be in the diff,
+		// so we first diff the set normally but with a new diff. Then, if
+		// there IS any change, we just set the change to the entire list.
+		tempD := new(terraform.InstanceDiff)
+		tempD.Attributes = make(map[string]*terraform.ResourceAttrDiff)
+		if err := m.diffList(k, schema, tempD, d, false); err != nil {
+			return err
+		}
+
+		// If we had no changes, then we're done
+		if tempD.Empty() {
+			return nil
+		}
+	}
+
+	// We have changes, so re-run the diff, but set a flag to force
+	// getting all diffs, even if there is no change.
+	return m.diffList(k, schema, diff, d, true)
 }
 
 func (m schemaMap) diffString(
 	k string,
 	schema *Schema,
 	diff *terraform.InstanceDiff,
-	d *ResourceData) error {
+	d *ResourceData,
+	all bool) error {
 	var originalN interface{}
 	var os, ns string
 	o, n, _, _ := d.diffChange(k)
@@ -646,7 +668,7 @@ func (m schemaMap) diffString(
 		return fmt.Errorf("%s: %s", k, err)
 	}
 
-	if os == ns {
+	if os == ns && !all {
 		// They're the same value. If there old value is not blank or we
 		// have an ID, then return right away since we're already setup.
 		if os != "" || d.Id() != "" {

helper/schema/schema_test.go

@@ -865,6 +865,14 @@ func TestSchemaMap_Diff(t *testing.T) {
 						Old: "2",
 						New: "3",
 					},
+					"ports.0": &terraform.ResourceAttrDiff{
+						Old: "1",
+						New: "1",
+					},
+					"ports.1": &terraform.ResourceAttrDiff{
+						Old: "2",
+						New: "2",
+					},
 					"ports.2": &terraform.ResourceAttrDiff{
 						Old: "",
 						New: "5",