From 12dd408823df158359e99fb01716f2059140c5c9 Mon Sep 17 00:00:00 2001 From: Alistair Coles Date: Wed, 16 Mar 2016 17:41:30 +0000 Subject: [PATCH] Put correct Etag and Accept-Ranges in EC 304 and 416 responses When using an EC policy, 304 responses to conditional GETs are missing the Accept-Ranges header and have the wrong ETag value. 412 responses also have the wrong etag. 416 responses to ranged GETs also have the wrong ETag. This patch ensures behaviour with EC policy is consistent with replication policy: - 304 and 416 responses have correct etag and Accept-Ranges - 412 responses have correct Etag but no Accept-Ranges Co-Authored-By: Mahati Chamarthy Co-Authored-By: Thiago da Silva Closes-Bug: #1496234 Closes-Bug: #1558197 Closes-Bug: #1558193 Change-Id: Ic21317b9e4f632f0751133a3383eb5487379e11f --- swift/common/swob.py | 2 +- swift/proxy/controllers/obj.py | 20 +++++---- test/functional/tests.py | 54 +++++++++++++++++++------ test/unit/proxy/controllers/test_obj.py | 10 +++-- test/unit/proxy/test_server.py | 54 +++++++++++++++++++------ 5 files changed, 103 insertions(+), 37 deletions(-) diff --git a/swift/common/swob.py b/swift/common/swob.py index 98ee37278e..704212084d 100644 --- a/swift/common/swob.py +++ b/swift/common/swob.py @@ -1299,7 +1299,7 @@ class Response(object): object length and body or app_iter to reset the content_length properties on the request. - It is ok to not call this method, the conditional resposne will be + It is ok to not call this method, the conditional response will be maintained for you when you __call__ the response. """ self.response_iter = self._response_iter(self.app_iter, self._body) diff --git a/swift/proxy/controllers/obj.py b/swift/proxy/controllers/obj.py index dea29eab3a..731a6d3aea 100644 --- a/swift/proxy/controllers/obj.py +++ b/swift/proxy/controllers/obj.py @@ -60,10 +60,10 @@ from swift.common.exceptions import ChunkReadTimeout, \ from swift.common.header_key_dict import HeaderKeyDict from swift.common.http import ( is_informational, is_success, is_client_error, is_server_error, - HTTP_CONTINUE, HTTP_CREATED, HTTP_MULTIPLE_CHOICES, + is_redirection, HTTP_CONTINUE, HTTP_CREATED, HTTP_MULTIPLE_CHOICES, HTTP_INTERNAL_SERVER_ERROR, HTTP_SERVICE_UNAVAILABLE, HTTP_INSUFFICIENT_STORAGE, HTTP_PRECONDITION_FAILED, HTTP_CONFLICT, - HTTP_UNPROCESSABLE_ENTITY) + HTTP_UNPROCESSABLE_ENTITY, HTTP_REQUESTED_RANGE_NOT_SATISFIABLE) from swift.common.storage_policy import (POLICIES, REPL_POLICY, EC_POLICY, ECDriverError, PolicyError) from swift.proxy.controllers.base import Controller, delay_denial, \ @@ -2065,8 +2065,12 @@ class ECObjectController(BaseObjectController): headers=resp_headers, conditional_response=True, app_iter=app_iter) - resp.accept_ranges = 'bytes' - app_iter.kickoff(req, resp) + try: + app_iter.kickoff(req, resp) + except HTTPException as err_resp: + # catch any HTTPException response here so that we can + # process response headers uniformly in _fix_response + resp = err_resp else: statuses = [] reasons = [] @@ -2086,10 +2090,12 @@ class ECObjectController(BaseObjectController): def _fix_response(self, resp): # EC fragment archives each have different bytes, hence different # etags. However, they all have the original object's etag stored in - # sysmeta, so we copy that here so the client gets it. + # sysmeta, so we copy that here (if it exists) so the client gets it. + resp.headers['Etag'] = resp.headers.get('X-Object-Sysmeta-Ec-Etag') + if (is_success(resp.status_int) or is_redirection(resp.status_int) or + resp.status_int == HTTP_REQUESTED_RANGE_NOT_SATISFIABLE): + resp.accept_ranges = 'bytes' if is_success(resp.status_int): - resp.headers['Etag'] = resp.headers.get( - 'X-Object-Sysmeta-Ec-Etag') resp.headers['Content-Length'] = resp.headers.get( 'X-Object-Sysmeta-Ec-Content-Length') resp.fix_conditional_response() diff --git a/test/functional/tests.py b/test/functional/tests.py index 4831c28b45..4dbfb67c5a 100644 --- a/test/functional/tests.py +++ b/test/functional/tests.py @@ -90,6 +90,14 @@ class Base(unittest2.TestCase): 'Status returned: %d Expected: %s' % (self.env.conn.response.status, status_or_statuses)) + def assert_header(self, header_name, expected_value): + try: + actual_value = self.env.conn.response.getheader(header_name) + except KeyError: + self.fail( + 'Expected header name %r not found in response.' % header_name) + self.assertEqual(expected_value, actual_value) + class Base2(object): def setUp(self): @@ -1640,32 +1648,35 @@ class TestFile(Base): self.assert_status(416) else: self.assertEqual(file_item.read(hdrs=hdrs), data[-i:]) + self.assert_header('etag', file_item.md5) + self.assert_header('accept-ranges', 'bytes') range_string = 'bytes=%d-' % (i) hdrs = {'Range': range_string} - self.assertTrue( - file_item.read(hdrs=hdrs) == data[i - file_length:], + self.assertEqual( + file_item.read(hdrs=hdrs), data[i - file_length:], range_string) range_string = 'bytes=%d-%d' % (file_length + 1000, file_length + 2000) hdrs = {'Range': range_string} self.assertRaises(ResponseError, file_item.read, hdrs=hdrs) self.assert_status(416) + self.assert_header('etag', file_item.md5) + self.assert_header('accept-ranges', 'bytes') range_string = 'bytes=%d-%d' % (file_length - 1000, file_length + 2000) hdrs = {'Range': range_string} - self.assertTrue( - file_item.read(hdrs=hdrs) == data[-1000:], range_string) + self.assertEqual(file_item.read(hdrs=hdrs), data[-1000:], range_string) hdrs = {'Range': '0-4'} - self.assertTrue(file_item.read(hdrs=hdrs) == data, range_string) + self.assertEqual(file_item.read(hdrs=hdrs), data, '0-4') # RFC 2616 14.35.1 # "If the entity is shorter than the specified suffix-length, the # entire entity-body is used." range_string = 'bytes=-%d' % (file_length + 10) hdrs = {'Range': range_string} - self.assertTrue(file_item.read(hdrs=hdrs) == data, range_string) + self.assertEqual(file_item.read(hdrs=hdrs), data, range_string) def testMultiRangeGets(self): file_length = 10000 @@ -2536,6 +2547,7 @@ class TestFileComparison(Base): hdrs = {'If-Match': 'bogus'} self.assertRaises(ResponseError, file_item.read, hdrs=hdrs) self.assert_status(412) + self.assert_header('etag', file_item.md5) def testIfMatchMultipleEtags(self): for file_item in self.env.files: @@ -2545,6 +2557,7 @@ class TestFileComparison(Base): hdrs = {'If-Match': '"bogus1", "bogus2", "bogus3"'} self.assertRaises(ResponseError, file_item.read, hdrs=hdrs) self.assert_status(412) + self.assert_header('etag', file_item.md5) def testIfNoneMatch(self): for file_item in self.env.files: @@ -2554,6 +2567,8 @@ class TestFileComparison(Base): hdrs = {'If-None-Match': file_item.md5} self.assertRaises(ResponseError, file_item.read, hdrs=hdrs) self.assert_status(304) + self.assert_header('etag', file_item.md5) + self.assert_header('accept-ranges', 'bytes') def testIfNoneMatchMultipleEtags(self): for file_item in self.env.files: @@ -2564,6 +2579,8 @@ class TestFileComparison(Base): '"bogus1", "bogus2", "%s"' % file_item.md5} self.assertRaises(ResponseError, file_item.read, hdrs=hdrs) self.assert_status(304) + self.assert_header('etag', file_item.md5) + self.assert_header('accept-ranges', 'bytes') def testIfModifiedSince(self): for file_item in self.env.files: @@ -2574,8 +2591,12 @@ class TestFileComparison(Base): hdrs = {'If-Modified-Since': self.env.time_new} self.assertRaises(ResponseError, file_item.read, hdrs=hdrs) self.assert_status(304) + self.assert_header('etag', file_item.md5) + self.assert_header('accept-ranges', 'bytes') self.assertRaises(ResponseError, file_item.info, hdrs=hdrs) self.assert_status(304) + self.assert_header('etag', file_item.md5) + self.assert_header('accept-ranges', 'bytes') def testIfUnmodifiedSince(self): for file_item in self.env.files: @@ -2586,8 +2607,10 @@ class TestFileComparison(Base): hdrs = {'If-Unmodified-Since': self.env.time_old_f2} self.assertRaises(ResponseError, file_item.read, hdrs=hdrs) self.assert_status(412) + self.assert_header('etag', file_item.md5) self.assertRaises(ResponseError, file_item.info, hdrs=hdrs) self.assert_status(412) + self.assert_header('etag', file_item.md5) def testIfMatchAndUnmodified(self): for file_item in self.env.files: @@ -2599,33 +2622,38 @@ class TestFileComparison(Base): 'If-Unmodified-Since': self.env.time_new} self.assertRaises(ResponseError, file_item.read, hdrs=hdrs) self.assert_status(412) + self.assert_header('etag', file_item.md5) hdrs = {'If-Match': file_item.md5, 'If-Unmodified-Since': self.env.time_old_f3} self.assertRaises(ResponseError, file_item.read, hdrs=hdrs) self.assert_status(412) + self.assert_header('etag', file_item.md5) def testLastModified(self): file_name = Utils.create_name() content_type = Utils.create_name() - file = self.env.container.file(file_name) - file.content_type = content_type - resp = file.write_random_return_resp(self.env.file_size) + file_item = self.env.container.file(file_name) + file_item.content_type = content_type + resp = file_item.write_random_return_resp(self.env.file_size) put_last_modified = resp.getheader('last-modified') + etag = file_item.md5 - file = self.env.container.file(file_name) - info = file.info() + file_item = self.env.container.file(file_name) + info = file_item.info() self.assertIn('last_modified', info) last_modified = info['last_modified'] self.assertEqual(put_last_modified, info['last_modified']) hdrs = {'If-Modified-Since': last_modified} - self.assertRaises(ResponseError, file.read, hdrs=hdrs) + self.assertRaises(ResponseError, file_item.read, hdrs=hdrs) self.assert_status(304) + self.assert_header('etag', etag) + self.assert_header('accept-ranges', 'bytes') hdrs = {'If-Unmodified-Since': last_modified} - self.assertTrue(file.read(hdrs=hdrs)) + self.assertTrue(file_item.read(hdrs=hdrs)) class TestFileComparisonUTF8(Base2, TestFileComparison): diff --git a/test/unit/proxy/controllers/test_obj.py b/test/unit/proxy/controllers/test_obj.py index 08a0be9e98..56eadcedf3 100755 --- a/test/unit/proxy/controllers/test_obj.py +++ b/test/unit/proxy/controllers/test_obj.py @@ -2395,7 +2395,7 @@ class TestECObjController(BaseObjectControllerMixin, unittest.TestCase): self.assertEqual(resp.status_int, 201) def test_GET_with_invalid_ranges(self): - # reall body size is segment_size - 10 (just 1 segment) + # real body size is segment_size - 10 (just 1 segment) segment_size = self.policy.ec_segment_size real_body = ('a' * segment_size)[:-10] @@ -2407,7 +2407,7 @@ class TestECObjController(BaseObjectControllerMixin, unittest.TestCase): segment_size, '%s-' % (segment_size + 10)) def test_COPY_with_invalid_ranges(self): - # reall body size is segment_size - 10 (just 1 segment) + # real body size is segment_size - 10 (just 1 segment) segment_size = self.policy.ec_segment_size real_body = ('a' * segment_size)[:-10] @@ -2420,6 +2420,7 @@ class TestECObjController(BaseObjectControllerMixin, unittest.TestCase): def _test_invalid_ranges(self, method, real_body, segment_size, req_range): # make a request with range starts from more than real size. + body_etag = md5(real_body).hexdigest() req = swift.common.swob.Request.blank( '/v1/a/c/o', method=method, headers={'Destination': 'c1/o', @@ -2430,7 +2431,8 @@ class TestECObjController(BaseObjectControllerMixin, unittest.TestCase): node_fragments = zip(*fragment_payloads) self.assertEqual(len(node_fragments), self.replicas()) # sanity - headers = {'X-Object-Sysmeta-Ec-Content-Length': str(len(real_body))} + headers = {'X-Object-Sysmeta-Ec-Content-Length': str(len(real_body)), + 'X-Object-Sysmeta-Ec-Etag': body_etag} start = int(req_range.split('-')[0]) self.assertTrue(start >= 0) # sanity title, exp = swob.RESPONSE_REASONS[416] @@ -2453,6 +2455,8 @@ class TestECObjController(BaseObjectControllerMixin, unittest.TestCase): self.assertEqual(resp.status_int, 416) self.assertEqual(resp.content_length, len(range_not_satisfiable_body)) self.assertEqual(resp.body, range_not_satisfiable_body) + self.assertEqual(resp.etag, body_etag) + self.assertEqual(resp.headers['Accept-Ranges'], 'bytes') if __name__ == '__main__': diff --git a/test/unit/proxy/test_server.py b/test/unit/proxy/test_server.py index 8aba81ffb1..fc44033dcf 100644 --- a/test/unit/proxy/test_server.py +++ b/test/unit/proxy/test_server.py @@ -39,6 +39,7 @@ from swift.obj import diskfile import re import random from collections import defaultdict +import uuid import mock from eventlet import sleep, spawn, wsgi, listen, Timeout, debug @@ -2273,9 +2274,10 @@ class TestObjectController(unittest.TestCase): self.assertEqual(len(error_lines), 0) # sanity self.assertEqual(len(warn_lines), 0) # sanity - @unpatch_policies - def test_conditional_GET_ec(self): - self.put_container("ec", "ec-con") + def _test_conditional_GET(self, policy): + container_name = uuid.uuid4().hex + object_path = '/v1/a/%s/conditionals' % container_name + self.put_container(policy.name, container_name) obj = 'this object has an etag and is otherwise unimportant' etag = md5(obj).hexdigest() @@ -2285,13 +2287,13 @@ class TestObjectController(unittest.TestCase): prosrv = _test_servers[0] sock = connect_tcp(('localhost', prolis.getsockname()[1])) fd = sock.makefile() - fd.write('PUT /v1/a/ec-con/conditionals HTTP/1.1\r\n' + fd.write('PUT %s HTTP/1.1\r\n' 'Host: localhost\r\n' 'Connection: close\r\n' 'Content-Length: %d\r\n' 'X-Storage-Token: t\r\n' 'Content-Type: application/octet-stream\r\n' - '\r\n%s' % (len(obj), obj)) + '\r\n%s' % (object_path, len(obj), obj)) fd.flush() headers = readuntil2crlfs(fd) exp = 'HTTP/1.1 201' @@ -2300,55 +2302,79 @@ class TestObjectController(unittest.TestCase): for verb, body in (('GET', obj), ('HEAD', '')): # If-Match req = Request.blank( - '/v1/a/ec-con/conditionals', + object_path, environ={'REQUEST_METHOD': verb}, headers={'If-Match': etag}) resp = req.get_response(prosrv) self.assertEqual(resp.status_int, 200) self.assertEqual(resp.body, body) + self.assertEqual(etag, resp.headers.get('etag')) + self.assertEqual('bytes', resp.headers.get('accept-ranges')) req = Request.blank( - '/v1/a/ec-con/conditionals', + object_path, environ={'REQUEST_METHOD': verb}, headers={'If-Match': not_etag}) resp = req.get_response(prosrv) self.assertEqual(resp.status_int, 412) + self.assertEqual(etag, resp.headers.get('etag')) req = Request.blank( - '/v1/a/ec-con/conditionals', + object_path, environ={'REQUEST_METHOD': verb}, headers={'If-Match': "*"}) resp = req.get_response(prosrv) self.assertEqual(resp.status_int, 200) self.assertEqual(resp.body, body) + self.assertEqual(etag, resp.headers.get('etag')) + self.assertEqual('bytes', resp.headers.get('accept-ranges')) # If-None-Match req = Request.blank( - '/v1/a/ec-con/conditionals', + object_path, environ={'REQUEST_METHOD': verb}, headers={'If-None-Match': etag}) resp = req.get_response(prosrv) self.assertEqual(resp.status_int, 304) + self.assertEqual(etag, resp.headers.get('etag')) + self.assertEqual('bytes', resp.headers.get('accept-ranges')) req = Request.blank( - '/v1/a/ec-con/conditionals', + object_path, environ={'REQUEST_METHOD': verb}, headers={'If-None-Match': not_etag}) resp = req.get_response(prosrv) self.assertEqual(resp.status_int, 200) self.assertEqual(resp.body, body) + self.assertEqual(etag, resp.headers.get('etag')) + self.assertEqual('bytes', resp.headers.get('accept-ranges')) req = Request.blank( - '/v1/a/ec-con/conditionals', + object_path, environ={'REQUEST_METHOD': verb}, headers={'If-None-Match': "*"}) resp = req.get_response(prosrv) self.assertEqual(resp.status_int, 304) + self.assertEqual(etag, resp.headers.get('etag')) + self.assertEqual('bytes', resp.headers.get('accept-ranges')) + error_lines = prosrv.logger.get_lines_for_level('error') warn_lines = prosrv.logger.get_lines_for_level('warning') self.assertEqual(len(error_lines), 0) # sanity self.assertEqual(len(warn_lines), 0) # sanity + @unpatch_policies + def test_conditional_GET_ec(self): + policy = POLICIES[3] + self.assertEqual('erasure_coding', policy.policy_type) # sanity + self._test_conditional_GET(policy) + + @unpatch_policies + def test_conditional_GET_replication(self): + policy = POLICIES[0] + self.assertEqual('replication', policy.policy_type) # sanity + self._test_conditional_GET(policy) + @unpatch_policies def test_GET_ec_big(self): self.put_container("ec", "ec-con") @@ -6543,7 +6569,7 @@ class TestObjectECRangedGET(unittest.TestCase): str(s) for s in range(431)) assert seg_size * 4 > len(cls.obj) > seg_size * 3, \ "object is wrong number of segments" - + cls.obj_etag = md5(cls.obj).hexdigest() cls.tiny_obj = 'tiny, tiny object' assert len(cls.tiny_obj) < seg_size, "tiny_obj too large" @@ -6691,9 +6717,11 @@ class TestObjectECRangedGET(unittest.TestCase): def test_unsatisfiable(self): # Goes just one byte too far off the end of the object, so it's # unsatisfiable - status, _junk, _junk = self._get_obj( + status, headers, _junk = self._get_obj( "bytes=%d-%d" % (len(self.obj), len(self.obj) + 100)) self.assertEqual(status, 416) + self.assertEqual(self.obj_etag, headers.get('Etag')) + self.assertEqual('bytes', headers.get('Accept-Ranges')) def test_off_end(self): # Ranged GET that's mostly off the end of the object, but overlaps