From e8a80e874a5086e94c5ae93e3a9191cb813d1631 Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Sat, 29 Oct 2016 17:21:41 +0200 Subject: [PATCH] Let users know entity size in 416 responses If a user sends a Range header with no satisfiable ranges, we send back a 416 Requested Range Not Satisfiable response. Previously however, there would be no indication of the size of the object they were requesting, so they wouldn't know how to craft a satisfiable range. We *do* send a Content-Length, but it is (correctly) the length of the error message. The RFC [1] has an answer for this: > A server generating a 416 (Range Not Satisfiable) response to a > byte-range request SHOULD send a Content-Range header field with an > unsatisfied-range value, as in the following example: > > Content-Range: bytes */1234 > > The complete-length in a 416 response indicates the current length of > the selected representation. Now, we'll send a Content-Range header for all 416 responses, including those coming from the object server as well as those generated on a proxy because of the Range mangling required to support EC policies. [1] RFC 7233, section 4.2, although similar language was used in RFC 2616, sections 10.4.17 and 14.16 Change-Id: I80c7390fc6f84a10a212b0641bb07a64dfccbd45 --- swift/common/middleware/dlo.py | 7 ++++++- swift/common/swob.py | 2 ++ swift/proxy/controllers/obj.py | 3 +++ test/functional/test_dlo.py | 1 + test/functional/tests.py | 5 +++++ test/unit/common/middleware/test_dlo.py | 6 ++++++ test/unit/common/test_swob.py | 2 ++ test/unit/proxy/test_server.py | 18 ++++++++++++++++-- 8 files changed, 41 insertions(+), 3 deletions(-) diff --git a/swift/common/middleware/dlo.py b/swift/common/middleware/dlo.py index aa6057c042..4aa6c8e12b 100644 --- a/swift/common/middleware/dlo.py +++ b/swift/common/middleware/dlo.py @@ -263,7 +263,12 @@ class GetContext(WSGIContext): byteranges = req.range.ranges_for_length( content_length_for_swob_range) if not byteranges: - return HTTPRequestedRangeNotSatisfiable(request=req) + headers = {'Accept-Ranges': 'bytes'} + if have_complete_listing: + headers['Content-Range'] = 'bytes */%d' % ( + content_length_for_swob_range, ) + return HTTPRequestedRangeNotSatisfiable( + request=req, headers=headers) first_byte, last_byte = byteranges[0] # For some reason, swob.Range.ranges_for_length adds 1 to the # last byte's position. diff --git a/swift/common/swob.py b/swift/common/swob.py index 275db486d1..4976d28194 100644 --- a/swift/common/swob.py +++ b/swift/common/swob.py @@ -1250,6 +1250,8 @@ class Response(object): if ranges == []: self.status = 416 close_if_possible(app_iter) + self.headers['Content-Range'] = \ + 'bytes */%d' % self.content_length # Setting body + app_iter to None makes us emit the default # body text from RESPONSE_REASONS. body = None diff --git a/swift/proxy/controllers/obj.py b/swift/proxy/controllers/obj.py index 9e710c7d68..000c149c12 100644 --- a/swift/proxy/controllers/obj.py +++ b/swift/proxy/controllers/obj.py @@ -2310,6 +2310,9 @@ class ECObjectController(BaseObjectController): resp.headers['Content-Length'] = resp.headers.get( 'X-Object-Sysmeta-Ec-Content-Length') resp.fix_conditional_response() + if resp.status_int == HTTP_REQUESTED_RANGE_NOT_SATISFIABLE: + resp.headers['Content-Range'] = 'bytes */%s' % resp.headers[ + 'X-Object-Sysmeta-Ec-Content-Length'] def _make_putter(self, node, part, req, headers): return MIMEPutter.connect( diff --git a/test/functional/test_dlo.py b/test/functional/test_dlo.py index 36473434bd..a8159eaf14 100644 --- a/test/functional/test_dlo.py +++ b/test/functional/test_dlo.py @@ -135,6 +135,7 @@ class TestDlo(Base): self.assertRaises(ResponseError, file_item.read, size=7, offset=50) self.assert_status(416) + self.assert_header('content-range', 'bytes */50') def test_copy(self): # Adding a new segment, copying the manifest, and then deleting the diff --git a/test/functional/tests.py b/test/functional/tests.py index d335d1d35e..e5bde960eb 100644 --- a/test/functional/tests.py +++ b/test/functional/tests.py @@ -1888,8 +1888,11 @@ class TestFile(Base): # Otherwise, the byte-range-set is unsatisfiable. self.assertRaises(ResponseError, file_item.read, hdrs=hdrs) self.assert_status(416) + self.assert_header('content-range', 'bytes */%d' % file_length) else: self.assertEqual(file_item.read(hdrs=hdrs), data[-i:]) + self.assert_header('content-range', 'bytes %d-%d/%d' % ( + file_length - i, file_length - 1, file_length)) self.assert_header('etag', file_item.md5) self.assert_header('accept-ranges', 'bytes') @@ -1903,6 +1906,7 @@ class TestFile(Base): hdrs = {'Range': range_string} self.assertRaises(ResponseError, file_item.read, hdrs=hdrs) self.assert_status(416) + self.assert_header('content-range', 'bytes */%d' % file_length) self.assert_header('etag', file_item.md5) self.assert_header('accept-ranges', 'bytes') @@ -2067,6 +2071,7 @@ class TestFile(Base): self.assertRaises(ResponseError, file_item.read, hdrs=hdrs) self.assert_status(416) + self.assert_header('content-range', 'bytes */%d' % file_length) def testRangedGetsWithLWSinHeader(self): file_length = 10000 diff --git a/test/unit/common/middleware/test_dlo.py b/test/unit/common/middleware/test_dlo.py index 04fbc4e614..d14509e20d 100644 --- a/test/unit/common/middleware/test_dlo.py +++ b/test/unit/common/middleware/test_dlo.py @@ -411,6 +411,12 @@ class TestDloGetManifest(DloTestCase): headers={'Range': 'bytes=25-30'}) status, headers, body = self.call_dlo(req) self.assertEqual(status, "416 Requested Range Not Satisfiable") + expected_headers = ( + ('Accept-Ranges', 'bytes'), + ('Content-Range', 'bytes */25'), + ) + for header_value_pair in expected_headers: + self.assertIn(header_value_pair, headers) def test_get_range_many_segments_satisfiable(self): req = swob.Request.blank('/v1/AUTH_test/mancon/manifest-many-segments', diff --git a/test/unit/common/test_swob.py b/test/unit/common/test_swob.py index c0993d5d1e..92c5bc24f6 100644 --- a/test/unit/common/test_swob.py +++ b/test/unit/common/test_swob.py @@ -1304,6 +1304,7 @@ class TestResponse(unittest.TestCase): self.assertIn('The Range requested is not available', body) self.assertEqual(resp.content_length, len(body)) self.assertEqual(resp.status, '416 Requested Range Not Satisfiable') + self.assertEqual(resp.content_range, 'bytes */10') resp = swift.common.swob.Response( body='1234567890', request=req, @@ -1321,6 +1322,7 @@ class TestResponse(unittest.TestCase): body = ''.join(resp([], start_response)) self.assertEqual(body, '1234567890') self.assertEqual(resp.status, '200 OK') + self.assertNotIn('Content-Range', resp.headers) resp = swift.common.swob.Response( body='1234567890', request=req, diff --git a/test/unit/proxy/test_server.py b/test/unit/proxy/test_server.py index 393d007da3..08e9e2abd3 100644 --- a/test/unit/proxy/test_server.py +++ b/test/unit/proxy/test_server.py @@ -6159,12 +6159,26 @@ class TestObjectECRangedGET(unittest.TestCase): def test_unsatisfiable(self): # Goes just one byte too far off the end of the object, so it's - # unsatisfiable + # unsatisfiable. This should be close enough that the object servers + # actually responded 206 + obj_len = len(self.obj) status, headers, _junk = self._get_obj( - "bytes=%d-%d" % (len(self.obj), len(self.obj) + 100)) + "bytes=%d-%d" % (obj_len, obj_len + 100)) self.assertEqual(status, 416) self.assertEqual(self.obj_etag, headers.get('Etag')) self.assertEqual('bytes', headers.get('Accept-Ranges')) + self.assertIn('Content-Range', headers) + self.assertEqual('bytes */%d' % obj_len, headers['Content-Range']) + + # Goes *way* too far off the end of the object, so we're looking at + # the (massaged) 416 from an object server + status, headers, _junk = self._get_obj( + "bytes=%d-" % (obj_len + 2 ** 30)) + self.assertEqual(status, 416) + self.assertEqual(self.obj_etag, headers.get('Etag')) + self.assertEqual('bytes', headers.get('Accept-Ranges')) + self.assertIn('Content-Range', headers) + self.assertEqual('bytes */%d' % obj_len, headers['Content-Range']) def test_off_end(self): # Ranged GET that's mostly off the end of the object, but overlaps