From 4c3e2c570a4b70c0d64832d30fedc8fbce18f0a7 Mon Sep 17 00:00:00 2001 From: Brian Adams Date: Wed, 4 Oct 2023 20:48:11 -0400 Subject: [PATCH 01/16] Use default tags for ec2 root block device --- internal/service/ec2/ec2_instance.go | 138 ++++++++++++++++++++++----- 1 file changed, 114 insertions(+), 24 deletions(-) diff --git a/internal/service/ec2/ec2_instance.go b/internal/service/ec2/ec2_instance.go index 5429c1cc0e57..50ba9f6bf614 100644 --- a/internal/service/ec2/ec2_instance.go +++ b/internal/service/ec2/ec2_instance.go @@ -266,7 +266,6 @@ func ResourceInstance() *schema.Resource { Computed: true, ForceNew: true, }, - "tags": tagsSchemaConflictsWith([]string{"volume_tags"}), "throughput": { Type: schema.TypeInt, Optional: true, @@ -1026,15 +1025,56 @@ func resourceInstanceCreate(ctx context.Context, d *schema.ResourceData, meta in }) } + tagSpecifications = getTagSpecificationsIn(ctx, ec2.ResourceTypeInstance) + + // Declare the map outside the loops so values aren't overwritten + tagValues := make(map[string]interface{}) + // Iterate over each tagSpecification + for i := 0; i < len(tagSpecifications); i++ { + // Iterate over each tag within the current tagSpecification + for j := 0; j < len(tagSpecifications[i].Tags); j++ { + // Extract the key and value from the current tag + key := *tagSpecifications[i].Tags[j].Key + value := *tagSpecifications[i].Tags[j].Value + // Add the key-value pair to the map + tagValues[key] = value //value + } + } + // tags in root_block_device and ebs_block_device + // Initialize a map to store block device tags blockDeviceTagsToCreate := map[string]map[string]interface{}{} - if v, ok := d.GetOk("root_block_device"); ok { - vL := v.([]interface{}) - for _, v := range vL { - bd := v.(map[string]interface{}) - if blockDeviceTags, ok := bd["tags"].(map[string]interface{}); ok && len(blockDeviceTags) > 0 { - if rootVolumeId := getRootVolumeId(instance); rootVolumeId != "" { - blockDeviceTagsToCreate[rootVolumeId] = blockDeviceTags + + // Check if "root_block_device" data exists + if rootBlockDeviceData, ok := d.GetOk("root_block_device"); ok { + // Convert the data to a slice of interfaces + rootBlockDevices := rootBlockDeviceData.([]interface{}) + + // Iterate through each root block device + for _, rootBlockDevice := range rootBlockDevices { + // Convert the root block device data to a map + blockDeviceData := rootBlockDevice.(map[string]interface{}) + + // Check if the block device has tags and if they are non-empty + if blockDeviceTags, tagsExist := blockDeviceData["tags"].(map[string]interface{}); tagsExist && len(blockDeviceTags) > 0 { + // Calculate the root volume ID based on instance information + rootVolumeID := getRootVolumeId(instance) + for key, value := range tagValues { + blockDeviceTags[key] = value + } + // If the root volume ID is not empty, associate the tags with it + if rootVolumeID != "" { + blockDeviceTagsToCreate[rootVolumeID] = blockDeviceTags + } + } else { + // Calculate the root volume ID based on instance information + rootVolumeID := getRootVolumeId(instance) + for key, value := range tagValues { + blockDeviceTags[key] = value + } + // If the root volume ID is not empty, associate the tags with it + if rootVolumeID != "" { + blockDeviceTagsToCreate[rootVolumeID] = blockDeviceTags } } } @@ -1454,21 +1494,79 @@ func resourceInstanceRead(ctx context.Context, d *schema.ResourceData, meta inte func resourceInstanceUpdate(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { var diags diag.Diagnostics conn := meta.(*conns.AWSClient).EC2Conn(ctx) + instance, err := FindInstanceByID(ctx, conn, d.Id()) - if d.HasChange("volume_tags") && !d.IsNewResource() { - volumeIds, err := getInstanceVolumeIDs(ctx, conn, d.Id()) - if err != nil { - return sdkdiag.AppendErrorf(diags, "updating EC2 Instance (%s): %s", d.Id(), err) + if err != nil { + return sdkdiag.AppendErrorf(diags, "reading EC2 Instance (%s): %s", d.Id(), err) + } + + // if d.HasChange("root_block_device.0.tags") && !d.IsNewResource() { + tagSpecifications := getTagSpecificationsIn(ctx, ec2.ResourceTypeInstance) + + // Declare the map outside the loops so values aren't overwritten + tagValues := make(map[string]interface{}) + // Iterate over each tagSpecification + for i := 0; i < len(tagSpecifications); i++ { + // Iterate over each tag within the current tagSpecification + for j := 0; j < len(tagSpecifications[i].Tags); j++ { + // Extract the key and value from the current tag + key := *tagSpecifications[i].Tags[j].Key + value := *tagSpecifications[i].Tags[j].Value + // Add the key-value pair to the map + tagValues[key] = value //value } + } - o, n := d.GetChange("volume_tags") + // tags in root_block_device and ebs_block_device + // Initialize a map to store block device tags + blockDeviceTagsToCreate := map[string]map[string]interface{}{} - for _, volumeId := range volumeIds { - if err := updateTags(ctx, conn, volumeId, o, n); err != nil { - return sdkdiag.AppendErrorf(diags, "updating volume_tags (%s): %s", volumeId, err) + // Check if "root_block_device" data exists + if rootBlockDeviceData, ok := d.GetOk("root_block_device"); ok { + // Convert the data to a slice of interfaces + rootBlockDevices := rootBlockDeviceData.([]interface{}) + + // Iterate through each root block device + for _, rootBlockDevice := range rootBlockDevices { + // Convert the root block device data to a map + blockDeviceData := rootBlockDevice.(map[string]interface{}) + + // Check if the block device has tags and if they are non-empty + if blockDeviceTags, tagsExist := blockDeviceData["tags"].(map[string]interface{}); tagsExist && len(blockDeviceTags) > 0 { + // Calculate the root volume ID based on instance information + rootVolumeID := getRootVolumeId(instance) + for key, value := range tagValues { + blockDeviceTags[key] = value + } + // If the root volume ID is not empty, associate the tags with it + if rootVolumeID != "" { + blockDeviceTagsToCreate[rootVolumeID] = blockDeviceTags + } + } else { + // Calculate the root volume ID based on instance information + rootVolumeID := getRootVolumeId(instance) + for key, value := range tagValues { + blockDeviceTags[key] = value + } + // If the root volume ID is not empty, associate the tags with it + if rootVolumeID != "" { + blockDeviceTagsToCreate[rootVolumeID] = blockDeviceTags + } } } } + volumeIds, err := getInstanceVolumeIDs(ctx, conn, d.Id()) + if err != nil { + return sdkdiag.AppendErrorf(diags, "updating EC2 Instance (%s): %s", d.Id(), err) + } + + o, n := d.GetChange("root_block_device.0.tags") + + for _, volumeId := range volumeIds { + if err := updateTags(ctx, conn, volumeId, o, n); err != nil { + return sdkdiag.AppendErrorf(diags, "updating volume_tags (%s): %s", volumeId, err) + } + } if d.HasChange("iam_instance_profile") && !d.IsNewResource() { request := &ec2.DescribeIamInstanceProfileAssociationsInput{ @@ -1936,14 +2034,6 @@ func resourceInstanceUpdate(ctx context.Context, d *schema.ResourceData, meta in } } } - - if d.HasChange("root_block_device.0.tags") { - o, n := d.GetChange("root_block_device.0.tags") - - if err := updateTags(ctx, conn, volumeID, o, n); err != nil { - return sdkdiag.AppendErrorf(diags, "updating tags for volume (%s): %s", volumeID, err) - } - } } // To modify capacity reservation attributes of an instance, instance state needs to be in ec2.InstanceStateNameStopped, From 9f8b516fd64b730786ffebad4a241ddbf49518ef Mon Sep 17 00:00:00 2001 From: Brian Adams Date: Wed, 4 Oct 2023 20:51:41 -0400 Subject: [PATCH 02/16] Small fix for accidental line removal --- internal/service/ec2/ec2_instance.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/service/ec2/ec2_instance.go b/internal/service/ec2/ec2_instance.go index 50ba9f6bf614..211a8ff829d0 100644 --- a/internal/service/ec2/ec2_instance.go +++ b/internal/service/ec2/ec2_instance.go @@ -266,6 +266,7 @@ func ResourceInstance() *schema.Resource { Computed: true, ForceNew: true, }, + "tags": tagsSchemaConflictsWith([]string{"volume_tags"}), "throughput": { Type: schema.TypeInt, Optional: true, From b08796c872aed6418987a0e84116dd018a2f78f5 Mon Sep 17 00:00:00 2001 From: Brian Adams Date: Wed, 4 Oct 2023 21:55:44 -0400 Subject: [PATCH 03/16] Use aws.StringValue to de-reference pointer --- internal/service/ec2/ec2_instance.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/internal/service/ec2/ec2_instance.go b/internal/service/ec2/ec2_instance.go index 211a8ff829d0..2b0ac5db6d5d 100644 --- a/internal/service/ec2/ec2_instance.go +++ b/internal/service/ec2/ec2_instance.go @@ -1035,8 +1035,8 @@ func resourceInstanceCreate(ctx context.Context, d *schema.ResourceData, meta in // Iterate over each tag within the current tagSpecification for j := 0; j < len(tagSpecifications[i].Tags); j++ { // Extract the key and value from the current tag - key := *tagSpecifications[i].Tags[j].Key - value := *tagSpecifications[i].Tags[j].Value + key := aws.StringValue(tagSpecifications[i].Tags[j].Key) + value := aws.StringValue(tagSpecifications[i].Tags[j].Value) // Add the key-value pair to the map tagValues[key] = value //value } @@ -1501,7 +1501,6 @@ func resourceInstanceUpdate(ctx context.Context, d *schema.ResourceData, meta in return sdkdiag.AppendErrorf(diags, "reading EC2 Instance (%s): %s", d.Id(), err) } - // if d.HasChange("root_block_device.0.tags") && !d.IsNewResource() { tagSpecifications := getTagSpecificationsIn(ctx, ec2.ResourceTypeInstance) // Declare the map outside the loops so values aren't overwritten @@ -1511,8 +1510,8 @@ func resourceInstanceUpdate(ctx context.Context, d *schema.ResourceData, meta in // Iterate over each tag within the current tagSpecification for j := 0; j < len(tagSpecifications[i].Tags); j++ { // Extract the key and value from the current tag - key := *tagSpecifications[i].Tags[j].Key - value := *tagSpecifications[i].Tags[j].Value + key := aws.StringValue(tagSpecifications[i].Tags[j].Key) + value := aws.StringValue(tagSpecifications[i].Tags[j].Value) // Add the key-value pair to the map tagValues[key] = value //value } From 0756b1dfb6938a6bbb78761952005d1fbf2594d4 Mon Sep 17 00:00:00 2001 From: Brian Adams Date: Thu, 5 Oct 2023 09:51:17 -0400 Subject: [PATCH 04/16] Remove extra else --- internal/service/ec2/ec2_instance.go | 188 +++++++++++++-------------- 1 file changed, 87 insertions(+), 101 deletions(-) diff --git a/internal/service/ec2/ec2_instance.go b/internal/service/ec2/ec2_instance.go index 2b0ac5db6d5d..841907b1ab0b 100644 --- a/internal/service/ec2/ec2_instance.go +++ b/internal/service/ec2/ec2_instance.go @@ -1044,60 +1044,53 @@ func resourceInstanceCreate(ctx context.Context, d *schema.ResourceData, meta in // tags in root_block_device and ebs_block_device // Initialize a map to store block device tags - blockDeviceTagsToCreate := map[string]map[string]interface{}{} - - // Check if "root_block_device" data exists - if rootBlockDeviceData, ok := d.GetOk("root_block_device"); ok { - // Convert the data to a slice of interfaces - rootBlockDevices := rootBlockDeviceData.([]interface{}) - - // Iterate through each root block device - for _, rootBlockDevice := range rootBlockDevices { - // Convert the root block device data to a map - blockDeviceData := rootBlockDevice.(map[string]interface{}) - - // Check if the block device has tags and if they are non-empty - if blockDeviceTags, tagsExist := blockDeviceData["tags"].(map[string]interface{}); tagsExist && len(blockDeviceTags) > 0 { - // Calculate the root volume ID based on instance information - rootVolumeID := getRootVolumeId(instance) - for key, value := range tagValues { - blockDeviceTags[key] = value - } - // If the root volume ID is not empty, associate the tags with it - if rootVolumeID != "" { - blockDeviceTagsToCreate[rootVolumeID] = blockDeviceTags - } - } else { - // Calculate the root volume ID based on instance information - rootVolumeID := getRootVolumeId(instance) - for key, value := range tagValues { - blockDeviceTags[key] = value - } - // If the root volume ID is not empty, associate the tags with it - if rootVolumeID != "" { - blockDeviceTagsToCreate[rootVolumeID] = blockDeviceTags + if d.HasChange("root_block_device.0.tags") { + blockDeviceTagsToCreate := map[string]map[string]interface{}{} + + // Check if "root_block_device" data exists + if rootBlockDeviceData, ok := d.GetOk("root_block_device"); ok { + // Convert the data to a slice of interfaces + rootBlockDevices := rootBlockDeviceData.([]interface{}) + + // Iterate through each root block device + for _, rootBlockDevice := range rootBlockDevices { + // Convert the root block device data to a map + blockDeviceData := rootBlockDevice.(map[string]interface{}) + + // Check if the block device has tags and if they are non-empty + if blockDeviceTags, tagsExist := blockDeviceData["tags"].(map[string]interface{}); tagsExist && len(blockDeviceTags) >= 0 { + // Calculate the root volume ID based on instance information + rootVolumeID := getRootVolumeId(instance) + for key, value := range tagValues { + blockDeviceTags[key] = value + } + // If the root volume ID is not empty, associate the tags with it + if rootVolumeID != "" { + blockDeviceTagsToCreate[rootVolumeID] = blockDeviceTags + } } } } - } - if v, ok := d.GetOk("ebs_block_device"); ok { - vL := v.(*schema.Set).List() - for _, v := range vL { - bd := v.(map[string]interface{}) - if blockDeviceTags, ok := bd["tags"].(map[string]interface{}); ok && len(blockDeviceTags) > 0 { - devName := bd["device_name"].(string) - if volumeId := getVolumeIdByDeviceName(instance, devName); volumeId != "" { - blockDeviceTagsToCreate[volumeId] = blockDeviceTags + if v, ok := d.GetOk("ebs_block_device"); ok { + vL := v.(*schema.Set).List() + for _, v := range vL { + bd := v.(map[string]interface{}) + if blockDeviceTags, ok := bd["tags"].(map[string]interface{}); ok && len(blockDeviceTags) > 0 { + devName := bd["device_name"].(string) + if volumeId := getVolumeIdByDeviceName(instance, devName); volumeId != "" { + blockDeviceTagsToCreate[volumeId] = blockDeviceTags + } } } } - } - for vol, blockDeviceTags := range blockDeviceTagsToCreate { - if err := createTags(ctx, conn, vol, Tags(tftags.New(ctx, blockDeviceTags))); err != nil { - log.Printf("[ERR] Error creating tags for EBS volume %s: %s", vol, err) + for vol, blockDeviceTags := range blockDeviceTagsToCreate { + if err := createTags(ctx, conn, vol, Tags(tftags.New(ctx, blockDeviceTags))); err != nil { + log.Printf("[ERR] Error creating tags for EBS volume %s: %s", vol, err) + } } + } // Update if we need to @@ -1501,70 +1494,63 @@ func resourceInstanceUpdate(ctx context.Context, d *schema.ResourceData, meta in return sdkdiag.AppendErrorf(diags, "reading EC2 Instance (%s): %s", d.Id(), err) } - tagSpecifications := getTagSpecificationsIn(ctx, ec2.ResourceTypeInstance) + if d.HasChange("root_block_device.0.tags") { - // Declare the map outside the loops so values aren't overwritten - tagValues := make(map[string]interface{}) - // Iterate over each tagSpecification - for i := 0; i < len(tagSpecifications); i++ { - // Iterate over each tag within the current tagSpecification - for j := 0; j < len(tagSpecifications[i].Tags); j++ { - // Extract the key and value from the current tag - key := aws.StringValue(tagSpecifications[i].Tags[j].Key) - value := aws.StringValue(tagSpecifications[i].Tags[j].Value) - // Add the key-value pair to the map - tagValues[key] = value //value + tagSpecifications := getTagSpecificationsIn(ctx, ec2.ResourceTypeInstance) + + // Declare the map outside the loops so values aren't overwritten + tagValues := make(map[string]interface{}) + // Iterate over each tagSpecification + for i := 0; i < len(tagSpecifications); i++ { + // Iterate over each tag within the current tagSpecification + for j := 0; j < len(tagSpecifications[i].Tags); j++ { + // Extract the key and value from the current tag + key := aws.StringValue(tagSpecifications[i].Tags[j].Key) + value := aws.StringValue(tagSpecifications[i].Tags[j].Value) + // Add the key-value pair to the map + tagValues[key] = value //value + } } - } - // tags in root_block_device and ebs_block_device - // Initialize a map to store block device tags - blockDeviceTagsToCreate := map[string]map[string]interface{}{} - - // Check if "root_block_device" data exists - if rootBlockDeviceData, ok := d.GetOk("root_block_device"); ok { - // Convert the data to a slice of interfaces - rootBlockDevices := rootBlockDeviceData.([]interface{}) - - // Iterate through each root block device - for _, rootBlockDevice := range rootBlockDevices { - // Convert the root block device data to a map - blockDeviceData := rootBlockDevice.(map[string]interface{}) - - // Check if the block device has tags and if they are non-empty - if blockDeviceTags, tagsExist := blockDeviceData["tags"].(map[string]interface{}); tagsExist && len(blockDeviceTags) > 0 { - // Calculate the root volume ID based on instance information - rootVolumeID := getRootVolumeId(instance) - for key, value := range tagValues { - blockDeviceTags[key] = value - } - // If the root volume ID is not empty, associate the tags with it - if rootVolumeID != "" { - blockDeviceTagsToCreate[rootVolumeID] = blockDeviceTags - } - } else { - // Calculate the root volume ID based on instance information - rootVolumeID := getRootVolumeId(instance) - for key, value := range tagValues { - blockDeviceTags[key] = value - } - // If the root volume ID is not empty, associate the tags with it - if rootVolumeID != "" { - blockDeviceTagsToCreate[rootVolumeID] = blockDeviceTags + // tags in root_block_device and ebs_block_device + // Initialize a map to store block device tags + blockDeviceTagsToCreate := map[string]map[string]interface{}{} + + // Check if "root_block_device" data exists + if rootBlockDeviceData, ok := d.GetOk("root_block_device"); ok { + // Convert the data to a slice of interfaces + rootBlockDevices := rootBlockDeviceData.([]interface{}) + + // Iterate through each root block device + for _, rootBlockDevice := range rootBlockDevices { + // Convert the root block device data to a map + blockDeviceData := rootBlockDevice.(map[string]interface{}) + + // Check if the block device has tags and if they are non-empty + if blockDeviceTags, tagsExist := blockDeviceData["tags"].(map[string]interface{}); tagsExist && len(blockDeviceTags) >= 0 { + // Calculate the root volume ID based on instance information + rootVolumeID := getRootVolumeId(instance) + for key, value := range tagValues { + blockDeviceTags[key] = value + } + // If the root volume ID is not empty, associate the tags with it + if rootVolumeID != "" { + blockDeviceTagsToCreate[rootVolumeID] = blockDeviceTags + } } } } - } - volumeIds, err := getInstanceVolumeIDs(ctx, conn, d.Id()) - if err != nil { - return sdkdiag.AppendErrorf(diags, "updating EC2 Instance (%s): %s", d.Id(), err) - } + volumeIds, err := getInstanceVolumeIDs(ctx, conn, d.Id()) + if err != nil { + return sdkdiag.AppendErrorf(diags, "updating EC2 Instance (%s): %s", d.Id(), err) + } - o, n := d.GetChange("root_block_device.0.tags") + o, n := d.GetChange("root_block_device.0.tags") - for _, volumeId := range volumeIds { - if err := updateTags(ctx, conn, volumeId, o, n); err != nil { - return sdkdiag.AppendErrorf(diags, "updating volume_tags (%s): %s", volumeId, err) + for _, volumeId := range volumeIds { + if err := updateTags(ctx, conn, volumeId, o, n); err != nil { + return sdkdiag.AppendErrorf(diags, "updating volume_tags (%s): %s", volumeId, err) + } } } From b6b7449923093d7ab5b05dce24e5cdb1d3379c15 Mon Sep 17 00:00:00 2001 From: Brian Adams Date: Thu, 5 Oct 2023 12:00:20 -0400 Subject: [PATCH 05/16] Remove trailing new line --- internal/service/ec2/ec2_instance.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/internal/service/ec2/ec2_instance.go b/internal/service/ec2/ec2_instance.go index 841907b1ab0b..16222ccbda34 100644 --- a/internal/service/ec2/ec2_instance.go +++ b/internal/service/ec2/ec2_instance.go @@ -1090,9 +1090,7 @@ func resourceInstanceCreate(ctx context.Context, d *schema.ResourceData, meta in log.Printf("[ERR] Error creating tags for EBS volume %s: %s", vol, err) } } - } - // Update if we need to return append(diags, resourceInstanceUpdate(ctx, d, meta)...) } @@ -1495,7 +1493,6 @@ func resourceInstanceUpdate(ctx context.Context, d *schema.ResourceData, meta in } if d.HasChange("root_block_device.0.tags") { - tagSpecifications := getTagSpecificationsIn(ctx, ec2.ResourceTypeInstance) // Declare the map outside the loops so values aren't overwritten @@ -2664,7 +2661,6 @@ func readBlockDeviceMappingsFromConfig(ctx context.Context, d *schema.ResourceDa } } } - return blockDevices, nil } From c9769c96c8b946724ce56feb55880f6eff9e23d4 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 29 Feb 2024 14:55:29 -0500 Subject: [PATCH 06/16] ec2/instance: Rework block device tags for default tags --- internal/service/ec2/ec2_instance.go | 295 ++++++++++++--------------- 1 file changed, 133 insertions(+), 162 deletions(-) diff --git a/internal/service/ec2/ec2_instance.go b/internal/service/ec2/ec2_instance.go index 16222ccbda34..0f91196cf3d9 100644 --- a/internal/service/ec2/ec2_instance.go +++ b/internal/service/ec2/ec2_instance.go @@ -266,7 +266,8 @@ func ResourceInstance() *schema.Resource { Computed: true, ForceNew: true, }, - "tags": tagsSchemaConflictsWith([]string{"volume_tags"}), + names.AttrTags: tagsSchemaConflictsWith([]string{"volume_tags"}), + names.AttrTagsAll: tftags.TagsSchemaComputed(), "throughput": { Type: schema.TypeInt, Optional: true, @@ -720,7 +721,8 @@ func ResourceInstance() *schema.Resource { Computed: true, ForceNew: true, }, - "tags": tagsSchemaConflictsWith([]string{"volume_tags"}), + names.AttrTags: tagsSchemaConflictsWith([]string{"volume_tags"}), + names.AttrTagsAll: tftags.TagsSchemaComputed(), "throughput": { Type: schema.TypeInt, Optional: true, @@ -937,8 +939,17 @@ func resourceInstanceCreate(ctx context.Context, d *schema.ResourceData, meta in return sdkdiag.AppendErrorf(diags, "collecting instance settings: %s", err) } + // instance itself tagSpecifications := getTagSpecificationsIn(ctx, ec2.ResourceTypeInstance) - tagSpecifications = append(tagSpecifications, tagSpecificationsFromMap(ctx, d.Get("volume_tags").(map[string]interface{}), ec2.ResourceTypeVolume)...) + + // block devices + defaultTagsConfig := meta.(*conns.AWSClient).DefaultTagsConfig + tagSpecifications = append(tagSpecifications, + tagSpecificationsFromKeyValue( + ctx, + defaultTagsConfig.MergeTags(tftags.New(ctx, d.Get("volume_tags").(map[string]interface{}))), + ec2.ResourceTypeVolume)...) + input := &ec2.RunInstancesInput{ BlockDeviceMappings: instanceOpts.BlockDeviceMappings, CapacityReservationSpecification: instanceOpts.CapacityReservationSpecification, @@ -1026,71 +1037,52 @@ func resourceInstanceCreate(ctx context.Context, d *schema.ResourceData, meta in }) } - tagSpecifications = getTagSpecificationsIn(ctx, ec2.ResourceTypeInstance) + // tags in root_block_device and ebs_block_device + blockDeviceTagsToCreate := map[string]map[string]interface{}{} + if v, ok := d.GetOk("root_block_device"); ok { + vL := v.([]interface{}) + for _, v := range vL { + bd := v.(map[string]interface{}) - // Declare the map outside the loops so values aren't overwritten - tagValues := make(map[string]interface{}) - // Iterate over each tagSpecification - for i := 0; i < len(tagSpecifications); i++ { - // Iterate over each tag within the current tagSpecification - for j := 0; j < len(tagSpecifications[i].Tags); j++ { - // Extract the key and value from the current tag - key := aws.StringValue(tagSpecifications[i].Tags[j].Key) - value := aws.StringValue(tagSpecifications[i].Tags[j].Value) - // Add the key-value pair to the map - tagValues[key] = value //value + blockDeviceTags, ok := bd["tags"].(map[string]interface{}) + if !ok || len(blockDeviceTags) == 0 { + continue + } + + volID := getRootVolumeId(instance) + if volID == "" { + continue + } + + blockDeviceTagsToCreate[volID] = blockDeviceTags } } - // tags in root_block_device and ebs_block_device - // Initialize a map to store block device tags - if d.HasChange("root_block_device.0.tags") { - blockDeviceTagsToCreate := map[string]map[string]interface{}{} - - // Check if "root_block_device" data exists - if rootBlockDeviceData, ok := d.GetOk("root_block_device"); ok { - // Convert the data to a slice of interfaces - rootBlockDevices := rootBlockDeviceData.([]interface{}) - - // Iterate through each root block device - for _, rootBlockDevice := range rootBlockDevices { - // Convert the root block device data to a map - blockDeviceData := rootBlockDevice.(map[string]interface{}) - - // Check if the block device has tags and if they are non-empty - if blockDeviceTags, tagsExist := blockDeviceData["tags"].(map[string]interface{}); tagsExist && len(blockDeviceTags) >= 0 { - // Calculate the root volume ID based on instance information - rootVolumeID := getRootVolumeId(instance) - for key, value := range tagValues { - blockDeviceTags[key] = value - } - // If the root volume ID is not empty, associate the tags with it - if rootVolumeID != "" { - blockDeviceTagsToCreate[rootVolumeID] = blockDeviceTags - } - } + if v, ok := d.GetOk("ebs_block_device"); ok { + vL := v.(*schema.Set).List() + for _, v := range vL { + bd := v.(map[string]interface{}) + + blockDeviceTags, ok := bd["tags"].(map[string]interface{}) + if !ok || len(blockDeviceTags) == 0 { + continue } - } - if v, ok := d.GetOk("ebs_block_device"); ok { - vL := v.(*schema.Set).List() - for _, v := range vL { - bd := v.(map[string]interface{}) - if blockDeviceTags, ok := bd["tags"].(map[string]interface{}); ok && len(blockDeviceTags) > 0 { - devName := bd["device_name"].(string) - if volumeId := getVolumeIdByDeviceName(instance, devName); volumeId != "" { - blockDeviceTagsToCreate[volumeId] = blockDeviceTags - } - } + volumeID := getVolumeIdByDeviceName(instance, bd["device_name"].(string)) + if volumeID == "" { + continue } + + blockDeviceTagsToCreate[volumeID] = blockDeviceTags } + } - for vol, blockDeviceTags := range blockDeviceTagsToCreate { - if err := createTags(ctx, conn, vol, Tags(tftags.New(ctx, blockDeviceTags))); err != nil { - log.Printf("[ERR] Error creating tags for EBS volume %s: %s", vol, err) - } + for vol, blockDeviceTags := range blockDeviceTagsToCreate { + if err := createTags(ctx, conn, vol, Tags(tftags.New(ctx, blockDeviceTags))); err != nil { + log.Printf("[ERR] Error creating tags for EBS volume %s: %s", vol, err) } } + // Update if we need to return append(diags, resourceInstanceUpdate(ctx, d, meta)...) } @@ -1310,7 +1302,11 @@ func resourceInstanceRead(ctx context.Context, d *schema.ResourceData, meta inte return sdkdiag.AppendErrorf(diags, "reading EC2 Instance (%s): %s", d.Id(), err) } - if err := d.Set("volume_tags", KeyValueTags(ctx, volumeTags).IgnoreAWS().Map()); err != nil { + defaultTagsConfig := meta.(*conns.AWSClient).DefaultTagsConfig + ignoreTagsConfig := meta.(*conns.AWSClient).IgnoreTagsConfig + tags := KeyValueTags(ctx, volumeTags).IgnoreAWS().IgnoreConfig(ignoreTagsConfig) + + if err := d.Set("volume_tags", tags.RemoveDefaultConfig(defaultTagsConfig).Map()); err != nil { return sdkdiag.AppendErrorf(diags, "setting volume_tags: %s", err) } } @@ -1324,9 +1320,10 @@ func resourceInstanceRead(ctx context.Context, d *schema.ResourceData, meta inte return sdkdiag.AppendErrorf(diags, "reading EC2 Instance (%s): %s", d.Id(), err) } - if err := readBlockDevices(ctx, d, instance, conn); err != nil { + if err := readBlockDevices(ctx, d, meta, instance); err != nil { return sdkdiag.AppendErrorf(diags, "reading EC2 Instance (%s): %s", d.Id(), err) } + if _, ok := d.GetOk("ephemeral_block_device"); !ok { d.Set("ephemeral_block_device", []interface{}{}) } @@ -1486,67 +1483,18 @@ func resourceInstanceRead(ctx context.Context, d *schema.ResourceData, meta inte func resourceInstanceUpdate(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { var diags diag.Diagnostics conn := meta.(*conns.AWSClient).EC2Conn(ctx) - instance, err := FindInstanceByID(ctx, conn, d.Id()) - if err != nil { - return sdkdiag.AppendErrorf(diags, "reading EC2 Instance (%s): %s", d.Id(), err) - } - - if d.HasChange("root_block_device.0.tags") { - tagSpecifications := getTagSpecificationsIn(ctx, ec2.ResourceTypeInstance) - - // Declare the map outside the loops so values aren't overwritten - tagValues := make(map[string]interface{}) - // Iterate over each tagSpecification - for i := 0; i < len(tagSpecifications); i++ { - // Iterate over each tag within the current tagSpecification - for j := 0; j < len(tagSpecifications[i].Tags); j++ { - // Extract the key and value from the current tag - key := aws.StringValue(tagSpecifications[i].Tags[j].Key) - value := aws.StringValue(tagSpecifications[i].Tags[j].Value) - // Add the key-value pair to the map - tagValues[key] = value //value - } - } - - // tags in root_block_device and ebs_block_device - // Initialize a map to store block device tags - blockDeviceTagsToCreate := map[string]map[string]interface{}{} - - // Check if "root_block_device" data exists - if rootBlockDeviceData, ok := d.GetOk("root_block_device"); ok { - // Convert the data to a slice of interfaces - rootBlockDevices := rootBlockDeviceData.([]interface{}) - - // Iterate through each root block device - for _, rootBlockDevice := range rootBlockDevices { - // Convert the root block device data to a map - blockDeviceData := rootBlockDevice.(map[string]interface{}) - - // Check if the block device has tags and if they are non-empty - if blockDeviceTags, tagsExist := blockDeviceData["tags"].(map[string]interface{}); tagsExist && len(blockDeviceTags) >= 0 { - // Calculate the root volume ID based on instance information - rootVolumeID := getRootVolumeId(instance) - for key, value := range tagValues { - blockDeviceTags[key] = value - } - // If the root volume ID is not empty, associate the tags with it - if rootVolumeID != "" { - blockDeviceTagsToCreate[rootVolumeID] = blockDeviceTags - } - } - } - } + if d.HasChange("volume_tags") && !d.IsNewResource() { volumeIds, err := getInstanceVolumeIDs(ctx, conn, d.Id()) if err != nil { return sdkdiag.AppendErrorf(diags, "updating EC2 Instance (%s): %s", d.Id(), err) } - o, n := d.GetChange("root_block_device.0.tags") + o, n := d.GetChange("volume_tags") - for _, volumeId := range volumeIds { - if err := updateTags(ctx, conn, volumeId, o, n); err != nil { - return sdkdiag.AppendErrorf(diags, "updating volume_tags (%s): %s", volumeId, err) + for _, volumeID := range volumeIds { + if err := updateTags(ctx, conn, volumeID, o, n); err != nil { + return sdkdiag.AppendErrorf(diags, "updating volume_tags (%s): %s", volumeID, err) } } } @@ -2017,6 +1965,22 @@ func resourceInstanceUpdate(ctx context.Context, d *schema.ResourceData, meta in } } } + + if d.HasChange("root_block_device.0.tags") { + o, n := d.GetChange("root_block_device.0.tags") + + if err := updateTags(ctx, conn, volumeID, o, n); err != nil { + return sdkdiag.AppendErrorf(diags, "updating tags for volume (%s): %s", volumeID, err) + } + } + + if d.HasChange("root_block_device.0.tags_all") && !d.HasChange("root_block_device.0.tags") { + o, n := d.GetChange("root_block_device.0.tags_all") + + if err := updateTags(ctx, conn, volumeID, o, n); err != nil { + return sdkdiag.AppendErrorf(diags, "updating tags for volume (%s): %s", volumeID, err) + } + } } // To modify capacity reservation attributes of an instance, instance state needs to be in ec2.InstanceStateNameStopped, @@ -2198,8 +2162,8 @@ func modifyInstanceAttributeWithStopStart(ctx context.Context, conn *ec2.EC2, in return nil } -func readBlockDevices(ctx context.Context, d *schema.ResourceData, instance *ec2.Instance, conn *ec2.EC2) error { - ibds, err := readBlockDevicesFromInstance(ctx, d, instance, conn) +func readBlockDevices(ctx context.Context, d *schema.ResourceData, meta interface{}, instance *ec2.Instance) error { + ibds, err := readBlockDevicesFromInstance(ctx, d, meta, instance) if err != nil { return fmt.Errorf("reading block devices: %w", err) } @@ -2250,43 +2214,7 @@ func readBlockDevices(ctx context.Context, d *schema.ResourceData, instance *ec2 return nil } -func associateInstanceProfile(ctx context.Context, d *schema.ResourceData, conn *ec2.EC2) error { - input := &ec2.AssociateIamInstanceProfileInput{ - InstanceId: aws.String(d.Id()), - IamInstanceProfile: &ec2.IamInstanceProfileSpecification{ - Name: aws.String(d.Get("iam_instance_profile").(string)), - }, - } - err := retry.RetryContext(ctx, iamPropagationTimeout, func() *retry.RetryError { - _, err := conn.AssociateIamInstanceProfileWithContext(ctx, input) - if err != nil { - if tfawserr.ErrMessageContains(err, "InvalidParameterValue", "Invalid IAM Instance Profile") { - return retry.RetryableError(err) - } - return retry.NonRetryableError(err) - } - return nil - }) - if tfresource.TimedOut(err) { - _, err = conn.AssociateIamInstanceProfileWithContext(ctx, input) - } - if err != nil { - return fmt.Errorf("associating instance profile: %s", err) - } - return nil -} - -func disassociateInstanceProfile(ctx context.Context, associationId *string, conn *ec2.EC2) error { - _, err := conn.DisassociateIamInstanceProfileWithContext(ctx, &ec2.DisassociateIamInstanceProfileInput{ - AssociationId: associationId, - }) - if err != nil { - return fmt.Errorf("disassociating instance profile: %w", err) - } - return nil -} - -func readBlockDevicesFromInstance(ctx context.Context, d *schema.ResourceData, instance *ec2.Instance, conn *ec2.EC2) (map[string]interface{}, error) { +func readBlockDevicesFromInstance(ctx context.Context, d *schema.ResourceData, meta interface{}, instance *ec2.Instance) (map[string]interface{}, error) { blockDevices := make(map[string]interface{}) blockDevices["ebs"] = make([]map[string]interface{}, 0) blockDevices["root"] = nil @@ -2310,6 +2238,7 @@ func readBlockDevicesFromInstance(ctx context.Context, d *schema.ResourceData, i // Need to call DescribeVolumes to get volume_size and volume_type for each // EBS block device + conn := meta.(*conns.AWSClient).EC2Conn(ctx) volResp, err := conn.DescribeVolumesWithContext(ctx, &ec2.DescribeVolumesInput{ VolumeIds: volIDs, }) @@ -2317,6 +2246,9 @@ func readBlockDevicesFromInstance(ctx context.Context, d *schema.ResourceData, i return nil, err } + defaultTagsConfig := meta.(*conns.AWSClient).DefaultTagsConfig + ignoreTagsConfig := meta.(*conns.AWSClient).IgnoreTagsConfig + for _, vol := range volResp.Volumes { instanceBd := instanceBlockDevices[aws.StringValue(vol.VolumeId)] bd := make(map[string]interface{}) @@ -2347,8 +2279,10 @@ func readBlockDevicesFromInstance(ctx context.Context, d *schema.ResourceData, i if instanceBd.DeviceName != nil { bd["device_name"] = aws.StringValue(instanceBd.DeviceName) } - if v, ok := d.GetOk("volume_tags"); (!ok || v == nil || len(v.(map[string]interface{})) == 0) && vol.Tags != nil { - bd["tags"] = KeyValueTags(ctx, vol.Tags).IgnoreAWS().Map() + if v, ok := d.GetOk("volume_tags"); !ok || v == nil || len(v.(map[string]interface{})) == 0 { + tags := KeyValueTags(ctx, vol.Tags).IgnoreAWS().IgnoreConfig(ignoreTagsConfig) + bd[names.AttrTags] = tags.RemoveDefaultConfig(defaultTagsConfig).Map() + bd[names.AttrTagsAll] = tags.Map() } if blockDeviceIsRoot(instanceBd, instance) { @@ -2378,6 +2312,42 @@ func blockDeviceIsRoot(bd *ec2.InstanceBlockDeviceMapping, instance *ec2.Instanc aws.StringValue(bd.DeviceName) == aws.StringValue(instance.RootDeviceName) } +func associateInstanceProfile(ctx context.Context, d *schema.ResourceData, conn *ec2.EC2) error { + input := &ec2.AssociateIamInstanceProfileInput{ + InstanceId: aws.String(d.Id()), + IamInstanceProfile: &ec2.IamInstanceProfileSpecification{ + Name: aws.String(d.Get("iam_instance_profile").(string)), + }, + } + err := retry.RetryContext(ctx, iamPropagationTimeout, func() *retry.RetryError { + _, err := conn.AssociateIamInstanceProfileWithContext(ctx, input) + if err != nil { + if tfawserr.ErrMessageContains(err, "InvalidParameterValue", "Invalid IAM Instance Profile") { + return retry.RetryableError(err) + } + return retry.NonRetryableError(err) + } + return nil + }) + if tfresource.TimedOut(err) { + _, err = conn.AssociateIamInstanceProfileWithContext(ctx, input) + } + if err != nil { + return fmt.Errorf("associating instance profile: %s", err) + } + return nil +} + +func disassociateInstanceProfile(ctx context.Context, associationId *string, conn *ec2.EC2) error { + _, err := conn.DisassociateIamInstanceProfileWithContext(ctx, &ec2.DisassociateIamInstanceProfileInput{ + AssociationId: associationId, + }) + if err != nil { + return fmt.Errorf("disassociating instance profile: %w", err) + } + return nil +} + func FetchRootDeviceName(ctx context.Context, conn *ec2.EC2, amiID string) (*string, error) { if amiID == "" { return nil, errors.New("Cannot fetch root device name for blank AMI ID.") @@ -2661,6 +2631,7 @@ func readBlockDeviceMappingsFromConfig(ctx context.Context, d *schema.ResourceDa } } } + return blockDevices, nil } @@ -3303,31 +3274,31 @@ func getInstanceVolumeIDs(ctx context.Context, conn *ec2.EC2, instanceId string) } func getRootVolumeId(instance *ec2.Instance) string { - rootVolumeId := "" + volID := "" for _, bd := range instance.BlockDeviceMappings { if bd.Ebs != nil && blockDeviceIsRoot(bd, instance) { if bd.Ebs.VolumeId != nil { - rootVolumeId = aws.StringValue(bd.Ebs.VolumeId) + volID = aws.StringValue(bd.Ebs.VolumeId) } break } } - return rootVolumeId + return volID } func getVolumeIdByDeviceName(instance *ec2.Instance, deviceName string) string { - volumeId := "" + volumeID := "" for _, bd := range instance.BlockDeviceMappings { if aws.StringValue(bd.DeviceName) == deviceName { if bd.Ebs != nil { - volumeId = aws.StringValue(bd.Ebs.VolumeId) + volumeID = aws.StringValue(bd.Ebs.VolumeId) break } } } - return volumeId + return volumeID } func blockDeviceTagsDefined(d *schema.ResourceData) bool { @@ -3335,7 +3306,7 @@ func blockDeviceTagsDefined(d *schema.ResourceData) bool { vL := v.([]interface{}) for _, v := range vL { bd := v.(map[string]interface{}) - if blockDeviceTags, ok := bd["tags"].(map[string]interface{}); ok && len(blockDeviceTags) > 0 { + if blockDeviceTags, ok := bd[names.AttrTags].(map[string]interface{}); ok && len(blockDeviceTags) > 0 { return true } } @@ -3345,7 +3316,7 @@ func blockDeviceTagsDefined(d *schema.ResourceData) bool { vL := v.(*schema.Set).List() for _, v := range vL { bd := v.(map[string]interface{}) - if blockDeviceTags, ok := bd["tags"].(map[string]interface{}); ok && len(blockDeviceTags) > 0 { + if blockDeviceTags, ok := bd[names.AttrTags].(map[string]interface{}); ok && len(blockDeviceTags) > 0 { return true } } From 5934bf5b4ad5acbaab31bf3963120caae0b12491 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 29 Feb 2024 14:56:11 -0500 Subject: [PATCH 07/16] ec2/instance: Test reworked block device with default tagging --- internal/service/ec2/ec2_instance_test.go | 268 +++++++++++++++++++++- 1 file changed, 267 insertions(+), 1 deletion(-) diff --git a/internal/service/ec2/ec2_instance_test.go b/internal/service/ec2/ec2_instance_test.go index bc55965d4967..d88542226bd8 100644 --- a/internal/service/ec2/ec2_instance_test.go +++ b/internal/service/ec2/ec2_instance_test.go @@ -1604,6 +1604,194 @@ func TestAccEC2Instance_BlockDeviceTags_ebsAndRoot(t *testing.T) { }) } +// Random Rules of EC2 Instance tags: +// (there is certainly a better place for this but...) +// 1. FIVE types of tags can be in play: +// - instance tags +// - default tags +// - volume tags +// - root block device tags +// - ebs block device tags +// 2. Instance tags are not applied to the block devices +// 2. EBS/root device tags conflict with volume tags +// 3. Default tags are applied to the instance and all block devices +// 4. Volume tags are applied at creation time to the root block device and ebs block devices +// 5. Root block device tags behave as expected +// 6. EBS block device tags cannot be updated + +// Not all possible combinations are possible such as vol tags with ebs tags +// tags: def vol rbd ebs +// 0 0 0 0 // tested elsewhere +// 0 0 0 1 // defaultTags1:step 1 +// 0 0 1 0 // defaultTags2:step 1 +// 0 0 1 1 // defaultTags1:step 2 +// 0 1 0 0 // defaultTags2:step 2 +// 0 1 0 1 // not possible, vol conflict +// 0 1 1 0 // not possible, vol conflict +// 0 1 1 1 // not possible, vol conflict +// 1 0 0 0 // defaultTags2:step 3 +// 1 0 0 1 // defaultTags1:step 3 +// 1 0 1 0 // defaultTags2:step 4 +// 1 0 1 1 // defaultTags1:step 4 +// 1 1 0 0 // defaultTags2:step 5 +// 1 1 0 1 // not possible, vol conflict +// 1 1 1 0 // not possible, vol conflict +// 1 1 1 1 // not possible, vol conflict + +func TestAccEC2Instance_BlockDeviceTags_defaultTagsVolumeTags(t *testing.T) { + ctx := acctest.Context(t) + var v ec2.Instance + resourceName := "aws_instance.test" + //rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + + emptyMap := map[string]string{} + mapWithOneKey1 := map[string]string{"brodo": "baggins"} + mapWithOneKey2 := map[string]string{"every": "gnomes"} + mapWithTwoKeys := map[string]string{"brodo": "baggins", "jelly": "bean"} + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(ctx, t) }, + ErrorCheck: acctest.ErrorCheck(t, ec2.EndpointsID), + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + CheckDestroy: testAccCheckInstanceDestroy(ctx), + Steps: []resource.TestStep{ + { // 1 defaultTags + Config: testAccInstanceConfig_blockDeviceTagsDefaultVolumeRBDEBS(mapWithOneKey2, emptyMap, emptyMap, emptyMap), + Check: resource.ComposeTestCheckFunc( + testAccCheckInstanceExists(ctx, resourceName, &v), + resource.TestCheckResourceAttr(resourceName, "tags.%", "0"), + resource.TestCheckResourceAttr(resourceName, "tags_all.%", "1"), + resource.TestCheckResourceAttr(resourceName, "volume_tags.%", "0"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.tags.%", "0"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.tags_all.%", "1"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.tags_all.every", "gnomes"), + resource.TestCheckResourceAttr(resourceName, "ebs_block_device.0.tags.%", "0"), + resource.TestCheckResourceAttr(resourceName, "ebs_block_device.0.tags_all.%", "1"), + resource.TestCheckResourceAttr(resourceName, "ebs_block_device.0.tags_all.every", "gnomes"), + ), + }, + { // 1 defaultTags + 1 volumeTags + Config: testAccInstanceConfig_blockDeviceTagsDefaultVolumeRBDEBS(mapWithOneKey2, mapWithOneKey1, emptyMap, emptyMap), + Check: resource.ComposeTestCheckFunc( + testAccCheckInstanceExists(ctx, resourceName, &v), + resource.TestCheckResourceAttr(resourceName, "tags.%", "0"), + resource.TestCheckResourceAttr(resourceName, "tags_all.%", "1"), + resource.TestCheckResourceAttr(resourceName, "volume_tags.%", "1"), + resource.TestCheckResourceAttr(resourceName, "volume_tags.brodo", "baggins"), + ), + }, + { // 1 defaultTags + 2 volumeTags + Config: testAccInstanceConfig_blockDeviceTagsDefaultVolumeRBDEBS(mapWithOneKey2, mapWithTwoKeys, emptyMap, emptyMap), + Check: resource.ComposeTestCheckFunc( + testAccCheckInstanceExists(ctx, resourceName, &v), + resource.TestCheckResourceAttr(resourceName, "tags.%", "0"), + resource.TestCheckResourceAttr(resourceName, "tags_all.%", "1"), + resource.TestCheckResourceAttr(resourceName, "volume_tags.%", "2"), + resource.TestCheckResourceAttr(resourceName, "volume_tags.brodo", "baggins"), + resource.TestCheckResourceAttr(resourceName, "volume_tags.jelly", "bean"), + ), + }, + { // 1 defaultTags + Config: testAccInstanceConfig_blockDeviceTagsDefaultVolumeRBDEBS(mapWithOneKey2, emptyMap, emptyMap, emptyMap), + Check: resource.ComposeTestCheckFunc( + testAccCheckInstanceExists(ctx, resourceName, &v), + resource.TestCheckResourceAttr(resourceName, "tags.%", "0"), + resource.TestCheckResourceAttr(resourceName, "tags_all.%", "1"), + resource.TestCheckResourceAttr(resourceName, "volume_tags.%", "0"), + ), + }, + { // no tags + Config: testAccInstanceConfig_blockDeviceTagsDefaultVolumeRBDEBS(emptyMap, emptyMap, emptyMap, emptyMap), + Check: resource.ComposeTestCheckFunc( + testAccCheckInstanceExists(ctx, resourceName, &v), + resource.TestCheckResourceAttr(resourceName, "tags.%", "0"), + resource.TestCheckResourceAttr(resourceName, "tags_all.%", "0"), + resource.TestCheckResourceAttr(resourceName, "volume_tags.%", "0"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.tags.%", "0"), + ), + }, + }, + }) +} + +func TestAccEC2Instance_BlockDeviceTags_defaultTagsEBSRoot(t *testing.T) { + ctx := acctest.Context(t) + var v ec2.Instance + resourceName := "aws_instance.test" + //rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + + emptyMap := map[string]string{} + mapWithOneKey1 := map[string]string{"gigi": "kitty"} + mapWithOneKey2 := map[string]string{"every": "gnomes"} + mapWithTwoKeys1 := map[string]string{"brodo": "baggins", "jelly": "bean"} + mapWithTwoKeys2 := map[string]string{"brodo": "baggins", "jelly": "andrew"} + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(ctx, t) }, + ErrorCheck: acctest.ErrorCheck(t, ec2.EndpointsID), + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + CheckDestroy: testAccCheckInstanceDestroy(ctx), + Steps: []resource.TestStep{ + { // 1 defaultTags + 0 rootTags + 1 ebsTags + Config: testAccInstanceConfig_blockDeviceTagsDefaultVolumeRBDEBS(mapWithOneKey2, emptyMap, emptyMap, mapWithOneKey1), + Check: resource.ComposeTestCheckFunc( + testAccCheckInstanceExists(ctx, resourceName, &v), + resource.TestCheckResourceAttr(resourceName, "tags.%", "0"), + resource.TestCheckResourceAttr(resourceName, "tags_all.%", "1"), + resource.TestCheckResourceAttr(resourceName, "volume_tags.%", "0"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.tags.%", "0"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.tags_all.%", "1"), + resource.TestCheckResourceAttr(resourceName, "ebs_block_device.0.tags.%", "1"), + resource.TestCheckResourceAttr(resourceName, "ebs_block_device.0.tags_all.%", "2"), + resource.TestCheckResourceAttr(resourceName, "ebs_block_device.0.tags_all.gigi", "kitty"), + resource.TestCheckResourceAttr(resourceName, "ebs_block_device.0.tags_all.every", "gnomes"), + ), + }, + { // 1 defaultTags + 2 rootTags + 1 ebsTags + Config: testAccInstanceConfig_blockDeviceTagsDefaultVolumeRBDEBS(mapWithOneKey2, emptyMap, mapWithTwoKeys1, mapWithOneKey1), + Check: resource.ComposeTestCheckFunc( + testAccCheckInstanceExists(ctx, resourceName, &v), + resource.TestCheckResourceAttr(resourceName, "tags.%", "0"), + resource.TestCheckResourceAttr(resourceName, "tags_all.%", "1"), + resource.TestCheckResourceAttr(resourceName, "volume_tags.%", "0"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.tags.%", "2"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.tags_all.%", "3"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.tags_all.every", "gnomes"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.tags_all.brodo", "baggins"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.tags_all.jelly", "bean"), + ), + }, + { // 1 defaultTags + 2 rootTags (1 update) + 1 ebsTags + Config: testAccInstanceConfig_blockDeviceTagsDefaultVolumeRBDEBS(mapWithOneKey2, emptyMap, mapWithTwoKeys2, mapWithOneKey1), + Check: resource.ComposeTestCheckFunc( + testAccCheckInstanceExists(ctx, resourceName, &v), + resource.TestCheckResourceAttr(resourceName, "tags.%", "0"), + resource.TestCheckResourceAttr(resourceName, "tags_all.%", "1"), + resource.TestCheckResourceAttr(resourceName, "volume_tags.%", "0"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.tags.%", "2"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.tags_all.%", "3"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.tags_all.every", "gnomes"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.tags_all.brodo", "baggins"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.tags_all.jelly", "andrew"), + ), + }, + { // 0 defaultTags + 2 rootTags + 1 ebsTags + Config: testAccInstanceConfig_blockDeviceTagsDefaultVolumeRBDEBS(emptyMap, emptyMap, mapWithTwoKeys2, mapWithOneKey1), + Check: resource.ComposeTestCheckFunc( + testAccCheckInstanceExists(ctx, resourceName, &v), + resource.TestCheckResourceAttr(resourceName, "tags.%", "0"), + resource.TestCheckResourceAttr(resourceName, "tags_all.%", "0"), + resource.TestCheckResourceAttr(resourceName, "volume_tags.%", "0"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.tags.%", "2"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.tags_all.%", "2"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.tags_all.brodo", "baggins"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.tags_all.jelly", "andrew"), + ), + }, + }, + }) +} + func TestAccEC2Instance_instanceProfileChange(t *testing.T) { ctx := acctest.Context(t) var v ec2.Instance @@ -6851,6 +7039,84 @@ resource "aws_instance" "test" { `, rName)) } +func mapToTagConfig(m map[string]string, indent int) string { + if len(m) == 0 { + return "" + } + + var tags []string + for k, v := range m { + tags = append(tags, fmt.Sprintf("%q = %q", k, v)) + } + + return fmt.Sprintf("%s\n", strings.Join(tags, fmt.Sprintf("\n%s", strings.Repeat(" ", indent)))) +} + +func testAccInstanceConfig_blockDeviceTagsDefaultVolumeRBDEBS(defTg, volTg, rbdTg, ebsTg map[string]string) string { + defTgCfg := "" + if len(defTg) > 0 { + defTgCfg = fmt.Sprintf(` +provider "aws" { + default_tags { + tags = { + %[1]s + } + } +}`, mapToTagConfig(defTg, 6)) + } + + volTgCfg := "" + if len(volTg) > 0 { + volTgCfg = fmt.Sprintf(` + volume_tags = { + %[1]s + }`, mapToTagConfig(volTg, 4)) + } + + rbdTgCfg := "" + if len(rbdTg) > 0 { + rbdTgCfg = fmt.Sprintf(` + tags = { + %[1]s + }`, mapToTagConfig(rbdTg, 6)) + } + + ebsTgCfg := "" + if len(ebsTg) > 0 { + ebsTgCfg = fmt.Sprintf(` + tags = { + %[1]s + }`, mapToTagConfig(ebsTg, 6)) + } + + return acctest.ConfigCompose( + acctest.ConfigLatestAmazonLinux2HVMEBSX8664AMI(), + fmt.Sprintf(` +%[1]s + +resource "aws_instance" "test" { + ami = data.aws_ami.amzn2-ami-minimal-hvm-ebs-x86_64.id + + instance_type = "t2.medium" + + %[2]s + + root_block_device { + volume_type = "gp2" + + %[3]s + } + + ebs_block_device { + device_name = "/dev/sdb" + volume_size = 1 + + %[4]s + } +} +`, defTgCfg, volTgCfg, rbdTgCfg, ebsTgCfg)) +} + func testAccInstanceConfig_blockDeviceTagsEBSTags(rName string) string { return acctest.ConfigCompose(acctest.ConfigLatestAmazonLinux2HVMEBSX8664AMI(), fmt.Sprintf(` resource "aws_instance" "test" { @@ -8497,7 +8763,7 @@ resource "aws_subnet" "test" { } } -# must be >= m3 and have an encrypted root volume to eanble hibernation +# must be >= m3 and have an encrypted root volume to enable hibernation resource "aws_instance" "test" { ami = data.aws_ami.amzn2-ami-minimal-hvm-ebs-x86_64.id hibernation = %[2]t From ef2a9be0dcec1a81c243ea82068095670b1eea86 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 29 Feb 2024 14:57:25 -0500 Subject: [PATCH 08/16] ec2/instance: Change func call used by instance --- internal/service/ec2/ec2_instance_data_source.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/internal/service/ec2/ec2_instance_data_source.go b/internal/service/ec2/ec2_instance_data_source.go index 3a9b5b44fb0e..ecc11abd0430 100644 --- a/internal/service/ec2/ec2_instance_data_source.go +++ b/internal/service/ec2/ec2_instance_data_source.go @@ -400,7 +400,6 @@ func DataSourceInstance() *schema.Resource { func dataSourceInstanceRead(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { var diags diag.Diagnostics conn := meta.(*conns.AWSClient).EC2Conn(ctx) - ignoreTagsConfig := meta.(*conns.AWSClient).IgnoreTagsConfig // Build up search parameters input := &ec2.DescribeInstancesInput{} @@ -430,7 +429,7 @@ func dataSourceInstanceRead(ctx context.Context, d *schema.ResourceData, meta in } log.Printf("[DEBUG] aws_instance - Single Instance ID found: %s", aws.StringValue(instance.InstanceId)) - if err := instanceDescriptionAttributes(ctx, d, instance, conn, ignoreTagsConfig); err != nil { + if err := instanceDescriptionAttributes(ctx, d, meta, instance); err != nil { return sdkdiag.AppendErrorf(diags, "reading EC2 Instance (%s): %s", aws.StringValue(instance.InstanceId), err) } @@ -456,8 +455,9 @@ func dataSourceInstanceRead(ctx context.Context, d *schema.ResourceData, meta in } // Populate instance attribute fields with the returned instance -func instanceDescriptionAttributes(ctx context.Context, d *schema.ResourceData, instance *ec2.Instance, conn *ec2.EC2, ignoreTagsConfig *tftags.IgnoreConfig) error { +func instanceDescriptionAttributes(ctx context.Context, d *schema.ResourceData, meta interface{}, instance *ec2.Instance) error { d.SetId(aws.StringValue(instance.InstanceId)) + conn := meta.(*conns.AWSClient).EC2Conn(ctx) instanceType := aws.StringValue(instance.InstanceType) instanceTypeInfo, err := FindInstanceTypeByName(ctx, conn, instanceType) @@ -538,6 +538,7 @@ func instanceDescriptionAttributes(ctx context.Context, d *schema.ResourceData, d.Set("monitoring", monitoringState == "enabled" || monitoringState == "pending") } + ignoreTagsConfig := meta.(*conns.AWSClient).IgnoreTagsConfig if err := d.Set("tags", KeyValueTags(ctx, instance.Tags).IgnoreAWS().IgnoreConfig(ignoreTagsConfig).Map()); err != nil { return fmt.Errorf("setting tags: %w", err) } @@ -548,7 +549,7 @@ func instanceDescriptionAttributes(ctx context.Context, d *schema.ResourceData, } // Block devices - if err := readBlockDevices(ctx, d, instance, conn); err != nil { + if err := readBlockDevices(ctx, d, meta, instance); err != nil { return fmt.Errorf("reading EC2 Instance (%s): %w", aws.StringValue(instance.InstanceId), err) } if _, ok := d.GetOk("ephemeral_block_device"); !ok { From 1ff2757023bf0005d1df28e87f8cf04b1a49733f Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 29 Feb 2024 14:57:42 -0500 Subject: [PATCH 09/16] ec2/instance: Change func call used by instance --- internal/service/ec2/ec2_spot_instance_request.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/service/ec2/ec2_spot_instance_request.go b/internal/service/ec2/ec2_spot_instance_request.go index 4cd8c7519e7b..0cb13a8c1140 100644 --- a/internal/service/ec2/ec2_spot_instance_request.go +++ b/internal/service/ec2/ec2_spot_instance_request.go @@ -317,7 +317,7 @@ func readInstance(ctx context.Context, d *schema.ResourceData, meta interface{}) "host": *instance.PrivateIpAddress, }) } - if err := readBlockDevices(ctx, d, instance, conn); err != nil { + if err := readBlockDevices(ctx, d, meta, instance); err != nil { return sdkdiag.AppendFromErr(diags, err) } From d7938bfb7f81ea333c899f814b87d3b937dce323 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 29 Feb 2024 14:58:15 -0500 Subject: [PATCH 10/16] intercept: Use names constant --- internal/provider/intercept.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/provider/intercept.go b/internal/provider/intercept.go index 7a5ad365f423..677649f5090c 100644 --- a/internal/provider/intercept.go +++ b/internal/provider/intercept.go @@ -248,7 +248,7 @@ func (r tagsResourceInterceptor) run(ctx context.Context, d schemaResourceData, break } - if d.GetRawPlan().GetAttr("tags_all").IsWhollyKnown() { + if d.GetRawPlan().GetAttr(names.AttrTagsAll).IsWhollyKnown() { if d.HasChange(names.AttrTagsAll) { if identifierAttribute := r.tags.IdentifierAttribute; identifierAttribute != "" { var identifier string From 8f30044762c3590b8af16616a843fd1b22521e86 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 29 Feb 2024 14:58:42 -0500 Subject: [PATCH 11/16] ec2/tags: Add convenience func --- internal/service/ec2/tags.go | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/internal/service/ec2/tags.go b/internal/service/ec2/tags.go index c04a9297bb9d..36adb27f13fa 100644 --- a/internal/service/ec2/tags.go +++ b/internal/service/ec2/tags.go @@ -51,6 +51,20 @@ func tagSpecificationsFromMap(ctx context.Context, m map[string]interface{}, t s } } +// tagSpecificationsFromMap returns the tag specifications for the given tag key/value map and resource type. +func tagSpecificationsFromKeyValue(ctx context.Context, tags tftags.KeyValueTags, resourceType string) []*ec2.TagSpecification { + if len(tags) == 0 { + return nil + } + + return []*ec2.TagSpecification{ + { + ResourceType: aws.String(resourceType), + Tags: Tags(tags.IgnoreAWS()), + }, + } +} + // getTagSpecificationsIn returns AWS SDK for Go v1 EC2 service tags from Context. // nil is returned if there are no input tags. func getTagSpecificationsIn(ctx context.Context, resourceType string) []*ec2.TagSpecification { @@ -92,3 +106,17 @@ func tagsSchemaConflictsWith(conflictsWith []string) *schema.Schema { return v } + +// tagSpecificationsFromMap returns the tag specifications for the given tag key/value map and resource type. +func resolveDuplicate(ctx context.Context, m map[string]interface{}, t string) []*ec2.TagSpecification { + if len(m) == 0 { + return nil + } + + return []*ec2.TagSpecification{ + { + ResourceType: aws.String(t), + Tags: Tags(tftags.New(ctx, m).IgnoreAWS()), + }, + } +} From b57f53157ae637c14e47732cbeb48b06d21b24fb Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 29 Feb 2024 15:04:07 -0500 Subject: [PATCH 12/16] ec2/instance/test: Clean up --- internal/service/ec2/ec2_instance_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/service/ec2/ec2_instance_test.go b/internal/service/ec2/ec2_instance_test.go index d88542226bd8..1999e3521d3d 100644 --- a/internal/service/ec2/ec2_instance_test.go +++ b/internal/service/ec2/ec2_instance_test.go @@ -1651,7 +1651,7 @@ func TestAccEC2Instance_BlockDeviceTags_defaultTagsVolumeTags(t *testing.T) { resource.ParallelTest(t, resource.TestCase{ PreCheck: func() { acctest.PreCheck(ctx, t) }, - ErrorCheck: acctest.ErrorCheck(t, ec2.EndpointsID), + ErrorCheck: acctest.ErrorCheck(t, names.EC2ServiceID), ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, CheckDestroy: testAccCheckInstanceDestroy(ctx), Steps: []resource.TestStep{ @@ -1728,7 +1728,7 @@ func TestAccEC2Instance_BlockDeviceTags_defaultTagsEBSRoot(t *testing.T) { resource.ParallelTest(t, resource.TestCase{ PreCheck: func() { acctest.PreCheck(ctx, t) }, - ErrorCheck: acctest.ErrorCheck(t, ec2.EndpointsID), + ErrorCheck: acctest.ErrorCheck(t, names.EC2ServiceID), ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, CheckDestroy: testAccCheckInstanceDestroy(ctx), Steps: []resource.TestStep{ From 7fdef670044c066aba225f6f8b0f77d8bc134a57 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 29 Feb 2024 15:10:19 -0500 Subject: [PATCH 13/16] Add changelog --- .changelog/33769.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/33769.txt diff --git a/.changelog/33769.txt b/.changelog/33769.txt new file mode 100644 index 000000000000..e4174359c238 --- /dev/null +++ b/.changelog/33769.txt @@ -0,0 +1,3 @@ +```release-note:enhancement +resource/aws_instance: Apply default tags to volumes/block devices managed through an `aws_instance`, add `ebs_block_device.*.tags_all` and `root_block_device.*.tags_all` attributes which include default tags +``` \ No newline at end of file From 63cb5dd5d852ab56c7adbc4a88dfcb6db6a38884 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 29 Feb 2024 15:34:43 -0500 Subject: [PATCH 14/16] ec2/instance/docs: Move comment to docs --- internal/service/ec2/ec2_instance.go | 60 +++++++++++------------ internal/service/ec2/ec2_instance_test.go | 34 ------------- website/docs/r/instance.html.markdown | 14 ++++++ 3 files changed, 44 insertions(+), 64 deletions(-) diff --git a/internal/service/ec2/ec2_instance.go b/internal/service/ec2/ec2_instance.go index 0f91196cf3d9..120b7c316cd1 100644 --- a/internal/service/ec2/ec2_instance.go +++ b/internal/service/ec2/ec2_instance.go @@ -1049,7 +1049,7 @@ func resourceInstanceCreate(ctx context.Context, d *schema.ResourceData, meta in continue } - volID := getRootVolumeId(instance) + volID := getRootVolID(instance) if volID == "" { continue } @@ -1068,12 +1068,12 @@ func resourceInstanceCreate(ctx context.Context, d *schema.ResourceData, meta in continue } - volumeID := getVolumeIdByDeviceName(instance, bd["device_name"].(string)) - if volumeID == "" { + volID := getVolIDByDeviceName(instance, bd["device_name"].(string)) + if volID == "" { continue } - blockDeviceTagsToCreate[volumeID] = blockDeviceTags + blockDeviceTagsToCreate[volID] = blockDeviceTags } } @@ -1485,16 +1485,16 @@ func resourceInstanceUpdate(ctx context.Context, d *schema.ResourceData, meta in conn := meta.(*conns.AWSClient).EC2Conn(ctx) if d.HasChange("volume_tags") && !d.IsNewResource() { - volumeIds, err := getInstanceVolumeIDs(ctx, conn, d.Id()) + volIDs, err := getInstanceVolIDs(ctx, conn, d.Id()) if err != nil { return sdkdiag.AppendErrorf(diags, "updating EC2 Instance (%s): %s", d.Id(), err) } o, n := d.GetChange("volume_tags") - for _, volumeID := range volumeIds { - if err := updateTags(ctx, conn, volumeID, o, n); err != nil { - return sdkdiag.AppendErrorf(diags, "updating volume_tags (%s): %s", volumeID, err) + for _, volID := range volIDs { + if err := updateTags(ctx, conn, volID, o, n); err != nil { + return sdkdiag.AppendErrorf(diags, "updating volume_tags (%s): %s", volID, err) } } } @@ -1882,10 +1882,10 @@ func resourceInstanceUpdate(ctx context.Context, d *schema.ResourceData, meta in } if d.HasChange("root_block_device.0") && !d.IsNewResource() { - volumeID := d.Get("root_block_device.0.volume_id").(string) + volID := d.Get("root_block_device.0.volume_id").(string) input := &ec2.ModifyVolumeInput{ - VolumeId: aws.String(volumeID), + VolumeId: aws.String(volID), } modifyVolume := false @@ -1931,11 +1931,11 @@ func resourceInstanceUpdate(ctx context.Context, d *schema.ResourceData, meta in _, err := conn.ModifyVolumeWithContext(ctx, input) if err != nil { - return sdkdiag.AppendErrorf(diags, "updating EC2 Instance (%s) volume (%s): %s", d.Id(), volumeID, err) + return sdkdiag.AppendErrorf(diags, "updating EC2 Instance (%s) volume (%s): %s", d.Id(), volID, err) } - if _, err := WaitVolumeModificationComplete(ctx, conn, volumeID, d.Timeout(schema.TimeoutUpdate)); err != nil { - return sdkdiag.AppendErrorf(diags, "waiting for EC2 Instance (%s) volume (%s) update: %s", d.Id(), volumeID, err) + if _, err := WaitVolumeModificationComplete(ctx, conn, volID, d.Timeout(schema.TimeoutUpdate)); err != nil { + return sdkdiag.AppendErrorf(diags, "waiting for EC2 Instance (%s) volume (%s) update: %s", d.Id(), volID, err) } } @@ -1969,16 +1969,16 @@ func resourceInstanceUpdate(ctx context.Context, d *schema.ResourceData, meta in if d.HasChange("root_block_device.0.tags") { o, n := d.GetChange("root_block_device.0.tags") - if err := updateTags(ctx, conn, volumeID, o, n); err != nil { - return sdkdiag.AppendErrorf(diags, "updating tags for volume (%s): %s", volumeID, err) + if err := updateTags(ctx, conn, volID, o, n); err != nil { + return sdkdiag.AppendErrorf(diags, "updating tags for volume (%s): %s", volID, err) } } if d.HasChange("root_block_device.0.tags_all") && !d.HasChange("root_block_device.0.tags") { o, n := d.GetChange("root_block_device.0.tags_all") - if err := updateTags(ctx, conn, volumeID, o, n); err != nil { - return sdkdiag.AppendErrorf(diags, "updating tags for volume (%s): %s", volumeID, err) + if err := updateTags(ctx, conn, volID, o, n); err != nil { + return sdkdiag.AppendErrorf(diags, "updating tags for volume (%s): %s", volID, err) } } } @@ -2636,18 +2636,18 @@ func readBlockDeviceMappingsFromConfig(ctx context.Context, d *schema.ResourceDa } func readVolumeTags(ctx context.Context, conn *ec2.EC2, instanceId string) ([]*ec2.Tag, error) { - volumeIds, err := getInstanceVolumeIDs(ctx, conn, instanceId) + volIDs, err := getInstanceVolIDs(ctx, conn, instanceId) if err != nil { - return nil, fmt.Errorf("getting tags for volumes (%s): %s", volumeIds, err) + return nil, fmt.Errorf("getting tags for volumes (%s): %s", volIDs, err) } resp, err := conn.DescribeTagsWithContext(ctx, &ec2.DescribeTagsInput{ Filters: attributeFiltersFromMultimap(map[string][]string{ - "resource-id": volumeIds, + "resource-id": volIDs, }), }) if err != nil { - return nil, fmt.Errorf("getting tags for volumes (%s): %s", volumeIds, err) + return nil, fmt.Errorf("getting tags for volumes (%s): %s", volIDs, err) } return tagsFromTagDescriptions(resp.Tags), nil @@ -3254,8 +3254,8 @@ func userDataHashSum(user_data string) string { return hex.EncodeToString(hash[:]) } -func getInstanceVolumeIDs(ctx context.Context, conn *ec2.EC2, instanceId string) ([]string, error) { - volumeIds := []string{} +func getInstanceVolIDs(ctx context.Context, conn *ec2.EC2, instanceId string) ([]string, error) { + volIDs := []string{} resp, err := conn.DescribeVolumesWithContext(ctx, &ec2.DescribeVolumesInput{ Filters: BuildAttributeFilterList(map[string]string{ @@ -3267,13 +3267,13 @@ func getInstanceVolumeIDs(ctx context.Context, conn *ec2.EC2, instanceId string) } for _, v := range resp.Volumes { - volumeIds = append(volumeIds, aws.StringValue(v.VolumeId)) + volIDs = append(volIDs, aws.StringValue(v.VolumeId)) } - return volumeIds, nil + return volIDs, nil } -func getRootVolumeId(instance *ec2.Instance) string { +func getRootVolID(instance *ec2.Instance) string { volID := "" for _, bd := range instance.BlockDeviceMappings { if bd.Ebs != nil && blockDeviceIsRoot(bd, instance) { @@ -3287,18 +3287,18 @@ func getRootVolumeId(instance *ec2.Instance) string { return volID } -func getVolumeIdByDeviceName(instance *ec2.Instance, deviceName string) string { - volumeID := "" +func getVolIDByDeviceName(instance *ec2.Instance, deviceName string) string { + volID := "" for _, bd := range instance.BlockDeviceMappings { if aws.StringValue(bd.DeviceName) == deviceName { if bd.Ebs != nil { - volumeID = aws.StringValue(bd.Ebs.VolumeId) + volID = aws.StringValue(bd.Ebs.VolumeId) break } } } - return volumeID + return volID } func blockDeviceTagsDefined(d *schema.ResourceData) bool { diff --git a/internal/service/ec2/ec2_instance_test.go b/internal/service/ec2/ec2_instance_test.go index 1999e3521d3d..f2938f58dfb0 100644 --- a/internal/service/ec2/ec2_instance_test.go +++ b/internal/service/ec2/ec2_instance_test.go @@ -1604,40 +1604,6 @@ func TestAccEC2Instance_BlockDeviceTags_ebsAndRoot(t *testing.T) { }) } -// Random Rules of EC2 Instance tags: -// (there is certainly a better place for this but...) -// 1. FIVE types of tags can be in play: -// - instance tags -// - default tags -// - volume tags -// - root block device tags -// - ebs block device tags -// 2. Instance tags are not applied to the block devices -// 2. EBS/root device tags conflict with volume tags -// 3. Default tags are applied to the instance and all block devices -// 4. Volume tags are applied at creation time to the root block device and ebs block devices -// 5. Root block device tags behave as expected -// 6. EBS block device tags cannot be updated - -// Not all possible combinations are possible such as vol tags with ebs tags -// tags: def vol rbd ebs -// 0 0 0 0 // tested elsewhere -// 0 0 0 1 // defaultTags1:step 1 -// 0 0 1 0 // defaultTags2:step 1 -// 0 0 1 1 // defaultTags1:step 2 -// 0 1 0 0 // defaultTags2:step 2 -// 0 1 0 1 // not possible, vol conflict -// 0 1 1 0 // not possible, vol conflict -// 0 1 1 1 // not possible, vol conflict -// 1 0 0 0 // defaultTags2:step 3 -// 1 0 0 1 // defaultTags1:step 3 -// 1 0 1 0 // defaultTags2:step 4 -// 1 0 1 1 // defaultTags1:step 4 -// 1 1 0 0 // defaultTags2:step 5 -// 1 1 0 1 // not possible, vol conflict -// 1 1 1 0 // not possible, vol conflict -// 1 1 1 1 // not possible, vol conflict - func TestAccEC2Instance_BlockDeviceTags_defaultTagsVolumeTags(t *testing.T) { ctx := acctest.Context(t) var v ec2.Instance diff --git a/website/docs/r/instance.html.markdown b/website/docs/r/instance.html.markdown index e51894b20c10..e9f0af9d00b5 100644 --- a/website/docs/r/instance.html.markdown +++ b/website/docs/r/instance.html.markdown @@ -178,6 +178,18 @@ resource "aws_instance" "this" { } ``` +## Tag Guide + +Here's a breakdown of the five types of tags you might encounter relative to an `aws_instance`: + +1. **Instance tags**: Applied to instances but not to `ebs_block_device` and `root_block_device` volumes. +2. **Default tags**: Applied to the instance and to `ebs_block_device` and `root_block_device` volumes. +3. **Volume tags**: Applied during creation to `ebs_block_device` and `root_block_device` volumes. +4. **Root block device tags**: Applied only to the `root_block_device` volume. These conflict with `volume_tags`. +5. **EBS block device tags**: Applied only to the specific `ebs_block_device` volume you configure them for and cannot be updated. These conflict with `volume_tags`. + +Do not use `volume_tags` if you plan to manage block device tags outside the `aws_instance` configuration, such as using `tags` in an [`aws_ebs_volume`](/docs/providers/aws/r/ebs_volume.html) resource attached via [`aws_volume_attachment`](/docs/providers/aws/r/volume_attachment.html). Doing so will result in resource cycling and inconsistent behavior. + ## Argument Reference This resource supports the following arguments: @@ -424,11 +436,13 @@ This resource exports the following attributes in addition to the arguments abov For `ebs_block_device`, in addition to the arguments above, the following attribute is exported: * `volume_id` - ID of the volume. For example, the ID can be accessed like this, `aws_instance.web.ebs_block_device.2.volume_id`. +* `tags_all` - Map of tags assigned to the resource, including those inherited from the provider [`default_tags` configuration block](https://registry.terraform.io/providers/hashicorp/aws/latest/docs#default_tags-configuration-block). For `root_block_device`, in addition to the arguments above, the following attributes are exported: * `volume_id` - ID of the volume. For example, the ID can be accessed like this, `aws_instance.web.root_block_device.0.volume_id`. * `device_name` - Device name, e.g., `/dev/sdh` or `xvdh`. +* `tags_all` - Map of tags assigned to the resource, including those inherited from the provider [`default_tags` configuration block](https://registry.terraform.io/providers/hashicorp/aws/latest/docs#default_tags-configuration-block). For `instance_market_options`, in addition to the arguments above, the following attributes are exported: From 57c6b5386e3535523a49f8ddbc93a21b1a450512 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 29 Feb 2024 15:46:52 -0500 Subject: [PATCH 15/16] ec2/instance: Remove unused, fix docs --- docs/resource-tagging.md | 20 ++++++++++---------- internal/service/ec2/ec2_instance.go | 1 - internal/service/ec2/ec2_instance_test.go | 1 + internal/service/ec2/tags.go | 18 ++---------------- 4 files changed, 13 insertions(+), 27 deletions(-) diff --git a/docs/resource-tagging.md b/docs/resource-tagging.md index 1f68ad52e1c6..eebca35f01e5 100644 --- a/docs/resource-tagging.md +++ b/docs/resource-tagging.md @@ -335,7 +335,7 @@ implement the logic to convert the configuration tags into the service tags, e.g === "Terraform Plugin SDK V2" ```go // Typically declared near conn := /*...*/ - defaultTagsConfig := meta.(*AWSClient).DefaultTagsConfig + defaultTagsConfig := meta.(*conns.AWSClient).DefaultTagsConfig tags := defaultTagsConfig.MergeTags(tftags.New(ctx, d.Get("tags").(map[string]interface{}))) input := &eks.CreateClusterInput{ @@ -349,7 +349,7 @@ If the service API does not allow passing an empty list, the logic can be adjust === "Terraform Plugin SDK V2" ```go // Typically declared near conn := /*...*/ - defaultTagsConfig := meta.(*AWSClient).DefaultTagsConfig + defaultTagsConfig := meta.(*conns.AWSClient).DefaultTagsConfig tags := defaultTagsConfig.MergeTags(tftags.New(ctx, d.Get("tags").(map[string]interface{}))) input := &eks.CreateClusterInput{ @@ -367,7 +367,7 @@ implement the logic to convert the configuration tags into the service API call === "Terraform Plugin SDK V2" ```go // Typically declared near conn := /*...*/ - defaultTagsConfig := meta.(*AWSClient).DefaultTagsConfig + defaultTagsConfig := meta.(*conns.AWSClient).DefaultTagsConfig tags := defaultTagsConfig.MergeTags(tftags.New(ctx, d.Get("tags").(map[string]interface{}))) /* ... creation steps ... */ @@ -380,18 +380,18 @@ implement the logic to convert the configuration tags into the service API call ``` Some EC2 resources (e.g., [`aws_ec2_fleet`](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/ec2_fleet)) have a `TagSpecifications` field in the `InputStruct` instead of a `Tags` field. -In these cases the `tagSpecificationsFromKeyValueTags()` helper function should be used. +In these cases the `tagSpecificationsFromKeyValue()` helper function should be used. This example shows using `TagSpecifications`: === "Terraform Plugin SDK V2" ```go // Typically declared near conn := /*...*/ - defaultTagsConfig := meta.(*AWSClient).DefaultTagsConfig + defaultTagsConfig := meta.(*conns.AWSClient).DefaultTagsConfig tags := defaultTagsConfig.MergeTags(tftags.New(ctx, d.Get("tags").(map[string]interface{}))) input := &ec2.CreateFleetInput{ /* ... other configuration ... */ - TagSpecifications: tagSpecificationsFromKeyValueTags(tags, ec2.ResourceTypeFleet), + TagSpecifications: tagSpecificationsFromKeyValue(tags, ec2.ResourceTypeFleet), } ``` @@ -402,8 +402,8 @@ In the resource `Read` operation, implement the logic to convert the service tag === "Terraform Plugin SDK V2" ```go // Typically declared near conn := /*...*/ - defaultTagsConfig := meta.(*AWSClient).DefaultTagsConfig - ignoreTagsConfig := meta.(*AWSClient).IgnoreTagsConfig + defaultTagsConfig := meta.(*conns.AWSClient).DefaultTagsConfig + ignoreTagsConfig := meta.(*conns.AWSClient).IgnoreTagsConfig /* ... other d.Set(...) logic ... */ @@ -424,8 +424,8 @@ use the generated `listTags` function, e.g., with Athena Workgroups: === "Terraform Plugin SDK V2" ```go // Typically declared near conn := /*...*/ - defaultTagsConfig := meta.(*AWSClient).DefaultTagsConfig - ignoreTagsConfig := meta.(*AWSClient).IgnoreTagsConfig + defaultTagsConfig := meta.(*conns.AWSClient).DefaultTagsConfig + ignoreTagsConfig := meta.(*conns.AWSClient).IgnoreTagsConfig /* ... other d.Set(...) logic ... */ diff --git a/internal/service/ec2/ec2_instance.go b/internal/service/ec2/ec2_instance.go index 120b7c316cd1..cd9db6f5f8a2 100644 --- a/internal/service/ec2/ec2_instance.go +++ b/internal/service/ec2/ec2_instance.go @@ -946,7 +946,6 @@ func resourceInstanceCreate(ctx context.Context, d *schema.ResourceData, meta in defaultTagsConfig := meta.(*conns.AWSClient).DefaultTagsConfig tagSpecifications = append(tagSpecifications, tagSpecificationsFromKeyValue( - ctx, defaultTagsConfig.MergeTags(tftags.New(ctx, d.Get("volume_tags").(map[string]interface{}))), ec2.ResourceTypeVolume)...) diff --git a/internal/service/ec2/ec2_instance_test.go b/internal/service/ec2/ec2_instance_test.go index f2938f58dfb0..955a6af27bda 100644 --- a/internal/service/ec2/ec2_instance_test.go +++ b/internal/service/ec2/ec2_instance_test.go @@ -7021,6 +7021,7 @@ func mapToTagConfig(m map[string]string, indent int) string { func testAccInstanceConfig_blockDeviceTagsDefaultVolumeRBDEBS(defTg, volTg, rbdTg, ebsTg map[string]string) string { defTgCfg := "" if len(defTg) > 0 { + //lintignore:AT004 defTgCfg = fmt.Sprintf(` provider "aws" { default_tags { diff --git a/internal/service/ec2/tags.go b/internal/service/ec2/tags.go index 36adb27f13fa..b4766af46db0 100644 --- a/internal/service/ec2/tags.go +++ b/internal/service/ec2/tags.go @@ -51,8 +51,8 @@ func tagSpecificationsFromMap(ctx context.Context, m map[string]interface{}, t s } } -// tagSpecificationsFromMap returns the tag specifications for the given tag key/value map and resource type. -func tagSpecificationsFromKeyValue(ctx context.Context, tags tftags.KeyValueTags, resourceType string) []*ec2.TagSpecification { +// tagSpecificationsFromKeyValue returns the tag specifications for the given tag key/value tags and resource type. +func tagSpecificationsFromKeyValue(tags tftags.KeyValueTags, resourceType string) []*ec2.TagSpecification { if len(tags) == 0 { return nil } @@ -106,17 +106,3 @@ func tagsSchemaConflictsWith(conflictsWith []string) *schema.Schema { return v } - -// tagSpecificationsFromMap returns the tag specifications for the given tag key/value map and resource type. -func resolveDuplicate(ctx context.Context, m map[string]interface{}, t string) []*ec2.TagSpecification { - if len(m) == 0 { - return nil - } - - return []*ec2.TagSpecification{ - { - ResourceType: aws.String(t), - Tags: Tags(tftags.New(ctx, m).IgnoreAWS()), - }, - } -} From 0716ee33677babeeb0aef6e205182a9d60754406 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 29 Feb 2024 15:55:52 -0500 Subject: [PATCH 16/16] ec2/instance: Small fixes --- internal/service/ec2/ec2_instance_test.go | 2 -- website/docs/r/instance.html.markdown | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/internal/service/ec2/ec2_instance_test.go b/internal/service/ec2/ec2_instance_test.go index 955a6af27bda..0cc26d69b3b0 100644 --- a/internal/service/ec2/ec2_instance_test.go +++ b/internal/service/ec2/ec2_instance_test.go @@ -1608,7 +1608,6 @@ func TestAccEC2Instance_BlockDeviceTags_defaultTagsVolumeTags(t *testing.T) { ctx := acctest.Context(t) var v ec2.Instance resourceName := "aws_instance.test" - //rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) emptyMap := map[string]string{} mapWithOneKey1 := map[string]string{"brodo": "baggins"} @@ -1684,7 +1683,6 @@ func TestAccEC2Instance_BlockDeviceTags_defaultTagsEBSRoot(t *testing.T) { ctx := acctest.Context(t) var v ec2.Instance resourceName := "aws_instance.test" - //rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) emptyMap := map[string]string{} mapWithOneKey1 := map[string]string{"gigi": "kitty"} diff --git a/website/docs/r/instance.html.markdown b/website/docs/r/instance.html.markdown index e9f0af9d00b5..eaa0a704050c 100644 --- a/website/docs/r/instance.html.markdown +++ b/website/docs/r/instance.html.markdown @@ -180,7 +180,7 @@ resource "aws_instance" "this" { ## Tag Guide -Here's a breakdown of the five types of tags you might encounter relative to an `aws_instance`: +These are the five types of tags you might encounter relative to an `aws_instance`: 1. **Instance tags**: Applied to instances but not to `ebs_block_device` and `root_block_device` volumes. 2. **Default tags**: Applied to the instance and to `ebs_block_device` and `root_block_device` volumes.