From ea49c489aa5ab603cb66be950e2cbec9490032f1 Mon Sep 17 00:00:00 2001 From: eladlachmi <110764839+eladlachmi@users.noreply.github.com> Date: Sun, 8 Oct 2023 17:42:03 +0300 Subject: [PATCH] [Bug] Write headers just before writing contents (#6726) * Write headers just before writing contents * Add test --- esti/s3_gateway_test.go | 22 ++++++++++ pkg/gateway/operations/getobject.go | 62 +++++++++++++++-------------- 2 files changed, 54 insertions(+), 30 deletions(-) diff --git a/esti/s3_gateway_test.go b/esti/s3_gateway_test.go index cdbc781a3ae..01a05665f6a 100644 --- a/esti/s3_gateway_test.go +++ b/esti/s3_gateway_test.go @@ -4,9 +4,12 @@ import ( "bytes" "io" "math/rand" + "net/http" + "net/url" "strings" "sync" "testing" + "time" "github.com/minio/minio-go/v7" "github.com/minio/minio-go/v7/pkg/credentials" @@ -168,6 +171,25 @@ func TestS3ReadObject(t *testing.T) { t.Errorf("Got contents \"%s\" but expected \"%s\"", string(got), contents) } }) + t.Run("ExistsPreSigned", func(t *testing.T) { + // using presigned URL so we can check the headers + // We expect the Content-Length header to be set + preSignedURL, err := client.Presign(ctx, http.MethodGet, repo, goodPath, time.Second*60, url.Values{}) + if err != nil { + t.Fatalf("client.Presign(%s, %s): %s", repo, goodPath, err) + } + + resp, err := http.Get(preSignedURL.String()) + if err != nil { + t.Fatalf("http.Get %s: %s", preSignedURL.String(), err) + } + defer func() { _ = resp.Body.Close() }() + contentLength := resp.Header.Get("Content-Length") + if contentLength == "" { + t.Errorf("Expected Content-Length header to be set") + } + }) + t.Run("Doesn't exist", func(t *testing.T) { res, err := client.GetObject(ctx, repo, badPath, minio.GetObjectOptions{}) if err != nil { diff --git a/pkg/gateway/operations/getobject.go b/pkg/gateway/operations/getobject.go index 1e7ab7b07a7..54669807c47 100644 --- a/pkg/gateway/operations/getobject.go +++ b/pkg/gateway/operations/getobject.go @@ -29,6 +29,7 @@ func (controller *GetObject) RequiredPermissions(_ *http.Request, repoID, _, pat func (controller *GetObject) Handle(w http.ResponseWriter, req *http.Request, o *PathOperation) { o.Incr("get_object", o.Principal, o.Repository.Name, o.Reference) + ctx := req.Context() query := req.URL.Query() if _, exists := query["versioning"]; exists { o.EncodeResponse(w, req, serde.VersioningConfiguration{}, http.StatusOK) @@ -41,7 +42,7 @@ func (controller *GetObject) Handle(w http.ResponseWriter, req *http.Request, o } beforeMeta := time.Now() - entry, err := o.Catalog.GetEntry(req.Context(), o.Repository.Name, o.Reference, o.Path, catalog.GetEntryParams{}) + entry, err := o.Catalog.GetEntry(ctx, o.Repository.Name, o.Reference, o.Path, catalog.GetEntryParams{}) metaTook := time.Since(beforeMeta) o.Log(req). WithField("took", metaTook). @@ -61,11 +62,7 @@ func (controller *GetObject) Handle(w http.ResponseWriter, req *http.Request, o _ = o.EncodeError(w, req, err, gatewayerrors.Codes.ToAPIErr(gatewayerrors.ErrInternalError)) return } - o.SetHeader(w, "Last-Modified", httputil.HeaderTimestamp(entry.CreationDate)) - o.SetHeader(w, "ETag", httputil.ETag(entry.Checksum)) - o.SetHeader(w, "Content-Type", entry.ContentType) - o.SetHeader(w, "Accept-Ranges", "bytes") - amzMetaWriteHeaders(w, entry.Metadata) + // TODO: the rest of https://docs.aws.amazon.com/en_pv/AmazonS3/latest/API/API_GetObject.html // range query var data io.ReadCloser @@ -83,38 +80,43 @@ func (controller *GetObject) Handle(w http.ResponseWriter, req *http.Request, o } // by here, we have a range we can use. } + + statusCode := http.StatusOK + contentLength := entry.Size + contentRange := "" + objectPointer := block.ObjectPointer{ + StorageNamespace: o.Repository.StorageNamespace, + IdentifierType: entry.AddressType.ToIdentifierType(), + Identifier: entry.PhysicalAddress, + } + if rangeSpec == "" || err != nil { // assemble a response body (range-less query) - o.SetHeader(w, "Content-Type", entry.ContentType) - o.SetHeader(w, "Content-Length", fmt.Sprintf("%d", entry.Size)) - data, err = o.BlockStore.Get(req.Context(), block.ObjectPointer{ - StorageNamespace: o.Repository.StorageNamespace, - IdentifierType: entry.AddressType.ToIdentifierType(), - Identifier: entry.PhysicalAddress, - }, entry.Size) - if err != nil { - _ = o.EncodeError(w, req, err, gatewayerrors.Codes.ToAPIErr(gatewayerrors.ErrInternalError)) - return - } + data, err = o.BlockStore.Get(ctx, objectPointer, entry.Size) } else { - contentRange := fmt.Sprintf("bytes %d-%d/%d", rng.StartOffset, rng.EndOffset, entry.Size) - o.SetHeader(w, "Content-Range", contentRange) - o.SetHeader(w, "Content-Length", fmt.Sprintf("%d", rng.Size())) - data, err = o.BlockStore.GetRange(req.Context(), block.ObjectPointer{ - StorageNamespace: o.Repository.StorageNamespace, - IdentifierType: entry.AddressType.ToIdentifierType(), - Identifier: entry.PhysicalAddress, - }, rng.StartOffset, rng.EndOffset) - if err != nil { - _ = o.EncodeError(w, req, err, gatewayerrors.Codes.ToAPIErr(gatewayerrors.ErrInternalError)) - return - } - w.WriteHeader(http.StatusPartialContent) + contentLength = rng.Size() + contentRange = fmt.Sprintf("bytes %d-%d/%d", rng.StartOffset, rng.EndOffset, entry.Size) + statusCode = http.StatusPartialContent + data, err = o.BlockStore.GetRange(ctx, objectPointer, rng.StartOffset, rng.EndOffset) + } + if err != nil { + _ = o.EncodeError(w, req, err, gatewayerrors.Codes.ToAPIErr(gatewayerrors.ErrInternalError)) + return } + o.SetHeader(w, "Last-Modified", httputil.HeaderTimestamp(entry.CreationDate)) + o.SetHeader(w, "ETag", httputil.ETag(entry.Checksum)) + o.SetHeader(w, "Content-Type", entry.ContentType) + o.SetHeader(w, "Accept-Ranges", "bytes") + if contentRange != "" { + o.SetHeader(w, "Content-Range", contentRange) + } + o.SetHeader(w, "Content-Length", fmt.Sprintf("%d", contentLength)) o.SetHeader(w, "X-Content-Type-Options", "nosniff") o.SetHeader(w, "X-Frame-Options", "SAMEORIGIN") o.SetHeader(w, "Content-Security-Policy", "default-src 'none'") + amzMetaWriteHeaders(w, entry.Metadata) + w.WriteHeader(statusCode) defer func() { _ = data.Close()