Skip to content

Fix for async deletion of non-existent resource#3980

Merged
EronWright merged 8 commits into
masterfrom
issue-3978
Feb 25, 2025
Merged

Fix for async deletion of non-existent resource#3980
EronWright merged 8 commits into
masterfrom
issue-3978

Conversation

@EronWright

@EronWright EronWright commented Feb 21, 2025

Copy link
Copy Markdown
Contributor

This PR fixes an error that occurs in the deletion of a (non-existent) Azure resource. The issue impacts resources that have an async-style DELETE call (as is the case for Subnet, see schema). The technical detail is that the 404 may come in the initial response as opposed to during polling, and that wasn't handled correctly..

The user-facing error is:

  azure-native:network:Subnet (subnet):
    error: the operation failed or was cancelled

The call to runtime.NewPoller should not be made with a failed response, otherwise we hit a precondition within it:

	// this is a back-stop in case the swagger is incorrect (i.e. missing one or more status codes for success).
	// ideally the codegen should return an error if the initial response failed and not even create a poller.
	if !poller.StatusCodeValid(resp) {
		return nil, errors.New("the operation failed or was cancelled")
	}

To fix, I encapsulated the request-to-final-response logic into an anonymous function, to be able to uniformly check for the 404 result. And I ensure that the initial response was successful before making a poller.

This PR relaxes the logic for detecting a 404 during resource deletion, to not care about the specific error code. I find that at least two codes are possible: ResourceNotFound and ResourceGroupNotFound. Note that the sync path did not check the error code anyway and a uniform solution is desirable.

TODO:

  • new unit tests
  • manual testing for various types of resources (w/ sync and async delete operations).

Closes #3978
See also: #3783

@EronWright EronWright requested a review from thomas11 February 21, 2025 19:57
@github-actions

Copy link
Copy Markdown
Contributor

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Comment thread provider/pkg/azure/client_azcore.go
Comment on lines +302 to +303
if err, ok := err.(*azcore.ResponseError); ok {
return nil, created, newResponseError(err.RawResponse)
}

@EronWright EronWright Feb 21, 2025

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nice catch!

{
StatusCode: 503, // temporary failure
Header: http.Header{"Location": []string{"https://management.azure.com/operation"}},
Body: io.NopCloser(strings.NewReader(`{"status": "Unavailable"}`)),

@EronWright EronWright Feb 21, 2025

Copy link
Copy Markdown
Contributor Author

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.

@EronWright EronWright marked this pull request as ready for review February 21, 2025 23:42
@codecov

codecov Bot commented Feb 22, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 73.91304% with 6 lines in your changes missing coverage. Please review.

Project coverage is 57.47%. Comparing base (b512293) to head (4fd3fc6).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
provider/pkg/azure/client_azcore.go 73.91% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3980      +/-   ##
==========================================
- Coverage   57.49%   57.47%   -0.03%     
==========================================
  Files          82       82              
  Lines       13078    13081       +3     
==========================================
- Hits         7519     7518       -1     
- Misses       4984     4987       +3     
- Partials      575      576       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment thread provider/pkg/azure/client_azcore.go
Comment on lines +302 to +303
if err, ok := err.(*azcore.ResponseError); ok {
return nil, created, newResponseError(err.RawResponse)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nice catch!

Comment thread provider/pkg/azure/client_azcore.go
@pulumi-bot

Copy link
Copy Markdown
Contributor

This PR has been shipped in release v2.89.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to destroy stack containing a deleted subnet

3 participants