r/aws_instance: Fix dirty plan on default VPC instances using vpc_sec…

…urity_group_ids

Unlike instances launched into a default VPC are allowed to refer to their security groups by ID, like other VPCs, or by name, like EC2-Classic. The current implementation of the function that reads instance security group data assumes that instances in a default VPC refer to their security groups by name. This causes an instance resource that is launched in a default VPC but using the `vpc_security_group_ids` parameter (instead of the `security_groups` parameter) to always have a diff in plans post-creation showing that the security groups need to be added to it.

This commit changes the behavior of the function that reads instance security data to be able to store BOTH the security group names and IDs in the case of an instance in a default VPC. Because both the `security_groups` and `vpc_security_group_ids` parameters are marked as "computed", it's valid to provide either in the resource configuration even though both will end up in state.

This commit also adds a failing test for the case of using `vpc_security_group_ids` with a default VPC, and ensures that both default VPC import tests are run in a region with a default VPC (which specifically must _not_ be an EC2-Classic region).

Fixes hashicorp#1799
Fixes hashicorp#1993

aws/import_aws_instance_test.go

@@ -30,7 +30,7 @@ func TestAccAWSInstance_importBasic(t *testing.T) {
 	})
 }
 
-func TestAccAWSInstance_importInDefaultVpc(t *testing.T) {
+func TestAccAWSInstance_importInDefaultVpcBySgName(t *testing.T) {
 	resourceName := "aws_instance.foo"
 	rInt := acctest.RandInt()
 
@@ -40,7 +40,29 @@ func TestAccAWSInstance_importInDefaultVpc(t *testing.T) {
 		CheckDestroy: testAccCheckInstanceDestroy,
 		Steps: []resource.TestStep{
 			resource.TestStep{
-				Config: testAccInstanceConfigInDefaultVpc(rInt),
+				Config: testAccInstanceConfigInDefaultVpcBySgName(rInt),
+			},
+
+			resource.TestStep{
+				ResourceName:      resourceName,
+				ImportState:       true,
+				ImportStateVerify: true,
+			},
+		},
+	})
+}
+
+func TestAccAWSInstance_importInDefaultVpcBySgId(t *testing.T) {
+	resourceName := "aws_instance.foo"
+	rInt := acctest.RandInt()
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckInstanceDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccInstanceConfigInDefaultVpcBySgId(rInt),
 			},
 
 			resource.TestStep{
@@ -76,7 +98,7 @@ func TestAccAWSInstance_importInEc2Classic(t *testing.T) {
 	})
 }
 
-func testAccInstanceConfigInDefaultVpc(rInt int) string {
+func testAccInstanceConfigInDefaultVpcBySgName(rInt int) string {
 	return fmt.Sprintf(`
 data "aws_ami" "ubuntu" {
   most_recent = true
@@ -94,9 +116,14 @@ data "aws_ami" "ubuntu" {
   owners = ["099720109477"] # Canonical
 }
 
+data "aws_vpc" "default" {
+	default = true
+}
+
 resource "aws_security_group" "sg" {
   name = "tf_acc_test_%d"
   description = "Test security group"
+	vpc_id = "${data.aws_vpc.default.id}"
 }
 
 resource "aws_instance" "foo" {
@@ -107,6 +134,42 @@ resource "aws_instance" "foo" {
 `, rInt)
 }
 
+func testAccInstanceConfigInDefaultVpcBySgId(rInt int) string {
+	return fmt.Sprintf(`
+data "aws_ami" "ubuntu" {
+  most_recent = true
+
+  filter {
+    name   = "name"
+    values = ["ubuntu/images/hvm-ssd/ubuntu-trusty-14.04-amd64-server-*"]
+  }
+
+  filter {
+    name   = "virtualization-type"
+    values = ["hvm"]
+  }
+
+  owners = ["099720109477"] # Canonical
+}
+
+data "aws_vpc" "default" {
+	default = true
+}
+
+resource "aws_security_group" "sg" {
+  name = "tf_acc_test_%d"
+  description = "Test security group"
+	vpc_id = "${data.aws_vpc.default.id}"
+}
+
+resource "aws_instance" "foo" {
+  ami             = "${data.aws_ami.ubuntu.id}"
+  instance_type   = "t2.micro"
+  vpc_security_group_ids = ["${aws_security_group.sg.id}"]
+}
+`, rInt)
+}
+
 func testAccInstanceConfigInEc2Classic(rInt int) string {
 	return fmt.Sprintf(`
 provider "aws" {

aws/resource_aws_instance.go

@@ -1462,66 +1462,57 @@ func readVolumeTags(conn *ec2.EC2, d *schema.ResourceData) error {
 
 // Determine whether we're referring to security groups with
 // IDs or names. We use a heuristic to figure this out. By default,
-// we use IDs if we're in a VPC. However, if we previously had an
-// all-name list of security groups, we use names. Or, if we had any
-// IDs, we use IDs.
+// we use IDs if we're in a VPC, and names otherwise (EC2-Classic).
+// However, the default VPC accepts either, so store them both here and let the
+// config determine which one to use in Plan and Apply.
 func readSecurityGroups(d *schema.ResourceData, instance *ec2.Instance, conn *ec2.EC2) error {
+	// An instance with a subnet is in a VPC; an instance without a subnet is in EC2-Classic.
 	hasSubnet := instance.SubnetId != nil && *instance.SubnetId != ""
-	useID := hasSubnet
+	useID, useName := hasSubnet, !hasSubnet
 
-	// We have no resource data (security_groups) during import
-	// so knowing which VPC is the instance part is useful
-	out, err := conn.DescribeVpcs(&ec2.DescribeVpcsInput{
-		VpcIds: []*string{instance.VpcId},
-	})
-	if err != nil {
-		log.Printf("[WARN] Unable to describe VPC %q: %s", *instance.VpcId, err)
-	} else if len(out.Vpcs) == 0 {
-		// This may happen in Eucalyptus Cloud
-		log.Printf("[WARN] Unable to retrieve VPCs")
-	} else {
-		isInDefaultVpc := *out.Vpcs[0].IsDefault
-		useID = !isInDefaultVpc
-	}
-
-	if v := d.Get("security_groups"); v != nil {
-		match := useID
-		sgs := v.(*schema.Set).List()
-		if len(sgs) > 0 {
-			match = false
-			for _, v := range v.(*schema.Set).List() {
-				if strings.HasPrefix(v.(string), "sg-") {
-					match = true
-					break
-				}
-			}
+	// If the instance is in a VPC, find out if that VPC is Default to determine
+	// whether to store names.
+	if instance.VpcId != nil && *instance.VpcId != "" {
+		out, err := conn.DescribeVpcs(&ec2.DescribeVpcsInput{
+			VpcIds: []*string{instance.VpcId},
+		})
+		if err != nil {
+			log.Printf("[WARN] Unable to describe VPC %q: %s", *instance.VpcId, err)
+		} else if len(out.Vpcs) == 0 {
+			// This may happen in Eucalyptus Cloud
+			log.Printf("[WARN] Unable to retrieve VPCs")
+		} else {
+			isInDefaultVpc := *out.Vpcs[0].IsDefault
+			useName = isInDefaultVpc
 		}
-
-		useID = match
 	}
 
 	// Build up the security groups
-	sgs := make([]string, 0, len(instance.SecurityGroups))
 	if useID {
+		sgs := make([]string, 0, len(instance.SecurityGroups))
 		for _, sg := range instance.SecurityGroups {
 			sgs = append(sgs, *sg.GroupId)
 		}
 		log.Printf("[DEBUG] Setting Security Group IDs: %#v", sgs)
 		if err := d.Set("vpc_security_group_ids", sgs); err != nil {
 			return err
 		}
-		if err := d.Set("security_groups", []string{}); err != nil {
+	} else {
+		if err := d.Set("vpc_security_group_ids", []string{}); err != nil {
 			return err
 		}
-	} else {
+	}
+	if useName {
+		sgs := make([]string, 0, len(instance.SecurityGroups))
 		for _, sg := range instance.SecurityGroups {
 			sgs = append(sgs, *sg.GroupName)
 		}
 		log.Printf("[DEBUG] Setting Security Group Names: %#v", sgs)
 		if err := d.Set("security_groups", sgs); err != nil {
 			return err
 		}
-		if err := d.Set("vpc_security_group_ids", []string{}); err != nil {
+	} else {
+		if err := d.Set("security_groups", []string{}); err != nil {
 			return err
 		}
 	}