Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resource/aws_s3_bucket: Prevent panics with various API read failures and prevent NoSuchBucket error on deletion #5842

Merged
merged 1 commit into from
Sep 13, 2018

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Sep 12, 2018

Rather than continue to piecemeal these interface conversion fixes, this implements them consistently all at once. Prevent panics such as these:

panic: interface conversion: interface {} is nil, not *s3.GetBucketLoggingOutput
2018-09-11T18:24:29.493Z [DEBUG] plugin.terraform-provider-aws_v1.22.0_x4: 
2018-09-11T18:24:29.493Z [DEBUG] plugin.terraform-provider-aws_v1.22.0_x4: goroutine 2526 [running]:
2018-09-11T18:24:29.493Z [DEBUG] plugin.terraform-provider-aws_v1.22.0_x4: github.com/terraform-providers/terraform-provider-aws/aws.resourceAwsS3BucketRead(0xc42786ce70, 0x28a1460, 0xc4247fa000, 0xc42786ce70, 0x0)
2018-09-11T18:24:29.493Z [DEBUG] plugin.terraform-provider-aws_v1.22.0_x4: 	/opt/teamcity-agent/work/222ea50a1b4f75f4/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_s3_bucket.go:877 +0x5d39

Ran across the NoSuchBucket error on deletion during testing:

--- FAIL: TestAccAWSS3Bucket_Cors_EmptyOrigin (45.32s)
    testing.go:588: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.

        Error: Error applying: 1 error occurred:
        	* aws_s3_bucket.bucket (destroy): 1 error occurred:
        	* aws_s3_bucket.bucket: Error deleting S3 Bucket: NoSuchBucket: The specified bucket does not exist

Aside from those cleans up some old issues/PRs as part of the consistency effort:

Closes #3459
Closes #3603
Closes #3620

Changes proposed in this pull request:

  • Prevent panic with GetBucketWebsite interface conversion
  • Prevent panic with GetBucketVersioning interface conversion
  • Prevent panic with GetBucketAccelerateConfiguration interface conversion
  • Prevent panic with GetBucketRequestPayment interface conversion
  • Prevent panic with GetBucketLogging interface conversion
  • Prevent panic with GetBucketReplication interface conversion
  • Prevent panic with GetBucketEncryption interface conversion
  • Prevent panic with GetBucketLocation interface conversion
  • Return successfully on NoSuchBucket error during deletion
  • Consistently perform read and error logic
  • Split TestAccAWSS3Bucket_Cors for testing stability (eventual consistency issues) and best practices (1 resource.Test per acceptance test)

Output from acceptance testing:

--- PASS: TestAccAWSS3Bucket_namePrefix (5.44s)
--- PASS: TestAccAWSS3Bucket_basic (5.55s)
--- PASS: TestAccAWSS3Bucket_generatedName (5.59s)
--- PASS: TestAccAWSS3Bucket_importWithPolicy (6.36s)
--- PASS: TestAccAWSS3Bucket_importBasic (6.37s)
--- PASS: TestAccAWSS3Bucket_RequestPayer (9.46s)
--- PASS: TestAccAWSS3Bucket_UpdateAcl (10.84s)
--- PASS: TestAccAWSS3Bucket_Policy (15.73s)
--- PASS: TestAccAWSS3Bucket_acceleration (20.32s)
--- PASS: TestAccAWSS3Bucket_Website_Simple (12.87s)
--- PASS: TestAccAWSS3Bucket_WebsiteRedirect (14.30s)
--- PASS: TestAccAWSS3Bucket_enableDefaultEncryption_whenAES256IsUsed (6.14s)
--- PASS: TestAccAWSS3Bucket_region (29.96s)
--- PASS: TestAccAWSS3Bucket_shouldFailNotFound (4.15s)
--- PASS: TestAccAWSS3Bucket_disableDefaultEncryption_whenDefaultEncryptionIsEnabled (11.38s)
--- PASS: TestAccAWSS3Bucket_WebsiteRoutingRules (22.50s)
--- PASS: TestAccAWSS3Bucket_Versioning (12.68s)
--- PASS: TestAccAWSS3Bucket_Cors_Update (9.86s)
--- PASS: TestAccAWSS3Bucket_Cors_Delete (5.56s)
--- PASS: TestAccAWSS3Bucket_Cors_EmptyOrigin (5.47s)
--- PASS: TestAccAWSS3Bucket_enableDefaultEncryption_whenTypical (25.40s)
--- PASS: TestAccAWSS3Bucket_Lifecycle (78.61s)
--- PASS: TestAccAWSS3Bucket_Logging (7.32s)
--- PASS: TestAccAWSS3Bucket_LifecycleExpireMarkerOnly (9.55s)
--- PASS: TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError (25.98s)
--- PASS: TestAccAWSS3Bucket_ReplicationWithoutStorageClass (38.67s)
--- PASS: TestAccAWSS3Bucket_Replication (113.15s)

… and prevent NoSuchBucket error on deletion

All changes:
* Prevent panic with GetBucketWebsite interface conversion
* Prevent panic with GetBucketVersioning interface conversion
* Prevent panic with GetBucketAccelerateConfiguration interface conversion
* Prevent panic with GetBucketRequestPayment interface conversion
* Prevent panic with GetBucketLogging interface conversion
* Prevent panic with GetBucketReplication interface conversion
* Prevent panic with GetBucketEncryption interface conversion
* Prevent panic with GetBucketLocation interface conversion
* Return successfully on NoSuchBucket error during deletion
* Consistently perform read logic and return more helpful error messages
* Refactor awserr logic to use isAWSErr() helper
* Split TestAccAWSS3Bucket_Cors for testing stability (eventual consistency issues) and best practices (1 resource.Test per acceptance test)
@bflad bflad added bug Addresses a defect in current functionality. crash Results from or addresses a Terraform crash or kernel panic. service/s3 Issues and PRs that pertain to the s3 service. labels Sep 12, 2018
@bflad bflad requested a review from a team September 12, 2018 02:41
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Sep 12, 2018
@bflad bflad added this to the v1.37.0 milestone Sep 13, 2018
@bflad bflad merged commit 94d1ced into master Sep 13, 2018
@bflad bflad deleted the b-aws_s3_bucket-assignment-ordering-panics branch September 13, 2018 18:34
bflad added a commit that referenced this pull request Sep 13, 2018
@bflad
Copy link
Contributor Author

bflad commented Sep 19, 2018

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

@ghost
Copy link

ghost commented Apr 3, 2020

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!

@ghost ghost locked and limited conversation to collaborators Apr 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. crash Results from or addresses a Terraform crash or kernel panic. service/s3 Issues and PRs that pertain to the s3 service. size/L Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CreateBucket fails on non-aws endpoint due to non implemented feature
2 participants