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

API upload object without using tmp file #4848

Merged
merged 3 commits into from
Dec 22, 2022
Merged
Show file tree
Hide file tree
Changes from 2 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
52 changes: 41 additions & 11 deletions pkg/api/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"fmt"
"io"
"mime"
"mime/multipart"
"net/http"
"net/mail"
"net/url"
Expand Down Expand Up @@ -2134,21 +2135,50 @@ func (c *Controller) UploadObject(w http.ResponseWriter, r *http.Request, reposi
allowOverwrite = false
}

// write the content
file, handler, err := r.FormFile("content")
if errors.Is(err, http.ErrMissingFile) {
writeError(w, r, http.StatusInternalServerError, fmt.Errorf("multipart uploads missing key 'content': %w", err))
return
}
// read request body parse multipart for "content" and upload the data
mt, p, err := mime.ParseMediaType(r.Header.Get("Content-Type"))
if err != nil {
writeError(w, r, http.StatusInternalServerError, err)
return
}
defer func() { _ = file.Close() }()
contentType := handler.Header.Get("Content-Type")
address := c.PathProvider.NewPath()
blob, err := upload.WriteBlob(ctx, c.BlockAdapter, repo.StorageNamespace, address, file, handler.Size, block.PutOpts{StorageClass: params.StorageClass})
if err != nil {
if !strings.HasPrefix(mt, "multipart/") {
writeError(w, r, http.StatusInternalServerError, http.ErrNotMultipart)
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this works for any multipart Content-Type, perhaps we should limit ourselves to a list of known types? (Package http probably limits itself in some way, so if we do the same would we be safe?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point - I'll limit it to "multipart/form-data". The Go's request implementation support form-data and optional mixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, the current implementation will be dangerous if we ever add another part to UploadObject in swagger.yml. Let's add a comment on its definition not to do that!

Currently we added the information in the swagger description: Only a single file per upload which must be named "content".

ortz marked this conversation as resolved.
Show resolved Hide resolved
boundary, ok := p["boundary"]
if !ok {
writeError(w, r, http.StatusInternalServerError, http.ErrMissingBoundary)
return
}
reader := multipart.NewReader(r.Body, boundary)
var (
contentUploaded bool
contentType string
blob *upload.Blob
)
for {
part, err := reader.NextPart()
if err == io.EOF {
break
}
ortz marked this conversation as resolved.
Show resolved Hide resolved
contentType = part.Header.Get("Content-Type")
partName := part.FormName()
// part is an io.Reader, deal with it
if !contentUploaded && partName == "content" {
// upload the first "content"
address := c.PathProvider.NewPath()
blob, err = upload.WriteBlob(ctx, c.BlockAdapter, repo.StorageNamespace, address, part, -1, block.PutOpts{StorageClass: params.StorageClass})
if err != nil {
_ = part.Close()
writeError(w, r, http.StatusInternalServerError, err)
return
}
contentUploaded = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not break here? Nothing remains that could be useful (I think).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix - wanted to complete the body processing - but I see that Close will do it for me!

}
_ = part.Close()
}
if !contentUploaded {
err := fmt.Errorf("multipart upload missing key 'content': %w", http.ErrMissingFile)
writeError(w, r, http.StatusInternalServerError, err)
return
}
Expand Down
8 changes: 4 additions & 4 deletions pkg/block/local/adapter.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func WithRemoveEmptyDir(b bool) func(a *Adapter) {
func NewAdapter(path string, opts ...func(a *Adapter)) (*Adapter, error) {
// Clean() the path so that misconfiguration does not allow path traversal.
path = filepath.Clean(path)
err := os.MkdirAll(path, 0700) //nolint: gomnd
err := os.MkdirAll(path, 0o700) //nolint: gomnd
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -102,7 +102,7 @@ func (l *Adapter) maybeMkdir(path string, f func(p string) (*os.File, error)) (*
return ret, err
}
d := filepath.Dir(filepath.Clean(path))
if err = os.MkdirAll(d, 0750); err != nil { //nolint: gomnd
if err = os.MkdirAll(d, 0o750); err != nil { //nolint: gomnd
return nil, err
}
return f(path)
Expand Down Expand Up @@ -237,7 +237,7 @@ func (l *Adapter) Get(_ context.Context, obj block.ObjectPointer, _ int64) (read
if err != nil {
return nil, err
}
f, err := os.OpenFile(filepath.Clean(p), os.O_RDONLY, 0600) //nolint: gomnd
f, err := os.OpenFile(filepath.Clean(p), os.O_RDONLY, 0o600) //nolint: gomnd
if os.IsNotExist(err) {
return nil, adapter.ErrDataNotFound
}
Expand Down Expand Up @@ -328,7 +328,7 @@ func (l *Adapter) CreateMultiPartUpload(_ context.Context, obj block.ObjectPoint
return nil, err
}
fullDir := path.Dir(fullPath)
err = os.MkdirAll(fullDir, 0750) //nolint: gomnd
err = os.MkdirAll(fullDir, 0o750) //nolint: gomnd
if err != nil {
return nil, err
}
Expand Down
36 changes: 36 additions & 0 deletions pkg/block/s3/adapter.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import (
"sync"
"time"

"github.com/aws/aws-sdk-go/service/s3/s3manager"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/awserr"
"github.com/aws/aws-sdk-go/aws/request"
Expand Down Expand Up @@ -137,6 +139,13 @@ func (a *Adapter) Put(ctx context.Context, obj block.ObjectPointer, sizeBytes in
if err != nil {
return err
}

// for unknown size we assume we like to stream content, will use s3manager to perform the request.
// we assume the caller may not have 1:1 request to s3 put object in this case as it may perform multipart upload
if sizeBytes == -1 {
return a.managerUpload(ctx, qualifiedKey, reader, opts)
}

putObject := s3.PutObjectInput{
Bucket: aws.String(qualifiedKey.StorageNamespace),
Key: aws.String(qualifiedKey.Key),
Expand Down Expand Up @@ -685,6 +694,33 @@ func (a *Adapter) extractS3Server(resp *http.Response) {
a.respServer = server
}

func (a *Adapter) managerUpload(ctx context.Context, qualifiedKey block.QualifiedKey, reader io.Reader, opts block.PutOpts) error {
client := a.clients.Get(ctx, qualifiedKey.StorageNamespace)
uploader := s3manager.NewUploaderWithClient(client)

input := &s3manager.UploadInput{
Bucket: aws.String(qualifiedKey.StorageNamespace),
Key: aws.String(qualifiedKey.Key),
Body: reader,
StorageClass: opts.StorageClass,
}
if a.ServerSideEncryption != "" {
input.ServerSideEncryption = aws.String(a.ServerSideEncryption)
}
if a.ServerSideEncryptionKmsKeyID != "" {
input.SSEKMSKeyId = aws.String(a.ServerSideEncryptionKmsKeyID)
}

output, err := uploader.UploadWithContext(ctx, input)
if err != nil {
return err
}
if aws.StringValue(output.ETag) == "" {
return ErrMissingETag
}
return nil
}

func extractAmzServerSideHeader(header http.Header) http.Header {
// return additional headers: x-amz-server-side-*
h := make(http.Header)
Expand Down