diff --git a/cmd/serve/s3/backend.go b/cmd/serve/s3/backend.go index fa1218e06..0f1bf608e 100644 --- a/cmd/serve/s3/backend.go +++ b/cmd/serve/s3/backend.go @@ -21,6 +21,7 @@ import ( "github.com/ncw/swift/v2" "github.com/rclone/gofakes3" "github.com/rclone/rclone/fs" + fshash "github.com/rclone/rclone/fs/hash" "github.com/rclone/rclone/fs/operations" "github.com/rclone/rclone/vfs" "github.com/rclone/rclone/vfs/vfscommon" @@ -560,6 +561,9 @@ func (b *s3Backend) PutObject( if err != nil { return result, err } + if b.s.etagHash(_vfs) != fshash.MD5 { + result.ETag = quoteETag(emptyHash(b.s.etagHash(_vfs))) + } return result, putDirMarker(_vfs, fp, objectName, input) } fp, err := bucketObjectPath(bucketName, objectName) @@ -639,12 +643,26 @@ func (b *s3Backend) PutObject( } } - _, err = _vfs.Stat(fp) + node, err := _vfs.Stat(fp) if err != nil { return result, err } - return result, b.tenant(ctx).storeMeta(_vfs, fp, meta) + if err := b.tenant(ctx).storeMeta(_vfs, fp, meta); err != nil { + return result, err + } + + // gofakes3 returns the MD5 of the data as the ETag, so another + // hash must be given to match what HeadObject returns. + if b.s.etagHash(_vfs) != fshash.MD5 { + result.ETag = quoteETag(b.objectHash(ctx, _vfs, fp, node)) + } + return result, nil +} + +// quoteETag returns hash as an ETag. +func quoteETag(hash []byte) string { + return `"` + hex.EncodeToString(hash) + `"` } // commitObject moves the object written to tmpFp into place at fp, @@ -842,7 +860,7 @@ func (b *s3Backend) CopyObject(ctx context.Context, srcBucket, srcKey, dstBucket } return gofakes3.CopyObjectResult{ - ETag: `"` + hex.EncodeToString(c.Hash) + `"`, + ETag: quoteETag(c.Hash), LastModified: gofakes3.NewContentTime(cStat.ModTime()), }, nil } diff --git a/cmd/serve/s3/list.go b/cmd/serve/s3/list.go index f174d0037..b238c2b99 100644 --- a/cmd/serve/s3/list.go +++ b/cmd/serve/s3/list.go @@ -119,14 +119,10 @@ func (l *lister) addObject(key string, entry vfs.Node) error { if l.keys >= l.max { return l.truncate() } - etag := getFileHash(entry, l.hashType) - if etag != "" { - etag = `"` + etag + `"` - } l.response.Add(&gofakes3.Content{ Key: key, LastModified: gofakes3.NewContentTime(entry.ModTime()), - ETag: etag, + ETag: quoteETag(getFileHashByte(entry, l.hashType)), Size: entry.Size(), StorageClass: gofakes3.StorageStandard, }) diff --git a/cmd/serve/s3/multipart.go b/cmd/serve/s3/multipart.go index 3a356efcd..bb14846b5 100644 --- a/cmd/serve/s3/multipart.go +++ b/cmd/serve/s3/multipart.go @@ -27,7 +27,6 @@ import ( "io" "math" "path" - "sort" "strings" "sync" "time" @@ -926,23 +925,27 @@ func (b *s3Backend) CompleteMultipartUpload(ctx context.Context, bucketName, obj } b.deleteUpload(uploadID, up) - // The ETag is the MD5 of the object when that is the hash used - // for ETags, so it is the same as HEAD returns for it, otherwise - // the S3 multipart ETag. It is stored, with the part sizes, so - // HEAD returns it while the object is unchanged. - etag := up.multipartETag(input) + // The ETag is the one HEAD returns for the object. With MD5 + // ETags the MD5 of the object, which the backend may not have, is + // stored with the part sizes so HEAD returns it while the object + // is unchanged. var etagHash []byte if b.s.etagHash(up.vfs) == fshash.MD5 { etagHash = up.objectMD5.Sum(nil) - etag = fmt.Sprintf("%q", hex.EncodeToString(etagHash)) } _ = up.tenant.storeObjectMeta(up.vfs, up.fp, objectMeta{ meta: up.meta, hash: etagHash, partSizes: up.listedPartSizes(input), }) + if etagHash == nil { + // The upload is complete, so isn't failed if the object can't be read + if node, err := up.vfs.Stat(up.fp); err == nil { + etagHash = b.objectHash(ctx, up.vfs, up.fp, node) + } + } - return "", etag, nil + return "", quoteETag(etagHash), nil } // listedPartSizes returns the sizes of the parts listed in input, in @@ -1210,24 +1213,3 @@ func (up *multipartUpload) abort() error { } return fh.Close() } - -// multipartETag computes the S3 multipart ETag for the assembled object: -// -// hex(md5(concat(part_md5s_in_order))) + "-" + N -func (up *multipartUpload) multipartETag(input *gofakes3.CompleteMultipartUploadRequest) string { - partNumbers := make([]int, 0, len(input.Parts)) - for _, p := range input.Parts { - partNumbers = append(partNumbers, p.PartNumber) - } - sort.Ints(partNumbers) - - up.mu.Lock() - concat := make([]byte, 0, len(partNumbers)*md5.Size) - for _, n := range partNumbers { - concat = append(concat, up.partMD5s[n]...) - } - up.mu.Unlock() - - sum := md5.Sum(concat) - return fmt.Sprintf("%q", fmt.Sprintf("%s-%d", hex.EncodeToString(sum[:]), len(partNumbers))) -} diff --git a/cmd/serve/s3/multipart_test.go b/cmd/serve/s3/multipart_test.go index 759aedf03..94246f160 100644 --- a/cmd/serve/s3/multipart_test.go +++ b/cmd/serve/s3/multipart_test.go @@ -6,6 +6,7 @@ import ( "bytes" "context" "crypto/md5" + "crypto/sha1" "encoding/base64" "encoding/hex" "fmt" @@ -13,6 +14,7 @@ import ( "math" "net/url" "path" + "strings" "sync" "sync/atomic" "testing" @@ -24,6 +26,7 @@ import ( _ "github.com/rclone/rclone/backend/memory" "github.com/rclone/rclone/cmd/serve/proxy" "github.com/rclone/rclone/fs" + "github.com/rclone/rclone/fs/hash" "github.com/rclone/rclone/fs/object" "github.com/rclone/rclone/fs/operations" "github.com/rclone/rclone/fstest" @@ -1956,3 +1959,84 @@ func TestMultipartETagAndParts(t *testing.T) { }) } } + +// TestETagHashConsistent checks the ETags returned when objects are +// made are the same as HEAD and listings return for them, whatever +// --etag-hash is. +func TestETagHashConsistent(t *testing.T) { + for _, etagHash := range []string{"", "SHA1"} { + for _, tc := range testRemotes { + t.Run(fmt.Sprintf("%s/%q", tc.name, etagHash), func(t *testing.T) { + // A directory marker on a memory remote is a virtual + // directory, which stops the VFS shared by the servers + // of plain ":memory:" seeing new buckets, so this needs + // its own. + backing := tc.backing + if backing == ":memory:" { + backing = fmt.Sprintf(":memory,description=etag%d:", testBackingCounter.Add(1)) + } + core, f, bucket := newMultipartTestServerOpt(t, backing, false, func(opt *Options) { + opt.EtagHash = etagHash + }) + ctx := context.Background() + sha1Hex := func(data []byte) string { + if etagHash == "" { + return "" + } + sum := sha1.Sum(data) + return hex.EncodeToString(sum[:]) + } + // The backend gives the hashes of objects, but not of + // directory markers + wantETag := func(data []byte) string { + if !f.Hashes().Contains(hash.SHA1) { + return "" + } + return sha1Hex(data) + } + etags := map[string]string{} + + data := []byte("put object") + info, err := core.PutObject(ctx, bucket, "put.txt", bytes.NewReader(data), int64(len(data)), "", "", minio.PutObjectOptions{}) + require.NoError(t, err) + assert.Equal(t, wantETag(data), info.ETag, "put") + etags["put.txt"] = info.ETag + + info, err = core.PutObject(ctx, bucket, "dir/", bytes.NewReader(nil), 0, "", "", minio.PutObjectOptions{}) + require.NoError(t, err) + assert.Equal(t, sha1Hex(nil), info.ETag, "directory marker") + etags["dir/"] = info.ETag + + uploadID, err := core.NewMultipartUpload(ctx, bucket, "multipart.bin", minio.PutObjectOptions{}) + require.NoError(t, err) + var all []byte + var parts []minio.CompletePart + for i, size := range []int{100 * 1024, 30 * 1024} { + data := []byte(random.String(size)) + all = append(all, data...) + p, err := core.PutObjectPart(ctx, bucket, "multipart.bin", uploadID, i+1, bytes.NewReader(data), int64(size), minio.PutObjectPartOptions{}) + require.NoError(t, err) + parts = append(parts, minio.CompletePart{PartNumber: i + 1, ETag: p.ETag}) + } + complete, err := core.CompleteMultipartUpload(ctx, bucket, "multipart.bin", uploadID, parts, minio.PutObjectOptions{}) + require.NoError(t, err) + assert.Equal(t, wantETag(all), complete.ETag, "multipart") + etags["multipart.bin"] = complete.ETag + + for key, etag := range etags { + stat, err := core.StatObject(ctx, bucket, key, minio.StatObjectOptions{}) + require.NoError(t, err, key) + assert.Equal(t, etag, stat.ETag, "HEAD %s", key) + } + listed := map[string]string{} + result, err := core.ListObjects(bucket, "", "", "", 1000) + require.NoError(t, err) + for _, obj := range result.Contents { + listed[obj.Key] = strings.Trim(obj.ETag, `"`) + } + delete(etags, "dir/") + assert.Equal(t, etags, listed) + }) + } + } +} diff --git a/cmd/serve/s3/serve_s3.md b/cmd/serve/s3/serve_s3.md index 6a2fcfe6c..037a3b244 100644 --- a/cmd/serve/s3/serve_s3.md +++ b/cmd/serve/s3/serve_s3.md @@ -65,11 +65,11 @@ cause problems for S3 clients which rely on the Etag being the MD5. Use `--etag-hash auto` to use the best hash the backend supports - with `--auth-proxy` that of each user's backend. -With `--etag-hash MD5`, the `ETag` of an object made by a multipart -upload is the MD5 of the whole object, like any other object, rather -than the `-N` suffixed `ETag` AWS S3 gives these. With another hash -it is the AWS S3 style multipart `ETag`, but `HeadObject` only -returns that while the object's metadata is kept (see below). +The `ETag` of an object made by a multipart upload is its hash, like +any other object, rather than the `-N` suffixed `ETag` AWS S3 gives +these. If the remote doesn't have the MD5 of objects, `HeadObject` +only returns the MD5 of these while the object's metadata is kept (see +below). ### Quickstart