Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rgw: fix 'copy part' without 'x-amz-copy-source-range' #20002

Merged
merged 1 commit into from Feb 2, 2018

Conversation

malc0lm
Copy link
Contributor

@malc0lm malc0lm commented Jan 18, 2018

http://tracker.ceph.com/issues/22729
Copying part without http header "x-amz-copy-source-range" will be mistaken for copying object.

@mattbenjamin
Copy link
Contributor

@malc0lm I think this should have a test case in s3tests; is that something you could provide?

@mattbenjamin mattbenjamin self-assigned this Jan 19, 2018
@malc0lm
Copy link
Contributor Author

malc0lm commented Jan 22, 2018

@mattbenjamin did you mean I need another pr which test copypart for ceph/s3-tests?

@malc0lm
Copy link
Contributor Author

malc0lm commented Jan 24, 2018

@mattbenjamin I had finished test case for copy part without "x-amz-copy-source-range", should I put a pr to ceph/s3-tests?

@mattbenjamin
Copy link
Contributor

mattbenjamin commented Jan 24, 2018 via email

Copy link
Contributor

@mattbenjamin mattbenjamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, I'll run provided tests today

@mattbenjamin
Copy link
Contributor

@malc0lm this needs a rebase--if you have time, could you send?

@@ -3275,7 +3275,7 @@ int RGWHandler_REST_S3::init(RGWRados *store, struct req_state *s,

const char *copy_source = s->info.env->get("HTTP_X_AMZ_COPY_SOURCE");

if (copy_source && !s->info.env->get("HTTP_X_AMZ_COPY_SOURCE_RANGE")) {
if (copy_source && !s->info.env->get("HTTP_X_AMZ_COPY_SOURCE_RANGE") && !s->info.args.exists("uploadId")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would format this as (long line, reading clarity):
if (copy_source &&
(! s->info.env->get("HTTP_X_AMZ_COPY_SOURCE_RANGE")) &&
(! s->info.args.exists("uploadId"))) {

@mattbenjamin
Copy link
Contributor

mergewise, rgw_op.cc:3381 needs something like
if ((! copy_source.empty()) &&
(!copy_source_range)) {

it will copy an entire source object

Signed-off-by: Malcolm Lee <fengxueyu35@126.com>
@malc0lm
Copy link
Contributor Author

malc0lm commented Jan 31, 2018

@mattbenjamin thanks for your reviewing and mergewise. I had rebased and updated commit, and will pulpito's fail be fixed automatically?

Copy link
Contributor

@mattbenjamin mattbenjamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mattbenjamin
Copy link
Contributor

@malc0lm had trouble testing this today due to an awsv4 signature failure not caused by this (of course); I am working with ali to run a boto3 version--will try to finish up tomorrow!

@malc0lm
Copy link
Contributor Author

malc0lm commented Feb 2, 2018

@mattbenjamin ok. thanks :)

@mattbenjamin
Copy link
Contributor

@malc0lm tests re-run with a fixed version of the boto3 s3-tests--will comment on that PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants