From 57471357fcb92cde54370e81d7e5e3de1284fee3 Mon Sep 17 00:00:00 2001 From: aemous Date: Fri, 1 Nov 2024 11:25:44 -0400 Subject: [PATCH 01/12] S3 200 Errors port. --- .../next-release/enhancement-s3-70184.json | 5 ++ awscli/botocore/args.py | 2 + awscli/botocore/handlers.py | 66 ++++++++++++-- tests/functional/botocore/test_s3.py | 85 +++++++++++++++++-- tests/unit/botocore/test_handlers.py | 7 +- 5 files changed, 145 insertions(+), 20 deletions(-) create mode 100644 .changes/next-release/enhancement-s3-70184.json diff --git a/.changes/next-release/enhancement-s3-70184.json b/.changes/next-release/enhancement-s3-70184.json new file mode 100644 index 000000000000..cb8579fd7913 --- /dev/null +++ b/.changes/next-release/enhancement-s3-70184.json @@ -0,0 +1,5 @@ +{ + "type": "enhancement", + "category": "s3", + "description": "Handle HTTP 200 responses with error information for all supported s3 operations." +} diff --git a/awscli/botocore/args.py b/awscli/botocore/args.py index f26db56573c5..09211fb0b4f3 100644 --- a/awscli/botocore/args.py +++ b/awscli/botocore/args.py @@ -349,6 +349,7 @@ def _compute_retry_max_attempts(self, config_kwargs): # overrides everything. retries = config_kwargs.get('retries') if retries is not None and 'max_attempts' in retries: + print(f'explicit max attempts provided in client config: {retries}') return # Otherwise we'll check the config store which checks env vars, # config files, etc. There is no default value for max_attempts @@ -359,6 +360,7 @@ def _compute_retry_max_attempts(self, config_kwargs): retries = {} config_kwargs['retries'] = retries retries['max_attempts'] = max_attempts + print(f'retries config: {retries}') def _compute_retry_mode(self, config_kwargs): retries = config_kwargs.get('retries') diff --git a/awscli/botocore/handlers.py b/awscli/botocore/handlers.py index 1ad78f5e55a9..8f42ceaa15e1 100644 --- a/awscli/botocore/handlers.py +++ b/awscli/botocore/handlers.py @@ -115,6 +115,7 @@ def escape_xml_payload(params, **kwargs): def check_for_200_error(response, **kwargs): + """This function has been deprecated, but is kept for backwards compatibility.""" # 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 @@ -135,20 +136,22 @@ def check_for_200_error(response, **kwargs): # trying to retrieve the response. See Endpoint._get_response(). return http_response, parsed = response - if _looks_like_special_case_error(http_response): + if _looks_like_special_case_error( + http_response.status_code, http_response.content + ): logger.debug("Error found for response with 200 status code, " "errors: %s, changing status code to " "500.", parsed) http_response.status_code = 500 -def _looks_like_special_case_error(http_response): - if http_response.status_code == 200: +def _looks_like_special_case_error(status_code, body): + if status_code == 200 and body: try: parser = ETree.XMLParser( target=ETree.TreeBuilder(), encoding='utf-8') - parser.feed(http_response.content) + parser.feed(body) root = parser.close() except XMLParseError: # In cases of network disruptions, we may end up with a partial @@ -156,6 +159,7 @@ def _looks_like_special_case_error(http_response): # 500 Service Errors and try again. return True if root.tag == 'Error': + print(f'RETURNING TRUE! LOOKS LIKE SPECIAL CASE') return True return False @@ -1134,6 +1138,54 @@ def document_expires_shape(section, event_name, **kwargs): ) +def _handle_200_error(operation_model, response_dict, **kwargs): + # S3 can return a 200 response with an error embedded in the body. + # Convert the 200 to a 500 for retry resolution in ``_update_status_code``. + if not _should_handle_200_error(operation_model, response_dict): + # Operations with streaming response blobs are excluded as they + # can't be reliably distinguished from an S3 error. + print(f'SHOULD NOT HANDLE THIS ONE. ') + return + if _looks_like_special_case_error( + response_dict['status_code'], response_dict['body'] + ): + response_dict['status_code'] = 500 + logger.debug( + f"Error found for response with 200 status code: {response_dict['body']}." + ) + print(f"Error found for response with 200 status code: {response_dict['body']}.") + + +def _should_handle_200_error(operation_model, response_dict): + output_shape = operation_model.output_shape + if ( + not response_dict + or operation_model.has_event_stream_output + or not output_shape + ): + return False + payload = output_shape.serialization.get('payload') + if payload is not None: + payload_shape = output_shape.members[payload] + if payload_shape.type_name in ('blob', 'string'): + return False + return True + + +def _update_status_code(response, **kwargs): + # Update the http_response status code when the parsed response has been + # modified in a handler. This enables retries for cases like ``_handle_200_error``. + if response is None: + return + http_response, parsed = response + parsed_status_code = parsed.get('ResponseMetadata', {}).get( + 'HTTPStatusCode', http_response.status_code + ) + print(f'response status code : parsed code = {http_response.status_code} : {parsed_status_code}') + if http_response.status_code != parsed_status_code: + http_response.status_code = parsed_status_code + + # This is a list of (event_name, handler). # When a Session is created, everything in this list will be # automatically registered with that Session. @@ -1158,6 +1210,7 @@ def document_expires_shape(section, event_name, **kwargs): ('after-call.cloudformation.GetTemplate', json_decode_template_body), ('after-call.s3.GetBucketLocation', parse_get_bucket_location), ('before-parse.s3.*', handle_expires_header), + ('before-parse.s3.*', _handle_200_error, REGISTER_FIRST), ('before-parameter-build', generate_idempotent_uuid), ('before-parameter-build.s3', validate_bucket_name), @@ -1196,10 +1249,7 @@ def document_expires_shape(section, event_name, **kwargs): ('before-call.glacier.UploadMultipartPart', add_glacier_checksums), ('before-call.ec2.CopySnapshot', inject_presigned_url_ec2), ('request-created.machine-learning.Predict', switch_host_machinelearning), - ('needs-retry.s3.UploadPartCopy', check_for_200_error, REGISTER_FIRST), - ('needs-retry.s3.CopyObject', check_for_200_error, REGISTER_FIRST), - ('needs-retry.s3.CompleteMultipartUpload', check_for_200_error, - REGISTER_FIRST), + ('needs-retry.s3.*', _update_status_code, REGISTER_FIRST), ('choose-signer.cognito-identity.GetId', disable_signing), ('choose-signer.cognito-identity.GetOpenIdToken', disable_signing), ('choose-signer.cognito-identity.UnlinkIdentity', disable_signing), diff --git a/tests/functional/botocore/test_s3.py b/tests/functional/botocore/test_s3.py index f35f010d652c..e52fec7b9935 100644 --- a/tests/functional/botocore/test_s3.py +++ b/tests/functional/botocore/test_s3.py @@ -29,7 +29,6 @@ UnsupportedS3ConfigurationError, UnsupportedS3AccesspointConfigurationError, ) -from botocore.parsers import ResponseParserError from botocore.loaders import Loader from botocore import UNSIGNED @@ -358,12 +357,12 @@ def create_stubbed_s3_client(self, **kwargs): http_stubber.start() return client, http_stubber - def test_s3_copy_object_with_empty_response(self): + def test_s3_copy_object_with_incomplete_response(self): self.client, self.http_stubber = self.create_stubbed_s3_client( region_name='us-east-1' ) - empty_body = b'' + incomplete_body = b'\n\n\n' complete_body = ( b'\n\n' b'"s0mEcH3cK5uM"' ) - self.http_stubber.add_response(status=200, body=empty_body) + self.http_stubber.add_response(status=200, body=incomplete_body) self.http_stubber.add_response(status=200, body=complete_body) response = self.client.copy_object( Bucket='bucket', @@ -385,19 +384,87 @@ def test_s3_copy_object_with_empty_response(self): self.assertEqual(response['ResponseMetadata']['HTTPStatusCode'], 200) self.assertTrue('CopyObjectResult' in response) - def test_s3_copy_object_with_incomplete_response(self): + +class TestS3200ErrorResponse(BaseS3OperationTest): + def create_s3_client(self, **kwargs): + client_kwargs = {"region_name": self.region} + client_kwargs.update(kwargs) + return self.session.create_client("s3", **client_kwargs) + + def create_stubbed_s3_client(self, **kwargs): + client = self.create_s3_client(**kwargs) + http_stubber = ClientHTTPStubber(client) + http_stubber.start() + return client, http_stubber + + def test_s3_200_with_error_response(self): self.client, self.http_stubber = self.create_stubbed_s3_client( region_name='us-east-1' ) - - incomplete_body = b'\n\n\n' - self.http_stubber.add_response(status=200, body=incomplete_body) - with self.assertRaises(ResponseParserError): + error_body = ( + b"" + b"SlowDown" + b"Please reduce your request rate." + b"" + ) + # Populate 5 attempts for SlowDown to validate + # we reached four max retries and raised an exception. + for i in range(4): + self.http_stubber.add_response(status=200, body=error_body) + with self.assertRaises(botocore.exceptions.ClientError) as context: self.client.copy_object( Bucket='bucket', CopySource='other-bucket/test.txt', Key='test.txt', ) + print(f'stubber requests: {self.http_stubber.requests}') + self.assertEqual(len(self.http_stubber.requests), 4) + self.assertEqual( + context.exception.response["ResponseMetadata"]["HTTPStatusCode"], + 500, + ) + self.assertEqual( + context.exception.response["Error"]["Code"], "SlowDown" + ) + + def test_s3_200_with_no_error_response(self): + self.client, self.http_stubber = self.create_stubbed_s3_client( + region_name="us-east-1" + ) + self.http_stubber.add_response(status=200, body=b"") + + response = self.client.copy_object( + Bucket="bucket", + CopySource="other-bucket/test.txt", + Key="test.txt", + ) + + # Validate that the status code remains 200. + self.assertEqual(len(self.http_stubber.requests), 1) + self.assertEqual(response["ResponseMetadata"]["HTTPStatusCode"], 200) + + def test_s3_200_with_error_response_on_streaming_operation(self): + self.client, self.http_stubber = self.create_stubbed_s3_client( + region_name="us-east-1" + ) + self.http_stubber.add_response(status=200, body=b"") + response = self.client.get_object(Bucket="bucket", Key="test.txt") + + # Validate that the status code remains 200 because we don't + # process 200-with-error responses on streaming operations. + self.assertEqual(len(self.http_stubber.requests), 1) + self.assertEqual(response["ResponseMetadata"]["HTTPStatusCode"], 200) + + def test_s3_200_response_with_no_body(self): + self.client, self.http_stubber = self.create_stubbed_s3_client( + region_name="us-east-1" + ) + self.http_stubber.add_response(status=200) + response = self.client.head_object(Bucket="bucket", Key="test.txt") + + # Validate that the status code remains 200 on operations without a body. + self.assertEqual(len(self.http_stubber.requests), 1) + self.assertEqual(response["ResponseMetadata"]["HTTPStatusCode"], 200) class TestAccesspointArn(BaseS3ClientConfigurationTest): diff --git a/tests/unit/botocore/test_handlers.py b/tests/unit/botocore/test_handlers.py index c6a4c76bd9f1..ef15055b1f57 100644 --- a/tests/unit/botocore/test_handlers.py +++ b/tests/unit/botocore/test_handlers.py @@ -1138,14 +1138,15 @@ def test_s3_special_case_is_before_other_retry(self): 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 + # the _update_status_code is before any of the retry logic in # botocore.retries.*. # 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('_update_status_code', names) + # TODO there may be a previous PR that i should port here too, including RetryHandler self.assertIn('needs_retry', names) - s3_200_handler = names.index('check_for_200_error') + s3_200_handler = names.index('_update_status_code') general_retry_handler = names.index('needs_retry') self.assertTrue(s3_200_handler < general_retry_handler, "S3 200 error handler was supposed to be before " From da5a8aaaa8ea639d9cac9d3a14177724a5517b14 Mon Sep 17 00:00:00 2001 From: aemous Date: Fri, 1 Nov 2024 16:18:45 -0400 Subject: [PATCH 02/12] Update test behavior. --- awscli/botocore/args.py | 3 --- awscli/botocore/handlers.py | 4 ---- tests/functional/botocore/test_s3.py | 9 ++++----- 3 files changed, 4 insertions(+), 12 deletions(-) diff --git a/awscli/botocore/args.py b/awscli/botocore/args.py index 09211fb0b4f3..7fb4721781a1 100644 --- a/awscli/botocore/args.py +++ b/awscli/botocore/args.py @@ -157,7 +157,6 @@ def compute_client_args(self, service_model, client_config, if raw_value is not None: parameter_validation = ensure_boolean(raw_value) - s3_config = self.compute_s3_config(client_config) configured_endpoint_url = self._compute_configured_endpoint_url( @@ -349,7 +348,6 @@ def _compute_retry_max_attempts(self, config_kwargs): # overrides everything. retries = config_kwargs.get('retries') if retries is not None and 'max_attempts' in retries: - print(f'explicit max attempts provided in client config: {retries}') return # Otherwise we'll check the config store which checks env vars, # config files, etc. There is no default value for max_attempts @@ -360,7 +358,6 @@ def _compute_retry_max_attempts(self, config_kwargs): retries = {} config_kwargs['retries'] = retries retries['max_attempts'] = max_attempts - print(f'retries config: {retries}') def _compute_retry_mode(self, config_kwargs): retries = config_kwargs.get('retries') diff --git a/awscli/botocore/handlers.py b/awscli/botocore/handlers.py index 8f42ceaa15e1..2274da56ddcf 100644 --- a/awscli/botocore/handlers.py +++ b/awscli/botocore/handlers.py @@ -159,7 +159,6 @@ def _looks_like_special_case_error(status_code, body): # 500 Service Errors and try again. return True if root.tag == 'Error': - print(f'RETURNING TRUE! LOOKS LIKE SPECIAL CASE') return True return False @@ -1144,7 +1143,6 @@ def _handle_200_error(operation_model, response_dict, **kwargs): if not _should_handle_200_error(operation_model, response_dict): # Operations with streaming response blobs are excluded as they # can't be reliably distinguished from an S3 error. - print(f'SHOULD NOT HANDLE THIS ONE. ') return if _looks_like_special_case_error( response_dict['status_code'], response_dict['body'] @@ -1153,7 +1151,6 @@ def _handle_200_error(operation_model, response_dict, **kwargs): logger.debug( f"Error found for response with 200 status code: {response_dict['body']}." ) - print(f"Error found for response with 200 status code: {response_dict['body']}.") def _should_handle_200_error(operation_model, response_dict): @@ -1181,7 +1178,6 @@ def _update_status_code(response, **kwargs): parsed_status_code = parsed.get('ResponseMetadata', {}).get( 'HTTPStatusCode', http_response.status_code ) - print(f'response status code : parsed code = {http_response.status_code} : {parsed_status_code}') if http_response.status_code != parsed_status_code: http_response.status_code = parsed_status_code diff --git a/tests/functional/botocore/test_s3.py b/tests/functional/botocore/test_s3.py index e52fec7b9935..c51981c3ada2 100644 --- a/tests/functional/botocore/test_s3.py +++ b/tests/functional/botocore/test_s3.py @@ -407,9 +407,9 @@ def test_s3_200_with_error_response(self): b"Please reduce your request rate." b"" ) - # Populate 5 attempts for SlowDown to validate - # we reached four max retries and raised an exception. - for i in range(4): + # Populate 3 attempts for SlowDown to validate + # we reached two max retries and raised an exception. + for i in range(3): self.http_stubber.add_response(status=200, body=error_body) with self.assertRaises(botocore.exceptions.ClientError) as context: self.client.copy_object( @@ -417,8 +417,7 @@ def test_s3_200_with_error_response(self): CopySource='other-bucket/test.txt', Key='test.txt', ) - print(f'stubber requests: {self.http_stubber.requests}') - self.assertEqual(len(self.http_stubber.requests), 4) + self.assertEqual(len(self.http_stubber.requests), 3) self.assertEqual( context.exception.response["ResponseMetadata"]["HTTPStatusCode"], 500, From 0771a068babe20b81b1ebdbb4f6b2bdb7b2e5dec Mon Sep 17 00:00:00 2001 From: aemous Date: Mon, 4 Nov 2024 10:02:54 -0500 Subject: [PATCH 03/12] Add s3 mv regression test. --- tests/functional/s3/test_mv_command.py | 18 ++++++++++++++++++ tests/unit/botocore/test_handlers.py | 1 - 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/tests/functional/s3/test_mv_command.py b/tests/functional/s3/test_mv_command.py index d673d11ce03c..774c38c73441 100644 --- a/tests/functional/s3/test_mv_command.py +++ b/tests/functional/s3/test_mv_command.py @@ -12,9 +12,11 @@ # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. import os +from sys import exception from awscrt.s3 import S3RequestType +import botocore.exceptions from awscli.compat import BytesIO from awscli.customizations.s3.utils import S3PathResolver from awscli.testutils import mock @@ -267,6 +269,22 @@ def test_download_with_checksum_mode_crc32(self): self.assertEqual(self.operations_called[1][0].name, 'GetObject') self.assertEqual(self.operations_called[1][1]['ChecksumMode'], 'ENABLED') + def test_slowdown_raises_error(self): + self.parsed_responses = [ + # Response for HeadObject + {"ContentLength": 100, "LastModified": "00:00:00Z"}, + # Response for GetObject + {'ETag': '"foo-1"', 'Body': BytesIO(b'foo')}, + # Response for DeleteObject + {'Error': {'Code': 'SlowDown', 'Message': 'Please reduce your request rate.'}} + ] + cmdline = f'{self.prefix} s3://bucket/foo {self.files.rootdir}' + stdout, stderr, _ = self.run_cmd(cmdline, expected_rc=1) + self.assertIn( + 'An error occurred (SlowDown) when calling the DeleteObject operation: Please reduce your request rate.', + stderr + ) + class TestMvWithCRTClient(BaseCRTTransferClientTest): def test_upload_move_using_crt_client(self): diff --git a/tests/unit/botocore/test_handlers.py b/tests/unit/botocore/test_handlers.py index ef15055b1f57..2ad714f7a3c6 100644 --- a/tests/unit/botocore/test_handlers.py +++ b/tests/unit/botocore/test_handlers.py @@ -1144,7 +1144,6 @@ def test_s3_special_case_is_before_other_retry(self): # care about the absolute order. names = self.get_handler_names(responses) self.assertIn('_update_status_code', names) - # TODO there may be a previous PR that i should port here too, including RetryHandler self.assertIn('needs_retry', names) s3_200_handler = names.index('_update_status_code') general_retry_handler = names.index('needs_retry') From 84272616dfb2d040ab9236f9452b7a4f5d004526 Mon Sep 17 00:00:00 2001 From: aemous Date: Tue, 5 Nov 2024 09:23:38 -0500 Subject: [PATCH 04/12] Merge in random bucket names. --- awscli/botocore/handlers.py | 1 + 1 file changed, 1 insertion(+) diff --git a/awscli/botocore/handlers.py b/awscli/botocore/handlers.py index 2274da56ddcf..25579672f1d4 100644 --- a/awscli/botocore/handlers.py +++ b/awscli/botocore/handlers.py @@ -1143,6 +1143,7 @@ def _handle_200_error(operation_model, response_dict, **kwargs): if not _should_handle_200_error(operation_model, response_dict): # Operations with streaming response blobs are excluded as they # can't be reliably distinguished from an S3 error. + print(f'Should not handle this since detectes as non-200 error: {response_dict}') return if _looks_like_special_case_error( response_dict['status_code'], response_dict['body'] From 3af9fd28c226a6b39e6c137aaee89a8fb58db07c Mon Sep 17 00:00:00 2001 From: aemous Date: Mon, 4 Nov 2024 15:14:40 -0500 Subject: [PATCH 05/12] Remove print statement --- awscli/botocore/handlers.py | 1 - 1 file changed, 1 deletion(-) diff --git a/awscli/botocore/handlers.py b/awscli/botocore/handlers.py index 25579672f1d4..2274da56ddcf 100644 --- a/awscli/botocore/handlers.py +++ b/awscli/botocore/handlers.py @@ -1143,7 +1143,6 @@ def _handle_200_error(operation_model, response_dict, **kwargs): if not _should_handle_200_error(operation_model, response_dict): # Operations with streaming response blobs are excluded as they # can't be reliably distinguished from an S3 error. - print(f'Should not handle this since detectes as non-200 error: {response_dict}') return if _looks_like_special_case_error( response_dict['status_code'], response_dict['body'] From 4f8a2911b828b8140d743a43c61faffc3ee92c09 Mon Sep 17 00:00:00 2001 From: aemous Date: Tue, 5 Nov 2024 09:51:34 -0500 Subject: [PATCH 06/12] Remove uncalled function. --- awscli/botocore/handlers.py | 31 ------------------------------- 1 file changed, 31 deletions(-) diff --git a/awscli/botocore/handlers.py b/awscli/botocore/handlers.py index 2274da56ddcf..8ba860957312 100644 --- a/awscli/botocore/handlers.py +++ b/awscli/botocore/handlers.py @@ -114,37 +114,6 @@ def escape_xml_payload(params, **kwargs): params['body'] = body -def check_for_200_error(response, **kwargs): - """This function has been deprecated, but is kept for backwards compatibility.""" - # 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. - if response is None: - # A None response can happen if an exception is raised while - # trying to retrieve the response. See Endpoint._get_response(). - return - http_response, parsed = response - if _looks_like_special_case_error( - http_response.status_code, http_response.content - ): - logger.debug("Error found for response with 200 status code, " - "errors: %s, changing status code to " - "500.", parsed) - http_response.status_code = 500 - - def _looks_like_special_case_error(status_code, body): if status_code == 200 and body: try: From d2c86a870e844735212394b0c7477ca2ce7681f7 Mon Sep 17 00:00:00 2001 From: aemous Date: Tue, 5 Nov 2024 10:45:32 -0500 Subject: [PATCH 07/12] Re-add function --- awscli/botocore/handlers.py | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/awscli/botocore/handlers.py b/awscli/botocore/handlers.py index 8ba860957312..2274da56ddcf 100644 --- a/awscli/botocore/handlers.py +++ b/awscli/botocore/handlers.py @@ -114,6 +114,37 @@ def escape_xml_payload(params, **kwargs): params['body'] = body +def check_for_200_error(response, **kwargs): + """This function has been deprecated, but is kept for backwards compatibility.""" + # 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. + if response is None: + # A None response can happen if an exception is raised while + # trying to retrieve the response. See Endpoint._get_response(). + return + http_response, parsed = response + if _looks_like_special_case_error( + http_response.status_code, http_response.content + ): + logger.debug("Error found for response with 200 status code, " + "errors: %s, changing status code to " + "500.", parsed) + http_response.status_code = 500 + + def _looks_like_special_case_error(status_code, body): if status_code == 200 and body: try: From 4a16636e4ea0c16e35f9212b223aadd8a65ef268 Mon Sep 17 00:00:00 2001 From: aemous Date: Tue, 5 Nov 2024 14:02:34 -0500 Subject: [PATCH 08/12] Remove unused imports. --- tests/functional/s3/test_mv_command.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/functional/s3/test_mv_command.py b/tests/functional/s3/test_mv_command.py index 774c38c73441..9e4608fde8fa 100644 --- a/tests/functional/s3/test_mv_command.py +++ b/tests/functional/s3/test_mv_command.py @@ -12,13 +12,10 @@ # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. import os -from sys import exception from awscrt.s3 import S3RequestType -import botocore.exceptions from awscli.compat import BytesIO -from awscli.customizations.s3.utils import S3PathResolver from awscli.testutils import mock from tests.functional.s3 import ( BaseS3TransferCommandTest, BaseCRTTransferClientTest From 2677e6e4044816b5295df43619eae8e6509e7ddf Mon Sep 17 00:00:00 2001 From: Ahmed Moustafa <35640105+aemous@users.noreply.github.com> Date: Tue, 5 Nov 2024 14:15:59 -0500 Subject: [PATCH 09/12] Update awscli/botocore/handlers.py Co-authored-by: Nate Prewitt --- awscli/botocore/handlers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/awscli/botocore/handlers.py b/awscli/botocore/handlers.py index 2274da56ddcf..4f5caf458664 100644 --- a/awscli/botocore/handlers.py +++ b/awscli/botocore/handlers.py @@ -137,7 +137,7 @@ def check_for_200_error(response, **kwargs): return http_response, parsed = response if _looks_like_special_case_error( - http_response.status_code, http_response.content + http_response.status_code, http_response.content ): logger.debug("Error found for response with 200 status code, " "errors: %s, changing status code to " From 8d0115cbaa706ae9f9aac47469e5c7ceaa7152e6 Mon Sep 17 00:00:00 2001 From: aemous Date: Tue, 5 Nov 2024 14:20:43 -0500 Subject: [PATCH 10/12] Remove unused function and calling tests. --- awscli/botocore/handlers.py | 31 ---------------------------- tests/unit/botocore/test_handlers.py | 27 ------------------------ 2 files changed, 58 deletions(-) diff --git a/awscli/botocore/handlers.py b/awscli/botocore/handlers.py index 4f5caf458664..8ba860957312 100644 --- a/awscli/botocore/handlers.py +++ b/awscli/botocore/handlers.py @@ -114,37 +114,6 @@ def escape_xml_payload(params, **kwargs): params['body'] = body -def check_for_200_error(response, **kwargs): - """This function has been deprecated, but is kept for backwards compatibility.""" - # 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. - if response is None: - # A None response can happen if an exception is raised while - # trying to retrieve the response. See Endpoint._get_response(). - return - http_response, parsed = response - if _looks_like_special_case_error( - http_response.status_code, http_response.content - ): - logger.debug("Error found for response with 200 status code, " - "errors: %s, changing status code to " - "500.", parsed) - http_response.status_code = 500 - - def _looks_like_special_case_error(status_code, body): if status_code == 200 and body: try: diff --git a/tests/unit/botocore/test_handlers.py b/tests/unit/botocore/test_handlers.py index 2ad714f7a3c6..564abdbf0ad2 100644 --- a/tests/unit/botocore/test_handlers.py +++ b/tests/unit/botocore/test_handlers.py @@ -429,33 +429,6 @@ def test_presigned_url_casing_changed_for_rds(self): params['PreSignedUrl']) self.assertIn('X-Amz-Signature', params['PreSignedUrl']) - def test_500_status_code_set_for_200_response(self): - http_response = mock.Mock() - http_response.status_code = 200 - http_response.content = """ - - AccessDenied - Access Denied - id - hostid - - """ - handlers.check_for_200_error((http_response, {})) - 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 - http_response.content = "" - handlers.check_for_200_error((http_response, {})) - # We don't touch the status code since there are no errors present. - self.assertEqual(http_response.status_code, 200) - - def test_500_response_can_be_none(self): - # A 500 response can raise an exception, which means the response - # object is None. We need to handle this case. - handlers.check_for_200_error(None) - def test_route53_resource_id(self): event = 'before-parameter-build.route-53.GetHostedZone' params = {'Id': '/hostedzone/ABC123', From 7c518a9ada2ea32d9ed62ea97250babbca1f485b Mon Sep 17 00:00:00 2001 From: Ahmed Moustafa <35640105+aemous@users.noreply.github.com> Date: Thu, 7 Nov 2024 11:42:52 -0500 Subject: [PATCH 11/12] Update .changes/next-release/enhancement-s3-70184.json Co-authored-by: Steve Yoo <106777148+hssyoo@users.noreply.github.com> --- .changes/next-release/enhancement-s3-70184.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changes/next-release/enhancement-s3-70184.json b/.changes/next-release/enhancement-s3-70184.json index cb8579fd7913..a549b5e190f4 100644 --- a/.changes/next-release/enhancement-s3-70184.json +++ b/.changes/next-release/enhancement-s3-70184.json @@ -1,5 +1,5 @@ { "type": "enhancement", - "category": "s3", + "category": "``s3``", "description": "Handle HTTP 200 responses with error information for all supported s3 operations." } From 49daff700beb2dd86e23422d4746a162b2a33847 Mon Sep 17 00:00:00 2001 From: aemous Date: Thu, 7 Nov 2024 12:14:35 -0500 Subject: [PATCH 12/12] Remove unneeded test. --- tests/functional/s3/test_mv_command.py | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/tests/functional/s3/test_mv_command.py b/tests/functional/s3/test_mv_command.py index 9e4608fde8fa..c6bebdb3b625 100644 --- a/tests/functional/s3/test_mv_command.py +++ b/tests/functional/s3/test_mv_command.py @@ -266,22 +266,6 @@ def test_download_with_checksum_mode_crc32(self): self.assertEqual(self.operations_called[1][0].name, 'GetObject') self.assertEqual(self.operations_called[1][1]['ChecksumMode'], 'ENABLED') - def test_slowdown_raises_error(self): - self.parsed_responses = [ - # Response for HeadObject - {"ContentLength": 100, "LastModified": "00:00:00Z"}, - # Response for GetObject - {'ETag': '"foo-1"', 'Body': BytesIO(b'foo')}, - # Response for DeleteObject - {'Error': {'Code': 'SlowDown', 'Message': 'Please reduce your request rate.'}} - ] - cmdline = f'{self.prefix} s3://bucket/foo {self.files.rootdir}' - stdout, stderr, _ = self.run_cmd(cmdline, expected_rc=1) - self.assertIn( - 'An error occurred (SlowDown) when calling the DeleteObject operation: Please reduce your request rate.', - stderr - ) - class TestMvWithCRTClient(BaseCRTTransferClientTest): def test_upload_move_using_crt_client(self):