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

handler, s3store, filestore: Serve GET requests from storage provider #1228

Merged
merged 14 commits into from
Dec 20, 2024
Merged
Prev Previous commit
Next Next commit
Return proper error for incomplete uploads
Acconut committed Dec 20, 2024
commit eb41cd5f98311d1a75739258b5924d88e0222a8d
5 changes: 4 additions & 1 deletion pkg/s3store/s3store.go
Original file line number Diff line number Diff line change
@@ -100,6 +100,9 @@ import (
// considered valid into a header value according to RFC2616.
var nonPrintableRegexp = regexp.MustCompile(`[^\x09\x20-\x7E]`)

// errIncompleteUpload is used when a client attempts to download an incomplete upload
var errIncompleteUpload = handler.NewError("ERR_INCOMPLETE_UPLOAD", "cannot stream non-finished upload", http.StatusBadRequest)

// See the handler.DataStore interface for documentation about the different
// methods.
type S3Store struct {
@@ -759,7 +762,7 @@ func (upload s3Upload) GetReader(ctx context.Context) (io.ReadCloser, error) {
})
if err == nil {
// The multipart upload still exists, which means we cannot download it yet
return nil, handler.NewError("ERR_INCOMPLETE_UPLOAD", "cannot stream non-finished upload", http.StatusBadRequest)
return nil, errIncompleteUpload
}

// The AWS Go SDK v2 has a bug where types.NoSuchUpload is not returned,
7 changes: 4 additions & 3 deletions pkg/s3store/serve_content.go
Original file line number Diff line number Diff line change
@@ -20,8 +20,6 @@ func (store S3Store) AsServableUpload(upload handler.Upload) handler.ServableUpl
}

func (su *s3Upload) ServeContent(ctx context.Context, w http.ResponseWriter, r *http.Request) error {
// TODO: If the upload is not yet finished, we don't even have to try to get the object.

input := &s3.GetObjectInput{
Bucket: aws.String(su.store.Bucket),
Key: su.store.keyWithPrefix(su.objectId),
@@ -60,7 +58,10 @@ func (su *s3Upload) ServeContent(ctx context.Context, w http.ResponseWriter, r *
var respErr *awshttp.ResponseError
if errors.As(err, &respErr) {
if respErr.HTTPStatusCode() == http.StatusNotFound || respErr.HTTPStatusCode() == http.StatusForbidden {
return handler.ErrNotFound
// If the object cannot be found, it means that the upload is not yet complete and we cannot serve it.
// At this stage it is not possible that the upload itself does not exist, because the handler
// alredy checked this case. Therefore, we can safely assume that the upload is still in progress.
return errIncompleteUpload
}

if respErr.HTTPStatusCode() == http.StatusNotModified {
4 changes: 2 additions & 2 deletions pkg/s3store/serve_content_test.go
Original file line number Diff line number Diff line change
@@ -144,7 +144,7 @@ func TestS3ServableUploadServeContentInternalError(t *testing.T) {
assert.Equal(expectedError, err)
}

func TestS3ServableUploadServeContentNotFound(t *testing.T) {
func TestS3ServableUploadServeContentIncomplete(t *testing.T) {
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()
assert := assert.New(t)
@@ -174,7 +174,7 @@ func TestS3ServableUploadServeContentNotFound(t *testing.T) {
r := httptest.NewRequest("GET", "/", nil)

err = servableUpload.ServeContent(context.Background(), w, r)
assert.Equal(handler.ErrNotFound, err)
assert.Equal(errIncompleteUpload, err)
}

func TestS3ServableUploadServeContentRangeNotSatisfiable(t *testing.T) {