diff --git a/doc/v3/api_samples/os-volumes/update-volume-req.json b/doc/v3/api_samples/os-volumes/update-volume-req.json index 09b42cf99691..bba735eec834 100644 --- a/doc/v3/api_samples/os-volumes/update-volume-req.json +++ b/doc/v3/api_samples/os-volumes/update-volume-req.json @@ -1,6 +1,5 @@ { "volumeAttachment": { - "volumeId": "a26887c6-c47b-4654-abb5-dfadf7d3f805", - "device": "/dev/sdd" + "volumeId": "a26887c6-c47b-4654-abb5-dfadf7d3f805" } } \ No newline at end of file diff --git a/nova/api/openstack/compute/plugins/v3/volumes.py b/nova/api/openstack/compute/plugins/v3/volumes.py index 848995a09269..be30179a5783 100644 --- a/nova/api/openstack/compute/plugins/v3/volumes.py +++ b/nova/api/openstack/compute/plugins/v3/volumes.py @@ -27,7 +27,6 @@ from nova import compute from nova import exception from nova.i18n import _ from nova import objects -from nova.openstack.common import uuidutils from nova import volume ALIAS = "os-volumes" @@ -264,31 +263,17 @@ class VolumeAttachmentController(wsgi.Controller): instance['uuid'], assigned_mountpoint)} - def _validate_volume_id(self, volume_id): - if not uuidutils.is_uuid_like(volume_id): - msg = _("Bad volumeId format: volumeId is " - "not in proper format (%s)") % volume_id - raise exc.HTTPBadRequest(explanation=msg) - @extensions.expected_errors((400, 404, 409)) + @validation.schema(volumes_schema.create_volume_attachment) def create(self, req, server_id, body): """Attach a volume to an instance.""" context = req.environ['nova.context'] authorize(context) authorize_attach(context, action='create') - if not self.is_valid_body(body, 'volumeAttachment'): - msg = _("volumeAttachment not specified") - raise exc.HTTPBadRequest(explanation=msg) - try: - volume_id = body['volumeAttachment']['volumeId'] - except KeyError: - msg = _("volumeId must be specified.") - raise exc.HTTPBadRequest(explanation=msg) + volume_id = body['volumeAttachment']['volumeId'] device = body['volumeAttachment'].get('device') - self._validate_volume_id(volume_id) - instance = common.get_instance(self.compute_api, context, server_id, want_objects=True) try: @@ -325,25 +310,17 @@ class VolumeAttachmentController(wsgi.Controller): @wsgi.response(202) @extensions.expected_errors((400, 404, 409)) + @validation.schema(volumes_schema.update_volume_attachment) def update(self, req, server_id, id, body): context = req.environ['nova.context'] authorize(context) authorize_attach(context, action='update') - if not self.is_valid_body(body, 'volumeAttachment'): - msg = _("volumeAttachment not specified") - raise exc.HTTPBadRequest(explanation=msg) - old_volume_id = id try: old_volume = self.volume_api.get(context, old_volume_id) - try: - new_volume_id = body['volumeAttachment']['volumeId'] - except KeyError: - msg = _("volumeId must be specified.") - raise exc.HTTPBadRequest(explanation=msg) - self._validate_volume_id(new_volume_id) + new_volume_id = body['volumeAttachment']['volumeId'] new_volume = self.volume_api.get(context, new_volume_id) except exception.VolumeNotFound as e: raise exc.HTTPNotFound(explanation=e.format_message()) diff --git a/nova/api/openstack/compute/schemas/v3/volumes.py b/nova/api/openstack/compute/schemas/v3/volumes.py index 897ae6a04784..c85d80305691 100644 --- a/nova/api/openstack/compute/schemas/v3/volumes.py +++ b/nova/api/openstack/compute/schemas/v3/volumes.py @@ -12,6 +12,8 @@ # License for the specific language governing permissions and limitations # under the License. +import copy + from nova.api.validation import parameter_types create = { @@ -58,3 +60,29 @@ snapshot_create = { 'required': ['snapshot'], 'additionalProperties': False, } + +create_volume_attachment = { + 'type': 'object', + 'properties': { + 'volumeAttachment': { + 'type': 'object', + 'properties': { + 'volumeId': parameter_types.volume_id, + 'device': { + 'type': 'string', + # NOTE: The validation pattern from match_device() in + # nova/block_device.py. + 'pattern': '(^/dev/x{0,1}[a-z]{0,1}d{0,1})([a-z]+)[0-9]*$' + } + }, + 'required': ['volumeId'], + 'additionalProperties': False, + }, + }, + 'required': ['volumeAttachment'], + 'additionalProperties': False, +} + +update_volume_attachment = copy.deepcopy(create_volume_attachment) +del update_volume_attachment['properties']['volumeAttachment'][ + 'properties']['device'] diff --git a/nova/tests/functional/v3/api_samples/os-volumes/update-volume-req.json.tpl b/nova/tests/functional/v3/api_samples/os-volumes/update-volume-req.json.tpl index 3d360a57bced..41411472ab09 100644 --- a/nova/tests/functional/v3/api_samples/os-volumes/update-volume-req.json.tpl +++ b/nova/tests/functional/v3/api_samples/os-volumes/update-volume-req.json.tpl @@ -1,6 +1,5 @@ { "volumeAttachment": { - "volumeId": "%(volume_id)s", - "device": "%(device)s" + "volumeId": "%(volume_id)s" } } diff --git a/nova/tests/functional/v3/test_volumes.py b/nova/tests/functional/v3/test_volumes.py index 0a5b65c61390..9b8640d2bca0 100644 --- a/nova/tests/functional/v3/test_volumes.py +++ b/nova/tests/functional/v3/test_volumes.py @@ -295,8 +295,7 @@ class VolumeAttachmentsSampleJsonTest(VolumeAttachmentsSampleBase): def test_volume_attachment_update(self): self.stubs.Set(cinder.API, 'get', fakes.stub_volume_get) subs = { - 'volume_id': 'a26887c6-c47b-4654-abb5-dfadf7d3f805', - 'device': '/dev/sdd' + 'volume_id': 'a26887c6-c47b-4654-abb5-dfadf7d3f805' } server_id = self._post_server() attach_id = 'a26887c6-c47b-4654-abb5-dfadf7d3f803' diff --git a/nova/tests/unit/api/openstack/compute/contrib/test_volumes.py b/nova/tests/unit/api/openstack/compute/contrib/test_volumes.py index 4229f386c5a7..7cb821077cc6 100644 --- a/nova/tests/unit/api/openstack/compute/contrib/test_volumes.py +++ b/nova/tests/unit/api/openstack/compute/contrib/test_volumes.py @@ -343,6 +343,8 @@ class VolumeApiTestV2(VolumeApiTestV21): class VolumeAttachTestsV21(test.TestCase): + validation_error = exception.ValidationError + def setUp(self): super(VolumeAttachTestsV21, self).setUp() self.stubs.Set(db, 'block_device_mapping_get_all_by_instance', @@ -532,7 +534,7 @@ class VolumeAttachTestsV21(test.TestCase): req.headers['content-type'] = 'application/json' req.environ['nova.context'] = self.context - self.assertRaises(webob.exc.HTTPBadRequest, self.attachments.create, + self.assertRaises(self.validation_error, self.attachments.create, req, FAKE_UUID, body=body) def test_attach_volume_without_volumeId(self): @@ -552,7 +554,21 @@ class VolumeAttachTestsV21(test.TestCase): req.headers['content-type'] = 'application/json' req.environ['nova.context'] = self.context - self.assertRaises(webob.exc.HTTPBadRequest, self.attachments.create, + self.assertRaises(self.validation_error, self.attachments.create, + req, FAKE_UUID, body=body) + + def test_attach_volume_with_extra_arg(self): + body = {'volumeAttachment': {'volumeId': FAKE_UUID_A, + 'device': '/dev/fake', + 'extra': 'extra_arg'}} + + req = webob.Request.blank('/v2/servers/id/os-volume_attachments') + req.method = 'POST' + req.body = jsonutils.dumps({}) + req.headers['content-type'] = 'application/json' + req.environ['nova.context'] = self.context + + self.assertRaises(self.validation_error, self.attachments.create, req, FAKE_UUID, body=body) def _test_swap(self, attachments, uuid=FAKE_UUID_A, @@ -561,8 +577,7 @@ class VolumeAttachTestsV21(test.TestCase): self.stubs.Set(compute_api.API, 'swap_volume', fake_func) - body = body or {'volumeAttachment': {'volumeId': FAKE_UUID_B, - 'device': '/dev/fake'}} + body = body or {'volumeAttachment': {'volumeId': FAKE_UUID_B}} req = webob.Request.blank('/v2/servers/id/os-volume_attachments/uuid') req.method = 'PUT' @@ -598,13 +613,23 @@ class VolumeAttachTestsV21(test.TestCase): def test_swap_volume_without_volumeId(self): body = {'volumeAttachment': {'device': '/dev/fake'}} - self.assertRaises(webob.exc.HTTPBadRequest, + self.assertRaises(self.validation_error, + self._test_swap, + self.attachments, + body=body) + + def test_swap_volume_with_extra_arg(self): + body = {'volumeAttachment': {'volumeId': FAKE_UUID_A, + 'device': '/dev/fake'}} + + self.assertRaises(self.validation_error, self._test_swap, self.attachments, body=body) class VolumeAttachTestsV2(VolumeAttachTestsV21): + validation_error = webob.exc.HTTPBadRequest def _set_up_controller(self): ext_mgr = extensions.ExtensionManager() @@ -619,6 +644,30 @@ class VolumeAttachTestsV2(VolumeAttachTestsV21): self.assertRaises(webob.exc.HTTPBadRequest, self._test_swap, self.attachments_no_update) + @mock.patch.object(compute_api.API, 'attach_volume', + return_value=[]) + def test_attach_volume_with_extra_arg(self, mock_attach): + # NOTE(gmann): V2 does not perform strong input validation + # so volume is attached successfully even with extra arg in + # request body. + body = {'volumeAttachment': {'volumeId': FAKE_UUID_A, + 'device': '/dev/fake', + 'extra': 'extra_arg'}} + req = webob.Request.blank('/v2/servers/id/os-volume_attachments') + req.method = 'POST' + req.body = jsonutils.dumps({}) + req.headers['content-type'] = 'application/json' + req.environ['nova.context'] = self.context + result = self.attachments.create(req, FAKE_UUID, body=body) + self.assertEqual(result['volumeAttachment']['id'], + '00000000-aaaa-aaaa-aaaa-000000000000') + + def test_swap_volume_with_extra_arg(self): + # NOTE(gmann): V2 does not perform strong input validation. + # Volume is swapped successfully even with extra arg in + # request body. So 'pass' this test for V2. + pass + class VolumeSerializerTest(test.TestCase): def _verify_volume_attachment(self, attach, tree):