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.

Sign up for GitHub

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

service/s3: Add d.IsNewResource checks and standardize retry logic #18537

Merged
merged 2 commits into from
Apr 2, 2021
Merged

service/s3: Add d.IsNewResource checks and standardize retry logic #18537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
714c0b4
service/s3: Add d.IsNewResource checks and standardize retry logic
bflad Apr 1, 2021
e796b06
resource/aws_s3_bucket_metric: Standardize on S3 Bucket Metrics Confi…
bflad Apr 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions aws/internal/service/s3/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package s3
// https://docs.aws.amazon.com/sdk-for-go/api/service/s3/#pkg-constants

const (
ErrCodeNoSuchConfiguration = "NoSuchConfiguration"
ErrCodeNoSuchPublicAccessBlockConfiguration = "NoSuchPublicAccessBlockConfiguration"
ErrCodeNoSuchTagSet = "NoSuchTagSet"
)
10 changes: 10 additions & 0 deletions aws/internal/service/s3/waiter/waiter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package waiter

import (
"time"
)

const (
// Maximum amount of time to wait for S3 changes to propagate
PropagationTimeout = 1 * time.Minute
)
39 changes: 28 additions & 11 deletions aws/resource_aws_s3_bucket_analytics_configuration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"
tfs3 "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/s3"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource"
)

Expand Down Expand Up @@ -147,19 +148,23 @@ func resourceAwsS3BucketAnalyticsConfigurationPut(d *schema.ResourceData, meta i

err := resource.Retry(1*time.Minute, func() *resource.RetryError {
_, err := s3conn.PutBucketAnalyticsConfiguration(input)

if tfawserr.ErrCodeEquals(err, s3.ErrCodeNoSuchBucket) {
Copy link
Contributor

@anGie44 anGie44 Apr 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

open q: why move this if-logic outside of the if err != nil check below? just curious b/c i've caught myself writing these types of checks w/in an if err != nil especially in some Delete CRUD ops of resources and i think my rationale at the time was that it didn't seem to make sense to pass the err around if it's nil but maybe that goes against our usual patterns...and i do like the visibility of the error check as you have now, rather than hiding within an if-block 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question! It's something I picked up from https://medium.com/@matryer/line-of-sight-in-code-186dd7cdea88

In this case since it is the unhappy path being nested under the if err != nil, it is probably okay to be nested if we wanted. Prior to these AWS SDK Go error checking helpers it used to be that this logic had to be nested (or duplicate the err != nil check) for the type assertions, but now that this detail is tucked away, it this allows for either style. 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh very cool, thanks! def sounds familiar coming from ruby land (and Sandi Metz's principles)

return resource.RetryableError(err)
}

if err != nil {
if isAWSErr(err, s3.ErrCodeNoSuchBucket, "") {
return resource.RetryableError(err)
}
return resource.NonRetryableError(err)
}
return nil
})
if isResourceTimeoutError(err) {

if tfresource.TimedOut(err) {
_, err = s3conn.PutBucketAnalyticsConfiguration(input)
}

if err != nil {
return fmt.Errorf("Error adding S3 analytics configuration: %w", err)
return fmt.Errorf("error adding S3 Bucket Analytics Configuration: %w", err)
}

d.SetId(fmt.Sprintf("%s:%s", bucket, name))
Expand All @@ -185,13 +190,25 @@ func resourceAwsS3BucketAnalyticsConfigurationRead(d *schema.ResourceData, meta

log.Printf("[DEBUG] Reading S3 bucket analytics configuration: %s", input)
output, err := conn.GetBucketAnalyticsConfiguration(input)

if !d.IsNewResource() && tfawserr.ErrCodeEquals(err, s3.ErrCodeNoSuchBucket) {
log.Printf("[WARN] S3 Bucket Analytics Configuration (%s) not found, removing from state", d.Id())
d.SetId("")
return nil
}

if !d.IsNewResource() && tfawserr.ErrCodeEquals(err, tfs3.ErrCodeNoSuchConfiguration) {
log.Printf("[WARN] S3 Bucket Analytics Configuration (%s) not found, removing from state", d.Id())
d.SetId("")
return nil
}

if err != nil {
if isAWSErr(err, s3.ErrCodeNoSuchBucket, "") || isAWSErr(err, "NoSuchConfiguration", "The specified configuration does not exist.") {
log.Printf("[WARN] %s S3 bucket analytics configuration not found, removing from state.", d.Id())
d.SetId("")
return nil
}
return fmt.Errorf("error getting S3 Bucket Analytics Configuration %q: %w", d.Id(), err)
return fmt.Errorf("error getting S3 Bucket Analytics Configuration (%s): %w", d.Id(), err)
}

if output == nil {
return fmt.Errorf("error getting S3 Bucket Analytics Configuration (%s): empty response", d.Id())
}

if err := d.Set("filter", flattenS3AnalyticsFilter(output.AnalyticsConfiguration.Filter)); err != nil {
Expand Down
81 changes: 53 additions & 28 deletions aws/resource_aws_s3_bucket_inventory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,16 @@ import (
"fmt"
"log"
"strings"
"time"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/s3"
"github.com/hashicorp/aws-sdk-go-base/tfawserr"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
tfs3 "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/s3"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/service/s3/waiter"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource"
)

func resourceAwsS3BucketInventory() *schema.Resource {
Expand Down Expand Up @@ -232,21 +235,26 @@ func resourceAwsS3BucketInventoryPut(d *schema.ResourceData, meta interface{}) e
}

log.Printf("[DEBUG] Putting S3 bucket inventory configuration: %s", input)
err := resource.Retry(1*time.Minute, func() *resource.RetryError {
err := resource.Retry(waiter.PropagationTimeout, func() *resource.RetryError {
_, err := conn.PutBucketInventoryConfiguration(input)

if tfawserr.ErrCodeEquals(err, s3.ErrCodeNoSuchBucket) {
return resource.RetryableError(err)
}

if err != nil {
if isAWSErr(err, s3.ErrCodeNoSuchBucket, "") {
return resource.RetryableError(err)
}
return resource.NonRetryableError(err)
}

return nil
})
if isResourceTimeoutError(err) {

if tfresource.TimedOut(err) {
_, err = conn.PutBucketInventoryConfiguration(input)
}

if err != nil {
return fmt.Errorf("Error putting S3 bucket inventory configuration: %s", err)
return fmt.Errorf("error putting S3 Bucket Inventory Configuration: %w", err)
}

d.SetId(fmt.Sprintf("%s:%s", bucket, name))
Expand All @@ -269,11 +277,17 @@ func resourceAwsS3BucketInventoryDelete(d *schema.ResourceData, meta interface{}

log.Printf("[DEBUG] Deleting S3 bucket inventory configuration: %s", input)
_, err = conn.DeleteBucketInventoryConfiguration(input)

if tfawserr.ErrCodeEquals(err, s3.ErrCodeNoSuchBucket) {
return nil
}

if tfawserr.ErrCodeEquals(err, tfs3.ErrCodeNoSuchConfiguration) {
return nil
}

if err != nil {
if isAWSErr(err, s3.ErrCodeNoSuchBucket, "") || isAWSErr(err, "NoSuchConfiguration", "The specified configuration does not exist.") {
return nil
}
return fmt.Errorf("Error deleting S3 bucket inventory configuration: %s", err)
return fmt.Errorf("error deleting S3 Bucket Inventory Configuration (%s): %w", d.Id(), err)
}

return nil
Expand All @@ -297,38 +311,49 @@ func resourceAwsS3BucketInventoryRead(d *schema.ResourceData, meta interface{})

log.Printf("[DEBUG] Reading S3 bucket inventory configuration: %s", input)
var output *s3.GetBucketInventoryConfigurationOutput
err = resource.Retry(1*time.Minute, func() *resource.RetryError {
err = resource.Retry(waiter.PropagationTimeout, func() *resource.RetryError {
var err error
output, err = conn.GetBucketInventoryConfiguration(input)

if d.IsNewResource() && tfawserr.ErrCodeEquals(err, s3.ErrCodeNoSuchBucket) {
return resource.RetryableError(err)
}

if d.IsNewResource() && tfawserr.ErrCodeEquals(err, tfs3.ErrCodeNoSuchConfiguration) {
return resource.RetryableError(err)
}

if err != nil {
if isAWSErr(err, s3.ErrCodeNoSuchBucket, "") || isAWSErr(err, "NoSuchConfiguration", "The specified configuration does not exist.") {
if d.IsNewResource() {
return resource.RetryableError(err)
}
return nil
}
return resource.NonRetryableError(err)
}

return nil
})
if isResourceTimeoutError(err) {

if tfresource.TimedOut(err) {
output, err = conn.GetBucketInventoryConfiguration(input)
if isAWSErr(err, s3.ErrCodeNoSuchBucket, "") || isAWSErr(err, "NoSuchConfiguration", "The specified configuration does not exist.") {
if !d.IsNewResource() {
return nil
}
}
}
if err != nil {
return fmt.Errorf("error getting S3 Bucket Inventory (%s): %s", d.Id(), err)

if !d.IsNewResource() && tfawserr.ErrCodeEquals(err, s3.ErrCodeNoSuchBucket) {
log.Printf("[WARN] S3 Bucket Inventory Configuration (%s) not found, removing from state", d.Id())
d.SetId("")
return nil
}

if output == nil || output.InventoryConfiguration == nil {
log.Printf("[WARN] %s S3 bucket inventory configuration not found, removing from state.", d.Id())
if !d.IsNewResource() && tfawserr.ErrCodeEquals(err, tfs3.ErrCodeNoSuchConfiguration) {
log.Printf("[WARN] S3 Bucket Inventory Configuration (%s) not found, removing from state", d.Id())
d.SetId("")
return nil
}

if err != nil {
return fmt.Errorf("error getting S3 Bucket Inventory Configuration (%s): %w", d.Id(), err)
}

if output == nil || output.InventoryConfiguration == nil {
return fmt.Errorf("error getting S3 Bucket Inventory Configuration (%s): empty response", d.Id())
}

d.Set("enabled", output.InventoryConfiguration.IsEnabled)
d.Set("included_object_versions", output.InventoryConfiguration.IncludedObjectVersions)

Expand Down
66 changes: 46 additions & 20 deletions aws/resource_aws_s3_bucket_metric.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,16 @@ import (
"fmt"
"log"
"strings"
"time"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/s3"
"github.com/hashicorp/aws-sdk-go-base/tfawserr"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"
tfs3 "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/s3"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/service/s3/waiter"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource"
)

func resourceAwsS3BucketMetric() *schema.Resource {
Expand Down Expand Up @@ -80,22 +83,27 @@ func resourceAwsS3BucketMetricPut(d *schema.ResourceData, meta interface{}) erro
MetricsConfiguration: metricsConfiguration,
}

log.Printf("[DEBUG] Putting metric configuration: %s", input)
err := resource.Retry(1*time.Minute, func() *resource.RetryError {
log.Printf("[DEBUG] Putting S3 Bucket Metrics Configuration: %s", input)
err := resource.Retry(waiter.PropagationTimeout, func() *resource.RetryError {
_, err := conn.PutBucketMetricsConfiguration(input)

if tfawserr.ErrCodeEquals(err, s3.ErrCodeNoSuchBucket) {
return resource.RetryableError(err)
}

if err != nil {
if isAWSErr(err, s3.ErrCodeNoSuchBucket, "") {
return resource.RetryableError(err)
}
return resource.NonRetryableError(err)
}

return nil
})
if isResourceTimeoutError(err) {

if tfresource.TimedOut(err) {
_, err = conn.PutBucketMetricsConfiguration(input)
}

if err != nil {
return fmt.Errorf("Error putting S3 metric configuration: %s", err)
return fmt.Errorf("error putting S3 Bucket Metrics Configuration: %w", err)
}

d.SetId(fmt.Sprintf("%s:%s", bucket, name))
Expand All @@ -116,13 +124,19 @@ func resourceAwsS3BucketMetricDelete(d *schema.ResourceData, meta interface{}) e
Id: aws.String(name),
}

log.Printf("[DEBUG] Deleting S3 bucket metric configuration: %s", input)
log.Printf("[DEBUG] Deleting S3 Bucket Metrics Configuration: %s", input)
_, err = conn.DeleteBucketMetricsConfiguration(input)

if tfawserr.ErrCodeEquals(err, s3.ErrCodeNoSuchBucket) {
return nil
}

if tfawserr.ErrCodeEquals(err, tfs3.ErrCodeNoSuchConfiguration) {
return nil
}

if err != nil {
if isAWSErr(err, s3.ErrCodeNoSuchBucket, "") || isAWSErr(err, "NoSuchConfiguration", "The specified configuration does not exist.") {
return nil
}
return fmt.Errorf("Error deleting S3 metric configuration: %w", err)
return fmt.Errorf("error deleting S3 Bucket Metrics Configuration (%s): %w", d.Id(), err)
}

return nil
Expand All @@ -144,15 +158,27 @@ func resourceAwsS3BucketMetricRead(d *schema.ResourceData, meta interface{}) err
Id: aws.String(name),
}

log.Printf("[DEBUG] Reading S3 bucket metrics configuration: %s", input)
log.Printf("[DEBUG] Reading S3 Bucket Metrics Configuration: %s", input)
output, err := conn.GetBucketMetricsConfiguration(input)

if !d.IsNewResource() && tfawserr.ErrCodeEquals(err, s3.ErrCodeNoSuchBucket) {
log.Printf("[WARN] S3 Bucket Metrics Configuration (%s) not found, removing from state", d.Id())
d.SetId("")
return nil
}

if !d.IsNewResource() && tfawserr.ErrCodeEquals(err, tfs3.ErrCodeNoSuchConfiguration) {
log.Printf("[WARN] S3 Bucket Metrics Configuration (%s) not found, removing from state", d.Id())
d.SetId("")
return nil
}

if err != nil {
if isAWSErr(err, s3.ErrCodeNoSuchBucket, "") || isAWSErr(err, "NoSuchConfiguration", "The specified configuration does not exist.") {
log.Printf("[WARN] %s S3 bucket metrics configuration not found, removing from state.", d.Id())
d.SetId("")
return nil
}
return err
return fmt.Errorf("error reading S3 Bucket Metrics Configuration (%s): %w", d.Id(), err)
}

if output == nil || output.MetricsConfiguration == nil {
return fmt.Errorf("error reading S3 Bucket Metrics Configuration (%s): empty response", d.Id())
}

if output.MetricsConfiguration.Filter != nil {
Expand Down
Loading