From 1c30d32264bc144fc26e02a3302306c50bfb2daa Mon Sep 17 00:00:00 2001 From: Feodor Tersin Date: Fri, 20 Feb 2015 20:21:14 +0300 Subject: [PATCH] Common exception handling Change-Id: Ibb2b9623f8d5fa7380c930e1b099028a94932b70 --- ec2api/api/__init__.py | 30 +++-- ec2api/api/image.py | 2 +- ec2api/api/instance.py | 2 +- ec2api/api/route_table.py | 6 +- ec2api/exception.py | 22 ++-- ec2api/tests/functional/api/test_images.py | 3 +- ec2api/tests/unit/base.py | 5 + ec2api/tests/unit/test_api_init.py | 30 ++++- ec2api/tests/unit/test_error_response.py | 132 --------------------- ec2api/tests/unit/test_image.py | 50 +++++--- ec2api/tests/unit/test_instance.py | 2 +- ec2api/tests/unit/test_tools.py | 9 ++ 12 files changed, 105 insertions(+), 188 deletions(-) delete mode 100644 ec2api/tests/unit/test_error_response.py diff --git a/ec2api/api/__init__.py b/ec2api/api/__init__.py index 1b3d5eeb..ae04d71d 100644 --- a/ec2api/api/__init__.py +++ b/ec2api/api/__init__.py @@ -317,7 +317,7 @@ def exception_to_ec2code(ex): return code -def ec2_error_ex(ex, req, code=None, message=None, unexpected=False): +def ec2_error_ex(ex, req, unexpected=False): """Return an EC2 error response. Return an EC2 error response based on passed exception and log @@ -332,12 +332,12 @@ def ec2_error_ex(ex, req, code=None, message=None, unexpected=False): Unexpected 5xx errors may contain sensitive information, suppress their messages for security. """ - if not code: - code = exception_to_ec2code(ex) - status = getattr(ex, 'code', None) - if not isinstance(status, int): - status = getattr(ex, 'status', None) - if not status or not isinstance(status, int): + code = exception_to_ec2code(ex) + for status_name in ('code', 'status', 'status_code', 'http_status'): + status = getattr(ex, status_name, None) + if isinstance(status, int): + break + else: status = 500 if unexpected: @@ -347,12 +347,6 @@ def ec2_error_ex(ex, req, code=None, message=None, unexpected=False): else: log_fun = LOG.debug log_msg = _("%(ex_name)s raised: %(ex_str)s") - # NOTE(jruzicka): For compatibility with EC2 API, treat expected - # exceptions as client (4xx) errors. The exception error code is 500 - # by default and most exceptions inherit this from EC2Exception even - # though they are actually client errors in most cases. - if status >= 500: - status = 400 exc_info = None context = req.environ['ec2api.context'] @@ -363,8 +357,14 @@ def ec2_error_ex(ex, req, code=None, message=None, unexpected=False): } log_fun(log_msg % log_msg_args, context=context, exc_info=exc_info) - if ex.args and not message and (not unexpected or status < 500): + if unexpected and status >= 500: + message = _('Unknown error occurred.') + elif getattr(ex, 'message', None): + message = unicode(ex.message) + elif ex.args and any(arg for arg in ex.args): message = " ".join(map(unicode, ex.args)) + else: + message = unicode(ex) if unexpected: # Log filtered environment for unexpected errors. env = req.environ.copy() @@ -372,8 +372,6 @@ def ec2_error_ex(ex, req, code=None, message=None, unexpected=False): if not isinstance(env[k], six.string_types): env.pop(k) log_fun(_('Environment: %s') % jsonutils.dumps(env)) - if not message: - message = _('Unknown error occurred.') return faults.ec2_error_response(request_id, code, message, status=status) diff --git a/ec2api/api/image.py b/ec2api/api/image.py index 4125df02..fa1956ba 100644 --- a/ec2api/api/image.py +++ b/ec2api/api/image.py @@ -25,7 +25,7 @@ import time import boto.s3.connection import eventlet -from glanceclient import exc as glance_exception +from glanceclient.common import exceptions as glance_exception from lxml import etree from oslo.config import cfg from oslo_concurrency import processutils diff --git a/ec2api/api/instance.py b/ec2api/api/instance.py index e9f8b023..4171fc2a 100644 --- a/ec2api/api/instance.py +++ b/ec2api/api/instance.py @@ -19,7 +19,7 @@ import itertools import random import re -from glanceclient import exc as glance_exception +from glanceclient.common import exceptions as glance_exception from novaclient import exceptions as nova_exception from oslo.config import cfg diff --git a/ec2api/api/route_table.py b/ec2api/api/route_table.py index cbc873fb..33f41cdd 100644 --- a/ec2api/api/route_table.py +++ b/ec2api/api/route_table.py @@ -67,9 +67,9 @@ def delete_route(context, route_table_id, destination_cidr_block): raise exception.InvalidParameterValue(msg) break else: - raise exception.InvalidRouteNotFound({ - 'route_table_id': route_table_id, - 'destination_cidr_block': destination_cidr_block}) + raise exception.InvalidRouteNotFound( + route_table_id=route_table_id, + destination_cidr_block=destination_cidr_block) rollback_route_table_state = copy.deepcopy(route_table) del route_table['routes'][route_index] with common.OnCrashCleaner() as cleaner: diff --git a/ec2api/exception.py b/ec2api/exception.py index 3ac24aef..ef9e2d36 100644 --- a/ec2api/exception.py +++ b/ec2api/exception.py @@ -23,6 +23,7 @@ SHOULD include dedicated exception logging. import sys from oslo.config import cfg +import six from ec2api.openstack.common.gettextutils import _ from ec2api.openstack.common import log as logging @@ -60,7 +61,7 @@ class EC2Exception(Exception): """ msg_fmt = _("An unknown exception occurred.") - code = 500 + code = 400 headers = {} safe = False @@ -76,12 +77,12 @@ class EC2Exception(Exception): if not message: try: message = self.msg_fmt % kwargs - except Exception: exc_info = sys.exc_info() # kwargs doesn't match a variable in the message # log the issue and the kwargs - LOG.exception(_('Exception in string format operation')) + LOG.exception(_('Exception in string format operation for ' + '%s exception'), self.__class__.__name__) for name, value in kwargs.iteritems(): LOG.error("%s: %s" % (name, value)) @@ -90,6 +91,14 @@ class EC2Exception(Exception): else: # at least get the core message out if something happened message = self.msg_fmt + elif not isinstance(message, six.string_types): + LOG.error(_("Message '%(msg)s' for %(ex)s exception is not " + "a string"), + {'msg': message, 'ex': self.__class__.__name__}) + if CONF.fatal_exception_format_errors: + raise TypeError(_('Invalid exception message format')) + else: + message = self.msg_fmt super(EC2Exception, self).__init__(message) @@ -101,17 +110,14 @@ class EC2Exception(Exception): class Unsupported(EC2Exception): msg_fmt = _("The specified request is unsupported. %(reason)s") - code = 400 class Overlimit(EC2Exception): msg_fmt = _("Limit exceeded.") - code = 400 class Invalid(EC2Exception): msg_fmt = _("Unacceptable parameters.") - code = 400 class InvalidRequest(Invalid): @@ -155,7 +161,6 @@ class ValidationError(Invalid): class EC2NotFound(EC2Exception): msg_fmt = _("Resource could not be found.") - code = 400 class InvalidInstanceIDNotFound(EC2NotFound): @@ -258,7 +263,6 @@ class InvalidAvailabilityZoneNotFound(EC2NotFound): class IncorrectState(EC2Exception): ec2_code = 'IncorrectState' - code = 400 msg_fmt = _("The resource is in incorrect state for the request - reason: " "'%(reason)s'") @@ -407,4 +411,4 @@ class RulesPerSecurityGroupLimitExceeded(Overlimit): class NovaDbInstanceNotFound(EC2Exception): - pass + code = 500 diff --git a/ec2api/tests/functional/api/test_images.py b/ec2api/tests/functional/api/test_images.py index 43de0dfe..4e8e8941 100644 --- a/ec2api/tests/functional/api/test_images.py +++ b/ec2api/tests/functional/api/test_images.py @@ -24,7 +24,7 @@ CONF = config.CONF class ImageTest(base.EC2TestCase): @testtools.skipUnless(CONF.aws.run_incompatible_tests, - "Openstack doesn't report right RootDeviceType") + "Openstack doesn't report right RootDeviceType") @testtools.skipUnless(CONF.aws.ebs_image_id, "EBS image id is not defined") def test_check_ebs_image_type(self): image_id = CONF.aws.ebs_image_id @@ -59,6 +59,7 @@ class ImageTest(base.EC2TestCase): self.assertIsNotNone(ebs.get('VolumeSize')) self.assertIsNotNone(ebs.get('VolumeType')) + @testtools.skipUnless(CONF.aws.ebs_image_id, "EBS image id is not defined") def test_describe_image_with_filters(self): image_id = CONF.aws.ebs_image_id resp, data = self.client.DescribeImages(ImageIds=[image_id]) diff --git a/ec2api/tests/unit/base.py b/ec2api/tests/unit/base.py index 656f8eda..0f970a23 100644 --- a/ec2api/tests/unit/base.py +++ b/ec2api/tests/unit/base.py @@ -15,6 +15,7 @@ import itertools import mock +from oslo.config import cfg from oslotest import base as test_base import ec2api.api.apirequest @@ -73,6 +74,10 @@ class ApiTestCase(test_base.BaseTestCase): self.isotime = isotime_patcher.start() self.addCleanup(isotime_patcher.stop) + conf = cfg.CONF + conf.set_override('fatal_exception_format_errors', True) + self.addCleanup(conf.reset) + def execute(self, action, args): ec2_request = ec2api.api.apirequest.APIRequest(action, 'fake_v1', args) ec2_context = self._create_context() diff --git a/ec2api/tests/unit/test_api_init.py b/ec2api/tests/unit/test_api_init.py index 5564eb11..c5e90957 100644 --- a/ec2api/tests/unit/test_api_init.py +++ b/ec2api/tests/unit/test_api_init.py @@ -15,7 +15,13 @@ import collections import uuid +from boto import exception as boto_exception +from cinderclient import exceptions as cinder_exception +from glanceclient.common import exceptions as glance_exception +from keystoneclient import exceptions as keystone_exception import mock +from neutronclient.common import exceptions as neutron_exception +from novaclient import exceptions as nova_exception from oslotest import base as test_base from ec2api import api @@ -83,9 +89,21 @@ class ApiInitTestCase(test_base.BaseTestCase): self.controller.fake_action.assert_called_once_with( self.fake_context, param='fake_param') - do_check(exception.EC2Exception('fake_msg'), 400, - 'EC2Exception', 'fake_msg') - do_check(KeyError('fake_msg'), 500, - 'KeyError', 'Unknown error occurred.') - do_check(exception.InvalidVpcIDNotFound('fake_msg'), 400, - 'InvalidVpcID.NotFound', 'fake_msg') + do_check(exception.EC2Exception('fake_msg'), + 400, 'EC2Exception', 'fake_msg') + do_check(KeyError('fake_msg'), + 500, 'KeyError', 'Unknown error occurred.') + do_check(exception.InvalidVpcIDNotFound('fake_msg'), + 400, 'InvalidVpcID.NotFound', 'fake_msg') + do_check(nova_exception.BadRequest(400, message='fake_msg'), + 400, 'BadRequest', 'fake_msg') + do_check(glance_exception.HTTPBadRequest(), + 400, 'HTTPBadRequest', 'HTTPBadRequest (HTTP 400)') + do_check(cinder_exception.BadRequest(400, message='fake_msg'), + 400, 'BadRequest', 'fake_msg') + do_check(neutron_exception.BadRequest(message='fake_msg'), + 400, 'BadRequest', 'fake_msg') + do_check(keystone_exception.BadRequest(message='fake_msg'), + 400, 'BadRequest', 'fake_msg') + do_check(boto_exception.S3ResponseError(400, '', 'fake_msg'), + 400, 'S3ResponseError', 'fake_msg') diff --git a/ec2api/tests/unit/test_error_response.py b/ec2api/tests/unit/test_error_response.py deleted file mode 100644 index 11348bb8..00000000 --- a/ec2api/tests/unit/test_error_response.py +++ /dev/null @@ -1,132 +0,0 @@ -# -# Copyright 2013 - Red Hat, Inc. -# -# Licensed under the Apache License, Version 2.0 (the "License"); you may -# not use this file except in compliance with the License. You may obtain -# a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License 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. -# - -""" -Unit tests for EC2 error responses. -""" - -from lxml import etree -from oslotest import base as test_base - -from ec2api import api as ec2 -from ec2api import context -from ec2api import wsgi - - -class TestClientExceptionEC2(Exception): - ec2_code = 'ClientException.Test' - message = "Test Client Exception." - code = 400 - - -class TestServerExceptionEC2(Exception): - ec2_code = 'ServerException.Test' - message = "Test Server Exception." - code = 500 - - -class Ec2ErrorResponseTestCase(test_base.BaseTestCase): - """Test EC2 error responses. - - This deals mostly with api/ec2/__init__.py code, especially - the ec2_error_ex helper. - """ - def setUp(self): - super(Ec2ErrorResponseTestCase, self).setUp() - self.context = context.RequestContext('test_user_id', - 'test_project_id') - self.req = wsgi.Request.blank('/test') - self.req.environ['ec2api.context'] = self.context - - def _validate_ec2_error(self, response, http_status, ec2_code, msg=None, - unknown_msg=False): - self.assertEqual(response.status_code, http_status, - 'Expected HTTP status %s' % http_status) - root_e = etree.XML(response.body) - self.assertEqual(root_e.tag, 'Response', - "Top element must be Response.") - errors_e = root_e.find('Errors') - self.assertEqual(len(errors_e), 1, - "Expected exactly one Error element in Errors.") - error_e = errors_e[0] - self.assertEqual(error_e.tag, 'Error', - "Expected Error element.") - # Code - code_e = error_e.find('Code') - self.assertIsNotNone(code_e, "Code element must be present.") - self.assertEqual(code_e.text, ec2_code) - # Message - if msg or unknown_msg: - message_e = error_e.find('Message') - self.assertIsNotNone(code_e, "Message element must be present.") - if msg: - self.assertEqual(message_e.text, msg) - elif unknown_msg: - self.assertEqual(message_e.text, "Unknown error occurred.", - "Error message should be anonymous.") - # RequestID - requestid_e = root_e.find('RequestID') - self.assertIsNotNone(requestid_e, - 'RequestID element should be present.') - self.assertEqual(requestid_e.text, self.context.request_id) - - def test_exception_ec2_4xx(self): - """Test response to EC2 exception with code = 400.""" - msg = "Test client failure." - err = ec2.ec2_error_ex(TestClientExceptionEC2(msg), self.req) - self._validate_ec2_error(err, TestClientExceptionEC2.code, - TestClientExceptionEC2.ec2_code, msg) - - def test_exception_ec2_5xx(self): - """Test response to EC2 exception with code = 500. - - Expected errors are treated as client ones even with 5xx code. - """ - msg = "Test client failure with 5xx error code." - err = ec2.ec2_error_ex(TestServerExceptionEC2(msg), self.req) - self._validate_ec2_error(err, 400, TestServerExceptionEC2.ec2_code, - msg) - - def test_unexpected_exception_ec2_4xx(self): - """Test response to unexpected EC2 exception with code = 400.""" - msg = "Test unexpected client failure." - err = ec2.ec2_error_ex(TestClientExceptionEC2(msg), self.req, - unexpected=True) - self._validate_ec2_error(err, TestClientExceptionEC2.code, - TestClientExceptionEC2.ec2_code, msg) - - def test_unexpected_exception_ec2_5xx(self): - """Test response to unexpected EC2 exception with code = 500. - - Server exception messages (with code >= 500 or without code) should - be filtered as they might contain sensitive information. - """ - msg = "Test server failure." - err = ec2.ec2_error_ex(TestServerExceptionEC2(msg), self.req, - unexpected=True) - self._validate_ec2_error(err, TestServerExceptionEC2.code, - TestServerExceptionEC2.ec2_code, - unknown_msg=True) - - def test_unexpected_exception_builtin(self): - """Test response to builtin unexpected exception. - - Server exception messages (with code >= 500 or without code) should - be filtered as they might contain sensitive information. - """ - msg = "Test server failure." - err = ec2.ec2_error_ex(RuntimeError(msg), self.req, unexpected=True) - self._validate_ec2_error(err, 500, 'RuntimeError', unknown_msg=True) diff --git a/ec2api/tests/unit/test_image.py b/ec2api/tests/unit/test_image.py index 7ef68732..e3c4dc80 100644 --- a/ec2api/tests/unit/test_image.py +++ b/ec2api/tests/unit/test_image.py @@ -160,7 +160,7 @@ class ImageTestCase(base.ApiTestCase): {'ImageLocation': fakes.LOCATION_IMAGE_1}) self.assertThat(resp, matchers.DictMatches( {'http_status_code': 200, - 'imageId': fakes.ID_EC2_IMAGE_1})) + 'imageId': fakes.ID_EC2_IMAGE_1})) s3_create.assert_called_once_with( mock.ANY, @@ -174,7 +174,7 @@ class ImageTestCase(base.ApiTestCase): 'Name': 'an image name'}) self.assertThat(resp, matchers.DictMatches( {'http_status_code': 200, - 'imageId': fakes.ID_EC2_IMAGE_1})) + 'imageId': fakes.ID_EC2_IMAGE_1})) s3_create.assert_called_once_with( mock.ANY, @@ -196,19 +196,31 @@ class ImageTestCase(base.ApiTestCase): 'BlockDeviceMapping.1.Ebs.SnapshotId': fakes.ID_EC2_SNAPSHOT_1}) self.assertThat(resp, matchers.DictMatches( {'http_status_code': 200, - 'imageId': fakes.ID_EC2_IMAGE_2})) + 'imageId': fakes.ID_EC2_IMAGE_2})) self.db_api.get_item_by_id.assert_called_once_with( mock.ANY, 'snap', fakes.ID_EC2_SNAPSHOT_1) self.db_api.add_item.assert_called_once_with( mock.ANY, 'ami', {'os_id': fakes.ID_OS_IMAGE_2, 'is_public': False}) - self.glance.images.create.assert_called_once_with( - is_public=False, size=0, name='fake_name', - properties={'root_device_name': fakes.ROOT_DEVICE_NAME_IMAGE_2, - 'block_device_mapping': json.dumps( - [{'device_name': fakes.ROOT_DEVICE_NAME_IMAGE_2, - 'delete_on_termination': True, - 'snapshot_id': fakes.ID_OS_SNAPSHOT_1}])}) + self.assertEqual(1, self.glance.images.create.call_count) + self.assertEqual((), self.glance.images.create.call_args[0]) + self.assertIn('properties', self.glance.images.create.call_args[1]) + self.assertIsInstance( + self.glance.images.create.call_args[1]['properties'], + dict) + bdm = self.glance.images.create.call_args[1]['properties'].pop( + 'block_device_mapping', None) + self.assertEqual( + {'is_public': False, + 'size': 0, + 'name': 'fake_name', + 'properties': { + 'root_device_name': fakes.ROOT_DEVICE_NAME_IMAGE_2}}, + self.glance.images.create.call_args[1]) + self.assertEqual([{'device_name': fakes.ROOT_DEVICE_NAME_IMAGE_2, + 'delete_on_termination': True, + 'snapshot_id': fakes.ID_OS_SNAPSHOT_1}], + json.loads(bdm)) def test_register_image_invalid_parameters(self): resp = self.execute('RegisterImage', {}) @@ -264,11 +276,11 @@ class ImageTestCase(base.ApiTestCase): 'DescribeImages', 'imagesSet', [('architecture', 'x86_64'), # TODO(ft): store a description in DB -# ('description', ''), + # ('description', ''), ('image-id', fakes.ID_EC2_IMAGE_1), ('image-type', 'machine'), # TODO(ft): support filtering by a boolean value -# ('is-public', True), + # ('is-public', True), ('kernel_id', fakes.ID_EC2_IMAGE_AKI_1,), ('name', 'fake_name'), ('owner-id', fakes.ID_OS_PROJECT), @@ -330,13 +342,13 @@ class ImageTestCase(base.ApiTestCase): do_check('blockDeviceMapping', fakes.ID_EC2_IMAGE_1, - {'blockDeviceMapping': - fakes.EC2_IMAGE_1['blockDeviceMapping']}) + {'blockDeviceMapping': ( + fakes.EC2_IMAGE_1['blockDeviceMapping'])}) do_check('blockDeviceMapping', fakes.ID_EC2_IMAGE_2, - {'blockDeviceMapping': - fakes.EC2_IMAGE_2['blockDeviceMapping']}) + {'blockDeviceMapping': ( + fakes.EC2_IMAGE_2['blockDeviceMapping'])}) @mock.patch.object(fakes.OSImage, 'update', autospec=True) def test_modify_image_attributes(self, osimage_update): @@ -604,9 +616,11 @@ class S3TestCase(base.ApiTestCase): 'image_location': 'fake_bucket/fake_manifest'}) def test_s3_malicious_tarballs(self): - self.assertRaises(exception.Invalid, + self.assertRaises( + exception.Invalid, image_api._s3_test_for_malicious_tarball, "/unused", os.path.join(os.path.dirname(__file__), 'abs.tar.gz')) - self.assertRaises(exception.Invalid, + self.assertRaises( + exception.Invalid, image_api._s3_test_for_malicious_tarball, "/unused", os.path.join(os.path.dirname(__file__), 'rel.tar.gz')) diff --git a/ec2api/tests/unit/test_instance.py b/ec2api/tests/unit/test_instance.py index cafbc0df..07aad0f9 100644 --- a/ec2api/tests/unit/test_instance.py +++ b/ec2api/tests/unit/test_instance.py @@ -17,7 +17,7 @@ import datetime import itertools import random -from glanceclient import exc as glance_exception +from glanceclient.common import exceptions as glance_exception import mock from novaclient import exceptions as nova_exception from oslotest import base as test_base diff --git a/ec2api/tests/unit/test_tools.py b/ec2api/tests/unit/test_tools.py index 884c5703..f442541d 100644 --- a/ec2api/tests/unit/test_tools.py +++ b/ec2api/tests/unit/test_tools.py @@ -15,6 +15,8 @@ import testtools +from ec2api import exception +from ec2api.tests.unit import base from ec2api.tests.unit import tools @@ -43,3 +45,10 @@ class TestToolsTestCase(testtools.TestCase): res = tools.patch_dict(d1, d2, ('b', 'e')) self.assertEqual({'a': 1, 'c': 33, 'd': 44}, res) self.assertEqual({'a': 1, 'b': 2, 'c': 3}, d1) + + +class TestBaseTestCase(base.ApiTestCase): + + def test_validate_exception_format_is_enabled_for_tests(self): + self.assertRaises(KeyError, exception.InvalidVpcRange, fake='value') + self.assertRaises(TypeError, exception.InvalidId, {'id': 'value'})