Allow import of aws_security_groups with more than one source_security_group_id rule (#9477)

* Allow import of aws_security_groups with more than one source_security_group_id rule

* Add acceptable test for security group with multiple source rules.

builtin/providers/aws/import_aws_security_group.go

@@ -25,7 +25,6 @@ func resourceAwsSecurityGroupImportState(
 		return nil, fmt.Errorf("security group not found")
 	}
 	sg := sgRaw.(*ec2.SecurityGroup)
-	sgId := d.Id()
 
 	// Start building our results
 	results := make([]*schema.ResourceData, 1,
@@ -33,60 +32,101 @@ func resourceAwsSecurityGroupImportState(
 	results[0] = d
 
 	// Construct the rules
-	ruleResource := resourceAwsSecurityGroupRule()
 	permMap := map[string][]*ec2.IpPermission{
 		"ingress": sg.IpPermissions,
 		"egress":  sg.IpPermissionsEgress,
 	}
 	for ruleType, perms := range permMap {
 		for _, perm := range perms {
-			// Construct the rule. We do this by populating the absolute
+			ds, err := resourceAwsSecurityGroupImportStatePerm(sg, ruleType, perm)
-			// minimum necessary for Refresh on the rule to work. This
+			if err != nil {
-			// happens to be a lot of fields since they're almost all needed
+				return nil, err
-			// for de-dupping.
-			id := ipPermissionIDHash(sgId, ruleType, perm)
-			d := ruleResource.Data(nil)
-			d.SetId(id)
-			d.SetType("aws_security_group_rule")
-			d.Set("security_group_id", sgId)
-			d.Set("type", ruleType)
-
-			// 'self' is false by default. Below, we range over the group ids and set true
-			// if the parent sg id is found
-			d.Set("self", false)
-
-			if len(perm.UserIdGroupPairs) > 0 {
-				s := perm.UserIdGroupPairs[0]
-
-				// Check for Pair that is the same as the Security Group, to denote self.
-				// Otherwise, mark the group id in source_security_group_id
-				isVPC := sg.VpcId != nil && *sg.VpcId != ""
-				if isVPC {
-					if *s.GroupId == *sg.GroupId {
-						d.Set("self", true)
-						// prune the self reference from the UserIdGroupPairs, so we don't
-						// have duplicate sg ids (both self and in source_security_group_id)
-						perm.UserIdGroupPairs = append(perm.UserIdGroupPairs[:0], perm.UserIdGroupPairs[0+1:]...)
-					}
-				} else {
-					if *s.GroupName == *sg.GroupName {
-						d.Set("self", true)
-						// prune the self reference from the UserIdGroupPairs, so we don't
-						// have duplicate sg ids (both self and in source_security_group_id)
-						perm.UserIdGroupPairs = append(perm.UserIdGroupPairs[:0], perm.UserIdGroupPairs[0+1:]...)
-					}
-				}
 			}
-
+			results = append(results, ds...)
-			// XXX If the rule contained more than one source security group, this
-			// will choose one of them. We actually need to create one rule for each
-			// source security group.
-			if err := setFromIPPerm(d, sg, perm); err != nil {
-				return nil, errwrap.Wrapf("Error importing AWS Security Group: {{err}}", err)
-			}
-			results = append(results, d)
 		}
 	}
 
 	return results, nil
 }
+
+func resourceAwsSecurityGroupImportStatePerm(sg *ec2.SecurityGroup, ruleType string, perm *ec2.IpPermission) ([]*schema.ResourceData, error) {
+	var result []*schema.ResourceData
+
+	if len(perm.UserIdGroupPairs) == 0 {
+		r, err := resourceAwsSecurityGroupImportStatePermPair(sg, ruleType, perm)
+		if err != nil {
+			return nil, err
+		}
+		result = append(result, r)
+	} else {
+		// If the rule contained more than one source security group, this
+		// will iterate over them and create one rule for each
+		// source security group.
+		for _, pair := range perm.UserIdGroupPairs {
+			p := &ec2.IpPermission{
+				FromPort:      perm.FromPort,
+				IpProtocol:    perm.IpProtocol,
+				IpRanges:      perm.IpRanges,
+				PrefixListIds: perm.PrefixListIds,
+				ToPort:        perm.ToPort,
+
+				UserIdGroupPairs: []*ec2.UserIdGroupPair{pair},
+			}
+
+			r, err := resourceAwsSecurityGroupImportStatePermPair(sg, ruleType, p)
+			if err != nil {
+				return nil, err
+			}
+			result = append(result, r)
+		}
+	}
+	return result, nil
+}
+
+func resourceAwsSecurityGroupImportStatePermPair(sg *ec2.SecurityGroup, ruleType string, perm *ec2.IpPermission) (*schema.ResourceData, error) {
+	// Construct the rule. We do this by populating the absolute
+	// minimum necessary for Refresh on the rule to work. This
+	// happens to be a lot of fields since they're almost all needed
+	// for de-dupping.
+	sgId := sg.GroupId
+	id := ipPermissionIDHash(*sgId, ruleType, perm)
+	ruleResource := resourceAwsSecurityGroupRule()
+	d := ruleResource.Data(nil)
+	d.SetId(id)
+	d.SetType("aws_security_group_rule")
+	d.Set("security_group_id", sgId)
+	d.Set("type", ruleType)
+
+	// 'self' is false by default. Below, we range over the group ids and set true
+	// if the parent sg id is found
+	d.Set("self", false)
+
+	if len(perm.UserIdGroupPairs) > 0 {
+		s := perm.UserIdGroupPairs[0]
+
+		// Check for Pair that is the same as the Security Group, to denote self.
+		// Otherwise, mark the group id in source_security_group_id
+		isVPC := sg.VpcId != nil && *sg.VpcId != ""
+		if isVPC {
+			if *s.GroupId == *sg.GroupId {
+				d.Set("self", true)
+				// prune the self reference from the UserIdGroupPairs, so we don't
+				// have duplicate sg ids (both self and in source_security_group_id)
+				perm.UserIdGroupPairs = append(perm.UserIdGroupPairs[:0], perm.UserIdGroupPairs[0+1:]...)
+			}
+		} else {
+			if *s.GroupName == *sg.GroupName {
+				d.Set("self", true)
+				// prune the self reference from the UserIdGroupPairs, so we don't
+				// have duplicate sg ids (both self and in source_security_group_id)
+				perm.UserIdGroupPairs = append(perm.UserIdGroupPairs[:0], perm.UserIdGroupPairs[0+1:]...)
+			}
+		}
+	}
+
+	if err := setFromIPPerm(d, sg, perm); err != nil {
+		return nil, errwrap.Wrapf("Error importing AWS Security Group: {{err}}", err)
+	}
+
+	return d, nil
+}

builtin/providers/aws/import_aws_security_group_test.go

@@ -54,3 +54,22 @@ func TestAccAWSSecurityGroup_importSelf(t *testing.T) {
 		},
 	})
 }
+
+func TestAccAWSSecurityGroup_importSourceSecurityGroup(t *testing.T) {
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckAWSSecurityGroupDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccAWSSecurityGroupConfig_importSourceSecurityGroup,
+			},
+
+			resource.TestStep{
+				ResourceName:      "aws_security_group.test_group_1",
+				ImportState:       true,
+				ImportStateVerify: true,
+			},
+		},
+	})
+}

builtin/providers/aws/resource_aws_security_group_test.go

@@ -1813,6 +1813,51 @@ resource "aws_security_group_rule" "allow_all-1" {
 }
 `
 
+const testAccAWSSecurityGroupConfig_importSourceSecurityGroup = `
+resource "aws_vpc" "foo" {
+  cidr_block = "10.1.0.0/16"
+
+  tags {
+    Name = "tf_sg_import_test"
+  }
+}
+
+resource "aws_security_group" "test_group_1" {
+  name        = "test group 1"
+  vpc_id      = "${aws_vpc.foo.id}"
+}
+
+resource "aws_security_group" "test_group_2" {
+  name        = "test group 2"
+  vpc_id      = "${aws_vpc.foo.id}"
+}
+
+resource "aws_security_group" "test_group_3" {
+  name        = "test group 3"
+  vpc_id      = "${aws_vpc.foo.id}"
+}
+
+resource "aws_security_group_rule" "allow_test_group_2" {
+  type      = "ingress"
+  from_port = 0
+  to_port   = 0
+  protocol  = "tcp"
+
+  source_security_group_id = "${aws_security_group.test_group_1.id}"
+  security_group_id = "${aws_security_group.test_group_2.id}"
+}
+
+resource "aws_security_group_rule" "allow_test_group_3" {
+  type      = "ingress"
+  from_port = 0
+  to_port   = 0
+  protocol  = "tcp"
+
+  source_security_group_id = "${aws_security_group.test_group_1.id}"
+  security_group_id = "${aws_security_group.test_group_3.id}"
+}
+`
+
 const testAccAWSSecurityGroupConfigPrefixListEgress = `
 resource "aws_vpc" "tf_sg_prefix_list_egress_test" {
     cidr_block = "10.0.0.0/16"