From 4baddbc6087442615ec0dff1027c6ccef1a4cb27 Mon Sep 17 00:00:00 2001
From: Milos Gajdos <milosthegajdos@gmail.com>
Date: Wed, 13 Dec 2023 08:58:21 +0000
Subject: [PATCH] fix: update S3 storage driver writer

This commit updates (writer).Writer() method in S3 storage driver to
handle the case where an append is attempted to a zer-size content.

S3 does not allow appending to already committed content, so we are
optiing to provide the following case as a narrowed down behaviour:
Writer can only append to zero byte content - in that case, a new S3
MultipartUpload is created that will be used for overriding the already
committed zero size content.

Appending to non-zero size content fails with error.

Co-authored-by: Cory Snider <corhere@gmail.com>
Signed-off-by: Milos Gajdos <milosthegajdos@gmail.com>
---
 registry/storage/driver/s3-aws/s3.go     | 29 +++++++++++++++++++++++-
 registry/storage/driver/storagedriver.go |  5 ++++
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/registry/storage/driver/s3-aws/s3.go b/registry/storage/driver/s3-aws/s3.go
index 8092bb7ef..168d3a3d1 100644
--- a/registry/storage/driver/s3-aws/s3.go
+++ b/registry/storage/driver/s3-aws/s3.go
@@ -680,6 +680,10 @@ func (d *driver) Reader(ctx context.Context, path string, offset int64) (io.Read
 
 // Writer returns a FileWriter which will store the content written to it
 // at the location designated by "path" after the call to Commit.
+// It only allows appending to paths with zero size committed content,
+// in which the existing content is overridden with the new content.
+// It returns storagedriver.ErrUnsupportedMethod when appending to paths
+// with non-zero committed content.
 func (d *driver) Writer(ctx context.Context, path string, appendMode bool) (storagedriver.FileWriter, error) {
 	key := d.s3Path(path)
 	if !appendMode {
@@ -713,7 +717,30 @@ func (d *driver) Writer(ctx context.Context, path string, appendMode bool) (stor
 		// if there were no more results to return after the first call, resp.IsTruncated would have been false
 		// and the loop would be exited without recalling ListMultipartUploads
 		if len(resp.Uploads) == 0 {
-			break
+			fi, err := d.Stat(ctx, path)
+			if err != nil {
+				return nil, parseError(path, err)
+			}
+
+			if fi.Size() == 0 {
+				resp, err := d.S3.CreateMultipartUploadWithContext(ctx, &s3.CreateMultipartUploadInput{
+					Bucket:               aws.String(d.Bucket),
+					Key:                  aws.String(key),
+					ContentType:          d.getContentType(),
+					ACL:                  d.getACL(),
+					ServerSideEncryption: d.getEncryptionMode(),
+					SSEKMSKeyId:          d.getSSEKMSKeyID(),
+					StorageClass:         d.getStorageClass(),
+				})
+				if err != nil {
+					return nil, err
+				}
+				return d.newWriter(ctx, key, *resp.UploadId, nil), nil
+			}
+			return nil, storagedriver.Error{
+				DriverName: driverName,
+				Detail:     fmt.Errorf("append to zero-size path %s unsupported", path),
+			}
 		}
 
 		var allParts []*s3.Part
diff --git a/registry/storage/driver/storagedriver.go b/registry/storage/driver/storagedriver.go
index d84645765..c12f79de3 100644
--- a/registry/storage/driver/storagedriver.go
+++ b/registry/storage/driver/storagedriver.go
@@ -74,6 +74,11 @@ type StorageDriver interface {
 
 	// Writer returns a FileWriter which will store the content written to it
 	// at the location designated by "path" after the call to Commit.
+	// A path may be appended to if it has not been committed, or if the
+	// existing committed content is zero length.
+	//
+	// The behaviour of appending to paths with non-empty committed content is
+	// undefined. Specific implementations may document their own behavior.
 	Writer(ctx context.Context, path string, append bool) (FileWriter, error)
 
 	// Stat retrieves the FileInfo for the given path, including the current