From 67ac0fca89d77c63958df0d3f2a9d13e26bef8eb Mon Sep 17 00:00:00 2001 From: flelain Date: Wed, 11 Dec 2024 10:47:58 +0100 Subject: [PATCH] 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 | 1 + cinder/api/v3/attachments.py | 4 +- cinder/exception.py | 5 + cinder/tests/unit/api/v3/test_attachments.py | 108 ++++++++++++++++++ .../unit/attachments/test_attachments_api.py | 22 ++-- cinder/volume/api.py | 4 +- ...x-500-http-error-on-resource-conflict.yaml | 8 ++ 7 files changed, 138 insertions(+), 14 deletions(-) create mode 100644 releasenotes/notes/fix-500-http-error-on-resource-conflict.yaml diff --git a/api-ref/source/v3/attachments.inc b/api-ref/source/v3/attachments.inc index 613ab4f7bfb..825403b6878 100644 --- a/api-ref/source/v3/attachments.inc +++ b/api-ref/source/v3/attachments.inc @@ -326,6 +326,7 @@ Response codes - 400 - 404 + - 409 Request ------- diff --git a/cinder/api/v3/attachments.py b/cinder/api/v3/attachments.py index f0c4110ccb5..a0cc86ac58c 100644 --- a/cinder/api/v3/attachments.py +++ b/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.") diff --git a/cinder/exception.py b/cinder/exception.py index e971e1c7c02..65cf50698e8 100644 --- a/cinder/exception.py +++ b/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.") diff --git a/cinder/tests/unit/api/v3/test_attachments.py b/cinder/tests/unit/api/v3/test_attachments.py index 35251357a2c..b4bd7019a14 100644 --- a/cinder/tests/unit/api/v3/test_attachments.py +++ b/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') diff --git a/cinder/tests/unit/attachments/test_attachments_api.py b/cinder/tests/unit/attachments/test_attachments_api.py index df5c0d8cfc6..beadb7b653c 100644 --- a/cinder/tests/unit/attachments/test_attachments_api.py +++ b/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, - self.volume_api.attachment_update, - self.context, - aref, - connector) + caught_exc = self.assertRaises( + exception.ResourceConflict, + self.volume_api.attachment_update, + self.context, aref, 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, - self.volume_api.attachment_update, - self.context, - vref.volume_attachment[1], - connector) + caught_exc = self.assertRaises( + exception.ResourceConflict, + self.volume_api.attachment_update, + self.context, + 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() diff --git a/cinder/volume/api.py b/cinder/volume/api.py index 0e20baff0f5..f4b5ee8b3eb 100644 --- a/cinder/volume/api.py +++ b/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, diff --git a/releasenotes/notes/fix-500-http-error-on-resource-conflict.yaml b/releasenotes/notes/fix-500-http-error-on-resource-conflict.yaml new file mode 100644 index 00000000000..bf87b1af6c2 --- /dev/null +++ b/releasenotes/notes/fix-500-http-error-on-resource-conflict.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + `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.