rgw: Fix incorrect content length and range for zero sized objects during range requests #10207

prallabh · 2016-07-08T10:56:32Z

Fixes: http://tracker.ceph.com/issues/16388

Signed-off-by: Pavan Rallabhandi PRallabhandi@walmartlabs.com

prallabh · 2016-07-11T10:50:07Z

@yehudasa can you please help review

prallabh · 2016-07-14T06:55:40Z

@cbodley, @oritwas can you please help review/assign as appropriate.

oritwas · 2016-07-14T10:12:25Z

This fixes the content-length header but the content-range is still not correct
I get with this fix for an empty object:
Content-Range: bytes 0--1/0

prallabh · 2016-07-14T10:18:44Z

That's true, should the range be 0-0/0 as well?

oritwas · 2016-07-14T12:03:30Z

it should be:
Content-Range: */0

prallabh · 2016-07-14T12:17:26Z

I guess you meant that to be whatever user specifies as the range? Thinking through this, wouldn't any range be irrelevant for zero sized objects?

oritwas · 2016-07-14T13:55:32Z

We need to respond with 416 (Range Not Satisfiable) and
Content-Range: */0

see http://httpwg.org/specs/rfc7233.html#status.416

oritwas · 2016-07-15T12:15:05Z

@prallabh , Lets return 416 and remove the content-range header . I think this will be the simplest

prallabh · 2016-07-16T07:02:01Z

@oritwas Thanks for the link, wasn't aware of it. Realized that we were returning 416 else where as well in the code, was in the process of building the content-range for this case. If we do not bother about the range header, will update the commit with just the content length and 416.

prallabh · 2016-07-16T15:32:24Z

@oritwas I ended up fixing the range header as per the RFC, please take a look at the same.

vsespb · 2016-07-18T11:24:13Z

Here is what Amazon S3 gives (yes, 416):

$ curl -v -H "Range: bytes=0-5242880" https://s3-eu-west-1.amazonaws.com/vsetest/s3test
* About to connect() to s3-eu-west-1.amazonaws.com port 443 (#0)
*   Trying 54.231.131.83... connected
* successfully set certificate verify locations:
*   CAfile: none
  CApath: /etc/ssl/certs
* SSLv3, TLS handshake, Client hello (1):
* SSLv3, TLS handshake, Server hello (2):
* SSLv3, TLS handshake, CERT (11):
* SSLv3, TLS handshake, Server key exchange (12):
* SSLv3, TLS handshake, Server finished (14):
* SSLv3, TLS handshake, Client key exchange (16):
* SSLv3, TLS change cipher, Client hello (1):
* SSLv3, TLS handshake, Finished (20):
* SSLv3, TLS change cipher, Client hello (1):
* SSLv3, TLS handshake, Finished (20):
* SSL connection using ECDHE-RSA-AES128-SHA
* Server certificate:
*    subject: C=US; ST=Washington; L=Seattle; O=Amazon.com Inc.; CN=*.s3-eu-west-1.amazonaws.com
*    start date: 2015-12-08 12:05:08 GMT
*    expire date: 2016-09-14 12:00:00 GMT
*    subjectAltName: s3-eu-west-1.amazonaws.com matched
*    issuer: C=US; O=DigiCert Inc; OU=www.digicert.com; CN=DigiCert Baltimore CA-2 G2
*    SSL certificate verify ok.
> GET /vsetest/s3test HTTP/1.1
> User-Agent: curl/7.22.0 (x86_64-pc-linux-gnu) libcurl/7.22.0 OpenSSL/1.0.1 zlib/1.2.3.4 libidn/1.23 librtmp/2.3
> Host: s3-eu-west-1.amazonaws.com
> Accept: */*
> Range: bytes=0-5242880
> 
< HTTP/1.1 416 Requested Range Not Satisfiable
< x-amz-request-id: EBA23471F0510B1A
< x-amz-id-2: RYTrIF6LmawFzMVTsRQd/5dqF3YRJ+8jof6WgXQ5EAr5oIkBorFcdOexrTqVnojpHHl1KMWkvZk=
< Content-Type: application/xml
< Transfer-Encoding: chunked
< Date: Mon, 18 Jul 2016 11:22:15 GMT
< Server: AmazonS3
< 
<?xml version="1.0" encoding="UTF-8"?>
* Connection #0 to host s3-eu-west-1.amazonaws.com left intact
* Closing connection #0
* SSLv3, TLS alert, Client hello (1):
<Error><Code>InvalidRange</Code><Message>The requested range is not satisfiable</Message><RangeRequested>bytes=0-5242880</RangeRequested><ActualObjectSize>0</ActualObjectSize><RequestId>EBA23471F0510B1A</RequestId><HostId>RYTrIF6LmawFzMVTsRQd/5dqF3YRJ+8jof6WgXQ5EAr5oIkBorFcdOexrTqVnojpHHl1KMWkvZk=</HostId></Error>

vsespb · 2016-07-18T11:26:02Z

I also wondering why you guys have travis "build", but don't write tests. Different condition-branch for handling zero is something tests designed for.

tchaikov · 2016-07-18T12:19:28Z

@vsespb we don't have travis build yet (but yes, we could). why do you say so?

vsespb · 2016-07-18T12:23:02Z

ok, jenkins, not travis.

prallabh · 2016-07-20T11:08:00Z

@oritwas ping

oritwas · 2016-07-20T11:11:31Z

lgtm, but it need more testing.
Can you add a test to s3tests for this?

prallabh · 2016-07-20T11:30:25Z

Thanks, do you mean to add a test under https://github.com/ceph/s3-tests. Have never done that, any pointers/documentation around best practices for that would help.

prallabh · 2016-07-22T11:18:25Z

@oritwas I have verified the fix with swift/s3 clients, can you please clarify on the test that you were referring to.

oritwas · 2016-07-22T12:55:30Z

I add a test to s3 tests for an empty object see ceph/s3-tests#119

could you try it an see if it passes?

prallabh · 2016-07-22T18:33:47Z

Thanks for the test, find below the result:

S3TEST_CONF=range.conf ./virtualenv/bin/nosetests s3tests.functional.test_s3:test_ranged_request_empty_object

.

Ran 1 test in 0.568s

OK

prallabh · 2016-07-25T06:23:55Z

@oritwas rebased.

prallabh · 2016-07-26T08:23:44Z

@oritwas ping

prallabh · 2016-07-27T12:51:43Z

@oritwas Realized that having the object size check in the Read::prepare would break while preforming operations like stat/download on a empty object (without a hint that the operation is a range request). Moved the checks ahead to RGWGetObj::execute where they should really be IMO. Please take a look, thanks!

oritwas · 2016-07-29T13:07:47Z

@prallabh can you rebase?

prallabh · 2016-07-29T14:42:21Z

@oritwas done

prallabh · 2016-08-01T05:45:35Z

@oritwas please let me know if there is anything pending from my side for merge.

oritwas · 2016-08-01T08:59:09Z

src/rgw/rgw_rest.cc

@@ -655,7 +655,9 @@ void end_header(struct req_state* s, RGWOp* op, const char *content_type,
      s->formatter->close_section();
    }
    s->formatter->output_footer();
-    dump_content_length(s, s->formatter->get_len());
+    if (s->obj_size) {


This seems to effect all error response and some require content-length. is it a problem to keep the content-length?

The content length here is that of the end header's and its not relevant in this context (I guess that should be true for all zero sized responses). For range requests on zero sized objects, without suppressing this header, the output looks something like this:

HTTP/1.1 416 Requested Range Not Satisfiable
Content-Range: bytes */0
Content-Length: 0
Accept-Ranges: bytes
X-Trans-Id: tx000000000000000000026-00579f3001-1011-default
Content-Length: 12
Accept-Ranges: bytes
Content-Type: text/plain; charset=utf-8
Date: Mon, 01 Aug 2016 11:18:25 GMT

you have here two content lengths headers,
it is because you add the content length even in case of an error see my comment on rgw_rest_swift.cc

oritwas · 2016-08-01T13:17:55Z

As both swift or AWS are not returning a content-range header in case of such error, we can do the same.
It will simplify the code just to return 416 error if the object is empty

prallabh · 2016-08-01T14:05:10Z

@oritwas Thanks, can you please check now if that is what you are expecting.

oritwas · 2016-08-01T14:12:00Z

src/rgw/rgw_rest_swift.cc

  dump_content_length(s, total_len);
+


small nit : extra empty line
other wise looks good

done, thanks!

Signed-off-by: Pavan Rallabhandi <PRallabhandi@walmartlabs.com>

prallabh · 2016-08-01T15:23:52Z

@oritwas updated and rebased, please help merge.

oritwas · 2016-08-01T16:22:01Z

looks good

oritwas added bug-fix rgw labels Jul 14, 2016

oritwas self-assigned this Jul 14, 2016

prallabh force-pushed the wip-16388 branch from 54d662b to ce63faa Compare July 16, 2016 15:30

oritwas added the needs-qa label Jul 20, 2016

prallabh force-pushed the wip-16388 branch from ce63faa to 9fb0f6f Compare July 25, 2016 05:22

prallabh force-pushed the wip-16388 branch from 9fb0f6f to 3b490da Compare July 26, 2016 07:11

prallabh force-pushed the wip-16388 branch from 3b490da to e67f9f9 Compare July 27, 2016 12:36

prallabh force-pushed the wip-16388 branch from e67f9f9 to 4e376cd Compare July 29, 2016 14:40

oritwas reviewed Aug 1, 2016
View reviewed changes

prallabh force-pushed the wip-16388 branch from 4e376cd to b79a74e Compare August 1, 2016 11:46

prallabh force-pushed the wip-16388 branch from b79a74e to eae6525 Compare August 1, 2016 14:03

oritwas reviewed Aug 1, 2016
View reviewed changes

root added 3 commits August 1, 2016 20:52

For zero sized objects, return 416 during range requests

a94ec15

Signed-off-by: Pavan Rallabhandi <PRallabhandi@walmartlabs.com>

For zero sized objects, let the range adhere as per the RFC

67bfbf4

Signed-off-by: Pavan Rallabhandi <PRallabhandi@walmartlabs.com>

For range requests, let the range be dumped before the error is honored.

a3b5a46

Signed-off-by: Pavan Rallabhandi <PRallabhandi@walmartlabs.com>

prallabh force-pushed the wip-16388 branch from eae6525 to a3b5a46 Compare August 1, 2016 15:23

oritwas assigned cbodley Aug 1, 2016

cbodley merged commit 4a90e64 into ceph:master Aug 1, 2016

cbodley mentioned this pull request Aug 12, 2016

Add range request to an empty object ceph/s3-tests#119

Merged

prallabh deleted the wip-16388 branch August 17, 2016 05:54

rgw: Fix incorrect content length and range for zero sized objects during range requests #10207

rgw: Fix incorrect content length and range for zero sized objects during range requests #10207

Conversation

prallabh commented Jul 8, 2016

prallabh commented Jul 11, 2016

prallabh commented Jul 14, 2016

oritwas commented Jul 14, 2016

prallabh commented Jul 14, 2016

oritwas commented Jul 14, 2016

prallabh commented Jul 14, 2016

oritwas commented Jul 14, 2016

oritwas commented Jul 15, 2016

prallabh commented Jul 16, 2016

prallabh commented Jul 16, 2016

vsespb commented Jul 18, 2016

vsespb commented Jul 18, 2016

tchaikov commented Jul 18, 2016 • edited

vsespb commented Jul 18, 2016

prallabh commented Jul 20, 2016

oritwas commented Jul 20, 2016

prallabh commented Jul 20, 2016

prallabh commented Jul 22, 2016 • edited

oritwas commented Jul 22, 2016

prallabh commented Jul 22, 2016

.

prallabh commented Jul 25, 2016

prallabh commented Jul 26, 2016

prallabh commented Jul 27, 2016 • edited

oritwas commented Jul 29, 2016

prallabh commented Jul 29, 2016

prallabh commented Aug 1, 2016

oritwas Aug 1, 2016

Choose a reason for hiding this comment

prallabh Aug 1, 2016 • edited

Choose a reason for hiding this comment

oritwas Aug 1, 2016

Choose a reason for hiding this comment

oritwas commented Aug 1, 2016 • edited

prallabh commented Aug 1, 2016

oritwas Aug 1, 2016 • edited

Choose a reason for hiding this comment

prallabh Aug 1, 2016

Choose a reason for hiding this comment

prallabh commented Aug 1, 2016

oritwas commented Aug 1, 2016

tchaikov commented Jul 18, 2016 •

edited

prallabh commented Jul 22, 2016 •

edited

prallabh commented Jul 27, 2016 •

edited

prallabh Aug 1, 2016 •

edited

oritwas commented Aug 1, 2016 •

edited

oritwas Aug 1, 2016 •

edited