From fba1675c8d7c5173ae930db8b989f45cac421575 Mon Sep 17 00:00:00 2001 From: Eron Wright Date: Fri, 21 Feb 2025 11:56:08 -0800 Subject: [PATCH 1/8] Fix for async deletion of non-existent resource --- provider/pkg/azure/client_azcore.go | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/provider/pkg/azure/client_azcore.go b/provider/pkg/azure/client_azcore.go index d5b37a7ab9cb..63a9f44f002e 100644 --- a/provider/pkg/azure/client_azcore.go +++ b/provider/pkg/azure/client_azcore.go @@ -206,6 +206,13 @@ func (c *azCoreClient) Delete(ctx context.Context, id, apiVersion, asyncStyle st if err != nil { return err } + if runtime.HasStatusCode(resp, http.StatusNotFound) { + // If the resource is already deleted, we don't want to return an error. + return nil + } + if !runtime.HasStatusCode(resp, http.StatusOK, http.StatusAccepted, http.StatusNoContent) { + return newResponseError(resp) + } // Some APIs are explicitly marked `x-ms-long-running-operation` and we should only do the // poll for the deletion result in that case. @@ -218,14 +225,16 @@ func (c *azCoreClient) Delete(ctx context.Context, id, apiVersion, asyncStyle st Frequency: time.Duration(c.deletePollingIntervalSeconds * int64(time.Second)), }) if err != nil { - respErr := err.(*azcore.ResponseError) - if resp.StatusCode == 202 && respErr.StatusCode == 404 && strings.Contains(respErr.Error(), "ResourceNotFound") { - // Consider this specific error to be a success of deletion. - // Work around https://github.com/pulumi/pulumi-azure-nextgen/issues/120 - return nil + if respErr, ok := err.(*azcore.ResponseError); ok { + // If the resource is already deleted, we don't want to return an error. + if respErr.StatusCode == http.StatusNotFound && respErr.ErrorCode == "ResourceNotFound" { + // If the resource is already deleted, we don't want to return an error. + return nil + } } + return err } - return err + return nil } if !runtime.HasStatusCode(resp, http.StatusOK, http.StatusAccepted, http.StatusNoContent, http.StatusNotFound) { From 20a090b415facbbd4a95a66bd84d4dfc05c1dc1d Mon Sep 17 00:00:00 2001 From: Eron Wright Date: Fri, 21 Feb 2025 13:26:41 -0800 Subject: [PATCH 2/8] refactored --- provider/pkg/azure/client_azcore.go | 54 +++++++++++++---------------- 1 file changed, 24 insertions(+), 30 deletions(-) diff --git a/provider/pkg/azure/client_azcore.go b/provider/pkg/azure/client_azcore.go index 63a9f44f002e..6047538ad999 100644 --- a/provider/pkg/azure/client_azcore.go +++ b/provider/pkg/azure/client_azcore.go @@ -202,45 +202,39 @@ func (c *azCoreClient) Delete(ctx context.Context, id, apiVersion, asyncStyle st return err } - resp, err := c.pipeline.Do(req) - if err != nil { - return err - } - if runtime.HasStatusCode(resp, http.StatusNotFound) { - // If the resource is already deleted, we don't want to return an error. - return nil - } - if !runtime.HasStatusCode(resp, http.StatusOK, http.StatusAccepted, http.StatusNoContent) { - return newResponseError(resp) - } - - // Some APIs are explicitly marked `x-ms-long-running-operation` and we should only do the - // poll for the deletion result in that case. - if asyncStyle != "" { - pt, err := runtime.NewPoller[any](resp, c.pipeline, nil) + err = func() error { + resp, err := c.pipeline.Do(req) if err != nil { return err } - _, err = pt.PollUntilDone(ctx, &runtime.PollUntilDoneOptions{ - Frequency: time.Duration(c.deletePollingIntervalSeconds * int64(time.Second)), - }) - if err != nil { - if respErr, ok := err.(*azcore.ResponseError); ok { - // If the resource is already deleted, we don't want to return an error. - if respErr.StatusCode == http.StatusNotFound && respErr.ErrorCode == "ResourceNotFound" { - // If the resource is already deleted, we don't want to return an error. - return nil - } + if !runtime.HasStatusCode(resp, http.StatusOK, http.StatusAccepted, http.StatusNoContent) { + return newResponseError(resp) + } + + // Some APIs are explicitly marked `x-ms-long-running-operation` and we should only do the + // poll for the deletion result in that case. + if asyncStyle != "" { + pt, err := runtime.NewPoller[any](resp, c.pipeline, nil) + if err != nil { + return err } + _, err = pt.PollUntilDone(ctx, &runtime.PollUntilDoneOptions{ + Frequency: time.Duration(c.deletePollingIntervalSeconds * int64(time.Second)), + }) return err } + return nil - } + }() - if !runtime.HasStatusCode(resp, http.StatusOK, http.StatusAccepted, http.StatusNoContent, http.StatusNotFound) { - return newResponseError(resp) + if err, ok := err.(*azcore.ResponseError); ok { + if err.StatusCode == http.StatusNotFound && err.ErrorCode == "ResourceNotFound" { + // If the resource is already deleted, we don't want to return an error. + return nil + } } - return err + + return nil } func (c *azCoreClient) Put(ctx context.Context, id string, bodyProps map[string]interface{}, From c66fe831ba0642b0e8bc8ef8025bd49334685d06 Mon Sep 17 00:00:00 2001 From: Eron Wright Date: Fri, 21 Feb 2025 13:51:55 -0800 Subject: [PATCH 3/8] bugfix --- provider/pkg/azure/client_azcore.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/provider/pkg/azure/client_azcore.go b/provider/pkg/azure/client_azcore.go index 6047538ad999..22defd516702 100644 --- a/provider/pkg/azure/client_azcore.go +++ b/provider/pkg/azure/client_azcore.go @@ -226,15 +226,13 @@ func (c *azCoreClient) Delete(ctx context.Context, id, apiVersion, asyncStyle st return nil }() - if err, ok := err.(*azcore.ResponseError); ok { if err.StatusCode == http.StatusNotFound && err.ErrorCode == "ResourceNotFound" { // If the resource is already deleted, we don't want to return an error. return nil } } - - return nil + return err } func (c *azCoreClient) Put(ctx context.Context, id string, bodyProps map[string]interface{}, From 78423992c70dcdf5ba51bb105eddc84562399073 Mon Sep 17 00:00:00 2001 From: Eron Wright Date: Fri, 21 Feb 2025 15:22:55 -0800 Subject: [PATCH 4/8] fix tests to use real error bodies --- provider/pkg/azure/client_azcore_test.go | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/provider/pkg/azure/client_azcore_test.go b/provider/pkg/azure/client_azcore_test.go index b378980ff69f..6494c260b354 100644 --- a/provider/pkg/azure/client_azcore_test.go +++ b/provider/pkg/azure/client_azcore_test.go @@ -312,13 +312,24 @@ func TestErrorStatusCodes(t *testing.T) { }) t.Run("DELETE ok", func(t *testing.T) { - for _, statusCode := range []int{200, 202, 204, 404} { + for _, statusCode := range []int{200, 202, 204} { client := newClientWithPreparedResponses([]*http.Response{{StatusCode: statusCode}}) err := client.Delete(context.Background(), "/subscriptions/123", "2022-09-01", "", nil) require.NoError(t, err, statusCode) } }) + t.Run("DELETE ResourceNotFound", func(t *testing.T) { + client := newClientWithPreparedResponses([]*http.Response{ + { + StatusCode: 404, + Body: io.NopCloser(strings.NewReader(`{"error": {"code": "ResourceNotFound"}}`)), + }, + }) + err := client.Delete(context.Background(), "/subscriptions/123", "2022-09-01", "", nil) + require.NoError(t, err) + }) + t.Run("DELETE error", func(t *testing.T) { for _, statusCode := range []int{201, 400, 401, 403, 409, 410, 500, 502, 503, 504} { client := newClientWithPreparedResponses([]*http.Response{{StatusCode: statusCode}}) @@ -418,7 +429,7 @@ func TestErrorStatusCodes(t *testing.T) { { StatusCode: 503, // temporary failure Header: http.Header{"Location": []string{"https://management.azure.com/operation"}}, - Body: io.NopCloser(strings.NewReader(`{"status": "Unavailable"}`)), + Body: io.NopCloser(strings.NewReader(`{"error": {"code": "Unavailable"}}`)), }, { StatusCode: 200, @@ -438,7 +449,7 @@ func TestErrorStatusCodes(t *testing.T) { }, { StatusCode: 404, - Body: io.NopCloser(strings.NewReader(`{"error": "ResourceNotFound"}`)), + Body: io.NopCloser(strings.NewReader(`{"error": {"code": "ResourceNotFound"}}`)), }, }) err := client.Delete(context.Background(), "/subscriptions/123/rg/rg", "2022-09-01", "the actual value doesn't matter!", nil) @@ -455,16 +466,18 @@ func TestErrorStatusCodes(t *testing.T) { { StatusCode: 503, // temporary failure Header: http.Header{"Location": []string{"https://management.azure.com/operation"}}, - Body: io.NopCloser(strings.NewReader(`{"status": "Unavailable"}`)), + Body: io.NopCloser(strings.NewReader(`{"error": {"code": "Unavailable"}}`)), }, { StatusCode: 400, Header: http.Header{"Location": []string{"https://management.azure.com/operation"}}, - Body: io.NopCloser(strings.NewReader(`{"status": "Unavailable"}`)), + Body: io.NopCloser(strings.NewReader(`{"error": {"code": "BadRequest"}}`)), }, }) err := client.Delete(context.Background(), "/subscriptions/123/rg/rg", "2022-09-01", "the actual value doesn't matter!", nil) require.Error(t, err) + require.IsType(t, &PulumiAzcoreResponseError{}, err) + require.Equal(t, "BadRequest", err.(*PulumiAzcoreResponseError).ErrorCode) }) } From 4fdc181ac549462df354fdfcd89b25e80d72fd01 Mon Sep 17 00:00:00 2001 From: Eron Wright Date: Fri, 21 Feb 2025 15:24:56 -0800 Subject: [PATCH 5/8] bugfix --- provider/pkg/azure/client_azcore.go | 3 ++- provider/pkg/azure/client_azcore_test.go | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/provider/pkg/azure/client_azcore.go b/provider/pkg/azure/client_azcore.go index 22defd516702..4e17bb0f4f8a 100644 --- a/provider/pkg/azure/client_azcore.go +++ b/provider/pkg/azure/client_azcore.go @@ -208,7 +208,7 @@ func (c *azCoreClient) Delete(ctx context.Context, id, apiVersion, asyncStyle st return err } if !runtime.HasStatusCode(resp, http.StatusOK, http.StatusAccepted, http.StatusNoContent) { - return newResponseError(resp) + return runtime.NewResponseError(resp) } // Some APIs are explicitly marked `x-ms-long-running-operation` and we should only do the @@ -231,6 +231,7 @@ func (c *azCoreClient) Delete(ctx context.Context, id, apiVersion, asyncStyle st // If the resource is already deleted, we don't want to return an error. return nil } + return newResponseError(err.RawResponse) } return err } diff --git a/provider/pkg/azure/client_azcore_test.go b/provider/pkg/azure/client_azcore_test.go index 6494c260b354..d068c07183d9 100644 --- a/provider/pkg/azure/client_azcore_test.go +++ b/provider/pkg/azure/client_azcore_test.go @@ -331,7 +331,7 @@ func TestErrorStatusCodes(t *testing.T) { }) t.Run("DELETE error", func(t *testing.T) { - for _, statusCode := range []int{201, 400, 401, 403, 409, 410, 500, 502, 503, 504} { + for _, statusCode := range []int{201, 400, 401, 403, 404, 409, 410, 500, 502, 503, 504} { client := newClientWithPreparedResponses([]*http.Response{{StatusCode: statusCode}}) err := client.Delete(context.Background(), "/subscriptions/123", "2022-09-01", "", nil) require.Error(t, err, statusCode) From 3c82a9a6f416e07fc40ddd06f6b959a9065de01e Mon Sep 17 00:00:00 2001 From: Eron Wright Date: Fri, 21 Feb 2025 15:29:14 -0800 Subject: [PATCH 6/8] consise error for async putOrPatch --- provider/pkg/azure/client_azcore.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/provider/pkg/azure/client_azcore.go b/provider/pkg/azure/client_azcore.go index 4e17bb0f4f8a..ed23810a69b1 100644 --- a/provider/pkg/azure/client_azcore.go +++ b/provider/pkg/azure/client_azcore.go @@ -299,6 +299,9 @@ func (c *azCoreClient) putOrPatch(ctx context.Context, method string, id string, Frequency: time.Duration(c.updatePollingIntervalSeconds * int64(time.Second)), }) if err != nil { + if err, ok := err.(*azcore.ResponseError); ok { + return nil, created, newResponseError(err.RawResponse) + } return nil, created, err } } From 96e9bfda8b4a5aa61279f20267b8bfb4ddfb0a95 Mon Sep 17 00:00:00 2001 From: Eron Wright Date: Fri, 21 Feb 2025 15:52:15 -0800 Subject: [PATCH 7/8] simplify --- provider/pkg/azure/client_azcore.go | 3 +-- provider/pkg/azure/client_azcore_test.go | 15 ++------------- 2 files changed, 3 insertions(+), 15 deletions(-) diff --git a/provider/pkg/azure/client_azcore.go b/provider/pkg/azure/client_azcore.go index ed23810a69b1..fb9835d1b431 100644 --- a/provider/pkg/azure/client_azcore.go +++ b/provider/pkg/azure/client_azcore.go @@ -223,11 +223,10 @@ func (c *azCoreClient) Delete(ctx context.Context, id, apiVersion, asyncStyle st }) return err } - return nil }() if err, ok := err.(*azcore.ResponseError); ok { - if err.StatusCode == http.StatusNotFound && err.ErrorCode == "ResourceNotFound" { + if err.StatusCode == http.StatusNotFound { // If the resource is already deleted, we don't want to return an error. return nil } diff --git a/provider/pkg/azure/client_azcore_test.go b/provider/pkg/azure/client_azcore_test.go index d068c07183d9..cd1ec0269ed0 100644 --- a/provider/pkg/azure/client_azcore_test.go +++ b/provider/pkg/azure/client_azcore_test.go @@ -312,26 +312,15 @@ func TestErrorStatusCodes(t *testing.T) { }) t.Run("DELETE ok", func(t *testing.T) { - for _, statusCode := range []int{200, 202, 204} { + for _, statusCode := range []int{200, 202, 204, 404} { client := newClientWithPreparedResponses([]*http.Response{{StatusCode: statusCode}}) err := client.Delete(context.Background(), "/subscriptions/123", "2022-09-01", "", nil) require.NoError(t, err, statusCode) } }) - t.Run("DELETE ResourceNotFound", func(t *testing.T) { - client := newClientWithPreparedResponses([]*http.Response{ - { - StatusCode: 404, - Body: io.NopCloser(strings.NewReader(`{"error": {"code": "ResourceNotFound"}}`)), - }, - }) - err := client.Delete(context.Background(), "/subscriptions/123", "2022-09-01", "", nil) - require.NoError(t, err) - }) - t.Run("DELETE error", func(t *testing.T) { - for _, statusCode := range []int{201, 400, 401, 403, 404, 409, 410, 500, 502, 503, 504} { + for _, statusCode := range []int{201, 400, 401, 403, 409, 410, 500, 502, 503, 504} { client := newClientWithPreparedResponses([]*http.Response{{StatusCode: statusCode}}) err := client.Delete(context.Background(), "/subscriptions/123", "2022-09-01", "", nil) require.Error(t, err, statusCode) From 8ef8cbc06f57f304ee1fb573f46e4a83b24f0855 Mon Sep 17 00:00:00 2001 From: Eron Wright Date: Fri, 21 Feb 2025 16:14:12 -0800 Subject: [PATCH 8/8] more unit tests --- provider/pkg/azure/client_azcore_test.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/provider/pkg/azure/client_azcore_test.go b/provider/pkg/azure/client_azcore_test.go index cd1ec0269ed0..f43d359ecead 100644 --- a/provider/pkg/azure/client_azcore_test.go +++ b/provider/pkg/azure/client_azcore_test.go @@ -429,6 +429,17 @@ func TestErrorStatusCodes(t *testing.T) { require.NoError(t, err) }) + t.Run("DELETE initial success on 404 when asyncStyle is given", func(t *testing.T) { + client := newClientWithPreparedResponses([]*http.Response{ + { + StatusCode: 404, + Body: io.NopCloser(strings.NewReader(`{"error": {"code": "ResourceNotFound"}}`)), + }, + }) + err := client.Delete(context.Background(), "/subscriptions/123/rg/rg", "2022-09-01", "the actual value doesn't matter!", nil) + require.NoError(t, err) + }) + t.Run("DELETE polling success on 404 when asyncStyle is given", func(t *testing.T) { client := newClientWithPreparedResponses([]*http.Response{ { @@ -445,6 +456,19 @@ func TestErrorStatusCodes(t *testing.T) { require.NoError(t, err) }) + t.Run("DELETE initial failure when asyncStyle is given", func(t *testing.T) { + client := newClientWithPreparedResponses([]*http.Response{ + { + StatusCode: 400, + Body: io.NopCloser(strings.NewReader(`{"error": {"code": "BadRequest"}}`)), + }, + }) + err := client.Delete(context.Background(), "/subscriptions/123/rg/rg", "2022-09-01", "the actual value doesn't matter!", nil) + require.Error(t, err) + require.IsType(t, &PulumiAzcoreResponseError{}, err) + require.Equal(t, "BadRequest", err.(*PulumiAzcoreResponseError).ErrorCode) + }) + t.Run("DELETE polling failure when asyncStyle is given", func(t *testing.T) { client := newClientWithPreparedResponses([]*http.Response{ {