From a8d2146266431d3e9f57cb8fabae3adb42786360 Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Fri, 4 Sep 2020 11:35:41 -0700 Subject: [PATCH] xlo: Drain error responses Otherwise, they get logged as 499s. While we're at it, also drain DLO bodies (if they're small). Change-Id: I7b54f25f42577020b10029c74f8fc01fa6fc591e --- swift/common/middleware/dlo.py | 7 +++++- swift/common/middleware/slo.py | 14 +++++++----- swift/common/request_helpers.py | 11 +++++++--- test/unit/common/middleware/test_dlo.py | 29 ++++++++++++++++++++++--- test/unit/common/middleware/test_slo.py | 17 +++++++++++---- 5 files changed, 62 insertions(+), 16 deletions(-) diff --git a/swift/common/middleware/dlo.py b/swift/common/middleware/dlo.py index 34a50f7dd3..5ab9d3dad3 100644 --- a/swift/common/middleware/dlo.py +++ b/swift/common/middleware/dlo.py @@ -130,7 +130,8 @@ from swift.common.swob import Request, Response, \ HTTPRequestedRangeNotSatisfiable, HTTPBadRequest, HTTPConflict, \ str_to_wsgi, wsgi_to_str, wsgi_quote, wsgi_unquote, normalize_etag from swift.common.utils import get_logger, \ - RateLimitedIterator, quote, close_if_possible, closing_if_possible + RateLimitedIterator, quote, close_if_possible, closing_if_possible, \ + drain_and_close from swift.common.request_helpers import SegmentedIterable, \ update_ignore_range_header from swift.common.wsgi import WSGIContext, make_subrequest, load_app_config @@ -376,6 +377,10 @@ class GetContext(WSGIContext): # make sure this response is for a dynamic large object manifest for header, value in self._response_headers: if (header.lower() == 'x-object-manifest'): + content_length = self._response_header_value('content-length') + if content_length is not None and int(content_length) < 1024: + # Go ahead and consume small bodies + drain_and_close(resp_iter) close_if_possible(resp_iter) response = self.get_or_head_response( req, wsgi_to_str(wsgi_unquote(value))) diff --git a/swift/common/middleware/slo.py b/swift/common/middleware/slo.py index bc60cf5e9f..8e616eed06 100644 --- a/swift/common/middleware/slo.py +++ b/swift/common/middleware/slo.py @@ -547,11 +547,15 @@ class SloGetContext(WSGIContext): sub_resp = sub_req.get_response(self.slo.app) if not sub_resp.is_success: - close_if_possible(sub_resp.app_iter) - raise ListingIterError( - 'while fetching %s, GET of submanifest %s ' - 'failed with status %d' % (req.path, sub_req.path, - sub_resp.status_int)) + # Error message should be short + body = sub_resp.body + if not six.PY2: + body = body.decode('utf-8') + msg = ('while fetching %s, GET of submanifest %s ' + 'failed with status %d (%s)') + raise ListingIterError(msg % ( + req.path, sub_req.path, sub_resp.status_int, + body if len(body) <= 60 else body[:57] + '...')) try: with closing_if_possible(sub_resp.app_iter): diff --git a/swift/common/request_helpers.py b/swift/common/request_helpers.py index 5fbbbf3b1b..000752e520 100644 --- a/swift/common/request_helpers.py +++ b/swift/common/request_helpers.py @@ -564,11 +564,16 @@ class SegmentedIterable(object): seg_req = data_or_req seg_resp = seg_req.get_response(self.app) if not is_success(seg_resp.status_int): - close_if_possible(seg_resp.app_iter) + # Error body should be short + body = seg_resp.body + if not six.PY2: + body = body.decode('utf8') raise SegmentError( 'While processing manifest %s, ' - 'got %d while retrieving %s' % - (self.name, seg_resp.status_int, seg_req.path)) + 'got %d (%s) while retrieving %s' % + (self.name, seg_resp.status_int, + body if len(body) <= 60 else body[:57] + '...', + seg_req.path)) elif ((seg_etag and (seg_resp.etag != seg_etag)) or (seg_size and (seg_resp.content_length != seg_size) and diff --git a/test/unit/common/middleware/test_dlo.py b/test/unit/common/middleware/test_dlo.py index 5f39840beb..d8f6d6aa7b 100644 --- a/test/unit/common/middleware/test_dlo.py +++ b/test/unit/common/middleware/test_dlo.py @@ -356,9 +356,28 @@ class TestDloGetManifest(DloTestCase): md5hex("aaaaa") + md5hex("bbbbb") + md5hex("ccccc") + md5hex("ddddd") + md5hex("eeeee")) self.assertEqual(headers.get("Etag"), expected_etag) + self.assertEqual(self.app.unread_requests, {}) + + def test_get_big_manifest(self): + self.app.register( + 'GET', '/v1/AUTH_test/mancon/big-manifest', + swob.HTTPOk, {'Content-Length': '17000', 'Etag': 'manifest-etag', + 'X-Object-Manifest': 'c/seg'}, + b'manifest-contents' * 1000) + req = swob.Request.blank('/v1/AUTH_test/mancon/big-manifest', + environ={'REQUEST_METHOD': 'GET'}) + status, headers, body = self.call_dlo(req) + headers = HeaderKeyDict(headers) + self.assertEqual(status, "200 OK") + self.assertEqual(headers["Content-Length"], "25") + self.assertEqual(body, b'aaaaabbbbbcccccdddddeeeee') + expected_etag = '"%s"' % md5hex( + md5hex("aaaaa") + md5hex("bbbbb") + md5hex("ccccc") + + md5hex("ddddd") + md5hex("eeeee")) + self.assertEqual(headers.get("Etag"), expected_etag) self.assertEqual(self.app.unread_requests, { # Since we don't know how big this will be, we just disconnect - ('GET', '/v1/AUTH_test/mancon/manifest'): 1, + ('GET', '/v1/AUTH_test/mancon/big-manifest'): 1, }) def test_get_range_on_segment_boundaries(self): @@ -573,9 +592,11 @@ class TestDloGetManifest(DloTestCase): environ={'REQUEST_METHOD': 'GET'}) status, headers, body = self.call_dlo(req) self.assertEqual(status, "409 Conflict") + self.assertEqual(self.app.unread_requests, {}) self.assertEqual(self.dlo.logger.get_lines_for_level('error'), [ 'While processing manifest /v1/AUTH_test/mancon/manifest, ' - 'got 403 while retrieving /v1/AUTH_test/c/seg_01', + 'got 403 (

Forbidden

Access was denied to this ' + 'reso...) while retrieving /v1/AUTH_test/c/seg_01', ]) def test_error_fetching_second_segment(self): @@ -591,9 +612,11 @@ class TestDloGetManifest(DloTestCase): self.assertEqual(status, "200 OK") # first segment made it out self.assertEqual(body, b'aaaaa') + self.assertEqual(self.app.unread_requests, {}) self.assertEqual(self.dlo.logger.get_lines_for_level('error'), [ 'While processing manifest /v1/AUTH_test/mancon/manifest, ' - 'got 403 while retrieving /v1/AUTH_test/c/seg_02', + 'got 403 (

Forbidden

Access was denied to this ' + 'reso...) while retrieving /v1/AUTH_test/c/seg_02', ]) def test_error_listing_container_first_listing_request(self): diff --git a/test/unit/common/middleware/test_slo.py b/test/unit/common/middleware/test_slo.py index daf2a5b4bf..a9343cf927 100644 --- a/test/unit/common/middleware/test_slo.py +++ b/test/unit/common/middleware/test_slo.py @@ -3255,9 +3255,12 @@ class TestSloGetManifest(SloTestCase): headers = HeaderKeyDict(headers) self.assertEqual(status, '200 OK') + self.assertEqual(b"aaaaabbbbbbbbbb", body) + self.assertEqual(self.app.unread_requests, {}) self.assertEqual(self.slo.logger.get_lines_for_level('error'), [ 'While processing manifest /v1/AUTH_test/gettest/manifest-abcd, ' - 'got 401 while retrieving /v1/AUTH_test/gettest/c_15' + 'got 401 (

Unauthorized

This server could not ' + 'verif...) while retrieving /v1/AUTH_test/gettest/c_15' ]) self.assertEqual(self.app.calls, [ ('GET', '/v1/AUTH_test/gettest/manifest-abcd'), @@ -3277,10 +3280,12 @@ class TestSloGetManifest(SloTestCase): self.assertEqual("200 OK", status) self.assertEqual(b"aaaaa", body) + self.assertEqual(self.app.unread_requests, {}) self.assertEqual(self.slo.logger.get_lines_for_level('error'), [ 'while fetching /v1/AUTH_test/gettest/manifest-abcd, GET of ' 'submanifest /v1/AUTH_test/gettest/manifest-bc failed with ' - 'status 401' + 'status 401 (

Unauthorized

This server could ' + 'not verif...)' ]) self.assertEqual(self.app.calls, [ ('GET', '/v1/AUTH_test/gettest/manifest-abcd'), @@ -3311,10 +3316,12 @@ class TestSloGetManifest(SloTestCase): status, headers, body = self.call_slo(req) self.assertEqual('409 Conflict', status) + self.assertEqual(self.app.unread_requests, {}) self.assertEqual(self.slo.logger.get_lines_for_level('error'), [ 'while fetching /v1/AUTH_test/gettest/manifest-manifest-a, GET ' 'of submanifest /v1/AUTH_test/gettest/manifest-a failed with ' - 'status 403' + 'status 403 (

Forbidden

Access was denied to ' + 'this reso...)' ]) def test_invalid_json_submanifest(self): @@ -3473,9 +3480,11 @@ class TestSloGetManifest(SloTestCase): status, headers, body = self.call_slo(req) self.assertEqual('409 Conflict', status) + self.assertEqual(self.app.unread_requests, {}) self.assertEqual(self.slo.logger.get_lines_for_level('error'), [ 'While processing manifest /v1/AUTH_test/gettest/' - 'manifest-not-exists, got 404 while retrieving /v1/AUTH_test/' + 'manifest-not-exists, got 404 (

Not Found

The ' + 'resource could not be foun...) while retrieving /v1/AUTH_test/' 'gettest/not_exists_obj' ])