search

Automattic / hostmgr

A tool for managing macOS VM hosts
Mozilla Public License 2.0
8 stars 3 forks source link

Throw error when signed s3 url returns non-200 status code #70

Closed crazytonyli closed 8 months ago

crazytonyli commented 8 months ago

Issue

URLSessionDataTask only returns error for network issues, but returns successful result for HTTP semantic errors (400, 500, etc.). That means the current download(object:) implementation would return the AWS's JSON content when AWS API throws an error.

This download(object:) function is used to get the mainfest.txt content during cleaning up VM images. If incorrect content is returned by this function, the clean up step will delete all local images.

Here is a test. For some reason, I can't get the async API to execute in Xcode Playground, so I had to use the completion block version, but I doubt they behave differently.

Screenshot 2023-10-24 at 9 56 55 AM

Changes

The fix is simply checking if the HTTP response is 200 OK and throwing an error if it's not.

This change has a small impact on teams pipelines. When AWS S3 API returns non-OK result, which is of course possible but I imagine should be relatively rare, the hostmgr vm cleanup command would fail. We call this command in pre-exit hook, and its failure would cause the running pipeline to fail. A potential fix is we can ignore those errors from our infrastructure side.

crazytonyli commented 8 months ago

Closing this PR since manifest.txt will soon become obsolete.