From 3fbb560af1e4d53e701c423311e4d01cb5b1f62a Mon Sep 17 00:00:00 2001 From: Tzu-Mainn Chen Date: Thu, 21 Nov 2019 21:27:36 +0000 Subject: [PATCH] Add owner to allocations and create relevant policies Add an owner to allocations. Depending on policy, a non-admin can then create an allocation and have the owner set to their project. Allocation processing then respects the owner. Change-Id: I2965a4a601b9fa2c0212097da37b104a3e5514df Story: #2006506 Task: #37540 --- .../source/baremetal-api-v1-allocation.inc | 15 ++ api-ref/source/parameters.yaml | 2 +- .../samples/allocation-create-response.json | 1 + .../samples/allocation-show-response.json | 1 + .../samples/allocation-update-response.json | 1 + .../samples/allocations-list-response.json | 2 + .../contributor/webapi-version-history.rst | 9 + ironic/api/controllers/v1/allocation.py | 66 +++++- ironic/api/controllers/v1/utils.py | 8 + ironic/api/controllers/v1/versions.py | 4 +- ironic/common/policy.py | 5 + ironic/common/release_mappings.py | 4 +- ironic/conductor/allocations.py | 2 + .../ce6c4b3cf5a2_add_allocation_owner.py | 32 +++ ironic/db/sqlalchemy/api.py | 2 +- ironic/db/sqlalchemy/models.py | 1 + ironic/objects/allocation.py | 28 ++- .../api/controllers/v1/test_allocation.py | 190 +++++++++++++++++- ironic/tests/unit/api/utils.py | 2 +- .../tests/unit/conductor/test_allocations.py | 22 ++ .../unit/db/sqlalchemy/test_migrations.py | 5 + ironic/tests/unit/db/utils.py | 1 + ironic/tests/unit/objects/test_allocation.py | 61 ++++++ ironic/tests/unit/objects/test_objects.py | 4 +- ...ocation-owner-policy-162c43b3abb91c76.yaml | 7 + 25 files changed, 456 insertions(+), 19 deletions(-) create mode 100644 ironic/db/sqlalchemy/alembic/versions/ce6c4b3cf5a2_add_allocation_owner.py create mode 100644 releasenotes/notes/allocation-owner-policy-162c43b3abb91c76.yaml diff --git a/api-ref/source/baremetal-api-v1-allocation.inc b/api-ref/source/baremetal-api-v1-allocation.inc index 040450d9c7..808005dba2 100644 --- a/api-ref/source/baremetal-api-v1-allocation.inc +++ b/api-ref/source/baremetal-api-v1-allocation.inc @@ -47,6 +47,9 @@ parameters must be missing or match the provided node. .. versionadded:: 1.58 Added support for backfilling allocations. +.. versionadded:: 1.60 + Introduced the ``owner`` field. + Normal response codes: 201 Error response codes: 400, 401, 403, 409, 503 @@ -63,6 +66,7 @@ Request - uuid: req_uuid - extra: req_extra - node: req_allocation_node + - owner: owner Request Example --------------- @@ -83,6 +87,7 @@ Response Parameters - resource_class: allocation_resource_class - state: allocation_state - traits: allocation_traits + - owner: owner - extra: extra - created_at: created_at - updated_at: updated_at @@ -104,6 +109,9 @@ Lists all Allocations. .. versionadded:: 1.52 Allocation API was introduced. +.. versionadded:: 1.60 + Introduced the ``owner`` field. + Normal response codes: 200 Error response codes: 400, 401, 403, 404 @@ -116,6 +124,7 @@ Request - node: r_allocation_node - resource_class: r_resource_class - state: r_allocation_state + - owner: owner - fields: fields - limit: limit - marker: marker @@ -135,6 +144,7 @@ Response Parameters - resource_class: allocation_resource_class - state: allocation_state - traits: allocation_traits + - owner: owner - extra: extra - created_at: created_at - updated_at: updated_at @@ -156,6 +166,9 @@ Shows details for an Allocation. .. versionadded:: 1.52 Allocation API was introduced. +.. versionadded:: 1.60 + Introduced the ``owner`` field. + Normal response codes: 200 Error response codes: 400, 401, 403, 404 @@ -181,6 +194,7 @@ Response Parameters - resource_class: allocation_resource_class - state: allocation_state - traits: allocation_traits + - owner: owner - extra: extra - created_at: created_at - updated_at: updated_at @@ -237,6 +251,7 @@ Response Parameters - resource_class: allocation_resource_class - state: allocation_state - traits: allocation_traits + - owner: owner - extra: extra - created_at: created_at - updated_at: updated_at diff --git a/api-ref/source/parameters.yaml b/api-ref/source/parameters.yaml index 8707d111ac..c2d15d54d7 100644 --- a/api-ref/source/parameters.yaml +++ b/api-ref/source/parameters.yaml @@ -1048,7 +1048,7 @@ nodes: type: array owner: description: | - A string or UUID of the tenant who owns the baremetal node. + A string or UUID of the tenant who owns the object. in: body required: false type: string diff --git a/api-ref/source/samples/allocation-create-response.json b/api-ref/source/samples/allocation-create-response.json index 8016ffc148..67357d7f53 100644 --- a/api-ref/source/samples/allocation-create-response.json +++ b/api-ref/source/samples/allocation-create-response.json @@ -15,6 +15,7 @@ ], "name": "allocation-1", "node_uuid": null, + "owner": null, "resource_class": "bm-large", "state": "allocating", "traits": [], diff --git a/api-ref/source/samples/allocation-show-response.json b/api-ref/source/samples/allocation-show-response.json index 62d13cc830..faf572e93d 100644 --- a/api-ref/source/samples/allocation-show-response.json +++ b/api-ref/source/samples/allocation-show-response.json @@ -15,6 +15,7 @@ ], "name": "allocation-1", "node_uuid": "6d85703a-565d-469a-96ce-30b6de53079d", + "owner": null, "resource_class": "bm-large", "state": "active", "traits": [], diff --git a/api-ref/source/samples/allocation-update-response.json b/api-ref/source/samples/allocation-update-response.json index 682e46d195..598699ce0c 100644 --- a/api-ref/source/samples/allocation-update-response.json +++ b/api-ref/source/samples/allocation-update-response.json @@ -17,6 +17,7 @@ }, "last_error": null, "created_at": "2019-06-04T07:46:25+00:00", + "owner": null, "resource_class": "CUSTOM_GOLD", "updated_at": "2019-06-06T03:28:19.496960+00:00", "traits": [], diff --git a/api-ref/source/samples/allocations-list-response.json b/api-ref/source/samples/allocations-list-response.json index ff05159f0c..992cdc1a30 100644 --- a/api-ref/source/samples/allocations-list-response.json +++ b/api-ref/source/samples/allocations-list-response.json @@ -17,6 +17,7 @@ ], "name": "allocation-1", "node_uuid": "6d85703a-565d-469a-96ce-30b6de53079d", + "owner": null, "resource_class": "bm-large", "state": "active", "traits": [], @@ -40,6 +41,7 @@ ], "name": "allocation-2", "node_uuid": null, + "owner": null, "resource_class": "bm-large", "state": "error", "traits": [ diff --git a/doc/source/contributor/webapi-version-history.rst b/doc/source/contributor/webapi-version-history.rst index 6b18ba8d1d..d2dcb73865 100644 --- a/doc/source/contributor/webapi-version-history.rst +++ b/doc/source/contributor/webapi-version-history.rst @@ -2,7 +2,16 @@ REST API Version History ======================== +1.60 (Ussuri, master) +--------------------- + +Added ``owner`` field to the allocation object. The field should match the +``project_id`` of the intended owner. If the ``owner`` field is set, the +allocation process will only match the allocation with a node that has the +same ``owner`` field set. + 1.59 (Ussuri, master) +--------------------- Added the ability to specify a ``vendor_data`` dictionary field in the ``configdrive`` parameter submitted with the deployment of a node. The value diff --git a/ironic/api/controllers/v1/allocation.py b/ironic/api/controllers/v1/allocation.py index 6e7916d61e..8022a077a3 100644 --- a/ironic/api/controllers/v1/allocation.py +++ b/ironic/api/controllers/v1/allocation.py @@ -37,6 +37,12 @@ from ironic import objects METRICS = metrics_utils.get_metrics_logger(__name__) +def hide_fields_in_newer_versions(obj): + # if requested version is < 1.60, hide owner field + if not api_utils.allow_allocation_owner(): + obj.owner = wsme.Unset + + class Allocation(base.APIBase): """API representation of an allocation. @@ -72,6 +78,9 @@ class Allocation(base.APIBase): resource_class = wsme.wsattr(wtypes.StringType(max_length=80)) """Requested resource class for this allocation""" + owner = wsme.wsattr(wtypes.text) + """Owner of allocation""" + # NOTE(dtantsur): candidate_nodes is a list of UUIDs on the database level, # but the API level also accept names, converting them on fly. candidate_nodes = wsme.wsattr([wtypes.text]) @@ -149,6 +158,8 @@ class Allocation(base.APIBase): :type fields: list of str """ + hide_fields_in_newer_versions(self) + if fields is not None: self.unset_fields_except(fields) @@ -165,7 +176,8 @@ class Allocation(base.APIBase): candidate_nodes=[], extra={'foo': 'bar'}, created_at=datetime.datetime(2000, 1, 1, 12, 0, 0), - updated_at=datetime.datetime(2000, 1, 1, 12, 0, 0)) + updated_at=datetime.datetime(2000, 1, 1, 12, 0, 0), + owner=None) return cls._convert_with_links(sample, 'http://localhost:6385') @@ -223,8 +235,8 @@ class AllocationsController(pecan.rest.RestController): return super(AllocationsController, self)._route(args, request) def _get_allocations_collection(self, node_ident=None, resource_class=None, - state=None, marker=None, limit=None, - sort_key='id', sort_dir='asc', + state=None, owner=None, marker=None, + limit=None, sort_key='id', sort_dir='asc', resource_url=None, fields=None): """Return allocations collection. @@ -236,6 +248,7 @@ class AllocationsController(pecan.rest.RestController): :param resource_url: Optional, URL to the allocation resource. :param fields: Optional, a list with a specified set of fields of the resource to be returned. + :param owner: project_id of owner to filter by """ limit = api_utils.validate_limit(limit) sort_dir = api_utils.validate_sort_dir(sort_dir) @@ -262,7 +275,8 @@ class AllocationsController(pecan.rest.RestController): possible_filters = { 'node_uuid': node_uuid, 'resource_class': resource_class, - 'state': state + 'state': state, + 'owner': owner } filters = {} @@ -282,12 +296,27 @@ class AllocationsController(pecan.rest.RestController): sort_key=sort_key, sort_dir=sort_dir) + def _check_allowed_allocation_fields(self, fields): + """Check if fetching a particular field of an allocation is allowed. + + Check if the required version is being requested for fields + that are only allowed to be fetched in a particular API version. + + :param fields: list or set of fields to check + :raises: NotAcceptable if a field is not allowed + """ + if fields is None: + return + if 'owner' in fields and not api_utils.allow_allocation_owner(): + raise exception.NotAcceptable() + @METRICS.timer('AllocationsController.get_all') @expose.expose(AllocationCollection, types.uuid_or_name, wtypes.text, wtypes.text, types.uuid, int, wtypes.text, wtypes.text, - types.listtype) + types.listtype, wtypes.text) def get_all(self, node=None, resource_class=None, state=None, marker=None, - limit=None, sort_key='id', sort_dir='asc', fields=None): + limit=None, sort_key='id', sort_dir='asc', fields=None, + owner=None): """Retrieve a list of allocations. :param node: UUID or name of a node, to get only allocations for that @@ -303,12 +332,17 @@ class AllocationsController(pecan.rest.RestController): :param sort_dir: direction to sort. "asc" or "desc". Default: asc. :param fields: Optional, a list with a specified set of fields of the resource to be returned. + :param owner: Filter by owner. """ cdict = api.request.context.to_policy_values() policy.authorize('baremetal:allocation:get', cdict, cdict) + self._check_allowed_allocation_fields(fields) + if owner is not None and not api_utils.allow_allocation_owner(): + raise exception.NotAcceptable() + return self._get_allocations_collection(node, resource_class, state, - marker, limit, + owner, marker, limit, sort_key, sort_dir, fields=fields) @@ -324,6 +358,8 @@ class AllocationsController(pecan.rest.RestController): cdict = api.request.context.to_policy_values() policy.authorize('baremetal:allocation:get', cdict, cdict) + self._check_allowed_allocation_fields(fields) + rpc_allocation = api_utils.get_rpc_allocation_with_suffix( allocation_ident) return Allocation.convert_with_links(rpc_allocation, fields=fields) @@ -338,7 +374,18 @@ class AllocationsController(pecan.rest.RestController): """ context = api.request.context cdict = context.to_policy_values() - policy.authorize('baremetal:allocation:create', cdict, cdict) + + try: + policy.authorize('baremetal:allocation:create', cdict, cdict) + self._check_allowed_allocation_fields(allocation.as_dict()) + except exception.HTTPForbidden: + owner = cdict.get('project_id') + if not owner or (allocation.owner and owner != allocation.owner): + raise + policy.authorize('baremetal:allocation:create_restricted', + cdict, cdict) + self._check_allowed_allocation_fields(allocation.as_dict()) + allocation.owner = owner if (allocation.name and not api_utils.is_valid_logical_name(allocation.name)): @@ -416,12 +463,15 @@ class AllocationsController(pecan.rest.RestController): def _validate_patch(self, patch): allowed_fields = ['name', 'extra'] + fields = set() for p in patch: path = p['path'].split('/')[1] if path not in allowed_fields: msg = _("Cannot update %s in an allocation. Only 'name' and " "'extra' are allowed to be updated.") raise exception.Invalid(msg % p['path']) + fields.add(path) + self._check_allowed_allocation_fields(fields) @METRICS.timer('AllocationsController.patch') @wsme.validate(types.uuid, [AllocationPatchType]) diff --git a/ironic/api/controllers/v1/utils.py b/ironic/api/controllers/v1/utils.py index e7bb207e66..cb8156db8d 100644 --- a/ironic/api/controllers/v1/utils.py +++ b/ironic/api/controllers/v1/utils.py @@ -1248,3 +1248,11 @@ def allow_allocation_backfill(): Version 1.58 of the API added support for backfilling allocations. """ return api.request.version.minor >= versions.MINOR_58_ALLOCATION_BACKFILL + + +def allow_allocation_owner(): + """Check if allocation owner field is allowed. + + Version 1.60 of the API added the owner field to the allocation object. + """ + return api.request.version.minor >= versions.MINOR_60_ALLOCATION_OWNER diff --git a/ironic/api/controllers/v1/versions.py b/ironic/api/controllers/v1/versions.py index f4ab84618b..19a3ac4a14 100644 --- a/ironic/api/controllers/v1/versions.py +++ b/ironic/api/controllers/v1/versions.py @@ -97,6 +97,7 @@ BASE_VERSION = 1 # v1.57: Add support for updating an exisiting allocation. # v1.58: Add support for backfilling allocations. # v1.59: Add support vendor data in configdrives. +# v1.60: Add owner to the allocation object. MINOR_0_JUNO = 0 MINOR_1_INITIAL_VERSION = 1 @@ -158,6 +159,7 @@ MINOR_56_BUILD_CONFIGDRIVE = 56 MINOR_57_ALLOCATION_UPDATE = 57 MINOR_58_ALLOCATION_BACKFILL = 58 MINOR_59_CONFIGDRIVE_VENDOR_DATA = 59 +MINOR_60_ALLOCATION_OWNER = 60 # When adding another version, update: # - MINOR_MAX_VERSION @@ -165,7 +167,7 @@ MINOR_59_CONFIGDRIVE_VENDOR_DATA = 59 # explanation of what changed in the new version # - common/release_mappings.py, RELEASE_MAPPING['master']['api'] -MINOR_MAX_VERSION = MINOR_59_CONFIGDRIVE_VENDOR_DATA +MINOR_MAX_VERSION = MINOR_60_ALLOCATION_OWNER # String representations of the minor and maximum versions _MIN_VERSION_STRING = '{}.{}'.format(BASE_VERSION, MINOR_1_INITIAL_VERSION) diff --git a/ironic/common/policy.py b/ironic/common/policy.py index da02187fa4..2290cb9175 100644 --- a/ironic/common/policy.py +++ b/ironic/common/policy.py @@ -430,6 +430,11 @@ allocation_policies = [ 'rule:is_admin', 'Create Allocation records', [{'path': '/allocations', 'method': 'POST'}]), + policy.DocumentedRuleDefault( + 'baremetal:allocation:create_restricted', + 'rule:baremetal:allocation:create', + 'Create Allocation records that are restricted to an owner', + [{'path': '/allocations', 'method': 'POST'}]), policy.DocumentedRuleDefault( 'baremetal:allocation:delete', 'rule:is_admin', diff --git a/ironic/common/release_mappings.py b/ironic/common/release_mappings.py index 695e3d8978..1f619aa0e0 100644 --- a/ironic/common/release_mappings.py +++ b/ironic/common/release_mappings.py @@ -197,10 +197,10 @@ RELEASE_MAPPING = { } }, 'master': { - 'api': '1.59', + 'api': '1.60', 'rpc': '1.48', 'objects': { - 'Allocation': ['1.0'], + 'Allocation': ['1.1'], 'Node': ['1.32'], 'Conductor': ['1.3'], 'Chassis': ['1.3'], diff --git a/ironic/conductor/allocations.py b/ironic/conductor/allocations.py index 216a4cb6f1..d1492bf38f 100644 --- a/ironic/conductor/allocations.py +++ b/ironic/conductor/allocations.py @@ -112,6 +112,8 @@ def _candidate_nodes(context, allocation): # NOTE(dtantsur): we assume that candidate_nodes were converted to # UUIDs on the API level. filters['uuid_in'] = allocation.candidate_nodes + if allocation.owner: + filters['owner'] = allocation.owner nodes = objects.Node.list(context, filters=filters) diff --git a/ironic/db/sqlalchemy/alembic/versions/ce6c4b3cf5a2_add_allocation_owner.py b/ironic/db/sqlalchemy/alembic/versions/ce6c4b3cf5a2_add_allocation_owner.py new file mode 100644 index 0000000000..f138358695 --- /dev/null +++ b/ironic/db/sqlalchemy/alembic/versions/ce6c4b3cf5a2_add_allocation_owner.py @@ -0,0 +1,32 @@ +# All Rights Reserved. +# +# 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. +"""add allocation owner + +Revision ID: ce6c4b3cf5a2 +Revises: 1e15e7122cc9 +Create Date: 2019-11-21 20:46:09.106592 + +""" + +from alembic import op +import sqlalchemy as sa + +# revision identifiers, used by Alembic. +revision = 'ce6c4b3cf5a2' +down_revision = '1e15e7122cc9' + + +def upgrade(): + op.add_column('allocations', sa.Column('owner', sa.String(255), + nullable=True)) diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py index 8ea75ba04c..8951eff3bb 100644 --- a/ironic/db/sqlalchemy/api.py +++ b/ironic/db/sqlalchemy/api.py @@ -354,7 +354,7 @@ class Connection(api.Connection): if filters is None: filters = dict() supported_filters = {'state', 'resource_class', 'node_uuid', - 'conductor_affinity'} + 'conductor_affinity', 'owner'} unsupported_filters = set(filters).difference(supported_filters) if unsupported_filters: msg = _("SqlAlchemy API does not support " diff --git a/ironic/db/sqlalchemy/models.py b/ironic/db/sqlalchemy/models.py index f9dcf467e4..4554eba902 100644 --- a/ironic/db/sqlalchemy/models.py +++ b/ironic/db/sqlalchemy/models.py @@ -340,6 +340,7 @@ class Allocation(Base): name = Column(String(255), nullable=True) node_id = Column(Integer, ForeignKey('nodes.id'), nullable=True) state = Column(String(15), nullable=False) + owner = Column(String(255), nullable=True) last_error = Column(Text, nullable=True) resource_class = Column(String(80), nullable=True) traits = Column(db_types.JsonEncodedList) diff --git a/ironic/objects/allocation.py b/ironic/objects/allocation.py index a279b886dd..da64cd7a8d 100644 --- a/ironic/objects/allocation.py +++ b/ironic/objects/allocation.py @@ -12,6 +12,7 @@ from oslo_utils import strutils from oslo_utils import uuidutils +from oslo_utils import versionutils from oslo_versionedobjects import base as object_base from ironic.common import exception @@ -25,7 +26,8 @@ from ironic.objects import notification @base.IronicObjectRegistry.register class Allocation(base.IronicObject, object_base.VersionedObjectDictCompat): # Version 1.0: Initial version - VERSION = '1.0' + # Version 1.1: Add owner field + VERSION = '1.1' dbapi = dbapi.get_instance() @@ -41,6 +43,7 @@ class Allocation(base.IronicObject, object_base.VersionedObjectDictCompat): 'candidate_nodes': object_fields.ListOfStringsField(nullable=True), 'extra': object_fields.FlexibleDictField(nullable=True), 'conductor_affinity': object_fields.IntegerField(nullable=True), + 'owner': object_fields.StringField(nullable=True), } def _convert_to_version(self, target_version, @@ -51,12 +54,30 @@ class Allocation(base.IronicObject, object_base.VersionedObjectDictCompat): the same, older, or newer than the version of the object. This is used for DB interactions as well as for serialization/deserialization. + Version 1.1: owner was added. For versions prior to this, it should be + set to None or removed. + :param target_version: the desired version of the object :param remove_unavailable_fields: True to remove fields that are unavailable in the target version; set this to True when (de)serializing. False to set the unavailable fields to appropriate values; set this to False for DB interactions. """ + target_version = versionutils.convert_version_to_tuple(target_version) + + # Convert the owner field. + owner_is_set = self.obj_attr_is_set('owner') + if target_version >= (1, 1): + if not owner_is_set: + self.owner = None + elif owner_is_set: + # Target version does not support owner, and it is set. + if remove_unavailable_fields: + # (De)serialising: remove unavailable fields. + delattr(self, 'owner') + elif self.owner is not None: + # DB: set unavailable fields to their default. + self.owner = None # NOTE(xek): We don't want to enable RPC on this call just yet. Remotable # methods can be used in the future to replace current explicit RPC calls. @@ -266,7 +287,8 @@ class AllocationCRUDNotification(notification.NotificationBase): @base.IronicObjectRegistry.register class AllocationCRUDPayload(notification.NotificationPayloadBase): # Version 1.0: Initial version - VERSION = '1.0' + # Version 1.1: Add allocation owner field. + VERSION = '1.1' SCHEMA = { 'candidate_nodes': ('allocation', 'candidate_nodes'), @@ -274,6 +296,7 @@ class AllocationCRUDPayload(notification.NotificationPayloadBase): 'extra': ('allocation', 'extra'), 'last_error': ('allocation', 'last_error'), 'name': ('allocation', 'name'), + 'owner': ('allocation', 'owner'), 'resource_class': ('allocation', 'resource_class'), 'state': ('allocation', 'state'), 'traits': ('allocation', 'traits'), @@ -287,6 +310,7 @@ class AllocationCRUDPayload(notification.NotificationPayloadBase): 'node_uuid': object_fields.StringField(nullable=True), 'state': object_fields.StringField(nullable=True), 'last_error': object_fields.StringField(nullable=True), + 'owner': object_fields.StringField(nullable=True), 'resource_class': object_fields.StringField(nullable=True), 'traits': object_fields.ListOfStringsField(nullable=True), 'candidate_nodes': object_fields.ListOfStringsField(nullable=True), diff --git a/ironic/tests/unit/api/controllers/v1/test_allocation.py b/ironic/tests/unit/api/controllers/v1/test_allocation.py index 3399fb5a48..faa7cf528b 100644 --- a/ironic/tests/unit/api/controllers/v1/test_allocation.py +++ b/ironic/tests/unit/api/controllers/v1/test_allocation.py @@ -30,6 +30,7 @@ from ironic.api.controllers import v1 as api_v1 from ironic.api.controllers.v1 import allocation as api_allocation from ironic.api.controllers.v1 import notification_utils from ironic.common import exception +from ironic.common import policy from ironic.conductor import rpcapi from ironic import objects from ironic.objects import fields as obj_fields @@ -67,6 +68,7 @@ class TestListAllocations(test_api_base.BaseApiTest): self.assertEqual(allocation.name, data['allocations'][0]['name']) self.assertEqual({}, data['allocations'][0]["extra"]) self.assertEqual(self.node.uuid, data['allocations'][0]["node_uuid"]) + self.assertEqual(allocation.owner, data['allocations'][0]["owner"]) # never expose the node_id self.assertNotIn('node_id', data['allocations'][0]) @@ -78,6 +80,7 @@ class TestListAllocations(test_api_base.BaseApiTest): self.assertEqual(allocation.uuid, data['uuid']) self.assertEqual({}, data["extra"]) self.assertEqual(self.node.uuid, data["node_uuid"]) + self.assertEqual(allocation.owner, data["owner"]) # never expose the node_id self.assertNotIn('node_id', data) @@ -318,6 +321,29 @@ class TestListAllocations(test_api_base.BaseApiTest): headers=self.headers) self.assertEqual(3, len(data['allocations'])) + def test_get_all_by_owner(self): + for i in range(5): + if i < 3: + owner = '12345' + else: + owner = '54321' + obj_utils.create_test_allocation( + self.context, + owner=owner, + uuid=uuidutils.generate_uuid(), + name='allocation%s' % i) + data = self.get_json("/allocations?owner=12345", + headers=self.headers) + self.assertEqual(3, len(data['allocations'])) + + def test_get_all_by_owner_not_allowed(self): + response = self.get_json("/allocations?owner=12345", + headers={api_base.Version.string: '1.59'}, + expect_errors=True) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.NOT_ACCEPTABLE, response.status_code) + self.assertTrue(response.json['error_message']) + def test_get_all_by_node_name(self): for i in range(5): if i < 3: @@ -393,6 +419,44 @@ class TestListAllocations(test_api_base.BaseApiTest): expect_errors=True, headers=self.headers) self.assertEqual(http_client.NOT_FOUND, res.status_code) + def test_allocation_owner_hidden_in_lower_version(self): + allocation = obj_utils.create_test_allocation(self.context, + node_id=self.node.id) + data = self.get_json( + '/allocations/%s' % allocation.uuid, + headers={api_base.Version.string: '1.59'}) + self.assertNotIn('owner', data) + data = self.get_json( + '/allocations/%s' % allocation.uuid, + headers=self.headers) + self.assertIn('owner', data) + + def test_allocation_owner_null_field(self): + allocation = obj_utils.create_test_allocation(self.context, + node_id=self.node.id, + owner=None) + data = self.get_json('/allocations/%s' % allocation.uuid, + headers=self.headers) + self.assertIsNone(data['owner']) + + def test_allocation_owner_present(self): + allocation = obj_utils.create_test_allocation(self.context, + node_id=self.node.id, + owner='12345') + data = self.get_json('/allocations/%s' % allocation.uuid, + headers=self.headers) + self.assertEqual(data['owner'], '12345') + + def test_get_owner_field(self): + allocation = obj_utils.create_test_allocation(self.context, + node_id=self.node.id, + owner='12345') + fields = 'owner' + response = self.get_json( + '/allocations/%s?fields=%s' % (allocation.uuid, fields), + headers=self.headers) + self.assertIn('owner', response) + class TestPatch(test_api_base.BaseApiTest): headers = {api_base.Version.string: str(api_v1.max_version())} @@ -592,6 +656,18 @@ class TestPatch(test_api_base.BaseApiTest): self.assertEqual(http_client.BAD_REQUEST, response.status_code) self.assertTrue(response.json['error_message']) + def test_update_owner_not_acceptable(self): + allocation = obj_utils.create_test_allocation( + self.context, owner='12345', uuid=uuidutils.generate_uuid()) + new_owner = '54321' + response = self.patch_json('/allocations/%s' % allocation.uuid, + [{'path': '/owner', + 'value': new_owner, + 'op': 'replace'}], + expect_errors=True, headers=self.headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.BAD_REQUEST, response.status_code) + def _create_locally(_api, _ctx, allocation, topic): if 'node_id' in allocation and allocation.node_id: @@ -639,6 +715,7 @@ class TestPost(test_api_base.BaseApiTest): self.assertIsNone(result['node_uuid']) self.assertEqual([], result['candidate_nodes']) self.assertEqual([], result['traits']) + self.assertIsNone(None, result['owner']) self.assertNotIn('node', result) return_created_at = timeutils.parse_isotime( result['created_at']).replace(tzinfo=None) @@ -837,6 +914,23 @@ class TestPost(test_api_base.BaseApiTest): self.assertEqual('application/json', response.content_type) self.assertTrue(response.json['error_message']) + def test_create_allocation_owner(self): + owner = '12345' + adict = apiutils.allocation_post_data(owner=owner) + self.post_json('/allocations', adict, headers=self.headers) + result = self.get_json('/allocations/%s' % adict['uuid'], + headers=self.headers) + self.assertEqual(owner, result['owner']) + + def test_create_allocation_owner_not_allowed(self): + owner = '12345' + adict = apiutils.allocation_post_data(owner=owner) + response = self.post_json('/allocations', adict, + headers={api_base.Version.string: '1.59'}, + expect_errors=True) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.NOT_ACCEPTABLE, response.status_int) + def test_backfill(self): node = obj_utils.create_test_node(self.context) adict = apiutils.allocation_post_data(node=node.uuid) @@ -901,13 +995,107 @@ class TestPost(test_api_base.BaseApiTest): def test_backfill_not_allowed(self): node = obj_utils.create_test_node(self.context) headers = {api_base.Version.string: '1.57'} - adict = apiutils.allocation_post_data(node=node.uuid) + adict = {'node': node.uuid} response = self.post_json('/allocations', adict, expect_errors=True, headers=headers) self.assertEqual(http_client.BAD_REQUEST, response.status_int) self.assertEqual('application/json', response.content_type) self.assertTrue(response.json['error_message']) + @mock.patch.object(policy, 'authorize', autospec=True) + def test_create_restricted_allocation(self, mock_authorize): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:allocation:create': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + + owner = '12345' + adict = apiutils.allocation_post_data() + headers = {api_base.Version.string: '1.60', 'X-Project-Id': owner} + response = self.post_json('/allocations', adict, headers=headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.CREATED, response.status_int) + self.assertEqual(owner, response.json['owner']) + result = self.get_json('/allocations/%s' % adict['uuid'], + headers=headers) + self.assertEqual(adict['uuid'], result['uuid']) + self.assertEqual(owner, result['owner']) + + @mock.patch.object(policy, 'authorize', autospec=True) + def test_create_restricted_allocation_older_version(self, mock_authorize): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:allocation:create': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + + owner = '12345' + adict = apiutils.allocation_post_data() + del adict['owner'] + headers = {api_base.Version.string: '1.59', 'X-Project-Id': owner} + response = self.post_json('/allocations', adict, headers=headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.CREATED, response.status_int) + result = self.get_json('/allocations/%s' % adict['uuid'], + headers=headers) + self.assertEqual(adict['uuid'], result['uuid']) + + @mock.patch.object(policy, 'authorize', autospec=True) + def test_create_restricted_allocation_forbidden(self, mock_authorize): + def mock_authorize_function(rule, target, creds): + raise exception.HTTPForbidden(resource='fake') + mock_authorize.side_effect = mock_authorize_function + + owner = '12345' + adict = apiutils.allocation_post_data() + headers = {api_base.Version.string: '1.60', 'X-Project-Id': owner} + response = self.post_json('/allocations', adict, expect_errors=True, + headers=headers) + self.assertEqual(http_client.FORBIDDEN, response.status_int) + self.assertEqual('application/json', response.content_type) + self.assertTrue(response.json['error_message']) + + @mock.patch.object(policy, 'authorize', autospec=True) + def test_create_restricted_allocation_with_owner(self, mock_authorize): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:allocation:create': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + + owner = '12345' + adict = apiutils.allocation_post_data(owner=owner) + adict['owner'] = owner + headers = {api_base.Version.string: '1.60', 'X-Project-Id': owner} + response = self.post_json('/allocations', adict, headers=headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.CREATED, response.status_int) + self.assertEqual(owner, response.json['owner']) + result = self.get_json('/allocations/%s' % adict['uuid'], + headers=headers) + self.assertEqual(adict['uuid'], result['uuid']) + self.assertEqual(owner, result['owner']) + + @mock.patch.object(policy, 'authorize', autospec=True) + def test_create_restricted_allocation_with_mismatch_owner( + self, mock_authorize): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:allocation:create': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + + owner = '12345' + adict = apiutils.allocation_post_data(owner=owner) + adict['owner'] = '54321' + headers = {api_base.Version.string: '1.60', 'X-Project-Id': owner} + response = self.post_json('/allocations', adict, expect_errors=True, + headers=headers) + self.assertEqual(http_client.FORBIDDEN, response.status_int) + self.assertEqual('application/json', response.content_type) + self.assertTrue(response.json['error_message']) + @mock.patch.object(rpcapi.ConductorAPI, 'destroy_allocation') class TestDelete(test_api_base.BaseApiTest): diff --git a/ironic/tests/unit/api/utils.py b/ironic/tests/unit/api/utils.py index 30444d00ab..55191fdc4c 100644 --- a/ironic/tests/unit/api/utils.py +++ b/ironic/tests/unit/api/utils.py @@ -190,7 +190,7 @@ def post_get_test_portgroup(**kw): _ALLOCATION_POST_FIELDS = {'resource_class', 'uuid', 'traits', 'candidate_nodes', 'name', 'extra', - 'node'} + 'node', 'owner'} def allocation_post_data(node=None, **kw): diff --git a/ironic/tests/unit/conductor/test_allocations.py b/ironic/tests/unit/conductor/test_allocations.py index dbdb01e866..69108450ed 100644 --- a/ironic/tests/unit/conductor/test_allocations.py +++ b/ironic/tests/unit/conductor/test_allocations.py @@ -348,6 +348,28 @@ class DoAllocateTestCase(db_base.DbTestCase): # All nodes are filtered out on the database level. self.assertFalse(mock_acquire.called) + @mock.patch.object(task_manager, 'acquire', autospec=True, + side_effect=task_manager.acquire) + def test_nodes_filtered_out_owner(self, mock_acquire): + # Owner does not match + obj_utils.create_test_node(self.context, + uuid=uuidutils.generate_uuid(), + owner='54321', + resource_class='x-large', + power_state='power off', + provision_state='available') + + allocation = obj_utils.create_test_allocation(self.context, + resource_class='x-large', + owner='12345') + allocations.do_allocate(self.context, allocation) + self.assertIn('no available nodes', allocation['last_error']) + self.assertIn('x-large', allocation['last_error']) + self.assertEqual('error', allocation['state']) + + # All nodes are filtered out on the database level. + self.assertFalse(mock_acquire.called) + @mock.patch.object(task_manager, 'acquire', autospec=True, side_effect=task_manager.acquire) def test_nodes_locked(self, mock_acquire): diff --git a/ironic/tests/unit/db/sqlalchemy/test_migrations.py b/ironic/tests/unit/db/sqlalchemy/test_migrations.py index f54b4f7cf3..715ec15c3e 100644 --- a/ironic/tests/unit/db/sqlalchemy/test_migrations.py +++ b/ironic/tests/unit/db/sqlalchemy/test_migrations.py @@ -964,6 +964,11 @@ class MigrationCheckersMixin(object): self.assertIsInstance(deploy_templates.c.extra.type, sqlalchemy.types.TEXT) + def _check_ce6c4b3cf5a2(self, engine, data): + allocations = db_utils.get_table(engine, 'allocations') + col_names = [column.name for column in allocations.c] + self.assertIn('owner', col_names) + def test_upgrade_and_version(self): with patch_with_engine(self.engine): self.migration_api.upgrade('head') diff --git a/ironic/tests/unit/db/utils.py b/ironic/tests/unit/db/utils.py index 3ac498bd56..81a46d89a3 100644 --- a/ironic/tests/unit/db/utils.py +++ b/ironic/tests/unit/db/utils.py @@ -595,6 +595,7 @@ def get_test_allocation(**kw): 'updated_at': kw.get('updated_at'), 'uuid': kw.get('uuid', uuidutils.generate_uuid()), 'version': kw.get('version', allocation.Allocation.VERSION), + 'owner': kw.get('owner', None), } diff --git a/ironic/tests/unit/objects/test_allocation.py b/ironic/tests/unit/objects/test_allocation.py index baa7d8d717..33fffddb5f 100644 --- a/ironic/tests/unit/objects/test_allocation.py +++ b/ironic/tests/unit/objects/test_allocation.py @@ -142,3 +142,64 @@ class TestAllocationObject(db_base.DbTestCase, obj_utils.SchemasTestMixIn): def test_payload_schemas(self): self._check_payload_schemas(objects.allocation, objects.Allocation.fields) + + +class TestConvertToVersion(db_base.DbTestCase): + + def setUp(self): + super(TestConvertToVersion, self).setUp() + self.fake_allocation = db_utils.get_test_allocation() + + def test_owner_supported_missing(self): + # Physical network not set, should be set to default. + allocation = objects.Allocation(self.context, **self.fake_allocation) + delattr(allocation, 'owner') + allocation.obj_reset_changes() + allocation._convert_to_version("1.1") + self.assertIsNone(allocation.owner) + self.assertEqual({'owner': None}, allocation.obj_get_changes()) + + def test_owner_supported_set(self): + # Physical network set, no change required. + allocation = objects.Allocation(self.context, **self.fake_allocation) + allocation.owner = 'owner1' + allocation.obj_reset_changes() + allocation._convert_to_version("1.1") + self.assertEqual('owner1', allocation.owner) + self.assertEqual({}, allocation.obj_get_changes()) + + def test_owner_unsupported_missing(self): + # Physical network not set, no change required. + allocation = objects.Allocation(self.context, **self.fake_allocation) + delattr(allocation, 'owner') + allocation.obj_reset_changes() + allocation._convert_to_version("1.0") + self.assertNotIn('owner', allocation) + self.assertEqual({}, allocation.obj_get_changes()) + + def test_owner_unsupported_set_remove(self): + # Physical network set, should be removed. + allocation = objects.Allocation(self.context, **self.fake_allocation) + allocation.owner = 'owner1' + allocation.obj_reset_changes() + allocation._convert_to_version("1.0") + self.assertNotIn('owner', allocation) + self.assertEqual({}, allocation.obj_get_changes()) + + def test_owner_unsupported_set_no_remove_non_default(self): + # Physical network set, should be set to default. + allocation = objects.Allocation(self.context, **self.fake_allocation) + allocation.owner = 'owner1' + allocation.obj_reset_changes() + allocation._convert_to_version("1.0", False) + self.assertIsNone(allocation.owner) + self.assertEqual({'owner': None}, allocation.obj_get_changes()) + + def test_owner_unsupported_set_no_remove_default(self): + # Physical network set, no change required. + allocation = objects.Allocation(self.context, **self.fake_allocation) + allocation.owner = None + allocation.obj_reset_changes() + allocation._convert_to_version("1.0", False) + self.assertIsNone(allocation.owner) + self.assertEqual({}, allocation.obj_get_changes()) diff --git a/ironic/tests/unit/objects/test_objects.py b/ironic/tests/unit/objects/test_objects.py index 865b597d6c..993a1eb9cc 100644 --- a/ironic/tests/unit/objects/test_objects.py +++ b/ironic/tests/unit/objects/test_objects.py @@ -714,9 +714,9 @@ expected_object_fingerprints = { 'TraitList': '1.0-33a2e1bb91ad4082f9f63429b77c1244', 'BIOSSetting': '1.0-fd4a791dc2139a7cc21cefbbaedfd9e7', 'BIOSSettingList': '1.0-33a2e1bb91ad4082f9f63429b77c1244', - 'Allocation': '1.0-25ebf609743cd3f332a4f80fcb818102', + 'Allocation': '1.1-38937f2854722f1057ec667b12878708', 'AllocationCRUDNotification': '1.0-59acc533c11d306f149846f922739c15', - 'AllocationCRUDPayload': '1.0-a82389d019f37cfe54b50049f73911b3', + 'AllocationCRUDPayload': '1.1-3c8849932b80380bb96587ff62e8f087', 'DeployTemplate': '1.1-4e30c8e9098595e359bb907f095bf1a9', 'DeployTemplateCRUDNotification': '1.0-59acc533c11d306f149846f922739c15', 'DeployTemplateCRUDPayload': '1.0-200857e7e715f58a5b6d6b700ab73a3b', diff --git a/releasenotes/notes/allocation-owner-policy-162c43b3abb91c76.yaml b/releasenotes/notes/allocation-owner-policy-162c43b3abb91c76.yaml new file mode 100644 index 0000000000..0f4c4fc918 --- /dev/null +++ b/releasenotes/notes/allocation-owner-policy-162c43b3abb91c76.yaml @@ -0,0 +1,7 @@ +--- +features: + - | + Adds an ``owner`` field to allocations. Depending on policy, a non-admin + can then create an allocation and have the owner set to their project. + Allocation processing will then ensure that only nodes with the same owner + are matched.