resource/aws_instance: Fix incorrect behavior of volume_tags (#12225)

aws/ec2_filters.go

@@ -69,30 +69,6 @@ func buildEC2TagFilterList(tags []*ec2.Tag) []*ec2.Filter {
 	return filters
 }
 
-// ec2AttributeFiltersFromMultimap returns an array of EC2 Filter objects to be used when listing resources.
-//
-// The keys of the specified map are the resource attributes names used in the filter - see the documentation
-// for the relevant "Describe" action for a list of the valid names. The resource must match all the filters
-// to be included in the result.
-// The values of the specified map are lists of resource attribute values used in the filter. The resource can
-// match any of the filter values to be included in the result.
-// See https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/Using_Filtering.html#Filtering_Resources_CLI for more details.
-func ec2AttributeFiltersFromMultimap(m map[string][]string) []*ec2.Filter {
-	if len(m) == 0 {
-		return nil
-	}
-
-	filters := []*ec2.Filter{}
-	for k, v := range m {
-		filters = append(filters, &ec2.Filter{
-			Name:   aws.String(k),
-			Values: aws.StringSlice(v),
-		})
-	}
-
-	return filters
-}
-
 // ec2TagFiltersFromMap returns an array of EC2 Filter objects to be used when listing resources.
 //
 // The filters represent exact matches for all the resource tags in the given key/value map.

aws/resource_aws_ebs_volume_test.go

@@ -497,6 +497,35 @@ func testAccCheckVolumeExists(n string, v *ec2.Volume) resource.TestCheckFunc {
 	}
 }
 
+func testAccCheckVolumeTagsNotChanged(volOld, volNew *ec2.Volume) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+
+		create := make(map[string]string)
+		newTags := make(map[string]string)
+		for _, t := range volNew.Tags {
+			create[aws.StringValue(t.Key)] = aws.StringValue(t.Value)
+			newTags[aws.StringValue(t.Key)] = aws.StringValue(t.Value)
+		}
+
+		remove := make(map[string]string)
+		oldTags := make(map[string]string)
+		for _, t := range volOld.Tags {
+			oldTags[aws.StringValue(t.Key)] = aws.StringValue(t.Value)
+			old, ok := create[aws.StringValue(t.Key)]
+			if !ok || old != aws.StringValue(t.Value) {
+				remove[aws.StringValue(t.Key)] = aws.StringValue(t.Value)
+			} else if ok {
+				delete(create, aws.StringValue(t.Key))
+			}
+		}
+
+		if len(create)+len(remove) != 0 {
+			return fmt.Errorf("Volume tags changed. Old tags: %+v, new tags: %+v", oldTags, newTags)
+		}
+		return nil
+	}
+}
+
 const testAccAwsEbsVolumeConfig = `
 data "aws_availability_zones" "available" {
   state = "available"

aws/resource_aws_instance.go

@@ -887,7 +887,7 @@ func resourceAwsInstanceRead(d *schema.ResourceData, meta interface{}) error {
 		return fmt.Errorf("error setting tags: %s", err)
 	}
 
-	volumeTags, err := readVolumeTags(conn, d.Id())
+	volumeTags, err := readRootVolumeTags(instance, conn)
 	if err != nil {
 		return err
 	}
@@ -993,16 +993,16 @@ func resourceAwsInstanceUpdate(d *schema.ResourceData, meta interface{}) error {
 	}
 
 	if d.HasChange("volume_tags") && !d.IsNewResource() {
-		volumeIds, err := getAwsInstanceVolumeIds(conn, d.Id())
+		rootVolumeId, err := getAwsInstanceRootVolumeId(d, conn)
 		if err != nil {
 			return err
 		}
 
 		o, n := d.GetChange("volume_tags")
 
-		for _, volumeId := range volumeIds {
-			if err := keyvaluetags.Ec2UpdateTags(conn, volumeId, o, n); err != nil {
-				return fmt.Errorf("error updating volume_tags (%s): %s", volumeId, err)
+		if rootVolumeId != "" {
+			if err := keyvaluetags.Ec2UpdateTags(conn, rootVolumeId, o, n); err != nil {
+				return fmt.Errorf("error updating volume_tags (%s): %s", rootVolumeId, err)
 			}
 		}
 	}
@@ -2007,24 +2007,38 @@ func readBlockDeviceMappingsFromConfig(d *schema.ResourceData, conn *ec2.EC2) ([
 	return blockDevices, nil
 }
 
-func readVolumeTags(conn *ec2.EC2, instanceId string) ([]*ec2.Tag, error) {
-	volumeIds, err := getAwsInstanceVolumeIds(conn, instanceId)
-	if err != nil {
-		return nil, err
+func readRootVolumeTags(instance *ec2.Instance, conn *ec2.EC2) ([]*ec2.Tag, error) {
+	rootVolumeId := getRootVolumeId(instance)
+	if rootVolumeId == "" {
+		return nil, nil
 	}
 
 	resp, err := conn.DescribeTags(&ec2.DescribeTagsInput{
-		Filters: ec2AttributeFiltersFromMultimap(map[string][]string{
-			"resource-id": volumeIds,
+		Filters: buildEC2AttributeFilterList(map[string]string{
+			"resource-id": rootVolumeId,
 		}),
 	})
 	if err != nil {
-		return nil, fmt.Errorf("error getting tags for volumes (%s): %s", volumeIds, err)
+		return nil, fmt.Errorf("error getting tags for volume (%s): %s", rootVolumeId, err)
 	}
 
 	return ec2TagsFromTagDescriptions(resp.Tags), nil
 }
 
+func getRootVolumeId(instance *ec2.Instance) string {
+	rootVolumeId := ""
+	for _, bd := range instance.BlockDeviceMappings {
+		if bd.Ebs != nil && blockDeviceIsRoot(bd, instance) {
+			if bd.Ebs.VolumeId != nil {
+				rootVolumeId = *bd.Ebs.VolumeId
+			}
+			break
+		}
+	}
+
+	return rootVolumeId
+}
+
 // 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, and names otherwise (EC2-Classic).
@@ -2402,23 +2416,21 @@ func userDataHashSum(user_data string) string {
 	return hex.EncodeToString(hash[:])
 }
 
-func getAwsInstanceVolumeIds(conn *ec2.EC2, instanceId string) ([]string, error) {
-	volumeIds := []string{}
-
-	resp, err := conn.DescribeVolumes(&ec2.DescribeVolumesInput{
-		Filters: buildEC2AttributeFilterList(map[string]string{
-			"attachment.instance-id": instanceId,
-		}),
+func getAwsInstanceRootVolumeId(d *schema.ResourceData, conn *ec2.EC2) (string, error) {
+	resp, err := conn.DescribeInstances(&ec2.DescribeInstancesInput{
+		InstanceIds: []*string{aws.String(d.Id())},
 	})
 	if err != nil {
-		return nil, fmt.Errorf("error getting volumes for instance (%s): %s", instanceId, err)
+		return "", err
 	}
 
-	for _, v := range resp.Volumes {
-		volumeIds = append(volumeIds, aws.StringValue(v.VolumeId))
+	if len(resp.Reservations) == 0 {
+		return "", errors.New("EC2 instance not found.")
 	}
 
-	return volumeIds, nil
+	instance := resp.Reservations[0].Instances[0]
+
+	return getRootVolumeId(instance), nil
 }
 
 func getCreditSpecifications(conn *ec2.EC2, instanceId string) ([]map[string]interface{}, error) {

aws/resource_aws_instance_test.go

@@ -1123,8 +1123,10 @@ func TestAccAWSInstance_volumeTags(t *testing.T) {
 }
 
 func TestAccAWSInstance_volumeTagsComputed(t *testing.T) {
-	var v ec2.Instance
-	resourceName := "aws_instance.test"
+	var inst ec2.Instance
+	var vol1, vol2 ec2.Volume
+	instanceName := "aws_instance.test"
+	volumeName := "aws_ebs_volume.test"
 
 	resource.ParallelTest(t, resource.TestCase{
 		PreCheck:     func() { testAccPreCheck(t) },
@@ -1134,11 +1136,27 @@ func TestAccAWSInstance_volumeTagsComputed(t *testing.T) {
 			{
 				Config: testAccCheckInstanceConfigWithAttachedVolume(),
 				Check: resource.ComposeTestCheckFunc(
-					testAccCheckInstanceExists(resourceName, &v),
+					testAccCheckInstanceExists(instanceName, &inst),
+					testAccCheckVolumeExists(volumeName, &vol1),
+					resource.TestCheckResourceAttr(
+						instanceName, "volume_tags.Name", "test-terraform-instance"),
+					resource.TestCheckResourceAttr(
+						volumeName, "tags.Name", "test-terraform-volume"),
 				),
 			},
 			{
-				ResourceName:      resourceName,
+				Config: testAccCheckInstanceConfigWithAttachedVolume(),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckVolumeExists(volumeName, &vol2),
+					resource.TestCheckResourceAttr(
+						instanceName, "volume_tags.Name", "test-terraform-instance"),
+					resource.TestCheckResourceAttr(
+						volumeName, "tags.Name", "test-terraform-volume"),
+					testAccCheckVolumeTagsNotChanged(&vol1, &vol2),
+				),
+			},
+			{
+				ResourceName:      instanceName,
 				ImportState:       true,
 				ImportStateVerify: true,
 			},
@@ -3935,6 +3953,11 @@ resource "aws_instance" "test" {
   tags = {
     Name = "test-terraform"
   }
+
+  volume_tags = {
+    Name     = "test-terraform-instance"
+    instance = "test"
+  }
 }
 
 resource "aws_ebs_volume" "test" {
@@ -3943,7 +3966,8 @@ resource "aws_ebs_volume" "test" {
   type              = "gp2"
 
   tags = {
-    Name = "test-terraform"
+    Name   = "test-terraform-volume"
+    volume = "test"
   }
 }