Fix azure.IsNotFound for new azcore error type (#3842)

Fixes an issue I discovered today when trying to reproduce another issue
report. `pulumi refresh` is broken for this provider:
```
$ PATH="$HOME/pulumi/pan/bin:$PATH" pulumi refresh --skip-preview -s dev

     pulumi:pulumi:Stack                      recoveryservices-protecteditem-dev  **failed**                1 error; 4 warnings
 ~   ├─ azure-native:resources:ResourceGroup  resourceGroup                       **refreshing failed**     1 error

  azure-native:resources:ResourceGroup (resourceGroup):
    error: Code="ResourceGroupNotFound" Message="Resource group 'resourceGroup3f871416' could not be found."
```

The reason is that #3783 regressed `azure.IsNotFound`, causing `Read` to
report an error when a resource doesn't exist in Azure.

This PR fixes the problem and adds test coverage.

provider/pkg/azure/azure.go

@@ -49,6 +49,11 @@ func IsNotFound(err error) bool {
 	if responseError, ok := err.(*azcore.ResponseError); ok {
 		return responseError.StatusCode == http.StatusNotFound
 	}
+
+	if responseError, ok := err.(*PulumiAzcoreResponseError); ok {
+		return responseError.StatusCode == http.StatusNotFound
+	}
+
 	return false
 }
 

provider/pkg/azure/azure_test.go

@@ -3,9 +3,13 @@
 package azure
 
 import (
+	"net/http"
 	"testing"
 
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud"
+	"github.com/Azure/go-autorest/autorest"
+	autorestAzure "github.com/Azure/go-autorest/autorest/azure"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -23,3 +27,38 @@ func TestGetCloudByName(t *testing.T) {
 		assert.Equal(t, tc.expected, GetCloudByName(tc.name), tc.name)
 	}
 }
+
+func TestIsNotFound(t *testing.T) {
+	t.Run("autorest", func(t *testing.T) {
+		assert.True(t, IsNotFound(&autorestAzure.RequestError{
+			DetailedError: autorest.DetailedError{
+				StatusCode: http.StatusNotFound,
+			},
+		}))
+		assert.False(t, IsNotFound(&autorestAzure.RequestError{
+			DetailedError: autorest.DetailedError{
+				StatusCode: http.StatusForbidden,
+			},
+		}))
+	})
+
+	t.Run("azcore", func(t *testing.T) {
+		assert.True(t, IsNotFound(&azcore.ResponseError{
+			StatusCode: http.StatusNotFound,
+		}))
+		assert.False(t, IsNotFound(&azcore.ResponseError{
+			StatusCode: http.StatusForbidden,
+		}))
+	})
+
+	t.Run("provider", func(t *testing.T) {
+		assert.True(t, IsNotFound(&PulumiAzcoreResponseError{
+			StatusCode: http.StatusNotFound,
+			ErrorCode:  "ResourceNotFound",
+		}))
+		assert.False(t, IsNotFound(&PulumiAzcoreResponseError{
+			StatusCode: http.StatusForbidden,
+			ErrorCode:  "Unauthorized",
+		}))
+	})
+}

provider/pkg/azure/client_azcore.go

@@ -529,6 +529,7 @@ func (r *requestAssertingTransporter) Do(req *http.Request) (*http.Response, err
 	return nil, nil
 }
 
+// newResponseError replaces an azcore.ResponseError, created from the given response, with a more concise error type (#3778).
 func newResponseError(resp *http.Response) error {
 	err := runtime.NewResponseError(resp)
 	azcoreErr, ok := err.(*azcore.ResponseError)
@@ -553,12 +554,23 @@ func newResponseError(resp *http.Response) error {
 		}
 	}
 
-	errCode := azcoreErr.ErrorCode
-	if errCode == "" {
-		errCode = fmt.Sprintf("%d", resp.StatusCode)
+	return &PulumiAzcoreResponseError{
+		StatusCode: resp.StatusCode,
+		ErrorCode:  azcoreErr.ErrorCode,
+		Message:    errMsg,
 	}
+}
+
+// We use this error type instead of azcore.ResponseError because the latter has a very verbose error message (#3778).
+type PulumiAzcoreResponseError struct {
+	StatusCode int
+	ErrorCode  string
+	Message    string
+}
 
-	// The capitalized message replicates the error message format of the previous autorest client.
-	//nolint:ST1005
-	return fmt.Errorf(`Code="%s" Message="%s"`, errCode, errMsg)
+func (e *PulumiAzcoreResponseError) Error() string {
+	if e.ErrorCode == "" {
+		return fmt.Sprintf(`Status=%d Message="%s"`, e.StatusCode, e.Message)
+	}
+	return fmt.Sprintf(`Status=%d Code="%s" Message="%s"`, e.StatusCode, e.ErrorCode, e.Message)
 }

provider/pkg/azure/client_azcore_test.go

@@ -781,7 +781,7 @@ func TestNewResponseError(t *testing.T) {
 		}
 		err := newResponseError(resp)
 		require.Error(t, err)
-		assert.Equal(t, "Code=\"Conflict\" Message=\"Foo already exists\"", err.Error())
+		assert.Equal(t, "Status=409 Code=\"Conflict\" Message=\"Foo already exists\"", err.Error())
 	})
 
 	t.Run("no message", func(t *testing.T) {
@@ -792,7 +792,7 @@ func TestNewResponseError(t *testing.T) {
 		}
 		err := newResponseError(resp)
 		require.Error(t, err)
-		assert.Equal(t, `Code="Conflict" Message="{"error": {"code": "Conflict"}}"`, err.Error())
+		assert.Equal(t, `Status=409 Code="Conflict" Message="{"error": {"code": "Conflict"}}"`, err.Error())
 	})
 
 	t.Run("unknown error", func(t *testing.T) {
@@ -802,6 +802,14 @@ func TestNewResponseError(t *testing.T) {
 		}
 		err := newResponseError(resp)
 		require.Error(t, err)
-		assert.Equal(t, `Code="409" Message="{"foo": "bar"}"`, err.Error())
+		assert.Equal(t, `Status=409 Message="{"foo": "bar"}"`, err.Error())
+	})
+
+	t.Run("404 is recognized by IsNotFound", func(t *testing.T) {
+		resp := &http.Response{
+			StatusCode: 404,
+			Body:       io.NopCloser(strings.NewReader(`not found`)),
+		}
+		assert.True(t, IsNotFound(newResponseError(resp)))
 	})
 }