Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for async deletion of non-existent resource #3980

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
+57 −27
Open

Fix for async deletion of non-existent resource #3980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fba1675
Fix for async deletion of non-existent resource
EronWright Feb 21, 2025
20a090b
refactored
EronWright Feb 21, 2025
c66fe83
bugfix
EronWright Feb 21, 2025
7842399
fix tests to use real error bodies
EronWright Feb 21, 2025
4fdc181
bugfix
EronWright Feb 21, 2025
3c82a9a
consise error for async putOrPatch
EronWright Feb 21, 2025
96e9bfd
simplify
EronWright Feb 21, 2025
8ef8cbc
more unit tests
EronWright Feb 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 27 additions & 23 deletions provider/pkg/azure/client_azcore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,34 +202,35 @@ func (c *azCoreClient) Delete(ctx context.Context, id, apiVersion, asyncStyle st
return err
}

resp, err := c.pipeline.Do(req)
if err != nil {
return err
}

// 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 {
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 !runtime.HasStatusCode(resp, http.StatusOK, http.StatusAccepted, http.StatusNoContent) {
return runtime.NewResponseError(resp)
}
return err
}

if !runtime.HasStatusCode(resp, http.StatusOK, http.StatusAccepted, http.StatusNoContent, http.StatusNotFound) {
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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thomas11 , I notice that normalizeLocationHeader isn't being called here, as is in putOrPatch.
Given this discussion, it probably doesn't matter, but I bring it up in case you'd like me to act on that.

if err != nil {
return err
}
_, err = pt.PollUntilDone(ctx, &runtime.PollUntilDoneOptions{
Frequency: time.Duration(c.deletePollingIntervalSeconds * int64(time.Second)),
})
return err
}
return nil
}()
if err, ok := err.(*azcore.ResponseError); ok {
if err.StatusCode == http.StatusNotFound {
// If the resource is already deleted, we don't want to return an error.
return nil
}
return newResponseError(err.RawResponse)
}
return err
}
Expand Down Expand Up @@ -297,6 +298,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)
}
Comment on lines +301 to +303
Copy link
Contributor Author

@EronWright EronWright Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix is actually a follow-up to #3783. I think it is justifiably in here because the Delete call is being similarly fixed here.

return nil, created, err
}
}
Expand Down
34 changes: 30 additions & 4 deletions provider/pkg/azure/client_azcore_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,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"}`)),
Copy link
Contributor Author

@EronWright EronWright Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we fake an error response, it is good to use the error schema because you can make assertions about the final error code.

Body: io.NopCloser(strings.NewReader(`{"error": {"code": "Unavailable"}}`)),
},
{
StatusCode: 200,
Expand All @@ -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{
{
Expand All @@ -438,13 +449,26 @@ 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)
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{
{
Expand All @@ -455,16 +479,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)
})
}

Expand Down
Loading