From 4f69ab3c5dfaa8dcd321bdabb74dc079f7973d82 Mon Sep 17 00:00:00 2001 From: Anish Kachinthaya Date: Fri, 19 Apr 2024 15:12:31 -0700 Subject: [PATCH] fix x-open-expired 404 on HEAD?part-number reqs Fixes a bug with the x-open-expired feature where our magic header does not get copied when refetching all manifests that causes 404 on HEAD requests with part-number=N query parameter since the object-server returns an empty response body and the proxy needs to refetch. The fix also applies to segment GET requests if the segments have expired. Change-Id: If0382d433f73cc0333bb4d0319fe1487b7783e4c --- swift/common/middleware/slo.py | 4 +- swift/common/request_helpers.py | 4 +- test/functional/__init__.py | 1 + test/functional/test_slo.py | 167 +++++++++++++++++++++++- test/probe/test_object_expirer.py | 142 +++++++++++++++++--- test/unit/common/middleware/test_slo.py | 112 ++++++++++++++++ 6 files changed, 409 insertions(+), 21 deletions(-) diff --git a/swift/common/middleware/slo.py b/swift/common/middleware/slo.py index 0c4fe90fce..160a9ca485 100644 --- a/swift/common/middleware/slo.py +++ b/swift/common/middleware/slo.py @@ -980,9 +980,11 @@ class SloGetContext(WSGIContext): friendly_close(resp_iter) del req.environ['swift.non_client_disconnect'] + headers_subset = ['x-auth-token', 'x-open-expired'] get_req = make_subrequest( req.environ, method='GET', - headers={'x-auth-token': req.headers.get('x-auth-token')}, + headers={k: req.headers.get(k) + for k in headers_subset if k in req.headers}, agent='%(orig)s SLO MultipartGET', swift_source='SLO') resp_iter = self._app_call(get_req.environ) new_resp_attrs = RespAttrs.from_headers(self._response_headers) diff --git a/swift/common/request_helpers.py b/swift/common/request_helpers.py index 32cc7e3f61..c014bd8df9 100644 --- a/swift/common/request_helpers.py +++ b/swift/common/request_helpers.py @@ -563,8 +563,8 @@ class SegmentedIterable(object): path = quote(seg_path) + '?multipart-manifest=get' seg_req = make_subrequest( self.req.environ, path=path, method='GET', - headers={'x-auth-token': self.req.headers.get( - 'x-auth-token')}, + headers={h: self.req.headers.get(h) + for h in ('x-auth-token', 'x-open-expired')}, agent=('%(orig)s ' + self.ua_suffix), swift_source=self.swift_source) diff --git a/test/functional/__init__.py b/test/functional/__init__.py index 7ea35f7311..ef8ca9dc77 100644 --- a/test/functional/__init__.py +++ b/test/functional/__init__.py @@ -579,6 +579,7 @@ def in_process_setup(the_object_server=object_server): 'container_update_timeout': '3', 'allow_account_management': 'true', 'account_autocreate': 'true', + 'allow_open_expired': 'true', 'allow_versions': 'True', 'allow_versioned_writes': 'True', # Below are values used by the functional test framework, as well as diff --git a/test/functional/test_slo.py b/test/functional/test_slo.py index 21a855cfd2..dc931d9e1d 100644 --- a/test/functional/test_slo.py +++ b/test/functional/test_slo.py @@ -18,6 +18,7 @@ import base64 import email.parser import itertools import json +import time from copy import deepcopy from unittest import SkipTest @@ -25,8 +26,9 @@ import six from six.moves import urllib from swift.common.swob import normalize_etag -from swift.common.utils import md5 +from swift.common.utils import md5, config_true_value +from test.functional import check_response, retry import test.functional as tf from test.functional import cluster_info from test.functional.tests import Utils, Base, Base2, BaseEnv @@ -51,6 +53,12 @@ def group_file_contents(file_contents): for char, grp in itertools.groupby(byte_iter)] +def md5hex(s): + if not isinstance(s, bytes): + s = s.encode('ascii') + return md5(s, usedforsecurity=False).hexdigest() + + class TestSloEnv(BaseEnv): slo_enabled = None # tri-state: None initially, then True/False @@ -450,6 +458,163 @@ class TestSlo(Base): self.assertEqual(headers['x-parts-count'], '5') start = end + 1 + def test_x_delete_at_with_part_number_and_open_expired(self): + cont_name = self.env.account2.container(self.env.container.name) + allow_open_expired = config_true_value(tf.cluster_info['swift'].get( + 'allow_open_expired', 'false')) + + if not allow_open_expired: + raise SkipTest('allow_open_expired is disabled') + + # data for segments + segments = [b'one', b'two', b'three', b'four'] + etags = [] + for segment in segments: + etags.append(md5hex(segment)) + + def put_manifest(url, token, parsed, conn, object_segments): + now = int(time.time()) + delete_time = now + 2 + manifest_data = [] + start = 0 + + for segment_object in range(len(object_segments)): + size = len(object_segments[segment_object]) + end = start + size - 1 + manifest_data.append({ + 'path': '/%s/segments/%s' % (cont_name, + str(segment_object)), + 'etag': etags[segment_object], + 'size_bytes': size, + }) + start = end + 1 + + conn.request( + 'PUT', + '%s/%s/manifest?multipart-manifest=put' % (parsed.path, + cont_name), + body=json.dumps(manifest_data), + headers={ + 'X-Auth-Token': token, + 'X-Delete-At': delete_time, + 'X-Static-Large-Object': 'true', + 'Content-Type': 'application/json' + } + ) + resp = check_response(conn) + body = resp.read() + self.assertEqual(resp.status, 201, + "Response status is not 201: %s" % body) + + def put_segments(url, token, parsed, conn, object_segments): + now = int(time.time()) + delete_time = now + 2 + + for objnum in range(len(object_segments)): + conn.request('PUT', '%s/%s/segments/%s' % ( + parsed.path, + cont_name, + str(objnum)), + body=object_segments[objnum], + headers={ + 'X-Auth-Token': token, + 'X-Delete-At': delete_time}) + resp = check_response(conn) + body = resp.read() + self.assertEqual(resp.status, 201, + "Response status is not 201: %s" % body) + + retry(put_segments, segments) + retry(put_manifest, segments) + + # get the manifest + def get_manifest(url, token, parsed, conn, extra_headers=None): + headers = {'X-Auth-Token': token} + if extra_headers: + headers.update(extra_headers) + conn.request( + 'GET', + '%s/%s/manifest?multipart-manifest=get' % + (parsed.path, cont_name), + '', headers) + return check_response(conn) + + resp = retry(get_manifest) + self.assertEqual(resp.status, 200) + # wait for the manifest to expire + # the objects will also have expired at the same time + # since their x-delete-at times are the same + time.sleep(3) + + resp = retry(get_manifest) + resp.read() + # check to see manifest has expired + self.assertEqual(resp.status, 404, resp.headers.get('x-trans-id')) + + def get_or_head_part(url, token, parsed, conn, + extra_headers=None, method='GET', + part_number=None): + headers = {'X-Auth-Token': token} + if extra_headers: + headers.update(extra_headers) + conn.request(method, '%s/%s/manifest?part-number=%s' % ( + parsed.path, + cont_name, + part_number + ), '', headers) + return check_response(conn) + + resp = retry(get_manifest, extra_headers={'X-Open-Expired': True}) + resp.read() + # read the expired object with magic x-open-expired header + self.assertEqual(resp.status, 200) + + for objnum in range(len(segments)): + part_num = str(objnum + 1) + resp = retry(get_or_head_part, + extra_headers={'X-Open-Expired': True}, + part_number=part_num,) + resp.read() + self.assertEqual(resp.status, 206) + + for objnum in range(len(segments)): + part_num = str(objnum + 1) + resp = retry(get_or_head_part, + extra_headers={'X-Open-Expired': True}, + method='HEAD', part_number=part_num) + resp.read() + self.assertEqual(resp.status, 206) + + # no x-open-expired case and it should 404 + for objnum in range(len(segments)): + part_num = str(objnum + 1) + resp = retry(get_or_head_part, + method='HEAD', part_number=part_num) + resp.read() + self.assertEqual(resp.status, 404) + + # same situation here + for objnum in range(len(segments)): + part_num = str(objnum + 1) + resp = retry(get_or_head_part, + method='GET', part_number=part_num) + resp.read() + self.assertEqual(resp.status, 404) + + def head_manifest(url, token, parsed, conn, extra_headers=None): + headers = {'X-Auth-Token': token} + if extra_headers: + headers.update(extra_headers) + conn.request('HEAD', '%s/%s/manifest' % (parsed.path, + cont_name), + '', headers) + return check_response(conn) + + resp = retry(head_manifest, extra_headers={'X-Open-Expired': True}) + resp.read() + # head expired object with magic x-open-expired header + self.assertEqual(resp.status, 200) + def test_slo_container_listing(self): # the listing object size should equal the sum of the size of the # segments, not the size of the manifest body diff --git a/test/probe/test_object_expirer.py b/test/probe/test_object_expirer.py index c0d4e734c9..a62717f6e0 100644 --- a/test/probe/test_object_expirer.py +++ b/test/probe/test_object_expirer.py @@ -410,6 +410,129 @@ class TestObjectExpirer(ReplProbeTest): headers={'X-Open-Expired': True}) self.assertEqual(e.exception.http_status, 404) + def _setup_test_slo_object(self): + segment_container = self.container_name + '_segments' + client.put_container(self.url, self.token, self.container_name, {}) + client.put_container(self.url, self.token, segment_container, {}) + client.put_object(self.url, self.token, + segment_container, 'segment_1', b'12') + client.put_object(self.url, self.token, + segment_container, 'segment_2', b'5678') + client.put_object( + self.url, self.token, self.container_name, 'slo', json.dumps([ + {'path': segment_container + '/segment_1'}, + {'data': 'Cg=='}, + {'path': segment_container + '/segment_2'}, + ]), query_string='multipart-manifest=put') + _, body = client.get_object(self.url, self.token, + self.container_name, 'slo') + self.assertEqual(body, b'12\n5678') + + return segment_container, self.container_name + + def test_open_expired_enabled_with_part_num(self): + allow_open_expired = config_true_value( + self.cluster_info['swift'].get('allow_open_expired') + ) + + if not allow_open_expired: + raise unittest.SkipTest( + "allow_open_expired is disabled in this swift cluster" + ) + + seg_container, container_name = self._setup_test_slo_object() + now = time.time() + delete_at = int(now + 1) + + client.post_object( + self.url, self.token, container_name, 'slo', + headers={ + 'X-Delete-At': str(delete_at), + 'X-Object-Meta-Test': 'foo' + } + ) + + # make sure auto-created containers get in the account listing + Manager(['container-updater']).once() + + # sleep until after expired but not reaped + while time.time() <= delete_at: + time.sleep(0.1) + + # should get a 404, object is expired + while True: + try: + client.head_object(self.url, self.token, container_name, 'slo') + time.sleep(1) # Wait for a short period before trying again + except ClientException as e: + # check if the object is expired + if e.http_status == 404: + break # The object is expired, so we can exit the loop + + resp_headers = client.head_object( + self.url, self.token, container_name, 'slo', + headers={'X-Open-Expired': True}, + query_string='part-number=1' + ) + + self.assertEqual(resp_headers.get('x-object-meta-test'), 'foo') + self.assertEqual(resp_headers.get('content-range'), 'bytes 0-1/7') + self.assertEqual(resp_headers.get('content-length'), '2') + self.assertEqual(resp_headers.get('x-parts-count'), '3') + self.assertEqual(resp_headers.get('x-static-large-object'), 'True') + self.assertEqual(resp_headers.get('accept-ranges'), 'bytes') + + with self.assertRaises(ClientException) as e: + client.head_object(self.url, self.token, container_name, 'slo') + self.assertEqual(e.exception.http_status, 404) + + now = time.time() + delete_at = int(now + 2) + for seg_obj_name in ['segment_1', 'segment_2']: + client.post_object( + self.url, self.token, seg_container, seg_obj_name, + headers={ + 'X-Open-Expired': True, + 'X-Segment-Meta-Test': 'segment-foo', + 'X-Delete-At': str(delete_at) + } + ) + + # make sure auto-created containers get in the account listing + Manager(['container-updater']).once() + while time.time() <= delete_at: + time.sleep(0.1) + + # should get a 404, segment object is expired + with self.assertRaises(ClientException) as e: + client.head_object(self.url, self.token, seg_container, + 'segment_2') + self.assertEqual(e.exception.http_status, 404) + + # magic of x-open-expired + resp_headers = client.head_object( + self.url, self.token, seg_container, 'segment_2', + headers={'X-Open-Expired': True}, + query_string='part-number=1' + ) + + # keep in mind that the segment object is expired + self.assertEqual(resp_headers.get('content-length'), '4') + self.assertTrue(time.time() > delete_at) + + # expirer runs to reap the whichever object was set for expiry + self.expirer.once() + + for seg_obj_name in ['segment_1', 'segment_2']: + # should get a 404 even with x-open-expired since object is reaped + with self.assertRaises(ClientException) as e: + client.head_object( + self.url, self.token, seg_container, seg_obj_name, + headers={'X-Open-Expired': True}, + query_string='part-number=1' + ) + self.assertEqual(e.exception.http_status, 404) + def test_open_expired_disabled(self): # When the global configuration option allow_open_expired is set to @@ -681,22 +804,7 @@ class TestObjectExpirer(ReplProbeTest): if not self.cluster_info.get('slo', {}).get('allow_async_delete'): raise unittest.SkipTest('allow_async_delete not enabled') - segment_container = self.container_name + '_segments' - client.put_container(self.url, self.token, self.container_name, {}) - client.put_container(self.url, self.token, segment_container, {}) - client.put_object(self.url, self.token, - segment_container, 'segment_1', b'1234') - client.put_object(self.url, self.token, - segment_container, 'segment_2', b'5678') - client.put_object( - self.url, self.token, self.container_name, 'slo', json.dumps([ - {'path': segment_container + '/segment_1'}, - {'data': 'Cg=='}, - {'path': segment_container + '/segment_2'}, - ]), query_string='multipart-manifest=put') - _, body = client.get_object(self.url, self.token, - self.container_name, 'slo') - self.assertEqual(body, b'1234\n5678') + segment_container, _ = self._setup_test_slo_object() client.delete_object( self.url, self.token, self.container_name, 'slo', @@ -717,7 +825,7 @@ class TestObjectExpirer(ReplProbeTest): ['segment_1', 'segment_2']) _, body = client.get_object(self.url, self.token, segment_container, 'segment_1') - self.assertEqual(body, b'1234') + self.assertEqual(body, b'12') _, body = client.get_object(self.url, self.token, segment_container, 'segment_2') self.assertEqual(body, b'5678') diff --git a/test/unit/common/middleware/test_slo.py b/test/unit/common/middleware/test_slo.py index 52518e8366..a6b8dc7eb0 100644 --- a/test/unit/common/middleware/test_slo.py +++ b/test/unit/common/middleware/test_slo.py @@ -1981,6 +1981,18 @@ class SloGETorHEADTestCase(SloTestCase): extra_headers={'X-Object-Meta-Nature': 'Regular'}, container='gettest') + def _setup_manifest_zero_byte(self): + """ + This is a zero-byte manifest. + """ + _single_segment_manifest = [ + {'name': '/gettest/zero', 'hash': md5hex(''), 'bytes': '0', + 'content_type': 'text/plain'}, + ] + self._setup_manifest( + 'zero-byte', _single_segment_manifest, + container='gettest') + def _setup_manifest_data(self): _data_manifest = [ { @@ -2011,6 +2023,21 @@ class SloGETorHEADTestCase(SloTestCase): 'X-Object-Meta-Plant': 'Ficus', }, container='gettest') + def _setup_manifest_bc_expires(self): + """ + This manifest's segments are all regular objects due to expire. + """ + _bc_expires_manifest = [ + {'name': '/gettest/b_5', 'hash': md5hex('b' * 5), 'bytes': '5', + 'content_type': 'text/plain'}, + {'name': '/gettest/c_10', 'hash': md5hex('c' * 10), 'bytes': '10', + 'content_type': 'text/plain'} + ] + self._setup_manifest('bc-expires', _bc_expires_manifest, + extra_headers={'X-Object-Meta-Plant': + 'Ficus-Expires'}, + container='gettest') + def _setup_manifest_abcd(self): """ This manifest uses manifest-bc as a sub-manifest! @@ -5703,6 +5730,8 @@ class TestPartNumber(SloGETorHEADTestCase): self._setup_manifest_abcd_subranges() self._setup_manifest_aabbccdd() self._setup_manifest_single_segment() + self._setup_manifest_zero_byte() + self._setup_manifest_bc_expires() # this b_50 object doesn't follow the alphabet convention self.app.register( @@ -5711,6 +5740,11 @@ class TestPartNumber(SloGETorHEADTestCase): 'Etag': md5hex('b' * 50)}, 'b' * 50) + # Setup POST req separately for expiring manifest + self.app.register('POST', + '/v1/AUTH_test/gettest/manifest-bc-expires', + swob.HTTPAccepted, {}) + self._setup_manifest_data() def test_head_part_number(self): @@ -5768,6 +5802,58 @@ class TestPartNumber(SloGETorHEADTestCase): ] self.assertEqual(self.app.calls, expected_calls) + def test_get_manifest_with_x_open_expired_part_num(self): + req = Request.blank( + '/v1/AUTH_test/gettest/manifest-bc-expires' + '?multipart-manifest=get', + environ={'REQUEST_METHOD': 'GET'}) + + captured_calls = [] + orig_call = FakeSwift.__call__ + + def pseudo_middleware(app, env, start_response): + captured_calls.append((env['REQUEST_METHOD'], env['PATH_INFO'])) + # pretend another middleware modified the path + # note: for convenience, the path "modification" actually results + # in one of the pre-registered paths + env['PATH_INFO'] += '' + return orig_call(app, env, start_response) + + with patch.object(FakeSwift, '__call__', pseudo_middleware): + status, headers, body = self.call_slo(req) + + self.assertEqual(status, '200 OK') + self.assertEqual([('GET', + '/v1/AUTH_test/gettest/manifest-bc-expires')], + captured_calls) + + t = str(int(time.time())) + req = Request.blank( + '/v1/AUTH_test/gettest/manifest-bc-expires', + environ={'REQUEST_METHOD': 'POST'}, + headers={'X-Delete-At': t} + ) + + with patch.object(FakeSwift, '__call__', pseudo_middleware): + status, headers, body = self.call_slo(req) + + self.assertEqual(status, '202 Accepted') + + req = Request.blank( + '/v1/AUTH_test/gettest/manifest-bc-expires?part-number=1', + environ={'REQUEST_METHOD': 'HEAD'}, + headers={'x-open-expired': 'true'}) + + with patch.object(FakeSwift, '__call__', pseudo_middleware): + status, headers, body = self.call_slo(req) + + self.assertEqual(status, '206 Partial Content') + self.assertEqual(headers['X-Static-Large-Object'], 'true') + self.assertEqual(headers['Etag'], '"%s"' % + self.manifest_bc_expires_slo_etag) + self.assertEqual(self.app.call_count, 4) + self.assertTrue(self.app.calls_with_headers[2][2]['X-Open-Expired']) + def test_get_part_number(self): # part number 1 is b_10 req = Request.blank( @@ -6028,6 +6114,32 @@ class TestPartNumber(SloGETorHEADTestCase): ] self.assertEqual(expected_calls, self.app.calls) + def test_part_number_zero_byte_manifest(self): + part_num = 1 + req = Request.blank( + '/v1/AUTH_test/gettest/manifest-zero-byte?' + 'partNumber=%s' % part_num, + method='HEAD') + status, headers, body = self.call_slo(req) + self.assertEqual(status, '200 OK') + self.assertEqual(headers['Etag'], + '"%s"' % self.manifest_zero_byte_slo_etag) + self.assertEqual(headers['Content-Length'], '0') + self.assertEqual(headers['X-Static-Large-Object'], 'true') + self.assertEqual(headers['X-Manifest-Etag'], + self.manifest_zero_byte_json_md5) + self.assertEqual(body, b'') # it's a HEAD request, after all + + expected_app_calls = [('HEAD', + '/v1/AUTH_test/gettest/manifest-zero-byte?' + 'partNumber=%s' % part_num)] + if not self.modern_manifest_headers: + expected_app_calls.append(( + 'GET', + '/v1/AUTH_test/gettest/manifest-zero-byte?' + 'partNumber=%s' % part_num)) + self.assertEqual(self.app.calls, expected_app_calls) + def test_part_number_zero_invalid_on_subrange(self): # either manifest, doesn't matter, part-number=0 is always invalid req = Request.blank(