helper/resource: Require explicit usage of NonRetryableError and Retr…

…yableError

Reference: hashicorp/terraform#17220
Reference: hashicorp/terraform-provider-aws#9779 (comment)
Reference: hashicorp/terraform-provider-aws#9812 (comment)

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

@@ -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}
 }

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")
+	}
+}