resource/aws_instance: Add `tags` parameter to `root_block_device`, `ebs_block_device` blocks (#12225) #15474

klebediev · 2020-10-04T08:31:43Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #12225, closes #17074, closes #5878, closes #5609, closes #5080, closes #770, closes #729, closes #884, closes #9156, closes #12226, closes #9033
Similar PR #12226 #9156

Release note for CHANGELOG:

 - resource/aws_instance: Add `tags` parameter to `root_block_device`, `ebs_block_device` blocks.

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSInstance_.*olumeTags'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSInstance_.*olumeTags -timeout 120m
=== RUN   TestAccAWSInstance_volumeTags
=== PAUSE TestAccAWSInstance_volumeTags
=== RUN   TestAccAWSInstance_rootVolumeTags
=== PAUSE TestAccAWSInstance_rootVolumeTags
=== RUN   TestAccAWSInstance_volumeTagsComputed
=== PAUSE TestAccAWSInstance_volumeTagsComputed
=== RUN   TestAccAWSInstance_rootVolumeTagsEbsAttached
=== PAUSE TestAccAWSInstance_rootVolumeTagsEbsAttached
=== CONT  TestAccAWSInstance_volumeTags
=== CONT  TestAccAWSInstance_volumeTagsComputed
=== CONT  TestAccAWSInstance_rootVolumeTagsEbsAttached
=== CONT  TestAccAWSInstance_rootVolumeTags
--- PASS: TestAccAWSInstance_volumeTagsComputed (173.91s)
--- PASS: TestAccAWSInstance_rootVolumeTagsEbsAttached (219.93s)
--- PASS: TestAccAWSInstance_rootVolumeTags (279.90s)
--- PASS: TestAccAWSInstance_volumeTags (289.16s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	291.061s

aws/resource_aws_instance.go

YakDriver · 2020-10-07T15:19:27Z

@klebediev I apologize if I was unclear. Please review #12225 (comment).

Rather than root_volume_tags, the ebs_block_device and root_block_device blocks should have tags. The schema would look something like this:

			"ebs_block_device": {
				Type:     schema.TypeSet,
				Optional: true,
				Computed: true,
				Elem: &schema.Resource{
					Schema: map[string]*schema.Schema{
						// other existing code...

						"snapshot_id": {
							Type:     schema.TypeString,
							Optional: true,
							Computed: true,
							ForceNew: true,
						},

						"tags": tagsSchema(),

						"volume_size": {
							Type:     schema.TypeInt,
							Optional: true,
							Computed: true,
							ForceNew: true,
						},

						// other existing code...
					},
				},
				// other existing code...
			},

			// "ephemeral_block_device"... can these be tagged? may need tags here too

			"root_block_device": {
				Type:     schema.TypeList,
				Optional: true,
				Computed: true,
				MaxItems: 1,
				Elem: &schema.Resource{
					// "You can only modify the volume size, volume type, and Delete on
					// Termination flag on the block device mapping entry for the root
					// device volume." - bit.ly/ec2bdmap
					Schema: map[string]*schema.Schema{
						// other existing code...
						"iops": {
							Type:             schema.TypeInt,
							Optional:         true,
							Computed:         true,
							DiffSuppressFunc: iopsDiffSuppressFunc,
						},

						"tags": tagsSchema(),

						"volume_size": {
							Type:     schema.TypeInt,
							Optional: true,
							Computed: true,
						},
						// other existing code...
					},
				},
			},

Hopefully, this example makes it more clear! Let me know if you have questions.

YakDriver

Instead of root_volume_tags, please add tags within the ebs_block_device and root_block_device blocks.

klebediev · 2020-10-07T15:54:14Z

ohh.. my fault, you were clear, but for some reason I was reading wrong comment (right above that you referred to)
okay, I'll be able to start working on this in several days

YakDriver · 2020-10-07T18:33:33Z

@klebediev Thanks for your work on this! We look forward to your update. If anything changes on your end with your schedule, let us know!

klebediev · 2020-10-18T05:23:26Z

$ TF_ACC=1 go test ./aws -v -count 1 -parallel 8 -run="TestAccAWSInstance_.*olumeTags" -timeout 120m
=== RUN   TestAccAWSInstance_volumeTags
=== PAUSE TestAccAWSInstance_volumeTags
=== RUN   TestAccAWSInstance_volumeTagsWithAttachedVolume
=== PAUSE TestAccAWSInstance_volumeTagsWithAttachedVolume
=== RUN   TestAccAWSInstance_blockDeviceVolumeTags
=== PAUSE TestAccAWSInstance_blockDeviceVolumeTags
=== CONT  TestAccAWSInstance_volumeTags
=== CONT  TestAccAWSInstance_blockDeviceVolumeTags
=== CONT  TestAccAWSInstance_volumeTagsWithAttachedVolume
--- PASS: TestAccAWSInstance_volumeTagsWithAttachedVolume (195.39s)
--- PASS: TestAccAWSInstance_blockDeviceVolumeTags (219.62s)
--- PASS: TestAccAWSInstance_volumeTags (296.43s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	299.135s

klebediev · 2020-10-18T07:15:17Z

Can't say I like how it's implemented (it's a bit tricky to deal with 2 or more parameters controlling the same functionality), but at least it works in all cases except importing when volume_tags set: in this case terraform apply is required after terraform import.
So far it's the best behavior I was able to achieve.
Kind of micro-breaking change I had to introduce: volume_tags switched to Computed: false

@YakDriver might you take a quick look at this and advise if this could be implemented better

YakDriver

--- PASS: TestAccAWSInstance_addSecondaryInterface (181.43s)
--- PASS: TestAccAWSInstance_addSecurityGroupNetworkInterface (190.56s)
--- PASS: TestAccAWSInstance_associatePublic_defaultPrivate (89.09s)
--- PASS: TestAccAWSInstance_associatePublic_defaultPublic (224.23s)
--- PASS: TestAccAWSInstance_associatePublic_explicitPrivate (100.27s)
--- PASS: TestAccAWSInstance_associatePublic_explicitPublic (214.61s)
--- PASS: TestAccAWSInstance_associatePublic_overridePrivate (99.56s)
--- PASS: TestAccAWSInstance_associatePublic_overridePublic (109.78s)
--- PASS: TestAccAWSInstance_associatePublicIPAndPrivateIP (114.88s)
--- PASS: TestAccAWSInstance_atLeastOneOtherEbsVolume (164.65s)
--- PASS: TestAccAWSInstance_basic (91.43s)
--- PASS: TestAccAWSInstance_blockDevices (93.37s)
--- PASS: TestAccAWSInstance_blockDeviceTags_ebsAndRoot (127.19s)
--- PASS: TestAccAWSInstance_blockDeviceTags_volumeTags (179.42s)
--- PASS: TestAccAWSInstance_blockDeviceTags_withAttachedVolume (173.43s)
--- PASS: TestAccAWSInstance_changeInstanceType (170.63s)
--- PASS: TestAccAWSInstance_CreditSpecification_Empty_NonBurstable (315.86s)
--- PASS: TestAccAWSInstance_creditSpecification_isNotAppliedToNonBurstable (114.38s)
--- PASS: TestAccAWSInstance_creditSpecification_standardCpuCredits (144.36s)
--- PASS: TestAccAWSInstance_creditSpecification_standardCpuCredits_t2Tot3Taint (454.12s)
--- PASS: TestAccAWSInstance_creditSpecification_unknownCpuCredits_t2 (110.36s)
--- PASS: TestAccAWSInstance_creditSpecification_unknownCpuCredits_t3 (338.63s)
--- PASS: TestAccAWSInstance_creditSpecification_unlimitedCpuCredits (138.75s)
--- PASS: TestAccAWSInstance_creditSpecification_unlimitedCpuCredits_t2Tot3Taint (225.89s)
--- PASS: TestAccAWSInstance_creditSpecification_unspecifiedDefaultsToStandard (122.18s)
--- PASS: TestAccAWSInstance_CreditSpecification_UnspecifiedToEmpty_NonBurstable (136.87s)
--- PASS: TestAccAWSInstance_creditSpecification_updateCpuCredits (176.97s)
--- PASS: TestAccAWSInstance_creditSpecificationT3_standardCpuCredits (122.82s)
--- PASS: TestAccAWSInstance_creditSpecificationT3_unlimitedCpuCredits (144.93s)
--- PASS: TestAccAWSInstance_creditSpecificationT3_unspecifiedDefaultsToUnlimited (324.56s)
--- PASS: TestAccAWSInstance_creditSpecificationT3_updateCpuCredits (136.49s)
--- PASS: TestAccAWSInstance_dedicatedInstance (111.88s)
--- PASS: TestAccAWSInstance_disableApiTermination (147.33s)
--- PASS: TestAccAWSInstance_disappears (68.70s)
--- PASS: TestAccAWSInstance_EbsBlockDevice_InvalidIopsForVolumeType (8.29s)
--- PASS: TestAccAWSInstance_EbsBlockDevice_InvalidThroughputForVolumeType (7.43s)
--- PASS: TestAccAWSInstance_EbsBlockDevice_KmsKeyArn (109.12s)
--- PASS: TestAccAWSInstance_EbsRootDevice_basic (138.20s)
--- PASS: TestAccAWSInstance_EbsRootDevice_ModifyAll (143.95s)
--- PASS: TestAccAWSInstance_EbsRootDevice_ModifyDeleteOnTermination (120.57s)
--- PASS: TestAccAWSInstance_EbsRootDevice_ModifyIOPS_Io1 (172.28s)
--- PASS: TestAccAWSInstance_EbsRootDevice_ModifyIOPS_Io2 (130.38s)
--- PASS: TestAccAWSInstance_EbsRootDevice_ModifySize (132.30s)
--- PASS: TestAccAWSInstance_EbsRootDevice_ModifyThroughput_Gp3 (152.25s)
--- PASS: TestAccAWSInstance_EbsRootDevice_ModifyType (141.72s)
--- PASS: TestAccAWSInstance_EbsRootDevice_MultipleBlockDevices_ModifyDeleteOnTermination (108.78s)
--- PASS: TestAccAWSInstance_EbsRootDevice_MultipleBlockDevices_ModifySize (172.46s)
--- PASS: TestAccAWSInstance_EbsRootDevice_MultipleDynamicEBSBlockDevices (213.17s)
--- PASS: TestAccAWSInstance_Empty_PrivateIP (104.27s)
--- PASS: TestAccAWSInstance_enclaveOptions (216.43s)
--- PASS: TestAccAWSInstance_forceNewAndTagsDrift (162.62s)
--- PASS: TestAccAWSInstance_getPasswordData_falseToTrue (153.69s)
--- PASS: TestAccAWSInstance_getPasswordData_trueToFalse (186.56s)
--- PASS: TestAccAWSInstance_GP2IopsDevice (156.85s)
--- PASS: TestAccAWSInstance_GP2WithIopsValue (7.61s)
--- PASS: TestAccAWSInstance_hibernation (199.64s)
--- PASS: TestAccAWSInstance_inDefaultVpcBySgId (92.83s)
--- PASS: TestAccAWSInstance_inDefaultVpcBySgName (103.11s)
--- PASS: TestAccAWSInstance_inEc2Classic (78.07s)
--- PASS: TestAccAWSInstance_instanceProfileChange (223.90s)
--- PASS: TestAccAWSInstance_ipv6_supportAddressCount (99.40s)
--- PASS: TestAccAWSInstance_ipv6_supportAddressCountWithIpv4 (100.63s)
--- PASS: TestAccAWSInstance_ipv6AddressCountAndSingleAddressCausesError (15.70s)
--- PASS: TestAccAWSInstance_keyPairCheck (101.77s)
--- PASS: TestAccAWSInstance_metadataOptions (178.23s)
--- PASS: TestAccAWSInstance_NetworkInstanceRemovingAllSecurityGroups (142.18s)
--- PASS: TestAccAWSInstance_NetworkInstanceSecurityGroups (119.27s)
--- PASS: TestAccAWSInstance_NetworkInstanceVPCSecurityGroupIDs (119.96s)
--- PASS: TestAccAWSInstance_NewNetworkInterface_EmptyPrivateIPAndSecondaryPrivateIPs (100.96s)
--- PASS: TestAccAWSInstance_NewNetworkInterface_EmptyPrivateIPAndSecondaryPrivateIPsUpdate (165.49s)
--- PASS: TestAccAWSInstance_NewNetworkInterface_PrivateIPAndSecondaryPrivateIPs (319.49s)
--- PASS: TestAccAWSInstance_NewNetworkInterface_PrivateIPAndSecondaryPrivateIPsUpdate (166.89s)
--- PASS: TestAccAWSInstance_NewNetworkInterface_PublicIPAndSecondaryPrivateIPs (423.94s)
--- PASS: TestAccAWSInstance_noAMIEphemeralDevices (62.00s)
--- PASS: TestAccAWSInstance_placementGroup (323.68s)
--- PASS: TestAccAWSInstance_primaryNetworkInterface (154.91s)
--- PASS: TestAccAWSInstance_primaryNetworkInterfaceSourceDestCheck (156.34s)
--- PASS: TestAccAWSInstance_privateIP (209.27s)
--- PASS: TestAccAWSInstance_RootBlockDevice_KmsKeyArn (82.50s)
--- PASS: TestAccAWSInstance_rootBlockDeviceMismatch (83.04s)
--- PASS: TestAccAWSInstance_rootInstanceStore (100.11s)
--- PASS: TestAccAWSInstance_sourceDestCheck (169.24s)
--- PASS: TestAccAWSInstance_tags (111.91s)
--- PASS: TestAccAWSInstance_UserData_EmptyStringToUnspecified (130.54s)
--- PASS: TestAccAWSInstance_UserData_UnspecifiedToEmptyString (133.43s)
--- PASS: TestAccAWSInstance_userDataBase64 (113.53s)
--- PASS: TestAccAWSInstance_withIamInstanceProfile (114.62s)
--- SKIP: TestAccAWSInstance_outpost (1.13s)

klebediev · 2021-01-13T18:09:22Z

@YakDriver thank you very much for all the work you've done!

YakDriver · 2021-01-13T18:33:54Z

@klebediev Thank you for your work on this!! 🎉 There were no real changes with the functionality of your fixes - some minor variable name tweaks and test tweaks. I did some gardening with all the attributes/arguments but that was really secondary. Also, I saw what you were discussing with EBS volumes since the instance really isn't handling those. At some point, I think we should either add support to manage EBS volumes properly from aws_instance or deprecate it and point people to aws_ebs_volume.

ghost · 2021-01-15T00:19:03Z

This has been released in version 3.24.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

tdmalone · 2021-01-15T11:08:38Z

Causes: #17125

yermulnik · 2021-01-15T14:38:33Z

Did this PR cause the removal of volume_tags block from aws_instance for me with the latest AWS provider version or did I hit something else?

  # module.mssql-2.aws_instance.mssql-ec2 will be updated in-place
  ~ resource "aws_instance" "mssql-ec2" {
[…]
      ~ volume_tags                  = {
          - "Application"          = "MSSQL" -> null
[…]
        }
        ebs_block_device {
            delete_on_termination = false
[…]
            tags                  = {}
[…]
        }
[…]
        root_block_device {
[…]
            tags                  = {}
[…]
        }
[…]
}

If it was indeed this PR what caused the change why wouldn't it worth to communicate this via CHANGELOG entry?

UPD: the aws_instance resource in my TF code has no volume_tags attribute set but only tags though this PR replaced "volume_tags": tagsSchemaComputed(), with "volume_tags": tagsSchema(), which means that previously the contents of this attribute might have been supplied by the provider (if by any case this can be a cause of the change I faced):

[…] If Optional is true and Computed is true, the user can specify a value for this field, but the provider may supply a value if the user does not. […]

This effectively removes the churn-ing that is happening when an ebs_volume is attached separately from the module Takes advantage of the new feature in the aws provider available as of 3.24.0. hashicorp/terraform-provider-aws#15474

ghost · 2021-02-13T17:10:12Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

klebediev requested a review from a team October 4, 2020 08:31

github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Oct 4, 2020

klebediev mentioned this pull request Oct 4, 2020

r/aws_instance: volume_tags works incorrectly with aws_volume_attachment resources #12225

Closed

klebediev force-pushed the feature/add-root-volume-tags branch 4 times, most recently from 383aba4 to 069f1e1 Compare October 4, 2020 10:04

klebediev commented Oct 4, 2020

View reviewed changes

aws/resource_aws_instance.go Outdated Show resolved Hide resolved

YakDriver self-assigned this Oct 7, 2020

YakDriver added partition/aws-us-gov Pertains to the aws-us-gov partition. and removed needs-triage Waiting for first response or review from a maintainer. labels Oct 7, 2020

YakDriver requested changes Oct 7, 2020

View reviewed changes

YakDriver added the waiting-response Maintainers are waiting on response from community or contributor. label Oct 7, 2020

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Oct 7, 2020

YakDriver added the waiting-response Maintainers are waiting on response from community or contributor. label Oct 7, 2020

klebediev force-pushed the feature/add-root-volume-tags branch 2 times, most recently from 73bc62e to 224bf72 Compare October 13, 2020 14:07

klebediev marked this pull request as draft October 13, 2020 14:50

klebediev force-pushed the feature/add-root-volume-tags branch 2 times, most recently from 649938c to f0cc4ab Compare October 17, 2020 21:42

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Oct 18, 2020

YakDriver added 3 commits January 13, 2021 10:46

resource/instance: Pre-merge cleanup

a0cccd7

docs/instance: Clean up documentation

0f1fddc

resource/instance: Fix linting

b2909d5

YakDriver approved these changes Jan 13, 2021

View reviewed changes

YakDriver merged commit cbbada6 into hashicorp:master Jan 13, 2021

github-actions bot added this to the v3.24.0 milestone Jan 13, 2021

This was referenced Jan 13, 2021

aws_instance detecting changes even though no changes done to the terraform file #6555

Closed

tags support for *_block_device within aws_instance resources #4017

Closed

dekimsey mentioned this pull request Jan 14, 2021

Provide a toggle to disable the volume_tags terraform-aws-modules/terraform-aws-ec2-instance#195

Closed

dekimsey mentioned this pull request Jan 15, 2021

Move volume_tags into dynamic blocks's tags parameter terraform-aws-modules/terraform-aws-ec2-instance#197

Closed

smiller171 mentioned this pull request Jan 19, 2021

[Feature] EBS tagging rhythmictech/terraform-aws-ec2-single#2

Open

ewbankkit mentioned this pull request Jan 23, 2021

ignore_tags not working for volume_tags or tags on nested root_block_device #17257

Closed

bflad pushed a commit that referenced this pull request Jan 26, 2021

docs/resource/aws_instance: Update documentation for #15474 (#17174)

3cae7e8

nigelellis mentioned this pull request Jan 29, 2021

Attached EBS Volume tags flip flop between volume_tags for instance on each apply terraform-aws-modules/terraform-aws-ec2-instance#164

Closed

ghost locked as resolved and limited conversation to collaborators Feb 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/aws_instance: Add `tags` parameter to `root_block_device`, `ebs_block_device` blocks (#12225) #15474

resource/aws_instance: Add `tags` parameter to `root_block_device`, `ebs_block_device` blocks (#12225) #15474

klebediev commented Oct 4, 2020 •

edited by YakDriver

Loading

YakDriver commented Oct 7, 2020 •

edited

Loading

YakDriver left a comment

klebediev commented Oct 7, 2020

YakDriver commented Oct 7, 2020

klebediev commented Oct 18, 2020

klebediev commented Oct 18, 2020

YakDriver left a comment

klebediev commented Jan 13, 2021

YakDriver commented Jan 13, 2021

ghost commented Jan 15, 2021

tdmalone commented Jan 15, 2021

yermulnik commented Jan 15, 2021 •

edited

Loading

ghost commented Feb 13, 2021

resource/aws_instance: Add tags parameter to root_block_device, ebs_block_device blocks (#12225) #15474

resource/aws_instance: Add tags parameter to root_block_device, ebs_block_device blocks (#12225) #15474

Conversation

klebediev commented Oct 4, 2020 • edited by YakDriver Loading

Community Note

YakDriver commented Oct 7, 2020 • edited Loading

YakDriver left a comment

Choose a reason for hiding this comment

klebediev commented Oct 7, 2020

YakDriver commented Oct 7, 2020

klebediev commented Oct 18, 2020

klebediev commented Oct 18, 2020

YakDriver left a comment

Choose a reason for hiding this comment

klebediev commented Jan 13, 2021

YakDriver commented Jan 13, 2021

ghost commented Jan 15, 2021

tdmalone commented Jan 15, 2021

yermulnik commented Jan 15, 2021 • edited Loading

ghost commented Feb 13, 2021

resource/aws_instance: Add `tags` parameter to `root_block_device`, `ebs_block_device` blocks (#12225) #15474

resource/aws_instance: Add `tags` parameter to `root_block_device`, `ebs_block_device` blocks (#12225) #15474

klebediev commented Oct 4, 2020 •

edited by YakDriver

Loading

YakDriver commented Oct 7, 2020 •

edited

Loading

yermulnik commented Jan 15, 2021 •

edited

Loading