Skip to content
Merged
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions pkg/destroy/aws/aws.go
Original file line number Diff line number Diff line change
Expand Up @@ -1716,6 +1716,11 @@ func deleteRoute53(session *session.Session, arn arn.ARN, logger logrus.FieldLog

sharedZoneID, err := getSharedHostedZone(client, id, logger)
if err != nil {
// In some cases AWS may return the zone in the list of tagged resources despite the fact
// it no longer exists.
if err.(awserr.Error).Code() == route53.ErrCodeNoSuchHostedZone {
return nil
Copy link
Contributor

@patrickdillon patrickdillon Mar 27, 2020

Choose a reason for hiding this comment

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

Should we return nil? I would think that if there is no sharedZone then the resources that are deleted after this would also not exist, but here:

https://github.com/openshift/installer/blob/5df5607169bb8b8c878b8b5cc108d1a6d0fec0d7/pkg/destroy/aws/aws.go#L1747

we proceed even if no sharedZone exists...

Copy link
Contributor

Choose a reason for hiding this comment

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

The not-found error here is for the case where the private zone does not exist. If the private zone exists, but there is no public zone, then getSharedHostedZone will return an empty string and a nil error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying.

As mentioned, this seems like an AWS problem so it is beyond our control to fix.

/lgtm
/approve

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am really unsure, sharedZoneID is used again on line 1761 so that's going to probably fail, line 1770 maybe? I don't know this code or route53 very well, it feels like it's game over if we can't get the shared zone ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops was looking at an old load of the tab. :)

}
return err
}

Expand Down