From 799fe82035314bfd9fbdb7936f9bfef4470d14aa Mon Sep 17 00:00:00 2001 From: Kyrill Lebediev Date: Mon, 2 Mar 2020 22:23:52 +0200 Subject: [PATCH] resource/aws_instance: Fix incorrect behavior of volume_tags (#12225) --- aws/ec2_filters.go | 24 ------------ aws/resource_aws_ebs_volume_test.go | 29 +++++++++++++++ aws/resource_aws_instance.go | 58 +++++++++++++++++------------ aws/resource_aws_instance_test.go | 34 ++++++++++++++--- 4 files changed, 93 insertions(+), 52 deletions(-) diff --git a/aws/ec2_filters.go b/aws/ec2_filters.go index 2390125727e..b15f32b9ff5 100644 --- a/aws/ec2_filters.go +++ b/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. diff --git a/aws/resource_aws_ebs_volume_test.go b/aws/resource_aws_ebs_volume_test.go index f86b3e44ade..ef469d42f8c 100644 --- a/aws/resource_aws_ebs_volume_test.go +++ b/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" diff --git a/aws/resource_aws_instance.go b/aws/resource_aws_instance.go index e470f621b95..966ee80a5f7 100644 --- a/aws/resource_aws_instance.go +++ b/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) { diff --git a/aws/resource_aws_instance_test.go b/aws/resource_aws_instance_test.go index 96ea177645e..234792a74a3 100644 --- a/aws/resource_aws_instance_test.go +++ b/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" } }