Conversation
| return handler.ProgressEvent{ | ||
| OperationStatus: handler.Failed, | ||
| Message: message, | ||
| HandlerErrorCode: string(types.HandlerErrorCodeNotFound)} |
There was a problem hiding this comment.
wrong mapping, and already handled in getHandlerErrorCode
| if resp != nil && resp.StatusCode == http.StatusBadRequest && strings.Contains(err.Error(), constants.Duplicate) { | ||
| pe.HandlerErrorCode = string(types.HandlerErrorCodeAlreadyExists) | ||
| } | ||
| if resp != nil && resp.StatusCode == http.StatusNotFound { |
There was a problem hiding this comment.
redundant as it's already done in GetFailedEventByResponse at the beginning of the func
| } | ||
|
|
||
| if response.StatusCode == http.StatusUnauthorized { | ||
| return handler.ProgressEvent{ |
There was a problem hiding this comment.
wrong mapping, and already handled in getHandlerErrorCode
| return string(types.HandlerErrorCodeInvalidRequest) | ||
| case http.StatusNotFound: | ||
| return string(types.HandlerErrorCodeNotFound) | ||
| case http.StatusConflict: |
There was a problem hiding this comment.
moved logic from GetFailedEventByResponse for StatusConflict
This reverts commit bec2d18.
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect HTTP status code to CloudFormation error code mappings that caused stacks to be deleted incorrectly when clusters had termination protection enabled. The fix consolidates error mapping logic into a single function and removes duplicate/incorrect mappings.
Changes:
- Consolidated HTTP status code to error code mapping logic into
getHandlerErrorCodefunction - Removed duplicate error handling code that incorrectly mapped
BadRequestandUnauthorizedtoNotFound - Removed redundant
NotFoundstatus code check in cluster error handling
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| cfn-resources/util/progressevent/failed_event.go | Consolidated error code mapping logic into getHandlerErrorCode and removed duplicate mapping code from GetFailedEventByResponse |
| cfn-resources/util/cluster_common.go | Removed redundant NotFound status code check that duplicated logic now in getHandlerErrorCode |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
marcosuma
left a comment
marcosuma
left a comment
There was a problem hiding this comment.
How should we interpret this? it seems a bug fix so ok to minor version I guess?
@marcosuma yes, it's a bug in the error type we return in some situations |
Proposed changes
Fix error code mapping to address HELP-88769. Note that this affects all CFN resources, not only cluster. Cluster E2E testings are passing, link.
CFN stack was being deleted successfully incorrectly for a cluster with termination protection because:
NotFounderror is returned, CFN assumes it's already deleted and that's okCANNOT_TERMINATE_CLUSTER_WHEN_TERMINATION_PROTECTION_ENABLEDAtlas error is400 Bad requestand is mapped incorrectly toNotFoundJira ticket: CLOUDP-380707
Type of change:
expected)
Manual QA performed:
Required Checklist:
make fmtand formatted my codeworks in Atlas
Further comments
Before (CFN stack deleted incorrectly):
In this PR (CFN stack deletion fails):