resource/aws_s3_bucket: Retry on GetBucketTagging 404 Errors

[camlow325]

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 #13008

Release note for CHANGELOG:

* resource/aws_s3_bucket: Retry on GetBucketTagging 404 errors due to eventual consistency ([#13008](https://github.com/terraform-providers/terraform-provider-aws/issues/13008))

The AWS S3 service has eventual consistency considerations. If a
GetBucketTagging call is made to obtain tags just after an S3 bucket is
first created, AWS may return an HTTP 404 (NotFound) error with a
NoSuchBucket error code.

A fix was added for this in #12418. It appears that the NoSuchBucket
errors are not retried with this fix, however. This commit adds some
extra logic which ensures that the code from the awserr.Error instance
is evaluated for retry.

Output for acceptance testing:

> make testacc TEST=./aws TESTARGS='-run=TestAccAWSS3Bucket_'
...
--- PASS: TestAccAWSS3Bucket_shouldFailNotFound (22.93s)
--- PASS: TestAccAWSS3Bucket_LifecycleRule_Expiration_EmptyConfigurationBlock (38.71s)
--- PASS: TestAccAWSS3Bucket_forceDestroyWithEmptyPrefixes (40.46s)
--- PASS: TestAccAWSS3Bucket_forceDestroy (40.47s)
--- PASS: TestAccAWSS3Bucket_enableDefaultEncryption_whenAES256IsUsed (45.72s)
--- PASS: TestAccAWSS3Bucket_basic (45.79s)
--- PASS: TestAccAWSS3Bucket_forceDestroyWithObjectLockEnabled (47.59s)
--- PASS: TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError (51.85s)
--- PASS: TestAccAWSS3Bucket_LifecycleBasic (95.95s)
--- PASS: TestAccAWSS3Bucket_LifecycleExpireMarkerOnly (69.16s)
--- PASS: TestAccAWSS3Bucket_enableDefaultEncryption_whenTypical (71.63s)
--- PASS: TestAccAWSS3Bucket_objectLock (74.52s)
--- PASS: TestAccAWSS3Bucket_disableDefaultEncryption_whenDefaultEncryptionIsEnabled (75.75s)
--- PASS: TestAccAWSS3Bucket_region (42.47s)
--- PASS: TestAccAWSS3Bucket_ReplicationWithoutPrefix (96.80s)
--- PASS: TestAccAWSS3Bucket_WebsiteRoutingRules (77.87s)
--- PASS: TestAccAWSS3Bucket_Versioning (106.51s)
--- PASS: TestAccAWSS3Bucket_UpdateGrant (110.50s)
--- PASS: TestAccAWSS3Bucket_GrantToAcl (71.89s)
--- PASS: TestAccAWSS3Bucket_AclToGrant (72.06s)
--- PASS: TestAccAWSS3Bucket_generatedName (47.42s)
--- PASS: TestAccAWSS3Bucket_UpdateAcl (74.86s)
--- PASS: TestAccAWSS3Bucket_namePrefix (44.58s)
--- PASS: TestAccAWSS3Bucket_RequestPayer (75.62s)
--- PASS: TestAccAWSS3Bucket_ReplicationWithoutStorageClass (97.00s)
--- PASS: TestAccAWSS3Bucket_Cors_EmptyOrigin (47.36s)
--- PASS: TestAccAWSS3Bucket_WebsiteRedirect (109.43s)
--- PASS: TestAccAWSS3Bucket_Website_Simple (109.41s)
--- PASS: TestAccAWSS3Bucket_ReplicationConfiguration_Rule_Destination_AddAccessControlTranslation (152.76s)
--- PASS: TestAccAWSS3Bucket_acceleration (81.59s)
--- PASS: TestAccAWSS3Bucket_Cors_Delete (36.58s)
--- PASS: TestAccAWSS3Bucket_Bucket_EmptyString (44.94s)
--- PASS: TestAccAWSS3Bucket_Logging (66.26s)
--- PASS: TestAccAWSS3Bucket_Policy (104.79s)
--- PASS: TestAccAWSS3Bucket_ReplicationConfiguration_Rule_Destination_AccessControlTranslation (176.75s)
--- PASS: TestAccAWSS3Bucket_Cors_Update (71.71s)
--- PASS: TestAccAWSS3Bucket_tagsWithNoSystemTags (135.94s)
--- PASS: TestAccAWSS3Bucket_tagsWithSystemTags (168.39s)
--- PASS: TestAccAWSS3Bucket_Replication (270.65s)
--- PASS: TestAccAWSS3Bucket_ReplicationSchemaV2 (273.25s)

[ewbankkit]

@camlow325 Thanks for addressing this.
It may make sense to move that errors.As check into retryOnAwsCode (and use the isAWSErr helper) as I can see this having more general application with other wrapped errors - #12991.

@bflad What do you think?

[bflad]

I would suggest hoisting the errors.As unwrapping fix into the retryOnAwsCode() function itself. 👍

[bflad]

Looks like this may also close #12907

[ewbankkit]

@camlow325 Changing

https://github.com/terraform-providers/terraform-provider-aws/blob/33b3d2d55d68cbe558ebcb10df7f85e0a35b6a72/aws/awserr.go#L42-L47

to

		if err != nil {
			var awsErr awserr.Error
			if errors.As(err, &awsErr) && awsErr.Code() == code {
				return resource.RetryableError(err)
			}
			return resource.NonRetryableError(err)

should do the trick.

[ewbankkit]

I think you can also remove RetryOnAwsCodes as it doesn't seem to be called anywhere.

[camlow325]

Thanks, @bflad and @ewbankkit. I updated the PR to use the isAWSErr() function from retryOnAwsCode() in order to have errors.As() be used to unwrap any nested awserr.Error instances from the error object. That seems to work well for having retries be done for GetBucketTagging 404 errors and the S3 bucket tests still pass with it as well. I also removed the RetryOnAwsCodes function that no longer appears to be used anywhere.

[ewbankkit]

Closes #12652.
Closes #12907.

[bpiper]

Just curious if there's anything stopping this PR from being approved/merged? Since it's been sitting inactive for several months and may fix a couple of very annoying issues.

[github-actions]

This functionality has been released in v3.57.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. Thank you!

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.