-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve error message for sigv4 and PermanentRedirect errors #968
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
# Copyright 2014 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"). You | ||
# may not use this file except in compliance with the License. A copy of | ||
# the License is located at | ||
# | ||
# http://aws.amazon.com/apache2.0/ | ||
# | ||
# or in the "license" file accompanying this file. This file is | ||
# distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF | ||
# ANY KIND, either express or implied. See the License for the specific | ||
# language governing permissions and limitations under the License. | ||
"""Give better S3 error messages. | ||
""" | ||
from awscli.customizations import utils | ||
|
||
|
||
REGION_ERROR_MSG = ( | ||
'You can fix this issue by explicity providing the correct region ' | ||
'location using the --region argument, the AWS_DEFAULT_REGION ' | ||
'environment variable, or the region variable in the AWS CLI ' | ||
"configuration file. You can get the bucket's location by " | ||
'running "aws s3api get-bucket-location --bucket BUCKET".' | ||
) | ||
|
||
|
||
def register_s3_error_msg(event_handlers): | ||
event_handlers.register('after-call.s3', enhance_error_msg) | ||
|
||
|
||
def enhance_error_msg(parsed, **kwargs): | ||
if parsed is None or 'Error' not in parsed: | ||
# There's no error message to enhance so we can continue. | ||
return | ||
if _is_sigv4_error_message(parsed): | ||
message = ( | ||
'You are attempting to operate on a bucket in a region ' | ||
'that requires Signature Version 4. ' | ||
) | ||
message += REGION_ERROR_MSG | ||
parsed['Error']['Message'] = message | ||
elif _is_permanent_redirect_message(parsed): | ||
endpoint = parsed['Error']['Endpoint'] | ||
message = parsed['Error']['Message'] | ||
new_message = message[:-1] + ': %s\n' % endpoint | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will the endpoint be the full name of the endpoint or will it be the name of the region? I could see this being confusing if it gives back the full name of the endpoint. But should be ok if the user just uses the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's true, it will be the full point, something like, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think later down the road if there are more requests. Always printing out the suggestion to run the |
||
new_message += REGION_ERROR_MSG | ||
parsed['Error']['Message'] = new_message | ||
|
||
|
||
def _is_sigv4_error_message(parsed): | ||
return ('Please use AWS4-HMAC-SHA256' in | ||
parsed.get('Error', {}).get('Message', '')) | ||
|
||
|
||
def _is_permanent_redirect_message(parsed): | ||
return parsed.get('Error', {}).get('Code', '') == 'PermanentRedirect' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
# Copyright 2014 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"). You | ||
# may not use this file except in compliance with the License. A copy of | ||
# the License is located at | ||
# | ||
# http://aws.amazon.com/apache2.0e | ||
# | ||
# or in the "license" file accompanying this file. This file is | ||
# distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF | ||
# ANY KIND, either express or implied. See the License for the specific | ||
# language governing permissions and limitations under the License. | ||
import copy | ||
|
||
from awscli.testutils import unittest | ||
from awscli.customizations import s3errormsg | ||
|
||
|
||
class TestGetRegionFromEndpoint(unittest.TestCase): | ||
|
||
def test_sigv4_error_message(self): | ||
parsed = { | ||
'Error': { | ||
'Message': 'Please use AWS4-HMAC-SHA256' | ||
} | ||
} | ||
s3errormsg.enhance_error_msg(parsed) | ||
# We should say how to fix the issue. | ||
self.assertIn('You can fix this issue', | ||
parsed['Error']['Message']) | ||
# We should mention the --region argument. | ||
self.assertIn('--region', parsed['Error']['Message']) | ||
# We should mention get-bucket-location | ||
self.assertIn('get-bucket-location', parsed['Error']['Message']) | ||
|
||
def test_301_error_message(self): | ||
parsed = { | ||
'Error': { | ||
'Code': 'PermanentRedirect', | ||
'Message': "Please use the correct endpoint.", | ||
'Endpoint': "myendpoint", | ||
} | ||
} | ||
s3errormsg.enhance_error_msg(parsed) | ||
# We should include the endpoint in the error message. | ||
error_message = parsed['Error']['Message'] | ||
self.assertIn('myendpoint', error_message) | ||
|
||
def test_error_message_not_enhanced(self): | ||
parsed = { | ||
'Error': { | ||
'Message': 'This is a different error message.', | ||
'Code': 'Other Message' | ||
} | ||
} | ||
expected = copy.deepcopy(parsed) | ||
s3errormsg.enhance_error_msg(parsed) | ||
# Nothing should have changed | ||
self.assertEqual(parsed, expected) | ||
|
||
def test_not_an_error_message(self): | ||
parsed = { | ||
'Success': 'response', | ||
'ResponseMetadata': {} | ||
} | ||
expected = copy.deepcopy(parsed) | ||
s3errormsg.enhance_error_msg(parsed) | ||
# Nothing should have changed | ||
self.assertEqual(parsed, expected) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if there was someway to notify the user what region they made the request with. But I don't think that the event emits that information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No there currently is not any of that info in the event, but it could be useful to add in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.