rgw: multipart uploads copy part support #5673
Conversation
jmunhoz
force-pushed
the
wip-multipart-uploads-cp
branch
from
8fff7db
to
4c78582
Compare
|
@yehudasa I moved the functionality to RGWCopyObj as suggested. Added some tests at ceph/s3-tests#75 too. |
|
@jmunhoz I think I liked the original version better. There's a lot of code copied here, it doesn't look like the correct way to go. What I would like to see is a reuse of the current code with adjustments where needed. If it's easier with your original approach then it's fine. |
|
@yehudasa ok, I will try to reuse the current code with adjustments where needed. |
jmunhoz
force-pushed
the
wip-multipart-uploads-cp
branch
from
4c78582
to
caceaa8
Compare
|
@yehudasa done. It reuses the current code with adjustments where needed. Rebased and fixed conflicts. |
jmunhoz
force-pushed
the
wip-multipart-uploads-cp
branch
from
caceaa8
to
b011252
Compare
|
@yehudasa rebased and fixed conflicts. |
|
@jmunhoz take a look here https://github.com/yehudasa/ceph/tree/wip-multipart-uploads-cp -- reworked your commit a bit to remove some accidental indentation issues, and other minor cosmetic cleanups that inflated the diff and made it impossible to review. Now that I see where you were going at, I'm not sure that consolidating the PutObj and the CopyObj is actually needed. At the point where we generate the op ( |
|
@yehudasa Not sure. I will have a look |
|
@jmunhoz ping |
|
@yehudasa Sorry! it is in my TODO reviewing the new suggested approach. I need to have a look in the code but I am not sure if generating a new operation would add a lot of redundant code... |
|
@jmunhoz hi--just checkin in; are you continuing with this change? |
|
@mattbenjamin hi! yes, my idea is retaking the first version and working from there. I am working in the aws4 chunked stream support now. I should be working on this feat in the short. Would it be ok? |
jmunhoz
force-pushed
the
wip-multipart-uploads-cp
branch
from
b011252
to
2777e81
Compare
|
Rebased and catched up the master head. I am experimenting some issues to test the feature with the old s3-tests. The current test cases (ceph/s3-tests#75) got old and they stop working. It doesn't look a problem with the feature though. I uploaded a test case working in python http://tracker.ceph.com/issues/12790#note-3 It is useful to validate the feature and as possible template for the s3-tests failing. |
| /* handle x-amz-copy-source */ | ||
|
|
||
| if (copy_source) { | ||
| bucket = s->info.env->get("HTTP_X_AMZ_COPY_SOURCE"); |
There was a problem hiding this comment.
@jmunhoz should do this prior to execute(), probably in get_params(). As it is, it's a security issue, we don't check here permissions of the source object, which needs to happen.
Add multipart uploads copy part feature. Fixes: http://tracker.ceph.com/issues/12790 Signed-off-by: Javier M. Mellid <jmunhoz@igalia.com>
Signed-off-by: Javier M. Mellid <jmunhoz@igalia.com>
jmunhoz
force-pushed
the
wip-multipart-uploads-cp
branch
from
2777e81
to
5d966a5
Compare
|
@yehudasa I updated the patch with your comments (tenant handling, const strings refs for input params, calling get_bucket_info() much earlier, checking permissions of the source object, etc) The param stuff and early errors are handled in get_params now. Checking permissions of the source object is handled in verify_permissions. I had a look in RGWCopyObj::verify_permission() to see how permission checking could be done in this case. I would appreciate if you double-check the RGWPutObj::verify_permission() code cause I am not sure if it is the right approach to verify or some more proper interface is in place. |
|
@yehudasa added comment there. thanks! |
|
@jmunhoz closing this one as there's another PR opened for this |
| dump_content_length(s, 0); | ||
| if (!s->info.env->get("HTTP_X_AMZ_COPY_SOURCE")) { | ||
| dump_etag(s, etag.c_str()); | ||
| dump_content_length(s, 0); |
There was a problem hiding this comment.
why are we setting the content length to 0 in the response header?
Add multipart uploads copy part feature.
Fixes: #12790
Signed-off-by: Javier M. Mellid jmunhoz@igalia.com