Updates to tags and ACLs should happen only for non new resources

[domwong]

Changes proposed in this pull request:

• During creation of EC2 instances, an extra call to CreateTags is applied to the attached EBS volume. This is unnecessary since the tags are already created when passed to the initial RunInstances call. Remove this extra call.
• diffTags incorrectly returns tags that exist in oldTags and newTags as needing to be created again. This causes unnecessary calls to CreateTags. Fix this to return only tags that need to be deleted and created.
• During creation of S3 buckets, an extra call to PutObjectACL is applied to the S3 bucket. This is unnecessary since the ACLs are already created when passed to the initial CreateBucket call. Remove this extra call.

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 (176.93s)
=== RUN   TestAccAWSInstance_volumeTagsComputed
--- PASS: TestAccAWSInstance_volumeTagsComputed (142.11s)
=== RUN   TestAccAWSInstance_forceNewAndTagsDrift
--- PASS: TestAccAWSInstance_forceNewAndTagsDrift (205.75s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	524.841s

$  make testacc TESTARGS='-run=TestAccAWSS3.*Tag'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSS3.*Tag -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSS3BucketMetric_WithFilterPrefixAndMultipleTags
--- PASS: TestAccAWSS3BucketMetric_WithFilterPrefixAndMultipleTags (83.62s)
=== RUN   TestAccAWSS3BucketMetric_WithFilterPrefixAndSingleTag
--- PASS: TestAccAWSS3BucketMetric_WithFilterPrefixAndSingleTag (80.15s)
=== RUN   TestAccAWSS3BucketMetric_WithFilterMultipleTags
--- PASS: TestAccAWSS3BucketMetric_WithFilterMultipleTags (80.13s)
=== RUN   TestAccAWSS3BucketMetric_WithFilterSingleTag
--- PASS: TestAccAWSS3BucketMetric_WithFilterSingleTag (82.22s)
=== RUN   TestAccAWSS3MultiBucket_withTags
--- PASS: TestAccAWSS3MultiBucket_withTags (43.56s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	369.731s

NOTE: I have not run all the tests, only those listed above

[domwong]

My understanding here is that "restricted" e.g. "gov cloud" accounts don't yet have the option to add tags at creation time so you need to set them afterwards. This change brings the behaviour in line with similar approach on line 848.

[bflad]

Hi @domwong 👋 Thanks for submitting this!

Would it be possible to split this PR into multiple? Usually handling one resource or data source per PR (when possible) is easiest for the maintainers to get quickly reviewed and merged, especially when one specific code update might need additional work.

I do have a question about the following:

diffTags incorrectly returns tags that exist in oldTags and newTags as needing to be created again. This causes unnecessary calls to CreateTags. Fix this to return only tags that need to be deleted and created.

Does this logic potentially exist to handle tag value updates for an existing tag key? Some service APIs do not support updating an existing tag so therefore require delete and create.

[domwong]

Hi @bflad,

Sure, will split this up and comment with new PR numbers here once done.

Does this logic potentially exist to handle tag value updates for an existing tag key? Some service APIs do not support updating an existing tag so therefore require delete and create.

That's a good point and one that I was not aware of. Will have a dig around and see which services this affects - sounds like I might have to drop that proposed change.

[domwong]

Closing in favour of #5106 #5107 #5108

[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!