From fa4a8b0a7e6f76324d9532416f1e2f0df9febff7 Mon Sep 17 00:00:00 2001 From: Tejaswini Duggaraju Date: Fri, 11 Oct 2019 14:16:55 -0700 Subject: [PATCH] Pass the source file information to the Move API of the driver Signed-off-by: Tejaswini Duggaraju Format the file using go fmt --- registry/storage/blobwriter.go | 10 ++++++++-- registry/storage/driver/azure/azure.go | 11 ++++++++-- registry/storage/driver/base/base.go | 4 ++-- registry/storage/driver/base/regulator.go | 4 ++-- registry/storage/driver/filesystem/driver.go | 2 +- registry/storage/driver/gcs/gcs_test.go | 10 +++++++++- registry/storage/driver/inmemory/driver.go | 2 +- registry/storage/driver/oss/oss.go | 2 +- registry/storage/driver/s3-aws/s3.go | 2 +- registry/storage/driver/s3-aws/s3_test.go | 10 +++++++++- registry/storage/driver/storagedriver.go | 5 ++++- registry/storage/driver/swift/swift.go | 2 +- .../storage/driver/testsuites/testsuites.go | 20 +++++++++++++++---- 13 files changed, 64 insertions(+), 20 deletions(-) diff --git a/registry/storage/blobwriter.go b/registry/storage/blobwriter.go index 54c79b6f3..19be14157 100644 --- a/registry/storage/blobwriter.go +++ b/registry/storage/blobwriter.go @@ -321,7 +321,8 @@ func (bw *blobWriter) moveBlob(ctx context.Context, desc distribution.Descriptor // the size here and write a zero-length file to blobPath if this is the // case. For the most part, this should only ever happen with zero-length // blobs. - if _, err := bw.blobStore.driver.Stat(ctx, bw.path); err != nil { + sourceFileInfo, err := bw.blobStore.driver.Stat(ctx, bw.path) + if err != nil { switch err := err.(type) { case storagedriver.PathNotFoundError: // HACK(stevvooe): This is slightly dangerous: if we verify above, @@ -340,11 +341,16 @@ func (bw *blobWriter) moveBlob(ctx context.Context, desc distribution.Descriptor default: return err // unrelated error } + } else { + // We will make sure that the size on the blob store matches with the local commited file. + if sourceFileInfo.Size() != desc.Size { + return distribution.ErrBlobInvalidLength + } } // TODO(stevvooe): We should also write the mediatype when executing this move. - return bw.blobStore.driver.Move(ctx, bw.path, blobPath) + return bw.blobStore.driver.Move(ctx, bw.path, blobPath, sourceFileInfo) } // removeResources should clean up all resources associated with the upload diff --git a/registry/storage/driver/azure/azure.go b/registry/storage/driver/azure/azure.go index 58c6293b2..957a25dcc 100644 --- a/registry/storage/driver/azure/azure.go +++ b/registry/storage/driver/azure/azure.go @@ -291,11 +291,18 @@ func (d *driver) List(ctx context.Context, path string) ([]string, error) { // Move moves an object stored at sourcePath to destPath, removing the original // object. -func (d *driver) Move(ctx context.Context, sourcePath string, destPath string) error { +func (d *driver) Move(ctx context.Context, sourcePath string, destPath string, sourceFileInfo storagedriver.FileInfo) error { srcBlobRef := d.client.GetContainerReference(d.container).GetBlobReference(sourcePath) sourceBlobURL := srcBlobRef.GetURL() destBlobRef := d.client.GetContainerReference(d.container).GetBlobReference(destPath) - err := destBlobRef.Copy(sourceBlobURL, nil) + options := &azure.CopyOptions{} + // sourceFileInfo can be nil if the source path is not found. If the file info is available + // we will ensure it is unmodified during the move operation. + if sourceFileInfo != nil { + lastModTime := sourceFileInfo.ModTime() + options.Source.IfUnmodifiedSince = &lastModTime + } + err := destBlobRef.Copy(sourceBlobURL, options) if err != nil { if is404(err) { return storagedriver.PathNotFoundError{Path: sourcePath} diff --git a/registry/storage/driver/base/base.go b/registry/storage/driver/base/base.go index 4511f0021..2233dd953 100644 --- a/registry/storage/driver/base/base.go +++ b/registry/storage/driver/base/base.go @@ -181,7 +181,7 @@ func (base *Base) List(ctx context.Context, path string) ([]string, error) { } // Move wraps Move of underlying storage driver. -func (base *Base) Move(ctx context.Context, sourcePath string, destPath string) error { +func (base *Base) Move(ctx context.Context, sourcePath string, destPath string, sourceFileInfo storagedriver.FileInfo) error { ctx, done := dcontext.WithTrace(ctx) defer done("%s.Move(%q, %q", base.Name(), sourcePath, destPath) @@ -192,7 +192,7 @@ func (base *Base) Move(ctx context.Context, sourcePath string, destPath string) } start := time.Now() - err := base.setDriverName(base.StorageDriver.Move(ctx, sourcePath, destPath)) + err := base.setDriverName(base.StorageDriver.Move(ctx, sourcePath, destPath, sourceFileInfo)) storageAction.WithValues(base.Name(), "Move").UpdateSince(start) return err } diff --git a/registry/storage/driver/base/regulator.go b/registry/storage/driver/base/regulator.go index 9c5e6cc41..61a2883b1 100644 --- a/registry/storage/driver/base/regulator.go +++ b/registry/storage/driver/base/regulator.go @@ -157,11 +157,11 @@ func (r *regulator) List(ctx context.Context, path string) ([]string, error) { // original object. // Note: This may be no more efficient than a copy followed by a delete for // many implementations. -func (r *regulator) Move(ctx context.Context, sourcePath string, destPath string) error { +func (r *regulator) Move(ctx context.Context, sourcePath string, destPath string, sourceFileInfo storagedriver.FileInfo) error { r.enter() defer r.exit() - return r.StorageDriver.Move(ctx, sourcePath, destPath) + return r.StorageDriver.Move(ctx, sourcePath, destPath, sourceFileInfo) } // Delete recursively deletes all objects stored at "path" and its subpaths. diff --git a/registry/storage/driver/filesystem/driver.go b/registry/storage/driver/filesystem/driver.go index 8fc9d1ca0..4b9da3b0d 100644 --- a/registry/storage/driver/filesystem/driver.go +++ b/registry/storage/driver/filesystem/driver.go @@ -252,7 +252,7 @@ func (d *driver) List(ctx context.Context, subPath string) ([]string, error) { // Move moves an object stored at sourcePath to destPath, removing the original // object. -func (d *driver) Move(ctx context.Context, sourcePath string, destPath string) error { +func (d *driver) Move(ctx context.Context, sourcePath string, destPath string, sourceFileInfo storagedriver.FileInfo) error { source := d.fullPath(sourcePath) dest := d.fullPath(destPath) diff --git a/registry/storage/driver/gcs/gcs_test.go b/registry/storage/driver/gcs/gcs_test.go index e58216be0..aec6311f9 100644 --- a/registry/storage/driver/gcs/gcs_test.go +++ b/registry/storage/driver/gcs/gcs_test.go @@ -7,6 +7,7 @@ import ( "io/ioutil" "os" "testing" + "time" dcontext "github.com/docker/distribution/context" storagedriver "github.com/docker/distribution/registry/storage/driver" @@ -292,6 +293,13 @@ func TestMoveDirectory(t *testing.T) { ctx := dcontext.Background() contents := []byte("contents") + sourcePath := "/parent/dir" + sourceFileInfo := storagedriver.FileInfoInternal{FileInfoFields: storagedriver.FileInfoFields{ + Path: sourcePath, + Size: int64(len(contents)), + ModTime: time.Now(), + IsDir: false, + }} // Create a regular file. err = driver.PutContent(ctx, "/parent/dir/foo", contents) if err != nil { @@ -304,7 +312,7 @@ func TestMoveDirectory(t *testing.T) { } }() - err = driver.Move(ctx, "/parent/dir", "/parent/other") + err = driver.Move(ctx, sourcePath, "/parent/other", sourceFileInfo) if err == nil { t.Fatalf("Moving directory /parent/dir /parent/other should have return a non-nil error\n") } diff --git a/registry/storage/driver/inmemory/driver.go b/registry/storage/driver/inmemory/driver.go index 4b1f5f48d..2cf65b525 100644 --- a/registry/storage/driver/inmemory/driver.go +++ b/registry/storage/driver/inmemory/driver.go @@ -207,7 +207,7 @@ func (d *driver) List(ctx context.Context, path string) ([]string, error) { // Move moves an object stored at sourcePath to destPath, removing the original // object. -func (d *driver) Move(ctx context.Context, sourcePath string, destPath string) error { +func (d *driver) Move(ctx context.Context, sourcePath string, destPath string, sourceFileInfo storagedriver.FileInfo) error { d.mutex.Lock() defer d.mutex.Unlock() diff --git a/registry/storage/driver/oss/oss.go b/registry/storage/driver/oss/oss.go index 5cd96203f..a5ac1fe48 100644 --- a/registry/storage/driver/oss/oss.go +++ b/registry/storage/driver/oss/oss.go @@ -407,7 +407,7 @@ const maxConcurrency = 10 // Move moves an object stored at sourcePath to destPath, removing the original // object. -func (d *driver) Move(ctx context.Context, sourcePath string, destPath string) error { +func (d *driver) Move(ctx context.Context, sourcePath string, destPath string, sourceFileInfo storagedriver.FileInfo) error { logrus.Infof("Move from %s to %s", d.ossPath(sourcePath), d.ossPath(destPath)) err := d.Bucket.CopyLargeFileInParallel(d.ossPath(sourcePath), d.ossPath(destPath), d.getContentType(), diff --git a/registry/storage/driver/s3-aws/s3.go b/registry/storage/driver/s3-aws/s3.go index 4b6c50d7b..08bbe44ed 100644 --- a/registry/storage/driver/s3-aws/s3.go +++ b/registry/storage/driver/s3-aws/s3.go @@ -696,7 +696,7 @@ func (d *driver) List(ctx context.Context, opath string) ([]string, error) { // Move moves an object stored at sourcePath to destPath, removing the original // object. -func (d *driver) Move(ctx context.Context, sourcePath string, destPath string) error { +func (d *driver) Move(ctx context.Context, sourcePath string, destPath string, sourceFileInfo storagedriver.FileInfo) error { /* This is terrible, but aws doesn't have an actual move. */ if err := d.copy(ctx, sourcePath, destPath); err != nil { return err diff --git a/registry/storage/driver/s3-aws/s3_test.go b/registry/storage/driver/s3-aws/s3_test.go index be02772e9..07cf7d6f9 100644 --- a/registry/storage/driver/s3-aws/s3_test.go +++ b/registry/storage/driver/s3-aws/s3_test.go @@ -7,6 +7,7 @@ import ( "os" "strconv" "testing" + "time" "gopkg.in/check.v1" @@ -303,7 +304,14 @@ func TestMoveWithMultipartCopy(t *testing.T) { t.Fatalf("unexpected error creating content: %v", err) } - err = d.Move(ctx, sourcePath, destPath) + sourceFileInfo := storagedriver.FileInfoInternal{FileInfoFields: storagedriver.FileInfoFields{ + Path: sourcePath, + Size: int64(len(contents)), + ModTime: time.Now(), + IsDir: false, + }} + + err = d.Move(ctx, sourcePath, destPath, sourceFileInfo) if err != nil { t.Fatalf("unexpected error moving file: %v", err) } diff --git a/registry/storage/driver/storagedriver.go b/registry/storage/driver/storagedriver.go index b220713f2..9d666ea8b 100644 --- a/registry/storage/driver/storagedriver.go +++ b/registry/storage/driver/storagedriver.go @@ -73,7 +73,10 @@ type StorageDriver interface { // original object. // Note: This may be no more efficient than a copy followed by a delete for // many implementations. - Move(ctx context.Context, sourcePath string, destPath string) error + // We will pass along the source file info so the driver can verify the + // parity of the moved object. + // Note: the source file info can be nil in the case the source path is not found. + Move(ctx context.Context, sourcePath string, destPath string, sourceFileInfo FileInfo) error // Delete recursively deletes all objects stored at "path" and its subpaths. Delete(ctx context.Context, path string) error diff --git a/registry/storage/driver/swift/swift.go b/registry/storage/driver/swift/swift.go index b64e3b66c..0c7d07d4b 100644 --- a/registry/storage/driver/swift/swift.go +++ b/registry/storage/driver/swift/swift.go @@ -498,7 +498,7 @@ func (d *driver) List(ctx context.Context, path string) ([]string, error) { // Move moves an object stored at sourcePath to destPath, removing the original // object. -func (d *driver) Move(ctx context.Context, sourcePath string, destPath string) error { +func (d *driver) Move(ctx context.Context, sourcePath string, destPath string, sourceFileInfo storagedriver.FileInfo) error { _, headers, err := d.Conn.Object(d.Container, d.swiftPath(sourcePath)) if err == nil { if manifest, ok := headers["X-Object-Manifest"]; ok { diff --git a/registry/storage/driver/testsuites/testsuites.go b/registry/storage/driver/testsuites/testsuites.go index 7cf7b379e..c189a447b 100644 --- a/registry/storage/driver/testsuites/testsuites.go +++ b/registry/storage/driver/testsuites/testsuites.go @@ -514,6 +514,12 @@ func (suite *DriverSuite) TestMove(c *check.C) { contents := randomContents(32) sourcePath := randomPath(32) destPath := randomPath(32) + sourceFileInfo := storagedriver.FileInfoInternal{FileInfoFields: storagedriver.FileInfoFields{ + Path: sourcePath, + Size: int64(len(contents)), + ModTime: time.Now(), + IsDir: false, + }} defer suite.deletePath(c, firstPart(sourcePath)) defer suite.deletePath(c, firstPart(destPath)) @@ -521,7 +527,7 @@ func (suite *DriverSuite) TestMove(c *check.C) { err := suite.StorageDriver.PutContent(suite.ctx, sourcePath, contents) c.Assert(err, check.IsNil) - err = suite.StorageDriver.Move(suite.ctx, sourcePath, destPath) + err = suite.StorageDriver.Move(suite.ctx, sourcePath, destPath, sourceFileInfo) c.Assert(err, check.IsNil) received, err := suite.StorageDriver.GetContent(suite.ctx, destPath) @@ -541,6 +547,12 @@ func (suite *DriverSuite) TestMoveOverwrite(c *check.C) { destPath := randomPath(32) sourceContents := randomContents(32) destContents := randomContents(64) + sourceFileInfo := storagedriver.FileInfoInternal{FileInfoFields: storagedriver.FileInfoFields{ + Path: sourcePath, + Size: int64(len(sourceContents)), + ModTime: time.Now(), + IsDir: false, + }} defer suite.deletePath(c, firstPart(sourcePath)) defer suite.deletePath(c, firstPart(destPath)) @@ -551,7 +563,7 @@ func (suite *DriverSuite) TestMoveOverwrite(c *check.C) { err = suite.StorageDriver.PutContent(suite.ctx, destPath, destContents) c.Assert(err, check.IsNil) - err = suite.StorageDriver.Move(suite.ctx, sourcePath, destPath) + err = suite.StorageDriver.Move(suite.ctx, sourcePath, destPath, sourceFileInfo) c.Assert(err, check.IsNil) received, err := suite.StorageDriver.GetContent(suite.ctx, destPath) @@ -576,7 +588,7 @@ func (suite *DriverSuite) TestMoveNonexistent(c *check.C) { err := suite.StorageDriver.PutContent(suite.ctx, destPath, contents) c.Assert(err, check.IsNil) - err = suite.StorageDriver.Move(suite.ctx, sourcePath, destPath) + err = suite.StorageDriver.Move(suite.ctx, sourcePath, destPath, nil) c.Assert(err, check.NotNil) c.Assert(err, check.FitsTypeOf, storagedriver.PathNotFoundError{}) c.Assert(strings.Contains(err.Error(), suite.Name()), check.Equals, true) @@ -596,7 +608,7 @@ func (suite *DriverSuite) TestMoveInvalid(c *check.C) { defer suite.deletePath(c, "/notadir") // Now try to move a non-existent file under it. - err = suite.StorageDriver.Move(suite.ctx, "/notadir/foo", "/notadir/bar") + err = suite.StorageDriver.Move(suite.ctx, "/notadir/foo", "/notadir/bar", nil) c.Assert(err, check.NotNil) // non-nil error }