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.

Sign up for GitHub

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

Jump to bottom

rgw: fix copy object issue #30866

Closed
wants to merge 1 commit into from
Closed

rgw: fix copy object issue #30866

wants to merge 1 commit into from

Conversation

DHB-liuhong
Copy link
Contributor

@DHB-liuhong DHB-liuhong commented Oct 11, 2019 •
edited

rgw:fix copy object successfully when the value of CopySourceIfMatch does not match its entity tag(ETag)
rgw:fix error return code when the value of CopySourceIfNoneMatch match its entity tag(ETag).

Signed-off-by: DHB-liuhong liuhong@cmss.chinamobile.com

@DHB-liuhong
Copy link
Contributor Author

DHB-liuhong commented Oct 11, 2019 •
edited

when use boto3 test copyobject function, found that copy object successfully when the value of CopySourceIfMatch does not match its entity tag(ETag).

  • boto3 python script-CopySourceIfMatch
    res=s3_client.copy_object(Bucket='bucket02', Key='file1-match-2', CopySource={'Bucket': 'bucket01', 'Key': 'file1'}, CopySourceIfMatch='be3702781230207e1a92c50b5fb56c29')
  • aws test-CopySourceIfMatch
    image
    when the value of CopySourceIfMatch does not match its entity tag(ETag). it returns 412.
  • ceph master test-CopySourceIfMatch
    image
    when the value of CopySourceIfMatch does not match its entity tag(ETag). it returns 200.
  • add error message
  • boto3 python script-CopySourceIfNoneMatch
    res=s3_client.copy_object(Bucket='lh-copy2', Key='file1-1', CopySource={'Bucket': 'lh-copy1', 'Key': 'file1'}, CopySourceIfNoneMatch='a1b1b825e928109c2e6120d877865dfc')
  • aws test-CopySourceIfNoneMatch
    when the value of CopySourceIfNoneMatch match its entity tag(ETag). it returns 412.
  • ceph master test-CopySourceIfNoneMatch
    when the value of CopySourceIfNoneMatch match its entity tag(ETag). it returns 200.

@DHB-liuhong DHB-liuhong changed the title rgw:fix copy object successfully when the value of CopySourceIfMatch … rgw:fix copy object issue Oct 12, 2019
@cbodley
Copy link
Contributor

cbodley commented Oct 23, 2019

thanks @DHB-liuhong! this appears to resolve https://tracker.ceph.com/issues/40808. radosgw has been failing the test cases from ceph/s3-tests#96, so i'll use those tests to validate your changes

cbodley
cbodley requested changes Oct 23, 2019
View reviewed changes
src/rgw/rgw_rados.cc Outdated
@@ -5637,7 +5637,7 @@ int RGWRados::Object::Read::prepare(optional_yield y)
string if_nomatch_str = rgw_string_unquote(conds.if_nomatch);
ldout(cct, 10) << "ETag: " << string(etag.c_str(), etag.length()) << " " << " If-NoMatch: " << if_nomatch_str << dendl;
if (if_nomatch_str.compare(0, etag.length(), etag.c_str(), etag.length()) == 0) {
return -ERR_NOT_MODIFIED;
return -ERR_PRECONDITION_FAILED;
Copy link
Contributor

Choose a reason for hiding this comment

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

this change looks correct for the CopyObj case, but not for the general case of GetObj with If-None-Match according to https://tools.ietf.org/html/rfc7232#section-3.2:

An origin server MUST NOT perform the requested method if the
condition evaluates to false; instead, the origin server MUST respond
with either a) the 304 (Not Modified) status code if the request
method is GET or HEAD or b) the 412 (Precondition Failed) status code
for all other request methods.

i feel like it's appropriate for RGWRados::Object::Read::prepare() to return -ERR_NOT_MODIFIED here so the GetObj case continues to work, but RGWRados::copy_obj() should map this -ERR_NOT_MODIFIED error to -ERR_PRECONDITION_FAILED

what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, @cbodley you are right, i have changed it.

@DHB-liuhong
Copy link
Contributor Author

@cbodley Please review it for me. Thanks

@cbodley
Copy link
Contributor

cbodley commented Nov 5, 2019

thanks, it looks great and the s3tests are passing 👍

could you please squash the changes into a single commit and add Fixes: https://tracker.ceph.com/issues/40808 to the commit message?

@DHB-liuhong
Copy link
Contributor Author

@cbodley i have done it.

@stale
Copy link

stale bot commented Jan 6, 2020

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Jan 6, 2020
@dang
Copy link
Contributor

dang commented Jan 6, 2020

Not stale.

@stale stale bot removed the stale label Jan 6, 2020
@liewegas liewegas changed the title rgw:fix copy object issue rgw: fix copy object issue Jan 22, 2020
@cbodley
Copy link
Contributor

cbodley commented Jan 23, 2020

@DHB-liuhong could you please rebase to resolve the merge conflict?

@stale
Copy link

stale bot commented Mar 23, 2020

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Mar 23, 2020
@DHB-liuhong
Copy link
Contributor Author

ok .i will deal it

@stale stale bot removed the stale label Apr 13, 2020
@DHB-liuhong
rgw:fix copy object successfully when the value of CopySourceIfMatch …
eaee74b 
…does not match its entity tag(ETag)

    or CopySourceIfNoneMatch match its entity tag

Fixes: https://tracker.ceph.com/issues/40808

Signed-off-by: DHB-liuhong <liuhong@cmss.chinamobile.com>
@DHB-liuhong
Copy link
Contributor Author

i have done it.

@stale
Copy link

stale bot commented Jul 11, 2020

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Jul 11, 2020
@stale
Copy link

stale bot commented Oct 10, 2020

This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution!

@stale stale bot closed this Oct 10, 2020
@mattbenjamin
Copy link
Contributor

should this be stale?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbodley cbodley cbodley approved these changes

Assignees
No one assigned
Labels
bug-fix needs-rebase rgw stale
Projects
None yet
Milestone
No milestone
6 participants
@DHB-liuhong @cbodley @dang @mattbenjamin @liewegas @batrick