openstack/swift
Code Issues Proposed changes

Put correct Etag and Accept-Ranges in EC 304 and 416 responses

Browse Source 
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 <mahati.chamarthy@gmail.com>
Co-Authored-By: Thiago da Silva <thiago@redhat.com>

Closes-Bug: #1496234
Closes-Bug: #1558197
Closes-Bug: #1558193
Change-Id: Ic21317b9e4f632f0751133a3383eb5487379e11f
This commit is contained in:
Alistair Coles 2016-03-16 17:41:30 +00:00
parent f2e344a4d9
commit 12dd408823
5 changed files with 103 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
swift/common/swob.py
View File

@ -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)

20
swift/proxy/controllers/obj.py
View File

@ -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()

54
test/functional/tests.py
View File

@ -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):

10
test/unit/proxy/controllers/test_obj.py
View File

@ -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__':

54
test/unit/proxy/test_server.py
View File

@ -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