Support api of COPY object #728

sunfch · 2019-11-19T03:11:56Z

What this PR does / why we need it:
The requirement of the PR is from aws s3 api, https://docs.aws.amazon.com/AmazonS3/latest/API/API_CopyObject.html

The PR provide a api that is to create a copy of an object that is already stored in S3. The size of object to copy must be less 5GB, because multipart of Copy Object is not implemented yet.

The implemented features are as follows:

request with conditions. x-amz-copy-source-if-match, x-amz-copy-source-if-none-match, ....
use "x-amz-metadata-directive" to replace with metadata provided in the request when source object is equal to target object.
copy object from a backend to another backend, or copy object in the same backend.

The most common use case is the following example. It will create a copy of object "test.txt" in the same bucket
curl -H "Content-type: application/xml" -H "X-Amz-Copy-Source: /bucket001/test.txt" -X PUT http://127.0.0.1:8089/v1/s3/bucket001/test.txt2

Not implemented:

Access Permissions, ACL --- ACLs and access permissions are not involved in the PR when copy object
Versioning. the same as above
SSE. the same as above

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #727

Special notes for your reviewer:

Release note:

sfzeng · 2019-11-19T06:13:17Z

api/pkg/s3/objectcopy.go

+		sourceObjectName = splits[1]
+	}
+	// If source object is empty, reply back error.
+	if sourceObjectName == "" {


Do we need to check if sourceBucketName equals to "" too?

I think it is necessary, and how about your suggestion?

I agree with you, if sourceBucketName equals to "", that's invalid. How do you think?

Thanks for your reviewing, I will add sourceBucketName condition check to it

Agree, we should check for both Object and Bucket. Else part for len(splits) == 2 should return error

@kumarashit Thanks, I add some lines to return error when the parts of len(splits) is not equal 2

sfzeng · 2019-11-19T06:22:12Z

api/pkg/s3/objectcopy.go

+	}
+
+	// Note that sourceObject and targetObject are pointers
+	targetObject := &pb.Object{}


It seems targetObject is not used after it is defined.

It seems that you are right, I will delete it in the next commit, thanks

rhsakarpos · 2019-11-19T06:32:23Z

api/pkg/s3/api-response.go

@@ -21,9 +21,11 @@ import (
 	"net/http"


Can you please point me to the Requirements document for the Copy Object feature ?

Specifically, is this only for AWS ?

if yes, how different is it from this https://docs.aws.amazon.com/AmazonS3/latest/API/API_CopyObject.html

Another question, what is our behavior, during copy, w.r.t the following
SSE
Versioning
CORS
ACL
Storage class

@rhsakarpos , Thanks for your reviewing. The Requirement for the Copy Object is from AWS. The PR implement partial features. It include Copy Object between distinct backends and adding new attribute and checking pre conditions in http header. The PR dont support SSE, versioning, CORS, ACL, Storage class.

Hi @sunfch if the source object already set SSE, Versioning, etc, will those settings be copied, means set to new object too?

@sfzeng , thanks for your review, SSE and Versioning are not include in the PR now

Signed-off-by: sunfch <sunspot0105@gmail.com>

hopkings2008 · 2019-11-20T01:18:30Z

LGTM

Signed-off-by: sunfch <sunspot0105@gmail.com>

sfzeng · 2019-11-21T07:06:18Z

s3/pkg/service/object.go

+		return err
+	}
+	if res.Written < srcObject.Size {
+		// TODO: delete incomplete object at backend


Hi @sunfch Could you please add a issue to trace those TODO things?

I have add a issue #740 to trace it

sfzeng · 2019-11-22T02:43:12Z

s3/pkg/meta/types/object.go

+func (o *Object) GetUpdateMetaSql() (string, []interface{}) {
+	version := math.MaxUint64 - uint64(o.LastModified)
+	attrs, _ := json.Marshal(o.CustomAttributes)
+	sql := "update objects set customattributes =? where bucketname=? and name=? and version=?"


From objectcopy.go in api, it seems user can update customattritubes, storage class, and content type, but here, only customattritubes will updated. Is that a bug?

@sfzeng , Thanks for your reviewing, this is a bug. I"ll fix it in next commit

acl and content type are updated in latest commit. storage class have not been updated, because there is no storage class in db schema. I add a issue #742 to trace it

sfzeng · 2019-11-22T02:51:58Z

s3/pkg/service/object.go

+		log.Errorln("failed to create storage. err:", err)
+		return err
+	}
+	reader, err := srcSd.Get(ctx, srcObject.Object, 0, srcObject.Size)


IMHO, it's better to get target bucket and target backend, and create target storage driver before get data from source backend.

thanks for your reviewing, I will do it as you proposed in next commit

and add contenttype and acl in update sql and adjust the sequence between source and target backend Signed-off-by: sunfch <sunspot0105@gmail.com>

kumarashit · 2019-11-22T05:32:54Z

api/pkg/s3/objectcopy.go

+		sourceObjectName = splits[1]
+	}
+	// If source object is empty, reply back error.
+	if sourceObjectName == "" {


Agree, we should check for both Object and Bucket. Else part for len(splits) == 2 should return error

api/pkg/s3/objectcopy.go

kumarashit · 2019-11-22T05:55:19Z

api/pkg/s3/objectcopy.go

+		return
+	}
+
+	//TODO: In a versioning-enabled bucket, you cannot change the storage class of a specific version of an object. When you copy it, Amazon S3 gives it a new version ID.


@nguptaopensds @rhsakarpos Can we please check this with respect to the versioning we are implementing

api/pkg/s3/objectcopy.go

kumarashit · 2019-11-22T05:59:04Z

api/pkg/s3/objectcopy.go

+			response.AddHeader("x-amz-version-id", result.VersionId)
+		}
+
+		log.Info("COPY object successfully.")


Should the log say that REPLACE object successfully? As this is REPLACE and not copy exactly. And the start of the loop log that as it's replace we are updating meta data only. Thinking from debugging perspective.

thanks for your reviewing, I will fix it as your suggestion in next commit.

api/pkg/s3/objectcopy.go

Add new comment when api of COPY return successfully fix the error for const number of max object size Signed-off-by: sunfch <sunspot0105@gmail.com>

kumarashit

LGTM

kumarashit · 2019-11-25T03:19:50Z

api/pkg/common/common.go

@@ -30,7 +30,7 @@ const (
 	MaxPaginationLimit      = 1000
 	DefaultPaginationLimit  = MaxPaginationLimit
 	DefaultPaginationOffset = 0
-	MaxObjectSize           = 5 * 1024 * 1024 // 5GB
+	MaxObjectSize           = 5 * 1024 * 1024 * 1024 // 5GB


Just confirming..1024 is the standard right? no 1000. please confirm GB or GiB

@kumarashit , Thanks for your reviewingm, I created a 510241024*1024 size file and put it to aws, and aws response with ok, so it should be 1024

kumarashit

LGTM

sfzeng reviewed Nov 19, 2019

View reviewed changes

rhsakarpos reviewed Nov 19, 2019

View reviewed changes

Support the api of COPY object

c6ded9a

Signed-off-by: sunfch <sunspot0105@gmail.com>

sunfch force-pushed the wip-copy-object branch from 636380d to c6ded9a Compare November 19, 2019 15:20

Delete unused code and check s3 error code returned from grpc server

ad88c85

Signed-off-by: sunfch <sunspot0105@gmail.com>

sfzeng reviewed Nov 21, 2019

View reviewed changes

sunfch mentioned this pull request Nov 21, 2019

delete object that has been written when request fails #740

Open

sfzeng reviewed Nov 22, 2019

View reviewed changes

Add sourceBucketName condition to check

3b73fb0

and add contenttype and acl in update sql and adjust the sequence between source and target backend Signed-off-by: sunfch <sunspot0105@gmail.com>

sunfch mentioned this pull request Nov 22, 2019

There is no storage class in object table, add it and support it in api of COPY, PUT, GET.... #742

Open

kumarashit requested changes Nov 22, 2019

View reviewed changes

sunfch mentioned this pull request Nov 24, 2019

How to handle with storage glacer and deeparchive class header in the api of COPY object #745

Open

Add the checking of len(splits) != 2

90789b2

Add new comment when api of COPY return successfully fix the error for const number of max object size Signed-off-by: sunfch <sunspot0105@gmail.com>

kumarashit approved these changes Nov 25, 2019

View reviewed changes

sfzeng approved these changes Nov 25, 2019

View reviewed changes

sfzeng merged commit 7045d23 into sodafoundation:development Nov 25, 2019

sfzeng mentioned this pull request Jun 11, 2020

Copy object #522

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support api of COPY object #728

Support api of COPY object #728

sunfch commented Nov 19, 2019 •

edited

Loading

sfzeng Nov 19, 2019

sunfch Nov 20, 2019

sfzeng Nov 21, 2019

sunfch Nov 22, 2019

kumarashit Nov 22, 2019

sunfch Nov 24, 2019

sfzeng Nov 19, 2019

sunfch Nov 20, 2019

rhsakarpos Nov 19, 2019

sunfch Nov 20, 2019

sfzeng Nov 21, 2019

sunfch Nov 21, 2019

hopkings2008 commented Nov 20, 2019

sfzeng Nov 21, 2019

sunfch Nov 21, 2019

sfzeng Nov 22, 2019

sunfch Nov 22, 2019

sunfch Nov 22, 2019 •

edited

Loading

sfzeng Nov 22, 2019

sunfch Nov 22, 2019

kumarashit Nov 22, 2019

kumarashit Nov 22, 2019

kumarashit Nov 22, 2019

sunfch Nov 24, 2019

kumarashit left a comment

kumarashit Nov 25, 2019

sunfch Nov 25, 2019

kumarashit left a comment

Support api of COPY object #728

Support api of COPY object #728

Conversation

sunfch commented Nov 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hopkings2008 commented Nov 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunfch Nov 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumarashit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumarashit left a comment

Choose a reason for hiding this comment

sunfch commented Nov 19, 2019 •

edited

Loading

sunfch Nov 22, 2019 •

edited

Loading