From 4f58f0eb957a4715248b3e5342279f62b028ef4a Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Tue, 8 Oct 2019 12:34:38 -0400 Subject: [PATCH] helper/resource: Require explicit usage of NonRetryableError and RetryableError Reference: https://github.com/hashicorp/terraform/pull/17220 Reference: https://github.com/terraform-providers/terraform-provider-aws/pull/9779#discussion_r315176586 Reference: https://github.com/terraform-providers/terraform-provider-aws/pull/9812#discussion_r315457893 It is currently possible to introduce subtle bugs or crashes when using `resource.RetryableError` and `resource.NonRetryableError` and allowing a `nil` error input. This PR proposes behavior that requires providers to be explicit with their usage of these errors, otherwise returns a bug reporting message to the operator. For example (https://github.com/terraform-providers/terraform-provider-aws/blob/4c0387645f982ddcd51b3ffe2cc8992c06fb9c2c/aws/resource_aws_elastic_beanstalk_application.go#L105): ```go var app *elasticbeanstalk.ApplicationDescription err := resource.Retry(30*time.Second, func() *resource.RetryError { var err error app, err = getBeanstalkApplication(d.Id(), conn) if err != nil { return resource.NonRetryableError(err) } if app == nil { if d.IsNewResource() { return resource.RetryableError(fmt.Errorf("Elastic Beanstalk Application %q not found", d.Id())) } // err is nil here return resource.NonRetryableError(err) } return nil }) if err != nil { return err } // app can be nil here, so this can crash Terraform d.Set("name", app.ApplicationName) ``` Another example (https://github.com/terraform-providers/terraform-provider-alicloud/blob/927a8e82386e0b32718aa5eef254255fdc36b070/alicloud/resource_alicloud_disk_attachment.go#L112): ```go return resource.Retry(5*time.Minute, func() *resource.RetryError { err := conn.DetachDisk(instanceID, diskID) if err != nil { if IsExceptedError(err, DiskIncorrectStatus) || IsExceptedError(err, InstanceLockedForSecurity) || IsExceptedError(err, DiskInvalidOperation) { return resource.RetryableError(fmt.Errorf("Detach Disk timeout and got an error: %#v", err)) } } disks, _, descErr := conn.DescribeDisks(&ecs.DescribeDisksArgs{ RegionId: getRegion(d, meta), DiskIds: []string{diskID}, }) if descErr != nil { log.Printf("[ERROR] Disk %s is not detached.", diskID) // err can be nil here return resource.NonRetryableError(err) } for _, disk := range disks { if disk.Status != ecs.DiskStatusAvailable { return resource.RetryableError(fmt.Errorf("Detach Disk timeout and got an error: %#v", err)) } } return nil }) ``` Another example (https://github.com/terraform-providers/terraform-provider-aws/blob/75c32b375140813b7994f8031e74b8588a08035a/aws/resource_aws_kms_alias.go#L176-L190): ```go func retryFindKmsAliasByName(conn *kms.KMS, name string) (*kms.AliasListEntry, error) { var resp *kms.AliasListEntry err := resource.Retry(1*time.Minute, func() *resource.RetryError { var err error resp, err = findKmsAliasByName(conn, name, nil) if err != nil { return resource.NonRetryableError(err) } if resp == nil { // err is nil here, so returns nil return resource.RetryableError(err) } return nil }) return resp, err } ``` Another example (https://github.com/terraform-providers/terraform-provider-aws/blob/00909998d919faf5494ab8f6b38241deb1957d99/aws/resource_aws_internet_gateway.go#L55-L65): ```go err = resource.Retry(5*time.Minute, func() *resource.RetryError { igRaw, _, err := IGStateRefreshFunc(conn, d.Id())() if igRaw != nil { return nil } if err == nil { // err is always nil here, so it will never retry return resource.RetryableError(err) } else { return resource.NonRetryableError(err) } }) ``` --- helper/resource/wait.go | 21 +++++++++++++++++---- helper/resource/wait_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 4 deletions(-) diff --git a/helper/resource/wait.go b/helper/resource/wait.go index e56a5155d10..7dede059dd2 100644 --- a/helper/resource/wait.go +++ b/helper/resource/wait.go @@ -1,6 +1,7 @@ package resource import ( + "errors" "sync" "time" ) @@ -66,19 +67,31 @@ type RetryError struct { } // RetryableError is a helper to create a RetryError that's retryable from a -// given error. +// given error. To prevent logic errors, will return an error when passed a +// nil error. func RetryableError(err error) *RetryError { if err == nil { - return nil + return &RetryError{ + Err: errors.New("empty retryable error received. " + + "This is a bug with the Terraform provider and should be " + + "reported as a GitHub issue in the provider repository."), + Retryable: false, + } } return &RetryError{Err: err, Retryable: true} } // NonRetryableError is a helper to create a RetryError that's _not_ retryable -// from a given error. +// from a given error. To prevent logic errors, will return an error when +// passed a nil error. func NonRetryableError(err error) *RetryError { if err == nil { - return nil + return &RetryError{ + Err: errors.New("empty non-retryable error received. " + + "This is a bug with the Terraform provider and should be " + + "reported as a GitHub issue in the provider repository."), + Retryable: false, + } } return &RetryError{Err: err, Retryable: false} } diff --git a/helper/resource/wait_test.go b/helper/resource/wait_test.go index 526b21ae3bb..f1a51c75f2f 100644 --- a/helper/resource/wait_test.go +++ b/helper/resource/wait_test.go @@ -93,3 +93,29 @@ func TestRetry_error(t *testing.T) { t.Fatal("timeout") } } + +func TestRetry_nilNonRetryableError(t *testing.T) { + t.Parallel() + + f := func() *RetryError { + return NonRetryableError(nil) + } + + err := Retry(1*time.Second, f) + if err == nil { + t.Fatal("should error") + } +} + +func TestRetry_nilRetryableError(t *testing.T) { + t.Parallel() + + f := func() *RetryError { + return RetryableError(nil) + } + + err := Retry(1*time.Second, f) + if err == nil { + t.Fatal("should error") + } +}