provider/aws: Security Rules drift and sorting changes

This commit adds failing tests to demonstrate the problem presented with AWS
aggregating the security group rules

builtin/providers/aws/resource_aws_security_group.go

@@ -227,7 +227,7 @@ func resourceAwsSecurityGroupCreate(d *schema.ResourceData, meta interface{}) er
 	}
 
 	// AWS defaults all Security Groups to have an ALLOW ALL egress rule. Here we
-	// revoke that rule, so users don't unknowningly have/use it.
+	// revoke that rule, so users don't unknowingly have/use it.
 	group := resp.(*ec2.SecurityGroup)
 	if group.VpcId != nil && *group.VpcId != "" {
 		log.Printf("[DEBUG] Revoking default egress rule for Security Group for %s", d.Id())
@@ -273,13 +273,26 @@ func resourceAwsSecurityGroupRead(d *schema.ResourceData, meta interface{}) erro
 
 	sg := sgRaw.(*ec2.SecurityGroup)
 
-	ingressRules := resourceAwsSecurityGroupIPPermGather(d.Id(), sg.IpPermissions)
+	remoteIngressRules := resourceAwsSecurityGroupIPPermGather(d.Id(), sg.IpPermissions)
-	egressRules := resourceAwsSecurityGroupIPPermGather(d.Id(), sg.IpPermissionsEgress)
+	remoteEgressRules := resourceAwsSecurityGroupIPPermGather(d.Id(), sg.IpPermissionsEgress)
+
+	//
+	// TODO enforce the seperation of ips and security_groups in a rule block
+	//
+
+	localIngressRules := d.Get("ingress").(*schema.Set).List()
+	localEgressRules := d.Get("egress").(*schema.Set).List()
+
+	// Loop through the local state of rules, doing a match against the remote
+	// ruleSet we built above.
+	ingressRules := matchRules("ingress", localIngressRules, remoteIngressRules)
+	egressRules := matchRules("egress", localEgressRules, remoteEgressRules)
 
 	d.Set("description", sg.Description)
 	d.Set("name", sg.GroupName)
 	d.Set("vpc_id", sg.VpcId)
 	d.Set("owner_id", sg.OwnerId)
+
 	if err := d.Set("ingress", ingressRules); err != nil {
 		log.Printf("[WARN] Error setting Ingress rule set for (%s): %s", d.Id(), err)
 	}
@@ -593,3 +606,219 @@ func SGStateRefreshFunc(conn *ec2.EC2, id string) resource.StateRefreshFunc {
 		return group, "exists", nil
 	}
 }
+
+// matchRules receives the group id, type of rules, and the local / remote maps
+// of rules. We iterate through the local set of rules trying to find a matching
+// remote rule, which may be structured differently because of how AWS
+// aggregates the rules under the to, from, and type.
+//
+//
+// Matching rules are written to state, with their elements removed from the
+// remote set
+//
+// If no match is found, we'll write the remote rule to state and let the graph
+// sort things out
+func matchRules(rType string, local []interface{}, remote []map[string]interface{}) []map[string]interface{} {
+	// For each local ip or security_group, we need to match against the remote
+	// ruleSet until all ips or security_groups are found
+
+	// saves represents the rules that have been identified to be saved to state,
+	// in the appropriate d.Set("{ingress,egress}") call.
+	var saves []map[string]interface{}
+	for _, raw := range local {
+		l := raw.(map[string]interface{})
+
+		var selfVal bool
+		if v, ok := l["self"]; ok {
+			selfVal = v.(bool)
+		}
+
+		// matching against self is required to detect rules that only include self
+		// as the rule. resourceAwsSecurityGroupIPPermGather parses the group out
+		// and replaces it with self if it's ID is found
+		localHash := idHash(rType, l["protocol"].(string), int64(l["to_port"].(int)), int64(l["from_port"].(int)), selfVal)
+
+		// loop remote rules, looking for a matching hash
+		for _, r := range remote {
+			var remoteSelfVal bool
+			if v, ok := r["self"]; ok {
+				remoteSelfVal = v.(bool)
+			}
+
+			// hash this remote rule and compare it for a match consideration with the
+			// local rule we're examining
+			rHash := idHash(rType, r["protocol"].(string), r["to_port"].(int64), r["from_port"].(int64), remoteSelfVal)
+			if rHash == localHash {
+				var numExpectedCidrs, numExpectedSGs, numRemoteCidrs, numRemoteSGs int
+				var matchingCidrs []string
+				var matchingSGs []string
+
+				// grab the local/remote cidr and sg groups, capturing the expected and
+				// actual counts
+				lcRaw, ok := l["cidr_blocks"]
+				if ok {
+					numExpectedCidrs = len(l["cidr_blocks"].([]interface{}))
+				}
+				lsRaw, ok := l["security_groups"]
+				if ok {
+					numExpectedSGs = len(l["security_groups"].(*schema.Set).List())
+				}
+
+				rcRaw, ok := r["cidr_blocks"]
+				if ok {
+					numRemoteCidrs = len(r["cidr_blocks"].([]string))
+				}
+
+				rsRaw, ok := r["security_groups"]
+				if ok {
+					numRemoteSGs = len(r["security_groups"].(*schema.Set).List())
+				}
+
+				// check some early failures
+				if numExpectedCidrs > numRemoteCidrs {
+					log.Printf("[DEBUG] Local rule has more CIDR blocks, continuing (%d/%d)", numExpectedCidrs, numRemoteCidrs)
+					continue
+				}
+				if numExpectedSGs > numRemoteSGs {
+					log.Printf("[DEBUG] Local rule has more Security Groups, continuing (%d/%d)", numExpectedSGs, numRemoteSGs)
+					continue
+				}
+
+				// match CIDRs by converting both to sets, and using Set methods
+				var localCidrs []interface{}
+				if lcRaw != nil {
+					localCidrs = lcRaw.([]interface{})
+				}
+				localCidrSet := schema.NewSet(schema.HashString, localCidrs)
+
+				// remote cidrs are presented as a slice of strings, so we need to
+				// reformat them into a slice of interfaces to be used in creating the
+				// remote cidr set
+				var remoteCidrs []string
+				if rcRaw != nil {
+					remoteCidrs = rcRaw.([]string)
+				}
+				// convert remote cidrs to a set, for easy comparisions
+				var list []interface{}
+				for _, s := range remoteCidrs {
+					list = append(list, s)
+				}
+				remoteCidrSet := schema.NewSet(schema.HashString, list)
+
+				// Build up a list of local cidrs that are found in the remote set
+				for _, s := range localCidrSet.List() {
+					if remoteCidrSet.Contains(s) {
+						matchingCidrs = append(matchingCidrs, s.(string))
+					}
+				}
+
+				// match SGs. Both local and remote are already sets
+				var localSGSet *schema.Set
+				if lsRaw == nil {
+					localSGSet = schema.NewSet(schema.HashString, nil)
+				} else {
+					localSGSet = lsRaw.(*schema.Set)
+				}
+
+				var remoteSGSet *schema.Set
+				if rsRaw == nil {
+					remoteSGSet = schema.NewSet(schema.HashString, nil)
+				} else {
+					remoteSGSet = rsRaw.(*schema.Set)
+				}
+
+				// Build up a list of local security groups that are found in the remote set
+				for _, s := range localSGSet.List() {
+					if remoteSGSet.Contains(s) {
+						matchingSGs = append(matchingSGs, s.(string))
+					}
+				}
+
+				// compare equalities for matches.
+				// If we found the number of cidrs and number of sgs, we declare a
+				// match, and then remove those elements from the remote rule, so that
+				// this remote rule can still be considered by other local rules
+				if numExpectedCidrs == len(matchingCidrs) {
+					if numExpectedSGs == len(matchingSGs) {
+						// confirm that self references match
+						var lSelf bool
+						var rSelf bool
+						if _, ok := l["self"]; ok {
+							lSelf = l["self"].(bool)
+						}
+						if _, ok := r["self"]; ok {
+							rSelf = r["self"].(bool)
+						}
+						if rSelf == lSelf {
+							delete(r, "self")
+							// pop local cidrs from remote
+							diffCidr := remoteCidrSet.Difference(localCidrSet)
+							var newCidr []string
+							for _, cRaw := range diffCidr.List() {
+								newCidr = append(newCidr, cRaw.(string))
+							}
+
+							// reassigning
+							if len(newCidr) > 0 {
+								r["cidr_blocks"] = newCidr
+							} else {
+								delete(r, "cidr_blocks")
+							}
+
+							// pop local sgs from remote
+							diffSGs := remoteSGSet.Difference(localSGSet)
+							if len(diffSGs.List()) > 0 {
+								r["security_groups"] = diffSGs
+							} else {
+								delete(r, "security_groups")
+							}
+
+							saves = append(saves, l)
+						}
+					}
+				}
+			}
+		}
+	}
+
+	// Here we catch any remote rules that have not been stripped of all self,
+	// cidrs, and security groups. We'll add remote rules here that have not been
+	// matched locally, and let the graph sort things out. This will happen when
+	// rules are added externally to Terraform
+	for _, r := range remote {
+		var lenCidr, lenSGs int
+		if rCidrs, ok := r["cidr_blocks"]; ok {
+			lenCidr = len(rCidrs.([]string))
+		}
+
+		if rawSGs, ok := r["security_groups"]; ok {
+			lenSGs = len(rawSGs.(*schema.Set).List())
+		}
+
+		if _, ok := r["self"]; ok {
+			if r["self"].(bool) == true {
+				lenSGs++
+			}
+		}
+
+		if lenSGs+lenCidr > 0 {
+			log.Printf("[DEBUG] Found a remote Rule that wasn't empty: (%#v)", r)
+			saves = append(saves, r)
+		}
+	}
+
+	return saves
+}
+
+// Creates a unique hash for the type, ports, and protocol, used as a key in
+// maps
+func idHash(rType, protocol string, toPort, fromPort int64, self bool) string {
+	var buf bytes.Buffer
+	buf.WriteString(fmt.Sprintf("%s-", rType))
+	buf.WriteString(fmt.Sprintf("%d-", toPort))
+	buf.WriteString(fmt.Sprintf("%d-", fromPort))
+	buf.WriteString(fmt.Sprintf("%s-", protocol))
+	buf.WriteString(fmt.Sprintf("%t-", self))
+
+	return fmt.Sprintf("rule-%d", hashcode.String(buf.String()))
+}

builtin/providers/aws/resource_aws_security_group_rules_matching_test.go

@@ -0,0 +1,693 @@
+package aws
+
+import (
+	"log"
+	"testing"
+
+	"github.com/hashicorp/terraform/helper/schema"
+)
+
+// testing rulesForGroupPermissions
+func TestRulesMixedMatching(t *testing.T) {
+	cases := []struct {
+		groupId string
+		local   []interface{}
+		remote  []map[string]interface{}
+		saves   []map[string]interface{}
+	}{
+		{
+			local: []interface{}{
+				map[string]interface{}{
+					"from_port":       80,
+					"to_port":         8000,
+					"protocol":        "tcp",
+					"cidr_blocks":     []interface{}{"172.8.0.0/16", "10.0.0.0/16"},
+					"security_groups": schema.NewSet(schema.HashString, []interface{}{"sg-9876", "sg-4444"}),
+				},
+			},
+			remote: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":       int64(80),
+					"to_port":         int64(8000),
+					"protocol":        "tcp",
+					"cidr_blocks":     []string{"172.8.0.0/16", "10.0.0.0/16"},
+					"security_groups": schema.NewSet(schema.HashString, []interface{}{"sg-9876", "sg-4444"}),
+				},
+			},
+			saves: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":       80,
+					"to_port":         8000,
+					"protocol":        "tcp",
+					"cidr_blocks":     []string{"172.8.0.0/16", "10.0.0.0/16"},
+					"security_groups": schema.NewSet(schema.HashString, []interface{}{"sg-9876", "sg-4444"}),
+				},
+			},
+		},
+		{
+			local: []interface{}{
+				map[string]interface{}{
+					"from_port":       80,
+					"to_port":         8000,
+					"protocol":        "tcp",
+					"security_groups": schema.NewSet(schema.HashString, []interface{}{"sg-9876", "sg-4444"}),
+				},
+			},
+			remote: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":       int64(80),
+					"to_port":         int64(8000),
+					"protocol":        "tcp",
+					"cidr_blocks":     []string{"172.8.0.0/16", "10.0.0.0/16"},
+					"security_groups": schema.NewSet(schema.HashString, []interface{}{"sg-9876", "sg-4444"}),
+				},
+			},
+			saves: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":       80,
+					"to_port":         8000,
+					"protocol":        "tcp",
+					"security_groups": schema.NewSet(schema.HashString, []interface{}{"sg-9876", "sg-4444"}),
+				},
+				map[string]interface{}{
+					"from_port":   int64(80),
+					"to_port":     int64(8000),
+					"protocol":    "tcp",
+					"cidr_blocks": []string{"172.8.0.0/16", "10.0.0.0/16"},
+				},
+			},
+		},
+		{
+			local: []interface{}{
+				map[string]interface{}{
+					"from_port":   80,
+					"to_port":     8000,
+					"protocol":    "tcp",
+					"cidr_blocks": []interface{}{"172.8.0.0/16", "10.0.0.0/16"},
+				},
+			},
+			remote: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":   int64(80),
+					"to_port":     int64(8000),
+					"protocol":    "tcp",
+					"cidr_blocks": []string{"172.8.0.0/16", "10.0.0.0/16"},
+				},
+			},
+			saves: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":   80,
+					"to_port":     8000,
+					"protocol":    "tcp",
+					"cidr_blocks": []string{"172.8.0.0/16", "10.0.0.0/16"},
+				},
+			},
+		},
+		{
+			local: []interface{}{
+				map[string]interface{}{
+					"from_port":       80,
+					"to_port":         8000,
+					"protocol":        "tcp",
+					"security_groups": schema.NewSet(schema.HashString, []interface{}{"sg-9876", "sg-4444"}),
+				},
+			},
+			remote: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":       int64(80),
+					"to_port":         int64(8000),
+					"protocol":        "tcp",
+					"security_groups": schema.NewSet(schema.HashString, []interface{}{"sg-9876", "sg-4444"}),
+				},
+			},
+			saves: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":       80,
+					"to_port":         8000,
+					"protocol":        "tcp",
+					"security_groups": schema.NewSet(schema.HashString, []interface{}{"sg-9876", "sg-4444"}),
+				},
+			},
+		},
+		{
+			local: []interface{}{
+				map[string]interface{}{
+					"from_port":   80,
+					"to_port":     8000,
+					"protocol":    "tcp",
+					"cidr_blocks": []interface{}{"172.8.0.0/16"},
+				},
+				map[string]interface{}{
+					"from_port":   80,
+					"to_port":     8000,
+					"protocol":    "tcp",
+					"cidr_blocks": []interface{}{"192.168.0.0/16"},
+				},
+			},
+			remote: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":   int64(80),
+					"to_port":     int64(8000),
+					"protocol":    "tcp",
+					"cidr_blocks": []string{"172.8.0.0/16", "192.168.0.0/16"},
+				},
+			},
+			saves: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":   80,
+					"to_port":     8000,
+					"protocol":    "tcp",
+					"cidr_blocks": []string{"172.8.0.0/16"},
+				},
+				map[string]interface{}{
+					"from_port":   80,
+					"to_port":     8000,
+					"protocol":    "tcp",
+					"cidr_blocks": []string{"192.168.0.0/16"},
+				},
+			},
+		},
+		{
+			local: []interface{}{},
+			remote: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":   int64(80),
+					"to_port":     int64(8000),
+					"protocol":    "tcp",
+					"cidr_blocks": []string{"172.8.0.0/16", "10.0.0.0/16"},
+				},
+			},
+			saves: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":   int64(80),
+					"to_port":     int64(8000),
+					"protocol":    "tcp",
+					"cidr_blocks": []string{"172.8.0.0/16", "10.0.0.0/16"},
+				},
+			},
+		},
+		// local and remote differ
+		{
+			local: []interface{}{
+				map[string]interface{}{
+					"from_port":   80,
+					"to_port":     8000,
+					"protocol":    "tcp",
+					"cidr_blocks": []interface{}{"172.8.0.0/16"},
+				},
+			},
+			remote: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":   int64(80),
+					"to_port":     int64(8000),
+					"protocol":    "tcp",
+					"cidr_blocks": []string{"10.0.0.0/16"},
+				},
+			},
+			// Because this is the remote rule being saved, we need to check for int64
+			// encoding. We could convert this code, but ultimately Terraform doesn't
+			// care it's for the reflect.DeepEqual in this test
+			saves: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":   int64(80),
+					"to_port":     int64(8000),
+					"protocol":    "tcp",
+					"cidr_blocks": []string{"10.0.0.0/16"},
+				},
+			},
+		},
+		// local with more rules and the remote (the remote should then be saved)
+		{
+			local: []interface{}{
+				map[string]interface{}{
+					"from_port":   80,
+					"to_port":     8000,
+					"protocol":    "tcp",
+					"cidr_blocks": []interface{}{"172.8.0.0/16", "10.8.0.0/16", "192.168.0.0/16"},
+				},
+			},
+			remote: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":   int64(80),
+					"to_port":     int64(8000),
+					"protocol":    "tcp",
+					"cidr_blocks": []string{"172.8.0.0/16", "192.168.0.0/16"},
+				},
+			},
+			saves: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":   int64(80),
+					"to_port":     int64(8000),
+					"protocol":    "tcp",
+					"cidr_blocks": []string{"172.8.0.0/16", "192.168.0.0/16"},
+				},
+			},
+		},
+		// 3 local rules
+		// this should trigger a diff (not shown)
+		{
+			local: []interface{}{
+				map[string]interface{}{
+					"from_port":   80,
+					"to_port":     8000,
+					"protocol":    "tcp",
+					"cidr_blocks": []interface{}{"172.8.0.0/16"},
+				},
+				map[string]interface{}{
+					"from_port":   80,
+					"to_port":     8000,
+					"protocol":    "tcp",
+					"cidr_blocks": []interface{}{"10.8.0.0/16"},
+				},
+				map[string]interface{}{
+					"from_port":   80,
+					"to_port":     8000,
+					"protocol":    "tcp",
+					"cidr_blocks": []interface{}{"192.168.0.0/16"},
+				},
+			},
+			remote: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":   int64(80),
+					"to_port":     int64(8000),
+					"protocol":    "tcp",
+					"cidr_blocks": []string{"172.8.0.0/16", "192.168.0.0/16"},
+				},
+			},
+			saves: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":   80,
+					"to_port":     8000,
+					"protocol":    "tcp",
+					"cidr_blocks": []string{"172.8.0.0/16"},
+				},
+				map[string]interface{}{
+					"from_port":   80,
+					"to_port":     8000,
+					"protocol":    "tcp",
+					"cidr_blocks": []string{"192.168.0.0/16"},
+				},
+			},
+		},
+		// a local rule with 2 cidrs, remote has 4 cidrs, shoudl be saved to match
+		// the local but also an extra rule found
+		{
+			local: []interface{}{
+				map[string]interface{}{
+					"from_port":   80,
+					"to_port":     8000,
+					"protocol":    "tcp",
+					"cidr_blocks": []interface{}{"172.8.0.0/16", "10.8.0.0/16"},
+				},
+			},
+			remote: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":   int64(80),
+					"to_port":     int64(8000),
+					"protocol":    "tcp",
+					"cidr_blocks": []string{"172.8.0.0/16", "192.168.0.0/16", "10.8.0.0/16", "206.8.0.0/16"},
+				},
+			},
+			saves: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":   80,
+					"to_port":     8000,
+					"protocol":    "tcp",
+					"cidr_blocks": []string{"172.8.0.0/16", "10.8.0.0/16"},
+				},
+				map[string]interface{}{
+					"from_port":   int64(80),
+					"to_port":     int64(8000),
+					"protocol":    "tcp",
+					"cidr_blocks": []string{"192.168.0.0/16", "206.8.0.0/16"},
+				},
+			},
+		},
+		// testing some SGS
+		{
+			local: []interface{}{},
+			remote: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":       int64(22),
+					"to_port":         int64(22),
+					"protocol":        "tcp",
+					"security_groups": schema.NewSet(schema.HashString, []interface{}{"sg-9876"}),
+				},
+			},
+			saves: []map[string]interface{}{
+				map[string]interface{}{
+					// we're saving the remote, so it will be int64 encoded
+					"from_port":       int64(22),
+					"to_port":         int64(22),
+					"protocol":        "tcp",
+					"security_groups": schema.NewSet(schema.HashString, []interface{}{"sg-9876"}),
+				},
+			},
+		},
+		// two local blocks that match a single remote group, but are saved as two
+		{
+			local: []interface{}{
+				map[string]interface{}{
+					"from_port":       22,
+					"to_port":         22,
+					"protocol":        "tcp",
+					"security_groups": schema.NewSet(schema.HashString, []interface{}{"sg-9876"}),
+				},
+				map[string]interface{}{
+					"from_port":       22,
+					"to_port":         22,
+					"protocol":        "tcp",
+					"security_groups": schema.NewSet(schema.HashString, []interface{}{"sg-4444"}),
+				},
+			},
+			remote: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port": int64(22),
+					"to_port":   int64(22),
+					"protocol":  "tcp",
+					"security_groups": schema.NewSet(
+						schema.HashString,
+						[]interface{}{
+							"sg-9876",
+							"sg-4444",
+						},
+					),
+				},
+			},
+			saves: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port": 22,
+					"to_port":   22,
+					"protocol":  "tcp",
+					"security_groups": schema.NewSet(
+						schema.HashString,
+						[]interface{}{
+							"sg-9876",
+						},
+					),
+				},
+				map[string]interface{}{
+					"from_port": 22,
+					"to_port":   22,
+					"protocol":  "tcp",
+					"security_groups": schema.NewSet(
+						schema.HashString,
+						[]interface{}{
+							"sg-4444",
+						},
+					),
+				},
+			},
+		},
+		// test self with other rules
+		{
+			local: []interface{}{
+				map[string]interface{}{
+					"from_port":       22,
+					"to_port":         22,
+					"protocol":        "tcp",
+					"self":            true,
+					"security_groups": schema.NewSet(schema.HashString, []interface{}{"sg-9876", "sg-4444"}),
+				},
+			},
+			remote: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port": int64(22),
+					"to_port":   int64(22),
+					"protocol":  "tcp",
+					"self":      true,
+				},
+			},
+			saves: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port": int64(22),
+					"to_port":   int64(22),
+					"protocol":  "tcp",
+					"self":      true,
+				},
+			},
+		},
+		// test self
+		{
+			local: []interface{}{
+				map[string]interface{}{
+					"from_port": 22,
+					"to_port":   22,
+					"protocol":  "tcp",
+					"self":      true,
+				},
+			},
+			remote: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port": int64(22),
+					"to_port":   int64(22),
+					"protocol":  "tcp",
+					"self":      true,
+				},
+			},
+			saves: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port": int64(22),
+					"to_port":   int64(22),
+					"protocol":  "tcp",
+					"self":      true,
+				},
+			},
+		},
+		{
+			local: []interface{}{
+				map[string]interface{}{
+					"from_port":       22,
+					"to_port":         22,
+					"protocol":        "tcp",
+					"security_groups": schema.NewSet(schema.HashString, []interface{}{"sg-9876", "sg-4444"}),
+				},
+			},
+			remote: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port": int64(22),
+					"to_port":   int64(22),
+					"protocol":  "tcp",
+					"self":      true,
+				},
+			},
+			saves: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port": int64(22),
+					"to_port":   int64(22),
+					"protocol":  "tcp",
+					"self":      true,
+				},
+			},
+		},
+		// mix of sgs and cidrs
+		{
+			local: []interface{}{
+				map[string]interface{}{
+					"from_port":   80,
+					"to_port":     8000,
+					"protocol":    "tcp",
+					"cidr_blocks": []interface{}{"172.8.0.0/16", "10.8.0.0/16", "192.168.0.0/16"},
+				},
+				map[string]interface{}{
+					"from_port":       80,
+					"to_port":         8000,
+					"protocol":        "tcp",
+					"security_groups": schema.NewSet(schema.HashString, []interface{}{"sg-9876", "sg-4444"}),
+				},
+			},
+			remote: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":       int64(80),
+					"to_port":         int64(8000),
+					"protocol":        "tcp",
+					"cidr_blocks":     []string{"172.8.0.0/16", "192.168.0.0/16"},
+					"security_groups": schema.NewSet(schema.HashString, []interface{}{"sg-9876", "sg-4444"}),
+				},
+			},
+			saves: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":   int64(80),
+					"to_port":     int64(8000),
+					"protocol":    "tcp",
+					"cidr_blocks": []string{"172.8.0.0/16", "192.168.0.0/16"},
+				},
+				map[string]interface{}{
+					"from_port":       int64(80),
+					"to_port":         int64(8000),
+					"protocol":        "tcp",
+					"security_groups": schema.NewSet(schema.HashString, []interface{}{"sg-9876", "sg-4444"}),
+				},
+			},
+		},
+		{
+			local: []interface{}{
+				map[string]interface{}{
+					"from_port":   80,
+					"to_port":     8000,
+					"protocol":    "tcp",
+					"cidr_blocks": []interface{}{"172.8.0.0/16", "10.8.0.0/16", "192.168.0.0/16"},
+				},
+				map[string]interface{}{
+					"from_port": 80,
+					"to_port":   8000,
+					"protocol":  "tcp",
+					"self":      true,
+				},
+			},
+			remote: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port":       int64(80),
+					"to_port":         int64(8000),
+					"protocol":        "tcp",
+					"cidr_blocks":     []string{"172.8.0.0/16", "192.168.0.0/16"},
+					"self":            true,
+					"security_groups": schema.NewSet(schema.HashString, []interface{}{"sg-9876", "sg-4444"}),
+				},
+			},
+			saves: []map[string]interface{}{
+				map[string]interface{}{
+					"from_port": 80,
+					"to_port":   8000,
+					"protocol":  "tcp",
+					"self":      true,
+				},
+				map[string]interface{}{
+					"from_port":       int64(80),
+					"to_port":         int64(8000),
+					"protocol":        "tcp",
+					"cidr_blocks":     []string{"172.8.0.0/16", "192.168.0.0/16"},
+					"security_groups": schema.NewSet(schema.HashString, []interface{}{"sg-9876", "sg-4444"}),
+				},
+			},
+		},
+	}
+	for i, c := range cases {
+		saves := matchRules("ingress", c.local, c.remote)
+		log.Printf("\n======\n\nSaves:\n%#v\n\nCS Saves:\n%#v\n\n======\n", saves, c.saves)
+		if err != nil {
+			t.Fatal(err)
+		}
+		log.Printf("\n\tTest %d:\n", i)
+
+		if len(saves) != len(c.saves) {
+			t.Fatalf("Expected %d saves, got %d", len(c.saves), len(saves))
+		}
+
+		shouldFind := len(c.saves)
+		var found int
+		for _, s := range saves {
+			for _, cs := range c.saves {
+				// deep equal cannot compare schema.Set's directly
+				// make sure we're not failing the reflect b/c of ports/type
+				for _, attr := range []string{"to_port", "from_port", "type"} {
+					if s[attr] != cs[attr] {
+						continue
+					}
+				}
+
+				var numExpectedCidrs, numExpectedSGs, numRemoteCidrs, numRemoteSGs int
+				// var matchingCidrs []string
+				// var matchingSGs []string
+
+				var cidrsMatch, sGsMatch bool
+
+				if _, ok := s["cidr_blocks"]; ok {
+					switch s["cidr_blocks"].(type) {
+					case []string:
+						numExpectedCidrs = len(s["cidr_blocks"].([]string))
+					default:
+						numExpectedCidrs = len(s["cidr_blocks"].([]interface{}))
+					}
+
+				}
+				if _, ok := s["security_groups"]; ok {
+					numExpectedSGs = len(s["security_groups"].(*schema.Set).List())
+				}
+
+				if _, ok := cs["cidr_blocks"]; ok {
+					numRemoteCidrs = len(cs["cidr_blocks"].([]string))
+				}
+
+				if _, ok := cs["security_groups"]; ok {
+					numRemoteSGs = len(cs["security_groups"].(*schema.Set).List())
+				}
+
+				// skip early
+				if numExpectedSGs != numRemoteSGs {
+					log.Printf("\n\ncontinuning on numRemoteSGs \n\n")
+					continue
+				}
+				if numExpectedCidrs != numRemoteCidrs {
+					log.Printf("\n\ncontinuning numRemoteCidrs\n\n")
+					continue
+				}
+
+				if numExpectedCidrs == 0 {
+					cidrsMatch = true
+				}
+				if numExpectedSGs == 0 {
+					sGsMatch = true
+				}
+
+				// convert save cidrs to set
+				var lcs []interface{}
+				if _, ok := s["cidr_blocks"]; ok {
+					switch s["cidr_blocks"].(type) {
+					case []string:
+						for _, c := range s["cidr_blocks"].([]string) {
+							lcs = append(lcs, c)
+						}
+					default:
+						for _, c := range s["cidr_blocks"].([]interface{}) {
+							lcs = append(lcs, c)
+						}
+					}
+				}
+				savesCidrs := schema.NewSet(schema.HashString, lcs)
+
+				// convert cs cidrs to set
+				var cslcs []interface{}
+				if _, ok := cs["cidr_blocks"]; ok {
+					for _, c := range cs["cidr_blocks"].([]string) {
+						cslcs = append(cslcs, c)
+					}
+				}
+				csCidrs := schema.NewSet(schema.HashString, cslcs)
+
+				if csCidrs.Equal(savesCidrs) {
+					log.Printf("\nmatched cidrs")
+					cidrsMatch = true
+				}
+
+				if rawS, ok := s["security_groups"]; ok {
+					outSet := rawS.(*schema.Set)
+					if rawL, ok := cs["security_groups"]; ok {
+						localSet := rawL.(*schema.Set)
+						if outSet.Equal(localSet) {
+							log.Printf("\nmatched sgs")
+							sGsMatch = true
+						}
+					}
+				}
+
+				var lSelf bool
+				var rSelf bool
+				if _, ok := s["self"]; ok {
+					lSelf = s["self"].(bool)
+				}
+				if _, ok := cs["self"]; ok {
+					rSelf = cs["self"].(bool)
+				}
+
+				if (sGsMatch && cidrsMatch) && (lSelf == rSelf) {
+					found++
+				}
+			}
+		}
+
+		if found != shouldFind {
+			t.Fatalf("Bad sg rule matches (%d / %d)", found, shouldFind)
+		}
+	}
+}

builtin/providers/aws/resource_aws_security_group_test.go

@@ -9,6 +9,7 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/awserr"
 	"github.com/aws/aws-sdk-go/service/ec2"
+	"github.com/hashicorp/terraform/helper/acctest"
 	"github.com/hashicorp/terraform/helper/resource"
 	"github.com/hashicorp/terraform/helper/schema"
 	"github.com/hashicorp/terraform/terraform"
@@ -383,6 +384,66 @@ func TestAccAWSSecurityGroup_DefaultEgress(t *testing.T) {
 	})
 }
 
+// Testing drift detection with groups containing the same port and types
+func TestAccAWSSecurityGroup_drift(t *testing.T) {
+	var group ec2.SecurityGroup
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckAWSSecurityGroupDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccAWSSecurityGroupConfig_drift(),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSSecurityGroupExists("aws_security_group.web", &group),
+					resource.TestCheckResourceAttr(
+						"aws_security_group.web", "description", "Used in the terraform acceptance tests"),
+					resource.TestCheckResourceAttr(
+						"aws_security_group.web", "ingress.3629188364.protocol", "tcp"),
+					resource.TestCheckResourceAttr(
+						"aws_security_group.web", "ingress.3629188364.from_port", "80"),
+					resource.TestCheckResourceAttr(
+						"aws_security_group.web", "ingress.3629188364.to_port", "8000"),
+					resource.TestCheckResourceAttr(
+						"aws_security_group.web", "ingress.3629188364.cidr_blocks.#", "1"),
+					resource.TestCheckResourceAttr(
+						"aws_security_group.web", "ingress.3629188364.cidr_blocks.0", "10.0.0.0/8"),
+				),
+			},
+		},
+	})
+}
+
+func TestAccAWSSecurityGroup_drift_complex(t *testing.T) {
+	var group ec2.SecurityGroup
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckAWSSecurityGroupDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccAWSSecurityGroupConfig_drift_complex(),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSSecurityGroupExists("aws_security_group.web", &group),
+					resource.TestCheckResourceAttr(
+						"aws_security_group.web", "description", "Used in the terraform acceptance tests"),
+					resource.TestCheckResourceAttr(
+						"aws_security_group.web", "ingress.3629188364.protocol", "tcp"),
+					resource.TestCheckResourceAttr(
+						"aws_security_group.web", "ingress.3629188364.from_port", "80"),
+					resource.TestCheckResourceAttr(
+						"aws_security_group.web", "ingress.3629188364.to_port", "8000"),
+					resource.TestCheckResourceAttr(
+						"aws_security_group.web", "ingress.3629188364.cidr_blocks.#", "1"),
+					resource.TestCheckResourceAttr(
+						"aws_security_group.web", "ingress.3629188364.cidr_blocks.0", "10.0.0.0/8"),
+				),
+			},
+		},
+	})
+}
+
 func testAccCheckAWSSecurityGroupDestroy(s *terraform.State) error {
 	conn := testAccProvider.Meta().(*AWSClient).ec2conn
 
@@ -556,6 +617,25 @@ func TestAccAWSSecurityGroup_tags(t *testing.T) {
 	})
 }
 
+func TestAccAWSSecurityGroup_CIDRandGroups(t *testing.T) {
+	var group ec2.SecurityGroup
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckAWSSecurityGroupDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccAWSSecurityGroupCombindCIDRandGroups,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSSecurityGroupExists("aws_security_group.mixed", &group),
+					// testAccCheckAWSSecurityGroupAttributes(&group),
+				),
+			},
+		},
+	})
+}
+
 func testAccCheckAWSSecurityGroupAttributesChanged(group *ec2.SecurityGroup) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
 		p := []*ec2.IpPermission{
@@ -931,3 +1011,133 @@ resource "aws_security_group" "baz" {
    description = "Used in the terraform acceptance tests"
 }
 `
+
+func testAccAWSSecurityGroupConfig_drift() string {
+	return fmt.Sprintf(`
+resource "aws_security_group" "web" {
+  name = "tf_acc_%d"
+  description = "Used in the terraform acceptance tests"
+
+  ingress {
+    protocol = "tcp"
+    from_port = 80
+    to_port = 8000
+    cidr_blocks = ["10.0.0.0/8"]
+  }
+
+  ingress {
+    protocol = "tcp"
+    from_port = 80
+    to_port = 8000
+    cidr_blocks = ["206.0.0.0/8"]
+  }
+
+        tags {
+                Name = "tf-acc-test"
+        }
+}
+`, acctest.RandInt())
+}
+
+func testAccAWSSecurityGroupConfig_drift_complex() string {
+	return fmt.Sprintf(`
+resource "aws_security_group" "otherweb" {
+  name        = "tf_acc_%d"
+  description = "Used in the terraform acceptance tests"
+}
+
+resource "aws_security_group" "web" {
+  name        = "tf_acc_%d"
+  description = "Used in the terraform acceptance tests"
+
+  ingress {
+    protocol    = "tcp"
+    from_port   = 80
+    to_port     = 8000
+    cidr_blocks = ["10.0.0.0/8"]
+  }
+
+  ingress {
+    protocol    = "tcp"
+    from_port   = 80
+    to_port     = 8000
+    cidr_blocks = ["206.0.0.0/8"]
+  }
+
+  ingress {
+    protocol        = "tcp"
+    from_port       = 22
+    to_port         = 22
+    security_groups = ["${aws_security_group.otherweb.id}"]
+  }
+
+  egress {
+    protocol    = "tcp"
+    from_port   = 80
+    to_port     = 8000
+    cidr_blocks = ["206.0.0.0/8"]
+  }
+
+  egress {
+    protocol    = "tcp"
+    from_port   = 80
+    to_port     = 8000
+    cidr_blocks = ["10.0.0.0/8"]
+  }
+
+  egress {
+    protocol        = "tcp"
+    from_port       = 22
+    to_port         = 22
+    security_groups = ["${aws_security_group.otherweb.id}"]
+  }
+
+  tags {
+    Name = "tf-acc-test"
+  }
+}`, acctest.RandInt(), acctest.RandInt())
+}
+
+const testAccAWSSecurityGroupCombindCIDRandGroups = `
+resource "aws_security_group" "two" {
+	name = "tf-test-1"
+	tags {
+		Name = "tf-test-1"
+	}
+}
+
+resource "aws_security_group" "one" {
+	name = "tf-test-2"
+	tags {
+		Name = "tf-test-w"
+	}
+}
+
+resource "aws_security_group" "three" {
+	name = "tf-test-3"
+	tags {
+		Name = "tf-test-3"
+	}
+}
+
+resource "aws_security_group" "mixed" {
+  name = "tf-mix-test"
+
+  ingress {
+    from_port   = 80
+    to_port     = 80
+    protocol    = "tcp"
+    cidr_blocks = ["10.0.0.0/16", "10.1.0.0/16", "10.7.0.0/16"]
+
+    security_groups = [
+      "${aws_security_group.one.id}",
+      "${aws_security_group.two.id}",
+      "${aws_security_group.three.id}",
+    ]
+  }
+
+  tags {
+    Name = "tf-mix-test"
+  }
+}
+`