From e0d18690683f77ea65367ccfa27d6933a59a7a4a Mon Sep 17 00:00:00 2001 From: Samuel Merritt Date: Thu, 7 Dec 2017 17:25:26 -0800 Subject: [PATCH] Fix suffix-byte-range responses for zero-byte EC objects. The object servers are correctly returning 200s for such objects, but we were misinterpreting the result in the proxy. We had assumed that a satisfiable byte-range contained at least one byte, which seems reasonable unless you gaze long into RFC 7233. Suffix byte ranges (e.g. "bytes=-32123") are not asking for the last N bytes of an object; they are asking for *up to* the last N bytes, or the whole thing if fewer than N bytes are available. In the EC machinery, we had code that assumed "has no bytes" == "unsatisfiable", which is not true in that specific case. Now we correctly handle a suffix-byte-range request that is satisfiable but receives zero bytes. Change-Id: I8295a6c1436f50f86a4c626d87de6bfedd74ab09 Closes-Bug: 1736840 --- swift/proxy/controllers/obj.py | 55 ++++++++++++++++++++++------------ test/unit/proxy/test_server.py | 11 ++++++- 2 files changed, 46 insertions(+), 20 deletions(-) diff --git a/swift/proxy/controllers/obj.py b/swift/proxy/controllers/obj.py index f5c2dfb4f8..52687e6311 100644 --- a/swift/proxy/controllers/obj.py +++ b/swift/proxy/controllers/obj.py @@ -1043,20 +1043,30 @@ class ECAppIter(object): return headers, frag_iters def _actual_range(self, req_start, req_end, entity_length): + # Takes 3 args: (requested-first-byte, requested-last-byte, + # actual-length). + # + # Returns a 3-tuple (first-byte, last-byte, satisfiable). + # + # It is possible to get (None, None, True). This means that the last + # N>0 bytes of a 0-byte object were requested, and we are able to + # satisfy that request by returning nothing. try: rng = Range("bytes=%s-%s" % ( req_start if req_start is not None else '', req_end if req_end is not None else '')) except ValueError: - return (None, None) + return (None, None, False) rfl = rng.ranges_for_length(entity_length) - if not rfl: - return (None, None) + if rfl and entity_length == 0: + return (None, None, True) + elif not rfl: + return (None, None, False) else: # ranges_for_length() adds 1 to the last byte's position # because webob once made a mistake - return (rfl[0][0], rfl[0][1] - 1) + return (rfl[0][0], rfl[0][1] - 1, True) def _fill_out_range_specs_from_obj_length(self, range_specs): # Add a few fields to each range spec: @@ -1083,21 +1093,23 @@ class ECAppIter(object): # _fill_out_range_specs_from_fa_length() requires the beginnings of # the response bodies. for spec in range_specs: - cstart, cend = self._actual_range( + cstart, cend, csat = self._actual_range( spec['req_client_start'], spec['req_client_end'], self.obj_length) spec['resp_client_start'] = cstart spec['resp_client_end'] = cend - spec['satisfiable'] = (cstart is not None and cend is not None) + spec['satisfiable'] = csat - sstart, send = self._actual_range( + sstart, send, _junk = self._actual_range( spec['req_segment_start'], spec['req_segment_end'], self.obj_length) seg_size = self.policy.ec_segment_size - if spec['req_segment_start'] is None and sstart % seg_size != 0: + if (spec['req_segment_start'] is None and + sstart is not None and + sstart % seg_size != 0): # Segment start may, in the case of a suffix request, need # to be rounded up (not down!) to the nearest segment boundary. # This reflects the trimming of leading garbage (partial @@ -1119,7 +1131,7 @@ class ECAppIter(object): # are omitted from the response entirely and also to put the right # Content-Range headers in a multipart/byteranges response. for spec in range_specs: - fstart, fend = self._actual_range( + fstart, fend, _junk = self._actual_range( spec['req_fragment_start'], spec['req_fragment_end'], fa_length) @@ -1281,16 +1293,21 @@ class ECAppIter(object): client_end = (min(client_end, self.obj_length - 1) if client_end is not None else self.obj_length - 1) - num_segments = int( - math.ceil(float(segment_end + 1 - segment_start) - / self.policy.ec_segment_size)) - # We get full segments here, but the client may have requested a - # byte range that begins or ends in the middle of a segment. - # Thus, we have some amount of overrun (extra decoded bytes) - # that we trim off so the client gets exactly what they - # requested. - start_overrun = client_start - segment_start - end_overrun = segment_end - client_end + if segment_start is None: + num_segments = 0 + start_overrun = 0 + end_overrun = 0 + else: + num_segments = int( + math.ceil(float(segment_end + 1 - segment_start) + / self.policy.ec_segment_size)) + # We get full segments here, but the client may have requested a + # byte range that begins or ends in the middle of a segment. + # Thus, we have some amount of overrun (extra decoded bytes) + # that we trim off so the client gets exactly what they + # requested. + start_overrun = client_start - segment_start + end_overrun = segment_end - client_end for i, next_seg in enumerate(segment_iter): # We may have a start_overrun of more than one segment in diff --git a/test/unit/proxy/test_server.py b/test/unit/proxy/test_server.py index 933817c5ae..2143b07cb9 100644 --- a/test/unit/proxy/test_server.py +++ b/test/unit/proxy/test_server.py @@ -6975,6 +6975,7 @@ class TestObjectECRangedGET(unittest.TestCase): cls.obj_name = 'range-get-test' cls.tiny_obj_name = 'range-get-test-tiny' cls.aligned_obj_name = 'range-get-test-aligned' + cls.zero_byte_obj_name = 'range-get-test-zero-byte' # Note: only works if called with unpatched policies prolis = _test_sockets[0] @@ -7010,7 +7011,8 @@ class TestObjectECRangedGET(unittest.TestCase): for obj_name, obj in ((cls.obj_name, cls.obj), (cls.tiny_obj_name, cls.tiny_obj), - (cls.aligned_obj_name, cls.aligned_obj)): + (cls.aligned_obj_name, cls.aligned_obj), + (cls.zero_byte_obj_name, b"")): sock = connect_tcp(('localhost', prolis.getsockname()[1])) fd = sock.makefile() fd.write('PUT /v1/a/ec-con/%s HTTP/1.1\r\n' @@ -7233,6 +7235,13 @@ class TestObjectECRangedGET(unittest.TestCase): self.assertEqual(headers['Content-Range'], 'bytes 8092-8191/8192') self.assertEqual(len(gotten_obj), 100) + def test_suffix_zero_byte_object(self): + status, headers, gotten_obj = self._get_obj("bytes=-100", + self.zero_byte_obj_name) + self.assertEqual(status, 200) + self.assertEqual(len(gotten_obj), 0) + self.assertEqual(gotten_obj, b"") + def test_suffix_two_segs(self): # Ask for enough data that we need the last two segments. The last # segment is short, though, so this ensures we compensate for that.