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.

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

S3 headIncompletePartForUpload function error exclusions seem insufficient #1010

Closed
jimydavis opened this issue Sep 28, 2023 · 6 comments · Fixed by #1019
Closed

S3 headIncompletePartForUpload function error exclusions seem insufficient #1010

jimydavis opened this issue Sep 28, 2023 · 6 comments · Fixed by #1019
Labels

Comments

@jimydavis
Copy link
Contributor

jimydavis commented Sep 28, 2023

In s3store.go, there's a number of errors that are ignored as below. I presume this is for fresh new uploads that have no incomplete parts:

func (store S3Store) headIncompletePartForUpload(ctx context.Context, uploadId string) (int64, error) {

...

  if err != nil {
    if isAwsError[*types.NoSuchKey](err) || isAwsError[*types.NotFound](err) || isAwsErrorCode(err, "AccessDenied") {
      err = nil
    }
    return 0, err
  }

However, yesterday on testing a fresh upload (no parts uploaded yet, completely fresh), I received

status=500 body="ERR_INTERNAL_SERVER_ERROR: operation error S3: HeadObject, https response error StatusCode: 403, RequestID: xxxxxxx, HostID: uploadID+multipartID, api error Forbidden: Forbidden\n"

In my debugger, the error looked like this:

image

Do we have to exclude Forbidden for fresh uploads without incomplete parts too or am I missing something?

Upon reading the AWS error codes documentation, Forbidden is not an error code. Only AccessDenied is. But as you can see from my debugger screenshot, I am indeed getting the Forbidden code from what presumably must be from AWS's servers and not within any of the client code.

Lastly, many people should encounter this error as incomplete parts are not always going to be there and when you do a HEAD on it, its going to come back empty.

Thank you.

edit: this seems to affect getIncompletePartForUpload too.

@Acconut
Copy link
Member

Acconut commented Sep 28, 2023

Do you have allowed all of these actions?

// s3:AbortMultipartUpload
// s3:DeleteObject
// s3:GetObject
// s3:ListMultipartUploadParts
// s3:PutObject

I don't know the exact details but depending on the permissions AWS S3 either returns a 404 Not Found or 403 Forbidden error for non-existing objects. It could also be that we are missing a permission in the above documentation.

The current code only handles the 404 case. We should also handle 403 better while also checking the documentation for missing permissions.

@jimydavis
Copy link
Contributor Author

jimydavis commented Sep 28, 2023

Yes I have all these permissions. Everything else works fine except when it tries to HEAD or GET .part files which don't exist yet for a fresh upload.

In fact after extensive testing, I have no idea when and when a .part file will appear. Sometimes an actual .part file appears in the S3 console browser. Other times, it goes through the shadow multi-part system which does not show up on the S3 GUI console. In both cases, I have seen the upload complete successfully.

Adding || isAwsErrorCode(err, "Forbidden") just makes the error go away. Full code below:

if err != nil && (isAwsError[*types.NoSuchKey](err) || isAwsError[*types.NotFound](err) || isAwsErrorCode(err, "AccessDenied")) || isAwsErrorCode(err, "Forbidden") {
  return nil, nil
}

In the AWS error code documentation: https://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html , you can see Forbidden is not an error code. It is just a HTTP status.

@jimydavis
Copy link
Contributor Author

@Acconut Hello! Wondering if you thought this has any merit for an addition of the Forbidden error type? Thanks! Congrats on launching 2.0.0 🥳

@Acconut
Copy link
Member

Acconut commented Oct 17, 2023

In fact after extensive testing, I have no idea when and when a .part file will appear. Sometimes an actual .part file appears in the S3 console browser.

The .part files are sometimes created when an upload is paused in the middle of the transfer. S3 has a minimum part size for its multipart uploads and if the amount of remaining data is smaller than this minimum part size, we store it in a .part file, which will be consumed when the upload is resumed.

Wondering if you thought this has any merit for an addition of the Forbidden error type?

Yes, that would be a sensible change. Feel free to open a PR for it. And thanks for the kind words :)

@jimydavis
Copy link
Contributor Author

Created a pull request here: #1019 - if anyone has friends at AWS, it would help to know why AWS returns "Forbidden" as an Error Code when the documentation does not mention it: https://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html

@Acconut
Copy link
Member

Acconut commented Oct 24, 2023

Thank you for the PR, @jimydavis. I hope this resolve the issue. If not, please let me know and we will reopen this.

it would help to know why AWS returns "Forbidden" as an Error Code when the documentation does not mention it

One reason could be that Forbidden is not an S3-specific error but a general IAM error and thus not included in this list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants