Clean-up retry logic, extract to utils

azurerm/resource_arm_role_assignment.go

@@ -3,12 +3,10 @@ package azurerm
 import (
 	"fmt"
 	"log"
-	"net/url"
 	"strings"
 	"time"
 
 	"github.com/Azure/azure-sdk-for-go/services/preview/authorization/mgmt/2018-01-01-preview/authorization"
-	"github.com/Azure/go-autorest/autorest"
 	"github.com/hashicorp/go-uuid"
 	"github.com/hashicorp/terraform/helper/resource"
 	"github.com/hashicorp/terraform/helper/schema"
@@ -184,32 +182,18 @@ func validateRoleDefinitionName(i interface{}, k string) ([]string, []error) {
 }
 
 func retryRoleAssignmentsClient(scope string, name string, properties authorization.RoleAssignmentCreateParameters, meta interface{}) func() *resource.RetryError {
-	urlErrorIsRetryable := func(err *url.Error) bool {
-		if err.Temporary() || err.Timeout() {
-			return true
-		}
-		return false
-	}
-
 	return func() *resource.RetryError {
 		roleAssignmentsClient := meta.(*ArmClient).roleAssignmentsClient
 		ctx := meta.(*ArmClient).StopContext
 
 		_, err := roleAssignmentsClient.Create(ctx, scope, name, properties)
 
 		if err != nil {
-			// Error is retryable only if it is a temporary error or timeout
-			switch e := err.(type) {
-			case autorest.DetailedError:
-				if ue, ok := e.Original.(*url.Error); ok && urlErrorIsRetryable(ue) {
-					return resource.RetryableError(err)
-				}
-			case *url.Error:
-				if urlErrorIsRetryable(e) {
-					return resource.RetryableError(err)
-				}
+			if utils.ResponseErrorIsRetryable(err) {
+				return resource.RetryableError(err)
+			} else {
+				return resource.NonRetryableError(err)
 			}
-			return resource.NonRetryableError(err)
 		}
 		return nil
 

azurerm/utils/response.go

@@ -1,6 +1,7 @@
 package utils
 
 import (
+	"net"
 	"net/http"
 
 	"github.com/Azure/go-autorest/autorest"
@@ -14,6 +15,21 @@ func ResponseWasNotFound(resp autorest.Response) bool {
 	return responseWasStatusCode(resp, http.StatusNotFound)
 }
 
+func ResponseErrorIsRetryable(err error) bool {
+	if arerr, ok := err.(autorest.DetailedError); ok {
+		err = arerr.Original
+	}
+
+	switch e := err.(type) {
+	case net.Error:
+		if e.Temporary() || e.Timeout() {
+			return true
+		}
+	}
+
+	return false
+}
+
 func responseWasStatusCode(resp autorest.Response, statusCode int) bool {
 	if r := resp.Response; r != nil {
 		if r.StatusCode == statusCode {

azurerm/utils/response_test.go

@@ -1,6 +1,7 @@
 package utils
 
 import (
+	"fmt"
 	"net/http"
 	"testing"
 
@@ -37,3 +38,40 @@ func TestResponseNotFound_StatusCodes(t *testing.T) {
 		}
 	}
 }
+
+type testNetError struct {
+	timeout   bool
+	temporary bool
+}
+
+// testNetError fulfills net.Error interface
+func (e testNetError) Error() string   { return "testError" }
+func (e testNetError) Timeout() bool   { return e.timeout }
+func (e testNetError) Temporary() bool { return e.temporary }
+
+func TestResponseErrorIsRetryable(t *testing.T) {
+	testCases := []struct {
+		desc           string
+		err            error
+		expectedResult bool
+	}{
+		{"Unhandled error types are not retryable", fmt.Errorf("Some other error"), false},
+		{"Temporary AND timeout errors are retryable", testNetError{true, true}, true},
+		{"Timeout errors are retryable", testNetError{true, false}, true},
+		{"Temporary errors are retryable", testNetError{false, true}, true},
+		{"net.Errors that are neither temporary nor timeouts are not retryable", testNetError{false, false}, false},
+		{"Retryable error nested in autorest.DetailedError is retryable", autorest.DetailedError{
+			Original: testNetError{true, true}}, true},
+		{"Unhandled error nested in autorest.DetailedError is not retryable", autorest.DetailedError{
+			Original: fmt.Errorf("Some other error")}, false},
+		{"nil is handled as non-retryable", nil, false},
+	}
+
+	for _, test := range testCases {
+		result := ResponseErrorIsRetryable(test.err)
+		if test.expectedResult != result {
+			t.Errorf("Expected '%v' for case '%s' - got '%v'",
+				test.expectedResult, test.desc, result)
+		}
+	}
+}