{"action":"created","issue":{"url":"https://api.github.com/repos/ceph/ceph/issues/5139","labels_url":"https://api.github.com/repos/ceph/ceph/issues/5139/labels{/name}","comments_url":"https://api.github.com/repos/ceph/ceph/issues/5139/comments","events_url":"https://api.github.com/repos/ceph/ceph/issues/5139/events","html_url":"https://github.com/ceph/ceph/pull/5139","id":92828562,"number":5139,"title":"rgw: add minimum support for copy multipart part","user":{"login":"jmunhoz","id":6533548,"avatar_url":"https://avatars.githubusercontent.com/u/6533548?v=3","gravatar_id":"","url":"https://api.github.com/users/jmunhoz","html_url":"https://github.com/jmunhoz","followers_url":"https://api.github.com/users/jmunhoz/followers","following_url":"https://api.github.com/users/jmunhoz/following{/other_user}","gists_url":"https://api.github.com/users/jmunhoz/gists{/gist_id}","starred_url":"https://api.github.com/users/jmunhoz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jmunhoz/subscriptions","organizations_url":"https://api.github.com/users/jmunhoz/orgs","repos_url":"https://api.github.com/users/jmunhoz/repos","events_url":"https://api.github.com/users/jmunhoz/events{/privacy}","received_events_url":"https://api.github.com/users/jmunhoz/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/ceph/ceph/labels/performance","name":"performance","color":"bfdadc"},{"url":"https://api.github.com/repos/ceph/ceph/labels/rgw","name":"rgw","color":"207de5"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":2,"created_at":"2015-07-03T09:06:40Z","updated_at":"2015-07-10T23:25:44Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/ceph/ceph/pulls/5139","html_url":"https://github.com/ceph/ceph/pull/5139","diff_url":"https://github.com/ceph/ceph/pull/5139.diff","patch_url":"https://github.com/ceph/ceph/pull/5139.patch"},"body":"Add minimum support in order to return a NotImplemented error code,\r\nrather than NoSuchKey.\r\n\r\nIt adds a new step in the process request processing flow in order to\r\ncheck if the requested feature is supported (fully or partially). This\r\nway, it can return a NotImplemented error as soon as possible if needed.\r\n\r\nThis approach improves the performance per request. The NotImplemented\r\ncheck happens as soon as possible. Server side doesn't need to compute\r\nrequest authorization on NotImplemented features.\r\n\r\nRelated: #7817\r\n\r\nSigned-off-by: Javier M. Mellid <jmunhoz@igalia.com>"},"comment":{"url":"https://api.github.com/repos/ceph/ceph/issues/comments/120549768","html_url":"https://github.com/ceph/ceph/pull/5139#issuecomment-120549768","issue_url":"https://api.github.com/repos/ceph/ceph/issues/5139","id":120549768,"user":{"login":"jmunhoz","id":6533548,"avatar_url":"https://avatars.githubusercontent.com/u/6533548?v=3","gravatar_id":"","url":"https://api.github.com/users/jmunhoz","html_url":"https://github.com/jmunhoz","followers_url":"https://api.github.com/users/jmunhoz/followers","following_url":"https://api.github.com/users/jmunhoz/following{/other_user}","gists_url":"https://api.github.com/users/jmunhoz/gists{/gist_id}","starred_url":"https://api.github.com/users/jmunhoz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jmunhoz/subscriptions","organizations_url":"https://api.github.com/users/jmunhoz/orgs","repos_url":"https://api.github.com/users/jmunhoz/repos","events_url":"https://api.github.com/users/jmunhoz/events{/privacy}","received_events_url":"https://api.github.com/users/jmunhoz/received_events","type":"User","site_admin":false},"created_at":"2015-07-10T23:25:44Z","updated_at":"2015-07-10T23:25:44Z","body":"@yehudasa adding the check to RGWCopyObj_ObjStore_S3::get_params() feels right and it is coherent with the rest of the code. I tried this implementation in order to share to you some points you might want to consider.\r\n\r\nThe first point is failing as soon as possible in front of non implemented features. Bearing in mind we are catching up AWS' S3 and its spec is not under our control it is possible we lack of full or partial implementations frequently. Failing in get_params() could be a bit late though. Having a look in the get_params's backtrace it shows the following:\r\n\r\n<pre>\r\nBreakpoint 1, RGWCopyObj_ObjStore_S3::get_params (this=0x7f471c1ee510) at rgw/rgw_rest_s3.cc:1425\r\n1425\t  if_mod = s->info.env->get(\"HTTP_X_AMZ_COPY_IF_MODIFIED_SINCE\");\r\n(gdb) bt\r\n#0  RGWCopyObj_ObjStore_S3::get_params (this=0x7f471c1ee510) at rgw/rgw_rest_s3.cc:1425\r\n#1  0x0000000000b46e32 in RGWCopyObj::verify_permission (this=0x7f471c1ee510) at rgw/rgw_op.cc:2269\r\n#2  0x0000000000a5e010 in process_request (store=0x3ebe250, rest=0x7fffe2dbab10, req=0x7f471c03cd30, client_io=0x7f47137fca40, olog=0x0) at rgw/rgw_main.cc:616\r\n</pre>\r\n\r\nIt would fail in op's verify_permission. It walked along the following logic in the process_request: authorization, permissions reading, op initialization and op mask verification.\r\n\r\nIn that point the server authorized the request (running multiples hashes and using a future AWS4 auth implementation even heavier that AWS2), pulling user info to check, initializing quota, etc. just to fail due to lack of implementation.\r\n\r\nAdding an op's \"implemented\" method looks natural to me. It looks the right place to fail ASAP placing code checking lack of implementation only.\r\n\r\nget_params() looks more natural to retrieve, check and save parameters coming in a valid and supported request only.\r\n\r\nIn some way, if we can factorize out the implementation checks on this op's early method I see some good advantages:\r\n\r\n * code fails in a simple and clear way\r\n * code doesn't generate cluster traffic (gathering user permissions, quota stuff, etc).\r\n * It fails as soon as possible avoiding extra computation.\r\n * feature support checks are as closed as possible to REST interface\r\n * code checking lack of implementation is not mixed with operational code. It should be more maintainable and you know with a simple look the features don't supported in the Ceph object gateway S3 API\r\n\r\nBy the way, I am Okay adding the check in RGWCopyObj_ObjStore_S3::get_params() if it looks the most suitable solution currently."}}