From 0d4f2ee4e0fd6d389d7d7a9d5fefe65d3137e7bf Mon Sep 17 00:00:00 2001 From: Ramana Raja Date: Sat, 16 Jul 2016 20:35:46 +0530 Subject: [PATCH] add access_key to share_access_map For backends with internal authentication system, e.g. Ceph, that return ``access_key`` (credential) for client identities that are granted share access: * Retrieve ``access_key`` as return value of driver's update_access() * Store ``access_key`` in ShareAccessMapping model * Expose it in access_list API APIImpact DocImpact Partially implements bp auth-access-keys Co-Authored-By: John Spray Change-Id: I486064f117cf3001dba7735ca92a7d89aee3ce5b --- api-ref/source/parameters.yaml | 7 ++ .../share-actions-grant-access-response.json | 1 + ...re-actions-list-access-rules-response.json | 6 +- api-ref/source/share-actions.inc | 2 + manila/api/openstack/api_version_request.py | 4 +- .../openstack/rest_api_version_history.rst | 4 + manila/api/v1/shares.py | 10 +- manila/api/v2/shares.py | 2 + manila/api/views/share_accesses.py | 61 +++++++++++++ manila/db/api.py | 5 + .../versions/63809d875e32_add_access_key.py | 36 ++++++++ manila/db/sqlalchemy/api.py | 11 +++ manila/db/sqlalchemy/models.py | 1 + manila/share/access.py | 40 +++++++- manila/share/api.py | 16 +--- manila/share/driver.py | 8 ++ manila/tests/api/v1/test_shares.py | 26 ++++-- manila/tests/api/v2/test_shares.py | 31 ++++--- manila/tests/api/views/test_share_accesses.py | 80 ++++++++++++++++ .../alembic/migrations_data_checks.py | 64 +++++++++++++ manila/tests/db/sqlalchemy/test_api.py | 11 +++ manila/tests/share/test_access.py | 91 ++++++++++++++++++- manila/tests/share/test_api.py | 15 +-- manila/tests/share/test_manager.py | 4 + manila_tempest_tests/config.py | 2 +- manila_tempest_tests/tests/api/test_rules.py | 12 ++- ...-to-share-access-map-2fda4c06a750e24e.yaml | 5 + 27 files changed, 497 insertions(+), 58 deletions(-) create mode 100644 manila/api/views/share_accesses.py create mode 100644 manila/db/migrations/alembic/versions/63809d875e32_add_access_key.py create mode 100644 manila/tests/api/views/test_share_accesses.py create mode 100644 releasenotes/notes/add-access-key-to-share-access-map-2fda4c06a750e24e.yaml diff --git a/api-ref/source/parameters.yaml b/api-ref/source/parameters.yaml index f4d03b42a5..7f1a01f278 100644 --- a/api-ref/source/parameters.yaml +++ b/api-ref/source/parameters.yaml @@ -373,6 +373,13 @@ access_id: in: body required: true type: string +access_key: + description: | + The access credential of the entity granted share access. + in: body + required: true + type: string + min_version: 2.21 access_level: description: | The access level to the share. To grant or deny diff --git a/api-ref/source/samples/share-actions-grant-access-response.json b/api-ref/source/samples/share-actions-grant-access-response.json index 823ee6612b..30210124ba 100644 --- a/api-ref/source/samples/share-actions-grant-access-response.json +++ b/api-ref/source/samples/share-actions-grant-access-response.json @@ -6,6 +6,7 @@ "access_type": "ip", "access_to": "0.0.0.0/0", "access_level": "rw", + "access_key": null, "id": "a25b2df3-90bd-4add-afa6-5f0dbbd50452" } } diff --git a/api-ref/source/samples/share-actions-list-access-rules-response.json b/api-ref/source/samples/share-actions-list-access-rules-response.json index af015fc3c8..74c5c1aed7 100644 --- a/api-ref/source/samples/share-actions-list-access-rules-response.json +++ b/api-ref/source/samples/share-actions-list-access-rules-response.json @@ -5,14 +5,16 @@ "state": "error", "id": "507bf114-36f2-4f56-8cf4-857985ca87c1", "access_type": "cert", - "access_to": "example.com" + "access_to": "example.com", + "access_key": null }, { "access_level": "rw", "state": "active", "id": "a25b2df3-90bd-4add-afa6-5f0dbbd50452", "access_type": "ip", - "access_to": "0.0.0.0/0" + "access_to": "0.0.0.0/0", + "access_key": null } ] } diff --git a/api-ref/source/share-actions.inc b/api-ref/source/share-actions.inc index 60b2ea64fc..917687a3f6 100644 --- a/api-ref/source/share-actions.inc +++ b/api-ref/source/share-actions.inc @@ -97,6 +97,7 @@ Response parameters - updated_at: updated_at_3 - access_type: access_type_2 - access_to: access_to_1 + - access_key: access_key - access: access - access_level: access_level_2 - id: id_7 @@ -169,6 +170,7 @@ Response parameters .. rest_parameters:: parameters.yaml - access_type: access_type_1 + - access_key: access_key - access_to: access_to_1 - access_level: access_level - state: state diff --git a/manila/api/openstack/api_version_request.py b/manila/api/openstack/api_version_request.py index 0f3cdc0f09..e5ca1c4ec3 100644 --- a/manila/api/openstack/api_version_request.py +++ b/manila/api/openstack/api_version_request.py @@ -72,14 +72,14 @@ REST_API_VERSION_HISTORY = """ * 2.19 - Share snapshot instances admin APIs (list/show/detail/reset-status). * 2.20 - Add MTU to the JSON response of share network show API. - + * 2.21 - Add access_key to the response of access_list API. """ # The minimum and maximum versions of the API supported # The default api version request is defined to be the # the minimum version of the API supported. _MIN_API_VERSION = "2.0" -_MAX_API_VERSION = "2.20" +_MAX_API_VERSION = "2.21" DEFAULT_API_VERSION = _MIN_API_VERSION diff --git a/manila/api/openstack/rest_api_version_history.rst b/manila/api/openstack/rest_api_version_history.rst index a8533a877e..af424139db 100644 --- a/manila/api/openstack/rest_api_version_history.rst +++ b/manila/api/openstack/rest_api_version_history.rst @@ -126,3 +126,7 @@ user documentation. 2.20 ---- Add MTU in share network show API. + +2.21 +---- + Add access_key in access_list API. diff --git a/manila/api/v1/shares.py b/manila/api/v1/shares.py index 66b717e56d..6d95be1d69 100644 --- a/manila/api/v1/shares.py +++ b/manila/api/v1/shares.py @@ -28,6 +28,7 @@ from webob import exc from manila.api import common from manila.api.openstack import wsgi +from manila.api.views import share_accesses as share_access_views from manila.api.views import shares as share_views from manila import db from manila import exception @@ -497,7 +498,8 @@ class ShareMixin(object): access_data.get('access_level')) except exception.ShareAccessExists as e: raise webob.exc.HTTPBadRequest(explanation=e.msg) - return {'access': access} + + return self._access_view_builder.view(req, access) def _deny_access(self, req, id, body): """Remove share access rule.""" @@ -521,8 +523,9 @@ class ShareMixin(object): context = req.environ['manila.context'] share = self.share_api.get(context, id) - access_list = self.share_api.access_get_all(context, share) - return {'access_list': access_list} + access_rules = self.share_api.access_get_all(context, share) + + return self._access_view_builder.list_view(req, access_rules) def _extend(self, req, id, body): """Extend size of a share.""" @@ -576,6 +579,7 @@ class ShareController(wsgi.Controller, ShareMixin, wsgi.AdminActionsMixin): def __init__(self): super(self.__class__, self).__init__() self.share_api = share.API() + self._access_view_builder = share_access_views.ViewBuilder() @wsgi.action('os-reset_status') def share_reset_status(self, req, id, body): diff --git a/manila/api/v2/shares.py b/manila/api/v2/shares.py index 12cfd88b26..f77da92750 100644 --- a/manila/api/v2/shares.py +++ b/manila/api/v2/shares.py @@ -18,6 +18,7 @@ from manila.api.openstack import wsgi from manila.api.v1 import share_manage from manila.api.v1 import share_unmanage from manila.api.v1 import shares +from manila.api.views import share_accesses as share_access_views from manila.api.views import shares as share_views from manila import share @@ -34,6 +35,7 @@ class ShareController(shares.ShareMixin, def __init__(self): super(self.__class__, self).__init__() self.share_api = share.API() + self._access_view_builder = share_access_views.ViewBuilder() @wsgi.Controller.api_version("2.4") def create(self, req, body): diff --git a/manila/api/views/share_accesses.py b/manila/api/views/share_accesses.py new file mode 100644 index 0000000000..b51491e982 --- /dev/null +++ b/manila/api/views/share_accesses.py @@ -0,0 +1,61 @@ +# Copyright (c) 2016 Red Hat, Inc. +# 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. + +from manila.api import common + + +class ViewBuilder(common.ViewBuilder): + """Model a share access API response as a python dictionary.""" + + _collection_name = 'share_accesses' + _detail_version_modifiers = [ + "add_access_key", + ] + + def list_view(self, request, accesses): + """View of a list of share accesses.""" + return {'access_list': [self.summary_view(request, access)['access'] + for access in accesses]} + + def summary_view(self, request, access): + """Summarized view of a single share access.""" + access_dict = { + 'id': access.get('id'), + 'access_level': access.get('access_level'), + 'access_to': access.get('access_to'), + 'access_type': access.get('access_type'), + 'state': access.get('state'), + } + self.update_versioned_resource_dict( + request, access_dict, access) + return {'access': access_dict} + + def view(self, request, access): + """Generic view of a single share access.""" + access_dict = { + 'id': access.get('id'), + 'share_id': access.get('share_id'), + 'access_level': access.get('access_level'), + 'access_to': access.get('access_to'), + 'access_type': access.get('access_type'), + 'state': access.get('state'), + } + self.update_versioned_resource_dict( + request, access_dict, access) + return {'access': access_dict} + + @common.ViewBuilder.versioned_method("2.21") + def add_access_key(self, context, access_dict, access): + access_dict['access_key'] = access.get('access_key') diff --git a/manila/db/api.py b/manila/db/api.py index 7acd73d368..6bc6f9d7f1 100644 --- a/manila/db/api.py +++ b/manila/db/api.py @@ -415,6 +415,11 @@ def share_access_get_all_for_share(context, share_id): return IMPL.share_access_get_all_for_share(context, share_id) +def share_access_update_access_key(context, access_id, access_key): + """Update the access_key field of a share access mapping.""" + return IMPL.share_access_update_access_key(context, access_id, access_key) + + def share_access_get_all_for_instance(context, instance_id, session=None): """Get all access rules related to a certain share instance.""" return IMPL.share_access_get_all_for_instance( diff --git a/manila/db/migrations/alembic/versions/63809d875e32_add_access_key.py b/manila/db/migrations/alembic/versions/63809d875e32_add_access_key.py new file mode 100644 index 0000000000..d972c4cc3b --- /dev/null +++ b/manila/db/migrations/alembic/versions/63809d875e32_add_access_key.py @@ -0,0 +1,36 @@ +# 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_access_key + +Revision ID: 63809d875e32 +Revises: 493eaffd79e1 +Create Date: 2016-07-16 20:53:05.958896 + +""" + +# revision identifiers, used by Alembic. +revision = '63809d875e32' +down_revision = '493eaffd79e1' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + op.add_column( + 'share_access_map', + sa.Column('access_key', sa.String(255), nullable=True)) + + +def downgrade(): + op.drop_column('share_access_map', 'access_key') diff --git a/manila/db/sqlalchemy/api.py b/manila/db/sqlalchemy/api.py index 3352ba4d56..478a73c207 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -1813,6 +1813,17 @@ def share_access_delete_all_by_share(context, share_id): filter_by(share_id=share_id).soft_delete() +@require_context +def share_access_update_access_key(context, access_id, access_key): + session = get_session() + with session.begin(): + mapping = (session.query(models.ShareAccessMapping). + filter_by(id=access_id).first()) + mapping.update({'access_key': access_key}) + mapping.save(session=session) + return mapping + + @require_context def share_instance_access_delete(context, mapping_id): session = get_session() diff --git a/manila/db/sqlalchemy/models.py b/manila/db/sqlalchemy/models.py index 9d8c025ed7..c1c4279222 100644 --- a/manila/db/sqlalchemy/models.py +++ b/manila/db/sqlalchemy/models.py @@ -538,6 +538,7 @@ class ShareAccessMapping(BASE, ManilaBase): share_id = Column(String(36), ForeignKey('shares.id')) access_type = Column(String(255)) access_to = Column(String(255)) + access_key = Column(String(255), nullable=True) access_level = Column(Enum(*constants.ACCESS_LEVELS), default=constants.ACCESS_LEVEL_RW) diff --git a/manila/share/access.py b/manila/share/access.py index 225942c089..00257091ed 100644 --- a/manila/share/access.py +++ b/manila/share/access.py @@ -17,6 +17,8 @@ from oslo_log import log import six from manila.common import constants +from manila import exception +from manila.i18n import _ from manila.i18n import _LI from manila import utils @@ -100,8 +102,9 @@ class ShareInstanceAccess(object): delete_rules = [] try: + access_keys = None try: - self.driver.update_access( + access_keys = self.driver.update_access( context, share_instance, rules, @@ -116,6 +119,15 @@ class ShareInstanceAccess(object): self._update_access_fallback(add_rules, context, delete_rules, remove_rules, share_instance, share_server) + + if access_keys: + self._validate_access_keys(rules, add_rules, delete_rules, + access_keys) + + for access_id, access_key in access_keys.items(): + self.db.share_access_update_access_key( + context, access_id, access_key) + except Exception: self.db.share_instance_update_access_status( context, @@ -146,6 +158,32 @@ class ShareInstanceAccess(object): "share instance: %s"), share_instance['id']) + @staticmethod + def _validate_access_keys(access_rules, add_rules, delete_rules, + access_keys): + if not isinstance(access_keys, dict): + msg = _("The access keys must be supplied as a dictionary that " + "maps rule IDs to access keys.") + raise exception.Invalid(message=msg) + + actual_rule_ids = sorted(access_keys) + expected_rule_ids = [] + if not (add_rules or delete_rules): + expected_rule_ids = [rule['id'] for rule in access_rules] + else: + expected_rule_ids = [rule['id'] for rule in add_rules] + if actual_rule_ids != sorted(expected_rule_ids): + msg = (_("The rule IDs supplied: %(actual)s do not match the " + "rule IDs that are expected: %(expected)s.") + % {'actual': actual_rule_ids, + 'expected': expected_rule_ids}) + raise exception.Invalid(message=msg) + + for access_key in access_keys.values(): + if not isinstance(access_key, six.string_types): + msg = (_("Access key %s is not string type.") % access_key) + raise exception.Invalid(message=msg) + def _check_needs_refresh(self, context, rules, share_instance): rule_ids = set([rule['id'] for rule in rules]) queried_rules = self.db.share_access_get_all_for_instance( diff --git a/manila/share/api.py b/manila/share/api.py index 119c502698..ae068fd2e0 100644 --- a/manila/share/api.py +++ b/manila/share/api.py @@ -1213,14 +1213,7 @@ class API(base.Base): # NOTE(tpsilva): refreshing share_access model access = self.db.share_access_get(ctx, access['id']) - return { - 'id': access['id'], - 'share_id': access['share_id'], - 'access_type': access['access_type'], - 'access_to': access['access_to'], - 'access_level': access['access_level'], - 'state': access['state'], - } + return access def allow_access_to_instance(self, context, share_instance, access): policy.check_policy(context, 'share', 'allow_access') @@ -1302,12 +1295,7 @@ class API(base.Base): """Returns all access rules for share.""" policy.check_policy(context, 'share', 'access_get_all') rules = self.db.share_access_get_all_for_share(context, share['id']) - return [{'id': rule.id, - 'access_type': rule.access_type, - 'access_to': rule.access_to, - 'access_level': rule.access_level, - 'state': rule.state, - } for rule in rules] + return rules def access_get(self, context, access_id): """Returns access rule with the id.""" diff --git a/manila/share/driver.py b/manila/share/driver.py index 4d1237b56d..0cbb73d346 100644 --- a/manila/share/driver.py +++ b/manila/share/driver.py @@ -532,6 +532,14 @@ class ShareDriver(object): :param delete_rules: Empty List or List of access rules which should be removed. access_rules doesn't contain these rules. :param share_server: None or Share server model + :returns: None, or a dictionary of ``access_id``, ``access_key`` as + key: value pairs for the rules added, where, ``access_id`` + is the UUID (string) of the access rule, and ``access_key`` + is the credential (string) of the entity granted access. + During recovery after error, the returned dictionary must + contain ``access_id``, ``access_key`` for all the rules that + the driver is ordered to resync, i.e. rules in the + ``access_rules`` parameter. """ raise NotImplementedError() diff --git a/manila/tests/api/v1/test_shares.py b/manila/tests/api/v1/test_shares.py index 0cd172c20a..7e66d70437 100644 --- a/manila/tests/api/v1/test_shares.py +++ b/manila/tests/api/v1/test_shares.py @@ -827,6 +827,9 @@ class ShareActionsTest(test.TestCase): self.mock_object(share_api.API, 'allow_access', mock.Mock(return_value={'fake': 'fake'})) + self.mock_object(self.controller._access_view_builder, 'view', + mock.Mock(return_value={'access': + {'fake': 'fake'}})) id = 'fake_share_id' body = {'os-allow_access': access} @@ -887,20 +890,23 @@ class ShareActionsTest(test.TestCase): body) def test_access_list(self): - def _fake_access_get_all(*args, **kwargs): - return [{"state": "fakestatus", - "id": "fake_share_id", - "access_type": "fakeip", - "access_to": "127.0.0.1"}] - - self.mock_object(share_api.API, "access_get_all", - _fake_access_get_all) + fake_access_list = [ + { + "state": "fakestatus", + "id": "fake_access_id", + "access_type": "fakeip", + "access_to": "127.0.0.1", + } + ] + self.mock_object(self.controller._access_view_builder, 'list_view', + mock.Mock(return_value={'access_list': + fake_access_list})) id = 'fake_share_id' body = {"os-access_list": None} req = fakes.HTTPRequest.blank('/v1/tenant1/shares/%s/action' % id) + res_dict = self.controller._access_list(req, id, body) - expected = _fake_access_get_all() - self.assertEqual(expected, res_dict['access_list']) + self.assertEqual({'access_list': fake_access_list}, res_dict) def test_extend(self): id = 'fake_share_id' diff --git a/manila/tests/api/v2/test_shares.py b/manila/tests/api/v2/test_shares.py index 5d0780ceba..91194fc7ab 100644 --- a/manila/tests/api/v2/test_shares.py +++ b/manila/tests/api/v2/test_shares.py @@ -1328,6 +1328,9 @@ class ShareActionsTest(test.TestCase): self.mock_object(share_api.API, 'allow_access', mock.Mock(return_value={'fake': 'fake'})) + self.mock_object(self.controller._access_view_builder, 'view', + mock.Mock(return_value={'access': + {'fake': 'fake'}})) id = 'fake_share_id' body = {'allow_access': access} @@ -1370,6 +1373,9 @@ class ShareActionsTest(test.TestCase): self.mock_object(share_api.API, 'allow_access', mock.Mock(return_value={'fake': 'fake'})) + self.mock_object(self.controller._access_view_builder, 'view', + mock.Mock(return_value={'access': + {'fake': 'fake'}})) req = fakes.HTTPRequest.blank( '/v2/shares/%s/action' % share_id, version=version) @@ -1419,20 +1425,23 @@ class ShareActionsTest(test.TestCase): body) def test_access_list(self): - def _fake_access_get_all(*args, **kwargs): - return [{"state": "fakestatus", - "id": "fake_share_id", - "access_type": "fakeip", - "access_to": "127.0.0.1"}] - - self.mock_object(share_api.API, "access_get_all", - _fake_access_get_all) + fake_access_list = [ + { + "state": "fakestatus", + "id": "fake_access_id", + "access_type": "fakeip", + "access_to": "127.0.0.1", + } + ] + self.mock_object(self.controller._access_view_builder, 'list_view', + mock.Mock(return_value={'access_list': + fake_access_list})) id = 'fake_share_id' body = {"os-access_list": None} - req = fakes.HTTPRequest.blank('/v1/tenant1/shares/%s/action' % id) + req = fakes.HTTPRequest.blank('/v2/tenant1/shares/%s/action' % id) + res_dict = self.controller._access_list(req, id, body) - expected = _fake_access_get_all() - self.assertEqual(expected, res_dict['access_list']) + self.assertEqual({'access_list': fake_access_list}, res_dict) @ddt.unpack @ddt.data( diff --git a/manila/tests/api/views/test_share_accesses.py b/manila/tests/api/views/test_share_accesses.py new file mode 100644 index 0000000000..85a18c5eec --- /dev/null +++ b/manila/tests/api/views/test_share_accesses.py @@ -0,0 +1,80 @@ +# Copyright (c) 2016 Red Hat, Inc. +# 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. + +import ddt + +from manila.api.openstack import api_version_request as api_version +from manila.api.views import share_accesses +from manila import test +from manila.tests.api import fakes + + +@ddt.ddt +class ViewBuilderTestCase(test.TestCase): + + def setUp(self): + super(ViewBuilderTestCase, self).setUp() + self.builder = share_accesses.ViewBuilder() + self.fake_access = { + 'id': 'fakeaccessid', + 'share_id': 'fakeshareid', + 'access_level': 'fakeaccesslevel', + 'access_to': 'fakeacccessto', + 'access_type': 'fakeaccesstype', + 'state': 'fakeaccessstate', + 'access_key': 'fakeaccesskey', + } + + def test_collection_name(self): + self.assertEqual('share_accesses', self.builder._collection_name) + + @ddt.data("2.20", "2.21") + def test_view(self, version): + req = fakes.HTTPRequest.blank('/shares', version=version) + + result = self.builder.view(req, self.fake_access) + + if (api_version.APIVersionRequest(version) < + api_version.APIVersionRequest("2.21")): + del self.fake_access['access_key'] + + self.assertEqual({'access': self.fake_access}, result) + + @ddt.data("2.20", "2.21") + def test_summary_view(self, version): + req = fakes.HTTPRequest.blank('/shares', version=version) + + result = self.builder.summary_view(req, self.fake_access) + + if (api_version.APIVersionRequest(version) < + api_version.APIVersionRequest("2.21")): + del self.fake_access['access_key'] + del self.fake_access['share_id'] + + self.assertEqual({'access': self.fake_access}, result) + + @ddt.data("2.20", "2.21") + def test_list_view(self, version): + req = fakes.HTTPRequest.blank('/shares', version=version) + accesses = [self.fake_access, ] + + result = self.builder.list_view(req, accesses) + + if (api_version.APIVersionRequest(version) < + api_version.APIVersionRequest("2.21")): + del self.fake_access['access_key'] + del self.fake_access['share_id'] + + self.assertEqual({'access_list': accesses}, result) diff --git a/manila/tests/db/migrations/alembic/migrations_data_checks.py b/manila/tests/db/migrations/alembic/migrations_data_checks.py index 75a201b7f0..ebe17258c3 100644 --- a/manila/tests/db/migrations/alembic/migrations_data_checks.py +++ b/manila/tests/db/migrations/alembic/migrations_data_checks.py @@ -920,3 +920,67 @@ class NewMTUColumnChecks(BaseMigrationChecks): self.test_case.assertTrue(db_result.rowcount >= len(ids)) for record in db_result: self.test_case.assertFalse(hasattr(record, 'mtu')) + + +@map_to_migration('63809d875e32') +class AddAccessKeyToShareAccessMapping(BaseMigrationChecks): + table_name = 'share_access_map' + access_key_column_name = 'access_key' + + def setup_upgrade_data(self, engine): + share_data = { + 'id': uuidutils.generate_uuid(), + 'share_proto': "CEPHFS", + 'size': 1, + 'snapshot_id': None, + 'user_id': 'fake', + 'project_id': 'fake' + } + share_table = utils.load_table('shares', engine) + engine.execute(share_table.insert(share_data)) + + share_instance_data = { + 'id': uuidutils.generate_uuid(), + 'deleted': 'False', + 'host': 'fake', + 'share_id': share_data['id'], + 'status': 'available', + 'access_rules_status': 'active' + } + share_instance_table = utils.load_table('share_instances', engine) + engine.execute(share_instance_table.insert(share_instance_data)) + + share_access_data = { + 'id': uuidutils.generate_uuid(), + 'share_id': share_data['id'], + 'access_type': 'cephx', + 'access_to': 'alice', + 'deleted': 'False' + } + share_access_table = utils.load_table(self.table_name, engine) + engine.execute(share_access_table.insert(share_access_data)) + + share_instance_access_data = { + 'id': uuidutils.generate_uuid(), + 'share_instance_id': share_instance_data['id'], + 'access_id': share_access_data['id'], + 'deleted': 'False' + } + share_instance_access_table = utils.load_table( + 'share_instance_access_map', engine) + engine.execute(share_instance_access_table.insert( + share_instance_access_data)) + + def check_upgrade(self, engine, data): + share_access_table = utils.load_table(self.table_name, engine) + rows = engine.execute(share_access_table.select()) + for row in rows: + self.test_case.assertTrue(hasattr(row, + self.access_key_column_name)) + + def check_downgrade(self, engine): + share_access_table = utils.load_table(self.table_name, engine) + rows = engine.execute(share_access_table.select()) + for row in rows: + self.test_case.assertFalse(hasattr(row, + self.access_key_column_name)) diff --git a/manila/tests/db/sqlalchemy/test_api.py b/manila/tests/db/sqlalchemy/test_api.py index 2c8a7a9936..f2a65924cd 100644 --- a/manila/tests/db/sqlalchemy/test_api.py +++ b/manila/tests/db/sqlalchemy/test_api.py @@ -120,6 +120,17 @@ class ShareAccessDatabaseAPITestCase(test.TestCase): "fake_status" ) + @ddt.data(None, 'rhubarb') + def test_share_access_update_access_key(self, key_value): + share = db_utils.create_share() + access = db_utils.create_access(share_id=share['id']) + + db_api.share_access_update_access_key(self.ctxt, access['id'], + key_value) + + access = db_api.share_access_get(self.ctxt, access['id']) + self.assertEqual(key_value, access['access_key']) + @ddt.ddt class ShareDatabaseAPITestCase(test.TestCase): diff --git a/manila/tests/share/test_access.py b/manila/tests/share/test_access.py index 500524c723..54c56d8df7 100644 --- a/manila/tests/share/test_access.py +++ b/manila/tests/share/test_access.py @@ -37,6 +37,10 @@ class ShareInstanceAccessTestCase(test.TestCase): self.share_instance = db_utils.create_share_instance( share_id=self.share['id'], access_rules_status=constants.STATUS_ERROR) + self.rule = db_utils.create_access( + id='fakeaccessid', + share_id=self.share['id'], + access_to='fakeaccessto') @ddt.data(True, False) def test_update_access_rules_maintenance_mode(self, maintenance_mode): @@ -64,7 +68,8 @@ class ShareInstanceAccessTestCase(test.TestCase): mock.Mock(return_value=existing_rules)) self.mock_object(db, "share_instance_update_access_status", mock.Mock()) - self.mock_object(self.driver, "update_access", mock.Mock()) + self.mock_object(self.driver, "update_access", + mock.Mock(return_value=None)) self.mock_object(self.share_access_helper, "_remove_access_rules", mock.Mock()) self.mock_object(self.share_access_helper, "_check_needs_refresh", @@ -85,6 +90,87 @@ class ShareInstanceAccessTestCase(test.TestCase): db.share_instance_update_access_status.assert_called_with( self.context, share_instance['id'], constants.STATUS_ACTIVE) + @ddt.data(None, {'fakeaccessid': 'fakeaccesskey'}) + def test_update_access_rules_returns_access_keys(self, access_keys): + share_instance = db_utils.create_share_instance( + id='fakeshareinstanceid', + share_id=self.share['id'], + access_rules_status=constants.STATUS_ACTIVE) + rules = [self.rule] + + self.mock_object(db, "share_instance_get", mock.Mock( + return_value=share_instance)) + self.mock_object(db, "share_access_get_all_for_instance", + mock.Mock(return_value=rules)) + self.mock_object(db, "share_instance_update_access_status", + mock.Mock()) + self.mock_object(db, "share_access_update_access_key", + mock.Mock()) + self.mock_object(self.driver, "update_access", + mock.Mock(return_value=access_keys)) + self.mock_object(self.share_access_helper, + "_remove_access_rules", mock.Mock()) + self.mock_object(self.share_access_helper, "_check_needs_refresh", + mock.Mock(return_value=False)) + + self.share_access_helper.update_access_rules( + self.context, share_instance['id'], add_rules=rules) + + self.driver.update_access.assert_called_once_with( + self.context, share_instance, rules, add_rules=rules, + delete_rules=[], share_server=None) + self.share_access_helper._remove_access_rules.assert_called_once_with( + self.context, [], share_instance['id']) + self.share_access_helper._check_needs_refresh.assert_called_once_with( + self.context, rules, share_instance) + if access_keys: + db.share_access_update_access_key.assert_called_with( + self.context, 'fakeaccessid', 'fakeaccesskey') + else: + self.assertFalse(db.share_access_update_access_key.called) + db.share_instance_update_access_status.assert_called_with( + self.context, share_instance['id'], constants.STATUS_ACTIVE) + + @ddt.data({'maintenance_mode': True, + 'access_keys': ['invalidaccesskey']}, + {'maintenance_mode': True, + 'access_keys': {'invalidaccessid': 'accesskey'}}, + {'maintenance_mode': True, + 'access_keys': {'fakeaccessid': 9}}, + {'maintenance_mode': False, + 'access_keys': {'fakeaccessid': 9}}) + @ddt.unpack + def test_update_access_rules_invalid_access_keys(self, maintenance_mode, + access_keys): + access_rules_status = ( + constants.STATUS_ERROR if maintenance_mode + else constants.STATUS_ACTIVE) + share_instance = db_utils.create_share_instance( + id='fakeid', + share_id=self.share['id'], + access_rules_status=access_rules_status) + + rules = [self.rule] + add_rules = [] if maintenance_mode else rules + + self.mock_object(db, "share_instance_get", mock.Mock( + return_value=share_instance)) + self.mock_object(db, "share_access_get_all_for_instance", + mock.Mock(return_value=rules)) + self.mock_object(db, "share_instance_update_access_status", + mock.Mock()) + self.mock_object(self.driver, "update_access", + mock.Mock(return_value=access_keys)) + + self.assertRaises(exception.Invalid, + self.share_access_helper.update_access_rules, + self.context, share_instance['id'], + add_rules=add_rules) + + self.driver.update_access.assert_called_once_with( + self.context, share_instance, rules, add_rules=add_rules, + delete_rules=[], share_server=None) + def test_update_access_rules_fallback(self): add_rules = [db_utils.create_access(share_id=self.share['id'])] delete_rules = [db_utils.create_access(share_id=self.share['id'])] @@ -158,7 +244,8 @@ class ShareInstanceAccessTestCase(test.TestCase): return_value=share_instance)) self.mock_object(db, "share_access_get_all_for_instance", mock.Mock(return_value=original_rules)) - mock_update_access = self.mock_object(self.driver, "update_access") + mock_update_access = self.mock_object(self.driver, "update_access", + mock.Mock(return_value=None)) self.mock_object(self.share_access_helper, '_check_needs_refresh', mock.Mock(side_effect=[True, False])) diff --git a/manila/tests/share/test_api.py b/manila/tests/share/test_api.py index fa4084613f..43b9fb8853 100644 --- a/manila/tests/share/test_api.py +++ b/manila/tests/share/test_api.py @@ -101,6 +101,7 @@ def fake_access(id, **kwargs): 'access_type': 'fakeacctype', 'access_to': 'fakeaccto', 'access_level': 'rw', + 'access_key': None, 'state': 'fakeactive', 'STATE_NEW': 'fakenew', 'STATE_ACTIVE': 'fakeactive', @@ -1630,13 +1631,10 @@ class ShareAPITestCase(test.TestCase): 'access_to': 'fake_access_to', 'access_level': level, } - fake_access_expected = copy.deepcopy(values) - fake_access_expected.update({ + fake_access = copy.deepcopy(values) + fake_access.update({ 'id': 'fake_access_id', 'state': constants.STATUS_ACTIVE, - }) - fake_access = copy.deepcopy(fake_access_expected) - fake_access.update({ 'deleted': 'fake_deleted', 'deleted_at': 'fake_deleted_at', 'instance_mappings': ['foo', 'bar'], @@ -1650,7 +1648,7 @@ class ShareAPITestCase(test.TestCase): self.context, share, fake_access['access_type'], fake_access['access_to'], level) - self.assertEqual(fake_access_expected, access) + self.assertEqual(fake_access, access) self.share_rpcapi.allow_access.assert_called_once_with( self.context, utils.IsAMatcher(models.ShareInstance), fake_access) @@ -1865,11 +1863,8 @@ class ShareAPITestCase(test.TestCase): self.mock_object(db_api, 'share_access_get_all_for_share', mock.Mock(return_value=rules)) actual = self.api.access_get_all(self.context, share) - for access in actual: - expected_access = values[access['id']] - expected_access.pop('share_id') - self.assertEqual(expected_access, access) + self.assertEqual(rules, actual) share_api.policy.check_policy.assert_called_once_with( self.context, 'share', 'access_get_all') db_api.share_access_get_all_for_share.assert_called_once_with( diff --git a/manila/tests/share/test_manager.py b/manila/tests/share/test_manager.py index 3aa04763a0..03cd065598 100644 --- a/manila/tests/share/test_manager.py +++ b/manila/tests/share/test_manager.py @@ -2419,6 +2419,10 @@ class ShareManagerTestCase(test.TestCase): mock.Mock(side_effect=exception.ShareResourceNotFound( share_id=share['id']))) if side_effect == 'delete_share': + self.mock_object( + self.share_manager.access_helper.driver, 'update_access', + mock.Mock(return_value=None) + ) self.mock_object( self.share_manager.driver, 'delete_share', mock.Mock(side_effect=exception.ShareResourceNotFound( diff --git a/manila_tempest_tests/config.py b/manila_tempest_tests/config.py index 58cf3893f6..4203a714b4 100644 --- a/manila_tempest_tests/config.py +++ b/manila_tempest_tests/config.py @@ -34,7 +34,7 @@ ShareGroup = [ help="The minimum api microversion is configured to be the " "value of the minimum microversion supported by Manila."), cfg.StrOpt("max_api_microversion", - default="2.20", + default="2.21", help="The maximum api microversion is configured to be the " "value of the latest microversion supported by Manila."), cfg.StrOpt("region", diff --git a/manila_tempest_tests/tests/api/test_rules.py b/manila_tempest_tests/tests/api/test_rules.py index 09422c2e8c..8e70a5fbd0 100644 --- a/manila_tempest_tests/tests/api/test_rules.py +++ b/manila_tempest_tests/tests/api/test_rules.py @@ -424,7 +424,7 @@ class ShareRulesTest(base.BaseSharesTest): elif CONF.share.enable_cephx_rules_for_protocols: cls.protocol = CONF.share.enable_cephx_rules_for_protocols[0] cls.access_type = "cephx" - cls.access_to = "alice" + cls.access_to = "eve" cls.shares_v2_client.share_protocol = cls.protocol cls.share = cls.create_share() @@ -465,7 +465,10 @@ class ShareRulesTest(base.BaseSharesTest): version=version) # verify keys - for key in ("id", "access_type", "access_to", "access_level"): + keys = ("id", "access_type", "access_to", "access_level") + if utils.is_microversion_ge(version, '2.21'): + keys += ("access_key", ) + for key in keys: [self.assertIn(key, r.keys()) for r in rules] for key in ('deleted', 'deleted_at', 'instance_mappings'): [self.assertNotIn(key, r.keys()) for r in rules] @@ -474,6 +477,11 @@ class ShareRulesTest(base.BaseSharesTest): self.assertEqual(self.access_type, rules[0]["access_type"]) self.assertEqual(self.access_to, rules[0]["access_to"]) self.assertEqual('rw', rules[0]["access_level"]) + if utils.is_microversion_ge(version, '2.21'): + if self.access_type == 'cephx': + self.assertIsNotNone(rules[0]['access_key']) + else: + self.assertIsNone(rules[0]['access_key']) # our share id in list and have no duplicates gen = [r["id"] for r in rules if r["id"] in rule["id"]] diff --git a/releasenotes/notes/add-access-key-to-share-access-map-2fda4c06a750e24e.yaml b/releasenotes/notes/add-access-key-to-share-access-map-2fda4c06a750e24e.yaml new file mode 100644 index 0000000000..ae59b7eeb0 --- /dev/null +++ b/releasenotes/notes/add-access-key-to-share-access-map-2fda4c06a750e24e.yaml @@ -0,0 +1,5 @@ +--- +features: + - Driver may return ``access_key``, an access credential, for client + identities granted share access. + - Added ``access_key`` to the JSON response of ``access_list`` API.