Remove tag on create logic for gov cloud since no longer applicable

[domwong]

Changes proposed in this pull request:

• It used to be that restricted cloud accounts (e.g. gov cloud) cannot create volumes with tags, they can only update it after creation. See here. However, this has recently changed as announced here. This PR removes the logic behind the restriction for gov cloud. NOTE it is unclear whether China cloud still has this restriction so leaving that logic in there

This capability is available at no additional cost in all AWS commercial regions, and the AWS GovCloud (US) Region.

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSInstance.*Tag'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSInstance.*Tag -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSInstance_volumeTags
--- PASS: TestAccAWSInstance_volumeTags (183.15s)
=== RUN   TestAccAWSInstance_volumeTagsComputed
--- FAIL: TestAccAWSInstance_volumeTagsComputed (18.74s)
	testing.go:518: Step 0 error: Error applying: 1 error(s) occurred:
		
		* aws_instance.foo: 1 error(s) occurred:
		
		* aws_instance.foo: Error launching source instance: InstanceLimitExceeded: You have requested more instances (1) than your current instance limit of 0 allows for the specified instance type. Please visit http://aws.amazon.com/contact-us/ec2-request to request an adjustment to this limit.
			status code: 400, request id: 67bd5f4e-4e29-4ea7-9ca7-27d85acacbfb
=== RUN   TestAccAWSInstance_forceNewAndTagsDrift
--- PASS: TestAccAWSInstance_forceNewAndTagsDrift (225.67s)
FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	427.607s
make: *** [testacc] Error 1

Note: test failure due to account limits

[tomelliff]

Is this still true considering this recent announcement?

[domwong]

@tomelliff perhaps not. I proposed this patch more because as someone working on non gov-cloud AWS the current behaviour results in unneeded CreateTags calls in "non restricted" regions.

If it's still not available for China cloud then the logic is right in that it needs to be restricted for some cases, but the calculation of restricted needs to change to just a check for whether it's China cloud or not.

EDIT: This line from that most recent announcement seems to suggest it's available everywhere so I'll look at removing the restricted check completely.

This capability is available at no additional cost in all AWS commercial regions, and the AWS GovCloud (US) Region.

[domwong]

I've made the changes, renamed, and updated the description on this PR. Thanks @tomelliff for the link.

[bflad]

I'm not entirely sure the mentioned line below implies AWS China support:

This capability is available at no additional cost in all AWS commercial regions, and the AWS GovCloud (US) Region.

AWS China is separate from AWS commercial and AWS GovCloud (US).

Unfortunately, we (HashiCorp) do not have an AWS China account to verify this either way so unless someone with that type of account can run the full acceptance testing and show all the results passing, this pull request cannot be accepted and merged. If the previous behavior with AWS China is present (at least during instance creation -- the update change is fine as that is on the operator to understand the API error), we can get this in.

---

Aside:

The usage of restricted/isGovCloud()/isChinaCloud() represented the first iteration of trying to support the 3 AWS partitions when they offered differing capabilities due to running different service API versions. While this approach of checking the AWS partition provides an immediate fix for these situations, it does not automatically handle if/when those new capabilities are enabled.

In newer iterations of this handling, we attempt to simply ignore read errors based on specific error messages, like UnsupportedOperation and passthrough any create/update errors. See https://github.com/terraform-providers/terraform-provider-aws/pull/3794/files for an example of this. I understand this situation is a little different, but I feel like the additional context here is relevant.

I thought I had created a technical debt issue to swap out usage of restricted/isGovCloud()/isChinaCloud() for the future-proofing behavior, but looks like I haven't done that yet. Unfortunately, it would not be a priority for the maintainers to implement, but we would certainly look at pull requests to update that code handling.

[bflad]

For what its worth, we could also remove the isChinaCloud() handling during tagging on creation in AWS China if we know what error message is getting returned then re-submitting the request without that problematic parameter. This would future-proof the resource for AWS China in that situation.

e.g.

runResp, err = conn.RunInstances(runOpts)
// Fake error message handling here -- we need the real one
if isAWSErr(err, "UnsupportedOperation", "TagSpecifications") {
  runOpts.TagSpecifications = nil
}
runResp, err = conn.RunInstances(runOpts)
// Rest of retry logic

[domwong]

Ah OK, thanks for the input and context. Didn't know the distinction between China et al. If it's unlikely we can get someone with a China cloud account to verify are we better off amending the PR to still check for isChinaCloud only?

[bflad]

are we better off amending the PR to still check for isChinaCloud only?

I think so, we can always remove it later if its actually supported.

[domwong]

I've reinstated the china cloud restriction @bflad

[bflad]

LGTM, thanks @domwong! 🚀

AWS Commercial (test failures unrelated):

Tests failed: 3 (3 new), passed: 66
=== RUN   TestAccAWSInstanceDataSource_keyPair
--- PASS: TestAccAWSInstanceDataSource_keyPair (101.45s)
=== RUN   TestAccAWSInstancesDataSource_basic
--- PASS: TestAccAWSInstancesDataSource_basic (103.33s)
=== RUN   TestAccAWSInstanceDataSource_blockDevices
--- PASS: TestAccAWSInstanceDataSource_blockDevices (104.21s)
=== RUN   TestAccAWSInstanceDataSource_rootInstanceStore
--- PASS: TestAccAWSInstanceDataSource_rootInstanceStore (110.72s)
=== RUN   TestAccAWSInstanceDataSource_creditSpecification
--- PASS: TestAccAWSInstanceDataSource_creditSpecification (111.09s)
=== RUN   TestAccAWSInstanceDataSource_SecurityGroups
--- PASS: TestAccAWSInstanceDataSource_SecurityGroups (111.89s)
=== RUN   TestAccAWSInstance_importInDefaultVpcBySgName
--- PASS: TestAccAWSInstance_importInDefaultVpcBySgName (113.80s)
=== RUN   TestAccAWSInstanceDataSource_tags
--- PASS: TestAccAWSInstanceDataSource_tags (116.92s)
=== RUN   TestAccAWSInstancesDataSource_tags
--- FAIL: TestAccAWSInstancesDataSource_tags (117.17s)
=== RUN   TestAccAWSInstanceDataSource_VPCSecurityGroups
--- PASS: TestAccAWSInstanceDataSource_VPCSecurityGroups (134.41s)
=== RUN   TestAccAWSInstanceDataSource_AzUserData
--- PASS: TestAccAWSInstanceDataSource_AzUserData (140.54s)
=== RUN   TestAccAWSInstanceDataSource_basic
--- PASS: TestAccAWSInstanceDataSource_basic (146.10s)
=== RUN   TestAccAWSInstance_GP2WithIopsValue
--- PASS: TestAccAWSInstance_GP2WithIopsValue (55.27s)
=== RUN   TestAccAWSInstance_blockDevices
--- PASS: TestAccAWSInstance_blockDevices (60.06s)
=== RUN   TestAccAWSInstanceDataSource_PlacementGroup
--- PASS: TestAccAWSInstanceDataSource_PlacementGroup (180.56s)
=== RUN   TestAccAWSInstanceDataSource_gp2IopsDevice
--- PASS: TestAccAWSInstanceDataSource_gp2IopsDevice (180.92s)
=== RUN   TestAccAWSInstanceDataSource_getPasswordData_falseToTrue
--- PASS: TestAccAWSInstanceDataSource_getPasswordData_falseToTrue (186.95s)
=== RUN   TestAccAWSInstance_ipv6AddressCountAndSingleAddressCausesError
--- PASS: TestAccAWSInstance_ipv6AddressCountAndSingleAddressCausesError (6.65s)
=== RUN   TestAccAWSInstance_importInEc2Classic
--- PASS: TestAccAWSInstance_importInEc2Classic (87.90s)
=== RUN   TestAccAWSInstance_rootInstanceStore
--- PASS: TestAccAWSInstance_rootInstanceStore (77.18s)
=== RUN   TestAccAWSInstanceDataSource_getPasswordData_trueToFalse
--- PASS: TestAccAWSInstanceDataSource_getPasswordData_trueToFalse (211.67s)
=== RUN   TestAccAWSInstancesDataSource_instance_state_names
--- FAIL: TestAccAWSInstancesDataSource_instance_state_names (214.19s)
=== RUN   TestAccAWSInstanceDataSource_VPC
--- PASS: TestAccAWSInstanceDataSource_VPC (223.88s)
=== RUN   TestAccAWSInstanceDataSource_privateIP
--- PASS: TestAccAWSInstanceDataSource_privateIP (233.95s)
=== RUN   TestAccAWSInstance_importBasic
--- PASS: TestAccAWSInstance_importBasic (235.57s)
=== RUN   TestAccAWSInstance_basic
--- PASS: TestAccAWSInstance_basic (135.25s)
=== RUN   TestAccAWSInstance_vpc
--- PASS: TestAccAWSInstance_vpc (100.57s)
=== RUN   TestAccAWSInstance_ipv6_supportAddressCount
--- PASS: TestAccAWSInstance_ipv6_supportAddressCount (81.75s)
=== RUN   TestAccAWSInstance_sourceDestCheck
--- PASS: TestAccAWSInstance_sourceDestCheck (124.79s)
=== RUN   TestAccAWSInstance_disableApiTermination
--- PASS: TestAccAWSInstance_disableApiTermination (121.81s)
=== RUN   TestAccAWSInstance_ipv6_supportAddressCountWithIpv4
--- PASS: TestAccAWSInstance_ipv6_supportAddressCountWithIpv4 (82.07s)
=== RUN   TestAccAWSInstance_noAMIEphemeralDevices
--- PASS: TestAccAWSInstance_noAMIEphemeralDevices (163.62s)
=== RUN   TestAccAWSInstance_GP2IopsDevice
--- PASS: TestAccAWSInstance_GP2IopsDevice (173.19s)
=== RUN   TestAccAWSInstance_volumeTags
--- PASS: TestAccAWSInstance_volumeTags (86.13s)
=== RUN   TestAccAWSInstance_userDataBase64
--- PASS: TestAccAWSInstance_userDataBase64 (210.71s)
=== RUN   TestAccAWSInstance_privateIP
--- PASS: TestAccAWSInstance_privateIP (82.21s)
=== RUN   TestAccAWSInstance_importInDefaultVpcBySgId
--- PASS: TestAccAWSInstance_importInDefaultVpcBySgId (222.63s)
=== RUN   TestAccAWSInstance_placementGroup
--- PASS: TestAccAWSInstance_placementGroup (168.17s)
=== RUN   TestAccAWSInstance_keyPairCheck
--- PASS: TestAccAWSInstance_keyPairCheck (91.50s)
=== RUN   TestAccAWSInstance_tags
--- PASS: TestAccAWSInstance_tags (135.47s)
=== RUN   TestAccAWSInstance_associatePublicIPAndPrivateIP
--- PASS: TestAccAWSInstance_associatePublicIPAndPrivateIP (111.49s)
=== RUN   TestAccAWSInstance_primaryNetworkInterface
--- PASS: TestAccAWSInstance_primaryNetworkInterface (84.98s)
=== RUN   TestAccAWSInstance_forceNewAndTagsDrift
--- PASS: TestAccAWSInstance_forceNewAndTagsDrift (107.01s)
=== RUN   TestAccAWSInstance_instanceProfileChange
--- PASS: TestAccAWSInstance_instanceProfileChange (144.41s)
=== RUN   TestAccAWSInstance_NetworkInstanceSecurityGroups
--- PASS: TestAccAWSInstance_NetworkInstanceSecurityGroups (196.94s)
=== RUN   TestAccAWSInstance_NetworkInstanceVPCSecurityGroupIDs
--- PASS: TestAccAWSInstance_NetworkInstanceVPCSecurityGroupIDs (196.00s)
=== RUN   TestAccAWSInstance_NetworkInstanceRemovingAllSecurityGroups
--- PASS: TestAccAWSInstance_NetworkInstanceRemovingAllSecurityGroups (199.33s)
=== RUN   TestAccAWSInstance_associatePublic_defaultPublic
--- FAIL: TestAccAWSInstance_associatePublic_defaultPublic (78.69s)
=== RUN   TestAccAWSInstance_associatePublic_defaultPrivate
--- PASS: TestAccAWSInstance_associatePublic_defaultPrivate (81.25s)
=== RUN   TestAccAWSInstance_associatePublic_explicitPublic
--- PASS: TestAccAWSInstance_associatePublic_explicitPublic (81.75s)
=== RUN   TestAccAWSInstance_associatePublic_overridePublic
--- PASS: TestAccAWSInstance_associatePublic_overridePublic (80.85s)
=== RUN   TestAccAWSInstance_associatePublic_explicitPrivate
--- PASS: TestAccAWSInstance_associatePublic_explicitPrivate (81.26s)
=== RUN   TestAccAWSInstance_addSecondaryInterface
--- PASS: TestAccAWSInstance_addSecondaryInterface (134.68s)
=== RUN   TestAccAWSInstance_multipleRegions
--- PASS: TestAccAWSInstance_multipleRegions (249.64s)
=== RUN   TestAccAWSInstance_withIamInstanceProfile
--- PASS: TestAccAWSInstance_withIamInstanceProfile (202.13s)
=== RUN   TestAccAWSInstance_associatePublic_overridePrivate
--- PASS: TestAccAWSInstance_associatePublic_overridePrivate (81.52s)
=== RUN   TestAccAWSInstance_rootBlockDeviceMismatch
--- PASS: TestAccAWSInstance_rootBlockDeviceMismatch (183.51s)
=== RUN   TestAccAWSInstance_creditSpecification_unspecifiedDefaultsToStandard
--- PASS: TestAccAWSInstance_creditSpecification_unspecifiedDefaultsToStandard (82.62s)
=== RUN   TestAccAWSInstance_creditSpecification_standardCpuCredits
--- PASS: TestAccAWSInstance_creditSpecification_standardCpuCredits (80.90s)
=== RUN   TestAccAWSInstance_creditSpecification_unlimitedCpuCredits
--- PASS: TestAccAWSInstance_creditSpecification_unlimitedCpuCredits (81.38s)
=== RUN   TestAccAWSInstance_volumeTagsComputed
--- PASS: TestAccAWSInstance_volumeTagsComputed (254.99s)
=== RUN   TestAccAWSInstance_primaryNetworkInterfaceSourceDestCheck
--- PASS: TestAccAWSInstance_primaryNetworkInterfaceSourceDestCheck (204.94s)
=== RUN   TestAccAWSInstance_getPasswordData_falseToTrue
--- PASS: TestAccAWSInstance_getPasswordData_falseToTrue (167.73s)
=== RUN   TestAccAWSInstance_getPasswordData_trueToFalse
--- PASS: TestAccAWSInstance_getPasswordData_trueToFalse (176.68s)
=== RUN   TestAccAWSInstance_addSecurityGroupNetworkInterface
--- PASS: TestAccAWSInstance_addSecurityGroupNetworkInterface (247.68s)
=== RUN   TestAccAWSInstance_changeInstanceType
--- PASS: TestAccAWSInstance_changeInstanceType (321.92s)
=== RUN   TestAccAWSInstance_creditSpecification_updateCpuCredits
--- PASS: TestAccAWSInstance_creditSpecification_updateCpuCredits (218.76s)
=== RUN   TestAccAWSInstance_creditSpecification_removalReturnsStandard
--- PASS: TestAccAWSInstance_creditSpecification_removalReturnsStandard (211.52s)
=== RUN   TestAccAWSInstance_creditSpecification_isNotAppliedToNonBurstable
--- PASS: TestAccAWSInstance_creditSpecification_isNotAppliedToNonBurstable (242.68s)

AWS GovCloud (US): not ran, see: #4987

[bflad]

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

[ghost]

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!