From 000bc72cc29b8637df496f2e6768281606a27058 Mon Sep 17 00:00:00 2001 From: Steve Baker Date: Thu, 10 Sep 2020 09:48:45 +1200 Subject: [PATCH] Convert volume/targets endpoint to plain JSON Change-Id: I4d7f13ffb1dd438d9136238fa75a3efaac4e8b7e Story: 1651346 Task: 10551 --- ironic/api/controllers/v1/volume_target.py | 352 ++++++------------ .../api/controllers/v1/test_volume_target.py | 14 +- ironic/tests/unit/api/utils.py | 7 +- 3 files changed, 127 insertions(+), 246 deletions(-) diff --git a/ironic/api/controllers/v1/volume_target.py b/ironic/api/controllers/v1/volume_target.py index 6667bcca5f..4830381633 100644 --- a/ironic/api/controllers/v1/volume_target.py +++ b/ironic/api/controllers/v1/volume_target.py @@ -12,7 +12,6 @@ # License for the specific language governing permissions and limitations # under the License. -import datetime from http import client as http_client from ironic_lib import metrics_utils @@ -20,14 +19,12 @@ from oslo_utils import uuidutils from pecan import rest from ironic import api -from ironic.api.controllers import base from ironic.api.controllers import link from ironic.api.controllers.v1 import collection from ironic.api.controllers.v1 import notification_utils as notify -from ironic.api.controllers.v1 import types from ironic.api.controllers.v1 import utils as api_utils -from ironic.api import expose -from ironic.api import types as atypes +from ironic.api import method +from ironic.common import args from ironic.common import exception from ironic.common.i18n import _ from ironic.common import policy @@ -35,197 +32,83 @@ from ironic import objects METRICS = metrics_utils.get_metrics_logger(__name__) -_DEFAULT_RETURN_FIELDS = ('uuid', 'node_uuid', 'volume_type', - 'boot_index', 'volume_id') +_DEFAULT_RETURN_FIELDS = ['uuid', 'node_uuid', 'volume_type', + 'boot_index', 'volume_id'] + +TARGET_SCHEMA = { + 'type': 'object', + 'properties': { + 'boot_index': {'type': 'integer'}, + 'extra': {'type': ['object', 'null']}, + 'node_uuid': {'type': 'string'}, + 'properties': {'type': ['object', 'null']}, + 'volume_id': {'type': 'string'}, + 'volume_type': {'type': 'string'}, + 'uuid': {'type': ['string', 'null']}, + }, + 'required': ['boot_index', 'node_uuid', 'volume_id', 'volume_type'], + 'additionalProperties': False, +} + +TARGET_VALIDATOR_EXTRA = args.dict_valid( + node_uuid=args.uuid, + uuid=args.uuid, +) + +TARGET_VALIDATOR = args.and_valid( + args.schema(TARGET_SCHEMA), + TARGET_VALIDATOR_EXTRA +) + +PATCH_ALLOWED_FIELDS = [ + 'boot_index', + 'extra', + 'node_uuid', + 'properties', + 'volume_id', + 'volume_type' +] -class VolumeTarget(base.APIBase): - """API representation of a volume target. +def convert_with_links(rpc_target, fields=None, sanitize=True): + target = api_utils.object_to_dict( + rpc_target, + link_resource='volume/targets', + fields=( + 'boot_index', + 'extra', + 'properties', + 'volume_id', + 'volume_type' + ) + ) + api_utils.populate_node_uuid(rpc_target, target) - This class enforces type checking and value constraints, and converts - between the internal object model and the API representation of a volume - target. - """ + if fields is not None: + api_utils.check_for_invalid_fields(fields, target) - _node_uuid = None - - def _get_node_uuid(self): - return self._node_uuid - - def _set_node_identifiers(self, value): - """Set both UUID and ID of a node for VolumeTarget object - - :param value: UUID, ID of a node, or atypes.Unset - """ - if value == atypes.Unset: - self._node_uuid = atypes.Unset - elif value and self._node_uuid != value: - try: - node = objects.Node.get(api.request.context, value) - self._node_uuid = node.uuid - # NOTE(smoriya): Create the node_id attribute on-the-fly - # to satisfy the api -> rpc object conversion. - self.node_id = node.id - except exception.NodeNotFound as e: - # Change error code because 404 (NotFound) is inappropriate - # response for a POST request to create a VolumeTarget - e.code = http_client.BAD_REQUEST # BadRequest - raise - - uuid = types.uuid - """Unique UUID for this volume target""" - - volume_type = atypes.wsattr(str, mandatory=True) - """The volume_type of volume target""" - - properties = {str: types.jsontype} - """The properties for this volume target""" - - boot_index = atypes.wsattr(int, mandatory=True) - """The boot_index of volume target""" - - volume_id = atypes.wsattr(str, mandatory=True) - """The volume_id for this volume target""" - - extra = {str: types.jsontype} - """The metadata for this volume target""" - - node_uuid = atypes.wsproperty(types.uuid, _get_node_uuid, - _set_node_identifiers, mandatory=True) - """The UUID of the node this volume target belongs to""" - - links = None - """A list containing a self link and associated volume target links""" - - def __init__(self, **kwargs): - self.fields = [] - fields = list(objects.VolumeTarget.fields) - for field in fields: - # Skip fields we do not expose. - if not hasattr(self, field): - continue - self.fields.append(field) - setattr(self, field, kwargs.get(field, atypes.Unset)) - - # NOTE(smoriya): node_id is an attribute created on-the-fly - # by _set_node_uuid(), it needs to be present in the fields so - # that as_dict() will contain node_id field when converting it - # before saving it in the database. - self.fields.append('node_id') - # NOTE(smoriya): node_uuid is not part of objects.VolumeTarget.- - # fields because it's an API-only attribute - self.fields.append('node_uuid') - # NOTE(jtaryma): Additionally to node_uuid, node_id is handled as a - # secondary identifier in case RPC volume target object dictionary - # was passed to the constructor. - self.node_uuid = kwargs.get('node_uuid') or kwargs.get('node_id', - atypes.Unset) - - @staticmethod - def _convert_with_links(target, url): - - target.links = [link.make_link('self', url, - 'volume/targets', - target.uuid), - link.make_link('bookmark', url, - 'volume/targets', - target.uuid, - bookmark=True) - ] + if not sanitize: return target - @classmethod - def convert_with_links(cls, rpc_target, fields=None, sanitize=True): - target = VolumeTarget(**rpc_target.as_dict()) + api_utils.sanitize_dict(target, fields) - if fields is not None: - api_utils.check_for_invalid_fields(fields, target.as_dict()) - - target = cls._convert_with_links(target, api.request.public_url) - - if not sanitize: - return target - - target.sanitize(fields) - - return target - - def sanitize(self, fields=None): - """Removes sensitive and unrequested data. - - Will only keep the fields specified in the ``fields`` parameter. - - :param fields: - list of fields to preserve, or ``None`` to preserve them all - :type fields: list of str - """ - - if fields is not None: - self.unset_fields_except(fields) - - # never expose the node_id attribute - self.node_id = atypes.Unset - - @classmethod - def sample(cls, expand=True): - time = datetime.datetime(2000, 1, 1, 12, 0, 0) - properties = {"auth_method": "CHAP", - "auth_username": "XXX", - "auth_password": "XXX", - "target_iqn": "iqn.2010-10.com.example:vol-X", - "target_portal": "192.168.0.123:3260", - "volume_id": "a2f3ff15-b3ea-4656-ab90-acbaa1a07607", - "target_lun": 0, - "access_mode": "rw"} - - sample = cls(uuid='667808d4-622f-4629-b629-07753a19e633', - volume_type='iscsi', - boot_index=0, - volume_id='a2f3ff15-b3ea-4656-ab90-acbaa1a07607', - properties=properties, - extra={'foo': 'bar'}, - created_at=time, - updated_at=time) - sample._node_uuid = '7ae81bb3-dec3-4289-8d6c-da80bd8001ae' - fields = None if expand else _DEFAULT_RETURN_FIELDS - return cls._convert_with_links(sample, 'http://localhost:6385', - fields=fields) + return target -class VolumeTargetPatchType(types.JsonPatchType): - - _api_base = VolumeTarget - - -class VolumeTargetCollection(collection.Collection): - """API representation of a collection of volume targets.""" - - targets = [VolumeTarget] - """A list containing volume target objects""" - - def __init__(self, **kwargs): - self._type = 'targets' - - @staticmethod - def convert_with_links(rpc_targets, limit, url=None, fields=None, - detail=None, **kwargs): - collection = VolumeTargetCollection() - collection.targets = [ - VolumeTarget.convert_with_links(p, fields=fields, sanitize=False) - for p in rpc_targets] - if detail: - kwargs['detail'] = detail - collection.next = collection.get_next(limit, url=url, fields=fields, - **kwargs) - for target in collection.targets: - target.sanitize(fields) - return collection - - @classmethod - def sample(cls): - sample = cls() - sample.targets = [VolumeTarget.sample(expand=False)] - return sample +def list_convert_with_links(rpc_targets, limit, url=None, fields=None, + detail=None, **kwargs): + if detail: + kwargs['detail'] = detail + return collection.list_convert_with_links( + items=[convert_with_links(p, fields=fields, sanitize=False) + for p in rpc_targets], + item_name='targets', + limit=limit, + url=url, + fields=fields, + sanitize_func=api_utils.sanitize_dict, + **kwargs + ) class VolumeTargetsController(rest.RestController): @@ -269,17 +152,19 @@ class VolumeTargetsController(rest.RestController): limit, marker_obj, sort_key=sort_key, sort_dir=sort_dir) - return VolumeTargetCollection.convert_with_links(targets, limit, - url=resource_url, - fields=fields, - sort_key=sort_key, - sort_dir=sort_dir, - detail=detail) + return list_convert_with_links(targets, limit, + url=resource_url, + fields=fields, + sort_key=sort_key, + sort_dir=sort_dir, + detail=detail) @METRICS.timer('VolumeTargetsController.get_all') - @expose.expose(VolumeTargetCollection, types.uuid_or_name, types.uuid, - int, str, str, types.listtype, - types.boolean) + @method.expose() + @args.validate(node=args.uuid_or_name, marker=args.uuid, + limit=args.integer, sort_key=args.string, + sort_dir=args.string, fields=args.string_list, + detail=args.boolean) def get_all(self, node=None, marker=None, limit=None, sort_key='id', sort_dir='asc', fields=None, detail=None): """Retrieve a list of volume targets. @@ -322,7 +207,8 @@ class VolumeTargetsController(rest.RestController): detail=detail) @METRICS.timer('VolumeTargetsController.get_one') - @expose.expose(VolumeTarget, types.uuid, types.listtype) + @method.expose() + @args.validate(target_uuid=args.uuid, fields=args.string_list) def get_one(self, target_uuid, fields=None): """Retrieve information about the given volume target. @@ -344,11 +230,12 @@ class VolumeTargetsController(rest.RestController): rpc_target = objects.VolumeTarget.get_by_uuid( api.request.context, target_uuid) - return VolumeTarget.convert_with_links(rpc_target, fields=fields) + return convert_with_links(rpc_target, fields=fields) @METRICS.timer('VolumeTargetsController.post') - @expose.expose(VolumeTarget, body=VolumeTarget, - status_code=http_client.CREATED) + @method.expose(status_code=http_client.CREATED) + @method.body('target') + @args.validate(target=TARGET_VALIDATOR) def post(self, target): """Create a new volume target. @@ -370,29 +257,30 @@ class VolumeTargetsController(rest.RestController): if self.parent_node_ident: raise exception.OperationNotPermitted() - target_dict = target.as_dict() # NOTE(hshiina): UUID is mandatory for notification payload - if not target_dict.get('uuid'): - target_dict['uuid'] = uuidutils.generate_uuid() + if not target.get('uuid'): + target['uuid'] = uuidutils.generate_uuid() - new_target = objects.VolumeTarget(context, **target_dict) + node = api_utils.replace_node_uuid_with_id(target) + + new_target = objects.VolumeTarget(context, **target) notify.emit_start_notification(context, new_target, 'create', - node_uuid=target.node_uuid) + node_uuid=node.uuid) with notify.handle_error_notification(context, new_target, 'create', - node_uuid=target.node_uuid): + node_uuid=node.uuid): new_target.create() notify.emit_end_notification(context, new_target, 'create', - node_uuid=target.node_uuid) + node_uuid=node.uuid) # Set the HTTP Location Header api.response.location = link.build_url('volume/targets', new_target.uuid) - return VolumeTarget.convert_with_links(new_target) + return convert_with_links(new_target) @METRICS.timer('VolumeTargetsController.patch') - @expose.validate(types.uuid, [VolumeTargetPatchType]) - @expose.expose(VolumeTarget, types.uuid, - body=[VolumeTargetPatchType]) + @method.expose() + @method.body('patch') + @args.validate(target_uuid=args.uuid, patch=args.patch) def patch(self, target_uuid, patch): """Update an existing volume target. @@ -423,6 +311,8 @@ class VolumeTargetsController(rest.RestController): if self.parent_node_ident: raise exception.OperationNotPermitted() + api_utils.patch_validate_allowed_fields(patch, PATCH_ALLOWED_FIELDS) + values = api_utils.get_patch_values(patch, '/node_uuid') for value in values: if not uuidutils.is_uuid_like(value): @@ -436,23 +326,28 @@ class VolumeTargetsController(rest.RestController): # 1) Remove node_id because it's an internal value and # not present in the API object # 2) Add node_uuid - target_dict['node_uuid'] = target_dict.pop('node_id', None) - target = VolumeTarget( - **api_utils.apply_jsonpatch(target_dict, patch)) + rpc_node = api_utils.replace_node_id_with_uuid(target_dict) - # Update only the fields that have changed. - for field in objects.VolumeTarget.fields: - try: - patch_val = getattr(target, field) - except AttributeError: - # Ignore fields that aren't exposed in the API - continue - if patch_val == atypes.Unset: - patch_val = None - if rpc_target[field] != patch_val: - rpc_target[field] = patch_val + target_dict = api_utils.apply_jsonpatch(target_dict, patch) + + try: + if target_dict['node_uuid'] != rpc_node.uuid: + rpc_node = objects.Node.get( + api.request.context, target_dict['node_uuid']) + except exception.NodeNotFound as e: + # Change error code because 404 (NotFound) is inappropriate + # response for a PATCH request to change a volume target + e.code = http_client.BAD_REQUEST # BadRequest + raise + + api_utils.patched_validate_with_schema( + target_dict, TARGET_SCHEMA, TARGET_VALIDATOR) + + api_utils.patch_update_changed_fields( + target_dict, rpc_target, fields=objects.VolumeTarget.fields, + schema=TARGET_SCHEMA, id_map={'node_id': rpc_node.id} + ) - rpc_node = objects.Node.get_by_id(context, rpc_target.node_id) notify.emit_start_notification(context, rpc_target, 'update', node_uuid=rpc_node.uuid) with notify.handle_error_notification(context, rpc_target, 'update', @@ -461,13 +356,14 @@ class VolumeTargetsController(rest.RestController): new_target = api.request.rpcapi.update_volume_target( context, rpc_target, topic) - api_target = VolumeTarget.convert_with_links(new_target) + api_target = convert_with_links(new_target) notify.emit_end_notification(context, new_target, 'update', node_uuid=rpc_node.uuid) return api_target @METRICS.timer('VolumeTargetsController.delete') - @expose.expose(None, types.uuid, status_code=http_client.NO_CONTENT) + @method.expose(status_code=http_client.NO_CONTENT) + @args.validate(target_uuid=args.uuid) def delete(self, target_uuid): """Delete a volume target. diff --git a/ironic/tests/unit/api/controllers/v1/test_volume_target.py b/ironic/tests/unit/api/controllers/v1/test_volume_target.py index 613f551a48..140ae6861e 100644 --- a/ironic/tests/unit/api/controllers/v1/test_volume_target.py +++ b/ironic/tests/unit/api/controllers/v1/test_volume_target.py @@ -28,13 +28,10 @@ from ironic.api.controllers import base as api_base from ironic.api.controllers import v1 as api_v1 from ironic.api.controllers.v1 import notification_utils from ironic.api.controllers.v1 import utils as api_utils -from ironic.api.controllers.v1 import volume_target as api_volume_target -from ironic.api import types as atypes from ironic.common import exception from ironic.conductor import rpcapi from ironic import objects from ironic.objects import fields as obj_fields -from ironic.tests import base from ironic.tests.unit.api import base as test_api_base from ironic.tests.unit.api import utils as apiutils from ironic.tests.unit.db import utils as dbutils @@ -48,15 +45,6 @@ def post_get_test_volume_target(**kw): return target -class TestVolumeTargetObject(base.TestCase): - - def test_volume_target_init(self): - target_dict = apiutils.volume_target_post_data(node_id=None) - del target_dict['extra'] - target = api_volume_target.VolumeTarget(**target_dict) - self.assertEqual(atypes.Unset, target.extra) - - class TestListVolumeTargets(test_api_base.BaseApiTest): headers = {api_base.Version.string: str(api_v1.max_version())} @@ -872,7 +860,7 @@ class TestPost(test_api_base.BaseApiTest): self.assertEqual('application/json', response.content_type) self.assertEqual(http_client.BAD_REQUEST, response.status_int) self.assertTrue(response.json['error_message']) - self.assertIn(b'Expected a UUID but received 123.', response.body) + self.assertIn(b"123 is not of type 'string'", response.body) def test_node_uuid_to_node_id_mapping(self): pdict = post_get_test_volume_target(node_uuid=self.node['uuid']) diff --git a/ironic/tests/unit/api/utils.py b/ironic/tests/unit/api/utils.py index 4eefb58b90..4e22688c74 100644 --- a/ironic/tests/unit/api/utils.py +++ b/ironic/tests/unit/api/utils.py @@ -126,11 +126,8 @@ def volume_connector_post_data(**kw): def volume_target_post_data(**kw): target = db_utils.get_test_volume_target(**kw) - # These values are not part of the API object - target.pop('node_id') - target.pop('version') - internal = vt_controller.VolumeTargetPatchType.internal_attrs() - return remove_internal(target, internal) + return remove_other_fields(target, + vt_controller.TARGET_SCHEMA['properties']) def chassis_post_data(**kw):