Common exception handling

Change-Id: Ibb2b9623f8d5fa7380c930e1b099028a94932b70

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)
-        code = exception_to_ec2code(ex)
+    for status_name in ('code', 'status', 'status_code', 'http_status'):
-    status = getattr(ex, 'code', None)
+        status = getattr(ex, status_name, None)
-    if not isinstance(status, int):
+        if isinstance(status, int):
-        status = getattr(ex, 'status', None)
+            break
-    if not status or not isinstance(status, int):
+    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)
 
 

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

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
 

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({
+        raise exception.InvalidRouteNotFound(
-            'route_table_id': route_table_id,
+            route_table_id=route_table_id,
-            'destination_cidr_block': destination_cidr_block})
+            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:

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

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])

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()

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,
+        do_check(exception.EC2Exception('fake_msg'),
-                 'EC2Exception', 'fake_msg')
+                 400, 'EC2Exception', 'fake_msg')
-        do_check(KeyError('fake_msg'), 500,
+        do_check(KeyError('fake_msg'),
-                 'KeyError', 'Unknown error occurred.')
+                 500, 'KeyError', 'Unknown error occurred.')
-        do_check(exception.InvalidVpcIDNotFound('fake_msg'), 400,
+        do_check(exception.InvalidVpcIDNotFound('fake_msg'),
-                 'InvalidVpcID.NotFound', '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')

ec2api/tests/unit/test_error_response.py

@@ -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)

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(
+        self.assertEqual(1, self.glance.images.create.call_count)
-            is_public=False, size=0, name='fake_name',
+        self.assertEqual((), self.glance.images.create.call_args[0])
-            properties={'root_device_name': fakes.ROOT_DEVICE_NAME_IMAGE_2,
+        self.assertIn('properties', self.glance.images.create.call_args[1])
-                        'block_device_mapping': json.dumps(
+        self.assertIsInstance(
-                            [{'device_name': fakes.ROOT_DEVICE_NAME_IMAGE_2,
+            self.glance.images.create.call_args[1]['properties'],
-                              'delete_on_termination': True,
+            dict)
-                              'snapshot_id': fakes.ID_OS_SNAPSHOT_1}])})
+        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':
+                 {'blockDeviceMapping': (
-                        fakes.EC2_IMAGE_1['blockDeviceMapping']})
+                        fakes.EC2_IMAGE_1['blockDeviceMapping'])})
 
         do_check('blockDeviceMapping',
                  fakes.ID_EC2_IMAGE_2,
-                 {'blockDeviceMapping':
+                 {'blockDeviceMapping': (
-                        fakes.EC2_IMAGE_2['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'))

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

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'})