From 66620ae8f8cd617c6284a9d56f9b1dfbe06584b2 Mon Sep 17 00:00:00 2001 From: James Saryerwinnie Date: Mon, 2 Jun 2014 12:49:15 -0700 Subject: [PATCH 1/2] Handle case where S3 returns a 200 error response From http://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectCOPY.html: There are two opportunities for a copy request to return an error. One can occur when Amazon S3 receives the copy request and the other can occur while Amazon S3 is copying the files. If the error occurs before the copy operation starts, you receive a standard Amazon S3 error. If the error occurs during the copy operation, the error response is embedded in the 200 OK response. This means that a 200 OK response can contain either a success or an error. Make sure to design your application to parse the contents of the response and handle it appropriately. Operations that need this behavior: - CompleteMultipartUpload - CopyObject - UploadPartCopy The fix here is to switch the status code to 500, because, for all intents and purposes, this is an error response. This ensures it goes through our normal 5xx retry logic and exception handling/raising code. --- botocore/handlers.py | 28 ++++++++++++++++++++++++++++ tests/unit/test_handlers.py | 27 +++++++++++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/botocore/handlers.py b/botocore/handlers.py index 7cc249a054..0e12114107 100644 --- a/botocore/handlers.py +++ b/botocore/handlers.py @@ -37,6 +37,31 @@ +def check_for_200_error(response, operation, **kwargs): + # From: http://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectCOPY.html + # There are two opportunities for a copy request to return an error. One + # can occur when Amazon S3 receives the copy request and the other can + # occur while Amazon S3 is copying the files. If the error occurs before + # the copy operation starts, you receive a standard Amazon S3 error. If the + # error occurs during the copy operation, the error response is embedded in + # the 200 OK response. This means that a 200 OK response can contain either + # a success or an error. Make sure to design your application to parse the + # contents of the response and handle it appropriately. + # + # So this handler checks for this case. Even though the server sends a + # 200 response, conceptually this should be handled exactly like a + # 500 response (with respect to raising exceptions, retries, etc.) + # We're connected *before* all the other retry logic handlers, so as long + # as we switch the error code to 500, we'll retry the error as expected. + http_response, parsed = response + if http_response.status_code == 200: + if 'Errors' in parsed: + logger.debug("Error found for response with 200 status code, " + "operation: %s, errors: %s, changing status code to " + "500.", operation, parsed) + http_response.status_code = 500 + + def decode_console_output(event_name, shape, value, **kwargs): try: value = base64.b64decode(six.b(value)).decode('utf-8') @@ -239,6 +264,9 @@ def copy_snapshot_encrypted(operation, params, **kwargs): ('before-call.s3.CopyObject', quote_source_header), ('before-call.ec2.CopySnapshot', copy_snapshot_encrypted), ('before-auth.s3', fix_s3_host), + ('needs-retry.s3.UploadPartCopy', check_for_200_error), + ('needs-retry.s3.CopyObject', check_for_200_error), + ('needs-retry.s3.CompleteMultipartUpload', check_for_200_error), ('service-created', register_retries_for_service), ('creating-endpoint.s3', maybe_switch_to_s3sigv4), ('creating-endpoint.ec2', maybe_switch_to_sigv4), diff --git a/tests/unit/test_handlers.py b/tests/unit/test_handlers.py index 7957a67da2..a5bfc3c9e4 100644 --- a/tests/unit/test_handlers.py +++ b/tests/unit/test_handlers.py @@ -21,6 +21,7 @@ from botocore.hooks import first_non_none_response from botocore.compat import quote from botocore.handlers import copy_snapshot_encrypted +from botocore.handlers import check_for_200_error class TestHandlers(BaseSessionTest): @@ -98,6 +99,32 @@ def test_copy_snapshot_encrypted(self): # We created an endpoint in the source region. operation.service.get_endpoint.assert_called_with('us-west-2') + def test_500_status_code_set_for_200_response(self): + http_response = mock.Mock() + http_response.status_code = 200 + parsed = { + 'Errors': [{ + "HostId": "hostid", + "Message": "An internal error occurred.", + "Code": "InternalError", + "RequestId": "123456789" + }] + } + check_for_200_error((http_response, parsed), 'MyOperationName') + self.assertEqual(http_response.status_code, 500) + + def test_200_response_with_no_error_left_untouched(self): + http_response = mock.Mock() + http_response.status_code = 200 + parsed = { + 'NotAnError': [{ + 'foo': 'bar' + }] + } + check_for_200_error((http_response, parsed), 'MyOperationName') + # We don't touch the status code since there are no errors present. + self.assertEqual(http_response.status_code, 200) + if __name__ == '__main__': unittest.main() From 26f883cbd593eccca031dd20384941ccba741e2f Mon Sep 17 00:00:00 2001 From: James Saryerwinnie Date: Mon, 2 Jun 2014 14:46:07 -0700 Subject: [PATCH 2/2] Add more tests for s3 200 error responses --- tests/unit/test_endpoint.py | 40 +++++++++++++++++++++++++++++++++++-- tests/unit/test_handlers.py | 35 ++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 2 deletions(-) diff --git a/tests/unit/test_endpoint.py b/tests/unit/test_endpoint.py index bbaf77e7c9..82d27d89f7 100644 --- a/tests/unit/test_endpoint.py +++ b/tests/unit/test_endpoint.py @@ -347,9 +347,9 @@ def test_retry_on_socket_errors(self): self.assertEqual(self.total_calls, 3) -class TestResetStreamOnRetry(unittest.TestCase): +class TestS3ResetStreamOnRetry(unittest.TestCase): def setUp(self): - super(TestResetStreamOnRetry, self).setUp() + super(TestS3ResetStreamOnRetry, self).setUp() self.total_calls = 0 self.auth = Mock() self.session = create_session(include_builtin_handlers=False) @@ -393,6 +393,42 @@ def test_reset_stream_on_retry(self): self.assertEqual(payload.literal_value.total_resets, 2) +class TestS3Retry200SpecialCases(unittest.TestCase): + def setUp(self): + super(TestS3Retry200SpecialCases, self).setUp() + self.total_calls = 0 + self.auth = Mock() + self.session = create_session(include_builtin_handlers=True) + self.service = Mock() + self.service.endpoint_prefix = 's3' + self.service.session = self.session + self.endpoint = RestEndpoint( + self.service, 'us-east-1', 'https://s3.amazonaws.com/', + auth=self.auth) + self.http_session = Mock() + self.endpoint.http_session = self.http_session + self.get_response_patch = patch('botocore.response.get_response') + self.get_response = self.get_response_patch.start() + self.retried_on_exception = None + + def tearDown(self): + self.get_response_patch.stop() + + def test_retry_special_case_s3_200_response(self): + # Test for: + # http://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectCOPY.html + op = Mock() + op.name = 'CopyObject' + op.http = {'uri': '', 'method': 'PUT'} + http_response = Mock() + http_response.status_code = 200 + parsed = {'Errors': [{'Code': 'InternalError', 'Message': 'foo'}]} + self.get_response.return_value = (http_response, parsed) + self.endpoint.make_request(op, {'headers': {}, 'payload': None}) + # The response code should have been switched to 500. + self.assertEqual(http_response.status_code, 500) + + class TestRestEndpoint(unittest.TestCase): def test_encode_uri_params_unicode(self): diff --git a/tests/unit/test_handlers.py b/tests/unit/test_handlers.py index a5bfc3c9e4..594e34639c 100644 --- a/tests/unit/test_handlers.py +++ b/tests/unit/test_handlers.py @@ -126,5 +126,40 @@ def test_200_response_with_no_error_left_untouched(self): self.assertEqual(http_response.status_code, 200) +class TestRetryHandlerOrder(BaseSessionTest): + def get_handler_names(self, responses): + names = [] + for response in responses: + handler = response[0] + if hasattr(handler, '__name__'): + names.append(handler.__name__) + elif hasattr(handler, '__class__'): + names.append(handler.__class__.__name__) + else: + names.append(str(handler)) + return names + + def test_s3_special_case_is_before_other_retry(self): + service = self.session.get_service('s3') + operation = service.get_operation('CopyObject') + responses = self.session.emit( + 'needs-retry.s3.CopyObject', + response=(mock.Mock(), mock.Mock()), endpoint=mock.Mock(), operation=operation, + attempts=1, caught_exception=None) + # This is implementation specific, but we're trying to verify that + # the check_for_200_error is before any of the retry logic in + # botocore.retryhandlers. + # Technically, as long as the relative order is preserved, we don't + # care about the absolute order. + names = self.get_handler_names(responses) + self.assertIn('check_for_200_error', names) + self.assertIn('RetryHandler', names) + s3_200_handler = names.index('check_for_200_error') + general_retry_handler = names.index('RetryHandler') + self.assertTrue(s3_200_handler < general_retry_handler, + "S3 200 error handler was suppose to be before " + "the general retry handler, but it was not.") + + if __name__ == '__main__': unittest.main()