Respond with HTTP 409 on resource conflict

When raised, 'cinder.exception.InvalidVolume' with reason 'Invalid volume:
duplicate connectors detected on volume' or 'volume state in error'
comes along with a 500 HTTP error.

This change aims at returning a 409 HTTP error instead.

It takes over patch https://review.opendev.org/c/openstack/cinder/+/856041,
taking comments into account.

Change-Id: I22daff4c806a7c896d2a838b3e2ac0a81113b3e8

api-ref/source/v3/attachments.inc

@@ -326,6 +326,7 @@ Response codes
 
    - 400
    - 404
+   - 409
 
 Request
 -------

cinder/api/v3/attachments.py

@@ -250,11 +250,11 @@ class AttachmentsController(wsgi.Controller):
                 self.volume_api.attachment_update(context,
                                                   attachment_ref,
                                                   connector))
-        except exception.NotAuthorized:
+        except (exception.NotAuthorized, exception.Invalid):
             raise
         except exception.CinderException as ex:
             err_msg = (
-                _("Unable to update attachment.(%s).") % ex.msg)
+                _("Unable to update attachment (%s).") % ex.msg)
             LOG.exception(err_msg)
         except Exception:
             err_msg = _("Unable to update the attachment.")

cinder/exception.py

@@ -220,6 +220,11 @@ class InvalidVolume(Invalid):
     message = _("Invalid volume: %(reason)s")
 
 
+class ResourceConflict(Invalid):
+    message = _("Resource conflict: %(reason)s")
+    code = 409
+
+
 class InvalidContentType(Invalid):
     message = _("Invalid content type %(content_type)s.")
 

cinder/tests/unit/api/v3/test_attachments.py

@@ -18,6 +18,7 @@
 from unittest import mock
 
 import ddt
+import webob
 
 from cinder.api import microversions as mv
 from cinder.api.v3 import attachments as v3_attachments
@@ -140,6 +141,113 @@ class AttachmentsAPITestCase(test.TestCase):
                           self.controller.update, req,
                           self.attachment1.id, body=body)
 
+    @mock.patch.object(volume_api.API, 'attachment_update')
+    def test_update_attachment_not_authorized(self, mock_update):
+        exc = exception.NotAuthorized(reason='Operation is not authorized.')
+        mock_update.side_effect = exc
+        req = fakes.HTTPRequest.blank('/v3/%s/attachments/%s' %
+                                      (fake.PROJECT_ID, self.attachment1.id),
+                                      version=mv.NEW_ATTACH,
+                                      use_admin_context=True)
+        body = {
+            "attachment":
+                {
+                    "connector": {'fake_key': 'fake_value',
+                                  'host': 'somehost',
+                                  'connection_info': 'a'},
+                },
+        }
+
+        self.assertRaises(exception.NotAuthorized,
+                          self.controller.update, req,
+                          self.attachment1.id, body=body)
+
+    @mock.patch('cinder.volume.api.API.attachment_update')
+    def test_update_attachment_invalid_volume_conflict(self, mock_update):
+        exc = exception.ResourceConflict(
+            reason='Duplicate connectors or improper volume status')
+        mock_update.side_effect = exc
+
+        req = fakes.HTTPRequest.blank('/v3/%s/attachments/%s' %
+                                      (fake.PROJECT_ID, self.attachment1.id),
+                                      version=mv.NEW_ATTACH,
+                                      use_admin_context=True)
+        body = {
+            "attachment":
+                {
+                    "connector": {'fake_key': 'fake_value',
+                                  'host': 'somehost',
+                                  'connection_info': 'a'},
+                },
+        }
+
+        self.assertRaises(exception.ResourceConflict,
+                          self.controller.update, req,
+                          self.attachment1.id, body=body)
+
+    @mock.patch.object(volume_api.API, 'attachment_update')
+    def test_update_attachment_generic_exception_invalid(self, mock_update):
+        exc = exception.Invalid(reason='Invalid class generic Exception')
+        mock_update.side_effect = exc
+        req = fakes.HTTPRequest.blank('/v3/%s/attachments/%s' %
+                                      (fake.PROJECT_ID, self.attachment1.id),
+                                      version=mv.NEW_ATTACH,
+                                      use_admin_context=True)
+        body = {
+            "attachment":
+                {
+                    "connector": {'fake_key': 'fake_value',
+                                  'host': 'somehost',
+                                  'connection_info': 'a'},
+                },
+        }
+
+        self.assertRaises(exception.Invalid,
+                          self.controller.update, req,
+                          self.attachment1.id, body=body)
+
+    @mock.patch.object(volume_api.API, 'attachment_update')
+    def test_update_attachment_cinder_exception(self, mock_update):
+        exc = exception.CinderException(reason='Generic Cinder Exception')
+        mock_update.side_effect = exc
+        req = fakes.HTTPRequest.blank('/v3/%s/attachments/%s' %
+                                      (fake.PROJECT_ID, self.attachment1.id),
+                                      version=mv.NEW_ATTACH,
+                                      use_admin_context=True)
+        body = {
+            "attachment":
+                {
+                    "connector": {'fake_key': 'fake_value',
+                                  'host': 'somehost',
+                                  'connection_info': 'a'},
+                },
+        }
+
+        self.assertRaises(webob.exc.HTTPInternalServerError,
+                          self.controller.update, req,
+                          self.attachment1.id, body=body)
+
+    @mock.patch.object(volume_api.API, 'attachment_update')
+    def test_update_attachment_all_other_exceptions(self, mock_update):
+        exc = Exception('The most generic Exception')
+        mock_update.side_effect = exc
+        req = fakes.HTTPRequest.blank('/v3/%s/attachments/%s' %
+                                      (fake.PROJECT_ID, self.attachment1.id),
+                                      version=mv.NEW_ATTACH,
+                                      use_admin_context=True)
+        body = {
+            "attachment":
+                {
+                    "connector": {'fake_key': 'fake_value',
+                                  'host': 'somehost',
+                                  'connection_info': 'a'},
+                },
+        }
+
+        self.assertRaises(webob.exc.HTTPInternalServerError,
+                          self.controller.update, req,
+                          self.attachment1.id, body=body)
+
     @ddt.data(mv.get_prior_version(mv.RESOURCE_FILTER),
               mv.RESOURCE_FILTER, mv.LIKE_FILTER)
     @mock.patch('cinder.api.common.reject_invalid_filters')

cinder/tests/unit/attachments/test_attachments_api.py

@@ -302,11 +302,11 @@ class AttachmentManagerTestCase(test.TestCase):
         vref.save()
         connector = {'fake': 'connector',
                      'host': 'somehost'}
-        self.assertRaises(exception.InvalidVolume,
+        caught_exc = self.assertRaises(
-                          self.volume_api.attachment_update,
+            exception.ResourceConflict,
-                          self.context,
+            self.volume_api.attachment_update,
-                          aref,
+            self.context, aref, connector)
-                          connector)
+        self.assertEqual(409, caught_exc.code)
 
     @mock.patch('cinder.db.sqlalchemy.api.volume_attachment_update',
                 return_value={})
@@ -342,11 +342,13 @@ class AttachmentManagerTestCase(test.TestCase):
         with mock.patch('cinder.objects.Volume.get_by_id', return_value=vref):
             with mock.patch.object(self.volume_api.volume_rpcapi,
                                    'attachment_update') as m_au:
-                self.assertRaises(exception.InvalidVolume,
+                caught_exc = self.assertRaises(
-                                  self.volume_api.attachment_update,
+                    exception.ResourceConflict,
-                                  self.context,
+                    self.volume_api.attachment_update,
-                                  vref.volume_attachment[1],
+                    self.context,
-                                  connector)
+                    vref.volume_attachment[1],
+                    connector)
+                self.assertEqual(409, caught_exc.code)
                 m_au.assert_not_called()
         mock_va_update.assert_not_called()
         mock_db_upd.assert_not_called()

cinder/volume/api.py

@@ -2548,7 +2548,7 @@ class API(base.Base):
                        'volume_id': volume_ref.id,
                        'volume_status': volume_ref.status}
             LOG.error(msg)
-            raise exception.InvalidVolume(reason=msg)
+            raise exception.ResourceConflict(reason=msg)
 
         if (len(volume_ref.volume_attachment) > 1 and
             not (volume_ref.multiattach or
@@ -2570,7 +2570,7 @@ class API(base.Base):
                 msg = _('duplicate connectors detected on volume '
                         '%(vol)s') % {'vol': volume_ref.id}
 
-                raise exception.InvalidVolume(reason=msg)
+                raise exception.ResourceConflict(reason=msg)
 
         connection_info = (
             self.volume_rpcapi.attachment_update(ctxt,

releasenotes/notes/fix-500-http-error-on-resource-conflict.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    `Bug #1907295 <https://bugs.launchpad.net/cinder/+bug/1907295>`_: Fixed
+    When a volume was not in the correct status to accept an attachment update
+    (e.g.: volume in error or duplicate connectors), the REST API was returning
+    a 500 (Internal Server Error). It now correctly returns the response code
+    409 (Conflict) in this situation.